Skip to content

Commit

Permalink
Remove pallet::getter usage from the bounties and child-bounties pall…
Browse files Browse the repository at this point in the history
…ets (paritytech#4392)

As per paritytech#3326, removes pallet::getter usage from the bounties and
child-bounties pallets. The syntax `StorageItem::<T, I>::get()` should
be used instead.

Changes to one pallet involved changes in the other, so I figured it'd
be best to combine these two.

cc @muraca

---------

Co-authored-by: Bastian Köcher <[email protected]>
  • Loading branch information
2 people authored and hitchhooker committed Jun 5, 2024
1 parent ff7fea6 commit ab843e1
Show file tree
Hide file tree
Showing 6 changed files with 88 additions and 75 deletions.
16 changes: 16 additions & 0 deletions prdoc/pr_4392.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# 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 both bounties and child bounties pallet

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

crates:
- name: pallet-bounties
bump: major
- name: pallet-child-bounties
bump: major
6 changes: 1 addition & 5 deletions substrate/frame/bounties/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,12 +303,10 @@ pub mod pallet {

/// Number of bounty proposals that have been made.
#[pallet::storage]
#[pallet::getter(fn bounty_count)]
pub type BountyCount<T: Config<I>, I: 'static = ()> = StorageValue<_, BountyIndex, ValueQuery>;

/// Bounties that have been made.
#[pallet::storage]
#[pallet::getter(fn bounties)]
pub type Bounties<T: Config<I>, I: 'static = ()> = StorageMap<
_,
Twox64Concat,
Expand All @@ -318,13 +316,11 @@ pub mod pallet {

/// The description of each bounty.
#[pallet::storage]
#[pallet::getter(fn bounty_descriptions)]
pub type BountyDescriptions<T: Config<I>, I: 'static = ()> =
StorageMap<_, Twox64Concat, BountyIndex, BoundedVec<u8, T::MaximumReasonLength>>;

/// Bounty indices that have been approved but not yet funded.
#[pallet::storage]
#[pallet::getter(fn bounty_approvals)]
pub type BountyApprovals<T: Config<I>, I: 'static = ()> =
StorageValue<_, BoundedVec<BountyIndex, T::MaxApprovals>, ValueQuery>;

Expand Down Expand Up @@ -849,7 +845,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
description.try_into().map_err(|_| Error::<T, I>::ReasonTooBig)?;
ensure!(value >= T::BountyValueMinimum::get(), Error::<T, I>::InvalidValue);

let index = Self::bounty_count();
let index = BountyCount::<T, I>::get();

// reserve deposit for new bounty
let bond = T::BountyDepositBase::get() +
Expand Down
51 changes: 27 additions & 24 deletions substrate/frame/bounties/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ fn propose_bounty_works() {
assert_eq!(Balances::free_balance(0), 100 - deposit);

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 0,
Expand All @@ -545,9 +545,12 @@ fn propose_bounty_works() {
}
);

assert_eq!(Bounties::bounty_descriptions(0).unwrap(), b"1234567890".to_vec());
assert_eq!(
pallet_bounties::BountyDescriptions::<Test>::get(0).unwrap(),
b"1234567890".to_vec()
);

assert_eq!(Bounties::bounty_count(), 1);
assert_eq!(pallet_bounties::BountyCount::<Test>::get(), 1);
});
}

Expand Down Expand Up @@ -598,10 +601,10 @@ fn close_bounty_works() {
assert_eq!(Balances::reserved_balance(0), 0);
assert_eq!(Balances::free_balance(0), 100 - deposit);

assert_eq!(Bounties::bounties(0), None);
assert_eq!(pallet_bounties::Bounties::<Test>::get(0), None);
assert!(!pallet_treasury::Proposals::<Test>::contains_key(0));

assert_eq!(Bounties::bounty_descriptions(0), None);
assert_eq!(pallet_bounties::BountyDescriptions::<Test>::get(0), None);
});
}

Expand All @@ -622,7 +625,7 @@ fn approve_bounty_works() {
let deposit: u64 = 80 + 5;

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 0,
Expand All @@ -632,7 +635,7 @@ fn approve_bounty_works() {
status: BountyStatus::Approved,
}
);
assert_eq!(Bounties::bounty_approvals(), vec![0]);
assert_eq!(pallet_bounties::BountyApprovals::<Test>::get(), vec![0]);

