Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pallet-balances] burn_allow_death extrinsic #3964

Merged
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
d1fadb2
Init commit
Dinonard Apr 3, 2024
e9c5ee5
Newline
Dinonard Apr 3, 2024
cc8641d
Modify interface
Dinonard Apr 3, 2024
fbfcb62
Polish
Dinonard Apr 3, 2024
f1db75e
Sort out missing weight functions
Dinonard Apr 3, 2024
0094a2c
CI fixes
Dinonard Apr 3, 2024
b8f4f48
More fixes
Dinonard Apr 3, 2024
d9493fe
Fix prdoc
Dinonard Apr 3, 2024
2c7c067
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard Apr 8, 2024
58268e4
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard Apr 12, 2024
56d3379
Update cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weight…
Dinonard Apr 30, 2024
45d191f
Update substrate/frame/balances/src/benchmarking.rs
Dinonard Apr 30, 2024
d8ade46
Review comments
Dinonard Apr 30, 2024
41d70ef
Merge remote-tracking branch 'origin/dinonard/pallet-balances-burn-ex…
Dinonard Apr 30, 2024
293ee0f
Formatting
Dinonard Apr 30, 2024
79db74f
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard Apr 30, 2024
1b48ad5
Comment
Dinonard Apr 30, 2024
9b6c3ce
Merge remote-tracking branch 'origin/dinonard/pallet-balances-burn-ex…
Dinonard Apr 30, 2024
6979ffa
Add Preservation to asset-conversion-ops
Dinonard Apr 30, 2024
21077a8
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
May 3, 2024
1617050
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 3, 2024
5657da1
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard May 3, 2024
3d48ae6
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard May 6, 2024
ac976b8
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
1a07ca2
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
May 6, 2024
976c936
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
ba662db
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
11a0475
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
eb29289
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
16f342a
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard May 6, 2024
a42db38
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
eb2f431
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
53e5112
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
fa0a300
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
051674d
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
27618af
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
f10a8eb
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
49b2ba7
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
d6d9936
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
851e629
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
190ca18
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_balances
May 6, 2024
be3e92f
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard May 7, 2024
24c6422
Merge branch 'master' into dinonard/pallet-balances-burn-extrinsics
Dinonard May 8, 2024
04f75af
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
May 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(Weight::from_parts(0, 1501))
.saturating_add(T::DbWeight::get().reads(1))
}
fn burn_allow_death() -> Weight {
// TODO
Dinonard marked this conversation as resolved.
Show resolved Hide resolved
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(Weight::from_parts(0, 1501))
.saturating_add(T::DbWeight::get().reads(1))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(Weight::from_parts(0, 1501))
.saturating_add(T::DbWeight::get().reads(1))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(Weight::from_parts(0, 1501))
.saturating_add(T::DbWeight::get().reads(1))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(Weight::from_parts(0, 1501))
.saturating_add(T::DbWeight::get().reads(1))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(Weight::from_parts(0, 1501))
.saturating_add(T::DbWeight::get().reads(1))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -161,4 +161,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(Weight::from_parts(0, 1501))
.saturating_add(T::DbWeight::get().reads(1))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(u.into())))
.saturating_add(Weight::from_parts(0, 2603).saturating_mul(u.into()))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
.saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(u.into())))
.saturating_add(Weight::from_parts(0, 2603).saturating_mul(u.into()))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
Weight::from_parts(5_617_000, 0)
.saturating_add(Weight::from_parts(0, 0))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
Weight::from_parts(5_628_000, 0)
.saturating_add(Weight::from_parts(0, 0))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
8 changes: 8 additions & 0 deletions polkadot/runtime/westend/src/weights/pallet_balances.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,4 +156,12 @@ impl<T: frame_system::Config> pallet_balances::WeightInfo for WeightInfo<T> {
Weight::from_parts(5_457_000, 0)
.saturating_add(Weight::from_parts(0, 0))
}
fn burn_allow_death() -> Weight {
// TODO
Weight::zero()
}
fn burn_keep_alive() -> Weight {
// TODO
Weight::zero()
}
}
10 changes: 7 additions & 3 deletions polkadot/xcm/xcm-builder/src/fungible_adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@
use super::MintLocation;
use frame_support::traits::{
tokens::{
fungible, Fortitude::Polite, Precision::Exact, Preservation::Preserve, Provenance::Minted,
fungible,
Fortitude::Polite,
Precision::Exact,
Preservation::{Expendable, Preserve},
Provenance::Minted,
},
Get,
};
Expand Down Expand Up @@ -100,7 +104,7 @@ impl<
}

