From 858185d58c581b694dae6d16e759e04ee422a561 Mon Sep 17 00:00:00 2001 From: Henrique Nogara Date: Thu, 19 Sep 2024 05:36:18 -0300 Subject: [PATCH] Return the actual weight when redeeming an NFT (#1725) * Return the actual weight when redeeming an NFT * Run benchmark * Remove deprecated remove_prefix call --- pallets/nft/src/benchmarking.rs | 2 +- pallets/nft/src/lib.rs | 47 +++++++-- pallets/runtime/tests/src/nft.rs | 158 ++++++++++++++++++++++++++++++- pallets/settlement/src/lib.rs | 24 +++-- 4 files changed, 203 insertions(+), 28 deletions(-) diff --git a/pallets/nft/src/benchmarking.rs b/pallets/nft/src/benchmarking.rs index cb1f3256e..e230f39b9 100644 --- a/pallets/nft/src/benchmarking.rs +++ b/pallets/nft/src/benchmarking.rs @@ -183,7 +183,7 @@ benchmarks! { let user = user::("target", 0); let asset_id = create_collection_issue_nfts::(&user, n, 1, PortfolioKind::Default); - }: _(user.origin, asset_id, NFTId(1), PortfolioKind::Default) + }: _(user.origin, asset_id, NFTId(1), PortfolioKind::Default, None) verify { for i in 1..n + 1 { assert!( diff --git a/pallets/nft/src/lib.rs b/pallets/nft/src/lib.rs index 54d3f3c0b..619ab49d8 100644 --- a/pallets/nft/src/lib.rs +++ b/pallets/nft/src/lib.rs @@ -1,7 +1,9 @@ #![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::StorageDoubleMap; use frame_support::traits::Get; use frame_support::weights::Weight; @@ -154,9 +156,20 @@ decl_module! { /// # Permissions /// * Asset /// * Portfolio - #[weight = ::WeightInfo::redeem_nft(T::MaxNumberOfCollectionKeys::get() as u32)] - pub fn redeem_nft(origin, asset_id: AssetID, nft_id: NFTId, portfolio_kind: PortfolioKind) -> DispatchResult { - Self::base_redeem_nft(origin, asset_id, nft_id, portfolio_kind) + #[weight = ::WeightInfo::redeem_nft( + number_of_keys.map_or( + u32::from(T::MaxNumberOfCollectionKeys::get()), + |v| u32::from(v) + ) + )] + pub fn redeem_nft( + origin, + asset_id: AssetID, + nft_id: NFTId, + portfolio_kind: PortfolioKind, + number_of_keys: Option + ) -> DispatchResultWithPostInfo { + Self::base_redeem_nft(origin, asset_id, nft_id, portfolio_kind, number_of_keys) } /// Forces the transfer of NFTs from a given portfolio to the caller's portfolio. @@ -237,7 +250,11 @@ decl_error! { /// The sender has an invalid CDD. InvalidNFTTransferInvalidSenderCDD, /// There's no asset associated to the given asset_id. - InvalidAssetID + InvalidAssetID, + /// The NFT is locked. + NFTIsLocked, + /// The number of keys in the collection is greater than the input. + NumberOfKeysIsLessThanExpected, } } @@ -405,7 +422,8 @@ impl Module { asset_id: AssetID, nft_id: NFTId, portfolio_kind: PortfolioKind, - ) -> DispatchResult { + number_of_keys: Option, + ) -> DispatchResultWithPostInfo { // Verifies if the collection exists let collection_id = CollectionAsset::try_get(&asset_id).map_err(|_| Error::::CollectionNotFound)?; @@ -423,6 +441,10 @@ impl Module { PortfolioNFT::contains_key(&caller_portfolio, (&asset_id, &nft_id)), Error::::NFTNotFound ); + ensure!( + !PortfolioLockedNFT::contains_key(&caller_portfolio, (&asset_id, &nft_id)), + Error::::NFTIsLocked + ); // Burns the NFT let new_supply = NFTsInCollection::get(&asset_id) @@ -434,9 +456,14 @@ impl Module { NFTsInCollection::insert(&asset_id, new_supply); NumberOfNFTs::insert(&asset_id, &caller_portfolio.did, new_balance); PortfolioNFT::remove(&caller_portfolio, (&asset_id, &nft_id)); - #[allow(deprecated)] - MetadataValue::remove_prefix((&collection_id, &nft_id), None); NFTOwner::remove(asset_id, nft_id); + let removed_keys = MetadataValue::drain_prefix((&collection_id, &nft_id)).count(); + if let Some(number_of_keys) = number_of_keys { + ensure!( + usize::from(number_of_keys) >= removed_keys, + Error::::NumberOfKeysIsLessThanExpected, + ); + } Self::deposit_event(Event::NFTPortfolioUpdated( caller_portfolio.did, @@ -445,7 +472,9 @@ impl Module { None, PortfolioUpdateReason::Redeemed, )); - Ok(()) + Ok(PostDispatchInfo::from(Some( + ::WeightInfo::redeem_nft(removed_keys as u32), + ))) } /// Tranfer ownership of all NFTs. diff --git a/pallets/runtime/tests/src/nft.rs b/pallets/runtime/tests/src/nft.rs index 0aeb82ffa..47f898a9b 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, @@ -36,6 +36,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. @@ -453,7 +454,8 @@ fn burn_nft_collection_not_found() { alice.origin(), Asset::generate_asset_id(alice.acc(), false), NFTId(1), - PortfolioKind::Default + PortfolioKind::Default, + None ), NFTError::CollectionNotFound ); @@ -477,7 +479,13 @@ fn burn_nft_not_found() { ); assert_noop!( - NFT::redeem_nft(alice.origin(), asset_id, NFTId(1), PortfolioKind::Default), + NFT::redeem_nft( + alice.origin(), + asset_id, + NFTId(1), + PortfolioKind::Default, + None + ), NFTError::NFTNotFound ); }); @@ -527,7 +535,13 @@ fn burn_nft_no_custody() { .unwrap(); assert_noop!( - NFT::redeem_nft(alice.origin(), asset_id, NFTId(1), PortfolioKind::Default), + NFT::redeem_nft( + alice.origin(), + asset_id, + NFTId(1), + PortfolioKind::Default, + None + ), PortfolioError::UnauthorizedCustodian ); }); @@ -563,7 +577,8 @@ fn burn_nft() { alice.origin(), asset_id, NFTId(1), - PortfolioKind::Default + PortfolioKind::Default, + None )); assert!(!MetadataValue::contains_key( (NFTCollectionId(1), NFTId(1)), @@ -1093,3 +1108,136 @@ fn controller_transfer_nft_not_owned() { ); }); } + +#[test] +fn redeem_wrong_number_of_keys() { + ExtBuilder::default().build().execute_with(|| { + let alice: User = User::new(AccountKeyring::Alice); + + let collection_keys: NFTCollectionKeys = vec![ + AssetMetadataKey::Local(AssetMetadataLocalKey(1)), + AssetMetadataKey::Local(AssetMetadataLocalKey(2)), + ] + .into(); + let asset_id = create_nft_collection( + alice.clone(), + AssetType::NonFungible(NonFungibleType::Derivative), + collection_keys, + ); + let nfts_metadata: Vec = vec![ + NFTMetadataAttribute { + key: AssetMetadataKey::Local(AssetMetadataLocalKey(1)), + value: AssetMetadataValue(b"test".to_vec()), + }, + NFTMetadataAttribute { + key: AssetMetadataKey::Local(AssetMetadataLocalKey(2)), + value: AssetMetadataValue(b"test".to_vec()), + }, + ]; + mint_nft( + alice.clone(), + asset_id, + nfts_metadata, + PortfolioKind::Default, + ); + + assert_noop!( + NFT::redeem_nft( + alice.origin(), + asset_id, + NFTId(1), + PortfolioKind::Default, + Some(1) + ), + NFTError::NumberOfKeysIsLessThanExpected + ); + }); +} + +#[test] +fn redeem_locked_nft() { + ExtBuilder::default().build().execute_with(|| { + let bob: User = User::new(AccountKeyring::Bob); + let alice: User = User::new(AccountKeyring::Alice); + + let asset_id = create_nft_collection( + alice.clone(), + AssetType::NonFungible(NonFungibleType::Derivative), + Vec::new().into(), + ); + mint_nft(alice.clone(), asset_id, Vec::new(), PortfolioKind::Default); + + let legs: Vec = vec![Leg::NonFungible { + sender: PortfolioId::default_portfolio(alice.did), + receiver: PortfolioId::default_portfolio(bob.did), + nfts: NFTs::new_unverified(asset_id, vec![NFTId(1)]), + }]; + assert_ok!(Settlement::add_and_affirm_instruction( + alice.origin(), + None, + SettlementType::SettleOnAffirmation, + None, + None, + legs, + vec![PortfolioId::default_portfolio(alice.did)], + None, + )); + + assert_noop!( + NFT::redeem_nft( + alice.origin(), + asset_id, + NFTId(1), + PortfolioKind::Default, + None + ), + NFTError::NFTIsLocked + ); + }); +} + +#[test] +fn reject_instruction_with_locked_asset() { + ExtBuilder::default().build().execute_with(|| { + let bob: User = User::new(AccountKeyring::Bob); + let alice: User = User::new(AccountKeyring::Alice); + + let asset_id = create_nft_collection( + alice.clone(), + AssetType::NonFungible(NonFungibleType::Derivative), + Vec::new().into(), + ); + mint_nft(alice.clone(), asset_id, Vec::new(), PortfolioKind::Default); + + let legs: Vec = vec![Leg::NonFungible { + sender: PortfolioId::default_portfolio(alice.did), + receiver: PortfolioId::default_portfolio(bob.did), + nfts: NFTs::new_unverified(asset_id, vec![NFTId(1)]), + }]; + assert_ok!(Settlement::add_and_affirm_instruction( + alice.origin(), + None, + SettlementType::SettleOnAffirmation, + None, + None, + legs, + vec![PortfolioId::default_portfolio(alice.did)], + None, + )); + + // Force token redemption + pallet_portfolio::PortfolioLockedNFT::remove( + PortfolioId::default_portfolio(alice.did), + (asset_id, NFTId(1)), + ); + + assert_noop!( + Settlement::reject_instruction( + alice.origin(), + InstructionId(0), + PortfolioId::default_portfolio(alice.did), + ), + PortfolioError::NFTNotLocked + ); + }); +} diff --git a/pallets/settlement/src/lib.rs b/pallets/settlement/src/lib.rs index f1764e3ec..85347eb9f 100644 --- a/pallets/settlement/src/lib.rs +++ b/pallets/settlement/src/lib.rs @@ -1278,9 +1278,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, @@ -1405,14 +1408,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 { @@ -1550,17 +1552,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 @@ -1942,7 +1940,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);