Include EIP-5267 discovery in EIP-712

[Amxx]

Fixes #3968
Fixes LIB-633

Note: the changes to EIP712.sol will require a re-initialization to set the _NAME and _VERSION variables. This will be a sensitive operation, as the re initialization risks changing the domain separator.

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

Note that in 5.0 we should do the following breaking change:

-bytes32 private immutable _TYPE_HASH = ...
+bytes32 private constant _TYPE_HASH = ...

It is breaking because in the upgradeable version this is transpiled to a "live" storage slot (it shouldn't)

[changeset-bot]

🦋 Changeset detected

Latest commit: 2918dcc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

The storage error is caused by the absence of gaps for EIP712 to grow into. This will not be an issue in the upgradeable contracts.

We should run that checks after the transpilation

[frangio]

I have a slightly bigger issue/question I want to raise in addition to the above comments.

The current upgradeable version of EIP712 is not efficient. The domain separator cache is removed, which is necessary because the implementation contract doesn't know the address of the proxy (needed for verifyingContract) at the time of deployment. The bad part is that _HASHED_NAME and _HASHED_VERSION are storage variables. I believe the main reason for that was that our upgrades tooling didn't support immutable variables at the moment. The change we're introducing in this PR will require reinitializing the name and version (this should be mentioned in the changelog!). If the upgradeable contract stores those in immutable ShortStrings, which I think it should, the user will need to pass name and version in the constructor. (This is great for us because it's unmissable, unlike a required _init function call which is easy to miss!)

This all sounds like a good opportunity to properly make EIP712Upgradeable more efficient by keeping everything that is possible in immutable variables, fixing the current problem of _HASHED_NAME and _HASHED_VERSION. I think we should do this (maybe not necessarily in this PR). What do you think?

Note: The key thing we would need to do is to add annotations on all immutable variables:

    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment

(state-variable-assignment only needed if actually assigned.)

[Amxx]

The issue with #aed5659 is that it will break the storage layout by removing variables (and expanding the gap) which the upgrades plugin does not currently support :/

[frangio]

will break the storage layout by removing variables (and expanding the gap) which the upgrades plugin does not currently support

We will fix this with a manual patch with placeholder variables.

---

I've renamed variables to camel case.

[frangio]

This looks good. Once we merge we should check in the upgradeable repo to confirm we're getting the results we expected.

[frangio]

LGTM

[songhobby]

I have a slightly bigger issue/question I want to raise in addition to the above comments.

The current upgradeable version of EIP712 is not efficient. The domain separator cache is removed, which is necessary because the implementation contract doesn't know the address of the proxy (needed for verifyingContract) at the time of deployment. The bad part is that _HASHED_NAME and _HASHED_VERSION are storage variables. I believe the main reason for that was that our upgrades tooling didn't support immutable variables at the moment. The change we're introducing in this PR will require reinitializing the name and version (this should be mentioned in the changelog!). If the upgradeable contract stores those in immutable ShortStrings, which I think it should, the user will need to pass name and version in the constructor. (This is great for us because it's unmissable, unlike a required _init function call which is easy to miss!)

This all sounds like a good opportunity to properly make EIP712Upgradeable more efficient by keeping everything that is possible in immutable variables, fixing the current problem of _HASHED_NAME and _HASHED_VERSION. I think we should do this (maybe not necessarily in this PR). What do you think?

Note: The key thing we would need to do is to add annotations on all immutable variables:

    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment

(state-variable-assignment only needed if actually assigned.)

Will this be adopted in the future? (Passing name and version or other custom variables to the constructor of proxy contracts to initialize immutable state) It messes up with the current proxy contract interface.
The current upgradeable eip 712 is efficient by leveraging the function _EIP712NameHash() to return a string literal.

[frangio]

It messes up with the current proxy contract interface.

Can you elaborate on what you mean here?

[songhobby]

It messes up with the current proxy contract interface.

Can you elaborate on what you mean here?

Correct me if I'm wrong. Proxies can not utilize the logic in the constructor of implementation contract.
Immutable variables must and can only be assigned in the constructor. To use such immutable variables in the proxy contract, they must be declared in the proxy contract too. This changes the standard proxy contract interface.
We can abstract away the immutable variables into a separate contract (Immutable) for proxy contract to inherit from, but we still need to invoke the constructor of Immutable in the proxy's constructor.

 // ERC1967Proxy
 
  uint256 immutable private dummy;
  constructor(address _logic, bytes memory _data) payable {
    _upgradeToAndCall(_logic, _data, false);
    dummy = 0;
  }

[frangio]

To use such immutable variables in the proxy contract, they must be declared in the proxy contract too.

This is not correct.

A proxy can definitely use immutable variables defined in its implementation contract. However, there is a caveat, in that the values of those immutable variables will be set when the implementation is deployed (in its constructor), and will be shared by all proxies that use that implementation contract.

In the OpenZeppelin Upgrades Plugins, the reason we discourage immutable variables in upgradeable contracts and disallow them by default is that this behavior may be unexpected. But the constructorArgs option allows making use of them and initializing them in implementation contracts.

In the case of EIP712, with this PR the name and version will be set as immutables in the implementation contract. The proxy will then use those immutable values. The domain separator is also cached as an immutable, but in _domainSeparatorV4 the proxy will detect that it can't use that cached value (because address(this) is different) and will recompute it.

[songhobby]

Thanks for the correction! I agree. I think the point I was trying to make was

To initialize immutable variables in the proxy contract, they must be declared in the proxy contract.

What I was trying to achieve is that multiple proxy contracts can use the same implementation contract but with different immutable variables initialized when deploying the proxy contracts just like those state varaibles initialized in the proxy contract's initializer.

It's a contradiction because immutable variables must be initialized in constructor. To initialize immutable variables when deploying proxy contracts, they must be initialized in the proxy's constructor. But proxy contract itself can not be upgradeaded.

So It is impossible to have immutable variables initialized when deploying proxy contract when making them upgradeable in the future.

In that regard, it's really no difference in defining a function returning a constant and an immutable variable.

[frangio]

In that regard, it's really no difference in defining a function returning a constant and an immutable variable.

That's correct.

What I was trying to achieve is that multiple proxy contracts can use the same implementation contract but with different immutable variables

This will not be possible with this version of EIP712. Is this ability important to you?