Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error wrapping order #835

Merged
merged 22 commits into from
Mar 15, 2024
Merged

Fix error wrapping order #835

merged 22 commits into from
Mar 15, 2024

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Mar 14, 2024

Fix error wrapping order.

Fixes #798.

See iotaledger/iota.go#709 for the rationale of the changes.

Quick Guide

  1. If you call another function that can return an error, you typically want to annotate that error with some context so the reader of the error can roughly understand the call chain. This is where we use ierrors.Wrap or ierrors.Wrapf. This prepends a message. For instance:
if err := vm.ChainSTVF(vmParams, iotago.ChainTransitionTypeDestroy, inputChain, nil); err != nil {
	return ierrors.Wrapf(err, "invalid destruction for %s %s", inputChain.Output.Type(), chainID)
}

which gives us an error like

failed to execute transaction: invalid destruction for AccountOutput 0x6a22cbe0e578b4d88440fd1ccced1c8903848a00f6cbcecd12bc3c4a2d144524: block issuer feature has not expired: feature expires in slot 4294967295 but the slot of the commitment input is 110
  1. If you detect an error condition and return a named error (e.g. iotago.ErrBlockIssuerNotExpired), you typically want to append a message with more details using ierrors.WithMessage or ierrors.WithMessagef. For instance:
if blockIssuerFeat.ExpirySlot >= vmParams.WorkingSet.Commitment.Slot {
    return ierrors.WithMessagef(iotago.ErrBlockIssuerNotExpired, "feature expires in slot %d but the slot of the commitment input is %d", blockIssuerFeat.ExpirySlot, vmParams.WorkingSet.Commitment.Slot)
}

Generally, in my opinion, the goal of the error message is that the further you read into the message, the more details you will see. In the above example you see an increasing level of detail (transaction level -> output level -> feature level). This makes it easy to understand that the error was a failed transaction, that an account output in the transaction was the culprit and that within that account, the block issuer feature was not expired.

To summarize:

  • If you propagate an error upwards, you should add context to the error, like what that component tried and failed to do. Use ierrors.Wrap for that.
  • If you return a named error you should add details to the error, like the commitment and expiry slot above. Use ierrors.WithMessage for that.

Join

As mentioned in the iota.go PR ierrors.Join is not what we typically want but we should make a decision if we use the style in iota.go, change how ierrors.Join works or add a new API like maybe ierrors.WithError. That's why I did not restyle the errors in iota-core because it might be unnecessary work and leaving the current ierrors.Join in place makes it easier to search and replace later than the equivalent using WithMessagef and %w.

Copy link
Contributor

@cyberphysic4l cyberphysic4l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good. Just a few small suggestions.

pkg/network/p2p/manager.go Outdated Show resolved Hide resolved
pkg/network/p2p/manager.go Outdated Show resolved Hide resolved
pkg/protocol/engine/mempool/v1/mempool.go Outdated Show resolved Hide resolved
pkg/requesthandler/blocks.go Outdated Show resolved Hide resolved
@PhilippGackstatter PhilippGackstatter merged commit b0aed00 into develop Mar 15, 2024
4 checks passed
@PhilippGackstatter PhilippGackstatter deleted the improve-errors branch March 15, 2024 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace errors with correct error level and Wrap/WithMessage
2 participants