fix(symbolic): re-registering an identical vm.mockCall replaces the stale entry - #16660
Open
gomesalexandre wants to merge 2 commits into
Open
fix(symbolic): re-registering an identical vm.mockCall replaces the stale entry#16660gomesalexandre wants to merge 2 commits into
gomesalexandre wants to merge 2 commits into
Conversation
…tale entry `add_call_mock` unconditionally pushed onto `state.call_mocks` on every `vm.mockCall`/`vm.mockCalls` invocation under `--symbolic`, so re-registering the same (callee, value, calldata) mock left both the stale and new entry in the vec. Selection resolves ties on `specificity()` by keeping the first-inserted candidate on a strict `>` comparison, so the stale mock always won over its own replacement -- diverging from the concrete cheatcode's map-insert (replace) semantics, already pinned by `testMockCallWithValue` in MockCall.t.sol. Dedupe at insert instead: search `call_mocks` for a structurally-matching entry and replace it in place, mirroring the sibling `set_function_mock` pattern in the same file. Deliberately leaves the specificity-based tie-break in calls.rs untouched, since that logic is relied on by other tests for genuinely-distinct overlapping mocks. Adds a regression test proving replace-on-remock, verified red (fails against the old push-only logic) before green.
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
gomesalexandre
marked this pull request as ready for review
September 5, 2026 00:37
gomesalexandre
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr,
mattsse and
stevencartavia
as code owners
September 5, 2026 00:37
figtracer
approved these changes
Sep 5, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
tl;dr
Under
--symbolic, re-registering the exact samevm.mockCall/vm.mockCalls(identicalcallee/value/calldata) left the stale mock winning forever, instead of replacing it like concrete mode does.The bug
add_call_mock(crates/evm/symbolic/src/executor/cheatcodes.rs) unconditionallypushed ontostate.call_mocks: Vec<CallMock>on everyvm.mockCall/vm.mockCallscall. Re-registering the same(callee, value, calldata)therefore left BOTH the old and new entry in the vec.Selection (
executor/calls.rs, untouched by this diff) resolves ties onspecificity()--(data.len(), value.is_some())-- by keeping the FIRST-inserted candidate on a strict>comparison. So the stale mock always won over its own replacement, diverging from the concrete cheatcode'sBTreeMap-keyed map-insert (replace) semantics, which are already pinned bytestMockCallWithValueintestdata/default/cheats/MockCall.t.sol.The fix
Dedupe at insert time, not at selection:
add_call_mocknow searchescall_mocksfor an existing entry whose(callee, value, data)structurally matches (newCallMock::matches_definition, mirroring the already-correct siblingset_function_mock/FunctionMock::matches_definitionpattern in the same file) and replaces it in place; otherwise it pushes as before.Deliberately does not touch the specificity-based tie-break in
calls.rs-- that logic is relied on by other tests for genuinely-distinct overlapping mocks, and changing it would risk disturbing that unrelated precedence behavior.Known limitation (by design, out of scope): dedup is exact structural equality, not solver-level equivalence. Two symbolic expressions that are equal only under a
vm.assumeconstraint won't dedupe. This matches how the rest of the mock-matching machinery already works and is a safe, conservative choice -- it never merges two definitions that aren't syntactically identical.Testing
checkMockCallRemockReplacesStaleValuetosymbolic_cheatcodes.rs, exercising the exact repro (mock a value, re-mock the identical callee/calldata with a new return, assert the new value wins). Verified genuinely red against the old push-only logic ([FAIL: symbolic counterexample did not replay...]), green after the fix ([PASS] checkMockCallRemockReplacesStaleValue).symbolic_cheatcodesCLI suite: 60/61 passed. The one failure (symbolic_mapping_storage_hooks, afuzz.runs must be greater than 0config panic) is pre-existing and unrelated -- confirmed identical against unmodified master viagit stash.cargo clippy -p foundry-evm-symbolic -- -D warnings: clean.cargo fmt --check: clean (only the usual nightly-only-feature notices, no diff on stable).mockCalls-array fallthrough behavior differs from concrete mode) -- both predate this diff and aren't touched by it.closes nothing (no tracked issue -- found via direct code inspection comparing concrete vs. symbolic mock-call semantics)