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: expand snapshot functionality #2056

Closed
mds1 opened this issue Jun 21, 2022 · 6 comments · Fixed by #8952
Closed

feat: expand snapshot functionality #2056

mds1 opened this issue Jun 21, 2022 · 6 comments · Fixed by #8952
Assignees
Labels
A-gas-snapshots Area: gas snapshotting/reporting C-forge Command: forge P-high Priority: high T-feature Type: feature
Milestone

Comments

@mds1
Copy link
Collaborator

mds1 commented Jun 21, 2022

Component

Forge

Describe the feature you would like

Inspired by this twitter convo

Feature Spec

Right now forge snapshot saves the gas usage of each test to a file called .gas-snapshot. This is useful for things like gas golfing, or ensuring that a changeset does not increase gas costs. Similar functionality can be applied to other properties of a codebase.

I propose modifying forge snapshot as follows:

Instead of being a single command, it now has subcommands to snapshot various things:

  • forge snapshot gas behaves like forge snapshot currently does.
  • forge snapshot size is equivalent to running forge build --sizes and saving the output table.
  • forge snapshot <fieldName> would let you snapshot any solc field name supported by forge inspect. By default this would snapshot the field for all contracts in the src directory

Instead of saving to the project root, these commands save a file called <property>.snapshot to a folder named something like snapshot, snap, or ss. Examples:

  • forge snapshot gas results in snapshot/gas.snapshot
  • forge snapshot storage-layout results in snapshot/storage-layout.snapshot

The --check, --diff, and --snap options should be supported for each subcommand and behave the same as they do now. Some subcommands may have additional options, such as:

  • --include-fuzz-tests for gas snapshots, or always including fuzz tests and just specifying the seed here
  • --asc and --desc should be supported for ones where sorting makes sense, such as gas and size snapshots.

Open questions

  1. Are we ok with implementing this breaking change to snapshot behavior without waiting for a specific breaking change foundry release? Let's get thoughts from snapshot users, but IMO this is useful enough and a small enough breaking change that we should be ok with it.
  2. forge inspect supports multiple variants for a given property. For example, you can see that forge inspect MyContract storage, forge inspect MyContract storageLayout, forge inspect MyContract storage-layout, and forge inspect MyContract storage_layout all print the storage layout. We'd want to standardize on which name we use for the output files. My suggestion is using the JSON field name output by solc, which for this example I believe is storage.

Additional context

No response

@mds1 mds1 added the T-feature Type: feature label Jun 21, 2022
@onbjerg onbjerg added this to Foundry Jun 21, 2022
@onbjerg onbjerg moved this to Todo in Foundry Jun 21, 2022
@onbjerg
Copy link
Member

onbjerg commented Jun 21, 2022

Ref #137 #1795

Also might complicate #1980 because of command arg flattening, see #1980 (comment)

Edit: Another consideration is that snapshot files are nameable right now, so it's possible to have e.g. a .fuzz-snapshot and a .unit-snapshot that you diff/check separately and we should probably preserve that behavior

One final consideration is that there's been some talk of merging snapshot/gas report but I can't find the convo

Edit 2: Given all above points an alternative suggestion (no strong preference, just wanted to add context to the issue):

  • forge snapshot/gas report merge. Gas reports already support writing to files, so tweaking the layout and adding diff/check should be sufficient to become up to date. For a while forge snapshot could be an alias of gas reporting so we don't have breaking changes
  • Introduce diff/check/save to file behavior for forge inspect to fill 2nd use case

@onbjerg onbjerg added C-forge Command: forge A-gas-snapshots Area: gas snapshotting/reporting labels Jun 21, 2022
@maurelian
Copy link

Only comment is that we'd definitely love to see this given I was speccing out a similar script for ourselves around the time this issue was created. For now I will likely follow the approach used by nomad.

@mds1
Copy link
Collaborator Author

mds1 commented May 18, 2023

Some other ideas for gas snapshots specifically can be found in #137

@zerosnacks
Copy link
Member

Would be great to upstream the core ideas from here: https://github.com/marktoda/forge-gas-snapshot as used here: https://github.com/Uniswap/v4-core/tree/main/.forge-snapshots

@zerosnacks
Copy link
Member

zerosnacks commented Jul 18, 2024

