Skip to content

Conversation

@reedsa
Copy link
Contributor

@reedsa reedsa commented Aug 30, 2024

What was wrong?

Contract function classes provide functions for extracting contract components. Contract event API methods do not yet exist. The first set of methods for find_events_by_identifier and get_event_by_identifier should be added.

Contract Event APIs should be provided, namely all_events, find_events_by_name and get_event_by_name.

How was it fixed?

Implement functions for Contract classes to find many events or getting a single event by identifier, find_events_by_identifier and get_event_by_identifier.

Additional Event APIs are now available (all_events, find_events_by_name, get_event_by_name, find_events_by_signature, get_event_by_signature, find_events_by_selector, get_event_by_selector, find_events_by_topic, and get_event_by_topic)

Todo:

  • Clean up commit history
  • Add or update documentation related to these changes
  • Add entry to the release notes

Cute Animal Picture

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

@reedsa reedsa force-pushed the contract-event-api branch from 4ae2950 to e1eabd2 Compare August 30, 2024 23:38
@reedsa reedsa force-pushed the contract-event-api branch from e1eabd2 to 711f58a Compare October 2, 2024 18:26
@reedsa reedsa changed the title Event Contract APIs find_events_by_identifier for Contract Events Oct 2, 2024
@reedsa reedsa changed the title find_events_by_identifier for Contract Events Contract Events APIs Oct 2, 2024
@reedsa reedsa force-pushed the contract-event-api branch from 479723f to 40cc3f9 Compare October 4, 2024 21:01
@reedsa reedsa marked this pull request as ready for review October 4, 2024 21:01
@reedsa reedsa force-pushed the contract-event-api branch from 40cc3f9 to 716b756 Compare October 7, 2024 18:56
reedsa pushed a commit to reedsa/web3.py that referenced this pull request Oct 8, 2024
@reedsa reedsa force-pushed the contract-event-api branch from 290094b to d4727ed Compare October 8, 2024 20:18
@reedsa reedsa requested review from fselmo, kclowes and pacrob October 8, 2024 20:23
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

This will be a great addition to get in. Currently as it stands, if I have an ABI that has both a Deposit event and a Deposited event, and I try to use contract.get_event_by_name() with either "Deposit" or "Deposited", I can get neither event. Instead, I get:

Web3ValueError: Could not find any event with matching name

I don't think this is desired behavior. Those are not ambiguous but distinct names. Can we add a test for something like this and make sure it passes? I saw a few nits in message wording, feel free to take or leave those as I see it in the function messages as well. I'll take another pass here once we get that test in.

return fns[0]


