Use new_owner as a storage check and move permissions logic to user f…

…acing function. Add integration test
rmrk-team · Oct 17, 2022 · 6e936c5 · 6e936c5
1 parent 5b931ab
commit 6e936c5
Show file tree

Hide file tree

Showing 5 changed files with 118 additions and 27 deletions.
diff --git 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<T::AccountId>,
+		new_owner: AccountIdOrCollectionNftTuple<T::AccountId>,
 	) -> Result<(T::AccountId, CollectionId, NftId), DispatchError> {
-		let owner = match pallet_uniques::Pallet::<T>::owner(collection_id, nft_id) {
-			Some(new_owner) => new_owner,
-			None => return Err(Error::<T>::NoAvailableNftId.into()),
-		};
-		let (root_owner, _root_nft) = Pallet::<T>::lookup_root_owner(collection_id, nft_id)?;
-
-		// Check ownership
-		ensure!(sender == root_owner, Error::<T>::NoPermission);
-
 		// Check NFT exists
 		ensure!(Pallet::<T>::nft_exists((collection_id, nft_id)), Error::<T>::NoAvailableNftId);
 
-		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.clone()),
+		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 {
@@ -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
@@ -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<T>,
 			collection_id: CollectionId,
 			nft_id: NftId,
-			_new_owner: AccountIdOrCollectionNftTuple<T::AccountId>,
+			new_owner: AccountIdOrCollectionNftTuple<T::AccountId>,
 		) -> DispatchResult {
 			let sender = ensure_signed(origin)?;
 
+			let (root_owner, _root_nft) = Pallet::<T>::lookup_root_owner(collection_id, nft_id)?;
+			// Check ownership
+			ensure!(sender == root_owner, Error::<T>::NoPermission);
+
+			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::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
@@ -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::<Test>::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::<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),
@@ -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::<Test>::get((COLLECTION_ID_0,NFT_ID_0, base_id)).is_some());
-		assert!(EquippableSlots::<Test>::get((COLLECTION_ID_0,NFT_ID_0, resource_id, base_id, slot_id)).is_some());
-
+		assert!(EquippableBases::<Test>::get((COLLECTION_ID_0, NFT_ID_0, base_id)).is_some());
+		assert!(EquippableSlots::<Test>::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::<Test>::get((COLLECTION_ID_0,NFT_ID_0, base_id)).is_none());
-		assert!(EquippableSlots::<Test>::get((COLLECTION_ID_0,NFT_ID_0, resource_id, base_id, slot_id)).is_none());
-
+		assert!(EquippableBases::<Test>::get((COLLECTION_ID_0, NFT_ID_0, base_id)).is_none());
+		assert!(EquippableSlots::<Test>::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
@@ -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
@@ -113,7 +113,7 @@ pub trait Nft<AccountId, BoundedString, BoundedResourceVec> {
 		sender: AccountId,
 		collection_id: CollectionId,
 		nft_id: NftId,
-		_new_owner: AccountIdOrCollectionNftTuple<AccountId>,
+		new_owner: AccountIdOrCollectionNftTuple<AccountId>,
 	) -> Result<(AccountId, CollectionId, NftId), DispatchError>;
 	fn nft_reject(
 		sender: AccountId,