Separate autopilot API native price estimator #4044
Conversation
| /// Creates a new CachingNativePriceEstimator with a background maintenance | ||
| /// task. | ||
| /// | ||
| /// The maintenance task periodically refreshes cached prices before they | ||
| /// expire. Use this for the primary estimator in a shared-cache setup. |
There was a problem hiding this comment.
Shouldn't cache maintenance be coupled with the cache itself?
In my head it makes more sense to create a single cache, comes with maintenance by default and when you want more structs to have access to it, hand out references (which you do with the clone on the struct with the Arc)
I'm a bit confused by the separation
There was a problem hiding this comment.
I don't clearly see your idea. Which estimators will the cache maintenance task will be using then?
| /// Main estimator, which runs the background cache maintenance task. | ||
| pub main: Arc<CachingNativePriceEstimator>, | ||
| /// API estimator that shares the cache with the main estimator but doesn't | ||
| /// run background task and might use a different set of sources. |
There was a problem hiding this comment.
How come it can use a different set of sources but not run the background task to update entries according to them?
What am I missing?
There was a problem hiding this comment.
The idea of the API estimators is that a subset of the main estimators. But after thinking more about it, we can probably run a maintenance task with estimators combined from api and main estimators to ensure everything is covered.
There was a problem hiding this comment.
I reworked the approach. It would require starting the review from scratch. Sorry about that.
| /// The main estimator includes all sources and runs the background | ||
| /// maintenance task. The API estimator (if `api_native` is provided) | ||
| /// uses its own set of sources but shares the cache, so it can return | ||
| /// cached prices without triggering requests to excluded sources. |
There was a problem hiding this comment.
The different set of sources is either poorly documented and needs a better explanation or it feels like a footgun / something that may cause a lot of confusion during investigations
There was a problem hiding this comment.
What exactly confuses you? Different sets of estimators are used for the orderbook and auction competition.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural improvement by separating the native price estimator for the Autopilot API and introducing a shared cache with source-aware maintenance. This refactoring effectively reduces memory usage and prevents redundant price fetches. The new NativePriceCache encapsulates caching logic cleanly, simplifying the CachingNativePriceEstimator and improving the overall design. The changes are well-structured and align with the goals outlined in the description. I've found one potential issue that could lead to a panic, which I've detailed in a specific comment.
| // will fetch the price during the next maintenance cycle. | ||
| // This should happen only for prices missing while building the auction. | ||
| // Otherwise malicious actors could easily cause the cache size to blow up. | ||
| let outdated_timestamp = now.checked_sub(self.inner.max_age).unwrap(); |
There was a problem hiding this comment.
The use of .unwrap() on checked_sub could cause a panic if self.inner.max_age is larger than the system's uptime. This might occur if max_age is configured to a large value (e.g., hours or days) and the service has recently restarted. To prevent this, it's crucial to handle the None case from checked_sub gracefully instead of unwrapping it directly.
let outdated_timestamp = match now.checked_sub(self.inner.max_age) {
Some(timestamp) => timestamp,
None => {
tracing::warn!(
max_age = ?self.inner.max_age,
?token,
"max_age is larger than system uptime, cannot create outdated placeholder entry for token"
);
return None;
}
};There was a problem hiding this comment.
That is the old code. No changes here.
| /// Creates a new CachingNativePriceEstimator. | ||
| /// | ||
| /// The estimator will use the provided cache for lookups and will fetch | ||
| /// prices on-demand for cache misses. Background maintenance (keeping the |
There was a problem hiding this comment.
Q: should we keep metrics on cache hits / misses?
There was a problem hiding this comment.
Wdym? I think I didn't touch metrics.
There was a problem hiding this comment.
By "keep" I meant like "keep an eye", but I wasn't clear, rephrasing it: should we start tracking cache hits/misses?
There was a problem hiding this comment.
Probably makes sense to build some metrics in a separate PR. Instead of hits/misses, I personally would be interested in cached values that are never accessed later.
| updated_at, | ||
| now, | ||
| Default::default(), | ||
| EstimatorSource::default(), |
There was a problem hiding this comment.
I found it a bit hard to reason about what primary and secondary is supposed to be. Given that this logic is already extremely specific it would probably make sense to just give them specific names?
There was a problem hiding this comment.
Yeah, I was also thinking about that. Which exactly names, for example? They are located in the shared crate, which doesn't know anything about autopilot or orderbook, I assume.
|
|
||
| /// Returns true if the cache is empty. | ||
| pub fn is_empty(&self) -> bool { | ||
| self.len() == 0 |
There was a problem hiding this comment.
nit: usually if there is a an is_empty() function it's best to use that instead of len() == 0. Likely doesn't matter for this case but for example the simplest linked list has len() == O(n) but is_empty() == O(1).
| /// originally fetched it. The cache's background maintenance task uses | ||
| /// this information to dispatch updates to the appropriate estimator, | ||
| /// ensuring each token is refreshed using the same source that | ||
| /// originally fetched it. |
There was a problem hiding this comment.
This seems to relate to the design considerations you mention in the description:
The source-tracking approach addresses a subtle issue with shared cache maintenance. With two estimators (primary using CoinGecko, secondary without), a naive approach of picking one estimator for all maintenance would fail:
If primary maintains everything: tokens initially fetched via secondary would start hitting CoinGecko during maintenance, defeating the purpose
If secondary maintains everything: tokens originally fetched via CoinGecko couldn't be properly refreshed
By tagging each cached entry with its source and dispatching maintenance accordingly, tokens stay with their original estimator throughout their cache lifetime.
However, I'm don't think this is thought through completely. With the exception for onchain placed orders (😬) all new native prices have to be fetched by an API request originally. Unless I'm missing something once a user places an order for a completely new token the autopilot will continue to update the cache without coingecko (since that is not part of the estimator that originally fetched the price). I suspect this token could only ever be upgraded to be fetched by coingecko after a restart puts it into the cache and marks it as a primary token.
What I think would be closer to what we try to achieve is that we only keep tokens warm that are actually used in the auction. So what I would imagine is this:
- API request for a completely new native price
- autopilot caches it but it's not marked for maintenance yet
- a. user never places an order => maintenance never refetches the token, eventually gets evicted from the cache
b. user places an order => when autopilot fetches the price for building the auction it marks it as "worthy of maintenance"
In that approach there would be only the main estimator (with coingecko) running the maintenance and only for the tokens that were explicitly marked by the autopilot. That deviates from what we currently do (where the API is configured to run the maintenance as well) but I think since we now have a single cache that's shared by everyone and has the prices for all tokens in the auction kept up to date the API probably can stop updating it's cache entirely.
And regarding the issue that you brought somewhere else where it can be an issue where the estimator that handled the orderbook native price request is "more powerful" (i.e. would find native prices that the estimator with coingecko is not able to) I'd like to point out that we could perhaps stick to 1 required native price estimator argument and an optional coingecko argument. Only if coingecko is enabled the need for a second estimator would arise and the autopilot could just add coingecko to the regular estimators.
That way the estimator used for maintaining the cache is always at least as capable as the estimator used to handle API requests.
All that being said I agree with @jmg-duarte that this is very complicated but unfortunately I was also not able to come up with a cleaner idea. 😞
There was a problem hiding this comment.
a. user never places an order => maintenance never refetches the token, eventually gets evicted from the cache
b. user places an order => when autopilot fetches the price for building the auction it marks it as "worthy of maintenance"
My worry here is that if something is placed into the cache, it has to be maintained. Otherwise, our orderbook will be sending stale results until the item expires in the cache. That is valid for the cases where no order was placed, but still, this affects the quote competition, I suppose. Do I miss something, or can we ignore this problem for the quote competition?
Description
Once we started forwarding native price estimates from the Orderbook to Autopilot, CoinGecko API usage went up. This happened because the estimator was disabled in the Orderbook and moved to Autopilot, which now handles all requests and also relies on CoinGecko.
This PR refactors native price estimation by splitting the price estimator into primary and secondary and introducing a shared cache between them, with source-aware maintenance.
Changes
Shared cache with source tracking
Source-aware maintenance
Optional secondary estimator
Design considerations
The source-tracking approach addresses a subtle issue with shared cache maintenance. With two estimators (primary using CoinGecko, secondary without), a naive approach of picking one estimator for all maintenance would fail:
By tagging each cached entry with its source and dispatching maintenance accordingly, tokens stay with their original estimator throughout their cache lifetime.
How to test
Existing tests.