From 4f2bc3e4fdc96b26e50cb01d26720232ab4a390f Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 7 Dec 2023 09:55:00 +0000 Subject: [PATCH 01/11] Remove GnosisSafe gas estimator (#2128) # Description Since the GnosisSafe isn't working and is not available as a gas estimator anymore (as detailed [here](https://github.com/cowprotocol/gas-estimation/pull/12)), I removed all code related to it. # Changes - Remove GnosisSafe gas estimator code ## How to test CI. ## Related Issues This PR should not be merged before [this](https://github.com/cowprotocol/infrastructure/pull/909) is. Related to issue #2094. --- crates/shared/src/arguments.rs | 1 - crates/shared/src/gas_price_estimation.rs | 5 ----- 2 files changed, 6 deletions(-) diff --git a/crates/shared/src/arguments.rs b/crates/shared/src/arguments.rs index 9dc63df69e..059ba21d5f 100644 --- a/crates/shared/src/arguments.rs +++ b/crates/shared/src/arguments.rs @@ -177,7 +177,6 @@ pub struct Arguments { /// a previous one fails. Individual estimators support different /// networks. `EthGasStation`: supports mainnet. /// `GasNow`: supports mainnet. - /// `GnosisSafe`: supports mainnet and goerli. /// `Web3`: supports every network. /// `Native`: supports every network. #[clap( diff --git a/crates/shared/src/gas_price_estimation.rs b/crates/shared/src/gas_price_estimation.rs index 242a5bf25f..8649f8a09c 100644 --- a/crates/shared/src/gas_price_estimation.rs +++ b/crates/shared/src/gas_price_estimation.rs @@ -8,7 +8,6 @@ use { GasNowGasStation, GasPrice1559, GasPriceEstimating, - GnosisSafeGasStation, PriorityGasPriceEstimating, Transport, }, @@ -22,7 +21,6 @@ use { pub enum GasEstimatorType { EthGasStation, GasNow, - GnosisSafe, Web3, BlockNative, Native, @@ -92,9 +90,6 @@ pub async fn create_priority_estimator( ensure!(is_mainnet(&network_id), "GasNow only supports mainnet"); estimators.push(Box::new(GasNowGasStation::new(client()))) } - GasEstimatorType::GnosisSafe => estimators.push(Box::new( - GnosisSafeGasStation::with_network_id(&network_id, client())?, - )), GasEstimatorType::Web3 => estimators.push(Box::new(web3.clone())), GasEstimatorType::Native => { match NativeGasEstimator::new(web3.transport().clone(), None).await { From 4ef426b1621b5d37fb0f24ccee5512bc1dab82fa Mon Sep 17 00:00:00 2001 From: Felix Leupold Date: Thu, 7 Dec 2023 11:30:23 +0100 Subject: [PATCH 02/11] [Trivial] Record mempool submission metrics in driver (#2127) # Description Needed to replace this [alert](https://cowservices.slack.com/archives/C036ZGSCJNP/p1701783487316099) Note, that this doesn't include information about the solver that is submitting the solution. I don't think this is necessarily required, so I didn't add it. --- crates/driver/src/infra/observe/metrics.rs | 3 +++ crates/driver/src/infra/observe/mod.rs | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/crates/driver/src/infra/observe/metrics.rs b/crates/driver/src/infra/observe/metrics.rs index bf2f957508..330e250867 100644 --- a/crates/driver/src/infra/observe/metrics.rs +++ b/crates/driver/src/infra/observe/metrics.rs @@ -16,6 +16,9 @@ pub struct Metrics { /// The results of the quoting process. #[metric(labels("solver", "result"))] pub quotes: prometheus::IntCounterVec, + /// The results of the mempool submission. + #[metric(labels("mempool", "result"))] + pub mempool_submission: 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 9b0d353293..b18e29c4e9 100644 --- a/crates/driver/src/infra/observe/mod.rs +++ b/crates/driver/src/infra/observe/mod.rs @@ -333,6 +333,15 @@ pub fn mempool_executed( ); } } + let result = match res { + Ok(_) => "Success", + Err(mempools::Error::Revert(_) | mempools::Error::SimulationRevert) => "Revert", + Err(mempools::Error::Other(_)) => "Other", + }; + metrics::get() + .mempool_submission + .with_label_values(&[&mempool.to_string(), result]) + .inc(); } /// Observe that an invalid DTO was received. From 07a966e50d8d70e2a52833623a6b8715542f81cb Mon Sep 17 00:00:00 2001 From: ilya Date: Thu, 7 Dec 2023 10:40:52 +0000 Subject: [PATCH 03/11] [Trivial] DEX rate limiter default config values (#2132) # Description Adjusts DEX rate limiter config default values ## How to test Using logs --- crates/solvers/src/infra/config/dex/file.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/solvers/src/infra/config/dex/file.rs b/crates/solvers/src/infra/config/dex/file.rs index 9f126618da..244954353d 100644 --- a/crates/solvers/src/infra/config/dex/file.rs +++ b/crates/solvers/src/infra/config/dex/file.rs @@ -78,15 +78,15 @@ fn default_smallest_partial_fill() -> eth::U256 { } fn default_back_off_growth_factor() -> f64 { - 1.0 + 2.0 } fn default_min_back_off() -> u64 { - Default::default() + 1 } fn default_max_back_off() -> u64 { - Default::default() + 8 } /// Loads the base solver configuration from a TOML file. From bb5a241789576c33e9d1307e296be11ce6af5b14 Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Thu, 7 Dec 2023 16:58:28 +0100 Subject: [PATCH 04/11] Speed up token meta data look up (#2134) # Description We've noticed weird delays between the autopilot sending an auction and the driver being ready to start working on it. The linked issue suggests that we spend a lot of time converting the auction and enriching it with token meta data. This should be super fast because all the data should be cached at all times. However, was one token that caused it all to break down: `0xeeeee...` (the address we use to denote the chains native token) This is roughly what happened: 1. every driver receives auction 2. every driver collects all the addresses for which we don't have cached metadata 3. this always yielded `0xeeee...` (it's not a real token so we never get data when calling ERC20 functions on it) 4. every driver wasted some time sending RPC requests for `0xeeee` 5. every driver tried to take an exclusive lock to write no data (empty vec) to the cache Overall this wasted time with unnecessary network requests and serially taking an exclusive lock for all drivers. # Changes * most importantly filter out `0xeee` * avoid sending duplicate RPC requests by using `RequestSharing` * don't take an exclusive lock if some other driver cached the same data in the meantime * moved logging of token balance update task out of the critical section All changes combined results in us now spending a couple microseconds on getting cached balances instead of seconds. And because this was the bulk of the work captured by the log `auction task execution time` we reduced that time to ~11ms. ## How to test e2e tests should still pass and manual test confirmed the reduced latency. Fixes #2133 --- .../driver/src/infra/api/routes/solve/mod.rs | 2 +- crates/driver/src/infra/tokens.rs | 132 ++++++++++++------ 2 files changed, 88 insertions(+), 46 deletions(-) diff --git a/crates/driver/src/infra/api/routes/solve/mod.rs b/crates/driver/src/infra/api/routes/solve/mod.rs index 997995f5cb..07fc1916fa 100644 --- a/crates/driver/src/infra/api/routes/solve/mod.rs +++ b/crates/driver/src/infra/api/routes/solve/mod.rs @@ -30,7 +30,7 @@ async fn route( .tap_err(|err| { observe::invalid_dto(err, "auction"); })?; - tracing::debug!(elapsed=?start.elapsed(), auction_id=%auction_id, "auction task execution time"); + tracing::debug!(elapsed = ?start.elapsed(), "auction task execution time"); let auction = state.pre_processor().prioritize(auction).await; let competition = state.competition(); let result = competition.solve(&auction).await; diff --git a/crates/driver/src/infra/tokens.rs b/crates/driver/src/infra/tokens.rs index f255943bf6..946c7319b6 100644 --- a/crates/driver/src/infra/tokens.rs +++ b/crates/driver/src/infra/tokens.rs @@ -3,10 +3,14 @@ use { domain::eth, infra::{blockchain, Ethereum}, }, + anyhow::Result, ethrpc::current_block::{self, CurrentBlockStream}, - futures::StreamExt, + futures::{FutureExt, StreamExt}, + itertools::Itertools, + model::order::BUY_ETH_ADDRESS, + shared::request_sharing::BoxRequestSharing, std::{ - collections::{HashMap, HashSet}, + collections::HashMap, sync::{Arc, RwLock}, }, tracing::Instrument, @@ -29,6 +33,7 @@ impl Fetcher { let inner = Arc::new(Inner { eth, cache: RwLock::new(HashMap::new()), + requests: BoxRequestSharing::labelled("token_info".into()), }); tokio::task::spawn( update_task(block_stream, Arc::downgrade(&inner)) @@ -70,10 +75,6 @@ async fn update_balances(inner: Arc) -> Result<(), blockchain::Error> { let futures = { let cache = inner.cache.read().unwrap(); let tokens = cache.keys().cloned().collect::>(); - tracing::debug!( - tokens = tokens.len(), - "updating settlement contract balances" - ); tokens.into_iter().map(|token| { let erc20 = inner.eth.erc20(token); async move { @@ -85,6 +86,11 @@ async fn update_balances(inner: Arc) -> Result<(), blockchain::Error> { }) }; + tracing::debug!( + tokens = futures.len(), + "updating settlement contract balances" + ); + // Don't hold on to the lock while fetching balances to allow concurrent // updates. This may lead to new entries arriving in the meantime, however // their balances should already be up-to-date. @@ -93,14 +99,22 @@ async fn update_balances(inner: Arc) -> Result<(), blockchain::Error> { .into_iter() .collect::>(); - let mut cache = inner.cache.write().unwrap(); - for (key, entry) in cache.iter_mut() { - if let Some(balance) = balances.remove(key) { - entry.balance = balance; - } else { - tracing::info!(?key, "key without balance update"); + let mut keys_without_balances = vec![]; + { + let mut cache = inner.cache.write().unwrap(); + for (key, entry) in cache.iter_mut() { + if let Some(balance) = balances.remove(key) { + entry.balance = balance; + } else { + // Avoid logging while holding the exclusive lock. + keys_without_balances.push(*key); + } } } + if !keys_without_balances.is_empty() { + tracing::info!(keys = ?keys_without_balances, "updated keys without balance"); + } + Ok(()) } @@ -108,60 +122,88 @@ async fn update_balances(inner: Arc) -> Result<(), blockchain::Error> { struct Inner { eth: Ethereum, cache: RwLock>, + requests: BoxRequestSharing>, } impl Inner { /// Fetches `Metadata` of the requested tokens from a node. async fn fetch_token_infos( &self, - tokens: HashSet, - ) -> Vec> { + tokens: &[eth::TokenAddress], + ) -> Vec> { let settlement = self.eth.contracts().settlement().address().into(); - let futures = tokens.into_iter().map(|token| async move { - let token = self.eth.erc20(token); - // Use `try_join` because these calls get batched under the hood - // so if one of them fails the others will as well. - // Also this way we won't get incomplete data for a token. - let (decimals, symbol, balance) = futures::future::try_join3( - token.decimals(), - token.symbol(), - token.balance(settlement), - ) - .await?; - Ok(( - token.address(), - Metadata { - decimals, - symbol, - balance, - }, - )) + let futures = tokens.iter().map(|token| { + let build_request = |token: ð::TokenAddress| { + let token = self.eth.erc20(*token); + async move { + // Use `try_join` because these calls get batched under the hood + // so if one of them fails the others will as well. + // Also this way we won't get incomplete data for a token. + let (decimals, symbol, balance) = futures::future::try_join3( + token.decimals(), + token.symbol(), + token.balance(settlement), + ) + .await + .ok()?; + + Some(( + token.address(), + Metadata { + decimals, + symbol, + balance, + }, + )) + } + .boxed() + }; + + self.requests.shared_or_else(*token, build_request) }); futures::future::join_all(futures).await } + /// Ensures that all the missing tokens are in the cache afterwards while + /// taking into account that the function might be called multiple times + /// for the same tokens. + async fn cache_missing_tokens(&self, tokens: &[eth::TokenAddress]) { + if tokens.is_empty() { + return; + } + + let fetched = self.fetch_token_infos(tokens).await; + { + let cache = self.cache.read().unwrap(); + if tokens.iter().all(|token| cache.contains_key(token)) { + // Often multiple callers are racing to fetch the same Metadata. + // If somebody else already cached the data we don't want to take an + // exclusive lock for nothing. + return; + } + } + self.cache + .write() + .unwrap() + .extend(fetched.into_iter().flatten()); + } + async fn get(&self, addresses: &[eth::TokenAddress]) -> HashMap { - let to_fetch: HashSet<_> = { + let to_fetch: Vec<_> = { let cache = self.cache.read().unwrap(); // Compute set of requested addresses that are not in cache. addresses .iter() - .filter(|address| !cache.contains_key(*address)) + // BUY_ETH_ADDRESS is just a marker and not a real address. We'll never be able to + // fetch data for it so ignore it to avoid taking exclusive locks all the time. + .filter(|address| !cache.contains_key(*address) && address.0.0 != BUY_ETH_ADDRESS) .cloned() + .unique() .collect() }; - // Fetch token infos not yet in cache. - if !to_fetch.is_empty() { - let fetched = self.fetch_token_infos(to_fetch).await; - - // Add valid token infos to cache. - self.cache - .write() - .unwrap() - .extend(fetched.into_iter().flatten()); - }; + self.cache_missing_tokens(&to_fetch).await; let cache = self.cache.read().unwrap(); // Return token infos from the cache. From 75920cb2a99028e1e422ee5477337acb31194a30 Mon Sep 17 00:00:00 2001 From: ilya Date: Thu, 7 Dec 2023 17:26:49 +0000 Subject: [PATCH 05/11] Optimize rows count metrics (#2135) # Description Noticed that the rows count query for the `order_events` table takes **150-200+ seconds** both in staging and prod. The proposal is to try out using rough row count estimation for large tables. # Changes `SELECT COUNT(*) ...` is replaced with `SELECT reltuples FROM pg_class WHERE relname='{table}';` for the `order_events` table. ## How to test Will analyze metrics to understand how often Postgres updates the statistics without manually triggering `ANALYZE`. ## Further optimization In case statistics update happens often enough, the same approach could be considered for other tables where very often row count update is not necessary. --------- Co-authored-by: Martin Beckmann --- crates/autopilot/src/database.rs | 14 +++++++++++++- crates/database/src/lib.rs | 14 ++++++++++---- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/crates/autopilot/src/database.rs b/crates/autopilot/src/database.rs index 33927a98d0..ca2af3bd71 100644 --- a/crates/autopilot/src/database.rs +++ b/crates/autopilot/src/database.rs @@ -28,12 +28,19 @@ impl Postgres { let metrics = Metrics::get(); // update table row metrics - for &table in database::ALL_TABLES { + for &table in database::TABLES { let mut ex = self.0.acquire().await?; let count = count_rows_in_table(&mut ex, table).await?; metrics.table_rows.with_label_values(&[table]).set(count); } + // update table row metrics + for &table in database::LARGE_TABLES { + let mut ex = self.0.acquire().await?; + let count = estimate_rows_in_table(&mut ex, table).await?; + metrics.table_rows.with_label_values(&[table]).set(count); + } + // update unused app data metric { let mut ex = self.0.acquire().await?; @@ -50,6 +57,11 @@ async fn count_rows_in_table(ex: &mut PgConnection, table: &str) -> sqlx::Result sqlx::query_scalar(&query).fetch_one(ex).await } +async fn estimate_rows_in_table(ex: &mut PgConnection, table: &str) -> sqlx::Result { + let query = format!("SELECT reltuples FROM pg_class WHERE relname='{table}';"); + sqlx::query_scalar(&query).fetch_one(ex).await +} + async fn count_unused_app_data(ex: &mut PgConnection) -> sqlx::Result { let query = r#" SELECT diff --git a/crates/database/src/lib.rs b/crates/database/src/lib.rs index cf17d547fb..77046a684e 100644 --- a/crates/database/src/lib.rs +++ b/crates/database/src/lib.rs @@ -44,10 +44,9 @@ use { pub type PgTransaction<'a> = sqlx::Transaction<'a, sqlx::Postgres>; -/// The names of all tables we use in the db. -pub const ALL_TABLES: &[&str] = &[ +/// The names of tables we use in the db. +pub const TABLES: &[&str] = &[ "orders", - "order_events", "trades", "invalidations", "quotes", @@ -69,10 +68,17 @@ pub const ALL_TABLES: &[&str] = &[ "app_data", ]; +/// The names of potentially big volume tables we use in the db. +pub const LARGE_TABLES: &[&str] = &["order_events"]; + +pub fn all_tables() -> impl Iterator { + TABLES.iter().copied().chain(LARGE_TABLES.iter().copied()) +} + /// Delete all data in the database. Only used by tests. #[allow(non_snake_case)] pub async fn clear_DANGER_(ex: &mut PgTransaction<'_>) -> sqlx::Result<()> { - for table in ALL_TABLES { + for table in all_tables() { ex.execute(format!("TRUNCATE {table};").as_str()).await?; } Ok(()) From 2f8b38eaecf69976f6c9ab4f894c3dc5acd36f5a Mon Sep 17 00:00:00 2001 From: ilya Date: Thu, 7 Dec 2023 17:45:51 +0000 Subject: [PATCH 06/11] Run order events insertion in parallel (#2131) # Description Executes order events db insert operation in parallel in order to propagate the auction among solvers as soon as possible. The same approach could be used in other parts of the application since this operation takes a while. ## How to test Logs observation ## Related Issues Relates to https://github.com/cowprotocol/services/issues/2095 --- crates/autopilot/src/run.rs | 2 +- crates/autopilot/src/run_loop.rs | 29 +++++++++++++++++------------ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/crates/autopilot/src/run.rs b/crates/autopilot/src/run.rs index df1f949afc..5a452f8218 100644 --- a/crates/autopilot/src/run.rs +++ b/crates/autopilot/src/run.rs @@ -619,7 +619,7 @@ pub async fn run(args: Arguments) { .await; let run = RunLoop { solvable_orders_cache, - database: db, + database: Arc::new(db), drivers: args.drivers.into_iter().map(Driver::new).collect(), current_block: current_block_stream, web3, diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 3519755b31..8f514e9bd2 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -44,7 +44,7 @@ use { pub struct RunLoop { pub solvable_orders_cache: Arc, - pub database: Postgres, + pub database: Arc, pub drivers: Vec, pub current_block: CurrentBlockStream, pub web3: Web3, @@ -309,17 +309,22 @@ impl RunLoop { ); let request = &request; - let start = Instant::now(); - self.database - .store_order_events( - &auction - .orders - .iter() - .map(|o| (o.metadata.uid, OrderEventLabel::Ready)) - .collect_vec(), - ) - .await; - tracing::debug!(elapsed=?start.elapsed(), aution_id=%id, "storing order events took"); + let db = self.database.clone(); + let events = auction + .orders + .iter() + .map(|o| (o.metadata.uid, OrderEventLabel::Ready)) + .collect_vec(); + // insert into `order_events` table operations takes a while and the result is + // ignored, so we run it in the background + tokio::spawn( + async move { + let start = Instant::now(); + db.store_order_events(&events).await; + tracing::debug!(elapsed=?start.elapsed(), events_count=events.len(), "stored order events"); + } + .instrument(tracing::Span::current()), + ); let start = Instant::now(); futures::future::join_all(self.drivers.iter().map(|driver| async move { From b1461932e00d46552f50ad01cd2e51be025df7fa Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Thu, 7 Dec 2023 21:31:49 +0100 Subject: [PATCH 07/11] Fix large table count query (#2142) # Description The current query returns a value in an unexpected format. # Changes Cast the returned value to a type that can be converted to `i64`. ## How to test Ran the modified query in `pgadmin` to check that the type returned from it is the same as is returned from the regular `count(*)` query. --- crates/autopilot/src/database.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/autopilot/src/database.rs b/crates/autopilot/src/database.rs index ca2af3bd71..f641263cb5 100644 --- a/crates/autopilot/src/database.rs +++ b/crates/autopilot/src/database.rs @@ -58,7 +58,7 @@ async fn count_rows_in_table(ex: &mut PgConnection, table: &str) -> sqlx::Result } async fn estimate_rows_in_table(ex: &mut PgConnection, table: &str) -> sqlx::Result { - let query = format!("SELECT reltuples FROM pg_class WHERE relname='{table}';"); + let query = format!("SELECT reltuples::bigint FROM pg_class WHERE relname='{table}';"); sqlx::query_scalar(&query).fetch_one(ex).await } From bd466e57699b86607a80f47d35f03a8622d5daee Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Fri, 8 Dec 2023 11:52:12 +0100 Subject: [PATCH 08/11] Reveal settled order already in /solve Response (#1975) # Description Implements only the part > orders they intend to execute and their in/out amounts from https://github.com/cowprotocol/services/issues/1949 Orders are no longer sent as a reponse to `/reveal` but as a response to `/solve`. I also added in/out amounts for each order. --------- Co-authored-by: Felix Leupold --- crates/autopilot/src/driver_model.rs | 17 ++- crates/autopilot/src/run_loop.rs | 64 +++++---- crates/driver/src/domain/competition/mod.rs | 25 +++- .../domain/competition/solution/settlement.rs | 24 +++- .../src/domain/competition/solution/trade.rs | 43 +++++- crates/driver/src/domain/eth/mod.rs | 2 +- .../infra/api/routes/reveal/dto/revealed.rs | 8 +- .../src/infra/api/routes/solve/dto/solved.rs | 32 ++++- .../src/tests/cases/merge_settlements.rs | 8 +- .../src/tests/cases/multiple_solutions.rs | 16 ++- .../driver/src/tests/cases/negative_scores.rs | 8 +- .../src/tests/cases/score_competition.rs | 15 +- crates/driver/src/tests/setup/mod.rs | 136 +++++++++++------- .../e2e/partially_fillable_observed_score.rs | 12 +- crates/model/src/solver_competition.rs | 47 ++++-- .../src/database/solver_competition.rs | 2 +- crates/solver/src/driver.rs | 2 +- 17 files changed, 318 insertions(+), 143 deletions(-) diff --git a/crates/autopilot/src/driver_model.rs b/crates/autopilot/src/driver_model.rs index f2d195570f..5dd07e472a 100644 --- a/crates/autopilot/src/driver_model.rs +++ b/crates/autopilot/src/driver_model.rs @@ -60,6 +60,7 @@ pub mod solve { primitive_types::{H160, U256}, serde::{Deserialize, Serialize}, serde_with::{serde_as, DisplayFromStr}, + std::collections::HashMap, }; #[serde_as] @@ -136,6 +137,18 @@ pub mod solve { pub call_data: Vec, } + #[serde_as] + #[derive(Clone, Debug, Default, Deserialize)] + #[serde(rename_all = "camelCase", deny_unknown_fields)] + pub struct TradedAmounts { + /// The effective amount that left the user's wallet including all fees. + #[serde_as(as = "HexOrDecimalU256")] + pub sell_amount: U256, + /// The effective amount the user received after all fees. + #[serde_as(as = "HexOrDecimalU256")] + pub buy_amount: U256, + } + #[serde_as] #[derive(Clone, Debug, Default, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] @@ -148,6 +161,7 @@ pub mod solve { pub score: U256, /// Address used by the driver to submit the settlement onchain. pub submission_address: H160, + pub orders: HashMap, } #[derive(Clone, Debug, Default, Deserialize)] @@ -159,7 +173,7 @@ pub mod solve { pub mod reveal { use { - model::{bytes_hex, order::OrderUid}, + model::bytes_hex, serde::{Deserialize, Serialize}, serde_with::serde_as, }; @@ -186,7 +200,6 @@ pub mod reveal { #[derive(Clone, Debug, Default, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct Response { - pub orders: Vec, pub calldata: Calldata, } } diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 8f514e9bd2..9e983e5ab6 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -8,7 +8,7 @@ use { driver_model::{ reveal::{self, Request}, settle, - solve::{self, Class}, + solve::{self, Class, TradedAmounts}, }, solvable_orders::SolvableOrdersCache, }, @@ -34,7 +34,7 @@ use { rand::seq::SliceRandom, shared::{remaining_amounts, token_list::AutoUpdatingTokenList}, std::{ - collections::{BTreeMap, HashSet}, + collections::{BTreeMap, HashMap, HashSet}, sync::{Arc, Mutex}, time::{Duration, Instant}, }, @@ -149,9 +149,8 @@ impl RunLoop { } }; - let events = revealed - .orders - .iter() + let events = solution + .order_ids() .map(|o| (*o, OrderEventLabel::Considered)) .collect::>(); self.database.store_order_events(&events).await; @@ -178,7 +177,7 @@ impl RunLoop { // Save order executions for all orders in the solution. Surplus fees for // limit orders will be saved after settling the order onchain. let mut order_executions = vec![]; - for order_id in &revealed.orders { + for order_id in solution.order_ids() { let auction_order = auction .orders .iter() @@ -239,22 +238,21 @@ impl RunLoop { solver_address: participant.solution.account, score: Some(Score::Solver(participant.solution.score.get())), ranking: Some(solutions.len() - index), + orders: participant + .solution + .orders() + .iter() + .map(|(id, order)| Order::Colocated { + id: *id, + sell_amount: order.sell_amount, + buy_amount: order.buy_amount, + }) + .collect(), // TODO: revisit once colocation is enabled (remove not populated // fields) Not all fields can be populated in the colocated world ..Default::default() }; if is_winner { - settlement.orders = revealed - .orders - .iter() - .map(|o| Order { - id: *o, - // TODO: revisit once colocation is enabled (remove not - // populated fields) Not all - // fields can be populated in the colocated world - ..Default::default() - }) - .collect(); settlement.call_data = revealed.calldata.internalized.clone(); settlement.uninternalized_call_data = Some(revealed.calldata.uninternalized.clone()); @@ -288,7 +286,7 @@ impl RunLoop { } tracing::info!(driver = %driver.name, "settling"); - match self.settle(driver, solution, &revealed).await { + match self.settle(driver, solution).await { Ok(()) => Metrics::settle_ok(driver), Err(err) => { Metrics::settle_err(driver, &err); @@ -385,6 +383,7 @@ impl RunLoop { id: solution.solution_id, account: solution.submission_address, score: NonZeroU256::new(solution.score).ok_or(ZeroScoreError)?, + orders: solution.orders, }) }) .collect()) @@ -414,15 +413,9 @@ impl RunLoop { /// Execute the solver's solution. Returns Ok when the corresponding /// transaction has been mined. - async fn settle( - &self, - driver: &Driver, - solved: &Solution, - revealed: &reveal::Response, - ) -> Result<(), SettleError> { - let events = revealed - .orders - .iter() + async fn settle(&self, driver: &Driver, solved: &Solution) -> Result<(), SettleError> { + let events = solved + .order_ids() .map(|uid| (*uid, OrderEventLabel::Executing)) .collect_vec(); self.database.store_order_events(&events).await; @@ -439,12 +432,12 @@ impl RunLoop { *self.in_flight_orders.lock().unwrap() = InFlightOrders { tx_hash, - orders: revealed.orders.iter().cloned().collect(), + orders: solved.orders.keys().copied().collect(), }; - let events = revealed + let events = solved .orders - .iter() + .keys() .map(|uid| (*uid, OrderEventLabel::Traded)) .collect_vec(); self.database.store_order_events(&events).await; @@ -583,6 +576,17 @@ struct Solution { id: u64, account: H160, score: NonZeroU256, + orders: HashMap, +} + +impl Solution { + pub fn order_ids(&self) -> impl Iterator { + self.orders.keys() + } + + pub fn orders(&self) -> &HashMap { + &self.orders + } } #[derive(Debug, thiserror::Error)] diff --git a/crates/driver/src/domain/competition/mod.rs b/crates/driver/src/domain/competition/mod.rs index 46ef2cf941..18fd581bb3 100644 --- a/crates/driver/src/domain/competition/mod.rs +++ b/crates/driver/src/domain/competition/mod.rs @@ -15,7 +15,10 @@ use { }, futures::{stream::FuturesUnordered, Stream, StreamExt}, itertools::Itertools, - std::{collections::HashSet, sync::Mutex}, + std::{ + collections::{HashMap, HashSet}, + sync::Mutex, + }, tap::TapFallible, }; @@ -171,7 +174,15 @@ impl Competition { let (mut score, settlement) = scores .into_iter() .max_by_key(|(score, _)| score.to_owned()) - .map(|(score, settlement)| (Solved { score }, settlement)) + .map(|(score, settlement)| { + ( + Solved { + score, + trades: settlement.orders(), + }, + settlement, + ) + }) .unzip(); *self.settlement.lock().unwrap() = settlement.clone(); @@ -217,7 +228,6 @@ impl Competition { .cloned() .ok_or(Error::SolutionNotAvailable)?; Ok(Revealed { - orders: settlement.orders(), internalized_calldata: settlement .calldata( self.eth.contracts().settlement(), @@ -335,6 +345,13 @@ async fn merge_settlements( #[derive(Debug)] pub struct Solved { pub score: Score, + pub trades: HashMap, +} + +#[derive(Debug, Default)] +pub struct Amounts { + pub sell: eth::TokenAmount, + pub buy: eth::TokenAmount, } /// Winning solution information revealed to the protocol by the driver before @@ -342,8 +359,6 @@ pub struct Solved { /// point. #[derive(Debug)] pub struct Revealed { - /// The orders solved by this solution. - pub orders: HashSet, /// The internalized calldata is the final calldata that appears onchain. pub internalized_calldata: Bytes>, /// The uninternalized calldata must be known so that the CoW solver team diff --git a/crates/driver/src/domain/competition/solution/settlement.rs b/crates/driver/src/domain/competition/solution/settlement.rs index 9c46cd22ef..f3a04a136b 100644 --- a/crates/driver/src/domain/competition/solution/settlement.rs +++ b/crates/driver/src/domain/competition/solution/settlement.rs @@ -303,12 +303,28 @@ impl Settlement { self.boundary.solver } - /// The settled user orders. - pub fn orders(&self) -> HashSet { + /// The settled user orders with their in/out amounts. + pub fn orders(&self) -> HashMap { self.solutions .values() - .flat_map(|solution| solution.user_trades().map(|trade| trade.order().uid)) - .collect() + .fold(Default::default(), |mut acc, solution| { + for trade in solution.user_trades() { + let order = acc.entry(trade.order().uid).or_default(); + order.sell = trade.sell_amount(&solution.prices).unwrap_or_else(|| { + // This should never happen, returning 0 is better than panicking, but we + // should still alert. + tracing::error!(uid = ?trade.order().uid, "could not compute sell_amount"); + 0.into() + }); + order.buy = trade.buy_amount(&solution.prices).unwrap_or_else(|| { + // This should never happen, returning 0 is better than panicking, but we + // should still alert. + tracing::error!(uid = ?trade.order().uid, "could not compute buy_amount"); + 0.into() + }); + } + acc + }) } /// Settlements have valid notify ID only if they are originated from a diff --git a/crates/driver/src/domain/competition/solution/trade.rs b/crates/driver/src/domain/competition/solution/trade.rs index 3f653476f3..67038e6299 100644 --- a/crates/driver/src/domain/competition/solution/trade.rs +++ b/crates/driver/src/domain/competition/solution/trade.rs @@ -1,6 +1,9 @@ -use crate::domain::{ - competition::{self, order}, - eth, +use { + crate::domain::{ + competition::{self, order}, + eth, + }, + std::collections::HashMap, }; /// A trade which executes an order as part of this solution. @@ -84,6 +87,40 @@ impl Fulfillment { Fee::Dynamic(fee) => fee, } } + + /// The effective amount that left the user's wallet including all fees. + pub fn sell_amount( + &self, + prices: &HashMap, + ) -> Option { + let before_fee = match self.order.side { + order::Side::Sell => self.executed.0, + order::Side::Buy => self + .executed + .0 + .checked_mul(*prices.get(&self.order.buy.token)?)? + .checked_div(*prices.get(&self.order.sell.token)?)?, + }; + Some(eth::TokenAmount( + before_fee.checked_add(self.solver_fee().0)?, + )) + } + + /// The effective amount the user received after all fees. + pub fn buy_amount( + &self, + prices: &HashMap, + ) -> Option { + let amount = match self.order.side { + order::Side::Buy => self.executed.0, + order::Side::Sell => self + .executed + .0 + .checked_mul(*prices.get(&self.order.sell.token)?)? + .checked_div(*prices.get(&self.order.buy.token)?)?, + }; + Some(eth::TokenAmount(amount)) + } } /// A fee that is charged for executing an order. diff --git a/crates/driver/src/domain/eth/mod.rs b/crates/driver/src/domain/eth/mod.rs index f339be6a19..1800a035b7 100644 --- a/crates/driver/src/domain/eth/mod.rs +++ b/crates/driver/src/domain/eth/mod.rs @@ -192,7 +192,7 @@ impl TokenAddress { /// An ERC20 token amount. /// /// https://eips.ethereum.org/EIPS/eip-20 -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct TokenAmount(pub U256); impl From for TokenAmount { diff --git a/crates/driver/src/infra/api/routes/reveal/dto/revealed.rs b/crates/driver/src/infra/api/routes/reveal/dto/revealed.rs index 0a9bca788e..1f256320b4 100644 --- a/crates/driver/src/infra/api/routes/reveal/dto/revealed.rs +++ b/crates/driver/src/infra/api/routes/reveal/dto/revealed.rs @@ -1,8 +1,5 @@ use { - crate::{ - domain::{competition, competition::order}, - util::serialize, - }, + crate::{domain::competition, util::serialize}, serde::Serialize, serde_with::serde_as, }; @@ -10,7 +7,6 @@ use { impl Revealed { pub fn new(reveal: competition::Revealed) -> Self { Self { - orders: reveal.orders.into_iter().map(Into::into).collect(), calldata: Calldata { internalized: reveal.internalized_calldata.into(), uninternalized: reveal.uninternalized_calldata.into(), @@ -23,8 +19,6 @@ impl Revealed { #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] pub struct Revealed { - #[serde_as(as = "Vec")] - orders: Vec<[u8; order::UID_LEN]>, calldata: Calldata, } diff --git a/crates/driver/src/infra/api/routes/solve/dto/solved.rs b/crates/driver/src/infra/api/routes/solve/dto/solved.rs index c765518baa..ef495c5093 100644 --- a/crates/driver/src/infra/api/routes/solve/dto/solved.rs +++ b/crates/driver/src/infra/api/routes/solve/dto/solved.rs @@ -1,11 +1,12 @@ use { crate::{ - domain::{competition, eth}, + domain::{competition, competition::order, eth}, infra::Solver, util::serialize, }, serde::Serialize, serde_with::serde_as, + std::collections::HashMap, }; impl Solved { @@ -31,10 +32,37 @@ impl Solution { solution_id, score: solved.score.0.get(), submission_address: solver.address().into(), + orders: solved + .trades + .into_iter() + .map(|(order_id, amounts)| { + ( + order_id.into(), + TradedAmounts { + sell_amount: amounts.sell.into(), + buy_amount: amounts.buy.into(), + }, + ) + }) + .collect(), } } } +#[serde_as] +#[derive(Debug, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct TradedAmounts { + /// The effective amount that left the user's wallet including all fees. + #[serde_as(as = "serialize::U256")] + pub sell_amount: eth::U256, + /// The effective amount the user received after all fees. + #[serde_as(as = "serialize::U256")] + pub buy_amount: eth::U256, +} + +type OrderId = [u8; order::UID_LEN]; + #[serde_as] #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] @@ -46,4 +74,6 @@ pub struct Solution { #[serde_as(as = "serialize::U256")] score: eth::U256, submission_address: eth::H160, + #[serde_as(as = "HashMap")] + orders: HashMap, } diff --git a/crates/driver/src/tests/cases/merge_settlements.rs b/crates/driver/src/tests/cases/merge_settlements.rs index 1430cbe2c6..f151f8e522 100644 --- a/crates/driver/src/tests/cases/merge_settlements.rs +++ b/crates/driver/src/tests/cases/merge_settlements.rs @@ -17,11 +17,11 @@ async fn possible() { .done() .await; - test.solve().await.ok(); - test.reveal() + test.solve() .await .ok() .orders(&[ab_order().name, cd_order().name]); + test.reveal().await.ok().calldata(); test.settle() .await // Even though the solver returned two solutions, the executed settlement is a @@ -54,7 +54,7 @@ async fn impossible() { // Only the first A-B order gets settled. - test.solve().await.ok(); - test.reveal().await.ok().orders(&[ab_order().name]); + test.solve().await.ok().orders(&[ab_order().name]); + test.reveal().await.ok().calldata(); test.settle().await.ok().await.ab_order_executed().await; } diff --git a/crates/driver/src/tests/cases/multiple_solutions.rs b/crates/driver/src/tests/cases/multiple_solutions.rs index 1875980bfa..dfac3c90d6 100644 --- a/crates/driver/src/tests/cases/multiple_solutions.rs +++ b/crates/driver/src/tests/cases/multiple_solutions.rs @@ -16,8 +16,12 @@ async fn valid() { .done() .await; - test.solve().await.ok().default_score(); - test.reveal().await.ok().orders(&[ab_order().name]); + test.solve() + .await + .ok() + .default_score() + .orders(&[ab_order().name]); + test.reveal().await.ok().calldata(); } /// Test that the invalid solution is discarded when the /solve endpoint @@ -33,6 +37,10 @@ async fn invalid() { .done() .await; - test.solve().await.ok().default_score(); - test.reveal().await.ok().orders(&[ab_order().name]); + test.solve() + .await + .ok() + .default_score() + .orders(&[ab_order().name]); + test.reveal().await.ok().calldata(); } diff --git a/crates/driver/src/tests/cases/negative_scores.rs b/crates/driver/src/tests/cases/negative_scores.rs index 06caea9ec2..d34c6c8c8d 100644 --- a/crates/driver/src/tests/cases/negative_scores.rs +++ b/crates/driver/src/tests/cases/negative_scores.rs @@ -36,6 +36,10 @@ async fn one_valid_solution() { }) .done() .await; - test.solve().await.ok().default_score(); - test.reveal().await.ok().orders(&[ab_order().name]); + test.solve() + .await + .ok() + .default_score() + .orders(&[ab_order().name]); + test.reveal().await.ok().calldata(); } diff --git a/crates/driver/src/tests/cases/score_competition.rs b/crates/driver/src/tests/cases/score_competition.rs index dbee014190..6ce2c66c2d 100644 --- a/crates/driver/src/tests/cases/score_competition.rs +++ b/crates/driver/src/tests/cases/score_competition.rs @@ -16,11 +16,10 @@ async fn solver_score_winner() { .done() .await; - assert_eq!( - test.solve().await.ok().score(), - 2902421280589416499u128.into() - ); - test.reveal().await.ok().orders(&[ab_order().name]); + let solve = test.solve().await.ok(); + assert_eq!(solve.score(), 2902421280589416499u128.into()); + solve.orders(&[ab_order().name]); + test.reveal().await.ok().calldata(); } #[tokio::test] @@ -36,6 +35,8 @@ async fn risk_adjusted_score_winner() { .done() .await; - assert!(test.solve().await.ok().score() != DEFAULT_SCORE_MIN.into()); - test.reveal().await.ok().orders(&[ab_order().name]); + let solve = test.solve().await.ok(); + assert!(solve.score() != DEFAULT_SCORE_MIN.into()); + solve.orders(&[ab_order().name]); + test.reveal().await.ok().calldata(); } diff --git a/crates/driver/src/tests/setup/mod.rs b/crates/driver/src/tests/setup/mod.rs index 09eef9c4d7..6078a396c3 100644 --- a/crates/driver/src/tests/setup/mod.rs +++ b/crates/driver/src/tests/setup/mod.rs @@ -624,7 +624,12 @@ impl Test { let status = res.status(); let body = res.text().await.unwrap(); tracing::debug!(?status, ?body, "got a response from /solve"); - Solve { status, body } + Solve { + status, + body, + fulfillments: &self.fulfillments, + blockchain: &self.blockchain, + } } /// Call the /reveal endpoint. @@ -643,12 +648,7 @@ impl Test { let status = res.status(); let body = res.text().await.unwrap(); tracing::debug!(?status, ?body, "got a response from /reveal"); - Reveal { - status, - body, - fulfillments: &self.fulfillments, - blockchain: &self.blockchain, - } + Reveal { status, body } } /// Call the /quote endpoint. @@ -745,24 +745,32 @@ impl Test { } /// A /solve response. -pub struct Solve { +pub struct Solve<'a> { status: StatusCode, body: String, + fulfillments: &'a [Fulfillment], + blockchain: &'a Blockchain, } -pub struct SolveOk { +pub struct SolveOk<'a> { body: String, + fulfillments: &'a [Fulfillment], + blockchain: &'a Blockchain, } -impl Solve { +impl<'a> Solve<'a> { /// Expect the /solve endpoint to have returned a 200 OK response. - pub fn ok(self) -> SolveOk { + pub fn ok(self) -> SolveOk<'a> { assert_eq!(self.status, hyper::StatusCode::OK); - SolveOk { body: self.body } + SolveOk { + body: self.body, + fulfillments: self.fulfillments, + blockchain: self.blockchain, + } } } -impl SolveOk { +impl<'a> SolveOk<'a> { fn solutions(&self) -> Vec { #[derive(serde::Deserialize)] struct Body { @@ -771,14 +779,22 @@ impl SolveOk { serde_json::from_str::(&self.body).unwrap().solutions } - /// Extracts the score from the response. Since response can contain - /// multiple solutions, it takes the score from the first solution. - pub fn score(&self) -> eth::U256 { + /// Extracts the first solution from the response. This is expected to be + /// always valid if there is a valid solution, as we expect from driver to + /// not send multiple solutions (yet). + fn solution(&self) -> serde_json::Value { let solutions = self.solutions(); assert_eq!(solutions.len(), 1); let solution = solutions[0].clone(); assert!(solution.is_object()); - assert_eq!(solution.as_object().unwrap().len(), 3); + assert_eq!(solution.as_object().unwrap().len(), 4); + solution + } + + /// Extracts the score from the response. Since response can contain + /// multiple solutions, it takes the score from the first solution. + pub fn score(&self) -> eth::U256 { + let solution = self.solution(); assert!(solution.get("score").is_some()); let score = solution.get("score").unwrap().as_str().unwrap(); eth::U256::from_dec_str(score).unwrap() @@ -804,35 +820,7 @@ impl SolveOk { pub fn empty(self) { assert!(self.solutions().is_empty()); } -} - -/// A /reveal response. -pub struct Reveal<'a> { - status: StatusCode, - body: String, - fulfillments: &'a [Fulfillment], - blockchain: &'a Blockchain, -} -impl<'a> Reveal<'a> { - /// Expect the /reveal endpoint to have returned a 200 OK response. - pub fn ok(self) -> RevealOk<'a> { - assert_eq!(self.status, hyper::StatusCode::OK); - RevealOk { - body: self.body, - fulfillments: self.fulfillments, - blockchain: self.blockchain, - } - } -} - -pub struct RevealOk<'a> { - body: String, - fulfillments: &'a [Fulfillment], - blockchain: &'a Blockchain, -} - -impl RevealOk<'_> { /// Check that the solution contains the expected orders. pub fn orders(self, order_names: &[&str]) -> Self { let expected_order_uids = order_names @@ -853,20 +841,58 @@ impl RevealOk<'_> { }) .sorted() .collect_vec(); + let solution = self.solution(); + assert!(solution.get("orders").is_some()); + let order_uids = serde_json::from_value::>( + solution.get("orders").unwrap().clone(), + ) + .unwrap() + .keys() + .cloned() + .sorted() + .collect_vec(); + assert_eq!(order_uids, expected_order_uids); + self + } +} + +/// A /reveal response. +pub struct Reveal { + status: StatusCode, + body: String, +} + +impl Reveal { + /// Expect the /reveal endpoint to have returned a 200 OK response. + pub fn ok(self) -> RevealOk { + assert_eq!(self.status, hyper::StatusCode::OK); + RevealOk { body: self.body } + } +} + +pub struct RevealOk { + body: String, +} + +impl RevealOk { + pub fn calldata(self) -> Self { let result: serde_json::Value = serde_json::from_str(&self.body).unwrap(); assert!(result.is_object()); - assert_eq!(result.as_object().unwrap().len(), 2); - assert!(result.get("orders").is_some()); - let order_uids = result - .get("orders") + assert_eq!(result.as_object().unwrap().len(), 1); + let calldata = result.get("calldata").unwrap().as_object().unwrap(); + assert_eq!(calldata.len(), 2); + assert!(!calldata + .get("internalized") .unwrap() - .as_array() + .as_str() .unwrap() - .iter() - .map(|order| order.as_str().unwrap().to_owned()) - .sorted() - .collect_vec(); - assert_eq!(order_uids, expected_order_uids); + .is_empty()); + assert!(!calldata + .get("uninternalized") + .unwrap() + .as_str() + .unwrap() + .is_empty()); self } } diff --git a/crates/e2e/tests/e2e/partially_fillable_observed_score.rs b/crates/e2e/tests/e2e/partially_fillable_observed_score.rs index 790fc8e6a6..3357cf6b7c 100644 --- a/crates/e2e/tests/e2e/partially_fillable_observed_score.rs +++ b/crates/e2e/tests/e2e/partially_fillable_observed_score.rs @@ -5,6 +5,7 @@ use { model::{ order::{OrderClass, OrderCreation, OrderKind}, signature::EcdsaSigningScheme, + solver_competition, }, secp256k1::SecretKey, shared::ethrpc::Web3, @@ -164,10 +165,11 @@ async fn test(web3: Web3) { assert!(solution_1.objective.fees > 0.); assert_ne!(solution_0.objective.fees, solution_1.objective.fees); - assert!(solution_0.orders[0].executed_amount > 0.into()); - assert!(solution_1.orders[0].executed_amount > 0.into()); - assert_ne!( - solution_0.orders[0].executed_amount, - solution_1.orders[0].executed_amount + assert!( + matches!(solution_0.orders[0], solver_competition::Order::Legacy{ executed_amount, ..} if executed_amount > 0.into()) ); + assert!( + matches!(solution_1.orders[0], solver_competition::Order::Legacy{ executed_amount, ..} if executed_amount > 0.into()) + ); + assert_ne!(solution_0.orders[0], solution_1.orders[0]); } diff --git a/crates/model/src/solver_competition.rs b/crates/model/src/solver_competition.rs index 9ea9f59a4d..cc45a35a03 100644 --- a/crates/model/src/solver_competition.rs +++ b/crates/model/src/solver_competition.rs @@ -155,12 +155,25 @@ impl Score { } #[serde_as] -#[derive(Clone, Debug, Default, Deserialize, Serialize, Eq, PartialEq)] -#[serde(rename_all = "camelCase")] -pub struct Order { - pub id: OrderUid, - #[serde_as(as = "HexOrDecimalU256")] - pub executed_amount: U256, +#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)] +#[serde(untagged)] +pub enum Order { + #[serde(rename_all = "camelCase")] + Colocated { + id: OrderUid, + /// The effective amount that left the user's wallet including all fees. + #[serde_as(as = "HexOrDecimalU256")] + sell_amount: U256, + /// The effective amount the user received after all fees. + #[serde_as(as = "HexOrDecimalU256")] + buy_amount: U256, + }, + #[serde(rename_all = "camelCase")] + Legacy { + id: OrderUid, + #[serde_as(as = "HexOrDecimalU256")] + executed_amount: U256, + }, } #[cfg(test)] @@ -215,7 +228,12 @@ mod tests { "id": "0x3333333333333333333333333333333333333333333333333333333333333333\ 3333333333333333333333333333333333333333\ 33333333", - "executedAmount": "12", + "sellAmount": "12", + "buyAmount": "13", + }, + { + "id": "0x4444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444", + "executedAmount": "14", } ], "callData": "0x13", @@ -259,10 +277,17 @@ mod tests { clearing_prices: btreemap! { H160([0x22; 20]) => 8.into(), }, - orders: vec![Order { - id: OrderUid([0x33; 56]), - executed_amount: 12.into(), - }], + orders: vec![ + Order::Colocated { + id: OrderUid([0x33; 56]), + sell_amount: 12.into(), + buy_amount: 13.into(), + }, + Order::Legacy { + id: OrderUid([0x44; 56]), + executed_amount: 14.into(), + }, + ], call_data: vec![0x13], uninternalized_call_data: Some(vec![0x13, 0x14]), }], diff --git a/crates/orderbook/src/database/solver_competition.rs b/crates/orderbook/src/database/solver_competition.rs index ca1f541e5d..1207d8f10d 100644 --- a/crates/orderbook/src/database/solver_competition.rs +++ b/crates/orderbook/src/database/solver_competition.rs @@ -184,7 +184,7 @@ mod tests { score: Default::default(), ranking: Some(1), clearing_prices: [Default::default()].into_iter().collect(), - orders: vec![Default::default()], + orders: vec![], call_data: vec![1, 2], uninternalized_call_data: Some(vec![1, 2, 3, 4]), }], diff --git a/crates/solver/src/driver.rs b/crates/solver/src/driver.rs index b65c6544c3..adb871020e 100644 --- a/crates/solver/src/driver.rs +++ b/crates/solver/src/driver.rs @@ -386,7 +386,7 @@ impl Driver { orders: rated_settlement .settlement .trades() - .map(|trade| solver_competition::Order { + .map(|trade| solver_competition::Order::Legacy { id: trade.order.metadata.uid, executed_amount: trade.executed_amount, }) From 1b585b5b08a34fe57e7c53cc6d9514c8daa7d5e6 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Fri, 8 Dec 2023 11:04:07 +0000 Subject: [PATCH 09/11] Simplify network name function (#2136) # Description An alternative to PR #2130. It simplifies the network name function to only include networks we actually use. If a network is unknown, the string name includes both network and chain id. Notably compared to #2130, there are no naming changes. # Changes - Removed unused networks. - Added Sepolia - Added extra information to unknown networks. ## How to test Unknown, as I don't know any protocol using other networks but those I left here. Specifically for Sepolia, you can compare with the autogenerated entry [here](https://github.com/cowprotocol/services/pull/2130/files#diff-6fe1d095c8577cf6c68e84fa7d84e110cad2d1a38b83a3421513e475e9692007R1088). --- crates/shared/src/network.rs | 159 ++--------------------------------- 1 file changed, 7 insertions(+), 152 deletions(-) diff --git a/crates/shared/src/network.rs b/crates/shared/src/network.rs index 4e846d6087..7b4e650b61 100644 --- a/crates/shared/src/network.rs +++ b/crates/shared/src/network.rs @@ -1,162 +1,17 @@ use std::time::Duration; /// Maps (NetworkId, ChainId) to the network name. -/// Contents of the "match" block obtained via: -/// wget -O - -o /dev/null https://chainid.network/chains.json | jq -r '.[] | [.networkId, .chainId, .name] | @tsv' | gawk '{print("(\""$1"\", "$2") => \""$3"\",")}' +/// If the output is from a known network, it represents the canonical name of +/// the network on CoW Protocol. pub fn network_name(network_id: &str, chain_id: u64) -> &'static str { + // You can find a list of available networks by network and chain id here: + // https://chainid.network/chains.json match (network_id, chain_id) { ("1", 1) => "Ethereum / Mainnet", - ("10", 10) => "Optimistic", - ("100", 100) => "xDAI", - ("10000", 10000) => "Smart", - ("10001", 10001) => "Smart", - ("1001", 1001) => "Klaytn", - ("1007", 1007) => "Newton", - ("1", 101) => "EtherInc", - ("1010", 1010) => "Evrice", - ("1012", 1012) => "Newton", - ("102", 102) => "Web3Games", - ("1023", 1023) => "Clover", - ("1024", 1024) => "Clover", - ("108", 108) => "ThunderCore", - ("11", 11) => "Metadium", - ("1122334455", 1122334455) => "IPOS", - ("1139", 1139) => "MathChain", - ("1140", 1140) => "MathChain", - ("12", 12) => "Metadium", - ("122", 122) => "Fuse", - ("128", 128) => "Huobi", - ("1284", 1284) => "Moonbeam", - ("1285", 1285) => "Moonriver", - ("1286", 1286) => "Moonrock", - ("1287", 1287) => "Moonbeam", - ("13", 13) => "Diode", - ("1313114", 1313114) => "Ether-1", - ("1313161554", 1313161554) => "NEAR", - ("1313161555", 1313161555) => "NEAR", - ("1313161556", 1313161556) => "NEAR", - ("1313500", 1313500) => "Xerom", - ("13371337", 13371337) => "PepChain", - ("137", 137) => "Matic", - ("14", 14) => "Flare", - ("15", 15) => "Diode", - ("16", 16) => "Flare", - ("162", 162) => "Lightstreams", - ("11235813", 1620) => "Atheios", - ("163", 163) => "Lightstreams", - ("1666600000", 1666600000) => "Harmony", - ("1666600001", 1666600001) => "Harmony", - ("1666600002", 1666600002) => "Harmony", - ("1666600003", 1666600003) => "Harmony", - ("1666700000", 1666700000) => "Harmony", - ("1666700001", 1666700001) => "Harmony", - ("1666700002", 1666700002) => "Harmony", - ("1666700003", 1666700003) => "Harmony", - ("17", 17) => "ThaiChain", - ("170", 170) => "HOO", - ("18", 18) => "ThunderCore", - ("18289463", 18289463) => "IOLite", - ("1", 1856) => "Teslafunds", - ("1987", 1987) => "EtherGem", - ("1", 2) => "Expanse", - ("20", 20) => "ELA-ETH-Sidechain", - ("200625", 200625) => "Akroma", - ("2020", 2020) => "420coin", - ("2021", 2021) => "Edgeware", - ("2022", 2022) => "Beresheet", - ("21", 21) => "ELA-ETH-Sidechain", - ("0", 211) => "Freight", - ("22", 22) => "ELA-DID-Sidechain", - ("23", 23) => "ELA-DID-Sidechain", - ("37129", 24484) => "Webchain", - ("246", 246) => "Energy", - ("246529", 246529) => "ARTIS", - ("246785", 246785) => "ARTIS", - ("37480", 24734) => "MintMe.com", - ("250", 250) => "Fantom", - ("256", 256) => "Huobi", - ("100", 269) => "High", - ("28945486", 28945486) => "Auxilium", - ("3", 3) => "Ethereum / Ropsten", - ("30", 30) => "RSK", - ("31", 31) => "RSK", - ("1", 31102) => "Ethersocial", - ("3125659152", 3125659152) => "Pirl", - ("322", 322) => "KuCoin", - ("32659", 32659) => "Fusion", - ("33", 33) => "GoodData", - ("35", 35) => "TBWG", - ("35855456", 35855456) => "Joys", - ("38", 38) => "Valorbit", - ("385", 385) => "Lisinski", - ("39797", 39797) => "Energi", - ("4", 4) => "Ethereum / Rinkeby", - ("40", 40) => "Telos", - ("41", 41) => "Telos", - ("42", 42) => "Ethereum / Kovan", - ("420", 420) => "Optimistic", - ("42069", 42069) => "pegglecoin", - ("42220", 42220) => "Celo", - ("43", 43) => "Darwinia", - ("43110", 43110) => "Athereum", - ("1", 43113) => "Avalanche", - ("1", 43114) => "Avalanche", - ("44", 44) => "Darwinia", - ("44787", 44787) => "Celo", - ("4689", 4689) => "IoTeX", - ("4690", 4690) => "IoTeX", - ("49797", 49797) => "Energi", - ("499", 499) => "Rupaya", ("5", 5) => "Ethereum / Goerli", - ("50", 50) => "XinFin", - ("51", 51) => "XinFin", - ("52", 52) => "CoinEx", - ("53", 53) => "CoinEx", - ("558", 558) => "Tao", - ("56", 56) => "Binance", - ("5869", 5869) => "Wegochain", - ("595", 595) => "Acala", - ("6", 6) => "Ethereum Classic / Kotti", - ("60", 60) => "GoChain", - ("1", 61) => "Ethereum Classic / Mainnet", - ("61717561", 61717561) => "Aquachain", - ("2", 62) => "Ethereum Classic / Morden", - ("62320", 62320) => "Celo", - ("7", 63) => "Ethereum Classic / Mordor", - ("64", 64) => "Ellaism", - ("65", 65) => "OKExChain", - ("66", 66) => "OKExChain", - ("67", 67) => "DBChain", - ("68", 68) => "SoterOne", - ("686", 686) => "Karura", - ("69", 69) => "Optimistic", - ("7", 7) => "ThaiChain", - ("73799", 73799) => "Energy", - ("76", 76) => "Mix", - ("77", 77) => "POA", - ("7762959", 7762959) => "Musicoin", - ("777", 777) => "Ethermint", - ("78", 78) => "PrimusChain", - ("78110", 78110) => "Firenze", - ("787", 787) => "Acala", - ("88", 8) => "Ubiq", - ("80001", 80001) => "Matic", - ("8029", 8029) => "MDGL", - ("82", 82) => "Meter", - ("1", 820) => "Callisto", - ("2", 821) => "Callisto", - ("8217", 8217) => "Klaytn", - ("8285", 8285) => "KorthoTest", - ("88", 88) => "TomoChain", - ("888", 888) => "Wanchain", - ("8995", 8995) => "bloxberg", - ("2", 9) => "Ubiq", - ("97", 97) => "Binance", - ("977", 977) => "Nepal", - ("99", 99) => "POA", - ("99415706", 99415706) => "Joys", - ("999", 999) => "Wanchain", - _ => "", + ("100", 100) => "xDAI", + ("11155111", 11155111) => "Ethereum / Sepolia", + _ => panic!("Unknown network (network_id={network_id}, chain_id={chain_id})"), } } From f7fec64b6679f7508a920ddce867a40266d53346 Mon Sep 17 00:00:00 2001 From: Felix Leupold Date: Fri, 8 Dec 2023 18:05:55 +0100 Subject: [PATCH 10/11] [Easy] Fix executed amount reporting for native ETH (#2148) # Description We are getting a lot of alerts a la > 2023-12-08T12:36:52.017Z ERROR request{id="3681"}:/solve{solver=oneinch-solve auction_id=8127130}: driver::domain::competition::solution::settlement: could not compute buy_amount uid=Uid(0xad576385f272c5363324bfed7b925141dd9f93dcd1c6f9569620c8e09c87c1d6743dbd073d951bc1e7ee276eb79a285595993d63657314b5) for order UIDs that are buying ETH. This is because those are buying the placeholder address `0xe...e` which isn't part of the price vector. Instead this address needs to be converted to the wrapped token equivalent. # Changes - [ ] Also pass Weth address into the sell/buy_amount functions ## How to test No more alerts --- .../domain/competition/solution/settlement.rs | 4 +- .../src/domain/competition/solution/trade.rs | 10 ++-- crates/driver/src/tests/cases/buy_eth.rs | 2 +- crates/driver/src/tests/setup/mod.rs | 50 +++++++++---------- 4 files changed, 33 insertions(+), 33 deletions(-) diff --git a/crates/driver/src/domain/competition/solution/settlement.rs b/crates/driver/src/domain/competition/solution/settlement.rs index f3a04a136b..b1762ac468 100644 --- a/crates/driver/src/domain/competition/solution/settlement.rs +++ b/crates/driver/src/domain/competition/solution/settlement.rs @@ -310,13 +310,13 @@ impl Settlement { .fold(Default::default(), |mut acc, solution| { for trade in solution.user_trades() { let order = acc.entry(trade.order().uid).or_default(); - order.sell = trade.sell_amount(&solution.prices).unwrap_or_else(|| { + order.sell = trade.sell_amount(&solution.prices, solution.weth).unwrap_or_else(|| { // This should never happen, returning 0 is better than panicking, but we // should still alert. tracing::error!(uid = ?trade.order().uid, "could not compute sell_amount"); 0.into() }); - order.buy = trade.buy_amount(&solution.prices).unwrap_or_else(|| { + order.buy = trade.buy_amount(&solution.prices, solution.weth).unwrap_or_else(|| { // This should never happen, returning 0 is better than panicking, but we // should still alert. tracing::error!(uid = ?trade.order().uid, "could not compute buy_amount"); diff --git a/crates/driver/src/domain/competition/solution/trade.rs b/crates/driver/src/domain/competition/solution/trade.rs index 67038e6299..ac4d1c4f9f 100644 --- a/crates/driver/src/domain/competition/solution/trade.rs +++ b/crates/driver/src/domain/competition/solution/trade.rs @@ -92,14 +92,15 @@ impl Fulfillment { pub fn sell_amount( &self, prices: &HashMap, + weth: eth::WethAddress, ) -> Option { let before_fee = match self.order.side { order::Side::Sell => self.executed.0, order::Side::Buy => self .executed .0 - .checked_mul(*prices.get(&self.order.buy.token)?)? - .checked_div(*prices.get(&self.order.sell.token)?)?, + .checked_mul(*prices.get(&self.order.buy.token.wrap(weth))?)? + .checked_div(*prices.get(&self.order.sell.token.wrap(weth))?)?, }; Some(eth::TokenAmount( before_fee.checked_add(self.solver_fee().0)?, @@ -110,14 +111,15 @@ impl Fulfillment { pub fn buy_amount( &self, prices: &HashMap, + weth: eth::WethAddress, ) -> Option { let amount = match self.order.side { order::Side::Buy => self.executed.0, order::Side::Sell => self .executed .0 - .checked_mul(*prices.get(&self.order.sell.token)?)? - .checked_div(*prices.get(&self.order.buy.token)?)?, + .checked_mul(*prices.get(&self.order.sell.token.wrap(weth))?)? + .checked_div(*prices.get(&self.order.buy.token.wrap(weth))?)?, }; Some(eth::TokenAmount(amount)) } diff --git a/crates/driver/src/tests/cases/buy_eth.rs b/crates/driver/src/tests/cases/buy_eth.rs index e1314a82d6..a3541c91f9 100644 --- a/crates/driver/src/tests/cases/buy_eth.rs +++ b/crates/driver/src/tests/cases/buy_eth.rs @@ -14,6 +14,6 @@ async fn test() { .done() .await; - test.solve().await.ok(); + test.solve().await.ok().orders(&[eth_order().name]); test.settle().await.ok().await.eth_order_executed().await; } diff --git a/crates/driver/src/tests/setup/mod.rs b/crates/driver/src/tests/setup/mod.rs index 6078a396c3..3df34c8579 100644 --- a/crates/driver/src/tests/setup/mod.rs +++ b/crates/driver/src/tests/setup/mod.rs @@ -24,7 +24,6 @@ use { }, ethcontract::BlockId, hyper::StatusCode, - itertools::Itertools, secp256k1::SecretKey, serde_with::serde_as, std::{ @@ -823,35 +822,34 @@ impl<'a> SolveOk<'a> { /// Check that the solution contains the expected orders. pub fn orders(self, order_names: &[&str]) -> Self { - let expected_order_uids = order_names - .iter() - .map(|name| { - self.fulfillments - .iter() - .find(|f| f.quoted_order.order.name == *name) - .unwrap_or_else(|| { - panic!( - "unexpected orders {order_names:?}: fulfillment not found in {:?}", - self.fulfillments, - ) - }) - .quoted_order - .order_uid(self.blockchain) - .to_string() - }) - .sorted() - .collect_vec(); let solution = self.solution(); assert!(solution.get("orders").is_some()); - let order_uids = serde_json::from_value::>( + let trades = serde_json::from_value::>( solution.get("orders").unwrap().clone(), ) - .unwrap() - .keys() - .cloned() - .sorted() - .collect_vec(); - assert_eq!(order_uids, expected_order_uids); + .unwrap(); + + for expected in order_names.iter().map(|name| { + self.fulfillments + .iter() + .find(|f| f.quoted_order.order.name == *name) + .unwrap_or_else(|| { + panic!( + "unexpected orders {order_names:?}: fulfillment not found in {:?}", + self.fulfillments, + ) + }) + }) { + let uid = expected.quoted_order.order_uid(self.blockchain); + let trade = trades + .get(&uid.to_string()) + .expect("Didn't find expected trade in solution"); + let u256 = |value: &serde_json::Value| { + eth::U256::from_dec_str(value.as_str().unwrap()).unwrap() + }; + assert!(u256(trade.get("buyAmount").unwrap()) == expected.quoted_order.buy); + assert!(u256(trade.get("sellAmount").unwrap()) == expected.quoted_order.sell); + } self } } From 54da1685dd2460b2100fc05e8c305b230dd537ad Mon Sep 17 00:00:00 2001 From: Felix Leupold Date: Fri, 8 Dec 2023 18:45:15 +0100 Subject: [PATCH 11/11] Report metrics for matched but unsettled orderes (#2147) # Description Implements logic similar to what we currently have in the solver crate to track how many orders were part of a solution that didn't end up winning. This can be used to populate alerts like [this one](https://g-0263500beb.grafana-workspace.eu-central-1.amazonaws.com/d/oPiE1Ennz/alerts-prod?editPanel=10&tab=query&orgId=1). https://github.com/cowprotocol/services/blob/1b585b5b08a34fe57e7c53cc6d9514c8daa7d5e6/crates/solver/src/analytics.rs#L19-L47 # Changes - [ ] Record the same metrics that used to be populated in the legacy solver/driver in the autopilot ## How to test Observice Grafana --- crates/autopilot/src/run_loop.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 9e983e5ab6..176f12c532 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -293,6 +293,12 @@ impl RunLoop { tracing::warn!(?err, driver = %driver.name, "settlement failed"); } } + let unsettled_orders: Vec<_> = solutions + .iter() + .flat_map(|p| p.solution.orders.keys()) + .filter(|uid| !solution.orders.contains_key(uid)) + .collect(); + Metrics::matched_unsettled(driver, unsettled_orders.as_slice()); } } @@ -638,6 +644,11 @@ struct Metrics { /// Tracks the result of driver `/settle` requests. #[metric(labels("driver", "result"))] settle: prometheus::IntCounterVec, + + /// Tracks the number of orders that were part of some but not the winning + /// solution together with the winning driver that did't include it. + #[metric(labels("ignored_by"))] + matched_unsettled: prometheus::IntCounterVec, } impl Metrics { @@ -716,4 +727,14 @@ impl Metrics { .with_label_values(&[&driver.name, label]) .inc(); } + + fn matched_unsettled(winning: &Driver, unsettled: &[&OrderUid]) { + if !unsettled.is_empty() { + tracing::debug!(?unsettled, "some orders were matched but not settled"); + } + Self::get() + .matched_unsettled + .with_label_values(&[&winning.name]) + .inc_by(unsettled.len() as u64); + } }