pallet-child-bounties index child bounty by parent bounty (#6255)

Resolves #5929 Migrates `ChildBountyDescriptions` to be indexed instead of unique child bounty id unique per all child bounties in the pallet to be unique per every parent bounty. Migrates `(ParentBounty, ChildBounty)` keys inside `ChildBounties` storage item to use new `ChildBounty` ids starting from `0`. @paritytech/frame-coders --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: DavidK <[email protected]> Co-authored-by: muharem <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]>
paritytech · Nov 6, 2024 · a479161 · a479161
1 parent b667c27
commit a479161
Show file tree

Hide file tree

Showing 8 changed files with 759 additions and 225 deletions.
diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs
@@ -1661,6 +1661,7 @@ pub mod migrations {
 		pub const PhragmenElectionPalletId: LockIdentifier = *b"phrelect";
 		/// Weight for balance unreservations
 		pub BalanceUnreserveWeight: Weight = weights::pallet_balances_balances::WeightInfo::<Runtime>::force_unreserve();
+		pub BalanceTransferAllowDeath: Weight = weights::pallet_balances_balances::WeightInfo::<Runtime>::transfer_allow_death();
 	}
 
 	// Special Config for Gov V1 pallets, allowing us to run migrations for them without
@@ -1710,6 +1711,7 @@ pub mod migrations {
         paras_registrar::migration::MigrateToV1<Runtime, ()>,
         pallet_referenda::migration::v1::MigrateV0ToV1<Runtime, ()>,
         pallet_referenda::migration::v1::MigrateV0ToV1<Runtime, pallet_referenda::Instance2>,
+        pallet_child_bounties::migration::MigrateV0ToV1<Runtime, BalanceTransferAllowDeath>,
 
         // Unlock & unreserve Gov1 funds
 

diff --git a/prdoc/pr_6255.prdoc b/prdoc/pr_6255.prdoc
@@ -0,0 +1,34 @@
+title: '[pallet-child-bounties] Index child bounties by parent bounty'
+doc:
+- audience: Runtime Dev
+  description: |
+    Index child bounties by their parent bounty, ensuring that their indexes are independent of
+    child bounties from other parent bounties. This will allow for predictable indexes and the
+    ability to batch creation and approval calls together.
+
+    ### Migration for Runtime Pallet Instance
+    Use `migration::v1::MigrateToV1Impl` storage migration type to translate ids for the active
+    child bounties and migrate the state to the new schema.
+
+    ### Migration for Clients
+    - Use new `ParentTotalChildBounties` storage item to iterate over child bounties for a certain
+    parent bounty;
+    - Use new `ChildBountyDescriptionsV1` storage item to get the bounty description instead of
+    removed `ChildBountyDescriptions`;
+    - Use `V0ToV1ChildBountyIds` storage item to look up the new child bounty id for a given 
+    old child bounty id;
+    - Update the child bounty account id derivation from `PalletId + "cb" + child_id` to
+    `PalletId + "cb" + bounty_id + child_id`.
+
+    ### Additional Notes
+    - The `ChildBountyCount` storage item is deprecated and will be remove in May 2025.
+
+crates:
+- name: pallet-child-bounties
+  bump: major
+- name: pallet-bounties
+  bump: major
+- name: rococo-runtime
+  bump: major
+- name: sp-core
+  bump: minor
diff --git a/substrate/frame/bounties/src/lib.rs b/substrate/frame/bounties/src/lib.rs
@@ -188,8 +188,11 @@ pub trait ChildBountyManager<Balance> {
 	/// Get the active child bounties for a parent bounty.
 	fn child_bounties_count(bounty_id: BountyIndex) -> BountyIndex;
 
-	/// Get total curator fees of children-bounty curators.
+	/// Take total curator fees of children-bounty curators.
 	fn children_curator_fees(bounty_id: BountyIndex) -> Balance;
+
+	/// Hook called when a parent bounty is removed.
+	fn bounty_removed(bounty_id: BountyIndex);
 }
 
 #[frame_support::pallet]
@@ -679,6 +682,7 @@ pub mod pallet {
 					*maybe_bounty = None;
 
 					BountyDescriptions::<T, I>::remove(bounty_id);
+					T::ChildBountyManager::bounty_removed(bounty_id);
 
 					Self::deposit_event(Event::<T, I>::BountyClaimed {
 						index: bounty_id,
@@ -776,7 +780,9 @@ pub mod pallet {
 						AllowDeath,
 					); // should not fail
 					debug_assert!(res.is_ok());
+
 					*maybe_bounty = None;
+					T::ChildBountyManager::bounty_removed(bounty_id);
 
 					Self::deposit_event(Event::<T, I>::BountyCanceled { index: bounty_id });
 					Ok(Some(<T as Config<I>>::WeightInfo::close_bounty_active()).into())
@@ -1054,4 +1060,6 @@ impl<Balance: Zero> ChildBountyManager<Balance> for () {
 	fn children_curator_fees(_bounty_id: BountyIndex) -> Balance {
 		Zero::zero()
 	}
+
+	fn bounty_removed(_bounty_id: BountyIndex) {}
 }
diff --git a/substrate/frame/child-bounties/src/benchmarking.rs b/substrate/frame/child-bounties/src/benchmarking.rs
@@ -151,7 +151,7 @@ fn activate_child_bounty<T: Config>(
 		bounty_setup.reason.clone(),
 	)?;
 
-	bounty_setup.child_bounty_id = ChildBountyCount::<T>::get() - 1;
+	bounty_setup.child_bounty_id = ParentTotalChildBounties::<T>::get(bounty_setup.bounty_id) - 1;
 
 	ChildBounties::<T>::propose_curator(
 		RawOrigin::Signed(bounty_setup.curator.clone()).into(),
@@ -205,7 +205,7 @@ benchmarks! {
 			bounty_setup.child_bounty_value,
 			bounty_setup.reason.clone(),
 		)?;
-		let child_bounty_id = ChildBountyCount::<T>::get() - 1;
+		let child_bounty_id = ParentTotalChildBounties::<T>::get(bounty_setup.bounty_id) - 1;
 
 	}: _(RawOrigin::Signed(bounty_setup.curator), bounty_setup.bounty_id,
 			child_bounty_id, child_curator_lookup, bounty_setup.child_bounty_fee)
@@ -221,7 +221,7 @@ benchmarks! {
 			bounty_setup.child_bounty_value,
 			bounty_setup.reason.clone(),
 		)?;
-		bounty_setup.child_bounty_id = ChildBountyCount::<T>::get() - 1;
+		bounty_setup.child_bounty_id = ParentTotalChildBounties::<T>::get(bounty_setup.bounty_id) - 1;
 
 		ChildBounties::<T>::propose_curator(
 			RawOrigin::Signed(bounty_setup.curator.clone()).into(),
@@ -296,7 +296,7 @@ benchmarks! {
 			bounty_setup.child_bounty_value,
 			bounty_setup.reason.clone(),
 		)?;
-		bounty_setup.child_bounty_id = ChildBountyCount::<T>::get() - 1;
+		bounty_setup.child_bounty_id = ParentTotalChildBounties::<T>::get(bounty_setup.bounty_id) - 1;
 
 	}: close_child_bounty(RawOrigin::Root, bounty_setup.bounty_id,
 		bounty_setup.child_bounty_id)

diff --git a/substrate/frame/child-bounties/src/lib.rs b/substrate/frame/child-bounties/src/lib.rs
@@ -53,11 +53,15 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 mod benchmarking;
+pub mod migration;
 mod tests;
 pub mod weights;
 
 extern crate alloc;
 
+/// The log target for this pallet.
+const LOG_TARGET: &str = "runtime::child-bounties";
+
 use alloc::vec::Vec;
 
 use frame_support::traits::{
@@ -134,7 +138,11 @@ pub mod pallet {
 
 	use super::*;
 
+	/// The in-code storage version.
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
 	#[pallet::pallet]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T>(_);
 
 	#[pallet::config]
@@ -184,16 +192,22 @@ pub mod pallet {
 		Canceled { index: BountyIndex, child_index: BountyIndex },
 	}
 
-	/// Number of total child bounties.
+	/// DEPRECATED: Replaced with `ParentTotalChildBounties` storage item keeping dedicated counts
+	/// for each parent bounty. Number of total child bounties. Will be removed in May 2025.
 	#[pallet::storage]
 	pub type ChildBountyCount<T: Config> = StorageValue<_, BountyIndex, ValueQuery>;
 
-	/// Number of child bounties per parent bounty.
+	/// Number of active child bounties per parent bounty.
 	/// Map of parent bounty index to number of child bounties.
 	#[pallet::storage]
 	pub type ParentChildBounties<T: Config> =
 		StorageMap<_, Twox64Concat, BountyIndex, u32, ValueQuery>;
 
+	/// Number of total child bounties per parent bounty, including completed bounties.
+	#[pallet::storage]
+	pub type ParentTotalChildBounties<T: Config> =
+		StorageMap<_, Twox64Concat, BountyIndex, u32, ValueQuery>;
+
 	/// Child bounties that have been added.
 	#[pallet::storage]
 	pub type ChildBounties<T: Config> = StorageDoubleMap<
@@ -205,10 +219,27 @@ pub mod pallet {
 		ChildBounty<T::AccountId, BalanceOf<T>, BlockNumberFor<T>>,
 	>;
 
-	/// The description of each child-bounty.
+	/// The description of each child-bounty. Indexed by `(parent_id, child_id)`.
+	///
+	/// This item replaces the `ChildBountyDescriptions` storage item from the V0 storage version.
+	#[pallet::storage]
+	pub type ChildBountyDescriptionsV1<T: Config> = StorageDoubleMap<
+		_,
+		Twox64Concat,
+		BountyIndex,
+		Twox64Concat,
+		BountyIndex,
+		BoundedVec<u8, T::MaximumReasonLength>,
+	>;
+
+	/// The mapping of the child bounty ids from storage version `V0` to the new `V1` version.
+	///
+	/// The `V0` ids based on total child bounty count [`ChildBountyCount`]`. The `V1` version ids
+	/// based on the child bounty count per parent bounty [`ParentTotalChildBounties`].
+	/// The item intended solely for client convenience and not used in the pallet's core logic.
 	#[pallet::storage]
-	pub type ChildBountyDescriptions<T: Config> =
-		StorageMap<_, Twox64Concat, BountyIndex, BoundedVec<u8, T::MaximumReasonLength>>;
+	pub type V0ToV1ChildBountyIds<T: Config> =
+		StorageMap<_, Twox64Concat, BountyIndex, (BountyIndex, BountyIndex)>;
 
 	/// The cumulative child-bounty curator fee for each parent bounty.
 	#[pallet::storage]
@@ -276,15 +307,19 @@ pub mod pallet {
 			)?;
 
 			// Get child-bounty ID.
-			let child_bounty_id = ChildBountyCount::<T>::get();
-			let child_bounty_account = Self::child_bounty_account_id(child_bounty_id);
+			let child_bounty_id = ParentTotalChildBounties::<T>::get(parent_bounty_id);
+			let child_bounty_account =
+				Self::child_bounty_account_id(parent_bounty_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));
+			ParentTotalChildBounties::<T>::insert(
+				parent_bounty_id,
+				child_bounty_id.saturating_add(1),
+			);
 
 			// Create child-bounty instance.
 			Self::create_child_bounty(
@@ -672,7 +707,8 @@ pub mod pallet {
 						);
 
 						// Make curator fee payment.
-						let child_bounty_account = Self::child_bounty_account_id(child_bounty_id);
+						let child_bounty_account =
+							Self::child_bounty_account_id(parent_bounty_id, child_bounty_id);
 						let balance = T::Currency::free_balance(&child_bounty_account);
 						let curator_fee = child_bounty.fee.min(balance);
 						let payout = balance.saturating_sub(curator_fee);
@@ -716,7 +752,7 @@ pub mod pallet {
 						});
 
 						// Remove the child-bounty description.
-						ChildBountyDescriptions::<T>::remove(child_bounty_id);
+						ChildBountyDescriptionsV1::<T>::remove(parent_bounty_id, child_bounty_id);
 
 						// Remove the child-bounty instance from the state.
 						*maybe_child_bounty = None;
@@ -772,6 +808,19 @@ pub mod pallet {
 			Ok(())
 		}
 	}
+
+	#[pallet::hooks]
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn integrity_test() {
+			let parent_bounty_id: BountyIndex = 1;
+			let child_bounty_id: BountyIndex = 2;
+			let _: T::AccountId = T::PalletId::get()
+				.try_into_sub_account(("cb", parent_bounty_id, child_bounty_id))
+				.expect(
+					"The `AccountId` type must be large enough to fit the child bounty account ID.",
+				);
+		}
+	}
 }
 
 impl<T: Config> Pallet<T> {
@@ -797,11 +846,14 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// The account ID of a child-bounty account.
-	pub fn child_bounty_account_id(id: BountyIndex) -> T::AccountId {
+	pub fn child_bounty_account_id(
+		parent_bounty_id: BountyIndex,
+		child_bounty_id: BountyIndex,
+	) -> T::AccountId {
 		// This function is taken from the parent (bounties) pallet, but the
 		// prefix is changed to have different AccountId when the index of
 		// parent and child is same.
-		T::PalletId::get().into_sub_account_truncating(("cb", id))
+		T::PalletId::get().into_sub_account_truncating(("cb", parent_bounty_id, child_bounty_id))
 	}
 
 	fn create_child_bounty(
@@ -818,7 +870,7 @@ impl<T: Config> Pallet<T> {
 			status: ChildBountyStatus::Added,
 		};
 		ChildBounties::<T>::insert(parent_bounty_id, child_bounty_id, &child_bounty);
-		ChildBountyDescriptions::<T>::insert(child_bounty_id, description);
+		ChildBountyDescriptionsV1::<T>::insert(parent_bounty_id, child_bounty_id, description);
 		Self::deposit_event(Event::Added { index: parent_bounty_id, child_index: child_bounty_id });
 	}
 
@@ -877,7 +929,8 @@ impl<T: Config> Pallet<T> {
 				// Transfer fund from child-bounty to parent bounty.
 				let parent_bounty_account =
 					pallet_bounties::Pallet::<T>::bounty_account_id(parent_bounty_id);
-				let child_bounty_account = Self::child_bounty_account_id(child_bounty_id);
+				let child_bounty_account =
+					Self::child_bounty_account_id(parent_bounty_id, child_bounty_id);
 				let balance = T::Currency::free_balance(&child_bounty_account);
 				let transfer_result = T::Currency::transfer(
 					&child_bounty_account,
@@ -888,7 +941,7 @@ impl<T: Config> Pallet<T> {
 				debug_assert!(transfer_result.is_ok());
 
 				// Remove the child-bounty description.
-				ChildBountyDescriptions::<T>::remove(child_bounty_id);
+				ChildBountyDescriptionsV1::<T>::remove(parent_bounty_id, child_bounty_id);
 
 				*maybe_child_bounty = None;
 
@@ -902,21 +955,37 @@ impl<T: Config> Pallet<T> {
 	}
 }
 
-// Implement ChildBountyManager to connect with the bounties pallet. This is
-// where we pass the active child bounties and child curator fees to the parent
-// bounty.
+/// Implement ChildBountyManager to connect with the bounties pallet. This is
+/// where we pass the active child bounties and child curator fees to the parent
+/// bounty.
+///
+/// Function `children_curator_fees` not only returns the fee but also removes cumulative curator
+/// fees during call.
 impl<T: Config> pallet_bounties::ChildBountyManager<BalanceOf<T>> for Pallet<T> {
+	/// Returns number of active child bounties for `bounty_id`
 	fn child_bounties_count(
 		bounty_id: pallet_bounties::BountyIndex,
 	) -> pallet_bounties::BountyIndex {
 		ParentChildBounties::<T>::get(bounty_id)
 	}
 
+	/// Returns cumulative child bounty curator fees for `bounty_id` also removing the associated
+	/// storage item. This function is assumed to be called when parent bounty is claimed.
 	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 = ChildrenCuratorFees::<T>::get(bounty_id);
 		ChildrenCuratorFees::<T>::remove(bounty_id);
 		children_fee_total
 	}
+
+	/// Clean up the storage on a parent bounty removal.
+	fn bounty_removed(bounty_id: BountyIndex) {
+		debug_assert!(ParentChildBounties::<T>::get(bounty_id).is_zero());
+		debug_assert!(ChildrenCuratorFees::<T>::get(bounty_id).is_zero());
+		debug_assert!(ChildBounties::<T>::iter_key_prefix(bounty_id).count().is_zero());
+		debug_assert!(ChildBountyDescriptionsV1::<T>::iter_key_prefix(bounty_id).count().is_zero());
+		ParentChildBounties::<T>::remove(bounty_id);
+		ParentTotalChildBounties::<T>::remove(bounty_id);
+	}
 }