Skip to content

Commit

Permalink
Remove pallet::getter usage from the pallet-balances (#4967)
Browse files Browse the repository at this point in the history
As per #3326, removes usage of the `pallet::getter` macro from the
balances pallet. The syntax `StorageItem::<T, I>::get()` should be used
instead.

Also, adds public functions for compatibility.

cc @muraca

polkadot address: 5GsLutpKjbzsbTphebs9Uy4YK6gTN47MAaz6njPktidjR5cp

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
  • Loading branch information
3 people authored Jul 30, 2024
1 parent 03c45b9 commit 7fbfc7e
Show file tree
Hide file tree
Showing 15 changed files with 167 additions and 106 deletions.
28 changes: 28 additions & 0 deletions prdoc/pr_4967.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# 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: "Remove `pallet::getter` usage from the balances pallet"

doc:
- audience: Runtime Dev
description: |
This PR removes the `pallet::getter`s from `pallet-balances`.
The syntax `StorageItem::<T, I>::get()` should be used instead.

crates:
- name: pallet-balances
bump: patch
- name: pallet-staking
bump: patch
- name: pallet-treasury
bump: patch
- name: pallet-bounties
bump: patch
- name: pallet-conviction-voting
bump: patch
- name: pallet-democracy
bump: patch
- name: pallet-elections-phragmen
bump: patch
- name: pallet-referenda
bump: patch
4 changes: 2 additions & 2 deletions substrate/frame/balances/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,13 +288,13 @@ mod benchmarks {

#[benchmark]
fn force_adjust_total_issuance() {
let ti = Balances::<T, I>::total_issuance();
let ti = TotalIssuance::<T, I>::get();
let delta = 123u32.into();

#[extrinsic_call]
_(RawOrigin::Root, AdjustmentDirection::Increase, delta);

assert_eq!(Balances::<T, I>::total_issuance(), ti + delta);
assert_eq!(TotalIssuance::<T, I>::get(), ti + delta);
}

/// Benchmark `burn` extrinsic with the worst possible condition - burn kills the account.
Expand Down
26 changes: 22 additions & 4 deletions substrate/frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -408,13 +408,11 @@ pub mod pallet {

/// The total units issued in the system.
#[pallet::storage]
#[pallet::getter(fn total_issuance)]
#[pallet::whitelist_storage]
pub type TotalIssuance<T: Config<I>, I: 'static = ()> = StorageValue<_, T::Balance, ValueQuery>;

/// The total units of outstanding deactivated balance in the system.
#[pallet::storage]
#[pallet::getter(fn inactive_issuance)]
#[pallet::whitelist_storage]
pub type InactiveIssuance<T: Config<I>, I: 'static = ()> =
StorageValue<_, T::Balance, ValueQuery>;
Expand Down Expand Up @@ -452,7 +450,6 @@ pub mod pallet {
///
/// Use of locks is deprecated in favour of freezes. See `https://github.com/paritytech/substrate/pull/12951/`
#[pallet::storage]
#[pallet::getter(fn locks)]
pub type Locks<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Blake2_128Concat,
Expand All @@ -465,7 +462,6 @@ pub mod pallet {
///
/// Use of reserves is deprecated in favour of holds. See `https://github.com/paritytech/substrate/pull/12951/`
#[pallet::storage]
#[pallet::getter(fn reserves)]
pub type Reserves<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Blake2_128Concat,
Expand Down Expand Up @@ -822,6 +818,28 @@ pub mod pallet {
}

impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// Public function to get the total issuance.
pub fn total_issuance() -> T::Balance {
TotalIssuance::<T, I>::get()
}

/// Public function to get the inactive issuance.
pub fn inactive_issuance() -> T::Balance {
InactiveIssuance::<T, I>::get()
}

/// Public function to access the Locks storage.
pub fn locks(who: &T::AccountId) -> WeakBoundedVec<BalanceLock<T::Balance>, T::MaxLocks> {
Locks::<T, I>::get(who)
}

/// Public function to access the reserves storage.
pub fn reserves(
who: &T::AccountId,
) -> BoundedVec<ReserveData<T::ReserveIdentifier, T::Balance>, T::MaxReserves> {
Reserves::<T, I>::get(who)
}

fn ed() -> T::Balance {
T::ExistentialDeposit::get()
}
Expand Down
40 changes: 22 additions & 18 deletions substrate/frame/balances/src/tests/currency_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ fn reward_should_work() {
]
);
assert_eq!(Balances::total_balance(&1), 20);
assert_eq!(Balances::total_issuance(), 120);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 120);
});
}

