mimblewimble/grin

Simplify error management

Opened this issue · 1 comments

Currently we have one or multiple error enum or struct per crate. Major part of such errors are wrappers on top of error variants from other crates. As result we have a huge graph of dependencies between errors which may be a good playground for a cuckoo solver. Here is an example:

$ rg "keychain::Error"
wallet/src/error.rs
44:     Keychain(keychain::Error),
180:impl From<keychain::Error> for Error {
181:    fn from(error: keychain::Error) -> Error {

pool/src/types.rs
189:    Keychain(keychain::Error),
223:impl From<keychain::Error> for PoolError {
224:    fn from(e: keychain::Error) -> PoolError {

chain/src/error.rs
76:     Keychain(keychain::Error),
218:impl From<keychain::Error> for Error {
219:    fn from(error: keychain::Error) -> Error {

core/src/core/committed.rs
28:     Keychain(keychain::Error),
43:impl From<keychain::Error> for Error {
44:     fn from(e: keychain::Error) -> Error {

core/src/core/transaction.rs
75:     Keychain(keychain::Error),
138:impl From<keychain::Error> for Error {
139:    fn from(e: keychain::Error) -> Error {

core/src/core/block.rs
61:     Keychain(keychain::Error),
99:impl From<keychain::Error> for Error {
100:    fn from(e: keychain::Error) -> Error {

wallet/src/libwallet/error.rs
61:     Keychain(keychain::Error),
254:impl From<keychain::Error> for Error {
255:    fn from(error: keychain::Error) -> Error {

core/src/libtx/error.rs
37:     Keychain(keychain::Error),
94:impl From<keychain::Error> for Error {
95:     fn from(error: keychain::Error) -> Error {

We have a lot of redundant code in each crate which converts error from other crates. At the same time the entire error management is pretty monolithic, single call can use errors from multiple crates.

Another issue that sometimes we cut backtrace on error conversion (From invocation).

I have a bit controversial proposal to extract errors into a separate crate, flatten (e.g to have single keychain error) and use it from all crates. We still have ability to add a context specific for a particular call. It's just to start discussion, I'm sure we can find a better solution than we have now.

I checked some multi-crate projects, found some common patterns, here are my suggestions:

  • We should not afraid to return error type from a different crate. The additional wrap makes code less transparent. For example we only get keychain::Error here https://github.com/mimblewimble/grin/blob/master/core/src/libtx/build.rs#L194 and if we return it the situation is pretty clear. If we wrap it into libtx::Error it's harder to say what happens here

  • In case if we get different types of error inside of a function we usually have to wrap it to return a single type. I'd argue that in this case we build an additional abstraction on top of existing ones wich justifies a new error kind. For example https://github.com/mimblewimble/grin/blob/master/core/src/libtx/aggsig.rs#L264 we get keychain and secp errors. We could convert it into function-specific error like Signature, but preserve stack trace adding .context(ErrorKind::Signature) to each call. In this case we clearly communicate what happened using language of the current abstraction layer (we could not sign), the underlying error could be checked later one to diagnose.

  • Having said that we should not wrap errors directly transforming between the types. In some cases the stack trace may be lost (I assume because we mix failure-based errors with regular errors). Using context we preserve stack trace.

  • I don't see much value into blanket variants like IOError (for example inside keychain error).

The main problem with that approach is what to show to a user. Currently we preserve the message when convert error between different types and then present it to a user. In this case even UnsufficientFunds error happens on very low level a user can see it and not a top level error like WalletError (I made it up).