fix: benchmarks (#598)

There was a circular call graph issue, where the deposit removal called
the hook, which in turn tried to remove the deposit again. A new
function on the `pallet-deposit-storage` is added which allows to delete
a deposit without calling into the hook.
Benchmarks now run fine for both `pallet-deposit-storage` and
`pallet-dip-provider` for both peregrine and DIP Provider template.

dip-template/runtimes/dip-provider/src/dip.rs

@@ -91,16 +91,33 @@ pub mod deposit {
 		}
 	}
 
+	/// The various different keys that can be stored in the storage-tracking
+	/// pallet.
+	/// Although the namespace is used to distinguish between keys, it is useful
+	/// to group all keys under the same enum to calculate the maximum length
+	/// that a key can take.
+	#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Clone, PartialEq, Eq, RuntimeDebug)]
+	pub struct DepositKey {
+		identifier: DidIdentifier,
+		commitment_version: IdentityCommitmentVersion,
+	}
+
+	impl From<(DidIdentifier, AccountId, IdentityCommitmentVersion)> for DepositKey {
+		fn from((identifier, _, version): (DidIdentifier, AccountId, IdentityCommitmentVersion)) -> Self {
+			Self {
+				identifier,
+				commitment_version: version,
+			}
+		}
+	}
+
 	/// The amount of tokens locked for each identity commitment.
 	pub const DEPOSIT_AMOUNT: Balance = 2 * UNIT;
 
 	/// The additional logic to execute whenever a deposit is removed by its
 	/// owner directly via the [`pallet_deposit_storage::Pallet`] pallet.
