Implement EIP 712

[Bhargavasomu]

What was wrong?

Fixes Issue #35

How was it fixed?

Example.js of the EIP was taken as reference.

TODOS

1. Supporting the scenario where the data could be arrays (Including error handling etc.)
2. Validation of the JSON Structured data. (With the use of Regex)
3. Appropriate Error Handling. (Especially in case where the data is not of the specified corresponding type)
4. Add tests for the failing or Exception rising cases
5. Add tests for the Array Data and its corresponding Errors or Exceptions.

Cute Animal Picture

[Bhargavasomu]

@kclowes @pipermerriam there is still stuff to be done and this is just a WIP, but could you please give me a preliminary review? Thankyou.

The hash values match with those given in Example.js which is mentioned in the EIP.

[fubuloubu]

@Bhargavasomu I'm working on adding EIP 712 support in a Plasma Cash client I am writting in Python here: https://github.com/zatoichi-labs/plasma-cash-vyper/tree/signed-structs

I can try and implement using your updates to see how the EIP 712 compatibility works in a practical use case.

[kclowes]

Looking good @Bhargavasomu! I just left a few comments

[Bhargavasomu]

Also @kclowes should we move out the EIP712 functionalities into a separate _utils file? Because it seems worth another file.

EDIT - I have added the best validation I can do to the JSON message in the latest push. Please check it out. The only part left now, is supporting the array data fields. Please let me know if the structure needs to be changed in the code before we finalize the stuff.

/cc @pipermerriam

[pipermerriam]

I

[Bhargavasomu]

Also I feel that this whole EIP 712 can have its own _utils module which would include the validation.py and hashing.py as separate files. Am I good to go on that (or) do you feel that putting all of them in the same file is good?

[pipermerriam]

I'm fine having an eth_account._utils.eip712 module.

[kclowes]

If it's all the same to you two, I'd rather call it something more descriptive like structured_data_signing or something rather than eip712

[pipermerriam]

High level this looks good. Leaving it to @kclowes for final review. @carver might want to give it a look too since this is the eth-account codebase and this stuff is sensitive code in case there are any red flags you see that I dont.

[carver]

@carver might want to give it a look too since this is the eth-account codebase and this stuff is sensitive code in case there are any red flags you see that I dont.

I have some ideas about how I would restructure the code (including some code not added in this PR), and add a new API, but it is not a quick thing so I'll save it for later. Edit: created #58

I don't see any red flags in any of the existing code. I didn't do a deep dive on the structured message encoder implementation, but it's all pretty separated from any key handling. No objections 👍

[davesque]

@Bhargavasomu I just cut a new eth-abi beta release that fixes the type parsing issue: ethereum/eth-abi#126

[Bhargavasomu]

@pipermerriam @kclowes The whole EIP has been implemented (Apart from the rendering changes that need to be made in web3.py side). Could you please review. Thankyou.

[Bhargavasomu]

@pipermerriam @kclowes review please.

[pipermerriam]

@kclowes all of my issues have been addressed (assuming the things in this review are cleaned up as somu see's fit)

[Bhargavasomu]

@pipermerriam I have addressed all your issues. Thankyou.

[kclowes]

Looks good to me @Bhargavasomu!

[davesque]

I think this was requested here. Just added some new methods to the eth-abi API to allow for checking if a type is recognized by the registry. Cut eth-abi v2.0.0-beta.8 just now which includes those features.

[davesque]

Method documentation found here:
https://eth-abi.readthedocs.io/en/latest/eth_abi.html#eth_abi.codec.ABIEncoder.is_encodable_type
https://eth-abi.readthedocs.io/en/latest/eth_abi.html#eth_abi.registry.ABIRegistry.has_encoder

Related exceptions (you probably won't have to worry about these too much):
https://eth-abi.readthedocs.io/en/latest/eth_abi.html#eth_abi.exceptions.NoEntriesFound
https://eth-abi.readthedocs.io/en/latest/eth_abi.html#eth_abi.exceptions.MultipleEntriesFound

[Bhargavasomu]

@davesque I don't think the new version of eth-abi which is 2.0.0b8 is found by python 3.5. It's recognized only by python 3.6

[davesque]

@Bhargavasomu Correct. Python 3.5 support (and, thus, also pypy support) was removed.

[Bhargavasomu]

@davesque but here in eth-account, we are supporting both python 3.5 and pypy.

@pipermerriam @carver should we remove the support for python 3.5 and pypy?

[pipermerriam]

@Bhargavasomu we're discussing internally what we want to do about this python issue. Maybe you can revert your last changes and keep this PR using it's own hand rolled utility and open an issue to track the need to update once python3.5 is no longer supported.

[Bhargavasomu]

This PR is now back to the old version and have opened the issue here #59

[kclowes]

This looks good to me @Bhargavasomu! Nice work!

[Bhargavasomu]

Is there something more to be done as part of this PR? If not, could someone please merge this?