feat: add count to expectCall cheatcodes

[reubenr0d]

Closes #4513

Motivation

Currently we can only assert that a call is made to a function using expectCall(), it would be nice to have another cheatcode to check the number of time a call was made, which can also be useful to assert if a call is not made.

Solution

Overriding the expectCall cheatcodes to also accept the number of times it is expected to be called and enforce the same.

[reubenr0d]

Adding cheatcodes to VM.sol : foundry-rs/forge-std#360

[mattsse]

wdyt @mds1

[mattsse]

I can see how this could be useful

[mds1]

Yep in favor of this, per #509 (comment). We should also add a similar overload for expectEmit in a follow up PR.

Also @reubenr0d two questions:

• Thanks for this + the forge-std PR! Can you also open a PR to the book repo for docs?
• If want to expect no calls for a given target/selector (i.e. just the selector, and not caring about the rest of the calldata), can I just past 4 bytes to the cheat? I forget how it works exactly 😅

[mattsse]

lgtm,

tests look great

[reubenr0d]

Can you also open a PR to the book repo for docs?

Sure, @mds1

can I just past 4 bytes to the cheat?

Yep, but as bytes not bytes4, so something like:

cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector), 0);

[mds1]

It looks like the regular expectCall methods without the new overload now fail if the call count is not 1, i.e. this was a breaking change because previously expectCall allowed any number of calls, but now it defaults to 1.

I'm not sure if this was intentional, but personally I'm ok with this change. Curious to hear your thoughts here @reubenr0d @mattsse. If we keep it, @reubenr0d do you mind getting another PR in to forge-std to update the code comments in Vm.sol to mention that the versions that don't take a count arg default to 1? (and mention this in the book PR too of course)

[reubenr0d]

I'm not sure if this was intentional

Oh that's right! It was intentional, but it didn't strike me earlier, good catch.

Personally I'm okay with this as well, but I will let you'll (@mds1 and @mattsse) decide on how to proceed here.

[lnist]

Just to chime in: we were using expectCall to generically verify all functions from our ABI had been called in a test with code similar to:

    for (uint i = 0; i < selectors.length; i++) {
      vm.expectCall(address(foo), selectors[i]);
    }

With the new API we cannot do this entirely generically since some functions will be invoked multiple times.

I am OK with the change, just wanted to mention it :)

[reubenr0d]

Hmm, on second thought and on seeing that other might have issues with this, I think it's best we preserve compatibility. I'll push a fix for this shortly.

[vicnaum]

If I put 10 expectCalls, would it expect 10 calls? I.e. do they sum up if I add more expectCall lines? Cause that's what I was thinking how it works and had several expectCalls lines if I expected several (identical) calls.

But now it fails after I've upgraded. So should I just leave one expectCall put a number there now?

[PaulRBerg]

If I put 10 expectCalls, would it expect 10 calls? I.e. do they sum up if I add more expectCall lines?

It should definitely work like this. See @reubenr0d's comment here:

#4845 (comment)