Functions in ERC721Base.sol not marked as virtual

Question

Functions in ERC721Base.sol not marked as virtual

JuanCarlos-Spaceport opened this issue 2 years ago · 3 comments

JuanCarlos-Spaceport commented 2 years ago

Hey,

I was going trough the implementation of ERC721 and noticed a little incoherence on the functions approve, setApprovalForAll and isApprovedForAll.

On other token implementations like ERC1155 or ERC20 those functions are marked as virtual, is this an intended implementation? If so, would't it make more sense to keep those functions as in the ERC721 definition?

Answer 1 · 2022-12-05T20:43:19.000Z

Which file exactly? Yes, there are some inconsistencies, especially in the case of ERC721 because I rarely use that standard. Some of these inconsistencies may have been fixed or partially fixed in #171.

Answer 2 · 2022-12-06T14:30:12.000Z

So far only noticed the inconsistencies on ERC721Base.sol, the methods approve, isApprovedForAll and setApprovalForAll should be marked as virtual.

This is not fixed in #171 but if you want to i can create a PR with the changes that should be made :)

Answer 3 · 2022-12-06T15:16:38.000Z

See ERC721BaseInternal. In SolidState, internal functions should always be marked virtual, while external functions should never be marked virtual. If you need to override behavior, override the internal function.