-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
use alloy GenericContractError
in reth_primitives::abi::decode_revert_reason
#4759
Conversation
I get some errors related to git dependencies not allowed. Specifically related to alloy which I actually need to use |
Codecov Report
... and 8 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@alessandromazza98 you need to add it here: Lines 97 to 103 in 0f9def0
|
@alessandromazza98 Can you please rebase your branch on top of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest LGTM after rebase
50e1caa
to
601ce54
Compare
Ok fixed the comments and changed branch where to put my PR into, hope to have done it correctly @DaniPopes ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last few things
Would be nice if we could add some test cases for this function, but I don't really know how to get them "efficiently". Maybe we can open an issue after this |
Solved comments. Speaking about the Let me know if you like it. |
I am interested in helping on this one. We can create a related issue and I could work on that. The properly test we should create some contracts that fails (both in Solidity and Vyper) and check if this function decodes the revert reason properly. Or maybe we could copy and paste some reverts from some contracts on-chain? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Closes #4730