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).
@fselmo fselmo force-pushed the async-ens-no-factory branch from 5d5cce9 to 0510960 Compare June 28, 2022 23:39
- 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 force-pushed the async-ens-no-factory branch from 0510960 to 9abde38 Compare June 29, 2022 00:11
@fselmo fselmo force-pushed the async-ens-no-factory branch from 9abde38 to c5f2b8c Compare June 29, 2022 00:21
@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!

fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 1, 2022
@fselmo fselmo force-pushed the async-ens-no-factory branch from 1853211 to 121688b Compare July 1, 2022 02:43
fselmo added a commit to fselmo/web3.py that referenced this pull request Jul 1, 2022
@fselmo fselmo force-pushed the async-ens-no-factory branch from 121688b to 7321d72 Compare July 1, 2022 03:02
@fselmo fselmo force-pushed the async-ens-no-factory branch from 7321d72 to 5a6d2e2 Compare July 1, 2022 21:21
@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.

4 participants