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

♻️ Enable expectRevert for internal Functions #298

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

pcaversaccio
Copy link
Owner

@pcaversaccio pcaversaccio commented Jan 28, 2025

🕓 Changelog

The Foundry PR #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 in various expectRevert test calls:

/**
 * @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 PR enables expectRevert for internal functions by setting allow_internal_expect_revert = true in the foundry.toml file.

🐶 Cute Animal Picture

image

@pcaversaccio pcaversaccio self-assigned this Jan 28, 2025
@pcaversaccio pcaversaccio added dependencies 🔁 Pull requests that update a dependency file refactor/cleanup ♻️ Code refactorings and cleanups labels Jan 28, 2025
@pcaversaccio pcaversaccio added this to the 0.1.1 milestone Jan 28, 2025
Signed-off-by: Pascal Marco Caversaccio <[email protected]>
@pcaversaccio pcaversaccio merged commit 9eeda76 into main Jan 28, 2025
14 of 15 checks passed
@pcaversaccio pcaversaccio deleted the test/expectRevert-cheatcode branch January 28, 2025 14:26
@zerosnacks
Copy link

zerosnacks commented Jan 31, 2025

Hi @pcaversaccio

It is also possible to use the in-line configuration as follows selectively on the failing function tests

/// forge-config: default.allow_internal_expect_revert = true
function test_foo() public {}

This is recommended over using the global configuration option, see:

https://book.getfoundry.sh/cheatcodes/expect-revert#description

@pcaversaccio
Copy link
Owner Author

Hi @pcaversaccio

It is also possible to use the in-line configuration as follows selectively on the failing function tests

/// forge-config: default.allow_internal_expect_revert = true
function test_foo() public {}

This is recommended over using the global configuration option, see:

https://book.getfoundry.sh/cheatcodes/expect-revert#description

Thx, I'm aware of this possibility, but I personally prefer to enable expectRevert for internal functions by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 🔁 Pull requests that update a dependency file refactor/cleanup ♻️ Code refactorings and cleanups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants