From 956ec6eac12631005f04bd9ab0411fcb06ab9845 Mon Sep 17 00:00:00 2001 From: kozyrevdev <73348153+kozyrevdev@users.noreply.github.com> Date: Fri, 01 Jul 2022 12:37:00 +0000 Subject: [PATCH] Merge pull request #410 from UniqueNetwork/fix/admin-transfer Fix/admin transfer --- --- a/pallets/nonfungible/src/lib.rs +++ b/pallets/nonfungible/src/lib.rs @@ -379,11 +379,7 @@ ) -> DispatchResult { let token_data = >::get((collection.id, token)).ok_or(>::TokenNotFound)?; - ensure!( - &token_data.owner == sender - || (collection.limits.owner_can_transfer() && collection.is_owner_or_admin(sender)), - >::NoPermission - ); + ensure!(&token_data.owner == sender, >::NoPermission); if collection.permissions.access() == AccessMode::AllowList { collection.check_allowlist(sender)?; @@ -665,12 +661,7 @@ let token_data = >::get((collection.id, token)).ok_or(>::TokenNotFound)?; - // TODO: require sender to be token, owner, require admins to go through transfer_from - ensure!( - &token_data.owner == from - || (collection.limits.owner_can_transfer() && collection.is_owner_or_admin(from)), - >::NoPermission - ); + ensure!(&token_data.owner == from, >::NoPermission); if collection.permissions.access() == AccessMode::AllowList { collection.check_allowlist(from)?; @@ -976,8 +967,12 @@ // `from`, `to` checked in [`transfer`] collection.check_allowlist(spender)?; } + + if collection.limits.owner_can_transfer() && collection.is_owner_or_admin(spender) { + return Ok(()); + } + if let Some(source) = T::CrossTokenAddressMapping::address_to_token(from) { - // TODO: should collection owner be allowed to perform this transfer? ensure!( >::check_indirectly_owned( spender.clone(), --- a/tests/package.json +++ b/tests/package.json @@ -60,6 +60,7 @@ "testAddToContractAllowList": "mocha --timeout 9999999 -r ts-node/register ./**/addToContractAllowList.test.ts", "testTransfer": "mocha --timeout 9999999 -r ts-node/register ./**/transfer.test.ts", "testBurnItem": "mocha --timeout 9999999 -r ts-node/register ./**/burnItem.test.ts", + "testAdminTransferAndBurn": "mocha --timeout 9999999 -r ts-node/register ./**/adminTransferAndBurn.test.ts", "testSetMintPermission": "mocha --timeout 9999999 -r ts-node/register ./**/setMintPermission.test.ts", "testCreditFeesToTreasury": "mocha --timeout 9999999 -r ts-node/register ./**/creditFeesToTreasury.test.ts", "testContractSponsoring": "mocha --timeout 9999999 -r ts-node/register ./**/contractSponsoring.test.ts", --- /dev/null +++ b/tests/src/adminTransferAndBurn.test.ts @@ -0,0 +1,72 @@ +// Copyright 2019-2022 Unique Network (Gibraltar) Ltd. +// This file is part of Unique Network. + +// Unique Network is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Unique Network is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Unique Network. If not, see . + +import {IKeyringPair} from '@polkadot/types/types'; +import chai from 'chai'; +import chaiAsPromised from 'chai-as-promised'; +import {default as usingApi} from './substrate/substrate-api'; +import { + createCollectionExpectSuccess, + createItemExpectSuccess, + transferExpectFailure, + transferFromExpectSuccess, + burnItemExpectFailure, + burnFromExpectSuccess, + setCollectionLimitsExpectSuccess, +} from './util/helpers'; + +chai.use(chaiAsPromised); +const expect = chai.expect; + +describe('Integration Test: ownerCanTransfer allows admins to use only transferFrom/burnFrom:', () => { + let alice: IKeyringPair; + let bob: IKeyringPair; + let charlie: IKeyringPair; + + before(async () => { + await usingApi(async (api, privateKeyWrapper) => { + alice = privateKeyWrapper('//Alice'); + bob = privateKeyWrapper('//Bob'); + charlie = privateKeyWrapper('//Charlie'); + }); + }); + + it('admin transfers other user\'s token', async () => { + await usingApi(async () => { + const collectionId = await createCollectionExpectSuccess(); + await setCollectionLimitsExpectSuccess(alice, collectionId, {ownerCanTransfer: true}); + + const tokenId = await createItemExpectSuccess(alice, collectionId, 'NFT', {Substrate: bob.address}); + + await transferExpectFailure(collectionId, tokenId, alice, charlie); + + await transferFromExpectSuccess(collectionId, tokenId, alice, bob, charlie, 1); + }); + }); + + it('admin burns other user\'s token', async () => { + await usingApi(async () => { + const collectionId = await createCollectionExpectSuccess(); + await setCollectionLimitsExpectSuccess(alice, collectionId, {ownerCanTransfer: true}); + + const tokenId = await createItemExpectSuccess(alice, collectionId, 'NFT', {Substrate: bob.address}); + + await burnItemExpectFailure(alice, collectionId, tokenId); + + await burnFromExpectSuccess(alice, bob, collectionId, tokenId); + }); + }); +}); --- a/tests/src/burnItem.test.ts +++ b/tests/src/burnItem.test.ts @@ -155,7 +155,7 @@ await addCollectionAdminExpectSuccess(alice, collectionId, bob.address); await usingApi(async (api) => { - const tx = api.tx.unique.burnItem(collectionId, tokenId, 1); + const tx = api.tx.unique.burnFrom(collectionId, {Substrate: alice.address}, tokenId, 1); const events = await submitTransactionAsync(bob, tx); const result = getGenericResult(events); --- a/tests/src/nesting/graphs.test.ts +++ b/tests/src/nesting/graphs.test.ts @@ -36,7 +36,7 @@ await usingApi(async (api, privateKeyWrapper) => { const alice = privateKeyWrapper('//Alice'); const collection = await buildComplexObjectGraph(api, alice); - await setCollectionLimitsExpectSuccess(alice, collection, {ownerCanTransfer: true}); + const tokenTwoParent = tokenIdToCross(collection, 1); // to self await expect( @@ -49,7 +49,7 @@ 'second transaction', ).to.be.rejectedWith(/structure\.OuroborosDetected/); await expect( - executeTransaction(api, alice, api.tx.unique.transfer(tokenIdToCross(collection, 8), collection, 2, 1)), + executeTransaction(api, alice, api.tx.unique.transferFrom(tokenTwoParent, tokenIdToCross(collection, 8), collection, 2, 1)), 'third transaction', ).to.be.rejectedWith(/structure\.OuroborosDetected/); }); --- a/tests/src/nesting/nest.test.ts +++ b/tests/src/nesting/nest.test.ts @@ -123,7 +123,7 @@ ], 'Children contents check at nesting #2'); // Move token B to a different user outside the nesting tree - await transferExpectSuccess(collectionA, tokenB, alice, bob); + await transferFromExpectSuccess(collectionA, tokenB, alice, targetAddress, bob); children = await getTokenChildren(api, collectionA, targetToken); expect(children.length).to.be.equal(1, 'Children length check at unnesting'); expect(children).to.be.have.deep.members([ @@ -470,7 +470,7 @@ await expect(executeTransaction( api, alice, - api.tx.unique.transfer(targetAddress, collection, newToken, 1), + api.tx.unique.transferFrom(targetAddress, {Substrate: bob.address}, collection, newToken, 1), ), 'while nesting another\'s token token').to.be.rejectedWith(/common\.AddressNotInAllowlist/); expect(await getTokenOwner(api, collection, newToken)).to.be.deep.equal({Substrate: bob.address}); --- a/tests/src/util/helpers.ts +++ b/tests/src/util/helpers.ts @@ -830,6 +830,25 @@ }); } +export async function burnItemExpectFailure(sender: IKeyringPair, collectionId: number, tokenId: number, value: number | bigint = 1) { + await usingApi(async (api) => { + const tx = api.tx.unique.burnItem(collectionId, tokenId, value); + + const events = await expect(submitTransactionExpectFailAsync(sender, tx)).to.be.rejected; + const result = getCreateCollectionResult(events); + // tslint:disable-next-line:no-unused-expression + expect(result.success).to.be.false; + }); +} + +export async function burnFromExpectSuccess(sender: IKeyringPair, from: IKeyringPair | CrossAccountId, collectionId: number, tokenId: number, value: number | bigint = 1) { + await usingApi(async (api) => { + const tx = api.tx.unique.burnFrom(collectionId, normalizeAccountId(from), tokenId, value); + const events = await submitTransactionAsync(sender, tx); + return getGenericResult(events).success; + }); +} + export async function approve( api: ApiPromise, -- gitstuff