Settle fast-path orders from the autopilot - #4729
Conversation
b6c9e30 to
9ceb810
Compare
9ceb810 to
a2eb919
Compare
d318fbe to
26528a1
Compare
|
Claude finished @AryanGodara's task in 4m 29s —— View job PR Review: Settle fast-path orders from the autopilotReviewed against One substantive finding, posted inline:
Nothing else blocking from my side. The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// Native prices for the order's tokens, from the estimator's cache only. | ||
| /// Missing prices are omitted; the driver tolerates a partial map. | ||
| async fn native_prices(&self, tokens: &[Address]) -> HashMap<Address, U256> { | ||
| let mut prices = HashMap::new(); | ||
| for &token in tokens { | ||
| match self | ||
| .native_price_estimator | ||
| .estimate_native_price(token, Duration::ZERO) | ||
| .await | ||
| { | ||
| Ok(price) => { | ||
| if let Some(price) = to_normalized_price(price) { | ||
| prices.insert(token, price); | ||
| } | ||
| } | ||
| Err(err) => tracing::warn!(?token, ?err, "no native price for fast-path token"), | ||
| } | ||
| } | ||
| prices | ||
| } |
There was a problem hiding this comment.
The doc comment says "from the estimator's cache only", but estimate_native_price(token, Duration::ZERO) does not behave as cache-only on a miss.
Looking at CachingNativePriceEstimator::estimate_native_price (crates/price-estimation/src/native_price_cache.rs:417): on a cache hit it returns immediately, but on a miss it falls through to estimate_prices_and_update_cache([token], .., timeout=0), which calls the underlying estimator with a 0 timeout — and, unlike fetch_prices (same file, line 389: if timeout.is_zero() { return prices; }), there is no time::timeout wrapper around it. So a cache miss here triggers a live fetch that can block for the full duration of a real HTTP request.
That's exactly the situation the reviewer wanted to avoid ("the fast path does not leave enough time to fetch them from scratch"). Because this runs in the spawned task it won't stall the notifier, but it can silently blow the fast-path submission window on any uncached token, and with one tokio::spawn per arriving order there's no bound on how many such fetches pile up under load.
fetch_prices(tokens, Duration::ZERO) is the genuinely cache-only path, but it's an inherent method on CachingNativePriceEstimator, not on the NativePriceEstimating trait — so either hold the concrete type here, or add a cache-only accessor to the trait. At minimum, please correct the comment so it doesn't claim cache-only behavior it doesn't have.
2 participants
Description
When a fast-path order arrives, the autopilot recovers its quote's winning solution and settles it out of competition via the driver's
/settle.Solves BE-62.
Stacked on #4737.
Changes
auction_id, then recover the winning solution + fill fromproposed_solutions/proposed_trade_executionsfastPathpayload to the autopilot's/settleDTOHow to test
No automated tests yet: the competition data this reads isn't persisted until BE-56, so the listener is no-op on real orders for now. I verified locally by seeding a competition in local DB (with all migrations) and asserting
fast_path_orderreturns it. The end-to-end test should land once BE-56 and orderbook fast-path support are in.For those reasons keeping this PR in draft for now