Clean Old Storage; Add Id to Event; Improve rpc call (#1704)

* Remove deprecated storage

* Add InstructionId, to event; Fix get_execute_instruction_info

* Add missing migration

pallets/asset/src/lib.rs

@@ -206,15 +206,6 @@ decl_storage! {
         pub AssetMetadataGlobalSpecs get(fn asset_metadata_global_specs):
             map hasher(twox_64_concat) AssetMetadataGlobalKey => Option<AssetMetadataSpec>;
 
-        /// Next Asset Metadata Local Key.
-        #[deprecated]
-        pub AssetMetadataNextLocalKey get(fn asset_metadata_next_local_key):
-            map hasher(blake2_128_concat) AssetID => AssetMetadataLocalKey;
-
-        /// Next Asset Metadata Global Key.
-        #[deprecated]
-        pub AssetMetadataNextGlobalKey get(fn asset_metadata_next_global_key): AssetMetadataGlobalKey;
-
         /// A list of assets that exempt all users from affirming its receivement.
         pub AssetsExemptFromAffirmation get(fn assets_exempt_from_affirmation):
             map hasher(blake2_128_concat) AssetID => bool;
@@ -1356,7 +1347,6 @@ impl<T: Config> Module<T> {
 
         // Next global key.
         let key = Self::update_current_asset_metadata_global_key()?;
-        AssetMetadataNextGlobalKey::set(key);
 
         // Store global key <-> name mapping.
         AssetMetadataGlobalNameToKey::insert(&name, key);
@@ -2508,7 +2498,6 @@ impl<T: Config> Module<T> {
 
         // Next local key for asset.
         let key = Self::update_current_asset_metadata_local_key(&asset_id)?;
-        AssetMetadataNextLocalKey::insert(asset_id, key);
 
         // Store local key <-> name mapping.
         AssetMetadataLocalNameToKey::insert(asset_id, &name, key);

pallets/asset/src/migrations.rs

@@ -70,7 +70,7 @@ mod v4 {
             pub AssetMetadataLocalSpecs get(fn asset_metadata_local_specs):
                 double_map hasher(blake2_128_concat) Ticker, hasher(twox_64_concat) AssetMetadataLocalKey => Option<AssetMetadataSpec>;
 
-            // This storage changed the Ticker key to AssetID.
+            // This storage has been removed.
             pub AssetMetadataNextLocalKey get(fn asset_metadata_next_local_key):
                 map hasher(blake2_128_concat) Ticker => AssetMetadataLocalKey;
 
@@ -89,6 +89,9 @@ mod v4 {
             // This storage changed the Ticker key to AssetID.
             pub CurrentAssetMetadataLocalKey get(fn current_asset_metadata_local_key):
                 map hasher(blake2_128_concat) Ticker => Option<AssetMetadataLocalKey>;
+
+            // This storage has been removed.
+            pub AssetMetadataNextGlobalKey get(fn asset_metadata_next_global_key): AssetMetadataGlobalKey;
         }
     }
 
@@ -295,17 +298,9 @@ pub(crate) fn migrate_to_v5<T: Config>() {
     });
     log::info!("{:?} items migrated", count);
 
-    let mut count = 0;
-    log::info!("Updating types for the AssetMetadataNextLocalKey storage");
-    v4::AssetMetadataNextLocalKey::drain().for_each(|(ticker, next)| {
-        count += 1;
-        let asset_id = ticker_to_asset_id
-            .entry(ticker)
-            .or_insert(AssetID::from(ticker));
-
-        AssetMetadataNextLocalKey::insert(asset_id, next);
-    });
-    log::info!("{:?} items migrated", count);
+    log::info!("Removing old AssetMetadataNextLocalKey storage");
+    let res = v4::AssetMetadataNextLocalKey::clear(u32::max_value(), None);
+    log::info!("{:?} items have been cleared", res.unique);
 
     let mut count = 0;
     log::info!("Moving items from TickersExemptFromAffirmation to AssetsExemptFromAffirmation");
@@ -363,4 +358,7 @@ pub(crate) fn migrate_to_v5<T: Config>() {
         TickerAssetID::insert(ticker, asset_id);
     }
     log::info!("{:?} items migrated", count);
+
+    log::info!("AssetMetadataNextGlobalKey has been cleared");
+    v4::AssetMetadataNextGlobalKey::kill();
 }

pallets/common/src/traits/settlement.rs

@@ -54,7 +54,7 @@ decl_event!(
         /// Venue not part of the token's allow list (did, AssetID, venue_id)
         VenueUnauthorized(IdentityId, AssetID, VenueId),
         /// Scheduling of instruction fails.
-        SchedulingFailed(DispatchError),
+        SchedulingFailed(InstructionId, DispatchError),
         /// Instruction is rescheduled.
         /// (caller DID, instruction_id)
         InstructionRescheduled(IdentityId, InstructionId),

pallets/nft/src/lib.rs

@@ -55,14 +55,6 @@ decl_storage!(
         pub MetadataValue get(fn metadata_value):
             double_map hasher(blake2_128_concat) (NFTCollectionId, NFTId), hasher(blake2_128_concat) AssetMetadataKey => AssetMetadataValue;
 
-        /// The next available id for an NFT collection.
-        #[deprecated]
-        pub NextCollectionId get(fn collection_id): NFTCollectionId;
-
-        /// The next available id for an NFT within a collection.
-        #[deprecated]
-        pub NextNFTId get(fn nft_id): map hasher(blake2_128_concat) NFTCollectionId => NFTId;
-
         /// The total number of NFTs in a collection
         pub NFTsInCollection get(fn nfts_in_collection): map hasher(blake2_128_concat) AssetID => NFTCount;
 
@@ -323,7 +315,6 @@ impl<T: Config> Module<T> {
 
         // Creates the nft collection
         let collection_id = Self::update_current_collection_id()?;
-        NextCollectionId::set(collection_id);
         let nft_collection = NFTCollection::new(collection_id, asset_id.clone());
         Collection::insert(&collection_id, nft_collection);
         CollectionKeys::insert(&collection_id, collection_keys);
@@ -389,7 +380,6 @@ impl<T: Config> Module<T> {
             .checked_add(1)
             .ok_or(Error::<T>::BalanceOverflow)?;
         let nft_id = Self::update_current_nft_id(&collection_id)?;
-        NextNFTId::insert(&collection_id, nft_id);
         NFTsInCollection::insert(&asset_id, new_supply);
         NumberOfNFTs::insert(&asset_id, &caller_portfolio.did, new_balance);
         for (metadata_key, metadata_value) in nft_attributes.into_iter() {
@@ -796,30 +786,3 @@ impl<T: Config> NFTTrait<T::RuntimeOrigin> for Module<T> {
         Module::<T>::create_nft_collection(origin, asset_id, nft_type, collection_keys)
     }
 }
-
-pub mod migration {
-    use frame_support::storage::{IterableStorageMap, StorageMap, StorageValue};
-    use sp_runtime::runtime_logger::RuntimeLogger;
-
-    use crate::{
-        Config, CurrentCollectionId, CurrentNFTId, NFTCollectionId, NextCollectionId, NextNFTId,
-    };
-
-    pub fn migrate_to_v3<T: Config>() {
-        RuntimeLogger::init();
-        log::info!(">>> Initializing CurrentNFTId and CurrentCollectionId storage");
-        initialize_storage::<T>();
-        log::info!(">>> Storage has been initialized");
-    }
-
-    fn initialize_storage<T: Config>() {
-        for (collection_id, current_nft_id) in NextNFTId::iter() {
-            CurrentNFTId::insert(collection_id, current_nft_id);
-        }
-
-        let collection_id = NextCollectionId::get();
-        if NFTCollectionId::default() != collection_id {
-            CurrentCollectionId::put(collection_id);
-        }
-    }
-}

pallets/nft/src/migrations.rs

@@ -36,6 +36,12 @@ mod v3 {
             // This storage changed the Ticker key to AssetID.
             pub NFTOwner get(fn nft_owner):
                 double_map hasher(blake2_128_concat) Ticker, hasher(blake2_128_concat) NFTId => Option<PortfolioId>;
+
+            // This storage has been removed.
+            pub NextCollectionId get(fn collection_id): NFTCollectionId;
+
+            // This storage has been removed.
+            pub NextNFTId get(fn nft_id): map hasher(blake2_128_concat) NFTCollectionId => NFTId;
         }
     }
 
@@ -107,4 +113,11 @@ pub(crate) fn migrate_to_v4<T: Config>() {
         NFTOwner::insert(asset_id, nft_id, portfolio);
     });
     log::info!("{:?} items migrated", count);
+
+    log::info!("NextCollectionId has been cleared");
+    v3::NextCollectionId::kill();
+
+    log::info!("Removing old NextNFTId storage");
+    let res = v3::NextNFTId::clear(u32::max_value(), None);
+    log::info!("{:?} items have been cleared", res.unique);
 }

pallets/runtime/common/src/runtime.rs

@@ -1100,7 +1100,7 @@ macro_rules! runtime_apis {
                 #[inline]
                 fn get_execute_instruction_info(
                     instruction_id: &InstructionId
-                ) -> ExecuteInstructionInfo {
+                ) -> Option<ExecuteInstructionInfo> {
                     Settlement::execute_instruction_info(instruction_id)
                 }
 

pallets/runtime/tests/src/asset_pallet/register_metadata.rs

@@ -5,8 +5,7 @@ use sp_keyring::AccountKeyring;
 use pallet_asset::{
     AssetMetadataGlobalKeyToName, AssetMetadataGlobalNameToKey, AssetMetadataGlobalSpecs,
     AssetMetadataLocalKeyToName, AssetMetadataLocalNameToKey, AssetMetadataLocalSpecs,
-    AssetMetadataNextGlobalKey, AssetMetadataNextLocalKey, CurrentAssetMetadataGlobalKey,
-    CurrentAssetMetadataLocalKey,
+    CurrentAssetMetadataGlobalKey, CurrentAssetMetadataLocalKey,
 };
 use polymesh_primitives::asset_metadata::{
     AssetMetadataGlobalKey, AssetMetadataLocalKey, AssetMetadataName, AssetMetadataSpec,
@@ -22,10 +21,6 @@ type Asset = pallet_asset::Module<TestStorage>;
 fn register_multiple_global_metadata() {
     ExtBuilder::default().build().execute_with(|| {
         assert_eq!(CurrentAssetMetadataGlobalKey::get(), None);
-        assert_eq!(
-            AssetMetadataNextGlobalKey::get(),
-            AssetMetadataGlobalKey::default()
-        );
 
         let asset_metadata_name = AssetMetadataName(b"MyGlobalKey".to_vec());
         let asset_metadata_spec = AssetMetadataSpec::default();
@@ -39,7 +34,6 @@ fn register_multiple_global_metadata() {
             CurrentAssetMetadataGlobalKey::get(),
             Some(AssetMetadataGlobalKey(1))
         );
-        assert_eq!(AssetMetadataNextGlobalKey::get(), AssetMetadataGlobalKey(1));
         assert_eq!(
             AssetMetadataGlobalNameToKey::get(asset_metadata_name.clone()),
             Some(AssetMetadataGlobalKey(1))
@@ -64,7 +58,6 @@ fn register_multiple_global_metadata() {
             CurrentAssetMetadataGlobalKey::get(),
             Some(AssetMetadataGlobalKey(2))
         );
-        assert_eq!(AssetMetadataNextGlobalKey::get(), AssetMetadataGlobalKey(2));
     })
 }
 
@@ -88,10 +81,6 @@ fn register_multiple_local_metadata() {
             CurrentAssetMetadataLocalKey::get(asset_id),
             Some(AssetMetadataLocalKey(1))
         );
-        assert_eq!(
-            AssetMetadataNextLocalKey::get(asset_id),
-            AssetMetadataLocalKey(1)
-        );
         assert_eq!(
             AssetMetadataLocalNameToKey::get(asset_id, asset_metadata_name.clone()),
             Some(AssetMetadataLocalKey(1))
@@ -117,9 +106,5 @@ fn register_multiple_local_metadata() {
             CurrentAssetMetadataLocalKey::get(asset_id),
             Some(AssetMetadataLocalKey(2))
         );
-        assert_eq!(
-            AssetMetadataNextLocalKey::get(asset_id),
-            AssetMetadataLocalKey(2)
-        );
     })
 }

pallets/runtime/tests/src/nft.rs

@@ -4,7 +4,7 @@ use frame_support::{assert_noop, assert_ok, StorageDoubleMap, StorageMap};
 
 use pallet_nft::{
     Collection, CollectionKeys, CurrentCollectionId, CurrentNFTId, MetadataValue, NFTOwner,
-    NFTsInCollection, NextCollectionId, NextNFTId, NumberOfNFTs,
+    NFTsInCollection, NumberOfNFTs,
 };
 use pallet_portfolio::PortfolioNFT;
 use polymesh_common_utilities::traits::nft::Event;
@@ -209,7 +209,6 @@ pub(crate) fn create_nft_collection(
     ));
     assert!(Collection::contains_key(NFTCollectionId(1)));
     assert_eq!(CollectionKeys::get(NFTCollectionId(1)).len(), n_keys);
-    assert_eq!(NextCollectionId::get(), NFTCollectionId(1));
     assert_eq!(CurrentCollectionId::get(), Some(NFTCollectionId(1)));
 
     asset_id
@@ -424,7 +423,6 @@ fn mint_nft_successfully() {
             NFTOwner::get(asset_id, NFTId(1)),
             Some(alice_default_portfolio)
         );
-        assert_eq!(NextNFTId::get(NFTCollectionId(1)), NFTId(1));
         assert_eq!(CurrentNFTId::get(NFTCollectionId(1)), Some(NFTId(1)));
     });
 }
