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: gas snapshots over arbitrary sections #8952

Merged
merged 56 commits into from
Oct 2, 2024

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Sep 24, 2024

Motivation

Supersedes #8755

Renaming of state_snapshot handled in #8945

This has been done so that the gas snapshot implementation can be reviewed more easily without being cluttered by renamings.

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

Supports:

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

Cheatcodes:

  • snapshotValue
  • snapshotGasLastCall (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"
}

In terms of accuracy this is a slight difference between the cheatcode implementation (A) and the native gasleft() implementation (B). I've added an assertion check in the test suite to make sure this does not drift further unexpectedly.

{
  "testGasComparisonCreateA": "99168",
  "testGasComparisonCreateB": "99168",
  "testGasComparisonEmptyA": "0",
  "testGasComparisonEmptyB": "0",
  "testGasComparisonExternalA": "26776",
  "testGasComparisonExternalB": "26776",
  "testGasComparisonFlareA": "5821936",
  "testGasComparisonFlareB": "5821936",
  "testGasComparisonInternalColdA": "22100",
  "testGasComparisonInternalColdB": "22106",
  "testGasComparisonInternalWarmA": "100",
  "testGasComparisonInternalWarmB": "106",
  "testGasComparisonNestedCallsA": "31955",
  "testGasComparisonNestedCallsB": "31955"
}

Supports FORGE_SNAPSHOT_CHECK=true flag to assert gas snapshots haven't changed across runs. By default on every invocation of forge test the snapshot directory is cleared out to prevent stale snapshots. To be able to perform the diff check this is disabled when the FORGE_SNAPSHOT_CHECK is set.

Screenshot from 2024-09-02 14-11-08

Tasks

  • 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
  • Add accurate internal gas tracking
  • Add gas snapshot clean up to forge clean

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.

  • Currently it is not possible to start / nest multiple snapshots without first closing the started snapshot. This could be added in a follow-up, see: feat: gas snapshots over arbitrary sections  #8952 (comment)

  • Currently gas snapshots are only supported in unit tests, not fuzz tests. 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.

  • Possible add snapshotGasNextCall to mirror vm.snapshotGasLastCall for improved user experience

testdata/foundry.toml Outdated Show resolved Hide resolved
@zerosnacks
Copy link
Member Author

zerosnacks commented Sep 30, 2024

There appears to have been some kind of regression related to grouping, temporarily moving back into draft

Resolved

@zerosnacks zerosnacks marked this pull request as draft September 30, 2024 09:21
@zerosnacks zerosnacks marked this pull request as ready for review September 30, 2024 09:45
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, have couple of nits and question re calling meter_gas_rrecord in step_end

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
// Check for differences in gas snapshots if `FORGE_SNAPSHOT_CHECK` is set.
// Exiting early with code 1 if differences are found.
if std::env::var("FORGE_SNAPSHOT_CHECK").is_ok() {
let differences_found = gas_snapshots.clone().into_iter().fold(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for fmt we use TextDiff::from_lines and then compute ratio to figure out if files differs / should be written to disk, wonder if it would be suitable to use same here

let diff = TextDiff::from_lines(&source, &output);
let new_format = diff.ratio() < 1.0;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I think this could be added as a future optimization, to only write snapshots that differ. Combine this with a glob over the files in the snapshots folder to filter out unmatched snapshots that only exist in the old and not in the new (to avoid staining).

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some stlye nits, otherwise lgtm

pending @klkvr and @grandizzy

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
5 participants