feat(forge): do not revert if prank has not been used

[PaulRBerg]

Component

Forge

Describe the feature you would like

Problem

#4826 made it possible to override the current prank with startPrank, but it also introduced a new requirement, i.e. the previous prank must have been used at least once for the second startPrank to be valid:

15ac502

This new requirement is quite problematic and self-defeating with respect to the original goal of this override feature because, in complex code bases with many test files, it is difficult to know if the previous prank was used.

Take, for instance, the Base_Test in my PRBProxy code base:

https://github.com/PaulRBerg/prb-proxy/blob/0b33e550bfd3c9264ccac22289745012a1b1b92d/test/Base.t.sol#L131

I need this prank as a default prank mode for all tests (so that I do not have to put it in every contract's setUp), but some tests need to override this behavior and start a new prank. See these fuzz tests:

https://github.com/PaulRBerg/prb-proxy/blob/0b33e550bfd3c9264ccac22289745012a1b1b92d/test/registry/deploy/deploy.t.sol#L26-L31

This is where I tried to replace my changePrank calls with vm.startPrank, and ended up getting the following error:

Reason: You cannot overwrite prank until it is applied at least once

Solutions

Possible ways to address this issue:

1. Undo 15ac502 (my preferred solution)
2. Add an override_unused_prank (or allow_unused_prank) config option in foundry.toml, which allows the user to override the latest prank even if it has not been used
3. Keep the behavior as is but do not deprecate changePrank in forge-std

WDYT, @4meta5?

Additional context

This restrictive behavior has apparently been implemented as a result of this suggestion left by @mds1:

What if I call vm.prank immediately followed by vm.prank (same question if either/both of those are replaced with startPrank)? I think we should make sure that reverts, because the first prank was never used

[PaulRBerg]

I just tried replacing changePrank with vm.startPrank in one of my other projects - and I encountered the same breaking behavior.

[mds1]

Hmm interesting. The use case does make sense, but I think we should try to avoid a config flag if possible for simplicity. I see two competing interests:

1. Make mistakes + poorly written tests less common by not allowing unused pranks
2. Make utils simpler/cleaner by allowing unused pranks

Does stopPrank work even if you never used the prank started with startPrank? I can't recall offhand, but if so I think that could be a solution: We keep the current behavior to make tests safer by default, and move the work to devex utils that need to check if a prank is active and conditionally stop if so

[PaulRBerg]

Does stopPrank work even if you never used the prank started with startPrank?

It does - changePrank wouldn't work otherwise.

move the work to devex utils that need to check if a prank is active and conditionally stop if so

This means not deprecating changePrank in Forge Std, and potentially refactoring it to read the current prank using the read prank feature being added in #4884.

[mds1]

Cool, that seems like a good solution to me then since we can then support both of the interests in #4942 (comment)

[urataps]

I have the same issue, perhaps it would be solved if we consider a contract deployment as applied state to startPrank, as only contract calls are counted. I have to do a mock call after setting the admin in base test contract to avoid the error.

[mds1]

Can this issue be closed now that we have the readCallers cheat? A user can write a utility method to handle the pranks accordingly

[Evalir]

Yep, I believe this can be closed.