-
Notifications
You must be signed in to change notification settings - Fork 153
Autopilot Native Price API #3988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f37f048 to
4699fb5
Compare
31968f3 to
5db5fc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a native price API in the autopilot service and configures the orderbook to forward native price requests to it, addressing inconsistencies caused by separate caches in different processes. The implementation includes a fallback mechanism to the legacy price estimators if the autopilot is unavailable.
Key Changes:
- Added an HTTP API server to autopilot exposing a
/native_price/:tokenendpoint - Implemented a forwarding native price estimator in the orderbook that delegates to the autopilot API with fallback support
- Updated e2e test infrastructure with a reverse proxy to support the dual-autopilot test scenario
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
crates/autopilot/src/infra/api.rs |
New HTTP API server for serving native price requests with error mapping |
crates/autopilot/src/infra/mod.rs |
Module declaration for the new API module |
crates/autopilot/src/run.rs |
Integration of the API server with graceful shutdown handling |
crates/autopilot/src/arguments.rs |
Added CLI argument for configuring the native price API bind address |
crates/autopilot/Cargo.toml |
Added axum, tower, and tower-http dependencies with tracing support |
crates/orderbook/src/native_price_forwarder.rs |
New forwarding estimator that proxies requests to autopilot with fallback logic |
crates/orderbook/src/lib.rs |
Module declaration for the native price forwarder |
crates/orderbook/src/run.rs |
Wraps the native price estimator with the forwarder when autopilot URL is configured |
crates/orderbook/src/arguments.rs |
Added CLI argument for the autopilot native price API URL |
crates/e2e/src/setup/proxy.rs |
New reverse proxy implementation for load balancing between two autopilot instances in tests |
crates/e2e/src/setup/mod.rs |
Module declaration for the proxy |
crates/e2e/src/setup/services.rs |
Updated default test configuration to use autopilot for native prices |
crates/e2e/tests/e2e/autopilot_leader.rs |
Updated dual-autopilot test to use the proxy for native price API failover |
Cargo.lock |
Dependency updates reflecting new autopilot dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
# Conflicts: # crates/e2e/tests/e2e/autopilot_leader.rs
MartinquaXD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks alright overall. Main confusion for me at the moment are the changes to the CLI args in the tests.
crates/e2e/src/setup/proxy.rs
Outdated
| let path = parts | ||
| .uri | ||
| .path_and_query() | ||
| .map(|pq: &axum::http::uri::PathAndQuery| pq.as_str()) | ||
| .unwrap_or(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this would better live inside try_backend. Ideally handling of the body bytes would also go there but since warp::hyper::body::to_bytes has to consume the Body this doesn't work well unfortunately.
| fn api_autopilot_arguments(&self) -> impl Iterator<Item = String> + use<> { | ||
| fn api_arguments(&self) -> impl Iterator<Item = String> + use<> { | ||
| [ | ||
| "--native-price-estimators=test_quoter|http://localhost:11088/test_solver".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this was already the case when I originally commented on this line but the --native-price-estimator argument in api_arguments now has to be overwritten in at least 1 setup (either api or autopilot) so I'm not sure if this should show up here at all. WDYT?
| fn api_autopilot_arguments(&self) -> impl Iterator<Item = String> + use<> { | ||
| fn api_arguments(&self) -> impl Iterator<Item = String> + use<> { | ||
| [ | ||
| "--native-price-estimators=test_quoter|http://localhost:11088/test_solver".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also originally I thought the new proxy is supposed to be used for every test - not just for the autopilot leader test. If it's only supposed to be used for very few tests making Forwarder the default here doesn't seem right. Sorry for the confusion.
And if the new proxy is only intended to be used for the autopilot leader tests the only tests that should require changes to the CLI args would be those, no?
|
|
jmg-duarte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit
MartinquaXD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just nits remaining.
Description
From the original issue #3831:
Changes
axumserver toautopilotto handle/native_pricerequestsDriverprefix to distinguish between legacy and new approachForwarderprefix is added to use the Forwarder native price estimator.dual_autopilot_only_leader_produces_auctionsrequires running 2 autopilots at the same time for some period of time. Since it doesn't make much sense to specify more than 2 autopilot URLs in the orderbook config, a reverse proxy is introduced to make this test pass. This proxy simulates k8s behavior: when 2 instances of the same service are running, requests are automatically routed between them. An alternative approach is to run 2 autopilot APIs on different ports and specify them in the orderbook config, but the reverse proxy simulates the production behavior.openapi.ymlfile with the hope that this component will be decentralized in the future.How to test
Updated e2e tests. This also requires running tests on staging manually.
Related Issues
Partially fixes #3831
Further implementation
Once we are confident the new approach works well, the old config with native price estimators can be dropped from the orderbook crate.