Add EIP-712 name and version getters by RenanSouza2 · Pull Request #4303 · OpenZeppelin/openzeppelin-contracts (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits15 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Fixes #4226

Add internal name and version getters for EIP712

PR Checklist

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

Collaborator

Amxx left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

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

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 previously approved these changes Jun 9, 2023

Collaborator

Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

@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 changed the titleEip712 getters Add EIP-712 name and version getters

Jun 10, 2023

This was referenced

Sep 10, 2024

This was referenced

Sep 12, 2024