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

feat(cheatcodes): support partial matching of custom errors in reverts #3725

Closed
PaulRBerg opened this issue Nov 21, 2022 · 9 comments · Fixed by #8763
Closed

feat(cheatcodes): support partial matching of custom errors in reverts #3725

PaulRBerg opened this issue Nov 21, 2022 · 9 comments · Fixed by #8763
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-test Command: forge test T-feature Type: feature
Milestone

Comments

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Nov 21, 2022

Component

Forge

Describe the feature you would like

Custom errors can have arguments, and sometimes these arguments are difficult to calculate in a testing environment, and they may be unrelated to the test at hand (e.g. a value computed in the internal function of a third-party contract).

It would be nice if we could expect a revert with a partial matching of custom errors, like this:

error MyError(uint256 arg);

function testFoo() {
    // only checks that revert starts with "0x30b1b565", the 4-byte signature of "MyError(uint256)"
    vm.expectPartialRevert(MyError.selector);
}

This may warrant a new VM cheatcode, to ensure backwards compatibility and avoid triggering false positives for people who assume the current vm.expectRevert cheatcode to look for an exact ABI match.

Naming ideas: vm.expectPartialRevert or vm.expectCustomError or vm.expectRevertStartsWith.

@PaulRBerg PaulRBerg added the T-feature Type: feature label Nov 21, 2022
@mds1
Copy link
Collaborator

mds1 commented Nov 22, 2022

Some questions to flesh out the spec here more:

  • What would be the behavior if you pass in empty bytes? Presumably everything would match, so I think this cheat should throw in that case.
  • What if there is a full match? Should that cause the test to fail since this cheat expects a partial match only?
  • Is there a use case for partial matching the end of the revert data, or the middle, or should this only consider the start?

@PaulRBerg
Copy link
Contributor Author

  1. I agree that the cheat should throw if the user provides empty bytes. It's more likely that that is a typo rather an intended value.
  2. No I don't think a full match should cause the test to fail. I was thinking about this along the lines of the startsWith function in JavaScript. Even if you pass the full string, that string technically still starts with the full string, so startsWith returns true.
  3. As per the answer to point 2, I was personally thinking about this as a Forge-enabled flavor of startsWith. I need this only for checking the first 4 bytes in a custom error revert. Others can chime in if they think middle or end-matching would be useful for their use cases.

@rkrasiuk rkrasiuk added Cmd-forge-test Command: forge test C-forge Command: forge A-cheatcodes Area: cheatcodes labels Nov 22, 2022
@nidhhoggr
Copy link

nidhhoggr commented Dec 18, 2022

I'm wondering myself how to do this. The following happens when using vm.expectRevert for a matching error with an argument provided.

        vm.expectRevert(SamBankmanPlayingCards.MaxTokenAllotment.selector);
        bankmanCardzInstance.mint{value: 0.1 ether}(1);

[FAIL. Reason: Error != expected error: 0xa12804b00000000000000000000000000000000000000000000000000000000000000002 != 0xa12804b0]

So I must catch the reason and then unpack the arguments. In my case the argument is uint8 and I expect it to be 2.

        try bankmanCardzInstance.mint{value: 0.1 ether}(1) {}
        catch(bytes memory reason) {
            //assert selector equality
            bytes4 expectedSelector = SamBankmanPlayingCards.MaxTokenAllotment.selector;
            bytes4 receivedSelector = bytes4(reason);
            assertEq(expectedSelector, receivedSelector);
            //assert argument equality
            bytes32 parsed; 
            assembly {parsed := mload(add(reason, 36))}
            assertEq(parsed, bytes32(uint(2)));
        }

Before this feature gets added, is there a more intuitive way to do this?

@PaulRBerg
Copy link
Contributor Author

is there a more intuitive way to do this?

What I did was to refactor my custom errors to not include values computed during the function execution, which are difficult to re-compute in a testing environment. These dynamic values are also not that useful for end users, who should care more about the inputs that triggered that custom errors. Therefore, I changed the dynamic values with the input values.

@nidhhoggr
Copy link

