diff --git a/crates/configs/src/native_price.rs b/crates/configs/src/native_price.rs index 427cbefed6..8916ab6e60 100644 --- a/crates/configs/src/native_price.rs +++ b/crates/configs/src/native_price.rs @@ -13,7 +13,7 @@ const fn default_cache_concurrent_requests() -> NonZeroUsize { } const fn default_results_required() -> NonZeroUsize { - NonZeroUsize::new(2).expect("value should not be zero") + NonZeroUsize::new(3).expect("value should not be zero") } #[derive(Debug, Clone, Deserialize)] diff --git a/crates/price-estimation/src/competition/mod.rs b/crates/price-estimation/src/competition/mod.rs index b2b1e1cb9e..f0e68437e0 100644 --- a/crates/price-estimation/src/competition/mod.rs +++ b/crates/price-estimation/src/competition/mod.rs @@ -37,18 +37,18 @@ type ResultWithIndex = (EstimatorIndex, Result); pub struct CompetitionEstimator { stages: Vec>, usable_results_for_early_return: NonZeroUsize, - ranking: PriceRanking, + quote_context_mode: QuoteContextMode, verification_mode: QuoteVerificationMode, } impl CompetitionEstimator { - pub fn new(stages: Vec>, ranking: PriceRanking) -> Self { + pub fn new(stages: Vec>, quote_context_mode: QuoteContextMode) -> Self { assert!(!stages.is_empty()); assert!(stages.iter().all(|stage| !stage.is_empty())); Self { stages, usable_results_for_early_return: NonZeroUsize::MAX, - ranking, + quote_context_mode, verification_mode: QuoteVerificationMode::Unverified, } } @@ -231,14 +231,14 @@ fn metrics() -> &'static Metrics { .expect("unexpected error getting metrics instance") } -/// The metric which decides the winning price estimate. +/// Controls which data will be provided in addition to the +/// quote to determine which quote is better. #[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 { +pub enum QuoteContextMode { + /// No additional data will be provided. + None, + /// Returns information to determine the gas cost of a quote. + GasCost { native: Arc, gas: Arc, }, @@ -361,7 +361,7 @@ mod tests { ("first".to_owned(), Arc::new(first)), ("second".to_owned(), Arc::new(second)), ]], - PriceRanking::MaxOutAmount, + QuoteContextMode::None, ); let result = priority.estimate(queries[0].clone()).await; @@ -444,7 +444,7 @@ mod tests { ("second".to_owned(), Arc::new(second)), ("third".to_owned(), Arc::new(third)), ]], - PriceRanking::MaxOutAmount, + QuoteContextMode::None, ); let racing = racing.with_early_return(1.try_into().unwrap()); @@ -521,7 +521,7 @@ mod tests { ("fourth".to_owned(), Arc::new(fourth)), ], ], - PriceRanking::MaxOutAmount, + QuoteContextMode::None, ); let racing = racing.with_early_return(2.try_into().unwrap()); @@ -591,7 +591,7 @@ mod tests { vec![("fourth".to_owned(), Arc::new(fourth))], ], usable_results_for_early_return: NonZeroUsize::new(2).unwrap(), - ranking: PriceRanking::MaxOutAmount, + quote_context_mode: QuoteContextMode::None, verification_mode: QuoteVerificationMode::Unverified, }; diff --git a/crates/price-estimation/src/competition/native.rs b/crates/price-estimation/src/competition/native.rs index 8292199a61..9aebe29dc4 100644 --- a/crates/price-estimation/src/competition/native.rs +++ b/crates/price-estimation/src/competition/native.rs @@ -7,8 +7,9 @@ use { alloy::primitives::Address, anyhow::Context, futures::{FutureExt, future::BoxFuture}, + itertools::{Either, Itertools}, model::order::OrderKind, - std::{cmp::Ordering, sync::Arc, time::Duration}, + std::{sync::Arc, time::Duration}, tracing::instrument, }; @@ -50,25 +51,36 @@ impl NativePriceEstimating for CompetitionEstimator, - 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), +/// There are no rewards or penalties for providing native prices which means +/// there are very little incentives for solvers to provide accurate prices. +/// This can lead to wildly inaccurate prices. Because native prices don't have +/// to be super competitive we pick the median price instead of the very best +/// one. +fn pick_median_price( + results: Vec>, +) -> Option> { + let (mut prices, errs): (Vec<_>, Vec<_>) = + results.into_iter().partition_map(|(idx, r)| match r { + Ok(price) => Either::Left((idx, price)), + Err(e) => Either::Right((idx, e)), + }); + if prices.is_empty() { + errs.into_iter() + .max_by(|(_, a), (_, b)| compare_error(a, b)) + .map(|(idx, e)| (idx, Err(e))) + } else { + // we sort prices from best to worst so that `len / 2` skews + // in favour of worse prices as high prices have worse consequences + prices.sort_by(|(_, a), (_, b)| b.total_cmp(a)); + let (idx, price) = prices.remove(prices.len() / 2); + Some((idx, Ok(price))) } } @@ -78,7 +90,7 @@ mod tests { super::*, crate::{ HEALTHY_PRICE_ESTIMATION_TIME, - competition::PriceRanking, + competition::QuoteContextMode, native::MockNativePriceEstimating, }, std::pin::Pin, @@ -90,7 +102,7 @@ mod tests { /// Returns the best native estimate with respect to the provided ranking /// and order kind. async fn best_response( - ranking: PriceRanking, + ranking: QuoteContextMode, estimates: Vec, ) -> Result { fn estimator(estimate: NativePriceFuture) -> Arc { @@ -119,26 +131,41 @@ mod tests { .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. + async fn returns_median_native_price() { + // with 3 or more estimates, the median (middle value) is returned, protecting + // us against outliers let best = best_response( - PriceRanking::MaxOutAmount, - vec![async { Ok(1.) }.boxed(), async { Ok(2.) }.boxed()], + QuoteContextMode::None, + vec![ + async { Ok(1.) }.boxed(), + async { Ok(100.) }.boxed(), + async { Ok(2.) }.boxed(), + ], ) .await; assert_eq!(best, Ok(2.)); } + #[tokio::test] + async fn median_price_skews_towards_worse_price() { + // with 2 estimates, we chose the worse price as unreasonably high prices + // cause more issues than low prices + let best = best_response( + QuoteContextMode::None, + vec![async { Ok(1.) }.boxed(), async { Ok(2.) }.boxed()], + ) + .await; + assert_eq!(best, Ok(1.)); + } + /// 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, + QuoteContextMode::None, vec![ async { Err(PriceEstimationError::RateLimited) }.boxed(), async { Err(PriceEstimationError::ProtocolInternal(anyhow::anyhow!("!"))) }.boxed(), @@ -153,7 +180,7 @@ mod tests { #[tokio::test] async fn prefer_estimate_over_error_native() { let best = best_response( - PriceRanking::MaxOutAmount, + QuoteContextMode::None, vec![ async { Ok(1.) }.boxed(), async { Err(PriceEstimationError::RateLimited) }.boxed(), @@ -171,7 +198,7 @@ mod tests { for price in [f64::NEG_INFINITY, -1., 0., f64::INFINITY, subnormal] { let best = best_response( - PriceRanking::MaxOutAmount, + QuoteContextMode::None, vec![async move { Ok(price) }.boxed()], ) .await; @@ -245,7 +272,7 @@ mod tests { ), )], ], - PriceRanking::MaxOutAmount, + QuoteContextMode::None, ) // allow bailing out after 1 successful result to prevent both // stages to be kicked-off at the same time (if we have too few stages diff --git a/crates/price-estimation/src/competition/quote.rs b/crates/price-estimation/src/competition/quote.rs index 7b0d3bd0b5..c9d44d3e4e 100644 --- a/crates/price-estimation/src/competition/quote.rs +++ b/crates/price-estimation/src/competition/quote.rs @@ -1,5 +1,5 @@ use { - super::{CompetitionEstimator, PriceRanking, compare_error}, + super::{CompetitionEstimator, QuoteContextMode, compare_error}, crate::{ Estimate, PriceEstimateResult, @@ -31,7 +31,9 @@ impl PriceEstimating for CompetitionEstimator> { OrderKind::Buy => query.sell_token, OrderKind::Sell => query.buy_token, }; - let get_context = self.ranking.provide_context(out_token, query.timeout); + let get_context = self + .quote_context_mode + .provide_context(out_token, query.timeout); // Filter out obviously wrong estimates: // - 0 gas cost would lead to us paying huge subsidies @@ -84,7 +86,7 @@ fn compare_quote_result( query: &Query, a: &PriceEstimateResult, b: &PriceEstimateResult, - context: &RankingContext, + context: &QuoteContext, prefer_verified_estimates: bool, ) -> Ordering { match (a, b) { @@ -102,27 +104,24 @@ fn compare_quote_result( } } -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); +fn compare_quote(query: &Query, a: &Estimate, b: &Estimate, context: &QuoteContext) -> Ordering { + let a = context.out_amount(a, query.kind); + let b = context.out_amount(b, query.kind); match query.kind { OrderKind::Buy => a.cmp(&b).reverse(), OrderKind::Sell => a.cmp(&b), } } -impl PriceRanking { +impl QuoteContextMode { async fn provide_context( &self, token: Address, timeout: Duration, - ) -> Result { + ) -> Result { match self { - PriceRanking::MaxOutAmount => Ok(RankingContext { - native_price: 1.0, - gas_price: 0., - }), - PriceRanking::BestBangForBuck { native, gas } => { + QuoteContextMode::None => Ok(QuoteContext::None), + QuoteContextMode::GasCost { native, gas } => { let gas = gas.clone(); let native = native.clone(); let gas = gas @@ -131,7 +130,7 @@ impl PriceRanking { let (native_price, gas_price) = futures::try_join!(native.estimate_native_price(token, timeout), gas)?; - Ok(RankingContext { + Ok(QuoteContext::GasCost { native_price, gas_price: gas_price as f64, }) @@ -140,35 +139,49 @@ impl PriceRanking { } } -struct RankingContext { - native_price: f64, - gas_price: f64, +enum QuoteContext { + /// Provides enough context to estimate how much of the quote's out_amount + /// would be lost due to paying for the gas cost. + /// 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 effective out_amount. + GasCost { + native_price: f64, + gas_price: f64, + }, + None, } -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 = f64::from(estimate.out_amount) * 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, - }; - match effective_eth_out { - // converts `NaN` and `(-∞, 0]` to `0` - v if v.is_sign_negative() || v.is_nan() => U256::ZERO, - // Previous case already covered negative infinity - v if v.is_infinite() => U256::MAX, - // Note on truncation: previously we used primitive_types::U256::from_f64_lossy which - // truncated the floating point, while alloy is slightly more faithful to the original - // value and rounds to closest integer: [0, 0.5) => 0, [0.5, 1] => 1 - v => U256::from(v.trunc()), +impl QuoteContext { + /// Computes the actual received value from this estimate given the + /// available context. + fn out_amount(&self, estimate: &Estimate, kind: OrderKind) -> U256 { + match self { + QuoteContext::None => estimate.out_amount, + QuoteContext::GasCost { + native_price, + gas_price, + } => { + let eth_out = f64::from(estimate.out_amount) * native_price; + let fees = estimate.gas as f64 * gas_price; + let effective_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, + }; + match effective_out { + // converts `NaN` and `(-∞, 0]` to `0` + v if v.is_sign_negative() || v.is_nan() => U256::ZERO, + // Previous case already covered negative infinity + v if v.is_infinite() => U256::MAX, + // Note on truncation: previously we used primitive_types::U256::from_f64_lossy + // which truncated the floating point, while alloy is + // slightly more faithful to the original value and rounds + // to closest integer: [0, 0.5) => 0, [0.5, 1] => 1 + v => U256::from(v.trunc()), + } + } } } } @@ -317,11 +330,11 @@ mod tests { Err(err) } - /// Builds a `BestBangForBuck` setup where every token is estimated + /// Builds a `GasCost` 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 { + fn gas_cost_mode() -> QuoteContextMode { // 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(); @@ -332,16 +345,16 @@ mod tests { max_fee_per_gas: 2, max_priority_fee_per_gas: 2, })); - PriceRanking::BestBangForBuck { + QuoteContextMode::GasCost { native: Arc::new(native), gas, } } - /// Returns the best estimate with respect to the provided ranking and order - /// kind. + /// Returns the best estimate with respect to the provided quote context + /// mode and order kind. async fn best_response( - ranking: PriceRanking, + quote_context_mode: QuoteContextMode, kind: OrderKind, estimates: Vec, verification: QuoteVerificationMode, @@ -363,7 +376,7 @@ mod tests { .map(|(i, e)| (format!("estimator_{i}"), estimator(e))) .collect(), ], - ranking.clone(), + quote_context_mode.clone(), ) .with_verification(verification); @@ -375,15 +388,15 @@ mod tests { .await } - /// Verifies that `PriceRanking::BestBangForBuck` correctly adjusts + /// Verifies that `QuoteContextMode::GasCost` 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() { + async fn gas_cost_adjusts_for_complexity() { let best = best_response( - bang_for_buck_ranking(), + gas_cost_mode(), OrderKind::Sell, vec![ // User effectively receives `100_000` `buy_token`. @@ -397,7 +410,7 @@ mod tests { assert_eq!(best, price(104_000, 1_000)); let best = best_response( - bang_for_buck_ranking(), + gas_cost_mode(), OrderKind::Buy, vec![ // User effectively pays `100_000` `sell_token`. @@ -418,7 +431,7 @@ mod tests { #[tokio::test] async fn discards_low_gas_cost_estimates() { let best = best_response( - bang_for_buck_ranking(), + gas_cost_mode(), OrderKind::Sell, vec![ // User effectively receives `100_000` `buy_token`. @@ -435,7 +448,7 @@ mod tests { assert_eq!(best, price(104_000, 1_000)); let best = best_response( - bang_for_buck_ranking(), + gas_cost_mode(), OrderKind::Buy, vec![ // User effectively pays `100_000` `sell_token`. @@ -458,7 +471,7 @@ mod tests { async fn returns_highest_priority_error() { // Returns errors with highest priority. let best = best_response( - PriceRanking::MaxOutAmount, + QuoteContextMode::None, OrderKind::Sell, vec![ error(PriceEstimationError::RateLimited), @@ -474,7 +487,7 @@ mod tests { #[tokio::test] async fn prefer_estimate_over_error() { let best = best_response( - PriceRanking::MaxOutAmount, + QuoteContextMode::None, OrderKind::Sell, vec![ price(1, 1_000_000), @@ -502,7 +515,7 @@ mod tests { }); let best = best_response( - PriceRanking::MaxOutAmount, + QuoteContextMode::None, OrderKind::Sell, vec![ better_unverified_quote.clone(), @@ -514,7 +527,7 @@ mod tests { assert_eq!(best, worse_verified_quote.clone()); let best = best_response( - PriceRanking::MaxOutAmount, + QuoteContextMode::None, OrderKind::Sell, vec![ better_unverified_quote.clone(), diff --git a/crates/price-estimation/src/factory.rs b/crates/price-estimation/src/factory.rs index 7f37cfd6b9..ca88ee3458 100644 --- a/crates/price-estimation/src/factory.rs +++ b/crates/price-estimation/src/factory.rs @@ -13,7 +13,7 @@ use { crate::{ ExternalSolver, buffered::{self, BufferedRequest, NativePriceBatchFetching}, - competition::PriceRanking, + competition::QuoteContextMode, config::{native_price::NativePriceConfig, price_estimation::BalanceOverridesConfigExt}, }, alloy::primitives::Address, @@ -358,11 +358,9 @@ impl<'a> PriceEstimatorFactory<'a> { gas: Arc, ) -> Result> { let estimators = self.get_estimators(solvers, |entry| &entry.optimal)?; - let competition_estimator = CompetitionEstimator::new( - vec![estimators], - PriceRanking::BestBangForBuck { native, gas }, - ) - .with_verification(self.args.quote_verification); + let competition_estimator = + CompetitionEstimator::new(vec![estimators], QuoteContextMode::GasCost { native, gas }) + .with_verification(self.args.quote_verification); Ok(Arc::new(self.sanitized(Arc::new(competition_estimator)))) } @@ -378,7 +376,7 @@ impl<'a> PriceEstimatorFactory<'a> { self.sanitized(Arc::new( CompetitionEstimator::new( vec![estimators], - PriceRanking::BestBangForBuck { native, gas }, + QuoteContextMode::GasCost { native, gas }, ) .with_early_return(fast_price_estimation_results_required), )), @@ -403,10 +401,9 @@ impl<'a> PriceEstimatorFactory<'a> { estimators.push(stages); } - let competition_estimator = - CompetitionEstimator::new(estimators, PriceRanking::MaxOutAmount) - .with_verification(self.args.quote_verification) - .with_early_return(results_required); + let competition_estimator = CompetitionEstimator::new(estimators, QuoteContextMode::None) + .with_verification(self.args.quote_verification) + .with_early_return(results_required); Ok(Box::new(competition_estimator)) }