fn reduce_checked(checking_account: AccountId, amount: Fungible::Balance) {
let ok = Fungible::burn_from(&checking_account, amount, Exact, Polite).is_ok();
let ok = Fungible::burn_from(&checking_account, amount, Expendable, Exact, Polite).is_ok();
debug_assert!(ok, "`can_reduce_checked` must have returned `true` immediately prior; qed");
}
}
Expand Down Expand Up @@ -210,7 +214,7 @@ impl<
let amount = Matcher::matches_fungible(what).ok_or(MatchError::AssetNotHandled)?;
let who = AccountIdConverter::convert_location(who)
.ok_or(MatchError::AccountIdConversionFailed)?;
Fungible::burn_from(&who, amount, Exact, Polite)
Fungible::burn_from(&who, amount, Expendable, Exact, Polite)
.map_err(|error| XcmError::FailedToTransactAsset(error.into()))?;
Ok(what.clone().into())
}
Expand Down
11 changes: 8 additions & 3 deletions polkadot/xcm/xcm-builder/src/fungibles_adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@

use frame_support::traits::{
tokens::{
fungibles, Fortitude::Polite, Precision::Exact, Preservation::Preserve, Provenance::Minted,
fungibles,
Fortitude::Polite,
Precision::Exact,
Preservation::{Expendable, Preserve},
Provenance::Minted,
},
Contains, Get,
};
Expand Down Expand Up @@ -176,7 +180,8 @@ impl<
}
fn reduce_checked(asset_id: Assets::AssetId, amount: Assets::Balance) {
let checking_account = CheckingAccount::get();
let ok = Assets::burn_from(asset_id, &checking_account, amount, Exact, Polite).is_ok();
let ok = Assets::burn_from(asset_id, &checking_account, amount, Expendable, Exact, Polite)
.is_ok();
debug_assert!(ok, "`can_reduce_checked` must have returned `true` immediately prior; qed");
}
}
Expand Down Expand Up @@ -295,7 +300,7 @@ impl<
let (asset_id, amount) = Matcher::matches_fungibles(what)?;
let who = AccountIdConverter::convert_location(who)
.ok_or(MatchError::AccountIdConversionFailed)?;
Assets::burn_from(asset_id, &who, amount, Exact, Polite)
Assets::burn_from(asset_id, &who, amount, Expendable, Exact, Polite)
.map_err(|e| XcmError::FailedToTransactAsset(e.into()))?;
Ok(what.clone().into())
}
Expand Down
16 changes: 16 additions & 0 deletions prdoc/pr_3964.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
title: Burn extrinsic call and `fn burn_from` `Preservation` argument

doc:
- audience: Runtime Dev
description: |
pallet-balances extrinsic calls has been expanded with `burn` call.
An argument flag is allowed to specify whether the account should be kept alive or not.
This in turn required a change to the fungible's `pub trait Mutate` `burn_from` function which now
also accepts `Preservation` as an argument.
In order to keep the behavior same as before, developers should simply specify `Preservation::Expandable`.

crates:
- name: frame-support
bump: major
- name: pallet-balances
bump: minor
9 changes: 8 additions & 1 deletion substrate/frame/asset-conversion/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,14 @@ pub mod pallet {
);

// burn the provided lp token amount that includes the fee
T::PoolAssets::burn_from(pool.lp_token.clone(), &sender, lp_token_burn, Exact, Polite)?;
T::PoolAssets::burn_from(
pool.lp_token.clone(),
&sender,
lp_token_burn,
Expendable,
Exact,
Polite,
)?;

T::Assets::transfer(*asset1, &pool_account, &withdraw_to, amount1, Expendable)?;
T::Assets::transfer(*asset2, &pool_account, &withdraw_to, amount2, Expendable)?;
Expand Down
38 changes: 38 additions & 0 deletions substrate/frame/balances/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,44 @@ mod benchmarks {
assert_eq!(Balances::<T, I>::total_issuance(), ti + delta);
}

// Benchmark `burn` extrinsic with the worst possible condition - burn kills the account.
Dinonard marked this conversation as resolved.
Show resolved Hide resolved
#[benchmark]
fn burn_allow_death() {
let existential_deposit = T::ExistentialDeposit::get();
let caller = whitelisted_caller();

// Give some multiple of the existential deposit
let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, balance);

// Burn enough to kill the account.
let burn_amount = balance - existential_deposit + 1u32.into();

#[extrinsic_call]
burn(RawOrigin::Signed(caller.clone()), burn_amount, false);

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 caller = whitelisted_caller();

// Give some multiple of the existential deposit
let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, balance);

// Burn minimum possible amount which should not kill the account.
let burn_amount = 1u32.into();

#[extrinsic_call]
burn(RawOrigin::Signed(caller.clone()), burn_amount, true);

assert_eq!(Balances::<T, I>::free_balance(&caller), balance - burn_amount);
}

