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(cheatcode): expect revert only for calls with greater depth than test #9537

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Dec 11, 2024

Motivation

Closes #3723
Closes #7238
Closes #3437

Breaking change, should be merged after pinned version.

  • record max depth of next call after expectRevert, compares with test depth (at which cheatcode was added). Does not apply for _expectCheatcodeRevert calls.
  • add allow_internal_expect_revert config to enable expect reverts for internal calls. defaults to false
  • fix and add tests

Solution

@grandizzy grandizzy added the T-likely-breaking Type: requires changes that can be breaking label Dec 11, 2024
@grandizzy grandizzy force-pushed the issue-7238 branch 2 times, most recently from dccc236 to 8a0ea49 Compare December 11, 2024 14:53
@grandizzy grandizzy changed the title [WIP] fix(cheatcode): expect revert only for calls with greater depth [WIP] fix(cheatcode): expect revert only for calls with greater depth than test Dec 11, 2024
@grandizzy grandizzy changed the title [WIP] fix(cheatcode): expect revert only for calls with greater depth than test [DO NOT MERGE] fix(cheatcode): expect revert only for calls with greater depth than test Dec 12, 2024
@grandizzy grandizzy marked this pull request as ready for review December 12, 2024 07:24
@grandizzy grandizzy changed the title [DO NOT MERGE] fix(cheatcode): expect revert only for calls with greater depth than test fix(cheatcode): expect revert only for calls with greater depth than test Jan 6, 2025
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.

lgtm

crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
@@ -86,6 +86,7 @@ contract AttachDelegationTest is DSTest {
assertEq(token.balanceOf(bob), 200);
}

/// forge-config: default.allow_internal_expect_revert = true
Copy link
Member

Choose a reason for hiding this comment

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

unrelated, pretty cool that this works

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-likely-breaking Type: requires changes that can be breaking
Projects
Status: No status
3 participants