Expand Down Expand Up @@ -473,7 +473,7 @@ fn slashing_balance_should_work() {
assert!(Balances::slash(&1, 42).1.is_zero());
assert_eq!(Balances::free_balance(1), 1);
assert_eq!(Balances::reserved_balance(1), 69);
assert_eq!(Balances::total_issuance(), 70);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 70);
});
}

Expand All @@ -488,7 +488,7 @@ fn withdrawing_balance_should_work() {
amount: 11,
}));
assert_eq!(Balances::free_balance(2), 100);
assert_eq!(Balances::total_issuance(), 100);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 100);
});
}

Expand Down Expand Up @@ -519,7 +519,7 @@ fn slashing_incomplete_balance_should_work() {
assert_eq!(Balances::slash(&1, 69).1, 49);
assert_eq!(Balances::free_balance(1), 1);
assert_eq!(Balances::reserved_balance(1), 21);
assert_eq!(Balances::total_issuance(), 22);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 22);
});
}

Expand All @@ -542,7 +542,7 @@ fn slashing_reserved_balance_should_work() {
assert_eq!(Balances::slash_reserved(&1, 42).1, 0);
assert_eq!(Balances::reserved_balance(1), 69);
assert_eq!(Balances::free_balance(1), 1);
assert_eq!(Balances::total_issuance(), 70);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 70);
});
}

Expand All @@ -554,7 +554,7 @@ fn slashing_incomplete_reserved_balance_should_work() {
assert_eq!(Balances::slash_reserved(&1, 69).1, 27);
assert_eq!(Balances::free_balance(1), 69);
assert_eq!(Balances::reserved_balance(1), 0);
assert_eq!(Balances::total_issuance(), 69);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 69);
});
}

Expand Down Expand Up @@ -654,12 +654,12 @@ fn transferring_too_high_value_should_not_panic() {
fn account_create_on_free_too_low_with_other() {
ExtBuilder::default().existential_deposit(100).build_and_execute_with(|| {
let _ = Balances::deposit_creating(&1, 100);
assert_eq!(Balances::total_issuance(), 100);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 100);

// No-op.
let _ = Balances::deposit_creating(&2, 50);
assert_eq!(Balances::free_balance(2), 0);
assert_eq!(Balances::total_issuance(), 100);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 100);
})
}

Expand All @@ -669,22 +669,22 @@ fn account_create_on_free_too_low() {
// No-op.
let _ = Balances::deposit_creating(&2, 50);
assert_eq!(Balances::free_balance(2), 0);
assert_eq!(Balances::total_issuance(), 0);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 0);
})
}

#[test]
fn account_removal_on_free_too_low() {
ExtBuilder::default().existential_deposit(100).build_and_execute_with(|| {
assert_eq!(Balances::total_issuance(), 0);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 0);

// Setup two accounts with free balance above the existential threshold.
let _ = Balances::deposit_creating(&1, 110);
let _ = Balances::deposit_creating(&2, 110);

assert_eq!(Balances::free_balance(1), 110);
assert_eq!(Balances::free_balance(2), 110);
assert_eq!(Balances::total_issuance(), 220);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 220);

// Transfer funds from account 1 of such amount that after this transfer
// the balance of account 1 will be below the existential threshold.
Expand All @@ -696,18 +696,18 @@ fn account_removal_on_free_too_low() {
assert_eq!(Balances::free_balance(2), 130);

// Verify that TotalIssuance tracks balance removal when free balance is too low.
assert_eq!(Balances::total_issuance(), 130);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 130);
});
}

