Skip to content

Simplify ERC721 revert reasons#3438

Merged
frangio merged 7 commits intoOpenZeppelin:masterfrom
frangio:simplify-721-reasons
Jun 1, 2022
Merged

Simplify ERC721 revert reasons#3438
frangio merged 7 commits intoOpenZeppelin:masterfrom
frangio:simplify-721-reasons

Conversation

@frangio
Copy link
Contributor

@frangio frangio commented May 27, 2022

Also adds a helper to check that a token ID is minted / exists.

@frangio frangio requested a review from Amxx May 27, 2022 01:21
@frangio frangio added this to the 4.7 milestone May 27, 2022
@frangio frangio self-assigned this May 30, 2022
@Amxx
Copy link
Collaborator

Amxx commented May 31, 2022

I'm not sure "token ID not minted" is the right message here. A token that has been minted and then burned doesn't exist anymore. IMO "token ID not minted" can be interpreted as "the ID was not minted" which in that case would be wrong.

@frangio
Copy link
Contributor Author

frangio commented May 31, 2022

Would you suggest another phrase?

I agree that the message is not strictly precise in the sense that you describe, but I think it's an improvement over the current ones. I don't see it as serious that a burned token then says "not minted".

@Amxx
Copy link
Collaborator

Amxx commented May 31, 2022

I still prefer "nonexistent token ID" or "invalid token ID"

Amxx
Amxx previously approved these changes Jun 1, 2022
@Amxx Amxx enabled auto-merge (squash) June 1, 2022 20:27
@frangio frangio disabled auto-merge June 1, 2022 21:22
@frangio frangio merged commit 62f2c05 into OpenZeppelin:master Jun 1, 2022
@frangio frangio deleted the simplify-721-reasons branch June 1, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants