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

Raise MismatchedABI exceptions with detailed error messaging #3491

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

reedsa
Copy link
Contributor

@reedsa reedsa commented Sep 19, 2024

What was wrong?

In get_abi_element, exception handling between functions and events is now shared. The messages reference functions, but should instead say element and provide additional details about any element that matches the name. The provided arguments and their types should be checked with the encodability for each element ABI in order to list each argument error.

Closes #964, #3482

How was it fixed?

Add logic for mismatched ABI errors to invoke a new private method that generates a list of errors for each ABI that matches the provided abi_element_identifier. The messages returned depend on the number of matches and if there are arguments, the number of matching ABIs that have the same number of arguments is also considered.

The messages will include all potential matches (by name) so that users can identify the function or event they intended to reference. The specific messages for each match depend on the arguments provided.

When an argument is not encodable to a certain type, the message appends information about the argument position, the provided value, and the expected type.

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 added a commit to reedsa/web3.py that referenced this pull request Sep 19, 2024
reedsa added a commit to reedsa/web3.py that referenced this pull request Sep 19, 2024
@reedsa reedsa marked this pull request as ready for review September 19, 2024 21:17
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 awesome! I left a few comments on the wording of the messages, but I think this will really help!

"Multiple elements were found matching 1 arguments.\n"
"Provided argument types: (str)\n"
"Provided keyword argument types: {}\n\n"
"Encountered problems with the following elements named `a`.\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It took me a minute to figure out what this error message was telling me. Maybe something along the lines of: Tried to find a matching ABI Element a, but encountered the following problems: 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

/cc our resident words guy ™️ @wolovim

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.

I made an initial pass to contribute to the messaging conversation. I think we can probably add (s) around the descriptors for dynamic singular or multiple values, e.g. 1 elements(s), 2 argument(s), etc...

I'll take a more careful pass at the rest on Monday 👀

web3/utils/abi.py Outdated Show resolved Hide resolved
web3/utils/abi.py Outdated Show resolved Hide resolved
web3/utils/abi.py Outdated Show resolved Hide resolved
web3/utils/abi.py Outdated Show resolved Hide resolved
web3/utils/abi.py Outdated Show resolved Hide resolved
web3/utils/abi.py Outdated Show resolved Hide resolved
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 looks great. I left some comments on error message wording. I think they could be good changes for clarity, including @kclowes comment.

I have some small outstanding questions on possible breaking changes as well. Exceptions that were raised before this PR and new classes seem to be raised now. Let's make sure we're not braking anything there.

tests/core/utilities/test_event_interface.py Show resolved Hide resolved
web3/utils/abi.py Show resolved Hide resolved
web3/contract/contract.py Outdated Show resolved Hide resolved
reedsa added a commit to reedsa/web3.py that referenced this pull request Sep 24, 2024
fselmo
fselmo previously approved these changes Sep 24, 2024
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.

lgtm! I retracted the accept for now... I think we need to solve for some edge cases in messaging, etc. See comments below.

@fselmo fselmo dismissed their stale review September 24, 2024 21:25

Some minor outstanding things to look into

@fselmo
Copy link
Collaborator

fselmo commented Sep 24, 2024

@reedsa I'm also seeing a recursion issue with simplistic ABI structures. If I try to get_abi_element() with an ABI that has a simple function and event with the same name, I enter into _build_abi_input_error() which tries to get_abi_element() with a signature and re-enters _build_abi_input_error(), etc...

reproduce with:

python -c "from web3.utils import get_abi_element;get_abi_element([{'type': 'function', 'name': 'Nonexistent'},{'type': 'event', 'name': 'Nonexistent'}], 'Nonexistent')"

@reedsa
Copy link
Contributor Author

reedsa commented Sep 24, 2024

@fselmo Interesting to see the function/event name overlap. I think the general rule of thumb in solidity is using TitleCase for events, camelCase for functions. Though that is not explicitly enforced by Solidity as far as I know.

I'll have a look at the empty messages and recursive issue, good finds!

@fselmo
Copy link
Collaborator

fselmo commented Sep 24, 2024

@fselmo Interesting to see the function/event name overlap. I think the general rule of thumb in solidity is using TitleCase for events, camelCase for functions. Though that is not explicitly enforced by Solidity as far as I know.

Yeah that was more of a lazy quick example to test. I think the important takeaway is a graceful handling of the recursion / edge case rather than just letting it recurse.

@reedsa
Copy link
Contributor Author

reedsa commented Sep 25, 2024

@fselmo Found a better approach that eliminates recursive calls entirely. Also doing validations up front in get_abi_element by calling validate_abi and makes those type of errors much clearer.

@reedsa reedsa requested a review from fselmo September 25, 2024 21:06
@fselmo
Copy link
Collaborator

