Skip to content

AyncENS/Contract #2370

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

Closed
wants to merge 8 commits into from
Closed

Conversation

dbfreem
Copy link
Contributor

@dbfreem dbfreem commented Mar 2, 2022

What was wrong?

Build an AsnycENS and implement the needed async parts in Contract

Related to Issue #1990

How was it fixed?

Sorry, ahead of time that this is a gigantic PR. As I started trying to AsyncENS I realized there were three main parts and I found it hard to separate them into smaller PRs.

  1. Changes made to AsyncEthereumTestProvider. There were changes that were needed to enable testing of Async methods.
  2. Changes made to Contract. Added several Async methods and classes. This is not a fully implemented AsyncContract yet but it is enough to get AsyncENS working.
  3. Changes made to ENS. Created the AsyncENS and BaseENS classes.

There was a fare amount of copy and paste in this PR. This was due to the fact of duplicate code needed when separating the Async and non Async methods/classes. I was able to separate out some of the logic and the data access in contract.py, so this cut down on "some" of the copy and paste in contract.py. I didn't see a good way to do that in ENS or changes to the AsyncEthereumTestProvider so there was a fare amount of copy and paste of existing code there.

Todo:

  • Add entry to the release notes
  • Finish adding test
  • Fix linting errores

@dbfreem
Copy link
Contributor Author

dbfreem commented Mar 2, 2022

@pacrob or @kclowes this is a work in progress but I did want to go ahead and put this out here for some review. Sorry, for it being such a huge PR but a lot of this stuff was tied together. I still need to work on test, linting, and cleanup.

@pacrob
Copy link
Contributor

pacrob commented Mar 3, 2022

Hey, thanks for getting this rolling! We discussed this morning and would really like to get all of contracts asynced at once, rather than bit by bit. Would you mind breaking the ENS portion out and saving it for later? We can then create a feature branch from what you've done and add methods one by one. Feature branch here:

https://github.com/ethereum/web3.py/tree/asyncify-contract

Open a PR against it with the contract work you've done. I can focus on this for the next bit too we can get it out quickly. I'll let you know what methods I'm working on so we're not duplicating.

Thanks again! LMK if you have questions.

@pmadhikar
Copy link
Contributor

pmadhikar commented Mar 9, 2022

It turned out that I needed to hack together async contracts for my PR here #2323. Is there some central place where I can see what is missing and make contributions without repeated work?

@pacrob
Copy link
Contributor

pacrob commented Mar 9, 2022

We're getting all the asyncified contract stuff together here:

https://github.com/ethereum/web3.py/tree/asyncify-contract

Major structural changes incoming from @dbfreem, then we can see what's needed.

@dbfreem dbfreem deleted the feature/ens_request branch July 9, 2022 10:17
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.

3 participants