add nano and pico BTC to Denomination enum #768

kafaichoi · 2022-01-10T15:04:09Z

Close 741

apoelstra

ACK 7645b26

Confirmed that the existing logic handles overflow conditions etc

Kixunil

P is confusing - it should mean "peta" - this needs to be handled similarly to milli/Mega
Also, maybe we want to reserve "N"?

tcharding

With @Kixunil's comment and the re-order, LGTM.

tcharding · 2022-01-10T22:29:57Z

src/util/amount.rs

            Denomination::MicroBitcoin => -2,
            Denomination::Bit => -2,
            Denomination::Satoshi => 0,
+            Denomination::NanoBitcoin => 1,
            Denomination::MilliSatoshi => 3,
+            Denomination::PicoBitcoin => 4,


Can we have these two lines added directly underneach MicroBitcoin? So that all the BTC sub-units are together.

fn precision(self) -> i32 { match self { Denomination::Bitcoin => -8, Denomination::MilliBitcoin => -5, Denomination::MicroBitcoin => -2, Denomination::NanoBitcoin => 1, Denomination::PicoBitcoin => 4, Denomination::Bit => -2, Denomination::Satoshi => 0, Denomination::MilliSatoshi => 3, } }

Sure. My initial thought is using precision order but I agree it makes more sense to group by unit

tcharding · 2022-01-10T22:30:12Z

src/util/amount.rs

            Denomination::Bit => "bits",
            Denomination::Satoshi => "satoshi",
            Denomination::MilliSatoshi => "msat",
+            Denomination::PicoBitcoin => "pBTC",
        })


And same ordering here.

tcharding · 2022-01-11T01:23:52Z

Also, maybe we want to reserve "N"?

What does 'N' conflict with?

ref: https://www.nist.gov/pml/weights-and-measures/metric-si-prefixes

kafaichoi · 2022-01-11T01:37:29Z

Also, maybe we want to reserve "N"?

What does 'N' conflict with?

ref: https://www.nist.gov/pml/weights-and-measures/metric-si-prefixes

I can only think of Newton 😃

Kixunil · 2022-01-11T11:41:47Z

I meant what if SI adds another thing that begins with N? Anyway, it might be a bit surprising if we disallow capitalization of other units and not this one. Still the error message would have to be different.

I'm not certain about this, just saying for consideration.

dr-orlovsky · 2022-01-11T12:22:24Z

I do agree we should stick to the single letter case for unit denominations even if the SI does not defines any meaning for alternative case of some letter. At the same time "btc/bitcoin" part may be of any case

kafaichoi · 2022-01-11T12:29:24Z

Right but then it should be a separate PR to remove the logic accepting any upper/lower case in from_str(if we really decide to do so)?

I believed I have addressed all other change requests🙏🏽.

Kixunil · 2022-01-11T13:04:10Z

@bruteforcecat it already rejects unexpected prefixes and I consider it a requirement that all future code does. Otherwise it's a huge footgun.

Kixunil

At least the error message needs to be fixed.

Kixunil · 2022-01-11T13:14:13Z

src/util/amount.rs

    /// - Plural or singular: sat, satoshi, bit, msat
    ///
    /// Due to ambiguity between mega and milli we prohibit usage of leading capital 'M'.
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        use self::ParseAmountError::*;

