feat: gas snapshots over arbitrary sections by zerosnacks · Pull Request #8755 · foundry-rs/foundry

zerosnacks · 2024-08-27T16:07:15Z

Motivation

Closes: #2065 + Closes: #2056 + Closes: #8713

Supports:

Uses a snapshots directory without . prefix to indicate it is meant to be checked in.

Cheatcodes:

snapshotValue
snapshotGas (wraps lastCallGas)
startSnapshotGas
stopSnapshotGas

All cheatcodes support grouping, allowing you to group arbitrary snapshots together by group name. If the group name is not supplied the name is used as filename.

We support multiple outputs:

If no group is supplied, uses the contract name as file name e.g. snapshots/GasSnapshotTest.json:

{
 "a": "123",
 "b": "456",
 "c": "789",
 "d": "123",
 "e": "456",
 "f": "789",
 "testSnapshotGasSection": "17439512"
}

Where the order is always sorted alphabetically and deterministic.

If a group name is supplied, uses the custom name e.g. CustomGroup (stored in snapshots/CustomGroup.json)

{
  "e": "456",
  "i": "456",
  "o": "123",
  "q": "789",
  "x": "123",
  "z": "789"
}

Supports FORGE_SNAPSHOT_CHECK=true flag to assert gas snapshots haven't changed across runs

Breaking

Updates generic snapshot to snapshotState throughout the codebase in a breaking way

Notes

As marked in feat(snapshots): track internal gas usage #3766 we do not support tracking internal gas.

To emulate internal gas tracking one could use:

    function testInternalSnapshotValue() public {
        uint256 a = gasleft();

        _run(1);

        uint256 b = gasleft();

        vm.snapshotValue("testInternalSnapshotValue", a - b);
    }

Alternatively this could be added to forge-std?

Includes some formatting fixes raised by the Solidity compiler as warnings that were causing issues

To do

Snapshots are currently not cleared by themselves at the start of running the test suite. It seems desirable that this would be the case to prevent staining
A FORGE_SNAPSHOT_CHECK=true environment variable to revert on a mismatch in the snapshot with gas used
If no group is passed automatically generate the snapshot group: for example testContractName as a default if not overridden with a specific string.
Add vm.snapshotGasLastCall to wrap lastCallGas? Single call is a very common use case and would be nice to skip the wrapping steps

Follow up

Deriving function names automatically is quite complex as the test being ran isn't aware of the function specific context, only the contract.

I've set it up in a way (w/ name: Option<String>) to easily add it in a follow-up PR in a non-breaking way.

If no name is passed automatically generate the snapshot name: for example testFunction as a default if not overridden with a specific string.

crates/cheatcodes/spec/src/vm.rs

marktoda · 2024-08-27T19:41:29Z

crates/cheatcodes/src/evm.rs

+        create_dir_all(ccx.state.config.paths.snapshots.clone())?;
+
+        // Write the snapshot to a file
+        let snapshot_path = ccx.state.config.paths.snapshots.join(name).join(".json");


would be nice to have a check for multiple snapshots with the same name (common mistake causing confusing snapshot thrashing based on test runner ordering)

recognize this is probably harder but would love if name could be optional and autogenerated as testContract.testFunction

Supportive, I think it should be possible

I was able to add support for generating group names by contract name but haven't figured out a way to support derived function names yet. This can be added as a follow-up PR in a non-breaking way once a good way has been found.

mds1 · 2024-08-28T14:54:59Z

crates/cheatcodes/spec/src/vm.rs

    /// To revert a snapshot use `revertTo`.
    #[cheatcode(group = Evm, safety = Unsafe)]
-    function snapshot() external returns (uint256 snapshotId);
+    function snapshotState() external returns (uint256 snapshotId);


I agree with this set of name change for state snapshots, since the term "snapshot" is now broad, but just noting this is a breaking change so we'll need to make sure to communicate that

