From 6c2e95ec94282e4da3e3dae66d0f1334916586d6 Mon Sep 17 00:00:00 2001 From: Parth Mittal Date: Wed, 3 Jul 2024 18:52:39 +0530 Subject: [PATCH 1/9] Balances Pallet: Emit events when TI is updated in currency impl --- substrate/frame/balances/src/impl_currency.rs | 15 ++++++++-- .../balances/src/tests/currency_tests.rs | 29 +++++++++++++------ .../balances/src/tests/reentrancy_tests.rs | 7 +++-- 3 files changed, 37 insertions(+), 14 deletions(-) diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index d5fe9934e239..ea2165cd8d3a 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -34,11 +34,12 @@ use frame_support::{ }; use frame_system::pallet_prelude::BlockNumberFor; pub use imbalances::{NegativeImbalance, PositiveImbalance}; +use sp_runtime::traits::Bounded; // wrapping these imbalances in a private module is necessary to ensure absolute privacy // of the inner member. mod imbalances { - use super::{result, Config, Imbalance, RuntimeDebug, Saturating, TryDrop, Zero}; + use super::*; use frame_support::traits::SameOrOther; use sp_std::mem; @@ -200,6 +201,9 @@ mod imbalances { /// Basic drop handler will just square up the total issuance. fn drop(&mut self) { >::mutate(|v| *v = v.saturating_add(self.0)); + if !self.0.is_zero() { + Pallet::::deposit_event(Event::::Issued { amount: self.0 }); + } } } @@ -207,6 +211,9 @@ mod imbalances { /// Basic drop handler will just square up the total issuance. fn drop(&mut self) { >::mutate(|v| *v = v.saturating_sub(self.0)); + if !self.0.is_zero() { + Pallet::::deposit_event(Event::::Rescinded { amount: self.0 }); + } } } } @@ -263,6 +270,8 @@ where Zero::zero() }); }); + + Pallet::::deposit_event(Event::::Rescinded { amount }); PositiveImbalance::new(amount) } @@ -279,6 +288,8 @@ where Self::Balance::max_value() }) }); + + Pallet::::deposit_event(Event::::Issued { amount }); NegativeImbalance::new(amount) } @@ -349,7 +360,7 @@ where }; account.free.saturating_reduce(actual); let remaining = value.saturating_sub(actual); - Ok((NegativeImbalance::new(actual), remaining)) + Ok((NegativeImbalance::new(dbg!(actual)), remaining)) }, ) { Ok((imbalance, remaining)) => { diff --git a/substrate/frame/balances/src/tests/currency_tests.rs b/substrate/frame/balances/src/tests/currency_tests.rs index 9ad4aca64406..c199b418c5c6 100644 --- a/substrate/frame/balances/src/tests/currency_tests.rs +++ b/substrate/frame/balances/src/tests/currency_tests.rs @@ -399,10 +399,16 @@ fn reward_should_work() { ExtBuilder::default().monied(true).build_and_execute_with(|| { assert_eq!(Balances::total_balance(&1), 10); assert_ok!(Balances::deposit_into_existing(&1, 10).map(drop)); - System::assert_last_event(RuntimeEvent::Balances(crate::Event::Deposit { - who: 1, - amount: 10, - })); + assert_eq!( + events(), + [ + RuntimeEvent::Balances(crate::Event::Deposit { + who: 1, + amount: 10, + }), + RuntimeEvent::Balances(crate::Event::Issued { amount: 10 }), + ] + ); assert_eq!(Balances::total_balance(&1), 20); assert_eq!(Balances::total_issuance(), 120); }); @@ -480,7 +486,7 @@ fn withdrawing_balance_should_work() { let _ = Balances::deposit_creating(&2, 111); let _ = Balances::withdraw(&2, 11, WithdrawReasons::TRANSFER, ExistenceRequirement::KeepAlive); - System::assert_last_event(RuntimeEvent::Balances(crate::Event::Withdraw { + System::assert_has_event(RuntimeEvent::Balances(crate::Event::Withdraw { who: 2, amount: 11, })); @@ -889,6 +895,7 @@ fn emit_events_with_existential_deposit() { [ RuntimeEvent::System(system::Event::NewAccount { account: 1 }), RuntimeEvent::Balances(crate::Event::Endowed { account: 1, free_balance: 100 }), + RuntimeEvent::Balances(crate::Event::Issued { amount: 100 }), RuntimeEvent::Balances(crate::Event::BalanceSet { who: 1, free: 100 }), ] ); @@ -902,6 +909,7 @@ fn emit_events_with_existential_deposit() { RuntimeEvent::System(system::Event::KilledAccount { account: 1 }), RuntimeEvent::Balances(crate::Event::DustLost { account: 1, amount: 99 }), RuntimeEvent::Balances(crate::Event::Slashed { who: 1, amount: 1 }), + RuntimeEvent::Balances(crate::Event::Rescinded { amount: 1 }), ] ); }); @@ -918,6 +926,7 @@ fn emit_events_with_no_existential_deposit_suicide() { RuntimeEvent::Balances(crate::Event::BalanceSet { who: 1, free: 100 }), RuntimeEvent::System(system::Event::NewAccount { account: 1 }), RuntimeEvent::Balances(crate::Event::Endowed { account: 1, free_balance: 100 }), + RuntimeEvent::Balances(crate::Event::Issued { amount: 100 }), ] ); @@ -929,6 +938,7 @@ fn emit_events_with_no_existential_deposit_suicide() { [ RuntimeEvent::System(system::Event::KilledAccount { account: 1 }), RuntimeEvent::Balances(crate::Event::Slashed { who: 1, amount: 100 }), + RuntimeEvent::Balances(crate::Event::Rescinded { amount: 100 }), ] ); }); @@ -954,7 +964,7 @@ fn slash_full_works() { assert_eq!(Balances::slash(&1, 1_000), (NegativeImbalance::new(1000), 0)); // Account is still alive assert!(!System::account_exists(&1)); - System::assert_last_event(RuntimeEvent::Balances(crate::Event::Slashed { + System::assert_has_event(RuntimeEvent::Balances(crate::Event::Slashed { who: 1, amount: 1000, })); @@ -969,7 +979,7 @@ fn slash_partial_works() { assert_eq!(Balances::slash(&1, 900), (NegativeImbalance::new(900), 0)); // Account is still alive assert!(System::account_exists(&1)); - System::assert_last_event(RuntimeEvent::Balances(crate::Event::Slashed { + System::assert_has_event(RuntimeEvent::Balances(crate::Event::Slashed { who: 1, amount: 900, })); @@ -983,7 +993,7 @@ fn slash_dusting_works() { // Slashed completed in full assert_eq!(Balances::slash(&1, 950), (NegativeImbalance::new(950), 0)); assert!(!System::account_exists(&1)); - System::assert_last_event(RuntimeEvent::Balances(crate::Event::Slashed { + System::assert_has_event(RuntimeEvent::Balances(crate::Event::Slashed { who: 1, amount: 950, })); @@ -998,7 +1008,7 @@ fn slash_does_not_take_from_reserve() { // Slashed completed in full assert_eq!(Balances::slash(&1, 900), (NegativeImbalance::new(800), 100)); assert_eq!(Balances::reserved_balance(&1), 100); - System::assert_last_event(RuntimeEvent::Balances(crate::Event::Slashed { + System::assert_has_event(RuntimeEvent::Balances(crate::Event::Slashed { who: 1, amount: 800, })); @@ -1399,6 +1409,7 @@ fn self_transfer_noop() { Event::Deposit { who: 1, amount: 100 }.into(), SysEvent::NewAccount { account: 1 }.into(), Event::Endowed { account: 1, free_balance: 100 }.into(), + Event::Issued { amount: 100 }.into(), ] ); assert_eq!(Balances::free_balance(1), 100); diff --git a/substrate/frame/balances/src/tests/reentrancy_tests.rs b/substrate/frame/balances/src/tests/reentrancy_tests.rs index 717f04978577..0b66f2c93e3a 100644 --- a/substrate/frame/balances/src/tests/reentrancy_tests.rs +++ b/substrate/frame/balances/src/tests/reentrancy_tests.rs @@ -52,7 +52,7 @@ fn transfer_dust_removal_tst1_should_work() { assert_eq!(Balances::free_balance(&1), 1050); // Verify the events - assert_eq!(System::events().len(), 12); + assert_eq!(System::events().len(), 14); System::assert_has_event(RuntimeEvent::Balances(crate::Event::Transfer { from: 2, @@ -93,7 +93,7 @@ fn transfer_dust_removal_tst2_should_work() { assert_eq!(Balances::free_balance(&1), 1500); // Verify the events - assert_eq!(System::events().len(), 10); + assert_eq!(System::events().len(), 12); System::assert_has_event(RuntimeEvent::Balances(crate::Event::Transfer { from: 2, @@ -139,7 +139,7 @@ fn repatriating_reserved_balance_dust_removal_should_work() { assert_eq!(Balances::free_balance(1), 1500); // Verify the events - assert_eq!(System::events().len(), 10); + assert_eq!(System::events().len(), 12); System::assert_has_event(RuntimeEvent::Balances(crate::Event::Transfer { from: 2, @@ -167,6 +167,7 @@ fn emit_events_with_no_existential_deposit_suicide_with_dust() { [ RuntimeEvent::System(system::Event::NewAccount { account: 1 }), RuntimeEvent::Balances(crate::Event::Endowed { account: 1, free_balance: 100 }), + RuntimeEvent::Balances(crate::Event::Issued { amount: 100 }), RuntimeEvent::Balances(crate::Event::BalanceSet { who: 1, free: 100 }), ] ); From 558f519eed7b604b30fc202a06b8d0dc1aaa08f6 Mon Sep 17 00:00:00 2001 From: Parth Mittal Date: Wed, 3 Jul 2024 19:27:30 +0530 Subject: [PATCH 2/9] Add PR doc --- prdoc/pr_4936.prdoc | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 prdoc/pr_4936.prdoc diff --git a/prdoc/pr_4936.prdoc b/prdoc/pr_4936.prdoc new file mode 100644 index 000000000000..0f17f1fb8975 --- /dev/null +++ b/prdoc/pr_4936.prdoc @@ -0,0 +1,11 @@ +# 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: Balances Pallet: Emit events when TI is updated in currency impl + +doc: + - audience: Runtime Dev + description: | + Previously, in the Currency impl, the implementation of pallet_balances was not emitting any instances of Issued and Rescinded events, even though the Fungible equivalent was. This PR adds the Issued and Rescinded events in appropriate places in impl_currency along with tests. + +crates: [ ] From 0f0c3263c3389fc9ac846351cbdbf0eb4f91df04 Mon Sep 17 00:00:00 2001 From: Parth Mittal Date: Wed, 3 Jul 2024 22:09:40 +0530 Subject: [PATCH 3/9] Remove dbg and improve prdoc --- prdoc/pr_4936.prdoc | 6 ++++-- substrate/frame/balances/src/impl_currency.rs | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/prdoc/pr_4936.prdoc b/prdoc/pr_4936.prdoc index 0f17f1fb8975..f9b7ee506a7a 100644 --- a/prdoc/pr_4936.prdoc +++ b/prdoc/pr_4936.prdoc @@ -1,11 +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: Balances Pallet: Emit events when TI is updated in currency impl +title: "Balances Pallet: Emit events when TI is updated in currency impl" doc: - audience: Runtime Dev description: | Previously, in the Currency impl, the implementation of pallet_balances was not emitting any instances of Issued and Rescinded events, even though the Fungible equivalent was. This PR adds the Issued and Rescinded events in appropriate places in impl_currency along with tests. -crates: [ ] +crates: +- name: pallet-balances + bump: patch diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index ea2165cd8d3a..218ef5a38068 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -360,7 +360,7 @@ where }; account.free.saturating_reduce(actual); let remaining = value.saturating_sub(actual); - Ok((NegativeImbalance::new(dbg!(actual)), remaining)) + Ok((NegativeImbalance::new(actual), remaining)) }, ) { Ok((imbalance, remaining)) => { From 84a6e9a78d355170f67124bbe2a07b56277965b1 Mon Sep 17 00:00:00 2001 From: Parth Mittal Date: Wed, 3 Jul 2024 23:08:53 +0530 Subject: [PATCH 4/9] Run cargo fmt and clippy --- substrate/frame/balances/src/lib.rs | 2 +- substrate/frame/balances/src/tests/currency_tests.rs | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index d01884293c09..f67120efe764 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/substrate/frame/balances/src/lib.rs @@ -171,7 +171,7 @@ pub use impl_currency::{NegativeImbalance, PositiveImbalance}; use scale_info::TypeInfo; use sp_runtime::{ traits::{ - AtLeast32BitUnsigned, Bounded, CheckedAdd, CheckedSub, MaybeSerializeDeserialize, + AtLeast32BitUnsigned, CheckedAdd, CheckedSub, MaybeSerializeDeserialize, Saturating, StaticLookup, Zero, }, ArithmeticError, DispatchError, FixedPointOperand, Perbill, RuntimeDebug, TokenError, diff --git a/substrate/frame/balances/src/tests/currency_tests.rs b/substrate/frame/balances/src/tests/currency_tests.rs index c199b418c5c6..fb69368a6216 100644 --- a/substrate/frame/balances/src/tests/currency_tests.rs +++ b/substrate/frame/balances/src/tests/currency_tests.rs @@ -402,10 +402,7 @@ fn reward_should_work() { assert_eq!( events(), [ - RuntimeEvent::Balances(crate::Event::Deposit { - who: 1, - amount: 10, - }), + RuntimeEvent::Balances(crate::Event::Deposit { who: 1, amount: 10 }), RuntimeEvent::Balances(crate::Event::Issued { amount: 10 }), ] ); From a8f9b26edf18bacc37e2de38311d7bbd3a38b6e2 Mon Sep 17 00:00:00 2001 From: Parth Mittal Date: Thu, 4 Jul 2024 10:45:50 +0530 Subject: [PATCH 5/9] Run cargo fmt --- substrate/frame/balances/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index f67120efe764..300c518cd21b 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/substrate/frame/balances/src/lib.rs @@ -171,8 +171,8 @@ pub use impl_currency::{NegativeImbalance, PositiveImbalance}; use scale_info::TypeInfo; use sp_runtime::{ traits::{ - AtLeast32BitUnsigned, CheckedAdd, CheckedSub, MaybeSerializeDeserialize, - Saturating, StaticLookup, Zero, + AtLeast32BitUnsigned, CheckedAdd, CheckedSub, MaybeSerializeDeserialize, Saturating, + StaticLookup, Zero, }, ArithmeticError, DispatchError, FixedPointOperand, Perbill, RuntimeDebug, TokenError, }; From 891c7289d000e9dc944289098fc952ccee35f86b Mon Sep 17 00:00:00 2001 From: Parth Mittal Date: Fri, 5 Jul 2024 19:01:42 +0530 Subject: [PATCH 6/9] tests: Adjust assertions for addeed Issued and Rescinded events --- substrate/bin/node/bench/src/import.rs | 7 ++++--- substrate/bin/node/cli/tests/basic.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/substrate/bin/node/bench/src/import.rs b/substrate/bin/node/bench/src/import.rs index e340869dea02..0b972650ef5a 100644 --- a/substrate/bin/node/bench/src/import.rs +++ b/substrate/bin/node/bench/src/import.rs @@ -121,22 +121,23 @@ impl core::Benchmark for ImportBenchmark { .inspect_state(|| { match self.block_type { BlockType::RandomTransfersKeepAlive => { - // should be 8 per signed extrinsic + 1 per unsigned + // should be 9 per signed extrinsic + 1 per unsigned // we have 2 unsigned (timestamp and glutton bloat) while the rest are // signed in the block. - // those 8 events per signed are: + // those 9 events per signed are: // - transaction paid for the transaction payment // - withdraw (Balances::Withdraw) for charging the transaction fee // - new account (System::NewAccount) as we always transfer fund to // non-existent account // - endowed (Balances::Endowed) for this new account + // - issued (Balances::Issued) as total issuance is increased // - successful transfer (Event::Transfer) for this transfer operation // - 2x deposit (Balances::Deposit and Treasury::Deposit) for depositing // the transaction fee into the treasury // - extrinsic success assert_eq!( kitchensink_runtime::System::events().len(), - (self.block.extrinsics.len() - 2) * 8 + 2, + (self.block.extrinsics.len() - 2) * 9 + 2, ); }, BlockType::Noop => { diff --git a/substrate/bin/node/cli/tests/basic.rs b/substrate/bin/node/cli/tests/basic.rs index b1f737ce399b..6d5e4c7a5757 100644 --- a/substrate/bin/node/cli/tests/basic.rs +++ b/substrate/bin/node/cli/tests/basic.rs @@ -380,6 +380,13 @@ fn full_native_block_import_works() { }), topics: vec![], }, + EventRecord { + phase: Phase::ApplyExtrinsic(1), + event: RuntimeEvent::Balances(pallet_balances::Event::Rescinded { + amount: fees * 2 / 10, + }), + topics: vec![], + }, EventRecord { phase: Phase::ApplyExtrinsic(1), event: RuntimeEvent::TransactionPayment( @@ -465,6 +472,13 @@ fn full_native_block_import_works() { }), topics: vec![], }, + EventRecord { + phase: Phase::ApplyExtrinsic(1), + event: RuntimeEvent::Balances(pallet_balances::Event::Rescinded { + amount: fees * 2 / 10, + }), + topics: vec![], + }, EventRecord { phase: Phase::ApplyExtrinsic(1), event: RuntimeEvent::TransactionPayment( From dddc2861a66941fb44cdb82e269f8dddd0891f56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 19 Jul 2024 17:08:03 +0200 Subject: [PATCH 7/9] Update substrate/frame/balances/src/impl_currency.rs --- substrate/frame/balances/src/impl_currency.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index ba0e9d481372..5965b2e80cda 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -210,8 +210,8 @@ mod imbalances { impl, I: 'static> Drop for NegativeImbalance { /// Basic drop handler will just square up the total issuance. fn drop(&mut self) { - >::mutate(|v| *v = v.saturating_sub(self.0)); if !self.0.is_zero() { + >::mutate(|v| *v = v.saturating_sub(self.0)); Pallet::::deposit_event(Event::::Rescinded { amount: self.0 }); } } From c9bce101e31fd5913658cee47557400c3215ab23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 19 Jul 2024 17:08:10 +0200 Subject: [PATCH 8/9] Update substrate/frame/balances/src/impl_currency.rs --- substrate/frame/balances/src/impl_currency.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/balances/src/impl_currency.rs b/substrate/frame/balances/src/impl_currency.rs index 5965b2e80cda..049f0cc626c2 100644 --- a/substrate/frame/balances/src/impl_currency.rs +++ b/substrate/frame/balances/src/impl_currency.rs @@ -200,8 +200,8 @@ mod imbalances { impl, I: 'static> Drop for PositiveImbalance { /// Basic drop handler will just square up the total issuance. fn drop(&mut self) { - >::mutate(|v| *v = v.saturating_add(self.0)); if !self.0.is_zero() { + >::mutate(|v| *v = v.saturating_add(self.0)); Pallet::::deposit_event(Event::::Issued { amount: self.0 }); } } From eb8454ffbb615f3efbfd39168a3daa6e839d6320 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 20 Jul 2024 00:34:52 +0200 Subject: [PATCH 9/9] Fix test --- Cargo.lock | 1 + substrate/bin/node/cli/Cargo.toml | 1 + substrate/bin/node/cli/tests/basic.rs | 10 +++++++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a424738685e7..6716c98b5d1c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -20836,6 +20836,7 @@ dependencies = [ "parity-scale-codec", "platforms", "polkadot-sdk", + "pretty_assertions", "rand", "regex", "sc-service-test", diff --git a/substrate/bin/node/cli/Cargo.toml b/substrate/bin/node/cli/Cargo.toml index ab665f0792a4..1856487ac9ca 100644 --- a/substrate/bin/node/cli/Cargo.toml +++ b/substrate/bin/node/cli/Cargo.toml @@ -71,6 +71,7 @@ wait-timeout = { workspace = true } wat = { workspace = true } serde_json = { workspace = true, default-features = true } scale-info = { features = ["derive", "serde"], workspace = true, default-features = true } +pretty_assertions.workspace = true # These testing-only dependencies are not exported by the Polkadot-SDK crate: node-testing = { workspace = true } diff --git a/substrate/bin/node/cli/tests/basic.rs b/substrate/bin/node/cli/tests/basic.rs index 6d5e4c7a5757..0a2e3fd25047 100644 --- a/substrate/bin/node/cli/tests/basic.rs +++ b/substrate/bin/node/cli/tests/basic.rs @@ -35,6 +35,7 @@ use kitchensink_runtime::{ }; use node_primitives::{Balance, Hash}; use node_testing::keyring::*; +use pretty_assertions::assert_eq; use wat; pub mod common; @@ -475,7 +476,7 @@ fn full_native_block_import_works() { EventRecord { phase: Phase::ApplyExtrinsic(1), event: RuntimeEvent::Balances(pallet_balances::Event::Rescinded { - amount: fees * 2 / 10, + amount: fees - fees * 8 / 10, }), topics: vec![], }, @@ -529,6 +530,13 @@ fn full_native_block_import_works() { }), topics: vec![], }, + EventRecord { + phase: Phase::ApplyExtrinsic(2), + event: RuntimeEvent::Balances(pallet_balances::Event::Rescinded { + amount: fees - fees * 8 / 10, + }), + topics: vec![], + }, EventRecord { phase: Phase::ApplyExtrinsic(2), event: RuntimeEvent::TransactionPayment(