Stop cleaning up token specific data on ERC-721 burn (#4561)

Co-authored-by: Francisco Giordano <fg@frang.io>

.changeset/fair-humans-peel.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`ERC721URIStorage`, `ERC721Royalty`: Stop resetting token-specific URI and royalties when burning.

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;
-    }
 }

contracts/token/ERC721/extensions/ERC721URIStorage.sol

@@ -58,19 +58,4 @@ abstract contract ERC721URIStorage is IERC4906, ERC721 {
         _tokenURIs[tokenId] = _tokenURI;
         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;
-    }
 }

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,21 @@ 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));
     });
   });
 

test/token/ERC721/extensions/ERC721URIStorage.test.js

@@ -88,7 +88,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]);
     });
@@ -96,9 +96,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);
+    });
   });
 });