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

test: legacy descriptors #1130

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

Conversation

realeinherjar
Copy link
Contributor

Description

We currently don't have much tests with legacy descriptors (pkh).
This PR adds some, mainly focused on fee stuff.

Notes to the reviewers

This discussion came up on #1115 and also in #1129 (it doesn't fully close).

Changelog notice

Added

  • Tests for fee calculation and fee bump with legacy descriptors.

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

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

Comment on lines -1200 to +1320
"shoulld be ok when outpoint does match psbt_input"
"should be ok when outpoint does match psbt_input"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This typo was autofixed by my linter.

@realeinherjar
Copy link
Contributor Author

This is ready, Cc @notmandatory and @danielabrozzoni.

(PS: Thanks for all the help, Daniela)

@@ -12,7 +12,7 @@ members = [
"example-crates/wallet_esplora_blocking",
"example-crates/wallet_esplora_async",
"nursery/tmp_plan",
"nursery/coin_select"
"nursery/coin_select",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

autoformat

Comment on lines -154 to +170
#[allow(unused_mut)]
#[allow(unused_mut)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

autoformat

Comment on lines +464 to +473
/// A floating point assert! that takes an additional third argument `delta`
/// as a tolerance value when test for equality the first and second argument.
macro_rules! assert_delta {
($x:expr, $y:expr, $d:expr) => {
if !($x - $y < $d || $y - $x < $d) {
panic!();
}
};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This the way I've found to ignore rounding errors when comparing FeeRate::from_sat_per_vb after the wallet signed with PKH descriptor.

Copy link
Member

Choose a reason for hiding this comment

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

I think once #1141 is in you'll be able to use a different function to compare fee rates like to_sat_per_kwu(self) or to_sat_per_vb_ceil(self).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am following #1141. Once it is in we can remove this.
It is your call if you want to add these tests before #1141.

@oleonardolima
Copy link
Contributor

I think the proposed tests are useful and a great addition. However the PR has become stale and I guess we could move it out from 1.0.0-beta milestone, @notmandatory maybe to 1.1.0 ?

@notmandatory notmandatory removed this from the 1.0.0-beta milestone Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants