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

feat: snapshot and revert cheatcodes #1162

Closed
jparklev opened this issue Apr 1, 2022 · 16 comments · Fixed by #1715
Closed

feat: snapshot and revert cheatcodes #1162

jparklev opened this issue Apr 1, 2022 · 16 comments · Fixed by #1715
Assignees
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-test Command: forge test D-hard Difficulty: hard P-low Priority: low T-feature Type: feature
Milestone

Comments

@jparklev
Copy link
Contributor

jparklev commented Apr 1, 2022

Component

Forge

Describe the feature you would like

Cheatcodes that allow you to take a snapshot, then revert the executer's state back to the snapshot. One possible API is:

bytes32 id = vm.snapshot();

// do something

vm.revertTo(id);

which would be similar to Ganache's evm_snapshot/evm_revert

The use case is to do some state changing operation(s), derive a value, then revert the changes (similar to using callStatic using ethers). It feels like the tricky part might be to revert state while preserving values assigned in the test contract before the reversion, but it's been a while since I looked through foundry's codebase ~

@hexonaut I think you mentioned this in chat a while ago as well?

Additional context

Example uses include:

  • AMM effective price "pseudo swaps" (determine the price from a swap before doing it)
  • Seeing how much interest would be collected w.o actually collecting it
  • Check the effects of a gov proposal on a mainnet fork w.o executing it

I know some people have achieved the same effect by passing a value in as a revert message and catching it, a bit like uni's quoter v2 does, but this feature would enable a cleaner pattern

@jparklev jparklev added the T-feature Type: feature label Apr 1, 2022
@hexonaut
Copy link
Contributor

hexonaut commented Apr 1, 2022

Yes I mentioned this. You can do this without cheatcodes via a revert though.

@jparklev
Copy link
Contributor Author

jparklev commented Apr 1, 2022

Yes I mentioned this. You can do this without cheatcodes via a revert though.

Yea, think it's not the best pattern, but certainly could be that it serves ppl's needs well enough for the moment. Out of curiosity, what's the use case you have?

@onbjerg
Copy link
Member

onbjerg commented Apr 1, 2022

This is pretty hard to do, and if it's solveable without cheatcodes, I'm not sure it's a good idea for us to implement it given the complexity. It might be better to publish a package that has helpers for this instead. cc @gakonst on thoughts on this

In terms of your two first examples as well, is there any reason you would not expose information like that in a view function? And for the third, I'm not sure I completely understand - can you elaborate?

@onbjerg onbjerg added Cmd-forge-test Command: forge test C-forge Command: forge A-cheatcodes Area: cheatcodes P-low Priority: low D-hard Difficulty: hard labels Apr 1, 2022
@jparklev
Copy link
Contributor Author

jparklev commented Apr 1, 2022

This is pretty hard to do, and if it's solveable without cheatcodes, I'm not sure it's a good idea for us to implement it given the complexity. It might be better to publish a package that has helpers for this instead

Fair, tbh if we can solve it with a helper function then agreed that meets the need and feels better

In terms of your two first examples as well, is there any reason you would not expose information like that in a view function?

Yea, some projects choose not to add a second path for view-only. I know uni v3 requires you to do the revert to get the price (zefram tweet), whereas balancer has a parallel view path

And for the third, I'm not sure I completely understand - can you elaborate?

yea, was thinking: fork mainnet -> execute maker gov proposal e.g. (which changes parameters) -> take some action (pay back dai debt) and read the resulting state -> revert back and use this value for some check. In fairness, this example is more contrived than the others and I haven't run into it personally

@emilianobonassi
Copy link

+1 for this, missing too when migrated from brownie

I am actually using the following workaround based on try/catch and external contracts (contexts)

It leverages an ancillary contract where you have to write (test) block statements where you revert

Fully solidity no cheat codes

pragma solidity ^0.8.0;

import {DSTest} from "ds-test/test.sol";

contract SimpleContext {
    uint256 public number;

    function setAndRevert(uint256 num) external {
        number = num;
        revert("ouch");
    }
}

contract SharedSetup is DSTest {
    SimpleContext context;

    function setUp() public {
        context = new SimpleContext();
    }

    function testSnapshot() external {
        uint256 preNum = context.number();
        uint256 num = 5;
        try context.setAndRevert(num) {} catch {
            assertEq(context.number(), preNum);
        }
    }
}

