From 80e0acbc45038fb3131c711dd8fff70b461f416b Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Wed, 10 Jan 2024 18:26:56 +0100 Subject: [PATCH 01/16] First filter invalid signatures (#2263) # Description Follow up for https://github.com/cowprotocol/services/pull/2220 First filter out orders with invalid signatures so we don't have to call bad token detection on them. --- crates/autopilot/src/solvable_orders.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/autopilot/src/solvable_orders.rs b/crates/autopilot/src/solvable_orders.rs index 4739cf507d..51d36be6c8 100644 --- a/crates/autopilot/src/solvable_orders.rs +++ b/crates/autopilot/src/solvable_orders.rs @@ -151,15 +151,15 @@ impl SolvableOrdersCache { let removed = counter.checkpoint("banned_user", &orders); invalid_order_uids.extend(removed); - let orders = filter_unsupported_tokens(orders, self.bad_token_detector.as_ref()).await?; - let removed = counter.checkpoint("unsupported_token", &orders); - invalid_order_uids.extend(removed); - let orders = filter_invalid_signature_orders(orders, self.signature_validator.as_ref()).await; let removed = counter.checkpoint("invalid_signature", &orders); invalid_order_uids.extend(removed); + let orders = filter_unsupported_tokens(orders, self.bad_token_detector.as_ref()).await?; + let removed = counter.checkpoint("unsupported_token", &orders); + invalid_order_uids.extend(removed); + let missing_queries: Vec<_> = orders.iter().map(Query::from_order).collect(); let fetched_balances = self.balance_fetcher.get_balances(&missing_queries).await; let balances = missing_queries From 197fdf5fe053b9a58ac63b1927950ac59311c54a Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Thu, 11 Jan 2024 10:30:47 +0100 Subject: [PATCH 02/16] Remove AuctionId::Centralized (#2264) # Description Further simplifies `OnSettlementEventUpdater` -> removes AuctionId::Centralized variant as we no longer expect for this component to ever reindex events from before colocation activation. Also led to cleaning up the unused functions for `auction_transaction` table. Which again led to cleaning up the solver competition model. --- .../src/database/auction_transaction.rs | 15 -- .../database/on_settlement_event_updater.rs | 41 +---- .../src/on_settlement_event_updater.rs | 30 +--- crates/database/src/auction_transaction.rs | 113 -------------- crates/database/src/solver_competition.rs | 2 +- crates/model/src/solver_competition.rs | 55 +------ .../src/database/solver_competition.rs | 142 +---------------- crates/orderbook/src/solver_competition.rs | 3 - crates/solver/src/driver.rs | 144 +----------------- .../solver/src/driver/solver_settlements.rs | 3 +- crates/solver/src/run.rs | 7 - 11 files changed, 20 insertions(+), 535 deletions(-) diff --git a/crates/autopilot/src/database/auction_transaction.rs b/crates/autopilot/src/database/auction_transaction.rs index 02e2511b6c..96cccc3edf 100644 --- a/crates/autopilot/src/database/auction_transaction.rs +++ b/crates/autopilot/src/database/auction_transaction.rs @@ -47,19 +47,4 @@ impl super::Postgres { ) .await } - - pub async fn get_auction_id( - &self, - tx_from: H160, - tx_nonce: i64, - ) -> Result, sqlx::Error> { - let _timer = super::Metrics::get() - .database_queries - .with_label_values(&["get_auction_id"]) - .start_timer(); - - let mut ex = self.pool.acquire().await?; - database::auction_transaction::get_auction_id(&mut ex, &ByteArray(tx_from.0), tx_nonce) - .await - } } diff --git a/crates/autopilot/src/database/on_settlement_event_updater.rs b/crates/autopilot/src/database/on_settlement_event_updater.rs index 64e34674c9..ff1760c179 100644 --- a/crates/autopilot/src/database/on_settlement_event_updater.rs +++ b/crates/autopilot/src/database/on_settlement_event_updater.rs @@ -9,6 +9,7 @@ use { /// Executed network fee for the gas costs. This fee is solver determined. type ExecutedFee = U256; +pub type AuctionId = i64; #[derive(Debug, Default, Clone)] pub struct AuctionData { @@ -20,40 +21,6 @@ pub struct AuctionData { pub order_executions: Vec<(OrderUid, ExecutedFee)>, } -#[derive(Debug, Clone)] -pub enum AuctionId { - /// We were able to recover this ID from our DB which means that it was - /// submitted by us when the protocol was not yet using colocated - /// drivers. This ID can therefore be trusted. - Centralized(i64), - /// This ID had to be recovered from the calldata of a settlement call. That - /// means it was submitted by a colocated driver. Because these drivers - /// could submit a solution at any time and with wrong or malicious IDs - /// this can not be trusted. For DB updates that modify existing - /// data based on these IDs we have to ensure they can only be executed once - /// (the first time we see this ID). That is required to prevent - /// malicious drivers from overwriting data for already settled - /// auctions. - Colocated(i64), -} - -impl AuctionId { - /// Returns the underlying `auction_id` assuming the caller verified that - /// the next DB update will not run into problems with this ID. - pub fn assume_verified(&self) -> i64 { - match &self { - Self::Centralized(id) => *id, - Self::Colocated(id) => *id, - } - } -} - -impl Default for AuctionId { - fn default() -> Self { - Self::Colocated(0) - } -} - #[derive(Debug, Default, Clone)] pub struct SettlementUpdate { pub block_number: i64, @@ -90,7 +57,7 @@ impl super::Postgres { // solutions. let insert_succesful = database::auction_transaction::try_insert_auction_transaction( ex, - auction_data.auction_id.assume_verified(), + auction_data.auction_id, &ByteArray(settlement_update.tx_from.0), settlement_update.tx_nonce, ) @@ -113,12 +80,12 @@ impl super::Postgres { .await .context("insert_settlement_observations")?; - if insert_succesful || matches!(auction_data.auction_id, AuctionId::Centralized(_)) { + if insert_succesful { for (order, executed_fee) in auction_data.order_executions { database::order_execution::save( ex, &ByteArray(order.0), - auction_data.auction_id.assume_verified(), + auction_data.auction_id, &u256_to_big_decimal(&executed_fee), ) .await diff --git a/crates/autopilot/src/on_settlement_event_updater.rs b/crates/autopilot/src/on_settlement_event_updater.rs index 57371e8c46..120cec41dc 100644 --- a/crates/autopilot/src/on_settlement_event_updater.rs +++ b/crates/autopilot/src/on_settlement_event_updater.rs @@ -33,14 +33,13 @@ use { crate::{ database::{ - on_settlement_event_updater::{AuctionData, AuctionId, SettlementUpdate}, + on_settlement_event_updater::{AuctionData, SettlementUpdate}, Postgres, }, decoded_settlement::{DecodedSettlement, DecodingError}, }, anyhow::{anyhow, Context, Result}, contracts::GPv2Settlement, - database::byte_array::ByteArray, ethrpc::{ current_block::{into_stream, CurrentBlockStream}, Web3, @@ -135,21 +134,9 @@ impl OnSettlementEventUpdater { .with_context(|| format!("convert nonce {hash:?}"))?; let domain_separator = DomainSeparator(self.contract.domain_separator().call().await?.0); - - let mut auction_id = + let auction_id = Self::recover_auction_id_from_calldata(&mut ex, &transaction, &domain_separator) - .await? - .map(AuctionId::Colocated); - if auction_id.is_none() { - // This settlement was issued BEFORE solver-driver colocation. - auction_id = database::auction_transaction::get_auction_id( - &mut ex, - &ByteArray(tx_from.0), - tx_nonce, - ) - .await? - .map(AuctionId::Centralized); - } + .await?; let mut update = SettlementUpdate { block_number: event.block_number, @@ -178,12 +165,11 @@ impl OnSettlementEventUpdater { let effective_gas_price = receipt .effective_gas_price .with_context(|| format!("no effective gas price {hash:?}"))?; - let auction_external_prices = - Postgres::get_auction_prices(&mut ex, auction_id.assume_verified()) - .await - .with_context(|| { - format!("no external prices for auction id {auction_id:?} and tx {hash:?}") - })?; + let auction_external_prices = Postgres::get_auction_prices(&mut ex, auction_id) + .await + .with_context(|| { + format!("no external prices for auction id {auction_id:?} and tx {hash:?}") + })?; let external_prices = ExternalPrices::try_from_auction_prices( self.native_token, auction_external_prices.clone(), diff --git a/crates/database/src/auction_transaction.rs b/crates/database/src/auction_transaction.rs index 9dc6ba431c..5555cf66dd 100644 --- a/crates/database/src/auction_transaction.rs +++ b/crates/database/src/auction_transaction.rs @@ -3,29 +3,6 @@ use { sqlx::{postgres::PgQueryResult, PgConnection}, }; -/// "upsert" because we might have previously unsuccessfully attempted to settle -/// an auction with the same address-nonce. -pub async fn upsert_auction_transaction( - ex: &mut PgConnection, - auction_id: AuctionId, - tx_from: &Address, - tx_nonce: i64, -) -> Result<(), sqlx::Error> { - const QUERY: &str = r#" -INSERT INTO auction_transaction (auction_id, tx_from, tx_nonce) -VALUES ($1, $2, $3) -ON CONFLICT (tx_from, tx_nonce) DO UPDATE -SET auction_id = EXCLUDED.auction_id - ;"#; - sqlx::query(QUERY) - .bind(auction_id) - .bind(tx_from) - .bind(tx_nonce) - .execute(ex) - .await?; - Ok(()) -} - /// Inserts a row **iff** we don't have an entry for the given `auction_id` yet. /// This is useful to associate a settlement transaction coming from a colocated /// driver with an auction. @@ -97,21 +74,6 @@ LIMIT 1 .await } -pub async fn get_auction_id( - ex: &mut PgConnection, - tx_from: &Address, - tx_nonce: i64, -) -> Result, sqlx::Error> { - const QUERY: &str = - r#"SELECT auction_id FROM auction_transaction WHERE tx_from = $1 AND tx_nonce = $2;"#; - let auction = sqlx::query_scalar(QUERY) - .bind(tx_from) - .bind(tx_nonce) - .fetch_optional(ex) - .await?; - Ok(auction) -} - pub async fn data_exists(ex: &mut PgConnection, auction_id: i64) -> Result { const QUERY: &str = r#"SELECT COUNT(*) FROM auction_transaction WHERE auction_id = $1;"#; let count: i64 = sqlx::query_scalar(QUERY) @@ -130,63 +92,6 @@ mod tests { std::ops::DerefMut, }; - fn is_duplicate_auction_id_error(err: &sqlx::Error) -> bool { - match err { - sqlx::Error::Database(err) => err.constraint() == Some("auction_transaction_pkey"), - _ => false, - } - } - - #[tokio::test] - #[ignore] - async fn postgres_double_insert_error() { - let mut db = PgConnection::connect("postgresql://").await.unwrap(); - let mut db = db.begin().await.unwrap(); - crate::clear_DANGER_(&mut db).await.unwrap(); - - upsert_auction_transaction(&mut db, 0, &Default::default(), 0) - .await - .unwrap(); - - // Doesn't error because whole row is the same. - upsert_auction_transaction(&mut db, 0, &Default::default(), 0) - .await - .unwrap(); - - // Errors because of primary key violation. - let err = upsert_auction_transaction(&mut db, 0, &Default::default(), 1) - .await - .unwrap_err(); - assert!(is_duplicate_auction_id_error(&err)); - } - - #[tokio::test] - #[ignore] - async fn upsert_auction_transaction_() { - let mut db = PgConnection::connect("postgresql://").await.unwrap(); - let mut db = db.begin().await.unwrap(); - crate::clear_DANGER_(&mut db).await.unwrap(); - - upsert_auction_transaction(&mut db, 0, &Default::default(), 0) - .await - .unwrap(); - - // same account-nonce other auction_id - upsert_auction_transaction(&mut db, 1, &Default::default(), 0) - .await - .unwrap(); - - let auction_id: i64 = sqlx::query_scalar("SELECT auction_id FROM auction_transaction") - .fetch_one(db.deref_mut()) - .await - .unwrap(); - assert_eq!(auction_id, 1); - - // reusing auction-id fails - let result = upsert_auction_transaction(&mut db, 1, &Default::default(), 1).await; - assert!(result.is_err()); - } - #[tokio::test] #[ignore] async fn insert_settlement_tx_info_() { @@ -280,24 +185,6 @@ mod tests { assert!(event.is_none()); } - #[tokio::test] - #[ignore] - async fn get_auction_id_test() { - let mut db = PgConnection::connect("postgresql://").await.unwrap(); - let mut db = db.begin().await.unwrap(); - crate::clear_DANGER_(&mut db).await.unwrap(); - - upsert_auction_transaction(&mut db, 5, &Default::default(), 3) - .await - .unwrap(); - - let auction_id = get_auction_id(&mut db, &Default::default(), 3) - .await - .unwrap() - .unwrap(); - assert_eq!(auction_id, 5); - } - #[tokio::test] #[ignore] async fn try_insert_auction_transaction_test() { diff --git a/crates/database/src/solver_competition.rs b/crates/database/src/solver_competition.rs index 17172ef37b..4c2ee98fc4 100644 --- a/crates/database/src/solver_competition.rs +++ b/crates/database/src/solver_competition.rs @@ -139,7 +139,7 @@ mod tests { .await .unwrap(); - crate::auction_transaction::upsert_auction_transaction(&mut db, id, &tx_from, tx_nonce) + crate::auction_transaction::try_insert_auction_transaction(&mut db, id, &tx_from, tx_nonce) .await .unwrap(); diff --git a/crates/model/src/solver_competition.rs b/crates/model/src/solver_competition.rs index a59d4bdd5e..6aa53c2bd6 100644 --- a/crates/model/src/solver_competition.rs +++ b/crates/model/src/solver_competition.rs @@ -4,52 +4,9 @@ use { primitive_types::{H160, H256, U256}, serde::{Deserialize, Serialize}, serde_with::serde_as, - std::collections::{BTreeMap, HashSet}, + std::collections::BTreeMap, }; -/// As a temporary measure the driver informs the api about per competition data -/// that should be stored in the database. This goes to the api through an -/// unlisted and authenticated http endpoint because we do not want the driver -/// to have a database connection. Once autopilot is handling the competition -/// this will no longer be needed. -#[serde_as] -#[derive(Clone, Debug, Default, Deserialize, Serialize)] -pub struct Request { - pub auction: AuctionId, - pub transaction: Transaction, - pub competition: SolverCompetitionDB, - pub executions: Vec<(OrderUid, Execution)>, - pub scores: Scores, - pub participants: HashSet, // solver addresses - pub prices: BTreeMap, // external prices for auction -} - -#[serde_as] -#[derive(Clone, Debug, Default, Deserialize, Serialize)] -pub struct Scores { - pub winner: H160, - #[serde_as(as = "HexOrDecimalU256")] - pub winning_score: U256, - #[serde_as(as = "HexOrDecimalU256")] - pub reference_score: U256, - pub block_deadline: u64, -} - -#[derive(Clone, Debug, Default, Deserialize, Serialize)] -pub struct Transaction { - pub account: H160, - pub nonce: u64, -} - -#[serde_as] -#[derive(Clone, Debug, Default, Deserialize, Serialize)] -pub struct Execution { - #[serde_as(as = "Option")] - pub surplus_fee: Option, - #[serde_as(as = "HexOrDecimalU256")] - pub scoring_fee: U256, -} - /// Stored directly in the database and turned into SolverCompetitionAPI for the /// `/solver_competition` endpoint. #[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] @@ -103,16 +60,6 @@ pub struct SolverSettlement { pub uninternalized_call_data: Option>, } -#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] -#[serde(rename_all = "camelCase")] -pub struct Objective { - pub total: f64, - pub surplus: f64, - pub fees: f64, - pub cost: f64, - pub gas: u64, -} - #[serde_as] #[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq)] pub enum Score { diff --git a/crates/orderbook/src/database/solver_competition.rs b/crates/orderbook/src/database/solver_competition.rs index 0982a4a448..ababfc419c 100644 --- a/crates/orderbook/src/database/solver_competition.rs +++ b/crates/orderbook/src/database/solver_competition.rs @@ -2,17 +2,11 @@ use { super::Postgres, crate::solver_competition::{Identifier, LoadSolverCompetitionError, SolverCompetitionStoring}, anyhow::{Context, Result}, - database::{ - auction_participants::Participant, - auction_prices::AuctionPrice, - byte_array::ByteArray, - settlement_scores::Score, - }, + database::byte_array::ByteArray, model::{ auction::AuctionId, solver_competition::{SolverCompetitionAPI, SolverCompetitionDB}, }, - number::conversions::u256_to_big_decimal, primitive_types::H256, sqlx::types::JsonValue, }; @@ -33,86 +27,6 @@ fn deserialize_solver_competition( #[async_trait::async_trait] impl SolverCompetitionStoring for Postgres { - async fn handle_request(&self, request: model::solver_competition::Request) -> Result<()> { - let json = &serde_json::to_value(&request.competition)?; - - let _timer = super::Metrics::get() - .database_queries - .with_label_values(&["handle_solver_competition_request"]) - .start_timer(); - - let mut ex = self.pool.begin().await.context("begin")?; - - database::solver_competition::save(&mut ex, request.auction, json) - .await - .context("solver_competition::save")?; - - let transaction = request.transaction; - database::auction_transaction::upsert_auction_transaction( - &mut ex, - request.auction, - &ByteArray(transaction.account.0), - transaction.nonce.try_into().context("convert nonce")?, - ) - .await - .context("upsert_auction_transaction")?; - - database::settlement_scores::insert( - &mut ex, - Score { - auction_id: request.auction, - winner: ByteArray(request.scores.winner.0), - winning_score: u256_to_big_decimal(&request.scores.winning_score), - reference_score: u256_to_big_decimal(&request.scores.reference_score), - block_deadline: request - .scores - .block_deadline - .try_into() - .context("convert block deadline")?, - simulation_block: request - .competition - .competition_simulation_block - .try_into() - .context("convert simulation block")?, - }, - ) - .await - .context("settlement_scores::insert")?; - - database::auction_participants::insert( - &mut ex, - request - .participants - .iter() - .map(|p| Participant { - auction_id: request.auction, - participant: ByteArray(p.0), - }) - .collect::>() - .as_slice(), - ) - .await - .context("auction_participants::insert")?; - - database::auction_prices::insert( - &mut ex, - request - .prices - .iter() - .map(|(token, price)| AuctionPrice { - auction_id: request.auction, - token: ByteArray(token.0), - price: u256_to_big_decimal(price), - }) - .collect::>() - .as_slice(), - ) - .await - .context("auction_prices::insert")?; - - ex.commit().await.context("commit") - } - async fn load_competition( &self, id: Identifier, @@ -171,59 +85,7 @@ impl SolverCompetitionStoring for Postgres { #[cfg(test)] mod tests { - use { - super::*, - model::solver_competition::{CompetitionAuction, Scores, SolverSettlement}, - primitive_types::H160, - std::collections::BTreeMap, - }; - - #[tokio::test] - #[ignore] - async fn postgres_solver_competition_roundtrip() { - let db = Postgres::new("postgresql://").unwrap(); - database::clear_DANGER(&db.pool).await.unwrap(); - - let request = model::solver_competition::Request { - auction: 0, - transaction: model::solver_competition::Transaction { - account: H160([7; 20]), - nonce: 8, - }, - competition: SolverCompetitionDB { - auction_start_block: 2, - competition_simulation_block: 4, - auction: CompetitionAuction { - orders: vec![Default::default()], - prices: [Default::default()].into_iter().collect(), - }, - solutions: vec![SolverSettlement { - solver: "asdf".to_string(), - solver_address: H160([1; 20]), - score: Default::default(), - ranking: 1, - clearing_prices: [Default::default()].into_iter().collect(), - orders: vec![], - call_data: Some(vec![1, 2]), - uninternalized_call_data: Some(vec![1, 2, 3, 4]), - }], - }, - executions: Default::default(), - scores: Scores { - winner: H160([1; 20]), - winning_score: 100.into(), - reference_score: 99.into(), - block_deadline: 10, - }, - participants: [H160([1; 20])].into(), - prices: BTreeMap::from([(H160([1; 20]), 1.into())]), - }; - db.handle_request(request.clone()).await.unwrap(); - let actual = db.load_competition(Identifier::Id(0)).await.unwrap(); - assert_eq!(actual.common, request.competition); - assert_eq!(actual.auction_id, 0); - assert_eq!(actual.transaction_hash, None); - } + use super::*; #[tokio::test] #[ignore] diff --git a/crates/orderbook/src/solver_competition.rs b/crates/orderbook/src/solver_competition.rs index 184391d733..85a53d52f9 100644 --- a/crates/orderbook/src/solver_competition.rs +++ b/crates/orderbook/src/solver_competition.rs @@ -17,9 +17,6 @@ pub enum Identifier { #[cfg_attr(test, mockall::automock)] #[async_trait::async_trait] pub trait SolverCompetitionStoring: Send + Sync { - /// Saves a new solver competition entry. - async fn handle_request(&self, request: model::solver_competition::Request) -> Result<()>; - /// Retrieves a solver competition entry by ID. /// /// Returns a `NotFound` error if no solver competition with that ID could diff --git a/crates/solver/src/driver.rs b/crates/solver/src/driver.rs index d7a3f23a20..150111b408 100644 --- a/crates/solver/src/driver.rs +++ b/crates/solver/src/driver.rs @@ -10,11 +10,10 @@ use { settlement::{PriceCheckTokens, Settlement}, settlement_ranker::SettlementRanker, settlement_rater::SettlementRating, - settlement_simulation, settlement_submission::{SolutionSubmitter, SubmissionError}, solver::{Auction, Solver, Solvers}, }, - anyhow::{anyhow, Context, Result}, + anyhow::{Context, Result}, contracts::GPv2Settlement, ethcontract::Account, ethrpc::{current_block::CurrentBlockStream, Web3}, @@ -23,13 +22,6 @@ use { model::{ auction::{AuctionId, AuctionWithId}, order::{Order, OrderUid}, - solver_competition::{ - self, - CompetitionAuction, - Execution, - SolverCompetitionDB, - SolverSettlement, - }, TokenPair, }, num::{rational::Ratio, BigInt}, @@ -37,12 +29,7 @@ use { shared::{ account_balances::BalanceFetching, external_prices::ExternalPrices, - http_solver::model::{ - AuctionResult, - InternalizationStrategy, - SolverRunError, - SubmissionResult, - }, + http_solver::model::{AuctionResult, SolverRunError, SubmissionResult}, recent_block_cache::Block, tenderly_api::TenderlyApi, }, @@ -67,8 +54,6 @@ pub struct Driver { native_token: H160, metrics: Arc, solver_time_limit: Duration, - block_time: Duration, - additional_mining_deadline: Duration, block_stream: CurrentBlockStream, solution_submitter: SolutionSubmitter, run_id: u64, @@ -97,8 +82,6 @@ impl Driver { web3: Web3, network_id: String, solver_time_limit: Duration, - block_time: Duration, - additional_mining_deadline: Duration, skip_non_positive_score_settlements: bool, block_stream: CurrentBlockStream, solution_submitter: SolutionSubmitter, @@ -140,8 +123,6 @@ impl Driver { native_token, metrics, solver_time_limit, - block_time, - additional_mining_deadline, block_stream, solution_submitter, run_id: 0, @@ -263,16 +244,6 @@ impl Driver { self.in_flight_orders.update_and_filter(&mut auction); - let auction_start_block = auction.block; - let competition_auction = CompetitionAuction { - orders: auction - .orders - .iter() - .map(|order| order.metadata.uid) - .collect(), - prices: auction.prices.clone(), - }; - auction.orders.retain(|order| { match ( order.data.partially_fillable, @@ -293,7 +264,6 @@ impl Driver { tracing::info!(count =% auction.orders.len(), "got orders"); self.metrics.orders_fetched(&auction.orders); - let auction_prices = auction.prices.clone(); let external_prices = ExternalPrices::try_from_auction_prices(self.native_token, auction.prices) .context("malformed auction prices")?; @@ -345,53 +315,8 @@ impl Driver { .rank_legal_settlements(run_solver_results, &external_prices, gas_price, auction_id) .await?; - // We don't know the exact block because simulation can happen over multiple - // blocks but this is a good approximation. - let block_during_simulation = self.block_stream.borrow().number; - DriverLogger::print_settlements(&rated_settlements); - // Report solver competition data to the api. - let solver_competition = SolverCompetitionDB { - auction_start_block, - competition_simulation_block: block_during_simulation, - auction: competition_auction, - solutions: rated_settlements - .iter() - .map(|(solver, rated_settlement)| SolverSettlement { - solver: solver.name().to_string(), - solver_address: solver.account().address(), - score: Some(rated_settlement.score), - ranking: rated_settlement.ranking, - clearing_prices: rated_settlement - .settlement - .clearing_prices() - .iter() - .map(|(address, price)| (*address, *price)) - .collect(), - orders: rated_settlement - .settlement - .trades() - .map(|trade| solver_competition::Order::Legacy { - id: trade.order.metadata.uid, - executed_amount: trade.executed_amount, - }) - .collect(), - call_data: Some(settlement_simulation::call_data( - rated_settlement - .settlement - .clone() - .encode(InternalizationStrategy::SkipInternalizableInteraction), // rating is done with internalizations - )), - uninternalized_call_data: rated_settlement - .settlement - .clone() - .encode_uninternalized_if_different() - .map(settlement_simulation::call_data), - }) - .collect(), - }; - let mut settlement_transaction_attempted = false; if let Some((winning_solver, winning_settlement)) = rated_settlements.pop() { tracing::info!( @@ -401,18 +326,6 @@ impl Driver { winning_settlement ); - let executions: Vec<(OrderUid, Execution)> = winning_settlement - .settlement - .user_trades() - .map(|trade| { - let execution = Execution { - surplus_fee: trade.surplus_fee(), - scoring_fee: trade.scoring_fee, - }; - (trade.order.metadata.uid, execution) - }) - .collect(); - let account = winning_solver.account(); let address = account.address(); let nonce = self @@ -421,59 +334,6 @@ impl Driver { .transaction_count(address, None) .await .context("transaction_count")?; - let transaction = model::solver_competition::Transaction { - account: address, - nonce: nonce - .try_into() - .map_err(|err| anyhow!("{err}")) - .context("convert nonce")?, - }; - let scores = model::solver_competition::Scores { - winner: address, - winning_score: winning_settlement.score.score(), - // second highest score, or 0 if there is only one score (see CIP20) - reference_score: rated_settlements - .last() - .map(|(_, settlement)| settlement.score.score()) - .unwrap_or_default(), - block_deadline: { - let deadline = self.solver_time_limit - + self.solution_submitter.max_confirm_time - + self.additional_mining_deadline; - let number_blocks = deadline.as_secs() / self.block_time.as_secs(); - block_during_simulation + number_blocks - }, - }; - let participants = solver_competition - .solutions - .iter() - .map(|solution| solution.solver_address) - .collect(); // to avoid duplicates - - // external prices for all tokens contained in the trades of a settlement - let prices = winning_settlement - .settlement - .trades() - .flat_map(|trade| { - let sell_token = trade.order.data.sell_token; - let buy_token = trade.order.data.buy_token; - let sell_price = auction_prices.get(&sell_token).cloned().unwrap_or_default(); - let buy_price = auction_prices.get(&buy_token).cloned().unwrap_or_default(); - [(sell_token, sell_price), (buy_token, buy_price)] - }) - .collect(); - tracing::debug!(?transaction, "winning solution transaction"); - - let solver_competition = model::solver_competition::Request { - auction: auction_id, - transaction, - competition: solver_competition, - executions, - scores, - participants, - prices, - }; - tracing::debug!(?solver_competition, "submitting competition info"); self.metrics .complete_runloop_until_transaction(start.elapsed()); diff --git a/crates/solver/src/driver/solver_settlements.rs b/crates/solver/src/driver/solver_settlements.rs index 804cc7ab1a..6ef5f5d4e6 100644 --- a/crates/solver/src/driver/solver_settlements.rs +++ b/crates/solver/src/driver/solver_settlements.rs @@ -1,5 +1,6 @@ use { - crate::{driver::solver_competition::Score, settlement::Settlement}, + crate::settlement::Settlement, + model::solver_competition::Score, num::BigRational, primitive_types::U256, }; diff --git a/crates/solver/src/run.rs b/crates/solver/src/run.rs index 84b23c1b76..bad5258512 100644 --- a/crates/solver/src/run.rs +++ b/crates/solver/src/run.rs @@ -548,11 +548,6 @@ pub async fn run(args: Arguments) { http_factory.create(), args.shared.solver_competition_auth.clone(), ); - let network_time_between_blocks = args - .shared - .network_block_interval - .or_else(|| shared::network::block_interval(&network_id, chain_id)) - .expect("unknown network block interval"); let balance_fetcher = account_balances::fetcher( &web3, @@ -576,8 +571,6 @@ pub async fn run(args: Arguments) { web3, network_id, args.solver_time_limit, - network_time_between_blocks, - args.additional_mining_deadline, args.skip_non_positive_score_settlements, current_block_stream.clone(), solution_submitter, From 04b18f67afe8556b2b15db69e533bf3c91451001 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Thu, 11 Jan 2024 20:26:49 +0100 Subject: [PATCH 03/16] Protocol fee adjustment in driver (#2213) # Description Replacement PR for https://github.com/cowprotocol/services/pull/2097 As @fleupold suggested, protocol fee adjustments are done fully in `driver`, in class `Fulfillment`. It goes like this: 1. Solver responds with solution (accounts for surplus_fee only) 2. Driver creates the fulfillment (executed amount + surplus_fee) based on (1) 3. Driver adjusts fulfillment for protocol fees. It - calculates protocol fee - reduces executed amount by protocol fee. (note that this is basically the same as increasing `surplus_fee`) 4. Driver encodes the fulfillment using the `SettlementEncoder` functionality, meaning custom price adjustments will be reused for protocol fee also. ## How to test Added unit tests for 1. sell limit order fok 2. buy limit order fok 3. sell limit order partial (with 3 fulfillments) (but now removed to keep the domain clean) --- .../src/domain/competition/solution/fee.rs | 218 ++++++++++ .../src/domain/competition/solution/mod.rs | 43 +- .../src/domain/competition/solution/trade.rs | 28 +- crates/driver/src/infra/notify/mod.rs | 1 - crates/driver/src/infra/solver/dto/mod.rs | 2 +- .../driver/src/infra/solver/dto/solution.rs | 19 +- crates/driver/src/tests/setup/driver.rs | 16 +- crates/driver/src/tests/setup/mod.rs | 4 +- crates/e2e/tests/e2e/main.rs | 1 + crates/e2e/tests/e2e/protocol_fee.rs | 379 ++++++++++++++++++ 10 files changed, 672 insertions(+), 39 deletions(-) create mode 100644 crates/driver/src/domain/competition/solution/fee.rs create mode 100644 crates/e2e/tests/e2e/protocol_fee.rs diff --git a/crates/driver/src/domain/competition/solution/fee.rs b/crates/driver/src/domain/competition/solution/fee.rs new file mode 100644 index 0000000000..987260e404 --- /dev/null +++ b/crates/driver/src/domain/competition/solution/fee.rs @@ -0,0 +1,218 @@ +//! Applies the protocol fee to the solution received from the solver. +//! +//! Solvers respond differently for the sell and buy orders. +//! +//! EXAMPLES: +//! +//! SELL ORDER +//! Selling 1 WETH for at least `x` amount of USDC. Solvers respond with +//! Fee = 0.05 WETH (always expressed in sell token) +//! Executed = 0.95 WETH (always expressed in target token) +//! +//! This response is adjusted by the protocol fee of 0.1 WETH: +//! Fee = 0.05 WETH + 0.1 WETH = 0.15 WETH +//! Executed = 0.95 WETH - 0.1 WETH = 0.85 WETH +//! +//! BUY ORDER +//! Buying 1 WETH for at most `x` amount of USDC. Solvers respond with +//! Fee = 10 USDC (always expressed in sell token) +//! Executed = 1 WETH (always expressed in target token) +//! +//! This response is adjusted by the protocol fee of 5 USDC: +//! Fee = 10 USDC + 5 USDC = 15 USDC +//! Executed = 1 WETH + +use { + super::trade::{Fee, Fulfillment, InvalidExecutedAmount}, + crate::domain::{ + competition::{ + order, + order::{FeePolicy, Side}, + }, + eth, + }, +}; + +impl Fulfillment { + /// Applies the protocol fee to the existing fulfillment creating a new one. + pub fn with_protocol_fee(&self, prices: ClearingPrices) -> Result { + let protocol_fee = self.protocol_fee(prices)?; + + // Increase the fee by the protocol fee + let fee = match self.surplus_fee() { + None => { + if !protocol_fee.is_zero() { + return Err(Error::ProtocolFeeOnStaticOrder); + } + Fee::Static + } + Some(fee) => { + Fee::Dynamic((fee.0.checked_add(protocol_fee).ok_or(Error::Overflow)?).into()) + } + }; + + // Reduce the executed amount by the protocol fee. This is because solvers are + // unaware of the protocol fee that driver introduces and they only account + // for their own fee. + let order = self.order().clone(); + let executed = match order.side { + order::Side::Buy => self.executed(), + order::Side::Sell => order::TargetAmount( + self.executed() + .0 + .checked_sub(protocol_fee) + .ok_or(Error::Overflow)?, + ), + }; + + Fulfillment::new(order, executed, fee).map_err(Into::into) + } + + fn protocol_fee(&self, prices: ClearingPrices) -> Result { + // TODO: support multiple fee policies + if self.order().fee_policies.len() > 1 { + return Err(Error::MultipleFeePolicies); + } + + match self.order().fee_policies.first() { + Some(FeePolicy::PriceImprovement { + factor, + max_volume_factor, + }) => { + let price_improvement_fee = self.price_improvement_fee(prices, *factor)?; + let max_volume_fee = self.volume_fee(prices, *max_volume_factor)?; + // take the smaller of the two + tracing::debug!(uid=?self.order().uid, price_improvement_fee=?price_improvement_fee, max_volume_fee=?max_volume_fee, protocol_fee=?(std::cmp::min(price_improvement_fee, max_volume_fee)), executed=?self.executed(), surplus_fee=?self.surplus_fee(), "calculated protocol fee"); + Ok(std::cmp::min(price_improvement_fee, max_volume_fee)) + } + Some(FeePolicy::Volume { factor }) => self.volume_fee(prices, *factor), + None => Ok(0.into()), + } + } + + fn price_improvement_fee( + &self, + prices: ClearingPrices, + factor: f64, + ) -> Result { + let sell_amount = self.order().sell.amount.0; + let buy_amount = self.order().buy.amount.0; + let executed = self.executed().0; + let executed_sell_amount = match self.order().side { + Side::Buy => { + // How much `sell_token` we need to sell to buy `executed` amount of `buy_token` + executed + .checked_mul(prices.buy) + .ok_or(Error::Overflow)? + .checked_div(prices.sell) + .ok_or(Error::DivisionByZero)? + } + Side::Sell => executed, + }; + // Sell slightly more `sell_token` to capture the `surplus_fee` + let executed_sell_amount_with_fee = executed_sell_amount + .checked_add( + // surplus_fee is always expressed in sell token + self.surplus_fee() + .map(|fee| fee.0) + .ok_or(Error::ProtocolFeeOnStaticOrder)?, + ) + .ok_or(Error::Overflow)?; + let surplus_in_sell_token = match self.order().side { + Side::Buy => { + // Scale to support partially fillable orders + let limit_sell_amount = sell_amount + .checked_mul(executed) + .ok_or(Error::Overflow)? + .checked_div(buy_amount) + .ok_or(Error::DivisionByZero)?; + // Remaining surplus after fees + // Do not return error if `checked_sub` fails because violated limit prices will + // be caught by simulation + limit_sell_amount + .checked_sub(executed_sell_amount_with_fee) + .unwrap_or(eth::U256::zero()) + } + Side::Sell => { + // Scale to support partially fillable orders + let limit_buy_amount = buy_amount + .checked_mul(executed_sell_amount_with_fee) + .ok_or(Error::Overflow)? + .checked_div(sell_amount) + .ok_or(Error::DivisionByZero)?; + // How much `buy_token` we get for `executed` amount of `sell_token` + let executed_buy_amount = executed + .checked_mul(prices.sell) + .ok_or(Error::Overflow)? + .checked_div(prices.buy) + .ok_or(Error::DivisionByZero)?; + // Remaining surplus after fees + // Do not return error if `checked_sub` fails because violated limit prices will + // be caught by simulation + let surplus = executed_buy_amount + .checked_sub(limit_buy_amount) + .unwrap_or(eth::U256::zero()); + // surplus in sell token + surplus + .checked_mul(prices.buy) + .ok_or(Error::Overflow)? + .checked_div(prices.sell) + .ok_or(Error::DivisionByZero)? + } + }; + apply_factor(surplus_in_sell_token, factor) + } + + fn volume_fee(&self, prices: ClearingPrices, factor: f64) -> Result { + let executed = self.executed().0; + let executed_sell_amount = match self.order().side { + Side::Buy => { + // How much `sell_token` we need to sell to buy `executed` amount of `buy_token` + executed + .checked_mul(prices.buy) + .ok_or(Error::Overflow)? + .checked_div(prices.sell) + .ok_or(Error::DivisionByZero)? + } + Side::Sell => executed, + }; + // Sell slightly more `sell_token` to capture the `surplus_fee` + let executed_sell_amount_with_fee = executed_sell_amount + .checked_add( + // surplus_fee is always expressed in sell token + self.surplus_fee() + .map(|fee| fee.0) + .ok_or(Error::ProtocolFeeOnStaticOrder)?, + ) + .ok_or(Error::Overflow)?; + apply_factor(executed_sell_amount_with_fee, factor) + } +} + +fn apply_factor(amount: eth::U256, factor: f64) -> Result { + Ok(amount + .checked_mul(eth::U256::from_f64_lossy(factor * 10000.)) + .ok_or(Error::Overflow)? + / 10000) +} + +/// Uniform clearing prices at which the trade was executed. +#[derive(Debug, Clone, Copy)] +pub struct ClearingPrices { + pub sell: eth::U256, + pub buy: eth::U256, +} + +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error("orders with non solver determined gas cost fees are not supported")] + ProtocolFeeOnStaticOrder, + #[error("multiple fee policies are not supported yet")] + MultipleFeePolicies, + #[error("overflow error while calculating protocol fee")] + Overflow, + #[error("division by zero error while calculating protocol fee")] + DivisionByZero, + #[error(transparent)] + InvalidExecutedAmount(#[from] InvalidExecutedAmount), +} diff --git a/crates/driver/src/domain/competition/solution/mod.rs b/crates/driver/src/domain/competition/solution/mod.rs index 716c7b994e..8b8b33273c 100644 --- a/crates/driver/src/domain/competition/solution/mod.rs +++ b/crates/driver/src/domain/competition/solution/mod.rs @@ -1,4 +1,5 @@ use { + self::fee::ClearingPrices, crate::{ boundary, domain::{ @@ -18,6 +19,7 @@ use { thiserror::Error, }; +pub mod fee; pub mod interaction; pub mod settlement; pub mod trade; @@ -49,7 +51,7 @@ impl Solution { solver: Solver, score: SolverScore, weth: eth::WethAddress, - ) -> Result { + ) -> Result { let solution = Self { id, trades, @@ -65,9 +67,9 @@ impl Solution { solution.clearing_price(trade.order().sell.token).is_some() && solution.clearing_price(trade.order().buy.token).is_some() }) { - Ok(solution) + Ok(solution.with_protocol_fees()?) } else { - Err(InvalidClearingPrices) + Err(SolutionError::InvalidClearingPrices) } } @@ -176,6 +178,29 @@ impl Solution { Settlement::encode(self, auction, eth, simulator).await } + pub fn with_protocol_fees(self) -> Result { + let mut trades = Vec::with_capacity(self.trades.len()); + for trade in self.trades { + match &trade { + Trade::Fulfillment(fulfillment) => match fulfillment.order().kind { + order::Kind::Market | order::Kind::Limit { .. } => { + let prices = ClearingPrices { + sell: self.prices[&fulfillment.order().sell.token.wrap(self.weth)], + buy: self.prices[&fulfillment.order().buy.token.wrap(self.weth)], + }; + let fulfillment = fulfillment.with_protocol_fee(prices)?; + trades.push(Trade::Fulfillment(fulfillment)) + } + order::Kind::Liquidity => { + trades.push(trade); + } + }, + Trade::Jit(_) => trades.push(trade), + } + } + Ok(Self { trades, ..self }) + } + /// Token prices settled by this solution, expressed using an arbitrary /// reference unit chosen by the solver. These values are only /// meaningful in relation to each others. @@ -279,8 +304,6 @@ pub enum Error { Boundary(#[from] boundary::Error), #[error("simulation error: {0:?}")] Simulation(#[from] simulator::Error), - #[error(transparent)] - Execution(#[from] trade::ExecutionError), #[error( "non bufferable tokens used: solution attempts to internalize tokens which are not trusted" )] @@ -293,6 +316,10 @@ pub enum Error { DifferentSolvers, } -#[derive(Debug, Error)] -#[error("invalid clearing prices")] -pub struct InvalidClearingPrices; +#[derive(Debug, thiserror::Error)] +pub enum SolutionError { + #[error("invalid clearing prices")] + InvalidClearingPrices, + #[error(transparent)] + ProtocolFee(#[from] fee::Error), +} diff --git a/crates/driver/src/domain/competition/solution/trade.rs b/crates/driver/src/domain/competition/solution/trade.rs index 9810ce7232..6fd642430a 100644 --- a/crates/driver/src/domain/competition/solution/trade.rs +++ b/crates/driver/src/domain/competition/solution/trade.rs @@ -35,7 +35,7 @@ impl Fulfillment { // the target amount. Otherwise, the executed amount must be equal to the target // amount. let valid_execution = { - let surplus_fee = match order.side { + let fee = match order.side { order::Side::Buy => order::TargetAmount::default(), order::Side::Sell => order::TargetAmount(match fee { Fee::Static => eth::U256::default(), @@ -43,13 +43,11 @@ impl Fulfillment { }), }; + let executed_with_fee = + order::TargetAmount(executed.0.checked_add(fee.0).ok_or(InvalidExecutedAmount)?); match order.partial { - order::Partial::Yes { available } => { - order::TargetAmount(executed.0 + surplus_fee.0) <= available - } - order::Partial::No => { - order::TargetAmount(executed.0 + surplus_fee.0) == order.target() - } + order::Partial::Yes { available } => executed_with_fee <= available, + order::Partial::No => executed_with_fee == order.target(), } }; @@ -97,6 +95,14 @@ impl Fulfillment { } } + /// Returns the solver determined fee if it exists. + pub fn surplus_fee(&self) -> Option { + match self.fee { + Fee::Static => None, + Fee::Dynamic(fee) => Some(fee), + } + } + /// The effective amount that left the user's wallet including all fees. pub fn sell_amount( &self, @@ -195,11 +201,3 @@ pub struct Execution { #[derive(Debug, thiserror::Error)] #[error("invalid executed amount")] pub struct InvalidExecutedAmount; - -#[derive(Debug, thiserror::Error)] -pub enum ExecutionError { - #[error("overflow error while calculating executed amounts")] - Overflow, - #[error("missing clearing price for {0:?}")] - ClearingPriceMissing(eth::TokenAddress), -} diff --git a/crates/driver/src/infra/notify/mod.rs b/crates/driver/src/infra/notify/mod.rs index cd96a9006c..d87d29bd57 100644 --- a/crates/driver/src/infra/notify/mod.rs +++ b/crates/driver/src/infra/notify/mod.rs @@ -77,7 +77,6 @@ pub fn encoding_failed( simulation_failed(solver, auction_id, solution_id, error, false); return; } - solution::Error::Execution(_) => return, solution::Error::FailingInternalization => return, solution::Error::DifferentSolvers => return, }; diff --git a/crates/driver/src/infra/solver/dto/mod.rs b/crates/driver/src/infra/solver/dto/mod.rs index 0adb77f95e..117d5c16fe 100644 --- a/crates/driver/src/infra/solver/dto/mod.rs +++ b/crates/driver/src/infra/solver/dto/mod.rs @@ -8,4 +8,4 @@ pub use {auction::Auction, notification::Notification, solution::Solutions}; #[derive(Debug, thiserror::Error)] #[error("{0}")] -pub struct Error(pub &'static str); +pub struct Error(pub String); diff --git a/crates/driver/src/infra/solver/dto/solution.rs b/crates/driver/src/infra/solver/dto/solution.rs index 2815667371..9143af46ed 100644 --- a/crates/driver/src/infra/solver/dto/solution.rs +++ b/crates/driver/src/infra/solver/dto/solution.rs @@ -34,7 +34,7 @@ impl Solutions { .find(|order| order.uid == fulfillment.order) // TODO this error should reference the UID .ok_or(super::Error( - "invalid order UID specified in fulfillment" + "invalid order UID specified in fulfillment".to_owned() ))? .clone(); @@ -51,7 +51,7 @@ impl Solutions { .map(competition::solution::Trade::Fulfillment) .map_err( |competition::solution::trade::InvalidExecutedAmount| { - super::Error("invalid trade fulfillment") + super::Error("invalid trade fulfillment".to_owned()) }, ) } @@ -117,7 +117,9 @@ impl Solutions { ) .map_err( |competition::solution::trade::InvalidExecutedAmount| { - super::Error("invalid executed amount in JIT order") + super::Error( + "invalid executed amount in JIT order".to_owned(), + ) }, )?, )), @@ -175,7 +177,7 @@ impl Solutions { .iter() .find(|liquidity| liquidity.id == interaction.id) .ok_or(super::Error( - "invalid liquidity ID specified in interaction", + "invalid liquidity ID specified in interaction".to_owned(), ))? .to_owned(); Ok(competition::solution::Interaction::Liquidity( @@ -206,8 +208,13 @@ impl Solutions { }, weth, ) - .map_err(|competition::solution::InvalidClearingPrices| { - super::Error("invalid clearing prices") + .map_err(|err| match err { + competition::solution::SolutionError::InvalidClearingPrices => { + super::Error("invalid clearing prices".to_owned()) + } + competition::solution::SolutionError::ProtocolFee(err) => { + super::Error(format!("could not incorporate protocol fee: {err}")) + } }) }) .collect() diff --git a/crates/driver/src/tests/setup/driver.rs b/crates/driver/src/tests/setup/driver.rs index 75c26e0d12..f7c3bfdd65 100644 --- a/crates/driver/src/tests/setup/driver.rs +++ b/crates/driver/src/tests/setup/driver.rs @@ -94,12 +94,16 @@ pub fn solve_req(test: &Test) -> serde_json::Value { "appData": "0x0000000000000000000000000000000000000000000000000000000000000000", "signingScheme": "eip712", "signature": format!("0x{}", hex::encode(quote.order_signature(&test.blockchain))), - "feePolicies": [{ - "priceImprovement": { - "factor": 0.5, - "maxVolumeFactor": 0.06 - } - }], + "feePolicies": match quote.order.kind { + order::Kind::Market => json!([]), + order::Kind::Liquidity => json!([]), + order::Kind::Limit { .. } => json!([{ + "priceImprovement": { + "factor": 0.0, + "maxVolumeFactor": 0.06 + } + }]), + }, })); } for fulfillment in test.fulfillments.iter() { diff --git a/crates/driver/src/tests/setup/mod.rs b/crates/driver/src/tests/setup/mod.rs index c575ec1b01..a7d2f6f0e2 100644 --- a/crates/driver/src/tests/setup/mod.rs +++ b/crates/driver/src/tests/setup/mod.rs @@ -79,7 +79,7 @@ impl Default for Score { } } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq)] pub struct Order { pub name: &'static str, @@ -287,7 +287,7 @@ impl Solver { } } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq)] pub struct Pool { pub token_a: &'static str, pub token_b: &'static str, diff --git a/crates/e2e/tests/e2e/main.rs b/crates/e2e/tests/e2e/main.rs index 560082fd2d..ea77b8ec80 100644 --- a/crates/e2e/tests/e2e/main.rs +++ b/crates/e2e/tests/e2e/main.rs @@ -18,6 +18,7 @@ mod order_cancellation; mod partial_fill; mod partially_fillable_balance; mod partially_fillable_pool; +mod protocol_fee; mod quoting; mod refunder; mod smart_contract_orders; diff --git a/crates/e2e/tests/e2e/protocol_fee.rs b/crates/e2e/tests/e2e/protocol_fee.rs new file mode 100644 index 0000000000..8c4045bb7f --- /dev/null +++ b/crates/e2e/tests/e2e/protocol_fee.rs @@ -0,0 +1,379 @@ +use { + e2e::{ + setup::{colocation::SolverEngine, *}, + tx, + }, + ethcontract::prelude::U256, + model::{ + order::{OrderCreation, OrderKind}, + signature::EcdsaSigningScheme, + }, + secp256k1::SecretKey, + shared::ethrpc::Web3, + web3::signing::SecretKeyRef, +}; + +#[tokio::test] +#[ignore] +async fn local_node_price_improvement_fee_sell_order() { + run_test(price_improvement_fee_sell_order_test).await; +} + +#[tokio::test] +#[ignore] +async fn local_node_price_improvement_fee_sell_order_capped() { + run_test(price_improvement_fee_sell_order_capped_test).await; +} + +#[tokio::test] +#[ignore] +async fn local_node_volume_fee_sell_order() { + run_test(volume_fee_sell_order_test).await; +} + +#[tokio::test] +#[ignore] +async fn local_node_price_improvement_fee_buy_order() { + run_test(price_improvement_fee_buy_order_test).await; +} + +#[tokio::test] +#[ignore] +async fn local_node_price_improvement_fee_buy_order_capped() { + run_test(price_improvement_fee_buy_order_capped_test).await; +} + +#[tokio::test] +#[ignore] +async fn local_node_volume_fee_buy_order() { + run_test(volume_fee_buy_order_test).await; +} + +async fn price_improvement_fee_sell_order_test(web3: Web3) { + let fee_policy = FeePolicyKind::PriceImprovement { + factor: 0.3, + max_volume_factor: 1.0, + }; + // Without protocol fee: + // Expected execution is 10000000000000000000 GNO for + // 9871415430342266811 DAI, with executed_surplus_fee = 167058994203399 GNO + // + // With protocol fee: + // surplus [DAI] = 9871415430342266811 DAI - 5000000000000000000 DAI = + // 4871415430342266811 DAI + // + // protocol fee = 0.3*surplus = 1461424629102680043 DAI = + // 1461424629102680043 DAI / 9871415430342266811 * + // (10000000000000000000 - 167058994203399) = 1480436341679873337 GNO + // + // final execution is 10000000000000000000 GNO for 8409990801239586768 DAI, with + // executed_surplus_fee = 1480603400674076736 GNO + // + // Settlement contract balance after execution = 1480603400674076736 GNO = + // 1480603400674076736 GNO * 8409990801239586768 / (10000000000000000000 - + // 1480603400674076736) = 1461589542731026166 DAI + execute_test( + web3.clone(), + fee_policy, + OrderKind::Sell, + 1480603400674076736u128.into(), + 1461589542731026166u128.into(), + ) + .await; +} + +async fn price_improvement_fee_sell_order_capped_test(web3: Web3) { + let fee_policy = FeePolicyKind::PriceImprovement { + factor: 1.0, + max_volume_factor: 0.1, + }; + // Without protocol fee: + // Expected executed_surplus_fee is 167058994203399 + // + // With protocol fee: + // Expected executed_surplus_fee is 167058994203399 + + // 0.1*10000000000000000000 = 1000167058994203400 + // + // Final execution is 10000000000000000000 GNO for 8884257395945205588 DAI, with + // executed_surplus_fee = 1000167058994203400 GNO + // + // Settlement contract balance after execution = 1000167058994203400 GNO = + // 1000167058994203400 GNO * 8884257395945205588 / (10000000000000000000 - + // 1000167058994203400) = 987322948025407485 DAI + execute_test( + web3.clone(), + fee_policy, + OrderKind::Sell, + 1000167058994203400u128.into(), + 987322948025407485u128.into(), + ) + .await; +} + +async fn volume_fee_sell_order_test(web3: Web3) { + let fee_policy = FeePolicyKind::Volume { factor: 0.1 }; + // Without protocol fee: + // Expected executed_surplus_fee is 167058994203399 + // + // With protocol fee: + // Expected executed_surplus_fee is 167058994203399 + + // 0.1*10000000000000000000 = 1000167058994203400 + // + // Settlement contract balance after execution = 1000167058994203400 GNO = + // 1000167058994203400 GNO * 8884257395945205588 / (10000000000000000000 - + // 1000167058994203400) = 987322948025407485 DAI + execute_test( + web3.clone(), + fee_policy, + OrderKind::Sell, + 1000167058994203400u128.into(), + 987322948025407485u128.into(), + ) + .await; +} + +async fn price_improvement_fee_buy_order_test(web3: Web3) { + let fee_policy = FeePolicyKind::PriceImprovement { + factor: 0.3, + max_volume_factor: 1.0, + }; + // Without protocol fee: + // Expected execution is 5040413426236634210 GNO for 5000000000000000000 DAI, + // with executed_surplus_fee = 167058994203399 GNO + // + // With protocol fee: + // surplus in sell token = 10000000000000000000 - 5040413426236634210 = + // 4959586573763365790 + // + // protocol fee in sell token = 0.3*4959586573763365790 = 1487875972129009737 + // + // expected executed_surplus_fee is 167058994203399 + 1487875972129009737 = + // 1488043031123213136 + // + // Settlement contract balance after execution = executed_surplus_fee GNO + execute_test( + web3.clone(), + fee_policy, + OrderKind::Buy, + 1488043031123213136u128.into(), + 1488043031123213136u128.into(), + ) + .await; +} + +async fn price_improvement_fee_buy_order_capped_test(web3: Web3) { + let fee_policy = FeePolicyKind::PriceImprovement { + factor: 1.0, + max_volume_factor: 0.1, + }; + // Without protocol fee: + // Expected execution is 5040413426236634210 GNO for 5000000000000000000 DAI, + // with executed_surplus_fee = 167058994203399 GNO + // + // With protocol fee: + // Expected executed_surplus_fee is 167058994203399 + 0.1*5040413426236634210 = + // 504208401617866820 + // + // Settlement contract balance after execution = executed_surplus_fee GNO + execute_test( + web3.clone(), + fee_policy, + OrderKind::Buy, + 504208401617866820u128.into(), + 504208401617866820u128.into(), + ) + .await; +} + +async fn volume_fee_buy_order_test(web3: Web3) { + let fee_policy = FeePolicyKind::Volume { factor: 0.1 }; + // Without protocol fee: + // Expected execution is 5040413426236634210 GNO for 5000000000000000000 DAI, + // with executed_surplus_fee = 167058994203399 GNO + // + // With protocol fee: + // Expected executed_surplus_fee is 167058994203399 + 0.1*5040413426236634210 = + // 504208401617866820 + // + // Settlement contract balance after execution = executed_surplus_fee GNO + execute_test( + web3.clone(), + fee_policy, + OrderKind::Buy, + 504208401617866820u128.into(), + 504208401617866820u128.into(), + ) + .await; +} + +// because of rounding errors, it's good enough to check that the expected value +// is within a very narrow range of the executed value +fn is_approximately_equal(executed_value: U256, expected_value: U256) -> bool { + let lower = expected_value * U256::from(99999999999u128) / U256::from(100000000000u128); // in percents = 99.999999999% + let upper = expected_value * U256::from(100000000001u128) / U256::from(100000000000u128); // in percents = 100.000000001% + executed_value >= lower && executed_value <= upper +} + +async fn execute_test( + web3: Web3, + fee_policy: FeePolicyKind, + order_kind: OrderKind, + expected_surplus_fee: U256, + expected_settlement_contract_balance: U256, +) { + let mut onchain = OnchainComponents::deploy(web3.clone()).await; + + let [solver] = onchain.make_solvers(to_wei(1)).await; + let [trader] = onchain.make_accounts(to_wei(1)).await; + let [token_gno, token_dai] = onchain + .deploy_tokens_with_weth_uni_v2_pools(to_wei(1_000), to_wei(1000)) + .await; + + // Fund trader accounts + token_gno.mint(trader.address(), to_wei(100)).await; + + // Create and fund Uniswap pool + token_gno.mint(solver.address(), to_wei(1000)).await; + token_dai.mint(solver.address(), to_wei(1000)).await; + tx!( + solver.account(), + onchain + .contracts() + .uniswap_v2_factory + .create_pair(token_gno.address(), token_dai.address()) + ); + tx!( + solver.account(), + token_gno.approve( + onchain.contracts().uniswap_v2_router.address(), + to_wei(1000) + ) + ); + tx!( + solver.account(), + token_dai.approve( + onchain.contracts().uniswap_v2_router.address(), + to_wei(1000) + ) + ); + tx!( + solver.account(), + onchain.contracts().uniswap_v2_router.add_liquidity( + token_gno.address(), + token_dai.address(), + to_wei(1000), + to_wei(1000), + 0_u64.into(), + 0_u64.into(), + solver.address(), + U256::max_value(), + ) + ); + + // Approve GPv2 for trading + tx!( + trader.account(), + token_gno.approve(onchain.contracts().allowance, to_wei(100)) + ); + + // Place Orders + let services = Services::new(onchain.contracts()).await; + let solver_endpoint = + colocation::start_baseline_solver(onchain.contracts().weth.address()).await; + colocation::start_driver( + onchain.contracts(), + vec![SolverEngine { + name: "test_solver".into(), + account: solver, + endpoint: solver_endpoint, + }], + ); + services.start_autopilot(vec![ + "--drivers=test_solver|http://localhost:11088/test_solver".to_string(), + "--fee-policy-skip-market-orders=false".to_string(), + fee_policy.to_string(), + ]); + services + .start_api(vec![ + "--price-estimation-drivers=test_solver|http://localhost:11088/test_solver".to_string(), + ]) + .await; + + let order = OrderCreation { + sell_token: token_gno.address(), + sell_amount: to_wei(10), + buy_token: token_dai.address(), + buy_amount: to_wei(5), + valid_to: model::time::now_in_epoch_seconds() + 300, + kind: order_kind, + ..Default::default() + } + .sign( + EcdsaSigningScheme::Eip712, + &onchain.contracts().domain_separator, + SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()), + ); + let uid = services.create_order(&order).await.unwrap(); + + // Drive solution + tracing::info!("Waiting for trade."); + wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 1 }) + .await + .unwrap(); + + wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 0 }) + .await + .unwrap(); + + onchain.mint_blocks_past_reorg_threshold().await; + let metadata_updated = || async { + onchain.mint_block().await; + let order = services.get_order(&uid).await.unwrap(); + is_approximately_equal(order.metadata.executed_surplus_fee, expected_surplus_fee) + }; + wait_for_condition(TIMEOUT, metadata_updated).await.unwrap(); + + // Check settlement contract balance + let balance_after = match order_kind { + OrderKind::Buy => token_gno + .balance_of(onchain.contracts().gp_settlement.address()) + .call() + .await + .unwrap(), + OrderKind::Sell => token_dai + .balance_of(onchain.contracts().gp_settlement.address()) + .call() + .await + .unwrap(), + }; + assert!(is_approximately_equal( + balance_after, + expected_settlement_contract_balance + )); +} + +enum FeePolicyKind { + /// How much of the order's price improvement over max(limit price, + /// best_bid) should be taken as a protocol fee. + PriceImprovement { factor: f64, max_volume_factor: f64 }, + /// How much of the order's volume should be taken as a protocol fee. + Volume { factor: f64 }, +} + +impl std::fmt::Display for FeePolicyKind { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + FeePolicyKind::PriceImprovement { + factor, + max_volume_factor, + } => write!( + f, + "--fee-policy-kind=priceImprovement:{}:{}", + factor, max_volume_factor + ), + FeePolicyKind::Volume { factor } => { + write!(f, "--fee-policy-kind=volume:{}", factor) + } + } + } +} From 5cdc7bc5a5d0934c750cae9d227170cb136bc76b Mon Sep 17 00:00:00 2001 From: ilya Date: Fri, 12 Jan 2024 09:35:22 +0000 Subject: [PATCH 04/16] Notify solvers on post-processing timed out (#2268) # Description `Seasolver` reported that some solutions were rejected for no visible reason. It turned out that if post-processing is timed out, we currently don't notify solvers about that. # Changes Notify solvers on post-processing timed out. ## How to test TBD --- crates/driver/src/domain/competition/mod.rs | 3 ++- crates/driver/src/infra/notify/mod.rs | 4 ++++ crates/driver/src/infra/notify/notification.rs | 2 ++ crates/driver/src/infra/solver/dto/notification.rs | 2 ++ crates/shared/src/http_solver/model.rs | 3 +++ crates/solvers/openapi.yml | 2 +- crates/solvers/src/api/routes/notify/dto/notification.rs | 2 ++ crates/solvers/src/boundary/legacy.rs | 3 +++ crates/solvers/src/domain/notification.rs | 1 + 9 files changed, 20 insertions(+), 2 deletions(-) diff --git a/crates/driver/src/domain/competition/mod.rs b/crates/driver/src/domain/competition/mod.rs index e6b7784901..b778773a71 100644 --- a/crates/driver/src/domain/competition/mod.rs +++ b/crates/driver/src/domain/competition/mod.rs @@ -131,7 +131,8 @@ impl Competition { .await .is_err() { - observe::postprocessing_timed_out(&settlements) + observe::postprocessing_timed_out(&settlements); + notify::postprocessing_timed_out(&self.solver, auction.id()) } // Score the settlements. diff --git a/crates/driver/src/infra/notify/mod.rs b/crates/driver/src/infra/notify/mod.rs index d87d29bd57..e7c96c3340 100644 --- a/crates/driver/src/infra/notify/mod.rs +++ b/crates/driver/src/infra/notify/mod.rs @@ -137,3 +137,7 @@ pub fn duplicated_solution_id( notification::Kind::DuplicatedSolutionId, ); } + +pub fn postprocessing_timed_out(solver: &Solver, auction_id: Option) { + solver.notify(auction_id, None, notification::Kind::PostprocessingTimedOut); +} diff --git a/crates/driver/src/infra/notify/notification.rs b/crates/driver/src/infra/notify/notification.rs index f129b8626d..2bd0ba8a2f 100644 --- a/crates/driver/src/infra/notify/notification.rs +++ b/crates/driver/src/infra/notify/notification.rs @@ -43,6 +43,8 @@ pub enum Kind { /// Some aspect of the driver logic failed preventing the solution from /// participating in the auction. DriverError(String), + /// On-chain solution postprocessing timed out. + PostprocessingTimedOut, } #[derive(Debug)] diff --git a/crates/driver/src/infra/solver/dto/notification.rs b/crates/driver/src/infra/solver/dto/notification.rs index 3251827fd2..b8cb5d1f5c 100644 --- a/crates/driver/src/infra/solver/dto/notification.rs +++ b/crates/driver/src/infra/solver/dto/notification.rs @@ -78,6 +78,7 @@ impl Notification { notify::Settlement::SimulationRevert => Kind::Cancelled, notify::Settlement::Fail => Kind::Fail, }, + notify::Kind::PostprocessingTimedOut => Kind::PostprocessingTimedOut, }, } } @@ -141,6 +142,7 @@ pub enum Kind { }, Cancelled, Fail, + PostprocessingTimedOut, } type BlockNo = u64; diff --git a/crates/shared/src/http_solver/model.rs b/crates/shared/src/http_solver/model.rs index 030608b194..4b9874cf8d 100644 --- a/crates/shared/src/http_solver/model.rs +++ b/crates/shared/src/http_solver/model.rs @@ -491,6 +491,9 @@ pub enum SolverRejectionReason { /// Some aspect of the driver logic failed. Driver(String), + + /// On-chain solution postprocessing timed out. + PostprocessingTimedOut, } #[derive(Debug, Serialize)] diff --git a/crates/solvers/openapi.yml b/crates/solvers/openapi.yml index 85756503ab..d8688eb177 100644 --- a/crates/solvers/openapi.yml +++ b/crates/solvers/openapi.yml @@ -56,7 +56,7 @@ paths: description: | The kind of notification. type: string - enum: [Timeout, EmptySolution, DuplicatedSolutionId, SimulationFailed, ZeroScore, ScoreHigherThanQuality, SuccessProbabilityOutOfRange, ObjectiveValueNonPositive, NonBufferableTokensUsed, SolverAccountInsufficientBalance, Success, Revert, Cancelled, Failed] + enum: [Timeout, EmptySolution, DuplicatedSolutionId, SimulationFailed, ZeroScore, ScoreHigherThanQuality, SuccessProbabilityOutOfRange, ObjectiveValueNonPositive, NonBufferableTokensUsed, SolverAccountInsufficientBalance, Success, Revert, Cancelled, Failed, PostprocessingTimedOut] responses: 200: description: notification successfully received. diff --git a/crates/solvers/src/api/routes/notify/dto/notification.rs b/crates/solvers/src/api/routes/notify/dto/notification.rs index 1b982c0df8..fa5d3e2de1 100644 --- a/crates/solvers/src/api/routes/notify/dto/notification.rs +++ b/crates/solvers/src/api/routes/notify/dto/notification.rs @@ -91,6 +91,7 @@ impl Notification { notification::Kind::Settled(notification::Settlement::SimulationRevert) } Kind::Fail => notification::Kind::Settled(notification::Settlement::Fail), + Kind::PostprocessingTimedOut => notification::Kind::PostprocessingTimedOut, }, } } @@ -155,6 +156,7 @@ pub enum Kind { }, Cancelled, Fail, + PostprocessingTimedOut, } type BlockNo = u64; diff --git a/crates/solvers/src/boundary/legacy.rs b/crates/solvers/src/boundary/legacy.rs index 12e93ed9e9..df0f706bb6 100644 --- a/crates/solvers/src/boundary/legacy.rs +++ b/crates/solvers/src/boundary/legacy.rs @@ -656,6 +656,9 @@ fn to_boundary_auction_result(notification: ¬ification::Notification) -> (i64 Kind::DriverError(reason) => { AuctionResult::Rejected(SolverRejectionReason::Driver(reason.clone())) } + Kind::PostprocessingTimedOut => { + AuctionResult::Rejected(SolverRejectionReason::PostprocessingTimedOut) + } }; (auction_id, auction_result) diff --git a/crates/solvers/src/domain/notification.rs b/crates/solvers/src/domain/notification.rs index 6eb8d7ccb3..85f72ba454 100644 --- a/crates/solvers/src/domain/notification.rs +++ b/crates/solvers/src/domain/notification.rs @@ -35,6 +35,7 @@ pub enum Kind { SolverAccountInsufficientBalance(RequiredEther), Settled(Settlement), DriverError(String), + PostprocessingTimedOut, } /// The result of winning solver trying to settle the transaction onchain. From e3cd24c126255407b0167214784a4420958a3bca Mon Sep 17 00:00:00 2001 From: Anuj Bansal Date: Fri, 12 Jan 2024 16:59:31 +0530 Subject: [PATCH 05/16] Add Request ID header to quote requests for easier debugging (#2261) # Description It is a bit difficult to debug quote requests from our reverse proxy right now as the requests don't pass in the request id header used in our backend. This PR sets the `X-REQUEST-ID` header in the request so that it can be logged in nginx for easier debugging. # Changes - [x] Add request_id to oneinch, paraswap and zeroex requests --- crates/shared/src/oneinch_api.rs | 3 +++ crates/shared/src/paraswap_api.rs | 6 ++++++ crates/shared/src/zeroex_api.rs | 3 +++ 3 files changed, 12 insertions(+) diff --git a/crates/shared/src/oneinch_api.rs b/crates/shared/src/oneinch_api.rs index 6bcd85665a..ae40a7d2f8 100644 --- a/crates/shared/src/oneinch_api.rs +++ b/crates/shared/src/oneinch_api.rs @@ -598,6 +598,9 @@ where block_stream.borrow().hash.to_string(), ); }; + if let Some(id) = observe::request_id::get_task_local_storage() { + request = request.header("X-REQUEST-ID", id); + } let response = request.send().await?; let status_code = response.status(); diff --git a/crates/shared/src/paraswap_api.rs b/crates/shared/src/paraswap_api.rs index 9957bf983c..b45e3d33c7 100644 --- a/crates/shared/src/paraswap_api.rs +++ b/crates/shared/src/paraswap_api.rs @@ -59,6 +59,9 @@ impl ParaswapApi for DefaultParaswapApi { self.block_stream.borrow().hash.to_string(), ); }; + if let Some(id) = observe::request_id::get_task_local_storage() { + request = request.header("X-REQUEST-ID", id); + } let response = request.send().await?; let status = response.status(); @@ -84,6 +87,9 @@ impl ParaswapApi for DefaultParaswapApi { self.block_stream.borrow().hash.to_string(), ); }; + if let Some(id) = observe::request_id::get_task_local_storage() { + request = request.header("X-REQUEST-ID", id); + } let response = request.send().await?; let status = response.status(); let response_text = response.text().await?; diff --git a/crates/shared/src/zeroex_api.rs b/crates/shared/src/zeroex_api.rs index 12649e953b..f8fa5225cd 100644 --- a/crates/shared/src/zeroex_api.rs +++ b/crates/shared/src/zeroex_api.rs @@ -542,6 +542,9 @@ impl DefaultZeroExApi { self.block_stream.borrow().hash.to_string(), ); }; + if let Some(id) = observe::request_id::get_task_local_storage() { + request = request.header("X-REQUEST-ID", id); + } let response = request.send().await.map_err(ZeroExResponseError::Send)?; From f81041911c12e8053f86d55123166d82fdd29db3 Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Fri, 12 Jan 2024 13:45:34 +0100 Subject: [PATCH 06/16] Refactor competition sorting (#2195) # Description With the introduction of "BestBangForBuck" sorting solvers could game the system by reporting unreasonably low gas costs which would end up with huge subsidies payed by the protocol because those values get used to determine the fee of an order. To fix that we try to catch outliers that report unreasonably low gas costs (less than half the median gas). # Changes - instead of passing in a comparator function we pass in a function that returns the preferred index because that allows for better introspection of all estimates - return `Ordering` from helper functions instead of `bool` - rename helper functions - compute median gas cost of all estimates and discard all estimates that report less than 50% of that to catch outliers ## How to test Added unit test --- crates/shared/src/price_estimation.rs | 11 + .../src/price_estimation/competition.rs | 388 +++++++++++++----- 2 files changed, 293 insertions(+), 106 deletions(-) diff --git a/crates/shared/src/price_estimation.rs b/crates/shared/src/price_estimation.rs index b85955995b..ca4cb67489 100644 --- a/crates/shared/src/price_estimation.rs +++ b/crates/shared/src/price_estimation.rs @@ -417,6 +417,17 @@ pub enum PriceEstimationError { ProtocolInternal(anyhow::Error), } +#[cfg(test)] +impl PartialEq for PriceEstimationError { + // Can't use `Self` here because `discriminant` is only defined for enums + // and the compiler is not smart enough to figure out that `Self` is always + // an enum here. + fn eq(&self, other: &PriceEstimationError) -> bool { + let me = self as &PriceEstimationError; + std::mem::discriminant(me) == std::mem::discriminant(other) + } +} + impl Clone for PriceEstimationError { fn clone(&self) -> Self { match self { diff --git a/crates/shared/src/price_estimation/competition.rs b/crates/shared/src/price_estimation/competition.rs index a0ae16c73c..a7098efb23 100644 --- a/crates/shared/src/price_estimation/competition.rs +++ b/crates/shared/src/price_estimation/competition.rs @@ -7,6 +7,7 @@ use { PriceEstimationError, Query, }, + anyhow::Context, futures::{ future::Future, stream::{FuturesUnordered, StreamExt}, @@ -41,9 +42,6 @@ impl From<&Query> for Trade { #[derive(Copy, Debug, Clone, Default, Eq, PartialEq)] struct EstimatorIndex(usize, usize); -#[derive(Copy, Debug, Clone, Default, Eq, PartialEq, Ord, PartialOrd)] -struct Wins(u64); - type PriceEstimationStage = Vec<(String, T)>; /// Price estimator that pulls estimates from various sources @@ -85,7 +83,9 @@ impl RacingCompetitionEstimator { get_single_result: impl Fn(&T, Q) -> futures::future::BoxFuture<'_, Result> + Send + 'static, - compare_results: impl Fn(&Result, &Result, &C) -> Ordering + Send + 'static, + pick_best_index: impl Fn(&[(EstimatorIndex, Result)], &C) -> Result + + Send + + 'static, provide_comparison_context: impl Future> + Send + 'static, ) -> futures::future::BoxFuture<'_, Result> { let start = Instant::now(); @@ -146,14 +146,7 @@ impl RacingCompetitionEstimator { } let context = provide_comparison_context.await?; - - let best_index = results - .iter() - .map(|(_, result)| result) - .enumerate() - .max_by(|a, b| compare_results(a.1, b.1, &context)) - .map(|(index, _)| index) - .unwrap(); + let best_index = pick_best_index(&results, &context)?; let (estimator_index, result) = &results[best_index]; let (estimator, _) = &self.inner[estimator_index.0][estimator_index.1]; tracing::debug!( @@ -203,12 +196,19 @@ impl PriceEstimating for RacingCompetitionEstimator> { query.clone(), query.kind, |estimator, query| estimator.estimate(query), - move |a, b, context| { - if is_second_quote_result_preferred(query.as_ref(), a, b, context) { - Ordering::Less - } else { - Ordering::Greater - } + move |results, context| { + results + .iter() + .map(|(_, result)| result) + .enumerate() + // Filter out 0 gas cost estimate because they are obviously wrong and would + // likely win the price competition which would lead to us paying huge + // subsidies. + .filter(|(_, r)| r.is_err() || r.as_ref().is_ok_and(|e| e.gas > 0)) + .max_by(|a, b| compare_quote_result(&query, a.1, b.1, context)) + .map(|(index, _)| index) + .with_context(|| "all price estimates reported 0 gas cost") + .map_err(PriceEstimationError::EstimatorInternal) }, context_future, ) @@ -225,12 +225,15 @@ impl NativePriceEstimating for RacingCompetitionEstimator> { } } -fn is_second_quote_result_preferred( +fn compare_quote_result( query: &Query, a: &PriceEstimateResult, b: &PriceEstimateResult, context: &RankingContext, -) -> bool { +) -> Ordering { match (a, b) { - (Ok(a), Ok(b)) => is_second_estimate_preferred(query, a, b, context), - (Ok(_), Err(_)) => false, - (Err(_), Ok(_)) => true, - (Err(a), Err(b)) => is_second_error_preferred(a, b), + (Ok(a), Ok(b)) => compare_quote(query, a, b, context), + (Ok(_), Err(_)) => Ordering::Greater, + (Err(_), Ok(_)) => Ordering::Less, + (Err(a), Err(b)) => compare_error(a, b), } } -fn is_second_native_result_preferred( +fn compare_native_result( a: &Result, b: &Result, -) -> bool { +) -> Ordering { match (a, b) { - (Ok(a), Ok(b)) => b >= a, - (Ok(_), Err(_)) => false, - (Err(_), Ok(_)) => true, - (Err(a), Err(b)) => is_second_error_preferred(a, b), + (Ok(a), Ok(b)) => a.total_cmp(b), + (Ok(_), Err(_)) => Ordering::Greater, + (Err(_), Ok(_)) => Ordering::Less, + (Err(a), Err(b)) => compare_error(a, b), } } -fn is_second_estimate_preferred( - query: &Query, - a: &Estimate, - b: &Estimate, - context: &RankingContext, -) -> bool { +fn compare_quote(query: &Query, a: &Estimate, b: &Estimate, context: &RankingContext) -> Ordering { let a = context.effective_eth_out(a, query.kind); let b = context.effective_eth_out(b, query.kind); match query.kind { - OrderKind::Buy => b < a, - OrderKind::Sell => a < b, + OrderKind::Buy => a.cmp(&b).reverse(), + OrderKind::Sell => a.cmp(&b), } } -fn is_second_error_preferred(a: &PriceEstimationError, b: &PriceEstimationError) -> bool { +fn compare_error(a: &PriceEstimationError, b: &PriceEstimationError) -> Ordering { // Errors are sorted by recoverability. E.g. a rate-limited estimation may // succeed if tried again, whereas unsupported order types can never recover // unless code changes. This can be used to decide which errors we want to @@ -317,7 +315,7 @@ fn is_second_error_preferred(a: &PriceEstimationError, b: &PriceEstimationError) // lowest priority } } - error_to_integer_priority(b) > error_to_integer_priority(a) + error_to_integer_priority(a).cmp(&error_to_integer_priority(b)) } #[derive(prometheus_metric_storage::MetricStorage, Clone, Debug)] @@ -438,6 +436,108 @@ mod tests { tokio::time::sleep, }; + fn price(out_amount: u128, gas: u64) -> PriceEstimateResult { + Ok(Estimate { + out_amount: out_amount.into(), + gas, + ..Default::default() + }) + } + + fn native_price(native_price: f64) -> NativePriceEstimateResult { + NativePriceEstimateResult::Ok(native_price) + } + + fn error(err: PriceEstimationError) -> Result { + Err(err) + } + + /// Builds a `BestBangForBuck` setup where every token is estimated + /// to be half as valuable as ETH and the gas price is 2. + /// That effectively means every unit of `gas` in an estimate worth + /// 4 units of `out_amount`. + fn bang_for_buck_ranking() -> PriceRanking { + // Make `out_token` half as valuable as `ETH` and set gas price to 2. + // That means 1 unit of `gas` is equal to 4 units of `out_token`. + let mut native = MockNativePriceEstimating::new(); + native + .expect_estimate_native_price() + .returning(move |_| async { Ok(0.5) }.boxed()); + let gas = Arc::new(FakeGasPriceEstimator::new(GasPrice1559 { + base_fee_per_gas: 2.0, + max_fee_per_gas: 2.0, + max_priority_fee_per_gas: 2.0, + })); + PriceRanking::BestBangForBuck { + native: Arc::new(native), + gas, + } + } + + /// Returns the best estimate with respect to the provided ranking and order + /// kind. + async fn best_response( + ranking: PriceRanking, + kind: OrderKind, + estimates: Vec, + ) -> PriceEstimateResult { + fn estimator(estimate: PriceEstimateResult) -> Arc { + let mut estimator = MockPriceEstimating::new(); + estimator + .expect_estimate() + .times(1) + .return_once(move |_| async move { estimate }.boxed()); + Arc::new(estimator) + } + + let priority: CompetitionEstimator> = CompetitionEstimator::new( + vec![estimates + .into_iter() + .enumerate() + .map(|(i, e)| (format!("estimator_{i}"), estimator(e))) + .collect()], + ranking.clone(), + ); + + priority + .estimate(Arc::new(Query { + kind, + ..Default::default() + })) + .await + } + + /// Returns the best native estimate with respect to the provided ranking + /// and order kind. + async fn best_native_response( + ranking: PriceRanking, + estimates: Vec, + ) -> NativePriceEstimateResult { + fn estimator(estimate: NativePriceEstimateResult) -> Arc { + let mut estimator = MockNativePriceEstimating::new(); + estimator + .expect_estimate_native_price() + .times(1) + .return_once(move |_| async move { estimate }.boxed()); + Arc::new(estimator) + } + + let priority: CompetitionEstimator> = + CompetitionEstimator::new( + vec![estimates + .into_iter() + .enumerate() + .map(|(i, e)| (format!("estimator_{i}"), estimator(e))) + .collect()], + ranking.clone(), + ); + + priority + .inner + .estimate_native_price(Default::default()) + .await + } + #[tokio::test] async fn works() { let queries = [ @@ -485,10 +585,12 @@ mod tests { let estimates = [ Estimate { out_amount: 1.into(), + gas: 1, ..Default::default() }, Estimate { out_amount: 2.into(), + gas: 1, ..Default::default() }, ]; @@ -574,6 +676,7 @@ mod tests { fn estimate(amount: u64) -> Estimate { Estimate { out_amount: amount.into(), + gas: 1, ..Default::default() } } @@ -635,6 +738,7 @@ mod tests { fn estimate(amount: u64) -> Estimate { Estimate { out_amount: amount.into(), + gas: 1, ..Default::default() } } @@ -711,6 +815,7 @@ mod tests { fn estimate(amount: u64) -> Estimate { Estimate { out_amount: amount.into(), + gas: 1, ..Default::default() } } @@ -771,71 +876,142 @@ mod tests { /// the simpler quote will be preferred. #[tokio::test] async fn best_bang_for_buck_adjusts_for_complexity() { - let estimator = |estimate| { - let mut estimator = MockPriceEstimating::new(); - estimator - .expect_estimate() - .times(1) - .returning(move |_| async move { Ok(estimate) }.boxed()); - Arc::new(estimator) - }; - let estimate = |out: u128, gas| Estimate { - out_amount: out.into(), - gas, - ..Default::default() - }; + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Sell, + vec![ + // User effectively receives `100_000` `buy_token`. + price(104_000, 1_000), + // User effectively receives `99_999` `buy_token`. + price(107_999, 2_000), + ], + ) + .await; + assert_eq!(best, price(104_000, 1_000)); - // Make `out_token` half as valuable as `ETH` and set gas price to 2. - // That means 1 unit of `gas` is equal to 4 units of `out_token`. - let mut native = MockNativePriceEstimating::new(); - native - .expect_estimate_native_price() - .returning(move |_| async { Ok(0.5) }.boxed()); - let gas = Arc::new(FakeGasPriceEstimator::new(GasPrice1559 { - base_fee_per_gas: 2.0, - max_fee_per_gas: 2.0, - max_priority_fee_per_gas: 2.0, - })); - let ranking = PriceRanking::BestBangForBuck { - native: Arc::new(native), - gas, - }; + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Buy, + vec![ + // User effectively pays `100_000` `sell_token`. + price(96_000, 1_000), + // User effectively pays `100_002` `sell_token`. + price(92_002, 2_000), + ], + ) + .await; + assert_eq!(best, price(96_000, 1_000)); + } - // tests are given as (quote_kind, preferred_estimate, worse_estimate) - let tests = [ - ( - OrderKind::Sell, + /// Same test as above but now we also add an estimate that should + /// win under normal circumstances but the `gas` cost is suspiciously + /// low so we discard it. This protects us from quoting unreasonably + /// low fees for user orders. + #[tokio::test] + async fn discards_low_gas_cost_estimates() { + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Sell, + vec![ // User effectively receives `100_000` `buy_token`. - estimate(104_000, 1_000), + price(104_000, 1_000), // User effectively receives `99_999` `buy_token`. - estimate(107_999, 2_000), - ), - ( - OrderKind::Buy, + price(107_999, 2_000), + // User effectively receives `104_000` `buy_token` but the estimate + // gets discarded because it quotes 0 gas. + price(104_000, 0), + ], + ) + .await; + assert_eq!(best, price(104_000, 1_000)); + + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Buy, + vec![ // User effectively pays `100_000` `sell_token`. - estimate(96_000, 1_000), - // User effectively pays `100_001` `sell_token`. - estimate(92_001, 2_000), - ), - ]; + price(96_000, 1_000), + // User effectively pays `100_002` `sell_token`. + price(92_002, 2_000), + // User effectively pays `99_000` `sell_token` but the estimate + // gets discarded because it quotes 0 gas. + price(99_000, 0), + ], + ) + .await; + assert_eq!(best, price(96_000, 1_000)); + } - for (quote_kind, preferred_estimate, worse_estimate) in tests { - let priority: CompetitionEstimator> = - CompetitionEstimator::new( - vec![vec![ - ("first".to_owned(), estimator(preferred_estimate)), - ("second".to_owned(), estimator(worse_estimate)), - ]], - ranking.clone(), - ); - - let result = priority - .estimate(Arc::new(Query { - kind: quote_kind, - ..Default::default() - })) - .await; - assert_eq!(result.unwrap(), preferred_estimate); - } + /// If all estimators returned an error we return the one with the highest + /// priority. + #[tokio::test] + async fn returns_highest_priority_error() { + // Returns errors with highest priority. + let best = best_response( + PriceRanking::MaxOutAmount, + OrderKind::Sell, + vec![ + error(PriceEstimationError::RateLimited), + error(PriceEstimationError::ProtocolInternal(anyhow::anyhow!("!"))), + ], + ) + .await; + assert_eq!(best, error(PriceEstimationError::RateLimited)); + } + + /// Any price estimate, no matter how bad, is preferred over an error. + #[tokio::test] + async fn prefer_estimate_over_error() { + let best = best_response( + PriceRanking::MaxOutAmount, + OrderKind::Sell, + vec![ + price(1, 1_000_000), + error(PriceEstimationError::RateLimited), + ], + ) + .await; + assert_eq!(best, price(1, 1_000_000)); + } + + /// If all estimators returned an error we return the one with the highest + /// priority. + #[tokio::test] + async fn returns_highest_native_price() { + // Returns errors with highest priority. + let best = best_native_response( + PriceRanking::MaxOutAmount, + vec![native_price(1.), native_price(2.)], + ) + .await; + assert_eq!(best, native_price(2.)); + } + + /// If all estimators returned an error we return the one with the highest + /// priority. + #[tokio::test] + async fn returns_highest_priority_error_native() { + // Returns errors with highest priority. + let best = best_native_response( + PriceRanking::MaxOutAmount, + vec![ + error(PriceEstimationError::RateLimited), + error(PriceEstimationError::ProtocolInternal(anyhow::anyhow!("!"))), + ], + ) + .await; + assert_eq!(best, error(PriceEstimationError::RateLimited)); + } + + /// Any native price estimate, no matter how bad, is preferred over an + /// error. + #[tokio::test] + async fn prefer_estimate_over_error_native() { + let best = best_native_response( + PriceRanking::MaxOutAmount, + vec![native_price(1.), error(PriceEstimationError::RateLimited)], + ) + .await; + assert_eq!(best, native_price(1.)); } } From 8694659d320d7bcf9667c8542d2bdbb08b988df4 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Fri, 12 Jan 2024 15:00:18 +0100 Subject: [PATCH 07/16] Rename `priceImprovement` to `Surplus` (#2273) # Description Renames FeePolicy variant from `priceImprovement` to `surplus`, since it better explains the current implementation of the protocol fee in the driver. Context: https://github.com/cowprotocol/services/pull/2213#pullrequestreview-1814990012 https://cowservices.slack.com/archives/C05N1FNP3MX/p1704971316558629 I expect shadow solver to fail again until next release. --- crates/autopilot/src/arguments.rs | 27 ++++++------ crates/autopilot/src/domain/fee/mod.rs | 22 +++++----- .../src/infra/persistence/dto/order.rs | 10 ++--- crates/driver/openapi.yml | 8 ++-- .../src/domain/competition/order/fees.rs | 18 ++++---- .../src/domain/competition/solution/fee.rs | 20 ++++----- .../src/infra/api/routes/solve/dto/auction.rs | 6 +-- crates/driver/src/tests/setup/driver.rs | 2 +- crates/e2e/tests/e2e/protocol_fee.rs | 41 +++++++++---------- crates/orderbook/src/dto/order.rs | 2 +- 10 files changed, 73 insertions(+), 83 deletions(-) diff --git a/crates/autopilot/src/arguments.rs b/crates/autopilot/src/arguments.rs index 49e14e6e05..d4a4395cf4 100644 --- a/crates/autopilot/src/arguments.rs +++ b/crates/autopilot/src/arguments.rs @@ -347,15 +347,15 @@ impl std::fmt::Display for Arguments { pub struct FeePolicy { /// Type of fee policy to use. Examples: /// - /// - Price improvement without cap - /// price_improvement:0.5:1.0 + /// - Surplus without cap + /// surplus:0.5:1.0 /// - /// - Price improvement with cap: - /// price_improvement:0.5:0.06 + /// - Surplus with cap: + /// surplus:0.5:0.06 /// /// - Volume based: /// volume:0.1 - #[clap(long, env, default_value = "priceImprovement:0.0:1.0")] + #[clap(long, env, default_value = "surplus:0.0:1.0")] pub fee_policy_kind: FeePolicyKind, /// Should protocol fees be collected or skipped for orders whose @@ -368,10 +368,10 @@ pub struct FeePolicy { impl FeePolicy { pub fn to_domain(self) -> domain::fee::Policy { match self.fee_policy_kind { - FeePolicyKind::PriceImprovement { + FeePolicyKind::Surplus { factor, max_volume_factor, - } => domain::fee::Policy::PriceImprovement { + } => domain::fee::Policy::Surplus { factor, max_volume_factor, }, @@ -382,9 +382,8 @@ impl FeePolicy { #[derive(clap::Parser, Debug, Clone)] pub enum FeePolicyKind { - /// How much of the order's price improvement over max(limit price, - /// best_bid) should be taken as a protocol fee. - PriceImprovement { factor: f64, max_volume_factor: f64 }, + /// How much of the order's surplus should be taken as a protocol fee. + Surplus { factor: f64, max_volume_factor: f64 }, /// How much of the order's volume should be taken as a protocol fee. Volume { factor: f64 }, } @@ -396,18 +395,18 @@ impl FromStr for FeePolicyKind { let mut parts = s.split(':'); let kind = parts.next().ok_or("missing fee policy kind")?; match kind { - "priceImprovement" => { + "surplus" => { let factor = parts .next() - .ok_or("missing price improvement factor")? + .ok_or("missing surplus factor")? .parse::() - .map_err(|e| format!("invalid price improvement factor: {}", e))?; + .map_err(|e| format!("invalid surplus factor: {}", e))?; let max_volume_factor = parts .next() .ok_or("missing max volume factor")? .parse::() .map_err(|e| format!("invalid max volume factor: {}", e))?; - Ok(Self::PriceImprovement { + Ok(Self::Surplus { factor, max_volume_factor, }) diff --git a/crates/autopilot/src/domain/fee/mod.rs b/crates/autopilot/src/domain/fee/mod.rs index 2323742484..0d994dae18 100644 --- a/crates/autopilot/src/domain/fee/mod.rs +++ b/crates/autopilot/src/domain/fee/mod.rs @@ -63,19 +63,17 @@ impl ProtocolFee { #[derive(Debug, Copy, Clone, PartialEq)] pub enum Policy { - /// If the order receives more than expected (positive deviation from - /// quoted amounts) pay the protocol a factor of the achieved - /// improvement. The fee is taken in `sell` token for `buy` - /// orders and in `buy` token for `sell` orders. - PriceImprovement { - /// Factor of price improvement the protocol charges as a fee. - /// Price improvement is the difference between executed price and - /// limit price or quoted price (whichever is better) + /// If the order receives more than limit price, take the protocol fee as a + /// percentage of the difference. The fee is taken in `sell` token for + /// `buy` orders and in `buy` token for `sell` orders. + Surplus { + /// Factor of surplus the protocol charges as a fee. + /// Surplus is the difference between executed price and limit price /// - /// E.g. if a user received 2000USDC for 1ETH while having been - /// quoted 1990USDC, their price improvement is 10USDC. - /// A factor of 0.5 requires the solver to pay 5USDC to - /// the protocol for settling this order. + /// E.g. if a user received 2000USDC for 1ETH while having a limit price + /// of 1990USDC, their surplus is 10USDC. A factor of 0.5 + /// requires the solver to pay 5USDC to the protocol for + /// settling this order. factor: f64, /// Cap protocol fee with a percentage of the order's volume. max_volume_factor: f64, diff --git a/crates/autopilot/src/infra/persistence/dto/order.rs b/crates/autopilot/src/infra/persistence/dto/order.rs index 797d1baf28..be99951138 100644 --- a/crates/autopilot/src/infra/persistence/dto/order.rs +++ b/crates/autopilot/src/infra/persistence/dto/order.rs @@ -270,7 +270,7 @@ impl From for domain::auction::order::EcdsaSignature { #[serde(rename_all = "camelCase")] pub enum FeePolicy { #[serde(rename_all = "camelCase")] - PriceImprovement { factor: f64, max_volume_factor: f64 }, + Surplus { factor: f64, max_volume_factor: f64 }, #[serde(rename_all = "camelCase")] Volume { factor: f64 }, } @@ -278,10 +278,10 @@ pub enum FeePolicy { impl From for FeePolicy { fn from(policy: domain::fee::Policy) -> Self { match policy { - domain::fee::Policy::PriceImprovement { + domain::fee::Policy::Surplus { factor, max_volume_factor, - } => Self::PriceImprovement { + } => Self::Surplus { factor, max_volume_factor, }, @@ -293,10 +293,10 @@ impl From for FeePolicy { impl From for domain::fee::Policy { fn from(policy: FeePolicy) -> Self { match policy { - FeePolicy::PriceImprovement { + FeePolicy::Surplus { factor, max_volume_factor, - } => Self::PriceImprovement { + } => Self::Surplus { factor, max_volume_factor, }, diff --git a/crates/driver/openapi.yml b/crates/driver/openapi.yml index b7094bdb15..97f76d60a2 100644 --- a/crates/driver/openapi.yml +++ b/crates/driver/openapi.yml @@ -418,16 +418,16 @@ components: The solver should make sure the fee policy is applied when computing their solution. type: object oneOf: - - $ref: "#/components/schemas/PriceImprovementFee" + - $ref: "#/components/schemas/SurplusFee" - $ref: "#/components/schemas/VolumeFee" - PriceImprovementFee: + SurplusFee: description: | - If the order receives more than expected (positive deviation from quoted amounts) pay the protocol a factor of the achieved improvement. + If the order receives more than limit price, pay the protocol a factor of the difference. type: object properties: kind: type: string - enum: ["priceImprovement"] + enum: ["surplus"] maxVolumeFactor: description: Never charge more than that percentage of the order volume. type: number diff --git a/crates/driver/src/domain/competition/order/fees.rs b/crates/driver/src/domain/competition/order/fees.rs index 6ef53cd075..0419a35c69 100644 --- a/crates/driver/src/domain/competition/order/fees.rs +++ b/crates/driver/src/domain/competition/order/fees.rs @@ -1,16 +1,14 @@ #[derive(Clone, Debug)] pub enum FeePolicy { - /// If the order receives more than expected (positive deviation from quoted - /// amounts) pay the protocol a factor of the achieved improvement. - /// The fee is taken in `sell` token for `buy` orders and in `buy` - /// token for `sell` orders. - PriceImprovement { - /// Factor of price improvement the protocol charges as a fee. - /// Price improvement is the difference between executed price and - /// limit price or quoted price (whichever is better) + /// If the order receives more than limit price, take the protocol fee as a + /// percentage of the difference. The fee is taken in `sell` token for + /// `buy` orders and in `buy` token for `sell` orders. + Surplus { + /// Factor of surplus the protocol charges as a fee. + /// Surplus is the difference between executed price and limit price /// - /// E.g. if a user received 2000USDC for 1ETH while having been quoted - /// 1990USDC, their price improvement is 10USDC. A factor of 0.5 + /// E.g. if a user received 2000USDC for 1ETH while having a limit price + /// of 1990USDC, their surplus is 10USDC. A factor of 0.5 /// requires the solver to pay 5USDC to the protocol for /// settling this order. factor: f64, diff --git a/crates/driver/src/domain/competition/solution/fee.rs b/crates/driver/src/domain/competition/solution/fee.rs index 987260e404..441f65fbb6 100644 --- a/crates/driver/src/domain/competition/solution/fee.rs +++ b/crates/driver/src/domain/competition/solution/fee.rs @@ -75,26 +75,22 @@ impl Fulfillment { } match self.order().fee_policies.first() { - Some(FeePolicy::PriceImprovement { + Some(FeePolicy::Surplus { factor, max_volume_factor, }) => { - let price_improvement_fee = self.price_improvement_fee(prices, *factor)?; - let max_volume_fee = self.volume_fee(prices, *max_volume_factor)?; + let fee_from_surplus = self.fee_from_surplus(prices, *factor)?; + let fee_from_volume = self.fee_from_volume(prices, *max_volume_factor)?; // take the smaller of the two - tracing::debug!(uid=?self.order().uid, price_improvement_fee=?price_improvement_fee, max_volume_fee=?max_volume_fee, protocol_fee=?(std::cmp::min(price_improvement_fee, max_volume_fee)), executed=?self.executed(), surplus_fee=?self.surplus_fee(), "calculated protocol fee"); - Ok(std::cmp::min(price_improvement_fee, max_volume_fee)) + tracing::debug!(uid=?self.order().uid, fee_from_surplus=?fee_from_surplus, fee_from_volume=?fee_from_volume, protocol_fee=?(std::cmp::min(fee_from_surplus, fee_from_volume)), executed=?self.executed(), surplus_fee=?self.surplus_fee(), "calculated protocol fee"); + Ok(std::cmp::min(fee_from_surplus, fee_from_volume)) } - Some(FeePolicy::Volume { factor }) => self.volume_fee(prices, *factor), + Some(FeePolicy::Volume { factor }) => self.fee_from_volume(prices, *factor), None => Ok(0.into()), } } - fn price_improvement_fee( - &self, - prices: ClearingPrices, - factor: f64, - ) -> Result { + fn fee_from_surplus(&self, prices: ClearingPrices, factor: f64) -> Result { let sell_amount = self.order().sell.amount.0; let buy_amount = self.order().buy.amount.0; let executed = self.executed().0; @@ -163,7 +159,7 @@ impl Fulfillment { apply_factor(surplus_in_sell_token, factor) } - fn volume_fee(&self, prices: ClearingPrices, factor: f64) -> Result { + fn fee_from_volume(&self, prices: ClearingPrices, factor: f64) -> Result { let executed = self.executed().0; let executed_sell_amount = match self.order().side { Side::Buy => { diff --git a/crates/driver/src/infra/api/routes/solve/dto/auction.rs b/crates/driver/src/infra/api/routes/solve/dto/auction.rs index 77d87b0f66..2bca940a27 100644 --- a/crates/driver/src/infra/api/routes/solve/dto/auction.rs +++ b/crates/driver/src/infra/api/routes/solve/dto/auction.rs @@ -119,10 +119,10 @@ impl Auction { .fee_policies .into_iter() .map(|policy| match policy { - FeePolicy::PriceImprovement { + FeePolicy::Surplus { factor, max_volume_factor, - } => competition::order::FeePolicy::PriceImprovement { + } => competition::order::FeePolicy::Surplus { factor, max_volume_factor, }, @@ -309,7 +309,7 @@ enum Class { #[serde(rename_all = "camelCase", deny_unknown_fields)] enum FeePolicy { #[serde(rename_all = "camelCase")] - PriceImprovement { factor: f64, max_volume_factor: f64 }, + Surplus { factor: f64, max_volume_factor: f64 }, #[serde(rename_all = "camelCase")] Volume { factor: f64 }, } diff --git a/crates/driver/src/tests/setup/driver.rs b/crates/driver/src/tests/setup/driver.rs index f7c3bfdd65..d64fc3608b 100644 --- a/crates/driver/src/tests/setup/driver.rs +++ b/crates/driver/src/tests/setup/driver.rs @@ -98,7 +98,7 @@ pub fn solve_req(test: &Test) -> serde_json::Value { order::Kind::Market => json!([]), order::Kind::Liquidity => json!([]), order::Kind::Limit { .. } => json!([{ - "priceImprovement": { + "surplus": { "factor": 0.0, "maxVolumeFactor": 0.06 } diff --git a/crates/e2e/tests/e2e/protocol_fee.rs b/crates/e2e/tests/e2e/protocol_fee.rs index 8c4045bb7f..a1bb5550c9 100644 --- a/crates/e2e/tests/e2e/protocol_fee.rs +++ b/crates/e2e/tests/e2e/protocol_fee.rs @@ -15,14 +15,14 @@ use { #[tokio::test] #[ignore] -async fn local_node_price_improvement_fee_sell_order() { - run_test(price_improvement_fee_sell_order_test).await; +async fn local_node_surplus_fee_sell_order() { + run_test(surplus_fee_sell_order_test).await; } #[tokio::test] #[ignore] -async fn local_node_price_improvement_fee_sell_order_capped() { - run_test(price_improvement_fee_sell_order_capped_test).await; +async fn local_node_surplus_fee_sell_order_capped() { + run_test(surplus_fee_sell_order_capped_test).await; } #[tokio::test] @@ -33,14 +33,14 @@ async fn local_node_volume_fee_sell_order() { #[tokio::test] #[ignore] -async fn local_node_price_improvement_fee_buy_order() { - run_test(price_improvement_fee_buy_order_test).await; +async fn local_node_surplus_fee_buy_order() { + run_test(surplus_fee_buy_order_test).await; } #[tokio::test] #[ignore] -async fn local_node_price_improvement_fee_buy_order_capped() { - run_test(price_improvement_fee_buy_order_capped_test).await; +async fn local_node_surplus_fee_buy_order_capped() { + run_test(surplus_fee_buy_order_capped_test).await; } #[tokio::test] @@ -49,8 +49,8 @@ async fn local_node_volume_fee_buy_order() { run_test(volume_fee_buy_order_test).await; } -async fn price_improvement_fee_sell_order_test(web3: Web3) { - let fee_policy = FeePolicyKind::PriceImprovement { +async fn surplus_fee_sell_order_test(web3: Web3) { + let fee_policy = FeePolicyKind::Surplus { factor: 0.3, max_volume_factor: 1.0, }; @@ -82,8 +82,8 @@ async fn price_improvement_fee_sell_order_test(web3: Web3) { .await; } -async fn price_improvement_fee_sell_order_capped_test(web3: Web3) { - let fee_policy = FeePolicyKind::PriceImprovement { +async fn surplus_fee_sell_order_capped_test(web3: Web3) { + let fee_policy = FeePolicyKind::Surplus { factor: 1.0, max_volume_factor: 0.1, }; @@ -132,8 +132,8 @@ async fn volume_fee_sell_order_test(web3: Web3) { .await; } -async fn price_improvement_fee_buy_order_test(web3: Web3) { - let fee_policy = FeePolicyKind::PriceImprovement { +async fn surplus_fee_buy_order_test(web3: Web3) { + let fee_policy = FeePolicyKind::Surplus { factor: 0.3, max_volume_factor: 1.0, }; @@ -161,8 +161,8 @@ async fn price_improvement_fee_buy_order_test(web3: Web3) { .await; } -async fn price_improvement_fee_buy_order_capped_test(web3: Web3) { - let fee_policy = FeePolicyKind::PriceImprovement { +async fn surplus_fee_buy_order_capped_test(web3: Web3) { + let fee_policy = FeePolicyKind::Surplus { factor: 1.0, max_volume_factor: 0.1, }; @@ -353,9 +353,8 @@ async fn execute_test( } enum FeePolicyKind { - /// How much of the order's price improvement over max(limit price, - /// best_bid) should be taken as a protocol fee. - PriceImprovement { factor: f64, max_volume_factor: f64 }, + /// How much of the order's surplus should be taken as a protocol fee. + Surplus { factor: f64, max_volume_factor: f64 }, /// How much of the order's volume should be taken as a protocol fee. Volume { factor: f64 }, } @@ -363,12 +362,12 @@ enum FeePolicyKind { impl std::fmt::Display for FeePolicyKind { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { - FeePolicyKind::PriceImprovement { + FeePolicyKind::Surplus { factor, max_volume_factor, } => write!( f, - "--fee-policy-kind=priceImprovement:{}:{}", + "--fee-policy-kind=surplus:{}:{}", factor, max_volume_factor ), FeePolicyKind::Volume { factor } => { diff --git a/crates/orderbook/src/dto/order.rs b/crates/orderbook/src/dto/order.rs index 8413fcd9b0..dffda385ae 100644 --- a/crates/orderbook/src/dto/order.rs +++ b/crates/orderbook/src/dto/order.rs @@ -50,7 +50,7 @@ pub struct Order { #[serde(rename_all = "camelCase")] pub enum FeePolicy { #[serde(rename_all = "camelCase")] - PriceImprovement { factor: f64, max_volume_factor: f64 }, + Surplus { factor: f64, max_volume_factor: f64 }, #[serde(rename_all = "camelCase")] Volume { factor: f64 }, } From 7ba88b2e66a2fc85b0126a4e363e47816e662743 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Fri, 12 Jan 2024 16:10:42 +0100 Subject: [PATCH 08/16] Rename fee policies (#2274) # Description Fixes https://github.com/cowprotocol/services/issues/2256 Simple renaming from `fee_policies` to `protocol_fees` to make sure the reader knows that the policies are used for protocol fee calculation. --- crates/autopilot/src/boundary/order.rs | 4 ++-- crates/autopilot/src/domain/auction/order.rs | 2 +- .../src/infra/persistence/dto/order.rs | 6 +++--- crates/autopilot/src/solvable_orders.rs | 10 +++++----- crates/driver/openapi.yml | 2 +- .../src/domain/competition/order/mod.rs | 4 ++-- .../src/domain/competition/solution/fee.rs | 4 ++-- crates/driver/src/domain/quote.rs | 2 +- .../src/infra/api/routes/solve/dto/auction.rs | 6 +++--- crates/driver/src/tests/setup/driver.rs | 20 +++++++++---------- crates/orderbook/src/dto/order.rs | 2 +- 11 files changed, 31 insertions(+), 31 deletions(-) diff --git a/crates/autopilot/src/boundary/order.rs b/crates/autopilot/src/boundary/order.rs index d7f25bc229..d7d40d26fc 100644 --- a/crates/autopilot/src/boundary/order.rs +++ b/crates/autopilot/src/boundary/order.rs @@ -2,7 +2,7 @@ use {crate::domain, shared::remaining_amounts}; pub fn to_domain( order: model::order::Order, - fee_policies: Vec, + protocol_fees: Vec, ) -> domain::Order { let remaining_order = remaining_amounts::Order::from(order.clone()); let order_is_untouched = remaining_order.executed_amount.is_zero(); @@ -15,6 +15,7 @@ pub fn to_domain( buy_amount: order.data.buy_amount, solver_fee: order.metadata.full_fee_amount, user_fee: order.data.fee_amount, + protocol_fees, valid_to: order.data.valid_to, kind: order.data.kind.into(), receiver: order.data.receiver, @@ -35,6 +36,5 @@ pub fn to_domain( class: order.metadata.class.into(), app_data: order.data.app_data.into(), signature: order.signature.into(), - fee_policies, } } diff --git a/crates/autopilot/src/domain/auction/order.rs b/crates/autopilot/src/domain/auction/order.rs index 976f2bcb9b..a06ad6b7e6 100644 --- a/crates/autopilot/src/domain/auction/order.rs +++ b/crates/autopilot/src/domain/auction/order.rs @@ -12,6 +12,7 @@ pub struct Order { pub buy_amount: U256, pub solver_fee: U256, pub user_fee: U256, + pub protocol_fees: Vec, pub kind: Kind, pub class: Class, pub valid_to: u32, @@ -27,7 +28,6 @@ pub struct Order { pub buy_token_balance: BuyTokenDestination, pub app_data: AppDataHash, pub signature: Signature, - pub fee_policies: Vec, } // uid as 56 bytes: 32 for orderDigest, 20 for ownerAddress and 4 for validTo diff --git a/crates/autopilot/src/infra/persistence/dto/order.rs b/crates/autopilot/src/infra/persistence/dto/order.rs index be99951138..3e2edfe1e6 100644 --- a/crates/autopilot/src/infra/persistence/dto/order.rs +++ b/crates/autopilot/src/infra/persistence/dto/order.rs @@ -24,6 +24,7 @@ pub struct Order { pub solver_fee: U256, #[serde_as(as = "HexOrDecimalU256")] pub user_fee: U256, + pub protocol_fees: Vec, pub valid_to: u32, pub kind: boundary::OrderKind, pub receiver: Option, @@ -40,7 +41,6 @@ pub struct Order { pub app_data: boundary::AppDataHash, #[serde(flatten)] pub signature: boundary::Signature, - pub fee_policies: Vec, } pub fn from_domain(order: domain::Order) -> Order { @@ -52,6 +52,7 @@ pub fn from_domain(order: domain::Order) -> Order { buy_amount: order.buy_amount, solver_fee: order.solver_fee, user_fee: order.user_fee, + protocol_fees: order.protocol_fees.into_iter().map(Into::into).collect(), valid_to: order.valid_to, kind: order.kind.into(), receiver: order.receiver, @@ -69,7 +70,6 @@ pub fn from_domain(order: domain::Order) -> Order { class: order.class.into(), app_data: order.app_data.into(), signature: order.signature.into(), - fee_policies: order.fee_policies.into_iter().map(Into::into).collect(), } } @@ -82,6 +82,7 @@ pub fn to_domain(order: Order) -> domain::Order { buy_amount: order.buy_amount, solver_fee: order.solver_fee, user_fee: order.user_fee, + protocol_fees: order.protocol_fees.into_iter().map(Into::into).collect(), valid_to: order.valid_to, kind: order.kind.into(), receiver: order.receiver, @@ -99,7 +100,6 @@ pub fn to_domain(order: Order) -> domain::Order { class: order.class.into(), app_data: order.app_data.into(), signature: order.signature.into(), - fee_policies: order.fee_policies.into_iter().map(Into::into).collect(), } } diff --git a/crates/autopilot/src/solvable_orders.rs b/crates/autopilot/src/solvable_orders.rs index 51d36be6c8..9a76d11ff8 100644 --- a/crates/autopilot/src/solvable_orders.rs +++ b/crates/autopilot/src/solvable_orders.rs @@ -77,7 +77,7 @@ pub struct SolvableOrdersCache { ethflow_contract_address: Option, weth: H160, limit_order_price_factor: BigDecimal, - fee_policy: domain::ProtocolFee, + protocol_fee: domain::ProtocolFee, } type Balances = HashMap; @@ -102,7 +102,7 @@ impl SolvableOrdersCache { ethflow_contract_address: Option, weth: H160, limit_order_price_factor: BigDecimal, - fee_policy: domain::ProtocolFee, + protocol_fee: domain::ProtocolFee, ) -> Arc { let self_ = Arc::new(Self { min_order_validity_period, @@ -120,7 +120,7 @@ impl SolvableOrdersCache { ethflow_contract_address, weth, limit_order_price_factor, - fee_policy, + protocol_fee, }); tokio::task::spawn( update_task(Arc::downgrade(&self_), update_interval, current_block) @@ -240,8 +240,8 @@ impl SolvableOrdersCache { .into_iter() .map(|order| { let quote = db_solvable_orders.quotes.get(&order.metadata.uid.into()); - let fee_policies = self.fee_policy.get(&order, quote); - boundary::order::to_domain(order, fee_policies) + let protocol_fees = self.protocol_fee.get(&order, quote); + boundary::order::to_domain(order, protocol_fees) }) .collect(), prices, diff --git a/crates/driver/openapi.yml b/crates/driver/openapi.yml index 97f76d60a2..7f644db14f 100644 --- a/crates/driver/openapi.yml +++ b/crates/driver/openapi.yml @@ -258,7 +258,7 @@ components: signature: description: Hex encoded bytes with `0x` prefix. type: string - feePolicies: + protocolFees: description: | Any protocol fee policies that apply to the order. diff --git a/crates/driver/src/domain/competition/order/mod.rs b/crates/driver/src/domain/competition/order/mod.rs index 4135ab54bf..96be3492ec 100644 --- a/crates/driver/src/domain/competition/order/mod.rs +++ b/crates/driver/src/domain/competition/order/mod.rs @@ -43,7 +43,7 @@ pub struct Order { /// The types of fees the protocol collects from the winning solver. /// Unless otherwise configured, the driver modifies solutions to take /// sufficient fee in the form of positive slippage. - pub fee_policies: Vec, + pub protocol_fees: Vec, } /// An amount denominated in the sell token of an [`Order`]. @@ -458,7 +458,7 @@ mod tests { data: Default::default(), signer: Default::default(), }, - fee_policies: Default::default(), + protocol_fees: Default::default(), }; assert_eq!( diff --git a/crates/driver/src/domain/competition/solution/fee.rs b/crates/driver/src/domain/competition/solution/fee.rs index 441f65fbb6..f9a468a9bf 100644 --- a/crates/driver/src/domain/competition/solution/fee.rs +++ b/crates/driver/src/domain/competition/solution/fee.rs @@ -70,11 +70,11 @@ impl Fulfillment { fn protocol_fee(&self, prices: ClearingPrices) -> Result { // TODO: support multiple fee policies - if self.order().fee_policies.len() > 1 { + if self.order().protocol_fees.len() > 1 { return Err(Error::MultipleFeePolicies); } - match self.order().fee_policies.first() { + match self.order().protocol_fees.first() { Some(FeePolicy::Surplus { factor, max_volume_factor, diff --git a/crates/driver/src/domain/quote.rs b/crates/driver/src/domain/quote.rs index 8a5142b48d..d99bca6cbd 100644 --- a/crates/driver/src/domain/quote.rs +++ b/crates/driver/src/domain/quote.rs @@ -131,7 +131,7 @@ impl Order { data: Default::default(), signer: Default::default(), }, - fee_policies: Default::default(), + protocol_fees: Default::default(), }], [ auction::Token { diff --git a/crates/driver/src/infra/api/routes/solve/dto/auction.rs b/crates/driver/src/infra/api/routes/solve/dto/auction.rs index 2bca940a27..d131b9813a 100644 --- a/crates/driver/src/infra/api/routes/solve/dto/auction.rs +++ b/crates/driver/src/infra/api/routes/solve/dto/auction.rs @@ -115,8 +115,8 @@ impl Auction { data: order.signature.into(), signer: order.owner.into(), }, - fee_policies: order - .fee_policies + protocol_fees: order + .protocol_fees .into_iter() .map(|policy| match policy { FeePolicy::Surplus { @@ -230,6 +230,7 @@ struct Order { solver_fee: eth::U256, #[serde_as(as = "serialize::U256")] user_fee: eth::U256, + protocol_fees: Vec, valid_to: u32, kind: Kind, receiver: Option, @@ -250,7 +251,6 @@ struct Order { signing_scheme: SigningScheme, #[serde_as(as = "serialize::Hex")] signature: Vec, - fee_policies: Vec, } #[derive(Debug, Deserialize)] diff --git a/crates/driver/src/tests/setup/driver.rs b/crates/driver/src/tests/setup/driver.rs index d64fc3608b..cd6d1dafe0 100644 --- a/crates/driver/src/tests/setup/driver.rs +++ b/crates/driver/src/tests/setup/driver.rs @@ -73,6 +73,16 @@ pub fn solve_req(test: &Test) -> serde_json::Value { "buyAmount": quote.buy_amount().to_string(), "solverFee": quote.order.user_fee.to_string(), "userFee": quote.order.user_fee.to_string(), + "protocolFees": match quote.order.kind { + order::Kind::Market => json!([]), + order::Kind::Liquidity => json!([]), + order::Kind::Limit { .. } => json!([{ + "surplus": { + "factor": 0.0, + "maxVolumeFactor": 0.06 + } + }]), + }, "validTo": u32::try_from(time::now().timestamp()).unwrap() + quote.order.valid_for.0, "kind": match quote.order.side { order::Side::Sell => "sell", @@ -94,16 +104,6 @@ pub fn solve_req(test: &Test) -> serde_json::Value { "appData": "0x0000000000000000000000000000000000000000000000000000000000000000", "signingScheme": "eip712", "signature": format!("0x{}", hex::encode(quote.order_signature(&test.blockchain))), - "feePolicies": match quote.order.kind { - order::Kind::Market => json!([]), - order::Kind::Liquidity => json!([]), - order::Kind::Limit { .. } => json!([{ - "surplus": { - "factor": 0.0, - "maxVolumeFactor": 0.06 - } - }]), - }, })); } for fulfillment in test.fulfillments.iter() { diff --git a/crates/orderbook/src/dto/order.rs b/crates/orderbook/src/dto/order.rs index dffda385ae..76fcce13ee 100644 --- a/crates/orderbook/src/dto/order.rs +++ b/crates/orderbook/src/dto/order.rs @@ -26,6 +26,7 @@ pub struct Order { pub solver_fee: U256, #[serde_as(as = "HexOrDecimalU256")] pub user_fee: U256, + pub protocol_fees: Vec, pub valid_to: u32, pub kind: OrderKind, pub receiver: Option, @@ -42,7 +43,6 @@ pub struct Order { pub app_data: AppDataHash, #[serde(flatten)] pub signature: Signature, - pub fee_policies: Vec, } #[serde_as] From 5122abec9c92642ce9b04f7382d379336a28f9f9 Mon Sep 17 00:00:00 2001 From: Felix Leupold Date: Fri, 12 Jan 2024 19:01:18 +0100 Subject: [PATCH 09/16] Add personal access token to release action (#2269) # Description The automated release action didn't trigger a deployment job, because > When you use GITHUB_TOKEN in your actions, all of the interactions with the repository are on behalf of the Github-actions bot. The operations act by Github-actions bot cannot trigger a new workflow run. # Changes - [X] use a personal access token instead of the github access token which triggers the build. ## How to test Test Repository: https://github.com/fleupold/weekly-release-action/actions --- .github/workflows/release.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 478c60b2a0..4665430088 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -14,6 +14,8 @@ jobs: uses: actions/checkout@v2 with: fetch-depth: 0 # Fetch all history for all branches and tags + # Requires "Read and Write access to code" permission + token: ${{ secrets.RELEASE_ACTION_ACCESS_TOKEN }} - name: Create Tag id: tag_version @@ -29,8 +31,6 @@ jobs: NEW_MINOR=$((MINOR+1)) NEW_TAG="${MAJOR}.${NEW_MINOR}.0" echo ::set-output name=tag::$NEW_TAG - git config --local user.email "action@github.com" - git config --local user.name "GitHub Action" git tag $NEW_TAG git push origin --tags From 398351d4d83af8a7f9eb3a9547e72c35b2d9fa3e Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Fri, 12 Jan 2024 22:20:29 +0100 Subject: [PATCH 10/16] Store fee policy (#2118) # Description Fixes https://github.com/cowprotocol/services/issues/2111 Defines the table for storing fee policies per ``. Currently supports taking fees as: 1. cut from a surplus - the price difference between the executed price and limit_price. Optionally, for this type, we also have a CAP on max volume (expressed in the same way as (2) 2. percent of volume (or more precise, percent of the `executed_amount`, so refers to sell token for sell orders and buy token for buy orders). Will be merged at the very end of feature implementation, since db migrations are irreversible. # Changes - [x] Created table for fee policies - [x] Implemented basic database function to read/write ## How to test Roundtrip UT --------- Co-authored-by: ilya Co-authored-by: Martin Beckmann --- .github/workflows/pull-request.yaml | 4 +- crates/autopilot/src/arguments.rs | 10 +- crates/autopilot/src/boundary/mod.rs | 2 +- crates/autopilot/src/database.rs | 24 ++--- crates/autopilot/src/database/fee_policies.rs | 102 ++++++++++++++++++ .../src/database/onchain_order_events.rs | 2 +- crates/autopilot/src/database/order_events.rs | 2 +- .../src/infra/persistence/dto/fee_policy.rs | 62 +++++++++++ .../src/infra/persistence/dto/mod.rs | 6 +- .../src/infra/persistence/dto/quote.rs | 4 +- crates/autopilot/src/infra/persistence/mod.rs | 27 +++++ crates/autopilot/src/run.rs | 2 +- crates/autopilot/src/run_loop.rs | 23 ++++ crates/e2e/tests/e2e/onchain_settlement.rs | 2 - database/README.md | 26 +++++ database/sql/V057__add_fee_policy.sql | 19 ++++ 16 files changed, 286 insertions(+), 31 deletions(-) create mode 100644 crates/autopilot/src/database/fee_policies.rs create mode 100644 crates/autopilot/src/infra/persistence/dto/fee_policy.rs create mode 100644 database/sql/V057__add_fee_policy.sql diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index cab51f7a02..f35d56578b 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -54,13 +54,13 @@ jobs: - uses: Swatinem/rust-cache@v2 # Start the build process in the background. The following cargo test command will automatically # wait for the build process to be done before proceeding. - - run: cargo build -p orderbook -p database --tests & + - run: cargo build -p orderbook -p database -p autopilot --tests & - uses: taiki-e/install-action@nextest - uses: yu-ichiro/spin-up-docker-compose-action@v1 with: file: docker-compose.yaml up-opts: -d db migrations - - run: cargo nextest run postgres -p orderbook -p database --test-threads 1 --run-ignored ignored-only + - run: cargo nextest run postgres -p orderbook -p database -p autopilot --test-threads 1 --run-ignored ignored-only test-local-node: timeout-minutes: 60 diff --git a/crates/autopilot/src/arguments.rs b/crates/autopilot/src/arguments.rs index d4a4395cf4..782657811b 100644 --- a/crates/autopilot/src/arguments.rs +++ b/crates/autopilot/src/arguments.rs @@ -54,7 +54,7 @@ pub struct Arguments { /// The number of order events to insert in a single batch. #[clap(long, env, default_value = "500")] - pub order_events_insert_batch_size: NonZeroUsize, + pub insert_batch_size: NonZeroUsize, /// Skip syncing past events (useful for local deployments) #[clap(long, env, action = clap::ArgAction::Set, default_value = "false")] @@ -254,7 +254,7 @@ impl std::fmt::Display for Arguments { order_events_cleanup_interval, order_events_cleanup_threshold, db_url, - order_events_insert_batch_size, + insert_batch_size, native_price_estimation_results_required, auction_update_interval, max_settlement_transaction_wait, @@ -322,11 +322,7 @@ impl std::fmt::Display for Arguments { "order_events_cleanup_threshold: {:?}", order_events_cleanup_threshold )?; - writeln!( - f, - "order_events_insert_batch_size: {}", - order_events_insert_batch_size - )?; + writeln!(f, "insert_batch_size: {}", insert_batch_size)?; writeln!( f, "native_price_estimation_results_required: {}", diff --git a/crates/autopilot/src/boundary/mod.rs b/crates/autopilot/src/boundary/mod.rs index a13d044bf2..83c8a4fc50 100644 --- a/crates/autopilot/src/boundary/mod.rs +++ b/crates/autopilot/src/boundary/mod.rs @@ -3,7 +3,7 @@ pub use { competition::Competition, order_events::{store_order_events, OrderEventLabel}, }, - database::orders::Quote as DatabaseQuote, + database, model::{ app_data::AppDataHash, interaction::InteractionData, diff --git a/crates/autopilot/src/database.rs b/crates/autopilot/src/database.rs index 6a3c6e3c60..319bd6aa11 100644 --- a/crates/autopilot/src/database.rs +++ b/crates/autopilot/src/database.rs @@ -1,9 +1,16 @@ +use { + sqlx::{Executor, PgConnection, PgPool}, + std::{num::NonZeroUsize, time::Duration}, + tracing::Instrument, +}; + mod auction; pub mod auction_prices; pub mod auction_transaction; pub mod competition; pub mod ethflow_events; mod events; +pub mod fee_policies; pub mod on_settlement_event_updater; pub mod onchain_order_events; pub mod order_events; @@ -11,15 +18,9 @@ pub mod orders; mod quotes; pub mod recent_settlements; -use { - sqlx::{Executor, PgConnection, PgPool}, - std::{num::NonZeroUsize, time::Duration}, - tracing::Instrument, -}; - #[derive(Debug, Clone)] pub struct Config { - pub order_events_insert_batch_size: NonZeroUsize, + pub insert_batch_size: NonZeroUsize, } #[derive(Debug, Clone)] @@ -29,15 +30,10 @@ pub struct Postgres { } impl Postgres { - pub async fn new( - url: &str, - order_events_insert_batch_size: NonZeroUsize, - ) -> sqlx::Result { + pub async fn new(url: &str, insert_batch_size: NonZeroUsize) -> sqlx::Result { Ok(Self { pool: PgPool::connect(url).await?, - config: Config { - order_events_insert_batch_size, - }, + config: Config { insert_batch_size }, }) } diff --git a/crates/autopilot/src/database/fee_policies.rs b/crates/autopilot/src/database/fee_policies.rs new file mode 100644 index 0000000000..0796d6ba6c --- /dev/null +++ b/crates/autopilot/src/database/fee_policies.rs @@ -0,0 +1,102 @@ +use { + crate::infra::persistence::dto, + sqlx::{PgConnection, QueryBuilder}, +}; + +pub async fn insert_batch( + ex: &mut PgConnection, + fee_policies: impl IntoIterator, +) -> Result<(), sqlx::Error> { + let mut query_builder = QueryBuilder::new( + "INSERT INTO fee_policies (auction_id, order_uid, kind, surplus_factor, \ + max_volume_factor, volume_factor) ", + ); + + query_builder.push_values(fee_policies, |mut b, fee_policy| { + b.push_bind(fee_policy.auction_id) + .push_bind(fee_policy.order_uid) + .push_bind(fee_policy.kind) + .push_bind(fee_policy.surplus_factor) + .push_bind(fee_policy.max_volume_factor) + .push_bind(fee_policy.volume_factor); + }); + + query_builder.build().execute(ex).await.map(|_| ()) +} + +pub async fn fetch( + ex: &mut PgConnection, + auction_id: dto::AuctionId, + order_uid: database::OrderUid, +) -> Result, sqlx::Error> { + const QUERY: &str = r#" + SELECT * FROM fee_policies + WHERE auction_id = $1 AND order_uid = $2 + ORDER BY application_order + "#; + let rows = sqlx::query_as::<_, dto::FeePolicy>(QUERY) + .bind(auction_id) + .bind(order_uid) + .fetch_all(ex) + .await? + .into_iter() + .collect(); + Ok(rows) +} + +#[cfg(test)] +mod tests { + use {super::*, database::byte_array::ByteArray, sqlx::Connection}; + + #[tokio::test] + #[ignore] + async fn postgres_roundtrip() { + let mut db = PgConnection::connect("postgresql://").await.unwrap(); + let mut db = db.begin().await.unwrap(); + database::clear_DANGER_(&mut db).await.unwrap(); + + // same primary key for all fee policies + let (auction_id, order_uid) = (1, ByteArray([1; 56])); + + // surplus fee policy without caps + let fee_policy_1 = dto::FeePolicy { + auction_id, + order_uid, + kind: dto::fee_policy::FeePolicyKind::Surplus, + surplus_factor: Some(0.1), + max_volume_factor: Some(1.0), + volume_factor: None, + }; + // surplus fee policy with caps + let fee_policy_2 = dto::FeePolicy { + auction_id, + order_uid, + kind: dto::fee_policy::FeePolicyKind::Surplus, + surplus_factor: Some(0.2), + max_volume_factor: Some(0.05), + volume_factor: None, + }; + // volume based fee policy + let fee_policy_3 = dto::FeePolicy { + auction_id, + order_uid, + kind: dto::fee_policy::FeePolicyKind::Volume, + surplus_factor: None, + max_volume_factor: None, + volume_factor: Some(0.06), + }; + insert_batch( + &mut db, + vec![ + fee_policy_1.clone(), + fee_policy_2.clone(), + fee_policy_3.clone(), + ], + ) + .await + .unwrap(); + + let output = fetch(&mut db, 1, order_uid).await.unwrap(); + assert_eq!(output, vec![fee_policy_1, fee_policy_2, fee_policy_3]); + } +} diff --git a/crates/autopilot/src/database/onchain_order_events.rs b/crates/autopilot/src/database/onchain_order_events.rs index 532e4aa419..6bfe1795d0 100644 --- a/crates/autopilot/src/database/onchain_order_events.rs +++ b/crates/autopilot/src/database/onchain_order_events.rs @@ -1379,7 +1379,7 @@ mod test { db: Postgres { pool: PgPool::connect_lazy("postgresql://").unwrap(), config: Config { - order_events_insert_batch_size: NonZeroUsize::new(500).unwrap(), + insert_batch_size: NonZeroUsize::new(500).unwrap(), }, }, web3, diff --git a/crates/autopilot/src/database/order_events.rs b/crates/autopilot/src/database/order_events.rs index f4f6382586..85912d8971 100644 --- a/crates/autopilot/src/database/order_events.rs +++ b/crates/autopilot/src/database/order_events.rs @@ -40,7 +40,7 @@ pub async fn store_order_events( timestamp: DateTime, ) -> Result<()> { let mut ex = db.pool.begin().await.context("begin transaction")?; - for chunk in events.chunks(db.config.order_events_insert_batch_size.get()) { + for chunk in events.chunks(db.config.insert_batch_size.get()) { let batch = chunk.iter().map(|(uid, label)| OrderEvent { order_uid: ByteArray(uid.0), timestamp, diff --git a/crates/autopilot/src/infra/persistence/dto/fee_policy.rs b/crates/autopilot/src/infra/persistence/dto/fee_policy.rs new file mode 100644 index 0000000000..bfbd52ec2e --- /dev/null +++ b/crates/autopilot/src/infra/persistence/dto/fee_policy.rs @@ -0,0 +1,62 @@ +use crate::{boundary, domain}; + +#[derive(Debug, Clone, PartialEq, sqlx::FromRow)] +pub struct FeePolicy { + pub auction_id: domain::AuctionId, + pub order_uid: boundary::database::OrderUid, + pub kind: FeePolicyKind, + pub surplus_factor: Option, + pub max_volume_factor: Option, + pub volume_factor: Option, +} + +impl FeePolicy { + pub fn from_domain( + auction_id: domain::AuctionId, + order_uid: domain::OrderUid, + policy: domain::fee::Policy, + ) -> Self { + match policy { + domain::fee::Policy::Surplus { + factor, + max_volume_factor, + } => Self { + auction_id, + order_uid: boundary::database::byte_array::ByteArray(order_uid.0), + kind: FeePolicyKind::Surplus, + surplus_factor: Some(factor), + max_volume_factor: Some(max_volume_factor), + volume_factor: None, + }, + domain::fee::Policy::Volume { factor } => Self { + auction_id, + order_uid: boundary::database::byte_array::ByteArray(order_uid.0), + kind: FeePolicyKind::Volume, + surplus_factor: None, + max_volume_factor: None, + volume_factor: Some(factor), + }, + } + } +} + +impl From for domain::fee::Policy { + fn from(row: FeePolicy) -> domain::fee::Policy { + match row.kind { + FeePolicyKind::Surplus => domain::fee::Policy::Surplus { + factor: row.surplus_factor.expect("missing surplus factor"), + max_volume_factor: row.max_volume_factor.expect("missing max volume factor"), + }, + FeePolicyKind::Volume => domain::fee::Policy::Volume { + factor: row.volume_factor.expect("missing volume factor"), + }, + } + } +} + +#[derive(Debug, Clone, PartialEq, sqlx::Type)] +#[sqlx(type_name = "PolicyKind", rename_all = "lowercase")] +pub enum FeePolicyKind { + Surplus, + Volume, +} diff --git a/crates/autopilot/src/infra/persistence/dto/mod.rs b/crates/autopilot/src/infra/persistence/dto/mod.rs index 176c75a13b..aca7bbdc36 100644 --- a/crates/autopilot/src/infra/persistence/dto/mod.rs +++ b/crates/autopilot/src/infra/persistence/dto/mod.rs @@ -1,5 +1,9 @@ pub mod auction; +pub mod fee_policy; pub mod order; pub mod quote; -pub use auction::{Auction, AuctionId, AuctionWithId}; +pub use { + auction::{Auction, AuctionId, AuctionWithId}, + fee_policy::FeePolicy, +}; diff --git a/crates/autopilot/src/infra/persistence/dto/quote.rs b/crates/autopilot/src/infra/persistence/dto/quote.rs index 23daf1ae95..52097ca48f 100644 --- a/crates/autopilot/src/infra/persistence/dto/quote.rs +++ b/crates/autopilot/src/infra/persistence/dto/quote.rs @@ -3,7 +3,9 @@ use { number::conversions::big_decimal_to_u256, }; -pub fn into_domain(quote: boundary::DatabaseQuote) -> Result { +pub fn into_domain( + quote: boundary::database::orders::Quote, +) -> Result { Ok(domain::Quote { order_uid: domain::OrderUid(quote.order_uid.0), sell_amount: big_decimal_to_u256("e.sell_amount).ok_or(AmountOverflow)?, diff --git a/crates/autopilot/src/infra/persistence/mod.rs b/crates/autopilot/src/infra/persistence/mod.rs index cd858c6c33..c3a776fe81 100644 --- a/crates/autopilot/src/infra/persistence/mod.rs +++ b/crates/autopilot/src/infra/persistence/mod.rs @@ -1,6 +1,8 @@ use { crate::{boundary, database::Postgres, domain}, + anyhow::Context, chrono::Utc, + itertools::Itertools, std::sync::Arc, tokio::time::Instant, tracing::Instrument, @@ -96,6 +98,31 @@ impl Persistence { .instrument(tracing::Span::current()), ); } + + /// Saves the given fee policies to the DB as a single batch. + pub async fn store_fee_policies( + &self, + auction_id: domain::AuctionId, + fee_policies: Vec<(domain::OrderUid, Vec)>, + ) -> anyhow::Result<()> { + let fee_policies = fee_policies + .into_iter() + .flat_map(|(order_uid, policies)| { + policies + .into_iter() + .map(move |policy| dto::FeePolicy::from_domain(auction_id, order_uid, policy)) + }) + .collect_vec(); + + let mut ex = self.postgres.pool.begin().await.context("begin")?; + for chunk in fee_policies.chunks(self.postgres.config.insert_batch_size.get()) { + crate::database::fee_policies::insert_batch(&mut ex, chunk.iter().cloned()) + .await + .context("fee_policies::insert_batch")?; + } + + ex.commit().await.context("commit") + } } #[derive(Debug, thiserror::Error)] diff --git a/crates/autopilot/src/run.rs b/crates/autopilot/src/run.rs index b918151880..db7f7173c3 100644 --- a/crates/autopilot/src/run.rs +++ b/crates/autopilot/src/run.rs @@ -109,7 +109,7 @@ pub async fn start(args: impl Iterator) { pub async fn run(args: Arguments) { assert!(args.shadow.is_none(), "cannot run in shadow mode"); - let db = Postgres::new(args.db_url.as_str(), args.order_events_insert_batch_size) + let db = Postgres::new(args.db_url.as_str(), args.insert_batch_size) .await .unwrap(); crate::database::run_database_metrics_work(db.clone()); diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index a91b324e51..044e8ccd00 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -163,6 +163,7 @@ impl RunLoop { .collect::>(); let mut prices = BTreeMap::new(); + let mut fee_policies = Vec::new(); let block_deadline = competition_simulation_block + self.submission_deadline + self.additional_deadline_for_rewards; @@ -176,6 +177,7 @@ impl RunLoop { .find(|auction_order| &auction_order.uid == order_id); match auction_order { Some(auction_order) => { + fee_policies.push((auction_order.uid, auction_order.protocol_fees.clone())); if let Some(price) = auction.prices.get(&auction_order.sell_token) { prices.insert(auction_order.sell_token, *price); } else { @@ -268,6 +270,16 @@ impl RunLoop { return; } + tracing::info!("saving fee policies"); + if let Err(err) = self + .persistence + .store_fee_policies(auction_id, fee_policies) + .await + { + Metrics::fee_policies_store_error(); + tracing::warn!(?err, "failed to save fee policies"); + } + tracing::info!(driver = %driver.name, "settling"); let submission_start = Instant::now(); match self.settle(driver, solution).await { @@ -589,6 +601,10 @@ struct Metrics { /// solution together with the winning driver that did't include it. #[metric(labels("ignored_by"))] matched_unsettled: prometheus::IntCounterVec, + + /// Tracks the number of database errors. + #[metric(labels("error_type"))] + db_metric_error: prometheus::IntCounterVec, } impl Metrics { @@ -677,6 +693,13 @@ impl Metrics { .with_label_values(&[&winning.name]) .inc_by(unsettled.len() as u64); } + + fn fee_policies_store_error() { + Self::get() + .db_metric_error + .with_label_values(&["fee_policies_store"]) + .inc(); + } } pub mod observe { diff --git a/crates/e2e/tests/e2e/onchain_settlement.rs b/crates/e2e/tests/e2e/onchain_settlement.rs index 84a70ba173..d4ae524a8c 100644 --- a/crates/e2e/tests/e2e/onchain_settlement.rs +++ b/crates/e2e/tests/e2e/onchain_settlement.rs @@ -132,8 +132,6 @@ async fn onchain_settlement(web3: Web3) { // Only start the autopilot now to ensure that these orders are settled in a // batch which seems to be expected in this test. - // However, this currently does not work because the driver will not merge the - // individual solutions because the token prices don't match after scaling. services.start_autopilot(vec![ "--drivers=test_solver|http://localhost:11088/test_solver".to_string(), ]); diff --git a/database/README.md b/database/README.md index 8ab922adab..f92a9f811e 100644 --- a/database/README.md +++ b/database/README.md @@ -248,6 +248,32 @@ Column | Type | Nullable | Details Indexes: - PRIMARY KEY: btree(`uid`) +### fee_policies + +Contains all relevant data of fee policies applied to orders during auctions. + +Column | Type | Nullable | Details +--------------------------|------------------------------|----------|-------- + auction_id | bigint | not null | unique identifier for the auction + order_uid | bytea | not null | 56 bytes identifier linking to the order in the `orders` table + application_order | serial | not null | the order in which the fee policies are inserted and applied + kind | [PolicyKind](#policykind) | not null | type of the fee policy, defined in the PolicyKind enum + surplus_factor | double precision | | percentage of the surplus for fee calculation; value is between 0 and 1 + max_volume_factor | double precision | | cap for the fee as a percentage of the order volume; value is between 0 and 1 + volume_factor | double precision | | fee percentage of the order volume; value is between 0 and 1 + +Indexes: +- PRIMARY KEY: composite key(`auction_id`, `order_uid`, `application_order`) + +#### Enums + +- #### PolicyKind + Enum for the `kind` column in `fee_policies` table. + + Values: + - `surplus`: The fee is based on the surplus achieved in the trade. + - `volume`: The fee is based on the volume of the order. + ### presignature\_events Stores data of [`PreSignature`](https://github.com/cowprotocol/contracts/blob/5e5c28877c1690415548de7bc4b5502f87e7f222/src/contracts/mixins/GPv2Signing.sol#L59-L61) events. This is a mechanism where users can supply a signature for an order\_uid even before creating the original order in the backend. These events can give or revoke a signature. diff --git a/database/sql/V057__add_fee_policy.sql b/database/sql/V057__add_fee_policy.sql new file mode 100644 index 0000000000..73b7b26807 --- /dev/null +++ b/database/sql/V057__add_fee_policy.sql @@ -0,0 +1,19 @@ +CREATE TYPE PolicyKind AS ENUM ('surplus', 'volume'); + +CREATE TABLE fee_policies ( + auction_id bigint NOT NULL, + order_uid bytea NOT NULL, + -- The order in which the fee policies are inserted and applied. + application_order SERIAL NOT NULL, + + -- The type of the fee policy. + kind PolicyKind NOT NULL, + -- The fee should be taken as a percentage of the surplus. The value is between 0 and 1. + surplus_factor double precision, + -- Cap the fee at a certain percentage of the order volume. The value is between 0 and 1. + max_volume_factor double precision, + -- The fee should be taken as a percentage of the order volume. The value is between 0 and 1. + volume_factor double precision, + + PRIMARY KEY (auction_id, order_uid, application_order) +); \ No newline at end of file From 74fdf64e07d803363eca624e0c30315ebc7a8a37 Mon Sep 17 00:00:00 2001 From: KRD <44735306+KRD-Kai@users.noreply.github.com> Date: Mon, 15 Jan 2024 10:33:12 +0000 Subject: [PATCH 11/16] Improve autopilot liveness check (#2236) # Description This PR creates a shared liveness implementation between shadow and regular autopilot mode. Both now populate a thread-safe last auction timestamp whenever an auction has processed. The liveness check compares the elapsed time since that recorded timestamp with the maximum auction age. # Changes - [x] Liveness checks are based on the last timestamp an auction runloop has successfully completed. - [x] The same liveness implementation is used across shadow and regular autopilot mode. Regular autopilot no longer uses the last update time in the solvable orders cache. ## How to test This can be tested manually by running the autopilot locally and checking http://localhost:9589/liveness. It responds with 200 if the autopilot is considered alive, 503 otherwise. Max auction age can also be tweaked using the --max-auction-age argument when running the autopilot. ## Related Issues - Fixes #2090 --- crates/autopilot/src/run.rs | 41 +++++++++++++++++++++++--------- crates/autopilot/src/run_loop.rs | 4 ++++ crates/autopilot/src/shadow.rs | 18 ++++++-------- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/crates/autopilot/src/run.rs b/crates/autopilot/src/run.rs index db7f7173c3..3bea17fd32 100644 --- a/crates/autopilot/src/run.rs +++ b/crates/autopilot/src/run.rs @@ -56,21 +56,39 @@ use { token_info::{CachedTokenInfoFetcher, TokenInfoFetcher}, token_list::{AutoUpdatingTokenList, TokenListConfiguration}, }, - std::{collections::HashSet, sync::Arc, time::Duration}, + std::{ + collections::HashSet, + sync::{Arc, RwLock}, + time::{Duration, Instant}, + }, tracing::Instrument, url::Url, }; -struct Liveness { - solvable_orders_cache: Arc, +pub struct Liveness { max_auction_age: Duration, + last_auction_time: RwLock, } #[async_trait::async_trait] impl LivenessChecking for Liveness { async fn is_alive(&self) -> bool { - let age = self.solvable_orders_cache.last_update_time().elapsed(); - age <= self.max_auction_age + let last_auction_time = self.last_auction_time.read().unwrap(); + let auction_age = last_auction_time.elapsed(); + auction_age <= self.max_auction_age + } +} + +impl Liveness { + pub fn new(max_auction_age: Duration) -> Liveness { + Liveness { + max_auction_age, + last_auction_time: RwLock::new(Instant::now()), + } + } + + pub fn auction(&self) { + *self.last_auction_time.write().unwrap() = Instant::now(); } } @@ -549,11 +567,9 @@ pub async fn run(args: Arguments) { .update(block) .await .expect("failed to perform initial solvable orders update"); - let liveness = Liveness { - max_auction_age: args.max_auction_age, - solvable_orders_cache: solvable_orders_cache.clone(), - }; - shared::metrics::serve_metrics(Arc::new(liveness), args.metrics_address); + + let liveness = Arc::new(Liveness::new(args.max_auction_age)); + shared::metrics::serve_metrics(liveness.clone(), args.metrics_address); let on_settlement_event_updater = crate::on_settlement_event_updater::OnSettlementEventUpdater { @@ -607,6 +623,7 @@ pub async fn run(args: Arguments) { in_flight_orders: Default::default(), persistence: infra::persistence::Persistence::new(args.s3.into().unwrap(), Arc::new(db)) .await, + liveness: liveness.clone(), }; run.run_forever().await; unreachable!("run loop exited"); @@ -653,7 +670,8 @@ async fn shadow_mode(args: Arguments) -> ! { .await }; - shared::metrics::serve_metrics(Arc::new(shadow::Liveness), args.metrics_address); + let liveness = Arc::new(Liveness::new(args.max_auction_age)); + shared::metrics::serve_metrics(liveness.clone(), args.metrics_address); let shadow = shadow::RunLoop::new( orderbook, @@ -661,6 +679,7 @@ async fn shadow_mode(args: Arguments) -> ! { trusted_tokens, args.score_cap, args.solve_deadline, + liveness.clone(), ); shadow.run_forever().await; diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 044e8ccd00..703ef86af6 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -9,6 +9,7 @@ use { solve::{self, TradedAmounts}, }, infra::{self, persistence::dto}, + run::Liveness, solvable_orders::SolvableOrdersCache, }, ::observe::metrics, @@ -49,6 +50,7 @@ pub struct RunLoop { pub max_settlement_transaction_wait: Duration, pub solve_deadline: Duration, pub in_flight_orders: Arc>, + pub liveness: Arc, } impl RunLoop { @@ -64,6 +66,8 @@ impl RunLoop { || last_block.replace(current_block) != Some(current_block) { observe::log_auction_delta(id, &previous, &auction); + self.liveness.auction(); + self.single_run(id, auction) .instrument(tracing::info_span!("auction", id)) .await; diff --git a/crates/autopilot/src/shadow.rs b/crates/autopilot/src/shadow.rs index 7684a0a98f..9f69c1d5f8 100644 --- a/crates/autopilot/src/shadow.rs +++ b/crates/autopilot/src/shadow.rs @@ -16,26 +16,18 @@ use { solve::{self}, }, infra, + run::Liveness, run_loop::{self, observe}, }, ::observe::metrics, number::nonzero::U256 as NonZeroU256, primitive_types::{H160, U256}, rand::seq::SliceRandom, - shared::{metrics::LivenessChecking, token_list::AutoUpdatingTokenList}, - std::{cmp, time::Duration}, + shared::token_list::AutoUpdatingTokenList, + std::{cmp, sync::Arc, time::Duration}, tracing::Instrument, }; -pub struct Liveness; -#[async_trait::async_trait] -impl LivenessChecking for Liveness { - async fn is_alive(&self) -> bool { - // can we somehow check that we keep processing auctions? - true - } -} - pub struct RunLoop { orderbook: infra::shadow::Orderbook, drivers: Vec, @@ -44,6 +36,7 @@ pub struct RunLoop { block: u64, score_cap: U256, solve_deadline: Duration, + liveness: Arc, } impl RunLoop { @@ -53,6 +46,7 @@ impl RunLoop { trusted_tokens: AutoUpdatingTokenList, score_cap: U256, solve_deadline: Duration, + liveness: Arc, ) -> Self { Self { orderbook, @@ -62,6 +56,7 @@ impl RunLoop { block: 0, score_cap, solve_deadline, + liveness, } } @@ -74,6 +69,7 @@ impl RunLoop { }; observe::log_auction_delta(id, &previous, &auction); previous = Some(auction.clone()); + self.liveness.auction(); self.single_run(id, auction) .instrument(tracing::info_span!("auction", id)) From cb759e73a42458c6ab793aa3bff74d596330f94b Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Mon, 15 Jan 2024 12:32:54 +0100 Subject: [PATCH 12/16] Fixes openapi for new Auction (#2270) # Description Follow up for https://github.com/cowprotocol/services/pull/2246 now the `/api/v1/auction` refers to the `AuctionOrder` which has it's own list of fields. --- crates/orderbook/openapi.yml | 147 ++++++++++++++++++++++++++++++++++- crates/solvers/openapi.yml | 2 +- 2 files changed, 147 insertions(+), 2 deletions(-) diff --git a/crates/orderbook/openapi.yml b/crates/orderbook/openapi.yml index 7696a1103e..9e928ecb66 100644 --- a/crates/orderbook/openapi.yml +++ b/crates/orderbook/openapi.yml @@ -901,6 +901,115 @@ components: allOf: - $ref: "#/components/schemas/OrderCreation" - $ref: "#/components/schemas/OrderMetaData" + AuctionOrder: + description: | + A solvable order included in the current batch auction. Contains the data forwarded to solvers for solving. + type: object + properties: + uid: + $ref: "#/components/schemas/UID" + sellToken: + description: see `OrderParameters::sellToken` + allOf: + - $ref: "#/components/schemas/Address" + buyToken: + description: see `OrderParameters::buyToken` + allOf: + - $ref: "#/components/schemas/Address" + sellAmount: + description: see `OrderParameters::sellAmount` + allOf: + - $ref: "#/components/schemas/TokenAmount" + buyAmount: + description: see `OrderParameters::buyAmount` + allOf: + - $ref: "#/components/schemas/TokenAmount" + userFee: + description: see `OrderParameters::feeAmount` + allOf: + - $ref: "#/components/schemas/TokenAmount" + solverFee: + description: Amount that the signed fee would be without subsidies. + allOf: + - $ref: "#/components/schemas/TokenAmount" + validTo: + description: see `OrderParameters::validTo` + type: integer + kind: + description: see `OrderParameters::kind` + allOf: + - $ref: "#/components/schemas/OrderKind" + receiver: + description: see `OrderParameters::receiver` + allOf: + - $ref: "#/components/schemas/Address" + nullable: true + owner: + $ref: "#/components/schemas/Address" + partiallyFillable: + description: see `OrderParameters::partiallyFillable` + type: boolean + executed: + description: | + Currently executed amount of sell/buy token, depending on the order kind. + allOf: + - $ref: "#/components/schemas/TokenAmount" + preInteractions: + description: | + The pre-interactions that need to be executed before the first execution of the order. + type: array + items: + $ref: "#/components/schemas/InteractionData" + postInteractions: + description: | + The post-interactions that need to be executed after the execution of the order. + type: array + items: + $ref: "#/components/schemas/InteractionData" + sellTokenBalance: + description: see `OrderParameters::sellTokenBalance` + allOf: + - $ref: "#/components/schemas/SellTokenSource" + default: "erc20" + buyTokenBalance: + description: see `OrderParameters::buyTokenBalance` + allOf: + - $ref: "#/components/schemas/BuyTokenDestination" + default: "erc20" + class: + $ref: "#/components/schemas/OrderClass" + appData: + $ref: "#/components/schemas/AppDataHash" + signature: + $ref: "#/components/schemas/Signature" + protocolFees: + description: | + The fee policies that are used to compute the protocol fees for this order. + type: array + items: + $ref: "#/components/schemas/FeePolicy" + required: + - uid + - sellToken + - buyToken + - sellAmount + - buyAmount + - userFee + - solverFee + - validTo + - kind + - receiver + - owner + - partiallyFillable + - executed + - preInteractions + - postInteractions + - sellTokenBalance + - buyTokenBalance + - class + - appData + - signature + - protocolFees Auction: description: | A batch auction for solving. @@ -925,7 +1034,7 @@ components: orders: type: array items: - $ref: "#/components/schemas/Order" + $ref: "#/components/schemas/AuctionOrder" description: | The solvable orders included in the auction. prices: @@ -1448,3 +1557,39 @@ components: totalSurplus: type: string description: The total surplus. + InteractionData: + type: object + properties: + target: + $ref: "#/components/schemas/Address" + value: + $ref: "#/components/schemas/TokenAmount" + call_data: + type: array + items: + $ref: "#/components/schemas/CallData" + description: The call data to be used for the interaction. + Surplus: + description: The protocol fee is taken as a percent of the surplus. + type: object + properties: + factor: + type: number + max_volume_factor: + type: number + required: + - factor + - max_volume_factor + Volume: + description: The protocol fee is taken as a percent of the order volume. + type: object + properties: + factor: + type: number + required: + - factor + FeePolicy: + description: Defines the ways to calculate the protocol fee. + oneOf: + - $ref: '#/components/schemas/Surplus' + - $ref: '#/components/schemas/Volume' diff --git a/crates/solvers/openapi.yml b/crates/solvers/openapi.yml index d8688eb177..86b365b260 100644 --- a/crates/solvers/openapi.yml +++ b/crates/solvers/openapi.yml @@ -721,7 +721,7 @@ components: - score properties: id: - description: An opaque identifier for the solution. + description: An opaque identifier for the solution. This is a solver generated number that is unique across multiple solutions within the auction. type: number prices: description: | From c86b374065b660d185978fbd37cb8a4b4a8f7aae Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Mon, 15 Jan 2024 13:51:48 +0100 Subject: [PATCH 13/16] Move driver_api to `infra` (#2278) # Description Third part of the https://github.com/cowprotocol/services/pull/2188 Moves driver_api to a separate `solvers` module in `infra`. Next up, moving the driver dtos into `infra`. --- crates/autopilot/src/boundary/mod.rs | 9 ++-- crates/autopilot/src/infra/mod.rs | 3 +- .../{driver_api.rs => infra/solvers/mod.rs} | 47 ++++++++++++++----- crates/autopilot/src/lib.rs | 2 +- crates/autopilot/src/run.rs | 13 +++-- crates/autopilot/src/run_loop.rs | 29 ++++++------ crates/autopilot/src/shadow.rs | 9 ++-- crates/autopilot/src/util/mod.rs | 14 ++++++ 8 files changed, 86 insertions(+), 40 deletions(-) rename crates/autopilot/src/{driver_api.rs => infra/solvers/mod.rs} (62%) create mode 100644 crates/autopilot/src/util/mod.rs diff --git a/crates/autopilot/src/boundary/mod.rs b/crates/autopilot/src/boundary/mod.rs index 83c8a4fc50..24ab9ee8e9 100644 --- a/crates/autopilot/src/boundary/mod.rs +++ b/crates/autopilot/src/boundary/mod.rs @@ -1,7 +1,10 @@ pub use { - crate::database::{ - competition::Competition, - order_events::{store_order_events, OrderEventLabel}, + crate::{ + database::{ + competition::Competition, + order_events::{store_order_events, OrderEventLabel}, + }, + driver_model::{reveal, settle, solve}, }, database, model::{ diff --git a/crates/autopilot/src/infra/mod.rs b/crates/autopilot/src/infra/mod.rs index 2d87c85a9c..ccaae143ba 100644 --- a/crates/autopilot/src/infra/mod.rs +++ b/crates/autopilot/src/infra/mod.rs @@ -1,5 +1,6 @@ pub mod blockchain; pub mod persistence; pub mod shadow; +pub mod solvers; -pub use {blockchain::Ethereum, persistence::Persistence}; +pub use {blockchain::Ethereum, persistence::Persistence, solvers::Driver}; diff --git a/crates/autopilot/src/driver_api.rs b/crates/autopilot/src/infra/solvers/mod.rs similarity index 62% rename from crates/autopilot/src/driver_api.rs rename to crates/autopilot/src/infra/solvers/mod.rs index b1c4d56552..0c9ce16f1b 100644 --- a/crates/autopilot/src/driver_api.rs +++ b/crates/autopilot/src/infra/solvers/mod.rs @@ -1,8 +1,7 @@ use { - crate::driver_model::{reveal, settle, solve}, + crate::{boundary, util}, anyhow::{anyhow, Context, Result}, reqwest::Client, - shared::{arguments::ExternalSolver, http_client::response_body_with_size_limit}, std::time::Duration, url::Url, }; @@ -17,10 +16,10 @@ pub struct Driver { } impl Driver { - pub fn new(driver: ExternalSolver) -> Self { + pub fn new(url: Url, name: String) -> Self { Self { - name: driver.name, - url: driver.url, + name, + url, client: Client::builder() .timeout(RESPONSE_TIME_LIMIT) .build() @@ -28,19 +27,25 @@ impl Driver { } } - pub async fn solve(&self, request: &solve::Request) -> Result { + pub async fn solve( + &self, + request: &boundary::solve::Request, + ) -> Result { self.request_response("solve", request, None).await } - pub async fn reveal(&self, request: &reveal::Request) -> Result { + pub async fn reveal( + &self, + request: &boundary::reveal::Request, + ) -> Result { self.request_response("reveal", request, None).await } pub async fn settle( &self, - request: &settle::Request, + request: &boundary::settle::Request, timeout: std::time::Duration, - ) -> Result { + ) -> Result { self.request_response("settle", request, Some(timeout)) .await } @@ -54,11 +59,11 @@ impl Driver { where Response: serde::de::DeserializeOwned, { - let url = shared::url::join(&self.url, path); + let url = util::join(&self.url, path); tracing::trace!( path=&url.path(), body=%serde_json::to_string_pretty(request).unwrap(), - "request", + "solver request", ); let mut request = self.client.post(url.clone()).json(request); @@ -72,7 +77,7 @@ impl Driver { .await .context("body")?; let text = String::from_utf8_lossy(&body); - tracing::trace!(%status, body=%text, "response"); + tracing::trace!(%status, body=%text, "solver response"); let context = || format!("url {url}, body {text:?}"); if status != 200 { return Err(anyhow!("bad status {status}, {}", context())); @@ -80,3 +85,21 @@ impl Driver { serde_json::from_slice(&body).with_context(|| format!("bad json {}", context())) } } + +/// Extracts the bytes of the response up to some size limit. +/// +/// Returns an error if the byte limit was exceeded. +pub async fn response_body_with_size_limit( + response: &mut reqwest::Response, + limit: usize, +) -> Result> { + let mut bytes = Vec::new(); + while let Some(chunk) = response.chunk().await? { + let slice: &[u8] = &chunk; + if bytes.len() + slice.len() > limit { + return Err(anyhow!("size limit exceeded")); + } + bytes.extend_from_slice(slice); + } + Ok(bytes) +} diff --git a/crates/autopilot/src/lib.rs b/crates/autopilot/src/lib.rs index 19807fd535..a86b130f90 100644 --- a/crates/autopilot/src/lib.rs +++ b/crates/autopilot/src/lib.rs @@ -3,7 +3,6 @@ pub mod boundary; pub mod database; pub mod decoded_settlement; pub mod domain; -pub mod driver_api; pub mod driver_model; pub mod event_updater; pub mod infra; @@ -13,5 +12,6 @@ pub mod run; pub mod run_loop; pub mod shadow; pub mod solvable_orders; +pub mod util; pub use self::run::{run, start}; diff --git a/crates/autopilot/src/run.rs b/crates/autopilot/src/run.rs index 3bea17fd32..4075009766 100644 --- a/crates/autopilot/src/run.rs +++ b/crates/autopilot/src/run.rs @@ -11,7 +11,6 @@ use { Postgres, }, domain, - driver_api::Driver, event_updater::{EventUpdater, GPv2SettlementContract}, infra::{self}, run_loop::RunLoop, @@ -613,7 +612,11 @@ pub async fn run(args: Arguments) { let run = RunLoop { eth, solvable_orders_cache, - drivers: args.drivers.into_iter().map(Driver::new).collect(), + drivers: args + .drivers + .into_iter() + .map(|driver| infra::Driver::new(driver.url, driver.name)) + .collect(), market_makable_token_list, submission_deadline: args.submission_deadline as u64, additional_deadline_for_rewards: args.additional_deadline_for_rewards as u64, @@ -637,7 +640,11 @@ async fn shadow_mode(args: Arguments) -> ! { args.shadow.expect("missing shadow mode configuration"), ); - let drivers = args.drivers.into_iter().map(Driver::new).collect(); + let drivers = args + .drivers + .into_iter() + .map(|driver| infra::Driver::new(driver.url, driver.name)) + .collect(); let trusted_tokens = { let web3 = shared::ethrpc::web3( diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 703ef86af6..47139271fa 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -2,7 +2,6 @@ use { crate::{ database::competition::Competition, domain::{self, auction::order::Class}, - driver_api::Driver, driver_model::{ reveal::{self, Request}, settle, @@ -40,9 +39,9 @@ use { pub struct RunLoop { pub eth: infra::Ethereum, pub persistence: infra::Persistence, + pub drivers: Vec, pub solvable_orders_cache: Arc, - pub drivers: Vec, pub market_makable_token_list: AutoUpdatingTokenList, pub submission_deadline: u64, pub additional_deadline_for_rewards: u64, @@ -364,7 +363,7 @@ impl RunLoop { /// Computes a driver's solutions for the solver competition. async fn solve( &self, - driver: &Driver, + driver: &infra::Driver, request: &solve::Request, ) -> Result>, SolveError> { let response = tokio::time::timeout(self.solve_deadline, driver.solve(request)) @@ -397,7 +396,7 @@ impl RunLoop { /// Ask the winning solver to reveal their solution. async fn reveal( &self, - driver: &Driver, + driver: &infra::Driver, auction: domain::AuctionId, solution_id: u64, ) -> Result { @@ -418,7 +417,7 @@ impl RunLoop { /// Execute the solver's solution. Returns Ok when the corresponding /// transaction has been mined. - async fn settle(&self, driver: &Driver, solved: &Solution) -> Result<(), SettleError> { + async fn settle(&self, driver: &infra::Driver, solved: &Solution) -> Result<(), SettleError> { let events = solved .order_ids() .map(|uid| (*uid, OrderEventLabel::Executing)) @@ -524,7 +523,7 @@ pub struct InFlightOrders { } struct Participant<'a> { - driver: &'a Driver, + driver: &'a infra::Driver, solution: Solution, } @@ -620,14 +619,14 @@ impl Metrics { Self::get().auction.set(auction_id) } - fn solve_ok(driver: &Driver, elapsed: Duration) { + fn solve_ok(driver: &infra::Driver, elapsed: Duration) { Self::get() .solve .with_label_values(&[&driver.name, "success"]) .observe(elapsed.as_secs_f64()) } - fn solve_err(driver: &Driver, elapsed: Duration, err: &SolveError) { + fn solve_err(driver: &infra::Driver, elapsed: Duration, err: &SolveError) { let label = match err { SolveError::Timeout => "timeout", SolveError::NoSolutions => "no_solutions", @@ -639,28 +638,28 @@ impl Metrics { .observe(elapsed.as_secs_f64()) } - fn solution_ok(driver: &Driver) { + fn solution_ok(driver: &infra::Driver) { Self::get() .solutions .with_label_values(&[&driver.name, "success"]) .inc(); } - fn solution_err(driver: &Driver, _: &ZeroScoreError) { + fn solution_err(driver: &infra::Driver, _: &ZeroScoreError) { Self::get() .solutions .with_label_values(&[&driver.name, "zero_score"]) .inc(); } - fn reveal_ok(driver: &Driver) { + fn reveal_ok(driver: &infra::Driver) { Self::get() .reveal .with_label_values(&[&driver.name, "success"]) .inc(); } - fn reveal_err(driver: &Driver, err: &RevealError) { + fn reveal_err(driver: &infra::Driver, err: &RevealError) { let label = match err { RevealError::AuctionMismatch => "mismatch", RevealError::Failure(_) => "error", @@ -671,14 +670,14 @@ impl Metrics { .inc(); } - fn settle_ok(driver: &Driver, time: Duration) { + fn settle_ok(driver: &infra::Driver, time: Duration) { Self::get() .settle_time .with_label_values(&[&driver.name, "success"]) .inc_by(time.as_millis().try_into().unwrap_or(u64::MAX)); } - fn settle_err(driver: &Driver, err: &SettleError, time: Duration) { + fn settle_err(driver: &infra::Driver, err: &SettleError, time: Duration) { let label = match err { SettleError::Failure(_) => "error", }; @@ -688,7 +687,7 @@ impl Metrics { .inc_by(time.as_millis().try_into().unwrap_or(u64::MAX)); } - fn matched_unsettled(winning: &Driver, unsettled: &[&domain::OrderUid]) { + fn matched_unsettled(winning: &infra::Driver, unsettled: &[&domain::OrderUid]) { if !unsettled.is_empty() { tracing::debug!(?unsettled, "some orders were matched but not settled"); } diff --git a/crates/autopilot/src/shadow.rs b/crates/autopilot/src/shadow.rs index 9f69c1d5f8..0ab7a78106 100644 --- a/crates/autopilot/src/shadow.rs +++ b/crates/autopilot/src/shadow.rs @@ -10,7 +10,6 @@ use { crate::{ domain::{self, auction::order::Class}, - driver_api::Driver, driver_model::{ reveal, solve::{self}, @@ -30,7 +29,7 @@ use { pub struct RunLoop { orderbook: infra::shadow::Orderbook, - drivers: Vec, + drivers: Vec, trusted_tokens: AutoUpdatingTokenList, auction: domain::AuctionId, block: u64, @@ -42,7 +41,7 @@ pub struct RunLoop { impl RunLoop { pub fn new( orderbook: infra::shadow::Orderbook, - drivers: Vec, + drivers: Vec, trusted_tokens: AutoUpdatingTokenList, score_cap: U256, solve_deadline: Duration, @@ -210,7 +209,7 @@ impl RunLoop { /// Computes a driver's solutions in the shadow competition. async fn participate( &self, - driver: &Driver, + driver: &infra::Driver, request: &solve::Request, ) -> Result { let proposed = tokio::time::timeout(self.solve_deadline, driver.solve(request)) @@ -253,7 +252,7 @@ impl RunLoop { } struct Participant<'a> { - driver: &'a Driver, + driver: &'a infra::Driver, solution: Result, } diff --git a/crates/autopilot/src/util/mod.rs b/crates/autopilot/src/util/mod.rs new file mode 100644 index 0000000000..bb1babe21b --- /dev/null +++ b/crates/autopilot/src/util/mod.rs @@ -0,0 +1,14 @@ +use url::Url; + +/// Joins a path with a URL, ensuring that there is only one slash between them. +/// It doesn't matter if the URL ends with a slash or the path starts with one. +pub fn join(url: &Url, mut path: &str) -> Url { + let mut url = url.to_string(); + while url.ends_with('/') { + url.pop(); + } + while path.starts_with('/') { + path = &path[1..] + } + Url::parse(&format!("{url}/{path}")).unwrap() +} From 72ce082ba267061bbc1a0c9fce323fc5d332d00c Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Mon, 15 Jan 2024 17:49:39 +0100 Subject: [PATCH 14/16] Fix liveness probe (#2288) # Description The newly added code for the liveness probe reports unhealthy if we skip too many empty auctions after another. # Changes Also update last processes auction timestamp when we skip an auction because it's empty. --- crates/autopilot/src/run_loop.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 47139271fa..aa407ef178 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -105,6 +105,8 @@ impl RunLoop { Class::Liquidity => true, Class::Limit => false, }) { + // Updating liveness probe to not report unhealthy due to this optimization + self.liveness.auction(); tracing::debug!("skipping empty auction"); return None; } From db9b0dec4200bd81af93215cef117cef9b2b54e9 Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Mon, 15 Jan 2024 21:10:30 +0100 Subject: [PATCH 15/16] Tidy up `CompetitionEstimator` code (#2281) # Description The current `CompetitionEstimator` code is pretty complex and hard to read. Mostly for 2 reasons: 1. generic implementation tries to do everything which is overly complex 2. specalized implementations live in the same file (native, regular) # Changes There are various changes in this PR but the most important one is making the generic implementation less generic. Instead of supporting all requirements of the native and regular price estimator implementations the generic version now only handles producing results, some logging and metrics. This allows the generic and specialized implementations to be easier to implement and therefore read IMO. Additional changes: * we no longer have `RacingCompetitionEstimator` and `CompetitionEstimator`; instead you can turn a `CompetitionEstimator` into a racing one by calling `.with_early_return()` on it * separated logic into 3 files: 1. main logic + tests of staging logic (i.e. generic aspect of the code) 2. code + tests specific to native price estimation 3. code + tests specific to regular price estimation (also contains bang for buck implementation) * deleted some unused code ## How to test All existing tests continue to pass without any changes (besides having to call `.with_early_return()`) ## How to review It's probably easiest to review the individual commits in order. Pay extra attention to `produce_results()`. I changed that code the most and the original code (`estimate_generic()`) was not trivial. --------- Co-authored-by: Martin Beckmann --- .../src/price_estimation/competition.rs | 1017 ----------------- .../src/price_estimation/competition/mod.rs | 533 +++++++++ .../price_estimation/competition/native.rs | 126 ++ .../src/price_estimation/competition/quote.rs | 305 +++++ crates/shared/src/price_estimation/factory.rs | 26 +- 5 files changed, 977 insertions(+), 1030 deletions(-) delete mode 100644 crates/shared/src/price_estimation/competition.rs create mode 100644 crates/shared/src/price_estimation/competition/mod.rs create mode 100644 crates/shared/src/price_estimation/competition/native.rs create mode 100644 crates/shared/src/price_estimation/competition/quote.rs diff --git a/crates/shared/src/price_estimation/competition.rs b/crates/shared/src/price_estimation/competition.rs deleted file mode 100644 index a7098efb23..0000000000 --- a/crates/shared/src/price_estimation/competition.rs +++ /dev/null @@ -1,1017 +0,0 @@ -use { - super::native::{NativePriceEstimateResult, NativePriceEstimating}, - crate::price_estimation::{ - Estimate, - PriceEstimateResult, - PriceEstimating, - PriceEstimationError, - Query, - }, - anyhow::Context, - futures::{ - future::Future, - stream::{FuturesUnordered, StreamExt}, - FutureExt as _, - TryFutureExt, - }, - gas_estimation::GasPriceEstimating, - model::order::OrderKind, - primitive_types::{H160, U256}, - std::{cmp::Ordering, fmt::Debug, num::NonZeroUsize, sync::Arc, time::Instant}, -}; - -#[derive(Debug, Clone, Hash, Eq, PartialEq)] -struct Trade { - sell_token: H160, - buy_token: H160, - kind: OrderKind, -} - -impl From<&Query> for Trade { - fn from(query: &Query) -> Self { - Self { - sell_token: query.sell_token, - buy_token: query.buy_token, - kind: query.kind, - } - } -} - -/// Stage index and index within stage of an estimator stored in the -/// [`CompetitionEstimator`] used as an identifier. -#[derive(Copy, Debug, Clone, Default, Eq, PartialEq)] -struct EstimatorIndex(usize, usize); - -type PriceEstimationStage = Vec<(String, T)>; - -/// Price estimator that pulls estimates from various sources -/// and competes on the best price. Sources are provided as a list of lists, the -/// outer list representing the sequential stage of the search, and the inner -/// list representing all source that should be queried in parallel in the given -/// stage Returns a price estimation early if there is a configurable number of -/// successful estimates for every query or if all price sources returned an -/// estimate. -pub struct RacingCompetitionEstimator { - inner: Vec>, - successful_results_for_early_return: NonZeroUsize, - ranking: PriceRanking, -} - -impl RacingCompetitionEstimator { - pub fn new( - inner: Vec>, - successful_results_for_early_return: NonZeroUsize, - ranking: PriceRanking, - ) -> Self { - assert!(!inner.is_empty()); - Self { - inner, - successful_results_for_early_return, - ranking, - } - } - - fn estimate_generic< - Q: Clone + Debug + Send + 'static, - R: Clone + Debug + Send, - E: Clone + Debug + Send, - C, - >( - &self, - query: Q, - kind: OrderKind, - get_single_result: impl Fn(&T, Q) -> futures::future::BoxFuture<'_, Result> - + Send - + 'static, - pick_best_index: impl Fn(&[(EstimatorIndex, Result)], &C) -> Result - + Send - + 'static, - provide_comparison_context: impl Future> + Send + 'static, - ) -> futures::future::BoxFuture<'_, Result> { - let start = Instant::now(); - async move { - let mut results = vec![]; - let mut iter = self.inner.iter().enumerate().peekable(); - // Process stages sequentially - 'outer: while let Some((stage_index, stage)) = iter.next() { - // Process estimators within each stage in parallel - let mut requests: Vec<_> = stage - .iter() - .enumerate() - .map(|(index, (_, estimator))| { - get_single_result(estimator, query.clone()) - .map(move |result| (EstimatorIndex(stage_index, index), result)) - .boxed() - }) - .collect(); - - // Make sure we also use the next stage(s) if this one does not have enough - // estimators to return early anyways - let missing_successes = - self.successful_results_for_early_return.get() - successes(&results); - while requests.len() < missing_successes && iter.peek().is_some() { - let (next_stage_index, next_stage) = iter.next().unwrap(); - requests.extend( - next_stage - .iter() - .enumerate() - .map(|(index, (_, estimator))| { - get_single_result(estimator, query.clone()) - .map(move |result| { - (EstimatorIndex(next_stage_index, index), result) - }) - .boxed() - }), - ) - } - - let mut futures: FuturesUnordered<_> = requests.into_iter().collect(); - while let Some((estimator_index, result)) = futures.next().await { - results.push((estimator_index, result.clone())); - let estimator = &self.inner[estimator_index.0][estimator_index.1].0; - tracing::debug!( - ?query, - ?result, - estimator, - requests = futures.len(), - results = results.len(), - elapsed = ?start.elapsed(), - "new price estimate" - ); - - if successes(&results) >= self.successful_results_for_early_return.get() { - break 'outer; - } - } - } - - let context = provide_comparison_context.await?; - let best_index = pick_best_index(&results, &context)?; - let (estimator_index, result) = &results[best_index]; - let (estimator, _) = &self.inner[estimator_index.0][estimator_index.1]; - tracing::debug!( - ?query, - ?result, - estimator, - elapsed = ?start.elapsed(), - "winning price estimate" - ); - - let total_estimators = self.inner.iter().fold(0, |sum, inner| sum + inner.len()) as u64; - let queried_estimators = results.len() as u64; - metrics() - .requests - .with_label_values(&["executed"]) - .inc_by(queried_estimators); - metrics() - .requests - .with_label_values(&["saved"]) - .inc_by(total_estimators - queried_estimators); - - if result.is_ok() { - // Collect stats for winner predictions. - metrics() - .queries_won - .with_label_values(&[estimator, kind.label()]) - .inc(); - } - result.clone() - } - .boxed() - } -} - -fn successes(results: &[(EstimatorIndex, Result)]) -> usize { - results.iter().filter(|(_, result)| result.is_ok()).count() -} - -impl PriceEstimating for RacingCompetitionEstimator> { - fn estimate(&self, query: Arc) -> futures::future::BoxFuture<'_, PriceEstimateResult> { - let out_token = match query.kind { - OrderKind::Buy => query.sell_token, - OrderKind::Sell => query.buy_token, - }; - let context_future = self.ranking.provide_context(out_token); - self.estimate_generic( - query.clone(), - query.kind, - |estimator, query| estimator.estimate(query), - move |results, context| { - results - .iter() - .map(|(_, result)| result) - .enumerate() - // Filter out 0 gas cost estimate because they are obviously wrong and would - // likely win the price competition which would lead to us paying huge - // subsidies. - .filter(|(_, r)| r.is_err() || r.as_ref().is_ok_and(|e| e.gas > 0)) - .max_by(|a, b| compare_quote_result(&query, a.1, b.1, context)) - .map(|(index, _)| index) - .with_context(|| "all price estimates reported 0 gas cost") - .map_err(PriceEstimationError::EstimatorInternal) - }, - context_future, - ) - } -} - -impl NativePriceEstimating for RacingCompetitionEstimator> { - fn estimate_native_price( - &self, - token: H160, - ) -> futures::future::BoxFuture<'_, NativePriceEstimateResult> { - let context_future = futures::future::ready(Ok(())); - self.estimate_generic( - token, - OrderKind::Buy, - |estimator, token| estimator.estimate_native_price(token), - move |results, _context| { - let best_index = results - .iter() - .map(|(_, result)| result) - .enumerate() - .max_by(|a, b| compare_native_result(a.1, b.1)) - .map(|(index, _)| index) - .expect("we get passed at least 1 result and did not filter out any of them"); - Ok(best_index) - }, - context_future, - ) - } -} - -/// Price estimator that pulls estimates from various sources -/// and competes on the best price. -pub struct CompetitionEstimator { - inner: RacingCompetitionEstimator, -} - -impl CompetitionEstimator { - pub fn new(inner: Vec>, ranking: PriceRanking) -> Self { - let number_of_estimators = - NonZeroUsize::new(inner.iter().fold(0, |sum, stage| sum + stage.len())) - .expect("Vec of estimators should not be empty."); - Self { - inner: RacingCompetitionEstimator::new(inner, number_of_estimators, ranking), - } - } -} - -impl PriceEstimating for CompetitionEstimator> { - fn estimate(&self, query: Arc) -> futures::future::BoxFuture<'_, PriceEstimateResult> { - self.inner.estimate(query) - } -} - -fn compare_quote_result( - query: &Query, - a: &PriceEstimateResult, - b: &PriceEstimateResult, - context: &RankingContext, -) -> Ordering { - match (a, b) { - (Ok(a), Ok(b)) => compare_quote(query, a, b, context), - (Ok(_), Err(_)) => Ordering::Greater, - (Err(_), Ok(_)) => Ordering::Less, - (Err(a), Err(b)) => compare_error(a, b), - } -} - -fn compare_native_result( - a: &Result, - b: &Result, -) -> Ordering { - match (a, b) { - (Ok(a), Ok(b)) => a.total_cmp(b), - (Ok(_), Err(_)) => Ordering::Greater, - (Err(_), Ok(_)) => Ordering::Less, - (Err(a), Err(b)) => compare_error(a, b), - } -} - -fn compare_quote(query: &Query, a: &Estimate, b: &Estimate, context: &RankingContext) -> Ordering { - let a = context.effective_eth_out(a, query.kind); - let b = context.effective_eth_out(b, query.kind); - match query.kind { - OrderKind::Buy => a.cmp(&b).reverse(), - OrderKind::Sell => a.cmp(&b), - } -} - -fn compare_error(a: &PriceEstimationError, b: &PriceEstimationError) -> Ordering { - // Errors are sorted by recoverability. E.g. a rate-limited estimation may - // succeed if tried again, whereas unsupported order types can never recover - // unless code changes. This can be used to decide which errors we want to - // cache - fn error_to_integer_priority(err: &PriceEstimationError) -> u8 { - match err { - // highest priority (prefer) - PriceEstimationError::RateLimited => 5, - PriceEstimationError::ProtocolInternal(_) => 4, - PriceEstimationError::EstimatorInternal(_) => 3, - PriceEstimationError::UnsupportedToken { .. } => 2, - PriceEstimationError::NoLiquidity => 1, - PriceEstimationError::UnsupportedOrderType(_) => 0, - // lowest priority - } - } - error_to_integer_priority(a).cmp(&error_to_integer_priority(b)) -} - -#[derive(prometheus_metric_storage::MetricStorage, Clone, Debug)] -#[metric(subsystem = "competition_price_estimator")] -struct Metrics { - /// Number of wins for a particular price estimator and order kind. - /// - /// Note that the order kind is included in the metric. This is because some - /// estimators only support sell orders (e.g. 1Inch) which would skew the - /// total metrics. Additionally, this allows us to see how different - /// estimators behave for buy vs sell orders. - #[metric(labels("estimator_type", "order_kind"))] - queries_won: prometheus::IntCounterVec, - - /// Number of requests we saved due to greedy selection based on historic - /// data. - #[metric(labels("status"))] - requests: prometheus::IntCounterVec, -} - -fn metrics() -> &'static Metrics { - Metrics::instance(observe::metrics::get_storage_registry()) - .expect("unexpected error getting metrics instance") -} - -/// The metric which decides the winning price estimate. -#[derive(Clone)] -pub enum PriceRanking { - /// The highest quoted `out_amount` gets picked regardless of trade - /// complexity. - MaxOutAmount, - /// Returns the estimate where `out_amount - fees` is highest. - BestBangForBuck { - native: Arc, - gas: Arc, - }, -} - -impl PriceRanking { - /// Spawns a task in the background that fetches the needed context for - /// picking the best estimate without delaying the actual price fetch - /// requests. - fn provide_context( - &self, - token: H160, - ) -> impl Future> { - let fut = match self { - PriceRanking::MaxOutAmount => async { - Ok(RankingContext { - native_price: 1.0, - gas_price: 0., - }) - } - .boxed(), - PriceRanking::BestBangForBuck { native, gas } => { - let gas = gas.clone(); - let native = native.clone(); - async move { - let gas = gas - .estimate() - .map_ok(|gas| gas.effective_gas_price()) - .map_err(PriceEstimationError::ProtocolInternal); - let (native_price, gas_price) = - futures::try_join!(native.estimate_native_price(token), gas)?; - - Ok(RankingContext { - native_price, - gas_price, - }) - } - .boxed() - } - }; - tokio::task::spawn(fut).map(Result::unwrap) - } -} - -struct RankingContext { - native_price: f64, - gas_price: f64, -} - -impl RankingContext { - /// Computes the actual received value from this estimate that takes `gas` - /// into account. If an extremely complex trade route would only result - /// in slightly more `out_amount` than a simple trade route the simple - /// trade route would report a higher `out_amount_in_eth`. This is also - /// referred to as "bang-for-buck" and what matters most to traders. - fn effective_eth_out(&self, estimate: &Estimate, kind: OrderKind) -> U256 { - let eth_out = estimate.out_amount.to_f64_lossy() * self.native_price; - let fees = estimate.gas as f64 * self.gas_price; - let effective_eth_out = match kind { - // High fees mean receiving less `buy_token` from your sell order. - OrderKind::Sell => eth_out - fees, - // High fees mean paying more `sell_token` for your buy order. - OrderKind::Buy => eth_out + fees, - }; - // converts `NaN` and `(-∞, 0]` to `0` - U256::from_f64_lossy(effective_eth_out) - } -} - -#[cfg(test)] -mod tests { - use { - super::*, - crate::{ - gas_price_estimation::FakeGasPriceEstimator, - price_estimation::{native::MockNativePriceEstimating, MockPriceEstimating}, - }, - anyhow::anyhow, - futures::channel::oneshot::channel, - gas_estimation::GasPrice1559, - model::order::OrderKind, - number::nonzero::U256 as NonZeroU256, - primitive_types::H160, - std::time::Duration, - tokio::time::sleep, - }; - - fn price(out_amount: u128, gas: u64) -> PriceEstimateResult { - Ok(Estimate { - out_amount: out_amount.into(), - gas, - ..Default::default() - }) - } - - fn native_price(native_price: f64) -> NativePriceEstimateResult { - NativePriceEstimateResult::Ok(native_price) - } - - fn error(err: PriceEstimationError) -> Result { - Err(err) - } - - /// Builds a `BestBangForBuck` setup where every token is estimated - /// to be half as valuable as ETH and the gas price is 2. - /// That effectively means every unit of `gas` in an estimate worth - /// 4 units of `out_amount`. - fn bang_for_buck_ranking() -> PriceRanking { - // Make `out_token` half as valuable as `ETH` and set gas price to 2. - // That means 1 unit of `gas` is equal to 4 units of `out_token`. - let mut native = MockNativePriceEstimating::new(); - native - .expect_estimate_native_price() - .returning(move |_| async { Ok(0.5) }.boxed()); - let gas = Arc::new(FakeGasPriceEstimator::new(GasPrice1559 { - base_fee_per_gas: 2.0, - max_fee_per_gas: 2.0, - max_priority_fee_per_gas: 2.0, - })); - PriceRanking::BestBangForBuck { - native: Arc::new(native), - gas, - } - } - - /// Returns the best estimate with respect to the provided ranking and order - /// kind. - async fn best_response( - ranking: PriceRanking, - kind: OrderKind, - estimates: Vec, - ) -> PriceEstimateResult { - fn estimator(estimate: PriceEstimateResult) -> Arc { - let mut estimator = MockPriceEstimating::new(); - estimator - .expect_estimate() - .times(1) - .return_once(move |_| async move { estimate }.boxed()); - Arc::new(estimator) - } - - let priority: CompetitionEstimator> = CompetitionEstimator::new( - vec![estimates - .into_iter() - .enumerate() - .map(|(i, e)| (format!("estimator_{i}"), estimator(e))) - .collect()], - ranking.clone(), - ); - - priority - .estimate(Arc::new(Query { - kind, - ..Default::default() - })) - .await - } - - /// Returns the best native estimate with respect to the provided ranking - /// and order kind. - async fn best_native_response( - ranking: PriceRanking, - estimates: Vec, - ) -> NativePriceEstimateResult { - fn estimator(estimate: NativePriceEstimateResult) -> Arc { - let mut estimator = MockNativePriceEstimating::new(); - estimator - .expect_estimate_native_price() - .times(1) - .return_once(move |_| async move { estimate }.boxed()); - Arc::new(estimator) - } - - let priority: CompetitionEstimator> = - CompetitionEstimator::new( - vec![estimates - .into_iter() - .enumerate() - .map(|(i, e)| (format!("estimator_{i}"), estimator(e))) - .collect()], - ranking.clone(), - ); - - priority - .inner - .estimate_native_price(Default::default()) - .await - } - - #[tokio::test] - async fn works() { - let queries = [ - Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(0), - buy_token: H160::from_low_u64_le(1), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Buy, - block_dependent: false, - }), - Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(2), - buy_token: H160::from_low_u64_le(3), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Sell, - block_dependent: false, - }), - Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(2), - buy_token: H160::from_low_u64_le(3), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Buy, - block_dependent: false, - }), - Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(3), - buy_token: H160::from_low_u64_le(4), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Buy, - block_dependent: false, - }), - Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(5), - buy_token: H160::from_low_u64_le(6), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Buy, - block_dependent: false, - }), - ]; - let estimates = [ - Estimate { - out_amount: 1.into(), - gas: 1, - ..Default::default() - }, - Estimate { - out_amount: 2.into(), - gas: 1, - ..Default::default() - }, - ]; - - let setup_estimator = |responses: Vec| { - let mut estimator = MockPriceEstimating::new(); - for response in responses { - estimator.expect_estimate().times(1).returning(move |_| { - let response = response.clone(); - { - async move { response }.boxed() - } - }); - } - estimator - }; - - let first = setup_estimator(vec![ - Ok(estimates[0]), - Ok(estimates[0]), - Ok(estimates[0]), - Err(PriceEstimationError::ProtocolInternal(anyhow!("a"))), - Err(PriceEstimationError::NoLiquidity), - ]); - - let second = setup_estimator(vec![ - Err(PriceEstimationError::ProtocolInternal(anyhow!(""))), - Ok(estimates[1]), - Ok(estimates[1]), - Err(PriceEstimationError::ProtocolInternal(anyhow!("b"))), - Err(PriceEstimationError::UnsupportedToken { - token: H160([0; 20]), - reason: "".to_string(), - }), - ]); - - let priority: CompetitionEstimator> = CompetitionEstimator::new( - vec![vec![ - ("first".to_owned(), Arc::new(first)), - ("second".to_owned(), Arc::new(second)), - ]], - PriceRanking::MaxOutAmount, - ); - - let result = priority.estimate(queries[0].clone()).await; - assert_eq!(result.as_ref().unwrap(), &estimates[0]); - - let result = priority.estimate(queries[1].clone()).await; - // buy 2 is better than buy 1 - assert_eq!(result.as_ref().unwrap(), &estimates[1]); - - let result = priority.estimate(queries[2].clone()).await; - // pay 1 is better than pay 2 - assert_eq!(result.as_ref().unwrap(), &estimates[0]); - - let result = priority.estimate(queries[3].clone()).await; - // arbitrarily returns one of equal priority errors - assert!(matches!( - result.as_ref().unwrap_err(), - PriceEstimationError::ProtocolInternal(err) - if err.to_string() == "a" || err.to_string() == "b", - )); - - let result = priority.estimate(queries[4].clone()).await; - // unsupported token has higher priority than no liquidity - assert!(matches!( - result.as_ref().unwrap_err(), - PriceEstimationError::UnsupportedToken { .. } - )); - } - - #[tokio::test] - async fn racing_estimator_returns_early() { - let query = Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(0), - buy_token: H160::from_low_u64_le(1), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Buy, - block_dependent: false, - }); - - fn estimate(amount: u64) -> Estimate { - Estimate { - out_amount: amount.into(), - gas: 1, - ..Default::default() - } - } - - let mut first = MockPriceEstimating::new(); - first.expect_estimate().times(1).returning(move |_| { - // immediately return an error (not enough to terminate price competition early) - async { Err(PriceEstimationError::NoLiquidity) }.boxed() - }); - - let mut second = MockPriceEstimating::new(); - second.expect_estimate().times(1).returning(|_| { - async { - sleep(Duration::from_millis(10)).await; - // return good result after some time; now we can terminate early - Ok(estimate(1)) - } - .boxed() - }); - - let mut third = MockPriceEstimating::new(); - third.expect_estimate().times(1).returning(move |_| { - async { - sleep(Duration::from_millis(20)).await; - unreachable!( - "This estimation gets canceled because the racing estimator already got \ - enough estimates to return early." - ) - } - .boxed() - }); - - let racing: RacingCompetitionEstimator> = - RacingCompetitionEstimator::new( - vec![vec![ - ("first".to_owned(), Arc::new(first)), - ("second".to_owned(), Arc::new(second)), - ("third".to_owned(), Arc::new(third)), - ]], - NonZeroUsize::new(1).unwrap(), - PriceRanking::MaxOutAmount, - ); - - let result = racing.estimate(query).await; - assert_eq!(result.as_ref().unwrap(), &estimate(1)); - } - - #[tokio::test] - async fn queries_stages_sequentially() { - let query = Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(0), - buy_token: H160::from_low_u64_le(1), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Sell, - block_dependent: false, - }); - - fn estimate(amount: u64) -> Estimate { - Estimate { - out_amount: amount.into(), - gas: 1, - ..Default::default() - } - } - - let mut first = MockPriceEstimating::new(); - first.expect_estimate().times(1).returning(move |_| { - async { - // First stage takes longer than second to test they are not executed in - // parallel - sleep(Duration::from_millis(20)).await; - Ok(estimate(1)) - } - .boxed() - }); - - let mut second = MockPriceEstimating::new(); - second.expect_estimate().times(1).returning(move |_| { - async { - sleep(Duration::from_millis(20)).await; - Err(PriceEstimationError::NoLiquidity) - } - .boxed() - }); - - let mut third = MockPriceEstimating::new(); - third - .expect_estimate() - .times(1) - .returning(move |_| async { Ok(estimate(3)) }.boxed()); - - let mut fourth = MockPriceEstimating::new(); - fourth.expect_estimate().times(1).returning(move |_| { - async { - sleep(Duration::from_millis(10)).await; - unreachable!( - "This estimation gets canceled because the racing estimator already got \ - enough estimates to return early." - ) - } - .boxed() - }); - - let racing: RacingCompetitionEstimator> = - RacingCompetitionEstimator::new( - vec![ - vec![ - ("first".to_owned(), Arc::new(first)), - ("second".to_owned(), Arc::new(second)), - ], - vec![ - ("third".to_owned(), Arc::new(third)), - ("fourth".to_owned(), Arc::new(fourth)), - ], - ], - NonZeroUsize::new(2).unwrap(), - PriceRanking::MaxOutAmount, - ); - - let result = racing.estimate(query).await; - assert_eq!(result.as_ref().unwrap(), &estimate(3)); - } - - #[tokio::test] - async fn combines_stages_if_threshold_bigger_than_next_stage_length() { - let query = Arc::new(Query { - verification: None, - sell_token: H160::from_low_u64_le(0), - buy_token: H160::from_low_u64_le(1), - in_amount: NonZeroU256::try_from(1).unwrap(), - kind: OrderKind::Sell, - block_dependent: false, - }); - - fn estimate(amount: u64) -> Estimate { - Estimate { - out_amount: amount.into(), - gas: 1, - ..Default::default() - } - } - - let (sender, mut receiver) = channel(); - - let mut first = MockPriceEstimating::new(); - - first.expect_estimate().times(1).return_once(move |_| { - async { - sleep(Duration::from_millis(20)).await; - let _ = sender.send(()); - Ok(estimate(1)) - } - .boxed() - }); - - let mut second = MockPriceEstimating::new(); - second.expect_estimate().times(1).return_once(move |_| { - async move { - // First stage hasn't finished yet - assert!(receiver.try_recv().unwrap().is_none()); - Err(PriceEstimationError::NoLiquidity) - } - .boxed() - }); - - // After the first combined stage is done, we are only missing one positive - // result, thus we query third but not fourth - let mut third = MockPriceEstimating::new(); - third - .expect_estimate() - .times(1) - .return_once(move |_| async move { Ok(estimate(1)) }.boxed()); - - let mut fourth = MockPriceEstimating::new(); - fourth.expect_estimate().never(); - - let racing: RacingCompetitionEstimator> = - RacingCompetitionEstimator { - inner: vec![ - vec![("first".to_owned(), Arc::new(first))], - vec![("second".to_owned(), Arc::new(second))], - vec![("third".to_owned(), Arc::new(third))], - vec![("fourth".to_owned(), Arc::new(fourth))], - ], - successful_results_for_early_return: NonZeroUsize::new(2).unwrap(), - ranking: PriceRanking::MaxOutAmount, - }; - - racing.estimate(query).await.unwrap(); - } - - /// Verifies that `PriceRanking::BestBangForBuck` correctly adjusts - /// `out_amount` of quotes based on the `gas` used for the quote. E.g. - /// if a quote requires a significantly more complex execution but does - /// not provide a significantly better `out_amount` than a simpler quote - /// the simpler quote will be preferred. - #[tokio::test] - async fn best_bang_for_buck_adjusts_for_complexity() { - let best = best_response( - bang_for_buck_ranking(), - OrderKind::Sell, - vec![ - // User effectively receives `100_000` `buy_token`. - price(104_000, 1_000), - // User effectively receives `99_999` `buy_token`. - price(107_999, 2_000), - ], - ) - .await; - assert_eq!(best, price(104_000, 1_000)); - - let best = best_response( - bang_for_buck_ranking(), - OrderKind::Buy, - vec![ - // User effectively pays `100_000` `sell_token`. - price(96_000, 1_000), - // User effectively pays `100_002` `sell_token`. - price(92_002, 2_000), - ], - ) - .await; - assert_eq!(best, price(96_000, 1_000)); - } - - /// Same test as above but now we also add an estimate that should - /// win under normal circumstances but the `gas` cost is suspiciously - /// low so we discard it. This protects us from quoting unreasonably - /// low fees for user orders. - #[tokio::test] - async fn discards_low_gas_cost_estimates() { - let best = best_response( - bang_for_buck_ranking(), - OrderKind::Sell, - vec![ - // User effectively receives `100_000` `buy_token`. - price(104_000, 1_000), - // User effectively receives `99_999` `buy_token`. - price(107_999, 2_000), - // User effectively receives `104_000` `buy_token` but the estimate - // gets discarded because it quotes 0 gas. - price(104_000, 0), - ], - ) - .await; - assert_eq!(best, price(104_000, 1_000)); - - let best = best_response( - bang_for_buck_ranking(), - OrderKind::Buy, - vec![ - // User effectively pays `100_000` `sell_token`. - price(96_000, 1_000), - // User effectively pays `100_002` `sell_token`. - price(92_002, 2_000), - // User effectively pays `99_000` `sell_token` but the estimate - // gets discarded because it quotes 0 gas. - price(99_000, 0), - ], - ) - .await; - assert_eq!(best, price(96_000, 1_000)); - } - - /// If all estimators returned an error we return the one with the highest - /// priority. - #[tokio::test] - async fn returns_highest_priority_error() { - // Returns errors with highest priority. - let best = best_response( - PriceRanking::MaxOutAmount, - OrderKind::Sell, - vec![ - error(PriceEstimationError::RateLimited), - error(PriceEstimationError::ProtocolInternal(anyhow::anyhow!("!"))), - ], - ) - .await; - assert_eq!(best, error(PriceEstimationError::RateLimited)); - } - - /// Any price estimate, no matter how bad, is preferred over an error. - #[tokio::test] - async fn prefer_estimate_over_error() { - let best = best_response( - PriceRanking::MaxOutAmount, - OrderKind::Sell, - vec![ - price(1, 1_000_000), - error(PriceEstimationError::RateLimited), - ], - ) - .await; - assert_eq!(best, price(1, 1_000_000)); - } - - /// If all estimators returned an error we return the one with the highest - /// priority. - #[tokio::test] - async fn returns_highest_native_price() { - // Returns errors with highest priority. - let best = best_native_response( - PriceRanking::MaxOutAmount, - vec![native_price(1.), native_price(2.)], - ) - .await; - assert_eq!(best, native_price(2.)); - } - - /// If all estimators returned an error we return the one with the highest - /// priority. - #[tokio::test] - async fn returns_highest_priority_error_native() { - // Returns errors with highest priority. - let best = best_native_response( - PriceRanking::MaxOutAmount, - vec![ - error(PriceEstimationError::RateLimited), - error(PriceEstimationError::ProtocolInternal(anyhow::anyhow!("!"))), - ], - ) - .await; - assert_eq!(best, error(PriceEstimationError::RateLimited)); - } - - /// Any native price estimate, no matter how bad, is preferred over an - /// error. - #[tokio::test] - async fn prefer_estimate_over_error_native() { - let best = best_native_response( - PriceRanking::MaxOutAmount, - vec![native_price(1.), error(PriceEstimationError::RateLimited)], - ) - .await; - assert_eq!(best, native_price(1.)); - } -} diff --git a/crates/shared/src/price_estimation/competition/mod.rs b/crates/shared/src/price_estimation/competition/mod.rs new file mode 100644 index 0000000000..ed99fbf809 --- /dev/null +++ b/crates/shared/src/price_estimation/competition/mod.rs @@ -0,0 +1,533 @@ +use { + super::native::NativePriceEstimating, + crate::price_estimation::PriceEstimationError, + futures::{ + future::{BoxFuture, FutureExt}, + stream::{FuturesUnordered, StreamExt}, + }, + gas_estimation::GasPriceEstimating, + model::order::OrderKind, + std::{cmp::Ordering, fmt::Debug, num::NonZeroUsize, sync::Arc, time::Instant}, +}; + +mod native; +mod quote; + +/// Stage index and index within stage of an estimator stored in the +/// [`CompetitionEstimator`] used as an identifier. +#[derive(Copy, Debug, Clone, Default, Eq, PartialEq)] +struct EstimatorIndex(usize, usize); + +type PriceEstimationStage = Vec<(String, T)>; +type ResultWithIndex = (EstimatorIndex, Result); + +/// Price estimator that pulls estimates from various sources +/// and competes on the best price. Sources are provided as a list of lists, the +/// outer list representing the sequential stage of the search, and the inner +/// list representing all source that should be queried in parallel in the given +/// stage Returns a price estimation early if there is a configurable number of +/// successful estimates for every query or if all price sources returned an +/// estimate. +pub struct CompetitionEstimator { + stages: Vec>, + usable_results_for_early_return: NonZeroUsize, + ranking: PriceRanking, +} + +impl CompetitionEstimator { + pub fn new(stages: Vec>, ranking: PriceRanking) -> Self { + assert!(!stages.is_empty()); + assert!(stages.iter().all(|stage| !stage.is_empty())); + Self { + stages, + usable_results_for_early_return: NonZeroUsize::MAX, + ranking, + } + } + + /// Enables the estimator to return after it got the configured number of + /// successful results instead of having to wait for all estimators to + /// return a result. + pub fn with_early_return(self, usable_results_for_early_return: NonZeroUsize) -> Self { + Self { + usable_results_for_early_return, + ..self + } + } + + /// Produce results for the given `input` until the caller does not expect + /// any more results or we produced all the results we can. + async fn produce_results( + &self, + query: Q, + result_is_usable: impl Fn(&Result) -> bool, + get_single_result: impl Fn(&T, Q) -> BoxFuture<'_, Result> + + Send + + 'static, + ) -> Vec> + where + Q: Clone + Debug + Send + 'static, + R: Clone + Debug + Send, + { + let start = Instant::now(); + let mut results = vec![]; + let mut stage_index = 0; + + let missing_results = |results: &[ResultWithIndex]| { + let usable = results.iter().filter(|(_, r)| result_is_usable(r)).count(); + self.usable_results_for_early_return + .get() + .saturating_sub(usable) + }; + + 'outer: while stage_index < self.stages.len() { + let mut requests = FuturesUnordered::new(); + + // Collect requests until it's at least theoretically possible to produce enough + // results to return early. + let requests_for_batch = missing_results(&results); + while stage_index < self.stages.len() && requests.len() < requests_for_batch { + let stage = &self.stages.get(stage_index).expect("index checked by loop"); + let futures = stage.iter().enumerate().map(|(index, (_name, estimator))| { + get_single_result(estimator, query.clone()) + .map(move |result| (EstimatorIndex(stage_index, index), result)) + .boxed() + }); + + requests.extend(futures); + stage_index += 1; + } + + while let Some((estimator_index, result)) = requests.next().await { + let (name, _estimator) = &self.stages[estimator_index.0][estimator_index.1]; + tracing::debug!( + ?query, + ?result, + estimator = name, + requests = requests.len(), + results = results.len(), + elapsed = ?start.elapsed(), + "new price estimate" + ); + results.push((estimator_index, result)); + + if missing_results(&results) == 0 { + break 'outer; + } + } + } + + results + } + + fn report_winner( + &self, + query: &Q, + kind: OrderKind, + (index, result): ResultWithIndex, + ) -> Result { + let EstimatorIndex(stage_index, estimator_index) = index; + let (name, _estimator) = &self.stages[stage_index][estimator_index]; + tracing::debug!(?query, ?result, estimator = name, "winning price estimate"); + if result.is_ok() { + metrics() + .queries_won + .with_label_values(&[name, kind.label()]) + .inc(); + } + result + } +} + +fn compare_error(a: &PriceEstimationError, b: &PriceEstimationError) -> Ordering { + // Errors are sorted by recoverability. E.g. a rate-limited estimation may + // succeed if tried again, whereas unsupported order types can never recover + // unless code changes. This can be used to decide which errors we want to + // cache + fn error_to_integer_priority(err: &PriceEstimationError) -> u8 { + match err { + // highest priority (prefer) + PriceEstimationError::RateLimited => 5, + PriceEstimationError::ProtocolInternal(_) => 4, + PriceEstimationError::EstimatorInternal(_) => 3, + PriceEstimationError::UnsupportedToken { .. } => 2, + PriceEstimationError::NoLiquidity => 1, + PriceEstimationError::UnsupportedOrderType(_) => 0, + // lowest priority + } + } + error_to_integer_priority(a).cmp(&error_to_integer_priority(b)) +} + +#[derive(prometheus_metric_storage::MetricStorage, Clone, Debug)] +#[metric(subsystem = "competition_price_estimator")] +struct Metrics { + /// Number of wins for a particular price estimator and order kind. + /// + /// Note that the order kind is included in the metric. This is because some + /// estimators only support sell orders (e.g. 1Inch) which would skew the + /// total metrics. Additionally, this allows us to see how different + /// estimators behave for buy vs sell orders. + #[metric(labels("estimator_type", "order_kind"))] + queries_won: prometheus::IntCounterVec, +} + +fn metrics() -> &'static Metrics { + Metrics::instance(observe::metrics::get_storage_registry()) + .expect("unexpected error getting metrics instance") +} + +/// The metric which decides the winning price estimate. +#[derive(Clone)] +pub enum PriceRanking { + /// The highest quoted `out_amount` gets picked regardless of trade + /// complexity. + MaxOutAmount, + /// Returns the estimate where `out_amount - fees` is highest. + BestBangForBuck { + native: Arc, + gas: Arc, + }, +} + +#[cfg(test)] +mod tests { + use { + super::*, + crate::price_estimation::{Estimate, MockPriceEstimating, PriceEstimating, Query}, + anyhow::anyhow, + futures::channel::oneshot::channel, + model::order::OrderKind, + number::nonzero::U256 as NonZeroU256, + primitive_types::H160, + std::time::Duration, + tokio::time::sleep, + }; + + #[tokio::test] + async fn works() { + let queries = [ + Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(0), + buy_token: H160::from_low_u64_le(1), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Buy, + block_dependent: false, + }), + Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(2), + buy_token: H160::from_low_u64_le(3), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Sell, + block_dependent: false, + }), + Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(2), + buy_token: H160::from_low_u64_le(3), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Buy, + block_dependent: false, + }), + Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(3), + buy_token: H160::from_low_u64_le(4), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Buy, + block_dependent: false, + }), + Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(5), + buy_token: H160::from_low_u64_le(6), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Buy, + block_dependent: false, + }), + ]; + let estimates = [ + Estimate { + out_amount: 1.into(), + gas: 1, + ..Default::default() + }, + Estimate { + out_amount: 2.into(), + gas: 1, + ..Default::default() + }, + ]; + + let setup_estimator = |responses: Vec>| { + let mut estimator = MockPriceEstimating::new(); + for response in responses { + estimator.expect_estimate().times(1).returning(move |_| { + let response = response.clone(); + { + async move { response }.boxed() + } + }); + } + estimator + }; + + let first = setup_estimator(vec![ + Ok(estimates[0]), + Ok(estimates[0]), + Ok(estimates[0]), + Err(PriceEstimationError::ProtocolInternal(anyhow!("a"))), + Err(PriceEstimationError::NoLiquidity), + ]); + + let second = setup_estimator(vec![ + Err(PriceEstimationError::ProtocolInternal(anyhow!(""))), + Ok(estimates[1]), + Ok(estimates[1]), + Err(PriceEstimationError::ProtocolInternal(anyhow!("b"))), + Err(PriceEstimationError::UnsupportedToken { + token: H160([0; 20]), + reason: "".to_string(), + }), + ]); + + let priority: CompetitionEstimator> = CompetitionEstimator::new( + vec![vec![ + ("first".to_owned(), Arc::new(first)), + ("second".to_owned(), Arc::new(second)), + ]], + PriceRanking::MaxOutAmount, + ); + + let result = priority.estimate(queries[0].clone()).await; + assert_eq!(result.as_ref().unwrap(), &estimates[0]); + + let result = priority.estimate(queries[1].clone()).await; + // buy 2 is better than buy 1 + assert_eq!(result.as_ref().unwrap(), &estimates[1]); + + let result = priority.estimate(queries[2].clone()).await; + // pay 1 is better than pay 2 + assert_eq!(result.as_ref().unwrap(), &estimates[0]); + + let result = priority.estimate(queries[3].clone()).await; + // arbitrarily returns one of equal priority errors + assert!(matches!( + result.as_ref().unwrap_err(), + PriceEstimationError::ProtocolInternal(err) + if err.to_string() == "a" || err.to_string() == "b", + )); + + let result = priority.estimate(queries[4].clone()).await; + // unsupported token has higher priority than no liquidity + assert!(matches!( + result.as_ref().unwrap_err(), + PriceEstimationError::UnsupportedToken { .. } + )); + } + + #[tokio::test] + async fn racing_estimator_returns_early() { + let query = Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(0), + buy_token: H160::from_low_u64_le(1), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Buy, + block_dependent: false, + }); + + fn estimate(amount: u64) -> Estimate { + Estimate { + out_amount: amount.into(), + gas: 1, + ..Default::default() + } + } + + let mut first = MockPriceEstimating::new(); + first.expect_estimate().times(1).returning(move |_| { + // immediately return an error (not enough to terminate price competition early) + async { Err(PriceEstimationError::NoLiquidity) }.boxed() + }); + + let mut second = MockPriceEstimating::new(); + second.expect_estimate().times(1).returning(|_| { + async { + sleep(Duration::from_millis(10)).await; + // return good result after some time; now we can terminate early + Ok(estimate(1)) + } + .boxed() + }); + + let mut third = MockPriceEstimating::new(); + third.expect_estimate().times(1).returning(move |_| { + async { + sleep(Duration::from_millis(20)).await; + unreachable!( + "This estimation gets canceled because the racing estimator already got \ + enough estimates to return early." + ) + } + .boxed() + }); + let racing: CompetitionEstimator> = CompetitionEstimator::new( + vec![vec![ + ("first".to_owned(), Arc::new(first)), + ("second".to_owned(), Arc::new(second)), + ("third".to_owned(), Arc::new(third)), + ]], + PriceRanking::MaxOutAmount, + ); + let racing = racing.with_early_return(1.try_into().unwrap()); + + let result = racing.estimate(query).await; + assert_eq!(result.as_ref().unwrap(), &estimate(1)); + } + + #[tokio::test] + async fn queries_stages_sequentially() { + let query = Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(0), + buy_token: H160::from_low_u64_le(1), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Sell, + block_dependent: false, + }); + + fn estimate(amount: u64) -> Estimate { + Estimate { + out_amount: amount.into(), + gas: 1, + ..Default::default() + } + } + + let mut first = MockPriceEstimating::new(); + first.expect_estimate().times(1).returning(move |_| { + async { + // First stage takes longer than second to test they are not executed in + // parallel + sleep(Duration::from_millis(20)).await; + Ok(estimate(1)) + } + .boxed() + }); + + let mut second = MockPriceEstimating::new(); + second.expect_estimate().times(1).returning(move |_| { + async { + sleep(Duration::from_millis(20)).await; + Err(PriceEstimationError::NoLiquidity) + } + .boxed() + }); + + let mut third = MockPriceEstimating::new(); + third + .expect_estimate() + .times(1) + .returning(move |_| async { Ok(estimate(3)) }.boxed()); + + let mut fourth = MockPriceEstimating::new(); + fourth.expect_estimate().times(1).returning(move |_| { + async { + sleep(Duration::from_millis(10)).await; + unreachable!( + "This estimation gets canceled because the racing estimator already got \ + enough estimates to return early." + ) + } + .boxed() + }); + + let racing: CompetitionEstimator> = CompetitionEstimator::new( + vec![ + vec![ + ("first".to_owned(), Arc::new(first)), + ("second".to_owned(), Arc::new(second)), + ], + vec![ + ("third".to_owned(), Arc::new(third)), + ("fourth".to_owned(), Arc::new(fourth)), + ], + ], + PriceRanking::MaxOutAmount, + ); + let racing = racing.with_early_return(2.try_into().unwrap()); + + let result = racing.estimate(query).await; + assert_eq!(result.as_ref().unwrap(), &estimate(3)); + } + + #[tokio::test] + async fn combines_stages_if_threshold_bigger_than_next_stage_length() { + let query = Arc::new(Query { + verification: None, + sell_token: H160::from_low_u64_le(0), + buy_token: H160::from_low_u64_le(1), + in_amount: NonZeroU256::try_from(1).unwrap(), + kind: OrderKind::Sell, + block_dependent: false, + }); + + fn estimate(amount: u64) -> Estimate { + Estimate { + out_amount: amount.into(), + gas: 1, + ..Default::default() + } + } + + let (sender, mut receiver) = channel(); + + let mut first = MockPriceEstimating::new(); + + first.expect_estimate().times(1).return_once(move |_| { + async { + sleep(Duration::from_millis(20)).await; + let _ = sender.send(()); + Ok(estimate(1)) + } + .boxed() + }); + + let mut second = MockPriceEstimating::new(); + second.expect_estimate().times(1).return_once(move |_| { + async move { + // First stage hasn't finished yet + assert!(receiver.try_recv().unwrap().is_none()); + Err(PriceEstimationError::NoLiquidity) + } + .boxed() + }); + + // After the first combined stage is done, we are only missing one positive + // result, thus we query third but not fourth + let mut third = MockPriceEstimating::new(); + third + .expect_estimate() + .times(1) + .return_once(move |_| async move { Ok(estimate(1)) }.boxed()); + + let mut fourth = MockPriceEstimating::new(); + fourth.expect_estimate().never(); + + let racing: CompetitionEstimator> = CompetitionEstimator { + stages: vec![ + vec![("first".to_owned(), Arc::new(first))], + vec![("second".to_owned(), Arc::new(second))], + vec![("third".to_owned(), Arc::new(third))], + vec![("fourth".to_owned(), Arc::new(fourth))], + ], + usable_results_for_early_return: NonZeroUsize::new(2).unwrap(), + ranking: PriceRanking::MaxOutAmount, + }; + + racing.estimate(query).await.unwrap(); + } +} diff --git a/crates/shared/src/price_estimation/competition/native.rs b/crates/shared/src/price_estimation/competition/native.rs new file mode 100644 index 0000000000..b9baab978b --- /dev/null +++ b/crates/shared/src/price_estimation/competition/native.rs @@ -0,0 +1,126 @@ +use { + super::{compare_error, CompetitionEstimator}, + crate::price_estimation::{native::NativePriceEstimating, PriceEstimationError}, + futures::future::{BoxFuture, FutureExt}, + model::order::OrderKind, + primitive_types::H160, + std::{cmp::Ordering, sync::Arc}, +}; + +impl NativePriceEstimating for CompetitionEstimator> { + fn estimate_native_price( + &self, + token: H160, + ) -> BoxFuture<'_, Result> { + async move { + let results = self + .produce_results(token, Result::is_ok, |e, q| e.estimate_native_price(q)) + .await; + let winner = results + .into_iter() + .max_by(|a, b| compare_native_result(&a.1, &b.1)) + .expect("we get passed at least 1 result and did not filter out any of them"); + self.report_winner(&token, OrderKind::Buy, winner) + } + .boxed() + } +} + +fn compare_native_result( + a: &Result, + b: &Result, +) -> Ordering { + match (a, b) { + (Ok(a), Ok(b)) => a.total_cmp(b), + (Ok(_), Err(_)) => Ordering::Greater, + (Err(_), Ok(_)) => Ordering::Less, + (Err(a), Err(b)) => compare_error(a, b), + } +} + +#[cfg(test)] +mod tests { + use { + super::*, + crate::price_estimation::{competition::PriceRanking, native::MockNativePriceEstimating}, + }; + + fn native_price(native_price: f64) -> Result { + Ok(native_price) + } + + fn error(err: PriceEstimationError) -> Result { + Err(err) + } + + /// Returns the best native estimate with respect to the provided ranking + /// and order kind. + async fn best_response( + ranking: PriceRanking, + estimates: Vec>, + ) -> Result { + fn estimator( + estimate: Result, + ) -> Arc { + let mut estimator = MockNativePriceEstimating::new(); + estimator + .expect_estimate_native_price() + .times(1) + .return_once(move |_| async move { estimate }.boxed()); + Arc::new(estimator) + } + + let priority: CompetitionEstimator> = + CompetitionEstimator::new( + vec![estimates + .into_iter() + .enumerate() + .map(|(i, e)| (format!("estimator_{i}"), estimator(e))) + .collect()], + ranking.clone(), + ); + + priority.estimate_native_price(Default::default()).await + } + + /// If all estimators returned an error we return the one with the highest + /// priority. + #[tokio::test] + async fn returns_highest_native_price() { + // Returns errors with highest priority. + let best = best_response( + PriceRanking::MaxOutAmount, + vec![native_price(1.), native_price(2.)], + ) + .await; + assert_eq!(best, native_price(2.)); + } + + /// If all estimators returned an error we return the one with the highest + /// priority. + #[tokio::test] + async fn returns_highest_priority_error_native() { + // Returns errors with highest priority. + let best = best_response( + PriceRanking::MaxOutAmount, + vec![ + error(PriceEstimationError::RateLimited), + error(PriceEstimationError::ProtocolInternal(anyhow::anyhow!("!"))), + ], + ) + .await; + assert_eq!(best, error(PriceEstimationError::RateLimited)); + } + + /// Any native price estimate, no matter how bad, is preferred over an + /// error. + #[tokio::test] + async fn prefer_estimate_over_error_native() { + let best = best_response( + PriceRanking::MaxOutAmount, + vec![native_price(1.), error(PriceEstimationError::RateLimited)], + ) + .await; + assert_eq!(best, native_price(1.)); + } +} diff --git a/crates/shared/src/price_estimation/competition/quote.rs b/crates/shared/src/price_estimation/competition/quote.rs new file mode 100644 index 0000000000..326d6aa48f --- /dev/null +++ b/crates/shared/src/price_estimation/competition/quote.rs @@ -0,0 +1,305 @@ +use { + super::{compare_error, CompetitionEstimator, PriceRanking}, + crate::price_estimation::{ + Estimate, + PriceEstimateResult, + PriceEstimating, + PriceEstimationError, + Query, + }, + anyhow::Context, + futures::future::{BoxFuture, FutureExt, TryFutureExt}, + model::order::OrderKind, + primitive_types::{H160, U256}, + std::{cmp::Ordering, sync::Arc}, +}; + +impl PriceEstimating for CompetitionEstimator> { + fn estimate(&self, query: Arc) -> BoxFuture<'_, PriceEstimateResult> { + async move { + let out_token = match query.kind { + OrderKind::Buy => query.sell_token, + OrderKind::Sell => query.buy_token, + }; + let get_context = self.ranking.provide_context(out_token); + + // Filter out 0 gas cost estimate because they are obviously wrong and would + // likely win the price competition which would lead to us paying huge + // subsidies. + let gas_is_reasonable = |r: &PriceEstimateResult| r.as_ref().is_ok_and(|r| r.gas > 0); + let get_results = self + .produce_results(query.clone(), gas_is_reasonable, |e, q| e.estimate(q)) + .map(Result::Ok); + + let (context, results) = futures::try_join!(get_context, get_results)?; + + let winner = results + .into_iter() + .filter(|(_index, r)| r.is_err() || gas_is_reasonable(r)) + .max_by(|a, b| compare_quote_result(&query, &a.1, &b.1, &context)) + .with_context(|| "all price estimates reported 0 gas cost") + .map_err(PriceEstimationError::EstimatorInternal)?; + self.report_winner(&query, query.kind, winner) + } + .boxed() + } +} + +fn compare_quote_result( + query: &Query, + a: &PriceEstimateResult, + b: &PriceEstimateResult, + context: &RankingContext, +) -> Ordering { + match (a, b) { + (Ok(a), Ok(b)) => compare_quote(query, a, b, context), + (Ok(_), Err(_)) => Ordering::Greater, + (Err(_), Ok(_)) => Ordering::Less, + (Err(a), Err(b)) => compare_error(a, b), + } +} + +fn compare_quote(query: &Query, a: &Estimate, b: &Estimate, context: &RankingContext) -> Ordering { + let a = context.effective_eth_out(a, query.kind); + let b = context.effective_eth_out(b, query.kind); + match query.kind { + OrderKind::Buy => a.cmp(&b).reverse(), + OrderKind::Sell => a.cmp(&b), + } +} + +impl PriceRanking { + async fn provide_context(&self, token: H160) -> Result { + match self { + PriceRanking::MaxOutAmount => Ok(RankingContext { + native_price: 1.0, + gas_price: 0., + }), + PriceRanking::BestBangForBuck { native, gas } => { + let gas = gas.clone(); + let native = native.clone(); + let gas = gas + .estimate() + .map_ok(|gas| gas.effective_gas_price()) + .map_err(PriceEstimationError::ProtocolInternal); + let (native_price, gas_price) = + futures::try_join!(native.estimate_native_price(token), gas)?; + + Ok(RankingContext { + native_price, + gas_price, + }) + } + } + } +} + +struct RankingContext { + native_price: f64, + gas_price: f64, +} + +impl RankingContext { + /// Computes the actual received value from this estimate that takes `gas` + /// into account. If an extremely complex trade route would only result + /// in slightly more `out_amount` than a simple trade route the simple + /// trade route would report a higher `out_amount_in_eth`. This is also + /// referred to as "bang-for-buck" and what matters most to traders. + fn effective_eth_out(&self, estimate: &Estimate, kind: OrderKind) -> U256 { + let eth_out = estimate.out_amount.to_f64_lossy() * self.native_price; + let fees = estimate.gas as f64 * self.gas_price; + let effective_eth_out = match kind { + // High fees mean receiving less `buy_token` from your sell order. + OrderKind::Sell => eth_out - fees, + // High fees mean paying more `sell_token` for your buy order. + OrderKind::Buy => eth_out + fees, + }; + // converts `NaN` and `(-∞, 0]` to `0` + U256::from_f64_lossy(effective_eth_out) + } +} + +#[cfg(test)] +mod tests { + use { + super::*, + crate::{ + gas_price_estimation::FakeGasPriceEstimator, + price_estimation::{native::MockNativePriceEstimating, MockPriceEstimating}, + }, + gas_estimation::GasPrice1559, + model::order::OrderKind, + }; + + fn price(out_amount: u128, gas: u64) -> PriceEstimateResult { + Ok(Estimate { + out_amount: out_amount.into(), + gas, + ..Default::default() + }) + } + + fn error(err: PriceEstimationError) -> Result { + Err(err) + } + + /// Builds a `BestBangForBuck` setup where every token is estimated + /// to be half as valuable as ETH and the gas price is 2. + /// That effectively means every unit of `gas` in an estimate worth + /// 4 units of `out_amount`. + fn bang_for_buck_ranking() -> PriceRanking { + // Make `out_token` half as valuable as `ETH` and set gas price to 2. + // That means 1 unit of `gas` is equal to 4 units of `out_token`. + let mut native = MockNativePriceEstimating::new(); + native + .expect_estimate_native_price() + .returning(move |_| async { Ok(0.5) }.boxed()); + let gas = Arc::new(FakeGasPriceEstimator::new(GasPrice1559 { + base_fee_per_gas: 2.0, + max_fee_per_gas: 2.0, + max_priority_fee_per_gas: 2.0, + })); + PriceRanking::BestBangForBuck { + native: Arc::new(native), + gas, + } + } + + /// Returns the best estimate with respect to the provided ranking and order + /// kind. + async fn best_response( + ranking: PriceRanking, + kind: OrderKind, + estimates: Vec, + ) -> PriceEstimateResult { + fn estimator(estimate: PriceEstimateResult) -> Arc { + let mut estimator = MockPriceEstimating::new(); + estimator + .expect_estimate() + .times(1) + .return_once(move |_| async move { estimate }.boxed()); + Arc::new(estimator) + } + + let priority: CompetitionEstimator> = CompetitionEstimator::new( + vec![estimates + .into_iter() + .enumerate() + .map(|(i, e)| (format!("estimator_{i}"), estimator(e))) + .collect()], + ranking.clone(), + ); + + priority + .estimate(Arc::new(Query { + kind, + ..Default::default() + })) + .await + } + + /// Verifies that `PriceRanking::BestBangForBuck` correctly adjusts + /// `out_amount` of quotes based on the `gas` used for the quote. E.g. + /// if a quote requires a significantly more complex execution but does + /// not provide a significantly better `out_amount` than a simpler quote + /// the simpler quote will be preferred. + #[tokio::test] + async fn best_bang_for_buck_adjusts_for_complexity() { + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Sell, + vec![ + // User effectively receives `100_000` `buy_token`. + price(104_000, 1_000), + // User effectively receives `99_999` `buy_token`. + price(107_999, 2_000), + ], + ) + .await; + assert_eq!(best, price(104_000, 1_000)); + + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Buy, + vec![ + // User effectively pays `100_000` `sell_token`. + price(96_000, 1_000), + // User effectively pays `100_002` `sell_token`. + price(92_002, 2_000), + ], + ) + .await; + assert_eq!(best, price(96_000, 1_000)); + } + + /// Same test as above but now we also add an estimate that should + /// win under normal circumstances but the `gas` cost is suspiciously + /// low so we discard it. This protects us from quoting unreasonably + /// low fees for user orders. + #[tokio::test] + async fn discards_low_gas_cost_estimates() { + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Sell, + vec![ + // User effectively receives `100_000` `buy_token`. + price(104_000, 1_000), + // User effectively receives `99_999` `buy_token`. + price(107_999, 2_000), + // User effectively receives `104_000` `buy_token` but the estimate + // gets discarded because it quotes 0 gas. + price(104_000, 0), + ], + ) + .await; + assert_eq!(best, price(104_000, 1_000)); + + let best = best_response( + bang_for_buck_ranking(), + OrderKind::Buy, + vec![ + // User effectively pays `100_000` `sell_token`. + price(96_000, 1_000), + // User effectively pays `100_002` `sell_token`. + price(92_002, 2_000), + // User effectively pays `99_000` `sell_token` but the estimate + // gets discarded because it quotes 0 gas. + price(99_000, 0), + ], + ) + .await; + assert_eq!(best, price(96_000, 1_000)); + } + + /// If all estimators returned an error we return the one with the highest + /// priority. + #[tokio::test] + async fn returns_highest_priority_error() { + // Returns errors with highest priority. + let best = best_response( + PriceRanking::MaxOutAmount, + OrderKind::Sell, + vec![ + error(PriceEstimationError::RateLimited), + error(PriceEstimationError::ProtocolInternal(anyhow::anyhow!("!"))), + ], + ) + .await; + assert_eq!(best, error(PriceEstimationError::RateLimited)); + } + + /// Any price estimate, no matter how bad, is preferred over an error. + #[tokio::test] + async fn prefer_estimate_over_error() { + let best = best_response( + PriceRanking::MaxOutAmount, + OrderKind::Sell, + vec![ + price(1, 1_000_000), + error(PriceEstimationError::RateLimited), + ], + ) + .await; + assert_eq!(best, price(1, 1_000_000)); + } +} diff --git a/crates/shared/src/price_estimation/factory.rs b/crates/shared/src/price_estimation/factory.rs index 9d55c5b078..05c7db0201 100644 --- a/crates/shared/src/price_estimation/factory.rs +++ b/crates/shared/src/price_estimation/factory.rs @@ -2,7 +2,7 @@ use { super::{ balancer_sor::BalancerSor, baseline::BaselinePriceEstimator, - competition::{CompetitionEstimator, RacingCompetitionEstimator}, + competition::CompetitionEstimator, external::ExternalPriceEstimator, http::HttpPriceEstimator, instrumented::InstrumentedPriceEstimator, @@ -354,13 +354,15 @@ impl<'a> PriceEstimatorFactory<'a> { gas: Arc, ) -> Result> { let estimators = self.get_estimators(sources, |entry| &entry.fast)?; - Ok(Arc::new(self.sanitized(Arc::new( - RacingCompetitionEstimator::new( - vec![estimators], - fast_price_estimation_results_required, - PriceRanking::BestBangForBuck { native, gas }, - ), - )))) + Ok(Arc::new( + self.sanitized(Arc::new( + CompetitionEstimator::new( + vec![estimators], + PriceRanking::BestBangForBuck { native, gas }, + ) + .with_early_return(fast_price_estimation_results_required), + )), + )) } pub fn native_price_estimator( @@ -384,11 +386,9 @@ impl<'a> PriceEstimatorFactory<'a> { }) .collect::>>>()?; - let competition_estimator = RacingCompetitionEstimator::new( - estimators, - results_required, - PriceRanking::MaxOutAmount, - ); + let competition_estimator = + CompetitionEstimator::new(estimators, PriceRanking::MaxOutAmount) + .with_early_return(results_required); let native_estimator = Arc::new(CachingNativePriceEstimator::new( Box::new(competition_estimator), self.args.native_price_cache_max_age, From 69a9683d9a47c07f07ce42093aa5a295258b520c Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Tue, 16 Jan 2024 10:48:06 +0100 Subject: [PATCH 16/16] Add tracing logs for fee policy creation (#2290) # Description Adding some trace logs for fee policies creation. There are some orders for which I can't figure out reliably why they are considered in-market without these logs. --- crates/autopilot/src/domain/fee/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/autopilot/src/domain/fee/mod.rs b/crates/autopilot/src/domain/fee/mod.rs index 0d994dae18..14e97d5b4e 100644 --- a/crates/autopilot/src/domain/fee/mod.rs +++ b/crates/autopilot/src/domain/fee/mod.rs @@ -46,6 +46,7 @@ impl ProtocolFee { return vec![]; }; + tracing::debug!(?order.metadata.uid, ?self.policy, ?order.data.sell_amount, ?order.data.buy_amount, ?quote, "checking if order is outside market price"); if boundary::is_order_outside_market_price( &order.data.sell_amount, &order.data.buy_amount,