From fca9a92e08e1c18ca885a12cb9f999a2d399cd9a Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Fri, 3 Jan 2025 11:23:52 +0100 Subject: [PATCH 01/23] Add log-only mode for metrics based bad token detection (#3203) # Description The metrics based bad token detection has not been tested with real data yet so actively filtering out tokens based on it is pretty risky. If the logic is too naive or simply has a bug we might filter out a bunch of tokens which are actually good which could reduce the throughput of the system drastically. # Changes To increase the confidence in the system this PR introduces a log only mode for the metrics based bad token detection. In that mode no tokens will be filtered out based on the metrics but we already get logs indicating this tokens would get filtered out by it. When we start running this mode in prod we should hopefully see 2 things: 1 good tokens don't get filtered out 2 tokens which currently cause alerts like `Driver Run Error Rate Too High` should be flagged by the logic `2` would then indicate that the new logic would reduce the unactionable alerts if the feature gets enabled fully. I only added logs when we update the metrics and a token "turns bad". This should keep the noise in the logs low while still providing all the necessary information. ## How to test Deploying this to prod aids in testing the overall feature --- .../domain/competition/bad_tokens/metrics.rs | 28 +++++++++++++++---- crates/driver/src/infra/api/mod.rs | 20 ++++++------- crates/driver/src/infra/config/file/load.rs | 1 + crates/driver/src/infra/config/file/mod.rs | 12 ++++++++ crates/driver/src/infra/solver/mod.rs | 1 + 5 files changed, 46 insertions(+), 16 deletions(-) diff --git a/crates/driver/src/domain/competition/bad_tokens/metrics.rs b/crates/driver/src/domain/competition/bad_tokens/metrics.rs index 570b32fee7..50917c6d53 100644 --- a/crates/driver/src/domain/competition/bad_tokens/metrics.rs +++ b/crates/driver/src/domain/competition/bad_tokens/metrics.rs @@ -10,11 +10,12 @@ struct TokenStatistics { pub struct DetectorBuilder(Arc>); impl DetectorBuilder { - pub fn build(self, failure_ratio: f64, required_measurements: u32) -> Detector { + pub fn build(self, failure_ratio: f64, required_measurements: u32, log_only: bool) -> Detector { Detector { failure_ratio, required_measurements, counter: self.0, + log_only, } } } @@ -29,15 +30,20 @@ pub struct Detector { failure_ratio: f64, required_measurements: u32, counter: Arc>, + log_only: bool, } impl Detector { pub fn get_quality(&self, token: ð::TokenAddress) -> Option { let measurements = self.counter.get(token)?; - let is_unsupported = measurements.attempts >= self.required_measurements - && (measurements.fails as f64 / measurements.attempts as f64) >= self.failure_ratio; + let is_unsupported = self.is_unsupported(&measurements); + (!self.log_only && is_unsupported).then_some(Quality::Unsupported) + } - is_unsupported.then_some(Quality::Unsupported) + fn is_unsupported(&self, measurements: &TokenStatistics) -> bool { + let token_failure_ratio = measurements.fails as f64 / measurements.attempts as f64; + measurements.attempts >= self.required_measurements + && token_failure_ratio >= self.failure_ratio } /// Updates the tokens that participated in settlements by @@ -48,11 +54,13 @@ impl Detector { token_pairs: &[(eth::TokenAddress, eth::TokenAddress)], failure: bool, ) { + let mut unsupported_tokens = vec![]; token_pairs .iter() .flat_map(|(token_a, token_b)| [token_a, token_b]) .for_each(|token| { - self.counter + let measurement = self + .counter .entry(*token) .and_modify(|counter| { counter.attempts += 1; @@ -62,6 +70,16 @@ impl Detector { attempts: 1, fails: u32::from(failure), }); + if self.is_unsupported(&measurement) { + unsupported_tokens.push(token); + } }); + + if !unsupported_tokens.is_empty() { + tracing::debug!( + tokens = ?unsupported_tokens, + "mark tokens as unsupported" + ); + } } } diff --git a/crates/driver/src/infra/api/mod.rs b/crates/driver/src/infra/api/mod.rs index e4b760822c..3047c796a6 100644 --- a/crates/driver/src/infra/api/mod.rs +++ b/crates/driver/src/infra/api/mod.rs @@ -73,21 +73,19 @@ impl Api { let router = routes::reveal(router); let router = routes::settle(router); + let bad_token_config = solver.bad_token_detection(); let mut bad_tokens = - bad_tokens::Detector::new(solver.bad_token_detection().tokens_supported.clone()); - if solver.bad_token_detection().enable_simulation_strategy { + bad_tokens::Detector::new(bad_token_config.tokens_supported.clone()); + if bad_token_config.enable_simulation_strategy { bad_tokens.with_simulation_detector(self.bad_token_detector.clone()); } - if solver.bad_token_detection().enable_metrics_strategy { - bad_tokens.with_metrics_detector( - metrics_bad_token_detector_builder.clone().build( - solver.bad_token_detection().metrics_strategy_failure_ratio, - solver - .bad_token_detection() - .metrics_strategy_required_measurements, - ), - ); + if bad_token_config.enable_metrics_strategy { + bad_tokens.with_metrics_detector(metrics_bad_token_detector_builder.clone().build( + bad_token_config.metrics_strategy_failure_ratio, + bad_token_config.metrics_strategy_required_measurements, + bad_token_config.metrics_strategy_log_only, + )); } let router = router.with_state(State(Arc::new(Inner { diff --git a/crates/driver/src/infra/config/file/load.rs b/crates/driver/src/infra/config/file/load.rs index 1497b4bdc9..9c19aad70a 100644 --- a/crates/driver/src/infra/config/file/load.rs +++ b/crates/driver/src/infra/config/file/load.rs @@ -123,6 +123,7 @@ pub async fn load(chain: Chain, path: &Path) -> infra::Config { metrics_strategy_required_measurements: config .bad_token_detection .metrics_strategy_required_measurements, + metrics_strategy_log_only: config.bad_token_detection.metrics_strategy_log_only, }, settle_queue_size: config.settle_queue_size, } diff --git a/crates/driver/src/infra/config/file/mod.rs b/crates/driver/src/infra/config/file/mod.rs index 4f38b43e0f..5677e60be7 100644 --- a/crates/driver/src/infra/config/file/mod.rs +++ b/crates/driver/src/infra/config/file/mod.rs @@ -708,6 +708,14 @@ pub struct BadTokenDetectionConfig { rename = "metrics-bad-token-detection-required-measurements" )] pub metrics_strategy_required_measurements: u32, + + /// Controls whether the metrics based detection strategy should only log + /// unsupported tokens or actually filter them out. + #[serde( + default = "default_metrics_bad_token_detector_log_only", + rename = "metrics-bad-token-detection-log-only" + )] + pub metrics_strategy_log_only: bool, } impl Default for BadTokenDetectionConfig { @@ -730,3 +738,7 @@ fn default_metrics_bad_token_detector_required_measurements() -> u32 { fn default_settle_queue_size() -> usize { 2 } + +fn default_metrics_bad_token_detector_log_only() -> bool { + true +} diff --git a/crates/driver/src/infra/solver/mod.rs b/crates/driver/src/infra/solver/mod.rs index 37aa96ef26..cc0802a5a2 100644 --- a/crates/driver/src/infra/solver/mod.rs +++ b/crates/driver/src/infra/solver/mod.rs @@ -316,4 +316,5 @@ pub struct BadTokenDetection { pub enable_metrics_strategy: bool, pub metrics_strategy_failure_ratio: f64, pub metrics_strategy_required_measurements: u32, + pub metrics_strategy_log_only: bool, } From 3ca92b7b43bd98042e3dd5bea9dcb44920dede4f Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Fri, 3 Jan 2025 12:28:53 +0100 Subject: [PATCH 02/23] only mark tokens as unsupported based on metrics for a limited time (#3205) # Description Currently the bad token detection based on metrics will mark tokens as unsupported forever. This is problematic for tokens which only have issues temporarily. For example this can happen when the most important pool for a token gets into a weird state or when a token gets paused or a while. # Changes Adjusts the logic to freeze tokens for a configurable period of time. Once the freeze period is over we give the token another chance (even if the stats indicate that it's currently unsupported). To not run into issues when a token is always bad the logic was built such that 1 more `bad` measurement is enough to freeze the token again. That way we can safely configure a very high `min_measurements` without having periods where a token that was flagged as bad can issues again because we need to get a lot new measurements to mark it as unsupported again. Additionally the PR simplifies how the metrics based bad token detector gets instantiated and gives each solver their completely separate instance (how it was originally communicated because each solver may support different tokens). ## How to test added a unit test --- .../domain/competition/bad_tokens/metrics.rs | 130 ++++++++++++++---- .../src/domain/competition/bad_tokens/mod.rs | 2 +- crates/driver/src/infra/api/mod.rs | 5 +- crates/driver/src/infra/config/file/load.rs | 3 + crates/driver/src/infra/config/file/mod.rs | 13 ++ crates/driver/src/infra/solver/mod.rs | 3 +- 6 files changed, 121 insertions(+), 35 deletions(-) diff --git a/crates/driver/src/domain/competition/bad_tokens/metrics.rs b/crates/driver/src/domain/competition/bad_tokens/metrics.rs index 50917c6d53..43e444dca2 100644 --- a/crates/driver/src/domain/competition/bad_tokens/metrics.rs +++ b/crates/driver/src/domain/competition/bad_tokens/metrics.rs @@ -1,23 +1,18 @@ -use {super::Quality, crate::domain::eth, dashmap::DashMap, std::sync::Arc}; +use { + super::Quality, + crate::domain::eth, + dashmap::DashMap, + std::{ + sync::Arc, + time::{Duration, Instant}, + }, +}; -#[derive(Default)] +#[derive(Default, Debug)] struct TokenStatistics { attempts: u32, fails: u32, -} - -#[derive(Default, Clone)] -pub struct DetectorBuilder(Arc>); - -impl DetectorBuilder { - pub fn build(self, failure_ratio: f64, required_measurements: u32, log_only: bool) -> Detector { - Detector { - failure_ratio, - required_measurements, - counter: self.0, - log_only, - } - } + flagged_unsupported_at: Option, } /// Monitors tokens to determine whether they are considered "unsupported" based @@ -31,19 +26,46 @@ pub struct Detector { required_measurements: u32, counter: Arc>, log_only: bool, + token_freeze_time: Duration, } impl Detector { - pub fn get_quality(&self, token: ð::TokenAddress) -> Option { - let measurements = self.counter.get(token)?; - let is_unsupported = self.is_unsupported(&measurements); + pub fn new( + failure_ratio: f64, + required_measurements: u32, + log_only: bool, + token_freeze_time: Duration, + ) -> Self { + Self { + failure_ratio, + required_measurements, + counter: Default::default(), + log_only, + token_freeze_time, + } + } + + pub fn get_quality(&self, token: ð::TokenAddress, now: Instant) -> Option { + let stats = self.counter.get(token)?; + if stats + .flagged_unsupported_at + .is_some_and(|t| now.duration_since(t) > self.token_freeze_time) + { + // Sometimes tokens only cause issues temporarily. If the token's freeze + // period expired we give it another chance to see if it still behaves badly. + return None; + } + + let is_unsupported = self.stats_indicate_unsupported(&stats); (!self.log_only && is_unsupported).then_some(Quality::Unsupported) } - fn is_unsupported(&self, measurements: &TokenStatistics) -> bool { - let token_failure_ratio = measurements.fails as f64 / measurements.attempts as f64; - measurements.attempts >= self.required_measurements - && token_failure_ratio >= self.failure_ratio + fn stats_indicate_unsupported(&self, stats: &TokenStatistics) -> bool { + let token_failure_ratio = match stats.attempts { + 0 => return false, + attempts => f64::from(stats.fails) / f64::from(attempts), + }; + stats.attempts >= self.required_measurements && token_failure_ratio >= self.failure_ratio } /// Updates the tokens that participated in settlements by @@ -54,32 +76,80 @@ impl Detector { token_pairs: &[(eth::TokenAddress, eth::TokenAddress)], failure: bool, ) { - let mut unsupported_tokens = vec![]; + let now = Instant::now(); + let mut new_unsupported_tokens = vec![]; token_pairs .iter() .flat_map(|(token_a, token_b)| [token_a, token_b]) .for_each(|token| { - let measurement = self + let mut stats = self .counter .entry(*token) .and_modify(|counter| { counter.attempts += 1; - counter.fails += u32::from(failure) + counter.fails += u32::from(failure); }) .or_insert_with(|| TokenStatistics { attempts: 1, fails: u32::from(failure), + flagged_unsupported_at: None, }); - if self.is_unsupported(&measurement) { - unsupported_tokens.push(token); + + // token neeeds to be frozen as unsupported for a while + if self.stats_indicate_unsupported(&stats) + && stats + .flagged_unsupported_at + .is_none_or(|t| now.duration_since(t) > self.token_freeze_time) + { + new_unsupported_tokens.push(token); + stats.flagged_unsupported_at = Some(now); } }); - if !unsupported_tokens.is_empty() { + if !new_unsupported_tokens.is_empty() { tracing::debug!( - tokens = ?unsupported_tokens, + tokens = ?new_unsupported_tokens, "mark tokens as unsupported" ); } } } + +#[cfg(test)] +mod tests { + use {super::*, ethcontract::H160}; + + /// Tests that a token only gets marked temporarily as unsupported. + /// After the freeze period it will be allowed again. + #[tokio::test] + async fn unfreeze_bad_tokens() { + const FREEZE_DURATION: Duration = Duration::from_millis(50); + let detector = Detector::new(0.5, 2, false, FREEZE_DURATION); + + let token_a = eth::TokenAddress(eth::ContractAddress(H160([1; 20]))); + let token_b = eth::TokenAddress(eth::ContractAddress(H160([2; 20]))); + + // token is reported as supported while we don't have enough measurements + assert_eq!(detector.get_quality(&token_a, Instant::now()), None); + detector.update_tokens(&[(token_a, token_b)], true); + assert_eq!(detector.get_quality(&token_a, Instant::now()), None); + detector.update_tokens(&[(token_a, token_b)], true); + + // after we got enough measurements the token gets marked as bad + assert_eq!( + detector.get_quality(&token_a, Instant::now()), + Some(Quality::Unsupported) + ); + + // after the freeze period is over the token gets reported as good again + tokio::time::sleep(FREEZE_DURATION).await; + assert_eq!(detector.get_quality(&token_a, Instant::now()), None); + + // after an unfreeze another bad measurement is enough to freeze it again + detector.update_tokens(&[(token_a, token_b)], true); + assert_eq!( + detector.get_quality(&token_a, Instant::now()), + Some(Quality::Unsupported) + ); + } +} diff --git a/crates/driver/src/domain/competition/bad_tokens/mod.rs b/crates/driver/src/domain/competition/bad_tokens/mod.rs index ffa1578425..d724bea442 100644 --- a/crates/driver/src/domain/competition/bad_tokens/mod.rs +++ b/crates/driver/src/domain/competition/bad_tokens/mod.rs @@ -132,7 +132,7 @@ impl Detector { } if let Some(metrics) = &self.metrics { - return metrics.get_quality(&token); + return metrics.get_quality(&token, now); } None diff --git a/crates/driver/src/infra/api/mod.rs b/crates/driver/src/infra/api/mod.rs index 3047c796a6..de7897cfa8 100644 --- a/crates/driver/src/infra/api/mod.rs +++ b/crates/driver/src/infra/api/mod.rs @@ -58,8 +58,6 @@ impl Api { app = routes::metrics(app); app = routes::healthz(app); - let metrics_bad_token_detector_builder = bad_tokens::metrics::DetectorBuilder::default(); - // Multiplex each solver as part of the API. Multiple solvers are multiplexed // on the same driver so only one liquidity collector collects the liquidity // for all of them. This is important because liquidity collection is @@ -81,10 +79,11 @@ impl Api { } if bad_token_config.enable_metrics_strategy { - bad_tokens.with_metrics_detector(metrics_bad_token_detector_builder.clone().build( + bad_tokens.with_metrics_detector(bad_tokens::metrics::Detector::new( bad_token_config.metrics_strategy_failure_ratio, bad_token_config.metrics_strategy_required_measurements, bad_token_config.metrics_strategy_log_only, + bad_token_config.metrics_strategy_token_freeze_time, )); } diff --git a/crates/driver/src/infra/config/file/load.rs b/crates/driver/src/infra/config/file/load.rs index 9c19aad70a..0c3e2b61f7 100644 --- a/crates/driver/src/infra/config/file/load.rs +++ b/crates/driver/src/infra/config/file/load.rs @@ -124,6 +124,9 @@ pub async fn load(chain: Chain, path: &Path) -> infra::Config { .bad_token_detection .metrics_strategy_required_measurements, metrics_strategy_log_only: config.bad_token_detection.metrics_strategy_log_only, + metrics_strategy_token_freeze_time: config + .bad_token_detection + .metrics_strategy_token_freeze_time, }, settle_queue_size: config.settle_queue_size, } diff --git a/crates/driver/src/infra/config/file/mod.rs b/crates/driver/src/infra/config/file/mod.rs index 5677e60be7..39ed135509 100644 --- a/crates/driver/src/infra/config/file/mod.rs +++ b/crates/driver/src/infra/config/file/mod.rs @@ -716,6 +716,15 @@ pub struct BadTokenDetectionConfig { rename = "metrics-bad-token-detection-log-only" )] pub metrics_strategy_log_only: bool, + + /// How long the metrics based bad token detection should flag a token as + /// unsupported before it allows to solve for that token again. + #[serde( + default = "default_metrics_bad_token_detector_freeze_time", + rename = "metrics-bad-token-detection-token-freeze-time", + with = "humantime_serde" + )] + pub metrics_strategy_token_freeze_time: Duration, } impl Default for BadTokenDetectionConfig { @@ -742,3 +751,7 @@ fn default_settle_queue_size() -> usize { fn default_metrics_bad_token_detector_log_only() -> bool { true } + +fn default_metrics_bad_token_detector_freeze_time() -> Duration { + Duration::from_secs(60 * 10) +} diff --git a/crates/driver/src/infra/solver/mod.rs b/crates/driver/src/infra/solver/mod.rs index cc0802a5a2..1b361b1c0b 100644 --- a/crates/driver/src/infra/solver/mod.rs +++ b/crates/driver/src/infra/solver/mod.rs @@ -22,7 +22,7 @@ use { derive_more::{From, Into}, num::BigRational, reqwest::header::HeaderName, - std::collections::HashMap, + std::{collections::HashMap, time::Duration}, tap::TapFallible, thiserror::Error, tracing::Instrument, @@ -317,4 +317,5 @@ pub struct BadTokenDetection { pub metrics_strategy_failure_ratio: f64, pub metrics_strategy_required_measurements: u32, pub metrics_strategy_log_only: bool, + pub metrics_strategy_token_freeze_time: Duration, } From 50eb0a00852de626cae1eaa1bcfd9ac9987555a1 Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Fri, 3 Jan 2025 15:51:26 +0100 Subject: [PATCH 03/23] Introduce Quality::Unknown variant (#3206) # Description Sparked by this [discussion](https://github.com/cowprotocol/services/pull/3205#discussion_r1901647654) I think that using `Option` is a bit implicit. # Changes I adjusted the bad token detection code to use `Quality::Unknown` where possible to make it more explicit what the strategy thinks about a token. This makes some of the code more verbose so I'm not super sold on this change yet. Let me know if you think this improves things. --- .../domain/competition/bad_tokens/cache.rs | 28 ++++++---- .../domain/competition/bad_tokens/metrics.rs | 56 ++++++++++--------- .../src/domain/competition/bad_tokens/mod.rs | 43 +++++++------- .../competition/bad_tokens/simulation.rs | 23 ++++---- 4 files changed, 80 insertions(+), 70 deletions(-) diff --git a/crates/driver/src/domain/competition/bad_tokens/cache.rs b/crates/driver/src/domain/competition/bad_tokens/cache.rs index 88f11fcd8e..7e8693154c 100644 --- a/crates/driver/src/domain/competition/bad_tokens/cache.rs +++ b/crates/driver/src/domain/competition/bad_tokens/cache.rs @@ -25,7 +25,7 @@ struct CacheEntry { /// when the decision on the token quality was made last_updated: Instant, /// whether the token is supported or not - quality: Quality, + is_supported: bool, } impl Cache { @@ -39,23 +39,21 @@ impl Cache { } /// Updates whether or not a token should be considered supported. - pub fn update_quality(&self, token: eth::TokenAddress, quality: Quality, now: Instant) { + pub fn update_quality(&self, token: eth::TokenAddress, is_supported: bool, now: Instant) { self.0 .cache .entry(token) - .and_modify(|value| { - if quality == Quality::Unsupported - || now.duration_since(value.last_updated) > self.0.max_age - { + .and_modify(|token| { + if !is_supported || now.duration_since(token.last_updated) > self.0.max_age { // Only update the value if the cached value is outdated by now or // if the new value is "Unsupported". This means on conflicting updates // we err on the conservative side and assume a token is unsupported. - value.quality = quality; + token.is_supported = is_supported; } - value.last_updated = now; + token.last_updated = now; }) .or_insert_with(|| CacheEntry { - quality, + is_supported, last_updated: now, }); } @@ -69,9 +67,15 @@ impl Cache { /// Returns the quality of the token if the cached value has not expired /// yet. - pub fn get_quality(&self, token: ð::TokenAddress, now: Instant) -> Option { - let token = self.0.cache.get(token)?; + pub fn get_quality(&self, token: ð::TokenAddress, now: Instant) -> Quality { + let Some(token) = self.0.cache.get(token) else { + return Quality::Unknown; + }; let still_valid = now.duration_since(token.last_updated) > self.0.max_age; - still_valid.then_some(token.quality) + match (still_valid, token.is_supported) { + (false, _) => Quality::Unknown, + (true, true) => Quality::Supported, + (true, false) => Quality::Unsupported, + } } } diff --git a/crates/driver/src/domain/competition/bad_tokens/metrics.rs b/crates/driver/src/domain/competition/bad_tokens/metrics.rs index 43e444dca2..c6951fbea6 100644 --- a/crates/driver/src/domain/competition/bad_tokens/metrics.rs +++ b/crates/driver/src/domain/competition/bad_tokens/metrics.rs @@ -45,27 +45,36 @@ impl Detector { } } - pub fn get_quality(&self, token: ð::TokenAddress, now: Instant) -> Option { - let stats = self.counter.get(token)?; + pub fn get_quality(&self, token: ð::TokenAddress, now: Instant) -> Quality { + let Some(stats) = self.counter.get(token) else { + return Quality::Unknown; + }; + if stats .flagged_unsupported_at .is_some_and(|t| now.duration_since(t) > self.token_freeze_time) { // Sometimes tokens only cause issues temporarily. If the token's freeze - // period expired we give it another chance to see if it still behaves badly. - return None; + // period expired we pretend we don't have enough information to give it + // another chance. If it still behaves badly it will get frozen immediately. + return Quality::Unknown; } - let is_unsupported = self.stats_indicate_unsupported(&stats); - (!self.log_only && is_unsupported).then_some(Quality::Unsupported) + match self.log_only { + true => Quality::Supported, + false => self.quality_based_on_stats(&stats), + } } - fn stats_indicate_unsupported(&self, stats: &TokenStatistics) -> bool { - let token_failure_ratio = match stats.attempts { - 0 => return false, - attempts => f64::from(stats.fails) / f64::from(attempts), - }; - stats.attempts >= self.required_measurements && token_failure_ratio >= self.failure_ratio + fn quality_based_on_stats(&self, stats: &TokenStatistics) -> Quality { + if stats.attempts < self.required_measurements { + return Quality::Unknown; + } + let token_failure_ratio = f64::from(stats.fails) / f64::from(stats.attempts); + match token_failure_ratio >= self.failure_ratio { + true => Quality::Unsupported, + false => Quality::Supported, + } } /// Updates the tokens that participated in settlements by @@ -96,7 +105,7 @@ impl Detector { }); // token neeeds to be frozen as unsupported for a while - if self.stats_indicate_unsupported(&stats) + if self.quality_based_on_stats(&stats) == Quality::Unsupported && stats .flagged_unsupported_at .is_none_or(|t| now.duration_since(t) > self.token_freeze_time) @@ -128,28 +137,23 @@ mod tests { let token_a = eth::TokenAddress(eth::ContractAddress(H160([1; 20]))); let token_b = eth::TokenAddress(eth::ContractAddress(H160([2; 20]))); + let token_quality = || detector.get_quality(&token_a, Instant::now()); - // token is reported as supported while we don't have enough measurements - assert_eq!(detector.get_quality(&token_a, Instant::now()), None); + // token is reported as unknown while we don't have enough measurements + assert_eq!(token_quality(), Quality::Unknown); detector.update_tokens(&[(token_a, token_b)], true); - assert_eq!(detector.get_quality(&token_a, Instant::now()), None); + assert_eq!(token_quality(), Quality::Unknown); detector.update_tokens(&[(token_a, token_b)], true); // after we got enough measurements the token gets marked as bad - assert_eq!( - detector.get_quality(&token_a, Instant::now()), - Some(Quality::Unsupported) - ); + assert_eq!(token_quality(), Quality::Unsupported); - // after the freeze period is over the token gets reported as good again + // after the freeze period is over the token gets reported as unknown again tokio::time::sleep(FREEZE_DURATION).await; - assert_eq!(detector.get_quality(&token_a, Instant::now()), None); + assert_eq!(token_quality(), Quality::Unknown); // after an unfreeze another bad measurement is enough to freeze it again detector.update_tokens(&[(token_a, token_b)], true); - assert_eq!( - detector.get_quality(&token_a, Instant::now()), - Some(Quality::Unsupported) - ); + assert_eq!(token_quality(), Quality::Unsupported); } } diff --git a/crates/driver/src/domain/competition/bad_tokens/mod.rs b/crates/driver/src/domain/competition/bad_tokens/mod.rs index d724bea442..ab241183d5 100644 --- a/crates/driver/src/domain/competition/bad_tokens/mod.rs +++ b/crates/driver/src/domain/competition/bad_tokens/mod.rs @@ -23,6 +23,9 @@ pub enum Quality { /// contract - see /// * probably tons of other reasons Unsupported, + /// The detection strategy does not have enough data to make an informed + /// decision. + Unknown, } #[derive(Default)] @@ -67,26 +70,23 @@ impl Detector { let buy = self.get_token_quality(order.buy.token, now); match (sell, buy) { // both tokens supported => keep order - (Some(Quality::Supported), Some(Quality::Supported)) => Either::Left(order), + (Quality::Supported, Quality::Supported) => Either::Left(order), // at least 1 token unsupported => drop order - (Some(Quality::Unsupported), _) | (_, Some(Quality::Unsupported)) => { - Either::Right(order.uid) - } + (Quality::Unsupported, _) | (_, Quality::Unsupported) => Either::Right(order.uid), // sell token quality is unknown => keep order if token is supported - (None, _) => { + (Quality::Unknown, _) => { let Some(detector) = &self.simulation_detector else { // we can't determine quality => assume order is good return Either::Left(order); }; - let quality = detector.determine_sell_token_quality(&order, now).await; - match quality { - Some(Quality::Supported) => Either::Left(order), + match detector.determine_sell_token_quality(&order, now).await { + Quality::Supported => Either::Left(order), _ => Either::Right(order.uid), } } // buy token quality is unknown => keep order (because we can't // determine quality and assume it's good) - (_, None) => Either::Left(order), + (_, Quality::Unknown) => Either::Left(order), } }); let (supported_orders, removed_uids): (Vec<_>, Vec<_>) = join_all(token_quality_checks) @@ -120,22 +120,27 @@ impl Detector { } } - fn get_token_quality(&self, token: eth::TokenAddress, now: Instant) -> Option { - if let Some(quality) = self.hardcoded.get(&token) { - return Some(*quality); + fn get_token_quality(&self, token: eth::TokenAddress, now: Instant) -> Quality { + match self.hardcoded.get(&token) { + None | Some(Quality::Unknown) => (), + Some(quality) => return *quality, } - if let Some(detector) = &self.simulation_detector { - if let Some(quality) = detector.get_quality(&token, now) { - return Some(quality); - } + if let Some(Quality::Unsupported) = self + .simulation_detector + .as_ref() + .map(|d| d.get_quality(&token, now)) + { + return Quality::Unsupported; } - if let Some(metrics) = &self.metrics { - return metrics.get_quality(&token, now); + if let Some(Quality::Unsupported) = + self.metrics.as_ref().map(|m| m.get_quality(&token, now)) + { + return Quality::Unsupported; } - None + Quality::Unknown } } diff --git a/crates/driver/src/domain/competition/bad_tokens/simulation.rs b/crates/driver/src/domain/competition/bad_tokens/simulation.rs index fe3291c306..dd8eb5e0f4 100644 --- a/crates/driver/src/domain/competition/bad_tokens/simulation.rs +++ b/crates/driver/src/domain/competition/bad_tokens/simulation.rs @@ -32,7 +32,7 @@ pub struct Detector(Arc); struct Inner { cache: Cache, detector: TraceCallDetectorRaw, - sharing: BoxRequestSharing>, + sharing: BoxRequestSharing, } impl Detector { @@ -49,14 +49,11 @@ impl Detector { /// Simulates how the sell token behaves during transfers. Assumes that /// the order owner has the required sell token balance and approvals /// set. - pub async fn determine_sell_token_quality( - &self, - order: &Order, - now: Instant, - ) -> Option { + pub async fn determine_sell_token_quality(&self, order: &Order, now: Instant) -> Quality { let cache = &self.0.cache; - if let Some(quality) = cache.get_quality(&order.sell.token, now) { - return Some(quality); + let quality = cache.get_quality(&order.sell.token, now); + if quality != Quality::Unknown { + return quality; } // The simulation detector gets used by multiple solvers at the same time @@ -93,20 +90,20 @@ impl Detector { match result { Err(err) => { tracing::debug!(?err, token=?sell_token.0, "failed to determine token quality"); - None + Quality::Unknown } Ok(TokenQuality::Good) => { inner .cache - .update_quality(sell_token, Quality::Supported, now); - Some(Quality::Supported) + .update_quality(sell_token, true, now); + Quality::Supported } Ok(TokenQuality::Bad { reason }) => { tracing::debug!(reason, token=?sell_token.0, "cache token as unsupported"); inner .cache - .update_quality(sell_token, Quality::Unsupported, now); - Some(Quality::Unsupported) + .update_quality(sell_token, false, now); + Quality::Unsupported } } } From c971b6756b151ab13c1e03f681eb2c0a48c21994 Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Mon, 6 Jan 2025 11:22:25 +0100 Subject: [PATCH 04/23] Replace `once-cell` and `lazy-static` with `std` types (#3208) # Description Since the functionality of `once-cell` and `lazy-static` was moved into the standard library it's the idiomatic thing to use the `std` types instead of the crates. # Changes Replaced all uses of `once-cell` and `lazy-static` with the equivalent `std` type. --- Cargo.lock | 7 ---- Cargo.toml | 2 -- crates/driver/Cargo.toml | 1 - crates/driver/src/infra/time.rs | 6 ++-- crates/e2e/src/api/zeroex.rs | 28 ++++++++-------- crates/ethrpc/Cargo.toml | 1 - crates/ethrpc/src/multicall.rs | 11 +++---- crates/model/Cargo.toml | 1 - crates/model/src/lib.rs | 24 +++++++------- crates/observe/Cargo.toml | 1 - crates/observe/src/metrics.rs | 10 ++++-- crates/shared/Cargo.toml | 1 - crates/shared/src/external_prices.rs | 10 +++--- .../shared/src/price_estimation/native/mod.rs | 5 ++- .../sources/balancer_v2/swap/fixed_point.rs | 21 ++++++------ .../swap/fixed_point/logexpmath.rs | 33 +++++++++++-------- .../sources/balancer_v2/swap/stable_math.rs | 6 ++-- .../sources/balancer_v2/swap/weighted_math.rs | 12 +++---- .../src/sources/uniswap_v2/pool_fetching.rs | 10 +++--- crates/solver/Cargo.toml | 2 -- crates/solver/src/interactions/balancer_v2.rs | 11 +++---- crates/solver/src/liquidity/slippage.rs | 12 ++++--- crates/solver/src/liquidity_collector.rs | 12 ++++--- 23 files changed, 109 insertions(+), 118 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d2bbe2848c..05cfd11c17 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1816,7 +1816,6 @@ dependencies = [ "hyper", "indexmap 2.2.6", "itertools 0.12.1", - "lazy_static", "maplit", "mimalloc", "mockall 0.12.1", @@ -2077,7 +2076,6 @@ dependencies = [ "hex", "hex-literal", "itertools 0.12.1", - "lazy_static", "maplit", "mockall 0.12.1", "observe", @@ -3118,7 +3116,6 @@ dependencies = [ "derive_more 1.0.0", "hex", "hex-literal", - "lazy_static", "maplit", "num", "number", @@ -3331,7 +3328,6 @@ dependencies = [ "atty", "console-subscriber", "futures", - "once_cell", "pin-project-lite", "prometheus", "prometheus-metric-storage", @@ -4485,7 +4481,6 @@ dependencies = [ "humantime", "indexmap 2.2.6", "itertools 0.12.1", - "lazy_static", "maplit", "mockall 0.12.1", "model", @@ -4581,14 +4576,12 @@ dependencies = [ "hex", "hex-literal", "itertools 0.12.1", - "lazy_static", "maplit", "mockall 0.12.1", "model", "num", "number", "observe", - "once_cell", "primitive-types", "prometheus", "prometheus-metric-storage", diff --git a/Cargo.toml b/Cargo.toml index f2ce7391f6..6c819517e9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,11 +28,9 @@ humantime-serde = "1.1.1" hyper = "0.14.29" indexmap = "2.2.6" itertools = "0.12.1" -lazy_static = "1.4.0" maplit = "1.0.2" mockall = "0.12.1" num = "0.4.3" -once_cell = "1.19.0" primitive-types = "0.12" prometheus = "0.13.4" prometheus-metric-storage = "0.5.0" diff --git a/crates/driver/Cargo.toml b/crates/driver/Cargo.toml index 0675c6a046..83c2a1983d 100644 --- a/crates/driver/Cargo.toml +++ b/crates/driver/Cargo.toml @@ -35,7 +35,6 @@ hex-literal = { workspace = true } humantime = { workspace = true } humantime-serde = { workspace = true } hyper = { workspace = true } -lazy_static = { workspace = true } indexmap = { workspace = true, features = ["serde"] } itertools = { workspace = true } mimalloc = { workspace = true } diff --git a/crates/driver/src/infra/time.rs b/crates/driver/src/infra/time.rs index f954ea3f64..e5bc83ee73 100644 --- a/crates/driver/src/infra/time.rs +++ b/crates/driver/src/infra/time.rs @@ -7,7 +7,7 @@ pub fn now() -> chrono::DateTime { /// During tests, the time is fixed. #[cfg(test)] pub fn now() -> chrono::DateTime { - use std::sync::OnceLock; - static TIME: OnceLock> = OnceLock::new(); - TIME.get_or_init(chrono::Utc::now).to_owned() + use std::sync::LazyLock; + static TIME: LazyLock> = LazyLock::new(chrono::Utc::now); + *TIME } diff --git a/crates/e2e/src/api/zeroex.rs b/crates/e2e/src/api/zeroex.rs index bef51dffe9..f3db4a4d73 100644 --- a/crates/e2e/src/api/zeroex.rs +++ b/crates/e2e/src/api/zeroex.rs @@ -3,17 +3,14 @@ use { autopilot::domain::eth::U256, chrono::{DateTime, NaiveDateTime, Utc}, driver::domain::eth::H256, - ethcontract::{ - common::abi::{encode, Token}, - private::lazy_static, - }, + ethcontract::common::abi::{encode, Token}, hex_literal::hex, model::DomainSeparator, shared::{ zeroex_api, zeroex_api::{Order, OrderMetadata, OrderRecord, ZeroExSignature}, }, - std::net::SocketAddr, + std::{net::SocketAddr, sync::LazyLock}, warp::{Filter, Reply}, web3::{signing, types::H160}, }; @@ -155,18 +152,19 @@ struct ZeroExDomainSeparator([u8; 32]); impl ZeroExDomainSeparator { // See pub fn new(chain_id: u64, contract_addr: H160) -> Self { - lazy_static! { - /// The EIP-712 domain name used for computing the domain separator. - static ref DOMAIN_NAME: [u8; 32] = signing::keccak256(b"ZeroEx"); + /// The EIP-712 domain name used for computing the domain separator. + static DOMAIN_NAME: LazyLock<[u8; 32]> = LazyLock::new(|| signing::keccak256(b"ZeroEx")); - /// The EIP-712 domain version used for computing the domain separator. - static ref DOMAIN_VERSION: [u8; 32] = signing::keccak256(b"1.0.0"); + /// The EIP-712 domain version used for computing the domain separator. + static DOMAIN_VERSION: LazyLock<[u8; 32]> = LazyLock::new(|| signing::keccak256(b"1.0.0")); + + /// The EIP-712 domain type used computing the domain separator. + static DOMAIN_TYPE_HASH: LazyLock<[u8; 32]> = LazyLock::new(|| { + signing::keccak256( + b"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)", + ) + }); - /// The EIP-712 domain type used computing the domain separator. - static ref DOMAIN_TYPE_HASH: [u8; 32] = signing::keccak256( - b"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)", - ); - } let abi_encode_string = encode(&[ Token::FixedBytes((*DOMAIN_TYPE_HASH).into()), Token::FixedBytes((*DOMAIN_NAME).into()), diff --git a/crates/ethrpc/Cargo.toml b/crates/ethrpc/Cargo.toml index a6e628dabc..fa6a0f9b24 100644 --- a/crates/ethrpc/Cargo.toml +++ b/crates/ethrpc/Cargo.toml @@ -16,7 +16,6 @@ async-trait = { workspace = true } futures = { workspace = true } hex = { workspace = true } hex-literal = { workspace = true } -lazy_static = { workspace = true } mockall = { workspace = true } observe = { path = "../observe" } primitive-types = { workspace = true } diff --git a/crates/ethrpc/src/multicall.rs b/crates/ethrpc/src/multicall.rs index 38d1e72f03..855d5b753e 100644 --- a/crates/ethrpc/src/multicall.rs +++ b/crates/ethrpc/src/multicall.rs @@ -8,8 +8,7 @@ use { tokens::{self, Tokenize as _}, }, hex_literal::hex, - lazy_static::lazy_static, - std::iter, + std::{iter, sync::LazyLock}, web3::{ self, api::Eth, @@ -160,12 +159,12 @@ fn decode(len: usize, return_data: Bytes) -> Vec, ExecutionError> type ReturnData = Vec<(bool, ethcontract::Bytes>)>; fn decode_return_data(len: usize, return_data: Bytes) -> Result { - lazy_static! { - static ref KIND: [ParamType; 1] = [ParamType::Array(Box::new(ParamType::Tuple(vec![ + static KIND: LazyLock<[ParamType; 1]> = LazyLock::new(|| { + [ParamType::Array(Box::new(ParamType::Tuple(vec![ ParamType::Bool, ParamType::Bytes, - ])),)]; - } + ])))] + }); let tokens = ethabi::decode(&*KIND, &return_data.0)?; let (results,) = <(ReturnData,)>::from_token(Token::Tuple(tokens))?; diff --git a/crates/model/Cargo.toml b/crates/model/Cargo.toml index d6a8f19af1..b27956955f 100644 --- a/crates/model/Cargo.toml +++ b/crates/model/Cargo.toml @@ -17,7 +17,6 @@ chrono = { workspace = true, features = ["serde", "clock"] } derive_more = { workspace = true } hex = { workspace = true, default-features = false } hex-literal = { workspace = true } -lazy_static = { workspace = true } number = { path = "../number" } num = { workspace = true } primitive-types = { workspace = true } diff --git a/crates/model/src/lib.rs b/crates/model/src/lib.rs index 044bb67bf5..34f8ab64c0 100644 --- a/crates/model/src/lib.rs +++ b/crates/model/src/lib.rs @@ -12,9 +12,8 @@ pub mod trade; use { hex::{FromHex, FromHexError}, - lazy_static::lazy_static, primitive_types::H160, - std::fmt, + std::{fmt, sync::LazyLock}, web3::{ ethabi::{encode, Token}, signing, @@ -115,18 +114,19 @@ impl std::fmt::Debug for DomainSeparator { impl DomainSeparator { pub fn new(chain_id: u64, contract_address: H160) -> Self { - lazy_static! { - /// The EIP-712 domain name used for computing the domain separator. - static ref DOMAIN_NAME: [u8; 32] = signing::keccak256(b"Gnosis Protocol"); + /// The EIP-712 domain name used for computing the domain separator. + static DOMAIN_NAME: LazyLock<[u8; 32]> = + LazyLock::new(|| signing::keccak256(b"Gnosis Protocol")); - /// The EIP-712 domain version used for computing the domain separator. - static ref DOMAIN_VERSION: [u8; 32] = signing::keccak256(b"v2"); + /// The EIP-712 domain version used for computing the domain separator. + static DOMAIN_VERSION: LazyLock<[u8; 32]> = LazyLock::new(|| signing::keccak256(b"v2")); - /// The EIP-712 domain type used computing the domain separator. - static ref DOMAIN_TYPE_HASH: [u8; 32] = signing::keccak256( - b"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)", - ); - } + /// The EIP-712 domain type used computing the domain separator. + static DOMAIN_TYPE_HASH: LazyLock<[u8; 32]> = LazyLock::new(|| { + signing::keccak256( + b"EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)", + ) + }); let abi_encode_string = encode(&[ Token::Uint((*DOMAIN_TYPE_HASH).into()), Token::Uint((*DOMAIN_NAME).into()), diff --git a/crates/observe/Cargo.toml b/crates/observe/Cargo.toml index d02cbfc0ce..f2b1029ef0 100644 --- a/crates/observe/Cargo.toml +++ b/crates/observe/Cargo.toml @@ -10,7 +10,6 @@ atty = "0.2" async-trait = { workspace = true } console-subscriber = "0.3.0" futures = { workspace = true } -once_cell = { workspace = true } pin-project-lite = "0.2.14" prometheus = { workspace = true } prometheus-metric-storage = { workspace = true } diff --git a/crates/observe/src/metrics.rs b/crates/observe/src/metrics.rs index 6afb4175e0..b62023590c 100644 --- a/crates/observe/src/metrics.rs +++ b/crates/observe/src/metrics.rs @@ -1,13 +1,17 @@ use { - once_cell::sync::OnceCell, prometheus::Encoder, - std::{collections::HashMap, convert::Infallible, net::SocketAddr, sync::Arc}, + std::{ + collections::HashMap, + convert::Infallible, + net::SocketAddr, + sync::{Arc, OnceLock}, + }, tokio::task::{self, JoinHandle}, warp::{Filter, Rejection, Reply}, }; /// Global metrics registry used by all components. -static REGISTRY: OnceCell = OnceCell::new(); +static REGISTRY: OnceLock = OnceLock::new(); /// Configure global metrics registry. /// diff --git a/crates/shared/Cargo.toml b/crates/shared/Cargo.toml index 492b6bfb6e..87cb190f1f 100644 --- a/crates/shared/Cargo.toml +++ b/crates/shared/Cargo.toml @@ -34,7 +34,6 @@ hex-literal = { workspace = true } humantime = { workspace = true } indexmap = { workspace = true } itertools = { workspace = true } -lazy_static = { workspace = true } maplit = { workspace = true } mockall = { workspace = true } model = { path = "../model" } diff --git a/crates/shared/src/external_prices.rs b/crates/shared/src/external_prices.rs index f7fb0b427b..6c3dc66c84 100644 --- a/crates/shared/src/external_prices.rs +++ b/crates/shared/src/external_prices.rs @@ -9,10 +9,12 @@ use { crate::conversions::U256Ext, anyhow::{bail, Result}, ethcontract::{H160, U256}, - lazy_static::lazy_static, model::order::BUY_ETH_ADDRESS, num::{BigInt, BigRational, One as _, ToPrimitive as _}, - std::collections::{BTreeMap, HashMap}, + std::{ + collections::{BTreeMap, HashMap}, + sync::LazyLock, + }, }; /// A collection of external prices used for converting token amounts to native @@ -72,9 +74,7 @@ impl Default for ExternalPrices { } } -lazy_static! { - static ref UNIT: BigInt = BigInt::from(1_000_000_000_000_000_000_u128); -} +static UNIT: LazyLock = LazyLock::new(|| BigInt::from(1_000_000_000_000_000_000_u128)); /// Converts a token price from the orderbook API `/auction` endpoint to an /// native token exchange rate. diff --git a/crates/shared/src/price_estimation/native/mod.rs b/crates/shared/src/price_estimation/native/mod.rs index 31ca840575..9a81d44538 100644 --- a/crates/shared/src/price_estimation/native/mod.rs +++ b/crates/shared/src/price_estimation/native/mod.rs @@ -1,12 +1,11 @@ use { crate::price_estimation::{PriceEstimating, PriceEstimationError, Query}, bigdecimal::{BigDecimal, ToPrimitive}, - cached::once_cell::sync::Lazy, futures::FutureExt, model::order::OrderKind, number::nonzero::U256 as NonZeroU256, primitive_types::{H160, U256}, - std::sync::Arc, + std::sync::{Arc, LazyLock}, }; mod coingecko; @@ -19,7 +18,7 @@ pub type NativePriceEstimateResult = Result; /// Convert from normalized price to floating point price pub fn from_normalized_price(price: BigDecimal) -> Option { - static ONE_E18: Lazy = Lazy::new(|| BigDecimal::try_from(1e18).unwrap()); + static ONE_E18: LazyLock = LazyLock::new(|| BigDecimal::try_from(1e18).unwrap()); // Divide by 1e18 to reverse the multiplication by 1e18 let normalized_price = price / ONE_E18.clone(); diff --git a/crates/shared/src/sources/balancer_v2/swap/fixed_point.rs b/crates/shared/src/sources/balancer_v2/swap/fixed_point.rs index f4edd33701..94015f9ed9 100644 --- a/crates/shared/src/sources/balancer_v2/swap/fixed_point.rs +++ b/crates/shared/src/sources/balancer_v2/swap/fixed_point.rs @@ -7,13 +7,13 @@ use { super::error::Error, anyhow::{bail, ensure, Context, Result}, ethcontract::U256, - lazy_static::lazy_static, num::{BigInt, BigRational}, number::conversions::{big_int_to_u256, u256_to_big_int}, std::{ convert::TryFrom, fmt::{self, Debug, Formatter}, str::FromStr, + sync::LazyLock, }, }; @@ -27,16 +27,13 @@ mod logexpmath; /// including error codes, from which the name (Balancer Fixed Point). pub struct Bfp(U256); -lazy_static! { - static ref ONE_18: U256 = U256::exp10(18); - static ref ONE_18_BIGINT: BigInt = u256_to_big_int(&ONE_18); - static ref ZERO: Bfp = Bfp(U256::zero()); - static ref EPSILON: Bfp = Bfp(U256::one()); - static ref ONE: Bfp = Bfp(*ONE_18); - static ref TWO: Bfp = Bfp(*ONE_18 * 2); - static ref FOUR: Bfp = Bfp(*ONE_18 * 4); - static ref MAX_POW_RELATIVE_ERROR: Bfp = Bfp(10000_usize.into()); -} +static ONE_18: LazyLock = LazyLock::new(|| U256::exp10(18)); +static ONE_18_BIGINT: LazyLock = LazyLock::new(|| u256_to_big_int(&ONE_18)); +static ZERO: LazyLock = LazyLock::new(|| Bfp(U256::zero())); +static ONE: LazyLock = LazyLock::new(|| Bfp(*ONE_18)); +static TWO: LazyLock = LazyLock::new(|| Bfp(*ONE_18 * 2)); +static FOUR: LazyLock = LazyLock::new(|| Bfp(*ONE_18 * 4)); +static MAX_POW_RELATIVE_ERROR: LazyLock = LazyLock::new(|| Bfp(10000_usize.into())); impl From for Bfp { fn from(num: usize) -> Self { @@ -223,6 +220,8 @@ mod tests { num::{BigInt, One, Zero}, }; + static EPSILON: LazyLock = LazyLock::new(|| Bfp(U256::one())); + #[test] fn parsing() { assert_eq!("1".parse::().unwrap(), Bfp::one()); diff --git a/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs b/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs index 660049a026..302053b7f6 100644 --- a/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs +++ b/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs @@ -5,27 +5,32 @@ use { super::super::error::Error, ethcontract::{I256, U256}, - lazy_static::lazy_static, - std::convert::TryFrom, + std::{convert::TryFrom, sync::LazyLock}, }; /// Fixed point number stored in a type of bit size 256 that stores exactly 18 /// decimal digits. type Ufixed256x18 = U256; -lazy_static! { - static ref ONE_18: I256 = I256::exp10(18); - static ref ONE_20: I256 = I256::exp10(20); - static ref ONE_36: I256 = I256::exp10(36); - static ref UFIXED256X18_ONE: Ufixed256x18 = U256::try_from(*ONE_18).unwrap(); - static ref MAX_NATURAL_EXPONENT: I256 = ONE_18.checked_mul(I256::from(130_i128)).unwrap(); - static ref MIN_NATURAL_EXPONENT: I256 = ONE_18.checked_mul(I256::from(-41_i128)).unwrap(); - static ref LN_36_LOWER_BOUND: I256 = ONE_18.checked_sub(I256::exp10(17)).unwrap(); - static ref LN_36_UPPER_BOUND: I256 = ONE_18.checked_add(I256::exp10(17)).unwrap(); - static ref MILD_EXPONENT_BOUND: Ufixed256x18 = (U256::one() << 254_u32) +static ONE_18: LazyLock = LazyLock::new(|| I256::exp10(18)); +static ONE_20: LazyLock = LazyLock::new(|| I256::exp10(20)); +static ONE_36: LazyLock = LazyLock::new(|| I256::exp10(36)); +static UFIXED256X18_ONE: LazyLock = + LazyLock::new(|| U256::try_from(*ONE_18).unwrap()); +static MAX_NATURAL_EXPONENT: LazyLock = + LazyLock::new(|| ONE_18.checked_mul(I256::from(130_i128)).unwrap()); +static MIN_NATURAL_EXPONENT: LazyLock = + LazyLock::new(|| ONE_18.checked_mul(I256::from(-41_i128)).unwrap()); +static LN_36_LOWER_BOUND: LazyLock = + LazyLock::new(|| ONE_18.checked_sub(I256::exp10(17)).unwrap()); +static LN_36_UPPER_BOUND: LazyLock = + LazyLock::new(|| ONE_18.checked_add(I256::exp10(17)).unwrap()); +static MILD_EXPONENT_BOUND: LazyLock = LazyLock::new(|| { + (U256::one() << 254_u32) .checked_div(U256::try_from(*ONE_20).unwrap()) - .unwrap(); -} + .unwrap() +}); + fn constant_x_20(i: u32) -> I256 { match i { 2 => 3_200_000_000_000_000_000_000_i128, diff --git a/crates/shared/src/sources/balancer_v2/swap/stable_math.rs b/crates/shared/src/sources/balancer_v2/swap/stable_math.rs index bedd679b78..f7610ef2b0 100644 --- a/crates/shared/src/sources/balancer_v2/swap/stable_math.rs +++ b/crates/shared/src/sources/balancer_v2/swap/stable_math.rs @@ -6,12 +6,10 @@ use { super::error::Error, crate::sources::balancer_v2::swap::{fixed_point::Bfp, math::BalU256}, ethcontract::U256, - lazy_static::lazy_static, + std::sync::LazyLock, }; -lazy_static! { - pub static ref AMP_PRECISION: U256 = U256::from(1000); -} +pub static AMP_PRECISION: LazyLock = LazyLock::new(|| U256::from(1000)); /// https://github.com/balancer-labs/balancer-v2-monorepo/blob/9eb7e44a4e9ebbadfe3c6242a086118298cadc9f/pkg/pool-stable-phantom/contracts/StableMath.sol#L57-L119 fn calculate_invariant(amplification_parameter: U256, balances: &[Bfp]) -> Result { diff --git a/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs b/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs index ae9fb72268..5b0aafa701 100644 --- a/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs +++ b/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs @@ -5,16 +5,14 @@ use { super::{error::Error, fixed_point::Bfp}, ethcontract::U256, - lazy_static::lazy_static, + std::sync::LazyLock, }; // https://github.com/balancer-labs/balancer-v2-monorepo/blob/6c9e24e22d0c46cca6dd15861d3d33da61a60b98/pkg/core/contracts/pools/weighted/WeightedMath.sol#L36-L37 -lazy_static! { - static ref MAX_IN_RATIO: Bfp = - Bfp::from_wei(U256::exp10(17).checked_mul(3_u32.into()).unwrap()); - static ref MAX_OUT_RATIO: Bfp = - Bfp::from_wei(U256::exp10(17).checked_mul(3_u32.into()).unwrap()); -} +static MAX_IN_RATIO: LazyLock = + LazyLock::new(|| Bfp::from_wei(U256::exp10(17).checked_mul(3_u32.into()).unwrap())); +static MAX_OUT_RATIO: LazyLock = + LazyLock::new(|| Bfp::from_wei(U256::exp10(17).checked_mul(3_u32.into()).unwrap())); /// https://github.com/balancer-labs/balancer-v2-monorepo/blob/6c9e24e22d0c46cca6dd15861d3d33da61a60b98/pkg/core/contracts/pools/weighted/WeightedMath.sol#L69-L100 /// It is not possible for the following addition balance_in.add(amount_in) to diff --git a/crates/shared/src/sources/uniswap_v2/pool_fetching.rs b/crates/shared/src/sources/uniswap_v2/pool_fetching.rs index 45bc54c536..b7966663f9 100644 --- a/crates/shared/src/sources/uniswap_v2/pool_fetching.rs +++ b/crates/shared/src/sources/uniswap_v2/pool_fetching.rs @@ -10,15 +10,17 @@ use { }, model::TokenPair, num::rational::Ratio, - std::{collections::HashSet, sync::RwLock, time::Duration}, + std::{ + collections::HashSet, + sync::{LazyLock, RwLock}, + time::Duration, + }, ttl_cache::TtlCache, }; const POOL_SWAP_GAS_COST: usize = 60_000; -lazy_static::lazy_static! { - static ref POOL_MAX_RESERVES: U256 = U256::from((1u128 << 112) - 1); -} +static POOL_MAX_RESERVES: LazyLock = LazyLock::new(|| U256::from((1u128 << 112) - 1)); /// This type denotes `(reserve_a, reserve_b, token_b)` where /// `reserve_a` refers to the reserve of the excluded token. diff --git a/crates/solver/Cargo.toml b/crates/solver/Cargo.toml index aa80153249..fdb0d4d768 100644 --- a/crates/solver/Cargo.toml +++ b/crates/solver/Cargo.toml @@ -22,13 +22,11 @@ observe = { path = "../observe" } hex = { workspace = true } hex-literal = { workspace = true } itertools = { workspace = true } -lazy_static = { workspace = true } maplit = { workspace = true } mockall = { workspace = true } model = { path = "../model" } num = { workspace = true } number = { path = "../number" } -once_cell = { workspace = true } primitive-types = { workspace = true } prometheus = { workspace = true } prometheus-metric-storage = { workspace = true } diff --git a/crates/solver/src/interactions/balancer_v2.rs b/crates/solver/src/interactions/balancer_v2.rs index abf6f2b4c1..6239843568 100644 --- a/crates/solver/src/interactions/balancer_v2.rs +++ b/crates/solver/src/interactions/balancer_v2.rs @@ -6,6 +6,7 @@ use { http_solver::model::TokenAmount, interaction::{EncodedInteraction, Interaction}, }, + std::sync::LazyLock, }; #[derive(Clone, Debug)] @@ -18,12 +19,10 @@ pub struct BalancerSwapGivenOutInteraction { pub user_data: Bytes>, } -lazy_static::lazy_static! { - /// An impossibly distant future timestamp. Note that we use `0x80000...00` - /// as the value so that it is mostly 0's to save small amounts of gas on - /// calldata. - pub static ref NEVER: U256 = U256::from(1) << 255; -} +/// An impossibly distant future timestamp. Note that we use `0x80000...00` +/// as the value so that it is mostly 0's to save small amounts of gas on +/// calldata. +pub static NEVER: LazyLock = LazyLock::new(|| U256::from(1) << 255); impl BalancerSwapGivenOutInteraction { pub fn encode_swap(&self) -> EncodedInteraction { diff --git a/crates/solver/src/liquidity/slippage.rs b/crates/solver/src/liquidity/slippage.rs index a4185aa909..60940314c4 100644 --- a/crates/solver/src/liquidity/slippage.rs +++ b/crates/solver/src/liquidity/slippage.rs @@ -5,9 +5,8 @@ use { anyhow::{Context as _, Result}, ethcontract::U256, num::{BigInt, BigRational, CheckedDiv, Integer as _, ToPrimitive as _}, - once_cell::sync::OnceCell, shared::{external_prices::ExternalPrices, http_solver::model::TokenAmount}, - std::{borrow::Cow, cmp}, + std::{borrow::Cow, cmp, sync::LazyLock}, }; /// Constant maximum slippage of 10 BPS (0.1%) to use for on-chain liquidity. @@ -86,9 +85,12 @@ impl SlippageContext<'_> { impl Default for SlippageContext<'static> { fn default() -> Self { - static CONTEXT: OnceCell<(ExternalPrices, SlippageCalculator)> = OnceCell::new(); - let (prices, calculator) = CONTEXT.get_or_init(Default::default); - Self { prices, calculator } + static CONTEXT: LazyLock<(ExternalPrices, SlippageCalculator)> = + LazyLock::new(Default::default); + Self { + prices: &CONTEXT.0, + calculator: &CONTEXT.1, + } } } diff --git a/crates/solver/src/liquidity_collector.rs b/crates/solver/src/liquidity_collector.rs index 52a3fed2bc..eefd0c6ccd 100644 --- a/crates/solver/src/liquidity_collector.rs +++ b/crates/solver/src/liquidity_collector.rs @@ -2,9 +2,13 @@ use { crate::liquidity::Liquidity, anyhow::Result, model::TokenPair, - once_cell::sync::OnceCell, shared::{baseline_solver::BaseTokens, recent_block_cache::Block}, - std::{collections::HashSet, future::Future, sync::Arc, time::Duration}, + std::{ + collections::HashSet, + future::Future, + sync::{Arc, OnceLock}, + time::Duration, + }, tracing::Instrument, }; @@ -50,7 +54,7 @@ impl LiquidityCollecting for LiquidityCollector { /// succeeds. Until the liquidity source has been initialised no liquidity will /// be provided. pub struct BackgroundInitLiquiditySource { - liquidity_source: Arc>, + liquidity_source: Arc>, } impl BackgroundInitLiquiditySource { @@ -66,7 +70,7 @@ impl BackgroundInitLiquiditySource { .liquidity_enabled .with_label_values(&[label]) .set(0); - let liquidity_source = Arc::new(OnceCell::new()); + let liquidity_source = Arc::new(OnceLock::new()); let inner = liquidity_source.clone(); let inner_label = label.to_owned(); tokio::task::spawn( From 0db8f43f9810124cd1dcca45d4c53d3a4ffb9eaf Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Mon, 6 Jan 2025 11:31:11 +0100 Subject: [PATCH 05/23] Remove `ttl-cache` dependency (#3207) # Description We currently have 2 dependencies for size and time based caching strategies. Given that we only have 1 usage of `ttl-cache` I replaced it with `cached` to reduce the total number of dependencies. One minor downside is that this now requires to take one more write lock but since there shouldn't be significant contention on the lock to begin with that should be okay. --- Cargo.lock | 16 ---------------- crates/shared/Cargo.toml | 1 - .../src/sources/uniswap_v2/pool_fetching.rs | 14 ++++++-------- 3 files changed, 6 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 05cfd11c17..cdeebd27b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2925,12 +2925,6 @@ dependencies = [ "vcpkg", ] -[[package]] -name = "linked-hash-map" -version = "0.5.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f" - [[package]] name = "linux-raw-sys" version = "0.4.14" @@ -4506,7 +4500,6 @@ dependencies = [ "tokio", "tracing", "tracing-subscriber", - "ttl_cache", "url", "web3", ] @@ -5413,15 +5406,6 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" -[[package]] -name = "ttl_cache" -version = "0.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4189890526f0168710b6ee65ceaedf1460c48a14318ceec933cb26baa492096a" -dependencies = [ - "linked-hash-map", -] - [[package]] name = "typenum" version = "1.17.0" diff --git a/crates/shared/Cargo.toml b/crates/shared/Cargo.toml index 87cb190f1f..622f3cf536 100644 --- a/crates/shared/Cargo.toml +++ b/crates/shared/Cargo.toml @@ -22,7 +22,6 @@ contracts = { path = "../contracts" } dashmap = { workspace = true } database = { path = "../database" } derive_more = { workspace = true } -ttl_cache = "0.5" derivative = { workspace = true } ethcontract = { workspace = true } ethrpc = { path = "../ethrpc" } diff --git a/crates/shared/src/sources/uniswap_v2/pool_fetching.rs b/crates/shared/src/sources/uniswap_v2/pool_fetching.rs index b7966663f9..377ecdcc6d 100644 --- a/crates/shared/src/sources/uniswap_v2/pool_fetching.rs +++ b/crates/shared/src/sources/uniswap_v2/pool_fetching.rs @@ -2,6 +2,7 @@ use { super::pair_provider::PairProvider, crate::{baseline_solver::BaselineSolvable, ethrpc::Web3, recent_block_cache::Block}, anyhow::Result, + cached::{Cached, TimedCache}, contracts::{errors::EthcontractErrorType, IUniswapLikePair, ERC20}, ethcontract::{errors::MethodError, BlockId, H160, U256}, futures::{ @@ -15,7 +16,6 @@ use { sync::{LazyLock, RwLock}, time::Duration, }, - ttl_cache::TtlCache, }; const POOL_SWAP_GAS_COST: usize = 60_000; @@ -184,8 +184,7 @@ impl BaselineSolvable for Pool { pub struct PoolFetcher { pub pool_reader: Reader, pub web3: Web3, - pub cache_time: Duration, - pub non_existent_pools: RwLock>, + pub non_existent_pools: RwLock>, } impl PoolFetcher { @@ -193,8 +192,7 @@ impl PoolFetcher { Self { pool_reader: reader, web3, - cache_time, - non_existent_pools: RwLock::new(TtlCache::new(usize::MAX)), + non_existent_pools: RwLock::new(TimedCache::with_lifespan(cache_time.as_secs())), } } } @@ -207,8 +205,8 @@ where async fn fetch(&self, token_pairs: HashSet, at_block: Block) -> Result> { let mut token_pairs: Vec<_> = token_pairs.into_iter().collect(); { - let non_existent_pools = self.non_existent_pools.read().unwrap(); - token_pairs.retain(|pair| !non_existent_pools.contains_key(pair)); + let mut non_existent_pools = self.non_existent_pools.write().unwrap(); + token_pairs.retain(|pair| non_existent_pools.cache_get(pair).is_none()); } let block = BlockId::Number(at_block.into()); let futures = token_pairs @@ -230,7 +228,7 @@ where tracing::debug!(token_pairs = ?new_missing_pairs, "stop indexing liquidity"); let mut non_existent_pools = self.non_existent_pools.write().unwrap(); for pair in new_missing_pairs { - non_existent_pools.insert(pair, (), self.cache_time); + non_existent_pools.cache_set(pair, ()); } } Ok(pools) From 07f4db6a02f8c263319c94e018a1195ddbe96b72 Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Mon, 6 Jan 2025 11:37:28 +0100 Subject: [PATCH 06/23] Move `time` dependency into `observe` crate (#3209) # Description Moves the `time` dependency into the `observe` crate because it's only used there. Also updates it to the latest version. --- Cargo.lock | 8 ++++---- Cargo.toml | 1 - crates/observe/Cargo.toml | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cdeebd27b2..d91055a2bd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5063,9 +5063,9 @@ dependencies = [ [[package]] name = "time" -version = "0.3.36" +version = "0.3.37" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5dfd88e563464686c916c7e46e623e520ddc6d79fa6641390f2e3fa86e83e885" +checksum = "35e7868883861bd0e56d9ac6efcaaca0d6d5d82a2a7ec8209ff492c07cf37b21" dependencies = [ "deranged", "itoa", @@ -5086,9 +5086,9 @@ checksum = "ef927ca75afb808a4d64dd374f00a2adf8d0fcff8e7b184af886c3c87ec4a3f3" [[package]] name = "time-macros" -version = "0.2.18" +version = "0.2.19" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f252a68540fde3a3877aeea552b832b40ab9a69e318efd078774a01ddee1ccf" +checksum = "2834e6017e3e5e4b9834939793b282bc03b37a3336245fa820e35e233e2a85de" dependencies = [ "num-conv", "time-core", diff --git a/Cargo.toml b/Cargo.toml index 6c819517e9..fbbd160162 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,7 +44,6 @@ serde_with = "3.8.1" sqlx = { version = "0.7", default-features = false, features = ["runtime-tokio", "tls-native-tls", "bigdecimal", "chrono", "postgres", "macros"] } strum = { version = "0.26.2", features = ["derive"] } tempfile = "3.10.1" -time = { version = "0.3.36", features = ["macros"] } thiserror = "1.0.61" toml = "0.8.14" tokio = { version = "1.38.0", features = ["tracing"] } diff --git a/crates/observe/Cargo.toml b/crates/observe/Cargo.toml index f2b1029ef0..75ebe24279 100644 --- a/crates/observe/Cargo.toml +++ b/crates/observe/Cargo.toml @@ -13,7 +13,7 @@ futures = { workspace = true } pin-project-lite = "0.2.14" prometheus = { workspace = true } prometheus-metric-storage = { workspace = true } -time = { workspace = true } +time = { version = "0.3.37", features = ["macros"] } tokio = { workspace = true, features = [ "fs" ] } tracing = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter", "fmt", "time"] } From 4d8206811464aaf3f9ac30c0c15a689f99ae91dd Mon Sep 17 00:00:00 2001 From: ilya Date: Mon, 6 Jan 2025 11:12:01 +0000 Subject: [PATCH 07/23] Get token first trade block API (#3197) # Description This is a pre-requisite for improving `internal buffers at risk` alert by eliminating false positives. The idea is to provide the token's first trade timestamp so https://github.com/cowprotocol/tenderly-alerts/ can then decide whether the token should be ignored. Currently, there is no way to return the timestamp since the `order_events` table gets cleaned up periodically. Technically, it can be used to get a timestamp, but the result will be useless if a token was deployed more than 30 days ago and wasn't traded for the last 30 days. Instead, the block number is returned, so Tenderly RPC must be used to fetch the block's timestamp. Otherwise, this would require access to an archive node from the orderbook side. ## Changes For some reason, this query takes around 20s on mainnet-prod for avg-popular tokens. Unfortunately, this couldn't be reproduced locally. I assume this is related to available resources on prod. I added indexes that improved the query speed ~x2.5 times. ## Caching To avoid querying the DB for the same tokens too often, I would consider introducing caching on the NGINX side rather than in memory since we often have multiple orderbook instances. Also, the first trade block never changes and can be kept in the NGINX cache forever. All the errors won't be kept in the cache. Requires an infra PR. ## How to test The query is tested on prod and locally. This would require further testing on prod by collecting metrics and adjusting resources. Potentially, `work_mem`, `max_parallel_workers_per_gather`, etc. --- crates/database/src/trades.rs | 52 +++++++++++++++++++ crates/orderbook/src/api.rs | 7 ++- .../orderbook/src/api/get_token_metadata.rs | 31 +++++++++++ crates/orderbook/src/database/orders.rs | 23 +++++++- crates/orderbook/src/dto/mod.rs | 8 +++ database/sql/V077__orders_token_indexes.sql | 3 ++ 6 files changed, 122 insertions(+), 2 deletions(-) create mode 100644 crates/orderbook/src/api/get_token_metadata.rs create mode 100644 database/sql/V077__orders_token_indexes.sql diff --git a/crates/database/src/trades.rs b/crates/database/src/trades.rs index 36c4c07dfd..38d2ca06d0 100644 --- a/crates/database/src/trades.rs +++ b/crates/database/src/trades.rs @@ -106,6 +106,31 @@ AND t.log_index BETWEEN (SELECT * from previous_settlement) AND $2 .await } +pub async fn token_first_trade_block( + ex: &mut PgConnection, + token: Address, +) -> Result, sqlx::Error> { + const QUERY: &str = r#" +SELECT MIN(sub.block_number) AS earliest_block +FROM ( + SELECT MIN(t.block_number) AS block_number + FROM trades t + JOIN orders o ON t.order_uid = o.uid + WHERE o.sell_token = $1 OR o.buy_token = $1 + + UNION ALL + + SELECT MIN(t.block_number) AS block_number + FROM trades t + JOIN jit_orders j ON t.order_uid = j.uid + WHERE j.sell_token = $1 OR j.buy_token = $1 +) AS sub +"#; + + let (block_number,) = sqlx::query_as(QUERY).bind(token).fetch_one(ex).await?; + Ok(block_number) +} + #[cfg(test)] mod tests { use { @@ -579,4 +604,31 @@ mod tests { }] ); } + + #[tokio::test] + #[ignore] + async fn postgres_token_first_trade_block() { + let mut db = PgConnection::connect("postgresql://").await.unwrap(); + let mut db = db.begin().await.unwrap(); + crate::clear_DANGER_(&mut db).await.unwrap(); + + let token = Default::default(); + assert_eq!(token_first_trade_block(&mut db, token).await.unwrap(), None); + + let (owners, order_ids) = generate_owners_and_order_ids(2, 2).await; + let event_index_a = EventIndex { + block_number: 123, + log_index: 0, + }; + let event_index_b = EventIndex { + block_number: 124, + log_index: 0, + }; + add_order_and_trade(&mut db, owners[0], order_ids[0], event_index_a, None, None).await; + add_order_and_trade(&mut db, owners[1], order_ids[1], event_index_b, None, None).await; + assert_eq!( + token_first_trade_block(&mut db, token).await.unwrap(), + Some(123) + ); + } } diff --git a/crates/orderbook/src/api.rs b/crates/orderbook/src/api.rs index a272950148..8eb7321f1f 100644 --- a/crates/orderbook/src/api.rs +++ b/crates/orderbook/src/api.rs @@ -23,6 +23,7 @@ mod get_order_by_uid; mod get_order_status; mod get_orders_by_tx; mod get_solver_competition; +mod get_token_metadata; mod get_total_surplus; mod get_trades; mod get_user_orders; @@ -105,7 +106,11 @@ pub fn handle_all_routes( ), ( "v1/get_total_surplus", - box_filter(get_total_surplus::get(database)), + box_filter(get_total_surplus::get(database.clone())), + ), + ( + "v1/get_token_metadata", + box_filter(get_token_metadata::get_token_metadata(database)), ), ]; diff --git a/crates/orderbook/src/api/get_token_metadata.rs b/crates/orderbook/src/api/get_token_metadata.rs new file mode 100644 index 0000000000..a08ec3a09d --- /dev/null +++ b/crates/orderbook/src/api/get_token_metadata.rs @@ -0,0 +1,31 @@ +use { + crate::database::Postgres, + hyper::StatusCode, + primitive_types::H160, + std::convert::Infallible, + warp::{reply, Filter, Rejection}, +}; + +fn get_native_prices_request() -> impl Filter + Clone { + warp::path!("v1" / "token" / H160 / "metadata").and(warp::get()) +} + +pub fn get_token_metadata( + db: Postgres, +) -> impl Filter + Clone { + get_native_prices_request().and_then(move |token: H160| { + let db = db.clone(); + async move { + let result = db.token_metadata(&token).await; + let response = match result { + Ok(metadata) => reply::with_status(reply::json(&metadata), StatusCode::OK), + Err(err) => { + tracing::error!(?err, ?token, "Failed to fetch token's first trade block"); + crate::api::internal_error_reply() + } + }; + + Result::<_, Infallible>::Ok(response) + } + }) +} diff --git a/crates/orderbook/src/database/orders.rs b/crates/orderbook/src/database/orders.rs index d077b1c8a2..51bce8a8c6 100644 --- a/crates/orderbook/src/database/orders.rs +++ b/crates/orderbook/src/database/orders.rs @@ -1,6 +1,6 @@ use { super::Postgres, - crate::orderbook::AddOrderError, + crate::{dto::TokenMetadata, orderbook::AddOrderError}, anyhow::{Context as _, Result}, app_data::AppDataHash, async_trait::async_trait, @@ -492,6 +492,27 @@ impl Postgres { .map(full_order_into_model_order) .collect::>>() } + + pub async fn token_metadata(&self, token: &H160) -> Result { + let timer = super::Metrics::get() + .database_queries + .with_label_values(&["token_first_trade_block"]) + .start_timer(); + + let mut ex = self.pool.acquire().await?; + let block_number = database::trades::token_first_trade_block(&mut ex, ByteArray(token.0)) + .await + .map_err(anyhow::Error::from)? + .map(u32::try_from) + .transpose() + .map_err(anyhow::Error::from)?; + + timer.stop_and_record(); + + Ok(TokenMetadata { + first_trade_block: block_number, + }) + } } #[async_trait] diff --git a/crates/orderbook/src/dto/mod.rs b/crates/orderbook/src/dto/mod.rs index fb3365dd95..b706e79d4d 100644 --- a/crates/orderbook/src/dto/mod.rs +++ b/crates/orderbook/src/dto/mod.rs @@ -5,3 +5,11 @@ pub use { auction::{Auction, AuctionId, AuctionWithId}, order::Order, }; +use {serde::Serialize, serde_with::serde_as}; + +#[serde_as] +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +pub struct TokenMetadata { + pub first_trade_block: Option, +} diff --git a/database/sql/V077__orders_token_indexes.sql b/database/sql/V077__orders_token_indexes.sql new file mode 100644 index 0000000000..d212427efd --- /dev/null +++ b/database/sql/V077__orders_token_indexes.sql @@ -0,0 +1,3 @@ +CREATE INDEX orders_sell_buy_tokens ON orders (sell_token, buy_token); + +CREATE INDEX jit_orders_sell_buy_tokens ON jit_orders (sell_token, buy_token); From 9b6743aac8323ee82cfe288ea84beea7f6714655 Mon Sep 17 00:00:00 2001 From: m-lord-renkse <160488334+m-lord-renkse@users.noreply.github.com> Date: Mon, 6 Jan 2025 12:37:56 +0100 Subject: [PATCH 08/23] Add block number to mempool_executed logs (#3200) # Description Add block number to mempool_executed logs, so we can simulate failed submissions in order to debug the reason of the failure. --------- Co-authored-by: ilya --- crates/driver/src/domain/mempools.rs | 24 ++++++++++++++++-------- crates/driver/src/infra/notify/mod.rs | 4 ++-- crates/driver/src/infra/observe/mod.rs | 2 +- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/crates/driver/src/domain/mempools.rs b/crates/driver/src/domain/mempools.rs index 3d4ed86135..f0cedd26af 100644 --- a/crates/driver/src/domain/mempools.rs +++ b/crates/driver/src/domain/mempools.rs @@ -105,7 +105,7 @@ impl Mempools { // settlement. This way we only run iterations in blocks that can potentially // include the settlement. let mut block_stream = into_stream(self.ethereum.current_block().clone()); - block_stream.next().await; + let block = block_stream.next().await; // The tx is simulated before submitting the solution to the competition, but a // delay between that and the actual execution can cause the simulation to be @@ -116,7 +116,7 @@ impl Mempools { ?err, "settlement tx simulation reverted before submitting to the mempool" ); - return Err(Error::SimulationRevert); + return Err(Error::SimulationRevert(block.map(|block| block.number))); } else { tracing::warn!( ?err, @@ -142,7 +142,12 @@ impl Mempools { }); match receipt { TxStatus::Executed => return Ok(hash.clone()), - TxStatus::Reverted => return Err(Error::Revert(hash.clone())), + TxStatus::Reverted => { + return Err(Error::Revert { + tx_id: hash.clone(), + block_number: block.number, + }) + } TxStatus::Pending => { // Check if the current block reached the submission deadline block number if block.number >= submission_deadline { @@ -172,7 +177,7 @@ impl Mempools { ?err, "tx started failing in mempool, cancelling" ); - return Err(Error::SimulationRevert); + return Err(Error::SimulationRevert(Some(block.number))); } else { tracing::warn!(?hash, ?err, "couldn't re-simulate tx"); } @@ -235,10 +240,13 @@ pub enum RevertProtection { #[derive(Debug, thiserror::Error)] pub enum Error { - #[error("Mined reverted transaction: {0:?}")] - Revert(eth::TxId), - #[error("Simulation started reverting during submission")] - SimulationRevert, + #[error("Mined reverted transaction: {tx_id:?}, block number: {block_number}")] + Revert { + tx_id: eth::TxId, + block_number: BlockNo, + }, + #[error("Simulation started reverting during submission, block number: {0:?}")] + SimulationRevert(Option), #[error("Settlement did not get included in time")] Expired, #[error("Strategy disabled for this tx")] diff --git a/crates/driver/src/infra/notify/mod.rs b/crates/driver/src/infra/notify/mod.rs index 4f33f3e5e6..edb4c55458 100644 --- a/crates/driver/src/infra/notify/mod.rs +++ b/crates/driver/src/infra/notify/mod.rs @@ -105,8 +105,8 @@ pub fn executed( ) { let kind = match res { Ok(hash) => notification::Settlement::Success(hash.clone()), - Err(Error::Revert(hash)) => notification::Settlement::Revert(hash.clone()), - Err(Error::SimulationRevert) => notification::Settlement::SimulationRevert, + Err(Error::Revert { tx_id: hash, .. }) => notification::Settlement::Revert(hash.clone()), + Err(Error::SimulationRevert { .. }) => notification::Settlement::SimulationRevert, Err(Error::Expired) => notification::Settlement::Expired, Err(Error::Other(_) | Error::Disabled) => notification::Settlement::Fail, }; diff --git a/crates/driver/src/infra/observe/mod.rs b/crates/driver/src/infra/observe/mod.rs index f8c1394e54..0dff50d918 100644 --- a/crates/driver/src/infra/observe/mod.rs +++ b/crates/driver/src/infra/observe/mod.rs @@ -339,7 +339,7 @@ pub fn mempool_executed( } let result = match res { Ok(_) => "Success", - Err(mempools::Error::Revert(_) | mempools::Error::SimulationRevert) => "Revert", + Err(mempools::Error::Revert { .. } | mempools::Error::SimulationRevert { .. }) => "Revert", Err(mempools::Error::Expired) => "Expired", Err(mempools::Error::Other(_)) => "Other", Err(mempools::Error::Disabled) => "Disabled", From 9463faa2f71d64ecca957176264d095539894496 Mon Sep 17 00:00:00 2001 From: ilya Date: Mon, 6 Jan 2025 13:15:39 +0000 Subject: [PATCH 09/23] [TRIVIAL] Run forked-node tests on main (#3210) After https://github.com/cowprotocol/services/pull/3201, the forked-node tests are no longer triggered on the `main` branch since the filter is configured for pull requests only. This PR fixes this. --- .github/workflows/pull-request.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index 372f1670bf..c3613f34b7 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -121,7 +121,9 @@ jobs: test-forked-node: # Do not run this job on forks since some secrets are required for it. - if: ${{ github.event.pull_request.head.repo.full_name == github.repository }} + if: | + (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'push' && github.ref == 'refs/heads/main') timeout-minutes: 60 runs-on: ubuntu-latest env: From c784e142befd6f3c94b60bb5f793ab84d611b926 Mon Sep 17 00:00:00 2001 From: ilya Date: Mon, 6 Jan 2025 18:29:21 +0000 Subject: [PATCH 10/23] Update `ethcontract-rs` (#3211) Updates `ethcontract-rs` to the latest version where a small bug was fixed. --- Cargo.lock | 20 ++++++++++---------- Cargo.toml | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d91055a2bd..9ce09b15ee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1966,9 +1966,9 @@ dependencies = [ [[package]] name = "ethcontract" -version = "0.25.7" +version = "0.25.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "de7f5ab3529a815a6f637e6ed6e62335cdeb535dce0dabd805941bfbfbd22e3f" +checksum = "6c83ea43ad75c3c78468966184a489c195822185df81d5fcaba4b1ba87f382c5" dependencies = [ "arrayvec", "aws-config 0.55.3", @@ -1993,9 +1993,9 @@ dependencies = [ [[package]] name = "ethcontract-common" -version = "0.25.7" +version = "0.25.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "acb731ef73bc8837d0002887263f43641786e34853f438779444293813f24992" +checksum = "51c3253a19d093d249024012ed2f4117d819958f79339fd24d8158fb05306747" dependencies = [ "ethabi", "hex", @@ -2009,9 +2009,9 @@ dependencies = [ [[package]] name = "ethcontract-derive" -version = "0.25.7" +version = "0.25.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "21e8f1e55f0f1c341859821a045d33867323fe8b778944bbf60cff883574cca1" +checksum = "d0f2a75ebcd8fe443014a9f1157fab5bb8950cff7c3a62079b748201ca32570c" dependencies = [ "anyhow", "ethcontract-common", @@ -2023,9 +2023,9 @@ dependencies = [ [[package]] name = "ethcontract-generate" -version = "0.25.7" +version = "0.25.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fc0983a5bc8284b4e7ac33a2818d63315cd19b3ca59ce90fea5a6837c1d65081" +checksum = "b0c0c474f16da0667eec38c2fc4c1ee7ad7cdfd879ffabfd57713444d851c6c7" dependencies = [ "Inflector", "anyhow", @@ -2039,9 +2039,9 @@ dependencies = [ [[package]] name = "ethcontract-mock" -version = "0.25.7" +version = "0.25.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6d56f0a4e3aba1a4f4aa06182c3cbe329c0bfecdd3280662abeabbf7620f9569" +checksum = "ff5cf1c4d38e002747e5ecc69fe50cd8d00d4080f1de32acb4cb6373093b8a1e" dependencies = [ "ethcontract", "hex", diff --git a/Cargo.toml b/Cargo.toml index fbbd160162..ae3831d7e7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,10 +13,10 @@ clap = { version = "4.5.6", features = ["derive", "env"] } dashmap = "6.1.0" derivative = "2.2.0" derive_more = { version = "1.0.0", features = ["full"] } -ethcontract = { version = "0.25.7", default-features = false, features = ["aws-kms"] } +ethcontract = { version = "0.25.8", default-features = false, features = ["aws-kms"] } mimalloc = "0.1.43" -ethcontract-generate = { version = "0.25.7", default-features = false } -ethcontract-mock = { version = "0.25.7", default-features = false } +ethcontract-generate = { version = "0.25.8", default-features = false } +ethcontract-mock = { version = "0.25.8", default-features = false } ethereum-types = "0.14.1" flate2 = "1.0.30" futures = "0.3.30" From 2f54635690cab3be34bedbd80a7bac1cc4219a32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Strug?= <47604705+mstrug@users.noreply.github.com> Date: Wed, 8 Jan 2025 11:26:34 +0100 Subject: [PATCH 11/23] Added `auction_id` field to `/settle` and `/reveal` driver requests. (#3212) # Description This is a continuation of the previous PR #3131 which added `auction_id` field as optional on the driver `/settle` and `/reveal` requests. Because this is API breaking change we had to wait for Solvers teams to updated their driver version, so they will support new `auction_id` field. Current PR changes `auction_id` field from optional to mandatory and starts sending it from the autopilot. # Changes This is mainly port of PR #3113 which introduced API break (and was reverted with PR #3131) on to the current main branch: updated driver and autopilot DTOs, added code to use these new fields and added additional condition on driver side to verify if provided `solution_id` is competing in provided `auction_id`. OpenAPI definition is already up to date. ## How to test Existing tests, esp. `driver_handles_solutions_based_on_id()`. --- .../autopilot/src/infra/solvers/dto/reveal.rs | 4 ++-- .../autopilot/src/infra/solvers/dto/settle.rs | 4 ++-- crates/autopilot/src/run_loop.rs | 2 +- crates/autopilot/src/shadow.rs | 2 +- crates/driver/src/domain/competition/mod.rs | 22 ++++++------------- .../api/routes/reveal/dto/reveal_request.rs | 4 ++-- .../driver/src/infra/api/routes/reveal/mod.rs | 15 ++++++++----- .../api/routes/settle/dto/settle_request.rs | 4 ++-- .../driver/src/infra/api/routes/settle/mod.rs | 9 +++----- 9 files changed, 30 insertions(+), 36 deletions(-) diff --git a/crates/autopilot/src/infra/solvers/dto/reveal.rs b/crates/autopilot/src/infra/solvers/dto/reveal.rs index 41971d28b1..6bda360d77 100644 --- a/crates/autopilot/src/infra/solvers/dto/reveal.rs +++ b/crates/autopilot/src/infra/solvers/dto/reveal.rs @@ -11,8 +11,8 @@ pub struct Request { /// Unique ID of the solution (per driver competition), to reveal. pub solution_id: u64, /// Auction ID in which the specified solution ID is competing. - #[serde_as(as = "Option")] - pub auction_id: Option, + #[serde_as(as = "serde_with::DisplayFromStr")] + pub auction_id: i64, } #[serde_as] diff --git a/crates/autopilot/src/infra/solvers/dto/settle.rs b/crates/autopilot/src/infra/solvers/dto/settle.rs index 5277eef76e..385ad68212 100644 --- a/crates/autopilot/src/infra/solvers/dto/settle.rs +++ b/crates/autopilot/src/infra/solvers/dto/settle.rs @@ -13,6 +13,6 @@ pub struct Request { /// The last block number in which the solution TX can be included pub submission_deadline_latest_block: u64, /// Auction ID in which the specified solution ID is competing. - #[serde_as(as = "Option")] - pub auction_id: Option, + #[serde_as(as = "serde_with::DisplayFromStr")] + pub auction_id: i64, } diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 53b70b4100..3b55738364 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -765,7 +765,7 @@ impl RunLoop { let request = settle::Request { solution_id, submission_deadline_latest_block, - auction_id: None, // Requires 2-stage release for API-break change + auction_id, }; driver .settle(&request, self.config.max_settlement_transaction_wait) diff --git a/crates/autopilot/src/shadow.rs b/crates/autopilot/src/shadow.rs index beddd86acc..44db5bd203 100644 --- a/crates/autopilot/src/shadow.rs +++ b/crates/autopilot/src/shadow.rs @@ -276,7 +276,7 @@ impl RunLoop { let revealed = driver .reveal(&reveal::Request { solution_id, - auction_id: None, // Requires 2-stage release for API-break change + auction_id: request.id, }) .await .map_err(Error::Reveal)?; diff --git a/crates/driver/src/domain/competition/mod.rs b/crates/driver/src/domain/competition/mod.rs index 2ecde0fdc7..e8c8d964ed 100644 --- a/crates/driver/src/domain/competition/mod.rs +++ b/crates/driver/src/domain/competition/mod.rs @@ -318,17 +318,14 @@ impl Competition { pub async fn reveal( &self, solution_id: u64, - auction_id: Option, + auction_id: auction::Id, ) -> Result { let settlement = self .settlements .lock() .unwrap() .iter() - .find(|s| { - s.solution().get() == solution_id - && auction_id.is_none_or(|id| s.auction_id.0 == id) - }) + .find(|s| s.solution().get() == solution_id && s.auction_id == auction_id) .cloned() .ok_or(Error::SolutionNotAvailable)?; Ok(Revealed { @@ -347,7 +344,7 @@ impl Competition { /// [`Competition::solve`] to generate the solution. pub async fn settle( &self, - auction_id: Option, + auction_id: auction::Id, solution_id: u64, submission_deadline: BlockNo, ) -> Result { @@ -413,16 +410,14 @@ impl Competition { tracing::error!(?err, "Failed to send /settle response"); } } - .instrument( - tracing::info_span!("/settle", solver, auction_id = ?auction_id.map(|id| id.0)), - ) + .instrument(tracing::info_span!("/settle", solver, %auction_id)) .await } } async fn process_settle_request( &self, - auction_id: Option, + auction_id: auction::Id, solution_id: u64, submission_deadline: BlockNo, ) -> Result { @@ -430,10 +425,7 @@ impl Competition { let mut lock = self.settlements.lock().unwrap(); let index = lock .iter() - .position(|s| { - s.solution().get() == solution_id - && auction_id.is_none_or(|id| s.auction_id == id) - }) + .position(|s| s.solution().get() == solution_id && s.auction_id == auction_id) .ok_or(Error::SolutionNotAvailable)?; // remove settlement to ensure we can't settle it twice by accident lock.swap_remove_front(index) @@ -537,7 +529,7 @@ fn merge(solutions: impl Iterator, auction: &Auction) -> Vec, + auction_id: auction::Id, solution_id: u64, submission_deadline: BlockNo, response_sender: oneshot::Sender>, diff --git a/crates/driver/src/infra/api/routes/reveal/dto/reveal_request.rs b/crates/driver/src/infra/api/routes/reveal/dto/reveal_request.rs index c782ed57fc..e0e762dd47 100644 --- a/crates/driver/src/infra/api/routes/reveal/dto/reveal_request.rs +++ b/crates/driver/src/infra/api/routes/reveal/dto/reveal_request.rs @@ -7,6 +7,6 @@ pub struct RevealRequest { /// Unique ID of the solution (per driver competition), to reveal. pub solution_id: u64, /// Auction ID in which the specified solution ID is competing. - #[serde_as(as = "Option")] - pub auction_id: Option, + #[serde_as(as = "serde_with::DisplayFromStr")] + pub auction_id: i64, } diff --git a/crates/driver/src/infra/api/routes/reveal/mod.rs b/crates/driver/src/infra/api/routes/reveal/mod.rs index b96ce290f8..7a2d6081e5 100644 --- a/crates/driver/src/infra/api/routes/reveal/mod.rs +++ b/crates/driver/src/infra/api/routes/reveal/mod.rs @@ -1,9 +1,12 @@ mod dto; use { - crate::infra::{ - api::{Error, State}, - observe, + crate::{ + domain::competition::auction, + infra::{ + api::{self, Error, State}, + observe, + }, }, tracing::Instrument, }; @@ -16,11 +19,13 @@ async fn route( state: axum::extract::State, req: axum::Json, ) -> Result, (hyper::StatusCode, axum::Json)> { + let auction_id = + auction::Id::try_from(req.auction_id).map_err(api::routes::AuctionError::from)?; let handle_request = async { observe::revealing(); let result = state .competition() - .reveal(req.solution_id, req.auction_id) + .reveal(req.solution_id, auction_id) .await; observe::revealed(state.solver().name(), &result); let result = result?; @@ -28,6 +33,6 @@ async fn route( }; handle_request - .instrument(tracing::info_span!("/reveal", solver = %state.solver().name(), req.auction_id)) + .instrument(tracing::info_span!("/reveal", solver = %state.solver().name(), %auction_id)) .await } diff --git a/crates/driver/src/infra/api/routes/settle/dto/settle_request.rs b/crates/driver/src/infra/api/routes/settle/dto/settle_request.rs index 844a716738..6f5735fbce 100644 --- a/crates/driver/src/infra/api/routes/settle/dto/settle_request.rs +++ b/crates/driver/src/infra/api/routes/settle/dto/settle_request.rs @@ -9,6 +9,6 @@ pub struct SettleRequest { /// The last block number in which the solution TX can be included pub submission_deadline_latest_block: u64, /// Auction ID in which this solution is competing. - #[serde_as(as = "Option")] - pub auction_id: Option, + #[serde_as(as = "serde_with::DisplayFromStr")] + pub auction_id: i64, } diff --git a/crates/driver/src/infra/api/routes/settle/mod.rs b/crates/driver/src/infra/api/routes/settle/mod.rs index 516e3efb70..1334693a6e 100644 --- a/crates/driver/src/infra/api/routes/settle/mod.rs +++ b/crates/driver/src/infra/api/routes/settle/mod.rs @@ -19,11 +19,8 @@ async fn route( state: axum::extract::State, req: axum::Json, ) -> Result<(), (hyper::StatusCode, axum::Json)> { - let auction_id = req - .auction_id - .map(auction::Id::try_from) - .transpose() - .map_err(Into::::into)?; + let auction_id = + auction::Id::try_from(req.auction_id).map_err(api::routes::AuctionError::from)?; let solver = state.solver().name().to_string(); let handle_request = async move { @@ -39,7 +36,7 @@ async fn route( observe::settled(state.solver().name(), &result); result.map(|_| ()).map_err(Into::into) } - .instrument(tracing::info_span!("/settle", solver, auction_id = ?auction_id.map(|id| id.0))); + .instrument(tracing::info_span!("/settle", solver, %auction_id)); // Handle `/settle` call in a background task to ensure that we correctly // submit the settlement (or cancellation) on-chain even if the server From 672b30fadc703560fc3dab624f485eb4455dd3a4 Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Wed, 8 Jan 2025 12:34:14 +0100 Subject: [PATCH 12/23] execute `eth_estimateGas` and `eth_createAccessList` on latest block (#3220) # Description For a while we had mysteriously failing transactions. What I mean is that we execute `eth_estimateGas` to see if a transaction would still work. If it fails we log all the data needed to re-simulate it on tenderly. But when you then actually re-simulate on tenderly the transaction would work. The solution unravels with this [issue](https://github.com/tomusdrw/rust-web3/issues/290). The spec for `eth_estimateGas` mentions 2 arguments: the tx and an identifier for a block. The block is supposed to be optional but geth had a bug for a very long time. When you don't pass a block argument it should be sent over the wire as `null`. But geth tried to deserialize `null` which then caused these calls to fail. As a workaround the `web3` crate stopped serializing the block tag if it was `None`. Eventually geth fixed this issue and implemented it such that a missing block tag would default to `pending`. Also a rule of thumb in node development is "if in doubt do whatever geth does" so reth also implemented the default to `pending`. Defaulting to `pending` is what can make these reverts unpredictable. When you simulate the tx on `pending` the node takes the state of the `latest` block PLUS transactions that it currently has in its mempool. So whenever we log a reverting tx that works on tenderly it only failed in our node because some tx in the nodes mempool interfered with the solution. Interestingly there also a lot of cases where you can even simulate the tx on the next block. In those cases simulating on `pending` caused the driver completely erroneously since the problematic tx didn't even make it into the next block.
example [log](https://aws-es.cow.fi/_dashboards/app/discover#/doc/86e4a5a0-4e4b-11ef-85c5-3946a99ed1a7/cowlogs-prod-2025.01.06?id=1F1ROpQBOTpjfSxxfB9z) ``` 2025-01-06T06:33:25.748Z INFO request{id="4114843"}:/solve{solver=extzeroex-solve auction_id=9990122}: driver::infra::observe: discarded solution: settlement encoding id=Id { id: 17739308, merged_solutions: [22] } err=Simulation(Revert(RevertError { err: Blockchain(AccessList(String("execution reverted"))), tx: Tx { from: Address(0x28b1bd44996105b5c14c4de41093226ff78a4eb1), to: Address(0x9008d19f58aabd9ed0d60971565aa8510560ab41), value: Ether(0), input: 0x13d79a0b0000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000012000000000000000000000000000000000000000000000000000000000000001c000000000000000000000000000000000000000000000000000000000000003e00000000000000000000000000000000000000000000000000000000000000004000000000000000000000000839d4a05d6d23a22365bd90c2114c199d53e674c000000000000000000000000eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee000000000000000000000000839d4a05d6d23a22365bd90c2114c199d53e674c000000000000000000000000eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee0000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000017c836d081141b7000000000000000000000000000000000000000000000034cafbfac2c90f727a000000000000000000000000000000000000000000000000017c836d081141b700000000000000000000000000000000000000000000003635c9adc5dea0000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000030000000000000000000000006a7fe93d1e194d31b8076c255779c4b12a45192c00000000000000000000000000000000000000000000003635c9adc5dea00000000000000000000000000000000000000000000000000000010df3f85654f86700000000000000000000000000000000000000000000000000000000677b7e7d5b203ebd4ff6f6667daa775bd3de4410b0b834f059fccecd85660cb72bf031e30000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003635c9adc5dea00000000000000000000000000000000000000000000000000000000000000000016000000000000000000000000000000000000000000000000000000000000000410fd3b1f82fc78a190ccf38bbfa08652ca518ef025802fe671c0b7e1b8cc1c8e762470c92b313dd597fde1022168575929770076f30cc6581377d174f76ce675e1c0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000000000000000046000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000001400000000000000000000000000000000000000000000000000000000000000300000000000000000000000000839d4a05d6d23a22365bd90c2114c199d53e674c000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000044095ea7b3000000000000000000000000def1c0ded9bec7f1a1670819833240f027b25eff00000000000000000000000000000000000000000000003635c9adc5dea0000000000000000000000000000000000000000000000000000000000000000000000000000000000000def1c0ded9bec7f1a1670819833240f027b25eff000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000128d9627aa4000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000003635c9adc5dea000000000000000000000000000000000000000000000000000000186565d4040733b00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000839d4a05d6d23a22365bd90c2114c199d53e674c000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2869584cd0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab410000000000000000000000000000000000000000b507dba868e8950578d34d3f000000000000000000000000000000000000000000000000000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000242e1a7d4d000000000000000000000000000000000000000000000000017c836d081141b70000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000986fea, access_list: AccessList({}) }, block: BlockNo(21563660) })) ``` Results in this [simulation](https://dashboard.tenderly.co/cow-protocol/staging/simulator/0b89b4d0-418f-40e3-8502-59c85e4da885) that even works 2 blocks past the block we logged.
# Changes Always estimate gas and create access lists on the `latest` block instead of `pending`. This should be the better default for 2 reasons: 1. it makes our simulation reverts consistent with what we see on tenderly 2. most blocks get built by sophisticated algorithms including and arranging blocks in a way that maximizes MEV. That means whatever transactions ordering the node applies in the `pending` likely isn't accurate anyway. --- crates/driver/src/infra/blockchain/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/driver/src/infra/blockchain/mod.rs b/crates/driver/src/infra/blockchain/mod.rs index a43a037bfe..6b85a00b08 100644 --- a/crates/driver/src/infra/blockchain/mod.rs +++ b/crates/driver/src/infra/blockchain/mod.rs @@ -182,7 +182,10 @@ impl Ethereum { .transport() .execute( "eth_createAccessList", - vec![serde_json::to_value(&tx).unwrap()], + vec![ + serde_json::to_value(&tx).unwrap(), + serde_json::Value::String("latest".into()), + ], ) .await?; if let Some(err) = json.get("error") { @@ -207,7 +210,7 @@ impl Ethereum { gas_price: self.simulation_gas_price().await, ..Default::default() }, - None, + Some(ethcontract::BlockNumber::Latest), ) .await .map(Into::into) From d13627dc3538e1d596bd8d667a010ca516ae5b34 Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Thu, 9 Jan 2025 16:11:55 +0100 Subject: [PATCH 13/23] Revert executing `eth_estimateGas` and `eth_createAccessList` on the latest block (#3227) While this PR would give us revert detection reproducible with tenderly it also leads to an overly optimistic revert protection. Meaning on average future reverts get detected too late which would result in solvers losing a lot of money. This was noticed by an external solver running the original PR in production and seeing their rewards vanish due to increased reverts. See this [thread](https://cowservices.slack.com/archives/C0690QX4R5X/p1736368423899369?thread_ts=1736279159.869429&cid=C0690QX4R5X) for reference. --- crates/driver/src/infra/blockchain/mod.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/driver/src/infra/blockchain/mod.rs b/crates/driver/src/infra/blockchain/mod.rs index 6b85a00b08..a43a037bfe 100644 --- a/crates/driver/src/infra/blockchain/mod.rs +++ b/crates/driver/src/infra/blockchain/mod.rs @@ -182,10 +182,7 @@ impl Ethereum { .transport() .execute( "eth_createAccessList", - vec![ - serde_json::to_value(&tx).unwrap(), - serde_json::Value::String("latest".into()), - ], + vec![serde_json::to_value(&tx).unwrap()], ) .await?; if let Some(err) = json.get("error") { @@ -210,7 +207,7 @@ impl Ethereum { gas_price: self.simulation_gas_price().await, ..Default::default() }, - Some(ethcontract::BlockNumber::Latest), + None, ) .await .map(Into::into) From 162ebc47f79b3ec2f91a35e62a63fbcad42cfa94 Mon Sep 17 00:00:00 2001 From: ilya Date: Thu, 9 Jan 2025 16:55:34 +0000 Subject: [PATCH 14/23] Support rust 1.84.0 (#3229) Fixes compilation on the latest stable rust version. --- crates/autopilot/src/database/onchain_order_events/mod.rs | 4 ++-- .../src/sources/balancer_v2/swap/fixed_point/logexpmath.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/autopilot/src/database/onchain_order_events/mod.rs b/crates/autopilot/src/database/onchain_order_events/mod.rs index 9ae6051493..bcbabd2ebf 100644 --- a/crates/autopilot/src/database/onchain_order_events/mod.rs +++ b/crates/autopilot/src/database/onchain_order_events/mod.rs @@ -435,8 +435,8 @@ type GeneralOnchainOrderPlacementData = ( OnchainOrderPlacement, Order, ); -async fn parse_general_onchain_order_placement_data<'a>( - quoter: &'a dyn OrderQuoting, +async fn parse_general_onchain_order_placement_data( + quoter: &'_ dyn OrderQuoting, order_placement_events_and_quotes_zipped: Vec<(EthContractEvent, i64, i64)>, domain_separator: DomainSeparator, settlement_contract: H160, diff --git a/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs b/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs index 302053b7f6..15375bca70 100644 --- a/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs +++ b/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs @@ -223,8 +223,8 @@ mod tests { constant_x: fn(u32) -> I256, constant_a: fn(u32) -> I256, ) { + let re = Regex::new(r".* ([ax])([\d]+) = (\d+);.*$").unwrap(); for line in code.split('\n').filter(|line| !line.trim().is_empty()) { - let re = Regex::new(r".* ([ax])([\d]+) = (\d+);.*$").unwrap(); let cap = re.captures_iter(line).next().unwrap(); match &cap[1] { "x" => assert_eq!(&cap[3], format!("{}", constant_x(cap[2].parse().unwrap()))), From b46fc728dde8fc648108f117c3e13c84481f4dd5 Mon Sep 17 00:00:00 2001 From: ilya Date: Thu, 9 Jan 2025 17:18:10 +0000 Subject: [PATCH 15/23] Bad token detector metrics (#3228) # Description Adds Prometheus metrics for driver's bad token detector strategies, so it will be possible to show how many tokens were marked as `bad` by each solver and strategy. It would be helpful to identify solvers that perform as not expected and to analyze the work of each detection strategy individually. ## How to test Create 2 grafana panels: the one that shows detected tokens by solver and another - by strategy. Added `hotfix` label to deploy it asap. --- .../domain/competition/bad_tokens/metrics.rs | 20 +++++++++++++++++-- .../competition/bad_tokens/simulation.rs | 4 +++- crates/driver/src/infra/api/mod.rs | 1 + crates/driver/src/infra/observe/metrics.rs | 3 +++ crates/driver/src/infra/observe/mod.rs | 2 +- 5 files changed, 26 insertions(+), 4 deletions(-) diff --git a/crates/driver/src/domain/competition/bad_tokens/metrics.rs b/crates/driver/src/domain/competition/bad_tokens/metrics.rs index c6951fbea6..daaad1ee7c 100644 --- a/crates/driver/src/domain/competition/bad_tokens/metrics.rs +++ b/crates/driver/src/domain/competition/bad_tokens/metrics.rs @@ -1,6 +1,9 @@ use { super::Quality, - crate::domain::eth, + crate::{ + domain::eth, + infra::{observe::metrics, solver}, + }, dashmap::DashMap, std::{ sync::Arc, @@ -27,6 +30,7 @@ pub struct Detector { counter: Arc>, log_only: bool, token_freeze_time: Duration, + solver: solver::Name, } impl Detector { @@ -35,6 +39,7 @@ impl Detector { required_measurements: u32, log_only: bool, token_freeze_time: Duration, + solver: solver::Name, ) -> Self { Self { failure_ratio, @@ -42,6 +47,7 @@ impl Detector { counter: Default::default(), log_only, token_freeze_time, + solver, } } @@ -120,6 +126,10 @@ impl Detector { tokens = ?new_unsupported_tokens, "mark tokens as unsupported" ); + metrics::get() + .bad_tokens_detected + .with_label_values(&[&self.solver.0, "metrics"]) + .inc_by(new_unsupported_tokens.len() as u64); } } } @@ -133,7 +143,13 @@ mod tests { #[tokio::test] async fn unfreeze_bad_tokens() { const FREEZE_DURATION: Duration = Duration::from_millis(50); - let detector = Detector::new(0.5, 2, false, FREEZE_DURATION); + let detector = Detector::new( + 0.5, + 2, + false, + FREEZE_DURATION, + solver::Name("mysolver".to_string()), + ); let token_a = eth::TokenAddress(eth::ContractAddress(H160([1; 20]))); let token_b = eth::TokenAddress(eth::ContractAddress(H160([2; 20]))); diff --git a/crates/driver/src/domain/competition/bad_tokens/simulation.rs b/crates/driver/src/domain/competition/bad_tokens/simulation.rs index dd8eb5e0f4..9b593b8726 100644 --- a/crates/driver/src/domain/competition/bad_tokens/simulation.rs +++ b/crates/driver/src/domain/competition/bad_tokens/simulation.rs @@ -8,7 +8,7 @@ use { }, eth, }, - infra, + infra::{self, observe::metrics}, }, futures::FutureExt, model::interaction::InteractionData, @@ -100,6 +100,8 @@ impl Detector { } Ok(TokenQuality::Bad { reason }) => { tracing::debug!(reason, token=?sell_token.0, "cache token as unsupported"); + // All solvers share the same cache for the simulation detector, so there is no need to specify the solver name here. + metrics::get().bad_tokens_detected.with_label_values(&["any", "simulation"]).inc(); inner .cache .update_quality(sell_token, false, now); diff --git a/crates/driver/src/infra/api/mod.rs b/crates/driver/src/infra/api/mod.rs index de7897cfa8..ccbdc5a178 100644 --- a/crates/driver/src/infra/api/mod.rs +++ b/crates/driver/src/infra/api/mod.rs @@ -84,6 +84,7 @@ impl Api { bad_token_config.metrics_strategy_required_measurements, bad_token_config.metrics_strategy_log_only, bad_token_config.metrics_strategy_token_freeze_time, + name.clone(), )); } diff --git a/crates/driver/src/infra/observe/metrics.rs b/crates/driver/src/infra/observe/metrics.rs index 330e250867..237206c668 100644 --- a/crates/driver/src/infra/observe/metrics.rs +++ b/crates/driver/src/infra/observe/metrics.rs @@ -19,6 +19,9 @@ pub struct Metrics { /// The results of the mempool submission. #[metric(labels("mempool", "result"))] pub mempool_submission: prometheus::IntCounterVec, + /// How many tokens detected by specific solver and strategy. + #[metric(labels("solver", "strategy"))] + pub bad_tokens_detected: prometheus::IntCounterVec, } /// Setup the metrics registry. diff --git a/crates/driver/src/infra/observe/mod.rs b/crates/driver/src/infra/observe/mod.rs index 0dff50d918..9714f80264 100644 --- a/crates/driver/src/infra/observe/mod.rs +++ b/crates/driver/src/infra/observe/mod.rs @@ -28,7 +28,7 @@ use { url::Url, }; -mod metrics; +pub mod metrics; /// Setup the observability. The log argument configures the tokio tracing /// framework. From d1972cde1eff3a7fd8fc07a2e4c95a91b4543481 Mon Sep 17 00:00:00 2001 From: ilya Date: Thu, 9 Jan 2025 17:55:52 +0000 Subject: [PATCH 16/23] [TRIVIAL] Hotfix GH job typo fix (#3230) There was a typo in the GH token name. --- .github/workflows/hotfix.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/hotfix.yml b/.github/workflows/hotfix.yml index 5a71a19b07..bd65cd97b7 100644 --- a/.github/workflows/hotfix.yml +++ b/.github/workflows/hotfix.yml @@ -17,7 +17,7 @@ jobs: - name: Check out uses: actions/checkout@v4 with: - token: "${{ secrets.HOTFIX_ACTION_JOB }}" + token: "${{ secrets.HOTFIX_ACTION_TOKEN }}" fetch-depth: 0 - name: Configure git From d9a7621a37a2377f76884613d787cad81aaa0744 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Fri, 10 Jan 2025 10:59:28 +0100 Subject: [PATCH 17/23] Remove full_fee_amount and solver_fee (#3223) # Description As part of fee breakdown refactor, the time has come to remove `full_fee_amount` and `solver_fee` from order API. These fields were relevant more than 2 years ago when orders with signed fee > 0 existed and when subsidies for that fee were calculated. Will wait for confirmation from frontend that these fields are no longer used anywhere, before merging. # Changes - [ ] Removed fields from the API and all dependent code paths. ## How to test Existing unit tests, e2e tests, postgresql tests. --- .../src/database/onchain_order_events/mod.rs | 3 --- crates/database/src/jit_orders.rs | 2 +- crates/database/src/orders.rs | 13 ++------- crates/model/src/order.rs | 27 ------------------- crates/orderbook/openapi.yml | 4 --- crates/orderbook/src/database/orders.rs | 7 ----- crates/shared/src/db_order_conversions.rs | 13 --------- crates/shared/src/order_validation.rs | 1 - crates/shared/src/remaining_amounts.rs | 7 ----- .../solver/src/liquidity/order_converter.rs | 1 - database/README.md | 1 - database/sql/V078__drop_full_fee_amount.sql | 2 ++ 12 files changed, 5 insertions(+), 76 deletions(-) create mode 100644 database/sql/V078__drop_full_fee_amount.sql diff --git a/crates/autopilot/src/database/onchain_order_events/mod.rs b/crates/autopilot/src/database/onchain_order_events/mod.rs index bcbabd2ebf..d96f740983 100644 --- a/crates/autopilot/src/database/onchain_order_events/mod.rs +++ b/crates/autopilot/src/database/onchain_order_events/mod.rs @@ -616,7 +616,6 @@ fn convert_onchain_order_placement( settlement_contract: ByteArray(settlement_contract.0), sell_token_balance: sell_token_source_into(order_data.sell_token_balance), buy_token_balance: buy_token_destination_into(order_data.buy_token_balance), - full_fee_amount: u256_to_big_decimal(&order_data.fee_amount), cancellation_timestamp: None, class: match order_data.fee_amount.is_zero() { true => OrderClass::Limit, @@ -925,7 +924,6 @@ mod test { settlement_contract: ByteArray(settlement_contract.0), sell_token_balance: sell_token_source_into(expected_order_data.sell_token_balance), buy_token_balance: buy_token_destination_into(expected_order_data.buy_token_balance), - full_fee_amount: u256_to_big_decimal(&expected_order_data.fee_amount), cancellation_timestamp: None, }; assert_eq!(onchain_order_placement, expected_onchain_order_placement); @@ -1036,7 +1034,6 @@ mod test { settlement_contract: ByteArray(settlement_contract.0), sell_token_balance: sell_token_source_into(expected_order_data.sell_token_balance), buy_token_balance: buy_token_destination_into(expected_order_data.buy_token_balance), - full_fee_amount: u256_to_big_decimal(&U256::zero()), cancellation_timestamp: None, }; assert_eq!(onchain_order_placement, expected_onchain_order_placement); diff --git a/crates/database/src/jit_orders.rs b/crates/database/src/jit_orders.rs index b17453d6a1..ef77a5ed04 100644 --- a/crates/database/src/jit_orders.rs +++ b/crates/database/src/jit_orders.rs @@ -18,7 +18,7 @@ use { pub const SELECT: &str = r#" o.uid, o.owner, o.creation_timestamp, o.sell_token, o.buy_token, o.sell_amount, o.buy_amount, -o.valid_to, o.app_data, o.fee_amount, o.fee_amount AS full_fee_amount, o.kind, o.partially_fillable, o.signature, +o.valid_to, o.app_data, o.fee_amount, o.kind, o.partially_fillable, o.signature, o.receiver, o.signing_scheme, '\x9008d19f58aabd9ed0d60971565aa8510560ab41'::bytea AS settlement_contract, o.sell_token_balance, o.buy_token_balance, 'liquidity'::OrderClass AS class, (SELECT COALESCE(SUM(t.buy_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_buy, diff --git a/crates/database/src/orders.rs b/crates/database/src/orders.rs index a0c4d11da9..ba3b663e80 100644 --- a/crates/database/src/orders.rs +++ b/crates/database/src/orders.rs @@ -96,7 +96,6 @@ pub struct Order { pub settlement_contract: Address, pub sell_token_balance: SellTokenSource, pub buy_token_balance: BuyTokenDestination, - pub full_fee_amount: BigDecimal, pub cancellation_timestamp: Option>, pub class: OrderClass, } @@ -140,11 +139,10 @@ INSERT INTO orders ( settlement_contract, sell_token_balance, buy_token_balance, - full_fee_amount, cancellation_timestamp, class ) -VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20) "#; pub async fn insert_order_and_ignore_conflicts( @@ -183,7 +181,6 @@ async fn insert_order_execute_sqlx( .bind(order.settlement_contract) .bind(order.sell_token_balance) .bind(order.buy_token_balance) - .bind(&order.full_fee_amount) .bind(order.cancellation_timestamp) .bind(order.class) .execute(ex) @@ -473,7 +470,6 @@ pub struct FullOrder { pub valid_to: i64, pub app_data: AppId, pub fee_amount: BigDecimal, - pub full_fee_amount: BigDecimal, pub kind: OrderKind, pub class: OrderClass, pub partially_fillable: bool, @@ -547,7 +543,7 @@ impl FullOrder { // that with the current amount of data this wouldn't be better. pub const SELECT: &str = r#" o.uid, o.owner, o.creation_timestamp, o.sell_token, o.buy_token, o.sell_amount, o.buy_amount, -o.valid_to, o.app_data, o.fee_amount, o.full_fee_amount, o.kind, o.partially_fillable, o.signature, +o.valid_to, o.app_data, o.fee_amount, o.kind, o.partially_fillable, o.signature, o.receiver, o.signing_scheme, o.settlement_contract, o.sell_token_balance, o.buy_token_balance, o.class, (SELECT COALESCE(SUM(t.buy_amount), 0) FROM trades t WHERE t.order_uid = o.uid) AS sum_buy, @@ -880,7 +876,6 @@ mod tests { assert_eq!(order.valid_to, full_order.valid_to); assert_eq!(order.app_data, full_order.app_data); assert_eq!(order.fee_amount, full_order.fee_amount); - assert_eq!(order.full_fee_amount, full_order.full_fee_amount); assert_eq!(order.kind, full_order.kind); assert_eq!(order.class, full_order.class); assert_eq!(order.partially_fillable, full_order.partially_fillable); @@ -1364,10 +1359,6 @@ mod tests { order_with_quote.full_order.fee_amount, full_order.fee_amount ); - assert_eq!( - order_with_quote.full_order.full_fee_amount, - full_order.full_fee_amount - ); assert_eq!(order_with_quote.full_order.kind, full_order.kind); assert_eq!(order_with_quote.full_order.class, full_order.class); assert_eq!( diff --git a/crates/model/src/order.rs b/crates/model/src/order.rs index 6aa3e4b78b..d06712d518 100644 --- a/crates/model/src/order.rs +++ b/crates/model/src/order.rs @@ -212,8 +212,6 @@ pub struct OrderData { /// as they should only ever be used to improve the price of a regular order /// and should not be settled on their own. /// This is 0 for limit orders as their fee gets taken from the surplus. - /// This is equal to `OrderMetadata::full_fee_amount` except for old orders - /// where the subsidy was applied (at the time when we used the subsidies). #[serde_as(as = "HexOrDecimalU256")] pub fee_amount: U256, pub kind: OrderKind, @@ -685,27 +683,6 @@ pub struct OrderMetadata { #[serde(flatten)] pub class: OrderClass, pub settlement_contract: H160, - /// This is `fee_amount` for liquidity orders. See comment on `fee_amount` - /// for the reasoning. - /// For market/limit orders it's the gas used of the best trade - /// execution we could find while quoting converted to an equivalent - /// `sell_token` amount. - /// Does not take partial fill into account. - /// - /// [TO BE DEPRECATED] - #[serde_as(as = "HexOrDecimalU256")] - pub full_fee_amount: U256, - /// The fee amount that should be used for objective value computations. - /// - /// This is different than the actual signed fee in that it - /// does not have any subsidies applied and may be scaled by a constant - /// factor to make matching orders more valuable from an objective value - /// perspective. - /// Does not take partial fill into account. - /// - /// [TO BE DEPRECATED] - #[serde_as(as = "HexOrDecimalU256")] - pub solver_fee: U256, #[serde(default, skip_serializing_if = "Option::is_none")] pub ethflow_data: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -1028,8 +1005,6 @@ mod tests { "executedSurplusFee": "1", "executedFee": "1", "executedFeeToken": "0x000000000000000000000000000000000000000a", - "fullFeeAmount": "115792089237316195423570985008687907853269984665640564039457584007913129639935", - "solverFee": "115792089237316195423570985008687907853269984665640564039457584007913129639935", "kind": "buy", "class": "limit", "partiallyFillable": false, @@ -1064,8 +1039,6 @@ mod tests { invalidated: true, status: OrderStatus::Open, settlement_contract: H160::from_low_u64_be(2), - full_fee_amount: U256::MAX, - solver_fee: U256::MAX, full_app_data: Some("123".to_string()), ..Default::default() }, diff --git a/crates/orderbook/openapi.yml b/crates/orderbook/openapi.yml index aa4c2da3ac..aea622a4a1 100644 --- a/crates/orderbook/openapi.yml +++ b/crates/orderbook/openapi.yml @@ -912,10 +912,6 @@ components: description: Order status. allOf: - $ref: "#/components/schemas/OrderStatus" - fullFeeAmount: - description: Amount that the signed fee would be without subsidies. - allOf: - - $ref: "#/components/schemas/TokenAmount" isLiquidityOrder: description: |- Liquidity orders are functionally the same as normal smart contract diff --git a/crates/orderbook/src/database/orders.rs b/crates/orderbook/src/database/orders.rs index 51bce8a8c6..cc12d71d93 100644 --- a/crates/orderbook/src/database/orders.rs +++ b/crates/orderbook/src/database/orders.rs @@ -207,7 +207,6 @@ async fn insert_order(order: &Order, ex: &mut PgConnection) -> Result<(), Insert settlement_contract: ByteArray(order.metadata.settlement_contract.0), sell_token_balance: sell_token_source_into(order.data.sell_token_balance), buy_token_balance: buy_token_destination_into(order.data.buy_token_balance), - full_fee_amount: u256_to_big_decimal(&order.metadata.full_fee_amount), cancellation_timestamp: None, }; @@ -632,11 +631,6 @@ fn full_order_into_model_order(order: FullOrder) -> Result { is_liquidity_order: class == OrderClass::Liquidity, class, settlement_contract: H160(order.settlement_contract.0), - full_fee_amount: big_decimal_to_u256(&order.full_fee_amount) - .context("full_fee_amount is not U256")?, - // Initialize unscaled and scale later when required. - solver_fee: big_decimal_to_u256(&order.full_fee_amount) - .context("solver_fee is not U256")?, ethflow_data, onchain_user, onchain_order_data, @@ -730,7 +724,6 @@ mod tests { valid_to: valid_to_timestamp.timestamp(), app_data: ByteArray([0; 32]), fee_amount: BigDecimal::default(), - full_fee_amount: BigDecimal::default(), kind: DbOrderKind::Sell, class: DbOrderClass::Liquidity, partially_fillable: true, diff --git a/crates/shared/src/db_order_conversions.rs b/crates/shared/src/db_order_conversions.rs index 44e2ae3320..c6e94610ff 100644 --- a/crates/shared/src/db_order_conversions.rs +++ b/crates/shared/src/db_order_conversions.rs @@ -59,18 +59,7 @@ pub fn full_order_into_model_order(order: database::orders::FullOrder) -> Result sender: onchain_user, placement_error: onchain_placement_error, }); - let full_fee_amount = - big_decimal_to_u256(&order.full_fee_amount).context("full_fee_amount is not U256")?; let fee_amount = big_decimal_to_u256(&order.fee_amount).context("fee_amount is not U256")?; - let solver_fee = match &class { - // Liquidity orders should never have a fee unless the owner bribes the protocol by setting - // one themselves. - OrderClass::Liquidity => fee_amount, - // We can't use `surplus_fee` or `fee_amount` here because those values include subsidies. - // All else being equal a solver would then prefer including an unsubsidized order over a - // subsidized one which we don't want. - OrderClass::Limit | OrderClass::Market => full_fee_amount, - }; let metadata = OrderMetadata { creation_date: order.creation_timestamp, @@ -100,8 +89,6 @@ pub fn full_order_into_model_order(order: database::orders::FullOrder) -> Result is_liquidity_order: class == OrderClass::Liquidity, class, settlement_contract: H160(order.settlement_contract.0), - full_fee_amount, - solver_fee, ethflow_data, onchain_user, onchain_order_data, diff --git a/crates/shared/src/order_validation.rs b/crates/shared/src/order_validation.rs index 05b2368238..e53211bb3f 100644 --- a/crates/shared/src/order_validation.rs +++ b/crates/shared/src/order_validation.rs @@ -713,7 +713,6 @@ impl OrderValidating for OrderValidator { creation_date: chrono::offset::Utc::now(), uid, settlement_contract, - full_fee_amount: data.fee_amount, class, full_app_data: match order.app_data { OrderCreationAppData::Both { full, .. } diff --git a/crates/shared/src/remaining_amounts.rs b/crates/shared/src/remaining_amounts.rs index a9b2a32a7f..d7da34706b 100644 --- a/crates/shared/src/remaining_amounts.rs +++ b/crates/shared/src/remaining_amounts.rs @@ -196,10 +196,6 @@ mod tests { partially_fillable: false, ..Default::default() }, - metadata: OrderMetadata { - full_fee_amount: 42.into(), - ..Default::default() - }, ..Default::default() } .into(); @@ -220,7 +216,6 @@ mod tests { }, metadata: OrderMetadata { executed_sell_amount_before_fees: 0.into(), - full_fee_amount: 13.into(), ..Default::default() }, ..Default::default() @@ -243,7 +238,6 @@ mod tests { }, metadata: OrderMetadata { executed_sell_amount_before_fees: 90.into(), - full_fee_amount: 200.into(), ..Default::default() }, ..Default::default() @@ -265,7 +259,6 @@ mod tests { }, metadata: OrderMetadata { executed_buy_amount: 9_u32.into(), - full_fee_amount: 200.into(), ..Default::default() }, ..Default::default() diff --git a/crates/solver/src/liquidity/order_converter.rs b/crates/solver/src/liquidity/order_converter.rs index 5425d557be..66894bbf1d 100644 --- a/crates/solver/src/liquidity/order_converter.rs +++ b/crates/solver/src/liquidity/order_converter.rs @@ -334,7 +334,6 @@ pub mod tests { }, metadata: OrderMetadata { executed_sell_amount_before_fees: 10.into(), - solver_fee: 60.into(), ..Default::default() }, ..Default::default() diff --git a/database/README.md b/database/README.md index 8b8980b95c..f3f6eb2d80 100644 --- a/database/README.md +++ b/database/README.md @@ -270,7 +270,6 @@ Column | Type | Nullable | Details settlement\_contract | bytea | not null | address of the contract that should be used to settle this order sell\_token\_balance | [enum](#selltokensource) | not null | defines how sell\_tokens need to be transferred into the settlement contract buy\_token\_balance | [enum](#buytokendestination) | not null | defined how buy\_tokens need to be transferred back to the user - full\_fee\_amount | numeric | not null | estimated execution cost in sell\_token of this order class | [enum](#orderclass) | not null | determines which special trade semantics will apply to the execution of this order diff --git a/database/sql/V078__drop_full_fee_amount.sql b/database/sql/V078__drop_full_fee_amount.sql new file mode 100644 index 0000000000..dff60d3f43 --- /dev/null +++ b/database/sql/V078__drop_full_fee_amount.sql @@ -0,0 +1,2 @@ +ALTER TABLE orders +DROP COLUMN full_fee_amount; From 49eb712fb0265b36389af4dffc6cd4a167a53799 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Strug?= <47604705+mstrug@users.noreply.github.com> Date: Mon, 13 Jan 2025 12:36:23 +0100 Subject: [PATCH 18/23] [TRIVIAL] Maintaining quote verified flag when creating an order (#3234) # Description Changed order `verified` flag to be used from quote original value when order is crated from that quote. The need for this change is that ~98% of quotes are verified successfully but most of the created `order_quotes` from that orders are not verified. ## How to test Updated e2e test to verify this particular case (it fails before applying this PR). --- crates/e2e/tests/e2e/place_order_with_quote.rs | 8 ++++---- crates/shared/src/order_quoting.rs | 4 +--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/e2e/tests/e2e/place_order_with_quote.rs b/crates/e2e/tests/e2e/place_order_with_quote.rs index 09d3bb55d3..6902fced4e 100644 --- a/crates/e2e/tests/e2e/place_order_with_quote.rs +++ b/crates/e2e/tests/e2e/place_order_with_quote.rs @@ -67,6 +67,7 @@ async fn place_order_with_quote(web3: Web3) { let quote_response = services.submit_quote("e_request).await.unwrap(); tracing::debug!(?quote_response); assert!(quote_response.id.is_some()); + assert!(quote_response.verified); let quote_metadata = crate::database::quote_metadata(services.db(), quote_response.id.unwrap()).await; @@ -99,9 +100,8 @@ async fn place_order_with_quote(web3: Web3) { &database::byte_array::ByteArray(order_uid.0), ) .await + .unwrap() .unwrap(); - assert!(order_quote.is_some()); - // compare quote metadata and order quote metadata - let order_quote_metadata = order_quote.unwrap().metadata; - assert_eq!(quote_metadata.unwrap().0, order_quote_metadata); + assert_eq!(quote_response.verified, order_quote.verified); + assert_eq!(quote_metadata.unwrap().0, order_quote.metadata); } diff --git a/crates/shared/src/order_quoting.rs b/crates/shared/src/order_quoting.rs index 60a78a141b..ccad1b6cc3 100644 --- a/crates/shared/src/order_quoting.rs +++ b/crates/shared/src/order_quoting.rs @@ -195,9 +195,7 @@ impl TryFrom for QuoteData { expiration: row.expiration_timestamp, quote_kind: row.quote_kind, solver: H160(row.solver.0), - // Even if the quote was verified at the time of creation - // it might no longer be accurate. - verified: false, + verified: row.verified, metadata: row.metadata.try_into()?, }) } From 01f35f8cbc8fc45bd4b09a1907312d36ef220560 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Tue, 14 Jan 2025 13:53:58 +0100 Subject: [PATCH 19/23] [TRIVIAL] Adjust message for breaking API changes (#3235) # Description https://github.com/cowprotocol/services/pull/3223 caused breaking the SAFE client even though they did not actually use the field. In the future, all API changes (without exception) need to be communicated with 1. Frontend team 2. Safe team # Changes - [ ] Adjusted the message --- .github/nitpicks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/nitpicks.yml b/.github/nitpicks.yml index 38c77b3a5b..b97c0f4c54 100644 --- a/.github/nitpicks.yml +++ b/.github/nitpicks.yml @@ -7,7 +7,7 @@ Reminder: Please consider backward compatibility when modifying the API specification. If breaking changes are unavoidable, ensure: - You explicitly pointed out breaking changes. - - You communicate the changes to affected teams. + - You communicate the changes to affected teams (at least Frontend team and SAFE team). - You provide proper versioning and migration mechanisms. pathFilter: - "**/openapi.yml" From 2725d5c099a3f9c0fff5bec9705f3ebbc6420ad3 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Wed, 15 Jan 2025 13:26:06 +0100 Subject: [PATCH 20/23] Use traces to extract the input for `settlement::Observer` (#3233) # Description Implements first part of https://github.com/cowprotocol/services/issues/3177 # Changes - [ ] Uses `trace_transaction` function to get traces and extract the calldata of `settle` call ## How to test Existing tests show that no regression bugs were introduced. Final e2e test should cover this part of the code. Although by manually testing the `trace_transaction` I am pretty sure the code is correct. --- crates/autopilot/src/domain/eth/mod.rs | 11 +- crates/autopilot/src/domain/settlement/mod.rs | 287 +++++++++++++++++- .../src/domain/settlement/observer.rs | 15 +- .../src/domain/settlement/transaction/mod.rs | 48 ++- crates/autopilot/src/infra/blockchain/mod.rs | 20 +- 5 files changed, 347 insertions(+), 34 deletions(-) diff --git a/crates/autopilot/src/domain/eth/mod.rs b/crates/autopilot/src/domain/eth/mod.rs index 9b187526ba..29ca478683 100644 --- a/crates/autopilot/src/domain/eth/mod.rs +++ b/crates/autopilot/src/domain/eth/mod.rs @@ -312,6 +312,13 @@ pub struct TradeEvent { pub order_uid: domain::OrderUid, } +/// A trace of a Call type of action. +#[derive(Debug, Clone, Default)] +pub struct TraceCall { + pub to: Address, + pub input: Calldata, +} + /// Any type of on-chain transaction. #[derive(Debug, Clone, Default)] pub struct Transaction { @@ -319,8 +326,6 @@ pub struct Transaction { pub hash: TxId, /// The address of the sender of the transaction. pub from: Address, - /// The call data of the transaction. - pub input: Calldata, /// The block number of the block that contains the transaction. pub block: BlockNo, /// The timestamp of the block that contains the transaction. @@ -329,4 +334,6 @@ pub struct Transaction { pub gas: Gas, /// The effective gas price of the transaction. pub gas_price: EffectiveGasPrice, + /// Traces of all Calls contained in the transaction. + pub trace_calls: Vec, } diff --git a/crates/autopilot/src/domain/settlement/mod.rs b/crates/autopilot/src/domain/settlement/mod.rs index 7908789719..dbcb4e14c4 100644 --- a/crates/autopilot/src/domain/settlement/mod.rs +++ b/crates/autopilot/src/domain/settlement/mod.rs @@ -29,8 +29,6 @@ pub struct Settlement { gas: eth::Gas, /// The effective gas price of the settlement transaction. gas_price: eth::EffectiveGasPrice, - /// The address of the solver that submitted the settlement transaction. - solver: eth::Address, /// The block number of the block that contains the settlement transaction. block: eth::BlockNo, /// The associated auction. @@ -146,7 +144,6 @@ impl Settlement { .collect(); Ok(Self { - solver: settled.solver, block: settled.block, gas: settled.gas, gas_price: settled.gas_price, @@ -216,14 +213,246 @@ impl From for Error { #[cfg(test)] mod tests { use { - crate::{ - domain, - domain::{auction, eth}, - }, + crate::domain::{self, auction, eth}, hex_literal::hex, std::collections::{HashMap, HashSet}, }; + // https://etherscan.io/tx/0x030623e438f28446329d8f4ff84db897907fcac59b9943b31b7be66f23c877af + // A transfer transaction that emits a settlement event, but it's not actually a + // swap. + #[test] + fn not_a_swap() { + let calldata = hex!( + " + 13d79a0b + 0000000000000000000000000000000000000000000000000000000000000080 + 00000000000000000000000000000000000000000000000000000000000000a0 + 00000000000000000000000000000000000000000000000000000000000000c0 + 00000000000000000000000000000000000000000000000000000000000000e0 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 0000000000000000000000000000000000000000000000000000000000000080 + 00000000000000000000000000000000000000000000000000000000000017a0 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000011 + 0000000000000000000000000000000000000000000000000000000000000220 + 0000000000000000000000000000000000000000000000000000000000000360 + 00000000000000000000000000000000000000000000000000000000000004a0 + 00000000000000000000000000000000000000000000000000000000000005e0 + 0000000000000000000000000000000000000000000000000000000000000720 + 0000000000000000000000000000000000000000000000000000000000000860 + 00000000000000000000000000000000000000000000000000000000000009a0 + 0000000000000000000000000000000000000000000000000000000000000ae0 + 0000000000000000000000000000000000000000000000000000000000000c20 + 0000000000000000000000000000000000000000000000000000000000000d60 + 0000000000000000000000000000000000000000000000000000000000000ea0 + 0000000000000000000000000000000000000000000000000000000000000fe0 + 0000000000000000000000000000000000000000000000000000000000001120 + 0000000000000000000000000000000000000000000000000000000000001260 + 00000000000000000000000000000000000000000000000000000000000013a0 + 00000000000000000000000000000000000000000000000000000000000014e0 + 0000000000000000000000000000000000000000000000000000000000001620 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000388674df3c7f96ac76c6fa06813b758322b5b64ce14bf46f0f9b4ec6f2 + d015ff9a9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000383f19e12409d5913e40bfde35a1607a1a43f1f9d26e76dd2d9d409cc7 + 50125f769008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 00000038a91a488e41e46dedbe71ae0c0d9f41be8de9f45ffc62271970362777 + 9e302e8d9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 0000003820621f141681025ffcbce03b51c3ea78c93da8739df9202210647968 + 6d2efad59008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000385846bbbcee39039678e529973cf6962b90d49663d5ff49220adfb240 + 4805d11e9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000388a608a3e6fd6f95797d376ffa35ae077d78440b627dce2b3d2278e04 + 8d37f7c09008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 00000038436b1ae2723b42982a435ede8f92033fbdf1505e5dac97c1099b5ffd + fc1debda9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 00000038c31724565adeb2d0e4d6a6980f45b9fd91bb0b858fa8c8ada300e697 + 45d1ab379008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 0000003812789b110b2bf7353d054d978dd30972f2f33e54c4c7b93a1f992d20 + fd2f29619008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 00000038f527dd3ad4938713bff3fb23a62d7b454caf40bc80f3eeed77c3d269 + d9ff8eb69008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000388a63a1ed3d671e94aafe29b9ad479340ee04ad1fb5796c73cb71b25a + e8e2b0a49008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000384acca1a9694919daee72c0f20b64a2c0103750c26ce9b25cc864609f + 2f4977269008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 00000038bdbeba9388a200bfa8c3f153c31a539622e2994a2cc59bfdc8c562a3 + 2f20b9919008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000383649cc08adbf44018756fec310e9348d64139f33530166ef53f1834f + 2f5c8c469008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 0000003864bee671badbb222a43006744c89b70a368ca34a356dcf41f755923e + aab5b4029008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 00000000000000000000000000000000000000000000000000000000000000a4 + ec6cb13f00000000000000000000000000000000000000000000000000000000 + 0000004000000000000000000000000000000000000000000000000000000000 + 0000000100000000000000000000000000000000000000000000000000000000 + 000000382285ea60762c9a58d407e5aa3b8c3287628290793bc7260b0e0324ed + 6f8bb1269008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 + 0000000000000000000000000000000000000000000000000000000000000000 + 000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2 + 0000000000000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000060 + 0000000000000000000000000000000000000000000000000000000000000044 + a9059cbb000000000000000000000000a03be496e67ec29bc62f01a428683d7f + 9c2049300000000000000000000000000000000000000000000000002136c5d9 + c1570aef00000000000000000000000000000000000000000000000000000000 + 0000000000000000000000000000000000000000000000000000000000000000" + ) + .to_vec(); + + let domain_separator = eth::DomainSeparator(hex!( + "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" + )); + let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( + "9008d19f58aabd9ed0d60971565aa8510560ab41" + ))); + let transaction = super::transaction::Transaction::try_new( + &domain::eth::Transaction { + trace_calls: vec![domain::eth::TraceCall { + to: settlement_contract, + input: calldata.into(), + }], + ..Default::default() + }, + &domain_separator, + settlement_contract, + ) + .unwrap_err(); + + // These transfer transactions don't have the auction_id attached so overall bad + // calldata is expected + assert!(matches!( + transaction, + super::transaction::Error::Decoding(_) + )); + } + // https://etherscan.io/tx/0xc48dc0d43ffb43891d8c3ad7bcf05f11465518a2610869b20b0b4ccb61497634 #[test] fn settlement() { @@ -306,12 +535,19 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let transaction = super::transaction::Transaction::new( + let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( + "9008d19f58aabd9ed0d60971565aa8510560ab41" + ))); + let transaction = super::transaction::Transaction::try_new( &domain::eth::Transaction { - input: calldata.into(), + trace_calls: vec![domain::eth::TraceCall { + to: settlement_contract, + input: calldata.into(), + }], ..Default::default() }, &domain_separator, + settlement_contract, ) .unwrap(); @@ -442,12 +678,19 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let transaction = super::transaction::Transaction::new( + let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( + "9008d19f58aabd9ed0d60971565aa8510560ab41" + ))); + let transaction = super::transaction::Transaction::try_new( &domain::eth::Transaction { - input: calldata.into(), + trace_calls: vec![domain::eth::TraceCall { + to: settlement_contract, + input: calldata.into(), + }], ..Default::default() }, &domain_separator, + settlement_contract, ) .unwrap(); @@ -610,12 +853,19 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let transaction = super::transaction::Transaction::new( + let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( + "9008d19f58aabd9ed0d60971565aa8510560ab41" + ))); + let transaction = super::transaction::Transaction::try_new( &domain::eth::Transaction { - input: calldata.into(), + trace_calls: vec![domain::eth::TraceCall { + to: settlement_contract, + input: calldata.into(), + }], ..Default::default() }, &domain_separator, + settlement_contract, ) .unwrap(); @@ -784,12 +1034,19 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let transaction = super::transaction::Transaction::new( + let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( + "9008d19f58aabd9ed0d60971565aa8510560ab41" + ))); + let transaction = super::transaction::Transaction::try_new( &domain::eth::Transaction { - input: calldata.into(), + trace_calls: vec![domain::eth::TraceCall { + to: settlement_contract, + input: calldata.into(), + }], ..Default::default() }, &domain_separator, + settlement_contract, ) .unwrap(); diff --git a/crates/autopilot/src/domain/settlement/observer.rs b/crates/autopilot/src/domain/settlement/observer.rs index 631539f9ef..d142646728 100644 --- a/crates/autopilot/src/domain/settlement/observer.rs +++ b/crates/autopilot/src/domain/settlement/observer.rs @@ -66,7 +66,8 @@ impl Observer { let transaction = match self.eth.transaction(event.transaction).await { Ok(transaction) => { let separator = self.eth.contracts().settlement_domain_separator(); - settlement::Transaction::new(&transaction, separator) + let settlement_contract = self.eth.contracts().settlement().address().into(); + settlement::Transaction::try_new(&transaction, separator, settlement_contract) } Err(err) => { tracing::warn!(hash = ?event.transaction, ?err, "no tx found"); @@ -95,7 +96,17 @@ impl Observer { (auction_id, settlement) } Err(err) => { - tracing::warn!(hash = ?event.transaction, ?err, "invalid settlement transaction"); + match err { + settlement::transaction::Error::MissingCalldata => { + tracing::error!(hash = ?event.transaction, ?err, "invalid settlement transaction") + } + settlement::transaction::Error::MissingAuctionId + | settlement::transaction::Error::Decoding(_) + | settlement::transaction::Error::SignatureRecover(_) + | settlement::transaction::Error::OrderUidRecover(_) => { + tracing::warn!(hash = ?event.transaction, ?err, "invalid settlement transaction") + } + } // default values so we don't get stuck on invalid settlement transactions (0.into(), None) } diff --git a/crates/autopilot/src/domain/settlement/transaction/mod.rs b/crates/autopilot/src/domain/settlement/transaction/mod.rs index 3b0e3f4540..d5877d00db 100644 --- a/crates/autopilot/src/domain/settlement/transaction/mod.rs +++ b/crates/autopilot/src/domain/settlement/transaction/mod.rs @@ -1,6 +1,10 @@ -use crate::{ - boundary, - domain::{self, auction::order, eth}, +use { + crate::{ + boundary, + domain::{self, auction::order, eth}, + }, + ethcontract::common::FunctionExt, + std::sync::LazyLock, }; mod tokenized; @@ -12,8 +16,6 @@ pub struct Transaction { pub hash: eth::TxId, /// The associated auction id. pub auction_id: domain::auction::Id, - /// The address of the solver that submitted the transaction. - pub solver: eth::Address, /// The block number of the block that contains the transaction. pub block: eth::BlockNo, /// The timestamp of the block that contains the transaction. @@ -27,18 +29,33 @@ pub struct Transaction { } impl Transaction { - pub fn new( + pub fn try_new( transaction: ð::Transaction, domain_separator: ð::DomainSeparator, + settlement_contract: eth::Address, ) -> Result { + // find trace call to settlement contract + let calldata = transaction + .trace_calls + .iter() + .find(|trace| is_settlement_trace(trace, settlement_contract)) + .map(|trace| trace.input.clone()) + // all transactions emitting settlement events should have a /settle call, + // otherwise it's an execution client bug + .ok_or(Error::MissingCalldata)?; + /// Number of bytes that may be appended to the calldata to store an /// auction id. const META_DATA_LEN: usize = 8; - let (data, metadata) = transaction - .input - .0 - .split_at(transaction.input.0.len() - META_DATA_LEN); + let (data, metadata) = calldata.0.split_at( + calldata + .0 + .len() + .checked_sub(META_DATA_LEN) + // should contain at least 4 bytes for function selector and 8 bytes for auction id + .ok_or(Error::MissingCalldata)?, + ); let metadata: Option<[u8; META_DATA_LEN]> = metadata.try_into().ok(); let auction_id = metadata .map(crate::domain::auction::Id::from_be_bytes) @@ -46,7 +63,6 @@ impl Transaction { Ok(Self { hash: transaction.hash, auction_id, - solver: transaction.from, block: transaction.block, timestamp: transaction.timestamp, gas: transaction.gas, @@ -116,6 +132,14 @@ impl Transaction { } } +fn is_settlement_trace(trace: ð::TraceCall, settlement_contract: eth::Address) -> bool { + static SETTLE_FUNCTION_SELECTOR: LazyLock<[u8; 4]> = LazyLock::new(|| { + let abi = &contracts::GPv2Settlement::raw_contract().interface.abi; + abi.function("settle").unwrap().selector() + }); + trace.to == settlement_contract && trace.input.0.starts_with(&*SETTLE_FUNCTION_SELECTOR) +} + /// Trade containing onchain observable data specific to a settlement /// transaction. #[derive(Debug, Clone)] @@ -152,6 +176,8 @@ pub struct ClearingPrices { #[derive(Debug, thiserror::Error)] pub enum Error { + #[error("settle calldata must exist for all transactions emitting settlement event")] + MissingCalldata, #[error("missing auction id")] MissingAuctionId, #[error(transparent)] diff --git a/crates/autopilot/src/infra/blockchain/mod.rs b/crates/autopilot/src/infra/blockchain/mod.rs index 6b8f3234cc..79f0ef1d75 100644 --- a/crates/autopilot/src/infra/blockchain/mod.rs +++ b/crates/autopilot/src/infra/blockchain/mod.rs @@ -103,9 +103,10 @@ impl Ethereum { } pub async fn transaction(&self, hash: eth::TxId) -> Result { - let (transaction, receipt) = tokio::try_join!( + let (transaction, receipt, traces) = tokio::try_join!( self.web3.eth().transaction(hash.0.into()), - self.web3.eth().transaction_receipt(hash.0) + self.web3.eth().transaction_receipt(hash.0), + self.web3.trace().transaction(hash.0), )?; let transaction = transaction.ok_or(Error::TransactionNotFound)?; let receipt = receipt.ok_or(Error::TransactionNotFound)?; @@ -121,13 +122,15 @@ impl Ethereum { .block(block_hash.into()) .await? .ok_or(Error::TransactionNotFound)?; - into_domain(transaction, receipt, block.timestamp).map_err(Error::IncompleteTransactionData) + into_domain(transaction, receipt, traces, block.timestamp) + .map_err(Error::IncompleteTransactionData) } } fn into_domain( transaction: web3::types::Transaction, receipt: web3::types::TransactionReceipt, + traces: Vec, timestamp: U256, ) -> anyhow::Result { Ok(eth::Transaction { @@ -136,7 +139,6 @@ fn into_domain( .from .ok_or(anyhow::anyhow!("missing from"))? .into(), - input: transaction.input.0.into(), block: receipt .block_number .ok_or(anyhow::anyhow!("missing block_number"))? @@ -151,6 +153,16 @@ fn into_domain( .ok_or(anyhow::anyhow!("missing effective_gas_price"))? .into(), timestamp: timestamp.as_u32(), + trace_calls: traces + .into_iter() + .filter_map(|trace| match trace.action { + web3::types::Action::Call(call) => Some(eth::TraceCall { + to: call.to.into(), + input: call.input.0.into(), + }), + _ => None, + }) + .collect(), }) } From b4e2bbdab467f0be1556769c0f49099d0cc0d77b Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Thu, 16 Jan 2025 09:08:44 +0100 Subject: [PATCH 21/23] =?UTF-8?q?Revert=20"Use=20traces=20to=20extract=20t?= =?UTF-8?q?he=20input=20for=20`settlement::Observer`=20(#=E2=80=A6=20(#323?= =?UTF-8?q?7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description Reverts https://github.com/cowprotocol/services/pull/3233 None of the Arbitrum nodes supports `trace_transaction`. Supposedly, `arbtrace_transaction` should be used instead, but I couldn't make it work. Also, not all backup nodes (read alchemy) for other networks (base, gc) support `trace_transaction`. I noticed that alchemy supports `debug_traceTransaction` that could be used instead. Overall, I should have checked this before merging the PR. I decided to revert this PR since, right now, Arbitrum staging is unable to work properly, and also, even with specific/per_chain implementation I am not sure I will be able to make it work with all PRIMARY/BACKUP nodes on ALL networks. This needs throughout testing first. --- crates/autopilot/src/domain/eth/mod.rs | 11 +- crates/autopilot/src/domain/settlement/mod.rs | 287 +----------------- .../src/domain/settlement/observer.rs | 15 +- .../src/domain/settlement/transaction/mod.rs | 48 +-- crates/autopilot/src/infra/blockchain/mod.rs | 20 +- 5 files changed, 34 insertions(+), 347 deletions(-) diff --git a/crates/autopilot/src/domain/eth/mod.rs b/crates/autopilot/src/domain/eth/mod.rs index 29ca478683..9b187526ba 100644 --- a/crates/autopilot/src/domain/eth/mod.rs +++ b/crates/autopilot/src/domain/eth/mod.rs @@ -312,13 +312,6 @@ pub struct TradeEvent { pub order_uid: domain::OrderUid, } -/// A trace of a Call type of action. -#[derive(Debug, Clone, Default)] -pub struct TraceCall { - pub to: Address, - pub input: Calldata, -} - /// Any type of on-chain transaction. #[derive(Debug, Clone, Default)] pub struct Transaction { @@ -326,6 +319,8 @@ pub struct Transaction { pub hash: TxId, /// The address of the sender of the transaction. pub from: Address, + /// The call data of the transaction. + pub input: Calldata, /// The block number of the block that contains the transaction. pub block: BlockNo, /// The timestamp of the block that contains the transaction. @@ -334,6 +329,4 @@ pub struct Transaction { pub gas: Gas, /// The effective gas price of the transaction. pub gas_price: EffectiveGasPrice, - /// Traces of all Calls contained in the transaction. - pub trace_calls: Vec, } diff --git a/crates/autopilot/src/domain/settlement/mod.rs b/crates/autopilot/src/domain/settlement/mod.rs index dbcb4e14c4..7908789719 100644 --- a/crates/autopilot/src/domain/settlement/mod.rs +++ b/crates/autopilot/src/domain/settlement/mod.rs @@ -29,6 +29,8 @@ pub struct Settlement { gas: eth::Gas, /// The effective gas price of the settlement transaction. gas_price: eth::EffectiveGasPrice, + /// The address of the solver that submitted the settlement transaction. + solver: eth::Address, /// The block number of the block that contains the settlement transaction. block: eth::BlockNo, /// The associated auction. @@ -144,6 +146,7 @@ impl Settlement { .collect(); Ok(Self { + solver: settled.solver, block: settled.block, gas: settled.gas, gas_price: settled.gas_price, @@ -213,246 +216,14 @@ impl From for Error { #[cfg(test)] mod tests { use { - crate::domain::{self, auction, eth}, + crate::{ + domain, + domain::{auction, eth}, + }, hex_literal::hex, std::collections::{HashMap, HashSet}, }; - // https://etherscan.io/tx/0x030623e438f28446329d8f4ff84db897907fcac59b9943b31b7be66f23c877af - // A transfer transaction that emits a settlement event, but it's not actually a - // swap. - #[test] - fn not_a_swap() { - let calldata = hex!( - " - 13d79a0b - 0000000000000000000000000000000000000000000000000000000000000080 - 00000000000000000000000000000000000000000000000000000000000000a0 - 00000000000000000000000000000000000000000000000000000000000000c0 - 00000000000000000000000000000000000000000000000000000000000000e0 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 0000000000000000000000000000000000000000000000000000000000000080 - 00000000000000000000000000000000000000000000000000000000000017a0 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000011 - 0000000000000000000000000000000000000000000000000000000000000220 - 0000000000000000000000000000000000000000000000000000000000000360 - 00000000000000000000000000000000000000000000000000000000000004a0 - 00000000000000000000000000000000000000000000000000000000000005e0 - 0000000000000000000000000000000000000000000000000000000000000720 - 0000000000000000000000000000000000000000000000000000000000000860 - 00000000000000000000000000000000000000000000000000000000000009a0 - 0000000000000000000000000000000000000000000000000000000000000ae0 - 0000000000000000000000000000000000000000000000000000000000000c20 - 0000000000000000000000000000000000000000000000000000000000000d60 - 0000000000000000000000000000000000000000000000000000000000000ea0 - 0000000000000000000000000000000000000000000000000000000000000fe0 - 0000000000000000000000000000000000000000000000000000000000001120 - 0000000000000000000000000000000000000000000000000000000000001260 - 00000000000000000000000000000000000000000000000000000000000013a0 - 00000000000000000000000000000000000000000000000000000000000014e0 - 0000000000000000000000000000000000000000000000000000000000001620 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000388674df3c7f96ac76c6fa06813b758322b5b64ce14bf46f0f9b4ec6f2 - d015ff9a9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000383f19e12409d5913e40bfde35a1607a1a43f1f9d26e76dd2d9d409cc7 - 50125f769008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 00000038a91a488e41e46dedbe71ae0c0d9f41be8de9f45ffc62271970362777 - 9e302e8d9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 0000003820621f141681025ffcbce03b51c3ea78c93da8739df9202210647968 - 6d2efad59008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000385846bbbcee39039678e529973cf6962b90d49663d5ff49220adfb240 - 4805d11e9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000388a608a3e6fd6f95797d376ffa35ae077d78440b627dce2b3d2278e04 - 8d37f7c09008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 00000038436b1ae2723b42982a435ede8f92033fbdf1505e5dac97c1099b5ffd - fc1debda9008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 00000038c31724565adeb2d0e4d6a6980f45b9fd91bb0b858fa8c8ada300e697 - 45d1ab379008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 0000003812789b110b2bf7353d054d978dd30972f2f33e54c4c7b93a1f992d20 - fd2f29619008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 00000038f527dd3ad4938713bff3fb23a62d7b454caf40bc80f3eeed77c3d269 - d9ff8eb69008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000388a63a1ed3d671e94aafe29b9ad479340ee04ad1fb5796c73cb71b25a - e8e2b0a49008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000384acca1a9694919daee72c0f20b64a2c0103750c26ce9b25cc864609f - 2f4977269008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 00000038bdbeba9388a200bfa8c3f153c31a539622e2994a2cc59bfdc8c562a3 - 2f20b9919008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000383649cc08adbf44018756fec310e9348d64139f33530166ef53f1834f - 2f5c8c469008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 0000003864bee671badbb222a43006744c89b70a368ca34a356dcf41f755923e - aab5b4029008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000009008d19f58aabd9ed0d60971565aa8510560ab41 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 00000000000000000000000000000000000000000000000000000000000000a4 - ec6cb13f00000000000000000000000000000000000000000000000000000000 - 0000004000000000000000000000000000000000000000000000000000000000 - 0000000100000000000000000000000000000000000000000000000000000000 - 000000382285ea60762c9a58d407e5aa3b8c3287628290793bc7260b0e0324ed - 6f8bb1269008d19f58aabd9ed0d60971565aa8510560ab41678716a000000000 - 0000000000000000000000000000000000000000000000000000000000000000 - 000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2 - 0000000000000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000060 - 0000000000000000000000000000000000000000000000000000000000000044 - a9059cbb000000000000000000000000a03be496e67ec29bc62f01a428683d7f - 9c2049300000000000000000000000000000000000000000000000002136c5d9 - c1570aef00000000000000000000000000000000000000000000000000000000 - 0000000000000000000000000000000000000000000000000000000000000000" - ) - .to_vec(); - - let domain_separator = eth::DomainSeparator(hex!( - "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" - )); - let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( - "9008d19f58aabd9ed0d60971565aa8510560ab41" - ))); - let transaction = super::transaction::Transaction::try_new( - &domain::eth::Transaction { - trace_calls: vec![domain::eth::TraceCall { - to: settlement_contract, - input: calldata.into(), - }], - ..Default::default() - }, - &domain_separator, - settlement_contract, - ) - .unwrap_err(); - - // These transfer transactions don't have the auction_id attached so overall bad - // calldata is expected - assert!(matches!( - transaction, - super::transaction::Error::Decoding(_) - )); - } - // https://etherscan.io/tx/0xc48dc0d43ffb43891d8c3ad7bcf05f11465518a2610869b20b0b4ccb61497634 #[test] fn settlement() { @@ -535,19 +306,12 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( - "9008d19f58aabd9ed0d60971565aa8510560ab41" - ))); - let transaction = super::transaction::Transaction::try_new( + let transaction = super::transaction::Transaction::new( &domain::eth::Transaction { - trace_calls: vec![domain::eth::TraceCall { - to: settlement_contract, - input: calldata.into(), - }], + input: calldata.into(), ..Default::default() }, &domain_separator, - settlement_contract, ) .unwrap(); @@ -678,19 +442,12 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( - "9008d19f58aabd9ed0d60971565aa8510560ab41" - ))); - let transaction = super::transaction::Transaction::try_new( + let transaction = super::transaction::Transaction::new( &domain::eth::Transaction { - trace_calls: vec![domain::eth::TraceCall { - to: settlement_contract, - input: calldata.into(), - }], + input: calldata.into(), ..Default::default() }, &domain_separator, - settlement_contract, ) .unwrap(); @@ -853,19 +610,12 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( - "9008d19f58aabd9ed0d60971565aa8510560ab41" - ))); - let transaction = super::transaction::Transaction::try_new( + let transaction = super::transaction::Transaction::new( &domain::eth::Transaction { - trace_calls: vec![domain::eth::TraceCall { - to: settlement_contract, - input: calldata.into(), - }], + input: calldata.into(), ..Default::default() }, &domain_separator, - settlement_contract, ) .unwrap(); @@ -1034,19 +784,12 @@ mod tests { let domain_separator = eth::DomainSeparator(hex!( "c078f884a2676e1345748b1feace7b0abee5d00ecadb6e574dcdd109a63e8943" )); - let settlement_contract = eth::Address(eth::H160::from_slice(&hex!( - "9008d19f58aabd9ed0d60971565aa8510560ab41" - ))); - let transaction = super::transaction::Transaction::try_new( + let transaction = super::transaction::Transaction::new( &domain::eth::Transaction { - trace_calls: vec![domain::eth::TraceCall { - to: settlement_contract, - input: calldata.into(), - }], + input: calldata.into(), ..Default::default() }, &domain_separator, - settlement_contract, ) .unwrap(); diff --git a/crates/autopilot/src/domain/settlement/observer.rs b/crates/autopilot/src/domain/settlement/observer.rs index d142646728..631539f9ef 100644 --- a/crates/autopilot/src/domain/settlement/observer.rs +++ b/crates/autopilot/src/domain/settlement/observer.rs @@ -66,8 +66,7 @@ impl Observer { let transaction = match self.eth.transaction(event.transaction).await { Ok(transaction) => { let separator = self.eth.contracts().settlement_domain_separator(); - let settlement_contract = self.eth.contracts().settlement().address().into(); - settlement::Transaction::try_new(&transaction, separator, settlement_contract) + settlement::Transaction::new(&transaction, separator) } Err(err) => { tracing::warn!(hash = ?event.transaction, ?err, "no tx found"); @@ -96,17 +95,7 @@ impl Observer { (auction_id, settlement) } Err(err) => { - match err { - settlement::transaction::Error::MissingCalldata => { - tracing::error!(hash = ?event.transaction, ?err, "invalid settlement transaction") - } - settlement::transaction::Error::MissingAuctionId - | settlement::transaction::Error::Decoding(_) - | settlement::transaction::Error::SignatureRecover(_) - | settlement::transaction::Error::OrderUidRecover(_) => { - tracing::warn!(hash = ?event.transaction, ?err, "invalid settlement transaction") - } - } + tracing::warn!(hash = ?event.transaction, ?err, "invalid settlement transaction"); // default values so we don't get stuck on invalid settlement transactions (0.into(), None) } diff --git a/crates/autopilot/src/domain/settlement/transaction/mod.rs b/crates/autopilot/src/domain/settlement/transaction/mod.rs index d5877d00db..3b0e3f4540 100644 --- a/crates/autopilot/src/domain/settlement/transaction/mod.rs +++ b/crates/autopilot/src/domain/settlement/transaction/mod.rs @@ -1,10 +1,6 @@ -use { - crate::{ - boundary, - domain::{self, auction::order, eth}, - }, - ethcontract::common::FunctionExt, - std::sync::LazyLock, +use crate::{ + boundary, + domain::{self, auction::order, eth}, }; mod tokenized; @@ -16,6 +12,8 @@ pub struct Transaction { pub hash: eth::TxId, /// The associated auction id. pub auction_id: domain::auction::Id, + /// The address of the solver that submitted the transaction. + pub solver: eth::Address, /// The block number of the block that contains the transaction. pub block: eth::BlockNo, /// The timestamp of the block that contains the transaction. @@ -29,33 +27,18 @@ pub struct Transaction { } impl Transaction { - pub fn try_new( + pub fn new( transaction: ð::Transaction, domain_separator: ð::DomainSeparator, - settlement_contract: eth::Address, ) -> Result { - // find trace call to settlement contract - let calldata = transaction - .trace_calls - .iter() - .find(|trace| is_settlement_trace(trace, settlement_contract)) - .map(|trace| trace.input.clone()) - // all transactions emitting settlement events should have a /settle call, - // otherwise it's an execution client bug - .ok_or(Error::MissingCalldata)?; - /// Number of bytes that may be appended to the calldata to store an /// auction id. const META_DATA_LEN: usize = 8; - let (data, metadata) = calldata.0.split_at( - calldata - .0 - .len() - .checked_sub(META_DATA_LEN) - // should contain at least 4 bytes for function selector and 8 bytes for auction id - .ok_or(Error::MissingCalldata)?, - ); + let (data, metadata) = transaction + .input + .0 + .split_at(transaction.input.0.len() - META_DATA_LEN); let metadata: Option<[u8; META_DATA_LEN]> = metadata.try_into().ok(); let auction_id = metadata .map(crate::domain::auction::Id::from_be_bytes) @@ -63,6 +46,7 @@ impl Transaction { Ok(Self { hash: transaction.hash, auction_id, + solver: transaction.from, block: transaction.block, timestamp: transaction.timestamp, gas: transaction.gas, @@ -132,14 +116,6 @@ impl Transaction { } } -fn is_settlement_trace(trace: ð::TraceCall, settlement_contract: eth::Address) -> bool { - static SETTLE_FUNCTION_SELECTOR: LazyLock<[u8; 4]> = LazyLock::new(|| { - let abi = &contracts::GPv2Settlement::raw_contract().interface.abi; - abi.function("settle").unwrap().selector() - }); - trace.to == settlement_contract && trace.input.0.starts_with(&*SETTLE_FUNCTION_SELECTOR) -} - /// Trade containing onchain observable data specific to a settlement /// transaction. #[derive(Debug, Clone)] @@ -176,8 +152,6 @@ pub struct ClearingPrices { #[derive(Debug, thiserror::Error)] pub enum Error { - #[error("settle calldata must exist for all transactions emitting settlement event")] - MissingCalldata, #[error("missing auction id")] MissingAuctionId, #[error(transparent)] diff --git a/crates/autopilot/src/infra/blockchain/mod.rs b/crates/autopilot/src/infra/blockchain/mod.rs index 79f0ef1d75..6b8f3234cc 100644 --- a/crates/autopilot/src/infra/blockchain/mod.rs +++ b/crates/autopilot/src/infra/blockchain/mod.rs @@ -103,10 +103,9 @@ impl Ethereum { } pub async fn transaction(&self, hash: eth::TxId) -> Result { - let (transaction, receipt, traces) = tokio::try_join!( + let (transaction, receipt) = tokio::try_join!( self.web3.eth().transaction(hash.0.into()), - self.web3.eth().transaction_receipt(hash.0), - self.web3.trace().transaction(hash.0), + self.web3.eth().transaction_receipt(hash.0) )?; let transaction = transaction.ok_or(Error::TransactionNotFound)?; let receipt = receipt.ok_or(Error::TransactionNotFound)?; @@ -122,15 +121,13 @@ impl Ethereum { .block(block_hash.into()) .await? .ok_or(Error::TransactionNotFound)?; - into_domain(transaction, receipt, traces, block.timestamp) - .map_err(Error::IncompleteTransactionData) + into_domain(transaction, receipt, block.timestamp).map_err(Error::IncompleteTransactionData) } } fn into_domain( transaction: web3::types::Transaction, receipt: web3::types::TransactionReceipt, - traces: Vec, timestamp: U256, ) -> anyhow::Result { Ok(eth::Transaction { @@ -139,6 +136,7 @@ fn into_domain( .from .ok_or(anyhow::anyhow!("missing from"))? .into(), + input: transaction.input.0.into(), block: receipt .block_number .ok_or(anyhow::anyhow!("missing block_number"))? @@ -153,16 +151,6 @@ fn into_domain( .ok_or(anyhow::anyhow!("missing effective_gas_price"))? .into(), timestamp: timestamp.as_u32(), - trace_calls: traces - .into_iter() - .filter_map(|trace| match trace.action { - web3::types::Action::Call(call) => Some(eth::TraceCall { - to: call.to.into(), - input: call.input.0.into(), - }), - _ => None, - }) - .collect(), }) } From 50f7c9a6e0c1e8b5fbac341e0b92291b78e8d10f Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Thu, 16 Jan 2025 09:22:33 +0100 Subject: [PATCH 22/23] Flashloan appdata (#3236) # Description Fixes: https://github.com/cowprotocol/services/issues/3214 # Changes Extend appdata format with `flashloan` field explained in the linked issue. ## How to test added an e2e test that shows it's now possible to create an order that contains flashloan hints in the appdata. --- crates/app-data/src/app_data.rs | 21 +++++- crates/e2e/tests/e2e/app_data.rs | 112 +++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 1 deletion(-) diff --git a/crates/app-data/src/app_data.rs b/crates/app-data/src/app_data.rs index c8cd70f5b5..7f12a00723 100644 --- a/crates/app-data/src/app_data.rs +++ b/crates/app-data/src/app_data.rs @@ -1,7 +1,7 @@ use { crate::{app_data_hash::hash_full_app_data, AppDataHash, Hooks}, anyhow::{anyhow, Context, Result}, - primitive_types::H160, + primitive_types::{H160, U256}, serde::{de, Deserialize, Deserializer, Serialize, Serializer}, std::{fmt, fmt::Display}, }; @@ -24,6 +24,24 @@ pub struct ProtocolAppData { pub signer: Option, pub replaced_order: Option, pub partner_fee: Option, + pub flashloan: Option, +} + +/// Contains information to hint at how a solver could make +/// use of flashloans to settle the associated order. +/// Since using flashloans introduces a bunch of complexities +/// all these hints are not binding for the solver. +#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq)] +pub struct Flashloan { + /// Which contract to request the flashloan from. + lender: Option, + /// Who should receive the borrowed tokens. If this is not + /// set the order owner will get the tokens. + borrower: Option, + /// Which token to flashloan. + token: H160, + /// How much of the token to flashloan. + amount: U256, } #[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq)] @@ -217,6 +235,7 @@ impl From for ProtocolAppData { signer: None, replaced_order: None, partner_fee: None, + flashloan: None, } } } diff --git a/crates/e2e/tests/e2e/app_data.rs b/crates/e2e/tests/e2e/app_data.rs index b6e9bbac14..4972c81c3f 100644 --- a/crates/e2e/tests/e2e/app_data.rs +++ b/crates/e2e/tests/e2e/app_data.rs @@ -20,6 +20,12 @@ async fn local_node_app_data() { run_test(app_data).await; } +#[tokio::test] +#[ignore] +async fn local_node_app_data_full_format() { + run_test(app_data_full_format).await; +} + // Test that orders can be placed with the new app data format. async fn app_data(web3: Web3) { let mut onchain = OnchainComponents::deploy(web3).await; @@ -177,3 +183,109 @@ async fn app_data(web3: Web3) { .unwrap_err(); dbg!(err); } + +/// Tests that orders can be placed with an app data JSON containing +/// all supported features. +async fn app_data_full_format(web3: Web3) { + let mut onchain = OnchainComponents::deploy(web3).await; + let [solver] = onchain.make_solvers(to_wei(1)).await; + let [trader] = onchain.make_accounts(to_wei(1)).await; + let [token_a, token_b] = onchain + .deploy_tokens_with_weth_uni_v2_pools(to_wei(1_000), to_wei(1_000)) + .await; + + token_a.mint(trader.address(), to_wei(10)).await; + tx!( + trader.account(), + token_a.approve(onchain.contracts().allowance, to_wei(10)) + ); + + let mut valid_to: u32 = model::time::now_in_epoch_seconds() + 300; + let mut create_order = |app_data| { + let order = OrderCreation { + app_data, + sell_token: token_a.address(), + sell_amount: to_wei(2), + buy_token: token_b.address(), + buy_amount: to_wei(1), + valid_to, + kind: OrderKind::Sell, + ..Default::default() + } + .sign( + EcdsaSigningScheme::Eip712, + &onchain.contracts().domain_separator, + SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()), + ); + // Adjust valid to make sure we get unique UIDs. + valid_to += 1; + order + }; + + let services = Services::new(&onchain).await; + services.start_protocol(solver).await; + + // Unknown hashes are not accepted. + let order0 = create_order(OrderCreationAppData::Hash { + hash: AppDataHash([0; 32]), + }); + let order_uid = services.create_order(&order0).await.unwrap(); + + // appdata using all features + let app_data = format!( + r#"{{ + "version": "0.9.0", + "appCode": "CoW Swap", + "environment": "barn", + "metadata": {{ + "quote": {{ + "slippageBps": "50" + }}, + "hooks": {{ + "pre": [ + {{ + "target": "0x0000000000000000000000000000000000000000", + "callData": "0x12345678", + "gasLimit": "21000" + }} + ], + "post": [ + {{ + "target": "0x0000000000000000000000000000000000000000", + "callData": "0x12345678", + "gasLimit": "21000" + }} + ] + }}, + "flashloan": {{ + "lender": "0x1111111111111111111111111111111111111111", + "borrower": "0x2222222222222222222222222222222222222222", + "token": "0x3333333333333333333333333333333333333333", + "amount": "1234" + }}, + "signer": "{:?}", + "replacedOrder": {{ + "uid": "{}" + }}, + "partnerFee": {{ + "bps": 1, + "recipient": "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + }} + }} + }}"#, + trader.address(), + order_uid + ); + + let app_data_hash = AppDataHash(hash_full_app_data(app_data.as_bytes())); + let order1 = create_order(OrderCreationAppData::Full { + full: app_data.to_string(), + }); + let uid = services.create_order(&order1).await.unwrap(); + let order1_ = services.get_order(&uid).await.unwrap(); + assert_eq!(order1_.data.app_data, app_data_hash); + assert_eq!(order1_.metadata.full_app_data, Some(app_data.to_string())); + + let app_data_ = services.get_app_data(app_data_hash).await.unwrap(); + assert_eq!(app_data_, app_data); +} From 395d605a821adbcf6f27fd7ed9125c9eb0c11838 Mon Sep 17 00:00:00 2001 From: Martin Magnus Date: Thu, 16 Jan 2025 14:52:45 +0100 Subject: [PATCH 23/23] Support faking balances for more tokens (#3238) # Description To verify more quotes we introduced some logic to figure out which storage slots need to be overwritten to fake a token balance for a given owner. That way our quote verification simulation will not revert when a user currently doesn't hold the necessary amounts of token which improves UX a lot. While looking into some quote we couldn't verify ([simulation](https://dashboard.tenderly.co/cow-protocol/production/simulator/e3456eaf-90e6-4212-9758-deef4b80948f?trace=0.3.0.3.0.0.0.5.0.0.1.0.0.0.0)) it became apparent that our current strategy to detect which storage slot needs to get overwritten doesn't work for some tokens. The current logic assumes a "normal" contract where member variables are laid out using the default solidity logic. This is not sufficient to detect proxy contracts which store their state at a hardcoded address since we can't predict where the author decided to put their member variables in their custom memory layout. However, most of the time people use libraries to handle that logic for them. These libraries hardcode the necessary information to find the member variable storing the balances in the token so we can just add support by probing for these known hardcoded values too. Kudos to @fedgiac for helping me debug the problem with the original proxy contract. # Changes This PR adds support for 2 libraries: OpenZeppelin and Solady. The OpenZeppeline implementation simply puts the `balances` member variable at a hardcoded position so we can use the existing encoding logic with the hardcoded address we found the in their source code. Solady is a bit different since it doesn't hash 64 bytes but instead only hashes 48 bytes to compute the storage slot of an addresses balance. For this I introduced another enum variant. This PR also fixes a bug with the original implementation. The original implementation works by calling `token.balanceOf(address)` with 25 storage slots. Each slot gets overwritten with a specific value. When `balanceOf()` returns that value we recover which storage slot we originally overwrote. This idea makes a lot of sense but had a problem with the very first override. The amount we override was computed with `U256::from(u64::from_be_bytes([i; 8]))` where `i` is an index variable. That means the first override always tried to set the balance to `0`. If you call `token.balanceOf(random_address)` it will return 0 because the address doesn't have any balances. The original logic interpreted this `0` as the sentinel `0` value it originally overwrote and therefore returned that the `balances` mapping lives in the `0th` storage slot. To fix that we now compute the sentinel values with `U256::from(u64::from_be_bytes([i + 1; 8]))` to ensure that we always search for non-zero values to detect the correct storage override. Note that this error only surfaced when none of the storage slots we tried to override was the correct one. If we were able to override the storage slot `balanceOf()` would no longer return `0` to cause the false positive. ## How to test added an ignored unit test detecting 1 storage slot of each variant: 1. standard solidity mapping + standard solidity layout 2. standard solidity mapping + hardcoded OpenZeppelin storage slot 3. custom Solady mapping Also there was already 1 e2e test that already tested the original logic. --- .../trade_verifier/balance_overrides.rs | 105 ++++++++--- .../balance_overrides/detector.rs | 167 +++++++++++++----- 2 files changed, 206 insertions(+), 66 deletions(-) diff --git a/crates/shared/src/price_estimation/trade_verifier/balance_overrides.rs b/crates/shared/src/price_estimation/trade_verifier/balance_overrides.rs index d1f85c03d3..d12d3c9621 100644 --- a/crates/shared/src/price_estimation/trade_verifier/balance_overrides.rs +++ b/crates/shared/src/price_estimation/trade_verifier/balance_overrides.rs @@ -86,7 +86,8 @@ impl Display for TokenConfiguration { fn fmt(&self, f: &mut Formatter) -> fmt::Result { let format_entry = |f: &mut Formatter, (addr, strategy): (&Address, &Strategy)| match strategy { - Strategy::Mapping { slot } => write!(f, "{addr:?}@{slot}"), + Strategy::SolidityMapping { slot } => write!(f, "{addr:?}@{slot}"), + Strategy::SoladyMapping => write!(f, "SoladyMapping({addr:?})"), }; let mut entries = self.0.iter(); @@ -121,7 +122,7 @@ impl FromStr for TokenConfiguration { .context("expected {addr}@{slot} format")?; Ok(( addr.parse()?, - Strategy::Mapping { + Strategy::SolidityMapping { slot: slot.parse()?, }, )) @@ -151,7 +152,7 @@ pub struct BalanceOverrideRequest { } /// Balance override strategy for a token. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub enum Strategy { /// Balance override strategy for tokens whose balances are stored in a /// direct Solidity mapping from token holder to balance amount in the @@ -160,29 +161,40 @@ pub enum Strategy { /// The strategy is configured with the storage slot [^1] of the mapping. /// /// [^1]: - Mapping { slot: U256 }, + SolidityMapping { slot: U256 }, + /// Strategy computing storage slot for balances based on the Solady library + /// [^1]. + /// + /// [^1]: + SoladyMapping, } impl Strategy { /// Computes the storage slot and value to override for a particular token /// holder and amount. fn state_override(&self, holder: &Address, amount: &U256) -> (H256, H256) { - match self { - Self::Mapping { slot } => { - let key = { - let mut buf = [0; 64]; - buf[12..32].copy_from_slice(holder.as_fixed_bytes()); - slot.to_big_endian(&mut buf[32..64]); - H256(signing::keccak256(&buf)) - }; - let value = { - let mut buf = [0; 32]; - amount.to_big_endian(&mut buf); - H256(buf) - }; - (key, value) + let key = match self { + Self::SolidityMapping { slot } => { + let mut buf = [0; 64]; + buf[12..32].copy_from_slice(holder.as_fixed_bytes()); + slot.to_big_endian(&mut buf[32..64]); + H256(signing::keccak256(&buf)) } - } + Self::SoladyMapping => { + let mut buf = [0; 32]; + buf[0..20].copy_from_slice(holder.as_fixed_bytes()); + buf[28..32].copy_from_slice(&[0x87, 0xa2, 0x11, 0xa2]); + H256(signing::keccak256(&buf)) + } + }; + + let value = { + let mut buf = [0; 32]; + amount.to_big_endian(&mut buf); + H256(buf) + }; + + (key, value) } } @@ -264,7 +276,7 @@ mod tests { async fn balance_override_computation() { let balance_overrides = BalanceOverrides { hardcoded: hashmap! { - addr!("DEf1CA1fb7FBcDC777520aa7f396b4E015F497aB") => Strategy::Mapping { + addr!("DEf1CA1fb7FBcDC777520aa7f396b4E015F497aB") => Strategy::SolidityMapping { slot: U256::from(0), }, }, @@ -290,7 +302,7 @@ mod tests { // You can verify the state override computation is correct by running: // ``` - // curl -X POST $RPC -H 'Content-Type: application/data' --data '{ + // curl -X POST $RPC -H 'Content-Type: application/json' --data '{ // "jsonrpc": "2.0", // "id": 0, // "method": "eth_call", @@ -327,4 +339,55 @@ mod tests { None, ); } + + #[tokio::test] + async fn balance_override_computation_solady() { + let balance_overrides = BalanceOverrides { + hardcoded: hashmap! { + addr!("0000000000c5dc95539589fbd24be07c6c14eca4") => Strategy::SoladyMapping, + }, + ..Default::default() + }; + + assert_eq!( + balance_overrides + .state_override(BalanceOverrideRequest { + token: addr!("0000000000c5dc95539589fbd24be07c6c14eca4"), + holder: addr!("d8dA6BF26964aF9D7eEd9e03E53415D37aA96045"), + amount: 0x42_u64.into(), + }) + .await, + Some(StateOverride { + state_diff: Some(hashmap! { + H256(hex!("f6a6656ed2d14bad3cdd3e8871db3f535a136a1b6cd5ae2dced8eb813f3d4e4f")) => + H256(hex!("0000000000000000000000000000000000000000000000000000000000000042")), + }), + ..Default::default() + }), + ); + + // You can verify the state override computation is correct by running: + // ``` + // curl -X POST $RPC -H 'Content-Type: application/json' --data '{ + // "jsonrpc": "2.0", + // "id": 0, + // "method": "eth_call", + // "params": [ + // { + // "to": "0x0000000000c5dc95539589fbd24be07c6c14eca4", + // "data": "0x70a08231000000000000000000000000d8dA6BF26964aF9D7eEd9e03E53415D37aA96045" + // }, + // "latest", + // { + // "0x0000000000c5dc95539589fbd24be07c6c14eca4": { + // "stateDiff": { + // "f6a6656ed2d14bad3cdd3e8871db3f535a136a1b6cd5ae2dced8eb813f3d4e4f": + // "0x0000000000000000000000000000000000000000000000000000000000000042" + // } + // } + // } + // ] + // }' + // ``` + } } diff --git a/crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs b/crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs index d8795b7ce4..257d3ef4d5 100644 --- a/crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs +++ b/crates/shared/src/price_estimation/trade_verifier/balance_overrides/detector.rs @@ -2,12 +2,13 @@ use { super::Strategy, crate::code_simulation::{CodeSimulating, SimulationError}, contracts::{dummy_contract, ERC20}, - ethcontract::{Address, U256}, + ethcontract::{Address, H256, U256}, ethrpc::extensions::StateOverride, maplit::hashmap, std::{ + collections::HashMap, fmt::{self, Debug, Formatter}, - sync::Arc, + sync::{Arc, LazyLock}, }, thiserror::Error, web3::{signing::keccak256, types::CallRequest}, @@ -22,9 +23,6 @@ pub struct Detector { } impl Detector { - /// Number of different slots to try out. - const TRIES: u8 = 25; - /// Creates a new balance override detector. pub fn new(simulator: Arc) -> Self { Self { simulator } @@ -34,49 +32,15 @@ impl Detector { /// Returns an `Err` if it cannot detect the strategy or an internal /// simulation fails. pub async fn detect(&self, token: Address) -> Result { - // This is a pretty unsophisticated strategy where we basically try a - // bunch of different slots and see which one sticks. We try balance - // mappings for the first `TRIES` slots; each with a unique value. - let mut tries = (0..Self::TRIES).map(|i| { - let strategy = Strategy::Mapping { - slot: U256::from(i), - }; - // Use an exact value which isn't too large or too small. This helps - // not have false positives for cases where the token balances in - // some other denomination from the actual token balance (such as - // stETH for example) and not run into issues with overflows. - let amount = U256::from(u64::from_be_bytes([i; 8])); - - (strategy, amount) - }); - - // On a technical note, Ethereum public addresses are, for the most - // part, generated by taking the 20 last bytes of a Keccak-256 hash (for - // things like contract creation, public address derivation from a - // Secp256k1 public key, etc.), so we use one for our heuristics from a - // 32-byte digest with no know pre-image, to prevent any weird - // interactions with the weird tokens of the world. - let holder = { - let mut address = Address::default(); - address.0.copy_from_slice(&keccak256(b"Moo!")[12..]); - address.0[19] = address.0[19].wrapping_sub(1); - address - }; - let token = dummy_contract!(ERC20, token); let call = CallRequest { to: Some(token.address()), - data: token.methods().balance_of(holder).m.tx.data, + data: token.methods().balance_of(*HOLDER).m.tx.data, ..Default::default() }; let overrides = hashmap! { token.address() => StateOverride { - state_diff: Some( - tries - .clone() - .map(|(strategy, amount)| strategy.state_override(&holder, &amount)) - .collect(), - ), + state_diff: Some(STORAGE_OVERRIDES.clone()), ..Default::default() }, }; @@ -86,13 +50,87 @@ impl Detector { .then(|| U256::from_big_endian(&output)) .ok_or(DetectionError::Decode)?; - let strategy = tries - .find_map(|(strategy, amount)| (amount == balance).then_some(strategy)) - .ok_or(DetectionError::NotFound)?; - Ok(strategy) + TESTED_STRATEGIES + .iter() + .find_map(|helper| (helper.balance == balance).then_some(helper.strategy.clone())) + .ok_or(DetectionError::NotFound) + } +} + +/// Contains all the information we need to determine which state override +/// was successful. +struct StrategyHelper { + /// strategy that was used to compute the state override + strategy: Strategy, + /// balance amount the strategy wrote into the storage + balance: U256, +} + +impl StrategyHelper { + fn new(strategy: Strategy, index: u8) -> Self { + Self { + strategy, + // Use an exact value which isn't too large or too small. This helps + // not have false positives for cases where the token balances in + // some other denomination from the actual token balance (such as + // stETH for example) and not run into issues with overflows. + // We also make sure that we avoid 0 because `balanceOf()` returns + // 0 by default so we can't use it to detect successful state overrides. + balance: U256::from(u64::from_be_bytes([index + 1; 8])), + } } } +/// Storage slot based on OpenZeppelin's ERC20Upgradeable contract [^1]. +/// +/// [^1]: +static OPEN_ZEPPELIN_ERC20_UPGRADEABLE: &str = + "52c63247e1f47db19d5ce0460030c497f067ca4cebf71ba98eeadabe20bace00"; + +/// Address which we try to override the balances for. +static HOLDER: LazyLock
= LazyLock::new(|| { + // On a technical note, Ethereum public addresses are, for the most + // part, generated by taking the 20 last bytes of a Keccak-256 hash (for + // things like contract creation, public address derivation from a + // Secp256k1 public key, etc.), so we use one for our heuristics from a + // 32-byte digest with no know pre-image, to prevent any weird + // interactions with the weird tokens of the world. + let mut address = Address::default(); + address.0.copy_from_slice(&keccak256(b"Moo!")[12..]); + address.0[19] = address.0[19].wrapping_sub(1); + address +}); + +/// All the strategies we use to detect where a token stores the balances. +static TESTED_STRATEGIES: LazyLock> = LazyLock::new(|| { + const FIRST_N_SLOTS: u8 = 25; + + // This is a pretty unsophisticated strategy where we basically try a + // bunch of different slots and see which one sticks. We try balance + // mappings for the first `TRIES` slots; each with a unique value. + (0..FIRST_N_SLOTS).map(|i| { + let strategy = Strategy::SolidityMapping { slot: U256::from(i) }; + StrategyHelper::new(strategy, i) + }) + // Afterwards we try hardcoded storage slots based on popular utility + // libraries like OpenZeppelin. + .chain((FIRST_N_SLOTS..).zip([ + Strategy::SolidityMapping{ slot: U256::from(OPEN_ZEPPELIN_ERC20_UPGRADEABLE) }, + Strategy::SoladyMapping, + ]).map(|(index, strategy)| { + StrategyHelper::new(strategy, index) + })) + .collect() +}); + +/// Storage overrides (storage_slot, value) for all tested strategies. +static STORAGE_OVERRIDES: LazyLock> = LazyLock::new(|| { + TESTED_STRATEGIES + .iter() + .map(|helper| helper.strategy.state_override(&HOLDER, &helper.balance)) + .collect::>() +}); + impl Debug for Detector { fn fmt(&self, f: &mut Formatter) -> fmt::Result { f.debug_struct("Detector") @@ -111,3 +149,42 @@ pub enum DetectionError { #[error(transparent)] Simulation(#[from] SimulationError), } + +#[cfg(test)] +mod tests { + use {super::*, ethrpc::create_env_test_transport, web3::Web3}; + + /// Tests that we can detect storage slots by probing the first + /// n slots or by checking hardcoded known slots. + /// Set `NODE_URL` environment to a mainnet RPC URL. + #[ignore] + #[tokio::test] + async fn detects_storage_slots() { + let detector = Detector { + simulator: Arc::new(Web3::new(create_env_test_transport())), + }; + + let storage = detector + .detect(addr!("c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2")) + .await + .unwrap(); + assert_eq!(storage, Strategy::SolidityMapping { slot: 3.into() }); + + let storage = detector + .detect(addr!("4956b52ae2ff65d74ca2d61207523288e4528f96")) + .await + .unwrap(); + assert_eq!( + storage, + Strategy::SolidityMapping { + slot: U256::from(OPEN_ZEPPELIN_ERC20_UPGRADEABLE), + } + ); + + let storage = detector + .detect(addr!("0000000000c5dc95539589fbd24be07c6c14eca4")) + .await + .unwrap(); + assert_eq!(storage, Strategy::SoladyMapping); + } +}