-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
New eth.sign and recovery hash generator #301
Conversation
You get a `bytes` object out, which will often be invalid ascii Kept toAscii for backwards compatibility
Also, bugfix odd-length hex that is not 0x prefixed
After removing old functionality: sha3 = compose(encode_hex, keccak, to_bytes)
Want to avoid confusion from sending in hex without 0x prefix, which could silently return a different result than you expect.
Plus: * Refactor argument test for None out to web3.utils.validation * Improved additional tests, and associated fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I'm thinking that the to_text
and to_bytes
functions should eventually be ported to ethereum-utils
but that's going to have to be part of a broader change deprecating the force_text
and force_bytes
utils.
"topics": [web3.fromAscii("test")], | ||
"payload": web3.fromAscii(payloads[len(payloads) - 1]), | ||
"topics": [topic], | ||
"payload": web3.toHex(text=payloads[-1]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't need to be cleaned up here, but it might be wise for us to remove the use of web3.toHex
here in favor of using the eth_utils.encode_hex
function so as to not make our tests dependent on this API.
What was wrong?
eth.sign
was ambiguous about the kind of data it received. It was also difficult to verify the message; users were completely on their own.How was it fixed?
eth.sign
now follows the pattern of the other converters, by passing one of these:(bytesdata, hexstr='0x..', text='my msg')
There is also a new hash generator
eth._recoveryMessageHash
which adds the preamble and generates the hash thatecrecover
takes as a first argument. The preamble is the one defined by geth, a loose standard until EIP 191 is solidified.It looks like we won't get
ecrecover
functionality until we upgrade pyethereum.Also, I'll add docs when I get a 🚢
Cute Animal Picture