-
Notifications
You must be signed in to change notification settings - Fork 151
Fix revert detection #4041
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
Fix revert detection #4041
Conversation
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.
Code Review
This pull request addresses a critical issue where transaction simulation failures were not being detected, which could lead to submitting transactions that are known to fail. The fix is implemented by correctly mapping gas estimation errors to Error::Rpc and updating the is_revert method to properly identify RPC error responses as reverts. The changes are logical and effectively solve the problem described. I have one comment regarding a related inconsistency that this change brings to light, which could be addressed in a follow-up.
| Error::ContractRpc(_) => true, | ||
| Error::Rpc(_) => false, | ||
| Error::Rpc(err) => err.is_error_resp() |
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.
This change is a good improvement for handling Error::Rpc. However, it highlights an inconsistency with how Error::ContractRpc is handled.
Error::ContractRpc(_) is always considered a revert, which is too broad as it would include transient network errors. Your change correctly handles this for Error::Rpc by checking is_error_resp().
This inconsistency could lead to different behavior for similar errors depending on whether they come from a contract call or a direct RPC call.
I recommend creating a follow-up to make the Error::ContractRpc handling more specific. For example:
use ethrpc::alloy::errors::ContractErrorExt;
// ...
Error::ContractRpc(err) => !err.is_node_error(),This would make the revert detection more consistent and robust across different error types.
use ethrpc::alloy::errors::ContractErrorExt;
// ...
Error::ContractRpc(err) => !err.is_node_error(),
jmg-duarte
left a comment
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.
I had noticed this on friday but I was not aware how bad it was. Great catch and thanks for the fix!
Description
#3985 changed (see here) the returned error variant. The most important issue is that
Error::is_revert()always returnsfalsefor the new returned error variant which causes some error handling to behavior to be wrong now:Changes
Change returned error variant to
Error::Rpcand implementis_revert()for it such that it's (mostly) compatible with the original code. Now any message returned from the RPC that is of this shape will be interpreted as a revert. The oldweb3crate had some client specific error message parsing but that seems overly specific so I think the new code should be sufficient to fix the errors.