Skip to content

test: add legacy descriptor tests #214

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

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

Conversation

luisschwab
Copy link
Member

@luisschwab luisschwab commented Apr 22, 2025

Description

This PR closes #134 and is a revival of bitcoindevkit/bdk#1130, which adds the following tests:

  • test_legacy_bump_fee_no_change_add_input_and_change()
  • test_legacy_bump_fee_add_input()
  • test_legacy_bump_fee_drain_wallet()
  • test_legacy_bump_fee_zero_abs()
  • test_legacy_create_tx_custom_sighash()
  • test_legacy_create_tx_default_sighash()
  • test_legacy_create_tx_absolute_high_fee()
  • test_legacy_create_tx_absolute_zero_fee()
  • test_legacy_create_tx_absolute_fee()
  • test_legacy_create_tx_custom_fee_rate()
  • test_legacy_get_funded_wallet_tx_fee_rate()

Changelog:

  • Add the above mentioned tests.
  • Add a new assert_fee_rate_legacy! macro for legacy transactions.
  • The check_fee! macro now returns Amount instead of Result<Amount>.
  • All wallet.sent_and_received got destructured to (sent, received) instead of sent_and_received.{0,1}.
  • Swap TweakedKeypair::to_inner() for TweakedKeypair::to_keypair().

Checklists

All Submissions:

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

@coveralls
Copy link

coveralls commented Apr 22, 2025

Pull Request Test Coverage Report for Build 15028696206

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 86.075%

Totals Coverage Status
Change from base Build 14921254296: 0.005%
Covered Lines: 7269
Relevant Lines: 8445

💛 - Coveralls

@luisschwab luisschwab force-pushed the feat/add-legacy-tests branch from 9f667ca to 5f9d18c Compare April 22, 2025 03:34
@ValuedMammal ValuedMammal added the tests New or improved tests label Apr 22, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Apr 22, 2025
@ValuedMammal ValuedMammal added this to the 2.0.0 milestone Apr 22, 2025
@luisschwab luisschwab force-pushed the feat/add-legacy-tests branch from 5f9d18c to 89643fb Compare May 9, 2025 23:14
@luisschwab luisschwab requested a review from ValuedMammal May 9, 2025 23:17
@luisschwab luisschwab force-pushed the feat/add-legacy-tests branch from 89643fb to 71ae907 Compare May 9, 2025 23:23
@luisschwab luisschwab requested a review from oleonardolima May 10, 2025 15:59
@ValuedMammal
Copy link
Collaborator

Side note: I think check_fee! should just return the bitcoin Amount unwrapped instead of being forced to deal with the Option everywhere. If we fail to calculate the fee it should probably just panic to indicate a failing test.

macro_rules! check_fee {
($wallet:expr, $psbt: expr) => {{
let tx = $psbt.clone().extract_tx().expect("failed to extract tx");
let tx_fee = $wallet.calculate_fee(&tx).ok();
assert_eq!(tx_fee, $psbt.fee_amount());
tx_fee
}};
}

Copy link
Collaborator

@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.

Code review ACK 71ae907

@luisschwab luisschwab force-pushed the feat/add-legacy-tests branch from 71ae907 to 80c2752 Compare May 14, 2025 16:13
@luisschwab
Copy link
Member Author

luisschwab commented May 14, 2025

Should I fix this clippy warning? It's unrelated.

error: use of deprecated method `bitcoin::key::TweakedKeypair::to_inner`: use to_keypair() instead
   --> wallet/src/wallet/signer.rs:580:14
    |
580 |             .to_inner(),
    |              ^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(deprecated)]`

@luisschwab luisschwab requested a review from ValuedMammal May 14, 2025 16:24
@ValuedMammal
Copy link
Collaborator

I would say go ahead and change TweakedKeypair::to_inner to use to_keypair instead. For context see rust-bitcoin/rust-bitcoin#4450.

@luisschwab luisschwab force-pushed the feat/add-legacy-tests branch from 7b53984 to dd31471 Compare May 14, 2025 18:48
@luisschwab luisschwab force-pushed the feat/add-legacy-tests branch from dd31471 to a5d1338 Compare May 14, 2025 18:50
@luisschwab
Copy link
Member Author

@ValuedMammal fixed.

@ValuedMammal
Copy link
Collaborator

It looks good overall, tests are passing for me.

@luisschwab luisschwab mentioned this pull request May 18, 2025
3 tasks
@tvpeter
Copy link
Contributor

tvpeter commented May 19, 2025

@ValuedMammal fixed.

When I updated to #a5d1338d1b0402db7c7000a538a6e81a805ce073, the tests failed locally because the compiler was using bitcoin version 0.32.5.

--> wallet/src/wallet/signer.rs:580:14
    |
578 |           None => keypair
    |  _________________-
579 | |             .tap_tweak(secp, psbt_input.tap_merkle_root)
580 | |             .to_keypair(),
    | |             -^^^^^^^^^^ method not found in `TweakedKeypair`
    | |_____________|
    |
    

I temporarily updated the version to 0.32.6, and after that, the compiler used the new version, and the tests passed. I'm uncertain if it's ideal to upgrade to 0.32.6, and I'm curious if anyone else has experienced the same issue.

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

Successfully merging this pull request may close these issues.

Add legacy wallet tests
4 participants