281 inaccuracy in approve chain extension behavior (#283)

* correct allowance behavior * use BalanceOf max value() instead of u128::MAX
pendulum-chain · Jul 20, 2023 · 43f0990 · 43f0990
1 parent 5b922a2
commit 43f0990
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 46 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/pallets/orml-currencies-allowance-extension/src/default_weights.rs b/pallets/orml-currencies-allowance-extension/src/default_weights.rs
@@ -2,7 +2,7 @@
 //! Autogenerated weights for `orml_currencies_allowance_extension`
 //!
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2023-07-18, STEPS: `100`, REPEAT: `10`, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! DATE: 2023-07-19, STEPS: `100`, REPEAT: `10`, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! WORST CASE MAP SIZE: `1000000`
 //! HOSTNAME: `Hugos-MacBook-Pro.local`, CPU: `<UNKNOWN>`
 //! EXECUTION: None, WASM-EXECUTION: Compiled, CHAIN: Some("foucoco"), DB CACHE: 1024
@@ -35,7 +35,7 @@ impl<T: frame_system::Config> crate::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `0`
-		// Minimum execution time: 5_000_000 picoseconds.
+		// Minimum execution time: 6_000_000 picoseconds.
 		Weight::from_parts(6_000_000, 0)
 			.saturating_add(Weight::from_parts(0, 0))
 			.saturating_add(T::DbWeight::get().writes(1))
@@ -46,23 +46,23 @@ impl<T: frame_system::Config> crate::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `0`
 		//  Estimated: `0`
-		// Minimum execution time: 5_000_000 picoseconds.
+		// Minimum execution time: 6_000_000 picoseconds.
 		Weight::from_parts(6_000_000, 0)
 			.saturating_add(Weight::from_parts(0, 0))
 			.saturating_add(T::DbWeight::get().writes(1))
 	}
 	/// Storage: TokenAllowance AllowedCurrencies (r:1 w:0)
 	/// Proof Skipped: TokenAllowance AllowedCurrencies (max_values: None, max_size: None, mode: Measured)
-	/// Storage: TokenAllowance Approvals (r:1 w:1)
+	/// Storage: TokenAllowance Approvals (r:0 w:1)
 	/// Proof Skipped: TokenAllowance Approvals (max_values: None, max_size: None, mode: Measured)
 	fn approve() -> Weight {
 		// Proof Size summary in bytes:
 		//  Measured:  `184`
-		//  Estimated: `7298`
+		//  Estimated: `3833`
 		// Minimum execution time: 11_000_000 picoseconds.
 		Weight::from_parts(12_000_000, 0)
-			.saturating_add(Weight::from_parts(0, 7298))
-			.saturating_add(T::DbWeight::get().reads(2))
+			.saturating_add(Weight::from_parts(0, 3833))
+			.saturating_add(T::DbWeight::get().reads(1))
 			.saturating_add(T::DbWeight::get().writes(1))
 	}
 	/// Storage: TokenAllowance AllowedCurrencies (r:1 w:0)
@@ -75,8 +75,8 @@ impl<T: frame_system::Config> crate::WeightInfo for WeightInfo<T> {
 		// Proof Size summary in bytes:
 		//  Measured:  `561`
 		//  Estimated: `14248`
-		// Minimum execution time: 29_000_000 picoseconds.
-		Weight::from_parts(30_000_000, 0)
+		// Minimum execution time: 31_000_000 picoseconds.
+		Weight::from_parts(33_000_000, 0)
 			.saturating_add(Weight::from_parts(0, 14248))
 			.saturating_add(T::DbWeight::get().reads(4))
 			.saturating_add(T::DbWeight::get().writes(3))

diff --git a/pallets/orml-currencies-allowance-extension/src/lib.rs b/pallets/orml-currencies-allowance-extension/src/lib.rs
@@ -10,7 +10,7 @@ use frame_support::{dispatch::DispatchResult, ensure, weights::Weight};
 #[cfg(test)]
 use mocktopus::macros::mockable;
 use orml_traits::MultiCurrency;
-use sp_runtime::{traits::*, ArithmeticError};
+use sp_runtime::traits::*;
 use sp_std::{convert::TryInto, prelude::*, vec};
 
 #[cfg(feature = "runtime-benchmarks")]
@@ -245,18 +245,7 @@ impl<T: Config> Pallet<T> {
 		amount: BalanceOf<T>,
 	) -> DispatchResult {
 		ensure!(Self::is_allowed_currency(id), Error::<T>::CurrencyNotLive);
-		Approvals::<T>::try_mutate((id, &owner, &delegate), |maybe_approved| -> DispatchResult {
-			let mut approved = match maybe_approved.take() {
-				// an approval already exists and is being updated
-				Some(a) => a,
-				// a new approval is created
-				None => Default::default(),
-			};
-
-			approved = approved.checked_add(&amount).ok_or(ArithmeticError::Overflow)?;
-			*maybe_approved = Some(approved);
-			Ok(())
-		})?;
+		Approvals::<T>::set((id, &owner, &delegate), Some(amount));
 		Self::deposit_event(Event::TransferApproved {
 			currency_id: id,
 			source: owner.clone(),
@@ -298,7 +287,10 @@ impl<T: Config> Pallet<T> {
 				if remaining.is_zero() {
 					*maybe_approved = None;
 				} else {
-					approved = remaining;
+					//decrement allowance only if it isn't max value (which acts as infinite allowance)
+					if approved != BalanceOf::<T>::max_value() {
+						approved = remaining;
+					}
 					*maybe_approved = Some(approved);
 				}
 				Ok(())