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 message order and reconsider panics #709

Merged
merged 52 commits into from
Mar 13, 2024

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Mar 8, 2024

Description of change

Fix error message order (ierrors.Wrap vs ierrors.WithMessage) and reconsider panicking in appropriate situations.

Implements part of iotaledger/iota-core#798.

Notes on Error Handling

We have basically three methods of error handling available in Go:

  • Named Errors
  • Anonymous Errors
  • Panics

Named Errors

A named error is one that is initialized as a static variable, e.g.:

ErrTxEssenceNetworkIDInvalid = ierrors.New("invalid network ID")

It is part of the public interface and can therefore be matched against, using, for example, ierrors.Is.
This one should therefore be used whenever we want either users to be able to match against the error or we want to test the specific case in which the error occurs.

Anonymous Errors

Because the idiomatic error handling in Go returns the error interface, the returned value can be opaque. All we know is that we can call Error() on the value and get a string representation for human consumption. An anonymous error is constructed ad-hoc, e.g. with ierrors.New or ierrors.Errorf:

func VersionFromBytes(b []byte) (Version, int, error) {
	if len(b) < VersionLength {
		return 0, 0, ierrors.New("invalid version bytes length")
	}

	return Version(b[0]), VersionLength, nil
}

Anonymous errors should be used for cases where we do not want to panic but we do not need the caller to be able to match against the returned error. This is the case for broken invariants that shouldn't happen, such as passing an incorrect length to VersionFromBytes. The error is intended to be read by a developer to figure out why their usage of the API was incorrect during development. It should not happen in a production deployment and therefore does not need to be matched against programmatically.

Deciding between named and anonymous errors is never easy. Anonymous errors make it harder to test against. The only way to make sure that an incorrect length passed to VersionFromBytes does return an error is to assert that an error is returned or matching against the string, both of which are bad practice. On the other hand, making all errors named bloats the public interface.

Panics

Panics are a lot like anonymous errors in that they do not have a public representation in the API and that they represent broken invariants. The major difference between returning an error and panicking is that the caller needs to deal with the returned error and that a panic takes away control from the caller.
Because a panics takes away control it must be used judiciously. A panic says: "this problem is serious enough that 'everything' should come to a halt."
Panics are okay to use when an essential invariant of a type or the system as a whole is broken. A concrete example is when we switch over all Address types (the Go types to be specific, not iotago.AddressType) and land in the default case. If we indeed handled all supported addresses then panicking in the default case is ok, because we can only run into that case if we added a new address type and have not handled it above or a user passed a type implementing the address interface (which is only possible when iota.go is used as a library). In both cases we want to panic to alert us to the unhandled type or the user to the unsupported type.

We must not panic in cases when users can construct input that would cause a panic, at least not when the input is passed to iota-core and then to iota.go, rather than to iota.go directly (e.g. as a library).
Because it is generally hard to verify that users can never do that for a given case, erring (no pun intended) on the side of caution and returning anonymous errors tends to be safer.

Error Wrapping

We have two ways of wrapping errors, Wrap and WithMessage (and their corresponding formatting versions).

The usage is very simple.
Wrap for prepending a message:

return ierrors.Wrapf(iotago.ErrSenderFeatureNotUnlocked, "output %d", outputIndex)

Which results in the error message:

output 1: sender feature is not unlocked

WithMessage for appending a message:

return ierrors.WithMessagef(iotago.ErrStakingEndEpochTooEarly, "end epoch %d should be >= %d", stakingFeat.EndEpoch, unbondingEpoch)

Which results in the error message:

staking end epoch must be set to the transaction epoch plus the unbonding period: end epoch 29 should be >= 30

Combining existing errors

Sometimes two existing errors need to be combined. Using Join is not desirable here, as it is, in my understanding, intended to group together a collection of errors. However, in our use cases we almost always want to wrap one error in another to create a hierarchical chain of errors. One example of this is:

totalIn, err = safemath.SafeAdd(totalIn, manaStored)
if err != nil {
    return 0, ierrors.WithMessagef(iotago.ErrManaOverflow, "%w", err)
}

We want to return ErrManaOverflow as a higher-level error for all kinds of overflows that can happen with Mana. This error is later mapped to its own transaction failure reason, TxFailureManaOverflow. We also want to include the underlying safemath error in the error chain so we get the detailed error of which calculation went wrong:

under- or overflow in Mana calculations: integer under- or overflow: 18446744073709551615 + 10

If we were to use Join we would not create a chain of errors but only group the errors together, which is reflected in the error message as a newline, separating the two errors:

under- or overflow in Mana calculations
integer under- or overflow: 18446744073709551615 + 10

This makes it look like they are unrelated and it is only clear from context here that they are in fact related.

We might want to add a dedicated function in ierrors for this, perhaps ierrors.WithError(iotago.ErrManaOverflow, err), analogous to WithMessage.

@PhilippGackstatter PhilippGackstatter force-pushed the error-handling-improvements branch from 86ce4db to 5f56f3d Compare March 11, 2024 10:01
}
totalIn, err = safemath.SafeAdd(totalIn, manaStored)
if err != nil {
return 0, ierrors.Wrapf(iotago.ErrManaOverflow, "%w", err)
return 0, ierrors.WithMessagef(iotago.ErrManaOverflow, "%w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not ierrors.Join() here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also change that for all the following occurrences in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see the PR description for why I chose to use this style over Join.

vm/vm.go Outdated Show resolved Hide resolved
vm/vm.go Outdated Show resolved Hide resolved
vm/nova/vm.go Outdated Show resolved Hide resolved
address.go Outdated Show resolved Hide resolved
transaction.go Show resolved Hide resolved
transaction.go Show resolved Hide resolved
transaction.go Show resolved Hide resolved
unlock.go Show resolved Hide resolved
unlock.go Show resolved Hide resolved
@PhilippGackstatter PhilippGackstatter marked this pull request as ready for review March 12, 2024 05:39
@muXxer muXxer merged commit 6304dd5 into develop Mar 13, 2024
4 checks passed
@muXxer muXxer deleted the error-handling-improvements branch March 13, 2024 10:57
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.

2 participants