From 1f99be31a2cf8ec6ebc2820420c8fddfad41d3c3 Mon Sep 17 00:00:00 2001 From: Phillip LeBlanc Date: Sat, 4 Nov 2023 18:36:02 +0900 Subject: [PATCH 1/3] Fix bug in matching logic for `trace_filter` --- crates/rpc/rpc-types/src/eth/trace/filter.rs | 135 +++++++++++++++---- 1 file changed, 106 insertions(+), 29 deletions(-) diff --git a/crates/rpc/rpc-types/src/eth/trace/filter.rs b/crates/rpc/rpc-types/src/eth/trace/filter.rs index 0a18d954b522..336f2d79e93b 100644 --- a/crates/rpc/rpc-types/src/eth/trace/filter.rs +++ b/crates/rpc/rpc-types/src/eth/trace/filter.rs @@ -52,7 +52,7 @@ pub enum TraceFilterMode { Intersection, } -/// Helper type for matching `from` and `to` addresses. +/// Helper type for matching `from` and `to` addresses. Empty sets match all addresses. #[derive(Debug, Clone, PartialEq, Eq)] pub struct TraceFilterMatcher { mode: TraceFilterMode, @@ -63,27 +63,40 @@ pub struct TraceFilterMatcher { impl TraceFilterMatcher { /// Returns `true` if the given `from` and `to` addresses match this filter. pub fn matches(&self, from: Address, to: Option
) -> bool { - // If `from_addresses` and `to_addresses` are empty, then match all transactions. + // 1. If both `from_addresses` and `to_addresses` are empty, match everything. if self.from_addresses.is_empty() && self.to_addresses.is_empty() { return true; } - match self.mode { - TraceFilterMode::Union => { - self.from_addresses.contains(&from) || - to.map_or(false, |to| self.to_addresses.contains(&to)) - } - TraceFilterMode::Intersection => { - self.from_addresses.contains(&from) && - to.map_or(false, |to| self.to_addresses.contains(&to)) - } + let from_matches = self.from_addresses.contains(&from); + let to_matches = to.map_or(false, |to_addr| self.to_addresses.contains(&to_addr)); + + // 2. Use the filter mode to match if both `from_addresses` and `to_addresses` are defined. + if !self.from_addresses.is_empty() && !self.to_addresses.is_empty() { + return match self.mode { + TraceFilterMode::Union => from_matches || to_matches, + TraceFilterMode::Intersection => from_matches && to_matches, + }; + } + + // 3. Match only against `to_addresses` if `from_addresses` is empty. + if self.from_addresses.is_empty() { + return to_matches; } + + // 4. Match only against `from_addresses` if `to_addresses` is empty. + if self.to_addresses.is_empty() { + return from_matches; + } + + unreachable!("Should not happen, all cases are covered above") } } #[cfg(test)] mod tests { use super::*; + use serde_json::json; #[test] fn test_parse_filter() { @@ -94,25 +107,89 @@ mod tests { } #[test] - fn test_filter_matcher() { - let s = r#"{"fromBlock": "0x3","toBlock": "0x5"}"#; - let filter: TraceFilter = serde_json::from_str(s).unwrap(); + fn test_filter_matcher_addresses_unspecified() { + let test_addr_d8 = "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".parse().unwrap(); + let test_addr_16 = "0x160f5f00288e9e1cc8655b327e081566e580a71d".parse().unwrap(); + let filter_json = json!({ + "fromBlock": "0x3", + "toBlock": "0x5", + }); + let filter: TraceFilter = + serde_json::from_value(filter_json).expect("Failed to parse filter"); let matcher = filter.matcher(); - assert!( - matcher.matches("0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".parse().unwrap(), None) - ); - assert!( - matcher.matches("0x160f5f00288e9e1cc8655b327e081566e580a71d".parse().unwrap(), None) - ); - - let s = r#"{"fromBlock": "0x3","toBlock": "0x5", "fromAddress": ["0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045"]}"#; - let filter: TraceFilter = serde_json::from_str(s).unwrap(); + assert!(matcher.matches(test_addr_d8, None)); + assert!(matcher.matches(test_addr_16, None)); + assert!(matcher.matches(test_addr_d8, Some(test_addr_16))); + assert!(matcher.matches(test_addr_16, Some(test_addr_d8))); + } + + #[test] + fn test_filter_matcher_from_address() { + let test_addr_d8 = "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".parse().unwrap(); + let test_addr_16 = "0x160f5f00288e9e1cc8655b327e081566e580a71d".parse().unwrap(); + let filter_json = json!({ + "fromBlock": "0x3", + "toBlock": "0x5", + "fromAddress": [test_addr_d8] + }); + let filter: TraceFilter = serde_json::from_value(filter_json).unwrap(); + let matcher = filter.matcher(); + assert!(matcher.matches(test_addr_d8, None)); + assert!(!matcher.matches(test_addr_16, None)); + assert!(matcher.matches(test_addr_d8, Some(test_addr_16))); + assert!(!matcher.matches(test_addr_16, Some(test_addr_d8))); + } + + #[test] + fn test_filter_matcher_to_address() { + let test_addr_d8 = "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".parse().unwrap(); + let test_addr_16 = "0x160f5f00288e9e1cc8655b327e081566e580a71d".parse().unwrap(); + let filter_json = json!({ + "fromBlock": "0x3", + "toBlock": "0x5", + "toAddress": [test_addr_d8], + }); + let filter: TraceFilter = serde_json::from_value(filter_json).unwrap(); + let matcher = filter.matcher(); + assert!(matcher.matches(test_addr_16, Some(test_addr_d8))); + assert!(!matcher.matches(test_addr_16, None)); + assert!(!matcher.matches(test_addr_d8, Some(test_addr_16))); + } + + #[test] + fn test_filter_matcher_both_addresses_union() { + let test_addr_d8 = "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".parse().unwrap(); + let test_addr_16 = "0x160f5f00288e9e1cc8655b327e081566e580a71d".parse().unwrap(); + let filter_json = json!({ + "fromBlock": "0x3", + "toBlock": "0x5", + "fromAddress": [test_addr_16], + "toAddress": [test_addr_d8], + }); + let filter: TraceFilter = serde_json::from_value(filter_json).unwrap(); + let matcher = filter.matcher(); + assert!(matcher.matches(test_addr_16, Some(test_addr_d8))); + assert!(matcher.matches(test_addr_16, None)); + assert!(matcher.matches(test_addr_d8, Some(test_addr_d8))); + assert!(!matcher.matches(test_addr_d8, Some(test_addr_16))); + } + + #[test] + fn test_filter_matcher_both_addresses_intersection() { + let test_addr_d8 = "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".parse().unwrap(); + let test_addr_16 = "0x160f5f00288e9e1cc8655b327e081566e580a71d".parse().unwrap(); + let filter_json = json!({ + "fromBlock": "0x3", + "toBlock": "0x5", + "fromAddress": [test_addr_16], + "toAddress": [test_addr_d8], + "mode": "intersection", + }); + let filter: TraceFilter = serde_json::from_value(filter_json).unwrap(); let matcher = filter.matcher(); - assert!( - matcher.matches("0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045".parse().unwrap(), None) - ); - assert!( - !matcher.matches("0x160f5f00288e9e1cc8655b327e081566e580a71d".parse().unwrap(), None) - ); + assert!(matcher.matches(test_addr_16, Some(test_addr_d8))); + assert!(!matcher.matches(test_addr_16, None)); + assert!(!matcher.matches(test_addr_d8, Some(test_addr_d8))); + assert!(!matcher.matches(test_addr_d8, Some(test_addr_16))); } } From 18ab2a9883c96f65462ae2755f5bc49303c8ba4b Mon Sep 17 00:00:00 2001 From: Phillip LeBlanc Date: Sat, 4 Nov 2023 18:45:52 +0900 Subject: [PATCH 2/3] Simplify logic using a local enum --- crates/rpc/rpc-types/src/eth/trace/filter.rs | 41 ++++++++++---------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/crates/rpc/rpc-types/src/eth/trace/filter.rs b/crates/rpc/rpc-types/src/eth/trace/filter.rs index 336f2d79e93b..eb0305386140 100644 --- a/crates/rpc/rpc-types/src/eth/trace/filter.rs +++ b/crates/rpc/rpc-types/src/eth/trace/filter.rs @@ -63,33 +63,32 @@ pub struct TraceFilterMatcher { impl TraceFilterMatcher { /// Returns `true` if the given `from` and `to` addresses match this filter. pub fn matches(&self, from: Address, to: Option
) -> bool { - // 1. If both `from_addresses` and `to_addresses` are empty, match everything. - if self.from_addresses.is_empty() && self.to_addresses.is_empty() { - return true; - } - let from_matches = self.from_addresses.contains(&from); let to_matches = to.map_or(false, |to_addr| self.to_addresses.contains(&to_addr)); - // 2. Use the filter mode to match if both `from_addresses` and `to_addresses` are defined. - if !self.from_addresses.is_empty() && !self.to_addresses.is_empty() { - return match self.mode { - TraceFilterMode::Union => from_matches || to_matches, - TraceFilterMode::Intersection => from_matches && to_matches, - }; + enum Match { + All, + From, + To, + Both(TraceFilterMode), } - // 3. Match only against `to_addresses` if `from_addresses` is empty. - if self.from_addresses.is_empty() { - return to_matches; - } - - // 4. Match only against `from_addresses` if `to_addresses` is empty. - if self.to_addresses.is_empty() { - return from_matches; + let match_type = match (self.from_addresses.is_empty(), self.to_addresses.is_empty()) { + (true, true) => Match::All, + (false, true) => Match::From, + (true, false) => Match::To, + (false, false) => Match::Both(self.mode), + }; + + match match_type { + Match::All => true, + Match::From => from_matches, + Match::To => to_matches, + Match::Both(mode) => match mode { + TraceFilterMode::Union => from_matches || to_matches, + TraceFilterMode::Intersection => from_matches && to_matches, + }, } - - unreachable!("Should not happen, all cases are covered above") } } From 14f00c380d0b1379d6fba1d58b21af7134ecaf8a Mon Sep 17 00:00:00 2001 From: Phillip LeBlanc Date: Sat, 4 Nov 2023 19:38:57 +0900 Subject: [PATCH 3/3] Just one match --- crates/rpc/rpc-types/src/eth/trace/filter.rs | 37 +++++++------------- 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/crates/rpc/rpc-types/src/eth/trace/filter.rs b/crates/rpc/rpc-types/src/eth/trace/filter.rs index eb0305386140..0c27d4bf09a1 100644 --- a/crates/rpc/rpc-types/src/eth/trace/filter.rs +++ b/crates/rpc/rpc-types/src/eth/trace/filter.rs @@ -63,30 +63,19 @@ pub struct TraceFilterMatcher { impl TraceFilterMatcher { /// Returns `true` if the given `from` and `to` addresses match this filter. pub fn matches(&self, from: Address, to: Option
) -> bool { - let from_matches = self.from_addresses.contains(&from); - let to_matches = to.map_or(false, |to_addr| self.to_addresses.contains(&to_addr)); - - enum Match { - All, - From, - To, - Both(TraceFilterMode), - } - - let match_type = match (self.from_addresses.is_empty(), self.to_addresses.is_empty()) { - (true, true) => Match::All, - (false, true) => Match::From, - (true, false) => Match::To, - (false, false) => Match::Both(self.mode), - }; - - match match_type { - Match::All => true, - Match::From => from_matches, - Match::To => to_matches, - Match::Both(mode) => match mode { - TraceFilterMode::Union => from_matches || to_matches, - TraceFilterMode::Intersection => from_matches && to_matches, + match (self.from_addresses.is_empty(), self.to_addresses.is_empty()) { + (true, true) => true, + (false, true) => self.from_addresses.contains(&from), + (true, false) => to.map_or(false, |to_addr| self.to_addresses.contains(&to_addr)), + (false, false) => match self.mode { + TraceFilterMode::Union => { + self.from_addresses.contains(&from) || + to.map_or(false, |to_addr| self.to_addresses.contains(&to_addr)) + } + TraceFilterMode::Intersection => { + self.from_addresses.contains(&from) && + to.map_or(false, |to_addr| self.to_addresses.contains(&to_addr)) + } }, } }