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

fix(forge): loadAllocs cheatcode doesn't persist when called in setUp #6297

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

clabby
Copy link
Contributor

@clabby clabby commented Nov 13, 2023

Overview

The loadAllocs cheatcode currently does not persist loaded account information if it is called in the setUp function. This is because none of the loaded accounts were marked as touched during the loading process.

Solution

Touch each account that is loaded into the revm journal in the load_allocs function in the DB backend.

Tests

Added a regression test for the fixed bug, asserting that the loaded accounts are persisted when the cheatcode is called in setUp.

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

this lgtm—looping in @mds1 for correctness

Comment on lines +42 to +44
// Restore the state snapshot prior to the allocs file being loaded.
vm.revertTo(snapshotId);

Copy link
Collaborator

@mds1 mds1 Nov 13, 2023

Choose a reason for hiding this comment

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

@Evalir do we have confidence/good tests for snapshot/revertTo cheats? We don't verify that state was correctly reset after this call and I know there's been some bugs on those cheats in the past

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did check it manually, but didn't commit them due to the tests in Snapshots.t.sol. Glad to add an explicit test for this in loadAllocs.t.sol, but figured we didn't want to cross-wires 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, I agree the tests for proper revertTo behavior are better suited elsewhere, so I'm ok leaving it as-is

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

Just one small comment but otherwise LGTM!

@Evalir Evalir merged commit b205b6a into foundry-rs:master Nov 13, 2023
19 checks passed
@clabby clabby deleted the cl/load-allocs-fix branch November 13, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants