-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 #8755
Conversation
crates/cheatcodes/src/evm.rs
Outdated
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recognize this is probably harder but would love if name could be optional and autogenerated as testContract.testFunction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supportive, I think it should be possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// -------- 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); | ||
function snapshotState() external returns (uint256 snapshotId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can should keep the old names and mark them as deprecated (#[cheatcode(... status = Deprecated)]
,
Deprecated, |
I don't believe the deprecated warnings logic is implemented yet, but it should be straight forward to implement. CC @mattsse @klkvr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-added the old cheatcodes and tagged them as Deprecated
, sharing the inner logic with the new implementation
crates/cheatcodes/src/evm.rs
Outdated
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess since BTreeMap is ordered on name
and presumably serde_json just iterates over it, it's likely deterministic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
319dbf1
to
f09dc7b
Compare
Going to split out the |
* 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 <[email protected]> * Update crates/cheatcodes/src/inspector.rs Co-authored-by: Arsenii Kulikov <[email protected]> * 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 <[email protected]>
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
(wrapslastCallGas
)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 thecontract name
as file name e.g.snapshots/GasSnapshotTest.json
:Where the order is always sorted alphabetically and deterministic.
If a
group
name is supplied, uses the custom name e.g.CustomGroup
(stored insnapshots/CustomGroup.json
)Supports
FORGE_SNAPSHOT_CHECK=true
flag to assert gas snapshots haven't changed across runsBreaking
snapshot
tosnapshotState
throughout the codebase in a breaking wayNotes
snapshots
): track internal gas usage #3766 we do not support tracking internal gas.To emulate internal gas tracking one could use:
Alternatively this could be added to
forge-std
?To do
FORGE_SNAPSHOT_CHECK=true
environment variable to revert on a mismatch in the snapshot with gas usedgroup
is passed automatically generate the snapshot group: for exampletestContractName
as a default if not overridden with a specific string.vm.snapshotGasLastCall
to wraplastCallGas
? Single call is a very common use case and would be nice to skip the wrapping stepsFollow 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.name
is passed automatically generate the snapshot name: for exampletestFunction
as a default if not overridden with a specific string.