Repository: rust-bitcoin/rust-bitcoin
Pull request #3719 (2024-12-10 15:42:45)

Change `SignedAmount` `MAX` and `MIN` to equal +/- `MAX_MONEY`
To prevent rounding errors converting to and from f64 change `SignedAmount` `MAX` and `MIN` to +/- `MAX_MONEY` which are within the limit in f64 that has issues.

Add checks to `from_str_in`, `checked_add`,  `checked_sub` and `checked_mul` that the result is within MIN and MAX.

Modify tests to work with new `MIN` and `MAX`.

Discussed in #3688 and #3691

yancyribbens: This PR conflicts with https://github.com/rust-bitcoin/rust-bitcoin/pull/3703 and https://github.com/rust-bitcoin/rust-bitcoin/pull/3697 which I've been waiting to get feedback on.  It would be appreciated if you don't agree with those PRs to comment on them with your suggestions.  I do agree that `MIN` should probably be redefined at some point as you have done in this PR.
jamillambert: > This PR conflicts with #3703 and #3697 which I've been waiting to get feedback on. It would be appreciated if you don't agree with those PRs to comment on them with your suggestions. I do agree that `MIN` should probably be redefined at some point as you have done in this PR.

I have rebased to remove the conflict with #3697.  #3703 deals with the issue #3696 which we decided to deal with separately to changing the `MAX` value to the lower `MAX_MONEY`.  This PR is just applying what was done in #3693 to `SignedAmount`.  
yancyribbens: >  This PR is just applying what was done in https://github.com/rust-bitcoin/rust-bitcoin/pull/3693 to SignedAmount.

What issue is this addressing then if not issue 3696?  The reason I point out the conflict with 3703 is that I don't think we need both `from_sats_checked` as introduced in 3703 and `check_min_max` here.




Patch:
diff --git a/units/src/amount/signed.rs b/units/src/amount/signed.rs
index 4237ba0b..05468579 100644
--- a/units/src/amount/signed.rs
+++ b/units/src/amount/signed.rs
@@ -58,9 +58,9 @@ impl SignedAmount {
     /// The maximum value allowed as an amount. Useful for sanity checking.
     pub const MAX_MONEY: SignedAmount = SignedAmount(21_000_000 * 100_000_000);
     /// The minimum value of an amount.
-    pub const MIN: SignedAmount = SignedAmount(i64::MIN);
+    pub const MIN: SignedAmount = SignedAmount(-21_000_000 * 100_000_000);
     /// The maximum value of an amount.
-    pub const MAX: SignedAmount = SignedAmount(i64::MAX);
+    pub const MAX: SignedAmount = SignedAmount::MAX_MONEY;
 
     /// Constructs a new [`SignedAmount`] with satoshi precision and the given number of satoshis.
     pub const fn from_sat(satoshi: i64) -> SignedAmount { SignedAmount(satoshi) }
@@ -112,8 +112,7 @@ pub fn from_str_in(s: &str, denom: Denomination) -> Result<SignedAmount, ParseAm
                 ParseAmountErrorInner::OutOfRange(OutOfRangeError::too_big(true)),
             )),
             (false, sat) => Ok(SignedAmount(sat as i64)),
