Skip to content

Asyncify contract #2441

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 91 commits into from
Closed

Asyncify contract #2441

wants to merge 91 commits into from

Conversation

pacrob
Copy link
Contributor

@pacrob pacrob commented Apr 25, 2022

What was wrong?

Contracts getting asynced
Related to Issue #2270

How was it fixed?

All contract functions are asynced. ENS support will be added separately.

Todo:

Cute Animal Picture

image

dbfreem and others added 30 commits March 3, 2022 20:50
Create Base/Async structure for ContractFunctions, ContractEvents, Contract, ContractConstructor, ContractFunction, ContractEvent, ContractCaller classes
* asyncify eth.contract

* Contract build_transaction method
* asyncify contract.transact and contract.estimate_gas
@dbfreem
Copy link
Contributor

dbfreem commented May 13, 2022

Something that the 2,359 line contract.py file got me thinking about was options for splitting it into a few different files. Splitting (if we decide to do so) would definitely be in another PR. This PR is big enough already :)

Off the top of my head, maybe the file tree looks something like:

- web3
    | contracts
        | __init__.py
        | _utils ? 
        | async_contract.py
        | base_contract.py
        | contract.py
    | eth
        | __init__.py
        | async_eth.py
        | base_eth.py
        ...

I totally agree with this approach. It will definitely help in visualizing the flow of the code.
For contract BaseContract, AsyncContract, Contract, Utils makes logical sense to me.

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.

This is looking good to me! I'm not sure how realistic this is, but before we merge, I'd like to try and have a more logical commit sequence. Let me know when you're done @pacrob and @dbfreem and I can look at that and decide if it's worth it or not.

@dbfreem
Copy link
Contributor

dbfreem commented May 13, 2022

I am all done @kclowes

@pacrob
Copy link
Contributor Author

pacrob commented May 13, 2022

I'm done too @kclowes

@fselmo
Copy link
Collaborator

fselmo commented May 17, 2022

I made it through! 🎉 I left some pretty minor comments around DRY-ing things up, but I don't see any major architectural changes. @fselmo - will you also take a look when you get a chance?

Something that the 2,359 line contract.py file got me thinking about was options for splitting it into a few different files. Splitting (if we decide to do so) would definitely be in another PR. This PR is big enough already :)

Off the top of my head, maybe the file tree looks something like:

- web3
    | contracts
        | __init__.py
        | _utils ? 
        | async_contract.py
        | base_contract.py
        | contract.py
    | eth
        | __init__.py
        | async_eth.py
        | base_eth.py
        ...

but I'm open to feedback! And then if we split the contract classes, should we do the same for all Async Modules have their own files? 🤔 food for thought

I would definitely be on board with this 👍

Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

lgtm! :shipit: 🚀

@fselmo fselmo self-requested a review May 20, 2022 18:55
@pacrob pacrob force-pushed the asyncify-contract branch 5 times, most recently from ac4073e to a426a23 Compare May 23, 2022 17:12
@pacrob pacrob force-pushed the asyncify-contract branch 3 times, most recently from 163780a to dae293a Compare May 23, 2022 21:12
@fselmo
Copy link
Collaborator

fselmo commented May 25, 2022

closed by #2484 - created as a cleaned up commit history version of these same changes

@fselmo fselmo closed this in #2484 May 25, 2022
@fselmo fselmo mentioned this pull request Jun 7, 2022
2 tasks
@pacrob pacrob deleted the asyncify-contract branch June 14, 2022 20:41
@dbfreem dbfreem mentioned this pull request Jul 10, 2022
1 task
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.

6 participants