Skip to content

feat(forge): add vm.stopRecord #10370

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

Merged
merged 11 commits into from
Apr 30, 2025
Merged

feat(forge): add vm.stopRecord #10370

merged 11 commits into from
Apr 30, 2025

Conversation

tushar994
Copy link
Contributor

@tushar994 tushar994 commented Apr 25, 2025

Motivation

solves #10240

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@tushar994 tushar994 changed the title add vm.stopAndReturnAccesses() cheatcode add vm.stopRecordAndReturnAccesses() cheatcode Apr 25, 2025
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good! However, I realized that this might not cover all use cases: calling stopRecordAndReturnAccesses resets the accesses and it returns them for one address; if I want to stop and return for multiple addresses, this is not possible as accesses calls after will always return empty arrays;

I think we need a stopRecord that stops recording without clearing, and then clear on the next record call instead.

@grandizzy @mattsse thoughts?

@tushar994 tushar994 changed the title add vm.stopRecordAndReturnAccesses() cheatcode WIP: add vm.stopRecordAndReturnAccesses() cheatcode Apr 26, 2025
@tushar994
Copy link
Contributor Author

@grandizzy sorry for noob question, but how do I run the integration tests in testdata/default/cheats locally?

@DaniPopes
Copy link
Member

they are defined here https://github.com/foundry-rs/foundry/blob/master/crates/forge/tests/it/cheats.rs
cargo test -p forge --test it test_cheats_local_default

@grandizzy
Copy link
Collaborator

grandizzy commented Apr 26, 2025

I think we need a stopRecord that stops recording without clearing, and then clear on the next record call instead.

@grandizzy @mattsse thoughts?

Thanks, this looks good! However, I realized that this might not cover all use cases: calling stopRecordAndReturnAccesses resets the accesses and it returns them for one address; if I want to stop and return for multiple addresses, this is not possible as accesses calls after will always return empty arrays;

I think we need a stopRecord that stops recording without clearing, and then clear on the next record call instead.

@grandizzy @mattsse thoughts?

Yeah, that makes sense. So stopRecord should not set state.accesses to None but rather set a flag to mark it as stopped/no new records allowed - that is to still be able to retrieve records after (would here make sense to have a resumeRecord that will reallow records in case one wants to skip some actions?). Then a new record call should set the flag to true and reset all recorded accesses. @DaniPopes @tushar994 wdyt?

@tushar994 tushar994 closed this Apr 26, 2025
@github-project-automation github-project-automation bot moved this to Done in Foundry Apr 26, 2025
@tushar994
Copy link
Contributor Author

I think we need a stopRecord that stops recording without clearing, and then clear on the next record call instead.
@grandizzy @mattsse thoughts?

Thanks, this looks good! However, I realized that this might not cover all use cases: calling stopRecordAndReturnAccesses resets the accesses and it returns them for one address; if I want to stop and return for multiple addresses, this is not possible as accesses calls after will always return empty arrays;
I think we need a stopRecord that stops recording without clearing, and then clear on the next record call instead.
@grandizzy @mattsse thoughts?

Yeah, that makes sense. So stopRecord should not set state.accesses to None but rather set a flag to mark it as stopped/no new records allowed - that is to still be able to retrieve records after (would here make sense to have a resumeRecord that will reallow records in case one wants to skip some actions?). Then a new record call should set the flag to true and reset all recorded accesses. @DaniPopes @tushar994 wdyt?

I had thought the same, but didnt implement it because I thought i'd just stick to what was proposed in the issue. Will implement that! thank you!

@tushar994 tushar994 reopened this Apr 26, 2025
@tushar994
Copy link
Contributor Author

sorry closed it by accident

@tushar994 tushar994 changed the title WIP: add vm.stopRecordAndReturnAccesses() cheatcode add stopRecord and resetRecord cheatcodes Apr 29, 2025
@tushar994
Copy link
Contributor Author

Have added two cheatcodes

  • resetRecord. This clears all records calls by accesses.
  • stopRecord. This pauses recording of calls.
    @grandizzy

please review. thank you!!

DaniPopes
DaniPopes previously approved these changes Apr 29, 2025
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Thanks! I fixed some nits. I also removed resetRecord since we reset on the next record call anyway, and so resetRecord can be "implemented" using vm.record(); vm.stopRecord(); if really necessary.

@DaniPopes DaniPopes changed the title add stopRecord and resetRecord cheatcodes feat(vm): add stopRecord Apr 29, 2025
@DaniPopes DaniPopes changed the title feat(vm): add stopRecord feat(cheats): add stopRecord Apr 29, 2025
@DaniPopes DaniPopes changed the title feat(cheats): add stopRecord feat(forge): add vm.stopRecord Apr 29, 2025
@DaniPopes DaniPopes enabled auto-merge (squash) April 29, 2025 23:13
@tushar994
Copy link
Contributor Author

Thanks! I fixed some nits. I also removed resetRecord since we reset on the next record call anyway, and so resetRecord can be "implemented" using vm.record(); vm.stopRecord(); if really necessary.

got it! makes sense. I thought keeping different commands for each keeps it more general, so it will accomodate more use cases. with the current implementation, theres no way to pause recording and then unpause with previous state intact. however, that also might not be a use case that anyone wants. thank you!!

@DaniPopes DaniPopes merged commit 378ded1 into foundry-rs:master Apr 30, 2025
22 checks passed
@tushar994 tushar994 deleted the 10240 branch April 30, 2025 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants