Skip to content

Commit

Permalink
Simplify ERC721 revert reasons (#3438)
Browse files Browse the repository at this point in the history
  • Loading branch information
@frangio
frangio committed Jun 1, 2022
1 parent 4fc9fd3 commit 62f2c05
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 21 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Expand Up @@ -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))

Expand Down
13 changes: 10 additions & 3 deletions contracts/token/ERC721/ERC721.sol
View file
Expand Up @@ -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;
}

Expand All @@ -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())) : "";
Expand Down Expand Up @@ -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];
}
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion contracts/token/ERC721/extensions/ERC721URIStorage.sol
View file
Expand Up @@ -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();
Expand Down
20 changes: 10 additions & 10 deletions test/token/ERC721/ERC721.behavior.js
View file
Expand Up @@ -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',
);
});
});
Expand Down Expand Up @@ -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',
);
});
});
Expand Down Expand Up @@ -276,7 +276,7 @@ function shouldBehaveLikeERC721 (errorPrefix, owner, newOwner, approved, another
nonExistentTokenId,
{ from: owner },
),
'ERC721: owner query for nonexistent token',
'ERC721: invalid token ID',
);
});
});
Expand Down Expand Up @@ -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');
});
});
});
Expand Down Expand Up @@ -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',
);
});
});
Expand Down Expand Up @@ -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',
);
});

Expand All @@ -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',
);
});
});
Expand Down Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});

Expand Down
6 changes: 3 additions & 3 deletions test/token/ERC721/extensions/ERC721Burnable.test.js
View file
Expand Up @@ -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');
});
Expand All @@ -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',
);
});
});
Expand All @@ -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',
);
});
});
Expand Down
6 changes: 3 additions & 3 deletions test/token/ERC721/extensions/ERC721URIStorage.test.js
View file
Expand Up @@ -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',
);
});

Expand Down Expand Up @@ -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',
);
});

Expand All @@ -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',
);
});
});
Expand Down

0 comments on commit 62f2c05

Please sign in to comment.