fix(cheatcodes): Fix `expectCall` behavior by Evalir · Pull Request #4912 · foundry-rs/foundry

Evalir · 2023-05-10T06:10:00Z

This is a breaking change.

Closes #4879.

Implements the intended behavior for the expectCall cheatcode, with and without count.

Behavior

expectCall will be invoked in two different modes depending on how the cheatcode was used:

If no count was provided, the mode will be NonCount. The behavior of this mode is:
- It is additive. Calling expectCall(...) N times, will set a lower bound of expected calls of N, but will not revert if more than N calls are matched. This is the legacy expectCall behavior.
- For the same calldata, cannot be overwritten with a Count expectCall cheatcode call.

Example of NonCount additive behavior:

// this will PASS
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2));
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2));
target.add(1, 2);
target.add(1, 2);
target.add(1, 2);

Example of how it cannot be overwritten:

// this will FAIL
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2));
// Cannot overwrite a non-count expectCall cheatcode call with a count one.
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 2);
target.add(1, 2);
target.add(1, 2);
target.add(1, 2);

If a count is provided, the mode will be Count. The behavior of this mode is:
- It will match exactly N calls and revert if more are found, N being the amount of calls to match specified.
- It is NOT additive. Calling expectCall(..., N) M times will fail. It can only be called once if it has a count, even if it is 0.
- For the same calldata, it cannot be overwritten with any other expectCall cheatcode call, with or without count.

This essentially means that for any different calldata, there is only one expectCall cheatcode "mode" active. It is either NonCount and additive, or Count and non-additive. Note that a partial match and a full match is considered different calldata, so you could additively match partial-matches and strictly match full-matches. Tests have been added for each of these cases.

Evalir · 2023-05-10T06:10:45Z

cc @PaulRBerg — you might be interested in nitpicking this to make sure we are happy with how this is implemented now!

Evalir

Some helpful comments for reviewing (I will add more docs to the code nevertheless)

My hope is that we can use this same structure & adapt it for the other expect* cheatcodes with count.

Evalir · 2023-05-10T06:12:45Z

evm/src/executor/inspector/cheatcodes/mod.rs

