Skip to content

test,refactor(wallet): Move bump fee + foreign utxo tests #1907

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

Closed

Conversation

ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented Mar 21, 2025

These files are added to wallet/tests directory

  • add_foreign_utxo.rs
  • build_fee_bump.rs
  • common.rs

fixes bitcoindevkit/bdk_wallet#21

Changelog notice

Checklists

All Submissions:

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

These files added to wallet/tests directory
- add_foreign_utxo.rs
- build_fee_bump.rs
@ValuedMammal ValuedMammal added this to the 1.2.0 milestone Mar 21, 2025
@ValuedMammal ValuedMammal self-assigned this Mar 21, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Mar 21, 2025
Copy link

@aagbotemi aagbotemi left a comment

Choose a reason for hiding this comment

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

Good job working on this PR @ValuedMammal

let mut psbt = builder.finish().unwrap();
wallet1.insert_txout(utxo.outpoint, utxo.txout);
let fee = check_fee!(wallet1, psbt);
let sent_received =
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it will be easier to read if this tuple is named (sent_amount, received_amount)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree

.only_witness_utxo()
.add_foreign_utxo(utxo.outpoint, psbt_input, foreign_utxo_satisfaction)
.unwrap();
let mut psbt = builder.finish().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about how the change output was handled. I can't see a test case or the method responsible for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If no change address is given, then the wallet looks for the next unused address from the change keychain

// get drain script
let mut drain_index = Option::<(KeychainKind, u32)>::None;
let drain_script = match params.drain_to {
Some(ref drain_recipient) => drain_recipient.clone(),
None => {

And the result of coin selection determines whether the change output is finally added.

// if there's change, create and add a change output
if let Excess::Change { amount, .. } = excess {
// create drain output
let drain_output = TxOut {
value: *amount,
script_pubkey: drain_script,
};

Copy link
Contributor

@tvpeter tvpeter left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I have learned a lot from this PR.

tACK 9ca1529

nit: I have left some comments. Thanks

@ValuedMammal
Copy link
Collaborator Author

@tvpeter thanks for reviewing. I agree with your suggestions, but since this is a refactor/ code move, it would be better in my opinion to not introduce logical changes and instead push those to a follow up PR.

@ValuedMammal ValuedMammal modified the milestones: 1.2.0, 1.3.0 Apr 3, 2025
@ValuedMammal ValuedMammal deleted the refactor/wallet-tests branch April 7, 2025 21:04
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Apr 7, 2025
@ValuedMammal
Copy link
Collaborator Author

Reopened bitcoindevkit/bdk_wallet#199

@notmandatory notmandatory removed this from the Wallet 2.0.0 milestone Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move bump-fee and foreign utxo tests to their own files
4 participants