From 620be0facb51783f09d3a1515502fb7024503bf1 Mon Sep 17 00:00:00 2001 From: David Levy Date: Thu, 24 Aug 2023 09:47:24 +0900 Subject: [PATCH] GMS-991: Add `safeBurn` and `safeBurnBatch`. (#69) --- contracts/errors/Errors.sol | 6 ++ .../token/erc721/abstract/ERC721Hybrid.sol | 28 +++++- .../erc721/abstract/ImmutableERC721Base.sol | 26 +++++ .../token/erc721/preset/ImmutableERC721.sol | 6 +- .../erc721/preset/ImmutableERC721MintByID.sol | 7 +- ...leERC721HybridPermissionedMintable.test.ts | 86 +++++++++++++++-- ...mmutableERC721PermissionedMintable.test.ts | 95 ++++++++++++++++--- 7 files changed, 229 insertions(+), 25 deletions(-) diff --git a/contracts/errors/Errors.sol b/contracts/errors/Errors.sol index a85907f4..0710a571 100644 --- a/contracts/errors/Errors.sol +++ b/contracts/errors/Errors.sol @@ -15,6 +15,12 @@ interface IImmutableERC721Errors { /// @dev Caller is not approved or owner error IImmutableERC721NotOwnerOrOperator(uint256 tokenId); + + /// @dev Current token owner is not what was expected + error IImmutableERC721MismatchedTokenOwner( + uint256 tokenId, + address currentOwner + ); } interface RoyaltyEnforcementErrors { diff --git a/contracts/token/erc721/abstract/ERC721Hybrid.sol b/contracts/token/erc721/abstract/ERC721Hybrid.sol index 0c476da2..65462fb7 100644 --- a/contracts/token/erc721/abstract/ERC721Hybrid.sol +++ b/contracts/token/erc721/abstract/ERC721Hybrid.sol @@ -181,6 +181,32 @@ abstract contract ERC721Hybrid is ERC721PsiBurnable, ERC721, IImmutableERC721Err } } + /// @dev Burn a token, checking the owner of the token against the parameter first. + function safeBurn(address owner, uint256 tokenId) public virtual { + address currentOwner = ownerOf(tokenId); + if (currentOwner != owner) { + revert IImmutableERC721MismatchedTokenOwner(tokenId, currentOwner); + } + + burn(tokenId); + } + + /// @dev A singular safe burn request. + struct IDBurn { + address owner; + uint256[] tokenIds; + } + + /// @dev Burn a batch of tokens, checking the owner of each token first. + function _safeBurnBatch(IDBurn[] memory burns) internal { + for (uint i = 0; i < burns.length; i++) { + IDBurn memory b = burns[i]; + for (uint j = 0; j < b.tokenIds.length; j++) { + safeBurn(b.owner, b.tokenIds[j]); + } + } + } + /// @dev overriding erc721 and erc721psi _safemint, super calls the `_safeMint` method of /// the erc721 implementation due to inheritance linearisation function _safeMint(address to, uint256 tokenId) internal virtual override(ERC721, ERC721Psi) { @@ -291,4 +317,4 @@ abstract contract ERC721Hybrid is ERC721PsiBurnable, ERC721, IImmutableERC721Err } return ERC721Psi.transferFrom(from, to, tokenId); } -} \ No newline at end of file +} diff --git a/contracts/token/erc721/abstract/ImmutableERC721Base.sol b/contracts/token/erc721/abstract/ImmutableERC721Base.sol index 669635d6..d36b42ec 100644 --- a/contracts/token/erc721/abstract/ImmutableERC721Base.sol +++ b/contracts/token/erc721/abstract/ImmutableERC721Base.sol @@ -48,6 +48,12 @@ abstract contract ImmutableERC721Base is uint256[] tokenIds; } + /// @dev A singular safe burn request. + struct IDBurn { + address owner; + uint256[] tokenIds; + } + /// @dev A singular batch transfer request struct TransferRequest { address from; @@ -216,6 +222,26 @@ abstract contract ImmutableERC721Base is _totalSupply--; } + /// @dev Burn a token, checking the owner of the token against the parameter first. + function safeBurn(address owner, uint256 tokenId) public virtual { + address currentOwner = ownerOf(tokenId); + if (currentOwner != owner) { + revert IImmutableERC721MismatchedTokenOwner(tokenId, currentOwner); + } + + burn(tokenId); + } + + /// @dev Burn a batch of tokens, checking the owner of each token first. + function _safeBurnBatch(IDBurn[] memory burns) public virtual { + for (uint i = 0; i < burns.length; i++) { + IDBurn memory b = burns[i]; + for (uint j = 0; j < b.tokenIds.length; j++) { + safeBurn(b.owner, b.tokenIds[j]); + } + } + } + /// ===== Internal functions ===== /// @dev mints specified token ids to specified address diff --git a/contracts/token/erc721/preset/ImmutableERC721.sol b/contracts/token/erc721/preset/ImmutableERC721.sol index 18447b19..0c51b253 100644 --- a/contracts/token/erc721/preset/ImmutableERC721.sol +++ b/contracts/token/erc721/preset/ImmutableERC721.sol @@ -53,6 +53,10 @@ contract ImmutableERC721 is ImmutableERC721HybridBase { _safeMintBatchByIDToMultiple(mints); } + function safeBurnBatch(IDBurn[] memory burns) external { + _safeBurnBatch(burns); + } + function safeTransferFromBatch(TransferRequest calldata tr) external { if (tr.tokenIds.length != tr.tos.length) { revert IImmutableERC721MismatchedTransferLengths(); @@ -63,4 +67,4 @@ contract ImmutableERC721 is ImmutableERC721HybridBase { } } -} \ No newline at end of file +} diff --git a/contracts/token/erc721/preset/ImmutableERC721MintByID.sol b/contracts/token/erc721/preset/ImmutableERC721MintByID.sol index a86a7aaa..3fffec45 100644 --- a/contracts/token/erc721/preset/ImmutableERC721MintByID.sol +++ b/contracts/token/erc721/preset/ImmutableERC721MintByID.sol @@ -89,6 +89,11 @@ contract ImmutableERC721MintByID is ImmutableERC721Base { } } + /// @dev Burn a batch of tokens, checking the owner of each token first. + function safeBurnBatch(IDBurn[] memory burns) external { + _safeBurnBatch(burns); + } + /// @dev Allows owner or operator to transfer a batch of tokens function safeTransferFromBatch(TransferRequest calldata tr) external { if (tr.tokenIds.length != tr.tos.length) { @@ -99,4 +104,4 @@ contract ImmutableERC721MintByID is ImmutableERC721Base { safeTransferFrom(tr.from, tr.tos[i], tr.tokenIds[i]); } } -} \ No newline at end of file +} diff --git a/test/token/erc721/ImmutableERC721HybridPermissionedMintable.test.ts b/test/token/erc721/ImmutableERC721HybridPermissionedMintable.test.ts index 3043accd..9f56b467 100644 --- a/test/token/erc721/ImmutableERC721HybridPermissionedMintable.test.ts +++ b/test/token/erc721/ImmutableERC721HybridPermissionedMintable.test.ts @@ -89,19 +89,19 @@ describe("ImmutableERC721", function () { }); it("Should allow minting of batch tokens", async function () { const mintRequests = [ - { to: user.address, tokenIds: [9, 10, 11] }, + { to: user.address, tokenIds: [9, 10, 11, 20] }, { to: owner.address, tokenIds: [12, 13, 14] }, ]; await erc721.connect(minter).safeMintBatch(mintRequests); - expect(await erc721.balanceOf(user.address)).to.equal(8); + expect(await erc721.balanceOf(user.address)).to.equal(9); expect(await erc721.balanceOf(owner.address)).to.equal(6); - expect(await erc721.totalSupply()).to.equal(14); - expect(await erc721.ownerOf(3)).to.equal(user.address); - expect(await erc721.ownerOf(4)).to.equal(user.address); - expect(await erc721.ownerOf(5)).to.equal(user.address); - expect(await erc721.ownerOf(6)).to.equal(owner.address); - expect(await erc721.ownerOf(7)).to.equal(owner.address); - expect(await erc721.ownerOf(8)).to.equal(owner.address); + expect(await erc721.totalSupply()).to.equal(15); + expect(await erc721.ownerOf(9)).to.equal(user.address); + expect(await erc721.ownerOf(10)).to.equal(user.address); + expect(await erc721.ownerOf(11)).to.equal(user.address); + expect(await erc721.ownerOf(12)).to.equal(owner.address); + expect(await erc721.ownerOf(13)).to.equal(owner.address); + expect(await erc721.ownerOf(14)).to.equal(owner.address); }); it("Should allow batch minting of tokens by quantity", async function () { @@ -163,6 +163,72 @@ describe("ImmutableERC721", function () { ); }); + it("Should not allow owner or approved to burn a token when specifying the incorrect owner", async function() { + await expect( + erc721.connect(user).safeBurn(owner.address, 5) + ).to.be.revertedWith('IImmutableERC721MismatchedTokenOwner').withArgs(5, user.address); + }); + + it("Should allow owner or approved to safely burn a token when specifying the correct owner", async function() { + const originalBalance = await erc721.balanceOf(user.address); + const originalSupply = await erc721.totalSupply(); + await erc721.connect(user).safeBurn(user.address, 5); + expect(await erc721.balanceOf(user.address)).to.equal( + originalBalance.sub(1) + ); + expect(await erc721.totalSupply()).to.equal( + originalSupply.sub(1) + ); + }); + + it("Should not allow owner or approved to burn a batch of tokens when specifying the incorrect owners", async function() { + const burns = [ + { + owner: user.address, + tokenIds: [12, 13, 14], + }, + { + owner: owner.address, + tokenIds: [9, 10, 11], + }, + ]; + await expect( + erc721.connect(user).safeBurnBatch(burns) + ).to.be.revertedWith('IImmutableERC721MismatchedTokenOwner').withArgs(12, owner.address); + }); + + it("Should allow owner or approved to safely burn a batch of tokens when specifying the correct owners", async function() { + const originalUserBalance = await erc721.balanceOf(user.address); + const originalOwnerBalance = await erc721.balanceOf(owner.address); + const originalSupply = await erc721.totalSupply(); + + // Set approval for owner to burn these tokens from user. + await erc721.connect(user).approve(owner.address, 9); + await erc721.connect(user).approve(owner.address, 10); + await erc721.connect(user).approve(owner.address, 11); + + const burns = [ + { + owner: owner.address, + tokenIds: [12, 13, 14], + }, + { + owner: user.address, + tokenIds: [9, 10, 11], + }, + ]; + await erc721.connect(owner).safeBurnBatch(burns); + expect(await erc721.balanceOf(user.address)).to.equal( + originalUserBalance.sub(3) + ); + expect(await erc721.balanceOf(owner.address)).to.equal( + originalOwnerBalance.sub(3) + ); + expect(await erc721.totalSupply()).to.equal( + originalSupply.sub(6) + ); + }); + it("Should prevent not approved to burn a batch of tokens", async function () { const first = await erc721.bulkMintThreshold(); await expect( @@ -197,7 +263,7 @@ describe("ImmutableERC721", function () { }); it("Should revert with a burnt tokenId", async function () { - const tokenId = 10; + const tokenId = 20; await erc721.connect(user).burn(tokenId); await expect(erc721.tokenURI(tokenId)).to.be.revertedWith( "ERC721: invalid token ID" diff --git a/test/token/erc721/ImmutableERC721PermissionedMintable.test.ts b/test/token/erc721/ImmutableERC721PermissionedMintable.test.ts index 61271380..c659de1b 100644 --- a/test/token/erc721/ImmutableERC721PermissionedMintable.test.ts +++ b/test/token/erc721/ImmutableERC721PermissionedMintable.test.ts @@ -99,26 +99,30 @@ describe("Immutable ERC721 Permissioned Mintable Test Cases", function () { it("Should allow safe minting of batch tokens", async function () { const mintRequests = [ - { to: user.address, tokenIds: [2, 3, 4] }, - { to: owner.address, tokenIds: [6, 7, 8] }, + { to: user.address, tokenIds: [2, 3, 4, 5, 6] }, + { to: owner.address, tokenIds: [7, 8, 9, 10, 11] }, ]; await erc721.connect(minter).safeMintBatch(mintRequests); - expect(await erc721.balanceOf(user.address)).to.equal(4); - expect(await erc721.balanceOf(owner.address)).to.equal(3); - expect(await erc721.totalSupply()).to.equal(7); + expect(await erc721.balanceOf(user.address)).to.equal(6); + expect(await erc721.balanceOf(owner.address)).to.equal(5); + expect(await erc721.totalSupply()).to.equal(11); expect(await erc721.ownerOf(2)).to.equal(user.address); expect(await erc721.ownerOf(3)).to.equal(user.address); expect(await erc721.ownerOf(4)).to.equal(user.address); - expect(await erc721.ownerOf(6)).to.equal(owner.address); + expect(await erc721.ownerOf(5)).to.equal(user.address); + expect(await erc721.ownerOf(6)).to.equal(user.address); expect(await erc721.ownerOf(7)).to.equal(owner.address); expect(await erc721.ownerOf(8)).to.equal(owner.address); + expect(await erc721.ownerOf(9)).to.equal(owner.address); + expect(await erc721.ownerOf(10)).to.equal(owner.address); + expect(await erc721.ownerOf(11)).to.equal(owner.address); }); it("Should allow owner or approved to burn a batch of tokens", async function () { - expect(await erc721.balanceOf(user.address)).to.equal(4); + expect(await erc721.balanceOf(user.address)).to.equal(6); await erc721.connect(user).burnBatch([1, 2]); - expect(await erc721.balanceOf(user.address)).to.equal(2); - expect(await erc721.totalSupply()).to.equal(5); + expect(await erc721.balanceOf(user.address)).to.equal(4); + expect(await erc721.totalSupply()).to.equal(9); }); it("Should prevent not approved to burn a batch of tokens", async function () { @@ -133,17 +137,84 @@ describe("Immutable ERC721 Permissioned Mintable Test Cases", function () { .to.be.revertedWith("IImmutableERC721TokenAlreadyBurned") .withArgs(1); }); + + it("Should not allow owner or approved to safely burn a token when specifying the incorrect owner", async function() { + await expect( + erc721.connect(user).safeBurn(owner.address, 3) + ).to.be.revertedWith('IImmutableERC721MismatchedTokenOwner').withArgs(3, user.address); + }); + + it("Should allow owner or approved to safely burn a token when specifying the correct owner", async function() { + const originalBalance = await erc721.balanceOf(user.address); + const originalSupply = await erc721.totalSupply(); + await erc721.connect(user).safeBurn(user.address, 3); + expect(await erc721.balanceOf(user.address)).to.equal( + originalBalance.sub(1) + ); + expect(await erc721.totalSupply()).to.equal( + originalSupply.sub(1) + ); + }); + + it("Should not allow owner or approved to burn a batch of tokens when specifying the incorrect owners", async function() { + const burns = [ + { + owner: user.address, + tokenIds: [7, 8, 9], + }, + { + owner: owner.address, + tokenIds: [4, 5, 6], + }, + ]; + + await expect( + erc721.connect(user).safeBurnBatch(burns) + ).to.be.revertedWith('IImmutableERC721MismatchedTokenOwner').withArgs(7, owner.address); + }); + + it("Should allow owner or approved to safely burn a batch of tokens when specifying the correct owners", async function() { + const originalUserBalance = await erc721.balanceOf(user.address); + const originalOwnerBalance = await erc721.balanceOf(owner.address); + const originalSupply = await erc721.totalSupply(); + + // Set approval for owner to burn these tokens from user. + await erc721.connect(user).approve(owner.address, 4); + await erc721.connect(user).approve(owner.address, 5); + await erc721.connect(user).approve(owner.address, 6); + + const burns = [ + { + owner: owner.address, + tokenIds: [7, 8, 9], + }, + { + owner: user.address, + tokenIds: [4, 5, 6], + }, + ]; + await erc721.connect(owner).safeBurnBatch(burns); + expect(await erc721.balanceOf(user.address)).to.equal( + originalUserBalance.sub(3) + ); + expect(await erc721.balanceOf(owner.address)).to.equal( + originalOwnerBalance.sub(3) + ); + expect(await erc721.totalSupply()).to.equal( + originalSupply.sub(6) + ); + }); }); describe("Base URI and Token URI", function () { it("Should return a non-empty tokenURI when the base URI is set", async function () { - const tokenId = 10; + const tokenId = 100; await erc721.connect(minter).mint(user.address, tokenId); expect(await erc721.tokenURI(tokenId)).to.equal(`${baseURI}${tokenId}`); }); it("Should revert with a burnt tokenId", async function () { - const tokenId = 10; + const tokenId = 100; await erc721.connect(user).burn(tokenId); await expect(erc721.tokenURI(tokenId)).to.be.revertedWith( "ERC721: invalid token ID" @@ -172,7 +243,7 @@ describe("Immutable ERC721 Permissioned Mintable Test Cases", function () { it("Should return an empty token URI when the base URI is not set", async function () { await erc721.setBaseURI(""); - const tokenId = 12; + const tokenId = 101; await erc721.connect(minter).mint(user.address, tokenId); expect(await erc721.tokenURI(tokenId)).to.equal(""); });