assert_noop!(
Bounties::close_bounty(RuntimeOrigin::root(), 0),
Expand All @@ -650,7 +653,7 @@ fn approve_bounty_works() {
assert_eq!(Balances::free_balance(0), 100);

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 0,
Expand Down Expand Up @@ -693,7 +696,7 @@ fn assign_curator_works() {
assert_ok!(Bounties::propose_curator(RuntimeOrigin::root(), 0, 4, fee));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee,
Expand All @@ -720,7 +723,7 @@ fn assign_curator_works() {
let expected_deposit = Bounties::calculate_curator_deposit(&fee);

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee,
Expand Down Expand Up @@ -755,7 +758,7 @@ fn unassign_curator_works() {
assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(4), 0));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee,
Expand All @@ -773,7 +776,7 @@ fn unassign_curator_works() {
assert_ok!(Bounties::unassign_curator(RuntimeOrigin::root(), 0));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee,
Expand Down Expand Up @@ -817,7 +820,7 @@ fn award_and_claim_bounty_works() {
assert_ok!(Bounties::award_bounty(RuntimeOrigin::signed(4), 0, 3));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee,
Expand Down Expand Up @@ -851,8 +854,8 @@ fn award_and_claim_bounty_works() {
assert_eq!(Balances::free_balance(3), 56);
assert_eq!(Balances::free_balance(Bounties::bounty_account_id(0)), 0);

assert_eq!(Bounties::bounties(0), None);
assert_eq!(Bounties::bounty_descriptions(0), None);
assert_eq!(pallet_bounties::Bounties::<Test>::get(0), None);
assert_eq!(pallet_bounties::BountyDescriptions::<Test>::get(0), None);
});
}

Expand Down Expand Up @@ -892,8 +895,8 @@ fn claim_handles_high_fee() {
assert_eq!(Balances::free_balance(3), 0);
assert_eq!(Balances::free_balance(Bounties::bounty_account_id(0)), 0);

assert_eq!(Bounties::bounties(0), None);
assert_eq!(Bounties::bounty_descriptions(0), None);
assert_eq!(pallet_bounties::Bounties::<Test>::get(0), None);
assert_eq!(pallet_bounties::BountyDescriptions::<Test>::get(0), None);
});
}

Expand All @@ -918,7 +921,7 @@ fn cancel_and_refund() {
));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 0,
Expand Down Expand Up @@ -978,8 +981,8 @@ fn award_and_cancel() {
assert_eq!(Balances::free_balance(0), 95);
assert_eq!(Balances::reserved_balance(0), 0);

assert_eq!(Bounties::bounties(0), None);
assert_eq!(Bounties::bounty_descriptions(0), None);
assert_eq!(pallet_bounties::Bounties::<Test>::get(0), None);
assert_eq!(pallet_bounties::BountyDescriptions::<Test>::get(0), None);
});
}

