Need correctly-upgradeable `contracts-upgradeable/metatx/ERC2771ContextUpgradeable.sol`.

[pegahcarter]

🧐 Motivation

We are moving towards an upgradeable future and should follow upgradeable standards across the repo.

📝 Details

ERC2771ContextUpgradeable.sol, while labeled as upgradeable, does not follow typical upgradeable conventions:

• There is no __ERC2771ContextUpgradeable_init() method.
• Storage is not namespaced.

I suggest the OZ team upgrades the contract.

[ernestognw]

Hi @pegahcarter, thanks for reporting.

The ERC2771ContextUpgradeable contract is working as expected. The trusted forwarder is stored in an immutable variable to avoid reading from storage every time.

This means that 1) because it's not in storage it doesn't require a namespace storage as it wouldn't collide with other variables, and 2) an initializer is not required since there's no other state to initialize.

Is there any specific need you have for the contract? In the case of replacing the forwarder, you can always override the trustedForwarder in an upgrade and set a new forwarder (or store it in a variable).

[pegahcarter]

From my understanding, the state set in the implementation is not read into the proxy. So, setting an immutable in BaseContract which calls ERC2771ContextUpgradeable in the constructor would not be read once the contract is plugged in as the implementation of proxy. Is that correct?

Edit: I've found from some research that immutables are able to be seen by the proxy contract, so this isn't technically an issue. However, I would still suggest using namespaced storage to follow conventions found across upgrade contracts, where for example, traditional ERC20/721 have immutables but the upgradeables do not. Additionally, it is a best practice to keep all state away from the implementation and set it upon initialization of the proxy.

[Amxx]

traditional ERC20/721 have immutables

The core ERC20 and ERC721 contract do NOT use immutable.

Some extension do use them to store values that are not subject to change (for example the cap of an ERC20Capped`, but these values are always "per instance". We feel that if you have an implementation for an upgradeable ERC20 token that includes ERC20CappedUpgradeable, all the proxy/clones that use that implementation should have the possibility to set different caps.

We believe that ERC2771Context is very different, in the sens that its actually a good approach to have all instances that use the same implementation also use the same forwarder. We also think that reading the forwarder from storage induces storage access cost that should be avoided.

If you want to read the forwarder from storage, you can do that easily by overriding the trustedForwarder() function:

abstract contract ERC2771ContextWithStorageUpgradeable is ERC2771ContextUpgradeable {
    // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.ERC2771ContextWithStorage")) - 1)) & ~bytes32(uint256(0xff))
    bytes32 private constant ERC2771ContextWithStorageStorageLocation = 0x61469ac04768c7d538ac14807828d5c931641c8631772801f7e0afeda9085900;

    function __ERC2771ContextWithStorageUpgradeable_init(address trustedForwarder_) internal onlyInitializing {
        StorageSlot.getAddressSlot(ERC2771ContextWithStorageStorageLocation).value = trustedForwarder_;
    }

    function trustedForwarder() public view virtual override returns (address) {
        return StorageSlot.getAddressSlot(ERC2771ContextWithStorageStorageLocation).value;
    }
}

[pegahcarter]

Thanks for pointing out I was wrong in the ERC20/721 immutability.

This short code solution is what I had in mind. So, in the end we're not truly dealing with immutable storage here if we can override it anyway.

The issue I have is requiring implementations to access the forwarder storage. I'm suggesting to keep implementations storage-free to reduce confusion and clearly separate use of storage. It doesn't make sense to mangle storage locations to reduce gas costs.