Introduce a better filter argument api with EventFilterBuilder #953

dylanjw · 2018-07-10T23:01:36Z

What was wrong?

Related to Issue #872

The existing filter apis are unable to properly differentiate array arguments and multiple match arguments.

How was it fixed?

This PR introduces a new "filter builder" api, where parameters can be set prior to deploying the filter. example:

filter_builder = myContract.events.myEvent.build_filter()
filter_builder.args['array_of_states'].match_single(("CA", "WA", "TX",))
filter_builder.args['array_of_states'].match_any(("CA", "WA", "TX",), ("MN", "OH",))
filter_builder.fromBlock = 0
filter_bulder.toBlock = "latest"
my_filter = filter_builder.deploy()

Additionally this PR contains a bug fix to the data argument filters, by replacing the regex mechanism with an abi decoding and direct value comparison. EDIT: Replacing the data_filter_set_function created the need to update the contract.py filter api's, as they are required to pass in the data filter set with the abi type.

Cute Animal Picture

dylanjw · 2018-07-16T23:17:29Z

web3/utils/normalizers.py

-@implicitly_identity
-def decode_abi_strings(abi_type, data):
-    if abi_type == 'string':
-        return abi_type, codecs.decode(data, 'utf8', 'backslashreplace')


Im getting various DecodingErrors when using the updated eth_abi package that has the change to return strings as string types.

Likely the string data in these tests are not encodable.

Hm, well feel free to copy in a trace if you want another pair of eyes on it.

BTW, there might be other things besides contract arguments/return values that depend on this decode_abi_strings() method, so it's not clear it should be deleted.

@carver I moved this to another PR since it needs it own discussion: #974

dylanjw · 2018-07-26T17:02:55Z

web3/contract.py

+        if topics is not None:
+            raise NotImplementedError(
+                "Directly setting topics is not currently supported. "
+                "Use the argument_filters parameter instead.")


I added this not implemented error to simplify things, as a temporary measure. The ability to set topics directly should be added back to createFilter

I was hoping to merge the manually entered and the generated topic arguments together, but I realize that that would yield filter results that are entirely different than adding the results from the two sets of filtered data. e.g. merging (A, B) with (C, D) to get a topic list like ((A, C), (B, D)). The merged topic list would match AB, AD, CB, CD instead of just AC, BD. For this reason, manually entering topics needs to either be disabled, or throw an exception if topics have already been generated.

dylanjw

Extend tests to include:

filtering dynamic types: bytes, string, un-fixed arrays

web3/contract.py

+        if topics is not None:
+            raise NotImplementedError(
+                "Directly setting topics is not currently supported. "
+                "Use the argument_filters parameter instead.")


pipermerriam · 2018-08-27T17:01:52Z

I don't see a problem with raising exceptions in cases where previously failures were silent.

carver · 2018-08-27T17:54:01Z

I'll summarize with a table

Thanks, that helped a lot! Getting support back for 5 is great 👍

what do you think about leaving the exceptions for the 2 cases that had been failing, but could be fixed.

Totally, if the code was already broken (or failing silently), then raising exceptions is a good option. 👍

carver

GTG when CI is green

dylanjw · 2018-08-28T17:33:04Z

Hypothesis has found a failing example, when the event array argument has 38 or more elements. Ive set the max number of elements to 35. This is probably a eth-tester issue.

dylanjw · 2018-08-30T20:44:31Z

@pipermerriam @carver I made some changes to limit value reset, and limit value setting after a filter has been deployed from a filter builder instance. I did the following to share the deployment state between the EventFilterBuilder object and the child argument objects: cf3f795 Is this an anti-pattern? It feels like an anti-pattern.

carver · 2018-08-31T18:30:51Z

web3/utils/events.py

+        self.formatter = formatter
+        self.event_topic = initialize_event_topics(self.event_abi)
+        self.args = AttributeDict(
+            _build_argument_filters_from_event_abi(event_abi, self._deployed_flag))


Re:

Is this an anti-pattern? It feels like an anti-pattern.

Yeah, I think so too. Maybe instead of this shared flag, replace the args attribute in deploy(). Something like (pseudocode):

- self._deployed_flag(True) + old_args = self.args + self.args = ImmutableArgs(old_args) + old_args.clear() # make it clear that people should not use an old saved copy of args

- Replace the data filter match function with ABI decoded value compare function - Include 'string' type utf-8 conversion in match function. - Disallow array types and match options in old filter API, as these remain broken. The array type arg filtering and multiple option arg matching is provided by EventFilterBuilder api.

dylanjw · 2018-09-19T23:41:29Z

@pipermerriam @carver Do you want to give this a final review? Im just about ready to merge.

carver

Would be nice to squash all the fixup commits.

carver · 2018-09-20T00:21:54Z

tests/core/filtering/test_contract_data_filters.py

@@ -45,52 +50,41 @@ def fixed_values(draw):

 @st.composite
 def array_values(draw):
-    matching = draw(st.lists(elements=st.binary(min_size=2, max_size=2)))
+    matching = draw(st.lists(elements=st.binary(min_size=2, max_size=2), max_size=35, min_size=1))


Hypothesis has found a failing example, when the event array argument has 38 or more elements. Ive set the max number of elements to 35. This is probably a eth-tester issue.

Did you find out any more about this? Does a filter with 38 elements work on, say, geth --dev?

I hate to have this constraint on the test, until we know what's going on.

This ended up being an issue with insufficient gas. I was thrown off in the beginning because I was getting an eth.exceptions.ValidationError: Insufficient gas error with sufficiently large arrays, but there is a range of lower array lengths that fail to emit the event without raising the ValidationError.

tests/core/filtering/test_contract_data_filters.py

