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

Remove deprecated pallet_balances's set_balance_deprecated and transfer dispatchables #1226

Merged
merged 13 commits into from
Sep 6, 2023
17 changes: 17 additions & 0 deletions prdoc/pr_1226.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
title: Removed deprecated `Balances::transfer` and `Balances::set_balance_deprecated` functions.

doc:
- audience: Builder
description: The Balances pallet's dispatchables `set_balance_deprecated` and `transfer` were deprecated in [paritytech/substrate#12951](https://github.com/paritytech/substrate/pull/12951) and have now been removed.
notes:
- Use `set_balance_deprecated` instead `force_set_balance` and `transfer_allow_death` instead of `transfer`.

migrations:
db: []

runtime: []

crates:
- name: pallet-balances

host_functions: []
6 changes: 5 additions & 1 deletion substrate/frame/asset-conversion/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1389,7 +1389,11 @@ fn cannot_block_pool_creation() {
let pool_account =
AssetConversion::get_pool_account(&AssetConversion::get_pool_id(token_2, token_1));
// And transfers the ED to that pool account
assert_ok!(Balances::transfer(RuntimeOrigin::signed(attacker), pool_account, ed));
assert_ok!(Balances::transfer_allow_death(
RuntimeOrigin::signed(attacker),
pool_account,
ed
));
// Then, the attacker creates 14 tokens and sends one of each to the pool account
for i in 10..25 {
create_tokens(attacker, vec![NativeOrAssetId::Asset(i)]);
Expand Down
63 changes: 0 additions & 63 deletions substrate/frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,53 +563,6 @@ pub mod pallet {
Ok(())
}

/// Set the regular balance of a given account; it also takes a reserved balance but this
/// must be the same as the account's current reserved balance.
///
/// The dispatch origin for this call is `root`.
///
/// WARNING: This call is DEPRECATED! Use `force_set_balance` instead.
#[pallet::call_index(1)]
#[pallet::weight(
T::WeightInfo::force_set_balance_creating() // Creates a new account.
.max(T::WeightInfo::force_set_balance_killing()) // Kills an existing account.
)]
pub fn set_balance_deprecated(
origin: OriginFor<T>,
who: AccountIdLookupOf<T>,
#[pallet::compact] new_free: T::Balance,
#[pallet::compact] old_reserved: T::Balance,
) -> DispatchResult {
ensure_root(origin)?;
let who = T::Lookup::lookup(who)?;
let existential_deposit = Self::ed();

let wipeout = new_free < existential_deposit;
let new_free = if wipeout { Zero::zero() } else { new_free };

// First we try to modify the account's balance to the forced balance.
let old_free = Self::try_mutate_account_handling_dust(
&who,
|account, _is_new| -> Result<T::Balance, DispatchError> {
let old_free = account.free;
ensure!(account.reserved == old_reserved, TokenError::Unsupported);
account.free = new_free;
Ok(old_free)
},
)?;

// This will adjust the total issuance, which was not done by the `mutate_account`
// above.
if new_free > old_free {
mem::drop(PositiveImbalance::<T, I>::new(new_free - old_free));
} else if new_free < old_free {
mem::drop(NegativeImbalance::<T, I>::new(old_free - new_free));
}

Self::deposit_event(Event::BalanceSet { who, free: new_free });
Ok(())
}

/// Exactly as `transfer_allow_death`, except the origin must be root and the source account
/// may be specified.
#[pallet::call_index(2)]
Expand Down Expand Up @@ -730,22 +683,6 @@ pub mod pallet {
}
}

/// Alias for `transfer_allow_death`, provided only for name-wise compatibility.
///
/// WARNING: DEPRECATED! Will be released in approximately 3 months.
#[pallet::call_index(7)]
#[pallet::weight(T::WeightInfo::transfer_allow_death())]
pub fn transfer(
origin: OriginFor<T>,
dest: AccountIdLookupOf<T>,
#[pallet::compact] value: T::Balance,
) -> DispatchResult {
let source = ensure_signed(origin)?;
Copy link
Member

Choose a reason for hiding this comment

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

The only thing that we could do is to always return an error with a message that the other call should be used.
But its unclean i think…

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we missed an opportunity to mark this function as #[deprecated]. Maybe this would've clued people in that this extrinsic is going away...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a deprecation message, tagging #[deprecated] wouldn't have helped much here anyway as external calls don't get notified of it. See #182 (comment)

let dest = T::Lookup::lookup(dest)?;
<Self as fungible::Mutate<_>>::transfer(&source, &dest, value, Expendable)?;
Ok(())
}

