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

Remove unwraps in bdk_cli and more detailed message for ChecksumMismatch #23

Merged
merged 1 commit into from
Mar 15, 2021

Conversation

RCasatta
Copy link
Member

@RCasatta RCasatta commented Mar 11, 2021

Description

This removes some unwraps in favor of ? during handling of the commands.

During development I frequently hit ChecksumMismatch while changing the descriptor and keeping the same wallet name, I imagined users may be confused so I detailed the message.

Notes to the reviewers

On top of #22

Error handling is a little nicer for end-user but it comes at the expense of losing stacktrace information in case of errors, so it may be controversial. However, we are now writing blog post and expanding user base so I think it is better.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've updated CHANGELOG.md

@RCasatta RCasatta marked this pull request as ready for review March 11, 2021 12:31
@thunderbiscuit
Copy link
Member

Great idea! The ChecksumMismatch error message has been bugging me for some time now, and I think a more user-friendly explanation as to what is going on is a really good thing.

Cargo.toml Outdated
@@ -27,7 +27,7 @@ clap = { version = "2.33", optional = true }
regex = {version = "1", optional = true }

[features]
default = []
default = ["repl", "electrum"]
Copy link
Member

Choose a reason for hiding this comment

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

The reason we didn't add any default features is because the wasm playground app doesn't need repl and can't use electrum .. but maybe this is OK if we just build bitcoindevkit.org/playground with default-features = false and features=["esplora"]. @afilini does that sound ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly. As long as there's a way to disable them it should be fine

@notmandatory
Copy link
Member

At some point I'd like to try adding anyhow to this project to add context to and help display errors, but I think we'd also need to add thiserror macro for bdk errors. For now I think the solution in the PR is a good step in the right direction.

@afilini
Copy link
Member

afilini commented Mar 15, 2021

This one probably needs rebasing after the force-push on #22

@RCasatta
Copy link
Member Author

rebased

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.

4 participants