Make function name more accurate

Replace hardcoded topic with deriving function Speed up tests Fix breaking change: filter argument options static types Set upper limit on array event arg tests Parametrize match_fn test Block value changes post-deployment Fix broken _utils imports Update filter data normalizer for eth-abi v2 Remove eventFilter pytest parameter Estimate gas for array argument tests

Also fix Web3 import

carver

GTG on green tests

dylanjw force-pushed the filter_params branch 2 times, most recently from 76dd3d3 to 2b0a16a Compare July 13, 2018 00:51

dylanjw mentioned this pull request Jul 16, 2018

decode_abi with a string returns a bytes value ethereum/eth-abi#81

Closed

dylanjw force-pushed the filter_params branch 3 times, most recently from e2cc598 to 2685ecc Compare July 16, 2018 22:42

dylanjw commented Jul 16, 2018

View reviewed changes

dylanjw force-pushed the filter_params branch 4 times, most recently from 0b04593 to 041e2ba Compare July 25, 2018 00:01

dylanjw commented Jul 26, 2018

View reviewed changes

web3/contract.py Outdated

if topics is not None:

raise NotImplementedError(

"Directly setting topics is not currently supported. "

"Use the argument_filters parameter instead.")

This comment was marked as resolved.

Sign in to view

dylanjw force-pushed the filter_params branch 3 times, most recently from ebcb41a to f00f03f Compare July 30, 2018 19:27

dylanjw mentioned this pull request Jul 30, 2018

Disallow configuring filters with both manual and generated topic lists #976

Merged

dylanjw force-pushed the filter_params branch 6 times, most recently from db30ac4 to 57daae3 Compare August 7, 2018 18:39

dylanjw changed the title ~~[WIP] Log filter builder~~ Introduce a better filter argument api with EventFilterBuilder Aug 7, 2018

dylanjw mentioned this pull request Aug 9, 2018

Extend filter tests with dynamic types and longer block ranges #997

Merged

13 tasks

dylanjw force-pushed the filter_params branch 5 times, most recently from eff2856 to 0b3bb9f Compare August 22, 2018 21:11

dylanjw force-pushed the filter_params branch 2 times, most recently from 7246875 to 18c69b4 Compare August 27, 2018 04:28

carver approved these changes Aug 27, 2018

View reviewed changes

dylanjw force-pushed the filter_params branch from 7fdea2c to cf3f795 Compare August 30, 2018 20:41

carver reviewed Aug 31, 2018

View reviewed changes

dylanjw force-pushed the filter_params branch 8 times, most recently from 06d5644 to 63c3cdc Compare September 19, 2018 20:19

dylanjw added 2 commits September 19, 2018 16:03

Add EventFilterBuilder the filter parameter manager

bb9f4cf

dylanjw force-pushed the filter_params branch 2 times, most recently from e7c5479 to a76402b Compare September 19, 2018 23:37

carver reviewed Sep 20, 2018

View reviewed changes

Remove XFAIL flags from dynamic arg filter tests

de15d50

Make function name more accurate

dylanjw force-pushed the filter_params branch from 4ccc612 to 801780e Compare September 20, 2018 21:53

dylanjw added 2 commits September 20, 2018 15:10

Add doc for build_filter method

c4a373d

Also fix Web3 import

dylanjw force-pushed the filter_params branch from 801780e to c4a373d Compare September 20, 2018 22:10

carver approved these changes Sep 20, 2018

View reviewed changes

dylanjw merged commit cc2b67c into ethereum:master Sep 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a better filter argument api with EventFilterBuilder #953

Introduce a better filter argument api with EventFilterBuilder #953

dylanjw commented Jul 10, 2018 •

edited

Loading

dylanjw Jul 16, 2018

carver Jul 17, 2018

dylanjw Jul 30, 2018

dylanjw Jul 26, 2018

dylanjw Jul 26, 2018

dylanjw left a comment

This comment was marked as resolved.

pipermerriam commented Aug 27, 2018

carver commented Aug 27, 2018

carver left a comment

dylanjw commented Aug 28, 2018 •

edited

Loading

dylanjw commented Aug 30, 2018 •

edited

Loading

carver Aug 31, 2018

dylanjw commented Sep 19, 2018

carver left a comment

carver Sep 20, 2018

dylanjw Sep 20, 2018 •

edited

Loading

carver left a comment

Introduce a better filter argument api with EventFilterBuilder #953

Introduce a better filter argument api with EventFilterBuilder #953

Conversation

dylanjw commented Jul 10, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

dylanjw Jul 16, 2018

Choose a reason for hiding this comment

carver Jul 17, 2018

Choose a reason for hiding this comment

dylanjw Jul 30, 2018

Choose a reason for hiding this comment

dylanjw Jul 26, 2018

Choose a reason for hiding this comment

dylanjw Jul 26, 2018

Choose a reason for hiding this comment

dylanjw left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

pipermerriam commented Aug 27, 2018

carver commented Aug 27, 2018

carver left a comment

Choose a reason for hiding this comment

dylanjw commented Aug 28, 2018 • edited Loading

dylanjw commented Aug 30, 2018 • edited Loading

carver Aug 31, 2018

Choose a reason for hiding this comment

dylanjw commented Sep 19, 2018

carver left a comment

Choose a reason for hiding this comment

carver Sep 20, 2018

Choose a reason for hiding this comment

dylanjw Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

carver left a comment

Choose a reason for hiding this comment

dylanjw commented Jul 10, 2018 •

edited

Loading

dylanjw commented Aug 28, 2018 •

edited

Loading

dylanjw commented Aug 30, 2018 •

edited

Loading

dylanjw Sep 20, 2018 •

edited

Loading