Support ERC191 v0 signatures

[NoahZinsmeister]

This PR adds support for ERC191 v0 signatures via a new eth_signTypedData_v0 RPC call:

web3.currentProvider.sendAsync({
  method: 'eth_signTypedData_v0',
  params: [
    <address>,
    [<address>, {type: 'uint256', value: '1'}, {type: 'string', value: 'test'}]
  ],
  from: <address>
}, (error, result) => {
  if (error || result.error) console.error(error)
  console.log(result.result)
})

An example of what this looks like (pending possible UI improvements):

This PR has two Companion PRs in eth-sig-util and eth-json-rpc-middleware. In this PR, those dependencies were pointed to personal Github packages with the changes applied. This should obviously be changed when those PRs are merged and the updated versions are available on NPM.

1. #44 on eth-sig-util. The temporary package.
2. #8 on eth-json-rpc-middleware. The temporary package.

[bitpshr]

Hi @NoahZinsmeister, thanks for taking the time to work on this. Apologies of my context is limited, but as I understood it, EIP-191 specifies a general format for handling signed data, and downstream proposals like EIP-712 specify implementations that adhere to this format. For example, eth_signTypedData from EIP-712 explicitly conforms to the format set forth in EIP-191:

The encoding is compliant with EIP-191. The 'version byte' is fixed to 0x01, the 'version specific data' is the 32-byte domain separator domainSeparator and the 'data to sign' is the 32-byte hashStruct(message).

Does EIP-191 actually define a new RPC method that we need to implement, or just a format for handling signed data in a general sense?

[NoahZinsmeister]

Hi @bitpshr, no problem, happy to help. Thanks for the response! Some context on the PR:

You're correct that ERC-712 specifies a particular valid ERC-191 signing format. This specification is actually just the 0x01/v1 version of ERC-191, as you correctly note. My understanding is that eth_signTypedData is the name of the RPC endpoint that handles this specification. It can also generically handle other specifications as well, however, which I'll explain below. (Note: if I'm mistaken about this, and eth_signTypedData is meant only for ERC-712, I'd be happy to discuss renaming the RPC endpoint used by this PR.)

In MetaMask, the underlying eth_signTypedData endpoint is actually already overloaded to handle two types of signing: ERC-712 signing via eth_signTypedData_v3, and the custom (soon to be deprecated?) MetaMask typed data signing via eth_signTypedData.

This PR and the companion PRs adds a third signing version (which is actually ERC-191 0x00/v0, hence the PR title) to the already overloaded eth_signTypedData, accessible via eth_signTypedData_v0.

I'd be more than happy to get into the implications for smart contracts of each of these three approaches, but for now will just note that:

• Each of these three specifications are valid methods of verifying signatures.
• Each has different implications for on-chain data structures related to signature verification.
• Each should be fully supported by Metamask, since each offer the ability to show users a robust set of (plaintext!) data that hashes to the thing they actually sign.

[danfinlay]

If we do add this fourth message format (not evaluating the merit atm), could we simply number it sequentially (v4)? Is there a defensible reason this deserves the v0 designation?

[NoahZinsmeister]

@danfinlay the only reason to keep it as v0 would be to keep the ERC-191 version byte in sync with the _vX suffix. (By which logic the existing eth_signTypedData should be v1.)

[bitpshr]

