Parse revert reason

[karlb]

What was wrong?

Solidity allows messages in require statements which can help a lot
when debugging. This commit parses the revert reasons from failing
calls and raises them as SolidityError exceptions.

Closes #941.

Todo:

• Add entry to the release notes
• Add unit test
• Add integration test @kclowes
• Fix failures from integration test @marcgarreau
• Bump eth-utils (to account for the replace_exceptions change)

Cute Animal Picture

[karlb]

Is this CI failure really caused by this PR? I don't see how the change could cause that.

[kclowes]

@karlb no, that failure is flaky. Issue is here. Feel free to ignore!

[kclowes]

Thank you for this change @karlb! I added a long comment about where I think the logic should reside. I think it would be good to add an integration test if you feel up to it, but I'm happy to add one if you don't. Let me know when this is ready for review!

[karlb]

I think it would be good to add an integration test if you feel up to it, but I'm happy to add one if you don't.

@kclowes I'll gladly take you up on the offer of adding an integration test. I don't see how to fit it in nicely, right now.

I'll take care of the other change tomorrow.

[kclowes]

Sounds good, thank you!

[karlb]

I've moved the code to method_formatters.py and added a basic test for get_error_formatters. Did I get the change done the way you intended?

I'll probably have to fix something when there's a proper integration test, since I didn't test across different eth clients, yet.

[kclowes]

@karlb I will keep digging into this tomorrow. I realized I read the original issue wrong and misdirected you towards putting the formatting in method_formatters. Based on the original issue, the formatting is correct when it goes througheth_call, but not when a function is called via contract.functions.<function_name>.call(). So I think we'll need to do something around here to catch the Revert and re-raise a SolidityError, but I'm not 100% sure that is the right path. This comment provides some direction as well. I will keep thinking about it and if you have any thoughts in the meantime, let me know.

I also don't think we need an integration test since eth_call is working as expected. I was able to write a quick contract with a revert function that you can use in the core tests to test the contract call interface. The commit is here, but eth-tester doesn't return the error message (it only returns the error type), so that will need to be addressed as well I think.

[karlb]

Based on the original issue, the formatting is correct when it goes through eth_call

It's not accidentally truncated. But I think web3py should detect reverts, parse the revert reason and raise that information as a proper Exception in that case, too. Getting

