Merge rust-bitcoin/rust-bitcoin#731: Improve parsing of `Denomination` string

f690b8e362 Be more liberal when parsing Denomination (Tobin Harding)
628168e493 Add missing white space character (Tobin Harding)

Pull request description:

  There is no reason to force users to use a particular format or case for `Denomination` strings. Users may wish to write any of the following and all seem reasonable
  - 100 sats
  - 100 sat
  - 100 SAT

  The same goes for various other `Denomination`s.

  - Patch 1 enables usage of "sats", "sat", "bit", "bits"
  - Patch 2 enables usage of various lower/uper case formatting

  Fixes: #729

ACKs for top commit:
  Kixunil:
    ACK f690b8e362
  apoelstra:
    ACK f690b8e362

Tree-SHA512: a785608e19a7ba6f689dc022cb17a709041ff56abeaa74649d0832a8bd8aac4593c7a79b46a47dd417796c588d669f50fb3c8b8a984be332ca38a1fef2dcd4ce

src/util/amount.rs

@@ -66,20 +66,61 @@ impl fmt::Display for Denomination {
 impl FromStr for Denomination {
     type Err = ParseAmountError;
 
+    /// Convert from a str to Denomination.
+    ///
+    /// Any combination of upper and/or lower case, excluding uppercase 'M' is considered valid.
+    /// - Singular: BTC, mBTC, uBTC
+    /// - 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> {
-        match s {
+        use self::ParseAmountError::*;
-            "BTC" => Ok(Denomination::Bitcoin),
+
-            "mBTC" => Ok(Denomination::MilliBitcoin),
+        if s.starts_with('M') {
-            "uBTC" => Ok(Denomination::MicroBitcoin),
+            return Err(denomination_from_str(s).map_or_else(
-            "bits" => Ok(Denomination::Bit),
+                || UnknownDenomination(s.to_owned()),
-            "satoshi" => Ok(Denomination::Satoshi),
+                |_| PossiblyConfusingDenomination(s.to_owned())
-            "sat" => Ok(Denomination::Satoshi),
+            ));
-            "msat" => Ok(Denomination::MilliSatoshi),
-            d => Err(ParseAmountError::UnknownDenomination(d.to_owned())),
         }
+
+        denomination_from_str(s).ok_or_else(|| UnknownDenomination(s.to_owned()))
     }
 }
 
+fn denomination_from_str(mut s: &str) -> Option<Denomination> {
+    if s.eq_ignore_ascii_case("BTC") {
+        return Some(Denomination::Bitcoin);
+    }
+
+    if s.eq_ignore_ascii_case("mBTC") {
+        return Some(Denomination::MilliBitcoin);
+    }
+
+    if s.eq_ignore_ascii_case("uBTC") {
+        return Some(Denomination::MicroBitcoin);
+    }
+
+    if s.ends_with('s') || s.ends_with('S') {
+        s = &s[..(s.len() - 1)];
+    }
+
+    if s.eq_ignore_ascii_case("bit") {
+        return Some(Denomination::Bit);
+    }
+    if s.eq_ignore_ascii_case("satoshi") {
+        return Some(Denomination::Satoshi);
+    }
+    if s.eq_ignore_ascii_case("sat") {
+        return Some(Denomination::Satoshi);
+    }
+
+    if s.eq_ignore_ascii_case("msat") {
+        return Some(Denomination::MilliSatoshi);
+    }
+
+    None
+}
+
 /// An error during amount parsing.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum ParseAmountError {
@@ -97,6 +138,8 @@ pub enum ParseAmountError {
     InvalidCharacter(char),
     /// The denomination was unknown.
     UnknownDenomination(String),
+    /// The denomination has multiple possible interpretations.
+    PossiblyConfusingDenomination(String)
 }
 
 impl fmt::Display for ParseAmountError {
@@ -108,7 +151,10 @@ impl fmt::Display for ParseAmountError {
             ParseAmountError::InvalidFormat => f.write_str("invalid number format"),
             ParseAmountError::InputTooLarge => f.write_str("input string was too large"),
             ParseAmountError::InvalidCharacter(c) => write!(f, "invalid character in input: {}", c),
-            ParseAmountError::UnknownDenomination(ref d) => write!(f, "unknown denomination: {}",d),
+            ParseAmountError::UnknownDenomination(ref d) => write!(f, "unknown denomination: {}", d),
+            ParseAmountError::PossiblyConfusingDenomination(ref d) => {
+                write!(f, "the 'M' at the beginning of {} should technically mean 'Mega' but that denomination is uncommon and maybe 'milli' was intended", d)
+            }
         }
     }
 }
@@ -1643,4 +1689,27 @@ mod tests {
         let sum = amounts.into_iter().checked_sum();
         assert_eq!(Some(SignedAmount::from_sat(3364)), sum);
     }
+
+    #[test]
+    fn denomination_string_acceptable_forms() {
+        // Non-exhaustive list of valid forms.
+        let valid = vec!["BTC", "btc", "mBTC", "mbtc", "uBTC", "ubtc", "SATOSHI","Satoshi", "Satoshis", "satoshis", "SAT", "Sat", "sats", "bit", "bits"];
+        for denom in valid.iter() {
+            assert!(Denomination::from_str(denom).is_ok());
+        }
+    }
+
+    #[test]
+    fn disallow_confusing_forms() {
+        // Non-exhaustive list of confusing forms.
+        let confusing = vec!["Msat", "Msats", "MSAT", "MSATS", "MSat", "MSats", "MBTC", "Mbtc"];
+        for denom in confusing.iter() {
+            match  Denomination::from_str(denom) {
+                Ok(_) => panic!("from_str should error for {}", denom),
+                Err(ParseAmountError::PossiblyConfusingDenomination(_)) => {},
+                Err(e) => panic!("unexpected error: {}", e),
+            }
+        }
+    }
 }
+