@PaulRBerg I appreciate your explanation, however, in my scenario I don't use the Error arguments for the end user, nor for dynamic values. Instead, I use them in areas of the code that essentially utilize the same error. I want a way to debug which line of code or function the custom error was triggered from. This sort of reflection isn't available in solidity so I use uint8 _from as the error argument. This way there are several areas of the code that can use the same error e.g. MaxTokenAllotment without having to create two separate errors to account any specific scenario. With all that being said, my use case spans beyond the scope of the question regarding how to adequately capture custom Error arguments.

To provide a method to capture beginsWith as such that it's useful for your scenario, the following is adequate:

            bytes4 expectedSelector = SamBankmanPlayingCards.MaxTokenAllotment.selector;
            bytes4 receivedSelector = bytes4(reason);
            assertEq(expectedSelector, receivedSelector);

However, there could be a scenario even if I'd like to assert that the custom error argument is within a generally expected range using comparison operators. e.g. >,< which would still be practical to test against unpredictable dynamic values.

@PaulRBerg
Copy link
Contributor Author

PaulRBerg commented Dec 19, 2022

Thanks also for explaining your use case, @nidhhoggr.

Interesting idea with the receivedSelector, but I suppose you would need to perform a low-level call to catch the revert reason yourself.

Regarding your need - are you aware of forge debug?

@nidhhoggr
Copy link

nidhhoggr commented Dec 19, 2022

@PaulRBerg Yes I'm familiar with debug but I need unit testing to check that the error was throw from a specific conditional and/or line in the code. This isn't possibly by checking the error alone because there could be multiple spots in the function that throw the same error. While I could just create another custom error to distinguish the two, if the custom error explains a particular scenario like MaxTokenAllotment then providing a from parameter (instead of creating a different error) solves the issue:
Example Function:
https://github.com/nidhhoggr/foundry-error-handling/blob/094e232479dc1897a1a3eeba45187873cc3c8cde/src/ErrorThrowingContract.sol#L26

Example Testing:
https://github.com/nidhhoggr/foundry-error-handling/blob/094e232479dc1897a1a3eeba45187873cc3c8cde/test/ETCTesting.t.sol#L59

Last, I was just thinking about how the selector get computed, it's from the first four bytes of the function signatures keccack256 hash

   /**
     _funcSig example:  "transferFrom(address,address,uint256)"
     */
    function getSelector(string calldata _funcSig) external pure returns (bytes4) {
        return bytes4(keccak256(bytes(_funcSig)));
    }

For this reason I think the method your proposing being name startsWith could be misleading because it might make people think they can derive the same assertion results from functions with similar names. For that reason I would simply use assertExpectedSelector.
https://github.com/nidhhoggr/foundry-error-handling/blob/094e232479dc1897a1a3eeba45187873cc3c8cde/test/ETCTesting.t.sol#L16

Notice the difference in signatures even through the methods names are similar and both startsWith the word transfer:

    "transfer(address,uint256)"
    0xa9059cbb
    "transferFrom(address,address,uint256)"
    0x23b872dd

@nidhhoggr
Copy link

Last but not least, the following answers my own question on a more "intuitive" way to use expectRevert for custom errors with parameters without the need for try/catch blocks with error parsing.

error MaxTokenAllotment(uint8 _from);

Where

  1. MaxTokenAllotment is the expected custom error
  2. uint(2) is the expected MaxTokenAllotment parameter

From

function testExpectCustomErrorWithParam() public {
    try contractInstance.mint{value: 0.1 ether}(1) {}
    catch(bytes memory reason) {
        //assert selector equality
        bytes4 expectedSelector = MaxTokenAllotment.selector;
        bytes4 receivedSelector = bytes4(reason);
        assertEq(expectedSelector, receivedSelector);
        //assert argument equality
        bytes32 parsed; 
        assembly {parsed := mload(add(reason, 36))}
        assertEq(parsed, bytes32(uint(2)));
    }
}   

To

function testExpectCustomErrorWithParam() public {
     vm.expectRevert(abi.encodePacked(MaxTokenAllotment.selector, uint(2)));
     contractInstance.mint{value: 0.1 ether}(1);
}

@DanL0
Copy link

DanL0 commented Jun 11, 2024

Related issue (misleading documentation in Vm.sol):
#7629

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-test Command: forge test T-feature Type: feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants