From 62f2c0531b2f283f953b5daab86ff0c1a95675f4 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 1 Jun 2022 18:22:13 -0300 Subject: [PATCH] Simplify ERC721 revert reasons (#3438) --- CHANGELOG.md | 2 +- contracts/token/ERC721/ERC721.sol | 13 +++++++++--- .../ERC721/extensions/ERC721URIStorage.sol | 2 +- test/token/ERC721/ERC721.behavior.js | 20 +++++++++---------- .../ERC721/extensions/ERC721Burnable.test.js | 6 +++--- .../extensions/ERC721URIStorage.test.js | 6 +++--- 6 files changed, 28 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f897e66de28..eb6b5c0a32a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ * `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)) * `MerkleProof`: add calldata versions of the functions to avoid copying input arrays to memory and save gas. ([#3200](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3200)) - * `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))) * `ERC721`: removed redundant require statement. ([#3434](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3434)) * `PaymentSplitter`: add `releasable` getters. ([#3350](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3350)) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 36500727d41..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: owner query for nonexistent token"); + require(owner != address(0), "ERC721: invalid token ID"); 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"); + _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: approved query for nonexistent token"); + _requireMinted(tokenId); return _tokenApprovals[tokenId]; } @@ -374,6 +374,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 view virtual { + require(_exists(tokenId), "ERC721: invalid token ID"); + } + /** * @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 f55fdb54c46..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), "ERC721URIStorage: URI query for nonexistent token"); + _requireMinted(tokenId); 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 aaec0b8f226..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: owner query for nonexistent token', + 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: owner query for nonexistent token', + 'ERC721: invalid token ID', ); }); }); @@ -276,7 +276,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another nonExistentTokenId, { from: owner }, ), - 'ERC721: owner query for nonexistent token', + '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: owner query for nonexistent token'); + '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: approved query for nonexistent token', + '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: owner query for nonexistent token', + 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: owner query for nonexistent token', + 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: owner query for nonexistent token', + 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: owner query for nonexistent token', + 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), 'ERC721Metadata: URI query for nonexistent token', + 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 60abea65d22..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: owner query for nonexistent token', + '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: approved query for nonexistent token', + 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: owner query for nonexistent token', + 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 c476ad0912a..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), 'ERC721URIStorage: URI query for nonexistent token', + 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), 'ERC721URIStorage: URI query for nonexistent token', + 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), 'ERC721URIStorage: URI query for nonexistent token', + this.token.tokenURI(firstTokenId), 'ERC721: invalid token ID', ); }); });