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 web3 #2819

Merged
merged 5 commits into from
Feb 22, 2023
Merged

Async web3 #2819

merged 5 commits into from
Feb 22, 2023

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Feb 16, 2023

What was wrong?

Related to Issue #1416

  • Separates AsyncWeb3 from Web3 in an effort to make async a first class citizen of the library. Ideally we refactor the structure of the files so they are organized as such for developers. I feel like there are enough changes here where re-organization should be a separate PR, which will be quite large as well.
  • Create AsyncMiddleware type that expects AsyncWeb3 rather than Web3. Account for this change down the tree through async middlewares and middleware onion.
  • Remove all cases of type: ignore that were fixed by the tighter typing for async web3 and related modules.

Things to note:

  • AsyncContractCaller inherits from BaseContractCaller which used the synchronous method parse_block_identifier in its __init__() method. This leads me to think we weren't testing all cases for initialization of the contract caller. This came up as an issue with typing and was addressed using the simpler parse_block_identifier_no_extra_call() method which technically shouldn't break anything since the previous structure would return a coroutine and also not work. This may need to be addressed in the future if we want to try to account for all elif cases that ContractCaller accounts for.

How was it fixed?

Todo:

Cute Animal Picture

20230207_091433

@fselmo fselmo force-pushed the async-web3 branch 3 times, most recently from 4ae59ac to d44bb72 Compare February 16, 2023 19:57
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.

Hooray! 🚀 I left some comments around adding a new TypeVar instead of Union["ContractFunction", "AsyncContractFunction"], that I think can help avoid some of the casting we're doing in the contract/async_contract files. It was sort of hard to figure out where to put the comments, so lmk if they don't make sense and we can hop on a call!

ens/ens.py Show resolved Hide resolved
web3/_utils/contracts.py Outdated Show resolved Hide resolved
web3/_utils/contracts.py Outdated Show resolved Hide resolved
web3/contract/async_contract.py Show resolved Hide resolved
web3/contract/utils.py Show resolved Hide resolved
web3/contract/utils.py Outdated Show resolved Hide resolved
web3/contract/utils.py Outdated Show resolved Hide resolved
)

# TODO: The no_extra_call method gets around the fact that we can't call
# the full async method from within a class's __init__ method. We need
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it's the right use case, but I think @pacrob used __aenter__ (or maybe another magic method?) for this one time. Do you have any insight @pacrob?

Copy link
Contributor

Choose a reason for hiding this comment

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

The __await__ magic method allows for things to be awaited at creation. There are a few examples of it in the codebase.

web3/eth/async_eth.py Outdated Show resolved Hide resolved
web3/main.py Outdated Show resolved Hide resolved
@kclowes kclowes mentioned this pull request Feb 16, 2023
1 task
@fselmo fselmo marked this pull request as ready for review February 16, 2023 21:52
@kclowes kclowes mentioned this pull request Feb 16, 2023
1 task
fselmo added a commit to fselmo/web3.py that referenced this pull request Feb 17, 2023
Copy link
Collaborator Author

@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.

@kclowes I addressed your comments in the latest commit. It looks like some typing broke for filter_builder.args when I added the TypeVar suggestion. I ignored those for now. It's not immediately obvious to me where that broke. [nevermind, fixed 🎉]

web3/main.py Outdated Show resolved Hide resolved
web3/_utils/contracts.py Outdated Show resolved Hide resolved
ens/ens.py Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request Feb 17, 2023
from web3.eth import AsyncEth # noqa: F401
from web3.main import ( # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it's from web3.main import, but we also use from web3 import all over the place and I can't find a pattern. Can we just use one or the other?

Copy link
Collaborator Author

@fselmo fselmo Feb 22, 2023

Choose a reason for hiding this comment

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

Yeah. I guess from web3.main might be "best" since the web3/__init__.py imports it from web3.main so it's a bit of a hand-off. I don't think it matters too much how it's imported internally. There may be some things that are defined in web3.main that we don't expose via web3/__init__.py at some point too. For example, I'm not sure that it makes much sense to expose BaseWeb3 there since that's not a complete class that is ready for use.

web3/geth.py Outdated Show resolved Hide resolved
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.

I left a couple comments, but overall lookin' good! I love all those # type: ignores going away.

@fselmo fselmo force-pushed the async-web3 branch 2 times, most recently from 9562800 to 7bb1440 Compare February 22, 2023 17:47
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 had a tiny nit.

web3/contract/utils.py Outdated Show resolved Hide resolved
- Create ``AsyncWeb3`` since ambiguity when instantiating modules with ``Union`` types leads to typing issues. Python's static typing forces us to make the async library a first class citizen.
- Refactor appropriately down the tree, accounting for the newly expected ``AsyncWeb3`` class in things like middlewares and modules, including ``ens``.
- Be more explicit within modules which type of ``w3`` is expected, ``Web3`` or ``AsyncWeb3``, as in the ``EthPM`` module, for example.
- Resolve some ``type: ignore`` cases along the way as some of the typing gets tightened.
- ``BaseContractCaller`` used the sync version of ``parse_block_identifier`` which would render any calls inside it useless for ``AsyncContractCaller`` which inherits from it. I'm assuming we don't test all cases here. This commit splits the bulk of that logic out to the sync and async __init__ methods for contract caller and calls a more appropriate method for parsing the block identifier that doesn't involve an extra call. This reduces some of the intended functionality, but that wasn't working to begin with.

This will need to be revised at some point to see if we can account for all desired cases within the async contract caller __init__.
fselmo added a commit to fselmo/web3.py that referenced this pull request Feb 22, 2023
fselmo added a commit to fselmo/web3.py that referenced this pull request Feb 22, 2023
- Improve typing by using `TypeVar` over `Union` in certain places to prevent casting
- Update links for geth namespaced api docs
@fselmo fselmo merged commit 2a88fa8 into ethereum:master Feb 22, 2023
@fselmo fselmo deleted the async-web3 branch April 25, 2023 16:48
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