Merge pull request #1421 from radixdlt/refactor/decimal_safe_ops_part2

Refactor decimal overflow handling - part 2
radixdlt · Aug 31, 2023 · 804987a · 804987a
2 parents 84485d3 + d0c8835
commit 804987a
Show file tree

Hide file tree

Showing 14 changed files with 746 additions and 464 deletions.
diff --git a/radix-engine-common/src/math/decimal.rs b/radix-engine-common/src/math/decimal.rs
diff --git a/radix-engine-common/src/math/precise_decimal.rs b/radix-engine-common/src/math/precise_decimal.rs
diff --git a/radix-engine-common/src/math/rounding_mode.rs b/radix-engine-common/src/math/rounding_mode.rs
@@ -1,3 +1,4 @@
+use core::cmp::Ordering;
 use sbor::Sbor;
 
 /// Defines the rounding strategy.
@@ -21,3 +22,61 @@ pub enum RoundingMode {
     /// The number is rounded to the nearest, and when it is halfway between two others, it's rounded toward the nearest even number. Also known as "Bankers Rounding".
     ToNearestMidpointToEven,
 }
+
+/// The resolved rounding strategy internal to the round method
+pub(crate) enum ResolvedRoundingStrategy {
+    RoundUp,
+    RoundDown,
+    RoundToEven,
+}
+
+impl ResolvedRoundingStrategy {
+    pub fn from_mode(
+        mode: RoundingMode,
+        is_positive: bool,
+        compare_to_midpoint: impl FnOnce() -> Ordering,
+    ) -> Self {
+        match mode {
+            RoundingMode::ToPositiveInfinity => ResolvedRoundingStrategy::RoundUp,
+            RoundingMode::ToNegativeInfinity => ResolvedRoundingStrategy::RoundDown,
+            RoundingMode::ToZero => ResolvedRoundingStrategy::towards_zero(is_positive),
+            RoundingMode::AwayFromZero => ResolvedRoundingStrategy::away_from_zero(is_positive),
+            RoundingMode::ToNearestMidpointTowardZero => Self::from_midpoint_ordering(
+                compare_to_midpoint(),
+                ResolvedRoundingStrategy::towards_zero(is_positive),
+            ),
+            RoundingMode::ToNearestMidpointAwayFromZero => Self::from_midpoint_ordering(
+                compare_to_midpoint(),
+                ResolvedRoundingStrategy::away_from_zero(is_positive),
+            ),
+            RoundingMode::ToNearestMidpointToEven => Self::from_midpoint_ordering(
+                compare_to_midpoint(),
+                ResolvedRoundingStrategy::RoundToEven,
+            ),
+        }
+    }
+
+    fn from_midpoint_ordering(ordering: Ordering, equal_strategy: Self) -> Self {
+        match ordering {
+            Ordering::Less => Self::RoundDown,
+            Ordering::Equal => equal_strategy,
+            Ordering::Greater => Self::RoundUp,
+        }
+    }
+
+    fn towards_zero(is_positive: bool) -> Self {
+        if is_positive {
+            Self::RoundDown
+        } else {
+            Self::RoundUp
+        }
+    }
+
+    fn away_from_zero(is_positive: bool) -> Self {
+        if is_positive {
+            Self::RoundUp
+        } else {
+            Self::RoundDown
+        }
+    }
+}
diff --git a/radix-engine-interface/src/blueprints/resource/mod.rs b/radix-engine-interface/src/blueprints/resource/mod.rs
@@ -203,14 +203,22 @@ pub enum WithdrawStrategy {
 }
 
 pub trait ForWithdrawal {
-    fn for_withdrawal(&self, divisibility: u8, withdraw_strategy: WithdrawStrategy) -> Decimal;
+    fn for_withdrawal(
+        &self,
+        divisibility: u8,
+        withdraw_strategy: WithdrawStrategy,
+    ) -> Option<Decimal>;
 }
 
 impl ForWithdrawal for Decimal {
-    fn for_withdrawal(&self, divisibility: u8, withdraw_strategy: WithdrawStrategy) -> Decimal {
+    fn for_withdrawal(
+        &self,
+        divisibility: u8,
+        withdraw_strategy: WithdrawStrategy,
+    ) -> Option<Decimal> {
         match withdraw_strategy {
-            WithdrawStrategy::Exact => self.clone(),
-            WithdrawStrategy::Rounded(mode) => self.round(divisibility, mode),
+            WithdrawStrategy::Exact => Some(self.clone()),
+            WithdrawStrategy::Rounded(mode) => self.safe_round(divisibility, mode),
         }
     }
 }

diff --git a/radix-engine-tests/tests/consensus_manager.rs b/radix-engine-tests/tests/consensus_manager.rs
@@ -1104,7 +1104,7 @@ fn validator_receives_no_emission_when_too_many_proposals_missed() {
 
 macro_rules! assert_close_to {
     ($a:expr, $b:expr) => {
-        if Decimal::from($a.safe_sub($b).unwrap()).abs() > dec!("0.0001") {
+        if Decimal::from($a.safe_sub($b).unwrap()).safe_abs().unwrap() > dec!("0.0001") {
             panic!("{} is not close to {}", $a, $b);
         }
     };

diff --git a/radix-engine/src/blueprints/pool/multi_resource_pool/blueprint.rs b/radix-engine/src/blueprints/pool/multi_resource_pool/blueprint.rs
@@ -575,7 +575,9 @@ impl MultiResourcePoolBlueprint {
                     if divisibility == 18 {
                         amount_to_contribute
                     } else {
-                        amount_to_contribute.round(divisibility, RoundingMode::ToNegativeInfinity)
+                        amount_to_contribute
+                            .safe_round(divisibility, RoundingMode::ToNegativeInfinity)
+                            .ok_or(MultiResourcePoolError::DecimalOverflowError)?
                     }
                 };
 
@@ -852,7 +854,9 @@ impl MultiResourcePoolBlueprint {
                     let amount_owed = if divisibility == 18 {
                         amount_owed
                     } else {
-                        amount_owed.round(divisibility, RoundingMode::ToNegativeInfinity)
+                        amount_owed
+                            .safe_round(divisibility, RoundingMode::ToNegativeInfinity)
+                            .ok_or(MultiResourcePoolError::DecimalOverflowError)?
                     };
 
                     Ok((resource_address, amount_owed))

diff --git a/radix-engine/src/blueprints/pool/one_resource_pool/blueprint.rs b/radix-engine/src/blueprints/pool/one_resource_pool/blueprint.rs
@@ -588,7 +588,9 @@ impl OneResourcePoolBlueprint {
         let amount_owed = if pool_resource_divisibility == 18 {
             amount_owed
         } else {
-            amount_owed.round(pool_resource_divisibility, RoundingMode::ToNegativeInfinity)
+            amount_owed
+                .safe_round(pool_resource_divisibility, RoundingMode::ToNegativeInfinity)
+                .ok_or(OneResourcePoolError::DecimalOverflowError)?
         };
 
         Ok(amount_owed)

diff --git a/radix-engine/src/blueprints/pool/two_resource_pool/blueprint.rs b/radix-engine/src/blueprints/pool/two_resource_pool/blueprint.rs
@@ -450,7 +450,7 @@ impl TwoResourcePoolBlueprint {
                                 .sqrt()
                                 .and_then(|c2_sqrt| c1_sqrt.safe_mul(c2_sqrt))
                         })
-                        .map(|d| d.round(19, RoundingMode::ToPositiveInfinity))
+                        .and_then(|d| d.safe_round(19, RoundingMode::ToPositiveInfinity))
                         .and_then(|d| Decimal::try_from(d).ok())
                         .ok_or(TwoResourcePoolError::DecimalOverflowError)?,
                     contribution1,
@@ -475,7 +475,7 @@ impl TwoResourcePoolBlueprint {
                                 .and_then(|d| d.sqrt())
                                 .and_then(|sqrt_cr2| sqrt_cr1.safe_mul(sqrt_cr2))
                         })
-                        .map(|d| d.round(19, RoundingMode::ToPositiveInfinity))
+                        .and_then(|d| d.safe_round(19, RoundingMode::ToPositiveInfinity))
                         .and_then(|d| Decimal::try_from(d).ok())
                         .ok_or(TwoResourcePoolError::DecimalOverflowError)?,
                     contribution1,
@@ -514,12 +514,15 @@ impl TwoResourcePoolBlueprint {
                         v @ Some((c1, c2)) if c1 <= contribution1 && c2 <= contribution2 => v,
                         _ => None,
                     })
-                    .map(|(c1, c2)| {
-                        (
-                            c1.round(divisibility1, RoundingMode::ToNegativeInfinity),
-                            c2.round(divisibility2, RoundingMode::ToNegativeInfinity),
-                        )
+                    .map(|(c1, c2)| -> Result<(Decimal, Decimal), RuntimeError> {
+                        Ok((
+                            c1.safe_round(divisibility1, RoundingMode::ToNegativeInfinity)
+                                .ok_or(TwoResourcePoolError::DecimalOverflowError)?,
+                            c2.safe_round(divisibility2, RoundingMode::ToNegativeInfinity)
+                                .ok_or(TwoResourcePoolError::DecimalOverflowError)?,
+                        ))
                     })
+                    .filter_map(Result::ok)
                     .map(
                         |(c1, c2)| -> Result<(Decimal, Decimal, Decimal), RuntimeError> {
                             let pool_units_to_mint = c1
@@ -825,7 +828,9 @@ impl TwoResourcePoolBlueprint {
                     let amount_owed = if divisibility == 18 {
                         amount_owed
                     } else {
-                        amount_owed.round(divisibility, RoundingMode::ToNegativeInfinity)
+                        amount_owed
+                            .safe_round(divisibility, RoundingMode::ToNegativeInfinity)
+                            .ok_or(TwoResourcePoolError::DecimalOverflowError)?
                     };
 
                     Ok((resource_address, amount_owed))

diff --git a/radix-engine/src/blueprints/resource/fungible/fungible_bucket.rs b/radix-engine/src/blueprints/resource/fungible/fungible_bucket.rs
@@ -77,7 +77,9 @@ impl FungibleBucketBlueprint {
 
         // Apply withdraw strategy
         let divisibility = Self::get_divisibility(api)?;
-        let amount = amount.for_withdrawal(divisibility, withdraw_strategy);
+        let amount = amount
+            .for_withdrawal(divisibility, withdraw_strategy)
+            .ok_or(BucketError::DecimalOverflow)?;
 
         // Check amount
         if !(check_fungible_amount(&amount, divisibility)) {

diff --git a/radix-engine/src/blueprints/resource/fungible/fungible_resource_manager.rs b/radix-engine/src/blueprints/resource/fungible/fungible_resource_manager.rs
@@ -802,7 +802,13 @@ impl FungibleResourceManagerBlueprint {
             )?
             .into_latest();
 
-        Ok(amount.for_withdrawal(divisibility, withdraw_strategy))
+        Ok(amount
+            .for_withdrawal(divisibility, withdraw_strategy)
+            .ok_or(RuntimeError::ApplicationError(
+                ApplicationError::FungibleResourceManagerError(
+                    FungibleResourceManagerError::UnexpectedDecimalComputationError,
+                ),
+            ))?)
     }
 
     fn assert_mintable<Y>(api: &mut Y) -> Result<(), RuntimeError>

diff --git a/radix-engine/src/blueprints/resource/fungible/fungible_vault.rs b/radix-engine/src/blueprints/resource/fungible/fungible_vault.rs
@@ -346,7 +346,11 @@ impl FungibleVaultBlueprint {
 
         // Apply withdraw strategy
         let divisibility = Self::get_divisibility(api)?;
-        let amount = amount.for_withdrawal(divisibility, withdraw_strategy);
+        let amount = amount
+            .for_withdrawal(divisibility, withdraw_strategy)
+            .ok_or(RuntimeError::ApplicationError(
+                ApplicationError::VaultError(VaultError::DecimalOverflow),
+            ))?;
 
         // Check amount
         if !check_fungible_amount(&amount, divisibility) {

diff --git a/radix-engine/src/blueprints/resource/non_fungible/non_fungible_bucket.rs b/radix-engine/src/blueprints/resource/non_fungible/non_fungible_bucket.rs
@@ -55,7 +55,9 @@ impl NonFungibleBucketBlueprint {
         }
 
         // Apply withdraw strategy
-        let amount = amount.for_withdrawal(0, withdraw_strategy);
+        let amount = amount
+            .for_withdrawal(0, withdraw_strategy)
+            .ok_or(BucketError::DecimalOverflow)?;
 
         // Check amount
         let n = check_non_fungible_amount(&amount).map_err(|_| {

diff --git a/radix-engine/src/blueprints/resource/non_fungible/non_fungible_resource_manager.rs b/radix-engine/src/blueprints/resource/non_fungible/non_fungible_resource_manager.rs
@@ -1504,7 +1504,13 @@ impl NonFungibleResourceManagerBlueprint {
     where
         Y: ClientApi<RuntimeError>,
     {
-        Ok(amount.for_withdrawal(0, withdraw_strategy))
+        Ok(amount
+            .for_withdrawal(0, withdraw_strategy)
+            .ok_or(RuntimeError::ApplicationError(
+                ApplicationError::NonFungibleResourceManagerError(
+                    NonFungibleResourceManagerError::UnexpectedDecimalComputationError,
+                ),
+            ))?)
     }
 }
 

diff --git a/radix-engine/src/blueprints/resource/non_fungible/non_fungible_vault.rs b/radix-engine/src/blueprints/resource/non_fungible/non_fungible_vault.rs
@@ -16,6 +16,7 @@ use radix_engine_interface::types::*;
 pub enum NonFungibleVaultError {
     MissingId(NonFungibleLocalId),
     NotEnoughAmount,
+    DecimalOverflow,
 }
 
 declare_native_blueprint_state! {
@@ -408,7 +409,12 @@ impl NonFungibleVaultBlueprint {
             }
         }
 
-        let amount = amount.for_withdrawal(0, withdraw_strategy);
+        let amount =
+            amount
+                .for_withdrawal(0, withdraw_strategy)
+                .ok_or(RuntimeError::ApplicationError(
+                    ApplicationError::NonFungibleVaultError(NonFungibleVaultError::DecimalOverflow),
+                ))?;
 
         // Check amount
         let n = check_non_fungible_amount(&amount).map_err(|_| {