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

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Jan 15, 2020

Fixes #4402

This PR adds a simple check before relevant balances functions that the "action to be performed" would result in a change.

For example: the function can_slash would normally query the free balance of a user and do a comparison function. However, if can_slash has value.is_zero(), we can simply return true without doing a storage read.

Further, a function like set_lock used to write zero amount locks, which we now skip. Same with add_vesting_schedule.

Ultimately, this PR also catches situations where some external logic, like the fee system, would try to do some action into the balances module with value zero. This may trigger unexpected side effects like emitting a ReapedAccount event when nothing changed in the system.

Please do not treat this PR as insubstantial. Would really want a second pair of eyes to confirm I have not broken any assumptions.

@@ -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.

@shawntabrizi shawntabrizi added the A0-please_review Pull request needs code review. label Jan 15, 2020
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

looks fine except for the comment. i would add notes into the docs of each function explaining that when amount is zero, the function is strictly a no-op.

@shawntabrizi shawntabrizi added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jan 15, 2020
@gavofyork gavofyork merged commit 33b3c23 into paritytech:master Jan 15, 2020
@shawntabrizi shawntabrizi deleted the shawntabrizi-patch-balances branch January 15, 2020 16:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReapedAcccount event is spammed when multiple transactions are placed in the same block
2 participants