Implement signing data with intended validator as part of EIP 191 #56

Bhargavasomu · 2019-02-20T07:54:30Z

What was wrong?

EIP 191 needs to be implemented as part of ethereum/web3.py#1241

How was it fixed?

Personal Sign was already implemented.
Support was added for signature version 0x00 (Signing Data with intended validator) by making defunct_hash_message take the signature_version and the version_specific_data as arguments. Then the signature_wrapper function was modified to add support to these signature versions.

Cute Animal Picture

Bhargavasomu · 2019-02-20T07:57:19Z

@pipermerriam could you please review? Thankyou.

Bhargavasomu · 2019-02-20T16:31:48Z

@kclowes I have a few doubts regarding this signing procedure. This PR aims to support the new signing mechanism as per EIP 191. But my major doubts are the following

During the whole lifecycle of the message which one signs, where is it being verified? (Could you please link me the code if we have it in web3.py)
Further even if it verified somewhere, where is the message being extracted properly? (Like from b'\x19Ethereum Signed Message:\n' where are we extracting the part and showing it to the user?) Because I hoped that this would be happening in web3.py, but I couldn't find it anywhere in my searches.

Could you please give me clarity on the above questions, so that I could do better testing with this signing mechanism? Thankyou.

/cc @pipermerriam

Bhargavasomu · 2019-02-23T11:36:00Z

@pipermerriam @kclowes, please correct me if my below stated claims are wrong anywhere.

I believe that the verification of the signature is done through recoverHash and recoverTransaction methods
Further I believe that the Exact Message extraction from the encoded message, is NOT needed anywhere throughout the process. For example, it is not needed to get the <msg> part from b'\x19Ethereum Signed Message:\n<msg>' anywhere throughout the lifecycle of the message.

The above claims of mine are based on the following reasons and scenario.
IIUC below is the workflow, intended for web3 or Metamask.

Note: In the below description, 3rd party also includes Smart Contract.

A 3rd party, requests something (like a transaction) from the user.
The 3rd party first encodes the message using web3.py as per EIP191 (or) EIP712 (Depends on what he chooses) and then hashes the message. This step is combined in the defunct_hash_message function.
That 3rd party sends the hash of the message (instead of direct message, for security reasons) to the user from which it is requesting, to sign the hash of the message.
The user uses web3.py to sign this hashed message using his/her public key (web3.eth.accounts.sign I guess, maybe wrong) and sends it to the 3rd party.
The 3rd party uses the recoverHash or recoverTransaction functions using the signature of the user to get the Account Address and verify that this address is indeed what they are looking for.
So in any of the above steps, the Exact Message is not needed to be decrypted or extracted from.

Requests and Confirmations

Based on above explanation, I don't see why web3.py has to be changed with respect to implementation of EIP191 or EIP712. In other words, web3.py should automatically reflect the EIP191 and EIP712 standard, with the changes made only in this repository (and not in web3.py).
That said, I think this PR is valid. Could you please give this a review? Thankyou.

kclowes · 2019-02-26T04:40:23Z

@Bhargavasomu sorry it has taken me so long to respond! I should have some time tomorrow and I'll definitely have time on Wednesday to review.

kclowes · 2019-02-27T23:50:36Z

@Bhargavasomu I need a little more time to wrap my head around the PR. I hope I'll be able to get you some feedback/answers to questions tomorrow!

kclowes · 2019-02-28T23:53:25Z

Based on above explanation, I don't see why web3.py has to be changed with respect to implementation of EIP191 or EIP712. In other words, web3.py should automatically reflect the EIP191 and EIP712 standard, with the changes made only in this repository (and not in web3.py).

Yes, I think that's correct!

kclowes

This is looking good, @Bhargavasomu! I left a few comments, mostly about wording. I also didn't see that this was just the implementation for EIP 191 at first so I left some comments about EIP 712. We can disregard the EIP 712 comments for now and take into account in the next PR.

eth_account/messages.py

eth_account/_utils/signing.py

Bhargavasomu · 2019-03-01T13:47:20Z

@kclowes done! Please review whenever you have time. Thankyou.

kclowes

Looks good @Bhargavasomu!

My comments are mostly nitpicky comments on the docstring.

Can we add some more tests to make sure the signature version b'\x00' runs like we'd expect? I'd like to see a test for each type of message (text, bytestring, and hexstr) and a test to make sure that the TypeError gets raised if the address is different than 20 bytes long. Then I think we'll be good to merge! Thank you!

eth_account/messages.py

Bhargavasomu · 2019-03-05T15:01:20Z

@kclowes @pipermerriam could you please check if the docs are right now? I still haven't added the tests, but will add them in a while.

Bhargavasomu · 2019-03-05T15:35:05Z

@kclowes I have added the new tests also. Please have a look and let me know. Thankyou!

kclowes

@Bhargavasomu I just left a few style nitpicks. Feel free to take or leave them. This is looking good!

tests/core/test_accounts.py

Bhargavasomu · 2019-03-07T06:59:57Z

@kclowes done, review please.

kclowes

Looks good to me! Thanks @Bhargavasomu!

tests/core/test_accounts.py

eth_account/_utils/signing.py

eth_account/messages.py

tests/core/test_accounts.py

Bhargavasomu · 2019-03-11T13:48:12Z

@pipermerriam done. Please review again. Thankyou.

pipermerriam · 2019-03-11T14:26:19Z

eth_account/messages.py

@@ -28,7 +28,7 @@ def defunct_hash_message(
    Currently you can only specify the ``signature_version`` as following.
    version ``0x45`` (version ``E``): ``b'\\x19Ethereum Signed Message:\\n'``
    concatenated with the number of bytes in the message.
-    NOTE: This is the defualt version followed, if the signature_version is not specified.
+    .. note:: This is the defualt version followed, if the signature_version is not specified.
    version ``0x00`` (version ``0``): Sign data with intended validator (EIP 191).


@Bhargavasomu can you build the docs locally and verify this renders correctly. I'm thinking there's some whitespace needed here but not sure exactly where and how much.

@pipermerriam I have fixed the docs, and the output is as follows.

tests/core/test_accounts.py

Bhargavasomu · 2019-03-11T17:22:48Z

@pipermerriam ready for another round of review. Thankyou.

Implement signing data with intended validator (EIP 191)

409bba2

kclowes reviewed Feb 28, 2019

View reviewed changes

eth_account/messages.py Outdated Show resolved Hide resolved

eth_account/messages.py Outdated Show resolved Hide resolved

eth_account/_utils/signing.py Outdated Show resolved Hide resolved

Bhargavasomu force-pushed the implement_eip_191 branch from 40dc5b9 to 4b31d84 Compare March 1, 2019 13:39

kclowes requested changes Mar 4, 2019

View reviewed changes

eth_account/messages.py Outdated Show resolved Hide resolved

eth_account/messages.py Outdated Show resolved Hide resolved

eth_account/messages.py Outdated Show resolved Hide resolved

Bhargavasomu force-pushed the implement_eip_191 branch from 4b31d84 to dfe87f9 Compare March 5, 2019 14:58

Modify Docstrings for inclusion of EIP191

81438a2

Bhargavasomu force-pushed the implement_eip_191 branch from dfe87f9 to 81438a2 Compare March 5, 2019 15:00

kclowes reviewed Mar 5, 2019

View reviewed changes

tests/core/test_accounts.py Outdated Show resolved Hide resolved

tests/core/test_accounts.py Outdated Show resolved Hide resolved

tests/core/test_accounts.py Outdated Show resolved Hide resolved

Add some more tests for version 0x00 of EIP 191

9043d76

Bhargavasomu force-pushed the implement_eip_191 branch from d3e554c to 9043d76 Compare March 7, 2019 06:57

kclowes approved these changes Mar 8, 2019

View reviewed changes

tests/core/test_accounts.py Outdated Show resolved Hide resolved

pipermerriam requested changes Mar 11, 2019

View reviewed changes

eth_account/_utils/signing.py Outdated Show resolved Hide resolved

eth_account/_utils/signing.py Outdated Show resolved Hide resolved

eth_account/messages.py Outdated Show resolved Hide resolved

tests/core/test_accounts.py Outdated Show resolved Hide resolved

pipermerriam reviewed Mar 11, 2019

View reviewed changes

tests/core/test_accounts.py Outdated Show resolved Hide resolved

Bhargavasomu added 4 commits March 11, 2019 22:46

Fix bug in the EIP 191 tests and tighten tests

4fd586a

Update docstrings and exception messages

95af186

Replace the signature versions as constant named variables

e06169c

Fix new EIP191 doc changes to make rendering smooth

321d676

Bhargavasomu force-pushed the implement_eip_191 branch from 828f3fb to 321d676 Compare March 11, 2019 17:18

pipermerriam approved these changes Mar 11, 2019

View reviewed changes

pipermerriam merged commit 7be70c3 into ethereum:master Mar 11, 2019

Bhargavasomu mentioned this pull request Mar 12, 2019

Implement recently added signing standards from Parity and Geth ethereum/web3.py#1241

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement signing data with intended validator as part of EIP 191 #56

Implement signing data with intended validator as part of EIP 191 #56

Bhargavasomu commented Feb 20, 2019

Bhargavasomu commented Feb 20, 2019

Bhargavasomu commented Feb 20, 2019

Bhargavasomu commented Feb 23, 2019 •

edited

Loading

kclowes commented Feb 26, 2019

kclowes commented Feb 27, 2019

kclowes commented Feb 28, 2019

kclowes left a comment

Bhargavasomu commented Mar 1, 2019

kclowes left a comment

Bhargavasomu commented Mar 5, 2019

Bhargavasomu commented Mar 5, 2019

kclowes left a comment

Bhargavasomu commented Mar 7, 2019

kclowes left a comment

Bhargavasomu commented Mar 11, 2019

pipermerriam Mar 11, 2019

Bhargavasomu Mar 11, 2019

Bhargavasomu commented Mar 11, 2019

Implement signing data with intended validator as part of EIP 191 #56

Implement signing data with intended validator as part of EIP 191 #56

Conversation

Bhargavasomu commented Feb 20, 2019

What was wrong?

How was it fixed?

Cute Animal Picture

Bhargavasomu commented Feb 20, 2019

Bhargavasomu commented Feb 20, 2019

Bhargavasomu commented Feb 23, 2019 • edited Loading

Requests and Confirmations

kclowes commented Feb 26, 2019

kclowes commented Feb 27, 2019

kclowes commented Feb 28, 2019

kclowes left a comment

Choose a reason for hiding this comment

Bhargavasomu commented Mar 1, 2019

kclowes left a comment

Choose a reason for hiding this comment

Bhargavasomu commented Mar 5, 2019

Bhargavasomu commented Mar 5, 2019

kclowes left a comment

Choose a reason for hiding this comment

Bhargavasomu commented Mar 7, 2019

kclowes left a comment

Choose a reason for hiding this comment

Bhargavasomu commented Mar 11, 2019

pipermerriam Mar 11, 2019

Choose a reason for hiding this comment

Bhargavasomu Mar 11, 2019

Choose a reason for hiding this comment

Bhargavasomu commented Mar 11, 2019

Bhargavasomu commented Feb 23, 2019 •

edited

Loading