Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Return early when fees/balances/values are zero #4628

Merged
merged 2 commits into from
Jan 15, 2020
Merged

Return early when fees/balances/values are zero #4628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bef895
Return early when fees/balances/values are zero
shawntabrizi Jan 15, 2020
42e59b4
Add docs about no-op
shawntabrizi Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion frame/balances/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,10 @@ 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
}

Expand All @@ -901,7 +904,10 @@ where
<FreeBalance<T, I>>::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() }
<TotalIssuance<T, I>>::mutate(|issued| {
*issued = issued.checked_sub(&amount).unwrap_or_else(|| {
amount = *issued;
Expand All @@ -911,7 +917,11 @@ 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() }
<TotalIssuance<T, I>>::mutate(|issued|
*issued = issued.checked_add(&amount).unwrap_or_else(|| {
amount = Self::Balance::max_value() - *issued;
Expand All @@ -921,16 +931,21 @@ 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.
//
// # <weight>
// 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.
// # </weight>
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
{
Expand All @@ -955,12 +970,15 @@ 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,
value: Self::Balance,
existence_requirement: ExistenceRequirement,
) -> DispatchResult {
if value.is_zero() { return Ok(()) }
Copy link
Member Author

@shawntabrizi shawntabrizi Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One side effect of this PR is that a transfer of zero just returns early.

Before, a transfer of zero would additionally charge a fee of T::CreationFee or T::TransferFee on top of the existing fee system.

Now, if the transfer is zero, no logic is run and these additional fees are skipped. A user would still pay the normal transaction fee calculated with the transaction_payment module.

This all makes sense to me, but introduces a new behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also means a transfer of 0 emits no Balances events.

let from_balance = Self::free_balance(transactor);
let to_balance = Self::free_balance(dest);
let would_create = to_balance.is_zero();
Expand Down Expand Up @@ -1001,12 +1019,16 @@ 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,
reasons: WithdrawReasons,
liveness: ExistenceRequirement,
) -> result::Result<Self::NegativeImbalance, DispatchError> {
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...
Expand All @@ -1026,10 +1048,15 @@ 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
) -> (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);

Expand All @@ -1049,21 +1076,29 @@ 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
) -> result::Result<Self::PositiveImbalance, DispatchError> {
if value.is_zero() { return Ok(PositiveImbalance::zero()) }

if Self::total_balance(who).is_zero() {
Err(Error::<T, I>::DeadAccount)?
}
Self::set_free_balance(who, Self::free_balance(who) + value);
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,
) -> 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
Expand Down Expand Up @@ -1122,7 +1157,10 @@ impl<T: Trait<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I>
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)
.checked_sub(&value)
.map_or(false, |new_balance|
Expand All @@ -1134,7 +1172,10 @@ where
<ReservedBalance<T, I>>::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);
if b < value {
Err(Error::<T, I>::InsufficientBalance)?
Expand All @@ -1146,30 +1187,40 @@ where
Ok(())
}

// 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);
let actual = cmp::min(b, value);
Self::set_free_balance(who, Self::free_balance(who) + actual);
Self::set_reserved_balance(who, b - actual);
value - actual
}

// 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
) -> (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.
Self::set_reserved_balance(who, b - slash);
(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,
value: Self::Balance,
) -> result::Result<Self::Balance, DispatchError> {
if value.is_zero() { return Ok (Zero::zero()) }
if Self::total_balance(beneficiary).is_zero() {
Err(Error::<T, I>::DeadAccount)?
}
Expand All @@ -1187,13 +1238,16 @@ 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,
amount: T::Balance,
until: T::BlockNumber,
reasons: WithdrawReasons,
) {
if amount.is_zero() { return }
let now = <frame_system::Module<T>>::block_number();
let mut new_lock = Some(BalanceLock { id, amount, until, reasons });
let mut locks = Self::locks(who).into_iter().filter_map(|l|
Expand Down Expand Up @@ -1275,12 +1329,14 @@ 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,
per_block: T::Balance,
starting_block: T::BlockNumber
) -> DispatchResult {
if locked.is_zero() { return Ok(()) }
if <Vesting<T, I>>::exists(who) {
Err(Error::<T, I>::ExistingVestingSchedule)?
}
Expand Down
31 changes: 17 additions & 14 deletions frame/transaction-payment/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,20 +218,23 @@ impl<T: Trait + Send + Sync> SignedExtension for ChargeTransactionPayment<T>
// 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
Expand Down