Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider EIP712 implementation #452

Closed
pakim249CAL opened this issue Feb 1, 2023 · 4 comments · Fixed by #458
Closed

Reconsider EIP712 implementation #452

pakim249CAL opened this issue Feb 1, 2023 · 4 comments · Fixed by #458
Assignees

Comments

@pakim249CAL
Copy link
Contributor

pakim249CAL commented Feb 1, 2023

#444 (comment)

@pakim249CAL
Copy link
Contributor Author

pakim249CAL commented Feb 2, 2023

After investigation, I've come to the following conclusions:

Using openzeppelin's EIP712 contract adds a significant amount of bytecode (~0.8kB). This is likely largely due to the domain separator being calculated on runtime rather than stored as an immutable value, and their inefficient use of strings in errors.

There is simply no way for a domain separator to be an immutable value in the implementation contract and be exclusive to the proxy contract without rearchitecting the proxy which I consider to be unsafe. AFAIK, our only options are to either:

  • Use a storage variable and initialize it
  • Calculate it on runtime

Considering that calculating on runtime invades into our scant bytecode space, I will try a solution where we use a storage var.

@MerlinEgalite
Copy link
Contributor

Yes, let's just try OZ upgradeable's version? I think this would be safer too

@pakim249CAL
Copy link
Contributor Author

That is also going to be no good. In my POC, it still takes an extraordinary amount of bytecode. I don't think bytecode was a priority for OZ's implementation. I'm reconsidering just reworking the existing code.

@pakim249CAL pakim249CAL changed the title Consider using standardize EIP712 Reconsider EIP712 implementation Feb 2, 2023
@pakim249CAL pakim249CAL linked a pull request Feb 3, 2023 that will close this issue
@MerlinEgalite
Copy link
Contributor

This should be closed right @pakim249CAL ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants