Feature/ens asyncify #2501

dbfreem · 2022-06-03T10:37:21Z

Here is AsyncENS. Still needs docs and cleanup

There was A LOT of copy and paste here from ENS since ENS is so dependent on Eth, and Contract there wasn't much code that could be put in BaseENS.

There is a possible breaking change in the way the modules are loaded in ens utils.py. To be consistent between AsyncENS and ENS I only loaded the Eth module when the Web3 instance is created
I would like to create a ENSFactory like I did AsyncENSFactory so that there is consistency. The reason that AsyncENSFactory was created was to support the async creation of the web3 with the stalecheck middleware. If I create an ENSFactory then I would remove the init and fromweb3 from ENS which would be breaking but drive consistency between AsyncENS and ENS

Todo:

Add entry to the release notes
Need to add docs stalecheck and AsyncENS

ens/main.py

fselmo · 2022-06-09T21:19:57Z

hey @dbfreem... I had a chance to review most of this yesterday. I was on a plane so I just kind of made the changes locally and it was mostly cleanup except a few missing awaits so I just pushed it as a separate commit since the work was done already. Open for discussion on anything I moved around though.

My reasoning behind the nit / reorganizing the code style is since these are all new lines of code, it sets it up a bit nicer for cleaner git blame / commit history in the future. Other than that I just silenced some warnings that were already present in the previous code base that may have been copied over and added the two missing awaits. I also chose to cast to Contract and AsyncContract over using # type: ignore.

This is a great addition and a really nice example of how we should end up splitting our sync + async modules in the future so there were some pretty crucial convention choices solidified here 🎉

I think it'd be nice to add the rest of the async tests such as for test_offchain_resolution.pyand test_wildcard_resolution.py... let me know if you're interested in tackling some of those. We are deploying those contracts on the async ENS setup so I think it'd be good to test it.

fselmo · 2022-06-09T21:25:03Z

I'd like to talk a bit about using the factory approach. I think the factory approach is a good way to handle the async init and I think I'm OK with the async setup being a bit different than sync since it was unavoidable really.

I don't mind not adding an ENS factory for sync if it's not necessary... but curious to hear what you guys think @dbfreem, @kclowes, @pacrob. Thoughts?

edit: We spoke in person and it seems fine to just have async be setup differently, so let's roll with that.

dbfreem · 2022-06-09T21:36:30Z

hey @dbfreem... I had a chance to review most of this yesterday. I was on a plane so I just kind of made the changes locally and it was mostly cleanup except a few missing awaits so I just pushed it as a separate commit since the work was done already. Open for discussion on anything I moved around though.

My reasoning behind the nit / reorganizing the code style is since these are all new lines of code, it sets it up a bit nicer for cleaner git blame / commit history in the future. Other than that I just silenced some warnings that were already present in the previous code base that may have been copied over and added the two missing awaits. I also chose to cast to Contract and AsyncContract over using # type: ignore.

This is a great addition and a really nice example of how we should end up splitting our sync + async modules in the future so there were some pretty crucial convention choices solidified here 🎉

I think it'd be nice to add the rest of the async tests such as for test_offchain_resolution.pyand test_wildcard_resolution.py... let me know if you're interested in tackling some of those. We are deploying those contracts on the async ENS setup so I think it'd be good to test it.

I totally welcome the collaboration so your changes are fine. I will take a look at adding those additional test.

dbfreem · 2022-06-11T00:56:45Z

@fselmo I added the test. It was a copy and paste of some of the existing test that were in those wildcard and offchain.

dbfreem · 2022-06-11T02:01:56Z

nevermind, still some work to do around the test.

dbfreem · 2022-06-12T10:39:06Z

ok @fselmo now this is ready. It was a lot of copy and paste of the existing test.

tests/ens/test_get_text.py

pacrob

one spelling nit, otherwise lgtm

ens/async_ens.py

fselmo

Hey @dbfreem, thanks so much for all your work on async. It has really helped get the ball rolling on so much of what was left.

I'm approving this here ~~but I just did an ultimate wrap up, added all the missing tests for async, and squashed your commits into one and both of mine into one with some good commit messages over at #2526.~~

Let me know if you have any remaining input.

The reason for another PR again (as was the case with Async Contract) is mostly due to the merge commits. In the future, if a git rebase upstream/master is done on the branch and the differences are resolved (instead of git merge upstream/master and fixing the differences), this allows for us to squash commits much easier when cleaning up the PR since the commits don't have multiple parents as is sometimes the case. (ultimately wasn't necessary, see edit)

Thanks again 👌

edit: In the end that wasn't necessary since I force-pushed my local cleaned up branch with the cherry-picked commits here which effectively got rid of the merge commits and was rebased against master locally... d'oh! Going to close that one instead and squash here so we can keep the conversation in one place.

PS: I'll squash once we get another approval so we can see the history a bit better.

dbfreem · 2022-06-17T22:48:11Z

I am all good.

* 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 module scope with a module-scoped event_loop fixture and add a module-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).

Reviewing need for AsyncENSFactory

fselmo · 2022-06-29T18:36:11Z