@hexonaut
Copy link
Contributor

hexonaut commented Apr 2, 2022

Here is a use case: https://github.com/makerdao/dss-cron/blob/add-oracle-mom/src/ClipperMomJob.sol#L73

Basically when you have to run something to see if it will succeed or not and you don't want it to actually execute in the tests. In the example above workable is expected to be called as a view by the keeper to see if this job is executable or not.

@gakonst gakonst changed the title feat: snapshot and revet cheatcodes feat: snapshot and revert cheatcodes Apr 2, 2022
@onbjerg
Copy link
Member

onbjerg commented Apr 3, 2022

This is extremely hard. I'm not sure how we would revert mid-execution and then start a new execution from a specific point. No real way to do that in REVM right now, so we'd have to figure out a way to add that upstream as well. I'm leaning heavily towards keeping this as a community utility contract instead of adding the cheatcodes.

We'd need to:

  • Snapshot the memory
  • Snapshot the stack
  • Snapshot all addresses + storage
  • When reverting, check if we reverted because of this cheatcode, get the snapshot, start a new execution but set the state to what we saved + the program counter to after the revert

We have a way to snapshot the memory and the stack, no real way to snapshot all the addresses and storage, and no real way to resume execution at some program counter

It would add a lot of complexity both in REVM and Foundry, so again, really hesitant - I understand the use case, but since this can be solved using try/catch, I don't see a good reason to add it given the complexity.

It would also not work well with DSTest, which could lead to a lot of confusion. For example, if you make a DSTest assertion that fails, and then call vm.revertTo(id), then that assertion is effectively ignored.

cc @gakonst and @mattsse for thoughts on this

@onbjerg
Copy link
Member

onbjerg commented Apr 3, 2022

Also cc @rakita if there is a non-complicated way to implement this with how REVM currently is - maybe you have some ideas :)

@emo-eth
Copy link
Contributor

emo-eth commented May 19, 2022

Thanks for linking this existing issue @onbjerg.
Would like to add that try/catch isn't currently suitable replacement when running differential tests, because assertions made within a try block that reverts don't count as failures.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.13;
import { Test } from "forge-std/Test.sol";

interface Impl {
    function fn() external;
}

contract ImplA is Impl {
    function fn() external {
        // do something
    }
}

contract ImplB is Impl {
    function fn() external {
        // do something similar
    }
}

contract DifferentialTest is Test {
    bytes32 PASS_HASH =
        0x092ff997991b7da0fc51f1dcc2bf12ccc1e998582e69bdc4232e5b6115713272;
    Impl implA;
    Impl implB;

    function setUp() public {
        implA = new ImplA();
        implB = new ImplB();
    }

    function testDifferential() public {
        try this.differentialLogic(implA) {} catch (bytes memory reason) {
            assertEq(keccak256(reason), PASS_HASH);
        }
        try this.differentialLogic(implB) {} catch (bytes memory reason) {
            assertEq(keccak256(reason), PASS_HASH);
        }
    }

    function differentialLogic(Impl impl) public {
        impl.fn();
        assertTrue(false);
        revert("PASS");
    }
}
Running 1 test for test/foundry/DifferentialTest.sol:DifferentialTest
[PASS] testDifferential() (gas: 35700)
Logs:
  Error: Assertion Failed
  Error: Assertion Failed

@onbjerg
Copy link
Member

onbjerg commented May 19, 2022

Are you sure? They should - does the reason actually match?

@emo-eth
Copy link
Contributor

emo-eth commented May 20, 2022

[PASS] testDifferential() (gas: 35700)
Logs:
  Error: Assertion Failed
  Error: Assertion Failed

