SatoshiPortal / boltz-rust

Boltz client rust crate
https://docs.boltz.exchange
30 stars 19 forks source link

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

Closed ok300 closed 3 months ago

ok300 commented 3 months ago

@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 commented 3 months ago

@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
i5hi commented 3 months ago

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