-        if s.starts_with('M') {
+        if s.starts_with(|ch| ch == 'M' || ch == 'P') {


This is correct, you just need to modify the error message handling of PossiblyConfusingDenomination

let (upper, lower) = match d.chars().next() { Some('M') => ("Mega", "milli"), Some('P') => ("Peta", "pico"), // This panic could be avoided by adding enum ConfusingDenomination { Mega, Peta } but is it worth it? _ => panic!("invalid error information"), }; write!(f, "the '{}' at the beginning of {} should technically mean '{}' but that denomination is uncommon and maybe '{}' was intended", d.chars().next().unwrap(), d, upper, lower)

Kixunil · 2022-01-11T13:16:08Z

src/util/amount.rs

@@ -100,6 +108,14 @@ fn denomination_from_str(mut s: &str) -> Option<Denomination> {
        return Some(Denomination::MicroBitcoin);
    }

+    if s.eq_ignore_ascii_case("nBTC") {
+        return Some(Denomination::NanoBitcoin);


I suggest adding inside this if:

if s.starts_with('N') { return None; }

How about doing this in from_str(https://github.com/rust-bitcoin/rust-bitcoin/pull/768/files#diff-37ff9af2290ccfd3f22d2a1e399973975fb6bd3fdae8203c223009403adb1c6eR95)?
Otherwise I feel like this private function and caller function(from_str) is a bit interleaving each other in responsibility.

kafaichoi · 2022-01-11T13:19:27Z

@bruteforcecat it already rejects unexpected prefixes and I consider it a requirement that all future code does. Otherwise it's a huge footgun.

I see. So basically we should only allow U as capital version of u(micro) as this is the one already supported in current code? Or should we just get ride of supporting U as well like @dr-orlovsky said?

Kixunil · 2022-01-11T13:24:08Z

Oh, yes, U was overlooked. Good catch! We should disable it and I think it's OK to do it in this PR. If others think it should be a different PR, it's also OK for me. :)

tcharding

Thanks for your ongoing work on this! I gave just a little style suggestion. Also perhaps we could have another test for 'U' and 'N'?

    #[test]
    fn disallow_unknown_denomination() {
        // Non-exhaustive list of unknown forms.
        let unknown = vec!["NBTC", "UBTC", "ABC", "abc"];
        for denom in unknown.iter() {
            match  Denomination::from_str(denom) {
                Ok(_) => panic!("from_str should error for {}", denom),
                Err(ParseAmountError::UnknownDenomination(_)) => {},
                Err(e) => panic!("unexpected error: {}", e),
            }
        }
    }

tcharding · 2022-01-11T22:09:56Z

src/util/amount.rs

+                    match d {
+                        D::MilliBitcoin | D::PicoBitcoin | D::MilliSatoshi =>
+                            Err(PossiblyConfusingDenomination(s.to_owned())),
+                        D::NanoBitcoin | D::MicroBitcoin =>
+                            Err(UnknownDenomination(s.to_owned())),


Its a little odd (to me at least) to see => without a {. I see there are other instances of that in our code base, I'm going to patch them :) Perhaps this is cleaner if written as:

denomination_from_str(s).map_or_else( || Err(UnknownDenomination(s.to_owned())), |d| { if s.starts_with(|ch : char| ch.is_uppercase() ) { match d { D::MilliBitcoin | D::PicoBitcoin | D::MilliSatoshi => { Err(PossiblyConfusingDenomination(s.to_owned())) }, D::NanoBitcoin | D::MicroBitcoin => { Err(UnknownDenomination(s.to_owned())) }, _ => Ok(d) } } else { Ok(d) } } )

hmm, I did the patch to remove all these codebase wide but it feels a bit too much like code churn. Feel free to ignore my suggestion :)

Thank you for the detail suggestion. I added the test case u mentioned above. But for this formatting, I actually ran cargo fmt and it returns this

match d { D::MilliBitcoin | D::PicoBitcoin | D::MilliSatoshi => { Err(PossiblyConfusingDenomination(s.to_owned())) } D::NanoBitcoin | D::MicroBitcoin => Err(UnknownDenomination(s.to_owned())), _ => Ok(d), }

So it looks like suggesting having not bracket when it can be fit to one line and vice versa.(btw is there any reason we are using cargo fmt?).

Yes, years of using rustfmt is why this jumped out at me. We don't use rustfmt in this project because the project predates rustfmt becoming popular/stable. Some folks want to introduce it but its not trivial to do so.

Kixunil · 2022-01-12T13:36:26Z

src/util/amount.rs

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        use self::ParseAmountError::*;
+        use self::Denomination as D;
+
+        denomination_from_str(s).map_or_else(


Could this be changed to match? I find map_or_else less readable.

Note that the whole thing could be much simpler with match:

let starts_with_uppercase = s.starts_with(|ch : char| ch.is_uppercase()); match denomination_from_str(s) { Some(D::MilliBitcoin) | Some(D::PicoBitcoin) | Some(D::MilliSatoshi) if starts_with_uppercase => { Err(PossiblyConfusingDenomination(s.to_owned())) }, Some(D::NanoBitcoin) | Some(D::MicroBitcoin) if starts_with_uppercase => { Err(UnknownDenomination(s.to_owned())) }, Some(denomination) => { Ok(denomination) }, None => { Err(UnknownDenomination(s.to_owned())) }, }

How about this?

match denomination_from_str(s) { None => Err(UnknownDenomination(s.to_owned())), Some(d @ D::MilliBitcoin) | Some(d @ D::PicoBitcoin) | Some(d @ D::MilliSatoshi) => { if s.starts_with(|ch : char| ch.is_uppercase() ) { Err(PossiblyConfusingDenomination(s.to_owned())) } else { Ok(d) } } Some(d @ D::NanoBitcoin) | Some(d @ D::MicroBitcoin) => { if s.starts_with(|ch : char| ch.is_uppercase() ) { Err(UnknownDenomination(s.to_owned())) } else { Ok(d) } } Some(d) => Ok(d), }

I don't like repeated if s.starts_with( but not sure if there is other solution other than nested match?

Flattened matches look better to me. Do you have some specific reason for your suggestion?
Using a variable should be fine to get rid of repeating.

ahh yours look very nice. I didn't know we can pattern match multiple enum variant with if guard(rust newbie).
I updated code using your suggestion except doing the starts_with check inline in if guard so we can save some unnecessary checking in other other cases(None, Some(d)).

Or do u think this minor optimization is not worth the code duplication?

FWIW I prefer @Kixunil's version using let starts_with_uppercase = s.starts_with(|ch : char| ch.is_uppercase()); :)

so we can save some unnecessary checking in other other cases

I was confident that the optimizer would figure it out but checked at godbolt.org and found it wouldn't.
Here's the fix without making the code ugly:

let starts_with_uppercase = || s.starts_with(|ch : char| ch.is_uppercase()); match denomination_from_str(s) { Some(D::MilliBitcoin) | Some(D::PicoBitcoin) | Some(D::MilliSatoshi) if starts_with_uppercase() => { Err(PossiblyConfusingDenomination(s.to_owned())) }, Some(D::NanoBitcoin) | Some(D::MicroBitcoin) if starts_with_uppercase() => { Err(UnknownDenomination(s.to_owned())) }, Some(denomination) => { Ok(denomination) }, None => { Err(UnknownDenomination(s.to_owned())) }, }

It's a bit uncommon style though. :)
Note that anyway calling that function are just two compares and jumps. I suspect that all the time wasted on them for all people running it is less than the amount of time we spent on this but hey, this is fun! :)

You are right that it's very trivial optimization. But since this one has the best of both world(it looks like the assembly code generated shorter than my previous clumsy one 😃), we will go for this option then?

Kixunil

ACK 0ce9152

There's a bit of room for improvement but not significantly important. Could be also improved in a followup PR.

Kixunil · 2022-01-13T13:08:35Z

src/util/amount.rs

+                    // This panic could be avoided by adding enum ConfusingDenomination { Mega, Peta } but is it worth it?
+                    _ => panic!("invalid error information"),
+                };
+                write!(f, "the '{}' at the beginning of {} should technically mean '{}' but that denomination is uncommon and maybe '{}' was intended", d.chars().next().unwrap(), d, upper, lower)


Damn, sorry for imperfect suggestion, this would've been nicer:

let (letter, upper, lower) = match d.chars().next() { Some('M') => ('M', "Mega", "milli"), Some('P') => ('P', "Peta", "pico"), // This panic could be avoided by adding enum ConfusingDenomination { Mega, Peta } but is it worth it? _ => panic!("invalid error information"), }; write!(f, "the '{}' at the beginning of {} should technically mean '{}' but that denomination is uncommon and maybe '{}' was intended", letter, d, upper, lower)

good idea. I updated it. sorry that u have to ack again.

Kixunil · 2022-01-13T13:09:57Z

src/util/amount.rs

+                Err(e) => panic!("unexpected error: {}", e),
+            }
+        }
+    }


Very good testing for specific error with clear panic messages!

Kixunil · 2022-01-13T13:14:50Z

(API break label because of adding variants to public enum; we need non_exhaustive really badly)

…amount denomiation. add disallow_unknown_denomination test

Kixunil

ACK 40f38b3

apoelstra

ACK 40f38b3

dr-orlovsky

NACK - seems like we have a bug in the code. Pls check my comment

dr-orlovsky · 2022-01-13T17:18:08Z

src/util/amount.rs

@@ -43,6 +47,8 @@ impl Denomination {
            Denomination::Bitcoin => -8,
            Denomination::MilliBitcoin => -5,
            Denomination::MicroBitcoin => -2,
+            Denomination::NanoBitcoin => 1,
+            Denomination::PicoBitcoin => 4,


I really sorry, but I do not get it. What is "picoBitcoin"? It should be 10^-12 and 3 orders of magnitude less than nanoBitcoin - i.e. less than satoshi by two orders of magnitude.

Suggested change

Denomination::PicoBitcoin => 4,

Denomination::PicoBitcoin => -2,

The PR is correct I believe, lets see if I can clarify. These numbers go the opposite way to intuition, see the comment

/// The number of decimal places more than a satoshi.

So BTC is 8 decimal places less than a satoshi (hence -8), PicoBitcoin is 4 decimal places more than a satoshi.

Does that help?

Yeah I was confused too when I reviewed it. My thinking was assuming MilliBitcoin and MicroBitcoin were correct, logically the number should be increased by three for each subsequent denomination: -2 + 3 == 1, 1 + 3 == 4.

right since the current precision is using satoshi as a base unit to do comparison but it's not the canonical smallest unit so we have both negative and positive number(that's the probably the part confuse people?)

I also found the function name precision not too clear to me. (In decimal, precision means total number of digit).

If we can treat Bitcoin as canonical largest unit(I doubt we need other unit larger than Bitcoin as total cap <21million BTC ?), maybe we can change this function to power (power of 10 relative to Bitcoin unit). and then it will be a decreasing negative number like(Here I think it's more natural to order by power)

impl Denomination { /// The power of 10 relative to Bitcoin fn power(self) -> i32 { match self { Denomination::Bitcoin => 0, Denomination::MilliBitcoin => -3, Denomination::MicroBitcoin => -6, Denomination::Bit => -6, Denomination::Satoshi => -8, Denomination::NanoBitcoin => -9, Denomination::MilliSatoshi => -11, Denomination::PicoBitcoin => -12, } }

Probably we may do this as a follow-up PR. I will merge the current PR since it already have reviews and solves at least one problem.

dr-orlovsky

Changing review to ACK 40f38b3 since it was my misunderstanding and not a bug

dr-orlovsky · 2022-01-14T09:23:52Z

src/util/amount.rs

-                || UnknownDenomination(s.to_owned()),
-                |_| PossiblyConfusingDenomination(s.to_owned())
-            ));
+        let starts_with_uppercase = || s.starts_with(|ch: char| ch.is_uppercase());


Post-merge nit for a follow-up PR: s.starts_with(char::is_uppercase);

Soon as edition 2018 lands we seriously have to get Clippy running on CI, this stuff is wasting precious reviewer resources :)

…tr::from_str for Deonomation 8fef869 repalce unncessary extra closure with function pointer in starts_with_uppercase closure inside Denomination from_str (KaFai Choi) Pull request description: Follow-up PR from #768 #768 (comment) ACKs for top commit: apoelstra: ACK 8fef869 dr-orlovsky: ACK 8fef869 Tree-SHA512: 3fd7d77805e047a692c53ca7677d7857baa00f529e15696790544a47cebe8a143c1c11f95401436d5322b6da24bc61cc9982a069c883b4815b6c50e8bd31553e

apoelstra previously approved these changes Jan 10, 2022

View reviewed changes

Kixunil requested changes Jan 10, 2022

View reviewed changes

tcharding reviewed Jan 11, 2022

View reviewed changes

kafaichoi dismissed apoelstra’s stale review via 248180d January 11, 2022 12:14

kafaichoi force-pushed the add-nano-pico-btc-to-denomination branch from 7645b26 to 248180d Compare January 11, 2022 12:14

add nano and pico BTC to Donomination enum

e80de8b

kafaichoi force-pushed the add-nano-pico-btc-to-denomination branch from 248180d to e80de8b Compare January 11, 2022 12:23

Kixunil requested changes Jan 11, 2022

View reviewed changes

kafaichoi changed the title ~~add nano and pico BTC to Donomination enum~~ WIP: add nano and pico BTC to Donomination enum Jan 11, 2022

kafaichoi changed the title ~~WIP: add nano and pico BTC to Donomination enum~~ add nano and pico BTC to Donomination enum Jan 11, 2022

kafaichoi marked this pull request as draft January 11, 2022 14:56

kafaichoi force-pushed the add-nano-pico-btc-to-denomination branch 2 times, most recently from 8b63b74 to c9b352c Compare January 11, 2022 15:25

kafaichoi marked this pull request as ready for review January 11, 2022 15:28

tcharding reviewed Jan 11, 2022

View reviewed changes

kafaichoi force-pushed the add-nano-pico-btc-to-denomination branch from c9b352c to f4602c1 Compare January 12, 2022 13:30

Kixunil reviewed Jan 12, 2022

View reviewed changes

kafaichoi force-pushed the add-nano-pico-btc-to-denomination branch 3 times, most recently from 464676e to 0ce9152 Compare January 13, 2022 12:56

Kixunil added the API break This PR requires a version bump for the next release label Jan 13, 2022

Kixunil previously approved these changes Jan 13, 2022

View reviewed changes

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Jan 13, 2022

Kixunil changed the title ~~add nano and pico BTC to Donomination enum~~ add nano and pico BTC to Denomination enum Jan 13, 2022

enforce strict SI(treat capital of m, u, n, p as invalid) in parsing …

40f38b3

…amount denomiation. add disallow_unknown_denomination test

kafaichoi dismissed Kixunil’s stale review via 40f38b3 January 13, 2022 13:27

kafaichoi force-pushed the add-nano-pico-btc-to-denomination branch from 0ce9152 to 40f38b3 Compare January 13, 2022 13:27

Kixunil approved these changes Jan 13, 2022

View reviewed changes

apoelstra approved these changes Jan 13, 2022

View reviewed changes

dr-orlovsky suggested changes Jan 13, 2022

View reviewed changes

dr-orlovsky approved these changes Jan 14, 2022

View reviewed changes

dr-orlovsky merged commit b165b8d into rust-bitcoin:master Jan 14, 2022

dr-orlovsky reviewed Jan 14, 2022

View reviewed changes

Kixunil mentioned this pull request Jan 14, 2022

Avoid closure in s.starts_with(); #783

Closed

kafaichoi mentioned this pull request Jan 14, 2022

replace unncessary closure with function pointer in FromStr::from_str for Deonomation #784

Merged

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

	Denomination::PicoBitcoin => 4,
	Denomination::PicoBitcoin => -2,

add nano and pico BTC to Denomination enum #768

add nano and pico BTC to Denomination enum #768

Conversation

kafaichoi commented Jan 10, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Jan 11, 2022

kafaichoi commented Jan 11, 2022

Kixunil commented Jan 11, 2022

dr-orlovsky commented Jan 11, 2022

kafaichoi commented Jan 11, 2022

Kixunil commented Jan 11, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kafaichoi commented Jan 11, 2022

Kixunil commented Jan 11, 2022

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Jan 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jan 13, 2022

Kixunil left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky Jan 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kafaichoi Jan 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Jan 12, 2022 •

edited

dr-orlovsky Jan 13, 2022 •

edited

kafaichoi Jan 14, 2022 •

edited