feat(forge): readCallers cheatcode #4884

xeno097 · 2023-05-05T15:26:25Z

Motivation

Solution

Implements a readPCallers cheat code that allows to read the current msg.sender and tx.origin addresses that will be used to send transactions in later calls

…k-cheatcode

Evalir

This lgtm. Looks like it needs linting.

Would appreciate some followup PRs for the book and forge-std! Deferring to Matt for final approval & merge.

xeno097 · 2023-05-05T15:43:28Z

This lgtm. Looks like it needs linting.

Would appreciate some followup PRs for the book and forge-std! Deferring to Matt for final approval & merge.

On it 😉

mds1 · 2023-05-05T15:43:35Z

Do we need the bool? In other words, do we actually care if a prank is active, or do we just care about what the msg.sender and tx.origin will be in the next call? I suspect it's the latter, meaning we can simplify this interface. If there is an active prank, you return the prank addresses, if there isn't you return the default sender addresses.

mds1 · 2023-05-05T15:45:40Z

Given that use case, we'd probably want to rename this to something more generic like readCallers.

Another consideration: how does this cheatcode behave in scripts, where instead of pranks you have broadcasts, but which have similar semantics of changing the msg.sender?

xeno097 · 2023-05-05T15:59:17Z

Do we need the bool? In other words, do we actually care if a prank is active, or do we just care about what the msg.sender and tx.origin will be in the next call? I suspect it's the latter, meaning we can simplify this interface. If there is an active prank, you return the prank addresses, if there isn't you return the default sender addresses.

Following what @PaulRBerg described in #4875, I preferred using a bool instead of returning the current caller and origin because in case the user wants to call any of the prank cheat codes he would only need to check the boolean instead of having to manually compare the pranked address and the original caller.
Also, nothing stops the user from calling vm.prank() with the test contract address, in that case, it would be ambiguous if there is an actual prank active or not. Calling vm.stopPrank or any other prank cheat codes could revert the test in that case. This would be an example for this scenario:

contract CounterTest is Test {
    Counter public counter;

    function setUp() public {
        counter = new Counter();
        counter.setNumber(0);
    }

    function testIncrement() public {
        vm.prank(address(this));

        (bool isActive, address newSender, ) = vm.readPrank();

        counter.increment();
        assertEq(counter.number(), 1);
    }
}

xeno097 · 2023-05-05T16:05:56Z

Given that use case, we'd probably want to rename this to something more generic like readCallers.

Another consideration: how does this cheatcode behave in scripts, where instead of pranks you have broadcasts, but which have similar semantics of changing the msg.sender?

I honestly hadn't thought about it, thank you for pointing it out. Maybe we should implement a new cheat code for that scenario considering that with broadcast you alter only the msg.sender, I think it would be cleaner this way.

mds1 · 2023-05-05T16:10:16Z

Following what @PaulRBerg described in #4875, I preferred using a bool instead of returning the current caller and origin because in case the user wants to call any of the prank cheat codes he would only need to check the boolean instead of having to manually compare the pranked address and the original caller.
Also, nothing stops the user from calling vm.prank() with the test contract address, in that case, it would be ambiguous if there is an actual prank active or not. Calling vm.stopPrank or any other prank cheat codes could revert the test in that case. This would be an example for this scenario:

Hmm, based on his description of this cheat:

It would be helpful to have a cheatcode for reading the current prank. The use case is testing utilities that don't have knowledge of when the current prank was started, but which require tweaking the msg.sender to perform a particular action in the contracts.

It's not clear what the details of the use case are, so it's hard to say which UX makes more sense. Note that you can also change the sender used in tests from the CLI with the --sender flag. Therefore just having "no active prank" does not mean the next caller is the default sender, and you'd need to use msg.sender to get the address. The use case I can think of for this cheat is something like getting nonces or state of an account, and in all those cases, you wouldn't actually care if a prank is active or not, and the data you actually want is "what are the next sender/origin addresses used"

For the script scenario, I think this cheat would have to (1) detect that we're in a script environment and revert, or (2) return the address that would be used for the next vm.broadcast() call for both the msg.sender and tx.origin

xeno097 · 2023-05-05T16:31:50Z

