Align priceQuality implementation with API docs - #4805
Conversation
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
|
Claude finished @fleupold's task in 3m 33s —— View job PR Review: Align priceQuality implementation with API docs
The change is well-structured and correctly implements the documented split between
One maintainability note (inline): the No correctness issues found. The behavior change for clients explicitly requesting |
| let unverified_price_estimator = price_estimator_factory | ||
| .unverified_price_estimator( | ||
| &config | ||
| .order_quoting | ||
| .price_estimation_drivers | ||
| .iter() | ||
| .map( | ||
| |price_estimator_driver| configs::native_price_estimators::ExternalSolver { | ||
| name: price_estimator_driver.name.clone(), | ||
| url: price_estimator_driver.url.clone(), | ||
| }, | ||
| ) | ||
| .collect::<Vec<_>>(), | ||
| native_price_estimator.clone(), | ||
| gas_price_estimator.clone(), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
This price_estimation_drivers.iter().map(...).collect() block is now repeated three times verbatim (here plus the price_estimator and fast_price_estimator calls above). Consider extracting it into a small local binding once, e.g.:
let price_estimation_solvers: Vec<_> = config
.order_quoting
.price_estimation_drivers
.iter()
.map(|d| configs::native_price_estimators::ExternalSolver {
name: d.name.clone(),
url: d.url.clone(),
})
.collect();and pass &price_estimation_solvers to all three. Reduces the risk of the three copies drifting.
MartinquaXD
left a comment
There was a problem hiding this comment.
Looks alright to me.
|
Given this is a breaking change, I'm holding off merging until we have figured out which existing integration partners should change their |
| Estimates do not get verified by | ||
| simulation. | ||
| Optimal: The price estimate is chosen among all price estimates, ranked | ||
| purely by the promised price. Estimates do not get verified by | ||
| simulation. | ||
| Verified: All price estimates get verified by simulation whenever | ||
| possible and verified estimates are preferred over unverified ones, | ||
| even when an unverified estimate promises a better price. The | ||
| response's `verified` flag indicates whether the returned estimate | ||
| was actually verified. |
There was a problem hiding this comment.
nit
| Estimates do not get verified by | |
| simulation. | |
| Optimal: The price estimate is chosen among all price estimates, ranked | |
| purely by the promised price. Estimates do not get verified by | |
| simulation. | |
| Verified: All price estimates get verified by simulation whenever | |
| possible and verified estimates are preferred over unverified ones, | |
| even when an unverified estimate promises a better price. The | |
| response's `verified` flag indicates whether the returned estimate | |
| was actually verified. | |
| Estimates do not get verified by simulation. | |
| Optimal: The price estimate is chosen among all price estimates, ranked | |
| purely by the promised price. Estimates do not get verified by simulation. | |
| Verified: All price estimates get verified by simulation whenever | |
| possible and verified estimates are preferred over unverified ones, | |
| even when an unverified estimate promises a better price. The | |
| response's `verified` flag indicates whether the returned estimate | |
| was actually verified. |
# Summary All "good" quotes are now `verified` instead of `optimal`. `fast` quotes remains as they are. <img width="475" height="338" alt="image" src="https://github.com/user-attachments/assets/e1795e93-8f97-46a1-b529-02e15cd97c13" /> <img width="486" height="255" alt="image" src="https://github.com/user-attachments/assets/50d87e05-84da-4712-a2ec-658a59121482" /> Historically, they have been the same. This will change on cowprotocol/services#4805. Switching to verified here preserves the current behaviour. SDK is also changing the default cowprotocol/cow-sdk#975, although we explicitly set the price quality in some areas. Changing here all to be always explicit. ##⚠️ Notes: - One path that is staying on `optimal`, and will be changed once backend changes, is the unfillable order updater. As we don't intend to place orders using this value - it's only there for reference of the target price - switching to optimal should save backend resources and deliver the UI update faster. # To Test 1. Load a price on SWAP, and check the `/quote` request in the console. Ignore the `fast` quality, those will still be there. 2. Check for the `priceQuality` in the request: * Should be `verified` 3. Check the response: * Should have `verified: true` 4. Check all order types * All non `fast` quotes should be `verified` instead of `optimal` * Order placing/execution should remain as is 5. When there's a pending limit order, the check the quote call for fetching the `fills at` column * Request should be sent with priceQuality `optimal`. # Self-checks - [x] I have read [CONTRIBUTING.md](../CONTRIBUTING.md) - [x] I have manually tested changes on Vercel preview deployment - [x] I have done self-review and (or) AI review - [x] I have addressed all comments from @coderabbitai - [x] I have less than three open PRs/Stacks in this repo at the moment of creating this PR <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Standard swap quotes now use verified pricing. * Quote selection, polling, loading, and trade readiness now consistently recognize final quotes. * Price checks for unfillable orders now use optimized pricing. * **Improvements** * TWAP and full-amount quotes now use shared quote settings. * **Tests** * Updated automated coverage and fixtures for verified and final quote behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
The API docs state that "optimal" quotes should only care about the out amount, without attempting or taking verification into account. However, the current logic uses the same behavior for optimal and verified quotes. This PR changes this.
Note, that verification can still be turned off server side by setting
verification-mode unverified.Streamed quotes continue to use the verified quoting logic (as per docs)
Changes
How to test
Adjusted e2e test