-
Notifications
You must be signed in to change notification settings - Fork 117
fix(tracing): convert empty revertReason to none #353
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
Open
jsvisa
wants to merge
7
commits into
paradigmxyz:main
Choose a base branch
from
jsvisa:empty-revert
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
afb2e03
fix(tracing): convert empty revert reason to empty
jsvisa 75d28c6
add test
jsvisa 336dc0b
decode Error or Panic
jsvisa 8db543d
fix: raw string to empty
jsvisa c68dbb4
clippy
jsvisa 109c4bb
add raw string testcase
jsvisa 9dd8c53
panic as geth str
jsvisa File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,22 +114,27 @@ pub(crate) fn load_account_code<DB: DatabaseRef>( | |
} | ||
|
||
/// Returns a non-empty revert reason if the output is a revert/error. | ||
/// Follows Geth's UnpackRevert logic | ||
/// <https://github.com/ethereum/go-ethereum/blob/4414e2833f92f437d0a68b53ed95ac5756a90a16/accounts/abi/abi.go#L278>. | ||
#[inline] | ||
pub(crate) fn maybe_revert_reason(output: &[u8]) -> Option<String> { | ||
let reason = match GenericRevertReason::decode(output)? { | ||
match GenericRevertReason::decode(output)? { | ||
GenericRevertReason::ContractError(err) => { | ||
match err { | ||
let reason = match err { | ||
// return the raw revert reason and don't use the revert's display message | ||
ContractError::Revert(revert) => revert.reason, | ||
// return as geth panic error string | ||
ContractError::Panic(panic) => panic.as_geth_str().to_string(), | ||
err => err.to_string(), | ||
}; | ||
|
||
if reason.is_empty() || reason.trim_matches('\0').is_empty() { | ||
None | ||
} else { | ||
Some(reason) | ||
} | ||
} | ||
GenericRevertReason::RawString(err) => err, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually, can we do this separately? this doesnt make much sense to me to not do this |
||
}; | ||
if reason.is_empty() { | ||
None | ||
} else { | ||
Some(reason) | ||
_ => None, | ||
} | ||
} | ||
|
||
|
@@ -153,4 +158,52 @@ mod tests { | |
let reason = maybe_revert_reason(&err[..]).unwrap(); | ||
assert_eq!(reason, "UniswapV2: INSUFFICIENT_INPUT_AMOUNT"); | ||
} | ||
|
||
#[test] | ||
fn decode_revert_reason_with_null_bytes() { | ||
let empty_err = GenericContractError::Revert("".into()); | ||
let encoded = empty_err.abi_encode(); | ||
assert!(maybe_revert_reason(&encoded).is_none()); | ||
|
||
let null_bytes_err = | ||
GenericContractError::Revert(String::from_utf8(vec![0u8; 32]).unwrap().into()); | ||
let encoded = null_bytes_err.abi_encode(); | ||
assert!(maybe_revert_reason(&encoded).is_none()); | ||
} | ||
|
||
#[test] | ||
fn decode_revert_reason_with_invalid_selector() { | ||
// Test data that doesn't start with revert or panic selector | ||
let invalid_data = vec![0x12, 0x34, 0x56, 0x78]; // Invalid selector | ||
assert_eq!( | ||
maybe_revert_reason(&invalid_data), | ||
None, | ||
"Should return None for invalid selector" | ||
); | ||
|
||
// Test data too short | ||
let short_data = vec![0x08, 0xc3]; // Only 2 bytes | ||
assert_eq!(maybe_revert_reason(&short_data), None, "Should return None for data too short"); | ||
|
||
// Test valid string but with proper selector validation | ||
let control_char_err = GenericContractError::Revert("\u{000f}.[l".into()); | ||
let encoded = control_char_err.abi_encode(); | ||
// This should now return the string since we're using Geth's validation logic | ||
assert_eq!(maybe_revert_reason(&encoded), Some("\u{000f}.[l".to_string())); | ||
|
||
// Test normal readable string | ||
let readable_err = GenericContractError::Revert("Normal error message".into()); | ||
let encoded = readable_err.abi_encode(); | ||
assert_eq!(maybe_revert_reason(&encoded), Some("Normal error message".to_string())); | ||
} | ||
|
||
#[test] | ||
fn decode_revert_reason_with_raw_string() { | ||
let non_readable_data = "\u{9a62}\u{0002}".as_bytes(); | ||
assert_eq!( | ||
maybe_revert_reason(non_readable_data), | ||
None, | ||
"Should return None for raw strings" | ||
); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think we only need to add the empty check here?
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 think raw string should also be omitted, changed into a simplify way
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.
why should this be omitted here?
can we only add the empty check, I dont see a reason to change this
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.
@mattsse, actually it was for another case, for this tx https://etherscan.io/tx/0x801eaaf69510d9a5fe9b8e8f2c9000f120d82b47ded984ee0fc875839684bc5e
reth will return as the raw string(not panic, or error), so abi decode will decode it as raw string, but geth only outputs for the panic or error:
https://github.com/alloy-rs/core/blob/175183bfb37e8806debb93f26b4a65fc77397697/crates/sol-types/src/types/interface/mod.rs#L466-L479
output:
sorry for the mixin issue
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.
but why would we not include the raw string error?
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.
Sorry for the delay, raw string represents invalid/malformed data, and this also follow Geth's behavior, which only handles proper
Error/Panic
abi-encoded errors.