Why do you call certain public functions by using the contract name?

I was looking at the _isApprovedOrOwner function in your implementation of ERC-721:

function _isApprovedOrOwner(address spender, uint256 tokenId) internal view virtual returns (bool) {
    address owner = ERC721.ownerOf(tokenId);
    return (spender == owner || isApprovedForAll(owner, spender) || getApproved(tokenId) == spender);
}

I'm curious. Why are you calling the ownerOf by front-loading the contract name ERC721? Why not write it like this instead?

address owner = ownerOf(tokenId);

Is the logic that the ownerOf method might be inherited, but you want the _isApprovedOrOwner function to always call the local ownerOf?

Do you mean that the method might be overridden? If so, yes. The reasoning behind this is that if the code ends up invoking an override of ownerOf it will probably break pretty badly.

Nowadays, I think we should stop doing this and just use the unqualified (potentially overriden) function. Our policy anyway is that if a developer overrides a function with custom logic it is their responsibility to make sure that other things don't break. It's very difficult/impossible for us to write contracts that are customizable while at the same time making sure they stay internally consistent given arbitrary customizations.

A good option could be to separate the function we use internally from the function used externally, so that they could be overriden separately.

1 Like

I've opened an issue to track the possibility of changing this:

Yes

Great, thanks for confirming.

Seems fair to me.

I don't like this approach because it's the kind of thing that can easily create confusion - there will be multiple functions for doing the same thing.

And there's also a contract size concern - it's not difficult to go over the 24kB limit.

Yeah I share this concern a bit as well.