impl_benchmark_test_suite! {
Balances,
crate::tests::ExtBuilder::default().build(),
Expand Down
25 changes: 24 additions & 1 deletion substrate/frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
ensure_signed(origin)?;
if who.is_empty() {
return Ok(Pays::Yes.into())
return Ok(Pays::Yes.into());
Dinonard marked this conversation as resolved.
Show resolved Hide resolved
}
let mut upgrade_count = 0;
for i in &who {
Expand Down Expand Up @@ -767,6 +767,29 @@ pub mod pallet {

Ok(())
}

/// Burn the specified liquid free balance from the origin account.
///
/// If the origin's account ends up below the existential deposit as a result
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
/// of the burn and `keep_alive` is false, the account will be reaped.
#[pallet::call_index(10)]
#[pallet::weight(if *keep_alive {T::WeightInfo::burn_allow_death() } else {T::WeightInfo::burn_keep_alive()})]
pub fn burn(
origin: OriginFor<T>,
#[pallet::compact] value: T::Balance,
Copy link
Member

Choose a reason for hiding this comment

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

An additional call is overkill, but a keep_alive: bool similar to thetransfer_all would be fine i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind doing that, but will need to change this function signature:
https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungible/regular.rs#L260

It might end being a slightly lager change, hope that's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated per request.

Will need help/suggestion with the remaining TODOs in the PR summary:

  • running benchmarks & updating weights (I cannot trigger the job)
  • check whether prdoc is ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know you said in another PR that current review process isn't sustainable (I assume due to human resources), but is there someone else we can ask to take a look at the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggwpez sorry for the tag but could you please give a timeline when this can be checked out? Can we get someone else to review it?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, it is best to ask in chat for review. We have the open #fellowship-open-channel:parity.io channel for that. I invited you.
Otherwise it can get lost or not have any priority...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot!

keep_alive: bool,
) -> DispatchResult {
let source = ensure_signed(origin)?;
let preservation = if keep_alive { Preserve } else { Expendable };
<Self as fungible::Mutate<_>>::burn_from(
&source,
value,
preservation,
Precision::Exact,
Polite,
)?;
Ok(())
}
}

impl<T: Config<I>, I: 'static> Pallet<T, I> {
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/balances/src/tests/currency_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ fn account_removal_on_free_too_low() {
fn burn_must_work() {
ExtBuilder::default().monied(true).build_and_execute_with(|| {
let init_total_issuance = Balances::total_issuance();
let imbalance = Balances::burn(10);
let imbalance = <Balances as Currency<_>>::burn(10);
assert_eq!(Balances::total_issuance(), init_total_issuance - 10);
drop(imbalance);
assert_eq!(Balances::total_issuance(), init_total_issuance);
Expand Down
44 changes: 44 additions & 0 deletions substrate/frame/balances/src/tests/dispatchable_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,3 +335,47 @@ fn force_adjust_total_issuance_rejects_more_than_inactive() {
assert_eq!(Balances::active_issuance(), 10);
});
}

#[test]
fn burn_works() {
ExtBuilder::default().build().execute_with(|| {
// Prepare account with initial balance
let (account, init_balance) = (1, 37);
assert_ok!(Balances::force_set_balance(RuntimeOrigin::root(), account, init_balance));
let init_issuance = Balances::total_issuance();
let (keep_alive, allow_death) = (true, false);

// 1. Cannot burn more than what's available
assert_noop!(
Balances::burn(Some(account).into(), init_balance + 1, allow_death),
TokenError::FundsUnavailable,
);

// 2. Burn some funds, without reaping the account
let burn_amount_1 = 1;
assert_ok!(Balances::burn(Some(account).into(), burn_amount_1, allow_death));
System::assert_last_event(RuntimeEvent::Balances(Event::Burned {
who: account,
amount: burn_amount_1,
}));
assert_eq!(Balances::total_issuance(), init_issuance - burn_amount_1);
assert_eq!(Balances::total_balance(&account), init_balance - burn_amount_1);

// 3. Cannot burn funds below existential deposit if `keep_alive` is `true`
let burn_amount_2 =
init_balance - burn_amount_1 - <Test as Config>::ExistentialDeposit::get() + 1;
assert_noop!(
Balances::burn(Some(account).into(), init_balance + 1, keep_alive),
TokenError::FundsUnavailable,
);

// 4. Burn some more funds, this time reaping the account
assert_ok!(Balances::burn(Some(account).into(), burn_amount_2, allow_death));
System::assert_last_event(RuntimeEvent::Balances(Event::Burned {
who: account,
amount: burn_amount_2,
}));
assert_eq!(Balances::total_issuance(), init_issuance - burn_amount_1 - burn_amount_2);
assert!(Balances::total_balance(&account).is_zero());
});
}
Loading
Loading