From 09fd84b1c2cd5d41921bf068e7fc9eec68d6d117 Mon Sep 17 00:00:00 2001 From: hashwarlock Date: Fri, 14 Oct 2022 11:06:51 -0500 Subject: [PATCH 1/3] deprecate the new_owner parameter. Add test & update NftInfo owner field to match uniques' owner during nft_send and nft_accept --- pallets/rmrk-core/src/functions.rs | 53 ++++++++---------------------- pallets/rmrk-core/src/lib.rs | 19 ++--------- tests/src/util/tx.ts | 2 +- traits/src/nft.rs | 2 +- 4 files changed, 19 insertions(+), 57 deletions(-) diff --git a/pallets/rmrk-core/src/functions.rs b/pallets/rmrk-core/src/functions.rs index 9836a738..85d50fe1 100644 --- a/pallets/rmrk-core/src/functions.rs +++ b/pallets/rmrk-core/src/functions.rs @@ -618,12 +618,12 @@ where }; sending_nft.owner = new_owner.clone(); - // Nfts::::insert(collection_id, nft_id, sending_nft); if approval_required { Nfts::::try_mutate_exists(collection_id, nft_id, |nft| -> DispatchResult { if let Some(nft) = nft { nft.pending = true; + nft.owner = new_owner.clone(); } Ok(()) })?; @@ -670,42 +670,24 @@ where sender: T::AccountId, collection_id: CollectionId, nft_id: NftId, - new_owner: AccountIdOrCollectionNftTuple, + _new_owner: AccountIdOrCollectionNftTuple, ) -> Result<(T::AccountId, CollectionId, NftId), DispatchError> { + let owner = match pallet_uniques::Pallet::::owner(collection_id, nft_id) { + Some(new_owner) => new_owner, + None => return Err(Error::::NoAvailableNftId.into()), + }; let (root_owner, _root_nft) = Pallet::::lookup_root_owner(collection_id, nft_id)?; // Check ownership ensure!(sender == root_owner, Error::::NoPermission); - // Get NFT info - let _sending_nft = - Nfts::::get(collection_id, nft_id).ok_or(Error::::NoAvailableNftId)?; - - // Prepare acceptance - let new_owner_account = match new_owner.clone() { - AccountIdOrCollectionNftTuple::AccountId(id) => id, - AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid) => { - // Check if NFT target exists - ensure!(Nfts::::contains_key(cid, nid), Error::::NoAvailableNftId); - - // Check if sending to self - ensure!( - (collection_id, nft_id) != (cid, nid), - Error::::CannotSendToDescendentOrSelf - ); - - // Check if collection_id & nft_id are descendent of cid & nid - ensure!( - !Pallet::::is_x_descendent_of_y(cid, nid, collection_id, nft_id), - Error::::CannotSendToDescendentOrSelf - ); - - let (recipient_root_owner, _root_nft) = Pallet::::lookup_root_owner(cid, nid)?; - ensure!(recipient_root_owner == root_owner, Error::::CannotAcceptNonOwnedNft); + // Check NFT exists + ensure!(Pallet::::nft_exists((collection_id, nft_id)), Error::::NoAvailableNftId); - // Convert to virtual account - Pallet::::nft_to_account_id::(cid, nid) - }, + let owner_account = match Pallet::::decode_nft_account_id::(owner.clone()) + { + Some((cid, nid)) => AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid), + None => AccountIdOrCollectionNftTuple::AccountId(owner.clone()), }; Nfts::::try_mutate(collection_id, nft_id, |nft| -> DispatchResult { @@ -715,21 +697,14 @@ where Ok(()) })?; - pallet_uniques::Pallet::::do_transfer( - collection_id, - nft_id, - new_owner_account.clone(), - |_class_details, _details| Ok(()), - )?; - Self::deposit_event(Event::NFTAccepted { sender, - recipient: new_owner, + recipient: owner_account, collection_id, nft_id, }); - Ok((new_owner_account, collection_id, nft_id)) + Ok((owner, collection_id, nft_id)) } fn nft_reject( diff --git a/pallets/rmrk-core/src/lib.rs b/pallets/rmrk-core/src/lib.rs index cf0cfb9a..23358f39 100644 --- a/pallets/rmrk-core/src/lib.rs +++ b/pallets/rmrk-core/src/lib.rs @@ -555,26 +555,13 @@ pub mod pallet { origin: OriginFor, collection_id: CollectionId, nft_id: NftId, - new_owner: AccountIdOrCollectionNftTuple, + _new_owner: AccountIdOrCollectionNftTuple, ) -> DispatchResult { let sender = ensure_signed(origin)?; - let (new_owner_account, collection_id, nft_id) = - Self::nft_accept(sender.clone(), collection_id, nft_id, new_owner.clone())?; - - pallet_uniques::Pallet::::do_transfer( - collection_id, - nft_id, - new_owner_account, - |_class_details, _details| Ok(()), - )?; + let (_owner_account, _collection_id, _nft_id) = + Self::nft_accept(sender.clone(), collection_id, nft_id, _new_owner)?; - Self::deposit_event(Event::NFTAccepted { - sender, - recipient: new_owner.clone(), - collection_id, - nft_id, - }); Ok(()) } diff --git a/tests/src/util/tx.ts b/tests/src/util/tx.ts index 7d1063ff..7adb3f21 100644 --- a/tests/src/util/tx.ts +++ b/tests/src/util/tx.ts @@ -551,7 +551,7 @@ export async function acceptNft( let nftBeforeOpt = await getNft(api, collectionId, nftId); - const tx = api.tx.rmrkCore.acceptNft(collectionId, nftId, newOwnerObj); + const tx = api.tx.rmrkCore.acceptNft(collectionId, nftId); const events = await executeTransaction(api, issuer, tx); const acceptResult = extractRmrkCoreTxResult( diff --git a/traits/src/nft.rs b/traits/src/nft.rs index efb603f3..72e3c53b 100644 --- a/traits/src/nft.rs +++ b/traits/src/nft.rs @@ -113,7 +113,7 @@ pub trait Nft { sender: AccountId, collection_id: CollectionId, nft_id: NftId, - new_owner: AccountIdOrCollectionNftTuple, + _new_owner: AccountIdOrCollectionNftTuple, ) -> Result<(AccountId, CollectionId, NftId), DispatchError>; fn nft_reject( sender: AccountId, From 5b931ab2bdf9732fef369b23cfdced79f3222390 Mon Sep 17 00:00:00 2001 From: HashWarlock Date: Fri, 14 Oct 2022 17:19:54 -0500 Subject: [PATCH 2/3] Fix integration test --- tests/src/util/tx.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/util/tx.ts b/tests/src/util/tx.ts index 7adb3f21..7d1063ff 100644 --- a/tests/src/util/tx.ts +++ b/tests/src/util/tx.ts @@ -551,7 +551,7 @@ export async function acceptNft( let nftBeforeOpt = await getNft(api, collectionId, nftId); - const tx = api.tx.rmrkCore.acceptNft(collectionId, nftId); + const tx = api.tx.rmrkCore.acceptNft(collectionId, nftId, newOwnerObj); const events = await executeTransaction(api, issuer, tx); const acceptResult = extractRmrkCoreTxResult( From 6e936c5709f6352dc009ca2c051b6771e1935409 Mon Sep 17 00:00:00 2001 From: hashwarlock Date: Mon, 17 Oct 2022 13:01:12 -0500 Subject: [PATCH 3/3] Use new_owner as a storage check and move permissions logic to user facing function. Add integration test --- pallets/rmrk-core/src/functions.rs | 23 ++++-------- pallets/rmrk-core/src/lib.rs | 22 ++++++++++-- pallets/rmrk-core/src/tests.rs | 41 ++++++++++++++++----- tests/src/acceptNft.test.ts | 57 ++++++++++++++++++++++++++++++ traits/src/nft.rs | 2 +- 5 files changed, 118 insertions(+), 27 deletions(-) diff --git a/pallets/rmrk-core/src/functions.rs b/pallets/rmrk-core/src/functions.rs index 85d50fe1..d00a36ff 100644 --- a/pallets/rmrk-core/src/functions.rs +++ b/pallets/rmrk-core/src/functions.rs @@ -670,24 +670,15 @@ where sender: T::AccountId, collection_id: CollectionId, nft_id: NftId, - _new_owner: AccountIdOrCollectionNftTuple, + new_owner: AccountIdOrCollectionNftTuple, ) -> Result<(T::AccountId, CollectionId, NftId), DispatchError> { - let owner = match pallet_uniques::Pallet::::owner(collection_id, nft_id) { - Some(new_owner) => new_owner, - None => return Err(Error::::NoAvailableNftId.into()), - }; - let (root_owner, _root_nft) = Pallet::::lookup_root_owner(collection_id, nft_id)?; - - // Check ownership - ensure!(sender == root_owner, Error::::NoPermission); - // Check NFT exists ensure!(Pallet::::nft_exists((collection_id, nft_id)), Error::::NoAvailableNftId); - let owner_account = match Pallet::::decode_nft_account_id::(owner.clone()) - { - Some((cid, nid)) => AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid), - None => AccountIdOrCollectionNftTuple::AccountId(owner.clone()), + let owner_account = match new_owner.clone() { + AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid) => + Pallet::::nft_to_account_id(cid, nid), + AccountIdOrCollectionNftTuple::AccountId(owner_account) => owner_account, }; Nfts::::try_mutate(collection_id, nft_id, |nft| -> DispatchResult { @@ -699,12 +690,12 @@ where Self::deposit_event(Event::NFTAccepted { sender, - recipient: owner_account, + recipient: new_owner, collection_id, nft_id, }); - Ok((owner, collection_id, nft_id)) + Ok((owner_account, collection_id, nft_id)) } fn nft_reject( diff --git a/pallets/rmrk-core/src/lib.rs b/pallets/rmrk-core/src/lib.rs index 23358f39..a44b808d 100644 --- a/pallets/rmrk-core/src/lib.rs +++ b/pallets/rmrk-core/src/lib.rs @@ -361,6 +361,7 @@ pub mod pallet { // Must unequip an item before sending (this only applies to the // rmrk-equip pallet but the send operation lives in rmrk-core) CannotSendEquippedItem, + CannotAcceptToNewOwner, } #[pallet::call] @@ -555,12 +556,29 @@ pub mod pallet { origin: OriginFor, collection_id: CollectionId, nft_id: NftId, - _new_owner: AccountIdOrCollectionNftTuple, + new_owner: AccountIdOrCollectionNftTuple, ) -> DispatchResult { let sender = ensure_signed(origin)?; + let (root_owner, _root_nft) = Pallet::::lookup_root_owner(collection_id, nft_id)?; + // Check ownership + ensure!(sender == root_owner, Error::::NoPermission); + + let _owner = match pallet_uniques::Pallet::::owner(collection_id, nft_id) { + Some(owner) => { + let owner_account = + match Pallet::::decode_nft_account_id::(owner.clone()) { + Some((cid, nid)) => + AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid), + None => AccountIdOrCollectionNftTuple::AccountId(owner), + }; + ensure!(new_owner == owner_account, Error::::CannotAcceptToNewOwner) + }, + None => return Err(Error::::NoAvailableNftId.into()), + }; + let (_owner_account, _collection_id, _nft_id) = - Self::nft_accept(sender.clone(), collection_id, nft_id, _new_owner)?; + Self::nft_accept(sender.clone(), collection_id, nft_id, new_owner)?; Ok(()) } diff --git a/pallets/rmrk-core/src/tests.rs b/pallets/rmrk-core/src/tests.rs index a4cf56c8..8ef33cd9 100644 --- a/pallets/rmrk-core/src/tests.rs +++ b/pallets/rmrk-core/src/tests.rs @@ -526,6 +526,16 @@ fn send_nft_to_minted_nft_works() { nft_id: 1, approval_required: true, })); + // Bob fails to accept NFT (0,1) for Bob + assert_noop!( + RMRKCore::accept_nft( + Origin::signed(BOB), + 0, + 1, + AccountIdOrCollectionNftTuple::AccountId(BOB), + ), + Error::::CannotAcceptToNewOwner + ); // Bob accepts NFT (0,1) for Bob-owned NFT (0,0) assert_ok!(RMRKCore::accept_nft( Origin::signed(BOB), @@ -555,6 +565,8 @@ fn send_nft_to_minted_nft_works() { nft_id: 2, approval_required: true, })); + // Owner should be the same derived AccountId for NFT (0,2) and nft_to_account_id(0,1) + assert_eq!(UNQ::Pallet::::owner(0, 2), Some(RMRKCore::nft_to_account_id(0, 1)),); // Bob accepts NFT (0,2) for Bob-owned NFT (0,1) assert_ok!(RMRKCore::accept_nft( Origin::signed(BOB), @@ -1435,7 +1447,7 @@ fn resource_removal_works() { // Create Composable resource let composable_resource = ComposableResource { parts: vec![0, 1].try_into().unwrap(), // BoundedVec of Parts - base: base_id, // base_id + base: base_id, // base_id metadata: None, slot: Some((base_id, slot_id)), }; @@ -1450,20 +1462,33 @@ fn resource_removal_works() { )); // Values should now exist in EquippableBases and EquippableSlots - assert!(EquippableBases::::get((COLLECTION_ID_0,NFT_ID_0, base_id)).is_some()); - assert!(EquippableSlots::::get((COLLECTION_ID_0,NFT_ID_0, resource_id, base_id, slot_id)).is_some()); - + assert!(EquippableBases::::get((COLLECTION_ID_0, NFT_ID_0, base_id)).is_some()); + assert!(EquippableSlots::::get(( + COLLECTION_ID_0, + NFT_ID_0, + resource_id, + base_id, + slot_id + )) + .is_some()); + // Remove resource assert_ok!(RMRKCore::remove_resource( Origin::signed(ALICE), COLLECTION_ID_0, NFT_ID_0, - resource_id, + resource_id, )); - assert!(EquippableBases::::get((COLLECTION_ID_0,NFT_ID_0, base_id)).is_none()); - assert!(EquippableSlots::::get((COLLECTION_ID_0,NFT_ID_0, resource_id, base_id, slot_id)).is_none()); - + assert!(EquippableBases::::get((COLLECTION_ID_0, NFT_ID_0, base_id)).is_none()); + assert!(EquippableSlots::::get(( + COLLECTION_ID_0, + NFT_ID_0, + resource_id, + base_id, + slot_id + )) + .is_none()); }); } diff --git a/tests/src/acceptNft.test.ts b/tests/src/acceptNft.test.ts index 17c6cfc4..98f1bb80 100644 --- a/tests/src/acceptNft.test.ts +++ b/tests/src/acceptNft.test.ts @@ -169,6 +169,63 @@ describe("integration test: accept NFT", () => { expect(isChild).to.be.false; }); + it("[negative] accept NFT", async () => { + const ownerAlice = alice; + const ownerBob = bob; + + const aliceCollectionId = await createTestCollection(alice); + const bobCollectionId = await createTestCollection(bob); + + const parentNftId = await mintNft( + api, + 0, + alice, + ownerAlice, + aliceCollectionId, + "parent-nft-metadata" + ); + const childNftId = await mintNft( + api, + 0, + bob, + ownerBob, + bobCollectionId, + "child-nft-metadata" + ); + + const parentNftId2 = await mintNft( + api, + 1, + alice, + ownerAlice, + aliceCollectionId, + "parent-nft-metadata2" + ); + + const newOwnerNFT: NftIdTuple = [aliceCollectionId, parentNftId]; + const notNewOwnerNFT: NftIdTuple = [aliceCollectionId, parentNftId2]; + + await sendNft( + api, + "pending", + ownerBob, + bobCollectionId, + childNftId, + newOwnerNFT + ); + const tx = acceptNft(api, alice, bobCollectionId, childNftId, notNewOwnerNFT); + + await expectTxFailure(/rmrkCore\.CannotAcceptToNewOwner/, tx); + + const isChild = await isNftChildOfAnother( + api, + bobCollectionId, + childNftId, + notNewOwnerNFT + ); + expect(isChild).to.be.false; + }); + after(() => { api.disconnect(); }); diff --git a/traits/src/nft.rs b/traits/src/nft.rs index 72e3c53b..efb603f3 100644 --- a/traits/src/nft.rs +++ b/traits/src/nft.rs @@ -113,7 +113,7 @@ pub trait Nft { sender: AccountId, collection_id: CollectionId, nft_id: NftId, - _new_owner: AccountIdOrCollectionNftTuple, + new_owner: AccountIdOrCollectionNftTuple, ) -> Result<(AccountId, CollectionId, NftId), DispatchError>; fn nft_reject( sender: AccountId,