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.

Sign up for GitHub

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

Jump to bottom

test(electrum): test for checking that fee calculation is correct #1685

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

test(electrum): test for checking that fee calculation is correct #1685

wants to merge 1 commit into from

Conversation

f3r10
Copy link
Contributor

@f3r10 f3r10 commented Nov 13, 2024 •
edited
Loading

Description

This PR adds a unit test for checking that the fee calculation of bdk_electrum is correct.

a "preliminary" tx to an address in Core's wallet such that the output created becomes the input of the next tx. This preliminary tx is the prev_tx that needs to be fetched in order to calculate the fee of the tx sent to the receiver. This way is better because the required outpoint is less determined. Then we assert we have the right (outpoint, txout) in the update TxGraph #1444 (comment)

Notes to the reviewers

fixes: #1444

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

@f3r10 f3r10 marked this pull request as ready for review November 14, 2024 18:28
@notmandatory notmandatory added this to the 1.0.0-beta milestone Nov 15, 2024
@LagginTimes
Copy link
Contributor

LagginTimes commented Nov 15, 2024 •
edited
Loading

Thank you for taking this on! It looks good to me so far.
Just a suggestion: squash your second commit into the first to keep the history cleaner.

Edit: Also just a reminder to address the clippy warning.

@notmandatory notmandatory removed this from the 1.0.0-beta milestone Nov 21, 2024
@f3r10 f3r10 force-pushed the test_electrum_fee_calculation branch from 5ed5266 to 99888fa Compare November 25, 2024 18:57
LagginTimes
LagginTimes reviewed Dec 2, 2024
View reviewed changes
Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

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

To ensure the CI tests pass, please rebase your PR and ensure the clippy warning has been fixed. Once that's done, everything should be good to go!

crates/electrum/tests/test_electrum.rs Outdated Show resolved Hide resolved
@f3r10 f3r10 force-pushed the test_electrum_fee_calculation branch 2 times, most recently from 6b67cab to 836b097 Compare December 3, 2024 17:27
@f3r10 f3r10 requested a review from LagginTimes December 3, 2024 17:43
@LagginTimes
Copy link
Contributor

ACK 836b097

LagginTimes
LagginTimes requested changes Dec 7, 2024
View reviewed changes
Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

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

It looks like another rebase is needed to ensure everything is up to date. Thank you for your continued efforts!

@f3r10 f3r10 force-pushed the test_electrum_fee_calculation branch from 836b097 to fb7ca7e Compare December 9, 2024 13:52
@f3r10 f3r10 requested a review from LagginTimes December 9, 2024 13:53
LagginTimes
LagginTimes approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@LagginTimes LagginTimes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK ecd3c3a

@f3r10 f3r10 force-pushed the test_electrum_fee_calculation branch 2 times, most recently from e6aae33 to ecd3c3a Compare December 9, 2024 23:22
@f3r10
Copy link
Contributor Author

f3r10 commented Dec 9, 2024

ACK fb7ca7e

Sorry @LagginTimes I did not sign the commit 😞
I just did it now

@f3r10 f3r10 requested a review from LagginTimes December 9, 2024 23:29
oleonardolima
oleonardolima reviewed Dec 12, 2024
View reviewed changes
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.

cACK ecd3c3a

crates/electrum/tests/test_electrum.rs Outdated
let txid = env.send(&addr_to_track, SEND_AMOUNT)?;

// Mine a block to confirm sent tx.
let hash_block = env.mine_blocks(1, None)?.into_iter().next();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would use block_hash, and prev_block_hash instead, but it's not a big deal. :)

crates/electrum/tests/test_electrum.rs
.graph()
.calculate_fee(&tx.tx)
.expect("fee must exist");

Copy link
Contributor

Choose a reason for hiding this comment

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

❓ You could probably assert the expected fee here too, to ensure it's the same 1650 as expected. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, maybe use it as constant like SEND_AMOUNT, e.g FEE_AMOUNT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point @oleonardolima I am going to add the additional assert.

@f3r10 f3r10 force-pushed the test_electrum_fee_calculation branch from ecd3c3a to 06e5248 Compare December 18, 2024 15:25
@ValuedMammal ValuedMammal added this to the 1.1.0 milestone Jan 3, 2025
oleonardolima
oleonardolima approved these changes Jan 14, 2025
View reviewed changes
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.

tACK 06e5248

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleonardolima oleonardolima oleonardolima approved these changes

@LagginTimes LagginTimes Awaiting requested review from LagginTimes

Assignees

@f3r10 f3r10

Labels
tests
Projects
BDK
Status: Needs Review
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

More tests for bdk_electrum fee calculation
5 participants
@f3r10 @LagginTimes @oleonardolima @notmandatory @ValuedMammal