Hey @dbfreem, I played around with this a bit and was able to remove the AsyncENSFactory. I added some tests for the stalecheck middleware too and rebased with some of the formatting PRs that came through recently adding black to some of these files.

I put that into a separate PR (#2547) since that's a concrete divergence from this PR. I'm still going to do a bit of cleanup there but curious on any feedback. Anything we're missing?

dbfreem · 2022-07-01T01:09:42Z

Hey @dbfreem, I played around with this a bit and was able to remove the AsyncENSFactory. I added some tests for the stalecheck middleware too and rebased with some of the formatting PRs that came through recently adding black to some of these files.

I put that into a separate PR (#2547) since that's a concrete divergence from this PR. I'm still going to do a bit of cleanup there but curious on any feedback. Anything we're missing?

That is awesome that you were able to remove the need for AsyncENSFactory. I have been busy lately and haven't had a chance to look over the full PR. Sorry.

fselmo · 2022-07-01T21:38:32Z

closed by #2547

dbfreem marked this pull request as draft June 3, 2022 10:37

dbfreem marked this pull request as ready for review June 3, 2022 10:37

dbfreem marked this pull request as draft June 3, 2022 10:40

dbfreem marked this pull request as ready for review June 5, 2022 13:37

dbfreem marked this pull request as draft June 5, 2022 13:39

dbfreem marked this pull request as ready for review June 6, 2022 09:57

fselmo reviewed Jun 7, 2022

View reviewed changes

ens/main.py Show resolved Hide resolved

kclowes mentioned this pull request Jun 9, 2022

Black linting - ENS #2509

Closed

1 task

pacrob reviewed Jun 17, 2022

View reviewed changes

tests/ens/test_get_text.py Outdated Show resolved Hide resolved

pacrob approved these changes Jun 17, 2022

View reviewed changes

fselmo force-pushed the feature/ens_asyncify branch from 770cc3b to 0337f36 Compare June 17, 2022 21:20

fselmo reviewed Jun 17, 2022

View reviewed changes

ens/async_ens.py Show resolved Hide resolved

fselmo force-pushed the feature/ens_asyncify branch from 0337f36 to 7f6b8fc Compare June 17, 2022 21:32

fselmo pushed a commit to fselmo/web3.py that referenced this pull request Jun 17, 2022

Additional testing for async ENS from PR ethereum#2501

9d3366d

fselmo mentioned this pull request Jun 17, 2022

Cleaned Up Async ENS #2526

Closed

fselmo previously approved these changes Jun 17, 2022

View reviewed changes

fselmo force-pushed the feature/ens_asyncify branch from 7f6b8fc to 4854897 Compare June 17, 2022 22:44

fselmo requested a review from kclowes June 17, 2022 22:44

fselmo force-pushed the feature/ens_asyncify branch from 4854897 to b2e44c8 Compare June 17, 2022 22:50

Main changes for Async ENS support

104405f

* 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

fselmo force-pushed the feature/ens_asyncify branch 3 times, most recently from 94e4850 to 02e0dc3 Compare June 21, 2022 17:16

fselmo added 2 commits June 21, 2022 14:54

Raise NotImplementedError when directly accessing async_w3.ens

2e35c85

fselmo force-pushed the feature/ens_asyncify branch from 02e0dc3 to 2e35c85 Compare June 21, 2022 20:55

fselmo mentioned this pull request Jun 28, 2022

Async ens no factory #2547

Merged

1 task

fselmo closed this Jul 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ens asyncify #2501

Feature/ens asyncify #2501

dbfreem commented Jun 3, 2022 •

edited

Loading

fselmo commented Jun 9, 2022 •

edited

Loading

fselmo commented Jun 9, 2022 •

edited

Loading

dbfreem commented Jun 9, 2022

dbfreem commented Jun 11, 2022

dbfreem commented Jun 11, 2022

dbfreem commented Jun 12, 2022

pacrob left a comment

fselmo left a comment •

edited

Loading

dbfreem commented Jun 17, 2022

fselmo commented Jun 29, 2022

dbfreem commented Jul 1, 2022

fselmo commented Jul 1, 2022

Feature/ens asyncify #2501

Feature/ens asyncify #2501

Conversation

dbfreem commented Jun 3, 2022 • edited Loading

Todo:

fselmo commented Jun 9, 2022 • edited Loading

fselmo commented Jun 9, 2022 • edited Loading

dbfreem commented Jun 9, 2022

dbfreem commented Jun 11, 2022

dbfreem commented Jun 11, 2022

dbfreem commented Jun 12, 2022

pacrob left a comment

Choose a reason for hiding this comment

fselmo left a comment • edited Loading

Choose a reason for hiding this comment

dbfreem commented Jun 17, 2022

fselmo commented Jun 29, 2022

dbfreem commented Jul 1, 2022

fselmo commented Jul 1, 2022

dbfreem commented Jun 3, 2022 •

edited

Loading

fselmo commented Jun 9, 2022 •

edited

Loading

fselmo commented Jun 9, 2022 •

edited

Loading

fselmo left a comment •

edited

Loading