asset-registry refactoring

pallets/asset-registry/src/benchmarking.rs

@@ -5,48 +5,37 @@ use super::*;
 #[allow(unused)]
 use crate::Pallet as AssetRegistry;
 use frame_benchmarking::benchmarks;
-use frame_support::{assert_ok, traits::fungibles::Inspect};
+use frame_support::assert_ok;
 use frame_system::RawOrigin;
 use xcm::opaque::latest::{
 	Junction::{GeneralIndex, PalletInstance, Parachain},
 	Junctions, MultiLocation,
 };
 
-pub const LOCAL_ASSET_ID: u32 = 10;
-
 benchmarks! {
-	where_clause {
-		where
-			T::Assets: Inspect<<T as frame_system::Config>::AccountId, AssetId = u32>,
-	}
-
 	register_reserve_asset {
+		let asset_id = T::BenchmarkHelper::get_registered_asset();
 		let asset_multi_location = MultiLocation {
 			parents: 1,
 			interior: Junctions::X3(Parachain(Default::default()), PalletInstance(Default::default()), GeneralIndex(Default::default()))
 		};
-
-	}: _(RawOrigin::Root, LOCAL_ASSET_ID, asset_multi_location.clone())
+	}: _(RawOrigin::Root, asset_id, asset_multi_location.clone())
 	verify {
-		let read_asset_multi_location = AssetRegistry::<T>::asset_id_multilocation(LOCAL_ASSET_ID)
-			.expect("error reading AssetIdMultiLocation");
-		assert_eq!(read_asset_multi_location, asset_multi_location);
+		assert_eq!(AssetIdMultiLocation::<T>::get(asset_id), Some(asset_multi_location));
 	}
 
 	unregister_reserve_asset {
+		let asset_id = T::BenchmarkHelper::get_registered_asset();
 		let asset_multi_location = MultiLocation {
 			parents: 1,
 			interior: Junctions::X3(Parachain(Default::default()), PalletInstance(Default::default()), GeneralIndex(Default::default()))
 		};
 
-		assert_ok!(AssetRegistry::<T>::register_reserve_asset(RawOrigin::Root.into(), LOCAL_ASSET_ID, asset_multi_location.clone()));
-		let read_asset_multi_location = AssetRegistry::<T>::asset_id_multilocation(LOCAL_ASSET_ID)
-			.expect("error reading AssetIdMultiLocation");
-		assert_eq!(read_asset_multi_location, asset_multi_location);
-
-	}: _(RawOrigin::Root, LOCAL_ASSET_ID)
+		assert_ok!(AssetRegistry::<T>::register_reserve_asset(RawOrigin::Root.into(), asset_id, asset_multi_location.clone()));
+		assert!(AssetIdMultiLocation::<T>::contains_key(asset_id));
+	}: _(RawOrigin::Root, asset_id)
 	verify {
-		assert_eq!(AssetRegistry::<T>::asset_id_multilocation(LOCAL_ASSET_ID), None);
+		assert_eq!(AssetIdMultiLocation::<T>::get(asset_id), None);
 	}
 
 	impl_benchmark_test_suite!(AssetRegistry, crate::mock::new_test_ext(), crate::mock::Test);

pallets/asset-registry/src/lib.rs

@@ -33,21 +33,27 @@ pub mod pallet {
 	type AssetIdOf<T> =
 		<<T as Config>::Assets as Inspect<<T as frame_system::Config>::AccountId>>::AssetId;
 
+	#[cfg(feature = "runtime-benchmarks")]
+	pub trait BenchmarkHelper<AssetId> {
+		fn get_registered_asset() -> AssetId;
+	}
+
 	#[pallet::config]
 	pub trait Config: frame_system::Config {
 		type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
 		type ReserveAssetModifierOrigin: EnsureOrigin<Self::RuntimeOrigin>;
 		type Assets: Inspect<Self::AccountId>;
 		type WeightInfo: WeightInfo;
+		/// Helper trait for benchmarks.
+		#[cfg(feature = "runtime-benchmarks")]
+		type BenchmarkHelper: BenchmarkHelper<AssetIdOf<Self>>;
 	}
 
 	#[pallet::storage]
-	#[pallet::getter(fn asset_id_multilocation)]
 	pub type AssetIdMultiLocation<T: Config> =
 		StorageMap<_, Blake2_128Concat, AssetIdOf<T>, MultiLocation>;
 
 	#[pallet::storage]
-	#[pallet::getter(fn asset_multilocation_id)]
 	pub type AssetMultiLocationId<T: Config> =
 		StorageMap<_, Blake2_128Concat, MultiLocation, AssetIdOf<T>>;
 
@@ -81,8 +87,7 @@ pub mod pallet {
 		) -> DispatchResult {
 			T::ReserveAssetModifierOrigin::ensure_origin(origin)?;
 
-			// verify asset exists on pallet-assets
-			ensure!(Self::asset_exists(asset_id), Error::<T>::AssetDoesNotExist);
+			ensure!(T::Assets::asset_exists(asset_id), Error::<T>::AssetDoesNotExist);
 
 			// verify asset is not yet registered
 			ensure!(
@@ -91,14 +96,14 @@ pub mod pallet {
 			);
 
 			// verify MultiLocation is valid
-			let parents_multi_location_ok = { asset_multi_location.parents == 1 };
-			let junctions_multi_location_ok = matches!(
-				asset_multi_location.interior,
-				Junctions::X3(Parachain(_), PalletInstance(_), GeneralIndex(_))
-			);
-
 			ensure!(
-				parents_multi_location_ok && junctions_multi_location_ok,
+				matches!(
+					asset_multi_location,
+					MultiLocation {
+						parents: 1,
+						interior: Junctions::X3(Parachain(_), PalletInstance(_), GeneralIndex(_))
+					}
+				),
 				Error::<T>::WrongMultiLocation
 			);
 
@@ -107,7 +112,6 @@ pub mod pallet {
 			AssetMultiLocationId::<T>::insert(&asset_multi_location, asset_id);
 
 			Self::deposit_event(Event::ReserveAssetRegistered { asset_id, asset_multi_location });
-
 			Ok(())
 		}
 
@@ -119,12 +123,10 @@ pub mod pallet {
 		) -> DispatchResult {
 			T::ReserveAssetModifierOrigin::ensure_origin(origin)?;
 
-			// verify asset is registered
-			let asset_multi_location =
-				AssetIdMultiLocation::<T>::get(asset_id).ok_or(Error::<T>::AssetIsNotRegistered)?;
-
 			// unregister asset
-			AssetIdMultiLocation::<T>::remove(asset_id);
+			let asset_multi_location =
+				AssetIdMultiLocation::<T>::mutate_exists(asset_id, Option::take)
+					.ok_or(Error::<T>::AssetIsNotRegistered)?;
 			AssetMultiLocationId::<T>::remove(&asset_multi_location);
 
 			Self::deposit_event(Event::ReserveAssetUnregistered { asset_id, asset_multi_location });
@@ -141,11 +143,4 @@ pub mod pallet {
 			AssetMultiLocationId::<T>::get(asset_type)
 		}
 	}
-
-	impl<T: Config> Pallet<T> {
-		// check if the asset exists
-		fn asset_exists(asset_id: AssetIdOf<T>) -> bool {
-			T::Assets::asset_exists(asset_id)
-		}
-	}
 }

pallets/asset-registry/src/mock.rs

@@ -57,11 +57,22 @@ impl system::Config for Test {
 	type MaxConsumers = ConstU32<16>;
 }
 
+#[cfg(feature = "runtime-benchmarks")]
+pub struct MockAssetRegistryBenchmarkHelper;
+#[cfg(feature = "runtime-benchmarks")]
+impl pallet_asset_registry::BenchmarkHelper<u32> for MockAssetRegistryBenchmarkHelper {
+	fn get_registered_asset() -> u32 {
+		LOCAL_ASSET_ID
+	}
+}
+
 impl pallet_asset_registry::Config for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type ReserveAssetModifierOrigin = frame_system::EnsureRoot<Self::AccountId>;
 	type Assets = Assets;
 	type WeightInfo = pallet_asset_registry::weights::SubstrateWeight<Test>;
+	#[cfg(feature = "runtime-benchmarks")]
+	type BenchmarkHelper = MockAssetRegistryBenchmarkHelper;
 }
 
 impl pallet_balances::Config for Test {

pallets/asset-registry/src/tests.rs

@@ -1,42 +1,67 @@
-use crate::{mock::*, Error};
 use frame_support::{assert_noop, assert_ok};
 use xcm::latest::prelude::*;
 
+use crate::{mock::*, AssetIdMultiLocation, AssetMultiLocationId, Error};
+
+const STATEMINE_ASSET_MULTI_LOCATION: MultiLocation = MultiLocation {
+	parents: 1,
+	interior: X3(
+		Parachain(StatemineParaIdInfo::get()),
+		PalletInstance(StatemineAssetsInstanceInfo::get()),
+		GeneralIndex(StatemineAssetIdInfo::get()),
+	),
+};
+
 #[test]
 fn register_reserve_asset_works() {
 	new_test_ext().execute_with(|| {
-		let statemine_para_id = StatemineParaIdInfo::get();
-		let statemine_assets_pallet = StatemineAssetsInstanceInfo::get();
-		let statemine_asset_id = StatemineAssetIdInfo::get();
+		assert_ok!(AssetRegistry::register_reserve_asset(
+			RuntimeOrigin::root(),
+			LOCAL_ASSET_ID,
+			STATEMINE_ASSET_MULTI_LOCATION,
+		));
 
-		let statemine_asset_multi_location = MultiLocation {
-			parents: 1,
-			interior: X3(
-				Parachain(statemine_para_id),
-				PalletInstance(statemine_assets_pallet),
-				GeneralIndex(statemine_asset_id),
+		assert_eq!(
+			AssetIdMultiLocation::<Test>::get(LOCAL_ASSET_ID),
+			Some(STATEMINE_ASSET_MULTI_LOCATION)
+		);
+		assert_eq!(
+			AssetMultiLocationId::<Test>::get(STATEMINE_ASSET_MULTI_LOCATION),
+			Some(LOCAL_ASSET_ID)
+		);
+	});
+}
+
+#[test]
+fn cannot_register_unexisting_asset() {
+	new_test_ext().execute_with(|| {
+		let unexisting_asset_id = 9999;
+
+		assert_noop!(
+			AssetRegistry::register_reserve_asset(
+				RuntimeOrigin::root(),
+				unexisting_asset_id,
+				STATEMINE_ASSET_MULTI_LOCATION,
 			),
-		};
+			Error::<Test>::AssetDoesNotExist
+		);
+	});
+}
 
+#[test]
+fn cannot_double_register() {
+	new_test_ext().execute_with(|| {
 		assert_ok!(AssetRegistry::register_reserve_asset(
 			RuntimeOrigin::root(),
 			LOCAL_ASSET_ID,
-			statemine_asset_multi_location.clone(),
+			STATEMINE_ASSET_MULTI_LOCATION,
 		));
 
-		let read_asset_multi_location = AssetRegistry::asset_id_multilocation(LOCAL_ASSET_ID)
-			.expect("error reading AssetIdMultiLocation");
-		assert_eq!(read_asset_multi_location, statemine_asset_multi_location);
-
-		let read_asset_id = AssetRegistry::asset_multilocation_id(&statemine_asset_multi_location)
-			.expect("error reading AssetMultiLocationId");
-		assert_eq!(read_asset_id, LOCAL_ASSET_ID);
-
 		assert_noop!(
 			AssetRegistry::register_reserve_asset(
 				RuntimeOrigin::root(),
 				LOCAL_ASSET_ID,
-				statemine_asset_multi_location,
+				STATEMINE_ASSET_MULTI_LOCATION,
 			),
 			Error::<Test>::AssetAlreadyRegistered
 		);
@@ -46,30 +71,25 @@ fn register_reserve_asset_works() {
 #[test]
 fn unregister_reserve_asset_works() {
 	new_test_ext().execute_with(|| {
-		let statemine_para_id = StatemineParaIdInfo::get();
-		let statemine_assets_pallet = StatemineAssetsInstanceInfo::get();
-		let statemine_asset_id = StatemineAssetIdInfo::get();
-
-		let statemine_asset_multi_location = MultiLocation {
-			parents: 1,
-			interior: X3(
-				Parachain(statemine_para_id),
-				PalletInstance(statemine_assets_pallet),
-				GeneralIndex(statemine_asset_id),
-			),
-		};
-
 		assert_ok!(AssetRegistry::register_reserve_asset(
 			RuntimeOrigin::root(),
 			LOCAL_ASSET_ID,
-			statemine_asset_multi_location.clone(),
+			STATEMINE_ASSET_MULTI_LOCATION,
 		));
 
 		assert_ok!(AssetRegistry::unregister_reserve_asset(RuntimeOrigin::root(), LOCAL_ASSET_ID));
 
-		assert!(AssetRegistry::asset_id_multilocation(LOCAL_ASSET_ID).is_none());
-		assert!(
-			AssetRegistry::asset_multilocation_id(statemine_asset_multi_location.clone()).is_none()
+		assert!(AssetIdMultiLocation::<Test>::get(LOCAL_ASSET_ID).is_none());
+		assert!(AssetMultiLocationId::<Test>::get(STATEMINE_ASSET_MULTI_LOCATION).is_none());
+	});
+}
+
+#[test]
+fn cannot_register_unregistered_asset() {
+	new_test_ext().execute_with(|| {
+		assert_noop!(
+			AssetRegistry::unregister_reserve_asset(RuntimeOrigin::root(), LOCAL_ASSET_ID),
+			Error::<Test>::AssetIsNotRegistered
 		);
 	});
 }

runtime/trappist/src/lib.rs

@@ -361,7 +361,7 @@ impl pallet_assets::Config for Runtime {
 	type RuntimeEvent = RuntimeEvent;
 	type Balance = AssetBalance;
 	type AssetId = AssetId;
-	type AssetIdParameter = codec::Compact<u32>;
+	type AssetIdParameter = codec::Compact<AssetId>;
 	type Currency = Balances;
 	type CreateOrigin = AsEnsureOriginWithArg<EnsureSigned<AccountId>>;
 	type ForceOrigin = AssetsForceOrigin;
@@ -559,11 +559,30 @@ impl pallet_dex::Config for Runtime {
 	type MinDeposit = ConstU128<{ UNITS }>;
 }
 
+#[cfg(feature = "runtime-benchmarks")]
+pub struct AssetRegistryBenchmarkHelper;
+#[cfg(feature = "runtime-benchmarks")]
+impl pallet_asset_registry::BenchmarkHelper<AssetId> for AssetRegistryBenchmarkHelper {
+	fn get_registered_asset() -> AssetId {
+		use sp_runtime::traits::StaticLookup;
+
+		let root = frame_system::RawOrigin::Root.into();
+		let asset_id = 1;
+		let caller = frame_benchmarking::whitelisted_caller();
+		let caller_lookup = <Runtime as frame_system::Config>::Lookup::unlookup(caller);
+		Assets::force_create(root, asset_id.into(), caller_lookup, true, 1)
+			.expect("Should have been able to force create asset");
+		asset_id
+	}
+}
+
 impl pallet_asset_registry::Config for Runtime {
 	type RuntimeEvent = RuntimeEvent;
 	type ReserveAssetModifierOrigin = frame_system::EnsureRoot<Self::AccountId>;
 	type Assets = Assets;
 	type WeightInfo = pallet_asset_registry::weights::SubstrateWeight<Runtime>;
+	#[cfg(feature = "runtime-benchmarks")]
+	type BenchmarkHelper = AssetRegistryBenchmarkHelper;
 }
 
 parameter_types! {