Skip to content

Commit

Permalink
Make legacy http estimators compatible with quote verification (#2388)
Browse files Browse the repository at this point in the history
# Description
So far quote verification was heavily geared towards price estimation
via drivers. But if we want to try out verification in prod to get a
good idea on how well the feature works we should also add support for
the legacy http price estimators.

# Changes
Applies existing pattern of putting a `TradeFinder` into a
`TraderEstimator`. This design seems overall pretty convoluted to me but
this is just the last stretch before we can finally move towards quotes
via drivers so it doesn't seem reasonable to introduce yet another
adapter pattern to make the price estimation hierarchy work somehow.

The only estimators remaining that are incompatible with quote
verification is the baseline and balancer solver. I think it's
reasonable to not implement the same thing for those and instead use the
data from all the other estimators, polish quote verification,
transition completely to driver based price estimation and drop all the
legacy price estimation code.

## How to test
CI
  • Loading branch information
MartinquaXD authored Feb 10, 2024
1 parent c4f6e51 commit c0fb701
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 41 deletions.
58 changes: 33 additions & 25 deletions crates/shared/src/price_estimation/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use {
baseline::BaselinePriceEstimator,
competition::CompetitionEstimator,
external::ExternalPriceEstimator,
http::HttpPriceEstimator,
http::{HttpPriceEstimator, HttpTradeFinder},
instrumented::InstrumentedPriceEstimator,
native::{self, NativePriceEstimator},
native_price_cache::CachingNativePriceEstimator,
Expand Down Expand Up @@ -578,33 +578,41 @@ impl PriceEstimatorCreating for HttpPriceEstimator {

fn init(factory: &PriceEstimatorFactory, name: &str, params: Self::Params) -> Result<Self> {
Ok(HttpPriceEstimator::new(
Arc::new(DefaultHttpSolverApi {
name: name.to_string(),
network_name: factory.network.name.clone(),
chain_id: factory.network.chain_id,
base: params.base,
solve_path: params.solve_path,
client: factory.components.http_factory.create(),
gzip_requests: false,
config: SolverConfig {
use_internal_buffers: Some(factory.shared_args.use_internal_buffers),
objective: Some(Objective::SurplusFeesCosts),
..Default::default()
},
}),
factory.components.uniswap_v2_pools.clone(),
factory.components.balancer_pools.clone(),
factory.components.uniswap_v3_pools.clone(),
factory.components.tokens.clone(),
factory.components.gas_price.clone(),
factory.network.native_token,
factory.network.base_tokens.clone(),
factory.network.name.clone(),
name.to_string(),
HttpTradeFinder::new(
Arc::new(DefaultHttpSolverApi {
name: name.to_string(),
network_name: factory.network.name.clone(),
chain_id: factory.network.chain_id,
base: params.base,
solve_path: params.solve_path,
client: factory.components.http_factory.create(),
gzip_requests: false,
config: SolverConfig {
use_internal_buffers: Some(factory.shared_args.use_internal_buffers),
objective: Some(Objective::SurplusFeesCosts),
..Default::default()
},
}),
factory.components.uniswap_v2_pools.clone(),
factory.components.balancer_pools.clone(),
factory.components.uniswap_v3_pools.clone(),
factory.components.tokens.clone(),
factory.components.gas_price.clone(),
factory.network.native_token,
factory.network.base_tokens.clone(),
factory.network.name.clone(),
factory.rate_limiter(name),
params.use_liquidity,
params.solver,
),
factory.rate_limiter(name),
params.use_liquidity,
params.solver,
))
}

fn verified(&self, verifier: &Arc<dyn TradeVerifying>) -> Option<Self> {
Some(self.verified(verifier.clone()))
}
}

impl From<&LegacySolver> for HttpPriceEstimatorParams {
Expand Down
95 changes: 79 additions & 16 deletions crates/shared/src/price_estimation/http.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use {
super::{trade_finder::TradeEstimator, trade_verifier::TradeVerifying},
crate::{
baseline_solver::BaseTokens,
http_solver::{
Expand Down Expand Up @@ -38,6 +39,7 @@ use {
uniswap_v3::pool_fetching::PoolFetching as UniswapV3PoolFetching,
},
token_info::TokenInfoFetching,
trade_finding::{Interaction, Quote, Trade, TradeError, TradeFinding},
},
anyhow::{anyhow, Context, Result},
ethcontract::{H160, U256},
Expand All @@ -53,7 +55,33 @@ use {
},
};

pub struct HttpPriceEstimator {
pub struct HttpPriceEstimator(TradeEstimator);

impl HttpPriceEstimator {
pub fn new(
label: String,
trade_finder: HttpTradeFinder,
rate_limiter: Arc<RateLimiter>,
) -> Self {
Self(TradeEstimator::new(
Arc::new(trade_finder),
rate_limiter,
label,
))
}

pub fn verified(&self, verifier: Arc<dyn TradeVerifying>) -> Self {
Self(self.0.clone().with_verifier(verifier))
}
}

impl PriceEstimating for HttpPriceEstimator {
fn estimate(&self, query: Arc<Query>) -> futures::future::BoxFuture<'_, PriceEstimateResult> {
self.0.estimate(query)
}
}

pub struct HttpTradeFinder {
api: Arc<dyn HttpSolverApi>,
sharing: RequestSharing<
Arc<Query>,
Expand All @@ -72,7 +100,7 @@ pub struct HttpPriceEstimator {
solver: H160,
}

impl HttpPriceEstimator {
impl HttpTradeFinder {
#[allow(clippy::too_many_arguments)]
pub fn new(
api: Arc<dyn HttpSolverApi>,
Expand Down Expand Up @@ -105,7 +133,7 @@ impl HttpPriceEstimator {
}
}

async fn estimate(&self, query: Arc<Query>) -> Result<Estimate, PriceEstimationError> {
async fn compute_trade(&self, query: Arc<Query>) -> Result<Trade, PriceEstimationError> {
let gas_price = U256::from_f64_lossy(
self.gas_info
.estimate()
Expand Down Expand Up @@ -245,21 +273,29 @@ impl HttpPriceEstimator {
for amm in settlement.amms.values() {
cost += self.extract_cost(&amm.cost)? * amm.execution.len();
}
for interaction in settlement.interaction_data {
for interaction in &settlement.interaction_data {
cost += self.extract_cost(&interaction.cost)?;
}
let gas = (cost / gas_price).as_u64().max(TRADE)
let gas_estimate = (cost / gas_price).as_u64().max(TRADE)
+ INITIALIZATION_COST // Call into contract
+ SETTLEMENT // overhead for entering the `settle()` function
+ ERC20_TRANSFER * 2; // transfer in and transfer out
Ok(Estimate {
Ok(Trade {
out_amount: match query.kind {
OrderKind::Buy => settlement.orders[&0].exec_sell_amount,
OrderKind::Sell => settlement.orders[&0].exec_buy_amount,
},
gas,
gas_estimate,
interactions: settlement
.interaction_data
.into_iter()
.map(|i| Interaction {
target: i.target,
value: i.value,
data: i.call_data,
})
.collect(),
solver: self.solver,
verified: false,
})
}

Expand Down Expand Up @@ -406,9 +442,36 @@ impl HttpPriceEstimator {
}
}

impl PriceEstimating for HttpPriceEstimator {
fn estimate(&self, query: Arc<Query>) -> BoxFuture<'_, PriceEstimateResult> {
self.estimate(query).boxed()
#[async_trait::async_trait]
impl TradeFinding for HttpTradeFinder {
async fn get_quote(&self, query: &Query) -> Result<Quote, TradeError> {
let price_estimate = self.compute_trade(Arc::new(query.clone())).await?;
Ok(Quote {
out_amount: price_estimate.out_amount,
gas_estimate: price_estimate.gas_estimate,
solver: price_estimate.solver,
})
}

async fn get_trade(&self, query: &Query) -> Result<Trade, TradeError> {
self.compute_trade(Arc::new(query.clone()))
.await
.map_err(Into::into)
}
}

impl PriceEstimating for HttpTradeFinder {
fn estimate(&self, query: Arc<Query>) -> futures::future::BoxFuture<'_, PriceEstimateResult> {
async {
let trade = self.compute_trade(query).await?;
Ok(Estimate {
out_amount: trade.out_amount,
gas: trade.gas_estimate,
solver: trade.solver,
verified: false,
})
}
.boxed()
}
}

Expand Down Expand Up @@ -482,7 +545,7 @@ mod tests {

let gas_price_estimating = Arc::new(FakeGasPriceEstimator::new(GasPrice1559::default()));

let estimator = HttpPriceEstimator::new(
let estimator = HttpTradeFinder::new(
Arc::new(api),
Arc::new(FakePoolFetcher(vec![])),
None,
Expand Down Expand Up @@ -538,7 +601,7 @@ mod tests {

let gas_price_estimating = Arc::new(FakeGasPriceEstimator::new(GasPrice1559::default()));

let estimator = HttpPriceEstimator::new(
let estimator = HttpTradeFinder::new(
Arc::new(api),
Arc::new(FakePoolFetcher(vec![])),
None,
Expand Down Expand Up @@ -584,7 +647,7 @@ mod tests {

let gas_price_estimating = Arc::new(FakeGasPriceEstimator::new(GasPrice1559::default()));

let estimator = HttpPriceEstimator::new(
let estimator = HttpTradeFinder::new(
Arc::new(api),
Arc::new(FakePoolFetcher(vec![])),
None,
Expand Down Expand Up @@ -681,7 +744,7 @@ mod tests {
..Default::default()
}));

let estimator = HttpPriceEstimator::new(
let estimator = HttpTradeFinder::new(
Arc::new(api),
Arc::new(FakePoolFetcher(vec![])),
None,
Expand Down Expand Up @@ -797,7 +860,7 @@ mod tests {
);
let gas_info = Arc::new(web3);

let estimator = HttpPriceEstimator {
let estimator = HttpTradeFinder {
api: Arc::new(DefaultHttpSolverApi {
name: "test".to_string(),
network_name: "1".to_string(),
Expand Down

0 comments on commit c0fb701

Please sign in to comment.