From 813fb12ba319e0f9b0d124fb8ccbe49e1dbf699d Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Mon, 13 May 2024 14:09:57 +0200 Subject: [PATCH] Separate driver list for naitive price estimates (#2698) # Description Pass native price estimator drivers as a separate list. This allows us to configure native price estimators which should not be used as a regular price estimator. This is useful for 2 reasons: 1. sometimes a solver gives bad quotes for regular price estimates but could still be used for native price estimates. This can not be done without this PR. 2. specifically for the zeroex we need to use a different solver setup for regular quotes as for native price estimates because they should use different API keys for accounting purposes. # Changes Any solver that should be used in the `--native-price-estimators` argument must also be passed in `--native-price-estimation-drivers` or `--price-estimation-legacy-solvers`. Unfortunately this is still quite weird to configure but at least decouples regular quoting from native price estimation more. ## How to test Adjusted e2e tests --- crates/autopilot/src/arguments.rs | 2 +- crates/autopilot/src/run.rs | 4 -- crates/e2e/src/setup/services.rs | 8 ++- crates/e2e/tests/e2e/quoting.rs | 4 +- crates/orderbook/src/arguments.rs | 2 +- crates/orderbook/src/run.rs | 4 -- crates/shared/src/arguments.rs | 2 +- crates/shared/src/price_estimation.rs | 58 ++++++++++--------- crates/shared/src/price_estimation/factory.rs | 42 +++++--------- playground/docker-compose.fork.yml | 3 +- 10 files changed, 61 insertions(+), 68 deletions(-) diff --git a/crates/autopilot/src/arguments.rs b/crates/autopilot/src/arguments.rs index f592c7a176..740864f9ad 100644 --- a/crates/autopilot/src/arguments.rs +++ b/crates/autopilot/src/arguments.rs @@ -90,7 +90,7 @@ pub struct Arguments { /// native token. Estimators with the same name need to also be specified as /// built-in, legacy or external price estimators (lookup happens in this /// order in case of name collisions) - #[clap(long, env, default_value_t)] + #[clap(long, env)] pub native_price_estimators: NativePriceEstimators, /// How many successful price estimates for each order will cause a native diff --git a/crates/autopilot/src/run.rs b/crates/autopilot/src/run.rs index 0f0985d4ae..7d291179e5 100644 --- a/crates/autopilot/src/run.rs +++ b/crates/autopilot/src/run.rs @@ -425,10 +425,6 @@ pub async fn run(args: Arguments) { let native_price_estimator = price_estimator_factory .native_price_estimator( args.native_price_estimators.as_slice(), - &PriceEstimatorSource::for_args( - &args.order_quoting.price_estimation_drivers, - &args.order_quoting.price_estimation_legacy_solvers, - ), args.native_price_estimation_results_required, ) .unwrap(); diff --git a/crates/e2e/src/setup/services.rs b/crates/e2e/src/setup/services.rs index a13bbc8351..d29248590d 100644 --- a/crates/e2e/src/setup/services.rs +++ b/crates/e2e/src/setup/services.rs @@ -94,7 +94,7 @@ impl<'a> Services<'a> { fn api_autopilot_arguments() -> impl Iterator { [ - "--native-price-estimators=test_quoter".to_string(), + "--native-price-estimators=test_quoter|http://localhost:11088/test_solver".to_string(), "--amount-to-estimate-prices-with=1000000000000000000".to_string(), "--block-stream-poll-interval=1s".to_string(), "--simulation-node-url=http://localhost:8545".to_string(), @@ -243,9 +243,11 @@ impl<'a> Services<'a> { let autopilot_args = vec![ "--drivers=test_solver|http://localhost:11088/test_solver".to_string(), "--price-estimation-drivers=test_quoter|http://localhost:11088/baseline_solver,test_solver|http://localhost:11088/test_solver".to_string(), + "--native-price-estimators=test_quoter|http://localhost:11088/baseline_solver,test_solver|http://localhost:11088/test_solver".to_string(), ]; let api_args = vec![ "--price-estimation-drivers=test_quoter|http://localhost:11088/baseline_solver,test_solver|http://localhost:11088/test_solver".to_string(), + "--native-price-estimators=test_quoter|http://localhost:11088/baseline_solver,test_solver|http://localhost:11088/test_solver".to_string(), ]; (autopilot_args, api_args) } else { @@ -253,11 +255,15 @@ impl<'a> Services<'a> { "--drivers=test_solver|http://localhost:11088/test_solver".to_string(), "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver" .to_string(), + "--native-price-estimators=test_quoter|http://localhost:11088/test_solver" + .to_string(), ]; let api_args = vec![ "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver" .to_string(), + "--native-price-estimators=test_quoter|http://localhost:11088/test_solver" + .to_string(), ]; (autopilot_args, api_args) }; diff --git a/crates/e2e/tests/e2e/quoting.rs b/crates/e2e/tests/e2e/quoting.rs index 4bacaf4508..b67f832bf2 100644 --- a/crates/e2e/tests/e2e/quoting.rs +++ b/crates/e2e/tests/e2e/quoting.rs @@ -215,7 +215,9 @@ async fn uses_stale_liquidity(web3: Web3) { wait_for_condition(TIMEOUT, || async { // Mint blocks until we evict the cached liquidty and fetch the new state. onchain.mint_block().await; - let next = services.submit_quote("e).await.unwrap(); + let Ok(next) = services.submit_quote("e).await else { + return false; + }; next.quote.buy_amount != first.quote.buy_amount }) .await diff --git a/crates/orderbook/src/arguments.rs b/crates/orderbook/src/arguments.rs index c33442c178..178d6fb3d6 100644 --- a/crates/orderbook/src/arguments.rs +++ b/crates/orderbook/src/arguments.rs @@ -90,7 +90,7 @@ pub struct Arguments { /// Which estimators to use to estimate token prices in terms of the chain's /// native token. - #[clap(long, env, default_value_t)] + #[clap(long, env)] pub native_price_estimators: NativePriceEstimators, /// How many successful price estimates for each order will cause a fast diff --git a/crates/orderbook/src/run.rs b/crates/orderbook/src/run.rs index 8e4085ff0f..3475e6ff18 100644 --- a/crates/orderbook/src/run.rs +++ b/crates/orderbook/src/run.rs @@ -380,10 +380,6 @@ pub async fn run(args: Arguments) { let native_price_estimator = price_estimator_factory .native_price_estimator( args.native_price_estimators.as_slice(), - &PriceEstimatorSource::for_args( - &args.order_quoting.price_estimation_drivers, - &args.order_quoting.price_estimation_legacy_solvers, - ), args.fast_price_estimation_results_required, ) .unwrap(); diff --git a/crates/shared/src/arguments.rs b/crates/shared/src/arguments.rs index b5be548825..b27864acf7 100644 --- a/crates/shared/src/arguments.rs +++ b/crates/shared/src/arguments.rs @@ -51,7 +51,7 @@ macro_rules! logging_args_with_default_filter { }; } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ExternalSolver { pub name: String, pub url: Url, diff --git a/crates/shared/src/price_estimation.rs b/crates/shared/src/price_estimation.rs index b4bcd0b06f..b56abb2fef 100644 --- a/crates/shared/src/price_estimation.rs +++ b/crates/shared/src/price_estimation.rs @@ -1,6 +1,6 @@ use { crate::{ - arguments::{display_option, display_secret_option}, + arguments::{display_option, display_secret_option, ExternalSolver}, conversions::U256Ext, trade_finding::Interaction, }, @@ -19,6 +19,7 @@ use { fmt::{self, Display, Formatter}, future::Future, hash::Hash, + str::FromStr, sync::Arc, time::{Duration, Instant}, }, @@ -42,15 +43,15 @@ pub struct NativePriceEstimators(Vec>); #[derive(Clone, Debug, Hash, Eq, PartialEq)] pub enum NativePriceEstimator { - GenericPriceEstimator(String), + Driver(ExternalSolver), OneInchSpotPriceApi, } impl Display for NativePriceEstimator { fn fmt(&self, f: &mut Formatter) -> fmt::Result { let formatter = match self { - NativePriceEstimator::GenericPriceEstimator(s) => s, - NativePriceEstimator::OneInchSpotPriceApi => "OneInchSpotPriceApi", + NativePriceEstimator::Driver(s) => format!("{}|{}", &s.name, s.url), + NativePriceEstimator::OneInchSpotPriceApi => "OneInchSpotPriceApi".into(), }; write!(f, "{}", formatter) } @@ -62,14 +63,6 @@ impl NativePriceEstimators { } } -impl Default for NativePriceEstimators { - fn default() -> Self { - Self(vec![vec![NativePriceEstimator::GenericPriceEstimator( - "Baseline".into(), - )]]) - } -} - impl Display for NativePriceEstimators { fn fmt(&self, f: &mut Formatter) -> fmt::Result { let formatter = self @@ -85,26 +78,32 @@ impl Display for NativePriceEstimators { } } -impl From<&str> for NativePriceEstimators { - fn from(s: &str) -> Self { - Self( +impl FromStr for NativePriceEstimators { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + Ok(Self( s.split(';') .map(|sub_list| { sub_list .split(',') - .map(NativePriceEstimator::from) - .collect::>() + .map(NativePriceEstimator::from_str) + .collect::>>() }) - .collect::>>(), - ) + .collect::>>>()?, + )) } } -impl From<&str> for NativePriceEstimator { - fn from(s: &str) -> Self { +impl FromStr for NativePriceEstimator { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { match s { - "OneInchSpotPriceApi" => NativePriceEstimator::OneInchSpotPriceApi, - estimator => NativePriceEstimator::GenericPriceEstimator(estimator.into()), + "OneInchSpotPriceApi" => Ok(NativePriceEstimator::OneInchSpotPriceApi), + estimator => Ok(NativePriceEstimator::Driver(ExternalSolver::from_str( + estimator, + )?)), } } } @@ -482,16 +481,19 @@ mod tests { // Arg::value_parser or #[arg(value_enum)]: // https://docs.rs/clap/latest/clap/_derive/index.html#arg-attributes - let parsed = |arg: &str| NativePriceEstimators::from(arg); + let parsed = |arg: &str| NativePriceEstimators::from_str(arg); let stringified = |arg: &NativePriceEstimators| format!("{arg}"); for repr in [ - &NativePriceEstimator::GenericPriceEstimator("Baseline".into()).to_string(), + &NativePriceEstimator::Driver( + ExternalSolver::from_str("baseline|http://localhost:1234/").unwrap(), + ) + .to_string(), &NativePriceEstimator::OneInchSpotPriceApi.to_string(), - "one,two;three,four", - &format!("one,two;{},four", NativePriceEstimator::OneInchSpotPriceApi), + "one|http://localhost:1111/,two|http://localhost:2222/;three|http://localhost:3333/,four|http://localhost:4444/", + &format!("one|http://localhost:1111/,two|http://localhost:2222/;{},four|http://localhost:4444/", NativePriceEstimator::OneInchSpotPriceApi), ] { - assert_eq!(stringified(&parsed(repr)), repr); + assert_eq!(stringified(&parsed(repr).unwrap()), repr); } } } diff --git a/crates/shared/src/price_estimation/factory.rs b/crates/shared/src/price_estimation/factory.rs index 1fea7d87ca..99d0e524b7 100644 --- a/crates/shared/src/price_estimation/factory.rs +++ b/crates/shared/src/price_estimation/factory.rs @@ -29,7 +29,7 @@ use { }, token_info::TokenInfoFetching, }, - anyhow::{anyhow, Context as _, Result}, + anyhow::{Context as _, Result}, ethcontract::H160, ethrpc::current_block::CurrentBlockStream, gas_estimation::GasPriceEstimating, @@ -206,33 +206,24 @@ impl<'a> PriceEstimatorFactory<'a> { fn create_native_estimator( &mut self, - source: NativePriceEstimatorSource, - external: &[PriceEstimatorSource], + source: &NativePriceEstimatorSource, ) -> Result<(String, Arc)> { match source { - NativePriceEstimatorSource::GenericPriceEstimator(estimator) => { + NativePriceEstimatorSource::Driver(driver) => { let native_token_price_estimation_amount = self.native_token_price_estimation_amount()?; - self.get_estimators(external, |entry| &entry.native)? - .into_iter() - .map( - |(name, estimator)| -> (String, Arc) { - ( - name, - Arc::new(NativePriceEstimator::new( - Arc::new(self.sanitized(estimator)), - self.network.native_token, - native_token_price_estimation_amount, - )), - ) - }, - ) - .find(|external| external.0 == estimator) - .ok_or(anyhow!( - "Couldn't find generic price estimator with name {} to instantiate native \ - estimator", - estimator - )) + let estimator = self + .get_estimator(&PriceEstimatorSource::External(driver.clone()))? + .native + .clone(); + Ok(( + driver.name.clone(), + Arc::new(NativePriceEstimator::new( + Arc::new(self.sanitized(estimator)), + self.network.native_token, + native_token_price_estimation_amount, + )), + )) } NativePriceEstimatorSource::OneInchSpotPriceApi => Ok(( "OneInchSpotPriceApi".into(), @@ -324,7 +315,6 @@ impl<'a> PriceEstimatorFactory<'a> { pub fn native_price_estimator( &mut self, native: &[Vec], - external: &[PriceEstimatorSource], results_required: NonZeroUsize, ) -> Result> { anyhow::ensure!( @@ -337,7 +327,7 @@ impl<'a> PriceEstimatorFactory<'a> { .map(|stage| { stage .iter() - .map(|source| self.create_native_estimator(source.clone(), external)) + .map(|source| self.create_native_estimator(source)) .collect::>>() }) .collect::>>>()?; diff --git a/playground/docker-compose.fork.yml b/playground/docker-compose.fork.yml index 045888669c..d01721c246 100644 --- a/playground/docker-compose.fork.yml +++ b/playground/docker-compose.fork.yml @@ -76,8 +76,8 @@ services: - EIP1271_SKIP_CREATION_VALIDATION=true - ENABLE_EIP1271_ORDERS=true - PRICE_ESTIMATORS=None - - NATIVE_PRICE_ESTIMATORS=baseline - PRICE_ESTIMATION_DRIVERS=baseline|http://driver/baseline + - NATIVE_PRICE_ESTIMATORS=baseline|http://driver/baseline - DRIVERS=baseline|http://driver/baseline - BIND_ADDRESS=0.0.0.0:80 - CHAIN_ID=$CHAIN @@ -109,6 +109,7 @@ services: - NATIVE_PRICE_CACHE_MAX_AGE=20m - SOLVER_TIME_LIMIT=5 - PRICE_ESTIMATION_DRIVERS=baseline|http://driver/baseline + - NATIVE_PRICE_ESTIMATORS=baseline|http://driver/baseline - DRIVERS=baseline|http://driver/baseline - SKIP_EVENT_SYNC=true - BASELINE_SOURCES=None