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.

Sign up for GitHub

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

Fix topic_list parameter implementation (matching and validation) to align with JSON-RPC spec #118

Merged
merged 4 commits into from
Aug 2, 2018
Merged

Fix topic_list parameter implementation (matching and validation) to align with JSON-RPC spec #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
132a6a0
Allow single + topic arrays to coincide
dylanjw Jul 31, 2018
967965a
Extend validators to allow single topics alongside nested topic arrays
dylanjw Jul 31, 2018
f289312
Fix bug in topic match function
dylanjw Jul 31, 2018
0392b0c
Update topic list validation
dylanjw Jul 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions eth_tester/normalization/inbound.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,21 @@
)

from eth_tester.validation.inbound import (
is_flat_topic_array,
is_valid_topic_array,
)


def normalize_topic(topic):
if topic is None:
return None
else:
return decode_hex(topic)


@to_tuple
def normalize_topic_list(topics):
for topic in topics:
if topic is None:
yield None
else:
yield decode_hex(topic)
yield normalize_topic(topic)


@to_tuple
Expand All @@ -56,11 +60,11 @@ def normalize_filter_params(from_block, to_block, address, topics):

if topics is None:
yield topics
elif is_flat_topic_array(topics):
yield normalize_topic_list(topics)
elif all(is_flat_topic_array(item) for item in topics):
elif is_valid_topic_array(topics):
yield tuple(
normalize_topic_list(item)
if is_list_like(item) else
normalize_topic(item)
for item
in topics
)
Expand Down
20 changes: 13 additions & 7 deletions eth_tester/utils/filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import itertools
from eth_utils import (
to_tuple,
is_bytes,
Expand Down Expand Up @@ -83,12 +84,13 @@ def is_flat_topic_array(value):
return is_tuple(value) and all(is_topic(item) for item in value)


def is_nested_topic_array(value):
return bool(value) and is_tuple(value) and all((is_topic_array(item) for item in value))
def is_valid_with_nested_topic_array(value):
return bool(value) and is_tuple(value) and all(
(is_flat_topic_array(item) if is_tuple(item) else is_topic(item) for item in value))


def is_topic_array(value):
return is_flat_topic_array(value) or is_nested_topic_array(value)
return is_flat_topic_array(value) or is_valid_with_nested_topic_array(value)


def check_single_topic_match(log_topic, filter_topic):
Expand Down Expand Up @@ -133,16 +135,20 @@ def check_if_log_matches_flat_topics(log_topics, filter_topics):
)


def extrapolate_flat_topic_from_topic_list(value):
_value = tuple(item if is_tuple(item) else (item,) for item in value)
return itertools.product(*_value)


def check_if_topics_match(log_topics, filter_topics):
if filter_topics is None:
return True
elif is_flat_topic_array(filter_topics):
return check_if_log_matches_flat_topics(log_topics, filter_topics)
elif is_nested_topic_array(filter_topics):
elif is_valid_with_nested_topic_array(filter_topics):
return any(
check_if_log_matches_flat_topics(log_topics, sub_filter_topics)
for sub_filter_topics
in filter_topics
check_if_log_matches_flat_topics(log_topics, topic_combination)
for topic_combination in extrapolate_flat_topic_from_topic_list(filter_topics)
)
else:
raise ValueError("Unrecognized topics format: {0}".format(filter_topics))
Expand Down
10 changes: 5 additions & 5 deletions eth_tester/validation/inbound.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,12 @@ def validate_account(value):
raise ValidationError("Address does not validate EIP55 checksum")


def is_flat_topic_array(value):
def is_valid_topic_array(value):
if not is_list_like(value):
return False
return all(is_topic(item) for item in value)
return all(
is_valid_topic_array(item) if is_list_like(item) else is_topic(item)
for item in value)


def validate_filter_params(from_block, to_block, address, topics):
Expand Down Expand Up @@ -133,9 +135,7 @@ def validate_filter_params(from_block, to_block, address, topics):
pass
elif not is_list_like(topics):
raise ValidationError(invalid_topics_message)
elif is_flat_topic_array(topics):
return True
elif all(is_flat_topic_array(item) for item in topics):
elif is_valid_topic_array(topics):
return True
else:
raise ValidationError(invalid_topics_message)
Expand Down
158 changes: 96 additions & 62 deletions tests/core/filter-utils/test_filter_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
check_if_log_matches,
is_topic,
is_flat_topic_array,
is_nested_topic_array,
is_valid_with_nested_topic_array,
is_topic_array,
extrapolate_flat_topic_from_topic_list,
)


