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

Allow eip712 signatures to use 712Domain as primaryType #6661

Closed
wants to merge 1 commit into from

Conversation

danfinlay
Copy link
Contributor

As discussed here:
https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286

And merged in sig-util here:
MetaMask/eth-sig-util#51

Allows the creation of cheaper-to-verify signatures. Cheers to @SilentSicero

@danfinlay
Copy link
Contributor Author

Oh we have to integrate this into our keyrings before this is a proper PR.

@danfinlay
Copy link
Contributor Author

danfinlay commented May 28, 2019

A brief rant:

The customary method of node-style dependencies (CommonJS require style) allows all dependencies to have their own versions of shared dependencies, which is great for making sure you don’t break dependencies, but can make a real pain out of managing a unified version bump across dependencies.

Ex: Bumping eth-sig-util requires bumping all of our eth-keyring modules up the dependency graph up to MetaMask:

An alternative approach, that I was discussing with @kumavis recently is treating dependencies like capabilities, in which case they are passed into dependent modules as constructor arguments. This means there’s more passing around modules, but it also means it’s easy to unify the behavior of many dependent modules in a complex graph, as well as potentially smaller bundle sizes. This is more like dependency injection, and makes dependency type-checking more important than perhaps more arbitrary semver strings.

An open question: Should I move our keyring controller behavior over to that pattern? Would make adding/revising signing methods easier in the future.

@whymarrh whymarrh deleted the eip712-enhancement branch July 23, 2019 19:25
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.

1 participant