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

AsyncHTTPProvider accepts middleware #1999

Merged
merged 7 commits into from
Jun 4, 2021

Conversation

kclowes
Copy link
Collaborator

@kclowes kclowes commented May 19, 2021

What was wrong?

The Async RPC provider didn't accept any middleware.

Related to Issue #1987, #1988, #2000

How was it fixed?

Added the ability for the AsyncHTTPProvider to accept middleware and added an async gas_price_strategy middleware to test it out. Also adds session caching, and now the EthereumTesterProvider inherits from the AsyncBaseProvider

Todo:

Cute Animal Picture

@kclowes kclowes force-pushed the async-gas-price-middleware branch from a47f0f1 to 13da9e8 Compare May 20, 2021 18:37
@kclowes kclowes added this to the Async Web3 API milestone May 21, 2021
@kclowes kclowes changed the title [WIP] Async gas price middleware Async gas price middleware May 21, 2021
@kclowes kclowes requested review from wolovim and pipermerriam May 21, 2021 17:12
@kclowes kclowes changed the title Async gas price middleware AsyncHTTPProvider accepts middleware May 21, 2021
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I wasn't able to glean "why" this middleware is being applied at the provider level rather than the web3 level. Otherwise, this looks structurally good. Still probably worth getting a quick review from @marcgarreau as I didn't use my fine-toothed-comb

web3/eth.py Outdated Show resolved Hide resolved
web3/middleware/__init__.py Show resolved Hide resolved
Copy link
Member

@wolovim wolovim left a comment

Choose a reason for hiding this comment

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

LGTM! A good sanity check could be to plug the provider w/ middleware into the benchmark suite. I'll get that wrapped up and merged now.

@kclowes kclowes force-pushed the async-gas-price-middleware branch from 8674d94 to 7788c2d Compare May 28, 2021 16:47
@kclowes kclowes force-pushed the async-gas-price-middleware branch from 7788c2d to 860142a Compare June 2, 2021 21:42
@kclowes kclowes changed the title AsyncHTTPProvider accepts middleware [WIP] AsyncHTTPProvider accepts middleware Jun 2, 2021
@kclowes kclowes force-pushed the async-gas-price-middleware branch 2 times, most recently from a36c1aa to 21a1553 Compare June 3, 2021 21:57
@kclowes kclowes changed the title [WIP] AsyncHTTPProvider accepts middleware AsyncHTTPProvider accepts middleware Jun 4, 2021
Copy link
Member

@wolovim wolovim left a comment

Choose a reason for hiding this comment

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

thanks for benchmarking! huge difference in sendTransaction; we may need to scale down the number of calls there.

appreciate the import warning too. still looks sane after the updates, and plenty of time to keep feeling out if we've got the API and code location right before we call it 'stable'. :shipit:

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