Skip to content

fix(cheatcode): expect revert only for calls with greater depth than test#9537

Merged
grandizzy merged 7 commits intofoundry-rs:masterfrom
grandizzy:issue-7238
Jan 27, 2025
Merged

fix(cheatcode): expect revert only for calls with greater depth than test#9537
grandizzy merged 7 commits intofoundry-rs:masterfrom
grandizzy:issue-7238

Conversation

@grandizzy
Copy link
Copy Markdown
Collaborator

@grandizzy grandizzy commented Dec 11, 2024 •
edited
Loading

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
mattsse
mattsse approved these changes Jan 24, 2025
View reviewed changes
Copy link
Copy Markdown
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

testdata/default/cheats/AttachDelegation.t.sol
assertEq(token.balanceOf(bob), 200);
}

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

@mattsse mattsse Jan 24, 2025

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

@grandizzy @mattsse
Update crates/cheatcodes/src/inspector.rs
583316f 
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
zerosnacks
zerosnacks approved these changes Jan 24, 2025
View reviewed changes
Copy link
Copy Markdown
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!

@grandizzy grandizzy merged commit ca466ae into foundry-rs:master Jan 27, 2025
@grandizzy grandizzy deleted the issue-7238 branch January 27, 2025 05:53
@pcaversaccio pcaversaccio mentioned this pull request Jan 28, 2025
Merged
pcaversaccio added a commit to pcaversaccio/snekmate that referenced this pull request Jan 28, 2025
@pcaversaccio
♻️ Enable expectRevert for internal Functions (#298)
9eeda76 
### 🕓 Changelog

The Foundry PR [#9537](foundry-rs/foundry#9537)
has disabled `expectRevert` for `internal` functions by default.
However, we need this functionality in the `ECDSATest` contract since we
use the `internal` function
[`to2098Format`](https://github.com/pcaversaccio/snekmate/blob/9f75d9276251ccd03254e62c7b5581d776099d52/test/utils/ECDSA.t.sol#L37-L52)
in various `expectRevert` test calls:

```solidity
/**
 * @dev Transforms a standard signature into an EIP-2098
 * compliant signature.
 * @param signature The secp256k1 64/65-bytes signature.
 * @return short The 64-bytes EIP-2098 compliant signature.
 */
function to2098Format(
    bytes memory signature
) internal view returns (bytes memory) {
    if (signature.length != 65) revert InvalidSignatureLength(self);  // <-- We need to enable this `revert`.
    if (uint8(signature[32]) >> 7 == 1) revert InvalidSignatureSValue(self); // <-- We need to enable this `revert`.
    bytes memory short = signature.slice(0, 64);
    uint8 parityBit = uint8(short[32]) | ((uint8(signature[64]) % 27) << 7);
    short[32] = bytes1(parityBit);
    return short;
}
```

This commit enables `expectRevert` for `internal` functions by setting
`allow_internal_expect_revert = true` in the `foundry.toml` file.

---------

Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
@frolic
Copy link
Copy Markdown

frolic commented Jan 28, 2025

Just spotted some test failures in MUD due to this change. (We intentionally use nightly in CI to make sure we're always ready for latest/upcoming foundry, to flag situations like this.)

MUD makes extensive use of internal libraries and free functions, but it's sort of unclear if the test failures we're seeing are intended. Wanted to check in before we enable the allow_internal_expect_revert config option across our repo + downstream user projects.

For example, this failing test:
https://github.com/latticexyz/mud/actions/runs/13013139151/job/36295534363

packages/schema-type test:ci: Encountered 1 failing test in test/solidity/SchemaType.t.sol:SchemaTypeTest
packages/schema-type test:ci: [FAIL: call didn't revert at a lower depth than cheatcode call depth] testSchemaTypeOverMaxLength() (gas: 3591)

Comes from a revert in this free function bound to a user type:
https://github.com/latticexyz/mud/blob/main/packages/schema-type/src/solidity/SchemaType.sol

Seems like any test that uses expectRevert for an operation that calls an internal/free function with a revert now fails with this new default. Maybe this is fine/expected, just want to know what the suggested best practice is going forward with these new defaults.

@grandizzy
Copy link
Copy Markdown
Collaborator Author

grandizzy commented Jan 28, 2025 •
edited
Loading

@holic yes, that's correct, internal/free functions with expect revert now fails if done at the same depth as test. You can turn on the old behavior by setting allow_internal_expect_revert = true if you are aware that test stops executing after first expectRevert (see #3437)

@frolic frolic mentioned this pull request Jan 28, 2025
Merged
@r0qs r0qs mentioned this pull request Jan 28, 2025
Closed
@janjakubnanista janjakubnanista mentioned this pull request Apr 7, 2025
Merged
@jenpaff jenpaff moved this to Completed in Foundry Apr 9, 2025
willbrown84 added a commit to willbrown84/snekmate that referenced this pull request Sep 23, 2025
@willbrown84
♻️ Enable expectRevert for internal Functions (#298)
b09ebff 
### 🕓 Changelog

The Foundry PR [#9537](foundry-rs/foundry#9537)
has disabled `expectRevert` for `internal` functions by default.
However, we need this functionality in the `ECDSATest` contract since we
use the `internal` function
[`to2098Format`](https://github.com/pcaversaccio/snekmate/blob/9f75d9276251ccd03254e62c7b5581d776099d52/test/utils/ECDSA.t.sol#L37-L52)
in various `expectRevert` test calls:

```solidity
/**
 * @dev Transforms a standard signature into an EIP-2098
 * compliant signature.
 * @param signature The secp256k1 64/65-bytes signature.
 * @return short The 64-bytes EIP-2098 compliant signature.
 */
function to2098Format(
    bytes memory signature
) internal view returns (bytes memory) {
    if (signature.length != 65) revert InvalidSignatureLength(self);  // <-- We need to enable this `revert`.
    if (uint8(signature[32]) >> 7 == 1) revert InvalidSignatureSValue(self); // <-- We need to enable this `revert`.
    bytes memory short = signature.slice(0, 64);
    uint8 parityBit = uint8(short[32]) | ((uint8(signature[64]) % 27) << 7);
    short[32] = bytes1(parityBit);
    return short;
}
```

This commit enables `expectRevert` for `internal` functions by setting
`allow_internal_expect_revert = true` in the `foundry.toml` file.

---------

Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
void-rider5560p added a commit to void-rider5560p/snekmate that referenced this pull request Sep 28, 2025
@void-rider5560p
♻️ Enable expectRevert for internal Functions (#298)
28a346a 
### 🕓 Changelog

The Foundry PR [#9537](foundry-rs/foundry#9537)
has disabled `expectRevert` for `internal` functions by default.
However, we need this functionality in the `ECDSATest` contract since we
use the `internal` function
[`to2098Format`](https://github.com/pcaversaccio/snekmate/blob/9f75d9276251ccd03254e62c7b5581d776099d52/test/utils/ECDSA.t.sol#L37-L52)
in various `expectRevert` test calls:

```solidity
/**
 * @dev Transforms a standard signature into an EIP-2098
 * compliant signature.
 * @param signature The secp256k1 64/65-bytes signature.
 * @return short The 64-bytes EIP-2098 compliant signature.
 */
function to2098Format(
    bytes memory signature
) internal view returns (bytes memory) {
    if (signature.length != 65) revert InvalidSignatureLength(self);  // <-- We need to enable this `revert`.
    if (uint8(signature[32]) >> 7 == 1) revert InvalidSignatureSValue(self); // <-- We need to enable this `revert`.
    bytes memory short = signature.slice(0, 64);
    uint8 parityBit = uint8(short[32]) | ((uint8(signature[64]) % 27) << 7);
    short[32] = bytes1(parityBit);
    return short;
}
```

This commit enables `expectRevert` for `internal` functions by setting
`allow_internal_expect_revert = true` in the `foundry.toml` file.

---------

Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattsse mattsse mattsse approved these changes

@zerosnacks zerosnacks zerosnacks approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@klkvr klkvr Awaiting requested review from klkvr

@yash-atreya yash-atreya Awaiting requested review from yash-atreya

Assignees

No one assigned

Labels

T-likely-breaking Type: requires changes that can be breaking

Projects

Foundry
Status: Completed

Milestone

No milestone

5 participants

@grandizzy @frolic @mattsse @zerosnacks @jenpaff