Expand Down Expand Up @@ -1015,7 +1018,7 @@ fn expire_and_unassign() {
assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(0), 0));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 10,
Expand Down Expand Up @@ -1065,7 +1068,7 @@ fn extend_expiry() {
assert_ok!(Bounties::extend_bounty_expiry(RuntimeOrigin::signed(4), 0, Vec::new()));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 10,
Expand All @@ -1079,7 +1082,7 @@ fn extend_expiry() {
assert_ok!(Bounties::extend_bounty_expiry(RuntimeOrigin::signed(4), 0, Vec::new()));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 10,
Expand Down Expand Up @@ -1190,7 +1193,7 @@ fn unassign_curator_self() {
assert_ok!(Bounties::unassign_curator(RuntimeOrigin::signed(1), 0));

assert_eq!(
Bounties::bounties(0).unwrap(),
pallet_bounties::Bounties::<Test>::get(0).unwrap(),
Bounty {
proposer: 0,
fee: 10,
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/child-bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ fn activate_bounty<T: Config>(
child_bounty_setup.reason.clone(),
)?;

child_bounty_setup.bounty_id = Bounties::<T>::bounty_count() - 1;
child_bounty_setup.bounty_id = pallet_bounties::BountyCount::<T>::get() - 1;

let approve_origin =
T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Expand Down
29 changes: 12 additions & 17 deletions substrate/frame/child-bounties/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,19 +181,16 @@ pub mod pallet {

/// Number of total child bounties.
#[pallet::storage]
#[pallet::getter(fn child_bounty_count)]
pub type ChildBountyCount<T: Config> = StorageValue<_, BountyIndex, ValueQuery>;

/// Number of child bounties per parent bounty.
/// Map of parent bounty index to number of child bounties.
#[pallet::storage]
#[pallet::getter(fn parent_child_bounties)]
pub type ParentChildBounties<T: Config> =
StorageMap<_, Twox64Concat, BountyIndex, u32, ValueQuery>;

/// Child bounties that have been added.
#[pallet::storage]
#[pallet::getter(fn child_bounties)]
pub type ChildBounties<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
Expand All @@ -205,13 +202,11 @@ pub mod pallet {

/// The description of each child-bounty.
#[pallet::storage]
#[pallet::getter(fn child_bounty_descriptions)]
pub type ChildBountyDescriptions<T: Config> =
StorageMap<_, Twox64Concat, BountyIndex, BoundedVec<u8, T::MaximumReasonLength>>;

/// The cumulative child-bounty curator fee for each parent bounty.
#[pallet::storage]
#[pallet::getter(fn children_curator_fees)]
pub type ChildrenCuratorFees<T: Config> =
StorageMap<_, Twox64Concat, BountyIndex, BalanceOf<T>, ValueQuery>;

Expand Down Expand Up @@ -251,7 +246,7 @@ pub mod pallet {
description.try_into().map_err(|_| BountiesError::<T>::ReasonTooBig)?;
ensure!(value >= T::ChildBountyValueMinimum::get(), BountiesError::<T>::InvalidValue);
ensure!(
Self::parent_child_bounties(parent_bounty_id) <=
ParentChildBounties::<T>::get(parent_bounty_id) <=
T::MaxActiveChildBountyCount::get() as u32,
Error::<T>::TooManyChildBounties,
);
Expand All @@ -276,15 +271,15 @@ pub mod pallet {
)?;

// Get child-bounty ID.
let child_bounty_id = Self::child_bounty_count();
let child_bounty_id = ChildBountyCount::<T>::get();
let child_bounty_account = Self::child_bounty_account_id(child_bounty_id);

// Transfer funds from parent bounty to child-bounty.
T::Currency::transfer(&parent_bounty_account, &child_bounty_account, value, KeepAlive)?;

// Increment the active child-bounty count.
<ParentChildBounties<T>>::mutate(parent_bounty_id, |count| count.saturating_inc());
<ChildBountyCount<T>>::put(child_bounty_id.saturating_add(1));
ParentChildBounties::<T>::mutate(parent_bounty_id, |count| count.saturating_inc());
ChildBountyCount::<T>::put(child_bounty_id.saturating_add(1));

// Create child-bounty instance.
Self::create_child_bounty(
Expand Down Expand Up @@ -710,12 +705,12 @@ pub mod pallet {
});

// Update the active child-bounty tracking count.
<ParentChildBounties<T>>::mutate(parent_bounty_id, |count| {
ParentChildBounties::<T>::mutate(parent_bounty_id, |count| {
count.saturating_dec()
});

// Remove the child-bounty description.
<ChildBountyDescriptions<T>>::remove(child_bounty_id);
ChildBountyDescriptions::<T>::remove(child_bounty_id);

// Remove the child-bounty instance from the state.
*maybe_child_bounty = None;
Expand Down Expand Up @@ -817,7 +812,7 @@ impl<T: Config> Pallet<T> {
fn ensure_bounty_active(
bounty_id: BountyIndex,
) -> Result<(T::AccountId, BlockNumberFor<T>), DispatchError> {
let parent_bounty = pallet_bounties::Pallet::<T>::bounties(bounty_id)
let parent_bounty = pallet_bounties::Bounties::<T>::get(bounty_id)
.ok_or(BountiesError::<T>::InvalidIndex)?;
if let BountyStatus::Active { curator, update_due } = parent_bounty.get_status() {
Ok((curator, update_due))
Expand Down Expand Up @@ -862,7 +857,7 @@ impl<T: Config> Pallet<T> {
ChildrenCuratorFees::<T>::mutate(parent_bounty_id, |value| {
*value = value.saturating_sub(child_bounty.fee)
});
<ParentChildBounties<T>>::mutate(parent_bounty_id, |count| {
ParentChildBounties::<T>::mutate(parent_bounty_id, |count| {
*count = count.saturating_sub(1)
});

Expand All @@ -880,7 +875,7 @@ impl<T: Config> Pallet<T> {
debug_assert!(transfer_result.is_ok());

// Remove the child-bounty description.
<ChildBountyDescriptions<T>>::remove(child_bounty_id);
ChildBountyDescriptions::<T>::remove(child_bounty_id);

*maybe_child_bounty = None;

Expand All @@ -901,14 +896,14 @@ impl<T: Config> pallet_bounties::ChildBountyManager<BalanceOf<T>> for Pallet<T>
fn child_bounties_count(
bounty_id: pallet_bounties::BountyIndex,
) -> pallet_bounties::BountyIndex {
Self::parent_child_bounties(bounty_id)
ParentChildBounties::<T>::get(bounty_id)
}

fn children_curator_fees(bounty_id: pallet_bounties::BountyIndex) -> BalanceOf<T> {
// This is asked for when the parent bounty is being claimed. No use of
// keeping it in state after that. Hence removing.
let children_fee_total = Self::children_curator_fees(bounty_id);
<ChildrenCuratorFees<T>>::remove(bounty_id);
let children_fee_total = ChildrenCuratorFees::<T>::get(bounty_id);
ChildrenCuratorFees::<T>::remove(bounty_id);
children_fee_total
}
}
Loading

0 comments on commit ab843e1

Please sign in to comment.