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

Parse tuple arguments from event #2211

Merged
merged 4 commits into from
Jan 13, 2022
Merged

Parse tuple arguments from event #2211

merged 4 commits into from
Jan 13, 2022

Conversation

cc7768
Copy link
Contributor

@cc7768 cc7768 commented Nov 19, 2021

What was wrong?

Events that have tuple-valued arguments are currently unable to be parsed.

Related to Issue #1629

How was it fixed?

I used the proposed fix from #1484

I'm struggling a little on incorporating the new test... I think I'm close but haven't quite had it come together yet.

I've done the following:

  • Modified tests/core/contracts/contract_sources/Emitter.sol
    • Added a new struct (TestTuple)
    • Created a new event (LogStructArgs) that uses TestTuple as an argument
    • Created a new function (logStruct) that emits the LogStructArgs event
  • Used this contract to update web3/_utils/module_testing/emitter_contract.py
    • Updated the ABI
    • Tried updating the CONTRACT_EMITTER_CODE and CONTRACT_EMITTER_RUNTIME but I ran into errors because I haven't done this before. What's the easiest way to generate these?
  • Modified web3/tests/core/contracts/conftest.py
    • Added LogStructArgs to the LogFunctions class
    • Added LogStructArgs to the LogTopics class
  • Updated the parameterizations in tests/core/contracts/test_extracting_event_data.py
    • Added parameterization for test_event_data_extraction
    • Added two parameterizations for test_event_rich_log~

Sorry for the lengthy read. I think it's close but I just need a little push to get it across the finish line.


@fselmo, picking up here:
Added a bit more to this PR, thanks @cc7768 for getting this started!

- Added a NestedTestTuple struct inside the TestTuple juuuuuust to be sure
- Added a test for the added event and function that was failing
- Added collapse_if_tuple() in a few more places that were missing, from the test I added, until it passed
- Separated the emitter contract into a version compiled with a newer solidity version (0.8.11) and the original one (compiled with 0.4.21) since the strict bytes check does not seem to work with solidity versions 0.5.0 and above. Created an issue here to track that.

Todo:

The main TODO item left is to add tests -- The lack of tests is why #1484 was never merged.

The only function that is changed is get_event_abi_types_for_decoding from web3/_utils/events.py. This function itself is only called by get_event_data from web3/_utils/events.py. The get_event_data function is only tested in tests/core/contracts/test_extracting_event_data.py so the TODO items, as far as I can tell, should be:

- [x] Ensure old tests continue to work with the update
- [x] Add new test case that incorporates having tuple arguments to an event
- [x] Add entry to the release notes

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@kclowes
Copy link
Collaborator

kclowes commented Nov 19, 2021

Thanks @cc7768! I will take a look!

@lachgil
Copy link

lachgil commented Nov 24, 2021

I have tried implementing the fix and still seeing issues

File "/usr/local/lib/python3.9/dist-packages/brownie/_cli/run.py", line 50, in main
return_value, frame = run(
File "/usr/local/lib/python3.9/dist-packages/brownie/project/scripts.py", line 103, in run
return_value = f_locals[method_name](*args, **kwargs)
File "./marketwatch.py", line 43, in main
event_printer(message_filter)
File "./marketwatch.py", line 32, in event_printer
for message in message_filter.get_new_entries():
File "/usr/local/lib/python3.9/dist-packages/web3/_utils/filters.py", line 161, in get_new_entries
return self._format_log_entries(log_entries)
File "/usr/local/lib/python3.9/dist-packages/web3/_utils/filters.py", line 172, in _format_log_entries
formatted_log_entries = [
File "/usr/local/lib/python3.9/dist-packages/web3/_utils/filters.py", line 172, in
formatted_log_entries = [
File "/usr/local/lib/python3.9/dist-packages/web3/_utils/filters.py", line 220, in is_valid_entry
return bool(self.data_filter_set_function(entry['data']))
File "cytoolz/functoolz.pyx", line 250, in cytoolz.functoolz.curry.call
File "/usr/local/lib/python3.9/dist-packages/web3/_utils/filters.py", line 255, in match_fn
decoded_values = codec.decode_abi(abi_types, HexBytes(data))
File "/usr/local/lib/python3.9/dist-packages/eth_abi/codec.py", line 173, in decode_abi
decoders = [
File "/usr/local/lib/python3.9/dist-packages/eth_abi/codec.py", line 174, in
self._registry.get_decoder(type_str)
File "/usr/local/lib/python3.9/dist-packages/eth_abi/registry.py", line 469, in get_decoder
return self._get_registration(self._decoders, type_str)
File "/usr/local/lib/python3.9/dist-packages/eth_abi/registry.py", line 354, in _get_registration
coder = super()._get_registration(mapping, type_str)
File "/usr/local/lib/python3.9/dist-packages/eth_abi/registry.py", line 336, in _get_registration
value = mapping.find(type_str)
File "/usr/local/lib/python3.9/dist-packages/eth_abi/registry.py", line 89, in find
raise NoEntriesFound("No matching entries for '{}' in {}".format(
NoEntriesFound: No matching entries for 'tuple' in decoder registry

@fselmo fselmo force-pushed the tuple_args branch 3 times, most recently from 437b45c to bf9f60b Compare January 13, 2022 00:35
@fselmo fselmo force-pushed the tuple_args branch 3 times, most recently from c499b3e to a57f532 Compare January 13, 2022 19:05
@fselmo fselmo changed the title WIP: Parse tuple arguments from event Parse tuple arguments from event Jan 13, 2022
- Nested tuples seemed to be broken when parsing event filters. This commit piggy backs off previous work and uses the collapse_if_tuple() method in the _build_argument_filters_from_even_abi() method. From testing, this seemes to have been a missing piece to get these issues resolved and the added test passing.
- Remove testing nested tuple method from tests that require the test id enum. These were added in the previous commit. Instead, add a test specific to testing the new method / event for tuples.
- Add testing for the new nested tuple function + event in the emitter contract.
- closes 1629
- closes 2298
`enable_strict_bytes_type_checking()` appears to be broken with solidity versions `0.5.0` and above. This commit separates a new emitter contract, compiled with solidity `0.8.11`, and the older emitter contract, compiled with solidity `0.4.21`. This way, we can update the tests using the `emitter` pytest modules while the strict bytes test can use the old emitter contract until we can update `enable_strict_bytes_type_checking()` to be compatible with newer solidity versions.
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.

Looks good to me. Thank you for getting this one over the line. I think there is at least one other issue we can close with this one! Left a few comments, but nothing blocking.

web3/_utils/events.py Outdated Show resolved Hide resolved
@fselmo fselmo merged commit c3d4254 into ethereum:master Jan 13, 2022
@cc7768
Copy link
Contributor Author

cc7768 commented Jan 14, 2022

Thank you @fselmo for getting this across the finish line! I hope I didn't leave you more work than if you had started from scratch 😅

I see that you're based in CO -- Let me know if you're around during ETH Denver so I can treat you to a beer or a coffee!

@fselmo
Copy link
Collaborator

fselmo commented Jan 14, 2022

Thank you @fselmo for getting this across the finish line! I hope I didn't leave you more work than if you had started from scratch 😅

Not at all, thanks for getting the ball rolling on this. Yep, the plan is to go to ETH Denver 🤞🏼

@Th0rgal
Copy link

Th0rgal commented Jan 14, 2022

Thanks for merging!

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.

5 participants