Skip to content

Conversation

reedsa
Copy link
Contributor

@reedsa reedsa commented Sep 4, 2024

What was wrong?

Closes #1704, #3482

The Contract class cannot initialize if the contract contains overloaded functions or events. Currently only the function/event name is used to filter ABI events from the contract ABI, so an exception is raised. Currently, events raise a ValueError when _get_event_abi is called on the Contract or ContractEvent class. Functions raise a MismatchedABI error.

Example:
BaseContractEvent._get_event_abi() calls eth-utils get_event_abi() and expects to return the ABI for the event class. The event classes are not created for every overloaded function. When there are contract events with the same name, only one is ever created and used.

How was it fixed?

Initialize the ContractFunctions or ContractEvents class with every ABI found in the contract, storing the abi signature in the class name. Along with the signature, the abi property is set for each class within the __init__ method. This ensures the correct instance is found and used for subsequent functions like get_logs().

get_abi_element now handles both functions and events using the ABI signature to find a unique ABI. The signature is represented by a string that includes it's name and list of input types within parentheses. name(arg1Type,arg2Type,...)

Exceptions are now shared between functions and events for consistency.

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 get-event-abi-ambiguity branch 4 times, most recently from bdc2282 to 4b2adad Compare September 5, 2024 21:07
reedsa added a commit to reedsa/web3.py that referenced this pull request Sep 5, 2024
@reedsa reedsa marked this pull request as ready for review September 5, 2024 22:36
@reedsa reedsa requested review from fselmo, kclowes and pacrob September 5, 2024 22:36
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.

LGTM! Just left a few nits!

@reedsa
Copy link
Contributor Author

reedsa commented Sep 6, 2024

The ambiguity with functions is also an issue unfortunately. Taking a look at how to disambiguate since right now it uses the "first" one.

reedsa added a commit to reedsa/web3.py that referenced this pull request Sep 10, 2024
@reedsa reedsa force-pushed the get-event-abi-ambiguity branch 2 times, most recently from 7b15976 to 4354b1a Compare September 11, 2024 01:03
@reedsa reedsa requested review from kclowes, fselmo and pacrob and removed request for fselmo, pacrob and kclowes September 11, 2024 19:51
@reedsa reedsa changed the title Initialize contracts with ambiguous events Initialize contracts having function or event overrides Sep 11, 2024
__getattr__ now uses the ABI to look up the function class by signature
Additional fixes for ens
Refactoring and docs update for ``get_abi_element`` and ``_build_filters``

Rename ``get_abi_element_identifier`` -> ``get_abi_element_signature``
…ith_name``

Further cleanup and refactoring
Remove docs from events iter
Fix docstring formatting
@reedsa reedsa force-pushed the get-event-abi-ambiguity branch from 1b05ab8 to e1d16e5 Compare September 12, 2024 19:58
@reedsa reedsa force-pushed the get-event-abi-ambiguity branch from 054b8e5 to 233fa23 Compare September 13, 2024 15:12
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

🐑

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.

Sweet! I didn't see anything major, just left a few small comments.

@reedsa reedsa requested review from kclowes and pacrob September 17, 2024 15:24
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

🐑

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.

LGTM! Since this is such a core component of our library, it's probably worth some manual testing to make sure old functionality still works as expected. I did some myself and have fairly high confidence in the tests, but it's probably worth one more run through!

@reedsa reedsa closed this Sep 24, 2024
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.

Events cannot retrieve their ABI if multiple events have the same name
3 participants