From 7d5d8a1bef2c4ed024c4459e1dbf66d663682742 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 1 Sep 2023 11:32:58 +0200 Subject: [PATCH 1/3] Do not cleanup token specific data on burn --- .changeset/fair-humans-peel.md | 5 ++++ .changeset/lemon-walls-flash.md | 5 ++++ .../token/ERC721/extensions/ERC721Royalty.sol | 13 --------- .../ERC721/extensions/ERC721URIStorage.sol | 15 ---------- .../ERC721/extensions/ERC721Royalty.test.js | 29 ++++++++++++------- .../extensions/ERC721URIStorage.test.js | 16 ++++++++-- 6 files changed, 41 insertions(+), 42 deletions(-) create mode 100644 .changeset/fair-humans-peel.md create mode 100644 .changeset/lemon-walls-flash.md diff --git a/.changeset/fair-humans-peel.md b/.changeset/fair-humans-peel.md new file mode 100644 index 00000000000..b3c61de7054 --- /dev/null +++ b/.changeset/fair-humans-peel.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC721Royalty`: Do not reset token-specific royalties when burning. diff --git a/.changeset/lemon-walls-flash.md b/.changeset/lemon-walls-flash.md new file mode 100644 index 00000000000..389bc033a09 --- /dev/null +++ b/.changeset/lemon-walls-flash.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ERC721URIStorage`: Do not reset the token-specific URI when burning. diff --git a/contracts/token/ERC721/extensions/ERC721Royalty.sol b/contracts/token/ERC721/extensions/ERC721Royalty.sol index 5b889b90d13..6f31368e251 100644 --- a/contracts/token/ERC721/extensions/ERC721Royalty.sol +++ b/contracts/token/ERC721/extensions/ERC721Royalty.sol @@ -24,17 +24,4 @@ abstract contract ERC721Royalty is ERC2981, ERC721 { function supportsInterface(bytes4 interfaceId) public view virtual override(ERC721, ERC2981) returns (bool) { return super.supportsInterface(interfaceId); } - - /** - * @dev See {ERC721-_update}. When burning, this override will additionally clear the royalty information for the token. - */ - function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { - address previousOwner = super._update(to, tokenId, auth); - - if (to == address(0)) { - _resetTokenRoyalty(tokenId); - } - - return previousOwner; - } } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index cd4845b7864..aeb32812ab9 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -62,19 +62,4 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 { emit MetadataUpdate(tokenId); } - - /** - * @dev See {ERC721-_update}. When burning, this override will additionally check if a - * token-specific URI was set for the token, and if so, it deletes the token URI from - * the storage mapping. - */ - function _update(address to, uint256 tokenId, address auth) internal virtual override returns (address) { - address previousOwner = super._update(to, tokenId, auth); - - if (to == address(0) && bytes(_tokenURIs[tokenId]).length != 0) { - delete _tokenURIs[tokenId]; - } - - return previousOwner; - } } diff --git a/test/token/ERC721/extensions/ERC721Royalty.test.js b/test/token/ERC721/extensions/ERC721Royalty.test.js index 1c0536bf5ff..ed202f5ce68 100644 --- a/test/token/ERC721/extensions/ERC721Royalty.test.js +++ b/test/token/ERC721/extensions/ERC721Royalty.test.js @@ -1,15 +1,15 @@ -const { BN, constants } = require('@openzeppelin/test-helpers'); +require('@openzeppelin/test-helpers'); const { shouldBehaveLikeERC2981 } = require('../../common/ERC2981.behavior'); const ERC721Royalty = artifacts.require('$ERC721Royalty'); contract('ERC721Royalty', function (accounts) { - const [account1, account2] = accounts; - const tokenId1 = new BN('1'); - const tokenId2 = new BN('2'); - const royalty = new BN('200'); - const salePrice = new BN('1000'); + const [account1, account2, recipient] = accounts; + const tokenId1 = web3.utils.toBN('1'); + const tokenId2 = web3.utils.toBN('2'); + const royalty = web3.utils.toBN('200'); + const salePrice = web3.utils.toBN('1000'); beforeEach(async function () { this.token = await ERC721Royalty.new('My Token', 'TKN'); @@ -25,15 +25,22 @@ contract('ERC721Royalty', function (accounts) { describe('token specific functions', function () { beforeEach(async function () { - await this.token.$_setTokenRoyalty(tokenId1, account1, royalty); + await this.token.$_setTokenRoyalty(tokenId1, recipient, royalty); }); - it('removes royalty information after burn', async function () { + it('royalty information are kept during burn and re-mint', async function () { await this.token.$_burn(tokenId1); - const tokenInfo = await this.token.royaltyInfo(tokenId1, salePrice); - expect(tokenInfo[0]).to.be.equal(constants.ZERO_ADDRESS); - expect(tokenInfo[1]).to.be.bignumber.equal(new BN('0')); + const tokenInfoA = await this.token.royaltyInfo(tokenId1, salePrice); + expect(tokenInfoA[0]).to.be.equal(recipient); + expect(tokenInfoA[1]).to.be.bignumber.equal(salePrice.mul(royalty).divn(1e4)); + + await this.token.$_mint(account2, tokenId1); + + const tokenInfoB = await this.token.royaltyInfo(tokenId1, salePrice); + expect(tokenInfoB[0]).to.be.equal(recipient); + expect(tokenInfoB[1]).to.be.bignumber.equal(salePrice.mul(royalty).divn(1e4)); + }); }); diff --git a/test/token/ERC721/extensions/ERC721URIStorage.test.js b/test/token/ERC721/extensions/ERC721URIStorage.test.js index 129515514c8..9647fc0b9c8 100644 --- a/test/token/ERC721/extensions/ERC721URIStorage.test.js +++ b/test/token/ERC721/extensions/ERC721URIStorage.test.js @@ -7,7 +7,7 @@ const { expectRevertCustomError } = require('../../../helpers/customError'); const ERC721URIStorageMock = artifacts.require('$ERC721URIStorageMock'); contract('ERC721URIStorage', function (accounts) { - const [owner] = accounts; + const [owner, other] = accounts; const name = 'Non Fungible Token'; const symbol = 'NFT'; @@ -84,7 +84,7 @@ contract('ERC721URIStorage', function (accounts) { }); it('tokens without URI can be burnt ', async function () { - await this.token.$_burn(firstTokenId, { from: owner }); + await this.token.$_burn(firstTokenId); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); }); @@ -92,9 +92,19 @@ contract('ERC721URIStorage', function (accounts) { it('tokens with URI can be burnt ', async function () { await this.token.$_setTokenURI(firstTokenId, sampleUri); - await this.token.$_burn(firstTokenId, { from: owner }); + await this.token.$_burn(firstTokenId); await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); }); + + it('tokens URI is kept if token is burnt and reminted ', async function () { + await this.token.$_setTokenURI(firstTokenId, sampleUri); + + await this.token.$_burn(firstTokenId); + await expectRevertCustomError(this.token.tokenURI(firstTokenId), 'ERC721NonexistentToken', [firstTokenId]); + + await this.token.$_mint(owner, firstTokenId); + expect(await this.token.tokenURI(firstTokenId)).to.be.equal(sampleUri); + }); }); }); From c66cae3a97639d1e504ee2378736d186425cb861 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sat, 2 Sep 2023 01:28:23 -0300 Subject: [PATCH 2/3] merge changesets --- .changeset/fair-humans-peel.md | 2 +- .changeset/lemon-walls-flash.md | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) delete mode 100644 .changeset/lemon-walls-flash.md diff --git a/.changeset/fair-humans-peel.md b/.changeset/fair-humans-peel.md index b3c61de7054..3c0dc3c0617 100644 --- a/.changeset/fair-humans-peel.md +++ b/.changeset/fair-humans-peel.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': major --- -`ERC721Royalty`: Do not reset token-specific royalties when burning. +`ERC721URIStorage`, `ERC721Royalty`: Stop resetting token-specific URI and royalties when burning. diff --git a/.changeset/lemon-walls-flash.md b/.changeset/lemon-walls-flash.md deleted file mode 100644 index 389bc033a09..00000000000 --- a/.changeset/lemon-walls-flash.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'openzeppelin-solidity': major ---- - -`ERC721URIStorage`: Do not reset the token-specific URI when burning. From 1896c9abf5b4f47867dfb8d9155372c3828d3449 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Sat, 2 Sep 2023 01:30:53 -0300 Subject: [PATCH 3/3] lint --- test/token/ERC721/extensions/ERC721Royalty.test.js | 3 +-- test/token/ERC721/extensions/ERC721URIStorage.test.js | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/test/token/ERC721/extensions/ERC721Royalty.test.js b/test/token/ERC721/extensions/ERC721Royalty.test.js index ed202f5ce68..78cba9858c6 100644 --- a/test/token/ERC721/extensions/ERC721Royalty.test.js +++ b/test/token/ERC721/extensions/ERC721Royalty.test.js @@ -5,7 +5,7 @@ const { shouldBehaveLikeERC2981 } = require('../../common/ERC2981.behavior'); const ERC721Royalty = artifacts.require('$ERC721Royalty'); contract('ERC721Royalty', function (accounts) { - const [account1, account2, recipient] = accounts; + const [account1, account2, recipient] = accounts; const tokenId1 = web3.utils.toBN('1'); const tokenId2 = web3.utils.toBN('2'); const royalty = web3.utils.toBN('200'); @@ -40,7 +40,6 @@ contract('ERC721Royalty', function (accounts) { const tokenInfoB = await this.token.royaltyInfo(tokenId1, salePrice); expect(tokenInfoB[0]).to.be.equal(recipient); expect(tokenInfoB[1]).to.be.bignumber.equal(salePrice.mul(royalty).divn(1e4)); - }); }); diff --git a/test/token/ERC721/extensions/ERC721URIStorage.test.js b/test/token/ERC721/extensions/ERC721URIStorage.test.js index 9647fc0b9c8..18b59f05d82 100644 --- a/test/token/ERC721/extensions/ERC721URIStorage.test.js +++ b/test/token/ERC721/extensions/ERC721URIStorage.test.js @@ -7,7 +7,7 @@ const { expectRevertCustomError } = require('../../../helpers/customError'); const ERC721URIStorageMock = artifacts.require('$ERC721URIStorageMock'); contract('ERC721URIStorage', function (accounts) { - const [owner, other] = accounts; + const [owner] = accounts; const name = 'Non Fungible Token'; const symbol = 'NFT';