Asyncify filters #2744

pacrob · 2022-12-02T21:35:47Z

What was wrong?

Filters needed asyncing

Related to Issue #1413
Closes #2573

How was it fixed?

Asynced filters

Todo:

Add entry to the release notes

Cute Animal Picture

fselmo

First pass comments... This looks great. Thanks for all that work! We should definitely do another pass at it after rebasing with the snake casing PR, as we discussed.

fselmo · 2022-12-05T21:57:06Z

tests/core/filtering/test_contract_past_event_filtering.py

+
+
+@pytest.fixture(scope="module")
+def event_loop():


This logic is used in a few files in this PR. Can we abstract this out to the tests/utils file as _event_loop_fixture_logic as you did with some of the other fixtures in these PRs?

I had not tried because it's a shorter fixture. I tried to extract the logic and couldn't get it to work. But I found a single-line solution elsewhere in the codebase and plugged than in.

Single-line solution seems to leave event loops unclosed, so I put the original solution back. I got it down to 3 lines though.

fselmo · 2022-12-05T21:57:40Z

tests/core/filtering/test_contract_topic_filters.py

+
+
+@pytest.fixture(scope="module")
+def event_loop():


Use fixture logic here as well.

single line solution found instead of extracting

fselmo · 2022-12-05T22:01:48Z

web3/contract.py

+        if not self.address:
+            raise TypeError(
+                "This method can be only called on "
+                "an instated contract with an address"


Suggested change

"an instated contract with an address"

"an instantiated contract with an address"

Unrelated to these changes but I think this is a typo?

fselmo · 2022-12-05T22:02:43Z

web3/contract.py

+    ) -> AsyncLogFilter:
+        """
+        Create filter object that tracks logs emitted by this contract event.
+        # optional --- update the params descriptions here or remove this line


One more copy / paste blip

fselmo · 2022-12-05T22:02:49Z

web3/contract.py

+    ) -> LogFilter:
+        """
+        Create filter object that tracks logs emitted by this contract event.
+        # optional --- update the params descriptions here or remove this line


I think this was copy / pasted from my comment in the PR that was merged in here 👀

web3/eth.py

fselmo · 2022-12-05T22:12:37Z

web3/middleware/filter.py

+) -> AsyncIterable[Tuple[Optional[BlockNumber], Optional[BlockNumber]]]:
+    """Returns an iterator unloading ranges of available blocks
+
+    starting from `fromBlock` to the latest mined block,


Suggested change

starting from `fromBlock` to the latest mined block,

starting from `from_block` to the latest mined block,

fselmo · 2022-12-05T22:12:48Z

web3/middleware/filter.py

+    """Returns an iterator unloading ranges of available blocks
+
+    starting from `fromBlock` to the latest mined block,
+    until reaching toBlock. e.g.:


Suggested change

until reaching toBlock. e.g.:

until reaching to_block. e.g.:

fselmo · 2022-12-05T22:14:55Z

web3/middleware/filter.py

