Virtual view functions

[Arachnid]

Please mark tokenURI in ERC721 virtual.

As it's not virtual, it can't be overridden by inheriting contracts. Making this virtual would allow contracts to implement programmatic generation of token URIs without using storage slots unnecessarily.

[nventuro]

Thanks for suggesting this @Arachnid!

When considering how virtual would apply to this library, we came to the conclusion that view methods should in general not be virtual: because access to the underlying state variables is not allowed (due to them being private), most overrides would not make sense in the first place.

I now think though that it may make sense to have virtual view methods, where their override is a function of the base implementation, like the following silly example for ERC20 balances:

function balanceOf(address account) public view override returns (uint256) {
   if (account == owner) {
     return UINT256_MAX;
   } else {
     return ERC20.balanceOf();
   }
}

This would allow you to implement an override for 721's tokenURI. However, I'd have to think some more on this.

Meanwhile, could you share a bit on what you had in mind for programmatic generation of the URI? Is it related to #1745?

[frangio]

The original reason why we decided to keep all view functions non-virtual was to make it easier for us to ensure derived contracts behave consistently. For example, if a derived contract overrides allowance(...), the implementation of transferFrom will not work as expected because it accesses the allowances storage mapping directly, rather than through the allowance getter. The general problem would be fixed if only getters accessed storage directly and all other functions used only getters, but then we'd have to pay attention that we're not making this mistake anywhere, so instead we decided to forbid overriding getters.

I think we can lift this restriction for some getters case-by-case. In the case of tokenURI I think it can make sense, but it seems incompatible with having _setTokenURI. I'm inclined to think we should integrate #1745 by default in ERC721, and allow overriding the tokenURI getter.

[Arachnid]

@nventuro Yes, similar to #1745. I want to be able to define a contract similar to this:

contract MyToken is ERC721, Ownable {
    using Strings for uint256;

    constructor() public ERC721("My Token", "MYT") { }
    
    function tokenURI(uint256 tokenId) public override returns(string memory) {
        return string(abi.encodePacked("https://example.com/token/", tokenId.fromUint256(), ".json"));
    }
}

I'd say that for the vast majority of tokens, all token URIs are procedurally generated, and so using a minimum of two storage slots to store their URI (or at least the suffix) is a huge waste. I'd like to be able to avoid it, at least for my own token.

Doing this does make _setTokenURI a no-op, but I'm happy with that - when I override ERC721 functions, I expect the functionality to change.

[Arachnid]

Another option here is for people like myself to implement IERC721Metadata ourselves - but the latest change, merging all this functionality into the base 721 contract seems to make that impossible.

[nventuro]

The original reason why we decided to keep all view functions non-virtual was to make it easier for us to ensure derived contracts behave consistently.

Heh, this is what I get for replying to these on a Sunday evening. I totally forgout about this, thanks for pointing it out @frangio!

Indeed, bundling #1745 seems like a good idea.

[Arachnid]

This is more a strategy thing than a specific issue, but I'd humbly suggest that allowing implementers to override parts of the behaviour is key to making OZ contracts as useful as possible. Certainly where it's problematic, as in the case you make with allowances, it needs to be documented at a minimum, but my personal preference order would be something like:

1. Use the public method internally where applicable, so that overrides work intuitively.
2. Document the caveats of overriding a method, but don't forbid doing so.
3. Forbid overriding the method.

There are bound to be many other use-cases where this is relevant, and by making it forbidden by the compiler, you make OZ useless in those use-cases.

[frangio]

We've decided to review this decision in the next release cycle since it's a backwards compatible change and we want to focus on getting 3.0 released as soon as possible. It's looking likely that we will make view functions virtual in general. Some more discussion in this forum thread.

[ProGamerCode]

I care about the extension functionality if its safe.

[nventuro]

Use the public method internally where applicable, so that overrides work intuitively.

@frangio what do you think about this? It would provide maximum flexibility at the cost of (likely) higher gas costs due to poor optimization, but it'd also open the door few a new kind of 'wrong' code. I believe there's enough advanced users that would benefit from this, and we wouldn't need to do much more than add the virtual attribute and write some documentation.

[frangio]

Yes I think we have to go ahead with this.

[frangio]

After offline conversation with @nventuro we think we will only implement this for select variables on a case-by-case basis. Doing this for all view functions can cause unforeseen issues and increase gas costs too much.

If we allowed overriding of ERC20.balanceOf, for example, it would force us to check twice that a transfer would not overflow, as illustrated by the following pseudocode.

function _transfer(...) {
    require(balanceOf(account) >= amount);
    balance[account] -= amount; // can overflow if `balanceOf()` returns a larger value than is in storage
}

[KaiRo-at]

OK, so I take it that I'll need to continue to do fully copies of OpenZeppelin code at times with just a virtual added - as e.g. ERC20 balanceOf() is indeed one function we had to override in the past for legit use cases (where we'd also override _transfer() BTW to first manifest the continuously virtually-minted balance with an actual _mint() and then do the actual transfer, so your pattern there would not break in our case), see the Mobilio token case I mentioned in the forum thread. I guess we are just too clever for plain OpenZeppelin. ;-)

[frangio]

There may definitely be use cases that are "too clever" to implement without copying the code and editing it. Please keep reporting these experiences anyway so we can analyze if there's anything we can do to support those changes natively.

