Fix `Amount` decimals handling by Kixunil · Pull Request #2951 · rust-bitcoin/rust-bitcoin

Kixunil · 2024-07-02T07:29:36Z

Displaying with minimal number of zeros is the default in Rust and we want to follow it. This was originally implemented in #716 but #2604 reversed it claiming this is common, however it broke people who rely on minimal display (e.g. BIP21) without fixing the root cause of #2136.

This reverts commit d887423 adds a test to prevent this change and also fixes the problem with {:0.8} not working.

Closes #2136
Closes #2948

Can we backport this one too?

apoelstra · 2024-07-02T13:04:32Z

To be clear: the intended behavior is to display fewer zeros than the user requests with the {.X} formatter, even though this differs from the numeric types in stdlib?

apoelstra · 2024-07-02T13:11:32Z

Ah, no, I see from your tests that you do want the new behavior of the precision specifier. But we have broken behavior of the width specifier, and I think what you are complaining about is the default behavior of no specifier is given.

Yeah, I can accept this, because your desired behavior is quite hard to obtain with the existing code. Even though I think {:.8} is what users will want overwhelmingly often, and now they need to write it explicitly.

And sure, I wouldn't mind backporting it since bip21 depends on it.

I am a little annoyed at your framing that you are "reverting" a PR that broke no tests because it "reverted intentional behavior", when both this PR and the "intentional behavior" one do many different things at once. And your complaint is not even about the main change in the "reverted" PR.

yancyribbens · 2024-07-02T15:58:41Z

assert_eq!(format!("{}", Amount::from_sat(1000000)), "0.01 BTC"); looks good.

Kixunil · 2024-07-02T16:37:59Z

But we have broken behavior of the width specifier

We haven't given that width is still the same. It just looks like we did because if both precision and width are set, precision must be exactly what specified (potentially overriding width) and width just adapts. This is consistent with Rust's floats. (And yes, I did a bunch of experiments to figure this out.)

do many different things at once

Yes, this is inaccurate. Since nobody ACKed yet, I'm going to rewrite the description.

And your complaint is not even about the main change in the "reverted" PR.

It is. The change in that PR forced {:08x} which was previously intentionally removed, so it made the whole situation strictly worse without fixing anything.

Displaying with minimal number of zeros is the default in Rust and we want to follow it. This was originally implemented in rust-bitcoin#716 but rust-bitcoin#2604 reversed it claiming this is common, however it broke people who rely on minimal display (e.g. BIP21) without fixing the root cause of rust-bitcoin#2136. This reverts commit d887423 adds a test to prevent this change and also fixes the problem with `{:0.8}` not working. Closes rust-bitcoin#2136 Closes rust-bitcoin#2948

Low precision was previously not considered because it was believed it could lead to misleading data being displayed. However it's quite possible that people using low precision know what they are doing and it's sometimes useful to show rounded numbers. To enable low precision we just compute what the rounded number would be and display that one instead. This actually fully closes rust-bitcoin#2136 since this issue was mentioned there along with previously-fixed displaying of higher precisions.

tcharding

ACK 467546f

`fmt_value_in` was added when `display_in` wasn't available. However common usage patterns seem to favor `display_in`. It can be used within format strings and supports formatting options. Removing it will simplify the codebase, so this deprecates it.

coveralls · 2024-07-03T05:44:25Z

Pull Request Test Coverage Report for Build 9772503688

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

28 of 31 (90.32%) changed or added relevant lines in 1 file are covered.
45 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.06%) to 82.943%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
units/src/amount.rs	28	31	90.32%

Files with Coverage Reduction	New Missed Lines	%
units/src/amount.rs	15	86.86%
bitcoin/src/blockdata/script/borrowed.rs	30	79.21%

Totals
Change from base Build 9763162782:	-0.06%
Covered Lines:	19504
Relevant Lines:	23515

💛 - Coveralls

apoelstra · 2024-07-03T21:27:17Z

Thanks for changing the description. This looks like a correct description of the problem and history and I understand now why you were so upset by it.

tcharding

ACK 3196c27

apoelstra

ACK 3196c27 I also really like how this reduces the complexity of the module, basically passing everything through to display_in

We an use `Amount::display_in` now, this was done on master in PR rust-bitcoin#2951.

107acf3 backport: Deprecate Amount::fmt_value_in (Tobin C. Harding) Pull request description: We an use `Amount::display_in` now, this was done on master in PR #2951. ACKs for top commit: shinghim: ACK 107acf3 apoelstra: ACK 107acf3; successfully ran local tests Tree-SHA512: b46156415a281caad5cd3238921f70dd8f575d700b901583fcdd9e037f51dab348d44efb1a74b385252a37d677d7fd7369ec7b623b680585a9427f1d1c448fb0

We an use `Amount::display_in` now, this was done on master in PR rust-bitcoin#2951.

Kixunil added the bug label Jul 2, 2024

github-actions bot added the C-units PRs modifying the units crate label Jul 2, 2024