Shouldn't `Amount` handle decimals formatting with `{:.8}` ? #2136

RCasatta · 2023-10-21T17:38:25Z

I would expect this to work

        assert_eq!("0.00000010 BTC", format!("{:.8}", Amount::from_sat(10)));

While it fails with:

  left: `"0.00000010 BTC"`,
 right: `"0.0000001 BTC"`', bitcoin/src/amount.rs:2379:9

The text was updated successfully, but these errors were encountered:

tcharding · 2023-10-21T19:55:58Z

Seems like a reasonable expectation.

Kixunil · 2023-10-22T06:21:52Z

Definitely a bug since we intended fully working formatting in Amount.

yancyribbens · 2023-10-30T08:54:08Z

I had a quick look at this bug, and it seems like the precision is not respected at all:

println!("amount {}", format!("{:.1}", Amount::from_sat(10)));
println!("amount {}", format!("{:.2}", Amount::from_sat(10)));
println!("amount {}", format!("{:.3}", Amount::from_sat(10)));
println!("amount {}", format!("{:.4}", Amount::from_sat(10)));
println!("amount {}", format!("{:.5}", Amount::from_sat(10)));
println!("amount {}", format!("{:.6}", Amount::from_sat(10)));
println!("amount {}", format!("{:.7}", Amount::from_sat(10)));
println!("amount {}", format!("{:.8}", Amount::from_sat(10)));
println!("amount {}", format!("{:.99}", Amount::from_sat(10)));

stdout:

amount 0.0000001 BTC                                                                           
amount 0.0000001 BTC                                                                           
amount 0.0000001 BTC
amount 0.0000001 BTC
amount 0.0000001 BTC
amount 0.0000001 BTC
amount 0.0000001 BTC
amount 0.0000001 BTC
amount 0.0000001 BTC

Also, the tests surrounding this all call macros, which I think could be simplified to helper functions.

Kixunil · 2023-10-30T08:56:30Z

Oh, now I remembered we purposely ignore precision because it'd cut off significant digits which is pretty bad in a financial application. But cutting zeros should be allowed.

yancyribbens · 2023-10-30T09:44:54Z

Oh, now I remembered we purposely ignore precision because it'd cut off significant digits which is pretty bad in a financial application.

Hmm well maybe panic or something if precision is used, because silently working may be disastrous if there's an assumption the precision is working.

But cutting zeros should be allowed.

in the bug stated above, it looks like the zero is cut when it should not be.

Kixunil · 2023-10-30T10:51:05Z

Oh, yeah, maybe panic for precision < 8 to be precise.

in the bug stated above, it looks like the zero is cut when it should not be.

Crap, my mind is broken today, sorry about that!

So yeah - no precision means infinite precision - don't panic, precision 0..8 - panic, precision 8+ - add zeroes.

RCasatta · 2023-10-30T14:32:05Z

because it'd cut off significant digits which is pretty bad in a financial application

If I ask any spreadsheet to format 0.01 with one decimal it gives me back "0.0" and it's fine.

The use case is putting bitcoin amounts in a text table so that you can read numbers in colums. I can get away with just {:.8} working but maybe someone has space constraint and wants {:.3} the colum total should sum with the underlying sats anyway.

