Open mds1 opened 2 years ago
Ref #137 #1795
Also might complicate https://github.com/foundry-rs/foundry/issues/1980 because of command arg flattening, see https://github.com/foundry-rs/foundry/issues/1980#issuecomment-1160987510
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 changesforge inspect
to fill 2nd use caseSome other ideas for gas snapshots specifically can be found in https://github.com/foundry-rs/foundry/issues/137
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
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:
read-write
access rights to a folder in the project root .snapshots
, meant to be checked into .git
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/
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)
append mode
?)FORGE_SNAPSHOT_CHECK
environment variable to revert on a mismatch in the snapshot with gas usedSome of my concerns:
vm.snapshot
cheatcode relating to capturing state snapshots: https://github.com/foundry-rs/foundry/blob/af97b2c75cbcfaba23462998ae75ca082bcca1f2/crates/cheatcodes/spec/src/vm.rs#L522-L558lastCallGas
, pauseGasMetering
, resumeGasMetering
, the Gas
struct.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
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 -
testContractName.testFunction
as a default if not overridden with a specific string. Would have loved to do this with the library if we had introspectionvm.snapLastCall
which wraps lastCallGas
? Single call is a very common use case and would be nice to skip the wrapping steps
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 likeforge snapshot
currently does.forge snapshot size
is equivalent to runningforge build --sizes
and saving the output table.forge snapshot <fieldName>
would let you snapshot any solc field name supported byforge inspect
. By default this would snapshot the field for all contracts in thesrc
directoryInstead of saving to the project root, these commands save a file called
<property>.snapshot
to a folder named something likesnapshot
,snap
, orss
. Examples:forge snapshot gas
results insnapshot/gas.snapshot
forge snapshot storage-layout
results insnapshot/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
forge inspect
supports multiple variants for a given property. For example, you can see thatforge inspect MyContract storage
,forge inspect MyContract storageLayout
,forge inspect MyContract storage-layout
, andforge 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 isstorage
.Additional context
No response