Expand Down Expand Up @@ -101,7 +102,7 @@ def test_is_topic(value, expected):
(TOPICS_MANY_WITH_NULL, True),
)
)
def test_is_flat_topic_array(value, expected):
def test_is_valid_topic_array_with_flat_topic_arrays(value, expected):
actual = is_flat_topic_array(value)
assert actual is expected

Expand All @@ -110,6 +111,7 @@ def test_is_flat_topic_array(value, expected):
NESTED_TOPICS_B = (TOPICS_EMPTY, TOPICS_SINGLE_NULL)
NESTED_TOPICS_C = (TOPICS_SINGLE_NULL, TOPICS_MANY)
NESTED_TOPICS_D = (TOPICS_MANY_WITH_NULL, TOPICS_MANY, TOPICS_EMPTY)
NESTED_TOPICS_E = (TOPIC_A, TOPICS_MANY, TOPICS_EMPTY)


@pytest.mark.parametrize(
Expand All @@ -130,25 +132,26 @@ def test_is_flat_topic_array(value, expected):
([b'a', None, b'b'], False),
(list(), False),
([None], False),
(TOPIC_A, False),
(TOPICS_EMPTY, False),
(TOPICS_SINGLE_NULL, False),
(TOPICS_MANY, False),
(TOPICS_MANY_WITH_NULL, False),
(([],), False),
(([tuple()],), False),
([tuple()], False),
((tuple(), []), False),
((TOPICS_EMPTY, (b'arst',)), False),
(TOPIC_A, False),
(TOPICS_EMPTY, False),
# good values
(TOPICS_SINGLE_NULL, True),
(TOPICS_MANY, True),
(TOPICS_MANY_WITH_NULL, True),
(NESTED_TOPICS_A, True),
(NESTED_TOPICS_B, True),
(NESTED_TOPICS_C, True),
(NESTED_TOPICS_D, True),
(NESTED_TOPICS_E, True),
)
)
def test_is_nested_topic_array(value, expected):
actual = is_nested_topic_array(value)
def test_is_valid_with_nested_topic_array(value, expected):
actual = is_valid_with_nested_topic_array(value)
assert actual is expected


Expand Down Expand Up @@ -272,12 +275,12 @@ def test_check_if_to_block_match(block_number, _type, to_block, expected):


FILTER_MATCH_ALL = tuple()
FILTER_MATCH_ANY_ONE = (None,)
FILTER_MATCH_ANY_TWO = (None, None)
FILTER_MATCH_ANY_THREE = (None, None, None)
FILTER_MATCH_ONLY_A = (TOPIC_A,)
FILTER_MATCH_ONLY_B = (TOPIC_B,)
FILTER_MATCH_ONLY_C = (TOPIC_C,)
FILTER_MATCH_ONE_OR_MORE = (None,)
FILTER_MATCH_TWO_OR_MORE = (None, None)
FILTER_MATCH_THREE_OR_MORE = (None, None, None)
FILTER_MATCH_A = (TOPIC_A,)
FILTER_MATCH_B = (TOPIC_B,)
FILTER_MATCH_C = (TOPIC_C,)
FILTER_MATCH_A_ANY = (TOPIC_A, None)
FILTER_MATCH_B_ANY = (TOPIC_B, None)
FILTER_MATCH_C_ANY = (TOPIC_C, None)
Expand Down Expand Up @@ -308,38 +311,38 @@ def test_check_if_to_block_match(block_number, _type, to_block, expected):
(TOPICS_B_A_C, FILTER_MATCH_ALL, True),
(TOPICS_B_C_A, FILTER_MATCH_ALL, True),
# length 1 matches
(TOPICS_EMPTY, FILTER_MATCH_ANY_ONE, False),
(TOPICS_ONLY_A, FILTER_MATCH_ANY_ONE, True),
(TOPICS_ONLY_B, FILTER_MATCH_ANY_ONE, True),
(TOPICS_ONLY_C, FILTER_MATCH_ANY_ONE, True),
(TOPICS_EMPTY, FILTER_MATCH_ONLY_A, False),
(TOPICS_EMPTY, FILTER_MATCH_ONLY_B, False),
(TOPICS_EMPTY, FILTER_MATCH_ONLY_C, False),
(TOPICS_ONLY_A, FILTER_MATCH_ONLY_A, True),
(TOPICS_ONLY_B, FILTER_MATCH_ONLY_B, True),
(TOPICS_ONLY_C, FILTER_MATCH_ONLY_C, True),
(TOPICS_ONLY_B, FILTER_MATCH_ONLY_A, False),
(TOPICS_ONLY_C, FILTER_MATCH_ONLY_A, False),
(TOPICS_ONLY_A, FILTER_MATCH_ONLY_B, False),
(TOPICS_ONLY_C, FILTER_MATCH_ONLY_B, False),
(TOPICS_ONLY_A, FILTER_MATCH_ONLY_C, False),
(TOPICS_ONLY_B, FILTER_MATCH_ONLY_C, False),
(TOPICS_A_A, FILTER_MATCH_ONLY_A, True),
(TOPICS_A_B, FILTER_MATCH_ONLY_A, True),
(TOPICS_A_C, FILTER_MATCH_ONLY_A, True),
(TOPICS_A_B_C, FILTER_MATCH_ONLY_A, True),
(TOPICS_A_C_B, FILTER_MATCH_ONLY_A, True),
(TOPICS_B_A, FILTER_MATCH_ONLY_A, False),
(TOPICS_B_C, FILTER_MATCH_ONLY_A, False),
(TOPICS_B_A_C, FILTER_MATCH_ONLY_A, False),
(TOPICS_B_C_A, FILTER_MATCH_ONLY_A, False),
(TOPICS_EMPTY, FILTER_MATCH_ONE_OR_MORE, False),
Copy link
Contributor Author