If this is not acceptable, maybe one idea is using the alternate repr for that, eg {:#.3} ?

apoelstra · 2023-10-30T17:09:52Z

I agree. Display losing data doesn't mean that the data is actually lost (e.g. in a spreadsheet application).

Kixunil · 2023-10-30T17:48:42Z

I was thinking about having enable_truncating_decimals method on the builder. What do you think?

apoelstra · 2023-10-30T20:07:15Z

I think that's too confusing and undiscoverable. The precision modifier has an expected behavior (which includes dropping data) and we should just follow that behavior.

Kixunil · 2023-10-30T20:15:49Z

Hmm, I guess you're right.

It is common to display bitcoins using trailing zeros upto 8 decimals. This commit enforces: - Displaying Amount in BTC with trailing zeroes by eight decimal places if the Amount is not precision specified. - Displaying Amount in BTC upto the precision specified truncating the insignificant zeros. - Displaying amount in BTC without any decimals if the remainder of the amount divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0` These are not breaking changes and all previous tests pass. A testcase are also added to for changes introduced. Resolves: rust-bitcoin#2136

It is common to display bitcoins using trailing zeros upto 8 decimals. This commit enforces: - Displaying Amount in BTC with trailing zeroes by eight decimal places if the Amount is not precision specified. - Displaying Amount in BTC upto the precision specified truncating the insignificant zeros. - Displaying amount in BTC without any decimals if the remainder of the amount divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0` These are not breaking changes and all previous tests pass. A testcase are also added to for changes introduced. Resolves: rust-bitcoin#2136 implement clippy recommendations

It is common to display bitcoins using trailing zeros upto 8 decimals. This commit enforces: - Displaying Amount in BTC with trailing zeroes by eight decimal places if the Amount is not precision specified. - Displaying Amount in BTC upto the precision specified truncating the insignificant zeros. - Displaying amount in BTC without any decimals if the remainder of the amount divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0` These are not breaking changes and all previous tests pass. A testcase are also added to for changes introduced. Resolves: rust-bitcoin#2136

It is common to display bitcoins using trailing zeros upto 8 decimals. This commit enforces: - Displaying Amount in BTC with trailing zeroes by eight decimal places if a precision on the Amount is not specified. - Displaying Amount in BTC upto the precision specified truncating the insignificant zeros. - Displaying amount in BTC without any decimals if the remainder of the amount divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0` These are not breaking changes and all previous tests pass. A testcase is added to for changes introduced. Resolves: rust-bitcoin#2136

d887423 Enforce displaying Amount with trailing zeros (448 OG) Pull request description: It is common to display bitcoins using trailing zeros upto 8 decimals. This commit enforces: - Displaying Amount in BTC with trailing zeroes by eight decimal places if a precision on the Amount is not specified. - Displaying Amount in BTC upto the precision specified truncating the insignificant zeros. - Displaying amount in BTC without any decimals if the remainder of the amount divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0` These are not breaking changes and all previous tests pass. A testcase is added to for changes introduced. Resolves: #2136 ACKs for top commit: sanket1729: ACK d887423 apoelstra: ACK d887423 Tree-SHA512: c32e41216477f60a8d95f164bf4a1f6644ea14adc7e169743ce419b0f26ecb6603f3a516f9b18d6508c04ce658f6a0a78ff3b0b062aeb7101b28bbb1e9d522bc

RCasatta · 2024-03-17T19:32:04Z

#2604 solve the issue as described in the first message.

However, it's not "doing like a spreedsheet" when I ask n digits.

In other words, the following fails:

assert_eq!("0.0 BTC", format!("{:.1}", Amount::from_sat(1)));

apoelstra · 2024-03-17T19:52:04Z

Let's re-open the issue. #2604 was by a new contributor who deliberately didn't break any existing tests (which I appreciate) but maybe we need to be more aggressive.

448-OG · 2024-04-02T12:46:23Z

#2604 solve the issue as described in the first message.

However, it's not "doing like a spreedsheet" when I ask n digits.

In other words, the following fails:
assert_eq!("0.0 BTC", format!("{:.1}", Amount::from_sat(1)));

How would this be handled? Would a new method to handle precision with truncating insignificant zeroes suffice? Maybe something like Amount::to_precision()

apoelstra · 2024-04-02T13:00:44Z

No, we don't need a new method. The existing fmt method should just handle precision correctly.

448-OG · 2024-04-02T13:33:44Z

Thanks! I would like to solve this. Will follow up for more questions if I don't fully understand the task

All characters beyond the needed precision are truncated In the case of a SignedAmount the default precision is now 8 just like the Amount Just like the existing code, no width is added if there exists fractional values exist just like the existing code works. Tests have been improved to handle precision Resolves: rust-bitcoin#2136

All characters beyond the needed precision are truncated In the case of a SignedAmount the default precision is now 8 just like the Amount Replaces self.fmt_value_in() with fmt_satoshi_in() so that FomartOptions is not made public which would be the requirements if self.fmt_value_in() is used. Tests have been improved to handle precision Resolves: rust-bitcoin#2136

Kixunil added the bug label Oct 22, 2023

Kixunil added the C-units PRs modifying the units crate label Jan 27, 2024

448-OG mentioned this issue Mar 16, 2024

Enforce displaying Amount with trailing zeros #2604

Merged

apoelstra closed this as completed in d887423 Mar 17, 2024

apoelstra reopened this Mar 17, 2024

448-OG linked a pull request Apr 13, 2024 that will close this issue

Respect precision while formatting Amount #2682

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shouldn't `Amount` handle decimals formatting with `{:.8}` ? #2136

Shouldn't `Amount` handle decimals formatting with `{:.8}` ? #2136

RCasatta commented Oct 21, 2023

tcharding commented Oct 21, 2023

Kixunil commented Oct 22, 2023

yancyribbens commented Oct 30, 2023

Kixunil commented Oct 30, 2023

yancyribbens commented Oct 30, 2023

Kixunil commented Oct 30, 2023

RCasatta commented Oct 30, 2023

apoelstra commented Oct 30, 2023

Kixunil commented Oct 30, 2023

apoelstra commented Oct 30, 2023

Kixunil commented Oct 30, 2023

RCasatta commented Mar 17, 2024

apoelstra commented Mar 17, 2024

448-OG commented Apr 2, 2024

apoelstra commented Apr 2, 2024

448-OG commented Apr 2, 2024

Shouldn't Amount handle decimals formatting with {:.8} ? #2136

Shouldn't Amount handle decimals formatting with {:.8} ? #2136

Comments

RCasatta commented Oct 21, 2023

tcharding commented Oct 21, 2023

Kixunil commented Oct 22, 2023

yancyribbens commented Oct 30, 2023

Kixunil commented Oct 30, 2023

yancyribbens commented Oct 30, 2023

Kixunil commented Oct 30, 2023

RCasatta commented Oct 30, 2023

apoelstra commented Oct 30, 2023

Kixunil commented Oct 30, 2023

apoelstra commented Oct 30, 2023

Kixunil commented Oct 30, 2023

RCasatta commented Mar 17, 2024

apoelstra commented Mar 17, 2024

448-OG commented Apr 2, 2024

apoelstra commented Apr 2, 2024

448-OG commented Apr 2, 2024

Shouldn't `Amount` handle decimals formatting with `{:.8}` ? #2136

Shouldn't `Amount` handle decimals formatting with `{:.8}` ? #2136