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

Cheatcode expectRevert doesn't catch error messages from libraries #4405

Closed
2 tasks done
pedrommaiaa opened this issue Feb 21, 2023 · 4 comments
Closed
2 tasks done
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-test Command: forge test T-bug Type: bug T-to-investigate Type: to investigate
Milestone

Comments

@pedrommaiaa
Copy link
Contributor

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (b44b045 2023-02-21T00:22:41.521901Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

Helper Contract:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {ERC20} from "solmate/tokens/ERC20.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";

contract Helper {
    ERC20 public token;

    constructor(ERC20 _token) { token = _token; }

    function doSafeTransfer(address to, uint256 amount) external {
        SafeTransferLib.safeTransfer(token, to, amount); 
    }
}

In the example bellow testRevertTransfer is expected to be successful as it was supposed to catch the error message from the library but in reality it doesn't.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test} from "forge-std/Test.sol";
import {Helper} from "../src/Helper.sol";
import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol";
import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";

contract LibraryTest is Test {
    Helper public helper;
    MockERC20 public token;

    function setUp() public {
        token = new MockERC20("MockToken", "TST", 18);
        helper = new Helper(token);
    }

    // This doesn't work.
    function testRevertTransfer() external {
        vm.expectRevert(bytes("TRANSFER_FAILED"));
        SafeTransferLib.safeTransfer(token, address(0xBEEF), 1e18);
    }

    // This works fine.
    function testRevertHelperTransfer() external {
        vm.expectRevert(bytes("TRANSFER_FAILED"));
        helper.doSafeTransfer(address(0xBEEF), 1e18);
    }
}

Repo with reproducible error: https://github.com/pedrommaiaa/Foundry-Library-Error
Issue also opened in the forge-std repo: foundry-rs/forge-std#306

@pedrommaiaa pedrommaiaa added the T-bug Type: bug label Feb 21, 2023
@ssadler
Copy link

ssadler commented Apr 7, 2023

As a workaround, you can define a wrapper for your pure function and call it using this in your test:

contract Tests {
  function testThing() public {
    vm.expectRevert("...");
    this.wrapPure();
  }
  function wrapPure() public { MyLib.wrapPure(); }
}

@ssadler
Copy link

ssadler commented Apr 7, 2023

I have an example where it's not catching a revert but it's not a library, instead it's an abstract contract.

The wrapper trick above works also in this case.

@highskore
Copy link

I have an example where it's not catching a revert but it's not a library, instead it's an abstract contract.

The wrapper trick above works also in this case.

Just faced a similar issue with an abstract contract and I managed to solve it using this method

@grandizzy
Copy link
Collaborator

expectRevert cheatcode expects an revert from the next external call, so in the original issue

    // This doesn't work.
    function testRevertTransfer() external {
        vm.expectRevert(bytes("TRANSFER_FAILED"));
        SafeTransferLib.safeTransfer(token, address(0xBEEF), 1e18);
    }

fails because SafeTransferLib is inlined in contract. To check behavior you can change the signature of SafeTransferLib.safeTransfer (lib/solmate/src/utils/SafeTransferLib.sol) to public instead internal and test is going to pass.

Going to close this one and track it with #5367 as there is a broader discussion about. thank you!

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-bug Type: bug T-to-investigate Type: to investigate
Projects
Status: Done
Development

No branches or pull requests

5 participants