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

Implement structured signing according to EIP712 #35

Closed
pipermerriam opened this issue Sep 26, 2018 · 14 comments
Closed

Implement structured signing according to EIP712 #35

pipermerriam opened this issue Sep 26, 2018 · 14 comments

Comments

@pipermerriam
Copy link
Member

pipermerriam commented Sep 26, 2018

What was wrong?

No support for EIP712 style signed messages: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md

How can it be fixed?

Implement a low level signing API for creating and verifying EIP712 style signatures with appropriate primatives for wiring up to a higher level integration into the Account class.

@pipermerriam
Copy link
Member Author

Up for discussion on what the name for this new signing method should be and what API it should expose. Given the complexity of structured messages, I don't think it is reasonable to try and define the API up-front, but suggesting a few names for the actual method.

  • signStructured
  • signEIP712
  • signStructuredData

@pipermerriam
Copy link
Member Author

cross linking to EIP191: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-191.md which has a different use case (and should be implemented independently if we see forward progress on adoption)

@carver
Copy link
Contributor

carver commented Sep 26, 2018

As long as 712 is in Draft status, the new method will need a big honking warning in the docs saying that it the hash is subject to change without a major version bump, and that it shouldn't be used in prod.

I think we're free to use snake_case here. Maybe sign_typed_data?

If the method takes in data (in whatever format) and returns the 32-byte hash, then it will be easy to use with these other eth-account methods:

@Bhargavasomu
Copy link
Contributor

@pipermerriam is this still up for me to work on? Further IIUC, then we are just adding another type of signature mechanism apart from the existing one. So, was this signature introduced for a specific use case (or) is it just another plugin kind of thing that the users could use?

@pipermerriam
Copy link
Member Author

Yes, you are still 👍 to run with this. I'm going to delegate to @kclowes to lead figuring out details of the implementation. My intuition is that it'll be 1-2 new signature APIs, one for the EIP191 with 0x00 prefix, and one for the EIP191 + EIP712 structured signing. These APIs probably exist both on the Web3 namespace somewhere, delegating to the underlying ethereum node for the signing, as well as on the eth-account API for locally managed private keys, potentially with a corresponding middleware that does the proper interception of messages to perform local signing.

@fubuloubu
Copy link
Contributor

Another API that would be handy to have for EIP 712 support:

def recoverHash(self, message_hash, vrs=None, signature=None):

Something similar to that which takes an EIP712-compatible object and a VRs/raw signature as args.

@carver
Copy link
Contributor

carver commented Mar 22, 2019

For now, I think it's fine to ask people to compose the two separately, like:

msg_hash = eip712_make_hash(message)  # the new API that needs to be added for signing, also
eth_account.Account.recoverHash(msg_hash, vrs)  # use the existing API for recovery

@fubuloubu
Copy link
Contributor

oh yeah, I didn't realize that it took the hash and not the raw message.

an API for hashing a struct would then be a requirement I would think.

@Bhargavasomu
Copy link
Contributor

@fubuloubu the API for hashing the whole EIP712 message, is being implemented in #57. But please note that it is not a separate API. It uses the existent defunct_hash_message as the entry point.

@fubuloubu
Copy link
Contributor

@pipermerriam Random naming question, why is it a defunct hash message? Defunct to me means "no longer functioning", does it have a particular technical definition that means something completely different?

@kclowes
Copy link
Collaborator

kclowes commented Mar 22, 2019

I have wondered this myself, @fubuloubu. Unless it means something specific, I think it would be a good idea to change it to something else

@pipermerriam
Copy link
Member Author

pipermerriam commented Mar 22, 2019

See this docstring:

Awkwardly, the number of bytes in the message is encoded in decimal ascii. So
if the message is 'abcde', then the length is encoded as the ascii
character '5'. This is one of the reasons that this message format is not preferred.
There is ambiguity when the message '00' is encoded, for example.
Only use this method if you must have compatibility with
:meth:`w3.eth.sign() <web3.eth.Eth.sign>`.

This message format results in ambiguity making it impossible to differentiate between fundamentally different messages that produce the same final result.

@carver
Copy link
Contributor

carver commented Mar 22, 2019

Random naming question, why is it a defunct hash message? Defunct to me means "no longer functioning", does it have a particular technical definition that means something completely different?

Haha, yeah, somehow some new functionality started sneaking into that function since I first wrote it ;)

It was only originally intended to only be used for version b'E' which is defunct. Other newer formats were intended to add new methods. I hadn't really designed what those other methods looked like yet, though, so there wasn't a template for people to follow yet.

Here are my first thoughts on a new API: #58

@carver
Copy link
Contributor

carver commented May 7, 2019

Closed & released in v0.4

@carver carver closed this as completed May 7, 2019
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

No branches or pull requests

5 participants