@dylanjw dylanjw Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This one im not sure on. Intuitively I would think (None,) should match all non-anonymous events excluding anonymous ones, but that is assuming.

Copy link
Contributor Author

@dylanjw dylanjw Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Tests results from ethereum/web3.py#985 show that clients diverge in how they treat None values. go-ethereum interprets None as matching any thing, and parity will match anything even the absence of a value.

I think it is up to us to decide what makes the most sense.

What should happen in these cases?

Topics Match params Parity match Go-ethereum match eth-tester match
[] [None,] Yes No
[TOPICA] [None, None] Yes No

Copy link
Contributor

Choose a reason for hiding this comment

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

The parity version makes more sense to me. You can already get the geth like functionally by removing the None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parity doesn't allow things like matching any one, or any two, like go-ethereum does. go-ethereum essentially treats None as a place specific wildcard. On the other hand None feels like a bad choice for a wildcard symbol, and is more intuitive that it provide no actual matching function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right, I got them switched up. The geth one makes more sense to me, too. 👍

(TOPICS_ONLY_A, FILTER_MATCH_ONE_OR_MORE, True),
(TOPICS_ONLY_B, FILTER_MATCH_ONE_OR_MORE, True),
(TOPICS_ONLY_C, FILTER_MATCH_ONE_OR_MORE, True),
(TOPICS_EMPTY, FILTER_MATCH_A, False),
(TOPICS_EMPTY, FILTER_MATCH_B, False),
(TOPICS_EMPTY, FILTER_MATCH_C, False),
(TOPICS_ONLY_A, FILTER_MATCH_A, True),
(TOPICS_ONLY_B, FILTER_MATCH_B, True),
(TOPICS_ONLY_C, FILTER_MATCH_C, True),
(TOPICS_ONLY_B, FILTER_MATCH_A, False),
(TOPICS_ONLY_C, FILTER_MATCH_A, False),
(TOPICS_ONLY_A, FILTER_MATCH_B, False),
(TOPICS_ONLY_C, FILTER_MATCH_B, False),
(TOPICS_ONLY_A, FILTER_MATCH_C, False),
(TOPICS_ONLY_B, FILTER_MATCH_C, False),
(TOPICS_A_A, FILTER_MATCH_A, True),
(TOPICS_A_B, FILTER_MATCH_A, True),
(TOPICS_A_C, FILTER_MATCH_A, True),
(TOPICS_A_B_C, FILTER_MATCH_A, True),
(TOPICS_A_C_B, FILTER_MATCH_A, True),
(TOPICS_B_A, FILTER_MATCH_A, False),
(TOPICS_B_C, FILTER_MATCH_A, False),
(TOPICS_B_A_C, FILTER_MATCH_A, False),
(TOPICS_B_C_A, FILTER_MATCH_A, False),
# length 2 matches
(TOPICS_EMPTY, FILTER_MATCH_ANY_TWO, False),
(TOPICS_A_A, FILTER_MATCH_ANY_TWO, True),
(TOPICS_A_B, FILTER_MATCH_ANY_TWO, True),
(TOPICS_ONLY_A, FILTER_MATCH_ANY_TWO, False),
(TOPICS_ONLY_B, FILTER_MATCH_ANY_TWO, False),
(TOPICS_ONLY_C, FILTER_MATCH_ANY_TWO, False),
(TOPICS_EMPTY, FILTER_MATCH_TWO_OR_MORE, False),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same assumption here. That a None value matches any topic that exists at that position, but not an absent topic.

