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

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

Merged
merged 5 commits into from Aug 2, 2018

Conversation

Projects
None yet
3 participants
@dylanjw
Contributor

dylanjw commented Jul 30, 2018

What was wrong?

Related to Issue #975

How was it fixed?

Only allow configuring log topic filters via the topics or argument_filters parameter and not both.

Cute Animal Picture

image

@dylanjw dylanjw force-pushed the dylanjw:merging_topic_lists branch from ddf7e6a to c7807f7 Jul 30, 2018

@dylanjw dylanjw force-pushed the dylanjw:merging_topic_lists branch 3 times, most recently from de098c7 to 6c6d8f2 Jul 30, 2018

@dylanjw dylanjw force-pushed the dylanjw:merging_topic_lists branch 2 times, most recently from 28b1a2d to 7ae8fe3 Jul 30, 2018

@dylanjw dylanjw force-pushed the dylanjw:merging_topic_lists branch from 7ae8fe3 to bec2e4e Jul 30, 2018

@dylanjw

This comment has been minimized.

Contributor

dylanjw commented Jul 30, 2018

@pipermerriam This could use another review. I added another bugfix to the topic list construction by removing the permutation of the full topic list by the topic options. Options are passed as lists in place of the bare topic string when multiple topics are to match on one argument. ie:

[ event_sig_topic, [option1_for_arg1, option2_for_arg1], arg2_topic ]
@@ -31,48 +31,46 @@ def hex_and_pad(i):
(
EVENT_1_ABI,
{},
[[EVENT_1_TOPIC]],

This comment has been minimized.

@carver

carver Jul 30, 2018

Collaborator

This API change doesn't have any downstream effects in the public API?

This comment has been minimized.

@dylanjw

dylanjw Jul 30, 2018

Contributor

There shouldn't be a bad effect. Because the topic list permutations were removed, construct_event_topics only needs to return a nested list when there are multiple options for a single argument filter.

There was a bug in how the topic list was being generated. This is an example of a correctly formated topic list to match logs with either of two values for the first indexed event arg:

[ event_sig_topic, [option1_for_arg1, option2_for_arg1], arg2_topic ]

whereas this is what we were generating to be equivalent to the above; we would pass full topic list permutation against each option:

[
    [ event_sig_topic, option1_for_arg1, arg2_topic ],
    [ event_sig_topic, option1_for_arg1, arg2_topic ]
]

This topic topic list actual means: any of [event_sig_topic, option1_for_arg1, arg2_topic ] in the first position and any of [event_sig_topic, option1_for_arg1, arg2_topic] in the second position. Node filters don't understand nested lists as full topic lists. The spec defines a single topic list that can have nested lists of options for each positional element.

This comment has been minimized.

@dylanjw

dylanjw Jul 30, 2018

Contributor

I can break this part out into another PR, and add a failing test.

This issue has been fixed in the new api: #953. Im fixing this function to make it easier to integrate the new filter builder api.

@carver

carver approved these changes Jul 31, 2018

dylanjw added some commits Aug 1, 2018

@dylanjw dylanjw merged commit 899a991 into ethereum:master Aug 2, 2018

28 checks passed

ci/circleci: doctest Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py35-core Your tests passed on CircleCI!
Details
ci/circleci: py35-ens Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-ethtester-pyethereum Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-ethtester-pyevm Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-http-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-http-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ipc-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ipc-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ws-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-goethereum-ws-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-parity-http Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-parity-ipc Your tests passed on CircleCI!
Details
ci/circleci: py35-integration-parity-ws Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-ens Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-ethtester-pyethereum Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-ethtester-pyevm Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-http-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-http-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ipc-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ipc-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ws-1.7.2 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-goethereum-ws-1.8.1 Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-parity-http Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-parity-ipc Your tests passed on CircleCI!
Details
ci/circleci: py36-integration-parity-ws Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment