Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meta: how to improve mocking in forge-std? #46

Closed
mds1 opened this issue Apr 21, 2022 · 24 comments
Closed

meta: how to improve mocking in forge-std? #46

mds1 opened this issue Apr 21, 2022 · 24 comments

Comments

@mds1
Copy link
Collaborator

mds1 commented Apr 21, 2022

Forge has two cheatcodes for mocking:

interface Vm {
    // Mocks a call to an address, returning specified data.
    // Calldata can either be strict or a partial match, e.g. if you only
    // pass a Solidity selector to the expected calldata, then the entire Solidity
    // function will be mocked.
    function mockCall(address, bytes calldata, bytes calldata) external;
    // Clears all mocked calls
    function clearMockedCalls() external;
}

This results in the following mocking UX native to forge:

  • Mocking responses at an address that already has code is straightforward using mockCall
  • Mocking responses at an address that does not have code is a bit clunky, as it requires etching dummy code to that address
  • Mocking reverts and out of gas scenarios is not possible natively
  • If you call a method at a mocked address and it doesn't exist, the revert reason is empty instead of something like "function selector not found on target address", which can be confusing. (This is a general forge issue, not mock specific).

This is ok, but not great. Given that, what should forge-std do to improve mocking UX, and what should be upstreamed to forge?

For context, various mocking contracts exist to help with the above scenarios (note that I have not carefully reviewed each, these are just my quick impressions):

  • Gnosis' MockContract.sol is very flexible, and allows you to mock reverts and out of gas errors
  • Waffle's Doppelganger.sol is similar. It has less code (so presumably faster to compile) but does not support mocking out of gas errors.
  • @maurelian's UniMock seems to be the smallest, and could probably be easily extended to mock an out of gas error.
  • @cleanunicorn's MockPrivder seems like another good option.

My initial reaction is:

  1. Improve forge so it can provide better revert reasons if the revert is due to a selector not being found.
  2. Improve forge's mockCall so it can support mocking reverts with specified data. (Maybe support mocking out of gas errors too? Not sure how common needing to mock OOG is, just mentioning it since gnosis's mock supports it).
  3. With the above, we can leave forge's mockCall as the default for when you need to mock responses on contracts that already exist.
  4. Then add one of the above mock contracts to forge-std with deployMock() and deployMock(address where) helper methods. This would be used in cases where you need a mock at an address that doesn't have any code.

I think this should should cover all popular use cases of mocking, and uncommon use cases probably don't need to be supported in forge-std. But let me know if there's common scenarios I missed!

Tagging @bbonanno @maurelian and @cleanunicorn since I think you all may have some thoughts/feedback on the above 🙂

@cleanunicorn
Copy link

One thing I did not realize it that people need out of gas simulation. I just created this issue in my mocking provider repo.

cleanunicorn/mockprovider#7

I will keep following this thread and I will try to contribute.

@ultrasecreth
Copy link

Another feature I'd like a lot, but probably needs to be done in foundry, is the ability to capture values and/or do better assertions on verify.

Right now, the only way to check that a dependency was called is to do

vm.expectCall(address(dep), abi.encodeWithSelector(dep.foo.selector, param1, paramN));
dep.foo();

There are a couple of issues with this approach

  • If at least one of the params doesn't exactly match, you get something like "method call was expected with calldata but got none", and the only way to find out what happened is to try to compare that with the calldata that you see in the execution trace.
  • It only allows for strict equality of the params
  • It doesn't allow to use the value later in the test, i.e, it requires to hardcode a value that may only stay the same as long as the setup doesn't change (like an address of a contract deployed internally)

So what I'd love to have is something like this

dep.foo();
bytes[] memory captures = vm.capure(address(dep), dep.foo.selector);

assertEq(captures.length, 1, "Method wasn't called the expected number of times!");

