-
Notifications
You must be signed in to change notification settings - Fork 135
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
Replace failure
with thiserror
#654
Replace failure
with thiserror
#654
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 👍
I went through the changes and didn't notice any logical mistakes. I also made some minor calls to CLI/API that returned an error and got the same response
let version = SlateVersion::V4; | ||
Ok(VersionedSlate::into_version(slate, version).map_err(|e| e.kind())?) | ||
VersionedSlate::into_version(slate, version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for removing redundant unwrap->wrap. I think there are a few more above in functions build_coinbase, receive_tx, finalize_tx
in api/src/foreign_rpc.rs
All good, many thanks for reviewing! |
Wallet counterpart for mimblewimble/grin#3728, note that the node PR will need to be merged before this will compile.
All should be as before, except
thiserror
doesn't support explicit backtraces so these have been removed from test output where they appeared.Review should again just check to ensure I haven't accidentally changed any logic.