Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Price competition bang for buck #2145

Merged
merged 18 commits into from
Dec 11, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions crates/autopilot/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,13 +413,6 @@ pub async fn run(args: Arguments) {
)
.expect("failed to initialize price estimator factory");

let price_estimator = price_estimator_factory
.price_estimator(&PriceEstimatorSource::for_args(
args.order_quoting.price_estimators.as_slice(),
&args.order_quoting.price_estimation_drivers,
&args.order_quoting.price_estimation_legacy_solvers,
))
.unwrap();
let native_price_estimator = price_estimator_factory
.native_price_estimator(
args.native_price_estimators.as_slice(),
Expand All @@ -431,6 +424,16 @@ pub async fn run(args: Arguments) {
args.native_price_estimation_results_required,
)
.unwrap();
let price_estimator = price_estimator_factory
.price_estimator(
&PriceEstimatorSource::for_args(
args.order_quoting.price_estimators.as_slice(),
&args.order_quoting.price_estimation_drivers,
&args.order_quoting.price_estimation_legacy_solvers,
),
native_price_estimator.clone(),
)
.unwrap();

let skip_event_sync_start = if args.skip_event_sync {
block_number_to_block_number_hash(&web3, BlockNumber::Latest).await
Expand Down
28 changes: 16 additions & 12 deletions crates/orderbook/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,15 +393,9 @@ pub async fn run(args: Arguments) {
)
.expect("failed to initialize price estimator factory");

let price_estimator = price_estimator_factory
.price_estimator(&PriceEstimatorSource::for_args(
args.order_quoting.price_estimators.as_slice(),
&args.order_quoting.price_estimation_drivers,
&args.order_quoting.price_estimation_legacy_solvers,
))
.unwrap();
let fast_price_estimator = price_estimator_factory
.fast_price_estimator(
let native_price_estimator = price_estimator_factory
.native_price_estimator(
args.native_price_estimators.as_slice(),
&PriceEstimatorSource::for_args(
args.order_quoting.price_estimators.as_slice(),
&args.order_quoting.price_estimation_drivers,
Expand All @@ -410,15 +404,25 @@ pub async fn run(args: Arguments) {
args.fast_price_estimation_results_required,
)
.unwrap();
let native_price_estimator = price_estimator_factory
.native_price_estimator(
args.native_price_estimators.as_slice(),
let price_estimator = price_estimator_factory
.price_estimator(
&PriceEstimatorSource::for_args(
args.order_quoting.price_estimators.as_slice(),
&args.order_quoting.price_estimation_drivers,
&args.order_quoting.price_estimation_legacy_solvers,
),
native_price_estimator.clone(),
)
.unwrap();
let fast_price_estimator = price_estimator_factory
.fast_price_estimator(
&PriceEstimatorSource::for_args(
args.order_quoting.price_estimators.as_slice(),
&args.order_quoting.price_estimation_drivers,
&args.order_quoting.price_estimation_legacy_solvers,
),
args.fast_price_estimation_results_required,
native_price_estimator.clone(),
)
.unwrap();

Expand Down
11 changes: 11 additions & 0 deletions crates/shared/src/price_estimation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,17 @@ impl Estimate {
let (sell_amount, buy_amount) = self.amounts(query);
buy_amount.to_f64_lossy() / sell_amount.to_f64_lossy()
}

/// 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 wourd report a higher `out_amount_in_eth`. This is also
/// referred to as "bang-for-buck" and what matters most to traders.
pub fn out_amount_in_eth(&self, native_out_token_price: f64) -> U256 {
let eth_value = self.out_amount.to_f64_lossy() * native_out_token_price;
let eth_value = U256::from_f64_lossy(eth_value);
eth_value.saturating_sub(self.gas.into())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this is the gas amount not the actual gas cost in ETH 🙈 (the new types in the driver/solver engine are really helpful to avoid this confusion).

let fee_parameters = FeeParameters {
gas_amount: trade_estimate.gas as _,
gas_price: gas_estimate.effective_gas_price(),
sell_token_price,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Will add a gas price estimator to get the actual gas used. 👍

}
}

pub type PriceEstimateResult = Result<Estimate, PriceEstimationError>;
Expand Down
126 changes: 84 additions & 42 deletions crates/shared/src/price_estimation/competition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,23 @@ type PriceEstimationStage<T> = Vec<(String, T)>;
/// 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<T> {
pub struct RacingCompetitionEstimator<T, N> {
inner: Vec<PriceEstimationStage<T>>,
successful_results_for_early_return: NonZeroUsize,
native: N,
}

impl<T: Send + Sync + 'static> RacingCompetitionEstimator<T> {
impl<T: Send + Sync + 'static, N: Send + Sync + 'static> RacingCompetitionEstimator<T, N> {
pub fn new(
inner: Vec<PriceEstimationStage<T>>,
successful_results_for_early_return: NonZeroUsize,
native: N,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can be option to not use a Noop estimator and make the 1:1 exchange rate for the native token estimates explicit?

) -> Self {
assert!(!inner.is_empty());
Self {
inner,
successful_results_for_early_return,
native,
}
}

Expand Down Expand Up @@ -182,24 +185,37 @@ fn successes<R, E>(results: &[(EstimatorIndex, Result<R, E>)]) -> usize {
results.iter().filter(|(_, result)| result.is_ok()).count()
}

impl PriceEstimating for RacingCompetitionEstimator<Arc<dyn PriceEstimating>> {
impl PriceEstimating
for RacingCompetitionEstimator<Arc<dyn PriceEstimating>, Arc<dyn NativePriceEstimating>>
{
fn estimate(&self, query: Arc<Query>) -> futures::future::BoxFuture<'_, PriceEstimateResult> {
self.estimate_generic(
query.clone(),
query.kind,
|estimator, query| estimator.estimate(query),
move |a, b| {
if is_second_quote_result_preferred(query.as_ref(), a, b) {
Ordering::Less
} else {
Ordering::Greater
}
},
)
async {
let out_token = match query.kind {
OrderKind::Buy => query.sell_token,
OrderKind::Sell => query.buy_token,
};
let native_price = self.native.estimate_native_price(out_token).await?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this await into the compare_results lambda and delay it as much as possible? This should increase our chances of not making estimates slower (the actual quote can run in parallel with the native estimate).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that. There are some notes, though:

  1. Currently we are passing in a NativePriceCache so the latency is only a concern for the first quote or whenever something got evicted from the cache so not that impactful
  2. This means the normal native price estimator will also call this code and we actually have to pass a Arc<dyn NativePriceEstimating> in the constructor whereas we can currently pass any type (I used ()) because we don't use it at all.

But I guess since you are in favor of making the NoopEstimator more explicit anyway you don't see 2 as a negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it a go and the inner future was written so generically that we can't get the token out of the Query. 😓
So either introduce yet another trait just so we can get that from the query or I need to find another way to not block on the native price fetching.
Will investigate further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to contain the damage (i.e. additional complexity) the best I could.
Instead of calling the native price estimator and gas estimator inside the generic function (which didn't work nicely) I opted for another argument in the generic function that takes a future that provides the needed context.
Now the supplied comparison function takes 2 results and this context.
The context is returned from a fused future (so we can await it multiple times) that awaits the result of a task that was spawned in the background so it does not delay the actual price estimation requests.

self.estimate_generic(
query.clone(),
query.kind,
|estimator, query| estimator.estimate(query),
move |a, b| {
if is_second_quote_result_preferred(query.as_ref(), a, b, native_price) {
Ordering::Less
} else {
Ordering::Greater
}
},
)
.await
}
.boxed()
}
}

impl NativePriceEstimating for RacingCompetitionEstimator<Arc<dyn NativePriceEstimating>> {
impl<N: Send + Sync + 'static> NativePriceEstimating
for RacingCompetitionEstimator<Arc<dyn NativePriceEstimating>, N>
{
fn estimate_native_price(
&self,
token: H160,
Expand All @@ -221,22 +237,24 @@ impl NativePriceEstimating for RacingCompetitionEstimator<Arc<dyn NativePriceEst

/// Price estimator that pulls estimates from various sources
/// and competes on the best price.
pub struct CompetitionEstimator<T> {
inner: RacingCompetitionEstimator<T>,
pub struct CompetitionEstimator<T, N> {
inner: RacingCompetitionEstimator<T, N>,
}

impl<T: Send + Sync + 'static> CompetitionEstimator<T> {
pub fn new(inner: Vec<Vec<(String, T)>>) -> Self {
impl<T: Send + Sync + 'static, N: Send + Sync + 'static> CompetitionEstimator<T, N> {
pub fn new(inner: Vec<Vec<(String, T)>>, native: N) -> 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),
inner: RacingCompetitionEstimator::new(inner, number_of_estimators, native),
}
}
}

impl PriceEstimating for CompetitionEstimator<Arc<dyn PriceEstimating>> {
impl PriceEstimating
for CompetitionEstimator<Arc<dyn PriceEstimating>, Arc<dyn NativePriceEstimating>>
{
fn estimate(&self, query: Arc<Query>) -> futures::future::BoxFuture<'_, PriceEstimateResult> {
self.inner.estimate(query)
}
Expand All @@ -246,9 +264,10 @@ fn is_second_quote_result_preferred(
query: &Query,
a: &PriceEstimateResult,
b: &PriceEstimateResult,
native_price: f64,
) -> bool {
match (a, b) {
(Ok(a), Ok(b)) => is_second_estimate_preferred(query, a, b),
(Ok(a), Ok(b)) => is_second_estimate_preferred(query, a, b, native_price),
(Ok(_), Err(_)) => false,
(Err(_), Ok(_)) => true,
(Err(a), Err(b)) => is_second_error_preferred(a, b),
Expand All @@ -267,10 +286,16 @@ fn is_second_native_result_preferred(
}
}

fn is_second_estimate_preferred(query: &Query, a: &Estimate, b: &Estimate) -> bool {
fn is_second_estimate_preferred(
query: &Query,
a: &Estimate,
b: &Estimate,
native_price: f64,
) -> bool {
let amount_out = |estimate: &Estimate| estimate.out_amount_in_eth(native_price);
match query.kind {
OrderKind::Buy => b.out_amount < a.out_amount,
OrderKind::Sell => a.out_amount < b.out_amount,
OrderKind::Buy => amount_out(b) < amount_out(a),
OrderKind::Sell => amount_out(a) < amount_out(b),
}
}

Expand Down Expand Up @@ -321,7 +346,7 @@ fn metrics() -> &'static Metrics {
mod tests {
use {
super::*,
crate::price_estimation::MockPriceEstimating,
crate::price_estimation::{native::MockNativePriceEstimating, MockPriceEstimating},
anyhow::anyhow,
futures::channel::oneshot::channel,
model::order::OrderKind,
Expand All @@ -331,6 +356,16 @@ mod tests {
tokio::time::sleep,
};

/// Builds native price estimator that returns the same native price for any
/// token.
fn mock_native_estimator() -> Arc<dyn NativePriceEstimating> {
let mut estimator = MockNativePriceEstimating::new();
estimator
.expect_estimate_native_price()
.returning(|_token| async { Ok(1.) }.boxed());
Arc::new(estimator)
}

#[tokio::test]
async fn works() {
let queries = [
Expand Down Expand Up @@ -418,11 +453,13 @@ mod tests {
}),
]);

let priority: CompetitionEstimator<Arc<dyn PriceEstimating>> =
CompetitionEstimator::new(vec![vec![
let priority: CompetitionEstimator<Arc<dyn PriceEstimating>, _> = CompetitionEstimator::new(
vec![vec![
("first".to_owned(), Arc::new(first)),
("second".to_owned(), Arc::new(second)),
]]);
]],
mock_native_estimator(),
);

let result = priority.estimate(queries[0].clone()).await;
assert_eq!(result.as_ref().unwrap(), &estimates[0]);
Expand Down Expand Up @@ -497,14 +534,15 @@ mod tests {
.boxed()
});

let racing: RacingCompetitionEstimator<Arc<dyn PriceEstimating>> =
let racing: RacingCompetitionEstimator<Arc<dyn PriceEstimating>, _> =
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(),
mock_native_estimator(),
);

let result = racing.estimate(query).await;
Expand Down Expand Up @@ -567,7 +605,7 @@ mod tests {
.boxed()
});

let racing: RacingCompetitionEstimator<Arc<dyn PriceEstimating>> =
let racing: RacingCompetitionEstimator<Arc<dyn PriceEstimating>, _> =
RacingCompetitionEstimator::new(
vec![
vec![
Expand All @@ -580,6 +618,7 @@ mod tests {
],
],
NonZeroUsize::new(2).unwrap(),
mock_native_estimator(),
);

let result = racing.estimate(query).await;
Expand Down Expand Up @@ -638,16 +677,19 @@ mod tests {
let mut fourth = MockPriceEstimating::new();
fourth.expect_estimate().never();

let racing: RacingCompetitionEstimator<Arc<dyn PriceEstimating>> =
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(),
};
let racing: RacingCompetitionEstimator<
Arc<dyn PriceEstimating>,
Arc<dyn NativePriceEstimating>,
> = 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(),
native: mock_native_estimator(),
};

racing.estimate(query).await.unwrap();
}
Expand Down
12 changes: 10 additions & 2 deletions crates/shared/src/price_estimation/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,22 +336,25 @@ impl<'a> PriceEstimatorFactory<'a> {
pub fn price_estimator(
&mut self,
sources: &[PriceEstimatorSource],
native: Arc<dyn NativePriceEstimating>,
) -> Result<Arc<dyn PriceEstimating>> {
let estimators = self.get_estimators(sources, |entry| &entry.optimal)?;
let competition_estimator = CompetitionEstimator::new(vec![estimators]);
let competition_estimator = CompetitionEstimator::new(vec![estimators], native);
Ok(Arc::new(self.sanitized(Arc::new(competition_estimator))))
}

pub fn fast_price_estimator(
&mut self,
sources: &[PriceEstimatorSource],
fast_price_estimation_results_required: NonZeroUsize,
native: Arc<dyn NativePriceEstimating>,
) -> Result<Arc<dyn PriceEstimating>> {
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,
native,
),
))))
}
Expand All @@ -377,7 +380,12 @@ impl<'a> PriceEstimatorFactory<'a> {
})
.collect::<Result<Vec<Vec<_>>>>()?;

let competition_estimator = RacingCompetitionEstimator::new(estimators, results_required);
// Hack: we made the `RacingCompetitionEstimator` generic to reuse it for
// regular quotes and native quotes but only the regular quotes actually
// need the native price estimator component (otherwise we'd have a
// circular dependency). That's why we use the `NoopEstimator` here.
let competition_estimator =
RacingCompetitionEstimator::new(estimators, results_required, ());
let native_estimator = Arc::new(CachingNativePriceEstimator::new(
Box::new(competition_estimator),
self.args.native_price_cache_max_age_secs,
Expand Down
Loading