Add `amount::Display` - make formatting configurable by Kixunil · Pull Request #716 · rust-bitcoin/rust-bitcoin

Kixunil · 2021-11-23T16:52:25Z

This significatnly refactors the formatting code to make formatting more
configurable. The main addition is the Display type which is a
builder that can configure denomination or other things (possibly more
in the future).

Further, this makes all representations of numbers minimal by default,
so should be documented as a possibly-breaking change.

Because of the effort to support all other fmt::Formatter options this
required practically complete rewrite of fmt_satoshi_in. As a
byproduct I took the opportunity of removing one allocation from there.

Closes #709

dr-orlovsky · 2021-11-23T16:58:43Z

the way Amount is displayed (and also parsed from string) is clearly poor. But I haven't got from the original discussion why just using formatting directives is not enough and we need to introduce dedicated Display type and builder.

Kixunil · 2021-11-23T17:09:02Z

Because there's no way to pass more than two possible custom formats into fmt::Formatter (not abusing other things which we actually want to use for what they were designed for).

dr-orlovsky · 2021-11-23T17:50:50Z

Concept ACK

apoelstra · 2021-12-20T15:45:11Z

concept ACK. It's not clear from skimming the PR what happens when you just try to Display a bare amount, but I guess it should be the same as using display_dynamic?

Kixunil · 2021-12-20T17:49:45Z

@apoelstra I didn't change the original implementation which was displaying amount in BTC with BTC suffix.

apoelstra · 2021-12-22T23:42:15Z

Ok, that's a reasonable default too.

Kixunil · 2022-02-09T13:19:50Z

Rebased, added tests, refactored&implemented SignedAmount

~~I'm not sure if the commits should be squashed though. Leaning a bit towards yes. What do you think?~~ Decided to go for it, since the commits were becoming a mess.

src/util/amount.rs

Kixunil · 2022-02-09T13:30:59Z

src/util/amount.rs

Forgot to mention: I'm making them separate tests because in practice it was very hard to identify which failed.

thomaseizinger

concept ACK

Some doc nits.

src/util/amount.rs

Kixunil · 2022-02-09T22:41:42Z

The MSRV is soooo old 😭

tcharding

Wow, thorough testing, nice effort.

src/util/amount.rs

Kixunil · 2022-02-10T12:40:10Z

@tcharding not thorough enough. I forgot to test precision > 0 branch and indeed it was broken.

Further, I originally wanted sats to behave like integer (ignoring .1) but it recently started feeling super-weird, so I changed it to also work like floats.

I just pushed both fixes.

Kixunil · 2022-02-10T13:00:11Z

The latest push changes naive manual exponentiation to faster pow method.

tcharding · 2022-02-11T05:59:05Z

src/util/amount.rs

+
 /// SignedAmount
 ///
 /// The [SignedAmount] type can be used to express Bitcoin amounts that supports


Not introduced in this PR but 'supports' shouldn't be plural.

There are two instances of this typo.

How about having an issue where people can link typos and once there are many we do a larger batch of fixes?

That's a good idea, should save time.

dr-orlovsky

utACK 31ae760

src/util/amount.rs

This significatnly refactors the amount formatting code to make formatting more configurable. The main addition is the `amount::Display` type which is a builder that can configure denomination or other things (possibly more in the future). Further, this makes all representations of numbers minimal by default, so should be documented as a possibly-breaking change. Because of the effort to support all other `fmt::Formatter` options this required practically complete rewrite of `fmt_satoshi_in`. As a byproduct I took the opportunity of removing one allocation from there. Closes rust-bitcoin#709

Kixunil · 2022-03-14T18:03:36Z

Renamed.

dr-orlovsky

ACK 4f1200d

tcharding

LGTM

sanket1729

ACK 4f1200d
Thanks for being super careful with this and the elaborate test cases. I learned a lot about format options reviewing this. Hopefully, this settles the Amount Display issue for a long time.

We should add this in release notes as a breaking change

tcharding · 2022-04-25T04:16:46Z

ACK 4f1200d

…matting configurable 4f1200d Added `amount::Display` - configurable formatting (Martin Habovstiak) Pull request description: This significatnly refactors the formatting code to make formatting more configurable. The main addition is the `Display` type which is a builder that can configure denomination or other things (possibly more in the future). Further, this makes all representations of numbers minimal by default, so should be documented as a possibly-breaking change. Because of the effort to support all other `fmt::Formatter` options this required practically complete rewrite of `fmt_satoshi_in`. As a byproduct I took the opportunity of removing one allocation from there. Closes #709 ACKs for top commit: tcharding: ACK 4f1200d dr-orlovsky: ACK 4f1200d sanket1729: ACK 4f1200d Tree-SHA512: 3fafdf63fd720fd4514e026e9d323ac45dfcd3d3a53a4943178b1e84e4cf7603cb6235ecd3989d46c4ae29453c4b0bb2f2a5996fbddf341cd3f68dc286062144

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

