From e5cc1de8208e6d3461b40b97ed5cf4338ad96010 Mon Sep 17 00:00:00 2001 From: Henrique Nogara Date: Mon, 19 Aug 2024 09:55:05 -0300 Subject: [PATCH] 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 | 11 ------ pallets/asset/src/migrations.rs | 22 +++++------ pallets/common/src/traits/settlement.rs | 2 +- pallets/nft/src/lib.rs | 37 ------------------- pallets/nft/src/migrations.rs | 13 +++++++ pallets/runtime/common/src/runtime.rs | 2 +- .../src/asset_pallet/register_metadata.rs | 17 +-------- pallets/runtime/tests/src/nft.rs | 6 +-- pallets/settlement/src/lib.rs | 19 +++++++--- rpc/runtime-api/src/settlement.rs | 3 +- rpc/src/settlement.rs | 4 +- 11 files changed, 44 insertions(+), 92 deletions(-) diff --git a/pallets/asset/src/lib.rs b/pallets/asset/src/lib.rs index a7f3ba37fa..068dc0c872 100644 --- a/pallets/asset/src/lib.rs +++ b/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; - /// 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 Module { // 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 Module { // 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); diff --git a/pallets/asset/src/migrations.rs b/pallets/asset/src/migrations.rs index 11b2fa1f83..819fabd87d 100644 --- a/pallets/asset/src/migrations.rs +++ b/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; - // 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; + + // 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() { }); 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() { TickerAssetID::insert(ticker, asset_id); } log::info!("{:?} items migrated", count); + + log::info!("AssetMetadataNextGlobalKey has been cleared"); + v4::AssetMetadataNextGlobalKey::kill(); } diff --git a/pallets/common/src/traits/settlement.rs b/pallets/common/src/traits/settlement.rs index a09ec32155..da10f98714 100644 --- a/pallets/common/src/traits/settlement.rs +++ b/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), diff --git a/pallets/nft/src/lib.rs b/pallets/nft/src/lib.rs index 72a6b504b0..54d3f3c0b9 100644 --- a/pallets/nft/src/lib.rs +++ b/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 Module { // 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 Module { .checked_add(1) .ok_or(Error::::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 NFTTrait for Module { Module::::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() { - RuntimeLogger::init(); - log::info!(">>> Initializing CurrentNFTId and CurrentCollectionId storage"); - initialize_storage::(); - log::info!(">>> Storage has been initialized"); - } - - fn initialize_storage() { - 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); - } - } -} diff --git a/pallets/nft/src/migrations.rs b/pallets/nft/src/migrations.rs index 8ba9bd9cb5..2553d70d9d 100644 --- a/pallets/nft/src/migrations.rs +++ b/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; + + // 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() { 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); } diff --git a/pallets/runtime/common/src/runtime.rs b/pallets/runtime/common/src/runtime.rs index 482dbe66d4..aa98a8a3b7 100644 --- a/pallets/runtime/common/src/runtime.rs +++ b/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 { Settlement::execute_instruction_info(instruction_id) } diff --git a/pallets/runtime/tests/src/asset_pallet/register_metadata.rs b/pallets/runtime/tests/src/asset_pallet/register_metadata.rs index b6501b151b..2dc7f3ff93 100644 --- a/pallets/runtime/tests/src/asset_pallet/register_metadata.rs +++ b/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; 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) - ); }) } diff --git a/pallets/runtime/tests/src/nft.rs b/pallets/runtime/tests/src/nft.rs index 2dcc783f98..0aeb82ffa4 100644 --- a/pallets/runtime/tests/src/nft.rs +++ b/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))); }); } diff --git a/pallets/settlement/src/lib.rs b/pallets/settlement/src/lib.rs index b41921bf0e..4c82fb54e4 100644 --- a/pallets/settlement/src/lib.rs +++ b/pallets/settlement/src/lib.rs @@ -1589,6 +1589,7 @@ impl Module { call, ) { Self::deposit_event(RawEvent::SchedulingFailed( + id, Error::::FailedToSchedule.into(), )); } @@ -2575,26 +2576,32 @@ impl Module { } /// Returns an instance of [`ExecuteInstructionInfo`]. - pub fn execute_instruction_info(instruction_id: &InstructionId) -> ExecuteInstructionInfo { + pub fn execute_instruction_info( + instruction_id: &InstructionId, + ) -> Option { + if !InstructionDetails::::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()), - ), + )), } } diff --git a/rpc/runtime-api/src/settlement.rs b/rpc/runtime-api/src/settlement.rs index 5956c2df0a..41f3dfa268 100644 --- a/rpc/runtime-api/src/settlement.rs +++ b/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; /// Returns an [`AffirmationCount`] instance containing the number of assets being sent/received from `portfolios`, /// and the number of off-chain assets in the instruction. diff --git a/rpc/src/settlement.rs b/rpc/src/settlement.rs index 7ba4236423..6858d25932 100644 --- a/rpc/src/settlement.rs +++ b/rpc/src/settlement.rs @@ -39,7 +39,7 @@ pub trait SettlementApi { &self, instruction_id: InstructionId, at: Option, - ) -> RpcResult; + ) -> RpcResult>; #[method(name = "settlement_getAffirmationCount")] fn get_affirmation_count( @@ -91,7 +91,7 @@ where &self, instruction_id: InstructionId, at: Option<::Hash>, - ) -> RpcResult { + ) -> RpcResult> { 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);