From 1a7e91b108826e75b7f17cdab6270568ab2ccf7e Mon Sep 17 00:00:00 2001 From: Mateo Date: Thu, 17 Oct 2024 15:52:44 +0200 Subject: [PATCH] rework comments --- crates/autopilot/src/arguments.rs | 100 ++++++++++++++- crates/autopilot/src/infra/solvers/mod.rs | 4 +- crates/autopilot/src/run.rs | 11 +- crates/autopilot/src/run_loop.rs | 120 ++++++------------ crates/orderbook/src/run.rs | 14 +- crates/shared/src/arguments.rs | 91 +------------ crates/shared/src/price_estimation.rs | 36 +++++- crates/shared/src/price_estimation/factory.rs | 3 +- 8 files changed, 193 insertions(+), 186 deletions(-) diff --git a/crates/autopilot/src/arguments.rs b/crates/autopilot/src/arguments.rs index cf31431dcb..1a2a2b077b 100644 --- a/crates/autopilot/src/arguments.rs +++ b/crates/autopilot/src/arguments.rs @@ -1,15 +1,22 @@ use { crate::{domain::fee::FeeFactor, infra}, - anyhow::Context, + anyhow::{ensure, Context}, clap::ValueEnum, - primitive_types::H160, + primitive_types::{H160, U256}, shared::{ - arguments::{display_list, display_option, ExternalSolver}, + arguments::{display_list, display_option}, bad_token::token_owner_finder, http_client, price_estimation::{self, NativePriceEstimators}, }, - std::{net::SocketAddr, num::NonZeroUsize, str::FromStr, time::Duration}, + std::{ + fmt, + fmt::{Display, Formatter}, + net::SocketAddr, + num::NonZeroUsize, + str::FromStr, + time::Duration, + }, url::Url, }; @@ -137,7 +144,8 @@ pub struct Arguments { )] pub trusted_tokens_update_interval: Duration, - /// A list of drivers in the following format: `|,|` + /// A list of drivers in the following format: + /// `|||` #[clap(long, env, use_value_delimiter = true)] pub drivers: Vec, @@ -361,6 +369,47 @@ impl std::fmt::Display for Arguments { } } +/// External solver driver configuration +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ExternalSolver { + pub name: String, + pub url: Url, + pub submission_address: H160, + pub fairness_threshold: Option, +} + +impl Display for ExternalSolver { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}({})", self.name, self.url) + } +} + +impl FromStr for ExternalSolver { + type Err = anyhow::Error; + + fn from_str(solver: &str) -> anyhow::Result { + let parts: Vec<&str> = solver.split('|').collect(); + ensure!(parts.len() >= 3, "not enough arguments for external solver"); + let (name, url) = (parts[0], parts[1]); + let url: Url = url.parse()?; + let submission_address = + H160::from_str(parts[2]).context("failed to parse submission address")?; + let fairness_threshold = match parts.get(3) { + Some(value) => { + Some(U256::from_dec_str(value).context("failed to parse fairness threshold")?) + } + None => None, + }; + + Ok(Self { + name: name.to_owned(), + url, + fairness_threshold, + submission_address, + }) + } +} + /// A fee policy to be used for orders base on it's class. /// Examples: /// - Surplus with a high enough cap for limit orders: surplus:0.5:0.9:limit @@ -519,7 +568,7 @@ impl FromStr for CowAmmConfig { #[cfg(test)] mod test { - use super::*; + use {super::*, hex_literal::hex}; #[test] fn test_fee_factor_limits() { @@ -544,4 +593,43 @@ mod test { .contains("Factor must be in the range [0, 1)"),) } } + + #[test] + fn parse_driver() { + let argument = "name1|http://localhost:8080|0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2"; + let driver = ExternalSolver::from_str(argument).unwrap(); + let expected = ExternalSolver { + name: "name1".into(), + url: Url::parse("http://localhost:8080").unwrap(), + fairness_threshold: None, + submission_address: H160::from_slice(&hex!("C02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2")), + }; + assert_eq!(driver, expected); + } + + #[test] + fn parse_driver_with_threshold() { + let argument = "name1|http://localhost:8080|0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2|1000000000000000000"; + let driver = ExternalSolver::from_str(argument).unwrap(); + let expected = ExternalSolver { + name: "name1".into(), + url: Url::parse("http://localhost:8080").unwrap(), + submission_address: H160::from_slice(&hex!("C02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2")), + fairness_threshold: Some(U256::exp10(18)), + }; + assert_eq!(driver, expected); + } + + #[test] + fn parse_driver_with_submission_address_and_threshold() { + let argument = "name1|http://localhost:8080|0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2|1000000000000000000"; + let driver = ExternalSolver::from_str(argument).unwrap(); + let expected = ExternalSolver { + name: "name1".into(), + url: Url::parse("http://localhost:8080").unwrap(), + submission_address: H160::from_slice(&hex!("C02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2")), + fairness_threshold: Some(U256::exp10(18)), + }; + assert_eq!(driver, expected); + } } diff --git a/crates/autopilot/src/infra/solvers/mod.rs b/crates/autopilot/src/infra/solvers/mod.rs index a7bfa0afe4..de7d51bb56 100644 --- a/crates/autopilot/src/infra/solvers/mod.rs +++ b/crates/autopilot/src/infra/solvers/mod.rs @@ -19,7 +19,7 @@ pub struct Driver { // winning solution should be discarded if it contains at least one order, which // another driver solved with surplus exceeding this driver's surplus by `threshold` pub fairness_threshold: Option, - pub submission_address: Option, + pub submission_address: eth::Address, client: Client, } @@ -28,7 +28,7 @@ impl Driver { url: Url, name: String, fairness_threshold: Option, - submission_address: Option, + submission_address: eth::Address, ) -> Self { Self { name, diff --git a/crates/autopilot/src/run.rs b/crates/autopilot/src/run.rs index 4dd9771e1c..447dc25e4d 100644 --- a/crates/autopilot/src/run.rs +++ b/crates/autopilot/src/run.rs @@ -348,7 +348,12 @@ pub async fn run(args: Arguments) { let price_estimator = price_estimator_factory .price_estimator( - &args.order_quoting.price_estimation_drivers, + &args + .order_quoting + .price_estimation_drivers + .iter() + .map(|price_estimator_driver| price_estimator_driver.clone().into()) + .collect::>(), native_price_estimator.clone(), gas_price_estimator.clone(), ) @@ -541,7 +546,7 @@ pub async fn run(args: Arguments) { driver.url, driver.name, driver.fairness_threshold.map(Into::into), - driver.submission_address.map(Into::into), + driver.submission_address.into(), )) }) .collect(), @@ -570,7 +575,7 @@ async fn shadow_mode(args: Arguments) -> ! { driver.url, driver.name, driver.fairness_threshold.map(Into::into), - driver.submission_address.map(Into::into), + driver.submission_address.into(), )) }) .collect(); diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 29c02b8b03..168a23c66b 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -721,63 +721,27 @@ impl RunLoop { request: &solve::Request, ) -> Result>, SolveError> { - // @TODO: to be deleted once the submission address in the driver configuration - // is not optional - if let Some(submission_address) = driver.submission_address { - let authenticator = self.eth.contracts().authenticator(); - let is_allowed = authenticator - .is_solver(submission_address.into()) - .call() - .await; - - // Do not send the request to the driver if the solver is denied - match is_allowed { - Ok(true) => {} - Ok(false) => return Err(SolveError::SolverDenyListed), - Err(err) => { - // log warning but discard solution anyway to be on the safe side - tracing::warn!( - driver = driver.name, - ?driver.submission_address, - ?err, - "failed to check if solver is deny listed" - ); - return Err(SolveError::SolverDenyListed); - } - } + let authenticator = self.eth.contracts().authenticator(); + let is_allowed = authenticator + .is_solver(driver.submission_address.into()) + .call() + .await; - let response = tokio::time::timeout(self.config.solve_deadline, driver.solve(request)) - .await - .map_err(|_| SolveError::Timeout)? - .map_err(SolveError::Failure)?; - if response.solutions.is_empty() { - return Err(SolveError::NoSolutions); + // Do not send the request to the driver if the solver is denied + match is_allowed { + Ok(true) => {} + Ok(false) => return Err(SolveError::SolverDenyListed), + Err(err) => { + tracing::warn!( + driver = driver.name, + ?driver.submission_address, + ?err, + "failed to check if solver is deny listed" + ); + return Err(SolveError::SolverDenyListed); } - // Filter the responses - Ok(response - .into_domain() - .into_iter() - .filter(|solution| { - solution - .as_ref() - .ok() - .map(|solution| solution.solver() == submission_address) - .unwrap_or_default() - }) - .collect()) - } else { - self.try_solve_legacy(driver, request).await } - } - // @TODO: Legacy try_solve, to be deleted once the submission address in the - // driver configuration is not optional - async fn try_solve_legacy( - &self, - driver: &infra::Driver, - request: &solve::Request, - ) -> Result>, SolveError> - { let response = tokio::time::timeout(self.config.solve_deadline, driver.solve(request)) .await .map_err(|_| SolveError::Timeout)? @@ -785,33 +749,29 @@ impl RunLoop { if response.solutions.is_empty() { return Err(SolveError::NoSolutions); } - let solutions = response.into_domain(); - - // TODO: remove this workaround when implementing #2780 - // Discard any solutions from solvers that got deny listed in the mean time. - let futures = solutions.into_iter().map(|solution| async { - let solution = solution?; - let solver = solution.solver(); - let authenticator = self.eth.contracts().authenticator(); - let is_allowed = authenticator.is_solver(solver.into()).call().await; - - match is_allowed { - Ok(true) => Ok(solution), - Ok(false) => Err(domain::competition::SolutionError::SolverDenyListed), - Err(err) => { - // log warning but discard solution anyway to be on the safe side - tracing::warn!( - driver = driver.name, - ?solver, - ?err, - "failed to check if solver is deny listed" - ); - Err(domain::competition::SolutionError::SolverDenyListed) - } - } - }); - - Ok(futures::future::join_all(futures).await) + // Filter the responses + Ok(response + .into_domain() + .into_iter() + .filter(|solution| { + solution + .as_ref() + .ok() + .map(|solution| { + let is_solution_from_driver = + solution.solver() == driver.submission_address; + if !is_solution_from_driver { + tracing::warn!( + driver = driver.name, + ?driver.submission_address, + "the solution is not received from the driver submission address" + ); + } + is_solution_from_driver + }) + .unwrap_or_default() + }) + .collect()) } /// Execute the solver's solution. Returns Ok when the corresponding diff --git a/crates/orderbook/src/run.rs b/crates/orderbook/src/run.rs index 8e87611994..033f0bb843 100644 --- a/crates/orderbook/src/run.rs +++ b/crates/orderbook/src/run.rs @@ -289,14 +289,24 @@ pub async fn run(args: Arguments) { let price_estimator = price_estimator_factory .price_estimator( - &args.order_quoting.price_estimation_drivers, + &args + .order_quoting + .price_estimation_drivers + .iter() + .map(|price_estimator| price_estimator.clone().into()) + .collect::>(), native_price_estimator.clone(), gas_price_estimator.clone(), ) .unwrap(); let fast_price_estimator = price_estimator_factory .fast_price_estimator( - &args.order_quoting.price_estimation_drivers, + &args + .order_quoting + .price_estimation_drivers + .iter() + .map(|price_estimator| price_estimator.clone().into()) + .collect::>(), args.fast_price_estimation_results_required, native_price_estimator.clone(), gas_price_estimator.clone(), diff --git a/crates/shared/src/arguments.rs b/crates/shared/src/arguments.rs index 5cf0c4e587..968c8888d2 100644 --- a/crates/shared/src/arguments.rs +++ b/crates/shared/src/arguments.rs @@ -55,8 +55,6 @@ macro_rules! logging_args_with_default_filter { pub struct ExternalSolver { pub name: String, pub url: Url, - pub submission_address: Option, - pub fairness_threshold: Option, } // The following arguments are used to configure the order creation process @@ -480,105 +478,20 @@ impl FromStr for ExternalSolver { fn from_str(solver: &str) -> Result { let parts: Vec<&str> = solver.split('|').collect(); - ensure!(parts.len() >= 2, "not enough arguments for external solver"); + ensure!(parts.len() >= 3, "not enough arguments for external solver"); let (name, url) = (parts[0], parts[1]); let url: Url = url.parse()?; - let mut submission_address = None; - let mut fairness_threshold = None; - - // @TODO: To remove the complex logic one submission address is not optional - if parts.len() == 4 { - submission_address = Some( - H160::from_str(parts.get(2).unwrap()) - .context("failed to parse submission address")?, - ); - fairness_threshold = Some( - U256::from_dec_str(parts.get(3).unwrap()) - .context("failed to parse fairness threshold")?, - ); - } - if parts.len() == 3 { - let part = parts.get(2).unwrap(); - if let Ok(value) = H160::from_str(part) { - submission_address = Some(value); - } - if let Ok(value) = U256::from_dec_str(part) { - fairness_threshold = Some(value); - } - ensure!( - submission_address.is_some() || fairness_threshold.is_some(), - "failed to parse external solver arguments" - ); - } Ok(Self { name: name.to_owned(), url, - fairness_threshold, - submission_address, }) } } #[cfg(test)] mod test { - use {super::*, hex_literal::hex}; - - #[test] - fn parse_driver() { - let argument = "name1|http://localhost:8080"; - let driver = ExternalSolver::from_str(argument).unwrap(); - let expected = ExternalSolver { - name: "name1".into(), - url: Url::parse("http://localhost:8080").unwrap(), - fairness_threshold: None, - submission_address: None, - }; - assert_eq!(driver, expected); - } - - #[test] - fn parse_driver_with_threshold() { - let argument = "name1|http://localhost:8080|1000000000000000000"; - let driver = ExternalSolver::from_str(argument).unwrap(); - let expected = ExternalSolver { - name: "name1".into(), - url: Url::parse("http://localhost:8080").unwrap(), - submission_address: None, - fairness_threshold: Some(U256::exp10(18)), - }; - assert_eq!(driver, expected); - } - - #[test] - fn parse_driver_with_submission_address() { - let argument = "name1|http://localhost:8080|0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2"; - let driver = ExternalSolver::from_str(argument).unwrap(); - let expected = ExternalSolver { - name: "name1".into(), - url: Url::parse("http://localhost:8080").unwrap(), - submission_address: Some(H160::from_slice(&hex!( - "C02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2" - ))), - fairness_threshold: None, - }; - assert_eq!(driver, expected); - } - - #[test] - fn parse_driver_with_submission_address_and_threshold() { - let argument = "name1|http://localhost:8080|0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2|1000000000000000000"; - let driver = ExternalSolver::from_str(argument).unwrap(); - let expected = ExternalSolver { - name: "name1".into(), - url: Url::parse("http://localhost:8080").unwrap(), - submission_address: Some(H160::from_slice(&hex!( - "C02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2" - ))), - fairness_threshold: Some(U256::exp10(18)), - }; - assert_eq!(driver, expected); - } + use {super::*}; #[test] fn parse_drivers_wrong_arguments() { diff --git a/crates/shared/src/price_estimation.rs b/crates/shared/src/price_estimation.rs index 6a8fa88674..5dbc24b455 100644 --- a/crates/shared/src/price_estimation.rs +++ b/crates/shared/src/price_estimation.rs @@ -1,10 +1,10 @@ use { crate::{ - arguments::{display_option, display_secret_option, ExternalSolver}, + arguments::{self, display_option, display_secret_option}, conversions::U256Ext, trade_finding::Interaction, }, - anyhow::Result, + anyhow::{ensure, Result}, ethcontract::{H160, U256}, futures::future::BoxFuture, itertools::Itertools, @@ -41,6 +41,36 @@ pub mod trade_verifier; #[derive(Clone, Debug)] pub struct NativePriceEstimators(Vec>); +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct ExternalSolver { + pub name: String, + pub url: Url, +} + +impl From for ExternalSolver { + fn from(value: arguments::ExternalSolver) -> Self { + Self { + name: value.name, + url: value.url, + } + } +} + +impl FromStr for ExternalSolver { + type Err = anyhow::Error; + + fn from_str(solver: &str) -> Result { + let parts: Vec<&str> = solver.split('|').collect(); + ensure!(parts.len() >= 2, "not enough arguments for external solver"); + let (name, url) = (parts[0], parts[1]); + let url: Url = url.parse()?; + Ok(Self { + name: name.to_owned(), + url, + }) + } +} + #[derive(Clone, Debug, Hash, Eq, PartialEq)] pub enum NativePriceEstimator { Driver(ExternalSolver), @@ -585,7 +615,7 @@ mod tests { for repr in [ &NativePriceEstimator::Driver( - ExternalSolver::from_str("baseline|http://localhost:1234/").unwrap(), + ExternalSolver::from_str("baseline|http://localhost:1234/|0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2").unwrap(), ) .to_string(), &NativePriceEstimator::OneInchSpotPriceApi.to_string(), diff --git a/crates/shared/src/price_estimation/factory.rs b/crates/shared/src/price_estimation/factory.rs index 37a2fe5f70..2a3cf1cf8c 100644 --- a/crates/shared/src/price_estimation/factory.rs +++ b/crates/shared/src/price_estimation/factory.rs @@ -12,7 +12,7 @@ use { PriceEstimating, }, crate::{ - arguments::{self, ExternalSolver}, + arguments, bad_token::BadTokenDetecting, baseline_solver::BaseTokens, code_fetching::CachedCodeFetcher, @@ -23,6 +23,7 @@ use { buffered::{self, BufferedRequest, NativePriceBatchFetching}, competition::PriceRanking, native::NativePriceEstimating, + ExternalSolver, }, token_info::TokenInfoFetching, },