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

Pass bytes instead of string to web3.eth.sign in is_account_locked() #478

Closed
wants to merge 0 commits into from

Conversation

JulianLiu
Copy link

@JulianLiu JulianLiu commented Sep 10, 2018

What was wrong?

to_hex() in eth-utils no longer accept string as a primitive (ref: commit), it will fail if data parameter is passed as str to web3.eth.sign

How was it fixed?

Convert string 'simple-test-data' to bytes before passing to web3.eth.sign

Cute Animal Picture

@@ -12,7 +13,7 @@ def is_account_locked(web3, account):
if isinstance(web3.providers[0], EthereumTesterProvider):
return not any((is_same_address(account, a) for a in web3.eth.accounts[:10]))
try:
web3.eth.sign(account, 'simple-test-data')
web3.eth.sign(account, to_bytes(text='simple-test-data'))
Copy link
Collaborator

@voith voith Oct 7, 2018

Choose a reason for hiding this comment

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

to_bytes is not needed here, use text param in web3.eth.sign instead.
Just do web3.eth.sign(w3.eth.accounts[1], text='simple-test-data')

@voith
Copy link
Collaborator

voith commented Oct 7, 2018

Ideally, would have loved to see a test for this, but eth-tester doesn't have eth-sign implemented at the moment. I have tested this locally, works fine.

Thanks, @JulianLiu for fixing this. I have made some minor adjustments to your PR. I will merge this as soon as master turns green.

@voith
Copy link
Collaborator

voith commented Oct 7, 2018

All python3.5 tests are failing, could be related to ethereum/web3.py#921. Will look into this tomorrow.

@voith
Copy link
Collaborator

voith commented Oct 7, 2018

Failing test taken care in #480

@voith
Copy link
Collaborator

voith commented Oct 8, 2018

@JulianLiu I accidentally closed your PR, while trying to rebase changes I made to your PR. I have opened a new PR #481 with your commit cherry-picked.
In case, you're installing populus from your own fork, then please cherry 68a5390. I accidentally deleted your commit while pushing to your fork and the PR closed automatically. I can't update your branch as the PR is closed, My humble apologies for the trouble.

@JulianLiu
Copy link
Author

@voith No worries. Thank you for the update :)

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.

2 participants