-
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
Remove eth-account deprecation warnings #1468
Conversation
8b31cff
to
5689026
Compare
@carver or @pipermerriam I am not sure why |
@kclowes taking a look at the source code for those two functions it doesn't appear they can be swapped 1:1. Looks like the return value from |
Having these fixed would be really nice! Can I help somehow? |
@MatthiasLohr I am at Devcon and probably won't have time to get to this PR this week so feel free to pick it up. I think fixing those last few failing tests are what's left to do! |
5689026
to
4f4fb49
Compare
4f4fb49
to
4c93901
Compare
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.
Have you verified that you've bumped the eth-account
dependency sufficiently high to ensure these new APIs are present?
Good thought! Yep, eth-account is bumped to 0.4.0 which has these new APIs 👍 |
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.
MInor rename suggestion due to the change in the message-signing API.
message_hash = defunct_hash_message(text=message) | ||
from_account = acct.recoverHash(message_hash, vrs=(v, r, s)) | ||
message_hash = encode_defunct(text=message) | ||
from_account = acct.recover_message(message_hash, vrs=(v, r, s)) |
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.
Since encode_defunct
returns a (signable) message now, the variable name should probably be changed from message_hash
. Maybe signable_message
, encoded_message
or encoded
is a reasonable name now.
Another very reasonable option is to rename message
=> message_text
, and message_hash
=> message
. (this might by my favorite)
from_account = acct.recoverHash(msg_hash, signature=signature_bytes) | ||
assert from_account == '0xFeC2079e80465cc8C687fFF9EE6386ca447aFec4' | ||
msg_hash = encode_defunct(b'\xbb\r\x8a\xba\x9f\xf7\xa1<N,s{i\x81\x86r\x83{\xba\x9f\xe2\x1d\xaa\xdd\xb3\xd6\x01\xda\x00\xb7)\xa1') # noqa: E501 | ||
from_account = acct.recover_message(msg_hash, signature=signature_bytes) |
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.
Same here, msg_hash
is an inaccurate variable name now.
from_account = acct.recoverHash(msg_hash, vrs=map(to_hex, (v, r, s))) | ||
assert from_account == '0xFeC2079e80465cc8C687fFF9EE6386ca447aFec4' | ||
msg_hash = encode_defunct(b'\xbb\r\x8a\xba\x9f\xf7\xa1<N,s{i\x81\x86r\x83{\xba\x9f\xe2\x1d\xaa\xdd\xb3\xd6\x01\xda\x00\xb7)\xa1') # noqa: E501 | ||
from_account = acct.recover_message(msg_hash, vrs=(v, r, s)) |
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.
and here
from_account = acct.recoverHash(msg_hash, vrs=(v, r, s)) | ||
assert from_account == '0xFeC2079e80465cc8C687fFF9EE6386ca447aFec4' | ||
msg_hash = encode_defunct(b'\xbb\r\x8a\xba\x9f\xf7\xa1<N,s{i\x81\x86r\x83{\xba\x9f\xe2\x1d\xaa\xdd\xb3\xd6\x01\xda\x00\xb7)\xa1') # noqa: E501 | ||
from_account = acct.recover_message(msg_hash, vrs=(v, r, s)) |
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.
ditto
@@ -230,18 +231,22 @@ def test_eth_account_recover_vrs_standard_v(acct): | |||
ids=['web3js_example', '31byte_r_and_s'], | |||
) | |||
def test_eth_account_sign(acct, message, key, expected_bytes, expected_hash, v, r, s, signature): | |||
message_hash = defunct_hash_message(text=message) | |||
assert message_hash == expected_hash | |||
message_hash = encode_defunct(text=message) |
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.
ditto
What was wrong?
There were lots of deprecation warnings from eth-account when the tests were run.
How was it fixed?
Began using the new eth-account methods that were introduced.
Todo:
Cute Animal Picture