Refactor wallet example to use Result and error codes.
#150
Conversation
| pub enum Error { | ||
| // Initialization errors | ||
| InvalidAdminWeight = 1, | ||
| AdminWeightsBelowThreshold = 2, | ||
| InvalidAdminCount = 3, | ||
| InvalidThreshold = 4, | ||
| AlreadyInitialized = 5, | ||
| // Payment errors | ||
| StoredPaymentMismatch = 6, | ||
| DuplicateSigner = 7, | ||
| UnauthorizedSigner = 8, | ||
| PaymentAlreadyExecuted = 9, | ||
| } |
There was a problem hiding this comment.
I don"t think we need to do this, but you think there could be an advantage in splitting these errors into two separate types? If the errors are unique to specific function sets, it would make it clearer which errors could be returned from which functions. I think in general this debated to be useful in the Rust ecosystem, so I"m not advocated strongly or anything, just wondering given the comments.
There was a problem hiding this comment.
I don"t have a strong opinion on this. I imagine splitting initialization-exclusive errors from everything else could be useful and wouldn"t hurt. For contracts with more than just 1 non-init fn I probably wouldn"t go further with splitting the error groups. We also should consider how hard would it be to consume and decode the contract error when/if we start returning it for the failed txs.
There was a problem hiding this comment.
We also should consider how hard would it be to consume and decode the contract error when/if we start returning it for the failed txs.
That"s a good point. Also, the "u32" naemspace of contract errors is shared across all error types in a single contract, so even if they were two separaet enums, it would be a bad idea to reuse integer values between them.
| fn extract_error<T, U: Debug>( | ||
| res: Result<Result<T, U>, Result<Error, Status>>, | ||
| ) -> Result<T, Error> { | ||
| match res { | ||
| Ok(v) => Ok(v.unwrap()), | ||
| Err(e) => Err(e.unwrap()), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Definitely interested in your thoughts about how to improve the try_ interface that this function unwraps. I wasn"t sure where the right balance was when I wrote the try_ interface, so I just went right to the extreme of every error condition is catchable.
There was a problem hiding this comment.
As I"m wrapping all the calls with extract_error, I can say that Result<T, Error> is the return type I"d want to see most of the time. However, I realize there could be scenarios where that"s not the case.
The conversion error on the function output doesn"t seem too useful. It"s hard for me to come up with a scenario where I would want to call another contract and then meaningfully react to the conversion error of the output value; I suspect 99.9% of the time this is just a bug in my contract and it would be ok if it trapped. I guess we need to make some decision on conversion errors in general.
The result for the error is pretty confusing; currently I"m not sure when would it be Err, so it"s hard for me to tell how valuable it is in general.
There was a problem hiding this comment.
I guess we need to make some decision on conversion errors in general.
Yes definitely, this is an area we need some more thought on.
I generally with trapping, however, I"m concerned with the possibility that a contract is exploitable by another contract returning the wrong in a certain situation, that causes a trap in a situation where a contract is therefore locked into a specific state. But maybe contracts have to code for this better and ensure they can"t be locked into a state based on another contract.
There was a problem hiding this comment.
The result for the error is pretty confusing; currently I"m not sure when would it be Err, so it"s hard for me to tell how valuable it is in general.
So there"s guarantee a contract returns an error defined their spec, and it"s also possible a contract returns a system error, such a trap, or an out of bounds operation on a Vec, etc. If those things happen the Err part of the Result will also be an Err containing a Status, and from a diagnostic point-of-view could be unpacked or viewed to see what that error is.
Maybe the problem is the overreliable on the nested Result, and maybe what we actually need is our own type that gives these things better names than simply Ok and Err.
Our own type could still behave like Err, even supporting htings like ? (I think).
There was a problem hiding this comment.
I"m concerned with the possibility that a contract is exploitable by another contract returning the wrong in a certain situation
I think that traps should normally cause everything to be reverted, shouldn"t they?
So there"s guarantee a contract returns an error defined their spec, and it"s also possible a contract returns a system error, such a trap, or an out of bounds operation on a Vec, etc. If those things happen the Err part of the Result will also be an Err containing a Status, and from a diagnostic point-of-view could be unpacked or viewed to see what that error is.
Yeah, that"s what I imagined, however I"m not sure how would that trigger now (maybe that would matter if we make try_call to treat panics as errors?). I agree though that some custom type would be cleaner. Basically we need to emulate Box<dyn Error> in regular Rust in some way.
There was a problem hiding this comment.
Ah I see the confusion, yes Rust tests do not currently behave consistently with a WASM deployment. These issues will probably address that and then the behavior you see in tests will make more sense:
|
The optimized build for this is 12,589 bytes. Prior to this change the optimized build is 12,030. 500 bytes is pretty cheap to pay for idiomatic Rust errors!!! |
|
Of that 500 bytes, 332 bytes is the additional contract spec for the contract error type, and the errors on each function. So that"s only 200 bytes of actual wasm instructions to support errors on every function, and return / bubble them up. There"s really no reason to favor panics over that. |
(cherry picked from commit 1562993)
What
Refactor wallet example to use
Resultand error codes.Compared to the panic approach this has 2-3 more lines of logic plus the errors themselves (and also a bit of boilerplate to extract the error in tests). As a benefit we get cleaner tests without
should_panicand more clear errors in cases that relied on the storage panic (that"s doable withpanicapproach too, but it"s less natural and more verbose, unlikeok_or).Why
Trying a different error reporting strategy.
Known limitations
N/A