We can should keep the old names and mark them as deprecated (#[cheatcode(... status = Deprecated)],

foundry/crates/cheatcodes/spec/src/cheatcode.rs

Line 48 in cb109b1

Deprecated,

), these kinds of breaking changes are not a good experience.

I don't believe the deprecated warnings logic is implemented yet, but it should be straight forward to implement. CC @mattsse @klkvr

Re-added the old cheatcodes and tagged them as Deprecated, sharing the inner logic with the new implementation

marktoda · 2024-08-28T15:28:50Z

crates/cheatcodes/src/evm.rs

+        snapshot.insert(name.clone(), value);
+
+        // Write the snapshot to a file, asserting that the write was successful.
+        let result = write_pretty_json_file(&snapshot_path, &snapshot).is_ok();


Love the idea of groups - one thought is, does BTreeMap serialize deterministically? Given test run ordering isn't deterministic afaik, worried thrashing of snapshot output will make diffs more difficult to read. Would ideally have deterministic output

I guess since BTreeMap is ordered on name and presumably serde_json just iterates over it, it's likely deterministic

Good point!

Given that upon applying an update the json is read into a BTreeMap, changes are applied and then serialized again the order should be deterministic. I'll need to add more tests to confirm this assumption is correct, but that is the goal.

crates/cheatcodes/spec/src/vm.rs

crates/cheatcodes/src/evm.rs

crates/cheatcodes/src/inspector.rs

…d stopSnapshotGas()

zerosnacks · 2024-09-23T17:27:19Z

Going to split out the state snapshot renaming into its own PR as it makes it difficult to iterate and review here

zerosnacks · 2024-09-24T14:48:14Z

Closing in favor of #8952

I've split out the work in this PR into: #8945 (snapshot renaming) and #8952 (gas snapshots over sections) that branches off #8945. This will make it easier to review and collaborate.

* update internal naming * further internals * deprecate cheats * update Solidity tests and add dedicated test for testing deprecated cheatcodes * clarify gas snapshots * fix build * final fixes * fix build * fix repro 6355 rename * add gas snapshot setup from #8755 * fix build + clippy warnings * fix cheatcodes * account for fixed CREATE / CALL gas cost * remove import * add stipend * recalculate after a - b setup * clear call_stipend, update tests * avoid double counting external calls * update cheatcodes, remove debug prints * enable assertions * clean up tests * clean up test names * remove snapshot directory on `forge clean` * do not remove all snapshots by default due to multiple test suites being able to be ran concurrently or sequentially + optimize gas snapshots check - skip if none were captured * handle edge case where we ask to compare but file does not exist, remove snapshot directory at a top level before test suites are ran * fix path issue when attempting removal * Update crates/cheatcodes/src/evm.rs Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com> * Update crates/cheatcodes/src/inspector.rs Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com> * refactor, apply recommended changes for last_snapshot_group, last_snapshot_name * remove gas snapshots from fuzz tests for now: this is largely due to it conflicting with the FORGE_SNAPSHOT_CHECK where it is highly likely that with different fuzzed input the gas measurement differs as well. In the future it would be an idea to capture the average gas * fix clippy * avoid setting to 0 unnecessarily * use if let Some * improve comments, clarify use of last_gas_used != 0 * fix merge conflict issue * fix arg ordering to address group naming regression * fix import * move snapshot name derivation to helper * only skip initial call w/ overhead, no special handling for call frames * add flare test * style nits + use helper method --------- Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>

zerosnacks added 10 commits August 6, 2024 11:50

snapshot -> snapshotState (breaking change)

5f1b49b

specify snapshot as state_snapshot to make room for gas_snapshot logic

43bb19c

further internal changes in preperation of snapshot_gas

a868b43

start adding gas snapshot logic

91ff76d

add basic writing to file

c36d74d

remove snapshot post dump

124303c

simplify, gas_end inline

267a244

add file

3daf782

update from master

9455c63

fix upstream

4c87e1b

zerosnacks changed the title ~~feat: gas snapshots~~ feat: gas snapshots over arbitrary section Aug 27, 2024

