From 713b8d223b2fda93fff54dd3791a2ee19a9e7b2b Mon Sep 17 00:00:00 2001 From: Yaroslav Bolyukin Date: Mon, 16 Jan 2023 22:37:31 +0000 Subject: [PATCH] Merge pull request #831 from UniqueNetwork/fix/rft-collection-admin-permissions --- --- a/pallets/common/src/lib.rs +++ b/pallets/common/src/lib.rs @@ -390,8 +390,10 @@ Ok(()) } - /// Return **true** if `user` was not allowed to have tokens, and he can ignore such restrictions. - pub fn ignores_allowance(&self, user: &T::CrossAccountId) -> bool { + /// Returns **true** if + /// * the `user`is a collection owner or admin + /// * the collection limits allow the owner/admins to transfer/burn any collection token + pub fn ignores_token_restrictions(&self, user: &T::CrossAccountId) -> bool { self.limits.owner_can_transfer() && self.is_owner_or_admin(user) } --- a/pallets/fungible/src/lib.rs +++ b/pallets/fungible/src/lib.rs @@ -677,8 +677,14 @@ // `from`, `to` checked in [`transfer`] collection.check_allowlist(spender)?; } + + if collection.ignores_token_restrictions(spender) { + return Ok(Self::compute_allowance_decrease( + collection, from, spender, amount, + )); + } + 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(), @@ -690,18 +696,25 @@ >::ApprovedValueTooLow, ); return Ok(None); - } - let allowance = >::get((collection.id, from, spender)).checked_sub(amount); - if allowance.is_none() { - ensure!( - collection.ignores_allowance(spender), - >::ApprovedValueTooLow - ); } + let allowance = Self::compute_allowance_decrease(collection, from, spender, amount); + ensure!(allowance.is_some(), >::ApprovedValueTooLow); + Ok(allowance) } + /// Returns `Some(amount)` if the `spender` have allowance to spend this amount. + /// Otherwise, it returns `None`. + fn compute_allowance_decrease( + collection: &FungibleHandle, + from: &T::CrossAccountId, + spender: &T::CrossAccountId, + amount: u128, + ) -> Option { + >::get((collection.id, from, spender)).checked_sub(amount) + } + /// Transfer fungible tokens from one account to another. /// Same as the [`transfer`][`Pallet::transfer`] but spender doesn't needs to be an owner of the token pieces. /// The owner should set allowance for the spender to transfer pieces. --- a/pallets/nonfungible/src/lib.rs +++ b/pallets/nonfungible/src/lib.rs @@ -1246,7 +1246,7 @@ collection.check_allowlist(spender)?; } - if collection.limits.owner_can_transfer() && collection.is_owner_or_admin(spender) { + if collection.ignores_token_restrictions(spender) { return Ok(()); } @@ -1269,11 +1269,8 @@ if >::get((collection.id, from, spender)) { return Ok(()); } - ensure!( - collection.ignores_allowance(spender), - >::ApprovedValueTooLow - ); - Ok(()) + + Err(>::ApprovedValueTooLow.into()) } /// Transfer NFT token from one account to another. --- a/pallets/refungible/src/lib.rs +++ b/pallets/refungible/src/lib.rs @@ -1177,6 +1177,13 @@ // `from`, `to` checked in [`transfer`] collection.check_allowlist(spender)?; } + + if collection.ignores_token_restrictions(spender) { + return Ok(Self::compute_allowance_decrease( + collection, token, from, &spender, amount, + )); + } + if let Some(source) = T::CrossTokenAddressMapping::address_to_token(from) { // TODO: should collection owner be allowed to perform this transfer? ensure!( @@ -1191,21 +1198,30 @@ ); return Ok(None); } - let allowance = - >::get((collection.id, token, from, &spender)).checked_sub(amount); + let allowance = Self::compute_allowance_decrease(collection, token, from, &spender, amount); + if allowance.is_some() { + return Ok(allowance); + } + // Allowance (if any) would be reduced if spender is also wallet operator if >::get((collection.id, from, spender)) { return Ok(allowance); } - if allowance.is_none() { - ensure!( - collection.ignores_allowance(spender), - >::ApprovedValueTooLow - ); - } - Ok(allowance) + Err(>::ApprovedValueTooLow.into()) + } + + /// Returns `Some(amount)` if the `spender` have allowance to spend this amount. + /// Otherwise, it returns `None`. + fn compute_allowance_decrease( + collection: &RefungibleHandle, + token: TokenId, + from: &T::CrossAccountId, + spender: &T::CrossAccountId, + amount: u128, + ) -> Option { + >::get((collection.id, token, from, spender)).checked_sub(amount) } /// Transfer RFT token pieces from one account to another. --- a/tests/src/nesting/unnest.test.ts +++ b/tests/src/nesting/unnest.test.ts @@ -16,14 +16,17 @@ import {IKeyringPair} from '@polkadot/types/types'; import {expect, itSub, Pallets, usingPlaygrounds} from '../util'; +import {UniqueFTCollection, UniqueNFToken, UniqueRFToken} from '../util/playgrounds/unique'; describe('Integration Test: Unnesting', () => { let alice: IKeyringPair; + let bob: IKeyringPair; + let charlie: IKeyringPair; before(async () => { await usingPlaygrounds(async (helper, privateKey) => { const donor = await privateKey({filename: __filename}); - [alice] = await helper.arrange.createAccounts([50n], donor); + [alice, bob, charlie] = await helper.arrange.createAccounts([200n, 50n, 50n], donor); }); }); @@ -83,6 +86,188 @@ expect(await token.getBalance(targetToken.nestingAccount())).to.be.equal(0n); expect(await targetToken.getChildren()).to.be.length(0); }); + + async function checkNestedAmountState({ + expectedBalance, + childrenShouldPresent, + nested, + targetNft, + }: { + expectedBalance: bigint, + childrenShouldPresent: boolean, + nested: UniqueFTCollection | UniqueRFToken, + targetNft: UniqueNFToken, + }) { + const balance = await nested.getBalance(targetNft.nestingAccount()); + expect(balance).to.be.equal(expectedBalance); + + const children = await targetNft.getChildren(); + + if (childrenShouldPresent) { + expect(children[0]).to.be.deep.equal({ + collectionId: nested.collectionId, + tokenId: (nested instanceof UniqueFTCollection) ? 0 : nested.tokenId, + }); + } else { + expect(children.length).to.be.equal(0); + } + } + + function ownerOrAdminUnnestCases(modes: ('ft' | 'nft' | 'rft')[]): { + mode: 'ft' | 'nft' | 'rft', + sender: string, + op: 'transfer' | 'burn', + requiredPallets: Pallets[], + }[] { + const senders = ['owner', 'admin']; + const ops = ['transfer', 'burn']; + + const cases = []; + for (const mode of modes) { + const requiredPallets = (mode === 'rft') + ? [Pallets.ReFungible] + : []; + + for (const sender of senders) { + for (const op of ops) { + cases.push({ + mode: mode as 'ft' | 'nft' | 'rft', + sender, + op: op as 'transfer' | 'burn', + requiredPallets, + }); + } + } + } + + return cases; + } + + ownerOrAdminUnnestCases(['ft', 'rft']).map(testCase => + itSub.ifWithPallets(`[${testCase.mode}]: allows a collection ${testCase.sender} to ${testCase.op} nested token`, testCase.requiredPallets, async({helper}) => { + const owner = alice; + const admin = bob; + + const unnester = (testCase.sender === 'owner') + ? owner + : admin; + + const collectionNFT = await helper.nft.mintCollection(owner); + await collectionNFT.setPermissions(owner, {nesting: {tokenOwner: true}}); + + const collectionNested = await helper[testCase.mode as 'ft' | 'rft'].mintCollection(owner, { + limits: { + ownerCanTransfer: true, + }, + }); + await collectionNested.addAdmin(owner, {Substrate: admin.address}); + + const targetNft = await collectionNFT.mintToken(owner, {Substrate: charlie.address}); + + let nested: UniqueFTCollection | UniqueRFToken; + const totalAmount = 5n; + const firstUnnestAmount = 2n; + const restUnnestAmount = totalAmount - firstUnnestAmount; + + if (collectionNested instanceof UniqueFTCollection) { + await collectionNested.mint(owner, totalAmount, {Substrate: charlie.address}); + nested = collectionNested; + } else { + nested = await collectionNested.mintToken(owner, totalAmount, {Substrate: charlie.address}); + } + + // transfer/burn `amount` of nested assets by `unnester`. + const doOperationAndCheck = async ({ + amount, + shouldBeNestedAfterOp, + }: { + amount: bigint, + shouldBeNestedAfterOp: boolean, + }) => { + const nestedBalanceBeforeOp = await nested.getBalance(targetNft.nestingAccount()); + + if (testCase.op === 'transfer') { + const bobBalanceBeforeOp = await nested.getBalance({Substrate: bob.address}); + + await nested.transferFrom(unnester, targetNft.nestingAccount(), {Substrate: bob.address}, amount); + expect(await nested.getBalance({Substrate: bob.address})).to.be.equal(bobBalanceBeforeOp + amount); + } else { + if (nested instanceof UniqueFTCollection) { + await nested.burnTokensFrom(unnester, targetNft.nestingAccount(), amount); + } else { + await nested.burnFrom(unnester, targetNft.nestingAccount(), amount); + } + } + + await checkNestedAmountState({ + expectedBalance: nestedBalanceBeforeOp - amount, + childrenShouldPresent: shouldBeNestedAfterOp, + nested, + targetNft, + }); + }; + + // Initial setup: nest (fungibles/rft parts). + // Check NFT's balance of nested assets and NFT's children. + await nested.transfer(charlie, targetNft.nestingAccount(), totalAmount); + await checkNestedAmountState({ + expectedBalance: totalAmount, + childrenShouldPresent: true, + nested, + targetNft, + }); + + // Transfer/burn only a part of nested assets. + // Check that NFT's balance of the nested assets correctly decreased and NFT's children are not changed. + await doOperationAndCheck({ + amount: firstUnnestAmount, + shouldBeNestedAfterOp: true, + }); + + // Transfer/burn all remaining nested assets. + // Check that NFT's balance of the nested assets is 0 and NFT has no more children. + await doOperationAndCheck({ + amount: restUnnestAmount, + shouldBeNestedAfterOp: false, + }); + })); + + ownerOrAdminUnnestCases(['nft']).map(testCase => + itSub(`[nft]: allows a collection ${testCase.sender} to ${testCase.op} nested token`, async ({helper}) => { + const owner = alice; + const admin = bob; + + const unnester = (testCase.sender === 'owner') + ? owner + : admin; + + const collectionNFT = await helper.nft.mintCollection(owner); + await collectionNFT.setPermissions(owner, {nesting: {tokenOwner: true}}); + + const collectionNested = await helper.nft.mintCollection(owner, { + limits: { + ownerCanTransfer: true, + }, + }); + await collectionNested.addAdmin(owner, {Substrate: admin.address}); + + const targetNft = await collectionNFT.mintToken(owner, {Substrate: charlie.address}); + const nested = await collectionNested.mintToken(owner, {Substrate: charlie.address}); + + await nested.transfer(charlie, targetNft.nestingAccount()); + expect(await targetNft.getChildren()).to.be.deep.equal([{ + collectionId: nested.collectionId, + tokenId: nested.tokenId, + }]); + + if (testCase.op === 'transfer') { + await nested.transferFrom(unnester, targetNft.nestingAccount(), {Substrate: bob.address}); + } else { + await nested.burnFrom(unnester, targetNft.nestingAccount()); + } + + expect((await targetNft.getChildren()).length).to.be.equal(0); + })); }); describe('Negative Test: Unnesting', () => { -- gitstuff