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

Add support for eth_signTypedData and personal_signTypedData RPC call #1319

Merged
merged 2 commits into from
May 1, 2019

Conversation

Bhargavasomu
Copy link
Contributor

What was wrong?

Related to Issue #1241

How was it fixed?

The eth_signTypedData was created by mimicking the eth_sign implementation throughout the codebase.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@Bhargavasomu Bhargavasomu force-pushed the eip_712 branch 3 times, most recently from 6c5d71a to 60b1afb Compare April 8, 2019 16:02
@Bhargavasomu
Copy link
Contributor Author

@kclowes I am getting the following error in most of the test cases on circle ci.

ValueError: Unknown RPC Endpoint: eth_signTypedData

I even tried by replacing eth_signTypedData with account_signTypedData, but I am getting the same error. Is there something I am missing? Thankyou.

@kclowes
Copy link
Collaborator

kclowes commented Apr 8, 2019

@Bhargavasomu It doesn't look like eth_signTypedData has been released yet for Geth, so we can add an xfail to those tests. An xfail example can be seen here:

pytest.xfail('Needs ability to efficiently control mining')

For Parity, it looks like we'll have to add the --jsonrpc-experimental flag (from this PR) when we start the parity node for the tests.

@Bhargavasomu Bhargavasomu force-pushed the eip_712 branch 25 times, most recently from 3527243 to 6f9c682 Compare April 9, 2019 11:38
@Bhargavasomu Bhargavasomu force-pushed the eip_712 branch 3 times, most recently from dd90672 to 455dfd9 Compare April 16, 2019 10:28
@Bhargavasomu
Copy link
Contributor Author

@kclowes The docs have been updated now too. Please review and let me know.

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple nitpicky comments, but this is looking really good.

I have one point to clarify: Is it true that the reason that we are doing so much validation of the typedData in eth-account and not here is because eth-account can be used without connection to a node?

Thanks!

web3/_utils/module_testing/eth_module.py Outdated Show resolved Hide resolved
web3/_utils/module_testing/personal_module.py Outdated Show resolved Hide resolved
web3/_utils/rpc_abi.py Outdated Show resolved Hide resolved
@Bhargavasomu Bhargavasomu force-pushed the eip_712 branch 9 times, most recently from 401af98 to 101b6a8 Compare April 18, 2019 07:18
@Bhargavasomu
Copy link
Contributor Author

@kclowes yes that is why I didn't put up the validations as part of web3.py.

  1. It can be used without a connection to the node.
  2. Moreover I feel that all such validations should be done on the node side, rather than at web3 side. (Please correct me if I am wrong.)

I have addressed the other issues. Please review once more. Thankyou.

docs/web3.geth.rst Outdated Show resolved Hide resolved
web3/_utils/module_testing/eth_module.py Outdated Show resolved Hide resolved
@Bhargavasomu
Copy link
Contributor Author

@kclowes I have addressed the issue about skip_if_testrpc. Please review. Thankyou.

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks @Bhargavasomu! @pipermerriam do you have any other feedback here?

@Bhargavasomu
Copy link
Contributor Author

@pipermerriam is this good to merge with?

@tuxxy
Copy link

tuxxy commented Apr 28, 2019

Bump on this PR? We (@nucypher) are currently blocking on this and this would alleviate the pressure a ton.

docs/web3.eth.rst Show resolved Hide resolved
tests/integration/parity/test_parity_http.py Show resolved Hide resolved
web3/_utils/module_testing/eth_module.py Outdated Show resolved Hide resolved
tests/integration/parity/common.py Outdated Show resolved Hide resolved
web3/_utils/module_testing/personal_module.py Outdated Show resolved Hide resolved
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.

5 participants