bug(forge)!: strip "revert: " from vm.expectRevert reason

[Hugoo]

Closes #10040

Solution

Please see the conv. here: #10040 (comment)

There is something to decide about backward compatibility + side effects

PR Checklist

• Added Tests
• Added Documentation
• Breaking changes

[mattsse]

this lgtm,

pending @grandizzy

[grandizzy]

Thank you, makes sense. please fix the fmt issue

[Hugoo]

@grandizzy done - sry my IDE was not set with +nightly

[grandizzy]

@Hugoo looks like the newly added testExpectRevertWithEncodedErrorPrefix test fails here https://github.com/foundry-rs/foundry/actions/runs/13993716850/job/39183491974#step:12:664 could you please check? (you can cargo test test_cheats_local_default locally) Thank you!

[Hugoo]

I had to do the check here because I need the decoded "actual" value.

It relies on the &decoder.decode which has: Note that this is just a best-effort guess, and should not be relied upon for anything other than user output.

I can add a specific check for this 0x08c379a0: the function selector for Error(string) (i.e., keccak256("Error(string)")[:4] above but i think it adds overload but lmk what you think.

[0xy0k]

To avoid relying purely on best-effort decoding, good to add a check for the 0x08c379a0 selector (Error(string)), which is the most common revert pattern. This keeps decoding accurate without affecting performance much.

[Hugoo]

I had a few weird cases, if I log like this:

println!("expected decoded: {} - expected reason: {}", expected, &stringify(expected_reason));
println!("actual decoded: {} - actual reason: {}", actual, &stringify(actual_revert.as_slice()));

When the revert message is a short string, revert("A") -> it gets decoded as EvmError: Revert from the known_contracts

function testExpectRevertWithEncodedErrorPrefix() public {
        Reverter reverter = new Reverter();
        vm.expectRevert(abi.encodeWithSignature("Error(string)", "A"));
        reverter.revertWithMessage("A");
}

expected decoded: revert: A - expected reason: 0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000014100000000000000000000000000000000000000000000000000000000000000

actual decoded: EvmError: Revert - actual reason: A ❌

If not, then it is decoded correctly:

function testExpectRevertWithEncodedErrorPrefix() public {
        Reverter reverter = new Reverter();
        vm.expectRevert(abi.encodeWithSignature("Error(string)", "randomString"));
        reverter.revertWithMessage("randomString");
}

expected decoded: revert: randomString - expected reason: 0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000c72616e646f6d537472696e670000000000000000000000000000000000000000
actual decoded: randomString - actual reason: randomString ✅

---

Try adding this to: testdata/default/cheats/ExpectRevert.t.sol

function testExpectRevertShortString() public {
        Reverter reverter = new Reverter();
        vm.expectRevert("A");
        reverter.revertWithMessage("A");
    }

Error:

Error (6675): Member "expectRevert" not unique after argument-dependent lookup in contract Vm.
  --> default/cheats/ExpectRevert.t.sol:89:9:
   |
89 |         vm.expectRevert("A");

There might be some contract conflicts and the decoder matchs it when decoding, I'm not sure I didn't have much time to look more into it.

[Hugoo]

nice @grandizzy thanks for handling the rest of this PR 🫡

[grandizzy]

nice @grandizzy thanks for handling the rest of this PR 🫡

np! yeah, still checking what would be the best way as this will be an breaking change, will update comments when ready

[grandizzy]

nice @grandizzy thanks for handling the rest of this PR 🫡

I added logic in RevertDecoder to try decoding as Revert and changed the order of checks - that is to avoid the EvmError if we can decode it as string. Going to need a review from team before merging.

[Hugoo]

yup nice thanks - yes i think this one is a little bit trickier than it looks like and def has breaking changes so... need to be handled with care 🫡

[yash-atreya]

lgtm!