fselmo commented Sep 25, 2024

@fselmo Found a better approach that eliminates recursive calls entirely. Also doing validations up front in get_abi_element by calling validate_abi and makes those type of errors much clearer.

I think we can probably still trim this messaging a bit. It seems quite lengthy when I think we can convey the same message more briefly. I'm seeing double errors as well, one for each of the identifying elements. Was that intentional?

$ python -c "from web3.utils import get_abi_element;get_abi_element([{'type': 'event', 'name': 'MyElement'}, {'type': 'function', 'name': 'MyElement'}], 'MyElement')"

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/fselmo/codigo/e/py/web3.py/web3/utils/abi.py", line 611, in get_abi_element
    raise MismatchedABI(error_diagnosis)
web3.exceptions.MismatchedABI: 
ABI Not Found!
Multiple elements were found that accept 0 argument(s).
Provided argument types: ()
Provided keyword argument types: {}

Tried to find a matching ABI element named `MyElement`, but encountered the following problems:
MyElement()
The provided identifier matches multiple elements.
If you meant to call `MyElement()`, please specify the full signature.
MyElement()
The provided identifier matches multiple elements.
If you meant to call `MyElement()`, please specify the full signature.

Doubled part:

MyElement()
The provided identifier matches multiple elements.
If you meant to call `MyElement()`, please specify the full signature.

What do you think? I can consider this a nit on my part if we think this is desirable. I'm not sure how much of an overhead it would be to not repeat the messaging per item found at the end.

@reedsa
Copy link
Contributor Author

reedsa commented Sep 25, 2024

Oh that does look strange for that case, since it lists both ABI elements in the abi that match the name. Maybe the error message should include the type of each function or event to make that clear. Would make it even more verbose so may not be much better. Something like:

MyElement() - Type: `function`
The provided identifier matches multiple elements.
If you meant to call `MyElement()`, please specify the full signature.
MyElement() - Type: `event`
The provided identifier matches multiple elements.
If you meant to call `MyElement()`, please specify the full signature.

@fselmo
Copy link
Collaborator

fselmo commented Sep 25, 2024

@reedsa I think I see the point of the repeat (in this case). I'm also breaking the API on purpose... an ABI should never really define an event or function like that. I think this looks good. Messaging seems pretty clear to me and I'm not sure I mind the verbosity as much. It is at least very descriptive of the issues.

Maybe the error message should include the type of each function or event to make that clear.

I think including the type would be a great clarifying addition 👌🏼

@fselmo
Copy link
Collaborator

fselmo commented Sep 25, 2024

@reedsa it would be ideal if we could avoid repeating the obvious parts. Something more along the lines of:

The provided identifier matches multiple elements. If you meant to call `MyElement`, please specify the full signature. 
    - signature: "MyElement()", type: "event"
    - signature: "MyElement()", type: "function"

Or something along those lines. Specifically removing the unnecessary repeat of "The provided identifier matches multiple elements. If you meant to call MyElement(), please specify the full signature. "

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.

Good stuff. There's a lot of surface area here, maybe one more approval couldn't hurt but lgtm!

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.

Looking good! I didn't see anything major really. Just a few small things I noticed, mostly in the testing department:

  • I didn't comment on all of the instances I saw, but left a comment around splitting out branching if statements in tests into separate tests. Feel free to take or leave.
  • I also left a few nits and see a few warnings in the tests that weren't there before that could be cleaned up 🧹 (UserWarnings in the extracting_event_data test, and some DeprecationWarnings in the test_abi test)

tests/core/contracts/test_contract_class_construction.py Outdated Show resolved Hide resolved
tests/core/contracts/test_contract_class_construction.py Outdated Show resolved Hide resolved
tests/core/contracts/test_contract_class_construction.py Outdated Show resolved Hide resolved
tests/core/contracts/test_contract_class_construction.py Outdated Show resolved Hide resolved
web3/_utils/contracts.py Show resolved Hide resolved
web3/utils/abi.py Outdated Show resolved Hide resolved
@reedsa reedsa requested review from kclowes and fselmo October 28, 2024 18:22
@reedsa
Copy link
Contributor Author

reedsa commented Oct 28, 2024

@kclowes I noticed that UserWarning has occurred for a while now but it's hard to pinpoint exactly what happened. There have been changes in late v6 and into v7 that may be the culprit. However I reverted a couple of those without success. I have created an issue to resolve that warning at some point in the near future.

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! I left one comment about recompiling with solidity 0.8.28 but then I think it's ready when you're ready to pull the trigger!

@@ -1,11 +1,11 @@
"""
Generated by `compile_contracts.py` script.
Compiled with Solidity v0.8.28.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should maybe recompile here with v0.8.28 before merge, assuming that's the latest version

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.

Improve error messages on mismatched argument types
3 participants