From 0bef8953f30fc91b7e46f4738122e1e68ce6da3b Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 15 Jan 2020 11:39:19 +0100 Subject: [PATCH 1/2] Return early when fees/balances/values are zero --- frame/balances/src/lib.rs | 54 ++++++++++++++++++++-------- frame/transaction-payment/src/lib.rs | 31 ++++++++-------- 2 files changed, 57 insertions(+), 28 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 1f8e099880b7d..03506764fff85 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -886,6 +886,7 @@ where } fn can_slash(who: &T::AccountId, value: Self::Balance) -> bool { + if value.is_zero() { return true } Self::free_balance(who) >= value } @@ -902,6 +903,7 @@ where } fn burn(mut amount: Self::Balance) -> Self::PositiveImbalance { + if amount.is_zero() { return PositiveImbalance::zero() } >::mutate(|issued| { *issued = issued.checked_sub(&amount).unwrap_or_else(|| { amount = *issued; @@ -912,6 +914,7 @@ where } fn issue(mut amount: Self::Balance) -> Self::NegativeImbalance { + if amount.is_zero() { return NegativeImbalance::zero() } >::mutate(|issued| *issued = issued.checked_add(&amount).unwrap_or_else(|| { amount = Self::Balance::max_value() - *issued; @@ -927,10 +930,11 @@ where // # fn ensure_can_withdraw( who: &T::AccountId, - _amount: T::Balance, + amount: T::Balance, reasons: WithdrawReasons, new_balance: T::Balance, ) -> DispatchResult { + if amount.is_zero() { return Ok(()) } if reasons.intersects(WithdrawReason::Reserve | WithdrawReason::Transfer) && Self::vesting_balance(who) > new_balance { @@ -961,6 +965,7 @@ where value: Self::Balance, existence_requirement: ExistenceRequirement, ) -> DispatchResult { + if value.is_zero() { return Ok(()) } let from_balance = Self::free_balance(transactor); let to_balance = Self::free_balance(dest); let would_create = to_balance.is_zero(); @@ -1007,6 +1012,8 @@ where reasons: WithdrawReasons, liveness: ExistenceRequirement, ) -> result::Result { + if value.is_zero() { return Ok(NegativeImbalance::zero()); } + let old_balance = Self::free_balance(who); if let Some(new_balance) = old_balance.checked_sub(&value) { // if we need to keep the account alive... @@ -1026,10 +1033,14 @@ where } } + // Slash an account, returning the negative imbalance created and any left over + // amount that could not be slashed. fn slash( who: &T::AccountId, value: Self::Balance ) -> (Self::NegativeImbalance, Self::Balance) { + if value.is_zero() { return (NegativeImbalance::zero(), Zero::zero()) } + let free_balance = Self::free_balance(who); let free_slash = cmp::min(free_balance, value); @@ -1053,6 +1064,8 @@ where who: &T::AccountId, value: Self::Balance ) -> result::Result { + if value.is_zero() { return Ok(PositiveImbalance::zero()) } + if Self::total_balance(who).is_zero() { Err(Error::::DeadAccount)? } @@ -1064,6 +1077,8 @@ where who: &T::AccountId, value: Self::Balance, ) -> Self::PositiveImbalance { + if value.is_zero() { return Self::PositiveImbalance::zero() } + let (imbalance, _) = Self::make_free_balance_be(who, Self::free_balance(who) + value); if let SignedImbalance::Positive(p) = imbalance { p @@ -1123,6 +1138,7 @@ where T::Balance: MaybeSerializeDeserialize + Debug { fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool { + if value.is_zero() { return true } Self::free_balance(who) .checked_sub(&value) .map_or(false, |new_balance| @@ -1135,6 +1151,7 @@ where } fn reserve(who: &T::AccountId, value: Self::Balance) -> result::Result<(), DispatchError> { + if value.is_zero() { return Ok(()) } let b = Self::free_balance(who); if b < value { Err(Error::::InsufficientBalance)? @@ -1146,7 +1163,9 @@ where Ok(()) } + // Unreserve some funds, returning any amount that was unable to be unreserved. fn unreserve(who: &T::AccountId, value: Self::Balance) -> Self::Balance { + if value.is_zero() { return Zero::zero() } let b = Self::reserved_balance(who); let actual = cmp::min(b, value); Self::set_free_balance(who, Self::free_balance(who) + actual); @@ -1154,10 +1173,13 @@ where value - actual } + // Slash from reserved balance, returning the negative imbalance created, + // and any amount that was unable to be slashed. fn slash_reserved( who: &T::AccountId, value: Self::Balance ) -> (Self::NegativeImbalance, Self::Balance) { + if value.is_zero() { return (NegativeImbalance::zero(), Zero::zero()) } let b = Self::reserved_balance(who); let slash = cmp::min(b, value); // underflow should never happen, but it if does, there's nothing to be done here. @@ -1170,6 +1192,7 @@ where beneficiary: &T::AccountId, value: Self::Balance, ) -> result::Result { + if value.is_zero() { return Ok (Zero::zero()) } if Self::total_balance(beneficiary).is_zero() { Err(Error::::DeadAccount)? } @@ -1194,20 +1217,22 @@ where until: T::BlockNumber, reasons: WithdrawReasons, ) { - let now = >::block_number(); - let mut new_lock = Some(BalanceLock { id, amount, until, reasons }); - let mut locks = Self::locks(who).into_iter().filter_map(|l| - if l.id == id { - new_lock.take() - } else if l.until > now { - Some(l) - } else { - None - }).collect::>(); - if let Some(lock) = new_lock { - locks.push(lock) + if !amount.is_zero(){ + let now = >::block_number(); + let mut new_lock = Some(BalanceLock { id, amount, until, reasons }); + let mut locks = Self::locks(who).into_iter().filter_map(|l| + if l.id == id { + new_lock.take() + } else if l.until > now { + Some(l) + } else { + None + }).collect::>(); + if let Some(lock) = new_lock { + locks.push(lock) + } + >::insert(who, locks); } - >::insert(who, locks); } fn extend_lock( @@ -1281,6 +1306,7 @@ where per_block: T::Balance, starting_block: T::BlockNumber ) -> DispatchResult { + if locked.is_zero() { return Ok(()) } if >::exists(who) { Err(Error::::ExistingVestingSchedule)? } diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index bae3096f3cdd1..fa73b0a9faf3e 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -218,20 +218,23 @@ impl SignedExtension for ChargeTransactionPayment // pay any fees. let tip = self.0; let fee = Self::compute_fee(len as u32, info, tip); - let imbalance = match T::Currency::withdraw( - who, - fee, - if tip.is_zero() { - WithdrawReason::TransactionPayment.into() - } else { - WithdrawReason::TransactionPayment | WithdrawReason::Tip - }, - ExistenceRequirement::KeepAlive, - ) { - Ok(imbalance) => imbalance, - Err(_) => return InvalidTransaction::Payment.into(), - }; - T::OnTransactionPayment::on_unbalanced(imbalance); + // Only mess with balances if fee is not zero. + if !fee.is_zero() { + let imbalance = match T::Currency::withdraw( + who, + fee, + if tip.is_zero() { + WithdrawReason::TransactionPayment.into() + } else { + WithdrawReason::TransactionPayment | WithdrawReason::Tip + }, + ExistenceRequirement::KeepAlive, + ) { + Ok(imbalance) => imbalance, + Err(_) => return InvalidTransaction::Payment.into(), + }; + T::OnTransactionPayment::on_unbalanced(imbalance); + } let mut r = ValidTransaction::default(); // NOTE: we probably want to maximize the _fee (of any type) per weight unit_ here, which From 42e59b48a265ce8b33b71c2c65c17529d5f8d9c7 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 15 Jan 2020 12:34:42 +0100 Subject: [PATCH 2/2] Add docs about no-op --- frame/balances/src/lib.rs | 60 +++++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 03506764fff85..b488b96701cf1 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -885,6 +885,8 @@ where Self::free_balance(who) + Self::reserved_balance(who) } + // Check if `value` amount of free balance can be slashed from `who`. + // Is a no-op if value to be slashed is zero. fn can_slash(who: &T::AccountId, value: Self::Balance) -> bool { if value.is_zero() { return true } Self::free_balance(who) >= value @@ -902,6 +904,8 @@ where >::get(who) } + // Burn funds from the total issuance, returning a positive imbalance for the amount burned. + // 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() } >::mutate(|issued| { @@ -913,6 +917,9 @@ where PositiveImbalance::new(amount) } + // Create new funds into the total issuance, returning a negative imbalance + // for the amount issued. + // 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() } >::mutate(|issued| @@ -924,6 +931,10 @@ where NegativeImbalance::new(amount) } + // Ensure that an account can withdraw from their free balance given any existing withdrawal + // restrictions like locks and vesting balance. + // Is a no-op if amount to be withdrawn is zero. + // // # // Despite iterating over a list of locks, they are limited by the number of // lock IDs, which means the number of runtime modules that intend to use and create locks. @@ -959,6 +970,8 @@ where } } + // Transfer some free balance from `transactor` to `dest`, respecting existence requirements. + // Is a no-op if value to be transferred is zero. fn transfer( transactor: &T::AccountId, dest: &T::AccountId, @@ -1006,6 +1019,8 @@ where Ok(()) } + // Withdraw some free balance from an account, respecting existence requirements. + // Is a no-op if value to be withdrawn is zero. fn withdraw( who: &T::AccountId, value: Self::Balance, @@ -1035,6 +1050,7 @@ where // Slash an account, returning the negative imbalance created and any left over // amount that could not be slashed. + // Is a no-op if value to be slashed is zero. fn slash( who: &T::AccountId, value: Self::Balance @@ -1060,6 +1076,8 @@ where } } + // Deposit some `value` into the free balance of an existing account. + // Is a no-op if the value to be deposited is zero. fn deposit_into_existing( who: &T::AccountId, value: Self::Balance @@ -1073,6 +1091,8 @@ where Ok(PositiveImbalance::new(value)) } + // Deposit some `value` into the free balance of `who`, possibly creating a new account. + // Is a no-op if the value to be deposited is zero. fn deposit_creating( who: &T::AccountId, value: Self::Balance, @@ -1137,6 +1157,8 @@ impl, I: Instance> ReservableCurrency for Module where T::Balance: MaybeSerializeDeserialize + Debug { + // Check if `who` can reserve `value` from their free balance. + // Is a no-op if value to be reserved is zero. fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool { if value.is_zero() { return true } Self::free_balance(who) @@ -1150,6 +1172,8 @@ where >::get(who) } + // Move `value` from the free balance from `who` to their reserved balance. + // Is a no-op if value to be reserved is zero. fn reserve(who: &T::AccountId, value: Self::Balance) -> result::Result<(), DispatchError> { if value.is_zero() { return Ok(()) } let b = Self::free_balance(who); @@ -1164,6 +1188,7 @@ where } // Unreserve some funds, returning any amount that was unable to be unreserved. + // Is a no-op if the value to be unreserved is zero. fn unreserve(who: &T::AccountId, value: Self::Balance) -> Self::Balance { if value.is_zero() { return Zero::zero() } let b = Self::reserved_balance(who); @@ -1175,6 +1200,7 @@ where // Slash from reserved balance, returning the negative imbalance created, // and any amount that was unable to be slashed. + // Is a no-op if the value to be slashed is zero. fn slash_reserved( who: &T::AccountId, value: Self::Balance @@ -1187,6 +1213,8 @@ where (NegativeImbalance::new(slash), value - slash) } + // Move the reserved balance of one account into the free balance of another. + // Is a no-op if the value to be moved is zero. fn repatriate_reserved( slashed: &T::AccountId, beneficiary: &T::AccountId, @@ -1210,6 +1238,8 @@ where { type Moment = T::BlockNumber; + // Set a lock on the balance of `who`. + // Is a no-op if lock amount is zero. fn set_lock( id: LockIdentifier, who: &T::AccountId, @@ -1217,22 +1247,21 @@ where until: T::BlockNumber, reasons: WithdrawReasons, ) { - if !amount.is_zero(){ - let now = >::block_number(); - let mut new_lock = Some(BalanceLock { id, amount, until, reasons }); - let mut locks = Self::locks(who).into_iter().filter_map(|l| - if l.id == id { - new_lock.take() - } else if l.until > now { - Some(l) - } else { - None - }).collect::>(); - if let Some(lock) = new_lock { - locks.push(lock) - } - >::insert(who, locks); + if amount.is_zero() { return } + let now = >::block_number(); + let mut new_lock = Some(BalanceLock { id, amount, until, reasons }); + let mut locks = Self::locks(who).into_iter().filter_map(|l| + if l.id == id { + new_lock.take() + } else if l.until > now { + Some(l) + } else { + None + }).collect::>(); + if let Some(lock) = new_lock { + locks.push(lock) } + >::insert(who, locks); } fn extend_lock( @@ -1300,6 +1329,7 @@ where /// /// If there already exists a vesting schedule for the given account, an `Err` is returned /// and nothing is updated. + /// Is a no-op if the amount to be vested is zero. fn add_vesting_schedule( who: &T::AccountId, locked: T::Balance,