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 duplicated InsufficientFunds error member #1441

Merged

Conversation

e1a0a0ea
Copy link
Contributor

closes #1440

Description

  • Replace CreateTxError::InsufficientFunds use by coin_selection::Error::InsufficientFunds
  • Remove InsufficientFunds member from CreateTxError enum
  • Rename coin_selection::Error to coin_selection::CoinSelectionError

Notes to the reviewers

  • We could also keep both members but rename one of them to avoid confusion

Checklists

All Submissions:

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

@ValuedMammal
Copy link
Contributor

I tend to favor the name Error and qualifying it with its containing module, i.e. coin_selection::Error.

@notmandatory
Copy link
Member

Please also setup commit signing, see: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

@matthiasdebernardini
Copy link

Maybe this belongs here better, basically, can this be disabled in anything but Network::Bitcoin?

#1440 (comment)

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

Concept ACK, I do agree with Value's comment, and mind Steve's comment above.

@oleonardolima
Copy link
Contributor

Maybe this belongs here better, basically, can this be disabled in anything but Network::Bitcoin?

#1440 (comment)

I personally don't think it's a good idea to "error-gate" based on the bitcoin::Network type, I would expect all behaviors that happen on mainnet (production per se) to happen on others, and even some nasty behaviors we may catch while testing on testnet/signet being prevented to land on mainnet code 😅.

@matthiasdebernardini
Copy link

That's a good point, @oleonardolima maybe the juice is not worth the squeeze on making this change.

@e1a0a0ea e1a0a0ea force-pushed the duplicated-insufficient-fund-error branch from 2cfae0b to 9410137 Compare May 14, 2024 16:29
oleonardolima

This comment was marked as duplicate.

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

Looking good! I left some comments, and I also think that it could be squashed into a single commit.

crates/bdk/tests/wallet.rs Outdated Show resolved Hide resolved
available: remaining_amount.saturating_sub(*change_fee),
});
return Err(CreateTxError::CoinSelection(
Error::InsufficientFunds {
Copy link
Contributor

Choose a reason for hiding this comment

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

@e1a0a0ea You could probably use a qualified version: coin_selection::Error::InsufficientFunds instead 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just import the InsufficientFunds variant itself.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of importing the enum variant (can become confusing imo). I prefer the more verbose, but clearer option.

@notmandatory
Copy link
Member

Please rebase again and fixup commits into one commit then this should be ready to merge.

@e1a0a0ea e1a0a0ea force-pushed the duplicated-insufficient-fund-error branch from fb0be23 to cc1b417 Compare June 5, 2024 10:05
…CreateTxError

review: move back to old error naming
@evanlinjin evanlinjin force-pushed the duplicated-insufficient-fund-error branch from cc1b417 to 29c8a00 Compare June 6, 2024 03:08
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 29c8a00

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 29c8a00

@evanlinjin evanlinjin merged commit b4a847f into bitcoindevkit:master Jun 6, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

InsufficientFunds is duplicated within coin_selection::Error and CreateTxError enums
6 participants