SolidityError('not allowed to monitor'')

is much more useful than

'Reverted 0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000166e6f7420616c6c6f77656420746f206d6f6e69746f7200000000000000000000'

The commit is here, but eth-tester doesn't return the error message (it only returns the error type), so that will need to be addressed as well I think.

Yes, eth-tester is special. IIRC, it does not return errors as the other eth clients do, but directly raises python exceptions on transaction failures. This is usually very helpful, but for this specific PR it is causing additional work, since it is just an additional case to handle.

I was able to write a quick contract with a revert function that you can use in the core tests to test the contract call interface.

Thanks! What is the easiest way (on a linux machine) to run that test trough geth and parity in addition to eth-tester (which seems to be what's used when I just use default pytest)?

[kclowes]

But I think web3py should detect reverts, parse the revert reason and raise that information as a proper Exception in that case, too.

Yes, I agree!

it does not return errors as the other eth clients do, but directly raises python exceptions on transaction failures

In an effort to be backend-agnostic it changes errors that are specific to one EVM (like Revert) to a TransactionFailed error.

What is the easiest way (on a linux machine) to run that test trough geth and parity?

I think we'd have to do something like this using the revert contract, then add it to the geth fixture at the end of the setup_chain_state method. After that, it could be added as a fixture in the respective conftest file, for example:

web3.py/tests/integration/parity/conftest.py

Line 127 in 6ce485f

@pytest.fixture(scope="module")

and then it could get used in the module_testing files. I will probably be able to get to that this week, or if you get to it first, that's great. Let me know if you need more clarification!

[karlb]

One sensible addition to the RevertContract could be a revert without message, so that we can test that case, too.

In an effort to be backend-agnostic it changes errors that are specific to one EVM (like Revert) to a TransactionFailed error.

I'm now catching the eth-tester exception to create the result I intended. We'll see whether that lines up with the results from other backends.

I didn't find any time to look at setting up geth and parity. If you take care of that, I'll gladly have a look at any new failures.

[kclowes]

One sensible addition to the RevertContract could be a revert without message, so that we can test that case, too.

Good idea. I'll add that too.

I didn't find any time to look at setting up geth and parity. If you take care of that, I'll gladly have a look at any new failures.

Sounds good, thank you!

[Exef]

Is it going to be merged soon?

If not @karlb, can I continue your work on this PR?

[kclowes]

@Exef - @karlb was waiting on me to write some tests and then I lost track of this. Sorry about that! If you want to write some integration tests, please feel free!

[karlb]

@Exef @kclowes Is one of you working on integration tests? I want to make sure that we don't accidentally have a deadlock where everyone waits for someone else to act next.

[kclowes]

@karlb it's on my list, but I've been working on other things. I should be able to dig back in either later today or tomorrow but I'll let you know if that changes! And if @Exef gets to it first, that's great!

[kclowes]

@karlb I just finished up the other PR that I was working on, so I should be able to get to this on Monday. Thanks for your patience!

[kclowes]

@karlb I was able to make some progress on this today. I got the geth integration fixture up with the revert contract, and a simple test written, but I haven't had time to figure out why it is failing. I don't yet have the parity tests running either, but I should be able to keep working on it tomorrow.

I also can't remember how I got code to you last time, so feel free to take or leave the commits I pushed to this branch!

[karlb]

@karlb I was able to make some progress on this today. I got the geth integration fixture up with the revert contract, and a simple test written, but I haven't had time to figure out why it is failing.

Unfortunately, I don't get the same error as in the CI locally:

pytest tests/integration/go_ethereum/test_goethereum_http.py::TestGoEthereumEthModuleTest::test_eth_call_revert_with_msg -x --tb=short
...
____________________________________________________________________________________________ TestGoEthereumEthModuleTest.test_eth_call_revert_with_msg[<lambda>] _____________________________________________________________________________________________
web3/_utils/module_testing/eth_module.py:664: in test_eth_call_revert_with_msg
    result = web3.codec.decode_single('bool', call_result)
venv/lib/python3.7/site-packages/eth_abi/codec.py:155: in decode_single
    return decoder(stream)
venv/lib/python3.7/site-packages/eth_abi/decoding.py:127: in __call__
    return self.decode(stream)
venv/lib/python3.7/site-packages/eth_abi/decoding.py:198: in decode
    raw_data = self.read_data_from_stream(stream)
venv/lib/python3.7/site-packages/eth_abi/decoding.py:308: in read_data_from_stream
    len(data),
E   eth_abi.exceptions.InsufficientDataBytes: Tried to read 32 bytes.  Only got 0 bytes

Full output: error.txt

Apparently I get back call_result = HexBytes('0x'). Is my local setup broken, do I run the tests the wrong way, or is something else happening?

I also can't remember how I got code to you last time, so feel free to take or leave the commits I pushed to this branch!

Please just push everything related into this branch!

[wolovim]

Got some cleanup to do, but at long last, I think the base functionality is there 🎉

One stylistic choice to highlight: I applied Geth's convention to format all revert messages as execution reverted: <reason>. I'm not attached to this idea; only priority is standardizing across Geth, Parity/OE, and eth-tester. The alternatives would be to strip off the prefix from Geth or invent our own.

[kclowes]

Awesome!!! I think Geth's convention is good, especially since we're raising the same SolidityError across the board. And I like that the revert word is in the message to give an extra clue.

[wolovim]

Ready for review @kclowes (and anyone else interested).

• More LOC than is ideal, but most are related to fixture generation with the revert contract and deduping two geth generation files.
• Feedback especially welcome on the revert reason parsing code. I left most of Karl's original implementation to parse the raw data from Parity/OE. An alternative to that would be to use eth-abi's decode_single function, but IMO the readability is similar.

Thanks! 🙏

[kclowes]

Hooray! This will be so great to get in! I had mostly nitpicky comments and one about using assert in the code.

Also, because I can't comment on the file, I don't think we want to commit tests/integration/tester.zip :)

[sobolev-igor]

Hi everyone!
Is there anything blocking this PR?

I'm having an issue connected to this PR (#1781)
I really hope that this PR will fix the issue and I look forward to this PR being merged!

[wolovim]

@sobolev-igor just leaving space for any final reviews. Expecting to merge later today or tomorrow, but next release is TBD.

[njgheorghita]

Looks good to me! I just left a couple of thoughts on the style here & there, but nothing critical.

[pipermerriam]

This all feels fragile. That might be "ok" but it still worries me. Any thoughts on just doing the simple check that the data contains "Reverted " and then skipping this parsing logic and embedding the whole response message in the revert exception?

[wolovim]

I might be misunderstanding, but I'd definitely like to include the parsed revert message and not put that on the end user's shoulders. Are you suggesting returning both the reason and the full error dict, just the error dict, or something else?

[wolovim]

Current thinking in support of :

• in related issues filed, users are generally looking for the plaintext reason.
• handling reverts is broken today, so even brittle parsing logic is an improvement.
• I don't expect clients to change this formatting on a whim.
• we can iterate on it.

[pipermerriam]

I'm proposing that we return the string which we are trying to parse in full. basically this whole context block would become:

if data.startswith("Reverted "):
    reason = data

I don't feel strongly about this. It just seems like it would be roughly the same usefulness to the user and would avoid the part that looks brittle.

[wolovim]

Makes good sense, but it feels like signing up for an endless trickle of "what is this? how do I parse this?" questions. I'd prefer to sign up for patching the logic on the hopefully very rare occasions required and deliver the superior UX. Will flag this as something to harden though.

[fubuloubu]

This seems like a breaking change to the API of expected failures for web3. What was the reasoning for intercepting TransactionFailed failures and converting to this error (which also seems poorly named, perhaps class AssertionError(TransactionFailed) would be better considering Solidity isn't the only smart contract language out there that has an assertion feature e.g. Vyper, Fe, etc.)