Price competition bang for buck #2145
Conversation
There was a problem hiding this comment.
I think this approach works as well, unfortunately we also need to pass information about the current gas price into the component as well in order to make things work.
If possible, I'd like to keep the concurrent logic of fetching quotes, native estimates and the gas estimate in parallel (which is why I though returning all responses and then picking the best given all three values may be easier to implement).
| OrderKind::Buy => query.sell_token, | ||
| OrderKind::Sell => query.buy_token, | ||
| }; | ||
| let native_price = self.native.estimate_native_price(out_token).await?; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
We can do that. There are some notes, though:
- Currently we are passing in a
NativePriceCacheso the latency is only a concern for the first quote or whenever something got evicted from the cache so not that impactful - 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| pub fn new( | ||
| inner: Vec<PriceEstimationStage<T>>, | ||
| successful_results_for_early_return: NonZeroUsize, | ||
| native: N, |
There was a problem hiding this comment.
nit: can be option to not use a Noop estimator and make the 1:1 exchange rate for the native token estimates explicit?
| 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()) |
There was a problem hiding this comment.
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).
services/crates/shared/src/order_quoting.rs
Lines 577 to 581 in 01e1c51
There was a problem hiding this comment.
Makes sense. Will add a gas price estimator to get the actual gas used. 👍
| let competition_estimator = RacingCompetitionEstimator::new( | ||
| estimators, | ||
| results_required, | ||
| PriceRanking::MaxOutAmount, |
There was a problem hiding this comment.
This argument is still kind of fake. We could pass BestBangForBuck here and it would change nothing because the native price wrapper around the generic estimation function does not use this variable.
But at least it's less implicit what is happening here.
Not sure if we can get rid of this wart without untangling the generic price estimator implementation again.
There was a problem hiding this comment.
Looks alright to me, but I agree this is very confusing (and I doubt we will kill this component altogether when we move to verified quotes, so we might be accruing some real debt here).
Would it make things simpler if compare_results and provide_additional_context would be combined in a single Comparator trait which allows to pick the winner async from an Iterator<Result<R,E>>?
Then NativePriceEstimating can just use a simple implementation for the comparator and RacingCompetitionEstimator can implement the async context aware logic?
| >( | ||
| &self, | ||
| query: Q, | ||
| kind: OrderKind, | ||
| get_single_result: impl Fn(&T, Q) -> futures::future::BoxFuture<'_, Result<R, E>> | ||
| + Send | ||
| + 'static, | ||
| compare_results: impl Fn(&Result<R, E>, &Result<R, E>) -> Ordering + Send + 'static, | ||
| compare_results: impl Fn(&Result<R, E>, &Result<R, E>, &C) -> Ordering + Send + 'static, | ||
| provide_additional_context: impl FusedFuture<Output = Result<C, E>> + Send + 'static, |
There was a problem hiding this comment.
Does this have to be so generic? Don't we always return a concrete RankingContext ?
There was a problem hiding this comment.
We don't have any context in the generic price estimation case because it doesn't appear to make sense to give the user the option between MaxOutAmount and BestBangForBuck because for BestBangForBuck we need to have a native price which is what we are trying to compute in that call in the first case.
So unless I'm missing something it's not really possible to decide in that case whether it makes sense to use a less complex trade with lower out_amount because we don't really know yet how the token relates to native ETH.
OTOH we could force C to always be a RankingContext (by providing the noop context) but at this point I'm not sure how much that would really improves things. :/
Description
We started to allow external solvers to provide quotes and incentivized them by giving them rewards for every trade where they reported the best quote.
At the moment we rank qoutes by the highest reported
out_amountwithout taking gas costs of complex routes into account. Because solvers are now competing on the highestout_amountthey started to propose more and more complex routes which would end up hurting the user because complex routes require a lot of gas to execute which would end up being deducted from the user in form of fees.In summary the current situation leads to the UI showing very high fees which leads to fewer trades being placed.
Changes
Instead of most
out_amountwe now rank bybang-for-buck(i.e.out_amount - gas_fees). To do that our price estimators now also need to be aware of the native price for theout_token. That way we can rank estimates by the totaleth_received.In order to keep the interface unchanged I decided to make the regular price estimator use a native price estimator under the hood.
Because the
CompetitionPriceEstimatorwas recently refactored to be usable for regular and native price estimates the native price estimator now also has the fieldnative. This is obviously not great but at least you don't have to pass an actual native price estimator and it also ends up not being used.How to test
Not sure how doable it is to set up an
e2etest for this but I should at least add a unit test using mocks that verifies that we now pick best "bang-for-buck".