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

chore: rename snapshot to be more specific #8945

Merged
merged 12 commits into from
Sep 27, 2024

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Sep 24, 2024

Motivation

Goal is to clarify the difference between forge snapshot, state snapshots and gas snapshots as we re-use the terminology. This is in preparation for #8952.

Splits out the snapshot renaming incl. cheatcode deprecation (with shared implementation) originally part of #8755 as there was consensus on this proposed change and it de-clutters the PR making it easier to review.

The new gas snapshot PR can be found here: #8952 and branches off of here.

Related:

Solution

Should include no user-facing breaking changes, yields deprecation warning as added here: #8883

@zerosnacks zerosnacks changed the title chore: rename state snapshot chore: rename snapshot to be more specific Sep 24, 2024
@zerosnacks zerosnacks marked this pull request as ready for review September 24, 2024 10:31
zerosnacks added a commit that referenced this pull request Sep 24, 2024
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, code refactored to reflect types of snapshot, cheatcodes marked as deprecated with replacement / helper fns to use same logic for both new and deprecated cheatcodes, no change in forge snapshot cmd syntax

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.

all of these seem fine,

the deprecation notice is great and this doesn't break afaict, but ptal @DaniPopes

Comment on lines +513 to +515
/// `snapshot` is being deprecated in favor of `snapshotState`. It will be removed in future versions.
#[cheatcode(group = Evm, safety = Unsafe, status = Deprecated(Some("replaced by `snapshotState`")))]
function snapshot() external returns (uint256 snapshotId);
Copy link
Member

Choose a reason for hiding this comment

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

seems fine

@zerosnacks
Copy link
Member Author

zerosnacks commented Sep 27, 2024

cc @DaniPopes would be great to get this reviewed / merged if you find time for it, thanks!

@mattsse mattsse merged commit dd86b30 into master Sep 27, 2024
21 checks passed
@mattsse mattsse deleted the zerosnacks/rename-state-snapshot branch September 27, 2024 17:43
@DaniPopes
Copy link
Member

Sorry, forgot, LGMT

@pcaversaccio
Copy link
Contributor

Can someone please update forge-std accordingly 😄?

pcaversaccio added a commit to pcaversaccio/createx that referenced this pull request Oct 3, 2024
### 🕓 Changelog

The Foundry PR [#8945](foundry-rs/foundry#8945)
introduced a renaming of the `snapshot` and `revertTo` cheat codes to
`snapshotState` and `revertToState`, respectively. This PR aligns our
test suite with the newly revised cheat codes.

---------

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
pcaversaccio added a commit to pcaversaccio/snekmate that referenced this pull request Oct 3, 2024
### 🕓 Changelog

The Foundry PR [#8945](foundry-rs/foundry#8945)
introduced a renaming of the `snapshot` and `revertTo` cheat codes, now
referred to as `snapshotState` and `revertToState`. In response, this PR
updates our test suite—specifically the `ERC721Test` contract—to align
with these revised cheat codes. Additionally, `ethers` has been upgraded
to the latest version
[`6.13.3`](https://github.com/ethers-io/ethers.js/releases/tag/v6.13.3),
and all submodules have been updated to their most recent available
commits.

---------

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
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
Development

Successfully merging this pull request may close these issues.

5 participants