Following what @PaulRBerg described in #4875, I preferred using a bool instead of returning the current caller and origin because in case the user wants to call any of the prank cheat codes he would only need to check the boolean instead of having to manually compare the pranked address and the original caller.
Also, nothing stops the user from calling vm.prank() with the test contract address, in that case, it would be ambiguous if there is an actual prank active or not. Calling vm.stopPrank or any other prank cheat codes could revert the test in that case. This would be an example for this scenario:

Hmm, based on his description of this cheat:

It would be helpful to have a cheatcode for reading the current prank. The use case is testing utilities that don't have knowledge of when the current prank was started, but which require tweaking the msg.sender to perform a particular action in the contracts.

It's not clear what the details of the use case are, so it's hard to say which UX makes more sense. Note that you can also change the sender used in tests from the CLI with the --sender flag. Therefore just having "no active prank" does not mean the next caller is the default sender, and you'd need to use msg.sender to get the address. The use case I can think of for this cheat is something like getting nonces or state of an account, and in all those cases, you wouldn't actually care if a prank is active or not, and the data you actually is what are the next sender/origin addresses used

For the script scenario, I think this cheat would have to (1) detect that we're in a script environment and revert, or (2) return the address that would be used for the next vm.broadcast() call for both the msg.sender and tx.origin

Hmm, maybe we should wait for him to provide a practical scenario where this cheat code would be used to narrow down the implementation and not diverge from his expected behavior.

PaulRBerg · 2023-05-05T17:34:25Z

we should wait for him to provide a practical scenario

Consider this function:

// src/Foo.sol
contract Foo {
     function doSomethingUseful() external onlyOwner {
         // --- snip --- //
    }
}

And imagine that your code base has hundreds of test files (as our code base does). Now, the use case is to be able to write a subroutine that calls the function above, like this:

// test/MyTest.sol
contract Utils {
    function usefulSubroutine() internal {
        address initialPrank = vm.readPrank();
        changePrank(users.admin);
        foo.doSomethingUseful();
        changePrank(initialPrank);
    }
}

The goal is to perform a call only an admin can perform, then switch back to whatever prank was before.

mds1 · 2023-05-05T21:13:38Z

The goal is to perform a call only an admin can perform, then switch back to whatever prank was before.

So for your use case, either approach would work. With the bool version of cheatcode, you'd:

check the current prank
if there is no prank, use a startPrank(admin) then stopPrank()
if there is a prank, use a startPrank(admin) then startPrank(prevPrankAddr)

With the no bool version, you'd:

check the current callers
do startPrank(admin) then startPrank(prevCallerAddr)

One downside of the second is that now you technically are in a prank state even though you weren't before, but functionality wise everything should be the same. This could be mitigated by forge detecting when a startPrank call matches the default senders and instead acting as a stop prank, but that feels dirty

I think I've convinced myself that bool approach is better for that reason, but that we should change the behavior so it returns the default (or configured via --sender) msg.sender/tx.origin when there is no prank, instead of the zero address

xeno097 · 2023-05-05T21:20:57Z

we should wait for him to provide a practical scenario

Consider this function:
// src/Foo.sol
contract Foo {
     function doSomethingUseful() external onlyOwner {
         // --- snip --- //
    }
}
And imagine that your code base has hundreds of test files (as our code base does). Now, the use case is to be able to write a subroutine that calls the function above, like this:
// test/MyTest.sol
contract Utils {
    function usefulSubroutine() internal {
        address initialPrank = vm.readPrank();
        changePrank(users.admin);
        foo.doSomethingUseful();
        changePrank(initialPrank);
    }
}
The goal is to perform a call only an admin can perform, then switch back to whatever prank was before.

Cool thanks for shedding some light on this. Please @mds1 tell me what should I do next. Do we keep the current implementation or change something?

xeno097 · 2023-05-05T21:32:13Z

The goal is to perform a call only an admin can perform, then switch back to whatever prank was before.

So for your use case, either approach would work. With the bool version of cheatcode, you'd:

check the current prank

if there is no prank, use a startPrank(admin) then stopPrank()

if there is a prank, use a startPrank(admin) then startPrank(prevPrankAddr)

With the no bool version, you'd:

check the current callers

do startPrank(admin) then startPrank(prevCallerAddr)

