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.

Sign up for GitHub

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
Merged

Remove deprecated pallet_balances's set_balance_deprecated and transfer dispatchables #1226

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
be30e7e
remove deprecated dispatchables
juangirini Aug 28, 2023
f085aab
update test
juangirini Aug 29, 2023
33ea65a
update tests
juangirini Aug 29, 2023
4065d12
Merge branch 'master' into jg/deprecate-set-balance-deprecated
juangirini Aug 29, 2023
141d165
Merge branch 'master' into jg/deprecate-set-balance-deprecated
juangirini Aug 29, 2023
e09564f
update tests
juangirini Aug 29, 2023
d362ce3
Merge branch 'master' into jg/deprecate-set-balance-deprecated
juangirini Aug 30, 2023
bf11c73
Merge branch 'master' into jg/deprecate-set-balance-deprecated
ggwpez Aug 30, 2023
57f22e0
Merge branch 'master' into jg/deprecate-set-balance-deprecated
juangirini Sep 4, 2023
b46095c
add prdocs
juangirini Sep 1, 2023
3a8fae6
add prdoc
juangirini Sep 4, 2023
498d086
Update docs/prdoc/pr_1226.prdoc
KiChjang Sep 5, 2023
72b3a0e
move prdoc file
juangirini Sep 6, 2023
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
17 changes: 17 additions & 0 deletions docs/prdoc/pr_1226.prdoc
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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