@@ -578,9 +576,7 @@ fn burn_nft() {
             (&asset_id, NFTId(1))
         ),);
         assert_eq!(NFTOwner::get(asset_id, NFTId(1)), None);
-        assert_eq!(NextNFTId::get(NFTCollectionId(1)), NFTId(1));
         assert_eq!(CurrentNFTId::get(NFTCollectionId(1)), Some(NFTId(1)));
-        assert_eq!(NextCollectionId::get(), NFTCollectionId(1));
         assert_eq!(CurrentCollectionId::get(), Some(NFTCollectionId(1)));
     });
 }

pallets/settlement/src/lib.rs

@@ -1589,6 +1589,7 @@ impl<T: Config> Module<T> {
             call,
         ) {
             Self::deposit_event(RawEvent::SchedulingFailed(
+                id,
                 Error::<T>::FailedToSchedule.into(),
             ));
         }
@@ -2575,26 +2576,32 @@ impl<T: Config> Module<T> {
     }
 
     /// Returns an instance of [`ExecuteInstructionInfo`].
-    pub fn execute_instruction_info(instruction_id: &InstructionId) -> ExecuteInstructionInfo {
+    pub fn execute_instruction_info(
+        instruction_id: &InstructionId,
+    ) -> Option<ExecuteInstructionInfo> {
+        if !InstructionDetails::<T>::contains_key(instruction_id) {
+            return None;
+        }
+
         let caller_did = SettlementDID.as_id();
         let instruction_asset_count = Self::get_instruction_asset_count(instruction_id);
         let mut weight_meter =
-            WeightMeter::max_limit(Self::execute_scheduled_instruction_minimum_weight());
+            WeightMeter::max_limit(Self::execute_manual_instruction_minimum_weight());
         match Self::execute_instruction_retryable(*instruction_id, caller_did, &mut weight_meter) {
-            Ok(_) => ExecuteInstructionInfo::new(
+            Ok(_) => Some(ExecuteInstructionInfo::new(
                 instruction_asset_count.fungible(),
                 instruction_asset_count.non_fungible(),
                 instruction_asset_count.off_chain(),
                 weight_meter.consumed(),
                 None,
-            ),
-            Err(e) => ExecuteInstructionInfo::new(
+            )),
+            Err(e) => Some(ExecuteInstructionInfo::new(
                 instruction_asset_count.fungible(),
                 instruction_asset_count.non_fungible(),
                 instruction_asset_count.off_chain(),
                 weight_meter.consumed(),
                 Some(e.into()),
-            ),
+            )),
         }
     }
 

rpc/runtime-api/src/settlement.rs

@@ -24,6 +24,7 @@ use polymesh_primitives::settlement::{
 use polymesh_primitives::PortfolioId;
 
 sp_api::decl_runtime_apis! {
+    #[api_version(2)]
     pub trait SettlementApi {
         /// Returns an [`ExecuteInstructionInfo`] instance containing the consumed weight and the number of fungible and non fungible
         /// tokens in the instruction. Executing an instruction includes verifying the compliance and transfer restrictions of all assets
@@ -37,7 +38,7 @@ sp_api::decl_runtime_apis! {
         ///     "params": [1]
         ///   }'
         /// ```
-        fn get_execute_instruction_info(instruction_id: &InstructionId) -> ExecuteInstructionInfo;
+        fn get_execute_instruction_info(instruction_id: &InstructionId) -> Option<ExecuteInstructionInfo>;
 
         /// Returns an [`AffirmationCount`] instance containing the number of assets being sent/received from `portfolios`,
         /// and the number of off-chain assets in the instruction.

rpc/src/settlement.rs

@@ -39,7 +39,7 @@ pub trait SettlementApi<BlockHash> {
         &self,
         instruction_id: InstructionId,
         at: Option<BlockHash>,
-    ) -> RpcResult<ExecuteInstructionInfo>;
+    ) -> RpcResult<Option<ExecuteInstructionInfo>>;
 
     #[method(name = "settlement_getAffirmationCount")]
     fn get_affirmation_count(
@@ -91,7 +91,7 @@ where
         &self,
         instruction_id: InstructionId,
         at: Option<<Block as BlockT>::Hash>,
-    ) -> RpcResult<ExecuteInstructionInfo> {
+    ) -> RpcResult<Option<ExecuteInstructionInfo>> {
         let api = self.client.runtime_api();
         // If the block hash is not supplied assume the best block.
         let at_hash = at.unwrap_or_else(|| self.client.info().best_hash);