Skip to content

Commit

Permalink
Return the actual weight when redeeming an NFT (#1725)
Browse files Browse the repository at this point in the history 
* Return the actual weight when redeeming an NFT

* Run benchmark

* Remove deprecated remove_prefix call
  • Loading branch information
@HenriqueNogara
HenriqueNogara authored Sep 19, 2024
1 parent 1cb6198 commit 858185d
Show file tree
Hide file tree
Showing 4 changed files with 203 additions and 28 deletions.
2 changes: 1 addition & 1 deletion pallets/nft/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ benchmarks! {
let user = user::<T>("target", 0);
let asset_id = create_collection_issue_nfts::<T>(&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!(
Expand Down
47 changes: 38 additions & 9 deletions pallets/nft/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -154,9 +156,20 @@ decl_module! {
/// # Permissions
/// * Asset
/// * Portfolio
#[weight = <T as Config>::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 = <T as Config>::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<u8>
) -> 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.
Expand Down Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -405,7 +422,8 @@ impl<T: Config> Module<T> {
asset_id: AssetID,
nft_id: NFTId,
portfolio_kind: PortfolioKind,
) -> DispatchResult {
number_of_keys: Option<u8>,
) -> DispatchResultWithPostInfo {
// Verifies if the collection exists
let collection_id =
CollectionAsset::try_get(&asset_id).map_err(|_| Error::<T>::CollectionNotFound)?;
Expand All @@ -423,6 +441,10 @@ impl<T: Config> Module<T> {
PortfolioNFT::contains_key(&caller_portfolio, (&asset_id, &nft_id)),
Error::<T>::NFTNotFound
);
ensure!(
!PortfolioLockedNFT::contains_key(&caller_portfolio, (&asset_id, &nft_id)),
Error::<T>::NFTIsLocked
);

// Burns the NFT
let new_supply = NFTsInCollection::get(&asset_id)
Expand All @@ -434,9 +456,14 @@ impl<T: Config> Module<T> {
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::<T>::NumberOfKeysIsLessThanExpected,
);
}

Self::deposit_event(Event::NFTPortfolioUpdated(
caller_portfolio.did,
Expand All @@ -445,7 +472,9 @@ impl<T: Config> Module<T> {
None,
PortfolioUpdateReason::Redeemed,
));
Ok(())
Ok(PostDispatchInfo::from(Some(
<T as Config>::WeightInfo::redeem_nft(removed_keys as u32),
)))
}

/// Tranfer ownership of all NFTs.
Expand Down
158 changes: 153 additions & 5 deletions pallets/runtime/tests/src/nft.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -36,6 +36,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.
Expand Down Expand Up @@ -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
);
Expand All @@ -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
);
});
Expand Down Expand Up @@ -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
);
});
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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<NFTMetadataAttribute> = 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<Leg> = 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<Leg> = 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
);
});
}
24 changes: 11 additions & 13 deletions pallets/settlement/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1278,9 +1278,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,
Expand Down Expand Up @@ -1405,14 +1408,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 {
Expand Down Expand Up @@ -1550,17 +1552,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
Expand Down Expand Up @@ -1942,7 +1940,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);
Expand Down

0 comments on commit 858185d

Please sign in to comment.