Merge pull request #230 from rmrk-team/bug/229-accept-nft-bug

Fix Bug 229 Send/Accept NFT
rmrk-team · Oct 18, 2022 · 7abb495 · 7abb495
2 parents 44beca9 + 6e936c5
commit 7abb495
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 56 deletions.
diff --git a/pallets/rmrk-core/src/functions.rs b/pallets/rmrk-core/src/functions.rs
@@ -620,12 +620,12 @@ where
 		};
 
 		sending_nft.owner = new_owner.clone();
-		// Nfts::<T>::insert(collection_id, nft_id, sending_nft);
 
 		if approval_required {
 			Nfts::<T>::try_mutate_exists(collection_id, nft_id, |nft| -> DispatchResult {
 				if let Some(nft) = nft {
 					nft.pending = true;
+					nft.owner = new_owner.clone();
 				}
 				Ok(())
 			})?;
@@ -674,40 +674,13 @@ where
 		nft_id: NftId,
 		new_owner: AccountIdOrCollectionNftTuple<T::AccountId>,
 	) -> Result<(T::AccountId, CollectionId, NftId), DispatchError> {
-		let (root_owner, _root_nft) = Pallet::<T>::lookup_root_owner(collection_id, nft_id)?;
-
-		// Check ownership
-		ensure!(sender == root_owner, Error::<T>::NoPermission);
-
-		// Get NFT info
-		let _sending_nft =
-			Nfts::<T>::get(collection_id, nft_id).ok_or(Error::<T>::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::<T>::contains_key(cid, nid), Error::<T>::NoAvailableNftId);
-
-				// Check if sending to self
-				ensure!(
-					(collection_id, nft_id) != (cid, nid),
-					Error::<T>::CannotSendToDescendentOrSelf
-				);
+		// Check NFT exists
+		ensure!(Pallet::<T>::nft_exists((collection_id, nft_id)), Error::<T>::NoAvailableNftId);
 
-				// Check if collection_id & nft_id are descendent of cid & nid
-				ensure!(
-					!Pallet::<T>::is_x_descendent_of_y(cid, nid, collection_id, nft_id),
-					Error::<T>::CannotSendToDescendentOrSelf
-				);
-
-				let (recipient_root_owner, _root_nft) = Pallet::<T>::lookup_root_owner(cid, nid)?;
-				ensure!(recipient_root_owner == root_owner, Error::<T>::CannotAcceptNonOwnedNft);
-
-				// Convert to virtual account
-				Pallet::<T>::nft_to_account_id::<T::AccountId>(cid, nid)
-			},
+		let owner_account = match new_owner.clone() {
+			AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid) =>
+				Pallet::<T>::nft_to_account_id(cid, nid),
+			AccountIdOrCollectionNftTuple::AccountId(owner_account) => owner_account,
 		};
 
 		Nfts::<T>::try_mutate(collection_id, nft_id, |nft| -> DispatchResult {
@@ -717,21 +690,14 @@ where
 			Ok(())
 		})?;
 
-		pallet_uniques::Pallet::<T>::do_transfer(
-			collection_id,
-			nft_id,
-			new_owner_account.clone(),
-			|_class_details, _details| Ok(()),
-		)?;
-
 		Self::deposit_event(Event::NFTAccepted {
 			sender,
 			recipient: new_owner,
 			collection_id,
 			nft_id,
 		});
 
-		Ok((new_owner_account, 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
@@ -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]
@@ -559,22 +560,26 @@ pub mod pallet {
 		) -> 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())?;
+			let (root_owner, _root_nft) = Pallet::<T>::lookup_root_owner(collection_id, nft_id)?;
+			// Check ownership
+			ensure!(sender == root_owner, Error::<T>::NoPermission);
 
-			pallet_uniques::Pallet::<T>::do_transfer(
-				collection_id,
-				nft_id,
-				new_owner_account,
-				|_class_details, _details| Ok(()),
-			)?;
+			let _owner = match pallet_uniques::Pallet::<T>::owner(collection_id, nft_id) {
+				Some(owner) => {
+					let owner_account =
+						match Pallet::<T>::decode_nft_account_id::<T::AccountId>(owner.clone()) {
+							Some((cid, nid)) =>
+								AccountIdOrCollectionNftTuple::CollectionAndNftTuple(cid, nid),
+							None => AccountIdOrCollectionNftTuple::AccountId(owner),
+						};
+					ensure!(new_owner == owner_account, Error::<T>::CannotAcceptToNewOwner)
+				},
+				None => return Err(Error::<T>::NoAvailableNftId.into()),
+			};
+
+			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/pallets/rmrk-core/src/tests.rs b/pallets/rmrk-core/src/tests.rs
@@ -540,6 +540,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::<Test>::CannotAcceptToNewOwner
+		);
 		// Bob accepts NFT (0,1) for Bob-owned NFT (0,0)
 		assert_ok!(RMRKCore::accept_nft(
 			Origin::signed(BOB),
@@ -569,6 +579,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::<Test>::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),

diff --git 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();
   });