zerosnacks changed the title ~~feat: gas snapshots over arbitrary section~~ feat: gas snapshots over arbitrary sections Aug 27, 2024

marktoda reviewed Aug 27, 2024

View reviewed changes

zerosnacks added 7 commits August 28, 2024 10:12

fix merge issues

a20af2d

fix Solidity compiler warnings and failing tests

1b522d6

remove snapshotData

c788ddd

add basic tests

86c5aab

add correct gas capturing

610f255

basic tests

2dff964

add basic group support

7eb0097

mds1 reviewed Aug 28, 2024

View reviewed changes

zerosnacks added 2 commits August 28, 2024 15:18

output in formatted json, add basic abstraction for grouping

b83d22b

simplify and cleanup

07cd7f3

marktoda reviewed Aug 28, 2024

View reviewed changes

zerosnacks added 2 commits August 28, 2024 15:35

fix clippy

fd83fe3

fix forge fmt

3d7013e

zerosnacks commented Aug 28, 2024

View reviewed changes

crates/cheatcodes/spec/src/vm.rs Show resolved Hide resolved

zerosnacks added 3 commits August 28, 2024 16:18

assert that alphabetic ordering is maintained

1180361

prepare for context aware derived names

b8453db

prepare for context aware group and name generation

22f3af4

zerosnacks added 3 commits September 10, 2024 13:19

clean up

ad1dfdc

only measure top-level calls, may be naive

d902a0d

record internal calls

ebc4efd

zerosnacks mentioned this pull request Sep 12, 2024

feature --gas-report-diff #8774

Closed

experiment with tracking

e966de1

zerosnacks mentioned this pull request Sep 17, 2024

feat(cheatcodes): random* cheatcodes to aid in symbolic testing #8882

Merged

zerosnacks added 5 commits September 20, 2024 15:05

fix merge conflict

87cedfd

fix formatter

a38f087

update cheats

3b234ed

start tracking depth

222c5c7

add measurement to create_end

f9cecae

klkvr requested changes Sep 23, 2024

View reviewed changes

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved

crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved

crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved

zerosnacks added 4 commits September 23, 2024 14:58

process feedback

7556786

fix depth

45db733

fix frame recording, remove record_frames

7d8b793

clean up

f09dc7b

zerosnacks force-pushed the zerosnacks/snapshots branch from 319dbf1 to f09dc7b Compare September 23, 2024 14:41

zerosnacks added 4 commits September 23, 2024 17:19

make sure to clean up gas records after processing

dc32333

fix bug, allow to close snapshot that use custom group using shorthan…

29c78da

…d stopSnapshotGas()

add additional tests and comparisons

1bc6e54

register overhead

98c8760

This was referenced Sep 24, 2024

chore: rename snapshot to be more specific #8945

Merged

feat(forge test): generate gas report of all functions in contract without the need to specify a test function for each #8713

Closed

zerosnacks added a commit that referenced this pull request Sep 24, 2024

add gas snapshot setup from #8755

9602aec

zerosnacks mentioned this pull request Sep 24, 2024

feat: gas snapshots over arbitrary sections #8952

Merged

6 tasks

zerosnacks closed this Sep 24, 2024

zerosnacks deleted the zerosnacks/snapshots branch October 4, 2024 07:55

Conversation

zerosnacks commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Supports:

Breaking

Notes

To do

Follow up

Uh oh!

Uh oh!

marktoda Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

marktoda Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

zerosnacks Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

zerosnacks Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

DaniPopes Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zerosnacks Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

marktoda Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marktoda Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

zerosnacks Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zerosnacks commented Sep 23, 2024

Uh oh!

zerosnacks commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zerosnacks commented Aug 27, 2024 •

edited

Loading

zerosnacks Sep 3, 2024 •

edited

Loading

DaniPopes Sep 3, 2024 •

edited

Loading

marktoda Aug 28, 2024 •

edited

Loading

zerosnacks commented Sep 24, 2024 •

edited

Loading