From 773056616ff2f5ee47567623908a43b7d972e337 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 4 Jun 2021 01:55:05 -0400 Subject: [PATCH 1/6] transfer_all --- frame/balances/src/lib.rs | 20 +++++++++++++++++++ frame/balances/src/tests.rs | 40 +++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 04dacc7858646..b964ae4cc3487 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -362,6 +362,26 @@ pub mod pallet { >::transfer(&transactor, &dest, value, KeepAlive)?; Ok(().into()) } + + /// Transfer the entire transferable balance from the caller account. + /// + /// # + /// - O(1). Just like transfer, but reading the user's transferable balance first. + /// # + #[pallet::weight(T::WeightInfo::transfer_keep_alive())] + pub fn transfer_all( + origin: OriginFor, + dest: ::Source, + keep_alive: bool, + ) -> DispatchResultWithPostInfo { + use fungible::Inspect; + let transactor = ensure_signed(origin)?; + let reducible_balance = Self::reducible_balance(&transactor, keep_alive); + let dest = T::Lookup::lookup(dest)?; + let keep_alive = if keep_alive { KeepAlive } else { AllowDeath }; + >::transfer(&transactor, &dest, reducible_balance, keep_alive.into())?; + Ok(().into()) + } } #[pallet::event] diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 38a49df37bdff..0fcb139735972 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -964,5 +964,45 @@ macro_rules! decl_tests { assert_eq!(Balances::total_balance(&2), 100); }); } + + #[test] + fn transfer_all_works() { + <$ext_builder>::default() + .existential_deposit(100) + .build() + .execute_with(|| { + // setup + assert_ok!(Balances::set_balance(Origin::root(), 1, 200, 0)); + assert_ok!(Balances::set_balance(Origin::root(), 2, 0, 0)); + // transfer all and allow death + assert_ok!(Balances::transfer_all(Some(1).into(), 2, false)); + assert_eq!(Balances::total_balance(&1), 0); + assert_eq!(Balances::total_balance(&2), 200); + + // setup + assert_ok!(Balances::set_balance(Origin::root(), 1, 200, 0)); + assert_ok!(Balances::set_balance(Origin::root(), 2, 0, 0)); + // transfer all and keep alive + assert_ok!(Balances::transfer_all(Some(1).into(), 2, true)); + assert_eq!(Balances::total_balance(&1), 100); + assert_eq!(Balances::total_balance(&2), 100); + + // setup + assert_ok!(Balances::set_balance(Origin::root(), 1, 200, 10)); + assert_ok!(Balances::set_balance(Origin::root(), 2, 0, 0)); + // transfer all and allow death w/ reserved + assert_ok!(Balances::transfer_all(Some(1).into(), 2, false)); + assert_eq!(Balances::total_balance(&1), 0); + assert_eq!(Balances::total_balance(&2), 200); + + // setup + assert_ok!(Balances::set_balance(Origin::root(), 1, 200, 10)); + assert_ok!(Balances::set_balance(Origin::root(), 2, 0, 0)); + // transfer all and keep alive w/ reserved + assert_ok!(Balances::transfer_all(Some(1).into(), 2, true)); + assert_eq!(Balances::total_balance(&1), 100); + assert_eq!(Balances::total_balance(&2), 110); + }); + } } } From a1f71f3cac8f2c3a795ea0cf3daf9f9e7f1c38fa Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 4 Jun 2021 02:11:42 -0400 Subject: [PATCH 2/6] benchmark --- frame/balances/src/benchmarking.rs | 16 ++++++++++++++++ frame/balances/src/lib.rs | 2 +- frame/balances/src/weights.rs | 11 +++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/frame/balances/src/benchmarking.rs b/frame/balances/src/benchmarking.rs index f89775146b136..201c9d2755b82 100644 --- a/frame/balances/src/benchmarking.rs +++ b/frame/balances/src/benchmarking.rs @@ -176,6 +176,22 @@ benchmarks_instance_pallet! { assert_eq!(Balances::::free_balance(&caller), Zero::zero()); assert_eq!(Balances::::free_balance(&recipient), transfer_amount); } + + // Benchmark `transfer_all` with the worst possible condition: + // * The recipient account is created + // * The sender is killed + transfer_all { + let caller = whitelisted_caller(); + let recipient: T::AccountId = account("recipient", 0, SEED); + let recipient_lookup: ::Source = T::Lookup::unlookup(recipient.clone()); + + // Give the sender account max funds, thus a transfer will not kill account. + let _ = as Currency<_>>::make_free_balance_be(&caller, T::Balance::max_value()); + }: _(RawOrigin::Signed(caller.clone()), recipient_lookup, false) + verify { + assert!(Balances::::free_balance(&caller).is_zero()); + assert_eq!(Balances::::free_balance(&recipient), T::Balance::max_value()); + } } impl_benchmark_test_suite!( diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index b964ae4cc3487..6fd02f6970bac 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -368,7 +368,7 @@ pub mod pallet { /// # /// - O(1). Just like transfer, but reading the user's transferable balance first. /// # - #[pallet::weight(T::WeightInfo::transfer_keep_alive())] + #[pallet::weight(T::WeightInfo::transfer_all())] pub fn transfer_all( origin: OriginFor, dest: ::Source, diff --git a/frame/balances/src/weights.rs b/frame/balances/src/weights.rs index 5f3cf2b6bd9a9..7f05f5269a254 100644 --- a/frame/balances/src/weights.rs +++ b/frame/balances/src/weights.rs @@ -49,6 +49,7 @@ pub trait WeightInfo { fn set_balance_creating() -> Weight; fn set_balance_killing() -> Weight; fn force_transfer() -> Weight; + fn transfer_all() -> Weight; } /// Weights for pallet_balances using the Substrate node and recommended hardware. @@ -64,6 +65,11 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } + fn transfer_all() -> Weight { + (61_075_000 as Weight) + .saturating_add(T::DbWeight::get().reads(1 as Weight)) + .saturating_add(T::DbWeight::get().writes(1 as Weight)) + } fn set_balance_creating() -> Weight { (32_255_000 as Weight) .saturating_add(T::DbWeight::get().reads(1 as Weight)) @@ -88,6 +94,11 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } + fn transfer_all() -> Weight { + (81_909_000 as Weight) + .saturating_add(RocksDbWeight::get().reads(1 as Weight)) + .saturating_add(RocksDbWeight::get().writes(1 as Weight)) + } fn transfer_keep_alive() -> Weight { (61_075_000 as Weight) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) From 37f80dc301d176066b7da015cb4367a3c7159fe5 Mon Sep 17 00:00:00 2001 From: Parity Bot Date: Fri, 4 Jun 2021 07:57:38 +0000 Subject: [PATCH 3/6] cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs --- frame/balances/src/weights.rs | 42 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/frame/balances/src/weights.rs b/frame/balances/src/weights.rs index 7f05f5269a254..cf1d7dff82848 100644 --- a/frame/balances/src/weights.rs +++ b/frame/balances/src/weights.rs @@ -18,7 +18,7 @@ //! Autogenerated weights for pallet_balances //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 3.0.0 -//! DATE: 2021-04-08, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2021-06-04, STEPS: `[50, ]`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]` //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128 // Executed Command: @@ -56,67 +56,67 @@ pub trait WeightInfo { pub struct SubstrateWeight(PhantomData); impl WeightInfo for SubstrateWeight { fn transfer() -> Weight { - (81_909_000 as Weight) + (91_896_000 as Weight) .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } fn transfer_keep_alive() -> Weight { - (61_075_000 as Weight) - .saturating_add(T::DbWeight::get().reads(1 as Weight)) - .saturating_add(T::DbWeight::get().writes(1 as Weight)) - } - fn transfer_all() -> Weight { - (61_075_000 as Weight) + (67_779_000 as Weight) .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } fn set_balance_creating() -> Weight { - (32_255_000 as Weight) + (36_912_000 as Weight) .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } fn set_balance_killing() -> Weight { - (38_513_000 as Weight) + (44_416_000 as Weight) .saturating_add(T::DbWeight::get().reads(1 as Weight)) .saturating_add(T::DbWeight::get().writes(1 as Weight)) } fn force_transfer() -> Weight { - (80_448_000 as Weight) + (90_811_000 as Weight) .saturating_add(T::DbWeight::get().reads(2 as Weight)) .saturating_add(T::DbWeight::get().writes(2 as Weight)) } + fn transfer_all() -> Weight { + (84_170_000 as Weight) + .saturating_add(T::DbWeight::get().reads(1 as Weight)) + .saturating_add(T::DbWeight::get().writes(1 as Weight)) + } } // For backwards compatibility and tests impl WeightInfo for () { fn transfer() -> Weight { - (81_909_000 as Weight) - .saturating_add(RocksDbWeight::get().reads(1 as Weight)) - .saturating_add(RocksDbWeight::get().writes(1 as Weight)) - } - fn transfer_all() -> Weight { - (81_909_000 as Weight) + (91_896_000 as Weight) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } fn transfer_keep_alive() -> Weight { - (61_075_000 as Weight) + (67_779_000 as Weight) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } fn set_balance_creating() -> Weight { - (32_255_000 as Weight) + (36_912_000 as Weight) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } fn set_balance_killing() -> Weight { - (38_513_000 as Weight) + (44_416_000 as Weight) .saturating_add(RocksDbWeight::get().reads(1 as Weight)) .saturating_add(RocksDbWeight::get().writes(1 as Weight)) } fn force_transfer() -> Weight { - (80_448_000 as Weight) + (90_811_000 as Weight) .saturating_add(RocksDbWeight::get().reads(2 as Weight)) .saturating_add(RocksDbWeight::get().writes(2 as Weight)) } + fn transfer_all() -> Weight { + (84_170_000 as Weight) + .saturating_add(RocksDbWeight::get().reads(1 as Weight)) + .saturating_add(RocksDbWeight::get().writes(1 as Weight)) + } } From 882ab1eaf0a9c2afbd6a2bfc91dad0e46d7c6c2d Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 4 Jun 2021 09:44:05 -0400 Subject: [PATCH 4/6] update --- frame/balances/src/benchmarking.rs | 14 ++++++++------ frame/balances/src/lib.rs | 13 ++++++++++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/frame/balances/src/benchmarking.rs b/frame/balances/src/benchmarking.rs index 201c9d2755b82..688bcbc262bdb 100644 --- a/frame/balances/src/benchmarking.rs +++ b/frame/balances/src/benchmarking.rs @@ -40,7 +40,7 @@ benchmarks_instance_pallet! { let existential_deposit = T::ExistentialDeposit::get(); let caller = whitelisted_caller(); - // Give some multiple of the existential deposit + creation fee + transfer fee + // Give some multiple of the existential deposit let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); let _ = as Currency<_>>::make_free_balance_be(&caller, balance); @@ -130,7 +130,7 @@ benchmarks_instance_pallet! { let source: T::AccountId = account("source", 0, SEED); let source_lookup: ::Source = T::Lookup::unlookup(source.clone()); - // Give some multiple of the existential deposit + creation fee + transfer fee + // Give some multiple of the existential deposit let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); let _ = as Currency<_>>::make_free_balance_be(&source, balance); @@ -154,7 +154,7 @@ benchmarks_instance_pallet! { let existential_deposit = T::ExistentialDeposit::get(); let caller = whitelisted_caller(); - // Give some multiple of the existential deposit + creation fee + transfer fee + // Give some multiple of the existential deposit let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); let _ = as Currency<_>>::make_free_balance_be(&caller, balance); @@ -185,12 +185,14 @@ benchmarks_instance_pallet! { let recipient: T::AccountId = account("recipient", 0, SEED); let recipient_lookup: ::Source = T::Lookup::unlookup(recipient.clone()); - // Give the sender account max funds, thus a transfer will not kill account. - let _ = as Currency<_>>::make_free_balance_be(&caller, T::Balance::max_value()); + // Give some multiple of the existential deposit + let existential_deposit = T::ExistentialDeposit::get(); + let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); + let _ = as Currency<_>>::make_free_balance_be(&caller, balance); }: _(RawOrigin::Signed(caller.clone()), recipient_lookup, false) verify { assert!(Balances::::free_balance(&caller).is_zero()); - assert_eq!(Balances::::free_balance(&recipient), T::Balance::max_value()); + assert_eq!(Balances::::free_balance(&recipient), balance); } } diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 099c71f1ce1c9..160b4d2a45b34 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -372,9 +372,16 @@ pub mod pallet { /// Transfer the entire transferable balance from the caller account. /// - /// # + /// The dispatch origin of this call must be Signed. + /// + /// - `dest`: The recipient of the transfer. + /// - `keep_alive`: A boolean to determine if the `transfer_all` operation should send all + /// of the funds the account has, causing the sender account to be killed (false), or + /// transfer everything except at least the existential deposit, which will guarantee to + /// keep the sender account alive (true). + /// # /// - O(1). Just like transfer, but reading the user's transferable balance first. - /// # + /// # #[pallet::weight(T::WeightInfo::transfer_all())] pub fn transfer_all( origin: OriginFor, @@ -1716,7 +1723,7 @@ impl, I: 'static> NamedReservableCurrency for Pallet< /// Is a no-op if value to be reserved is zero. fn reserve_named(id: &Self::ReserveIdentifier, who: &T::AccountId, value: Self::Balance) -> DispatchResult { if value.is_zero() { return Ok(()) } - + Reserves::::try_mutate(who, |reserves| -> DispatchResult { match reserves.binary_search_by_key(id, |data| data.id) { Ok(index) => { From 928825e6eb8dc6a0eee47e22313f7a9d19d4e4d9 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 5 Jun 2021 17:55:31 -0400 Subject: [PATCH 5/6] add note --- frame/balances/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 160b4d2a45b34..e41f3f2085e7c 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -372,6 +372,12 @@ pub mod pallet { /// Transfer the entire transferable balance from the caller account. /// + /// NOTE: This function is only attempt to transfer _transferable_ balances. This means that + /// any locked, reserved, or existential deposits (when `keep_alive` is `true`), will not be + /// transferred by this function. To ensure that this function results in a killed account, + /// you might need to prepare the account by removing any reference counters, storage + /// deposits, etc... + /// /// The dispatch origin of this call must be Signed. /// /// - `dest`: The recipient of the transfer. From 6e59809f89772fdb133a924ef1cba54c63f3228f Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 8 Jun 2021 14:58:02 +0200 Subject: [PATCH 6/6] typo --- frame/balances/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index e41f3f2085e7c..105c5d08a659c 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -372,7 +372,7 @@ pub mod pallet { /// Transfer the entire transferable balance from the caller account. /// - /// NOTE: This function is only attempt to transfer _transferable_ balances. This means that + /// NOTE: This function only attempts to transfer _transferable_ balances. This means that /// any locked, reserved, or existential deposits (when `keep_alive` is `true`), will not be /// transferred by this function. To ensure that this function results in a killed account, /// you might need to prepare the account by removing any reference counters, storage