Skip to content

Commit

Permalink
Simplify price estimation api (#1844)
Browse files Browse the repository at this point in the history 
Our price estimation API is annoying to work with and extend for 2
reasons:
1. it returns a stream (this was done for historic reasons and has been
worked around more elegantly by now)
2. it supports fetching prices for multiple trades (only a single
estimator could theoretically support that but it's really not worth it
to keep it around for that one IMO)

This PR also adds 2 changes:
1. removed some `debug_asserts`. These are unnecessary because we have
the `SanitizedPriceEstimator`
2. remove rate limiting from the baseline estimator (it's very fast and
does not need that).

These 2 facts can make it very annoying to add new features and very
hard to understand the code (IMO working with streams is always
extremely annoying).

### Test Plan

Unit tests still need to be updated to use the simpler API (hence the
draft status)
  • Loading branch information
@MartinquaXD
MartinquaXD authored Sep 15, 2023
1 parent 58308af commit 6832df5
Show file tree
Hide file tree
Showing 18 changed files with 839 additions and 1,352 deletions.
29 changes: 13 additions & 16 deletions crates/autopilot/src/solvable_orders.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ impl OrderFilterCounter {
mod tests {
use {
super::*,
futures::{FutureExt, StreamExt},
futures::FutureExt,
maplit::{btreemap, hashset},
mockall::predicate::eq,
model::{
Expand Down Expand Up @@ -788,27 +788,24 @@ mod tests {

let mut native_price_estimator = MockNativePriceEstimating::new();
native_price_estimator
.expect_estimate_native_prices()
.withf(move |tokens| *tokens == [token1])
.returning(|_| futures::stream::iter([(0, Ok(2.))].into_iter()).boxed());
.expect_estimate_native_price()
.withf(move |token| *token == token1)
.returning(|_| async { Ok(2.) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.times(1)
.withf(move |tokens| *tokens == [token2])
.returning(|_| {
futures::stream::iter([(0, Err(PriceEstimationError::NoLiquidity))].into_iter())
.boxed()
});
.withf(move |token| *token == token2)
.returning(|_| async { Err(PriceEstimationError::NoLiquidity) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.times(1)
.withf(move |tokens| *tokens == [token3])
.returning(|_| futures::stream::iter([(0, Ok(0.25))].into_iter()).boxed());
.withf(move |token| *token == token3)
.returning(|_| async { Ok(0.25) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.times(1)
.withf(move |tokens| *tokens == [token4])
.returning(|_| futures::stream::iter([(0, Ok(0.))].into_iter()).boxed());
.withf(move |token| *token == token4)
.returning(|_| async { Ok(0.) }.boxed());

let native_price_estimator = CachingNativePriceEstimator::new(
Box::new(native_price_estimator),
Expand Down
8 changes: 1 addition & 7 deletions crates/orderbook/src/api/get_native_price.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use {
anyhow::Result,
ethcontract::H160,
futures::StreamExt,
serde::Serialize,
shared::{
api::{ApiReply, IntoWarpReply},
Expand Down Expand Up @@ -32,12 +31,7 @@ pub fn get_native_price(
get_native_prices_request().and_then(move |token: H160| {
let estimator = estimator.clone();
async move {
let result = estimator
.estimate_native_prices(std::slice::from_ref(&token))
.next()
.await
.unwrap()
.1;
let result = estimator.estimate_native_price(&token).await;
let reply = match result {
Ok(price) => with_status(
warp::reply::json(&PriceResponse::from(price)),
Expand Down
166 changes: 84 additions & 82 deletions crates/shared/src/order_quoting.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use {
super::price_estimation::{
self,
native::{native_single_estimate, NativePriceEstimating},
single_estimate,
native::NativePriceEstimating,
PriceEstimating,
PriceEstimationError,
},
Expand Down Expand Up @@ -540,17 +539,19 @@ impl OrderQuoter {
_ => self.now.now() + chrono::Duration::seconds(STANDARD_QUOTE_VALIDITY_SECONDS),
};

let trade_query = parameters.to_price_query();
let trade_query = Arc::new(parameters.to_price_query());
let (gas_estimate, trade_estimate, sell_token_price, _) = futures::try_join!(
self.gas_estimator
.estimate()
.map_err(PriceEstimationError::ProtocolInternal),
single_estimate(self.price_estimator.as_ref(), &trade_query),
native_single_estimate(self.native_price_estimator.as_ref(), &parameters.sell_token),
self.price_estimator.estimate(trade_query.clone()),
self.native_price_estimator
.estimate_native_price(&parameters.sell_token),
// We don't care about the native price of the buy_token for the quote but we need it
// when we build the auction. To prevent creating orders which we can't settle later on
// we make the native buy_token price a requirement here as well.
native_single_estimate(self.native_price_estimator.as_ref(), &parameters.buy_token),
self.native_price_estimator
.estimate_native_price(&parameters.buy_token),
)?;

let (quoted_sell_amount, quoted_buy_amount) = match &parameters.side {
Expand Down Expand Up @@ -749,7 +750,7 @@ mod tests {
},
chrono::Utc,
ethcontract::H160,
futures::StreamExt as _,
futures::FutureExt,
gas_estimation::GasPrice1559,
mockall::{predicate::eq, Sequence},
model::{quote::Validity, time},
Expand Down Expand Up @@ -808,9 +809,9 @@ mod tests {

let mut price_estimator = MockPriceEstimating::new();
price_estimator
.expect_estimates()
.expect_estimate()
.withf(|q| {
q == [price_estimation::Query {
**q == price_estimation::Query {
verification: Some(Verification {
from: H160([3; 20]),
..Default::default()
Expand All @@ -819,33 +820,34 @@ mod tests {
buy_token: H160([2; 20]),
in_amount: NonZeroU256::try_from(100).unwrap(),
kind: OrderKind::Sell,
}]
}
})
.returning(|_| {
futures::stream::iter([Ok(price_estimation::Estimate {
out_amount: 42.into(),
gas: 3,
solver: H160([1; 20]),
})])
.enumerate()
async {
Ok(price_estimation::Estimate {
out_amount: 42.into(),
gas: 3,
solver: H160([1; 20]),
})
}
.boxed()
});

let mut native_price_estimator = MockNativePriceEstimating::new();
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let sell_token = parameters.sell_token;
move |q| q == [sell_token]
move |q| q == &sell_token
})
.returning(|_| futures::stream::iter([Ok(0.2)]).enumerate().boxed());
.returning(|_| async { Ok(0.2) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let buy_token = parameters.buy_token;
move |q| q == [buy_token]
move |q| q == &buy_token
})
.returning(|_| futures::stream::iter([Ok(0.2)]).enumerate().boxed());
.returning(|_| async { Ok(0.2) }.boxed());

let gas_estimator = FakeGasPriceEstimator(Arc::new(Mutex::new(gas_price)));

Expand Down Expand Up @@ -939,9 +941,9 @@ mod tests {

let mut price_estimator = MockPriceEstimating::new();
price_estimator
.expect_estimates()
.expect_estimate()
.withf(|q| {
q == [price_estimation::Query {
**q == price_estimation::Query {
verification: Some(Verification {
from: H160([3; 20]),
..Default::default()
Expand All @@ -950,33 +952,34 @@ mod tests {
buy_token: H160([2; 20]),
in_amount: NonZeroU256::try_from(100).unwrap(),
kind: OrderKind::Sell,
}]
}
})
.returning(|_| {
futures::stream::iter([Ok(price_estimation::Estimate {
out_amount: 42.into(),
gas: 3,
solver: H160([1; 20]),
})])
.enumerate()
async {
Ok(price_estimation::Estimate {
out_amount: 42.into(),
gas: 3,
solver: H160([1; 20]),
})
}
.boxed()
});

let mut native_price_estimator = MockNativePriceEstimating::new();
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let sell_token = parameters.sell_token;
move |q| q == [sell_token]
move |q| q == &sell_token
})
.returning(|_| futures::stream::iter([Ok(0.2)]).enumerate().boxed());
.returning(|_| async { Ok(0.2) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let buy_token = parameters.buy_token;
move |q| q == [buy_token]
move |q| q == &buy_token
})
.returning(|_| futures::stream::iter([Ok(0.2)]).enumerate().boxed());
.returning(|_| async { Ok(0.2) }.boxed());

let gas_estimator = FakeGasPriceEstimator(Arc::new(Mutex::new(gas_price)));

Expand Down Expand Up @@ -1068,9 +1071,9 @@ mod tests {

let mut price_estimator = MockPriceEstimating::new();
price_estimator
.expect_estimates()
.expect_estimate()
.withf(|q| {
q == [price_estimation::Query {
**q == price_estimation::Query {
verification: Some(Verification {
from: H160([3; 20]),
..Default::default()
Expand All @@ -1079,33 +1082,34 @@ mod tests {
buy_token: H160([2; 20]),
in_amount: NonZeroU256::try_from(42).unwrap(),
kind: OrderKind::Buy,
}]
}
})
.returning(|_| {
futures::stream::iter([Ok(price_estimation::Estimate {
out_amount: 100.into(),
gas: 3,
solver: H160([1; 20]),
})])
.enumerate()
async {
Ok(price_estimation::Estimate {
out_amount: 100.into(),
gas: 3,
solver: H160([1; 20]),
})
}
.boxed()
});

let mut native_price_estimator = MockNativePriceEstimating::new();
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let sell_token = parameters.sell_token;
move |q| q == [sell_token]
move |q| q == &sell_token
})
.returning(|_| futures::stream::iter([Ok(0.2)]).enumerate().boxed());
.returning(|_| async { Ok(0.2) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let buy_token = parameters.buy_token;
move |q| q == [buy_token]
move |q| q == &buy_token
})
.returning(|_| futures::stream::iter([Ok(0.2)]).enumerate().boxed());
.returning(|_| async { Ok(0.2) }.boxed());

let gas_estimator = FakeGasPriceEstimator(Arc::new(Mutex::new(gas_price)));

Expand Down Expand Up @@ -1198,31 +1202,32 @@ mod tests {
};

let mut price_estimator = MockPriceEstimating::new();
price_estimator.expect_estimates().returning(|_| {
futures::stream::iter([Ok(price_estimation::Estimate {
out_amount: 100.into(),
gas: 200,
solver: H160([1; 20]),
})])
.enumerate()
price_estimator.expect_estimate().returning(|_| {
async {
Ok(price_estimation::Estimate {
out_amount: 100.into(),
gas: 200,
solver: H160([1; 20]),
})
}
.boxed()
});

let mut native_price_estimator = MockNativePriceEstimating::new();
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let sell_token = parameters.sell_token;
move |q| q == [sell_token]
move |q| q == &sell_token
})
.returning(|_| futures::stream::iter([Ok(1.)]).enumerate().boxed());
.returning(|_| async { Ok(1.) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let buy_token = parameters.buy_token;
move |q| q == [buy_token]
move |q| q == &buy_token
})
.returning(|_| futures::stream::iter([Ok(1.)]).enumerate().boxed());
.returning(|_| async { Ok(1.) }.boxed());

let gas_estimator = FakeGasPriceEstimator(Arc::new(Mutex::new(gas_price)));

Expand Down Expand Up @@ -1267,35 +1272,32 @@ mod tests {
};

let mut price_estimator = MockPriceEstimating::new();
price_estimator.expect_estimates().returning(|_| {
futures::stream::iter([Ok(price_estimation::Estimate {
out_amount: 100.into(),
gas: 200,
solver: H160([1; 20]),
})])
.enumerate()
price_estimator.expect_estimate().returning(|_| {
async {
Ok(price_estimation::Estimate {
out_amount: 100.into(),
gas: 200,
solver: H160([1; 20]),
})
}
.boxed()
});

let mut native_price_estimator = MockNativePriceEstimating::new();
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let sell_token = parameters.sell_token;
move |q| q == [sell_token]
move |q| q == &sell_token
})
.returning(|_| futures::stream::iter([Ok(1.)]).enumerate().boxed());
.returning(|_| async { Ok(1.) }.boxed());
native_price_estimator
.expect_estimate_native_prices()
.expect_estimate_native_price()
.withf({
let buy_token = parameters.buy_token;
move |q| q == [buy_token]
move |q| q == &buy_token
})
.returning(|_| {
futures::stream::iter([Err(PriceEstimationError::NoLiquidity)])
.enumerate()
.boxed()
});
.returning(|_| async { Err(PriceEstimationError::NoLiquidity) }.boxed());

let gas_estimator = FakeGasPriceEstimator(Arc::new(Mutex::new(gas_price)));

Expand Down
Loading

0 comments on commit 6832df5

Please sign in to comment.