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

Properly handle eth-tester exception messages #2783

Merged

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Jan 18, 2023

What was wrong?

  • After switching to abi.encode() across the code base, some eth-tester messages were not properly decoded.

context: eth-tester wraps some exceptions in other exceptions. This parsing needs to be accounted for as it was broken when we switched from eth-abi's encode_single to abi.encode.

How was it fixed?

Properly parse encoded messages from wrapped exceptions coming from eth-tester

Todo:

Cute Animal Picture

raw_error_msg = (
first_arg if not isinstance(first_arg, Exception) else first_arg.args[0]
)
reason = abi.decode(["string"], raw_error_msg[4:])[0]
Copy link
Collaborator Author

@fselmo fselmo Jan 18, 2023

Choose a reason for hiding this comment

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

The biggest change, since this refactor doesn't show it too well, is this line. Parentheses removed here:

["(string)"] -> ["string"]

@fselmo fselmo force-pushed the fix-eth-tester-exception-message-parsing branch 3 times, most recently from aa194b4 to c8dc81b Compare January 18, 2023 19:07
raw_error_msg = (
first_arg if not isinstance(first_arg, Exception) else first_arg.args[0]
)
reason = (
Copy link
Collaborator Author

@fselmo fselmo Jan 18, 2023

Choose a reason for hiding this comment

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

Also new... if the reason is bytes, decode as string, else use the raw message. Previously, we just assumed it was bytes which is probably always true... but this is an added check.

first_arg = e.args[0]
try:
# sometimes eth-tester wraps an exception in another exception
raw_error_msg = (
Copy link
Collaborator Author

@fselmo fselmo Jan 18, 2023

Choose a reason for hiding this comment

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

In case eth-tester isn't wrapping one exception in another... check to see if we have a wrapped exception, otherwise take the first arg as the raw error message. This was previously vaguely handled by catching an AttributeError.

@fselmo fselmo marked this pull request as ready for review January 18, 2023 19:10
@fselmo fselmo requested review from kclowes and pacrob January 18, 2023 19:10
- After switching to ``abi.encode()```, ``eth-tester`` messages were not properly decoded
@fselmo fselmo force-pushed the fix-eth-tester-exception-message-parsing branch from 76724e9 to 2d65236 Compare January 18, 2023 21:19
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 PR looks good to me. Is the wrapping something we should fix in eth-tester? It seems less than ideal to wrap an exception in an exception, but I'm not sure why it was added in the first place.

@fselmo
Copy link
Collaborator Author

fselmo commented Jan 19, 2023

Is the wrapping something we should fix in eth-tester? It seems less than ideal to wrap an exception in an exception, but I'm not sure why it was added in the first place.

Yeah, specifically here. I wonder if it was originally done to catch just one type of general exception from eth-tester (TransactionFailed)? 🤔

@fselmo fselmo merged commit 96b7785 into ethereum:master Jan 19, 2023
@fselmo fselmo deleted the fix-eth-tester-exception-message-parsing branch April 3, 2024 20:52
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.

2 participants