Add EIP-712 name and version getters

[RenanSouza2]

Fixes #4226

Add internal name and version getters for EIP712

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: ce00023

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]

IMO, eip712Domain() public should use these two new internal function (in case they are ever overriden)

EDIT: actually, if the values ever change (because of an override) that would cause a lot of trouble ... so I'll remove the virtual keyword for the 2 internal functions.

[RenanSouza2]

IMO, eip712Domain() public should use these two new internal function (in case they are ever overriden)

EDIT: actually, if the values ever change (because of an override) that would cause a lot of trouble ... so I'll remove the virtual keyword for the 2 internal functions.

Yes, agreed

how about an internal setFunction(atring) so it can also be overriden?

Edit: saw your edit now

[Amxx]

I don't think this values should ever be changed. If they were, the caches would be invalid ... and we can't update the immutable cache.

Also I don't think frontends are designed to handle such changes

[RenanSouza2]

Makes sense, I commited with the getters used internaly and there are no more changes to be done, right?

Also the checks / tests-upgradeable is failling but I gess it it some conflict with the upgradeable contract that already have this getters, I`ll see how to fix this

[Amxx]

It looks good to me, but I'd like @ernestognw and @frangio 's opinion on whether the getters should be virtual.

Its critical that the values returned by the getters are constant in time, because the eip721domain must match the (immutable) cached values.

[Amxx]

LGTM

[ernestognw]

It looks good to me, but I'd like @ernestognw and @frangio 's opinion on whether the getters should be virtual.

Its critical that the values returned by the getters are constant in time, because the eip721domain must match the (immutable) cached values.

Perhaps there's a use case in overriding any of those values. For example, if a contract wants to invalidate a domain within an upgrade (eg. reject signatures based on the version).

Still, I'm not convinced it's worth the risk of letting users override them.

Are you really sure those values will never change? @Amxx

[frangio]

@jbcarpanelli suggested making all functions virtual but adding an annotation saying whether overriding is either fully okay, or okay under certain conditions (invoking super), or probably never okay. The idea is to still allow these overrides for the shadowy coders who want that but to document the potential implications.

E.g.:

@custom:overrideability { unrestricted | restricted | discouraged }

[ernestognw]

I had to update the upgradeable scripts. However, there were a few issues related to previous changes we've merged.
I think it makes sense but since it's my first time using the upgradeable patch I'll require a review from @frangio and @Amxx again.

Almost there!

[Amxx]

Perhaps there's a use case in overriding any of those values. For example, if a contract wants to invalidate a domain within an upgrade (eg. reject signatures based on the version).

If the internal getters are override, and the value changes during the contract's life, the eip712domain will be updated but the cache will not be modified. The domain separator used for recovery will stay the old one.

On the upgradeable version of this contract, the storage is very different. It doesn't have a cache, and recomputes the domainSeparator every time. The upgradeable version already includes getters, and they are virtual.

So any decision we make here should NOT be motivated by contract upgradeability.

[frangio]

LGTM.