feat(cheatcodes): Make expectEmit only work for the next call

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

@@ -123,47 +123,62 @@ pub struct ExpectedEmit {
 }
 
 pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address) {
-    // Fill or check the expected emits
-    if let Some(next_expect_to_fill) =
-        state.expected_emits.iter_mut().find(|expect| expect.log.is_none())
-    {
-        // We have unfilled expects, so we fill the first one
-        next_expect_to_fill.log = Some(log);
-    } else if let Some(next_expect) = state.expected_emits.iter_mut().find(|expect| !expect.found) {
-        // We do not have unfilled expects, so we try to match this log with the first unfound
-        // log that we expect
-        let expected =
-            next_expect.log.as_ref().expect("we should have a log to compare against here");
-
-        let expected_topic_0 = expected.topics.get(0);
-        let log_topic_0 = log.topics.get(0);
-
-        // same topic0 and equal number of topics should be verified further, others are a no
-        // match
-        if expected_topic_0
-            .zip(log_topic_0)
-            .map_or(false, |(a, b)| a == b && expected.topics.len() == log.topics.len())
-        {
-            // Match topics
-            next_expect.found = log
-                .topics
-                .iter()
-                .skip(1)
-                .enumerate()
-                .filter(|(i, _)| next_expect.checks[*i])
-                .all(|(i, topic)| topic == &expected.topics[i + 1]);
-
-            // Maybe match source address
-            if let Some(addr) = next_expect.address {
-                next_expect.found &= addr == *address;
-            }
+    // Fill or check the expected emits.
+    // We expect for emit checks to be filled as they're declared (from oldest to newest),
+    // so we fill them and push them to the back of the queue.
+    // If the user has properly filled all the emits, they'll end up in their original order.
+    // If not, we can detect this later and error out.
+
+    // if there's anything to fill, we need to pop back.
+    let event_to_fill_or_check =
+        if state.expected_emits.iter().any(|expected| expected.log.is_none()) {
+            state.expected_emits.pop_back()
+        } else {
+            // Else, we need to pop from the front in the order the events were added to the queue.
+            state.expected_emits.pop_front()
+        };
+
+    let mut event_to_fill_or_check =
+        event_to_fill_or_check.expect("We should have an emit to fill or check. This is a bug");
 
-            // Maybe match data
-            if next_expect.checks[3] {
-                next_expect.found &= expected.data == log.data;
+    match event_to_fill_or_check.log {
+        Some(ref expected) => {
+            let expected_topic_0 = expected.topics.get(0);
+            let log_topic_0 = log.topics.get(0);
+
+            // same topic0 and equal number of topics should be verified further, others are a no
+            // match
+            if expected_topic_0
+                .zip(log_topic_0)
+                .map_or(false, |(a, b)| a == b && expected.topics.len() == log.topics.len())
+            {
+                // Match topics
+                event_to_fill_or_check.found = log
+                    .topics
+                    .iter()
+                    .skip(1)
+                    .enumerate()
+                    .filter(|(i, _)| event_to_fill_or_check.checks[*i])
+                    .all(|(i, topic)| topic == &expected.topics[i + 1]);
+
+                // Maybe match source address
+                if let Some(addr) = event_to_fill_or_check.address {
+                    event_to_fill_or_check.found &= addr == *address;
+                }
+
+                // Maybe match data
+                if event_to_fill_or_check.checks[3] {
+                    event_to_fill_or_check.found &= expected.data == log.data;
+                }
             }
         }
+        // Fill the event.
+        None => {
+            event_to_fill_or_check.log = Some(log);
+        }
     }
+
+    state.expected_emits.push_back(event_to_fill_or_check);
 }
 
 #[derive(Clone, Debug, Default)]
@@ -234,33 +249,33 @@ pub fn apply<DB: DatabaseExt>(
             expect_revert(state, Some(inner.0.into()), data.journaled_state.depth())
         }
         HEVMCalls::ExpectEmit0(_) => {
-            state.expected_emits.push(ExpectedEmit {
-                depth: data.journaled_state.depth() - 1,
+            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(ExpectedEmit {
-                depth: data.journaled_state.depth() - 1,
+            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(ExpectedEmit {
-                depth: data.journaled_state.depth() - 1,
+            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(ExpectedEmit {
-                depth: data.journaled_state.depth() - 1,
+            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()

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

@@ -29,7 +29,7 @@ use revm::{
 };
 use serde_json::Value;
 use std::{
-    collections::{BTreeMap, HashMap},
+    collections::{BTreeMap, HashMap, VecDeque},
     fs::File,
     io::BufReader,
     ops::Range,
@@ -129,7 +129,7 @@ pub struct Cheatcodes {
     pub expected_calls: BTreeMap<Address, Vec<(ExpectedCallData, u64)>>,
 
     /// Expected emits
-    pub expected_emits: Vec<ExpectedEmit>,
+    pub expected_emits: VecDeque<ExpectedEmit>,
 
     /// Map of context depths to memory offset ranges that may be written to within the call depth.
     pub allowed_mem_writes: BTreeMap<u64, Vec<Range<u64>>>,
@@ -753,21 +753,36 @@ where
             }
         }
 
-        // Handle expected emits at current depth
-        if !self
+        // At the end of the call,
+        // we need to check if we've found all the emits.
+        // We know we've found all the expected emits in the right order
+        // if the queue is fully matched.
+        // If it's not fully matched, then either:
+        // 1. Not enough events were emitted (we'll know this because the amount of times we
+        // inspected events will be less than the size of the queue) 2. The wrong events
+        // were emitted (The inspected events should match the size of the queue, but still some
+        // events will not be matched)
+
+        // First, check that we're at the call depth where the emits were declared from.
+        let should_check_emits = self
             .expected_emits
             .iter()
-            .filter(|expected| expected.depth == data.journaled_state.depth())
-            .all(|expected| expected.found)
-        {
-            return (
-                InstructionResult::Revert,
-                remaining_gas,
-                "Log != expected log".to_string().encode().into(),
-            )
-        } else {
-            // Clear the emits we expected at this depth that have been found
-            self.expected_emits.retain(|expected| !expected.found)
+            .any(|expected| expected.depth == data.journaled_state.depth());
+        // If so, check the emits
+        if should_check_emits {
+            // Not all emits were matched.
+            if self.expected_emits.iter().any(|expected| !expected.found) {
+                return (
+                    InstructionResult::Revert,
+                    remaining_gas,
+                    "Log != expected log".to_string().encode().into(),
+                )
+            } else {
+                // All emits were found, we're good.
+                // Clear the queue, as we expect the user to declare more events for the next call
+                // if they wanna match further events.
+                self.expected_emits.clear()
+            }
         }
 
         // If the depth is 0, then this is the root call terminating
@@ -818,7 +833,7 @@ where
                 return (
                     InstructionResult::Revert,
                     remaining_gas,
-                    "Expected an emit, but no logs were emitted afterward"
+                    "Expected an emit, but no logs were emitted afterward. You might have mismatched events or not enough events were emitted."
                         .to_string()
                         .encode()
                         .into(),

testdata/cheats/ExpectEmit.t.sol

@@ -29,6 +29,11 @@ contract Emitter {
         emit Something(topic1[1], topic2[1], topic3[1], data[1]);
     }
 
+    function emitAndNest() public {
+        emit Something(1, 2, 3, 4);
+        emitNested(Emitter(address(this)), 1, 2, 3, 4);
+    }
+
     function emitNested(Emitter inner, uint256 topic1, uint256 topic2, uint256 topic3, uint256 data) public {
         inner.emitEvent(topic1, topic2, topic3, data);
     }
@@ -182,6 +187,15 @@ contract ExpectEmitTest is DSTest {
         );
     }
 
+    function testExpectedEmitMultipleNested() public {
+        cheats.expectEmit();
+        emit Something(1, 2, 3, 4);
+        cheats.expectEmit();
+        emit Something(1, 2, 3, 4);
+
+        emitter.emitAndNest();
+    }
+
     function testExpectEmitMultipleWithArgs() public {
         cheats.expectEmit(true, true, true, true);
         emit Something(1, 2, 3, 4);
@@ -232,6 +246,18 @@ contract ExpectEmitTest is DSTest {
         caller.f();
     }
 
+    function testFailNoEmitDirectlyOnNextCall() public {
+        LowLevelCaller caller = new LowLevelCaller();
+
+        cheats.expectEmit(true, true, true, true);
+        emit Something(1, 2, 3, 4);
+
+        // This call does not emit. As emit expects the next call to emit, this should fail.
+        caller.f();
+        // This call does emit, but it is a call later than expected.
+        emitter.emitEvent(1, 2, 3, 4);
+    }
+
     /// Ref: issue #760
     function testFailDifferentIndexedParameters() public {
         cheats.expectEmit(true, false, false, false);
@@ -250,14 +276,15 @@ contract ExpectEmitTest is DSTest {
     /// was invoked ends.
     ///
     /// Ref: issue #1214
-    function testExpectEmitIsCheckedWhenCurrentCallTerminates() public {
-        cheats.expectEmit(true, true, true, true);
-        emitter.doesNothing();
-        emit Something(1, 2, 3, 4);
-
-        // This should fail since `SomethingElse` in the test
-        // and in the `Emitter` contract have differing
-        // amounts of indexed topics.
-        emitter.emitEvent(1, 2, 3, 4);
-    }
+    /// Note: this is now illegal behaviour.
+    // function testExpectEmitIsCheckedWhenCurrentCallTerminates() public {
+    //     cheats.expectEmit(true, true, true, true);
+    //     emitter.doesNothing();
+    //     emit Something(1, 2, 3, 4);
+
+    //     // This should fail since `SomethingElse` in the test
+    //     // and in the `Emitter` contract have differing
+    //     // amounts of indexed topics.
+    //     emitter.emitEvent(1, 2, 3, 4);
+    // }
 }

testdata/cheats/GetDeployedCode.t.sol

@@ -33,8 +33,8 @@ contract GetDeployedCodeTest is DSTest {
         Override over = Override(overrideAddress);
 
         vm.expectEmit(true, false, false, true);
-        over.emitPayload(address(0), "hello");
         emit Payload(address(this), address(0), "hello");
+        over.emitPayload(address(0), "hello");
     }
 }