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

Add extra verification for getTransactionReceipt (make sure blockHash is not None) #1158

Merged
merged 2 commits into from
Jan 7, 2019

Conversation

nicolashardy
Copy link
Contributor

What was wrong?

wait for transaction receipt does not wait for blockhash to exist

How was it fixed?

added verification that blockHash is not None

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@nicolashardy nicolashardy changed the title Update transactions.py Adding extra verification for getTransactionReceipt (wait blockHash to exist) Dec 12, 2018
@nicolashardy nicolashardy changed the title Adding extra verification for getTransactionReceipt (wait blockHash to exist) Add extra verification for getTransactionReceipt (make sure blockHash is not None) Dec 12, 2018
@kclowes
Copy link
Collaborator

kclowes commented Jan 2, 2019

@nicolashardy if you merge in master the doctest failure should be taken care of! Thanks! Just kidding. I just learned I should do a rebase instead of merging so I'll take care of that soon. Sorry for the confusion! Thanks for the contribution!

@pipermerriam
Copy link
Member

Wondering if you can get us a test in place within our integration test suite for this.

https://github.com/ethereum/web3.py/blob/master/tests/integration/parity/common.py

@nicolashardy
Copy link
Contributor Author

Actually this is already tested : https://github.com/ethereum/web3.py/blob/master/tests/integration/parity/common.py#L85

@pipermerriam pipermerriam merged commit cad3432 into ethereum:master Jan 7, 2019
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.

3 participants