#[test]
fn burn_must_work() {
ExtBuilder::default().monied(true).build_and_execute_with(|| {
let init_total_issuance = Balances::total_issuance();
let init_total_issuance = pallet_balances::TotalIssuance::<Test>::get();
let imbalance = <Balances as Currency<_>>::burn(10);
assert_eq!(Balances::total_issuance(), init_total_issuance - 10);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), init_total_issuance - 10);
drop(imbalance);
assert_eq!(Balances::total_issuance(), init_total_issuance);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), init_total_issuance);
});
}

Expand Down Expand Up @@ -1396,7 +1396,7 @@ fn freezing_and_locking_should_work() {
#[test]
fn self_transfer_noop() {
ExtBuilder::default().existential_deposit(100).build_and_execute_with(|| {
assert_eq!(Balances::total_issuance(), 0);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 0);
let _ = Balances::deposit_creating(&1, 100);

// The account is set up properly:
Expand All @@ -1410,7 +1410,7 @@ fn self_transfer_noop() {
]
);
assert_eq!(Balances::free_balance(1), 100);
assert_eq!(Balances::total_issuance(), 100);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 100);

// Transfers to self are No-OPs:
let _g = StorageNoopGuard::new();
Expand All @@ -1425,7 +1425,11 @@ fn self_transfer_noop() {

assert!(events().is_empty());
assert_eq!(Balances::free_balance(1), 100, "Balance unchanged by self transfer");
assert_eq!(Balances::total_issuance(), 100, "TI unchanged by self transfers");
assert_eq!(
pallet_balances::TotalIssuance::<Test>::get(),
100,
"TI unchanged by self transfers"
);
}
});
}
35 changes: 19 additions & 16 deletions substrate/frame/balances/src/tests/dispatchable_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,9 @@ fn set_balance_handles_killing_account() {
#[test]
fn set_balance_handles_total_issuance() {
ExtBuilder::default().build_and_execute_with(|| {
let old_total_issuance = Balances::total_issuance();
let old_total_issuance = pallet_balances::TotalIssuance::<Test>::get();
assert_ok!(Balances::force_set_balance(RuntimeOrigin::root(), 1337, 69));
assert_eq!(Balances::total_issuance(), old_total_issuance + 69);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), old_total_issuance + 69);
assert_eq!(Balances::total_balance(&1337), 69);
assert_eq!(Balances::free_balance(&1337), 69);
});
Expand Down Expand Up @@ -236,12 +236,12 @@ fn force_adjust_total_issuance_example() {
ExtBuilder::default().build_and_execute_with(|| {
// First we set the TotalIssuance to 64 by giving Alice a balance of 64.
assert_ok!(Balances::force_set_balance(RuntimeOrigin::root(), ALICE, 64));
let old_ti = Balances::total_issuance();
let old_ti = pallet_balances::TotalIssuance::<Test>::get();
assert_eq!(old_ti, 64, "TI should be 64");

// Now test the increase:
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Inc, 32));
let new_ti = Balances::total_issuance();
let new_ti = pallet_balances::TotalIssuance::<Test>::get();
assert_eq!(old_ti + 32, new_ti, "Should increase by 32");

// If Alice tries to call it, it errors:
Expand All @@ -256,19 +256,19 @@ fn force_adjust_total_issuance_example() {
fn force_adjust_total_issuance_works() {
ExtBuilder::default().build_and_execute_with(|| {
assert_ok!(Balances::force_set_balance(RuntimeOrigin::root(), 1337, 64));
let ti = Balances::total_issuance();
let ti = pallet_balances::TotalIssuance::<Test>::get();

// Increase works:
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Inc, 32));
assert_eq!(Balances::total_issuance(), ti + 32);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), ti + 32);
System::assert_last_event(RuntimeEvent::Balances(Event::TotalIssuanceForced {
old: 64,
new: 96,
}));

// Decrease works:
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Dec, 64));
assert_eq!(Balances::total_issuance(), ti - 32);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), ti - 32);
System::assert_last_event(RuntimeEvent::Balances(Event::TotalIssuanceForced {
old: 96,
new: 32,
Expand All @@ -280,19 +280,19 @@ fn force_adjust_total_issuance_works() {
fn force_adjust_total_issuance_saturates() {
ExtBuilder::default().build_and_execute_with(|| {
assert_ok!(Balances::force_set_balance(RuntimeOrigin::root(), 1337, 64));
let ti = Balances::total_issuance();
let ti = pallet_balances::TotalIssuance::<Test>::get();
let max = <Test as Config>::Balance::max_value();
assert_eq!(ti, 64);

// Increment saturates:
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Inc, max));
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Inc, 123));
assert_eq!(Balances::total_issuance(), max);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), max);

// Decrement saturates:
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Dec, max));
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Dec, 123));
assert_eq!(Balances::total_issuance(), 0);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 0);
});
}

