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

Asyncify eth.get_transaction_receipt and eth.wait_for_transaction_receipt #2265

Merged

Conversation

pacrob
Copy link
Contributor

@pacrob pacrob commented Dec 16, 2021

What was wrong?

Added async eth.get_transaction_receipt and eth.wait_for_transaction_receipt

Related to Issue #1413
Closes Issue #2129

How was it fixed?

Todo:

Cute Animal Picture

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

@pacrob pacrob force-pushed the asyncify_eth.wait_for_transaction_receipt branch 3 times, most recently from dcc26d0 to 54abdde Compare December 16, 2021 23:52
@pacrob pacrob force-pushed the asyncify_eth.wait_for_transaction_receipt branch from 54abdde to 4a311ec Compare December 16, 2021 23:57
@pacrob pacrob changed the title [WIP] Asyncify eth.get_transaction_receipt and eth.wait_for_transaction_receipt Asyncify eth.get_transaction_receipt and eth.wait_for_transaction_receipt Dec 17, 2021
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

This looks good to me! Looks like there is a left over comment that can be removed. Did you manually test it to make sure it works like you think it should?

mungers=[default_root_munger]
)

# _get_transaction_receipt_awaitable: Method[Callable[[_Hash32],
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can go :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, thanks for catching

tx_receipt = None
if tx_receipt is not None:
break
_timeout.sleep(poll_latency)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to pop up again after approving this, but I think we want to be able to await this so it's not blocking. I think the most straightforward route is to get rid of the timeout context and use await asyncio.sleep here. If you want to see if it makes a difference at all, you could add it to our benchmarking suite and see what happens :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. _timeout.sleep replaced with await asyncio.sleep

@pacrob pacrob force-pushed the asyncify_eth.wait_for_transaction_receipt branch from be2c88f to 8eebc82 Compare December 17, 2021 22:19
@pacrob pacrob force-pushed the asyncify_eth.wait_for_transaction_receipt branch from 8eebc82 to 5c7863e Compare December 17, 2021 22:22
@pacrob pacrob merged commit c70f7fb into ethereum:master Dec 17, 2021
@pacrob pacrob deleted the asyncify_eth.wait_for_transaction_receipt branch December 17, 2021 22:43
@pacrob pacrob restored the asyncify_eth.wait_for_transaction_receipt branch December 17, 2021 22:47
pacrob pushed a commit to pacrob/web3.py that referenced this pull request Dec 17, 2021
pacrob pushed a commit that referenced this pull request Dec 21, 2021
- cleaned up async wait_for_transaction_receipt()
- added tests for sync wait_for_transaction_receipt()
- updated tests for async wait_for_transaction_receipt()
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