Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify ERC721 revert reasons #3438

Merged
merged 7 commits into from Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Expand Up @@ -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)))
* `ERC721`: removed redundant require statement. ([#3434](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3434))

## 4.6.0 (2022-04-26)
Expand Down
13 changes: 10 additions & 3 deletions contracts/token/ERC721/ERC721.sol
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
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
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
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
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