Opinion: prank can be a rather unintuitive

[fubhy]

I find myself making the same mistake over and over again ... Previously I've been using prank a lot instead of startPrank but due to this, I'm wondering if I should exclusively use startPrank. Here's why:

At first glance, prank seems great. Single line, just applies to the next call, no need to undo afterwards, etc. ... However, what if the next line after prank looks like this:

vm.prank(bob);
myContract.setSomething(address(new OtherContract));

I don't know if I'm alone with this, but I keep running into this: prank in this case just applies to the creation of OtherContract, not
the actual call to setSomething.

It's probably a bit late for a change here but I wonder if it would be better to default prank to behave like startPrank and have a prankOnce that explicitly only applies to the next instruction.

The problem isn't that I'm not aware of prank only applying to the next instruction but rather that I constantly write these single-line double-instructions and am then confused why my test fails.

Thoughts?

[fubhy]

Btw. is this the wrong place to discuss that? Should I open an issue in forge-std?

[onbjerg]

It's probably a bit late for a change here but I wonder if it would be better to default prank to behave like startPrank and have a prankOnce that explicitly only applies to the next instruction.

That would imply all calls would get pranked by default which is definitely not desireable. I think the current separation is fine, although it might have been more clear if we had called it prankOnce. As far as I know though the naming is from DappTools, so kind of set in stone in that sense

Btw. is this the wrong place to discuss that? Should I open an issue in forge-std?

No this is the correct place

[fubhy]

Maybe this is something that can be solved better with IDE syntax highlighting explcitly highlighting the context of the call if it's pranked.

[fubhy]

Contuining our test migration from the same test case this morning...

I still think that the behavior is non-intuitive. I also surveyed one of my colleagues (who has never used dapptools or foundry before) how he expects this to work: he intuitively also assumed it would basically apply to the whole line afterwards at least.

Although more verbose, I think I'd find a stack-based prank state API more intuitive and easier to follow when reading a test.

E.g. one where you not only start and stop pranking but essentially push and pop the current "prankster" onto and from a stack. That would also be helpful for utility functions within the tests that need to temporarily change the active prankster.

And when looking at a few lines where you push before and pop after a few statemements, it's immediately clear what context the calls in between are in.

In other languages, I'd have probably expected this API to use some kind of closure syntax

prank(bob).apply(() => {
  // Everything in here is pranked by `bob`, after the closure it's reset to the outer scope.
});

Obviously we can't do that here. But the closes to that would be a push and pop-like API.

I know that this feedback is super late and it's probably not actionable at this point but and it makes sense to stay in-line with DappTools but maybe this can be added in addition.

I'd love to hear what others think about this concern after trying to "take a step back" and evaluating the intuitiveness of it with a fresh and unbiased look at it.

[fubhy]

Is it? Because then the code comments here might be wrong?

https://github.com/foundry-rs/forge-std/blob/master/src/Vm.sol#L57-L61

At least those comments read as if stopPrank resets it to address(this) instead of just poping the last added startPrank value from the stack.

[onbjerg]

Oh in that sense I guess it's true but I still don't really know how having it be stack-based in that sense would solve your original issue? The prank would still refer to only one call.

[fubhy]

Oh and I know that we can't do it line-by-line. I'm not asking for that. I'm just saying that by simply reading that one line in that context someone who's unfamiliar with the inner workings of the prank cheatcode would likely expect it to apply the both statements:

vm.prank(bob);
myContract.setSomething(address(new OtherContract));

[fubhy]

Oh in that sense I guess it's true but I still don't really know how having it be stack-based in that sense would solve your original issue? The prank would still refer to only one call.

Sorry, I'm not eloquently expressing myself. Maybe to shortly summarize:

1. The vm.prank cheatcode (as currently documented as the primary means of pranking) can be unintuitive in some cases like in my example and can therefore cause hard-to-catch mistakes
2. I believe the startPrank / stopPrank behavior is much more intuitive and would personally prefer if that was documented as the "canonical" way of "pranking" (and if it was using a stack)
3. I acknowledge that there is also the inverse scenario where you actually do want to prank the inner-statement of multiple expressions in a single-line but I'd argue that that's even worse in terms of readability / comprehensability (imho tests should be excellent in terms of code comprehensability)

If solidity was a bit more versatile, we could express and scope pranking context within a closure.

prank(bob).apply(() => {
  // Everything in here is pranked by `bob`, after the closure it's reset to the outer scope.
});

Since we can't do that, the explicity start/stop syntax with push/pop behavior is imho what comes closest:

// Pushes to stack (instead of straight up replacing the previous prankster)
vm.startPrank(bob);

// This test util might internally have it's own start/stop pranking context for parts of its logic.
someTestUtil();

// Despite the previous test util having its own pranking scope internally, the following line is scoped to `bob` again.
myContract.setSomething(address(new OtherContract));

 // Pops from the stack [returns to previous no-prank state or previously selected prankster if any]
vm.stopPrank();

// NOTE: Should likely throw if there's a dangling (unclosed) pranking context at the end of a test.

Yes, this is a bit more verbose but it leaves less room for foot-guns imho and is much clearer in communicating the intent. I'd argue that, while surely it's not a good idea to change or remove the behavior of vm.prank() to not break backwards compatibility (also with DappTools), it might make sense to adjust start/stopPrank in this manner and promote it as the canonical pranking mechanism due to its (in my opinion) better comprehensability.

This is all just my opinion and might be a matter of taste but since I ran into this several times yesterday I simply wanted to bring this up. Something might be wrong in my brain for me to run into this repeatedly but in the moment where I was unconsciously writing that code it simply seemed natural that it would behave in the way of pranking the entire following line. Obviously it makes no sense but I wasn't thinkin that far, despite knowing how it's supposed to behave.

Do you see my point or does none of this make sense? :-P

[onbjerg]

One day perhaps 😄 ethereum/solidity#12297