fix: empty setUp with default sender on forge script (foundry-rs#3817)

* clean-up nonce correction

* dont correct nonce from default sender

* add docs for nonce management on forge script

* fix

* add test

Author: joshieDo

cli/src/cmd/forge/script/runner.rs

@@ -114,15 +114,8 @@ impl ScriptRunner {
                         .extend(setup_traces.map(|traces| (TraceKind::Setup, traces)).into_iter());
                     logs.extend_from_slice(&setup_logs);
 
-                    // We call the `setUp()` function with self.sender, and if there haven't been
-                    // any broadcasts, then the EVM cheatcode module hasn't corrected the nonce.
-                    // So we have to
-                    if transactions.is_none() || transactions.as_ref().unwrap().is_empty() {
-                        self.executor.set_nonce(
-                            self.sender,
-                            sender_nonce.as_u64() + libraries.len() as u64,
-                        )?;
-                    }
+                    self.maybe_correct_nonce(sender_nonce, libraries.len())?;
+
                     (
                         !reverted,
                         gas_used,
@@ -148,15 +141,8 @@ impl ScriptRunner {
                         .extend(setup_traces.map(|traces| (TraceKind::Setup, traces)).into_iter());
                     logs.extend_from_slice(&setup_logs);
 
-                    // We call the `setUp()` function with self.sender, and if there haven't been
-                    // any broadcasts, then the EVM cheatcode module hasn't corrected the nonce.
-                    // So we have to
-                    if transactions.is_none() || transactions.as_ref().unwrap().is_empty() {
-                        self.executor.set_nonce(
-                            self.sender,
-                            sender_nonce.as_u64() + libraries.len() as u64,
-                        )?;
-                    }
+                    self.maybe_correct_nonce(sender_nonce, libraries.len())?;
+
                     (
                         !reverted,
                         gas_used,
@@ -187,6 +173,29 @@ impl ScriptRunner {
         ))
     }
 
+    /// We call the `setUp()` function with self.sender, and if there haven't been
+    /// any broadcasts, then the EVM cheatcode module hasn't corrected the nonce.
+    /// So we have to.
+    fn maybe_correct_nonce(
+        &mut self,
+        sender_initial_nonce: U256,
+        libraries_len: usize,
+    ) -> eyre::Result<()> {
+        if let Some(ref cheatcodes) = self.executor.inspector_config().cheatcodes {
+            if !cheatcodes.corrected_nonce {
+                self.executor
+                    .set_nonce(self.sender, sender_initial_nonce.as_u64() + libraries_len as u64)?;
+            }
+            self.executor
+                .inspector_config_mut()
+                .cheatcodes
+                .as_mut()
+                .expect("exists")
+                .corrected_nonce = false;
+        }
+        Ok(())
+    }
+
     /// Executes the method that will collect all broadcastable transactions.
     pub fn script(&mut self, address: Address, calldata: Bytes) -> eyre::Result<ScriptResult> {
         self.call(self.sender, address, calldata, U256::zero(), false)

cli/test-utils/src/script.rs

@@ -239,6 +239,7 @@ impl ScriptTester {
 /// Various `forge` script results
 #[derive(Debug)]
 pub enum ScriptOutcome {
+    OkNoEndpoint,
     OkSimulation,
     OkBroadcast,
     WarnSpecifyDeployer,
@@ -253,6 +254,7 @@ pub enum ScriptOutcome {
 impl ScriptOutcome {
     pub fn as_str(&self) -> &'static str {
         match self {
+            ScriptOutcome::OkNoEndpoint => "If you wish to simulate on-chain transactions pass a RPC URL.",
             ScriptOutcome::OkSimulation => "SIMULATION COMPLETE. To broadcast these",
             ScriptOutcome::OkBroadcast => "ONCHAIN EXECUTION COMPLETE & SUCCESSFUL",
             ScriptOutcome::WarnSpecifyDeployer => "You have more than one deployer who could predeploy libraries. Using `--sender` instead.",
@@ -267,6 +269,7 @@ impl ScriptOutcome {
 
     pub fn is_err(&self) -> bool {
         match self {
+            ScriptOutcome::OkNoEndpoint |
             ScriptOutcome::OkSimulation |
             ScriptOutcome::OkBroadcast |
             ScriptOutcome::WarnSpecifyDeployer => false,

cli/tests/it/script.rs

@@ -848,3 +848,12 @@ contract Demo {
             .join("tests/fixtures/can_execute_script_and_skip_contracts.stdout"),
     );
 });
+
+forgetest_async!(
+    can_run_script_with_empty_setup,
+    |prj: TestProject, cmd: TestCommand| async move {
+        let mut tester = ScriptTester::new_broadcast_without_endpoint(cmd, prj.root());
+
+        tester.add_sig("BroadcastEmptySetUp", "run()").simulate(ScriptOutcome::OkNoEndpoint);
+    }
+);

docs/dev/scripting.md

@@ -4,6 +4,7 @@
 1. [High level overview](#high-level-overview)
     1. [Notes](#notes)
 2. [Script Execution](#script-execution)
+3. [Nonce Management](#nonce-management)
 
 ## High level overview
 
@@ -84,3 +85,54 @@ Executor::call-. BroadcastableTransactions .->ScriptArgs::handle_broadcastable_t
 
 ```
 
+
+## Nonce Management
+
+During the first execution stage on `forge script`, foundry has to adjust the nonce from the sender to make sure the execution and state are as close as possible to its on-chain representation.
+
+Making sure that `msg.sender` is our signer when calling `setUp()` and `run()` and that its nonce is correct (decreased by one on each call) when calling `vm.broadcast` to create a contract.
+
+We skip this, if the user hasn't set a sender and they're using the `Config::DEFAULT_SENDER`.
+
+
+```mermaid
+graph TD
+
+    ScriptRunner::setup-->default_foundry_caller-deployScript;
+    default_foundry_caller-deployScript-->user_sender-deployLibs;
+    user_sender-deployLibs-->Contract.setUp;
+    Contract.setUp-->A0{Executor::call};
+    A0-->vm.broadcast;
+    A0-->vm.startBroadcast;
+    A0-->vm.getNonce;
+
+    vm.broadcast--> A{cheatcode.corrected_nonce}
+    vm.startBroadcast-->A
+    vm.getNonce-->A
+
+    A--true-->continue_setUp;
+    A--false-->B[sender_nonce=-1];
+    B-->C[cheatcode.corrected_nonce=true];
+    C-->continue_setUp;
+    continue_setUp-->end_setUp;
+    end_setUp-->D{cheatcode.corrected_nonce}
+    D--true-->E[cheatcode.corrected_nonce=false];
+    D--false-->F[sender_nonce=initial_nonce+predeployed_libraries_count];
+    E-->ScriptRunner::script;
+    F-->ScriptRunner::script;
+    ScriptRunner::script-->Contract.run;
+    Contract.run-->G{Executor::call};
+    G-->H[vm.broadcast];
+    G-->I[vm.startBroadcast];
+    G-->J[vm.getNonce];
+
+    H--> K{cheatcode.corrected_nonce}
+    I-->K
+    J-->K
+
+    K--true-->continue_run;
+    K--false-->L[sender_nonce=-1];
+    L-->M[cheatcode.corrected_nonce=true];
+    M-->continue_run;
+    continue_run-->end_run;
+```

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

@@ -20,6 +20,7 @@ use ethers::{
     signers::{LocalWallet, Signer},
     types::{Address, U256},
 };
+use foundry_config::Config;
 use revm::{Bytecode, Database, EVMData};
 use tracing::trace;
 
@@ -454,7 +455,7 @@ fn correct_sender_nonce<DB: Database>(
     db: &mut DB,
     state: &mut Cheatcodes,
 ) -> Result<(), DB::Error> {
-    if !state.corrected_nonce {
+    if !state.corrected_nonce && sender != Config::DEFAULT_SENDER {
         with_journaled_account(journaled_state, db, sender, |account| {
             account.info.nonce = account.info.nonce.saturating_sub(1);
             state.corrected_nonce = true;

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

@@ -124,7 +124,8 @@ pub struct Cheatcodes {
     /// Current broadcasting information
     pub broadcast: Option<Broadcast>,
 
-    /// Used to correct the nonce of --sender after the initiating call
+    /// Used to correct the nonce of --sender after the initiating call. For more, check
+    /// `docs/scripting`.
     pub corrected_nonce: bool,
 
     /// Scripting based transactions

evm/src/executor/mod.rs

@@ -373,11 +373,11 @@ impl Executor {
         // Persist cheatcode state
         let mut cheatcodes = result.cheatcodes.take();
         if let Some(cheats) = cheatcodes.as_mut() {
-            if !cheats.broadcastable_transactions.is_empty() {
-                // Clear broadcast state from cheatcode state
-                cheats.broadcastable_transactions.clear();
-                cheats.corrected_nonce = false;
-            }
+            // Clear broadcastable transactions
+            cheats.broadcastable_transactions.clear();
+
+            // corrected_nonce value is needed outside of this context (setUp), so we don't
+            // reset it.
         }
         self.inspector_config.cheatcodes = cheatcodes;
     }

testdata/cheats/Broadcast.t.sol

@@ -424,3 +424,14 @@ contract MultiChainBroadcastLink is DSTest {
         new Test();
     }
 }
+
+contract BroadcastEmptySetUp is DSTest {
+    Cheats constant cheats = Cheats(HEVM_ADDRESS);
+
+    function setUp() public {}
+
+    function run() public {
+        cheats.broadcast();
+        new Test();
+    }
+}