-            (true, sat) if sat == i64::MIN.unsigned_abs() => Ok(SignedAmount(i64::MIN)),
-            (true, sat) if sat > i64::MIN.unsigned_abs() => Err(ParseAmountError(
+            (true, sat) if sat > SignedAmount::MIN.to_sat().unsigned_abs() => Err(ParseAmountError(
                 ParseAmountErrorInner::OutOfRange(OutOfRangeError::too_small()),
             )),
             (true, sat) => Ok(SignedAmount(-(sat as i64))),
@@ -249,7 +248,7 @@ pub const fn checked_abs(self) -> Option<SignedAmount> {
     pub const fn checked_add(self, rhs: SignedAmount) -> Option<SignedAmount> {
         // No `map()` in const context.
         match self.0.checked_add(rhs.0) {
-            Some(res) => Some(SignedAmount(res)),
+            Some(res) => SignedAmount(res).check_min_max(),
             None => None,
         }
     }
@@ -260,7 +259,7 @@ pub const fn checked_add(self, rhs: SignedAmount) -> Option<SignedAmount> {
     pub const fn checked_sub(self, rhs: SignedAmount) -> Option<SignedAmount> {
         // No `map()` in const context.
         match self.0.checked_sub(rhs.0) {
-            Some(res) => Some(SignedAmount(res)),
+            Some(res) => SignedAmount(res).check_min_max(),
             None => None,
         }
     }
@@ -271,7 +270,7 @@ pub const fn checked_sub(self, rhs: SignedAmount) -> Option<SignedAmount> {
     pub const fn checked_mul(self, rhs: i64) -> Option<SignedAmount> {
         // No `map()` in const context.
         match self.0.checked_mul(rhs) {
-            Some(res) => Some(SignedAmount(res)),
+            Some(res) => SignedAmount(res).check_min_max(),
             None => None,
         }
     }
@@ -337,6 +336,15 @@ pub fn to_unsigned(self) -> Result<Amount, OutOfRangeError> {
             Ok(Amount::from_sat(self.to_sat() as u64))
         }
     }
+
+    /// Checks the amount is within the allowed range.
+    const fn check_min_max(self) -> Option<SignedAmount> {
+        if self.0 < Self::MIN.0 || self.0 > Self::MAX.0 {
+            None
+        } else {
+            Some(self)
+        }
+    }
 }
 
 impl default::Default for SignedAmount {


Test patch:
diff --git a/units/src/amount/tests.rs b/units/src/amount/tests.rs
index 0d6c27a5..b2a0dfac 100644
--- a/units/src/amount/tests.rs
+++ b/units/src/amount/tests.rs
@@ -283,7 +283,7 @@ fn parsing() {
     assert_eq!(p("1", btc), Ok(Amount::from_sat(1_000_000_00)));
     assert_eq!(sp("-.5", btc), Ok(SignedAmount::from_sat(-500_000_00)));
     #[cfg(feature = "alloc")]
-    assert_eq!(sp(&i64::MIN.to_string(), sat), Ok(SignedAmount::from_sat(i64::MIN)));
+    assert_eq!(sp(&SignedAmount::MIN.to_sat().to_string(), sat), Ok(SignedAmount::MIN));
     assert_eq!(p("1.1", btc), Ok(Amount::from_sat(1_100_000_00)));
     assert_eq!(p("100", sat), Ok(Amount::from_sat(100)));
     assert_eq!(p("55", sat), Ok(Amount::from_sat(55)));
@@ -571,7 +571,11 @@ fn scase(s: &str, expected: Result<SignedAmount, impl Into<ParseError>>) {
     scase("-0.1 satoshi", Err(TooPreciseError { position: 3 }));
     case("0.123456 mBTC", Err(TooPreciseError { position: 7 }));
     scase("-1.001 bits", Err(TooPreciseError { position: 5 }));
-    scase("-200000000000 BTC", Err(OutOfRangeError::too_small()));
+    scase("-21000001 BTC", Err(OutOfRangeError::too_small()));
+    scase("21000001 BTC", Err(OutOfRangeError::too_big(true)));
+    scase("-2100000000000001 SAT", Err(OutOfRangeError::too_small()));
+    scase("2100000000000001 SAT", Err(OutOfRangeError::too_big(true)));
+    case("21000001 BTC", Err(OutOfRangeError::too_big(false)));
     case("18446744073709551616 sat", Err(OutOfRangeError::too_big(false)));
 
     ok_case(".5 bits", Amount::from_sat(50));
@@ -580,8 +584,9 @@ fn scase(s: &str, expected: Result<SignedAmount, impl Into<ParseError>>) {
     ok_scase("-5 satoshi", SignedAmount::from_sat(-5));
     ok_case("0.10000000 BTC", Amount::from_sat(100_000_00));
     ok_scase("-100 bits", SignedAmount::from_sat(-10_000));
-    #[cfg(feature = "alloc")]
-    ok_scase(&format!("{} SAT", i64::MIN), SignedAmount::from_sat(i64::MIN));
+    ok_case("21000000 BTC", Amount::MAX);
+    ok_scase("21000000 BTC", SignedAmount::MAX);
+    ok_scase("-21000000 BTC", SignedAmount::MIN);
 }
 
 #[cfg(feature = "alloc")]