Traces:
  [35700] DifferentialTest::testDifferential() 
    ├─ [13937] DifferentialTest::differentialLogic(ImplA: [0xce71065d4017f316ec606fe4422e11eb2c47c246]) 
    │   ├─ [98] ImplA::fn() 
    │   │   └─ ← ()
    │   ├─ emit log(: "Error: Assertion Failed")
    │   ├─ [0] VM::store(VM: [0x7109709ecfa91a80626ff3989d68f67f5b1dd12d], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001) 
    │   │   └─ ← ()
    │   └─ ← "PASS"
    ├─ [13937] DifferentialTest::differentialLogic(ImplB: [0x185a4dc360ce69bdccee33b3784b0282f7961aea]) 
    │   ├─ [98] ImplB::fn() 
    │   │   └─ ← ()
    │   ├─ emit log(: "Error: Assertion Failed")
    │   ├─ [0] VM::store(VM: [0x7109709ecfa91a80626ff3989d68f67f5b1dd12d], 0x6661696c65640000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000001) 
    │   │   └─ ← ()
    │   └─ ← "PASS"
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 971.38µs

@onbjerg Yes, test passes fine even though the assertions in differentialLogic are logged as failing. Think this is a bug?

Edit: Looks like this is because the failure is getting stored to the HEVM address, which will also be reverted. Can probably revert with the value at that slot instead and check if it's true.

Edit 2: Reverting with HEVM storage slot and checking that the value isn't true seems to work nicely, actually!

@rakita
Copy link
Contributor

rakita commented May 20, 2022

This is extremely hard. I'm not sure how we would revert mid-execution and then start a new execution from a specific point. No real way to do that in REVM right now, so we'd have to figure out a way to add that upstream as well. I'm leaning heavily towards keeping this as a community utility contract instead of adding the cheatcodes.

We'd need to:

  • Snapshot the memory
  • Snapshot the stack
  • Snapshot all addresses + storage
  • When reverting, check if we reverted because of this cheatcode, get the snapshot, start a new execution but set the state to what we saved + the program counter to after the revert

We have a way to snapshot the memory and the stack, no real way to snapshot all the addresses and storage, and no real way to resume execution at some program counter

It would add a lot of complexity both in REVM and Foundry, so again, really hesitant - I understand the use case, but since this can be solved using try/catch, I don't see a good reason to add it given the complexity.

It would also not work well with DSTest, which could lead to a lot of confusion. For example, if you make a DSTest assertion that fails, and then call vm.revertTo(id), then that assertion is effectively ignored.

cc @gakonst and @mattsse for thoughts on this

Just saw this. :)

I see, this seems tricky to do and covers all edge cases, for account+storage you could 'just' clone SubRoutine that is where storage changes are, but you will need to be aware of where snapshot is called, it is hard to jump from one contract subroutine to another, selfdestruct is one more thing you need to be aware.

Maybe we can do it in another way to leverage EVM revert mechanism, do something as breakpoint system:

  • Set "breakpoint" on vm.snapshot(); and on vm.revertTo(id)
  • with vm.revertTo(id) trigger the revert of a call and do some magic to catch that revert and recall the contract again. Basically in step_end check if returned from call/create or not, decrease program counter to one item before so that it is executed again, but you will need to push input arguments again on stack :D
  • Reexecute all contracts until breakpoint on vm.snapshot() and then jump to the breaking point of revertTo by setting PC to it.
  • It needs to be available only in same contract call
  • Yeah, this is getting complicated :D

@onbjerg
Copy link
Member

onbjerg commented May 23, 2022

@jameswenzel I think you want catch Error(string memory reason), going by the Solidity docs?

@tynes
Copy link
Contributor

tynes commented Jun 7, 2022

It would be amazing to be able to access the state diff since the last snapshot. The UX that I'm thinking of:

  • spin up an anvil fork
  • take a snapshot
  • deploy upgrades
  • access state diff

Then the state diff could be looked at line by line to ensure that the upgrade is exactly what is desired

@gakonst
Copy link
Member

gakonst commented Jun 8, 2022

@tynes you can already do that with the vm.record & vm.accesses cheat codes https://twitter.com/devtooligan/status/1532763122259271681

@onbjerg
Copy link
Member

onbjerg commented Jun 9, 2022

As a note here the foundational work for this feature is being worked on by @mattsse in #1715

@onbjerg onbjerg added this to the v1.0.0 milestone Jul 1, 2022
@onbjerg onbjerg linked a pull request Jul 1, 2022 that will close this issue
3 tasks
@onbjerg onbjerg moved this from Todo to In Progress in Foundry Jul 1, 2022
Repository owner moved this from In Progress to Done in Foundry Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-test Command: forge test D-hard Difficulty: hard P-low Priority: low T-feature Type: feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants