Skip to content

Fix listing validation: add failsafe for nonexistent tokens #364

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

Merged
merged 7 commits into from
Jul 17, 2023

Conversation

kumaryash90
Copy link
Member

No description provided.

address operator;

// failsafe for reverts in case of non-existent tokens
try IERC721(_assetContract).ownerOf(_tokenId) returns (address _owner) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this just return the zero address? it doesn't throw in solidity right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it won't throw. The operator and owner variables are zero address by default. The check below will evaluate to false in case these remain zero due to external calls failing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then why do we need the try catch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ownerOf function does throw an error in some ERC-721 contracts out there. For example, this is the ownerOf code in the OpenZeppelin ERC-721 contract:

function ownerOf(uint256 tokenId) public view virtual override returns (address) {
        address owner = _owners[tokenId];
        require(owner != address(0), "ERC721: invalid token ID");
        return owner;
}

try IERC721(_assetContract).ownerOf(_tokenId) returns (address _owner) {
owner = _owner;

try IERC721(_assetContract).getApproved(_tokenId) returns (address _operator) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh is this the call that throws? i would assume that also just returns false if the tokenId doesnt exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah both ownerOf and getApproved check if the token-id exists, and revert if not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For most cases, including create and update listing, this is fine. The call would just revert.
But it affects the view function for getting the valid listings.

@Vunguyen0909
Copy link

Yes

@nkrishang nkrishang merged commit 11f6726 into main Jul 17, 2023
@kumaryash90 kumaryash90 deleted the directlistings-fix-validation branch July 17, 2023 17:23
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.

4 participants