From 838e5a7b01d016e6d3709e69d79afc1573eccdb6 Mon Sep 17 00:00:00 2001 From: Adam Dossa Date: Fri, 13 Sep 2024 15:56:58 +0100 Subject: [PATCH] Nft fixes (#1719) * Fix NFTs * Update max keys to 20 * Fix benchmarks --- Cargo.lock | 2 +- Cargo.toml | 2 +- pallets/nft/src/benchmarking.rs | 2 +- pallets/nft/src/lib.rs | 31 +++++-- pallets/runtime/develop/src/runtime.rs | 4 +- pallets/runtime/mainnet/src/runtime.rs | 4 +- pallets/runtime/testnet/src/runtime.rs | 4 +- pallets/runtime/tests/src/nft.rs | 117 ++++++++++++++++++++++++- pallets/runtime/tests/src/storage.rs | 2 +- pallets/settlement/src/lib.rs | 24 +++-- 10 files changed, 161 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 64ded24001..ea2a2397f2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6516,7 +6516,7 @@ dependencies = [ [[package]] name = "polymesh" -version = "6.3.3" +version = "6.3.4" dependencies = [ "clap 4.4.11", "frame-benchmarking", diff --git a/Cargo.toml b/Cargo.toml index e83196beb8..79289ac17b 100644 --- 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 index 9ba3208c00..5c2aa0c46a 100644 --- 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( diff --git a/pallets/nft/src/lib.rs b/pallets/nft/src/lib.rs index 40d3c6c0d9..79a6b60c49 100644 --- 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; /// 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 = ::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 Module { 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::::CollectionNotFound)?; @@ -422,6 +428,10 @@ impl Module { PortfolioNFT::contains_key(&caller_portfolio, (&ticker, &nft_id)), Error::::NFTNotFound ); + ensure!( + !PortfolioLockedNFT::contains_key(&caller_portfolio, (&ticker, &nft_id)), + Error::::NFTIsLocked + ); // Burns the NFT let new_supply = NFTsInCollection::get(&ticker) @@ -433,9 +443,14 @@ impl Module { 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 Module { None, PortfolioUpdateReason::Redeemed, )); - Ok(()) + Ok(PostDispatchInfo::from(Some( + ::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 index b1ad18b939..9210708a9a 100644 --- 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 index fc2a2b25ff..57b52180d9 100644 --- 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 index da9efcef67..ee11204137 100644 --- 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 index f9b2c802a6..a7d46d44c4 100644 --- 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; type NFTError = pallet_nft::Error; type Portfolio = pallet_portfolio::Module; type PortfolioError = pallet_portfolio::Error; +type Settlement = pallet_settlement::Module; type System = frame_system::Pallet; /// 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 = 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 = 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 index 21349cf32e..9616c2fcce 100644 --- 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 index aac860ae7b..3c4a39a280 100644 --- a/pallets/settlement/src/lib.rs +++ b/pallets/settlement/src/lib.rs @@ -1252,9 +1252,12 @@ impl Module { let instruction_details = InstructionDetails::::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 Module { Ok(()) } - fn release_asset_locks_and_transfer_pending_legs( + fn transfer_pending_legs( instruction_id: InstructionId, instruction_legs: &[(LegId, Leg)], instruction_memo: Option, 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 Module { 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 Module { } }; // 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);