Nft fixes (#1719)

* Fix NFTs * Update max keys to 20 * Fix benchmarks
PolymeshAssociation · Sep 13, 2024 · 838e5a7 · 838e5a7
1 parent 7e570da
commit 838e5a7
Show file tree

Hide file tree

Showing 10 changed files with 161 additions and 31 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "polymesh"
-version = "6.3.3"
+version = "6.3.4"
 authors = ["PolymeshAssociation"]
 build = "build.rs"
 edition = "2021"

diff --git a/pallets/nft/src/benchmarking.rs b/pallets/nft/src/benchmarking.rs
@@ -18,7 +18,7 @@ use polymesh_primitives::{IdentityId, PortfolioKind, WeightMeter};
 
 use crate::*;
 
-const MAX_COLLECTION_KEYS: u32 = 255;
+const MAX_COLLECTION_KEYS: u32 = 20;
 
 /// Creates an NFT collection with `n` global metadata keys.
 fn create_collection<T: Config>(

diff --git a/pallets/nft/src/lib.rs b/pallets/nft/src/lib.rs
@@ -1,7 +1,10 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 use codec::{Decode, Encode};
-use frame_support::dispatch::{DispatchError, DispatchResult};
+use frame_support::dispatch::{
+    DispatchError, DispatchResult, DispatchResultWithPostInfo, PostDispatchInfo,
+};
+use frame_support::storage::child::KillStorageResult;
 use frame_support::storage::StorageDoubleMap;
 use frame_support::traits::Get;
 use frame_support::weights::Weight;
@@ -51,7 +54,8 @@ decl_storage!(
         pub CollectionKeys get(fn collection_keys): map hasher(blake2_128_concat) NFTCollectionId => BTreeSet<AssetMetadataKey>;
 
         /// The metadata value of an nft given its collection id, token id and metadata key.
-        pub MetadataValue get(fn metadata_value): double_map hasher(blake2_128_concat) (NFTCollectionId, NFTId), hasher(blake2_128_concat) AssetMetadataKey => AssetMetadataValue;
+        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]
@@ -156,7 +160,7 @@ decl_module! {
         /// * Asset
         /// * Portfolio
         #[weight = <T as Config>::WeightInfo::redeem_nft(T::MaxNumberOfCollectionKeys::get() as u32)]
-        pub fn redeem_nft(origin, ticker: Ticker, nft_id: NFTId, portfolio_kind: PortfolioKind) -> DispatchResult {
+        pub fn redeem_nft(origin, ticker: Ticker, nft_id: NFTId, portfolio_kind: PortfolioKind) -> DispatchResultWithPostInfo {
             Self::base_redeem_nft(origin, ticker, nft_id, portfolio_kind)
         }
 
@@ -241,6 +245,8 @@ decl_error! {
         InvalidNFTTransferInvalidSenderCDD,
         /// Ticker and NFT ticker don't match
         InvalidNFTTransferInconsistentTicker,
+        /// The NFT is locked.
+        NFTIsLocked
     }
 }
 
@@ -404,7 +410,7 @@ impl<T: Config> Module<T> {
         ticker: Ticker,
         nft_id: NFTId,
         portfolio_kind: PortfolioKind,
-    ) -> DispatchResult {
+    ) -> DispatchResultWithPostInfo {
         // Verifies if the collection exists
         let collection_id =
             CollectionTicker::try_get(&ticker).map_err(|_| Error::<T>::CollectionNotFound)?;
@@ -422,6 +428,10 @@ impl<T: Config> Module<T> {
             PortfolioNFT::contains_key(&caller_portfolio, (&ticker, &nft_id)),
             Error::<T>::NFTNotFound
         );
+        ensure!(
+            !PortfolioLockedNFT::contains_key(&caller_portfolio, (&ticker, &nft_id)),
+            Error::<T>::NFTIsLocked
+        );
 
         // Burns the NFT
         let new_supply = NFTsInCollection::get(&ticker)
@@ -433,9 +443,14 @@ impl<T: Config> Module<T> {
         NFTsInCollection::insert(&ticker, new_supply);
         NumberOfNFTs::insert(&ticker, &caller_portfolio.did, new_balance);
         PortfolioNFT::remove(&caller_portfolio, (&ticker, &nft_id));
-        #[allow(deprecated)]
-        MetadataValue::remove_prefix((&collection_id, &nft_id), None);
         NFTOwner::remove(ticker, nft_id);
+        let removed_keys = {
+            #[allow(deprecated)]
+            match MetadataValue::remove_prefix((&collection_id, &nft_id), None) {
+                KillStorageResult::AllRemoved(n) => n,
+                KillStorageResult::SomeRemaining(n) => n,
+            }
+        };
 
         Self::deposit_event(Event::NFTPortfolioUpdated(
             caller_portfolio.did,
@@ -444,7 +459,9 @@ impl<T: Config> Module<T> {
             None,
             PortfolioUpdateReason::Redeemed,
         ));
-        Ok(())
+        Ok(PostDispatchInfo::from(Some(
+            <T as Config>::WeightInfo::redeem_nft(removed_keys),
+        )))
     }
 
     /// Tranfer ownership of all NFTs.

diff --git a/pallets/runtime/develop/src/runtime.rs b/pallets/runtime/develop/src/runtime.rs
@@ -57,7 +57,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     authoring_version: 1,
     // `spec_version: aaa_bbb_ccd` should match node version v`aaa.bbb.cc`
     // N.B. `d` is unpinned from the binary version
-    spec_version: 6_003_030,
+    spec_version: 6_003_040,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 4,
@@ -140,7 +140,7 @@ parameter_types! {
     pub MaxOutLen: u32 = 8 * 1024;
 
     // NFT:
-    pub const MaxNumberOfCollectionKeys: u8 = u8::MAX;
+    pub const MaxNumberOfCollectionKeys: u8 = 20;
 
     // Portfolio:
     pub const MaxNumberOfFungibleMoves: u32 = 10;

diff --git a/pallets/runtime/mainnet/src/runtime.rs b/pallets/runtime/mainnet/src/runtime.rs
@@ -53,7 +53,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     authoring_version: 1,
     // `spec_version: aaa_bbb_ccd` should match node version v`aaa.bbb.cc`
     // N.B. `d` is unpinned from the binary version
-    spec_version: 6_003_030,
+    spec_version: 6_003_040,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 4,
@@ -135,7 +135,7 @@ parameter_types! {
     pub MaxOutLen: u32 = 8 * 1024;
 
     // NFT:
-    pub const MaxNumberOfCollectionKeys: u8 = u8::MAX;
+    pub const MaxNumberOfCollectionKeys: u8 = 20;
 
     // Portfolio:
     pub const MaxNumberOfFungibleMoves: u32 = 10;

diff --git a/pallets/runtime/testnet/src/runtime.rs b/pallets/runtime/testnet/src/runtime.rs
@@ -55,7 +55,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     authoring_version: 1,
     // `spec_version: aaa_bbb_ccd` should match node version v`aaa.bbb.cc`
     // N.B. `d` is unpinned from the binary version
-    spec_version: 6_003_030,
+    spec_version: 6_003_040,
     impl_version: 0,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 4,
@@ -138,7 +138,7 @@ parameter_types! {
     pub MaxOutLen: u32 = 8 * 1024;
 
     // NFT:
-    pub const MaxNumberOfCollectionKeys: u8 = u8::MAX;
+    pub const MaxNumberOfCollectionKeys: u8 = 20;
 
     // Portfolio:
     pub const MaxNumberOfFungibleMoves: u32 = 10;

diff --git a/pallets/runtime/tests/src/nft.rs b/pallets/runtime/tests/src/nft.rs
@@ -14,7 +14,7 @@ use polymesh_primitives::asset_metadata::{
     AssetMetadataKey, AssetMetadataLocalKey, AssetMetadataName, AssetMetadataSpec,
     AssetMetadataValue,
 };
-use polymesh_primitives::settlement::InstructionId;
+use polymesh_primitives::settlement::{InstructionId, Leg, SettlementType};
 use polymesh_primitives::{
     AuthorizationData, Claim, ClaimType, Condition, ConditionType, CountryCode, IdentityId,
     NFTCollectionId, NFTCollectionKeys, NFTId, NFTMetadataAttribute, NFTs, PortfolioId,
@@ -35,6 +35,7 @@ type NFT = pallet_nft::Module<TestStorage>;
 type NFTError = pallet_nft::Error<TestStorage>;
 type Portfolio = pallet_portfolio::Module<TestStorage>;
 type PortfolioError = pallet_portfolio::Error<TestStorage>;
+type Settlement = pallet_settlement::Module<TestStorage>;
 type System = frame_system::Pallet<TestStorage>;
 
 /// Successfully creates an NFT collection and an Asset.
@@ -1176,3 +1177,117 @@ fn controller_transfer_unauthorized_agent2() {
         );
     });
 }
+
+#[test]
+fn redeem_locked_nft() {
+    ExtBuilder::default().build().execute_with(|| {
+        // First we need to create a collection and mint one NFT
+        let bob: User = User::new(AccountKeyring::Bob);
+        let alice: User = User::new(AccountKeyring::Alice);
+        let ticker: Ticker = Ticker::from_slice_truncated(b"TICKER".as_ref());
+        let legs: Vec<Leg> = vec![Leg::NonFungible {
+            sender: PortfolioId::default_portfolio(alice.did),
+            receiver: PortfolioId::default_portfolio(bob.did),
+            nfts: NFTs::new_unverified(ticker, vec![NFTId(1)]),
+        }];
+
+        create_nft_collection(
+            alice.clone(),
+            ticker.clone(),
+            AssetType::NonFungible(NonFungibleType::Derivative),
+            Vec::new().into(),
+        );
+        mint_nft(
+            alice.clone(),
+            ticker.clone(),
+            Vec::new(),
+            PortfolioKind::Default,
+        );
+
+        let venue_id = Settlement::venue_counter();
+        assert_ok!(Settlement::create_venue(
+            alice.origin(),
+            Default::default(),
+            vec![alice.acc()],
+            Default::default(),
+        ));
+
+        assert_ok!(Settlement::add_and_affirm_instruction(
+            alice.origin(),
+            venue_id,
+            SettlementType::SettleOnAffirmation,
+            None,
+            None,
+            legs,
+            vec![PortfolioId::default_portfolio(alice.did)],
+            None,
+        ));
+
+        assert_noop!(
+            NFT::redeem_nft(alice.origin(), ticker, NFTId(1), PortfolioKind::Default),
+            NFTError::NFTIsLocked
+        );
+    });
+}
+
+#[test]
+fn reject_instruction_with_locked_asset() {
+    ExtBuilder::default().build().execute_with(|| {
+        // First we need to create a collection and mint one NFT
+        let bob: User = User::new(AccountKeyring::Bob);
+        let alice: User = User::new(AccountKeyring::Alice);
+        let ticker: Ticker = Ticker::from_slice_truncated(b"TICKER".as_ref());
+        let legs: Vec<Leg> = vec![Leg::NonFungible {
+            sender: PortfolioId::default_portfolio(alice.did),
+            receiver: PortfolioId::default_portfolio(bob.did),
+            nfts: NFTs::new_unverified(ticker, vec![NFTId(1)]),
+        }];
+
+        create_nft_collection(
+            alice.clone(),
+            ticker.clone(),
+            AssetType::NonFungible(NonFungibleType::Derivative),
+            Vec::new().into(),
+        );
+        mint_nft(
+            alice.clone(),
+            ticker.clone(),
+            Vec::new(),
+            PortfolioKind::Default,
+        );
+
+        let venue_id = Settlement::venue_counter();
+        assert_ok!(Settlement::create_venue(
+            alice.origin(),
+            Default::default(),
+            vec![alice.acc()],
+            Default::default(),
+        ));
+
+        assert_ok!(Settlement::add_and_affirm_instruction(
+            alice.origin(),
+            venue_id,
+            SettlementType::SettleOnAffirmation,
+            None,
+            None,
+            legs,
+            vec![PortfolioId::default_portfolio(alice.did)],
+            None,
+        ));
+
+        // Force token redemption
+        pallet_portfolio::PortfolioLockedNFT::remove(
+            PortfolioId::default_portfolio(alice.did),
+            (ticker, NFTId(1)),
+        );
+
+        assert_noop!(
+            Settlement::reject_instruction(
+                alice.origin(),
+                InstructionId(0),
+                PortfolioId::default_portfolio(alice.did),
+            ),
+            PortfolioError::NFTNotLocked
+        );
+    });
+}
diff --git a/pallets/runtime/tests/src/storage.rs b/pallets/runtime/tests/src/storage.rs
@@ -226,7 +226,7 @@ parameter_types! {
     pub const MaxPeerDataEncodingSize: u32 = 1_000;
     pub const ReportLongevity: u64 =
         BondingDuration::get() as u64 * SessionsPerEra::get() as u64 * EpochDuration::get();
-    pub const MaxNumberOfCollectionKeys: u8 = u8::MAX;
+    pub const MaxNumberOfCollectionKeys: u8 = 20;
     pub const MaxNumberOfFungibleMoves: u32 = 10;
     pub const MaxNumberOfNFTsMoves: u32 = 100;
     pub const MaxNumberOfOffChainAssets: u32 = 10;

diff --git a/pallets/settlement/src/lib.rs b/pallets/settlement/src/lib.rs
@@ -1252,9 +1252,12 @@ impl<T: Config> Module<T> {
             let instruction_details = InstructionDetails::<T>::take(instruction_id);
             Self::ensure_allowed_venue(&instruction_legs, instruction_details.venue_id)?;
 
-            // Attempts to release the locks and transfer all fungible an non fungible assets
+            // Attempts to release the locks
+            Self::release_locks(instruction_id, &instruction_legs)?;
+
+            // Transfer all fungible an non fungible assets
             let instruction_memo = InstructionMemos::get(&instruction_id);
-            match Self::release_asset_locks_and_transfer_pending_legs(
+            match Self::transfer_pending_legs(
                 instruction_id,
                 &instruction_legs,
                 instruction_memo,
@@ -1377,14 +1380,13 @@ impl<T: Config> Module<T> {
         Ok(())
     }
 
-    fn release_asset_locks_and_transfer_pending_legs(
+    fn transfer_pending_legs(
         instruction_id: InstructionId,
         instruction_legs: &[(LegId, Leg)],
         instruction_memo: Option<Memo>,
         caller_did: IdentityId,
         weight_meter: &mut WeightMeter,
     ) -> Result<(), LegId> {
-        Self::unchecked_release_locks(instruction_id, instruction_legs);
         for (leg_id, leg) in instruction_legs {
             if Self::instruction_leg_status(instruction_id, leg_id) == LegStatus::ExecutionPending {
                 match leg {
@@ -1520,17 +1522,13 @@ impl<T: Config> Module<T> {
         Ok(filtered_legs)
     }
 
-    fn unchecked_release_locks(id: InstructionId, instruction_legs: &[(LegId, Leg)]) {
+    fn release_locks(id: InstructionId, instruction_legs: &[(LegId, Leg)]) -> DispatchResult {
         for (leg_id, leg) in instruction_legs {
-            match Self::instruction_leg_status(id, leg_id) {
-                LegStatus::ExecutionPending => {
-                    // This can never return an error since the settlement module
-                    // must've locked these tokens when instruction was affirmed
-                    let _ = Self::unlock_via_leg(&leg);
-                }
-                LegStatus::ExecutionToBeSkipped(_, _) | LegStatus::PendingTokenLock => {}
+            if let LegStatus::ExecutionPending = Self::instruction_leg_status(id, leg_id) {
+                Self::unlock_via_leg(&leg)?;
             }
         }
+        Ok(())
     }
 
     /// Schedule a given instruction to be executed on the next block only if the
@@ -1911,7 +1909,7 @@ impl<T: Config> Module<T> {
             }
         };
         // All checks have been made - write to storage
-        Self::unchecked_release_locks(instruction_id, &legs);
+        Self::release_locks(instruction_id, &legs)?;
         let _ = T::Scheduler::cancel_named(instruction_id.execution_name());
         // Remove all data from storage
         Self::prune_rejected_instruction(instruction_id);