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 pallet::getter usage from the pallet-balances #4967

Merged
merged 7 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Loading