(TOPICS_A_A, FILTER_MATCH_TWO_OR_MORE, True),
(TOPICS_A_B, FILTER_MATCH_TWO_OR_MORE, True),
(TOPICS_ONLY_A, FILTER_MATCH_TWO_OR_MORE, False),
(TOPICS_ONLY_B, FILTER_MATCH_TWO_OR_MORE, False),
(TOPICS_ONLY_C, FILTER_MATCH_TWO_OR_MORE, False),
(TOPICS_A_A, FILTER_MATCH_A_B, False),
(TOPICS_A_B, FILTER_MATCH_A_B, True),
(TOPICS_A_B_C, FILTER_MATCH_A_B, True),
Expand Down Expand Up @@ -368,26 +371,29 @@ def test_check_if_to_block_match(block_number, _type, to_block, expected):
(TOPICS_A_B, FILTER_MATCH_ANY_C, False),
(TOPICS_A_B_C, FILTER_MATCH_ANY_C, False),
# length 3 matches
(TOPICS_EMPTY, FILTER_MATCH_ANY_THREE, False),
(TOPICS_A_B_C, FILTER_MATCH_ANY_THREE, True),
(TOPICS_A_C_B, FILTER_MATCH_ANY_THREE, True),
(TOPICS_B_A_C, FILTER_MATCH_ANY_THREE, True),
(TOPICS_B_C_A, FILTER_MATCH_ANY_THREE, True),
(TOPICS_A_A, FILTER_MATCH_ANY_THREE, False),
(TOPICS_EMPTY, FILTER_MATCH_THREE_OR_MORE, False),
(TOPICS_A_B_C, FILTER_MATCH_THREE_OR_MORE, True),
(TOPICS_A_C_B, FILTER_MATCH_THREE_OR_MORE, True),
(TOPICS_B_A_C, FILTER_MATCH_THREE_OR_MORE, True),
(TOPICS_B_C_A, FILTER_MATCH_THREE_OR_MORE, True),
(TOPICS_A_A, FILTER_MATCH_THREE_OR_MORE, False),
(TOPICS_A_B_C, FILTER_MATCH_A_B_C, True),
(TOPICS_A_C_B, FILTER_MATCH_A_B_C, False),
(TOPICS_A_C_B, FILTER_MATCH_A_C_B, True),
(TOPICS_A_B_C, FILTER_MATCH_A_C_B, False),
# nested matches
(TOPICS_EMPTY, (FILTER_MATCH_ONLY_A, FILTER_MATCH_ONLY_B, FILTER_MATCH_ONLY_C), False),
(TOPICS_ONLY_A, (FILTER_MATCH_ONLY_A, FILTER_MATCH_ONLY_B, FILTER_MATCH_ONLY_C), True),
(TOPICS_ONLY_B, (FILTER_MATCH_ONLY_A, FILTER_MATCH_ONLY_B, FILTER_MATCH_ONLY_C), True),
(TOPICS_ONLY_C, (FILTER_MATCH_ONLY_A, FILTER_MATCH_ONLY_B, FILTER_MATCH_ONLY_C), True),
(TOPICS_A_B, (FILTER_MATCH_ONLY_B, FILTER_MATCH_ONLY_C), False),
(TOPICS_A_B, (FILTER_MATCH_ONLY_B, FILTER_MATCH_ONLY_C, FILTER_MATCH_ONLY_A), True),
# positional topic options matches
(TOPICS_EMPTY, (FILTER_MATCH_A, FILTER_MATCH_B, FILTER_MATCH_C), False),
(TOPICS_ONLY_A, (FILTER_MATCH_A, FILTER_MATCH_B, FILTER_MATCH_C), False),
(TOPICS_ONLY_B, (FILTER_MATCH_A, FILTER_MATCH_B, FILTER_MATCH_C), False),
(TOPICS_ONLY_C, (FILTER_MATCH_A, FILTER_MATCH_B, FILTER_MATCH_C), False),
(TOPICS_A_B, (FILTER_MATCH_B, FILTER_MATCH_C), False),
(TOPICS_A_B, (FILTER_MATCH_B, FILTER_MATCH_C, FILTER_MATCH_A), False),
(TOPICS_A_C, (FILTER_MATCH_A_ANY, FILTER_MATCH_ANY_A), True),
(TOPICS_B_A, (FILTER_MATCH_A_ANY, FILTER_MATCH_ANY_A), True),
(TOPICS_B_C, (FILTER_MATCH_A_ANY, FILTER_MATCH_ANY_A), False),
(TOPICS_B_C, (FILTER_MATCH_A_ANY, FILTER_MATCH_ANY_A), True),
(TOPICS_A_B_C, (FILTER_MATCH_A, FILTER_MATCH_A_B_C, FILTER_MATCH_C), True),
(TOPICS_A_B_C, (FILTER_MATCH_A, FILTER_MATCH_A_C_B, FILTER_MATCH_C), True),
(TOPICS_A_B_C, (FILTER_MATCH_A, FILTER_MATCH_A_C_B, TOPIC_C), True),
),
ids=topic_id,
)
Expand Down Expand Up @@ -464,8 +470,8 @@ def _make_filter(from_block=None, to_block=None, topics=None, addresses=None):
# topics
(_make_log(topics=(TOPIC_A,)), _make_filter(topics=FILTER_MATCH_ALL), True),
(_make_log(topics=(TOPIC_A, TOPIC_B)), _make_filter(topics=FILTER_MATCH_ALL), True),
(_make_log(topics=(TOPIC_A,)), _make_filter(topics=FILTER_MATCH_ONLY_A), True),
(_make_log(topics=(TOPIC_B,)), _make_filter(topics=FILTER_MATCH_ONLY_A), False),
(_make_log(topics=(TOPIC_A,)), _make_filter(topics=FILTER_MATCH_A), True),
(_make_log(topics=(TOPIC_B,)), _make_filter(topics=FILTER_MATCH_A), False),
(_make_log(topics=(TOPIC_A, TOPIC_A)), _make_filter(topics=FILTER_MATCH_A_ANY), True),
(_make_log(topics=(TOPIC_A, TOPIC_B)), _make_filter(topics=FILTER_MATCH_A_ANY), True),
(_make_log(topics=(TOPIC_B, TOPIC_A)), _make_filter(topics=FILTER_MATCH_A_ANY), False),
Expand All @@ -491,3 +497,31 @@ def _make_filter(from_block=None, to_block=None, topics=None, addresses=None):
def test_check_if_log_matches(log_entry, filter_params, expected):
actual = check_if_log_matches(log_entry, **filter_params)
assert actual == expected


