This repository was archived by the owner on May 13, 2022. It is now read-only.
Contract eth_signTypedData#176
Merged
loredanacirstea merged 6 commits intoraiden-network:masterfrom Nov 25, 2017
Merged
Conversation
LefterisJP
approved these changes
Nov 25, 2017
|
|
||
| // Hash the prefixed message string | ||
| bytes32 prefixed_message_hash = keccak256(prefixed_message); | ||
| var message_hash = keccak256( |
Contributor
There was a problem hiding this comment.
you and me both know that this should be kept in sync with the function, but just add a comment here for future maintainers that these hashed strings should be in sync with this function's parameters
| returns (address) | ||
| { | ||
| var hash = keccak256( | ||
| keccak256('address Address', 'uint32 Value', 'uint192 Value2'), |
contracts/utils/sign.py
Outdated
| names e.g. ('receiver', 'block_created', 'balance') | ||
| data e.g. ('0x5601ea8445a5d96eeebf89a67c4199fbb7a43fbb', 3000, 1000) | ||
| """ | ||
| assert len(types) == len(data) == len(names), 'Check argument length.' |
Contributor
There was a problem hiding this comment.
I think the assert message should not read like a comment, but like an error message. So something like:
'Argument tuple length mismatch'
contracts/tests/utils.py
Outdated
| from utils.sign import eth_signed_typed_data_message | ||
|
|
||
| def balance_proof_hash(receiver, block, balance): | ||
| return eth_signed_typed_data_message(('address', ('uint', 32), ('uint', 192)), |
Contributor
There was a problem hiding this comment.
Style:
return eth_signed_typed_data_message(
('address', ('uint', 32), ('uint', 192)),
('receiver', 'block_created', 'balance'),
(receiver, block, balance)
)
|
|
||
| contract_verified_address = contract.call().verifyBalanceProof( | ||
| receiver, open_block_number, | ||
| balance, balance_msg_sig) |
Contributor
There was a problem hiding this comment.
style: Close parentheses in its own line. Each arg in its own line
contracts/tests/test_ecverify.py
Outdated
| web3.testing.mine(30) | ||
| verified_address = ecverify_test_contract.call().verify_ecrecover_output(hash_prefixed_message, r, s, v) | ||
| verified_address = ecverify_test_contract.call().verify_ecrecover_output(balance_message_hash, r, s, v) | ||
| # assert address == verified_address |
contracts/tests/test_ecverify.py
Outdated
| balance_msg_sig, addr = sign.check(balance_message_hash, tester.k0) | ||
| assert addr == A | ||
|
|
||
| # r = signed_message[:32]; |
Contributor
|
Left some comments. Once addressed this looks good. Finally, getting rid of those ugly string concatenations :) |
As suggested in the PR review.
As suggested in the PR review
Contributor
Author
|
Comments addressed |
This was referenced Nov 28, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #139 (EIP712 proposal)
fixes #140 (gas cost)
fixes #86 (string concatenations -> ugly code)
eth_signTypedData-based logic for balance proof messages instead ofpersonal_signReasons for not supporting both EIP712 & previous logic for verifying the balance proof:
verifyBalanceProoffunction to use when closing a channel.Other reasons for implementing EIP712:
Populus gas cost estimations now, with
eth_signTypedData-based logic (might be higher in real life):test_close_by_receiver 60149
test_close_by_sender 69928 (cooperative close)
test_close_by_sender_challenge_settle_by_receiver 108124
test_close_by_sender_challenge_settle_by_sender 103498
Be aware that:
however, I do not expect the contract implementation to change in a breaking waywe need to add 2 methods for calculating the message hash; the second method is not yet implemented [WIP] Add eth_signTypedData as a standard for machine-verifiable and human-readable typed data signing with Ethereum keys ethereum/EIPs#712 (comment) ; code: https://github.com/0xProject/EIPs/blob/01dfc0f9a4122d8ad8817c503447cab8efa8a6c4/EIPS/eip-signTypedData.md#pseudocode-examples