Considering the growing popularity of https://github.com/marktoda/forge-gas-snapshot (used in Uniswap V4, Balancer V3, Euler V2) I've proposed to add native functionality for its feature set.

There is rough consensus on moving ahead with the following changes:

  • Add new read-write access rights to a folder in the project root .snapshots, meant to be checked into .git
  • Adding entries to the foundry.toml configuration to complement the changes (such as .snapshot folder name / location)
  • Enable the (over)writing of individual snaps into the .snapshots folder (`.snapshots/.snap) through a number of cheatcodes:
    • Clear:

      • vm.snapStart(string calldata name)
      • vm.snapEnd() (ends the most recently opened snap)
    • Possibly:

      • vm.snap(string calldata name, uint256 value)
      • vm.snapSize(string calldata name, address target)
    • Optionally (if possible):

      • vm.snap(string calldata name, function() external fn) (snapshot a given external closure)
      • vm.snap(string calldata name, function() internal fn) (snapshot a given internal closure)
  • Possibly adding support for grouping related snapshots into a single file (append mode?)
  • A FORGE_SNAPSHOT_CHECK environment variable to revert on a mismatch in the snapshot with gas used

Some of my concerns:

  • The current existence of the vm.snapshot cheatcode relating to capturing state snapshots:
    // -------- State Snapshots --------
    /// Snapshot the current state of the evm.
    /// Returns the ID of the snapshot that was created.
    /// To revert a snapshot use `revertTo`.
    #[cheatcode(group = Evm, safety = Unsafe)]
    function snapshot() external returns (uint256 snapshotId);
    /// Revert the state of the EVM to a previous snapshot
    /// Takes the snapshot ID to revert to.
    ///
    /// Returns `true` if the snapshot was successfully reverted.
    /// Returns `false` if the snapshot does not exist.
    ///
    /// **Note:** This does not automatically delete the snapshot. To delete the snapshot use `deleteSnapshot`.
    #[cheatcode(group = Evm, safety = Unsafe)]
    function revertTo(uint256 snapshotId) external returns (bool success);
    /// Revert the state of the EVM to a previous snapshot and automatically deletes the snapshots
    /// Takes the snapshot ID to revert to.
    ///
    /// Returns `true` if the snapshot was successfully reverted and deleted.
    /// Returns `false` if the snapshot does not exist.
    #[cheatcode(group = Evm, safety = Unsafe)]
    function revertToAndDelete(uint256 snapshotId) external returns (bool success);
    /// Removes the snapshot with the given ID created by `snapshot`.
    /// Takes the snapshot ID to delete.
    ///
    /// Returns `true` if the snapshot was successfully deleted.
    /// Returns `false` if the snapshot does not exist.
    #[cheatcode(group = Evm, safety = Unsafe)]
    function deleteSnapshot(uint256 snapshotId) external returns (bool success);
    /// Removes _all_ snapshots previously created by `snapshot`.
    #[cheatcode(group = Evm, safety = Unsafe)]
    function deleteSnapshots() external;
  • We have a lot of cheatcodes dealing with gas: lastCallGas, pauseGasMetering, resumeGasMetering, the Gas struct.
  • Not a huge fan of the snap* naming but the cheatcode has to be short and memorable as developers will likely add it throughout their tests (https://github.com/search?q=repo%3AUniswap%2Fv4-core%20snapStart&type=code)

@mds1 curious on your thoughts about this proposal

@marktoda
Copy link
Contributor

Thanks for proposing this @zerosnacks - I'm (obviously) very in favor of upstreaming this into Foundry. For a bit more context, the primary reason we prefer this type of snapshotting at Uniswap is to get maximal granularity, allowing us to check how the gas of specific flows change over time in version control.

Generally I think the approach makes sense. A couple ideas -

  • Possible to automatically generate the snapshot name? For example testContractName.testFunction as a default if not overridden with a specific string. Would have loved to do this with the library if we had introspection
  • Possibly merge all snaps into files by test contract? The file-per-snap gets a bit unwieldy, you can see we have dozens of files in v4 snaps dir
  • Potentially add vm.snapLastCall which wraps lastCallGas? Single call is a very common use case and would be nice to skip the wrapping steps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-gas-snapshots Area: gas snapshotting/reporting C-forge Command: forge P-high Priority: high T-feature Type: feature
Projects
Status: Completed
5 participants