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

invariant: #6694 add preserve_state config #7219

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

grandizzy
Copy link
Collaborator

Motivation

Solution

  • use call_raw_committing if preserve_state and fail_on_revert enabled

…e state (e.g. using cheatcodes like roll, warp), see foundry-rs#6694

- active only in conjunction with fail_on_revert true
@mds1
Copy link
Collaborator

mds1 commented Feb 26, 2024

See also #5444, so deferring to @Evalir on how this should be implemented.

Committing/preserving state is more intuitive and should be the default though—assuming the performance issue is resolved, what is the reason you'd want to disable it? It should also apply whether or not fail_on_revert is enabled

@grandizzy
Copy link
Collaborator Author

See also #5444, so deferring to @Evalir on how this should be implemented.

Committing/preserving state is more intuitive and should be the default though—assuming the performance issue is resolved, what is the reason you'd want to disable it? It should also apply whether or not fail_on_revert is enabled

the only reason for disabling by default and applying with fail_on_revert is to avoid any potential performance issue for existing integrations, so projects could tweak these params, run with them and report back if something really bad. then after some time passed and feedback collected should be good to turn defaults on...

@grandizzy grandizzy changed the title invariant: add preserve_state config invariant: #6694 add preserve_state config Feb 27, 2024
@grandizzy grandizzy marked this pull request as ready for review February 27, 2024 09:34
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.

I think that makes sense and a reasonable usecase

lgtm, pending @Evalir

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