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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
54 changes: 40 additions & 14 deletions frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -902,6 +903,7 @@ where
}

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 @@ -912,6 +914,7 @@ where
}

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 @@ -927,10 +930,11 @@ where
// # </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 Down Expand Up @@ -961,6 +965,7 @@ where
value: Self::Balance,
existence_requirement: ExistenceRequirement,
) -> DispatchResult {
if value.is_zero() { return Ok(()) }
Copy link
Member Author

@shawntabrizi shawntabrizi Jan 15, 2020

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 @@ -1007,6 +1012,8 @@ where
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 +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);

Expand All @@ -1053,6 +1064,8 @@ where
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)?
}
Expand All @@ -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
Expand Down Expand Up @@ -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|
Expand All @@ -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::<T, I>::InsufficientBalance)?
Expand All @@ -1146,18 +1163,23 @@ 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);
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.
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.
Expand All @@ -1170,6 +1192,7 @@ where
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 @@ -1194,20 +1217,22 @@ where
until: T::BlockNumber,
reasons: WithdrawReasons,
) {
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|
if l.id == id {
new_lock.take()
} else if l.until > now {
Some(l)
} else {
None
}).collect::<Vec<_>>();
if let Some(lock) = new_lock {
locks.push(lock)
if !amount.is_zero(){
shawntabrizi marked this conversation as resolved.
Show resolved Hide resolved
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|
if l.id == id {
new_lock.take()
} else if l.until > now {
Some(l)
} else {
None
}).collect::<Vec<_>>();
if let Some(lock) = new_lock {
locks.push(lock)
}
<Locks<T, I>>::insert(who, locks);
}
<Locks<T, I>>::insert(who, locks);
}

fn extend_lock(
Expand Down Expand Up @@ -1281,6 +1306,7 @@ where
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
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