fix(forge): expectCall with no count

[reubenr0d]

Motivation

#4833 introduced a breaking change to expectCall whereby if no count was specified it would default to 1. This PR preserves the older behaviour of checking that a call is made at least once if count is not specified.

Solution

Change count type to Option<u64> and revert to older behaviour if is_none()

[mds1]

Thanks! Agreed it makes sense to revert this back to the original behavior per the discussion in the prior PR

[mattsse]

yep, agree

[PaulRBerg]

I don't think this is working as expected.

I pulled the latest Forge:

forge 0.2.0 (f128ff9 2023-04-29T00:16:05.704419000Z)

And our existing tests (which are using multiple standard count-free expectCall cheats in a single test) are failing with this error:

[FAIL. Reason: Expected at least one call to 0x2cc9... with data 0x4bc78..., but got none

Upgrading to the latest Vm and using the new expectCall overload with a count parameter fixes this.

[mattsse]

@reubenr0d mind taking another look at this?

Do you an example where it broke @PaulRBerg?

[PaulRBerg]

@mattsse sure, here's an example.

Code Snippet

pragma solidity >=0.8.19 <0.9.0;

import { Test } from "forge-std/Test.sol";

contract Foo {
    function func() external pure returns (uint256) {
        return 1;
    }
}

contract Bar {
    Foo internal foo;

    constructor(Foo foo_) {
        foo = foo_;
    }

    function func(uint256 times) external view returns (uint256) {
        for (uint256 i = 0; i < times; ++i) {
            foo.func();
        }
        return 1;
    }
}

contract ExpectCallTest is Test {
    Foo internal foo = new Foo();
    Bar internal bar = new Bar(foo);

    // Fails on forge 0.2.0 (f128ff9 2023-04-29T00:16:05.704419000Z)
    function test_MultipleExpectCalls_1() external {
        vm.expectCall({ callee: address(foo), data: abi.encodeWithSelector(foo.func.selector) });
        vm.expectCall({ callee: address(foo), data: abi.encodeWithSelector(foo.func.selector) });
        bar.func({ times: 2 });
    }

    // Passes
    function test_MultipleExpectCalls_2() external {
        vm.expectCall({ callee: address(foo), data: abi.encodeWithSelector(foo.func.selector), count: uint64(2) });
        bar.func({ times: 2 });
    }
}

[reubenr0d]

Thanks for reporting, @PaulRBerg. This was also reported here: #4833 (comment)
My bad, I didn't realise earlier that expectCall could be called multiple times to assert the number of times the call was made.

This needs some more work in that case!

Just to clarify,

1. we want to retain the exact behaviour of the older expectCall without count, whereby if the target is called say 2 times, it would assert that the call is made 2 or more times.

// This should 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);

2. when count is passed and there are partial matches, it seems more intuitive to match all partial and full matches:

// This should PASS
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 2); // This is counted
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector), 2); // This is also counted
target.add(1, 2);
target.add(1, 2);

But this behaviour would be different from if count was not specified, in the older version:

cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2)); // This would pass
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector)); // This would fail
target.add(1, 2);

3. If count is specified more than once for a full match, overwriting the count feels better:

// This should PASS
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 5); // This count is overwritten
cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector, 1, 2), 2); // This count is used
target.add(1, 2);
target.add(1, 2);

While in the older versions, when count is not passed, the least expected count is incremented, like in point 1.

There are some conflicting ideas here; keen to hear on what you'll think on how we should proceed with this.
cc @mattsse @mds1

[mds1]

Nice write up, so my thoughts are:

1. This sounds good to me
2. Also sounds good
3. This should revert if we can, feels safer than silently overwriting the first declaration

[PaulRBerg]

we want to retain the exact behaviour of the older expectCall without count, whereby if the target is called say 2 times, it would assert that the call is made 2 or more times.

💯

This is actually a useful thing in and of itself; even if there had not been a path dependence on this behavior, I would have preferred Foundry to offer it because it is needed e.g. when fuzzing and the exact number of token transfers cannot be known in advance.

Basically, when not passing count as a parameter, Foundry should retain its previously additive behavior.

[PaulRBerg]

@reubenr0d do you have an ETA for reverting the previous behavior as per point 1 above?

Sorry to chase - it's just that our test suite is broken now, and the only ways to fix it are to either (i) remove certain vm.expectCall cheats or (ii) wait until the previous behavior is brought back (the additive behavior).

[mattsse]

Basically, when not passing count as a parameter, Foundry should retain its previously additive behavior.

agree, @reubenr0d do you have bandwidth to make this work?

[reubenr0d]

I’m a bit held up at the moment, but I should be able to get around to it later in the week.

[reubenr0d]

oof, this also broke a test in one of our repos:

function test_Metadata() public {
  string memory symbol = "FOO";
  vm.mockCall(asset, abi.encodeCall(IERC20Metadata.symbol, ()), abi.encode(symbol));

  vm.expectCall(asset, abi.encodeCall(IERC20Metadata.symbol, ())); // Passes
  assertEq(tenderizer.name(), string(abi.encodePacked("tender", symbol, " ", validator)), "invalid name"); // Calls asset.symbol

  vm.expectCall(asset, abi.encodeCall(IERC20Metadata.symbol, ())); // Fails
  assertEq(tenderizer.symbol(), string(abi.encodePacked("t", symbol, "_", validator)), "invalid symbol"); // Calls asset.symbol
}

I guess ideally they would be in two separate test functions, but I should preserve this functionality as well I suppose.

[PaulRBerg]

My guess is that a lot of Foundry repos have been affected by this breaking change - many of which will not manage to trace the issue to our discussion here, and will end up opening new issues or asking questions in the Telegram group.

As a quick patch, I would suggest reverting the changes introduced by this PR (as well as #4833) until the additive behavior of expectCall without count is brought back.

[mattsse]

I can try to fix this asap but need a test case that I can just use.

[PaulRBerg]

Doesn't my code snippet from above work, @mattsse?

We ended up disabling all of our expectCall tests (by marking the functions as internal), but we'd like to enable them as we prepare for another audit later this month.

[PaulRBerg]

Can confirm that #4912 has resolved the issue we've been having with our tests. They work now!

Thanks @Evalir.