-	pub type DepositCollectorHooks = FixedDepositCollectorViaDepositsPallet<
-		DipProviderDepositNamespace,
-		ConstU128<DEPOSIT_AMOUNT>,
-		(DidIdentifier, IdentityCommitmentVersion),
-	>;
+	pub type DepositCollectorHooks =
+		FixedDepositCollectorViaDepositsPallet<DipProviderDepositNamespace, ConstU128<DEPOSIT_AMOUNT>, DepositKey>;
 
 	pub enum CommitmentDepositRemovalHookError {
 		DecodeKey,
@@ -129,14 +146,17 @@ pub mod deposit {
 		fn on_deposit_reclaimed(
 			_namespace: &<Runtime as pallet_deposit_storage::Config>::Namespace,
 			key: &DepositKeyOf<Runtime>,
-			deposit: DepositEntryOf<Runtime>,
+			_deposit: DepositEntryOf<Runtime>,
 		) -> Result<(), Self::Error> {
-			let (identifier, commitment_version) = <(DidIdentifier, IdentityCommitmentVersion)>::decode(&mut &key[..])
-				.map_err(|_| CommitmentDepositRemovalHookError::DecodeKey)?;
-			pallet_dip_provider::Pallet::<Runtime>::delete_identity_commitment_storage_entry(
+			let DepositKey {
+				identifier,
+				commitment_version,
+			} = DepositKey::decode(&mut &key[..]).map_err(|_| CommitmentDepositRemovalHookError::DecodeKey)?;
+			// No hook must be called otherwise it would try to delete the deposit again,
+			// leading to a circular call graph with leads to failure as soon as the deposit
+			// is trying to be deleted again.
+			pallet_dip_provider::Pallet::<Runtime>::delete_identity_commitment_storage_entry_without_hook(
 				&identifier,
-				// Deposit owner is the only one authorized to remove the deposit.
-				&deposit.deposit.owner,
 				commitment_version,
 			)
 			.map_err(|_| {
@@ -164,7 +184,7 @@ pub mod deposit {
 			let namespace = DepositNamespaces::DipProvider;
 			let did_identifier = DidIdentifier::from([200u8; 32]);
 			let commitment_version = 0u16;
-			let key: DepositKeyOf<Runtime> = (did_identifier.clone(), 0)
+			let key = DepositKey::from((did_identifier.clone(), submitter.clone(), commitment_version))
 				.encode()
 				.try_into()
 				.expect("Should not fail to create a key for a DIP commitment.");

pallets/pallet-deposit-storage/src/deposit.rs

@@ -74,7 +74,7 @@ where
 	Runtime: pallet_dip_provider::Config + Config,
 	DepositsNamespace: Get<Runtime::Namespace>,
 	FixedDepositAmount: Get<BalanceOf<Runtime>>,
-	DepositKey: From<(Runtime::Identifier, IdentityCommitmentVersion)> + Encode,
+	DepositKey: From<(Runtime::Identifier, Runtime::AccountId, IdentityCommitmentVersion)> + Encode,
 {
 	type Error = u16;
 
@@ -85,7 +85,7 @@ where
 		version: IdentityCommitmentVersion,
 	) -> Result<(), Self::Error> {
 		let namespace = DepositsNamespace::get();
-		let key = DepositKey::from((identifier.clone(), version))
+		let key = DepositKey::from((identifier.clone(), submitter.clone(), version))
 			.encode()
 			.try_into()
 			.map_err(|_| {
@@ -120,19 +120,22 @@ where
 
 	fn on_commitment_removed(
 		identifier: &Runtime::Identifier,
-		_submitter: &Runtime::AccountId,
+		submitter: &Runtime::AccountId,
 		_commitment: &IdentityCommitmentOf<Runtime>,
 		version: pallet_dip_provider::IdentityCommitmentVersion,
 	) -> Result<(), Self::Error> {
 		let namespace = DepositsNamespace::get();
-		let key = (identifier, version).encode().try_into().map_err(|_| {
-			log::error!(
-				"Failed to convert tuple ({:#?}, {version}) to BoundedVec with max length {}",
-				identifier,
-				Runtime::MaxKeyLength::get()
-			);
-			FixedDepositCollectorViaDepositsPalletError::Internal
-		})?;
+		let key = DepositKey::from((identifier.clone(), submitter.clone(), version))
+			.encode()
+			.try_into()
+			.map_err(|_| {
+				log::error!(
+					"Failed to convert tuple ({:#?}, {version}) to BoundedVec with max length {}",
+					identifier,
+					Runtime::MaxKeyLength::get()
+				);
+				FixedDepositCollectorViaDepositsPalletError::Internal
+			})?;
 		Pallet::<Runtime>::remove_deposit(&namespace, &key, None).map_err(|e| match e {
 			pallet_error if pallet_error == DispatchError::from(Error::<Runtime>::DepositNotFound) => {
 				FixedDepositCollectorViaDepositsPalletError::DepositNotFound

pallets/pallet-dip-provider/src/lib.rs

@@ -205,10 +205,18 @@ pub mod pallet {
 			dispatcher: &T::AccountId,
 			version: IdentityCommitmentVersion,
 		) -> Result<IdentityCommitmentOf<T>, Error<T>> {
-			let commitment =
-				IdentityCommitments::<T>::take(identifier, version).ok_or(Error::<T>::CommitmentNotFound)?;
+			let commitment = Self::delete_identity_commitment_storage_entry_without_hook(identifier, version)?;
 			T::ProviderHooks::on_commitment_removed(identifier, dispatcher, &commitment, version)
 				.map_err(|e| Error::<T>::Hook(e.into()))?;
+			Ok(commitment)
+		}
+
+		pub fn delete_identity_commitment_storage_entry_without_hook(
+			identifier: &T::Identifier,
+			version: IdentityCommitmentVersion,
+		) -> Result<IdentityCommitmentOf<T>, Error<T>> {
+			let commitment =
+				IdentityCommitments::<T>::take(identifier, version).ok_or(Error::<T>::CommitmentNotFound)?;
 			Self::deposit_event(Event::<T>::VersionedIdentityDeleted {
 				identifier: identifier.clone(),
 				version,

runtimes/peregrine/src/dip/deposit.rs

@@ -23,7 +23,7 @@ use pallet_deposit_storage::{
 };
 use pallet_dip_provider::IdentityCommitmentVersion;
 use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
-use runtime_common::constants::dip_provider::COMMITMENT_DEPOSIT;
+use runtime_common::{constants::dip_provider::COMMITMENT_DEPOSIT, AccountId};
 use scale_info::TypeInfo;
 use sp_core::{ConstU128, RuntimeDebug};
 
@@ -44,6 +44,9 @@ impl Get<DepositNamespace> for DipProviderDepositNamespace {
 
 /// The various different keys that can be stored in the storage-tracking
 /// pallet.
+/// Although the namespace is used to distinguish between keys, it is useful to
+/// group all keys under the same enum to calculate the maximum length that a
+/// key can take.
 #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Clone, PartialEq, Eq, RuntimeDebug)]
 pub enum DepositKey {
 	DipProvider {
@@ -52,8 +55,8 @@ pub enum DepositKey {
 	},
 }
 
-impl From<(DidIdentifier, IdentityCommitmentVersion)> for DepositKey {
-	fn from((identifier, version): (DidIdentifier, IdentityCommitmentVersion)) -> Self {
+impl From<(DidIdentifier, AccountId, IdentityCommitmentVersion)> for DepositKey {
+	fn from((identifier, _, version): (DidIdentifier, AccountId, IdentityCommitmentVersion)) -> Self {
 		Self::DipProvider { identifier, version }
 	}
 }
@@ -90,14 +93,15 @@ impl DepositStorageHooks<Runtime> for DepositHooks {
 	fn on_deposit_reclaimed(
 		_namespace: &<Runtime as pallet_deposit_storage::Config>::Namespace,
 		key: &DepositKeyOf<Runtime>,
-		deposit: DepositEntryOf<Runtime>,
+		_deposit: DepositEntryOf<Runtime>,
 	) -> Result<(), Self::Error> {
 		let DepositKey::DipProvider { identifier, version } =
 			DepositKey::decode(&mut &key[..]).map_err(|_| CommitmentDepositRemovalHookError::DecodeKey)?;
-		pallet_dip_provider::Pallet::<Runtime>::delete_identity_commitment_storage_entry(
+		// No hook must be called otherwise it would try to delete the deposit again,
+		// leading to a circular call graph with leads to failure as soon as the deposit
+		// is trying to be deleted again.
+		pallet_dip_provider::Pallet::<Runtime>::delete_identity_commitment_storage_entry_without_hook(
 			&identifier,
-			// Deposit owner is the only one authorized to remove the deposit.
-			&deposit.deposit.owner,
 			version,
 		)
 		.map_err(|_| {
@@ -125,10 +129,11 @@ impl pallet_deposit_storage::traits::BenchmarkHooks<Runtime> for PalletDepositSt
 		let namespace = DepositNamespace::DipProvider;
 		let did_identifier = DidIdentifier::from([200u8; 32]);
 		let commitment_version = 0u16;
-		let key: DepositKeyOf<Runtime> = (did_identifier.clone(), 0)
-			.encode()
-			.try_into()
-			.expect("Should not fail to create a key for a DIP commitment.");
+		let key: DepositKeyOf<Runtime> =
+			DepositKey::from((did_identifier.clone(), submitter.clone(), commitment_version))
+				.encode()
+				.try_into()
+				.expect("Should not fail to create a key for a DIP commitment.");
 
 		pallet_dip_provider::IdentityCommitments::<Runtime>::insert(
 			&did_identifier,