Stop consuming uniform clearing prices in the autopilot#4370
Stop consuming uniform clearing prices in the autopilot#4370jmg-duarte wants to merge 2 commits into
Conversation
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
Scoring uses auction-level native prices and settlement verification reads prices from on-chain calldata, so per-solution UCPs are dead weight. Drop them from the autopilot domain `Solution`, the `SolutionError::InvalidPrice` variant, and the `proposed_solutions` price columns (now written as empty arrays). The `/solve` wire format is preserved for rolling-deploy safety: - autopilot tolerates a missing `clearingPrices` (`#[serde(default)]`) - our driver still emits it from existing settlement data `clearingPrices` is marked `deprecated: true` in the driver and orderbook OpenAPI specs. A follow-up PR will remove the field, the deprecation log, and the empty DB columns.
jmg-duarte
force-pushed
the
jmgd/remove-ucp/2
branch
from
d750674 to
ebf806c
Compare
![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 deprecates and removes the use of uniform clearing prices across the autopilot and driver components. Key changes include removing the prices field from the Solution domain model, updating the persistence layer to store empty price vectors, and marking clearing_prices as deprecated in the OpenAPI specifications and DTOs to maintain backward compatibility during rolling deployments. No critical issues found; I have no feedback to provide.
AryanGodara
left a comment
AryanGodara
left a comment
There was a problem hiding this comment.
LGTM.
just one comment regarding (possible) dead code removal after this PR merges
| impl Solution { | ||
| pub fn into_domain( | ||
| self, | ||
| ) -> Result<domain::competition::Solution, domain::competition::SolutionError> { |
There was a problem hiding this comment.
A question regarding post-PR cleanup.
now that InvalidPrice is gone, Solution::into_domain always returns Ok(...), which makes the Result<_, SolutionError> wrapper dead end-to-end.
Metrics::solution_err (run_loop.rs:1006), the filter_map over solutions (run_loop.rs:641), and the partition_result in shadow.rs:230 all become unreachable.
The two remaining SolutionError variants (ZeroScore, SolverDenyListed) have no construction sites anywhere in the workspace; that part was already preexisting, but this PR is now makes the wrapper itself unreachable
I tried removing this locally, and it works cleanly. ie, tests/linter/check all passing 👀
There was a problem hiding this comment.
Will take a look, good find!
| self, | ||
| ) -> Vec<Result<domain::competition::Solution, domain::competition::SolutionError>> { | ||
| for solution in &self.solutions { | ||
| if !solution.clearing_prices.is_empty() { |
There was a problem hiding this comment.
nit: we only care about whether the driver still sends UCP for any solutions, right?
In that case we could just emit a single log.
| price_tokens: vec![], | ||
| price_values: vec![], |
There was a problem hiding this comment.
Could you explain how we are going to compute captured fees going forward?
I remember a discussion with the solver team stating that instead of reading the "original" token price from the calldata we would read them from the DB but this is now also dropping that information, right?
3 participants
Description
The autopilot no longer needs per-solution uniform clearing prices: scoring uses auction-level native prices, and on-chain settlement verification reads UCPs straight from the calldata. This PR removes UCPs from the autopilot's domain model and persistence path while keeping the wire format intact, so the change is safe under a rolling k8s deploy.
Changes
pricesfrom the autopilot's domainSolution(field, constructor arg, andprices()getter).SolutionError::InvalidPriceand itsinvalid_pricemetric label — price validation is no longer performed when ingesting a/solveresponse.proposed_solutions.price_tokens/price_values. TheNOT NULLcolumns are retained for now (an empty array still satisfies the constraint); they can be dropped in a follow-up.clearingPrices: add#[serde(default)]to the deserializer and emit adebug!log when a driver still sends a non-empty map, so we can chase down stragglers before fully removing the field.clearingPriceson/solveresponses (with a deprecation comment) so autopilots running the previous code can still deserialize during the rolling deploy.clearingPricesasdeprecated: truein the driver/solveresponse schema and in the orderbooksolver_competition_v2schema, with a note explaining that recent autopilots will return it empty.A follow-up PR will remove the driver-side field, the deprecation log, and (optionally) drop the now-unused
proposed_solutions.price_tokens/price_valuescolumns.How to test
cargo nextest run -p autopilot -p driver— unit tests, includingwinner_selectiontests updated to construct solutions without prices.solver_competition_v2for an auction produced by the new autopilot and confirmclearingPricesis{}; for an auction produced before the change it remains populated.Staging
Ran in staging with these orders:
Logs: