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

refactor EIP712 #458

Merged
merged 3 commits into from Feb 6, 2023
Merged

refactor EIP712 #458

merged 3 commits into from Feb 6, 2023

Conversation

pakim249CAL
Copy link
Contributor

@pakim249CAL pakim249CAL commented Feb 2, 2023

Pull Request

Issue(s) fixed

This pull request:

This POC limits EIP712 changes to only the changes needed to have the domain separator be calculated on runtime so that the proxy contract is the verifying authority rather than the implementation.

It doesn't seem viable to use the full EIP712 openzeppelin implementation right now because it takes up too much bytecode.

@pakim249CAL pakim249CAL changed the title feat: add poc feat: add 712 poc Feb 2, 2023
@Rubilmax Rubilmax changed the base branch from dev to fix/ci-solc February 3, 2023 08:39
@Rubilmax Rubilmax changed the base branch from fix/ci-solc to dev February 3, 2023 08:39
Copy link
Contributor

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

Referencing: #453 (comment)

I'd be more incline to approve this change if we at least use ECDSA.recover from OZ

src/MorphoInternal.sol Show resolved Hide resolved
MerlinEgalite
MerlinEgalite previously approved these changes Feb 3, 2023
src/MorphoInternal.sol Show resolved Hide resolved
@MerlinEgalite
Copy link
Contributor

I think #444 can be linked to this PR too

Tristan22400
Tristan22400 previously approved these changes Feb 3, 2023
@pakim249CAL
Copy link
Contributor Author

Referencing: #453 (comment)

I'd be more incline to approve this change if we at least use ECDSA.recover from OZ

I haven't tested, but just to give you a quick answer I believe that ECDSA.sol is a big culprit in bytecode waste due to its numerous error messages in the form of strings.

@pakim249CAL pakim249CAL changed the title feat: add 712 poc refactor EIP712 Feb 3, 2023
@Rubilmax
Copy link
Contributor

Rubilmax commented Feb 6, 2023

Referencing: #453 (comment)

I'd be more incline to approve this change if we at least use ECDSA.recover from OZ

Approved in the end, because I just compared OZ's implementation to ours. With this change, our way of doing it is very similar to OZ's

@Rubilmax
Copy link
Contributor

Rubilmax commented Feb 6, 2023

There's a catch: each Morpho instance will share the same EIP712 version, because it's stored as a constant. This has no impact as we've discussed before and would require further changes to move it to immutable, so I approve this PR as is.

@Tristan22400
Copy link
Contributor

Tristan22400 commented Feb 6, 2023

There's a catch: each Morpho instance will share the same EIP712 version, because it's stored as a constant. This has no impact as we've discussed before and would require further changes to move it to immutable, so I approve this PR as is.

I am not sure to understand your point. What does it change to go from constant to immutable ?

@Rubilmax
Copy link
Contributor

Rubilmax commented Feb 6, 2023

I am not sure to understand your point. What does it change to go from constant to immutable ?

Moving to an immutable would allow it to be set at deployment, customized according to the e-mode or set by governance, for example. So each Morpho instance could in practice have a specific eip712 version

@pakim249CAL pakim249CAL merged commit ab8b977 into dev Feb 6, 2023
@pakim249CAL pakim249CAL deleted the poc/eip712-1 branch February 6, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconsider EIP712 implementation What would be the impact of delegate on the account manager approve system?
4 participants