Expand All @@ -316,13 +316,13 @@ fn force_adjust_total_issuance_rejects_more_than_inactive() {
assert_ok!(Balances::force_set_balance(RuntimeOrigin::root(), 1337, 64));
Balances::deactivate(16u32.into());

assert_eq!(Balances::total_issuance(), 64);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 64);
assert_eq!(Balances::active_issuance(), 48);

// Works with up to 48:
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Dec, 40),);
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Dec, 8),);
assert_eq!(Balances::total_issuance(), 16);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 16);
assert_eq!(Balances::active_issuance(), 0);
// Errors with more than 48:
assert_noop!(
Expand All @@ -331,7 +331,7 @@ fn force_adjust_total_issuance_rejects_more_than_inactive() {
);
// Increasing again increases the inactive issuance:
assert_ok!(Balances::force_adjust_total_issuance(RawOrigin::Root.into(), Inc, 10),);
assert_eq!(Balances::total_issuance(), 26);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 26);
assert_eq!(Balances::active_issuance(), 10);
});
}
Expand All @@ -342,7 +342,7 @@ fn burn_works() {
// 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 init_issuance = pallet_balances::TotalIssuance::<Test>::get();
let (keep_alive, allow_death) = (true, false);

// 1. Cannot burn more than what's available
Expand All @@ -358,7 +358,7 @@ fn burn_works() {
who: account,
amount: burn_amount_1,
}));
assert_eq!(Balances::total_issuance(), init_issuance - burn_amount_1);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), 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`
Expand All @@ -375,7 +375,10 @@ fn burn_works() {
who: account,
amount: burn_amount_2,
}));
assert_eq!(Balances::total_issuance(), init_issuance - burn_amount_1 - burn_amount_2);
assert_eq!(
pallet_balances::TotalIssuance::<Test>::get(),
init_issuance - burn_amount_1 - burn_amount_2
);
assert!(Balances::total_balance(&account).is_zero());
});
}
6 changes: 3 additions & 3 deletions substrate/frame/bounties/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,13 +241,13 @@ fn accepted_spend_proposal_ignored_outside_spend_period() {
#[test]
fn unused_pot_should_diminish() {
ExtBuilder::default().build_and_execute(|| {
let init_total_issuance = Balances::total_issuance();
let init_total_issuance = pallet_balances::TotalIssuance::<Test>::get();
Balances::make_free_balance_be(&Treasury::account_id(), 101);
assert_eq!(Balances::total_issuance(), init_total_issuance + 100);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), init_total_issuance + 100);

<Treasury as OnInitialize<u64>>::on_initialize(2);
assert_eq!(Treasury::pot(), 50);
assert_eq!(Balances::total_issuance(), init_total_issuance + 50);
assert_eq!(pallet_balances::TotalIssuance::<Test>::get(), init_total_issuance + 50);
});
}

Expand Down
Loading

0 comments on commit 7fbfc7e

Please sign in to comment.