-            for (address, expecteds) in &self.expected_calls {
-                for (expected, actual_count) in expecteds {
-                    let ExpectedCallData { calldata, gas, min_gas, value, count } = expected;
+            for (address, calldatas) in &self.expected_calls {


As a note-to-self, this and all other "handlers" on the different hooks should probably be abstracted into their own function and properly documented for easy modification later.

Evalir · 2023-05-10T06:16:06Z

evm/src/executor/inspector/cheatcodes/mod.rs


    /// Expected calls
-    pub expected_calls: BTreeMap<Address, Vec<(ExpectedCallData, u64)>>,
+    pub expected_calls: BTreeMap<Address, BTreeMap<Bytes, (ExpectedCallData, u64)>>,


This is now storing this information

BTreeMap<Address, // -> Target BTreeMap<Bytes, // -> Calldata. Selectors are NOT deduped, so partial matches and full matches can be applied. (ExpectedCallData, // -> The expected call data to match. Note that the calldata is now held as the key instead of inside this struct. u64) // -> The actual amount of times this call was seen > >

Evalir · 2023-05-10T06:17:53Z

evm/src/executor/inspector/cheatcodes/mod.rs

-                }) {
-                    *count += 1;
+                    {
+                        *actual_count += 1;


For any of the two modes, the discrimination of how to interpret the actual count is done later—our only job here is detecting if all values properly match for the current call.

Evalir · 2023-05-10T06:22:58Z

evm/src/executor/inspector/cheatcodes/mod.rs

-                                InstructionResult::Revert,
-                                remaining_gas,
-                                format!("Expected at least one call to {address:?} with {expected_values}, but got none")
+                    match call_type {


Match depending of mode:

Count: count Must be strictly actual_count, if not we should fail.

NonCount: actual_count must be at least equal to count (therefore, if we see that count is bigger than actual_count, we should fail as we didn't see enough calls).

For reference:

count: Amount of times a call must be seen.

actual_count Amount of times a call was actually seen.

evm/src/executor/inspector/cheatcodes/expect.rs

PaulRBerg · 2023-05-10T10:10:11Z

Hey @Evalir, thanks very much for this PR, as well as the helpful clarifications.

The proposal looks great; it is a good idea to have two separate modes for expectCall. I only have one question about this:

For the same calldata, cannot be overwritten with a Count expectCall cheatcode call.

By "cannot", do you mean Foundry will revert, or will it silently ignore the attempt to override the calldata's expect?

Evalir · 2023-05-10T14:32:25Z

Hey @PaulRBerg — Foundry will revert.

…ability lint

mattsse

few nits.

test lgtm.
seems like this fixes a few issues @PaulRBerg

@mds1 wdyt?

mattsse · 2023-05-11T20:18:52Z

evm/src/executor/inspector/cheatcodes/expect.rs

@@ -220,6 +230,54 @@ fn expect_safe_memory(state: &mut Cheatcodes, start: u64, end: u64, depth: u64)
    Ok(Bytes::new())
 }



can we please add a few docs here what this checks exactly checks and when this is supposed to return a revert

done in ce54d4b

mattsse · 2023-05-11T20:20:37Z

evm/src/executor/inspector/cheatcodes/mod.rs


    /// Expected calls
-    pub expected_calls: BTreeMap<Address, Vec<(ExpectedCallData, u64)>>,
+    pub expected_calls: BTreeMap<Address, BTreeMap<Vec<u8>, (ExpectedCallData, u64)>>,


this signature is a bit complex

I'm fine with making a wrapper type for this so it can be properly documented

done in ce54d4b

mds1 · 2023-05-11T21:50:09Z

This is great, thanks a lot @Evalir! Let's also get the book updated with the info from the PR description which is really thorough and helpful 💯

mattsse

lgtm, holding off on book changes so we have these before we publish.

Evalir added 2 commits May 10, 2023 01:55

chore: add tests to test proper behavior

3a93c1d

fix(cheatcodes): properly handle all cases for expectCall

d239e34

Evalir requested review from mattsse and mds1 May 10, 2023 06:10

Evalir commented May 10, 2023

View reviewed changes

chore: allow too many arguments

3ee294f

Evalir commented May 10, 2023

View reviewed changes

evm/src/executor/inspector/cheatcodes/expect.rs Show resolved Hide resolved

Evalir added 2 commits May 10, 2023 13:52

chore: store calldata as a vec instead of bytes to avoid interior mut…

4e95152

…ability lint

chore: more clippy

fac6366

Evalir mentioned this pull request May 10, 2023

feat(cheatcodes): Make expectEmit only work for the next call #4920

Merged

mattsse requested changes May 11, 2023

View reviewed changes

chore: add more docs and abstract signature

ce54d4b

Evalir requested a review from mattsse May 11, 2023 20:55

mattsse approved these changes May 11, 2023

View reviewed changes

Evalir mentioned this pull request May 12, 2023

feat(cheatcodes): document new expect call behavior foundry-rs/book#895

Merged

mattsse approved these changes May 12, 2023

View reviewed changes

mattsse merged commit bd4b290 into master May 12, 2023

mattsse deleted the evalir/fix_expect_call branch May 12, 2023 13:13

This was referenced May 12, 2023

Re-enable expectCall tests in cancelMultiple and withdrawMultiple sablier-labs/lockup#487

Closed

fix(forge): expectCall with no count #4845

Merged

		@@ -220,6 +230,54 @@ fn expect_safe_memory(state: &mut Cheatcodes, start: u64, end: u64, depth: u64)
		Ok(Bytes::new())
		}

Conversation

Evalir commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Behavior

Uh oh!

Evalir commented May 10, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PaulRBerg commented May 10, 2023

Uh oh!

Evalir commented May 10, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 commented May 11, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Evalir commented May 10, 2023 •

edited

Loading