One downside of the second is that now you technically are in a prank state even though you weren't before, but functionality wise everything should be the same. This could be mitigated by forge detecting when a startPrank call matches the default senders and instead acting as a stop prank, but that feels dirty

I think I've convinced myself that bool approach is better for that reason, but that we should change the behavior so it returns the default (or configured via --sender) msg.sender/tx.origin when there is no prank, instead of the zero address

Nice I'll do that. thank you!

… is set

…k-cheatcode

xeno097 · 2023-05-07T01:32:23Z

The goal is to perform a call only an admin can perform, then switch back to whatever prank was before.

So for your use case, either approach would work. With the bool version of cheatcode, you'd:

check the current prank

if there is no prank, use a startPrank(admin) then stopPrank()

if there is a prank, use a startPrank(admin) then startPrank(prevPrankAddr)

With the no bool version, you'd:

check the current callers

do startPrank(admin) then startPrank(prevCallerAddr)

One downside of the second is that now you technically are in a prank state even though you weren't before, but functionality wise everything should be the same. This could be mitigated by forge detecting when a startPrank call matches the default senders and instead acting as a stop prank, but that feels dirty

I think I've convinced myself that bool approach is better for that reason, but that we should change the behavior so it returns the default (or configured via --sender) msg.sender/tx.origin when there is no prank, instead of the zero address

Done! Sorry for the delayed changes

…k-cheatcode

mds1 · 2023-05-08T17:44:29Z

@xeno097 what approach did you go with for how this cheat behaves in scripts? I don't see any tests using this cheat in a script env so I'm not sure it's currently handled. Quoting from above I think the options are:

For the script scenario, I think this cheat would have to (1) detect that we're in a script environment and revert, or (2) return the address that would be used for the next vm.broadcast() call for both the msg.sender and tx.origin

xeno097 · 2023-05-08T18:27:47Z

@xeno097 what approach did you go with for how this cheat behaves in scripts? I don't see any tests using this cheat in a script env so I'm not sure it's currently handled. Quoting from above I think the options are:

For the script scenario, I think this cheat would have to (1) detect that we're in a script environment and revert, or (2) return the address that would be used for the next vm.broadcast() call for both the msg.sender and tx.origin

Sorry, my bad. Of the 2 options, I prefer the revert one because I think it is cleaner. Maybe later we can add an equivalent cheat code for the broadcasting scenario called readBroadcast to keep things nice and separated. What do you think @mds1?

…new cheatcode logic

…code

…s form read_callers

…k-cheatcode

mattsse · 2023-05-20T11:51:42Z

I haven't followed the recent discussion here closely, but it looks like this is ready for review?

mds1 · 2023-05-22T00:12:40Z

This is not ready for review yet, it needs to be updated to one of the too versions described in #4884 (comment)

@wighawag @PaulRBerg just want to sanity check we're all on board with that approach, and see if either of you have a preference on which of those two we use?

PaulRBerg · 2023-05-22T08:19:54Z

I'd go with the first option described in #4884 (comment), i.e. the one which uses this enum:

enum CallerMode {
  None
  Broadcast
  StartBroadcast
  Prank
  StartPrank
}

It's more explicit and specific; whereas isRecurring could easily be confused to mean something else.

xeno097 · 2023-05-22T13:56:23Z

I'd go with the first option described in #4884 (comment), i.e. the one which uses this enum:
enum CallerMode {
  None
  Broadcast
  StartBroadcast
  Prank
  StartPrank
}
It's more explicit and specific; whereas isRecurring could easily be confused to mean something else.

Nice! I had already updated the implementation with this option last week. Let's see if @wighawag has any comment about this if not I think this is ready for review @mds1 @mattsse

wighawag · 2023-05-22T16:48:31Z

This is good for me, I also prefer the enum version. thanks!

…k-cheatcode

Evalir

I think this looks good! Sorry for the massive delay @xeno097 .

cc @mattsse @mds1 for final approve/merge

Can we please follow up with a forge-std/book PR?

ci failure is also unrelated

evm/src/executor/inspector/cheatcodes/env.rs

Evalir · 2023-06-02T16:28:13Z

testdata/cheats/ReadCallers.t.sol

