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

Special case for Nethermind\Gnosis revert errors #2739

Merged
merged 9 commits into from Dec 1, 2022

Conversation

Aureliaar
Copy link
Contributor

@Aureliaar Aureliaar commented Nov 25, 2022

What was wrong?

Certain forms of errors, eg. generated on Spaceneth\Nethermind setups, would fail to return revert reasons on contracts.
Of the form: 'Reverted 0x4f776e6572496420646f6573206e6f7420657869737420696e207265676973747279'

How was it fixed?

A small edge case handling was added in the error_formatting.

Todo:

  • Add entry to the release notes
    This doesn't really seem worth mentioning in release notes. Let me know if that's wanted.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

Of the form: 'Reverted 0x4f776e6572496420646f6573206e6f7420657869737420696e207265676973747279'
@kclowes
Copy link
Collaborator

kclowes commented Nov 28, 2022

Thanks for this! Do you mind adding a test for this case in this file? I think we should also add a newsfragment, but I'm happy to do that if you don't want to. I can also fix the lint errors too, just let me know.

@Aureliaar
Copy link
Contributor Author

Will do!
Will also try to clean up the linter errors, but feel free to fix up the formatting how you like

@kclowes
Copy link
Collaborator

kclowes commented Nov 28, 2022

sounds good, thanks! eventually we'll have autoformatting on commit, but it hasn't reached the top of our priority queue yet.

@Aureliaar
Copy link
Contributor Author

Having some trouble running the tests locally, hope you don't mind me using your circleci to verify correctness!

@Aureliaar
Copy link
Contributor Author

Tests look fine!

I'd be grateful if you could fix the linter error, it seems to survive a pass of Black Formatter and it's actually pre-existing code

Likewise with the newsfragment.

Thanks for the prompt responses, and looking forward to seeing this merged

@kclowes
Copy link
Collaborator

kclowes commented Nov 30, 2022

Yep, no problem! Thanks again!

@kclowes kclowes merged commit 3d0b1cf into ethereum:master Dec 1, 2022
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.

None yet

2 participants