Merge pull request #1890 from CosmWasm/1485-remove-mixed-arithmetic

[2.0] Remove mixed uint decimal arithmetic
CosmWasm · Oct 17, 2023 · 18696df · 18696df
2 parents b515e5c + c55e126
commit 18696df
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 140 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,8 +12,12 @@ and this project adheres to
   constructor, remove `SubMsgExecutionResponse` (Use `SubMsgResponse` instead)
   and remove `PartialEq<&str> for Addr` (validate the address and use
   `PartialEq<Addr> for Addr` instead). ([#1879])
+- cosmwasm-std: Remove `Mul<Decimal> for Uint128` and
+  `Mul<Decimal256> for Uint256`. Use `Uint{128,256}::mul_floor` instead.
+  ([#1890])
 
 [#1879]: https://github.com/CosmWasm/cosmwasm/pull/1879
+[#1890]: https://github.com/CosmWasm/cosmwasm/pull/1890
 
 ## [1.5.0-rc.0]
 

diff --git a/MIGRATING.md b/MIGRATING.md
@@ -54,6 +54,14 @@ major releases of `cosmwasm`. Note that you can also view the
 
   But keep in mind that this is case sensitive (while addresses are not).
 
+- Replace all uses of `Mul<Decimal> for Uint128` and
+  `Mul<Decimal256> for Uint256` with `Uint{128,256}::mul_floor`:
+
+  ```diff
+  -Uint128::new(123456) * Decimal::percent(1);
+  +Uint128::new(123456).mul_floor(Decimal::percent(1));
+  ```
+
 ## 1.4.x -> 1.5.0
 
 - Update `cosmwasm-*` dependencies in Cargo.toml (skip the ones you don't use):

diff --git a/contracts/staking/src/contract.rs b/contracts/staking/src/contract.rs
@@ -154,7 +154,7 @@ pub fn bond(deps: DepsMut, env: Env, info: MessageInfo) -> StdResult<Response> {
     // TODO: this is just temporary check - we should use dynamic query or have a way to recover
     assert_bonds(&supply, bonded)?;
     let to_mint = if supply.issued.is_zero() || bonded.is_zero() {
-        FALLBACK_RATIO * payment.amount
+        payment.amount.mul_floor(FALLBACK_RATIO)
     } else {
         payment.amount.multiply_ratio(supply.issued, bonded)
     };
@@ -193,7 +193,7 @@ pub fn unbond(deps: DepsMut, env: Env, info: MessageInfo, amount: Uint128) -> St
     let owner_raw = deps.api.addr_canonicalize(invest.owner.as_str())?;
 
     // calculate tax and remainer to unbond
-    let tax = amount * invest.exit_tax;
+    let tax = amount.mul_floor(invest.exit_tax);
 
     // deduct all from the account
     let balance = may_load_map(deps.storage, PREFIX_BALANCE, &sender_raw)?.unwrap_or_default();

diff --git a/packages/std/src/math/decimal.rs b/packages/std/src/math/decimal.rs
@@ -671,30 +671,6 @@ impl MulAssign for Decimal {
 }
 forward_ref_op_assign!(impl MulAssign, mul_assign for Decimal, Decimal);
 
-/// Both d*u and u*d with d: Decimal and u: Uint128 returns an Uint128. There is no
-/// specific reason for this decision other than the initial use cases we have. If you
-/// need a Decimal result for the same calculation, use Decimal(d*u) or Decimal(u*d).
-impl Mul<Decimal> for Uint128 {
-    type Output = Self;
-
-    #[allow(clippy::suspicious_arithmetic_impl)]
-    fn mul(self, rhs: Decimal) -> Self::Output {
-        // 0*a and b*0 is always 0
-        if self.is_zero() || rhs.is_zero() {
-            return Uint128::zero();
-        }
-        self.multiply_ratio(rhs.0, Decimal::DECIMAL_FRACTIONAL)
-    }
-}
-
-impl Mul<Uint128> for Decimal {
-    type Output = Uint128;
-
-    fn mul(self, rhs: Uint128) -> Self::Output {
-        rhs * self
-    }
-}
-
 impl Div for Decimal {
     type Output = Self;
 
@@ -1544,44 +1520,6 @@ mod tests {
         );
     }
 
-    #[test]
-    // in this test the Decimal is on the right
-    fn uint128_decimal_multiply() {
-        // a*b
-        let left = Uint128::new(300);
-        let right = Decimal::one() + Decimal::percent(50); // 1.5
-        assert_eq!(left * right, Uint128::new(450));
-
-        // a*0
-        let left = Uint128::new(300);
-        let right = Decimal::zero();
-        assert_eq!(left * right, Uint128::new(0));
-
-        // 0*a
-        let left = Uint128::new(0);
-        let right = Decimal::one() + Decimal::percent(50); // 1.5
-        assert_eq!(left * right, Uint128::new(0));
-    }
-
-    #[test]
-    // in this test the Decimal is on the left
-    fn decimal_uint128_multiply() {
-        // a*b
-        let left = Decimal::one() + Decimal::percent(50); // 1.5
-        let right = Uint128::new(300);
-        assert_eq!(left * right, Uint128::new(450));
-
-        // 0*a
-        let left = Decimal::zero();
-        let right = Uint128::new(300);
-        assert_eq!(left * right, Uint128::new(0));
-
-        // a*0
-        let left = Decimal::one() + Decimal::percent(50); // 1.5
-        let right = Uint128::new(0);
-        assert_eq!(left * right, Uint128::new(0));
-    }
-
     #[test]
     #[allow(clippy::op_ref)]
     fn decimal_implements_div() {
@@ -2211,14 +2149,14 @@ mod tests {
         // Does the same as the old workaround `Uint128::one() * my_decimal`.
         // This block can be deleted as part of https://github.com/CosmWasm/cosmwasm/issues/1485.
         let tests = vec![
-            Decimal::from_str("12.345").unwrap(),
-            Decimal::from_str("0.98451384").unwrap(),
-            Decimal::from_str("178.0").unwrap(),
-            Decimal::MIN,
-            Decimal::MAX,
+            (Decimal::from_str("12.345").unwrap(), 12u128),
+            (Decimal::from_str("0.98451384").unwrap(), 0u128),
+            (Decimal::from_str("178.0").unwrap(), 178u128),
+            (Decimal::MIN, 0u128),
+            (Decimal::MAX, u128::MAX / Decimal::DECIMAL_FRACTIONAL.u128()),
         ];
-        for my_decimal in tests.into_iter() {
-            assert_eq!(my_decimal.to_uint_floor(), Uint128::one() * my_decimal);
+        for (my_decimal, expected) in tests.into_iter() {
+            assert_eq!(my_decimal.to_uint_floor(), Uint128::new(expected));
         }
     }
 

diff --git a/packages/std/src/math/decimal256.rs b/packages/std/src/math/decimal256.rs
@@ -680,30 +680,6 @@ impl MulAssign for Decimal256 {
 }
 forward_ref_op_assign!(impl MulAssign, mul_assign for Decimal256, Decimal256);
 
-/// Both d*u and u*d with d: Decimal256 and u: Uint256 returns an Uint256. There is no
-/// specific reason for this decision other than the initial use cases we have. If you
-/// need a Decimal256 result for the same calculation, use Decimal256(d*u) or Decimal256(u*d).
-impl Mul<Decimal256> for Uint256 {
-    type Output = Self;
-
-    #[allow(clippy::suspicious_arithmetic_impl)]
-    fn mul(self, rhs: Decimal256) -> Self::Output {
-        // 0*a and b*0 is always 0
-        if self.is_zero() || rhs.is_zero() {
-            return Uint256::zero();
-        }
-        self.multiply_ratio(rhs.0, Decimal256::DECIMAL_FRACTIONAL)
-    }
-}
-
-impl Mul<Uint256> for Decimal256 {
-    type Output = Uint256;
-
-    fn mul(self, rhs: Uint256) -> Self::Output {
-        rhs * self
-    }
-}
-
 impl Div for Decimal256 {
     type Output = Self;
 
@@ -1606,44 +1582,6 @@ mod tests {
         );
     }
 
-    #[test]
-    // in this test the Decimal256 is on the right
-    fn uint128_decimal_multiply() {
-        // a*b
-        let left = Uint256::from(300u128);
-        let right = Decimal256::one() + Decimal256::percent(50); // 1.5
-        assert_eq!(left * right, Uint256::from(450u32));
-
-        // a*0
-        let left = Uint256::from(300u128);
-        let right = Decimal256::zero();
-        assert_eq!(left * right, Uint256::from(0u128));
-
-        // 0*a
-        let left = Uint256::from(0u128);
-        let right = Decimal256::one() + Decimal256::percent(50); // 1.5
-        assert_eq!(left * right, Uint256::from(0u128));
-    }
-
-    #[test]
-    // in this test the Decimal256 is on the left
-    fn decimal256_uint128_multiply() {
-        // a*b
-        let left = Decimal256::one() + Decimal256::percent(50); // 1.5
-        let right = Uint256::from(300u128);
-        assert_eq!(left * right, Uint256::from(450u128));
-
-        // 0*a
-        let left = Decimal256::zero();
-        let right = Uint256::from(300u128);
-        assert_eq!(left * right, Uint256::from(0u128));
-
-        // a*0
-        let left = Decimal256::one() + Decimal256::percent(50); // 1.5
-        let right = Uint256::from(0u128);
-        assert_eq!(left * right, Uint256::from(0u128));
-    }
-
     #[test]
     #[allow(clippy::op_ref)]
     fn decimal256_implements_div() {
@@ -2311,14 +2249,26 @@ mod tests {
         // Does the same as the old workaround `Uint256::one() * my_decimal`.
         // This block can be deleted as part of https://github.com/CosmWasm/cosmwasm/issues/1485.
         let tests = vec![
-            Decimal256::from_str("12.345").unwrap(),
-            Decimal256::from_str("0.98451384").unwrap(),
-            Decimal256::from_str("178.0").unwrap(),
-            Decimal256::MIN,
-            Decimal256::MAX,
+            (
+                Decimal256::from_str("12.345").unwrap(),
+                Uint256::from(12u128),
+            ),
+            (
+                Decimal256::from_str("0.98451384").unwrap(),
+                Uint256::from(0u128),
+            ),
+            (
+                Decimal256::from_str("178.0").unwrap(),
+                Uint256::from(178u128),
+            ),
+            (Decimal256::MIN, Uint256::from(0u128)),
+            (
+                Decimal256::MAX,
+                Uint256::MAX / Decimal256::DECIMAL_FRACTIONAL,
+            ),
         ];
-        for my_decimal in tests.into_iter() {
-            assert_eq!(my_decimal.to_uint_floor(), Uint256::one() * my_decimal);
+        for (my_decimal, expected) in tests.into_iter() {
+            assert_eq!(my_decimal.to_uint_floor(), expected);
         }
     }