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

Allow provider init with cache configuration & add tests #3395

Merged
merged 2 commits into from
May 15, 2024

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented May 14, 2024

What was wrong?

Closes #3394

How was it fixed?

  • Allow provider init with request cache configuration
  • Add test to test that all providers init without caching by default and can be instantiated with caching configs

Todo:

  • Clean up commit history
  • Add or update documentation related to these changes
  • Add entry to the release notes

Cute Animal Picture

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

@fselmo fselmo marked this pull request as ready for review May 14, 2024 23:34
@fselmo fselmo requested review from kclowes, pacrob and reedsa May 14, 2024 23:34
@clover-es
Copy link

clover-es commented May 15, 2024

Maybe tests test_caching_requests_does_not_share_state_between_providers and test_async_request_caching_does_not_share_state_between_providers should be adapted as well?

I know previously wasn't tested either (probably that was an error), but I think that if the purpose is to test whether cache is shared, they should be initialized with cache_allowed_requests=True.

Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

nice work, lgtm!

@fselmo
Copy link
Collaborator Author

fselmo commented May 15, 2024

I know previously wasn't tested either (probably that was an error), but I think that if the purpose is to test whether cache is shared, they should be initialized with cache_allowed_requests=True.

That is definitely the case, good catch. Will add that in here.


edit: I also added to those tests the case where a cache is shared, as a sanity check and for quicker code readability.

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.

LGTM! Just left a small comment on the IPCProvider ⛵

web3/providers/ipc.py Show resolved Hide resolved
@fselmo fselmo merged commit 4aacf77 into ethereum:main May 15, 2024
65 of 71 checks passed
@fselmo fselmo deleted the update-request-caching branch May 15, 2024 17:47
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.

Request Caching can't be configured via providers classes constructors
4 participants