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

bug(cheatcodes): vm.getNonce(player) the nonce is incorrect #8811

Closed
EthanOK opened this issue Sep 5, 2024 Discussed in #8584 · 7 comments · Fixed by #8854 or #8878
Closed

bug(cheatcodes): vm.getNonce(player) the nonce is incorrect #8811

EthanOK opened this issue Sep 5, 2024 Discussed in #8584 · 7 comments · Fixed by #8854 or #8878
Assignees
Labels
A-cheatcodes Area: cheatcodes T-bug Type: bug T-to-investigate Type: to investigate
Milestone

Comments

@EthanOK
Copy link

EthanOK commented Sep 5, 2024

Discussed in #8584

Originally posted by EthanOK August 2, 2024
In the latest version, it will only be increased in the new Contract, but not in the calling contract.
Test demo

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import {Test} from "forge-std/Test.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract VmNonceTest is Test {
    address public player = vm.addr(666666);

    function setUp() external {}

    function testNonce() public {
        vm.startPrank(player, player);
        ERC20 token = new ERC20("", "");
        token.transfer(address(2), 0);
        assertEq(2, vm.getNonce(player));
        vm.stopPrank();
    }

    function testNonce_2() public {
        vm.startPrank(player, player);
        new ERC20("", "");
        new ERC20("", "");
        assertEq(2, vm.getNonce(player));
        vm.stopPrank();
    }
}

forge --version
forge 0.2.0 (0116be1 2024-07-09T00:18:45.429480000Z)

forge test --match-contract VmNonceTest -vvv

[FAIL. Reason: assertion failed: 2 != 1] testNonce() (gas: 479181)
Traces:
  [479181] VmNonceTest::testNonce()
    ├─ [0] VM::startPrank(0xacB4888c9780e1edf78Ad7d3b3Ef5084a9c94895, 0xacB4888c9780e1edf78Ad7d3b3Ef5084a9c94895)
    │   └─ ← [Return] 
    ├─ [432587] → new ERC20@0x1eaEcB92761a82978FdE14BCdd01383fb84162Fe
    │   └─ ← [Return] 2130 bytes of code
    ├─ [7138] ERC20::transfer(0x0000000000000000000000000000000000000002, 0)
    │   ├─ emit Transfer(from: 0xacB4888c9780e1edf78Ad7d3b3Ef5084a9c94895, to: 0x0000000000000000000000000000000000000002, value: 0)
    │   └─ ← [Return] true
    ├─ [0] VM::getNonce(0xacB4888c9780e1edf78Ad7d3b3Ef5084a9c94895) [staticcall]
    │   └─ ← [Return] 1
    ├─ [0] VM::assertEq(2, 1) [staticcall]
    │   └─ ← [Revert] assertion failed: 2 != 1
    └─ ← [Revert] assertion failed: 2 != 1

[PASS] testNonce_2() (gas: 937254)
```</div>
@zerosnacks zerosnacks added T-bug Type: bug A-cheatcodes Area: cheatcodes labels Sep 5, 2024
@zerosnacks zerosnacks changed the title Forge Test: vm.getNonce(player), Obtaining the nonce is incorrect. bug(cheatcodes): vm.getNonce(player) the nonce is incorrect Sep 5, 2024
@zerosnacks zerosnacks added the T-to-investigate Type: to investigate label Sep 6, 2024
@mattsse
Copy link
Member

mattsse commented Sep 6, 2024

@grandizzy it looks like we're not catching the prank during token.transfer(address(2), 0);

@grandizzy grandizzy self-assigned this Sep 6, 2024
@klkvr
Copy link
Member

klkvr commented Sep 8, 2024

I think it's working as expected? external calls shouldn't increase nonce without --isolate

@klkvr
Copy link
Member

klkvr commented Sep 8, 2024

It works like this even with --isolate actually, for consistency

@grandizzy
Copy link
Collaborator

to prevent confusion, wonder if it make sense adding a vm.getPrankNonce(address) cheatcode where we'd keep a parallel track of nonce pranked addresses, @klkvr @mattsse wdyt?

@mds1
Copy link
Collaborator

mds1 commented Sep 11, 2024

external calls shouldn't increase nonce without --isolate

Oh really, why is this? It seems preferable for nonces to behave normally. Incrementing them for new contract deploys but not for calls feels like a bug to me

I don't like vm.getPrankNonce(address) because it should apply for the test contract too

function testFoo() public {
    console.log(vm.getNonce(address(this)));

    counter.increment();
    console.log(vm.getNonce(address(this)));

    new Counter();
    console.log(vm.getNonce(address(this)));
}

Logs:

Ran 1 test for test/Counter.t.sol:CounterTest
[PASS] testFoo() (gas: 117056)
Logs:
  2
  2
  3

But I would expect

Ran 1 test for test/Counter.t.sol:CounterTest
[PASS] testFoo() (gas: 117056)
Logs:
  2
  3
  4

@klkvr
Copy link
Member

klkvr commented Sep 11, 2024

external calls shouldn't increase nonce without --isolate

Oh really, why is this? It seems preferable for nonces to behave normally. Incrementing them for new contract deploys but not for calls feels like a bug to me

when we were adding --isolate, I've focused on making it as much backwards compatible as possible (benchmark was foundry's CI passing), there were some tests hardcoding addresses which were breaking if we've increased nonce correctly

though I think it probably makes sense to break this now exclusively for --isolate

@grandizzy
Copy link
Collaborator

reopening as PR was reverted for further improvements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes T-bug Type: bug T-to-investigate Type: to investigate
Projects
None yet
6 participants