From d421ee826a065d162f90d8a2de114ab7f960f0fc Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Wed, 10 May 2023 18:41:55 -0400 Subject: [PATCH 01/15] chore: add new expect emit logic --- .../executor/inspector/cheatcodes/expect.rs | 105 ++++++++++-------- 1 file changed, 60 insertions(+), 45 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index a657dfd86960..2e802ae6365a 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/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.0.iter().any(|expected| expected.log.is_none()) { + state.expected_emits.0.pop_back() + } else { + // Else, we need to pop from the front in the order the events were added to the queue. + state.expected_emits.0.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.0.push_back(event_to_fill_or_check); } #[derive(Clone, Debug, Default)] @@ -234,16 +249,16 @@ pub fn apply( 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.0.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.0.push_back(ExpectedEmit { + depth: data.journaled_state.depth(), checks: [true, true, true, true], address: Some(inner.0), ..Default::default() @@ -251,16 +266,16 @@ pub fn apply( Ok(Bytes::new()) } HEVMCalls::ExpectEmit2(inner) => { - state.expected_emits.push(ExpectedEmit { - depth: data.journaled_state.depth() - 1, + state.expected_emits.0.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.0.push_back(ExpectedEmit { + depth: data.journaled_state.depth(), checks: [inner.0, inner.1, inner.2, inner.3], address: Some(inner.4), ..Default::default() From ab2a6bf7de55f2535a1495e4807909ade42bfebc Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Wed, 10 May 2023 18:42:46 -0400 Subject: [PATCH 02/15] feat: handle expect emits on the next immediate call and error appropiately --- evm/src/executor/inspector/cheatcodes/mod.rs | 63 ++++++++++++++------ 1 file changed, 46 insertions(+), 17 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index ce824d935a09..0dc05cebdd61 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -29,7 +29,8 @@ use revm::{ }; use serde_json::Value; use std::{ - collections::{BTreeMap, HashMap}, + collections::{BTreeMap, HashMap, VecDeque}, + f32::consts::E, fs::File, io::BufReader, ops::Range, @@ -129,7 +130,7 @@ pub struct Cheatcodes { pub expected_calls: BTreeMap>, /// Expected emits - pub expected_emits: Vec, + pub expected_emits: (VecDeque, u64), /// Map of context depths to memory offset ranges that may be written to within the call depth. pub allowed_mem_writes: BTreeMap>>, @@ -528,7 +529,7 @@ where data: &bytes::Bytes, ) { // Match logs if `expectEmit` has been called - if !self.expected_emits.is_empty() { + if !self.expected_emits.0.is_empty() { handle_expect_emit( self, RawLog { @@ -537,6 +538,8 @@ where }, &b160_to_h160(*address), ); + // Increment the number of inspected emits. + self.expected_emits.1 += 1; } // Stores this log if `recordLogs` has been called @@ -753,21 +756,47 @@ 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 + .0 .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 found. + if self.expected_emits.0.iter().any(|expected| !expected.found) { + // Not enough emits were found + if self.expected_emits.0.len() as u64 > self.expected_emits.1 { + return ( + InstructionResult::Revert, + remaining_gas, + "Not enough events were emitted".to_string().encode().into(), + ) + } else { + // The wrong emits were 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.0.clear(); + } } // If the depth is 0, then this is the root call terminating @@ -814,7 +843,7 @@ where } // Check if we have any leftover expected emits - if !self.expected_emits.is_empty() { + if !self.expected_emits.0.is_empty() { return ( InstructionResult::Revert, remaining_gas, From 63d1006233f8ec3369a298e62f4c97e1bef73fe9 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Wed, 10 May 2023 18:42:59 -0400 Subject: [PATCH 03/15] chore: tests --- testdata/cheats/ExpectEmit.t.sol | 57 ++++++++++++++++++++++----- testdata/cheats/GetDeployedCode.t.sol | 2 +- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index a5e22576a914..a34152ff23d6 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -4,6 +4,14 @@ pragma solidity >=0.8.18; import "ds-test/test.sol"; import "./Cheats.sol"; +contract NestedEmitter { + event SomethingDifferent(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); + + function emitEvent(uint256 topic1, uint256 topic2, uint256 topic3, uint256 data) public { + emit SomethingDifferent(topic1, topic2, topic3, data); + } +} + contract Emitter { event Something(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); @@ -29,6 +37,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); } @@ -59,6 +72,8 @@ contract ExpectEmitTest is DSTest { event SomethingElse(uint256 indexed topic1); + event SomethingDifferent(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); + function setUp() public { emitter = new Emitter(); } @@ -182,6 +197,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 +256,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 +286,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); + // } } diff --git a/testdata/cheats/GetDeployedCode.t.sol b/testdata/cheats/GetDeployedCode.t.sol index 4e9753cd992d..9021e4253e5d 100644 --- a/testdata/cheats/GetDeployedCode.t.sol +++ b/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"); } } From 6ede49f3e46d3c7d539f85ce8f04d64e2208ab45 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Wed, 10 May 2023 20:12:23 -0400 Subject: [PATCH 04/15] chore: simplify errors --- evm/src/executor/inspector/cheatcodes/mod.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index 0dc05cebdd61..ce581c18b7d1 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -774,28 +774,18 @@ where .any(|expected| expected.depth == data.journaled_state.depth()); // If so, check the emits if should_check_emits { - // Not all emits were found. + // Not all emits were matched. if self.expected_emits.0.iter().any(|expected| !expected.found) { - // Not enough emits were found - if self.expected_emits.0.len() as u64 > self.expected_emits.1 { - return ( - InstructionResult::Revert, - remaining_gas, - "Not enough events were emitted".to_string().encode().into(), - ) - } else { - // The wrong emits were 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.0.clear(); + self.expected_emits.0.clear() } } @@ -847,7 +837,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(), From 81655086facad4b333764ab96f92ddfb2b68c399 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Wed, 10 May 2023 20:25:25 -0400 Subject: [PATCH 05/15] chore: remove unused actual count --- evm/src/executor/inspector/cheatcodes/expect.rs | 16 ++++++++-------- evm/src/executor/inspector/cheatcodes/mod.rs | 13 +++++-------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index 2e802ae6365a..9405b5157378 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -131,11 +131,11 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address // if there's anything to fill, we need to pop back. let event_to_fill_or_check = - if state.expected_emits.0.iter().any(|expected| expected.log.is_none()) { - state.expected_emits.0.pop_back() + 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.0.pop_front() + state.expected_emits.pop_front() }; let mut event_to_fill_or_check = @@ -178,7 +178,7 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address } } - state.expected_emits.0.push_back(event_to_fill_or_check); + state.expected_emits.push_back(event_to_fill_or_check); } #[derive(Clone, Debug, Default)] @@ -249,7 +249,7 @@ pub fn apply( expect_revert(state, Some(inner.0.into()), data.journaled_state.depth()) } HEVMCalls::ExpectEmit0(_) => { - state.expected_emits.0.push_back(ExpectedEmit { + state.expected_emits.push_back(ExpectedEmit { depth: data.journaled_state.depth(), checks: [true, true, true, true], ..Default::default() @@ -257,7 +257,7 @@ pub fn apply( Ok(Bytes::new()) } HEVMCalls::ExpectEmit1(inner) => { - state.expected_emits.0.push_back(ExpectedEmit { + state.expected_emits.push_back(ExpectedEmit { depth: data.journaled_state.depth(), checks: [true, true, true, true], address: Some(inner.0), @@ -266,7 +266,7 @@ pub fn apply( Ok(Bytes::new()) } HEVMCalls::ExpectEmit2(inner) => { - state.expected_emits.0.push_back(ExpectedEmit { + state.expected_emits.push_back(ExpectedEmit { depth: data.journaled_state.depth(), checks: [inner.0, inner.1, inner.2, inner.3], ..Default::default() @@ -274,7 +274,7 @@ pub fn apply( Ok(Bytes::new()) } HEVMCalls::ExpectEmit3(inner) => { - state.expected_emits.0.push_back(ExpectedEmit { + state.expected_emits.push_back(ExpectedEmit { depth: data.journaled_state.depth(), checks: [inner.0, inner.1, inner.2, inner.3], address: Some(inner.4), diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index ce581c18b7d1..100e00f94e0b 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -130,7 +130,7 @@ pub struct Cheatcodes { pub expected_calls: BTreeMap>, /// Expected emits - pub expected_emits: (VecDeque, u64), + pub expected_emits: VecDeque, /// Map of context depths to memory offset ranges that may be written to within the call depth. pub allowed_mem_writes: BTreeMap>>, @@ -529,7 +529,7 @@ where data: &bytes::Bytes, ) { // Match logs if `expectEmit` has been called - if !self.expected_emits.0.is_empty() { + if !self.expected_emits.is_empty() { handle_expect_emit( self, RawLog { @@ -538,8 +538,6 @@ where }, &b160_to_h160(*address), ); - // Increment the number of inspected emits. - self.expected_emits.1 += 1; } // Stores this log if `recordLogs` has been called @@ -769,13 +767,12 @@ where // First, check that we're at the call depth where the emits were declared from. let should_check_emits = self .expected_emits - .0 .iter() .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.0.iter().any(|expected| !expected.found) { + if self.expected_emits.iter().any(|expected| !expected.found) { return ( InstructionResult::Revert, remaining_gas, @@ -785,7 +782,7 @@ where // 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.0.clear() + self.expected_emits.clear() } } @@ -833,7 +830,7 @@ where } // Check if we have any leftover expected emits - if !self.expected_emits.0.is_empty() { + if !self.expected_emits.is_empty() { return ( InstructionResult::Revert, remaining_gas, From 0133beb1bfa4d0330ccefcad032d17e72b06be81 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Wed, 10 May 2023 20:27:17 -0400 Subject: [PATCH 06/15] chore: clippy --- evm/src/executor/inspector/cheatcodes/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index 100e00f94e0b..d95c5c6b605f 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -30,7 +30,6 @@ use revm::{ use serde_json::Value; use std::{ collections::{BTreeMap, HashMap, VecDeque}, - f32::consts::E, fs::File, io::BufReader, ops::Range, @@ -773,11 +772,11 @@ where 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(), - ) + 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 From 41cd379c8f38ee2f3fb21acef90df86447bbaeec Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Wed, 10 May 2023 20:30:28 -0400 Subject: [PATCH 07/15] chore: remove unneeded test artifacts --- testdata/cheats/ExpectEmit.t.sol | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index a34152ff23d6..5686f1cc9c7c 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -4,14 +4,6 @@ pragma solidity >=0.8.18; import "ds-test/test.sol"; import "./Cheats.sol"; -contract NestedEmitter { - event SomethingDifferent(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); - - function emitEvent(uint256 topic1, uint256 topic2, uint256 topic3, uint256 data) public { - emit SomethingDifferent(topic1, topic2, topic3, data); - } -} - contract Emitter { event Something(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); @@ -72,8 +64,6 @@ contract ExpectEmitTest is DSTest { event SomethingElse(uint256 indexed topic1); - event SomethingDifferent(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); - function setUp() public { emitter = new Emitter(); } From f99720d753d89e25040a7f3171557e9db61ac30d Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 11 May 2023 10:33:45 -0400 Subject: [PATCH 08/15] chore: ignore STATICCALLs --- evm/src/executor/inspector/cheatcodes/mod.rs | 4 +++- testdata/cheats/ExpectEmit.t.sol | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index d95c5c6b605f..fbacc2909ae3 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -767,7 +767,9 @@ where let should_check_emits = self .expected_emits .iter() - .any(|expected| expected.depth == data.journaled_state.depth()); + .any(|expected| expected.depth == data.journaled_state.depth()) && + // Ignore staticcalls + !call.is_static; // If so, check the emits if should_check_emits { // Not all emits were matched. diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index 5686f1cc9c7c..0338a3100a48 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -38,6 +38,10 @@ contract Emitter { inner.emitEvent(topic1, topic2, topic3, data); } + function getVar() public view returns (uint256) { + return 1; + } + /// Ref: issue #1214 function doesNothing() public pure {} @@ -269,6 +273,13 @@ contract ExpectEmitTest is DSTest { emitter.emitSomethingElse(1); } + function testCanDoStaticCall() public { + cheats.expectEmit(true, true, true, true); + emit Something(emitter.getVar(), 2, 3, 4); + + emitter.emitEvent(1, 2, 3, 4); + } + /// This test will fail if we check that all expected logs were emitted /// after every call from the same depth as the call that invoked the cheatcode. /// From fe2afecf4b4aa502f95de9cfbdab6b7bae166efd Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 11 May 2023 12:57:37 -0400 Subject: [PATCH 09/15] chore: fix additive behavior --- evm/src/executor/inspector/cheatcodes/expect.rs | 14 +++++++++++--- evm/src/executor/inspector/cheatcodes/mod.rs | 4 +++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index 9405b5157378..e8cf12c46cd7 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -127,14 +127,23 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address // 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 not, the queue will not be in the order the events will be intended to be filled, + // and we'll be able to later detect this and bail. + + // First, we can return early if all events have been matched. + // This allows a contract to arbitrarily emit more events than expected (additive behavior), + // as long as all the previous events were matched in the order they were expected to be. + if !state.expected_emits.iter().any(|expected| !expected.found) { + return + } // 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, if there are any events that are unmatched, we try to match to match them + // in the order declared, so we start popping from the front (like a queue). } else { - // Else, we need to pop from the front in the order the events were added to the queue. state.expected_emits.pop_front() }; @@ -177,7 +186,6 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address event_to_fill_or_check.log = Some(log); } } - state.expected_emits.push_back(event_to_fill_or_check); } diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index fbacc2909ae3..b929c5f17b20 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -527,7 +527,6 @@ where topics: &[B256], data: &bytes::Bytes, ) { - // Match logs if `expectEmit` has been called if !self.expected_emits.is_empty() { handle_expect_emit( self, @@ -831,6 +830,9 @@ where } // 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. + self.expected_emits.retain(|expected| !expected.found); + // If not empty, we got mismatched emits if !self.expected_emits.is_empty() { return ( InstructionResult::Revert, From a41f1123b974169e7f92e1bf4624ad1ff31b0767 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 11 May 2023 12:57:47 -0400 Subject: [PATCH 10/15] chore: add more tests --- testdata/cheats/ExpectEmit.t.sol | 32 +++++++++++++++++++++++++++++++- testdata/fork/Transact.t.sol | 1 + 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index 0338a3100a48..b801c6fa2dfa 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -5,6 +5,8 @@ import "ds-test/test.sol"; import "./Cheats.sol"; contract Emitter { + uint256 public thing; + event Something(uint256 indexed topic1, uint256 indexed topic2, uint256 indexed topic3, uint256 data); /// This event has 0 indexed topics, but the one in our tests @@ -45,6 +47,10 @@ contract Emitter { /// Ref: issue #1214 function doesNothing() public pure {} + function changeThing(uint256 num) public { + thing = num; + } + /// Ref: issue #760 function emitSomethingElse(uint256 data) public { emit SomethingElse(data); @@ -280,6 +286,30 @@ contract ExpectEmitTest is DSTest { emitter.emitEvent(1, 2, 3, 4); } + /// Tests for additive behavior. + // As long as we match the event we want in order, it doesn't matter which events are emitted afterwards. + function testAdditiveBehavior() public { + cheats.expectEmit(true, true, true, true, address(emitter)); + emit Something(1, 2, 3, 4); + + emitter.emitMultiple( + [uint256(1), uint256(5)], [uint256(2), uint256(6)], [uint256(3), uint256(7)], [uint256(4), uint256(8)] + ); + } + + /// This test should fail, as the call to `changeThing` is not a static call. + /// While we can ignore static calls, we cannot ignore normal calls. + function testFailEmitOnlyAppliesToNextCall() public { + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + // This works because it's a staticcall. + emitter.doesNothing(); + // This should make the test fail as it's a normal call. + emitter.changeThing(block.timestamp); + + emitter.emitEvent(1, 2, 3, 4); + } + /// This test will fail if we check that all expected logs were emitted /// after every call from the same depth as the call that invoked the cheatcode. /// @@ -287,7 +317,7 @@ contract ExpectEmitTest is DSTest { /// was invoked ends. /// /// Ref: issue #1214 - /// Note: this is now illegal behaviour. + /// NOTE: This is now invalid behavior. // function testExpectEmitIsCheckedWhenCurrentCallTerminates() public { // cheats.expectEmit(true, true, true, true); // emitter.doesNothing(); diff --git a/testdata/fork/Transact.t.sol b/testdata/fork/Transact.t.sol index 58d040e8881f..af84c38f3e91 100644 --- a/testdata/fork/Transact.t.sol +++ b/testdata/fork/Transact.t.sol @@ -71,6 +71,7 @@ contract TransactOnForkTest is DSTest { // expect a call to USDT's transfer vm.expectCall(address(USDT), abi.encodeWithSelector(IERC20.transfer.selector, recipient, transferAmount)); + // expect a Transfer event to be emitted vm.expectEmit(true, true, false, true, address(USDT)); emit Transfer(address(sender), address(recipient), transferAmount); From 28af854537b83b42c2ea083a3d204dcb177e0b20 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 11 May 2023 13:03:09 -0400 Subject: [PATCH 11/15] chore: lint --- testdata/fork/Transact.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/testdata/fork/Transact.t.sol b/testdata/fork/Transact.t.sol index af84c38f3e91..58d040e8881f 100644 --- a/testdata/fork/Transact.t.sol +++ b/testdata/fork/Transact.t.sol @@ -71,7 +71,6 @@ contract TransactOnForkTest is DSTest { // expect a call to USDT's transfer vm.expectCall(address(USDT), abi.encodeWithSelector(IERC20.transfer.selector, recipient, transferAmount)); - // expect a Transfer event to be emitted vm.expectEmit(true, true, false, true, address(USDT)); emit Transfer(address(sender), address(recipient), transferAmount); From 420d8146b69189450ae60196817bc9dded9dba92 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 11 May 2023 18:20:11 -0400 Subject: [PATCH 12/15] chore: be able to match in between events rather than strictly full-sequences --- .../executor/inspector/cheatcodes/expect.rs | 12 +++++- testdata/cheats/ExpectEmit.t.sol | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index e8cf12c46cd7..d97d44c1ee65 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -180,13 +180,23 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address event_to_fill_or_check.found &= expected.data == log.data; } } + + // If we found the event, we can push it to the back of the queue + // and begin expecting the next event. + if event_to_fill_or_check.found { + state.expected_emits.push_back(event_to_fill_or_check); + } else { + // We did not match this event, so we need to keep waiting for the right one to appear. + state.expected_emits.push_front(event_to_fill_or_check); + } + } // Fill the event. None => { event_to_fill_or_check.log = Some(log); + state.expected_emits.push_back(event_to_fill_or_check); } } - state.expected_emits.push_back(event_to_fill_or_check); } #[derive(Clone, Debug, Default)] diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index b801c6fa2dfa..be3c58254b21 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -17,6 +17,8 @@ contract Emitter { /// Ref: issue #760 event SomethingElse(uint256 data); + event SomethingNonIndexed(uint256 data); + function emitEvent(uint256 topic1, uint256 topic2, uint256 topic3, uint256 data) public { emit Something(topic1, topic2, topic3, data); } @@ -36,6 +38,13 @@ contract Emitter { emitNested(Emitter(address(this)), 1, 2, 3, 4); } + function emitOutOfExactOrder() public { + emit SomethingNonIndexed(1); + emit Something(1, 2, 3, 4); + emit Something(1, 2, 3, 4); + emit Something(1, 2, 3, 4); + } + function emitNested(Emitter inner, uint256 topic1, uint256 topic2, uint256 topic3, uint256 data) public { inner.emitEvent(topic1, topic2, topic3, data); } @@ -74,6 +83,8 @@ contract ExpectEmitTest is DSTest { event SomethingElse(uint256 indexed topic1); + event SomethingNonIndexed(uint256 data); + function setUp() public { emitter = new Emitter(); } @@ -217,6 +228,37 @@ contract ExpectEmitTest is DSTest { ); } + function testExpectEmitCanMatchWithoutExactOrder() public { + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + + emitter.emitOutOfExactOrder(); + } + + function testFailExpectEmitCanMatchWithoutExactOrder() public { + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + // This should fail, as this event is never emitted + // in between the other two Something events. + cheats.expectEmit(true, true, true, true); + emit SomethingElse(1); + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + + emitter.emitOutOfExactOrder(); + } + + function testExpectEmitCanMatchWithoutExactOrder2() public { + cheats.expectEmit(true, true, true, true); + emit SomethingNonIndexed(1); + cheats.expectEmit(true, true, true, true); + emit Something(1, 2, 3, 4); + + emitter.emitOutOfExactOrder(); + } + function testExpectEmitAddress() public { cheats.expectEmit(address(emitter)); emit Something(1, 2, 3, 4); From a1d81361b46f0f7c1cf1d1fad8eadefde78bc933 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 11 May 2023 18:25:08 -0400 Subject: [PATCH 13/15] chore: clippy --- evm/src/executor/inspector/cheatcodes/expect.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index d97d44c1ee65..ffb723272ad6 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -186,10 +186,10 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address if event_to_fill_or_check.found { state.expected_emits.push_back(event_to_fill_or_check); } else { - // We did not match this event, so we need to keep waiting for the right one to appear. + // We did not match this event, so we need to keep waiting for the right one to + // appear. state.expected_emits.push_front(event_to_fill_or_check); } - } // Fill the event. None => { From 2d0ed7c05cff91607964b3512059648191391644 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Thu, 11 May 2023 18:30:40 -0400 Subject: [PATCH 14/15] chore: lint expect emit --- testdata/cheats/ExpectEmit.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index be3c58254b21..717f11db07bb 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -240,7 +240,7 @@ contract ExpectEmitTest is DSTest { function testFailExpectEmitCanMatchWithoutExactOrder() public { cheats.expectEmit(true, true, true, true); emit Something(1, 2, 3, 4); - // This should fail, as this event is never emitted + // This should fail, as this event is never emitted // in between the other two Something events. cheats.expectEmit(true, true, true, true); emit SomethingElse(1); From d72a935f944a7a823e9c448dbdf39c6cbeb9151f Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Fri, 12 May 2023 09:56:42 -0400 Subject: [PATCH 15/15] chore: simplify if --- evm/src/executor/inspector/cheatcodes/expect.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index ffb723272ad6..325b1b553d8a 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -133,7 +133,7 @@ pub fn handle_expect_emit(state: &mut Cheatcodes, log: RawLog, address: &Address // First, we can return early if all events have been matched. // This allows a contract to arbitrarily emit more events than expected (additive behavior), // as long as all the previous events were matched in the order they were expected to be. - if !state.expected_emits.iter().any(|expected| !expected.found) { + if state.expected_emits.iter().all(|expected| expected.found) { return }