3196c27 Deprecate `Amount::fmt_value_in` (Martin Habovstiak) 467546f Round `Amount` when requested precision is too low (Martin Habovstiak) 7c95a77 Fix `Amount` decimals handling (Martin Habovstiak) Pull request description: 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? ACKs for top commit: 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` Tree-SHA512: 3221f83086ac55af3d4caad03fe2b619be303533bba12096040419d119600c8597938809e171662f11b515d469156b083b2072b901d445e4fdfc7b1062cf7b6a

Kixunil added enhancement help wanted API break This PR requires a version bump for the next release labels Nov 23, 2021

tcharding mentioned this pull request Jan 11, 2022

de/serde for Amount by way of as_string #730

Closed

Kixunil force-pushed the configurable_amount_display branch from 5267db1 to a41c43f Compare February 9, 2022 13:17

Kixunil marked this pull request as ready for review February 9, 2022 13:17

Kixunil removed the help wanted label Feb 9, 2022

Kixunil changed the title ~~Add DisplayAmount - make formatting configurable~~ Add amount::Display - make formatting configurable Feb 9, 2022

Kixunil commented Feb 9, 2022

View reviewed changes

src/util/amount.rs Outdated Show resolved Hide resolved

Kixunil commented Feb 9, 2022

View reviewed changes

thomaseizinger reviewed Feb 9, 2022

View reviewed changes

src/util/amount.rs Outdated Show resolved Hide resolved

src/util/amount.rs Outdated Show resolved Hide resolved

src/util/amount.rs Outdated Show resolved Hide resolved

src/util/amount.rs Outdated Show resolved Hide resolved

Kixunil force-pushed the configurable_amount_display branch 3 times, most recently from 2b8cc5d to 88bd771 Compare February 9, 2022 22:40

Kixunil mentioned this pull request Feb 9, 2022

Rust 2018 & MSRV bump 1.41.1 tracking issue #510

Closed

20 tasks

tcharding reviewed Feb 10, 2022

View reviewed changes

src/util/amount.rs Outdated Show resolved Hide resolved

Kixunil force-pushed the configurable_amount_display branch 2 times, most recently from 3a0eb09 to c991b07 Compare February 10, 2022 12:37

Kixunil force-pushed the configurable_amount_display branch from c991b07 to 31ae760 Compare February 10, 2022 12:59

tcharding reviewed Feb 11, 2022

View reviewed changes

dr-orlovsky previously approved these changes Mar 12, 2022

View reviewed changes

src/util/amount.rs Outdated Show resolved Hide resolved

Kixunil dismissed dr-orlovsky’s stale review via 4f1200d March 14, 2022 18:03

Kixunil force-pushed the configurable_amount_display branch from 31ae760 to 4f1200d Compare March 14, 2022 18:03

dr-orlovsky approved these changes Mar 18, 2022

View reviewed changes

dr-orlovsky added this to the 0.29.0 milestone Mar 18, 2022

Kixunil added the before edition bump We want to merge this before edition is bumped label Apr 20, 2022

tcharding approved these changes Apr 21, 2022

View reviewed changes

sanket1729 approved these changes Apr 22, 2022

View reviewed changes

sanket1729 added the release notes mention label Apr 22, 2022

sanket1729 merged commit bcc923c into rust-bitcoin:master Apr 30, 2022

Kixunil deleted the configurable_amount_display branch May 2, 2022 11:53

Kixunil mentioned this pull request May 2, 2022

msat formatting support #981

Open

Kixunil mentioned this pull request Jul 2, 2024

Enforce displaying Amount with trailing zeros #2604

Merged

Kixunil mentioned this pull request Jul 2, 2024

Fix Amount decimals handling #2951

Merged

jlest01 added a commit to jlest01/rust-bitcoin that referenced this pull request Aug 10, 2024

Update secp256k1 dependency to use PR rust-bitcoin#716

a293987

Conversation

Kixunil commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky commented Nov 23, 2021

Uh oh!

Kixunil commented Nov 23, 2021

Uh oh!

dr-orlovsky commented Nov 23, 2021

Uh oh!

apoelstra commented Dec 20, 2021

Uh oh!

Kixunil commented Dec 20, 2021

Uh oh!

apoelstra commented Dec 22, 2021

Uh oh!

Kixunil commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Kixunil Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kixunil commented Feb 9, 2022

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kixunil commented Feb 10, 2022

Uh oh!

Kixunil commented Feb 10, 2022

Uh oh!

tcharding Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

Kixunil Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

tcharding Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

Kixunil Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kixunil commented Mar 14, 2022

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Kixunil commented Nov 23, 2021 •

edited

Loading

Kixunil commented Feb 9, 2022 •

edited

Loading