-
Notifications
You must be signed in to change notification settings - Fork 1.8k
bugfix: Fix batching for AutoProvider #3712
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
Conversation
|
|
||
|
|
||
| def load_provider_from_environment() -> BaseProvider: | ||
| def load_provider_from_environment() -> Optional[JSONBaseProvider]: |
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.
This narrows scope by a small margin, but I'd argue it's a bugfix. Thoughts here? Batching requires JSONBaseProvider. All of web3 does at the moment, for that matter.
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.
I think that's fine
|
cc: @antazoey |
- We can't set the provider, we must let `AutoProvider` decide when it makes proxy calls using the "active" provider. - For the reason above, we need to implement a proxy batch request so that it can instead pull the active provider and make the batch request using it. - Make sure current tests fail with the old implementation and pass with the new one. closes ethereum#3711
ae360ca to
4e7bbe7
Compare
kclowes
left a comment
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.
LGTM! Nice test updates!
|
|
||
|
|
||
| def load_provider_from_environment() -> BaseProvider: | ||
| def load_provider_from_environment() -> Optional[JSONBaseProvider]: |
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.
I think that's fine
What was wrong?
Closes #3711
How was it fixed?
We can't set the provider, we must let
AutoProviderdecide when it makes proxy calls using the "active" provider.For the reason above, we need to implement a proxy batch request so that it can instead pull the active provider and make the batch request using it.
Make sure current tests fail with the old implementation and pass with the new one.
Todo:
Cute Animal Picture