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

Add CreateTxError and use as error type for TxBuilder::finish() #1028

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Jul 6, 2023

Description

To remove some places where there were .expect("TODO") I added a new CreateTxError type which is returned from TxBuilder::finish(). I also updated related tests and doc tests.

Fixes #996 (comment)

Also added fallible Wallet::try_get_address() and try_get_internal_address() to return Result with a possible D:WriteError when a PersistBackend is used. This should fix #996.

I removed catch-all bdk::Error and replaced usages with new types and updated related functions, fixes #994.

Notes to the reviewers

I didn't add all possible bdk::Error types that Wallet::create_tx() and TxBuilder::finish() functions might throw. It's probably not too much more work but will take a bit more research so I want to make sure this is the right general approach first.

I added anyhow to the dev-dependencies so I could remove some .expect() lines from the docs tests and make the examples closer to what an end user should do. I also used the anyhow!() macro to replace a few places that were using the bdk::Error::Generic in example code.

I also moved the module level error.rs file to wallet/error.rs so no one would be tempted to make any new catch all errors and to make it clear that all the errors in it are wallet module related.

Changelog notice

Changed

  • Updated bdk module to use new context specific error types
    • wallet: MiniscriptPsbtError, CreateTxError, BuildFeeBumpError error enums
    • coin_selection: module Error enum
  • Renamed fallible Wallet address functions to try_get_address() and try_get_internal_address()

Removed

  • Removed catch-all top level bdk::Error enum
  • Removed impl_error macro

Added

  • Added infallible Wallet get_address(), get_internal_address functions

Checklists

All Submissions:

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

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory self-assigned this Jul 6, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.1 milestone Jul 6, 2023
@notmandatory notmandatory marked this pull request as draft July 6, 2023 03:10
@notmandatory notmandatory changed the title Add CreateTxError as error type for TxBuilder::finish() Add CreateTxError and use as error type for TxBuilder::finish() Jul 6, 2023
@notmandatory notmandatory force-pushed the tmp_fix_write_error branch 5 times, most recently from 17b3987 to d99e4a5 Compare July 6, 2023 03:53
@notmandatory notmandatory marked this pull request as ready for review July 6, 2023 04:00
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Thank you for taking this on!

I appreciate your attempt to be conservative in the amount of changes made. However, the current state of this PR does not result in a better API since the caller still has to handle a large list of redundant error variants.

If it was a PR to purely remove the .expect("TODO")s, I would just return a bdk::Error::Generic or introduce a new variant in bdk::Error.

However, I would prefer this PR to fix the error-handling API for tx creation completely (as it seems you already have done the majority of the work).

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved
@evanlinjin
Copy link
Member

But we probably just need a quick fix for the next release.

@notmandatory notmandatory marked this pull request as draft July 8, 2023 01:11
@notmandatory notmandatory force-pushed the tmp_fix_write_error branch 2 times, most recently from be04de6 to cc4ba36 Compare July 8, 2023 02:20
@notmandatory
Copy link
Member Author

I added fallible Wallet::try_get_address() and try_get_internal_address() to return Result with a possible D:WriteError when a PersistBackend is used. This should fix #996.

@notmandatory
Copy link
Member Author

notmandatory commented Jul 8, 2023

I removed the CreateTxError::Bdk variant and moved all the needed variants from the bdk::Error to the CreateTxError enum, plus created concrete variants for any errors that were using Generic. A few other things I'd like to do, but don't need to be in this PR:

  • add BuildFeeBumpError enum for Wallet::build_fee_bump
  • add SignError enum for Wallet::sign
  • add FinalizeError enum for Wallet::finalize
  • eliminate (or greatly diminish) bdk::Error
  • move the wallet error enums and related code to a wallet::error file
  • add fallible and infallible versions of any others Wallet functions it makes sense for

@notmandatory notmandatory marked this pull request as ready for review July 8, 2023 04:56
@notmandatory notmandatory mentioned this pull request Jul 13, 2023
24 tasks
@notmandatory
Copy link
Member Author

