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

Add BIP21 test for amounts where f64 parsing may lose precision #67

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Aug 20, 2024

No description provided.

@ok300 ok300 force-pushed the ok300-fix-mrh-bip21-amount-rounding branch from 712167e to e34710e Compare August 20, 2024 10:05
@ok300 ok300 force-pushed the ok300-fix-mrh-bip21-amount-rounding branch from e34710e to 5fe27fa Compare August 20, 2024 10:09
@ok300
Copy link
Contributor Author

ok300 commented Aug 20, 2024

@i5hi we found some edge-cases when directly parsing the BIP21 (floating-point) BTC amount to (integer) sat amount. I added a unit test to demonstrate it:

 thread 'swaps::magic_routing::test_bip21_parsing_with_rounding_edge_cases' panicked at src/swaps/magic_routing.rs:176:9:
assertion `left == right` failed
  left: 998
 right: 999

What happens is that the initial f64 seems to lose precision for some values when multiplied by 100_000_000.0 during the conversion to sats. This doesn't happen for all values, but only for some.

One fix for this is to change this crate's BIP21 methods, so that instead of f64 they return a bitcoin::Amount :

// Before
pub fn parse_bip21(uri: &str) -> Result<(String, String, f64, Option<String>), Error>;

// After
pub fn parse_bip21(uri: &str) -> Result<(String, String, bitcoin::Amount, Option<String>), Error>;

There is a method in the bitcoin crate that builds an Amount from a string: https://docs.rs/bitcoin/latest/bitcoin/struct.Amount.html#method.from_str_in . The returned amount can then be expressed by the caller in whichever units (amount.to_btc() , amount.to_sat(), etc.) without risking precision loss.

Should I implement this fix in this PR?

@ok300 ok300 marked this pull request as ready for review August 27, 2024 09:23
@ok300
Copy link
Contributor Author

ok300 commented Aug 27, 2024

@i5hi it's ready for review.

An unrelated test is failing but I wasn't sure how or if I can fix it:

failures:
    network::electrum::tests::test_electrum_default_clients

@michael1011 michael1011 requested a review from i5hi August 27, 2024 10:58
@i5hi
Copy link
Collaborator

i5hi commented Aug 30, 2024

Nice patch!
The electrum test is failing on trunk as well. Issue with Blockstreams testnet server.
Thanks @ok300 !

@i5hi i5hi closed this Aug 30, 2024
@i5hi i5hi reopened this Aug 30, 2024
@i5hi i5hi merged commit d6f0815 into SatoshiPortal:trunk Aug 30, 2024
6 of 8 checks passed
@ok300 ok300 deleted the ok300-fix-mrh-bip21-amount-rounding branch September 2, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants