From 8cc2705ace1f0f7916c49c8713713fcbc18129fb Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 26 May 2022 22:15:12 -0300 Subject: [PATCH 1/5] Simplify ERC721 revert reasons --- contracts/token/ERC721/ERC721.sol | 8 ++++---- .../ERC721/extensions/ERC721URIStorage.sol | 2 +- test/token/ERC721/ERC721.behavior.js | 20 +++++++++---------- .../ERC721/extensions/ERC721Burnable.test.js | 6 +++--- .../extensions/ERC721URIStorage.test.js | 6 +++--- 5 files changed, 21 insertions(+), 21 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index d13026d7695..d29aa603d5c 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -69,7 +69,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { */ function ownerOf(uint256 tokenId) public view virtual override returns (address) { address owner = _owners[tokenId]; - require(owner != address(0), "ERC721: owner query for nonexistent token"); + require(owner != address(0), "ERC721: token ID not minted"); return owner; } @@ -91,7 +91,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * @dev See {IERC721Metadata-tokenURI}. */ function tokenURI(uint256 tokenId) public view virtual override returns (string memory) { - require(_exists(tokenId), "ERC721Metadata: URI query for nonexistent token"); + require(_exists(tokenId), "ERC721: token ID not minted"); string memory baseURI = _baseURI(); return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : ""; @@ -125,7 +125,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * @dev See {IERC721-getApproved}. */ function getApproved(uint256 tokenId) public view virtual override returns (address) { - require(_exists(tokenId), "ERC721: approved query for nonexistent token"); + require(_exists(tokenId), "ERC721: token ID not minted"); return _tokenApprovals[tokenId]; } @@ -230,7 +230,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * - `tokenId` must exist. */ function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) { - require(_exists(tokenId), "ERC721: operator query for nonexistent token"); + require(_exists(tokenId), "ERC721: token ID not minted"); address owner = ERC721.ownerOf(tokenId); return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender); } diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index f55fdb54c46..091e6788b1c 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -18,7 +18,7 @@ abstract contract ERC721URIStorage is ERC721 { * @dev See {IERC721Metadata-tokenURI}. */ function tokenURI(uint256 tokenId) public view virtual override returns (string memory) { - require(_exists(tokenId), "ERC721URIStorage: URI query for nonexistent token"); + require(_exists(tokenId), "ERC721: token ID not minted"); string memory _tokenURI = _tokenURIs[tokenId]; string memory base = _baseURI(); diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index cab4330c41a..c688035b7b1 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -66,7 +66,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('reverts', async function () { await expectRevert( - this.token.ownerOf(tokenId), 'ERC721: owner query for nonexistent token', + this.token.ownerOf(tokenId), 'ERC721: token ID not minted', ); }); }); @@ -201,7 +201,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('reverts', async function () { await expectRevert( transferFunction.call(this, owner, other, nonExistentTokenId, { from: owner }), - 'ERC721: operator query for nonexistent token', + 'ERC721: token ID not minted', ); }); }); @@ -276,7 +276,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another nonExistentTokenId, { from: owner }, ), - 'ERC721: operator query for nonexistent token', + 'ERC721: token ID not minted', ); }); }); @@ -534,7 +534,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another context('when the given token ID does not exist', function () { it('reverts', async function () { await expectRevert(this.token.approve(approved, nonExistentTokenId, { from: operator }), - 'ERC721: owner query for nonexistent token'); + 'ERC721: token ID not minted'); }); }); }); @@ -623,7 +623,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('reverts', async function () { await expectRevert( this.token.getApproved(nonExistentTokenId), - 'ERC721: approved query for nonexistent token', + 'ERC721: token ID not minted', ); }); }); @@ -678,7 +678,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another describe('_burn', function () { it('reverts when burning a non-existent token id', async function () { await expectRevert( - this.token.burn(nonExistentTokenId), 'ERC721: owner query for nonexistent token', + this.token.burn(nonExistentTokenId), 'ERC721: token ID not minted', ); }); @@ -704,13 +704,13 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('deletes the token', async function () { expect(await this.token.balanceOf(owner)).to.be.bignumber.equal('1'); await expectRevert( - this.token.ownerOf(firstTokenId), 'ERC721: owner query for nonexistent token', + this.token.ownerOf(firstTokenId), 'ERC721: token ID not minted', ); }); it('reverts when burning a token id that has been deleted', async function () { await expectRevert( - this.token.burn(firstTokenId), 'ERC721: owner query for nonexistent token', + this.token.burn(firstTokenId), 'ERC721: token ID not minted', ); }); }); @@ -846,7 +846,7 @@ function shouldBehaveLikeERC721Enumerable (errorPrefix, owner, newOwner, approve describe('_burn', function () { it('reverts when burning a non-existent token id', async function () { await expectRevert( - this.token.burn(firstTokenId), 'ERC721: owner query for nonexistent token', + this.token.burn(firstTokenId), 'ERC721: token ID not minted', ); }); @@ -906,7 +906,7 @@ function shouldBehaveLikeERC721Metadata (errorPrefix, name, symbol, owner) { it('reverts when queried for non existent token id', async function () { await expectRevert( - this.token.tokenURI(nonExistentTokenId), 'ERC721Metadata: URI query for nonexistent token', + this.token.tokenURI(nonExistentTokenId), 'ERC721: token ID not minted', ); }); diff --git a/test/token/ERC721/extensions/ERC721Burnable.test.js b/test/token/ERC721/extensions/ERC721Burnable.test.js index bbf56e6a787..550fd05f9ae 100644 --- a/test/token/ERC721/extensions/ERC721Burnable.test.js +++ b/test/token/ERC721/extensions/ERC721Burnable.test.js @@ -37,7 +37,7 @@ contract('ERC721Burnable', function (accounts) { it('burns the given token ID and adjusts the balance of the owner', async function () { await expectRevert( this.token.ownerOf(tokenId), - 'ERC721: owner query for nonexistent token', + 'ERC721: token ID not minted', ); expect(await this.token.balanceOf(owner)).to.be.bignumber.equal('1'); }); @@ -60,7 +60,7 @@ contract('ERC721Burnable', function (accounts) { context('getApproved', function () { it('reverts', async function () { await expectRevert( - this.token.getApproved(tokenId), 'ERC721: approved query for nonexistent token', + this.token.getApproved(tokenId), 'ERC721: token ID not minted', ); }); }); @@ -69,7 +69,7 @@ contract('ERC721Burnable', function (accounts) { describe('when the given token ID was not tracked by this contract', function () { it('reverts', async function () { await expectRevert( - this.token.burn(unknownTokenId, { from: owner }), 'ERC721: operator query for nonexistent token', + this.token.burn(unknownTokenId, { from: owner }), 'ERC721: token ID not minted', ); }); }); diff --git a/test/token/ERC721/extensions/ERC721URIStorage.test.js b/test/token/ERC721/extensions/ERC721URIStorage.test.js index c476ad0912a..e823c56bd9b 100644 --- a/test/token/ERC721/extensions/ERC721URIStorage.test.js +++ b/test/token/ERC721/extensions/ERC721URIStorage.test.js @@ -31,7 +31,7 @@ contract('ERC721URIStorage', function (accounts) { it('reverts when queried for non existent token id', async function () { await expectRevert( - this.token.tokenURI(nonExistentTokenId), 'ERC721URIStorage: URI query for nonexistent token', + this.token.tokenURI(nonExistentTokenId), 'ERC721: token ID not minted', ); }); @@ -78,7 +78,7 @@ contract('ERC721URIStorage', function (accounts) { expect(await this.token.exists(firstTokenId)).to.equal(false); await expectRevert( - this.token.tokenURI(firstTokenId), 'ERC721URIStorage: URI query for nonexistent token', + this.token.tokenURI(firstTokenId), 'ERC721: token ID not minted', ); }); @@ -89,7 +89,7 @@ contract('ERC721URIStorage', function (accounts) { expect(await this.token.exists(firstTokenId)).to.equal(false); await expectRevert( - this.token.tokenURI(firstTokenId), 'ERC721URIStorage: URI query for nonexistent token', + this.token.tokenURI(firstTokenId), 'ERC721: token ID not minted', ); }); }); From 6cb133f3f3ccf645e7b45365ed09196b851f7465 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 26 May 2022 22:20:37 -0300 Subject: [PATCH 2/5] add internal helper _requireMinted --- contracts/token/ERC721/ERC721.sol | 13 ++++++++++--- .../token/ERC721/extensions/ERC721URIStorage.sol | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index d29aa603d5c..f52962a8efd 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -91,7 +91,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * @dev See {IERC721Metadata-tokenURI}. */ function tokenURI(uint256 tokenId) public view virtual override returns (string memory) { - require(_exists(tokenId), "ERC721: token ID not minted"); + _requireMinted(tokenId); string memory baseURI = _baseURI(); return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : ""; @@ -125,7 +125,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * @dev See {IERC721-getApproved}. */ function getApproved(uint256 tokenId) public view virtual override returns (address) { - require(_exists(tokenId), "ERC721: token ID not minted"); + _requireMinted(tokenId); return _tokenApprovals[tokenId]; } @@ -230,7 +230,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * - `tokenId` must exist. */ function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) { - require(_exists(tokenId), "ERC721: token ID not minted"); + _requireMinted(tokenId); address owner = ERC721.ownerOf(tokenId); return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender); } @@ -375,6 +375,13 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { emit ApprovalForAll(owner, operator, approved); } + /** + * @dev Reverts if the `tokenId` has not been minted yet. + */ + function _requireMinted(uint256 tokenId) internal virtual { + require(_exists(tokenId), "ERC721: token ID not minted"); + } + /** * @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address. * The call is not executed if the target address is not a contract. diff --git a/contracts/token/ERC721/extensions/ERC721URIStorage.sol b/contracts/token/ERC721/extensions/ERC721URIStorage.sol index 091e6788b1c..43ddc124d5f 100644 --- a/contracts/token/ERC721/extensions/ERC721URIStorage.sol +++ b/contracts/token/ERC721/extensions/ERC721URIStorage.sol @@ -18,7 +18,7 @@ abstract contract ERC721URIStorage is ERC721 { * @dev See {IERC721Metadata-tokenURI}. */ function tokenURI(uint256 tokenId) public view virtual override returns (string memory) { - require(_exists(tokenId), "ERC721: token ID not minted"); + _requireMinted(tokenId); string memory _tokenURI = _tokenURIs[tokenId]; string memory base = _baseURI(); From 41aca4a5e82f3e587ea9f22a98cb2fad17a766ad Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 26 May 2022 22:21:49 -0300 Subject: [PATCH 3/5] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e65f4f7bef8..0c3178f0e0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ * `EnumerableMap`: add new `Bytes32ToUintMap` map type. ([#3416](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3416)) * `SafeCast`: add support for many more types, using procedural code generation. ([#3245](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3245)) * `MerkleProof`: add `multiProofVerify` to prove multiple values are part of a Merkle tree. ([#3276](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3276)) - * `ERC721`, `ERC1155`: simplified revert reasons. ([#3254](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3254)) + * `ERC721`, `ERC1155`: simplified revert reasons. ([#3254](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3254), ([#3438](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3438))) ## 4.6.0 (2022-04-26) From 9d54939eb7c1ed4c5c0d235752bcb50abd585eea Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 30 May 2022 17:22:16 -0300 Subject: [PATCH 4/5] make function view --- contracts/token/ERC721/ERC721.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index f52962a8efd..efa3c5c17aa 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -378,7 +378,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { /** * @dev Reverts if the `tokenId` has not been minted yet. */ - function _requireMinted(uint256 tokenId) internal virtual { + function _requireMinted(uint256 tokenId) internal view virtual { require(_exists(tokenId), "ERC721: token ID not minted"); } From ca7d42f9c93a703e027693045fec20639047ecdc Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 1 Jun 2022 16:37:09 -0300 Subject: [PATCH 5/5] change revert reason text --- contracts/token/ERC721/ERC721.sol | 4 ++-- test/token/ERC721/ERC721.behavior.js | 20 +++++++++---------- .../ERC721/extensions/ERC721Burnable.test.js | 6 +++--- .../extensions/ERC721URIStorage.test.js | 6 +++--- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 39062c3a65b..62cb43be61d 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -69,7 +69,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { */ function ownerOf(uint256 tokenId) public view virtual override returns (address) { address owner = _owners[tokenId]; - require(owner != address(0), "ERC721: token ID not minted"); + require(owner != address(0), "ERC721: invalid token ID"); return owner; } @@ -378,7 +378,7 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { * @dev Reverts if the `tokenId` has not been minted yet. */ function _requireMinted(uint256 tokenId) internal view virtual { - require(_exists(tokenId), "ERC721: token ID not minted"); + require(_exists(tokenId), "ERC721: invalid token ID"); } /** diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index c688035b7b1..d6aa4e6b919 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -66,7 +66,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('reverts', async function () { await expectRevert( - this.token.ownerOf(tokenId), 'ERC721: token ID not minted', + this.token.ownerOf(tokenId), 'ERC721: invalid token ID', ); }); }); @@ -201,7 +201,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('reverts', async function () { await expectRevert( transferFunction.call(this, owner, other, nonExistentTokenId, { from: owner }), - 'ERC721: token ID not minted', + 'ERC721: invalid token ID', ); }); }); @@ -276,7 +276,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another nonExistentTokenId, { from: owner }, ), - 'ERC721: token ID not minted', + 'ERC721: invalid token ID', ); }); }); @@ -534,7 +534,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another context('when the given token ID does not exist', function () { it('reverts', async function () { await expectRevert(this.token.approve(approved, nonExistentTokenId, { from: operator }), - 'ERC721: token ID not minted'); + 'ERC721: invalid token ID'); }); }); }); @@ -623,7 +623,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('reverts', async function () { await expectRevert( this.token.getApproved(nonExistentTokenId), - 'ERC721: token ID not minted', + 'ERC721: invalid token ID', ); }); }); @@ -678,7 +678,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another describe('_burn', function () { it('reverts when burning a non-existent token id', async function () { await expectRevert( - this.token.burn(nonExistentTokenId), 'ERC721: token ID not minted', + this.token.burn(nonExistentTokenId), 'ERC721: invalid token ID', ); }); @@ -704,13 +704,13 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another it('deletes the token', async function () { expect(await this.token.balanceOf(owner)).to.be.bignumber.equal('1'); await expectRevert( - this.token.ownerOf(firstTokenId), 'ERC721: token ID not minted', + this.token.ownerOf(firstTokenId), 'ERC721: invalid token ID', ); }); it('reverts when burning a token id that has been deleted', async function () { await expectRevert( - this.token.burn(firstTokenId), 'ERC721: token ID not minted', + this.token.burn(firstTokenId), 'ERC721: invalid token ID', ); }); }); @@ -846,7 +846,7 @@ function shouldBehaveLikeERC721Enumerable (errorPrefix, owner, newOwner, approve describe('_burn', function () { it('reverts when burning a non-existent token id', async function () { await expectRevert( - this.token.burn(firstTokenId), 'ERC721: token ID not minted', + this.token.burn(firstTokenId), 'ERC721: invalid token ID', ); }); @@ -906,7 +906,7 @@ function shouldBehaveLikeERC721Metadata (errorPrefix, name, symbol, owner) { it('reverts when queried for non existent token id', async function () { await expectRevert( - this.token.tokenURI(nonExistentTokenId), 'ERC721: token ID not minted', + this.token.tokenURI(nonExistentTokenId), 'ERC721: invalid token ID', ); }); diff --git a/test/token/ERC721/extensions/ERC721Burnable.test.js b/test/token/ERC721/extensions/ERC721Burnable.test.js index 550fd05f9ae..e8fc3349431 100644 --- a/test/token/ERC721/extensions/ERC721Burnable.test.js +++ b/test/token/ERC721/extensions/ERC721Burnable.test.js @@ -37,7 +37,7 @@ contract('ERC721Burnable', function (accounts) { it('burns the given token ID and adjusts the balance of the owner', async function () { await expectRevert( this.token.ownerOf(tokenId), - 'ERC721: token ID not minted', + 'ERC721: invalid token ID', ); expect(await this.token.balanceOf(owner)).to.be.bignumber.equal('1'); }); @@ -60,7 +60,7 @@ contract('ERC721Burnable', function (accounts) { context('getApproved', function () { it('reverts', async function () { await expectRevert( - this.token.getApproved(tokenId), 'ERC721: token ID not minted', + this.token.getApproved(tokenId), 'ERC721: invalid token ID', ); }); }); @@ -69,7 +69,7 @@ contract('ERC721Burnable', function (accounts) { describe('when the given token ID was not tracked by this contract', function () { it('reverts', async function () { await expectRevert( - this.token.burn(unknownTokenId, { from: owner }), 'ERC721: token ID not minted', + this.token.burn(unknownTokenId, { from: owner }), 'ERC721: invalid token ID', ); }); }); diff --git a/test/token/ERC721/extensions/ERC721URIStorage.test.js b/test/token/ERC721/extensions/ERC721URIStorage.test.js index e823c56bd9b..eeacf5ebe36 100644 --- a/test/token/ERC721/extensions/ERC721URIStorage.test.js +++ b/test/token/ERC721/extensions/ERC721URIStorage.test.js @@ -31,7 +31,7 @@ contract('ERC721URIStorage', function (accounts) { it('reverts when queried for non existent token id', async function () { await expectRevert( - this.token.tokenURI(nonExistentTokenId), 'ERC721: token ID not minted', + this.token.tokenURI(nonExistentTokenId), 'ERC721: invalid token ID', ); }); @@ -78,7 +78,7 @@ contract('ERC721URIStorage', function (accounts) { expect(await this.token.exists(firstTokenId)).to.equal(false); await expectRevert( - this.token.tokenURI(firstTokenId), 'ERC721: token ID not minted', + this.token.tokenURI(firstTokenId), 'ERC721: invalid token ID', ); }); @@ -89,7 +89,7 @@ contract('ERC721URIStorage', function (accounts) { expect(await this.token.exists(firstTokenId)).to.equal(false); await expectRevert( - this.token.tokenURI(firstTokenId), 'ERC721: token ID not minted', + this.token.tokenURI(firstTokenId), 'ERC721: invalid token ID', ); }); });