Hi @NoahZinsmeister, thanks for the clarification. I see now that EIP-191 specifies a general scheme for signed data to follow, as well as a v0 version of handling signed data (I missed this since it's only one sentence in the proposal!) Candidly, I worry about the growing number of options for signing data via RPC methods, especially since EIP-712 also supports the ability to pass a contract address as an intended validator. Why not just use EIP-712 with a validator contract?

Regardless, I don't think this method should be implemented as part of the eth_signTypedData namespace. This namespace is specific to EIP-712 and the RPC method it defines. The version scheme (and function overloading) we currently use there is unrelated to the associated version byte in signatures, and instead has to do with the history of how that proposal developed. For example, v1 is an early implementation that some dapps still use, v2 is a Cipher-specific early implementation, and v3 is the canonical implementation that's currently spec-adherent.

Taking this a bit further, I don't think we should (or can) actively support this signature scheme until a canonical RPC method is either added to EIP-191 or standardized by some other related proposal. For example, what should the method be called across different browsers? What's its signature? How does it handle errors? All such method semantics are defined in other proposals that define new RPC methods like EIP-712, but EIP-191 offers no information. Without a standardized RPC method with known semantics, there's no guarantee if or how other dapp browsers will implement this functionality, making it mostly useless to dapps that want any form of browser portability.

I don't think the information in EIP-191 that suggestions a v0 signature scheme in passing is enough information to implement an RPC method across multiple clients.

[NoahZinsmeister]

Hi @bitpshr thanks for the response. To address some of your concerns:

Why not just use EIP-712 with a validator contract?

ERC-712 imposes a non-trivial cost on dApp developers; namely, it requires them to include an unwieldy and error-prone amount of hard-coded strings in their smart contracts. Developers who choose not to do this should not be penalized by clients such as MetaMask for using simpler signature schemes that, along with ERC-712 signatures, have the property of being composed of the hashes of human-readable plaintext data.

Regardless, I don't think this method should be implemented as part of the eth_signTypedData namespace.

That's fair! When it comes to namespaces, there are 3 options:

1. Let eth_signTypedData be a catch-all for all ERC-191-compatible signatures, where individual clients are tasked with overloading this call and/or interpreting the received 'typed data' correctly (presumably, in MetaMask's case, using _vX suffixes).
2. Reserve eth_signTypedData exclusively for ERC-712 signatures, and use a different name for e.g. ERC-191 v0 signatures.
3. Come up with a different name for an endpoint that would function like option 1, and route eth_signTypedData/ERC-712 calls internally to this endpoint.

Without a standardized RPC method with known semantics, there's no guarantee if or how other dapp browsers will implement this functionality, making it mostly useless to dapps that want any form of browser portability.

Personally, I think that option 1 above is robust enough to address these concerns. This is evidenced precisely by the existence of MetaMask's current implementation of eth_signTypedData, which is not compliant with any public spec. I'd argue that eth_signTypedData should accept any ERC-191 signature scheme with sufficient community buy-in.

And while I agree that it would be great to have a unified and explicit ERC-191 spec (should this be discussed in ERC-191, ERC-712, or another ERC?), the absence of such should not prevent MetaMask from overloading eth_signTypedData, at least until these proposals are hammered out, especially considering that this overloading is exactly what the current implementation does. This would be a strict (and opt-in) improvement over current UX, and if anything would only accelerate the development of a unified cross-browser spec.

[bitpshr]

Hi @NoahZinsmeister. The only reason MetaMask overloads signing methods right now is for backwards compatibility. This will go away in the future.

Of the naming options you listed, the only standards-compliant option is # 2, to come up with a totally unique endpoint for v0-style signatures. This highlights the issue of implementing a proprietary RPC method: we're making the RPC implementation up as we go. No v0 RPC method exists in any standard yet beyond a one-sentence description of what the scheme (not the method) could look like. While I understand the use case for this signature (sort of...just so developers don't have to deal with the verbosity of EIP-712?) I don't feel that it warrants a MetaMask-specific, one-off RPC method before any discussion around such an endpoint in any public forum. How would any other dapp browser also know how to add this experimental functionality?

The work in this pull request looks good. I'm not against the implementation, but I do think MetaMask has a responsibility as a major dapp browser to introduce APIs slowly, only when they reach even some form of pseudo-standardization or maturity; otherwise, we run the risk of having major dapps prematurely buy into experimental functionality, a painful issue we're still dealing with today.

The only way I could see this landing in MetaMask is behind some obviously-experimental RPC method name so developers know they're using completely proprietary functionality, like metamask_signSimpleData or something similar. Still, there just doesn't seem to be anything to implement yet: no method signature, no method name, no error handling semantics, no mention that an RPC method should ever even exist, no RPC-anything, just:

Version 0 has <20 byte address> for the version specific data, and the address is the intended validator.

If you think it's important enough, I think it would be great to begin discussion around the need for this RPC method on Ethereum Magicians, either as an addition to EIP-191 or as a new related proposal. If RPC method semantics are discussed, we'd be much more inclined to add this functionality since it's implementable both in MetaMask and other across other browser clients.

[whymarrh]

Hey @NoahZinsmeister, thanks for submitting a pull request! As an open source project, we thrive on community contributions, so thank you!

While this change may implement some great improvements, our contribution guidelines require that new features first get team buy-in as issues before considering specific pull requests.

Since we do not yet have a feature request issue that justifies this particular change, we are closing the pull request for now. Please open an issue, make the case for this change, and reference this pull request in the issue. From there we can discuss whether this is a change that MetaMask would like to incorporate.

For changes to our external-facing APIs, we further recommend opening an EIP or at least a post on ethereum-magicians.org, to engage community participation in the development of this proposed standard, before advocating for MetaMask-specific integration.