+
+async def async_get_logs_multipart(
+    w3: "Web3",
+    startBlock: BlockNumber,


We should use start_block and stop_block here, as well as change these in the synchronous get_logs_multipart. This can be in a separate PR if preferred but I think it's a pretty small change that would be simple enough to review if it gets its own commit.

kclowes

Thanks for taking this on! It's looking good to me! I had a few small comments, and had similar thoughts around camelCase/snake_case-ing as @fselmo said. And I assume you've checked this a few times already, but it's definitely worth a manual check here to double check filter and async filters work before merging.

conftest.py

web3/middleware/filter.py

web3/eth.py

conftest.py

kclowes · 2022-12-07T17:21:37Z

@pacrob I just noticed that we have a bunch more warnings in our tests now as well. Looks like they're related to unclosed event loops in the core tests.

pacrob · 2022-12-07T23:02:19Z

@pacrob I just noticed that we have a bunch more warnings in our tests now as well. Looks like they're related to unclosed event loops in the core tests.

I had updated the locations where event_loop needed a different scope to what I thought was a more concise bit of code. But apparently it also didn't close the loop. Reverted, and associated warning are gone.

kclowes · 2022-12-07T23:13:46Z

Sweet! Test failure should be fixed in #2751

* async filters eth module

* update method_formatters.py to select the appropriate sync/async filter type

* asyncify filter middleware and tests

# async contract filter methods * set up async filter testing fixtures * async a few filter tests

* async existing filter test * async test_filter_against_transaction_logs * async test_filter_against_pending_transactions * async test_contract_get_logs * async test_contract_topic_filters * async test_contract_past_event_filtering * async test_contract_on_event_filtering * async test_contract_data_filters

* clean up event_loop fixtures, types, and some snekcasing

pacrob force-pushed the asyncify-filters branch 3 times, most recently from 8c454d6 to eead3d6 Compare December 2, 2022 22:01

pacrob requested review from kclowes and fselmo December 2, 2022 22:13

fselmo reviewed Dec 5, 2022

View reviewed changes

kclowes reviewed Dec 5, 2022

View reviewed changes

conftest.py Show resolved Hide resolved

web3/middleware/filter.py Show resolved Hide resolved

web3/eth.py Outdated Show resolved Hide resolved

conftest.py Show resolved Hide resolved

pacrob mentioned this pull request Dec 6, 2022

clean up asyncify-filters #2749

Merged

pacrob force-pushed the asyncify-filters branch 2 times, most recently from fd5cfa5 to ab52bd3 Compare December 8, 2022 20:37

Paul Robinson and others added 10 commits December 20, 2022 10:42

[WIP] Async filters feature branch (#2588)

f76aa25

* async filters eth module

asynced Filter and EventFilterBuilder classes

cc17153

allow eth.filter to return appropriate sync/async filter types (#2645)

0844ff8

* update method_formatters.py to select the appropriate sync/async filter type

Asyncify filter middleware (#2663)

d7f49b6

* asyncify filter middleware and tests

async contract filter methods & start async filter testing (#2715)

125840a

# async contract filter methods * set up async filter testing fixtures * async a few filter tests

clean up asyncify-filters (#2749)

63c601f

* clean up event_loop fixtures, types, and some snekcasing

replace event_loops + cleanup

d7f1665

AsyncEth filter typing

4edb7dd

type sync filter and ignore typing for async filter tests

a9fac74

pacrob force-pushed the asyncify-filters branch from ab52bd3 to a9fac74 Compare December 19, 2022 21:43

snekcase async methods

949a125

pacrob force-pushed the asyncify-filters branch from d9bfdb7 to 949a125 Compare December 20, 2022 05:09

pacrob merged commit 9f08272 into master Dec 23, 2022

pacrob deleted the asyncify-filters branch December 23, 2022 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asyncify filters #2744

Asyncify filters #2744

pacrob commented Dec 2, 2022 •

edited

Loading

fselmo left a comment

fselmo Dec 5, 2022

pacrob Dec 6, 2022

pacrob Dec 7, 2022

fselmo Dec 5, 2022

pacrob Dec 6, 2022

fselmo Dec 5, 2022

pacrob Dec 6, 2022

fselmo Dec 5, 2022

pacrob Dec 6, 2022

fselmo Dec 5, 2022

pacrob Dec 6, 2022

fselmo Dec 5, 2022

pacrob Dec 6, 2022

fselmo Dec 5, 2022

pacrob Dec 6, 2022

fselmo Dec 5, 2022

pacrob Dec 6, 2022

kclowes left a comment

kclowes commented Dec 7, 2022 •

edited

Loading

pacrob commented Dec 7, 2022

kclowes commented Dec 7, 2022 •

edited

Loading

	"an instated contract with an address"
	"an instantiated contract with an address"

	starting from `fromBlock` to the latest mined block,
	starting from `from_block` to the latest mined block,

Asyncify filters #2744

Asyncify filters #2744

Conversation

pacrob commented Dec 2, 2022 • edited Loading

What was wrong?

How was it fixed?

Todo:

Cute Animal Picture

fselmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kclowes left a comment

Choose a reason for hiding this comment

kclowes commented Dec 7, 2022 • edited Loading

pacrob commented Dec 7, 2022

kclowes commented Dec 7, 2022 • edited Loading

pacrob commented Dec 2, 2022 •

edited

Loading

kclowes commented Dec 7, 2022 •

edited

Loading

kclowes commented Dec 7, 2022 •

edited

Loading