Skip to content

Commit

Permalink
Add support for feature pallet_balances/insecure_zero_ed in benchma…
Browse files Browse the repository at this point in the history
…rks and testing (#7379)

# Description

Currently benchmarks and tests on pallet_balances would fail when the
feature insecure_zero_ed is enabled. This PR allows to run such
benchmark and tests keeping into account the fact that accounts would
not be deleted when their balance goes below a threshold.

## Integration

*In depth notes about how this PR should be integrated by downstream
projects. This part is mandatory, and should be
reviewed by reviewers, if the PR does NOT have the `R0-Silent` label. In
case of a `R0-Silent`, it can be ignored.*

## Review Notes

*In depth notes about the **implementation** details of your PR. This
should be the main guide for reviewers to
understand your approach and effectively review it. If too long, use

[`<details>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details)*.

*Imagine that someone who is depending on the old code wants to
integrate your new code and the only information that
they get is this section. It helps to include example usage and default
value here, with a `diff` code-block to show
possibly integration.*

*Include your leftover TODOs, if any, here.*

# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [x] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [x] I have made corresponding changes to the documentation (if
applicable)
* [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

You can remove the "Checklist" section once all have been checked. Thank
you for your contribution!

✄
-----------------------------------------------------------------------------

---------

Co-authored-by: Rodrigo Quelhas <[email protected]>
  • Loading branch information
manuelmauro and RomarQ authored Jan 29, 2025
1 parent 1d05b9a commit 80e30ec
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 7 deletions.
13 changes: 13 additions & 0 deletions prdoc/pr_7379.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "Add support for feature pallet_balances/insecure_zero_ed in benchmarks and testing"

doc:
- audience: Runtime Dev
description: |
Currently benchmarks and tests on pallet_balances would fail when the feature insecure_zero_ed is enabled. This PR allows to run such benchmark and tests keeping into account the fact that accounts would not be deleted when their balance goes below a threshold.

crates:
- name: pallet-balances
bump: patch
31 changes: 25 additions & 6 deletions substrate/frame/balances/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@ mod benchmarks {
#[extrinsic_call]
_(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount);

assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&caller), balance - transfer_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
}

assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
}

Expand Down Expand Up @@ -173,7 +178,12 @@ mod benchmarks {
#[extrinsic_call]
_(RawOrigin::Root, source_lookup, recipient_lookup, transfer_amount);

assert_eq!(Balances::<T, I>::free_balance(&source), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&source), balance - transfer_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&source), Zero::zero());
}

assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
}

Expand Down Expand Up @@ -208,7 +218,12 @@ mod benchmarks {
#[extrinsic_call]
transfer_allow_death(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount);

assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&caller), balance - transfer_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
}

assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
}

Expand Down Expand Up @@ -308,7 +323,7 @@ mod benchmarks {
/// Benchmark `burn` extrinsic with the worst possible condition - burn kills the account.
#[benchmark]
fn burn_allow_death() {
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let caller = whitelisted_caller();

// Give some multiple of the existential deposit
Expand All @@ -321,13 +336,17 @@ mod benchmarks {
#[extrinsic_call]
burn(RawOrigin::Signed(caller.clone()), burn_amount, false);

assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&caller), balance - burn_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
}
}

// Benchmark `burn` extrinsic with the case where account is kept alive.
#[benchmark]
fn burn_keep_alive() {
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let caller = whitelisted_caller();

// Give some multiple of the existential deposit
Expand Down
4 changes: 3 additions & 1 deletion substrate/frame/balances/src/tests/currency_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use frame_support::{
BalanceStatus::{Free, Reserved},
Currency,
ExistenceRequirement::{self, AllowDeath, KeepAlive},
Hooks, InspectLockableCurrency, LockIdentifier, LockableCurrency, NamedReservableCurrency,
InspectLockableCurrency, LockIdentifier, LockableCurrency, NamedReservableCurrency,
ReservableCurrency, WithdrawReasons,
},
StorageNoopGuard,
Expand Down Expand Up @@ -1136,7 +1136,9 @@ fn operations_on_dead_account_should_not_change_state() {

#[test]
#[should_panic = "The existential deposit must be greater than zero!"]
#[cfg(not(feature = "insecure_zero_ed"))]
fn zero_ed_is_prohibited() {
use frame_support::traits::Hooks;
// These functions all use `mutate_account` which may introduce a storage change when
// the account never existed to begin with, and shouldn't exist in the end.
ExtBuilder::default().existential_deposit(0).build_and_execute_with(|| {
Expand Down

0 comments on commit 80e30ec

Please sign in to comment.