Per comment: #1025 (comment)

I'm moving this to future alpha release.

@notmandatory notmandatory removed this from the 1.0.0-alpha.1 milestone Jul 15, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.4 milestone Jul 18, 2023
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

+1 on adding CreateTxError. left some comments.

crates/bdk/src/wallet/mod.rs Show resolved Hide resolved
example-crates/wallet_electrum/src/main.rs Outdated Show resolved Hide resolved
@nondiremanuel
Copy link

@notmandatory we didn't talk about this PR in the last calls. Is it correct to keep it into alpha.3 milestone? Shall we discuss it in the next call?

@notmandatory
Copy link
Member Author

@notmandatory we didn't talk about this PR in the last calls. Is it correct to keep it into alpha.3 milestone? Shall we discuss it in the next call?

I don't think we need to discuss it until after alpha.2 is released. Then I'll have to rebase it and fix a few small things and will bug folks for a re-review.

@notmandatory notmandatory force-pushed the tmp_fix_write_error branch 2 times, most recently from 26ce0fa to 55e2c9f Compare November 7, 2023 17:22
@notmandatory
Copy link
Member Author

I've removed the cfg for fmt::Display and created a new issue for that change (if we want to do it). Tests passing and ready for re-review.

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

I made minor comments, but nothing that should block this from moving forward

crates/bdk/examples/mnemonic_to_descriptors.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/tx_builder.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/error.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/mod.rs Show resolved Hide resolved
@notmandatory
Copy link
Member Author

notmandatory commented Nov 14, 2023

I pushed a couple more commits to add changes suggested by @ValuedMammal. I'll squash commits to clean up the history prior to merging.

@matthiasdebernardini
Copy link

Hey! This needs fixing too, "not enough"

Self::NotEnoughItemsSelected(err) => write!(f, "Not enought items selected: {}", err),

Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

Just nits. LGTM. ACK 35a108b

crates/bdk/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved
crates/bdk/src/wallet/mod.rs Show resolved Hide resolved
@notmandatory notmandatory force-pushed the tmp_fix_write_error branch 2 times, most recently from 92f8579 to 1ec4716 Compare November 15, 2023 21:53
@evanlinjin
Copy link
Member

I don't think this should be addressed in this PR, but I think we should use idiomatic naming for methods: https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

I.e. Wallet::get_address should be Wallet::address.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

ACK 35a108b

Thank you for this work. Now the errors are more domain-specific and detailed.

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

Fantastic work, than you.

I think Lloyd's comment should be fixed, as it should be fairly trivial. I left a comment on that.

crates/bdk/src/wallet/mod.rs Show resolved Hide resolved
@notmandatory
Copy link
Member Author

I think Lloyd's comment should be fixed, as it should be fairly trivial. I left a comment on that.

Ohhhh! 🤦‍♂️ thanks for the help here, now I get it. I'm going to push your fix, rebase, clean up the history a little, and then it should be ready to merge.

…ror mod

refactor(bdk)!: remove impl_error macro
refactor(wallet)!: add MiniscriptPsbtError, CreateTxError, BuildFeeBumpError error enums
refactor(coin_selection)!: add module Error enum
test(bdk): use anyhow dev-dependency for all tests
…ss functions

refactor!(wallet)!: rename fallible Wallet try_get_address(), try_get_internal_address functions
@notmandatory notmandatory dismissed LLFourn’s stale review November 16, 2023 18:39

I made the requested fix (with some help from Daniela)

@notmandatory notmandatory merged commit 46d39be into bitcoindevkit:master Nov 16, 2023
12 checks passed
@notmandatory notmandatory mentioned this pull request Dec 13, 2023
4 tasks
@notmandatory notmandatory mentioned this pull request Jan 3, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Wallet::get_address should return an Result bdk::Wallet should have context-specific error types
9 participants