feat(forge): change startPrank to overwrite existing prank instead of erroring #4826

4meta5 · 2023-04-26T03:49:58Z

Motivation

There is user demand for a startPrank or changePrank cheatcode that allows optimistically setting a new prank such that any existing prank is overwritten and there is no error if the existing prank is not set.

Solution

startPrank is changed to allow overwriting existing pranks instead of erroring. It is not able to overwrite existing pranks unless the existing prank has already been applied at least once.

mattsse

codewise lgtm,

wdyt @mds1

mds1 · 2023-04-26T12:26:40Z

Should we just change the behavior of startPrank to work this way instead of introducing a new cheat? It feels intuitive that startPrank would end any active pranks in order to start a new one. Then we'd also want a PR to forge-std that updates changePrank to just revert explaining this breaking change. cc @PaulRBerg

Also it looks like this only adds startChangePrank(address,address), but we also need startChangePrank(address) (or, if we just change startPrank, make sure to change behavior in both

Also one edge case to make sure we handle is:

vm.startPrank(msgSender, txOrigin);
// --- do stuff ---

// now we change prank, but need to make sure to reset txOrigin
vm.startPrank(msgSender);

PaulRBerg · 2023-04-26T12:29:47Z

Should we just change the behavior of startPrank to work this way

Works for me

…rank instead of erroring as per review suggestion

mattsse

lgtm,

pending @mds1 and lint

mds1 · 2023-04-28T15:39:50Z

Looks good! Just two questions:

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
I don't see new tests for startPrank, just tests for prank, so just want to confirm the above question, which was:

Also one edge case to make sure we handle is:

vm.startPrank(msgSender, txOrigin);
// --- do stuff ---

// now we change prank, but need to make sure to reset txOrigin
vm.startPrank(msgSender);

4meta5 · 2023-04-28T19:34:15Z

Both suggestions are reasonable.

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

Agreed, implemented in 15ac502

Will add tests for this as well

I don't see new tests for startPrank, just tests for prank, so just want to confirm the above question, which was:
Also one edge case to make sure we handle is:
vm.startPrank(msgSender, txOrigin);
// --- do stuff ---

// now we change prank, but need to make sure to reset txOrigin
vm.startPrank(msgSender); 

Will add tests for startPrank as well

…r suggestion

mattsse

lgtm, this seems to work!

just rebased because we landed a big pr on master

Will add tests for startPrank as well

this is the only thing missing, right?

4meta5 · 2023-04-29T23:37:03Z

cc @mds1 How do we use a prank in the tests? Specifically, I want to invoke Cheatcodes::call such that the prank is applied here https://github.com/foundry-rs/foundry/pull/4826/files#diff-3a6637039aa2043110b6bf828cf9c802d7621bdd472a536ea43f56421026eb64R597-R616 ? This would require executing a call that that uses the prank. I don't see any example of this in Prank.t.sol.

I do expect the added tests to fail now because we added an error for calls to prank when an existing prank has been added and not yet applied (suggestion 1). So until we add the call between successive startPranks, they will fail.

Evalir · 2023-05-10T19:35:11Z

hey hey @4meta5 thanks for the work! I've pushed some commits to your branch to get it over the line, hope you don't mind:

rebased to make sure it's up to par with master
added the startPrank tests and tweaked an error being returned to use one of our internal utils.

cc @mattsse / @mds1 should be g2g bar CI (github is having problems)

mds1 · 2023-05-10T20:01:39Z

@Evalir just confirming if you or @4meta5 handled these items too / have test cases for them? #4826 (comment)

Evalir · 2023-05-10T20:07:31Z

Ah whoops, seems I stashed & didn't upload that edge case: It is handled, but just pushed the missing test

Evalir · 2023-05-10T20:08:19Z

testdata/cheats/Prank.t.sol

+    function testStartPrank0AfterPrank1(address sender, address origin) public {
+        // Perform the prank
+        address oldOrigin = tx.origin;
+        Victim victim = new Victim();
+        cheats.startPrank(sender, origin);
+        victim.assertCallerAndOrigin(
+            sender, "msg.sender was not set during prank", origin, "tx.origin was not set during prank"
+        );
+
+        // Overwrite the prank
+        cheats.startPrank(sender);
+        victim.assertCallerAndOrigin(
+            sender, "msg.sender was not set during prank", oldOrigin, "tx.origin invariant failed"
+        );
+
+        cheats.stopPrank();
+        // Ensure we cleaned up correctly after stopping the prank
+        victim.assertCallerAndOrigin(
+            address(this), "msg.sender was not cleaned up", oldOrigin, "tx.origin invariant failed"
+        );
+    }


Just to confirm: this should be the edge case you're talking about @mds1 right?

Yep that's it, thanks!

mattsse

sweet,

tysm @4meta5

PaulRBerg · 2023-05-11T09:01:57Z

This is awesome! I'm glad that this was implemented.

Am I correct that now I can replace all of our uses of changePrank with vm.startPrank?

mds1 · 2023-05-11T12:59:25Z

Yep you should be able to!

We should also get a PR in to forge-std that logs a deprecation warning when using changePrank, and open a forge-std issue to track deleting it eventually. cc @Evalir if you're able to do that

urataps · 2023-08-31T09:29:48Z

Cool feature, but should contract deployment count also as if state was applied?

Currently I have some code that fails with this:

vm.startPrank(owner);
// deploy some contracts

vm.startPrank(stranger)
// check some stuff

4meta5 added 2 commits April 25, 2023 22:53

init start change prank

2f89779

testChangePrank

a19209a

mattsse approved these changes Apr 26, 2023

View reviewed changes

mds1 mentioned this pull request Apr 27, 2023

changePrank fails with No prank in progress to stop #4835

Closed

2 tasks

revert startChangePrank and change startPrank to overwrite existing p…

97c99f1

…rank instead of erroring as per review suggestion

4meta5 changed the title ~~feat(forge): cheat that starts or changes the prank~~ feat(forge): change startPrank to overwrite existing prank instead of erroring Apr 27, 2023

add tests for prank0 after prank1 and prank1 after prank0

4efb00b

mattsse approved these changes Apr 28, 2023

View reviewed changes

fmt

4b476c1

4meta5 and others added 3 commits April 28, 2023 16:10

add error if prank is not used at least once before overwritten as pe…

15ac502

…r suggestion

fmt

853731b

Merge branch 'master' into feat-start-change-prank

f68dff3

mattsse approved these changes Apr 29, 2023

View reviewed changes

4meta5 added 5 commits April 29, 2023 17:33

unit test for startPrank0 - and startPrank1 -

4aad6f3

fix

56fe709

remove clones by only updating prank after first time applied

a29f337

fmt

2a89075

more readable names

c8d8506

EdNoepel mentioned this pull request Apr 30, 2023

Forge 1.0 support ajna-finance/ajna-core#778

Merged

Evalir added 2 commits May 10, 2023 14:23

Merge branch 'master' into feat-start-change-prank

70a8e12

chore: fix/add tests, use ensure util

bb779a5

chore: add missing edge case test

a48965a

Evalir reviewed May 10, 2023

View reviewed changes

mattsse approved these changes May 11, 2023

View reviewed changes

mattsse merged commit c1dbafd into foundry-rs:master May 11, 2023

4meta5 deleted the feat-start-change-prank branch May 11, 2023 16:28

Evalir mentioned this pull request May 16, 2023

chore(CHANGELOG.md): add startPrank changes #4953

Merged

Evalir mentioned this pull request Aug 14, 2023

chore(StdCheats): deprecate changePrank foundry-rs/forge-std#434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(forge): change startPrank to overwrite existing prank instead of erroring #4826

feat(forge): change startPrank to overwrite existing prank instead of erroring #4826

4meta5 commented Apr 26, 2023 •

edited

Loading

mattsse left a comment

mds1 commented Apr 26, 2023

PaulRBerg commented Apr 26, 2023

mattsse left a comment •

edited

Loading

mds1 commented Apr 28, 2023

4meta5 commented Apr 28, 2023 •

edited

Loading

mattsse left a comment •

edited

Loading

4meta5 commented Apr 29, 2023 •

edited

Loading

Evalir commented May 10, 2023

mds1 commented May 10, 2023

Evalir commented May 10, 2023

Evalir May 10, 2023

mds1 May 10, 2023

mattsse left a comment

PaulRBerg commented May 11, 2023

mds1 commented May 11, 2023

urataps commented Aug 31, 2023

feat(forge): change startPrank to overwrite existing prank instead of erroring #4826

feat(forge): change startPrank to overwrite existing prank instead of erroring #4826

Conversation

4meta5 commented Apr 26, 2023 • edited Loading

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

mds1 commented Apr 26, 2023

PaulRBerg commented Apr 26, 2023

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

mds1 commented Apr 28, 2023

4meta5 commented Apr 28, 2023 • edited Loading

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

4meta5 commented Apr 29, 2023 • edited Loading

Evalir commented May 10, 2023

mds1 commented May 10, 2023

Evalir commented May 10, 2023

Evalir May 10, 2023

Choose a reason for hiding this comment

mds1 May 10, 2023

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

PaulRBerg commented May 11, 2023

mds1 commented May 11, 2023

urataps commented Aug 31, 2023

4meta5 commented Apr 26, 2023 •

edited

Loading

mattsse left a comment •

edited

Loading

4meta5 commented Apr 28, 2023 •

edited

Loading

mattsse left a comment •

edited

Loading

4meta5 commented Apr 29, 2023 •

edited

Loading