From 80e30ec3cdccae8e9099bd67840ff8737b043496 Mon Sep 17 00:00:00 2001 From: Manuel Mauro Date: Wed, 29 Jan 2025 23:11:52 +0100 Subject: [PATCH] Add support for feature `pallet_balances/insecure_zero_ed` in benchmarks and testing (#7379) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # 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 [`
`](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 --- prdoc/pr_7379.prdoc | 13 ++++++++ substrate/frame/balances/src/benchmarking.rs | 31 +++++++++++++++---- .../balances/src/tests/currency_tests.rs | 4 ++- 3 files changed, 41 insertions(+), 7 deletions(-) create mode 100644 prdoc/pr_7379.prdoc diff --git a/prdoc/pr_7379.prdoc b/prdoc/pr_7379.prdoc new file mode 100644 index 000000000000..0bd904346d68 --- /dev/null +++ b/prdoc/pr_7379.prdoc @@ -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 diff --git a/substrate/frame/balances/src/benchmarking.rs b/substrate/frame/balances/src/benchmarking.rs index c825300218d4..a761f8e2af82 100644 --- a/substrate/frame/balances/src/benchmarking.rs +++ b/substrate/frame/balances/src/benchmarking.rs @@ -65,7 +65,12 @@ mod benchmarks { #[extrinsic_call] _(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount); - assert_eq!(Balances::::free_balance(&caller), Zero::zero()); + if cfg!(feature = "insecure_zero_ed") { + assert_eq!(Balances::::free_balance(&caller), balance - transfer_amount); + } else { + assert_eq!(Balances::::free_balance(&caller), Zero::zero()); + } + assert_eq!(Balances::::free_balance(&recipient), transfer_amount); } @@ -173,7 +178,12 @@ mod benchmarks { #[extrinsic_call] _(RawOrigin::Root, source_lookup, recipient_lookup, transfer_amount); - assert_eq!(Balances::::free_balance(&source), Zero::zero()); + if cfg!(feature = "insecure_zero_ed") { + assert_eq!(Balances::::free_balance(&source), balance - transfer_amount); + } else { + assert_eq!(Balances::::free_balance(&source), Zero::zero()); + } + assert_eq!(Balances::::free_balance(&recipient), transfer_amount); } @@ -208,7 +218,12 @@ mod benchmarks { #[extrinsic_call] transfer_allow_death(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount); - assert_eq!(Balances::::free_balance(&caller), Zero::zero()); + if cfg!(feature = "insecure_zero_ed") { + assert_eq!(Balances::::free_balance(&caller), balance - transfer_amount); + } else { + assert_eq!(Balances::::free_balance(&caller), Zero::zero()); + } + assert_eq!(Balances::::free_balance(&recipient), transfer_amount); } @@ -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::(); let caller = whitelisted_caller(); // Give some multiple of the existential deposit @@ -321,13 +336,17 @@ mod benchmarks { #[extrinsic_call] burn(RawOrigin::Signed(caller.clone()), burn_amount, false); - assert_eq!(Balances::::free_balance(&caller), Zero::zero()); + if cfg!(feature = "insecure_zero_ed") { + assert_eq!(Balances::::free_balance(&caller), balance - burn_amount); + } else { + assert_eq!(Balances::::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::(); let caller = whitelisted_caller(); // Give some multiple of the existential deposit diff --git a/substrate/frame/balances/src/tests/currency_tests.rs b/substrate/frame/balances/src/tests/currency_tests.rs index a6377c3ad72e..0e5d7ccb46de 100644 --- a/substrate/frame/balances/src/tests/currency_tests.rs +++ b/substrate/frame/balances/src/tests/currency_tests.rs @@ -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, @@ -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(|| {