[EvilJordan]

Here's a (maybe stupid) use-case:
I'd like to override both totalSupply and tokenByIndex to restrict calls to authorized callers only. Combined with a random or pseudo-random, non-incrementing (meaning not using Counters) tokenId, this would make it leagues harder to programmatically discover every token minted by the contract.

[Arachnid]

I'm strongly in favor of letting people shoot themselves in the foot here. Put appropriate warnings in documentation, sure, but not making these methods overridable just erects needless barriers for use.

Forcing people to copy the code and modify it will result in out-of-date copies of the code that will not get important bugfixes and security updates, too.

[abcoathup]

uri in ERC1155 is another case that could benefit from being made virtual
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.1.0/contracts/token/ERC1155/ERC1155.sol#L75

This would allow developers to implement a decentralized URI scheme using content addressing such as IPFS with a unique token URI per token ID.

[k06a]

I see the ability to make decimals()/name()/symbol() of ERC20 virtual, this would make these methods cheaper for on-chain calls since it would not involve SLOAD at all.

[frangio]

I will use this issue to collect all functions people have requested be made virtual. We want to get around to this soon.

• ERC721.tokenURI (this issue)
• ERC721.ownerOf (Please mark virtual override in ERC721 ownerOf (and potentially others) #2373)
• ERC721._isApprovedOrOwner (Override ERC721's _isApprovedOrOwner #2232)
• ERC721.isApprovedForAll (Migrate Contracts to Solidity v0.6 #2080 (comment))
• ERC20.balanceOf (Virtual balanceOf in ERC20 #2400)

[abcoathup]

• ERC1155.uri Improve extensibility of ERC1155 #2003 (comment)
• ERC1155.isApprovedForAll Improve extensibility of ERC1155 #2003 (comment)

[dievardump]

Hi there.

I wanted to second the "making virtual" of ERC1155.uri and also ERC721.tokenURI (to be honest, any that could have a usecase where developers need a specific behavior in their contract).

Adding my real world usecase if it helps to understand why this is needed:
I am creating a platform allowing users to create NFTs; can be ERC721 or ERC1155.
Those NFTs can be created by different users.

I do not host the NFT's JSON, they are uploaded on IPFS by the users, so I am not able to have a "one uri that covers all NFTS by replacing with {id}".
Especially since this make it too centralized as I would need to host the JSON files up to date, and if my platform was any day shat down, this would make NFTs not usable anymore.

So I have in my contract a mapping (tokenId => string) private tokenURIs; and I wish to use uri(uint256) to get the right uri for the current requested id.

Right now I can't just extend OZ contract because uri(uint256) is not virtual. This is the only function that I need to modify, and this is overkill to be forced to copy the contract locally. Adding virtual would still be compliant with the standard and I hope to see it coming soon.

[sapph1re]

I would love to see name() and symbol() virtual to enable overriding them. In my case, my client's desire is to have name changeable for a potential rebranding, and even though already discussed with the client that it might be not good to change name, it's still a desired feature.

So for now I had to clone OpenZeppelin's contracts to implement this. I believe it's better to leave it up to developers to decide whether they really want to override certain parts of the code, rather than completely restrict it.

[frangio]

Thank you all for the feedback. We're going to work on this feature for the next release.

[Skyge]

I want to add virtual for balance and totalSupply to override them, cause I meet a case, need a deflationary ERC20 token, so the balance of the user depends on underlying amount and factor, that is:

balanceOf(account) = balance * factor

so does the totalSupply

[sylar217]

Thank you all for the feedback. We're going to work on this feature for the next release.

@frangio is there a plan to make the uri() virtual in near term for ERC1155. If the release is nearby then I can probably wait, otherwise would figure out a workaround.

[frangio]

@sylar217 We plan to release virtual view functions during January.

[frangio]

We're actively working on this issue. Our current plan is to make view functions virtual, except when that causes consistency issues with the rest of the contract that are not trivial to fix. The typical explample is: if ERC20.balanceOf is virtual, does ERC20.transfer use the potentially overriden function to check that there is enough balance, or does it use its balances mapping directly? For these cases we will evaluate them one by one and depending on user requests and gas cost implications may make some of them virtual.

[frangio]

Virtual view functions will be available in the next release. The release candidate is already available for testing on npm at @openzeppelin/contracts@next.

Note that in many cases overriding a function will not imply that other functions in the contract will automatically adapt to the overridden definitions. For example, overriding ERC20.balanceOf does not guarantee that that balance is available for transferring. However, it will generaly be possible to obtain the desired behavior by overriding other functions in the contract, in this case ERC20._transfer. Understand that this limitation was necessary to keep the contracts simple and consistent, but it means that the responsibility to ensure correctness when functions are overriden will be on the users and not on the OpenZeppelin maintainers. People who wish to override should consult the source code to fully understand the impact it will have, and should consider whether they need to override additional functions to achieve the desired behavior. As always, feel free to ask for help in the OpenZeppelin Forum and we'll be happy to help.

[santisiri]

Might be useful to include this observation you make @frangio on the comments in the contracts. Will these upgrades be available also on the upgradeable contracts? (Might just clone them for the time being)

[frangio]

@santisiri These changes will be available on the upgradeable contracts tomorrow at the same time that we release the final 3.4 package.