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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

e1a0a0ea
Copy link

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.

@@ -4,7 +4,7 @@ use assert_matches::assert_matches;
use bdk::descriptor::{calc_checksum, IntoWalletDescriptor};
use bdk::psbt::PsbtUtils;
use bdk::signer::{SignOptions, SignerError};
use bdk::wallet::coin_selection::{self, LargestFirstCoinSelection};
use bdk::wallet::coin_selection::{self, CoinSelectionError, LargestFirstCoinSelection};
Copy link
Contributor

Choose a reason for hiding this comment

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

@e1a0a0ea Does this change still applies?

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.

@notmandatory
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

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