def find_events_by_identifier(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should we rename this to get_events_by_identifier since the singular is get_event_by_identifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a convention that follows the find_* used for lookups which return a list of items, and get_* when expecting a single result. The get method takes the resulting list from the find and just returns a single element (if there is not a single result in the list, the get method raises an exception).

"""
if len(events) > 1:
raise Web3ValueError(
f"Found multiple events with matching {identifier}. " f"Found: {events!r}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: use "identifier" in the message for clarity

Suggested change
f"Found multiple events with matching {identifier}. " f"Found: {events!r}"
f"Found multiple events with matching identifier `{identifier}`: {events!r}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think identifier is a little confusing because we use abi_element_identifier to represent the name of the ABIFunction or ABIEvent.

In this context, identifier is a string that represents the type of "check" passed in. For example, get_function_by_signature uses find_functions_by_identifier by passing a "check" function that retrieves all functions from the ABI which passes the check. Then the resulting list of ABI components is passed to get_function_by_identifier to ensure there is only one result in the list and returns the ABI component itself. If not, an exception is raised, and the identifier is "signature", indicating the search type.

f"Found multiple events with matching {identifier}. " f"Found: {events!r}"
)
elif len(events) == 0:
raise Web3ValueError(f"Could not find any event with matching {identifier}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
raise Web3ValueError(f"Could not find any event with matching {identifier}")
raise Web3ValueError(f"Could not find any event with matching identifier `{identifier}`")

"""
if len(fns) > 1:
raise Web3ValueError(
f"Found multiple functions with matching {identifier}. " f"Found: {fns!r}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see now we already phrase it this way with functions... take or leave really. Tiniest of nits.

Suggested change
f"Found multiple functions with matching {identifier}. " f"Found: {fns!r}"
f"Found multiple functions with matching identifier `{identifier}`. Found: {fns!r}"

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.

This is looking good to me once the Deposit/Deposited edge case that @fselmo found is cleaned up. Those doctests are super nice, thanks for getting those in. I just left a few comments!

Returns the transaction hash for the deploy transaction.

.. code-block:: python
.. doctest:: contractmethods
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice work with this!

]

with request_mocker(w3, mock_results={"eth_getLogs": get_logs_response}):
logs = emitter.events.LogNoArguments().get_logs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have any tests to make sure that an event called with parens still passes? I'm fine taking them out here, but it might be good to have an explicit test to make sure it still works since it's documented. And the parens/no parens might be worth removing in v8. It feels antithetical to the zen of python, and I don't think there is a whole lot of value add in being able to call events with both parentheses and without. Although I can see the argument for maintaining consistency with ContractFunctions /shrug

Raises a Web3ValueError if the signature is invalid or if there is no match or
more than one is found.
"""
if " " in signature:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just strip out the spaces instead of raising here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled this logic from get_function_by_signature but it might be better to strip out spaces in the long run. I can strip those in get_event_by_signature for now.

>>> contract.get_event_by_name('Approval')
<Event Approval(address,address,uint256)>

.. py:classmethod:: Contract.find_events_by_selector(selector)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work with the 4byte selector too or just the whole thing?

Copy link
Contributor Author

@reedsa reedsa Oct 16, 2024

Choose a reason for hiding this comment

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

I'm not certain I've come across events that are represented by a 4byte selector, so this is implemented to match the whole event signature encoding. If it makes sense to use the 4byte selector I can update this to match the function selector.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right. This is events. I think we can just leave it as-is. If we get a request to change it later, we can.

Stuart Reed added 4 commits October 18, 2024 09:44
Contract utils for `get_event_by_name`, `get_event_by_signature`, `get_event_by_selector`, `get_event_by_topic`
Contract utils for `find_events_by_name`, `find_events_by_signature`, `find_events_by_selector`, `find_events_by_topic`
@reedsa reedsa force-pushed the contract-event-api branch from 32c0e16 to 465def7 Compare October 18, 2024 15:45
@reedsa
Copy link
Contributor Author

reedsa commented Oct 18, 2024

@fselmo I havent reproduced the error you saw but added tests to cover those cases for both functions and events. Please let me know what the ABI was that you used and maybe there's an edge case I'm missing. Thanks!

@reedsa reedsa requested review from fselmo and kclowes October 18, 2024 16:33
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! Just left a nit or two, and some typing suggestions/clarifications 🚀

address: ChecksumAddress,
callable_check: Callable[..., Any],
) -> List["AsyncContractEvent"]:
return cast(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a redundant cast

return PropertyCheckingFactory(class_name, (cls,), kwargs)
def factory(
cls, class_name: str, **kwargs: Any
) -> Union["ContractEvent", "AsyncContractEvent"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this should be TContractEvent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that using TContractEvent requires one argument for factory to be of that same type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, okay. Thanks for looking into it!

)

@combomethod
def find_events_by_identifier(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:find/get_functions/events_by_id are all beneath the # Private helpers comment. Consider moving above 🤷

address: ChecksumAddress,
callable_check: Callable[..., Any],
) -> List["ContractEvent"]:
return cast(
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant cast

@reedsa reedsa merged commit dc02336 into ethereum:main Oct 21, 2024
71 checks passed
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.

3 participants