Fix AssetManager's update_asset_metadata (#461)

* Fix assetmanager metadata update

.github/workflows/generate_dolphin_weights_files.yml

@@ -216,6 +216,14 @@ jobs:
               id: pallet_preimage
               name: pallet_preimage
             iterations: 20
+          -
+            extrinsic:
+              id: '*'
+              name: pallet_asset_manager
+            pallet:
+              id: pallet_asset_manager
+              name: pallet_asset_manager
+            iterations: 20
     steps:
       -
         uses: actions/download-artifact@v2

pallets/asset-manager/src/benchmarking.rs

@@ -40,11 +40,11 @@ benchmarks! {
 
 	}: _(RawOrigin::Root, location.clone(), metadata.clone())
 	verify {
-		assert_eq!(Pallet::<T>::asset_id_location(<T::AssetConfig as AssetConfig<T>>::NativeAssetId::get()), Some(location));
+		assert_eq!(Pallet::<T>::asset_id_location(<T::AssetConfig as AssetConfig<T>>::StartNonNativeAssetId::get()), Some(location));
 	}
 
 	set_units_per_second {
-		let start = <T::AssetConfig as AssetConfig<T>>::NativeAssetId::get();
+		let start = <T::AssetConfig as AssetConfig<T>>::StartNonNativeAssetId::get();
 		let end = start + 1000;
 		for i in start..end {
 
@@ -68,7 +68,7 @@ benchmarks! {
 	}
 
 	update_asset_location {
-		let start = <T::AssetConfig as AssetConfig<T>>::NativeAssetId::get();
+		let start = <T::AssetConfig as AssetConfig<T>>::StartNonNativeAssetId::get();
 		let end = start + 1000;
 		for i in start..end {
 
@@ -90,7 +90,7 @@ benchmarks! {
 	}
 
 	update_asset_metadata {
-		let start = <T::AssetConfig as AssetConfig<T>>::NativeAssetId::get();
+		let start = <T::AssetConfig as AssetConfig<T>>::StartNonNativeAssetId::get();
 		let end = start + 1000;
 		for i in start..end {
 
@@ -111,7 +111,7 @@ benchmarks! {
 	}
 
 	mint_asset {
-		let start = <T::AssetConfig as AssetConfig<T>>::NativeAssetId::get();
+		let start = <T::AssetConfig as AssetConfig<T>>::StartNonNativeAssetId::get();
 		let end = start + 1000;
 		for i in start..end {
 

pallets/asset-manager/src/lib.rs

@@ -113,7 +113,7 @@ pub mod pallet {
 	impl<T: Config> Default for GenesisConfig<T> {
 		fn default() -> Self {
 			Self {
-				start_id: <T::AssetConfig as AssetConfig<T>>::NativeAssetId::get(),
+				start_id: <T::AssetConfig as AssetConfig<T>>::StartNonNativeAssetId::get(),
 				_marker: PhantomData,
 			}
 		}
@@ -173,6 +173,8 @@ pub mod pallet {
 		ErrorCreatingAsset,
 		/// Update a non-exist asset.
 		UpdateNonExistAsset,
+		/// Cannot update reserved assets metadata (0 and 1)
+		CannotUpdateNativeAssetMetadata,
 		/// Asset already registered.
 		AssetAlreadyRegistered,
 		/// Error on minting asset.
@@ -216,7 +218,7 @@ pub mod pallet {
 	impl<T: Config> Pallet<T> {
 		/// Register a new asset in the asset manager.
 		///
-		/// * `origin`: Caller of this extrinsic, the access control is specfied by `ForceOrigin`.
+		/// * `origin`: Caller of this extrinsic, the access control is specified by `ForceOrigin`.
 		/// * `location`: Location of the asset.
 		/// * `metadata`: Asset metadata.
 		/// * `min_balance`: Minimum balance to keep an account alive, used in conjunction with `is_sufficient`.
@@ -255,7 +257,7 @@ pub mod pallet {
 
 		/// Update an asset by its asset id in the asset manager.
 		///
-		/// * `origin`: Caller of this extrinsic, the access control is specfied by `ForceOrigin`.
+		/// * `origin`: Caller of this extrinsic, the access control is specified by `ForceOrigin`.
 		/// * `asset_id`: AssetId to be updated.
 		/// * `location`: `location` to update the asset location.
 		#[pallet::weight(T::WeightInfo::update_asset_location())]
@@ -288,7 +290,7 @@ pub mod pallet {
 
 		/// Update an asset's metadata by its `asset_id`
 		///
-		/// * `origin`: Caller of this extrinsic, the access control is specfied by `ForceOrigin`.
+		/// * `origin`: Caller of this extrinsic, the access control is specified by `ForceOrigin`.
 		/// * `asset_id`: AssetId to be updated.
 		/// * `metadata`: new `metadata` to be associated with `asset_id`.
 		#[pallet::weight(T::WeightInfo::update_asset_metadata())]
@@ -299,18 +301,26 @@ pub mod pallet {
 			metadata: <T::AssetConfig as AssetConfig<T>>::AssetRegistrarMetadata,
 		) -> DispatchResult {
 			T::ModifierOrigin::ensure_origin(origin)?;
+			ensure!(
+				asset_id != <T::AssetConfig as AssetConfig<T>>::NativeAssetId::get(),
+				Error::<T>::CannotUpdateNativeAssetMetadata
+			);
 			ensure!(
 				AssetIdLocation::<T>::contains_key(&asset_id),
 				Error::<T>::UpdateNonExistAsset
 			);
+			<T::AssetConfig as AssetConfig<T>>::AssetRegistrar::update_asset_metadata(
+				asset_id,
+				metadata.clone().into(),
+			)?;
 			AssetIdMetadata::<T>::insert(&asset_id, &metadata);
 			Self::deposit_event(Event::<T>::AssetMetadataUpdated { asset_id, metadata });
 			Ok(())
 		}
 
 		/// Update an asset by its asset id in the asset manager.
 		///
-		/// * `origin`: Caller of this extrinsic, the access control is specfied by `ForceOrigin`.
+		/// * `origin`: Caller of this extrinsic, the access control is specified by `ForceOrigin`.
 		/// * `asset_id`: AssetId to be updated.
 		/// * `units_per_second`: units per second for `asset_id`
 		#[pallet::weight(T::WeightInfo::set_units_per_second())]
@@ -335,7 +345,7 @@ pub mod pallet {
 
 		/// Mint asset by its asset id to a beneficiary.
 		///
-		/// * `origin`: Caller of this extrinsic, the access control is specfied by `ForceOrigin`.
+		/// * `origin`: Caller of this extrinsic, the access control is specified by `ForceOrigin`.
 		/// * `asset_id`: AssetId to be updated.
 		/// * `beneficiary`: Account to mint the asset.
 		/// * `amount`: Amount of asset being minted.

pallets/asset-manager/src/tests.rs

@@ -13,17 +13,14 @@
 //
 // You should have received a copy of the GNU General Public License
 // along with Manta.  If not, see <http://www.gnu.org/licenses/>.
-//
-// The pallet-tx-pause pallet is forked from Acala's transaction-pause module https://github.com/AcalaNetwork/Acala/tree/master/modules/transaction-pause
-// The original license is the following - SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
 
 //! unit tests for asset-manager
 
 use crate::{
-	self as asset_manager, AssetIdLocation, AssetIdMetadata, LocationAssetId, UnitsPerSecond,
+	self as asset_manager, AssetIdLocation, AssetIdMetadata, Error, LocationAssetId, UnitsPerSecond,
 };
 use asset_manager::mock::*;
-use frame_support::{assert_noop, assert_ok};
+use frame_support::{assert_noop, assert_ok, traits::fungibles::InspectMetadata};
 use manta_primitives::assets::{
 	AssetConfig, AssetLocation, AssetRegistrarMetadata, FungibleLedger,
 };
@@ -130,7 +127,7 @@ fn register_asset_should_work() {
 		// Register twice will fail
 		assert_noop!(
 			AssetManager::register_asset(Origin::root(), source_location, asset_metadata.clone()),
-			crate::Error::<Runtime>::LocationAlreadyExists
+			Error::<Runtime>::LocationAlreadyExists
 		);
 		// Register a new asset
 		assert_ok!(AssetManager::register_asset(
@@ -144,17 +141,25 @@ fn register_asset_should_work() {
 
 #[test]
 fn update_asset() {
+	let original_name = b"Kusama".to_vec();
+	let original_symbol = b"KSM".to_vec();
+	let original_decimals = 12;
 	let asset_metadata = AssetRegistrarMetadata {
-		name: b"Kusama".to_vec(),
-		symbol: b"KSM".to_vec(),
-		decimals: 12,
+		name: original_name,
+		symbol: original_symbol,
+		decimals: original_decimals,
 		min_balance: 1u128,
 		evm_address: None,
 		is_frozen: false,
 		is_sufficient: true,
 	};
 	let mut new_metadata = asset_metadata.clone();
-	new_metadata.is_frozen = true;
+	let new_name = b"NotKusama".to_vec();
+	let new_symbol = b"NotKSM".to_vec();
+	let new_decimals = original_decimals + 1;
+	new_metadata.name = new_name.clone();
+	new_metadata.symbol = new_symbol.clone();
+	new_metadata.decimals = new_decimals;
 	let source_location = AssetLocation(VersionedMultiLocation::V1(MultiLocation::parent()));
 	let new_location = AssetLocation(VersionedMultiLocation::V1(MultiLocation::new(
 		1,
@@ -172,12 +177,24 @@ fn update_asset() {
 			AssetIdLocation::<Runtime>::get(asset_id),
 			Some(source_location.clone())
 		);
-		// Update the asset metadata
+		// Cannot update asset 1. Will be reserved for the native asset.
+		let native_asset_id = <MantaAssetConfig as AssetConfig<Runtime>>::NativeAssetId::get();
+		assert_noop!(
+			AssetManager::update_asset_metadata(
+				Origin::root(),
+				native_asset_id,
+				new_metadata.clone(),
+			),
+			Error::<Runtime>::CannotUpdateNativeAssetMetadata
+		);
 		assert_ok!(AssetManager::update_asset_metadata(
 			Origin::root(),
 			asset_id,
-			new_metadata.clone()
-		));
+			new_metadata.clone(),
+		),);
+		assert_eq!(Assets::name(&asset_id), new_name);
+		assert_eq!(Assets::symbol(&asset_id), new_symbol);
+		assert_eq!(Assets::decimals(&asset_id), new_decimals);
 		// Update the asset location
 		assert_ok!(AssetManager::update_asset_location(
 			Origin::root(),
@@ -191,24 +208,35 @@ fn update_asset() {
 			125u128
 		));
 		assert_eq!(UnitsPerSecond::<Runtime>::get(asset_id), Some(125));
+		let next_asset_id = asset_id + 1;
 		// Update a non-exist asset should fail
 		assert_noop!(
-			AssetManager::update_asset_location(Origin::root(), 2, new_location.clone()),
-			crate::Error::<Runtime>::UpdateNonExistAsset
+			AssetManager::update_asset_location(
+				Origin::root(),
+				next_asset_id,
+				new_location.clone()
+			),
+			Error::<Runtime>::UpdateNonExistAsset
 		);
 		assert_noop!(
-			AssetManager::update_asset_metadata(Origin::root(), 2, new_metadata.clone()),
-			crate::Error::<Runtime>::UpdateNonExistAsset
+			AssetManager::update_asset_metadata(
+				Origin::root(),
+				next_asset_id,
+				new_metadata.clone()
+			),
+			Error::<Runtime>::UpdateNonExistAsset
 		);
-		// Update an asset to an existing location will fail
+		// Re-registering the original location and metadata should work,
+		// as we modified the previous asset.
 		assert_ok!(AssetManager::register_asset(
 			Origin::root(),
 			source_location.clone(),
 			asset_metadata.clone()
 		));
+		// But updating the asset to an existing location will fail.
 		assert_noop!(
-			AssetManager::update_asset_location(Origin::root(), 1, new_location),
-			crate::Error::<Runtime>::LocationAlreadyExists
+			AssetManager::update_asset_location(Origin::root(), next_asset_id, new_location),
+			Error::<Runtime>::LocationAlreadyExists
 		);
 	})
 }