-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
minimal changes to allow for custom error decoding #2795
Conversation
Thanks for the PR @joseluu! We are focused on getting v6 stable out right now, so we don't have the bandwidth to review right now but please ping me if you haven't heard anything in ~2 weeks! |
@kclowes gentle reminder |
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.
Thank you @joseluu! I started looking at this again. We'll need a test before we merge. I think the best thing to do is add a custom error to the existing revert contract. The source can be found under web3/_utils/contract_sources/RevertContract.sol
, and then you'll need to recompile the contract, and put the bytecode and new ABI in web3/_utils/contract_sources/contract_data/revert_contract.py
. Then some tests can be added in web3/_utils/module_testing/eth_module.py
, similar to the test_eth_call_revert_with_msg
/test_eth_call_revert_without_msg
tests in that file. Let me know if you want me to take over, or if you have any questions! Thanks again!
Hoping this PR can be merged soon |
…lity with existing exception handlers, sending to a wrong selector leads to an error with no data, this should be a ContractLogicError as before
@Shchepetov - does this PR fix your issue? As in - are you trying to get the raw data from a custom error revert? Or just a regular one? @fselmo - would love to get your eyes/opinions here when you get a chance! |
I cherry-picked these commits to a new branch - #2916 - that I made (and therefore have CI permissions on), and CI is passing there. |
Thanks @kclowes for filling-in the test requirements, I have had little time recently to devote for this. @fselmo : interpreting the error using the current contract ABI is certainly an improvement, it will ease simple cases where the error is raised by the contract itself and this will help a lot debug cases where one needs the extra information. However, in the general case, the error may be raised by another contract down the call chain, in which case the unformatted error should be passed as it is now for the application code to deal with it. |
Thanks @joseluu!
No problem! Thanks for getting it rolling.
Thanks for this info. It sounds like the ideal case is a option (maybe a flag passed to edit: there are already a couple issues to track, I don't think I need to add yet another :) |
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.
CI is passing on #2916, so this is okay to merge!
What was wrong?
In case of reverting in a smart contract using the "custom error" solidity construct available since solidity 0.8.4 (https://blog.soliditylang.org/2021/04/21/custom-errors/) , the details of the error were not available to the python application code
Related to Issue #
#2793
#2336
How was it fixed?
defining a new type of exception: ContractCustomError
raising this exception along with the data allows the application code to take it into account
example code that can be used by an application to decode a failed tx_hash for a contract whose abi is in variable DT_abi:
Todo:
Cute Animal Picture