feat(cheatcodes): 1.0 cheatcode changes (#5045)

* feat(`cheatcodes`): Make expectCall only work for the next call's subcalls (#5032) * chore: make expect call only work for the next call * chore: make expectCall actually check only the next call's subcalls * chore: fmt * chore: introduce checks at the main call level, not at the subcall level * chore: handle dangling expected calls gracefully * chore: fix tests * chore: fmt * chore: forge fmt * chore: actually exclude depth the cheatcode was called from * chore: tests * chore: better docs * chore: comment out impossible to check condition on expectCall * chore: remove unused check * fix(cheatcodes): Correct `expectRevert` behavior (#4945) * chore: add repro test to pass * chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end * chore: tests * chore: add more repro tests * chore: fmt * chore: clippy * chore: fixup problematic tests, mark them as not working properly * chore: forge fmt * chore: forge fmt * Update evm/src/executor/inspector/cheatcodes/mod.rs * chore: add more info to changelog * chore: fmt * chore(tests): add more cases for `expectEmit` (#5076) * chore(tests): add more extreme cases for expectEmit * chore(tests): add next call fail case for expectEmit * chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135) * chore: add edge-cases * chore: add edge case covering #4920 (comment) * feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit` (#5144) * feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert * chore: add tests * chore: fmt * chore: fmt
foundry-rs · Jun 26, 2023 · 86a2440 · 86a2440
1 parent 794f831
commit 86a2440
Show file tree

Hide file tree

Showing 22 changed files with 842 additions and 151 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,8 +27,9 @@ To use the latest pinned nightly on your CI, modify your Foundry installation st
 
 - [expectEmit](https://github.com/foundry-rs/foundry/pull/4920) will now only work for the next call.
 - expectCall will now only work if the call(s) are made exactly after the cheatcode is invoked.
+- [expectRevert will now work if the next call does revert](https://github.com/foundry-rs/foundry/pull/4945), instead of expecting a revert during the whole test.
+  - This will very likely break your tests. Please make sure that all the calls you expect to revert are external, and if not, abstract them into a separate contract so that they can be called externally and the cheatcode can be used.
 - `-m`, the deprecated alias for `--mt` or `--match-test`, has now been removed.
-- expectRevert will now work if the next call does revert, instead of expecting a revert during the whole test.
 - [startPrank will now override the existing prank instead of erroring](https://github.com/foundry-rs/foundry/pull/4826).
 - [precompiles will not be compatible with all cheatcodes](https://github.com/foundry-rs/foundry/pull/4905).
 - The difficulty and prevrandao cheatcodes now [fail if not used with the correct EVM version](https://github.com/foundry-rs/foundry/pull/4904).

diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs
@@ -45,6 +45,16 @@ fn expect_revert(state: &mut Cheatcodes, reason: Option<Bytes>, depth: u64) -> R
     Ok(Bytes::new())
 }
 
+fn expect_emit(
+    state: &mut Cheatcodes,
+    address: Option<H160>,
+    depth: u64,
+    checks: [bool; 4],
+) -> Result {
+    state.expected_emits.push_back(ExpectedEmit { depth, address, checks, ..Default::default() });
+    Ok(Bytes::new())
+}
+
 #[instrument(skip_all, fields(expected_revert, status, retdata = hex::encode(&retdata)))]
 pub fn handle_expect_revert(
     is_create: bool,
@@ -213,6 +223,8 @@ pub struct ExpectedCallData {
     pub count: u64,
     /// The type of call
     pub call_type: ExpectedCallType,
+    /// The depth at which this call must be checked
+    pub depth: u64,
 }
 
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
@@ -289,6 +301,7 @@ fn expect_call(
     min_gas: Option<u64>,
     count: u64,
     call_type: ExpectedCallType,
+    depth: u64,
 ) -> Result {
     match call_type {
         ExpectedCallType::Count => {
@@ -300,8 +313,10 @@ fn expect_call(
                 !expecteds.contains_key(&calldata),
                 "Counted expected calls can only bet set once."
             );
-            expecteds
-                .insert(calldata, (ExpectedCallData { value, gas, min_gas, count, call_type }, 0));
+            expecteds.insert(
+                calldata,
+                (ExpectedCallData { value, gas, min_gas, count, call_type, depth }, 0),
+            );
             Ok(Bytes::new())
         }
         ExpectedCallType::NonCount => {
@@ -319,7 +334,7 @@ fn expect_call(
                 // If it does not exist, then create it.
                 expecteds.insert(
                     calldata,
-                    (ExpectedCallData { value, gas, min_gas, count, call_type }, 0),
+                    (ExpectedCallData { value, gas, min_gas, count, call_type, depth }, 0),
                 );
             }
             Ok(Bytes::new())
@@ -342,39 +357,26 @@ pub fn apply<DB: DatabaseExt>(
             expect_revert(state, Some(inner.0.into()), data.journaled_state.depth())
         }
         HEVMCalls::ExpectEmit0(_) => {
-            state.expected_emits.push_back(ExpectedEmit {
-                depth: data.journaled_state.depth(),
-                checks: [true, true, true, true],
-                ..Default::default()
-            });
-            Ok(Bytes::new())
-        }
-        HEVMCalls::ExpectEmit1(inner) => {
-            state.expected_emits.push_back(ExpectedEmit {
-                depth: data.journaled_state.depth(),
-                checks: [true, true, true, true],
-                address: Some(inner.0),
-                ..Default::default()
-            });
-            Ok(Bytes::new())
-        }
-        HEVMCalls::ExpectEmit2(inner) => {
-            state.expected_emits.push_back(ExpectedEmit {
-                depth: data.journaled_state.depth(),
-                checks: [inner.0, inner.1, inner.2, inner.3],
-                ..Default::default()
-            });
-            Ok(Bytes::new())
-        }
-        HEVMCalls::ExpectEmit3(inner) => {
-            state.expected_emits.push_back(ExpectedEmit {
-                depth: data.journaled_state.depth(),
-                checks: [inner.0, inner.1, inner.2, inner.3],
-                address: Some(inner.4),
-                ..Default::default()
-            });
-            Ok(Bytes::new())
+            expect_emit(state, None, data.journaled_state.depth(), [true, true, true, true])
         }
+        HEVMCalls::ExpectEmit1(inner) => expect_emit(
+            state,
+            Some(inner.0),
+            data.journaled_state.depth(),
+            [true, true, true, true],
+        ),
+        HEVMCalls::ExpectEmit2(inner) => expect_emit(
+            state,
+            None,
+            data.journaled_state.depth(),
+            [inner.0, inner.1, inner.2, inner.3],
+        ),
+        HEVMCalls::ExpectEmit3(inner) => expect_emit(
+            state,
+            Some(inner.4),
+            data.journaled_state.depth(),
+            [inner.0, inner.1, inner.2, inner.3],
+        ),
         HEVMCalls::ExpectCall0(inner) => expect_call(
             state,
             inner.0,
@@ -384,6 +386,7 @@ pub fn apply<DB: DatabaseExt>(
             None,
             1,
             ExpectedCallType::NonCount,
+            data.journaled_state.depth(),
         ),
         HEVMCalls::ExpectCall1(inner) => expect_call(
             state,
@@ -394,6 +397,7 @@ pub fn apply<DB: DatabaseExt>(
             None,
             inner.2,
             ExpectedCallType::Count,
+            data.journaled_state.depth(),
         ),
         HEVMCalls::ExpectCall2(inner) => expect_call(
             state,
@@ -404,6 +408,7 @@ pub fn apply<DB: DatabaseExt>(
             None,
             1,
             ExpectedCallType::NonCount,
+            data.journaled_state.depth(),
         ),
         HEVMCalls::ExpectCall3(inner) => expect_call(
             state,
@@ -414,6 +419,7 @@ pub fn apply<DB: DatabaseExt>(
             None,
             inner.3,
             ExpectedCallType::Count,
+            data.journaled_state.depth(),
         ),
         HEVMCalls::ExpectCall4(inner) => {
             let value = inner.1;
@@ -430,6 +436,7 @@ pub fn apply<DB: DatabaseExt>(
                 None,
                 1,
                 ExpectedCallType::NonCount,
+                data.journaled_state.depth(),
             )
         }
         HEVMCalls::ExpectCall5(inner) => {
@@ -447,6 +454,7 @@ pub fn apply<DB: DatabaseExt>(
                 None,
                 inner.4,
                 ExpectedCallType::Count,
+                data.journaled_state.depth(),
             )
         }
         HEVMCalls::ExpectCallMinGas0(inner) => {
@@ -464,6 +472,7 @@ pub fn apply<DB: DatabaseExt>(
                 Some(inner.2 + positive_value_cost_stipend),
                 1,
                 ExpectedCallType::NonCount,
+                data.journaled_state.depth(),
             )
         }
         HEVMCalls::ExpectCallMinGas1(inner) => {
@@ -481,6 +490,7 @@ pub fn apply<DB: DatabaseExt>(
                 Some(inner.2 + positive_value_cost_stipend),
                 inner.4,
                 ExpectedCallType::Count,
+                data.journaled_state.depth(),
             )
         }
         HEVMCalls::MockCall0(inner) => {

diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs
@@ -591,7 +591,10 @@ where
                         // The gas matches, if provided
                         expected.gas.map_or(true, |gas| gas == call.gas_limit) &&
                         // The minimum gas matches, if provided
-                        expected.min_gas.map_or(true, |min_gas| min_gas <= call.gas_limit)
+                        expected.min_gas.map_or(true, |min_gas| min_gas <= call.gas_limit) &&
+                        // The expected depth is smaller than the actual depth,
+                        // which means we're in the subcalls of the call were we expect to find the matches.
+                        expected.depth < data.journaled_state.depth()
                     {
                         *actual_count += 1;
                     }
@@ -767,7 +770,16 @@ where
 
         // Handle expected reverts
         if let Some(expected_revert) = &self.expected_revert {
-            if data.journaled_state.depth() <= expected_revert.depth {
+            // Irrespective of whether a revert will be matched or not, disallow having expected
+            // reverts alongside expected emits or calls.
+            if !self.expected_calls.is_empty() || !self.expected_emits.is_empty() {
+                return (
+                    InstructionResult::Revert,
+                    remaining_gas,
+                    "Cannot expect a function to revert while trying to match expected calls or events.".to_string().encode().into(),
+                )
+            }
+            if data.journaled_state.depth() == expected_revert.depth {
                 let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
                 return match handle_expect_revert(
                     false,
@@ -818,14 +830,18 @@ where
             }
         }
 
-        // If the depth is 0, then this is the root call terminating
-        if data.journaled_state.depth() == 0 {
-            // Match expected calls
-            for (address, calldatas) in &self.expected_calls {
-                // Loop over each address, and for each address, loop over each calldata it expects.
-                for (calldata, (expected, actual_count)) in calldatas {
-                    // Grab the values we expect to see
-                    let ExpectedCallData { gas, min_gas, value, count, call_type } = expected;
+        // Match expected calls
+        for (address, calldatas) in &self.expected_calls {
+            // Loop over each address, and for each address, loop over each calldata it expects.
+            for (calldata, (expected, actual_count)) in calldatas {
+                // Grab the values we expect to see
+                let ExpectedCallData { gas, min_gas, value, count, call_type, depth } = expected;
+                // Only check calls in the corresponding depth,
+                // or if the expected depth is higher than the current depth. This is correct, as
+                // the expected depth can only be bigger than the current depth if
+                // we're either terminating the root call (the test itself), or exiting the intended
+                // call that contained the calls we expected to see.
+                if depth >= &data.journaled_state.depth() {
                     let calldata = Bytes::from(calldata.clone());
 
                     // We must match differently depending on the type of call we expect.
@@ -884,6 +900,18 @@ where
                     }
                 }
             }
+        }
+
+        // If the depth is 0, then this is the root call terminating
+        if data.journaled_state.depth() == 0 {
+            // See if there's a dangling expectRevert that should've been matched.
+            if self.expected_revert.is_some() {
+                return (
+                    InstructionResult::Revert,
+                    remaining_gas,
+                    "A `vm.expectRevert`was left dangling. Make sure that calls you expect to revert are external".encode().into(),
+                )
+            }
 
             // Check if we have any leftover expected emits
             // First, if any emits were found at the root call, then we its ok and we remove them.
@@ -1048,7 +1076,7 @@ where
 
         // Handle expected reverts
         if let Some(expected_revert) = &self.expected_revert {
-            if data.journaled_state.depth() <= expected_revert.depth {
+            if data.journaled_state.depth() == expected_revert.depth {
                 let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
                 return match handle_expect_revert(
                     true,

diff --git a/forge/tests/it/repros.rs b/forge/tests/it/repros.rs
@@ -149,6 +149,12 @@ fn test_issue_3221() {
     test_repro!("Issue3221");
 }
 
+// <https://github.com/foundry-rs/foundry/issues/3437>
+#[test]
+fn test_issue_3437() {
+    test_repro!("Issue3437");
+}
+
 // <https://github.com/foundry-rs/foundry/issues/3708>
 #[test]
 fn test_issue_3708() {
@@ -238,6 +244,12 @@ fn test_issue_3703() {
     test_repro!("Issue3703");
 }
 
+// <https://github.com/foundry-rs/foundry/issues/3723>
+#[test]
+fn test_issue_3723() {
+    test_repro!("Issue3723");
+}
+
 // <https://github.com/foundry-rs/foundry/issues/3753>
 #[test]
 fn test_issue_3753() {
@@ -256,6 +268,12 @@ fn test_issue_4586() {
     test_repro!("Issue4586");
 }
 
+// https://github.com/foundry-rs/foundry/issues/4832
+#[test]
+fn test_issue_4832() {
+    test_repro!("Issue4832");
+}
+
 // <https://github.com/foundry-rs/foundry/issues/5038>
 #[test]
 fn test_issue_5038() {

diff --git a/testdata/cheats/Etch.t.sol b/testdata/cheats/Etch.t.sol
@@ -14,12 +14,12 @@ contract EtchTest is DSTest {
         assertEq(string(code), string(target.code));
     }
 
-    function testEtchNotAvailableOnPrecompiles() public {
-        address target = address(1);
-        bytes memory code = hex"1010";
-        cheats.expectRevert(
-            bytes("Etch cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead")
-        );
-        cheats.etch(target, code);
-    }
+    // function testEtchNotAvailableOnPrecompiles() public {
+    //     address target = address(1);
+    //     bytes memory code = hex"1010";
+    //     cheats.expectRevert(
+    //         bytes("Etch cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead")
+    //     );
+    //     cheats.etch(target, code);
+    // }
 }