Skip to content

Commit 89b7470

Browse files
Hugoograndizzy
Hugoo
and
grandizzy
authored
bug(forge)!: strip "revert: " from vm.expectRevert reason (#10144)
* bug(forge)!: strip "revert: " from vm.expectRevert reason * Impl update, update decoder: - match on ContractError:Revert - move checks before split first chunk / EvmError --------- Co-authored-by: grandizzy <38490174+grandizzy@users.noreply.github.com> Co-authored-by: grandizzy <grandizzy.the.egg@gmail.com>
1 parent 92e020e commit 89b7470

File tree

7 files changed

+127
-93
lines changed

7 files changed

+127
-93
lines changed

‎crates/cheatcodes/src/test/revert_handlers.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,12 @@ fn handle_revert(
108108
} else {
109109
(&stringify(&actual_revert), &stringify(expected_reason))
110110
};
111-
Err(fmt_err!("Error != expected error: {} != {}", actual, expected,))
111+
112+
if expected == actual {
113+
return Ok(());
114+
}
115+
116+
Err(fmt_err!("Error != expected error: {} != {}", actual, expected))
112117
}
113118
}
114119

‎crates/evm/core/src/decode.rs

Lines changed: 72 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@ use crate::abi::{console, Vm};
44
use alloy_dyn_abi::JsonAbiExt;
55
use alloy_json_abi::{Error, JsonAbi};
66
use alloy_primitives::{hex, map::HashMap, Log, Selector};
7-
use alloy_sol_types::{SolEventInterface, SolInterface, SolValue};
7+
use alloy_sol_types::{
8+
ContractError::Revert, RevertReason, RevertReason::ContractError, SolEventInterface,
9+
SolInterface, SolValue,
10+
};
811
use foundry_common::SELECTOR_LEN;
912
use itertools::Itertools;
1013
use revm::interpreter::InstructionResult;
@@ -138,65 +141,91 @@ impl RevertDecoder {
138141
///
139142
/// See [`decode`](Self::decode) for more information.
140143
pub fn maybe_decode(&self, err: &[u8], status: Option<InstructionResult>) -> Option<String> {
141-
let Some((selector, data)) = err.split_first_chunk::<SELECTOR_LEN>() else {
142-
if let Some(status) = status {
143-
if !status.is_ok() {
144-
return Some(format!("EvmError: {status:?}"));
145-
}
146-
}
147-
return if err.is_empty() {
148-
None
149-
} else {
150-
Some(format!("custom error bytes {}", hex::encode_prefixed(err)))
151-
};
152-
};
153-
154144
if let Some(reason) = SkipReason::decode(err) {
155145
return Some(reason.to_string());
156146
}
157147

158-
// Solidity's `Error(string)` or `Panic(uint256)`, or `Vm`'s custom errors.
148+
// Solidity's `Error(string)` (handled separately in order to strip revert: prefix)
149+
if let Some(ContractError(Revert(revert))) = RevertReason::decode(err) {
150+
return Some(revert.reason);
151+
}
152+
153+
// Solidity's `Panic(uint256)` and `Vm`'s custom errors.
159154
if let Ok(e) = alloy_sol_types::ContractError::<Vm::VmErrors>::abi_decode(err, false) {
160155
return Some(e.to_string());
161156
}
162157

163-
// Custom errors.
164-
if let Some(errors) = self.errors.get(selector) {
165-
for error in errors {
166-
// If we don't decode, don't return an error, try to decode as a string later.
167-
if let Ok(decoded) = error.abi_decode_input(data, false) {
168-
return Some(format!(
169-
"{}({})",
170-
error.name,
171-
decoded.iter().map(foundry_common::fmt::format_token).format(", ")
172-
));
158+
let string_decoded = decode_as_non_empty_string(err);
159+
160+
if let Some((selector, data)) = err.split_first_chunk::<SELECTOR_LEN>() {
161+
// Custom errors.
162+
if let Some(errors) = self.errors.get(selector) {
163+
for error in errors {
164+
// If we don't decode, don't return an error, try to decode as a string
165+
// later.
166+
if let Ok(decoded) = error.abi_decode_input(data, false) {
167+
return Some(format!(
168+
"{}({})",
169+
error.name,
170+
decoded.iter().map(foundry_common::fmt::format_token).format(", ")
171+
));
172+
}
173173
}
174174
}
175-
}
176175

177-
// ABI-encoded `string`.
178-
if let Ok(s) = String::abi_decode(err, true) {
179-
return Some(s);
176+
if string_decoded.is_some() {
177+
return string_decoded
178+
}
179+
180+
// Generic custom error.
181+
return Some({
182+
let mut s = format!("custom error {}", hex::encode_prefixed(selector));
183+
if !data.is_empty() {
184+
s.push_str(": ");
185+
match std::str::from_utf8(data) {
186+
Ok(data) => s.push_str(data),
187+
Err(_) => s.push_str(&hex::encode(data)),
188+
}
189+
}
190+
s
191+
})
180192
}
181193

182-
// ASCII string.
183-
if err.is_ascii() {
184-
return Some(std::str::from_utf8(err).unwrap().to_string());
194+
if string_decoded.is_some() {
195+
return string_decoded
185196
}
186197

187-
// Generic custom error.
188-
Some({
189-
let mut s = format!("custom error {}", hex::encode_prefixed(selector));
190-
if !data.is_empty() {
191-
s.push_str(": ");
192-
match std::str::from_utf8(data) {
193-
Ok(data) => s.push_str(data),
194-
Err(_) => s.push_str(&hex::encode(data)),
195-
}
198+
if let Some(status) = status {
199+
if !status.is_ok() {
200+
return Some(format!("EvmError: {status:?}"));
196201
}
197-
s
198-
})
202+
}
203+
if err.is_empty() {
204+
None
205+
} else {
206+
Some(format!("custom error bytes {}", hex::encode_prefixed(err)))
207+
}
208+
}
209+
}
210+
211+
/// Helper function that decodes provided error as an ABI encoded or an ASCII string (if not empty).
212+
fn decode_as_non_empty_string(err: &[u8]) -> Option<String> {
213+
// ABI-encoded `string`.
214+
if let Ok(s) = String::abi_decode(err, true) {
215+
if !s.is_empty() {
216+
return Some(s);
217+
}
218+
}
219+
220+
// ASCII string.
221+
if err.is_ascii() {
222+
let msg = std::str::from_utf8(err).unwrap().to_string();
223+
if !msg.is_empty() {
224+
return Some(msg);
225+
}
199226
}
227+
228+
None
200229
}
201230

202231
fn trimmed_hex(s: &[u8]) -> String {

‎crates/forge/tests/cli/failure_assertions.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@ forgetest!(expect_revert_tests_should_fail, |prj, cmd| {
5050
[FAIL: next call did not revert as expected] testShouldFailExpectRevertDidNotRevert() ([GAS])
5151
[FAIL: Error != expected error: but reverts with this message != should revert with this message] testShouldFailExpectRevertErrorDoesNotMatch() ([GAS])
5252
[FAIL: next call did not revert as expected] testShouldFailRevertNotOnImmediateNextCall() ([GAS])
53-
[FAIL: revert: some message] testShouldFailexpectCheatcodeRevertForCreate() ([GAS])
54-
[FAIL: revert: revert] testShouldFailexpectCheatcodeRevertForExtCall() ([GAS])
53+
[FAIL: some message] testShouldFailexpectCheatcodeRevertForCreate() ([GAS])
54+
[FAIL: revert] testShouldFailexpectCheatcodeRevertForExtCall() ([GAS])
5555
Suite result: FAILED. 0 passed; 7 failed; 0 skipped; [ELAPSED]
5656
...
5757
"#,
@@ -238,7 +238,7 @@ forgetest!(mem_safety_test_should_fail, |prj, cmd| {
238238
r#"[COMPILING_FILES] with [SOLC_VERSION]
239239
[SOLC_VERSION] [ELAPSED]
240240
...
241-
[FAIL: revert: Expected call to fail] testShouldFailExpectSafeMemoryCall() ([GAS])
241+
[FAIL: Expected call to fail] testShouldFailExpectSafeMemoryCall() ([GAS])
242242
[FAIL: memory write at offset 0x100 of size 0x60 not allowed; safe range: (0x00, 0x60] U (0x80, 0x100]] testShouldFailExpectSafeMemory_CALL() ([GAS])
243243
[FAIL: memory write at offset 0x100 of size 0x60 not allowed; safe range: (0x00, 0x60] U (0x80, 0x100]] testShouldFailExpectSafeMemory_CALLCODE() ([GAS])
244244
[FAIL: memory write at offset 0xA0 of size 0x20 not allowed; safe range: (0x00, 0x60] U (0x80, 0xA0]; counterexample: calldata=[..] args=[..]] testShouldFailExpectSafeMemory_CALLDATACOPY(uint256) (runs: 0, [AVG_GAS])
@@ -336,7 +336,7 @@ contract FailingSetupTest is DSTest {
336336
r#"[COMPILING_FILES] with [SOLC_VERSION]
337337
[SOLC_VERSION] [ELAPSED]
338338
...
339-
[FAIL: revert: setup failed predictably] setUp() ([GAS])
339+
[FAIL: setup failed predictably] setUp() ([GAS])
340340
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; [ELAPSED]
341341
...
342342
"#

‎crates/forge/tests/cli/script.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ forgetest_async!(assert_exit_code_error_on_failure_script, |prj, cmd| {
152152

153153
// run command and assert error exit code
154154
cmd.assert_failure().stderr_eq(str![[r#"
155-
Error: script failed: revert: failed
155+
Error: script failed: failed
156156
157157
"#]]);
158158
});
@@ -168,7 +168,7 @@ forgetest_async!(assert_exit_code_error_on_failure_script_with_json, |prj, cmd|
168168

169169
// run command and assert error exit code
170170
cmd.assert_failure().stderr_eq(str![[r#"
171-
Error: script failed: revert: failed
171+
Error: script failed: failed
172172
173173
"#]]);
174174
});

‎crates/forge/tests/cli/test_cmd.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -893,17 +893,17 @@ Compiler run successful!
893893
894894
Ran 4 tests for test/ReplayFailures.t.sol:ReplayFailuresTest
895895
[PASS] testA() ([GAS])
896-
[FAIL: revert: testB failed] testB() ([GAS])
896+
[FAIL: testB failed] testB() ([GAS])
897897
[PASS] testC() ([GAS])
898-
[FAIL: revert: testD failed] testD() ([GAS])
898+
[FAIL: testD failed] testD() ([GAS])
899899
Suite result: FAILED. 2 passed; 2 failed; 0 skipped; [ELAPSED]
900900
901901
Ran 1 test suite [ELAPSED]: 2 tests passed, 2 failed, 0 skipped (4 total tests)
902902
903903
Failing tests:
904904
Encountered 2 failing tests in test/ReplayFailures.t.sol:ReplayFailuresTest
905-
[FAIL: revert: testB failed] testB() ([GAS])
906-
[FAIL: revert: testD failed] testD() ([GAS])
905+
[FAIL: testB failed] testB() ([GAS])
906+
[FAIL: testD failed] testD() ([GAS])
907907
908908
Encountered a total of 2 failing tests, 2 tests succeeded
909909
@@ -917,16 +917,16 @@ Encountered a total of 2 failing tests, 2 tests succeeded
917917
No files changed, compilation skipped
918918
919919
Ran 2 tests for test/ReplayFailures.t.sol:ReplayFailuresTest
920-
[FAIL: revert: testB failed] testB() ([GAS])
921-
[FAIL: revert: testD failed] testD() ([GAS])
920+
[FAIL: testB failed] testB() ([GAS])
921+
[FAIL: testD failed] testD() ([GAS])
922922
Suite result: FAILED. 0 passed; 2 failed; 0 skipped; [ELAPSED]
923923
924924
Ran 1 test suite [ELAPSED]: 0 tests passed, 2 failed, 0 skipped (2 total tests)
925925
926926
Failing tests:
927927
Encountered 2 failing tests in test/ReplayFailures.t.sol:ReplayFailuresTest
928-
[FAIL: revert: testB failed] testB() ([GAS])
929-
[FAIL: revert: testD failed] testD() ([GAS])
928+
[FAIL: testB failed] testB() ([GAS])
929+
[FAIL: testD failed] testD() ([GAS])
930930
931931
Encountered a total of 2 failing tests, 0 tests succeeded
932932
@@ -2123,8 +2123,8 @@ forgetest_init!(should_generate_junit_xml_report, |prj, cmd| {
21232123
<system-out>[FAIL: panic: assertion failed (0x01)] test_junit_assert_fail() ([GAS])</system-out>
21242124
</testcase>
21252125
<testcase name="test_junit_revert_fail()" time="[..]">
2126-
<failure message="revert: Revert"/>
2127-
<system-out>[FAIL: revert: Revert] test_junit_revert_fail() ([GAS])</system-out>
2126+
<failure message="Revert"/>
2127+
<system-out>[FAIL: Revert] test_junit_revert_fail() ([GAS])</system-out>
21282128
</testcase>
21292129
<system-out>Suite result: FAILED. 0 passed; 2 failed; 0 skipped; [ELAPSED]</system-out>
21302130
</testsuite>

0 commit comments

Comments
 (0)