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

EIP-712: Sign typed data #4803

Merged
merged 4 commits into from Sep 17, 2018

Conversation

@bitpshr
Copy link
Member

bitpshr commented Jul 15, 2018

This pull request implements necessary changes to fully support the current version of EIP-712 in both the old and new user interfaces. Note that most of the heavy-lifting of this implementation is handled by the updated signTypedData function in eth-sig-util.

Note: For now, we're displaying domain data as an inspectable object just like actual message data. Eventually, we should probably render this domain data differently (notice the mocked up "Domain / URL / Contract" section here, cc @cjeria.)

Resolves #4752
References ethereum/EIPs/pull/1222

Todo:

  • Update signTypedData and recoverTypedSignature
  • Bump eth-sig-util in eth-simple-keyring
  • Bump eth-sig-util in eth-hd-keyring
  • Bump both keyrings in eth-keyring-controller
  • Bump eth-keyring-controller in the extension
  • Update old UI to display data
  • Update new UI to display data
  • Ensure a non-breaking path for dapps using the old signTypedData method.

preview

@bitpshr bitpshr requested review from danfinlay, kumavis and danjm Jul 15, 2018

@@ -221,7 +238,7 @@ module.exports = class TypedMessageManager extends EventEmitter {
msg.status = status
this._updateMsg(msg)
this.emit(`${msgId}:${status}`, msg)
if (status === 'rejected' || status === 'signed') {
if (status === 'rejected' || status === 'signed' || status === 'errored') {

This comment has been minimized.

Copy link
@jennypollack

jennypollack Jul 17, 2018

Contributor

unclear when status gets set to 'errored' due to errors thrown in validateParams

This comment has been minimized.

Copy link
@bitpshr

bitpshr Jul 17, 2018

Author Member

I'm not sure I follow; what's unclear? The conditions that cause a message to be considered invalid? Error descriptions should be fine; are you suggesting to improve the documentation for the validateParams method to explicitly state what conditions are validated?

This comment has been minimized.

Copy link
@jennypollack

jennypollack Jul 17, 2018

Contributor

Ah sorry I was being dense, I didn't see the call to set the message status in metamask-controller

@danfinlay

This comment has been minimized.

Copy link
Contributor

danfinlay commented Jul 18, 2018

Does this PR enforce the validity of the domain field? I think enforcing the domain.url at least is a basic security feature we should include here. I believe that's part of the spec?

@leroldary

This comment has been minimized.

Copy link

leroldary commented Jul 19, 2018

@danfinlay The spec doesn't mention a domain.url field. See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-domainseparator.
It only explicitly mentions the verification of the chainId. Not sure if it's done in this PR.

@danfinlay

This comment has been minimized.

Copy link
Contributor

danfinlay commented Jul 19, 2018

Thanks for the links (was posting from phone earlier, doing bad research). I don't think we're enforcing that chainId parameter yet, would be a good improvement to make.

@bitpshr

This comment has been minimized.

Copy link
Member Author

bitpshr commented Jul 19, 2018

Thanks @danfinlay and @leroldary, great catch with the domain.chainId enforcement. I updated the pull request accordingly. I also confirmed with the EIP author that the domain separator isn't meant to validate the name field beyond schema validation.

@MrChico

This comment has been minimized.

Copy link

MrChico commented Jul 25, 2018

Hey, where can I find the tests associated with this functionality? Do they also include the smart contract verification of typed signatures?

@ipatka

This comment has been minimized.

Copy link

ipatka commented Jul 25, 2018

@MrChico Check out Example.js and Example.sol in the EIP712 repo
https://github.com/ethereum/EIPs/tree/master/assets/eip-712

@bitpshr

This comment has been minimized.

Copy link
Member Author

bitpshr commented Jul 25, 2018

@ipatka is correct, the EIP itself has example resources, and our implementation based on these resources lives in eth-sig-util. Also note that the EIP's example code intentionally (and temporarily) omitted array encoding, so we submitted an upstream PR with this functionality at ethereum/EIPs#1242 @MrChico.

@danfinlay
Copy link
Contributor

danfinlay left a comment

Ideally we render the domain info as more of a table, but that's no blocker. Let's get this out there!

Oh, also: Needs a changelog entry!!!

@cjeria

This comment has been minimized.

Copy link
Contributor

cjeria commented Jul 26, 2018

test failing but we want to merge!

@metamaskbot

This comment has been minimized.

Copy link

metamaskbot commented Jul 27, 2018

Builds ready [e45fa27]: mascara, chrome, firefox, edge, opera
@ipatka

This comment has been minimized.

Copy link

ipatka commented Jul 27, 2018

Is it possible to detect which Metamask version is installed and if this feature is available?

@whymarrh

This comment has been minimized.

Copy link
Contributor

whymarrh commented Jul 27, 2018

@ipatka this PR hasn't been merged into the application just yet

@ipatka

This comment has been minimized.

Copy link

ipatka commented Jul 27, 2018

@whymarrh right, is there a way to detect the version once it is merged?

@ipatka

This comment has been minimized.

Copy link

ipatka commented Jul 27, 2018

Also, will this break apps that use the previous implementation of EIP712 or will it support both?

@bitpshr

This comment has been minimized.

Copy link
Member Author

bitpshr commented Jul 27, 2018

Hi @ipatka. We plan to add a deprecation warning to the current implementation of eth_signTypedData (#4905) and ultimately replace it with the new implementation when this PR lands. We don't plan to concurrently support both versions since the prior version is both experimental and no longer adherent to any specification.

@danfinlay @cjeria CI all green again, but I think we should only land this after #4905 lands and the next release is cut. As soon as that's done, this can hit develop (and go out with the next release thereafter.)

@whymarrh

This comment was marked as outdated.

Copy link
Contributor

whymarrh commented Jul 28, 2018

@Maayu2415 please try to keep the discussion on-topic. If you have support questions, please reach out to support@metamask.io and they'll help answer any questions! Thanks!

@sullof

This comment has been minimized.

Copy link

sullof commented Sep 4, 2018

@bitpshr That is very helpful. Thanks so much.

@bitpshr

This comment has been minimized.

Copy link
Member Author

bitpshr commented Sep 10, 2018

This pull request has been rebased and updated to concurrently support both the old and new versions of EIP-712. To provide dapps with the smoothest transition possible, each version of this proposal is and will be uniquely name-spaced. For example, the original version will be available at eth_signTypedData_v1, the current version will be available at eth_signTypedData_v2, and if EIP-712 were to change again, that new version would be available at eth_signTypedData_v3, and so on.

Note: For now, the canonical RPC method eth_signTypedData still points to the original version of EIP-712, not the new version. This gives dapps sufficient time to test and develop against the new version at eth_signTypedData_v2 while not breaking dapps using the old version. We should blog about support of this new version, and also include a specific date when eth_signTypedData will point to this new version.

cc @danfinlay @bdresser

@metamaskbot

This comment has been minimized.

Copy link

metamaskbot commented Sep 10, 2018

Builds ready [edb7bba]: mascara, chrome, firefox, edge, opera

@bitpshr bitpshr referenced this pull request Sep 10, 2018

Closed

EIP-712: Array support #22

assert.doesNotThrow(() => {
sigUtil.typedSignatureHash(params.data)
}, 'Expected EIP712 typed data')
switch (params.version) {

This comment has been minimized.

Copy link
@bitpshr

bitpshr Sep 10, 2018

Author Member

For now, since there are only two versions of the proposal, we include both validation logic paths in the same TypedMessageManager class. If there ends up being even more iterations of the proposal with different validation schemes (unlikely, but possible), we can break this class down into a base Manager class and multiple implementations that extend it with a custom validateParams method.

return this.getState()
})
const version = msgParams.version
try {

This comment has been minimized.

Copy link
@bitpshr

bitpshr Sep 10, 2018

Author Member

Previously, we called down into the KeyringController, which in turn called into a specific keyring, which then called called signTypedData. This dependency depth made changes very difficult and it seemed like potentially-unnecessary indirection. To get around this, the MetaMaskController just does the signing directly without traversing down to a specific keyring. This also lends itself well to a future of signature-specific controllers at the extension level.

@@ -1253,6 +1263,9 @@ module.exports = class MetamaskController extends EventEmitter {
engine.push(createLoggerMiddleware({ origin }))
engine.push(filterMiddleware)
engine.push(this.preferencesController.requestWatchAsset.bind(this.preferencesController))
engine.push(this.createTypedDataMiddleware('eth_signTypedData', 'V1').bind(this))
engine.push(this.createTypedDataMiddleware('eth_signTypedData_v1', 'V1').bind(this))
engine.push(this.createTypedDataMiddleware('eth_signTypedData_v2', 'V2').bind(this))

This comment has been minimized.

Copy link
@bitpshr

bitpshr Sep 10, 2018

Author Member

Instead of using processTypedMessage when initializing the web3-provider-engine, we use custom middleware to intercept eth_signTypedData methods. This allows us to have fine-grained control over custom and non-custom RPC methods without touching the provider engine itself.

Note that for now, eth_signTypedData is still pointing to V1 of the EIP-712 proposal.

This comment has been minimized.

Copy link
@danfinlay

danfinlay Sep 12, 2018

Contributor

I told Pete Kim of Cipher that his implementation could be v2, and so the final one (this one) would be v3. Mind if we change to that? I think that will be a nice gesture to them, and leaves us open to backwards-support any apps using their version.

@metamaskbot

This comment has been minimized.

Copy link

metamaskbot commented Sep 11, 2018

Builds ready [b5fef2a]: mascara, chrome, firefox, edge, opera
@@ -1253,6 +1263,9 @@ module.exports = class MetamaskController extends EventEmitter {
engine.push(createLoggerMiddleware({ origin }))
engine.push(filterMiddleware)
engine.push(this.preferencesController.requestWatchAsset.bind(this.preferencesController))
engine.push(this.createTypedDataMiddleware('eth_signTypedData', 'V1').bind(this))
engine.push(this.createTypedDataMiddleware('eth_signTypedData_v1', 'V1').bind(this))
engine.push(this.createTypedDataMiddleware('eth_signTypedData_v2', 'V2').bind(this))

This comment has been minimized.

Copy link
@danfinlay

danfinlay Sep 12, 2018

Contributor

I told Pete Kim of Cipher that his implementation could be v2, and so the final one (this one) would be v3. Mind if we change to that? I think that will be a nice gesture to them, and leaves us open to backwards-support any apps using their version.

Addressed.

@danfinlay
Copy link
Contributor

danfinlay left a comment

This looks great, and non breaking! Can't wait to get this in the hands of developers!

@metamaskbot

This comment has been minimized.

Copy link

metamaskbot commented Sep 13, 2018

Builds ready [42fdcf6]: mascara, chrome, firefox, edge, opera

@danfinlay danfinlay merged commit 934b103 into develop Sep 17, 2018

16 checks passed

ci/circleci: prep-build Your tests passed on CircleCI!
Details
ci/circleci: prep-deps-npm Your tests passed on CircleCI!
Details
ci/circleci: prep-docs Your tests passed on CircleCI!
Details
ci/circleci: prep-scss Your tests passed on CircleCI!
Details
ci/circleci: test-deps Your tests passed on CircleCI!
Details
ci/circleci: test-e2e-beta-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-e2e-beta-firefox Your tests passed on CircleCI!
Details
ci/circleci: test-e2e-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-e2e-firefox Your tests passed on CircleCI!
Details
ci/circleci: test-integration-flat-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-integration-flat-firefox Your tests passed on CircleCI!
Details
ci/circleci: test-integration-mascara-chrome Your tests passed on CircleCI!
Details
ci/circleci: test-integration-mascara-firefox Your tests passed on CircleCI!
Details
ci/circleci: test-lint Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
coverage/coveralls Coverage decreased (-0.3%) to 54.538%
Details

@tmashuang tmashuang referenced this pull request Sep 17, 2018

Merged

Version 4.10.0 #5281

@qcdll qcdll referenced this pull request Sep 25, 2018

Open

fully integrate EIP-712 #117

@loriopatrick

This comment has been minimized.

Copy link

loriopatrick commented Oct 15, 2018

What's the status of this feature with the recent rollbacks? Release v4.14.0 does not seem to support it. We're currently using release v4.10.0 with success but would like to get our application in the hands others. It's hard to get people to manually setup an older version of MetaMask for a variety of reasons. Thanks.

@loriopatrick

This comment has been minimized.

Copy link

loriopatrick commented Oct 17, 2018

^ Resolved. Support added back in v4.14.0 however the parameters where swapped. It is now [signer, typedData].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.