+    Cheats constant cheats = Cheats(HEVM_ADDRESS);
+
+    function testReadCallersWithNoActivePrankOrBroadcast() public {
+        // Arrange


Could we remove these? :D

yup np 😅

evm/src/executor/inspector/cheatcodes/env.rs

mds1 · 2023-06-02T17:58:35Z

I won't be able to look at this until sometime monday, but cc'ing @wighawag @PaulRBerg in case you have the bandwidth the foundryup -P 4844 and test this PR

…k-cheatcode

mattsse

test+code lgtm!

any objections @mds1 ?

mattsse · 2023-06-06T15:50:29Z

testdata/cheats/Cheats.sol

+    enum CallerMode {
+        None,
+        Broadcast,
+        RecurrentBroadcast,
+        Prank,
+        RecurrentPrank
+    }


this we'd need to move to forge-std?

yea @xeno097 once this is merged do you mind opening a PR to forge-std to add these to VmSafe?

Yep no problem! I think I also need to add a new page to the fpundry book with documentation for the cheatcode right?

Yes please that would be great! 🙏

mds1

lgtm (assuming CI failures are unrelated, deferring to @mattsse / @Evalir there). Thanks a lot @xeno097!

Evalir · 2023-06-06T18:02:41Z

yup ci failure is unrelated. merging!

xeno097 added 5 commits May 4, 2023 23:15

feat(forge): started working on the readPrank cheat code

8a730c1

test(forge): started implementing tests for readPrank cheat code

dc3c231

refactor(forge): refactored read_prank func

47b38d7

test(forge): improved tests for readPrank cheat code

907ee5c

Merge branch 'master' of github.com:foundry-rs/foundry into read-pran…

d88b688

…k-cheatcode

xeno097 changed the title ~~Read prank cheatcode~~ feat(forge): readPrank cheatcode May 5, 2023

xeno097 changed the title ~~feat(forge): readPrank cheatcode~~ feat(forge): readPrank cheatcode May 5, 2023

Evalir requested review from mattsse and Evalir May 5, 2023 15:31

Evalir approved these changes May 5, 2023

View reviewed changes

fix(forge): fixed readPrank solidity test formatting

d90c98e

Evalir self-requested a review May 5, 2023 15:49

chore(forge): forge fmt

7e5ae8c

xeno097 added 2 commits May 6, 2023 20:51

feat(forge): replaced 0 address with default msg.sender when no prank…

b685ce5

… is set

Merge branch 'master' of github.com:foundry-rs/foundry into read-pran…

2a11ead

…k-cheatcode

Merge branch 'master' of github.com:foundry-rs/foundry into read-pran…

2a58047

…k-cheatcode

xeno097 added 6 commits May 17, 2023 21:49

feat(forge): renamed readPrank to readCallers and started working on …

db2455a

…new cheatcode logic

refactor(forge): renamed ReadPrank.t.sol -> ReadCallers.t.sol

49592fa

refactor(forge): updated the read_callers cheat code returndata

bf8170d

test(forge): added tests for broadcasting case for readCallers cheat …

b8a8e24

…code

refactor(forge): added the CallerMode enum to remove hardcoded number…

4834ead

…s form read_callers

Merge branch 'master' of github.com:foundry-rs/foundry into read-pran…

9bc8bd3

…k-cheatcode

xeno097 changed the title ~~feat(forge): readPrank cheatcode~~ feat(forge): readCallers cheatcode May 18, 2023

xeno097 added 3 commits May 28, 2023 19:15

Merge branch 'master' of github.com:foundry-rs/foundry into read-pran…

5916fa9

…k-cheatcode

Merge branch 'master' of github.com:foundry-rs/foundry into read-pran…

375b19e

…k-cheatcode

chore(forge): hevm.rs

6b191a8

Evalir approved these changes Jun 2, 2023

View reviewed changes

xeno097 added 2 commits June 2, 2023 22:28

Merge branch 'master' of github.com:foundry-rs/foundry into read-pran…

cb05f40

…k-cheatcode

docs(forge): added doc comments to read_callers func and CallerMode enum

b1dc89a

mattsse approved these changes Jun 6, 2023

View reviewed changes

mds1 approved these changes Jun 6, 2023

View reviewed changes

Evalir merged commit 42e7153 into foundry-rs:master Jun 6, 2023

xeno097 deleted the read-prank-cheatcode branch June 8, 2023 18:04

This was referenced Jun 8, 2023

feat: add CallerMode enum and readCallers cheat code foundry-rs/forge-std#400

Merged

feat: add readCallers cheatcode docs foundry-rs/book#917

Merged

Evalir mentioned this pull request Jun 25, 2023

feat(forge): cheatcode for getting the current prank #4875

Closed

PaulRBerg mentioned this pull request Jul 20, 2023

Add utility for changing the protocol fee which uses the newly introduced "readCallers" cheat sablier-labs/lockup#628

Closed

Evalir mentioned this pull request Aug 1, 2023

fix(cheatcodes): disallow using vm.prank after vm.startPrank #5520

Merged

Evalir mentioned this pull request Aug 21, 2023

Ability to detect different "environments" in scripts/tests #2900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(forge): readCallers cheatcode #4884

feat(forge): readCallers cheatcode #4884

xeno097 commented May 5, 2023 •

edited

Loading

Evalir left a comment

xeno097 commented May 5, 2023

mds1 commented May 5, 2023

mds1 commented May 5, 2023

xeno097 commented May 5, 2023

xeno097 commented May 5, 2023

mds1 commented May 5, 2023 •

edited

Loading

xeno097 commented May 5, 2023

PaulRBerg commented May 5, 2023 •

edited

Loading

mds1 commented May 5, 2023

xeno097 commented May 5, 2023

xeno097 commented May 5, 2023

xeno097 commented May 7, 2023

mds1 commented May 8, 2023

xeno097 commented May 8, 2023 •

edited

Loading

mattsse commented May 20, 2023

mds1 commented May 22, 2023

PaulRBerg commented May 22, 2023

xeno097 commented May 22, 2023

wighawag commented May 22, 2023

Evalir left a comment •

edited

Loading

Evalir Jun 2, 2023

xeno097 Jun 2, 2023

mds1 commented Jun 2, 2023

mattsse left a comment

mattsse Jun 6, 2023

mds1 Jun 6, 2023 •

edited

Loading

xeno097 Jun 6, 2023

mds1 Jun 7, 2023

mds1 left a comment •

edited

Loading

Evalir commented Jun 6, 2023

feat(forge): readCallers cheatcode #4884

feat(forge): readCallers cheatcode #4884

Conversation

xeno097 commented May 5, 2023 • edited Loading

Motivation

Solution

Evalir left a comment

Choose a reason for hiding this comment

xeno097 commented May 5, 2023

mds1 commented May 5, 2023

mds1 commented May 5, 2023

xeno097 commented May 5, 2023

xeno097 commented May 5, 2023

mds1 commented May 5, 2023 • edited Loading

xeno097 commented May 5, 2023

PaulRBerg commented May 5, 2023 • edited Loading

mds1 commented May 5, 2023

xeno097 commented May 5, 2023

xeno097 commented May 5, 2023

xeno097 commented May 7, 2023

mds1 commented May 8, 2023

xeno097 commented May 8, 2023 • edited Loading

mattsse commented May 20, 2023

mds1 commented May 22, 2023

PaulRBerg commented May 22, 2023

xeno097 commented May 22, 2023

wighawag commented May 22, 2023

Evalir left a comment • edited Loading

Choose a reason for hiding this comment

Evalir Jun 2, 2023

Choose a reason for hiding this comment

xeno097 Jun 2, 2023

Choose a reason for hiding this comment

mds1 commented Jun 2, 2023

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jun 6, 2023

Choose a reason for hiding this comment

mds1 Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

xeno097 Jun 6, 2023

Choose a reason for hiding this comment

mds1 Jun 7, 2023

Choose a reason for hiding this comment

mds1 left a comment • edited Loading

Choose a reason for hiding this comment

Evalir commented Jun 6, 2023

xeno097 commented May 5, 2023 •

edited

Loading

mds1 commented May 5, 2023 •

edited

Loading

PaulRBerg commented May 5, 2023 •

edited

Loading

xeno097 commented May 8, 2023 •

edited

Loading

Evalir left a comment •

edited

Loading

mds1 Jun 6, 2023 •

edited

Loading

mds1 left a comment •

edited

Loading