Raise MismatchedABI exceptions with detailed error messaging

[reedsa]

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

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

[fselmo]

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

[fselmo]

@reedsa I approved but I think we need to tighten up the messaging a little bit. It looks like we only append problems when there are matching names. Perhaps we should make this code block a bit more like:

if abi_signatures_matching_names:
    error += f"Tried to find a matching ABI element named `{name}`, but encountered the following problems:\n"

    for abi_signature in abi_signatures_matching_names:
        error += _build_abi_input_error(...)

Because if I run something like the following:

$ python -c "from web3.utils import get_abi_element;get_abi_element({}, 'nonexistent')"

I will get "encountered the following problems:" with no "problems" after it, just a blank.

Some minor outstanding things to look into

[fselmo]

@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]

@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]

@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]

@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.

[fselmo]

@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]

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]

@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]

@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. "

[fselmo]

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

[kclowes]

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)

[kclowes]

This looks like it's exactly the same as lines 81-85?

[kclowes]

nit:

Suggested change

	# Contract `abi`` function should not override `web3` attribute
	# Contract `abi`` function should not override `abi` attribute

[kclowes]

It might also be worth adding a few of these tests on the ContractCaller class. I can't remember how the factory is instantiated over there. The test would look something like:

assert contract.caller.abi() is True

[reedsa]

Yikes, this is really not working. I'm trying to figure out where the collisions are occurring. It seems to be intertwined with the ContractFunctions class.

[reedsa]

@kclowes in my debugging, it appears that the abi property gets overridden when there is an abi() function in the contract. I assume this is because the special characters are ignored, causing both properties to be set with the function as the value. It should instead set abi to the ABI, and abi() to the function. It doesnt seem like there is a way to force the value to be the actual ABI. From what I see, setting the function to the abi() property also sets abi.

It is wildly confusing to trace this edge case through the code and perhaps doesnt make sense anyway. It is also an issue with the other class properties like w3/w3() and address/address() cases.

I am thinking it might just be better to disallow these specific function names to be used in a contract altogether. Would that be reasonable, especially since I was trying to do this on top of the other overloaded contract functions?

[kclowes]

Is this just in the ContractCaller class? If so, think it would be fine to disallow functions with the w3/abi/address names via the ContractCaller, as long as it gets documented well. I wonder if we could raise an error or something if someone tries to access the function name? I don't necessarily think that we should block a contract abi with those method names in there, but I could be convinced if you feel strongly. If we do go that path (not allowing w3/abi/address function names on ContractCaller), would you add an issue so that we can track it?

[kclowes]

nit:

Suggested change

	# Contract `abi` function should not override `web3` attribute
	# Contract `abi` function should not override `abi` attribute

[kclowes]

Feel free to take or leave, but we've been moving towards having separate tests instead of having if branches within the test. For example, having a test like test_appropriate_exceptions_based_on_namespaces_with_no_abi or something and moving those tests to a new test function. I can also see keeping them here since it's the same assertion. I'll leave it up to you :)

[kclowes]

Are these being used anywhere now? We may be able to get rid of them entirely For some reason Github won't let me delete. I see they're being used later on. Don't mind me :)

[kclowes]

Might be worth putting here that the first match gets returned. Is it the first match based on the ABI order?