diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 683ebce2b1693..d10a5e8064155 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -642,7 +642,7 @@ impl BitOr for Reasons { type Output = Reasons; fn bitor(self, other: Reasons) -> Reasons { if self == other { - return self + return self; } Reasons::All } @@ -801,11 +801,11 @@ impl, I: 'static> Pallet { mint: bool, ) -> DepositConsequence { if amount.is_zero() { - return DepositConsequence::Success + return DepositConsequence::Success; } if mint && TotalIssuance::::get().checked_add(&amount).is_none() { - return DepositConsequence::Overflow + return DepositConsequence::Overflow; } let new_total_balance = match account.total().checked_add(&amount) { @@ -814,7 +814,7 @@ impl, I: 'static> Pallet { }; if new_total_balance < T::ExistentialDeposit::get() { - return DepositConsequence::BelowMinimum + return DepositConsequence::BelowMinimum; } // NOTE: We assume that we are a provider, so don't need to do any checks in the @@ -829,11 +829,11 @@ impl, I: 'static> Pallet { account: &AccountData, ) -> WithdrawConsequence { if amount.is_zero() { - return WithdrawConsequence::Success + return WithdrawConsequence::Success; } if TotalIssuance::::get().checked_sub(&amount).is_none() { - return WithdrawConsequence::Underflow + return WithdrawConsequence::Underflow; } let new_total_balance = match account.total().checked_sub(&amount) { @@ -850,7 +850,7 @@ impl, I: 'static> Pallet { if frame_system::Pallet::::can_dec_provider(who) { WithdrawConsequence::ReducedToZero(new_total_balance) } else { - return WithdrawConsequence::WouldDie + return WithdrawConsequence::WouldDie; } } else { WithdrawConsequence::Success @@ -865,7 +865,7 @@ impl, I: 'static> Pallet { // Eventual free funds must be no less than the frozen balance. let min_balance = account.frozen(Reasons::All); if new_free_balance < min_balance { - return WithdrawConsequence::Frozen + return WithdrawConsequence::Frozen; } success @@ -1006,14 +1006,14 @@ impl, I: 'static> Pallet { status: Status, ) -> Result { if value.is_zero() { - return Ok(Zero::zero()) + return Ok(Zero::zero()); } if slashed == beneficiary { return match status { Status::Free => Ok(Self::unreserve(slashed, value)), Status::Reserved => Ok(value.saturating_sub(Self::reserved_balance(slashed))), - } + }; } let ((actual, _maybe_one_dust), _maybe_other_dust) = Self::try_mutate_account_with_dust( @@ -1026,16 +1026,18 @@ impl, I: 'static> Pallet { let actual = cmp::min(from_account.reserved, value); ensure!(best_effort || actual == value, Error::::InsufficientBalance); match status { - Status::Free => + Status::Free => { to_account.free = to_account .free .checked_add(&actual) - .ok_or(ArithmeticError::Overflow)?, - Status::Reserved => + .ok_or(ArithmeticError::Overflow)? + }, + Status::Reserved => { to_account.reserved = to_account .reserved .checked_add(&actual) - .ok_or(ArithmeticError::Overflow)?, + .ok_or(ArithmeticError::Overflow)? + }, } from_account.reserved -= actual; Ok(actual) @@ -1094,7 +1096,7 @@ impl, I: 'static> fungible::Inspect for Pallet impl, I: 'static> fungible::Mutate for Pallet { fn mint_into(who: &T::AccountId, amount: Self::Balance) -> DispatchResult { if amount.is_zero() { - return Ok(()) + return Ok(()); } Self::try_mutate_account(who, |account, _is_new| -> DispatchResult { Self::deposit_consequence(who, amount, account, true).into_result()?; @@ -1111,7 +1113,7 @@ impl, I: 'static> fungible::Mutate for Pallet { amount: Self::Balance, ) -> Result { if amount.is_zero() { - return Ok(Self::Balance::zero()) + return Ok(Self::Balance::zero()); } let actual = Self::try_mutate_account( who, @@ -1142,15 +1144,19 @@ impl, I: 'static> fungible::Transfer for Pallet impl, I: 'static> fungible::Unbalanced for Pallet { fn set_balance(who: &T::AccountId, amount: Self::Balance) -> DispatchResult { - Self::mutate_account(who, |account| { - account.free = amount; + Self::mutate_account(who, |account| -> DispatchResult { + // Balance is the same type and will not overflow + // The provided `fungibles::Unbalanced` functions always provide `set_balance` with `free + reserved + amount` + // free = new_balance - reserved + account.free = + amount.checked_sub(&account.reserved).ok_or(ArithmeticError::Underflow)?; Self::deposit_event(Event::BalanceSet { who: who.clone(), free: account.free, reserved: account.reserved, }); - })?; - Ok(()) + Ok(()) + })? } fn set_total_issuance(amount: Self::Balance) { @@ -1166,7 +1172,7 @@ impl, I: 'static> fungible::InspectHold for Pallet, I: 'static> fungible::InspectHold for Pallet, I: 'static> fungible::MutateHold for Pallet { fn hold(who: &T::AccountId, amount: Self::Balance) -> DispatchResult { if amount.is_zero() { - return Ok(()) + return Ok(()); } ensure!(Self::can_reserve(who, amount), Error::::InsufficientBalance); Self::mutate_account(who, |a| { @@ -1195,7 +1201,7 @@ impl, I: 'static> fungible::MutateHold for Pallet Result { if amount.is_zero() { - return Ok(amount) + return Ok(amount); } // Done on a best-effort basis. Self::try_mutate_account(who, |a, _| { @@ -1401,7 +1407,7 @@ where // Check if `value` amount of free balance can be slashed from `who`. fn can_slash(who: &T::AccountId, value: Self::Balance) -> bool { if value.is_zero() { - return true + return true; } Self::free_balance(who) >= value } @@ -1418,7 +1424,7 @@ where // Is a no-op if amount to be burned is zero. fn burn(mut amount: Self::Balance) -> Self::PositiveImbalance { if amount.is_zero() { - return PositiveImbalance::zero() + return PositiveImbalance::zero(); } >::mutate(|issued| { *issued = issued.checked_sub(&amount).unwrap_or_else(|| { @@ -1434,7 +1440,7 @@ where // Is a no-op if amount to be issued it zero. fn issue(mut amount: Self::Balance) -> Self::NegativeImbalance { if amount.is_zero() { - return NegativeImbalance::zero() + return NegativeImbalance::zero(); } >::mutate(|issued| { *issued = issued.checked_add(&amount).unwrap_or_else(|| { @@ -1464,7 +1470,7 @@ where new_balance: T::Balance, ) -> DispatchResult { if amount.is_zero() { - return Ok(()) + return Ok(()); } let min_balance = Self::account(who).frozen(reasons.into()); ensure!(new_balance >= min_balance, Error::::LiquidityRestrictions); @@ -1480,7 +1486,7 @@ where existence_requirement: ExistenceRequirement, ) -> DispatchResult { if value.is_zero() || transactor == dest { - return Ok(()) + return Ok(()); } Self::try_mutate_account_with_dust( @@ -1548,10 +1554,10 @@ where /// inconsistent or `can_slash` wasn't used appropriately. fn slash(who: &T::AccountId, value: Self::Balance) -> (Self::NegativeImbalance, Self::Balance) { if value.is_zero() { - return (NegativeImbalance::zero(), Zero::zero()) + return (NegativeImbalance::zero(), Zero::zero()); } if Self::total_balance(who).is_zero() { - return (NegativeImbalance::zero(), value) + return (NegativeImbalance::zero(), value); } for attempt in 0..2 { @@ -1600,7 +1606,7 @@ where who: who.clone(), amount: value.saturating_sub(not_slashed), }); - return (imbalance, not_slashed) + return (imbalance, not_slashed); }, Err(_) => (), } @@ -1618,7 +1624,7 @@ where value: Self::Balance, ) -> Result { if value.is_zero() { - return Ok(PositiveImbalance::zero()) + return Ok(PositiveImbalance::zero()); } Self::try_mutate_account( @@ -1643,7 +1649,7 @@ where /// - `value` is so large it would cause the balance of `who` to overflow. fn deposit_creating(who: &T::AccountId, value: Self::Balance) -> Self::PositiveImbalance { if value.is_zero() { - return Self::PositiveImbalance::zero() + return Self::PositiveImbalance::zero(); } Self::try_mutate_account( @@ -1676,7 +1682,7 @@ where liveness: ExistenceRequirement, ) -> result::Result { if value.is_zero() { - return Ok(NegativeImbalance::zero()) + return Ok(NegativeImbalance::zero()); } Self::try_mutate_account( @@ -1749,7 +1755,7 @@ where /// Always `true` if value to be reserved is zero. fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool { if value.is_zero() { - return true + return true; } Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| { Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance).is_ok() @@ -1765,7 +1771,7 @@ where /// Is a no-op if value to be reserved is zero. fn reserve(who: &T::AccountId, value: Self::Balance) -> DispatchResult { if value.is_zero() { - return Ok(()) + return Ok(()); } Self::try_mutate_account(who, |account, _| -> DispatchResult { @@ -1785,10 +1791,10 @@ where /// Is a no-op if the value to be unreserved is zero or the account does not exist. fn unreserve(who: &T::AccountId, value: Self::Balance) -> Self::Balance { if value.is_zero() { - return Zero::zero() + return Zero::zero(); } if Self::total_balance(who).is_zero() { - return value + return value; } let actual = match Self::mutate_account(who, |account| { @@ -1804,7 +1810,7 @@ where // This should never happen since we don't alter the total amount in the account. // If it ever does, then we should fail gracefully though, indicating that nothing // could be done. - return value + return value; }, }; @@ -1821,10 +1827,10 @@ where value: Self::Balance, ) -> (Self::NegativeImbalance, Self::Balance) { if value.is_zero() { - return (NegativeImbalance::zero(), Zero::zero()) + return (NegativeImbalance::zero(), Zero::zero()); } if Self::total_balance(who).is_zero() { - return (NegativeImbalance::zero(), value) + return (NegativeImbalance::zero(), value); } // NOTE: `mutate_account` may fail if it attempts to reduce the balance to the point that an @@ -1853,7 +1859,7 @@ where who: who.clone(), amount: value.saturating_sub(not_slashed), }); - return (imbalance, not_slashed) + return (imbalance, not_slashed); }, Err(_) => (), } @@ -1902,7 +1908,7 @@ where value: Self::Balance, ) -> DispatchResult { if value.is_zero() { - return Ok(()) + return Ok(()); } Reserves::::try_mutate(who, |reserves| -> DispatchResult { @@ -1931,7 +1937,7 @@ where value: Self::Balance, ) -> Self::Balance { if value.is_zero() { - return Zero::zero() + return Zero::zero(); } Reserves::::mutate_exists(who, |maybe_reserves| -> Self::Balance { @@ -1978,7 +1984,7 @@ where value: Self::Balance, ) -> (Self::NegativeImbalance, Self::Balance) { if value.is_zero() { - return (NegativeImbalance::zero(), Zero::zero()) + return (NegativeImbalance::zero(), Zero::zero()); } Reserves::::mutate(who, |reserves| -> (Self::NegativeImbalance, Self::Balance) { @@ -2017,15 +2023,16 @@ where status: Status, ) -> Result { if value.is_zero() { - return Ok(Zero::zero()) + return Ok(Zero::zero()); } if slashed == beneficiary { return match status { Status::Free => Ok(Self::unreserve_named(id, slashed, value)), - Status::Reserved => - Ok(value.saturating_sub(Self::reserved_balance_named(id, slashed))), - } + Status::Reserved => { + Ok(value.saturating_sub(Self::reserved_balance_named(id, slashed))) + }, + }; } Reserves::::try_mutate(slashed, |reserves| -> Result { @@ -2123,7 +2130,7 @@ where reasons: WithdrawReasons, ) { if amount.is_zero() || reasons.is_empty() { - return + return; } let mut new_lock = Some(BalanceLock { id, amount, reasons: reasons.into() }); let mut locks = Self::locks(who) @@ -2145,7 +2152,7 @@ where reasons: WithdrawReasons, ) { if amount.is_zero() || reasons.is_empty() { - return + return; } let mut new_lock = Some(BalanceLock { id, amount, reasons: reasons.into() }); let mut locks = Self::locks(who) diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 8f5470ae3cac2..981d9ba5cdec8 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -24,7 +24,7 @@ macro_rules! decl_tests { ($test:ty, $ext_builder:ty, $existential_deposit:expr) => { use crate::*; - use sp_runtime::{ArithmeticError, FixedPointNumber, traits::{SignedExtension, BadOrigin}}; + use sp_runtime::{ArithmeticError, TokenError, FixedPointNumber, traits::{SignedExtension, BadOrigin}}; use frame_support::{ assert_noop, assert_storage_noop, assert_ok, assert_err, traits::{ @@ -1263,5 +1263,163 @@ macro_rules! decl_tests { assert_eq!(Balances::reserved_balance(&1337), 42); }); } + + #[test] + fn fungible_unbalanced_trait_set_balance_works() { + <$ext_builder>::default().build().execute_with(|| { + assert_eq!(>::balance(&1337), 0); + assert_ok!(>::set_balance(&1337, 100)); + assert_eq!(>::balance(&1337), 100); + + assert_ok!(Balances::reserve(&1337, 60)); + assert_eq!(Balances::free_balance(1337) , 40); + assert_eq!(Balances::reserved_balance(1337), 60); + + assert_noop!(>::set_balance(&1337, 0), ArithmeticError::Underflow); + + assert_ok!(>::set_balance(&1337, 60)); + assert_eq!(Balances::free_balance(1337) , 0); + assert_eq!(Balances::reserved_balance(1337), 60); + }); + } + + #[test] + fn fungible_unbalanced_trait_set_total_issuance_works() { + <$ext_builder>::default().build().execute_with(|| { + assert_eq!(>::total_issuance(), 0); + >::set_total_issuance(100); + assert_eq!(>::total_issuance(), 100); + }); + } + + #[test] + fn fungible_unbalanced_trait_decrease_balance_simple_works() { + <$ext_builder>::default().build().execute_with(|| { + // An Account that starts at 100 + assert_ok!(>::set_balance(&1337, 100)); + // and reserves 50 + assert_ok!(Balances::reserve(&1337, 50)); + // and is decreased by 20 + assert_ok!(>::decrease_balance(&1337, 20)); + // should end up at 80. + assert_eq!(>::balance(&1337), 80); + }); + } + + #[test] + fn fungible_unbalanced_trait_decrease_balance_works() { + <$ext_builder>::default().build().execute_with(|| { + assert_ok!(>::set_balance(&1337, 100)); + assert_eq!(>::balance(&1337), 100); + + assert_noop!( + >::decrease_balance(&1337, 101), + TokenError::NoFunds + ); + assert_eq!( + >::decrease_balance(&1337, 100), + Ok(100) + ); + assert_eq!(>::balance(&1337), 0); + + // free: 40, reserved: 60 + assert_ok!(>::set_balance(&1337, 100)); + assert_ok!(Balances::reserve(&1337, 60)); + assert_eq!(Balances::free_balance(1337) , 40); + assert_eq!(Balances::reserved_balance(1337), 60); + assert_noop!( + >::decrease_balance(&1337, 41), + TokenError::NoFunds + ); + assert_eq!( + >::decrease_balance(&1337, 40), + Ok(40) + ); + assert_eq!(>::balance(&1337), 60); + assert_eq!(Balances::free_balance(1337), 0); + assert_eq!(Balances::reserved_balance(1337), 60); + }); + } + + #[test] + fn fungible_unbalanced_trait_decrease_balance_at_most_works() { + <$ext_builder>::default().build().execute_with(|| { + assert_ok!(>::set_balance(&1337, 100)); + assert_eq!(>::balance(&1337), 100); + + assert_eq!( + >::decrease_balance_at_most(&1337, 101), + 100 + ); + assert_eq!(>::balance(&1337), 0); + + assert_ok!(>::set_balance(&1337, 100)); + assert_eq!( + >::decrease_balance_at_most(&1337, 100), + 100 + ); + assert_eq!(>::balance(&1337), 0); + + // free: 40, reserved: 60 + assert_ok!(>::set_balance(&1337, 100)); + assert_ok!(Balances::reserve(&1337, 60)); + assert_eq!(Balances::free_balance(1337) , 40); + assert_eq!(Balances::reserved_balance(1337), 60); + assert_eq!( + >::decrease_balance_at_most(&1337, 0), + 0 + ); + assert_eq!(Balances::free_balance(1337) , 40); + assert_eq!(Balances::reserved_balance(1337), 60); + assert_eq!( + >::decrease_balance_at_most(&1337, 10), + 10 + ); + assert_eq!(Balances::free_balance(1337), 30); + assert_eq!( + >::decrease_balance_at_most(&1337, 200), + 30 + ); + assert_eq!(>::balance(&1337), 60); + assert_eq!(Balances::free_balance(1337), 0); + assert_eq!(Balances::reserved_balance(1337), 60); + }); + } + + #[test] + fn fungible_unbalanced_trait_increase_balance_works() { + <$ext_builder>::default().build().execute_with(|| { + assert_noop!( + >::increase_balance(&1337, 0), + TokenError::BelowMinimum + ); + assert_eq!( + >::increase_balance(&1337, 1), + Ok(1) + ); + assert_noop!( + >::increase_balance(&1337, u64::MAX), + ArithmeticError::Overflow + ); + }); + } + + #[test] + fn fungible_unbalanced_trait_increase_balance_at_most_works() { + <$ext_builder>::default().build().execute_with(|| { + assert_eq!( + >::increase_balance_at_most(&1337, 0), + 0 + ); + assert_eq!( + >::increase_balance_at_most(&1337, 1), + 1 + ); + assert_eq!( + >::increase_balance_at_most(&1337, u64::MAX), + u64::MAX - 1 + ); + }); + } } } diff --git a/frame/support/src/traits/tokens/fungible/balanced.rs b/frame/support/src/traits/tokens/fungible/balanced.rs index ed9c3a1afa480..ed1a032283bf5 100644 --- a/frame/support/src/traits/tokens/fungible/balanced.rs +++ b/frame/support/src/traits/tokens/fungible/balanced.rs @@ -164,8 +164,8 @@ pub trait Unbalanced: Inspect { amount: Self::Balance, ) -> Result { let old_balance = Self::balance(who); - let (mut new_balance, mut amount) = if old_balance < amount { - return Err(TokenError::NoFunds.into()) + let (mut new_balance, mut amount) = if Self::reducible_balance(who, false) < amount { + return Err(TokenError::NoFunds.into()); } else { (old_balance - amount, amount) }; @@ -186,8 +186,9 @@ pub trait Unbalanced: Inspect { /// Return the imbalance by which the account was reduced. fn decrease_balance_at_most(who: &AccountId, amount: Self::Balance) -> Self::Balance { let old_balance = Self::balance(who); - let (mut new_balance, mut amount) = if old_balance < amount { - (Zero::zero(), old_balance) + let old_free_balance = Self::reducible_balance(who, false); + let (mut new_balance, mut amount) = if old_free_balance < amount { + (old_balance.saturating_sub(old_free_balance), old_free_balance) } else { (old_balance - amount, amount) }; @@ -225,7 +226,7 @@ pub trait Unbalanced: Inspect { let old_balance = Self::balance(who); let new_balance = old_balance.checked_add(&amount).ok_or(ArithmeticError::Overflow)?; if new_balance < Self::minimum_balance() { - return Err(TokenError::BelowMinimum.into()) + return Err(TokenError::BelowMinimum.into()); } if old_balance != new_balance { Self::set_balance(who, new_balance)?; diff --git a/frame/support/src/traits/tokens/fungibles/balanced.rs b/frame/support/src/traits/tokens/fungibles/balanced.rs index a75832e4c440f..06cebd35d1b2e 100644 --- a/frame/support/src/traits/tokens/fungibles/balanced.rs +++ b/frame/support/src/traits/tokens/fungibles/balanced.rs @@ -185,8 +185,8 @@ pub trait Unbalanced: Inspect { amount: Self::Balance, ) -> Result { let old_balance = Self::balance(asset, who); - let (mut new_balance, mut amount) = if old_balance < amount { - return Err(TokenError::NoFunds.into()) + let (mut new_balance, mut amount) = if Self::reducible_balance(asset, who, false) < amount { + return Err(TokenError::NoFunds.into()); } else { (old_balance - amount, amount) }; @@ -211,8 +211,9 @@ pub trait Unbalanced: Inspect { amount: Self::Balance, ) -> Self::Balance { let old_balance = Self::balance(asset, who); - let (mut new_balance, mut amount) = if old_balance < amount { - (Zero::zero(), old_balance) + let old_free_balance = Self::reducible_balance(asset, who, false); + let (mut new_balance, mut amount) = if old_free_balance < amount { + (old_balance.saturating_sub(old_free_balance), old_free_balance) } else { (old_balance - amount, amount) }; @@ -251,7 +252,7 @@ pub trait Unbalanced: Inspect { let old_balance = Self::balance(asset, who); let new_balance = old_balance.checked_add(&amount).ok_or(ArithmeticError::Overflow)?; if new_balance < Self::minimum_balance(asset) { - return Err(TokenError::BelowMinimum.into()) + return Err(TokenError::BelowMinimum.into()); } if old_balance != new_balance { Self::set_balance(asset, who, new_balance)?; diff --git a/frame/transaction-payment/asset-tx-payment/src/tests.rs b/frame/transaction-payment/asset-tx-payment/src/tests.rs index eec3862eedc38..3a817b3d67d4a 100644 --- a/frame/transaction-payment/asset-tx-payment/src/tests.rs +++ b/frame/transaction-payment/asset-tx-payment/src/tests.rs @@ -198,7 +198,6 @@ impl HandleCredit for CreditToBlockAuthor { } impl pallet_asset_tx_payment::Config for Runtime { - type Event = Event; type Fungibles = Assets; type OnChargeAssetTransaction = FungiblesAdapter< pallet_assets::BalanceToAssetBalance,