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

Support overloaded events when finding event ABI #1705

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pet3ris
Copy link

@Pet3ris Pet3ris commented Aug 11, 2020

What was wrong?

Related to Issue #1704.

How was it fixed?

Make sure event arguments are used to disambiguate events (in addition to names) when overloaded events are present.

Todo:

Cute Animal Picture

image

@Pet3ris Pet3ris force-pushed the feature-multiple-events branch 4 times, most recently from 7a4f372 to 62446fc Compare August 11, 2020 19:53
@wolovim
Copy link
Member

wolovim commented Aug 11, 2020

Thanks for the PR! If we don't have a test that includes overloaded events, we ought to include one. If you're up for taking that on, great, otherwise I can take over.

@Pet3ris
Copy link
Author

Pet3ris commented Aug 11, 2020

@marcgarreau please do - this was a very incremental change without broader context!

@Pet3ris
Copy link
Author

Pet3ris commented Aug 11, 2020

I don't think I fully solved the underlying root cause. One observation:

  • For an event that's generated from factory (part of <web3_contract>.events) <event>.abi can sometimes be None and calling <event>.get_event_abi() directly cannot be used as a work-around as there is now way to pass in the arguments

@wolovim
Copy link
Member

wolovim commented Aug 11, 2020

Yeah, there's a bit more to the story here. In doing some quick tests, looks like the effects can be seen through more APIs. For example, contract_instance.events.OverloadedEvent.getLogs() returns a ValueError: Multiple events found. Will dig in deeper tomorrow.

edit: I guess there's an argument to be made here that that's a desirable response instead of lumping both event's logs together, for example. This events API isn't exactly friendly to overloading.

@pipermerriam
Copy link
Member

A brief scan of our code that handles contract events suggests we have no handling in place for multiple events with the same name.

one example here:

web3.py/web3/contract.py

Lines 231 to 244 in b9d7677

def __init__(self, abi: ABI, web3: 'Web3', address: Optional[ChecksumAddress] = None) -> None:
if abi:
self.abi = abi
self._events = filter_by_type('event', self.abi)
for event in self._events:
setattr(
self,
event['name'],
ContractEvent.factory(
event['name'],
web3=web3,
contract_abi=self.abi,
address=address,
event_name=event['name']))

It will take a bit of clever problem solving to improve these APIs to support this, but it should be largely doable.

  1. If we have two similarly named events we should probably expose a mechanism that would allow direct access by topic if the topic is known.
  2. If the user is creating a filter, and the filter contains any filters against the arguments (indexed or not) we typically be capable of determining which of the duplicate named events the filter is intended for by matching against argument types.
  3. In the case that there are duplicate named events and we cannot determine which event is intended which is definitely going to be an unavoidable case, we need to be clever and construct the filter such that it matches all of the event topics for the various overloaded events.

We already do similar things for functions, but there we have the luxury of always having the arguments and thus only rarely can we not differentiate between functions with duplicate names. But I think with events we have the advantage of not actually ending up in a situation where we cannot do what the user wants, it just might end up containing extra logs that they don't want since we'll over-match on the filter parameters since we'll have to provide multiple topics.

@Pet3ris
Copy link
Author

Pet3ris commented Aug 12, 2020

FWIW, my specific need (that this PR doesn't solve yet) was to get access to an overloaded event's ABI. Seems like neither <event>.abi nor <event>._get_event_abi() works at this stage but I'm not 100% this is an intended part of the web3.py API. As you both indicate, there are lots of other positive implications related to fixing overloaded events.

@wolovim wolovim mentioned this pull request Dec 9, 2020
larryob added a commit to AffineLabs/contracts that referenced this pull request Jun 24, 2022
web3.py doesn't support overloaded events: ethereum/web3.py#1705
larryob added a commit to AffineLabs/contracts that referenced this pull request Jun 24, 2022
web3.py doesn't support overloaded events: ethereum/web3.py#1705
@ptrcarta
Copy link

Is there a suggested workaround? I'm incurring in this when trying to query the ActionPaused events of Compound Comptroller, which is overloaded

@dthinkr
Copy link

dthinkr commented Nov 6, 2023

Here is a further fix of the issue. Change find_matching_event_abi to avoid it from throwing an error. Instead, let it return the first element.

if len(event_abi_candidates) >= 1: return event_abi_candidates[0]
After the fix, I was able to run this with no problem.

events = contract.events.File(*argument_names).getLogs(fromBlock=start_block, toBlock=end_block)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants