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

Native assertions are not skipped for expectRevert #8049

Closed
jinxinglim opened this issue Jun 4, 2024 · 8 comments · Fixed by #8263
Closed

Native assertions are not skipped for expectRevert #8049

jinxinglim opened this issue Jun 4, 2024 · 8 comments · Fixed by #8263
Labels
T-bug Type: bug

Comments

@jinxinglim
Copy link

Issue

According to the foundry book, after calling expectRevert, calls to other cheatcodes before the reverting call are ignored.

However, when running a test like

    function test_assertTrue_string() public {
        string memory err = "throw test";
        vm.expectRevert(bytes(err));
        assertTrue(false, err);
    }

the vm.assertTrue call is executed and the test passed.
image

Question

Are the native assertions such as assertTrue, assertFalse, assertEq, etc not skipped for expectRevert? Are there any other exceptions as well? Thank you.

@mds1 mds1 transferred this issue from foundry-rs/forge-std Jun 4, 2024
@mds1
Copy link
Collaborator

mds1 commented Jun 4, 2024

I was able to reproduce this on the latest foundry, cc @klkvr

@klkvr
Copy link
Member

klkvr commented Jun 4, 2024

This is happening because the revert is being catched on the level of the test contract itself.

After calling expectRevert, calls to other cheatcodes before the reverting call are ignored.

this means that when expectRevert is active we do not expect the next cheatcode call to revert, but if it does, there is not much we can do

@mds1
Copy link
Collaborator

mds1 commented Jun 4, 2024

Hmm, ideally the test would fail here though, this feels like a footgun. Would this be solved by the proposed v1 changes for expectRevert? I'm not sure what the latest on those is

@jinxinglim
Copy link
Author

Hi @mds1 and @klkvr. Thank you for looking into this issue. Can I check if there is any update on this? Thank you.

Anyway, I am Jin Xing, part of the Kontrol team, where Kontrol combines KEVM and Foundry to grant developers the ability to perform formal verification without learning a new language or tool. So we want to make sure that the behaviour of this issue (expecting revert for assertions) in Foundry is done similarly in Kontrol.

@klkvr
Copy link
Member

klkvr commented Jun 10, 2024

hi @jinxinglim what exactly is your usecase for this?

expectRevert does not catch reverts for cheatcode calls (which native assertions are as well), and changing this would be a breaking change

@jinxinglim
Copy link
Author

Hi @klkvr. We are fine with either direction you guys will be taking as we just want Kontrol to behave similarly when it comes to expertRevert. At the current state, we have made Kontrol to not catch reverts for cheatcode calls as stated in the Foundry book.

expectRevert does not catch reverts for cheatcode calls (which native assertions are as well), and changing this would be a breaking change

But the issue is that when we run the following test, it does catch the revert from assertTrue(false, err), a cheatcode in Foundry and making it passed:

    function test_assertTrue_string() public {
        string memory err = "throw test";
        vm.expectRevert(bytes(err));
        assertTrue(false, err);
    }

Thus, we would like to clarify if expectRevert catches reverts for native assertions.

@klkvr
Copy link
Member

klkvr commented Jun 10, 2024

expectRevert catches reverts on its depth or above. So, in your example it does not expect assertTrue call on depth 1 to revert, however, the revert of assertTrue is being propagated by Solidity to revert entire test contract call, thus causing a revert on depth 0 which expectRevert catches.

I see how this is confusing though, and this indeed will be fixed by v1 expectRevert changes

@jinxinglim
Copy link
Author

expectRevert catches reverts on its depth or above. So, in your example it does not expect assertTrue call on depth 1 to revert, however, the revert of assertTrue is being propagated by Solidity to revert entire test contract call, thus causing a revert on depth 0 which expectRevert catches.

Oh. Thank you for the explanation!

this indeed will be fixed by v1 expectRevert changes

If that is the case, we will continue to make expectRevert to not catch not catch reverts for cheatcode calls (which native assertions are as well) in Kontrol so to be in sync with the v1 expectRevert changes as you mentioned. Just to check if you guys have an estimate on when this v1 change will be released? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants