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

Async ens no factory #2547

Merged
merged 5 commits into from Jul 1, 2022
Merged

Async ens no factory #2547

merged 5 commits into from Jul 1, 2022

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jun 28, 2022

What was wrong?

Builds on #2501 by @dbfreem

This time I felt the need to branch off into a separate PR because it is more than just adding tests and organizing. I went a different route and was able to remove the need for AsyncENSFactory. This does build on the initial main changes for async ENS but we can keep the discussion on this alternate PR separated.

How was it fixed?

Todo:

Cute Animal Picture

moogcato

dbfreem and others added 2 commits June 27, 2022 16:08
* Change ``ens.main`` to ``ens.ens``
* Added AsyncENSFactory to handle async creation of AsyncENS
* Reorganize ENS, AsyncENS, and BaseENS into separate modules
* Added async ENS tests
* Updated docs
* newsfragment
* Add some missing awaits
* Add a lot more testing around AsyncENS. We can think about splitting async ENS tests off into their own CI job if they get too big, but we should provide ample testing around the async implementation to make sure it's all wired up correctly.
* Cleanup for better readability and consistency across ENS and AsyncENS
* Keep async_ens_setup as session scope with a session-scoped event_loop fixture and add a session-scoped async_w3 for ens module

* Fix some minor warnings with optionals where ``None`` is being passed in
* nit: Since we are adding so many new lines, match code style to ideal rest of library for consistency. Trailing commas with new arguments allow for a cleaner git blame / git history since the next bit of code added would only touch the new line that is created, etc. (``black`` formatting PRs are coming in now though so this wasn't very important after the first commit that was squashed here).
ens/utils.py Show resolved Hide resolved
- Create the stalecheck middleware for ENS in a similar way as to the others in order to remove the need to ``await`` it within the ``init_async_web3`` method, and therefore within the ``AsyncENS`` ``__init__`` method. This removes the need for the ``AsyncENSFactory`` altogether since it removes the need for the ``await``.
@fselmo fselmo mentioned this pull request Jun 29, 2022
2 tasks
@fselmo fselmo marked this pull request as ready for review June 29, 2022 18:51
@fselmo fselmo requested review from kclowes and pacrob June 29, 2022 18:52
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

one naming nit, otherwise lg!

fselmo added a commit to fselmo/web3.py that referenced this pull request Jun 29, 2022
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 few nits. Thank you @dbfreem and @fselmo!

tests/core/middleware/test_stalecheck.py Show resolved Hide resolved
tests/ens/test_get_text.py Show resolved Hide resolved
web3/main.py Outdated Show resolved Hide resolved
web3/main.py Outdated Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 1, 2022
fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 1, 2022
web3/main.py Show resolved Hide resolved
@fselmo fselmo merged commit d56cf6e into ethereum:master Jul 1, 2022
@fselmo fselmo deleted the async-ens-no-factory branch April 3, 2024 20:51
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.

None yet

4 participants