(int param1, address param2, ... , paramX) = abi.decode(captures[0], (int, address, ...);

assertGt(param1, 0); // Can do non-exact match of the param
// Can call something else on param2 now that I know it's address
// etc

In a way it's kinda similar to the existing function accesses(address) external returns (bytes32[] memory reads, bytes32[] memory writes);, but for contract interactions.

Thoughts?

@cleanunicorn
Copy link

cleanunicorn commented Apr 22, 2022

@bbonanno You should have a closer look at my library because, as I understand, it already supports the features you're requesting.

For example you can mock a method, without the need to specify the exact parameters. As long as the msg.sig matches, it can reply with an expected result.

An example:

// Make it return false whenever calling .isEven(anything)
provider.givenSelectorReturnResponse(
    // Respond to `.isEven()`
    abi.encodePacked(IOddEven.isEven.selector),
    // Encode the response
    MockProvider.ReturnData({
        success: true,
        data: abi.encodePacked(bool(false))
    }),
    // Log the event
    false
);

That means that you could call the mocked method .isEven() with any argument and it will reply with false.

provider.isEven(1) == false
provider.isEven(42) == false

Another way to achieve even more flexibility is to set a default response for ANY kind of request.

Code example:

// Make the provider successfully respond with 42 for any request
provider.setDefaultResponse(
    MockProvider.ReturnData({
        success: true,
        data: abi.encode(uint256(42))
    })
);

If you want to inspect the requests being made you can even enable logging and check the logged calls afterward.

provider.getCallData(0); // 0 is the index of the call, it logs all requests.

A caveat to enabling default responses is if there is any call that uses the opcode STATICCALL, logging won't work, since it will try to write to the provider's state, thus it will fail.

@ultrasecreth
Copy link

@cleanunicorn the partial mocking is also possible with the standard mockCall, that's not an issue.

For the capturing, looking at the logs is what I do now, but is not ideal, and it doesn't solve the issue of not doing exact comparison with the values.

I'm trying to do something like an ArgumentCaptor in mockito (if you ever used it)

@cleanunicorn
Copy link

I spent some time trying to understand Mockito, but I am not a Java developer but the idiosyncratic approach doesn't make a lot of sense to me.

This is the article I spent most time with https://www.baeldung.com/mockito-argumentcaptor

And I did not understand the advantages of using Mockito vs normal mocking.

Going back to your "ideal" code example, it still doesn't make a lot of sense. I guess this is because you didn't have enough time to explain how everything should work.

I am going to split that into multiple sections and try to explain what I understand and ask additional questions.

dep.foo();

I assume this just calls the method that you want to mock or test. Unfortunately, it's not what what is mocked and what is tested, what is the implementation being tested and what is the mocked code connected to the implementation.

bytes[] memory captures = vm.capure(address(dep), dep.foo.selector);

I assume this captures the previous calls to the dep contract instance, specifically the method identified by dep.foo.selector. This should be possible after the call was made since it is called after dep.foo().

assertEq(captures.length, 1, "Method wasn't called the expected number of times!");

This checks how many calls were made to the method identified by dep.foo(), however it's not clear why this is a test since the first line of code in the snippet calls dep.foo() exactly once. Maybe I don't understand where the code ends and the test begins.

(int param1, address param2, ... , paramX) = abi.decode(captures[0], (int, address, ...);

This takes the first logged call and decodes something. It's not clear what it decodes because the method dep.foo() does not accept any arguments.

assertGt(param1, 0); // Can do non-exact match of the param
// Can call something else on param2 now that I know it's address
// etc

This check makes sense to me, I understand the non-exact match you want to do.


Coming back to my mocker, it seems this is possible. Here is a snippet that mocks a contract that is expecting a call from Implementation.

Drop this in Remix and play with it.

import "https://github.com/cleanunicorn/mockprovider/blob/master/src/MockProvider.sol";

contract Implementation {
    address callInto;
    
    constructor(address callInto_) {
        callInto = callInto_; 
    }

    function foo() public {
        callInto.call(
            abi.encodeWithSelector(0x11223344, 1, 2, 3, 4)
        );
    }
}

contract TestImplementation {
    function test_foo() public {
        MockProvider provider = new MockProvider();

        provider.setDefaultResponse(MockProvider.ReturnData({
            success: true,
            data: ""
        }));

        Implementation implementation = new Implementation(provider);
        implementation.foo();

        MockProvider.ReturnData memory rd = provider.getCallData(0);

        (uint a, address b, uint c, uint d) = abi.decode(rd.data, (uint, address, uint, uint));

        // You can check each item individually
        // a > 0
        // b == 2
        // c not even
        // ...
    }
}

You can test each item (a, b, c, d) individually with your own rules.

@ultrasecreth
Copy link

Yeah, now I realised my example is a bit shit, but you totally nailed it, that's exactly what I'm talking about.

Having a look at the code and looks cool, my noob self didn't know that fallback could return stuff, hence my LenientMock being so limited 😅

For your first question, given the way the Java compiler and the JVM works, mockito allows you to create mocks without having to write specific classes for each one, pretty much what your lib does, albeit is waaaay simpler given the tools (or lack of) that solidity has.

I think your lib is an amazing starting point, but I also think is missing a few features, I'm all up for adding a few PRs if you like :)

Also not sure if the path is to merge that with forge-std or to keep it as a separate lib

Thoughts?

@cleanunicorn
Copy link

I think it's great that you want to contribute. I am not sure if the current interaction API format should be simplified.

In terms of being merged with forge-std, currently I am not sure. The ability to mock is something really useful.

If I start adding forge-specific features it could be merged, bit not required to. Otherwise it can be framework agnostic.

@mds1
Copy link
Collaborator Author

mds1 commented Apr 27, 2022

Thanks for the discussion @bbonanno and @cleanunicorn! 🙂

Here's one more mocking library, by @PARABRAHMAN0 (h/t @gakonst in the foundry telegram)

Would love to also get some thoughts from @maurelian and @PaulRBerg. Either way, I'll try to share my own thoughts / propose a path forward tomorrow in the next few days

@gakonst
Copy link
Member

gakonst commented Apr 27, 2022

yeah the UX with this mocking library is quite nice: https://github.com/OlympusDAO/test-utils/blob/master/src/test/mocking.t.sol#L36-L61
image

@ultrasecreth
Copy link

Nice, I didn't know Solidity had higher order functions!
I like this API, the downside is that you'd need all the permutations of input/output param types, so not sure how feasible it is.

@PaulRBerg
Copy link
Contributor

PaulRBerg commented May 3, 2022

I've just caught up on everything that has been discussed here. My thoughts:

  1. Mocking calls to contracts that do not exist is much more useful than mocking calls to deployed contracts.
    1. As per my test classification, a unit test is a unit test only if all other contracts are mocked, and writing unit tests is a good thing (I personally think that Ethereum devs focus too much on integration tests - lack of good mocking UX may play a role in that)
    2. If I put in the work to deploy a contract, I could just inherit from the original contract and write a test-specific version of it, rather than mock it.
  2. I don't think that mocking OOG errors is that useful and/ or common. When entrusting third-party contracts to perform some work, a good rule of thumb is to reserve a gas stipend (this is what I do in PRBProxy). I don't see why I'd be interested in throwing an out-of-gas in a third-party mock instead of honing in on my stipend calculations.
  3. An explicit reason (with function name, selector, and all) for why a mock call reverted would certainly be nice if it was provided by forge.

In terms of what mock contract to choose, my votes go to @cleanunicorn's MockProvider and OlympusDAO's mocking.sol. The latter's ability to compose .mock with the mocked function selector is awesome, certainly much better DX than creating a mock provider. But I'm not sure how robust and extensible it is - it looks like a testing gizmo they built for their internal needs at Olympus.

  • Mock contract size matters because it is going to be deployed a great many times and this may impact test runtime efficiency - this is why I wouldn't go with Gnosis' implementation.
  • @maurelian's UniMock is nice but looks very similar to MockProvider. I chose the latter simply because it seems better packaged as an open-source tool (dedicated repo + @cleanunicorn even wrote tests for it).
  • Waffle's Doppelganger.sol is okay, but Waffle has received little maintenance over the last year or so and it's unlikely that they will respond to any of our feature requests in timely manner.

@maurelian
Copy link

  • Just catching up here myself. I really appreciate the interest in unimock, it was fun to build and something I thought there was a need for.

That said I don't plan to maintain it, so anyone is welcome to copy the code into a new repo and run with it if they wish.
Otherwise it feels to me like @cleanunicorn's MockProvider is the natural choice, as it is already more mature, and he is actively supporting and improving it.

@mds1
Copy link
Collaborator Author

mds1 commented May 7, 2022

Awesome, yea I tend to agree with the last two responses in that:

  • The OlympusDAO lib is great UX, but it won't generalize well unfortunately since you need a lot of overloads
  • @cleanunicorn's lib is the way to go, especially appreciate his ongoing maintenance and improvements.

Admittedly I'm not the biggest user of mocks in forge so I'm not the best person to answer this, but here's my next two questions:

  1. How much of this belongs in forge-std, and what should we improve in foundry? For example, I can imagine updating vm.mockCall to (1) place code at an address if it doesn't exist, and (2) have an overload or vm.mockRevert cheatcode to to support mocking reverts. If that's done, is there still value in adding a mocking library to forge-std? My main concern around asking this is that too much stuff in forge-std will slow down compilation times since solidity compilation is slower than rust cheat codes.
  2. If we do integrate @cleanunicorn's lib:
    1. Where does the vm.mockCall cheatcode fit into dev workflows?
    2. Should the lib be a dependency or upstreamed here? Even if upstreamed here, I don't think it needs to have anything forge specific, so e.g. hardhat users could still use it by installing forge-std

@mds1
Copy link
Collaborator Author

mds1 commented Aug 3, 2022

Just an update here that as of foundry-rs/foundry#2576, foundry now automatically etches a single byte to empty accounts as part of vm.mockCall, so currently I'm uncertain of the value of adding a mocking library.

@fubhy
Copy link

fubhy commented Aug 12, 2022

Does it make sense to add a vm.mockCallForward cheatcode that lets you forward the call to a function in the local test class so that mocks can have some level of logic? So that instead of mocking a full contract, you could just forward a single call? That would've helped me in foundry-rs/foundry#2740 too

@ultrasecreth
Copy link

Does it make sense to add a vm.mockCallForward cheatcode that lets you forward the call to a function in the local test class so that mocks can have some level of logic? So that instead of mocking a full contract, you could just forward a single call? That would've helped me in foundry-rs/foundry#2740 too

I like it, it'd be the equivalent of mocking an answer, so I guess the param would be a function type reference to a public function in the same test? (not sure if the compiler would like that though)

@mds1
Copy link
Collaborator Author

mds1 commented Aug 12, 2022

Does it make sense to add a vm.mockCallForward cheatcode that lets you forward the call to a function in the local test class so that mocks can have some level of logic? So that instead of mocking a full contract, you could just forward a single call? That would've helped me in foundry-rs/foundry#2740 too

Sorry do you mind clarifying? Not sure I totally follow the idea here. Maybe an example would help.

@fubhy
Copy link

fubhy commented Aug 12, 2022

Does it make sense to add a vm.mockCallForward cheatcode that lets you forward the call to a function in the local test class so that mocks can have some level of logic? So that instead of mocking a full contract, you could just forward a single call? That would've helped me in foundry-rs/foundry#2740 too

Sorry do you mind clarifying? Not sure I totally follow the idea here. Maybe an example would help.

Yeah... Something like this:

contract MyTest is Test {
    // This would have the same i/o signature as the mocked function.
    function myMockFunction(address foo) public returns(string memory) {
        return "hello world!";
    }

    function testSomething() public {
        // Redirect calls to `123.foo` to `this.myMockFunction`
        vm.mockCall(address(123), "foo(address)", address(this), this.myMockFunction.selector);
        
        ...
    }
}

@mds1
Copy link
Collaborator Author

mds1 commented Aug 12, 2022

Ah I see. Though I don't follow see the use case for it / when would you need to do that? FWIW you can also vm.etch any code you want at address(123) to have methods return different data

@fubhy
Copy link

fubhy commented Aug 12, 2022

I had a case just now in an integration test where it was rather difficult to figure out exactly what values the function I wanted to mock would be called with and I wanted to return something based on the input. So yeah, add some minimal amount of dynamic logic to the test. I generally try to avoid that and normally would consider that a bad idea but it was hard to get around in this instance.

vm.etch overrides the entire code at the address. I just wanted to override a single function on the contract at that address (one that's called internally by the other contract).

@mds1
Copy link
Collaborator Author

mds1 commented Aug 12, 2022

Seems like a niche use case so I'm not sure it's worth a cheatcode, but an issue in the foundry repo would be the right place to discuss that.

vm.etch overrides the entire code at the address. I just wanted to override a single function on the contract at that address (one that's called internally by the other contract).

Yep, so you can take the original source, modify it, then compile and use deployCode / etch. (Admittedly that's a bit tedious and requires having the original source code)

@rpedroni
Copy link

Hey @mds1 ! (always fun to see you around here :)
Question regarding these mocks: Is it somehow possible to introspect the mocked function, similar to how jest does mocks?

Let's say I mock a specific contract/function and want to test that it was called N times, each time with xyz parameters, is this currently possible?

@mds1
Copy link
Collaborator Author

mds1 commented Mar 16, 2023

Hey! I'm not too familiar with how jest mocks work, but from that description I don't think it's currently possible, though there is a feature request for it: foundry-rs/foundry#4513.

@mds1
Copy link
Collaborator Author

mds1 commented Apr 3, 2023

Going to close this now that we have a mockCallRevert cheat (ref foundry-rs/foundry#4343 and #337), since that covers most use cases

@mds1 mds1 closed this as not planned Won't fix, can't repro, duplicate, stale Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants