mimblewimble / grin

Minimal implementation of the Mimblewimble protocol.
https://grin.mw/
Apache License 2.0
5.04k stars 989 forks source link

Simplify error management #2542

Open hashmap opened 5 years ago

hashmap commented 5 years ago

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.

hashmap commented 5 years ago

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

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).