@pytest.mark.parametrize(
'topic_list_input,expected_flat_topics',
(
(
('A', ('A','B'), 'A'),
(
('A', 'A', 'A'),
('A', 'B', 'A')
),
),
(
('A', ('A','B', 'C'), ('A', 'B')),
(
('A', 'A', 'A'),
('A', 'A', 'B'),
('A', 'B', 'A'),
('A', 'B', 'B'),
('A', 'C', 'A'),
('A', 'C', 'B')
),
)
)
)
def test_extrapolate_flat_topic_from_topic_list(topic_list_input, expected_flat_topics):
assert tuple(extrapolate_flat_topic_from_topic_list(topic_list_input)) == expected_flat_topics

1 change: 1 addition & 0 deletions tests/core/validation/test_inbound_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ def test_filter_id_input_validation(validator, filter_id, is_valid):
(_make_filter_params(topics=[TOPIC_A, TOPIC_B]), True),
(_make_filter_params(topics=[TOPIC_A, None]), True),
(_make_filter_params(topics=[[TOPIC_A], [TOPIC_B]]), True),
(_make_filter_params(topics=[TOPIC_A, [TOPIC_B, TOPIC_A]]), True),
(_make_filter_params(topics=[[TOPIC_A], [TOPIC_B, None]]), True),
(_make_filter_params(topics=[ADDRESS_A]), False),
(_make_filter_params(topics=[ADDRESS_A, TOPIC_B]), False),
Expand Down