/// Set the regular balance of a given account.
///
/// The dispatch origin for this call is `root`.
Expand Down
5 changes: 4 additions & 1 deletion substrate/frame/safe-mode/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ impl InstanceFilter<RuntimeCall> for ProxyType {
match self {
ProxyType::Any => true,
ProxyType::JustTransfer => {
matches!(c, RuntimeCall::Balances(pallet_balances::Call::transfer { .. }))
matches!(
c,
RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { .. })
)
},
ProxyType::JustUtility => matches!(c, RuntimeCall::Utility { .. }),
}
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/safe-mode/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,7 @@ fn fails_when_explicit_origin_required() {
}

fn call_transfer() -> RuntimeCall {
RuntimeCall::Balances(pallet_balances::Call::transfer { dest: 1, value: 1 })
RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { dest: 1, value: 1 })
}

fn signed(who: u64) -> RuntimeOrigin {
Expand Down
5 changes: 4 additions & 1 deletion substrate/frame/tx-pause/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ impl InstanceFilter<RuntimeCall> for ProxyType {
match self {
ProxyType::Any => true,
ProxyType::JustTransfer => {
matches!(c, RuntimeCall::Balances(pallet_balances::Call::transfer { .. }))
matches!(
c,
RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { .. })
)
},
ProxyType::JustUtility => matches!(c, RuntimeCall::Utility { .. }),
}
Expand Down
18 changes: 9 additions & 9 deletions substrate/frame/tx-pause/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ fn can_pause_specific_call() {

assert_ok!(TxPause::pause(
RuntimeOrigin::signed(mock::PauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer")
full_name::<Test>(b"Balances", b"transfer_allow_death")
));

assert_err!(
Expand Down Expand Up @@ -69,7 +69,7 @@ fn can_unpause_specific_call() {
new_test_ext().execute_with(|| {
assert_ok!(TxPause::pause(
RuntimeOrigin::signed(mock::PauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer"),
full_name::<Test>(b"Balances", b"transfer_allow_death"),
));
assert_err!(
call_transfer(2, 1).dispatch(RuntimeOrigin::signed(2)),
Expand All @@ -78,7 +78,7 @@ fn can_unpause_specific_call() {

assert_ok!(TxPause::unpause(
RuntimeOrigin::signed(mock::UnpauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer"),
full_name::<Test>(b"Balances", b"transfer_allow_death"),
));
assert_ok!(call_transfer(4, 1).dispatch(RuntimeOrigin::signed(0)));
});
Expand All @@ -92,7 +92,7 @@ fn can_filter_balance_in_batch_when_paused() {

assert_ok!(TxPause::pause(
RuntimeOrigin::signed(mock::PauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer"),
full_name::<Test>(b"Balances", b"transfer_allow_death"),
));

assert_ok!(batch_call.clone().dispatch(RuntimeOrigin::signed(0)));
Expand All @@ -111,7 +111,7 @@ fn can_filter_balance_in_proxy_when_paused() {
new_test_ext().execute_with(|| {
assert_ok!(TxPause::pause(
RuntimeOrigin::signed(mock::PauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer"),
full_name::<Test>(b"Balances", b"transfer_allow_death"),
));

assert_ok!(Proxy::add_proxy(RuntimeOrigin::signed(1), 2, ProxyType::JustTransfer, 0));
Expand Down Expand Up @@ -152,7 +152,7 @@ fn fails_to_pause_unpausable_call_when_other_call_is_paused() {

assert_ok!(TxPause::pause(
RuntimeOrigin::signed(mock::PauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer"),
full_name::<Test>(b"Balances", b"transfer_allow_death"),
));

assert_ok!(call_transfer_keep_alive(3, 1).dispatch(RuntimeOrigin::signed(3)));
Expand Down Expand Up @@ -181,13 +181,13 @@ fn fails_to_pause_already_paused_pallet() {
new_test_ext().execute_with(|| {
assert_ok!(TxPause::pause(
RuntimeOrigin::signed(mock::PauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer"),
full_name::<Test>(b"Balances", b"transfer_allow_death"),
));

assert_noop!(
TxPause::pause(
RuntimeOrigin::signed(mock::PauseOrigin::get()),
full_name::<Test>(b"Balances", b"transfer"),
full_name::<Test>(b"Balances", b"transfer_allow_death"),
),
Error::<Test>::IsPaused
);
Expand All @@ -208,7 +208,7 @@ fn fails_to_unpause_not_paused_pallet() {
}

pub fn call_transfer(dest: u64, value: u64) -> RuntimeCall {
RuntimeCall::Balances(pallet_balances::Call::transfer { dest, value })
RuntimeCall::Balances(pallet_balances::Call::transfer_allow_death { dest, value })
}

pub fn call_transfer_keep_alive(dest: u64, value: u64) -> RuntimeCall {
Expand Down