[feature] add ergonomic /quote v2 endpoint (#4058)#4088
[feature] add ergonomic /quote v2 endpoint (#4058)#4088theghostmac wants to merge 16 commits intocowprotocol:mainfrom
Conversation
…andle new params and return formats
…r it alongside existing endpoints
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![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 adds a new /v2/quote endpoint to simplify order creation. The implementation reuses the existing /v1/quote logic and then applies slippage. My review found a critical vulnerability in the slippage calculation that could lead to creating incorrectly priced orders. The details are in the specific comment.
| match order.kind { | ||
| OrderKind::Sell => { | ||
| // buyAmount = buyAmount * (10000 - slippageBps) / 10000 | ||
| order.buy_amount = U256::uint_try_from( | ||
| order | ||
| .buy_amount | ||
| .widening_mul(U256::from(MAX_BPS - slippage_factor)) | ||
| / U512::from(MAX_BPS), | ||
| ) | ||
| .unwrap_or(U256::MAX); | ||
| } | ||
| OrderKind::Buy => { | ||
| // sellAmount = sellAmount * (10000 + slippageBps) / 10000 | ||
| order.sell_amount = U256::uint_try_from( | ||
| order | ||
| .sell_amount | ||
| .widening_mul(U256::from(MAX_BPS + slippage_factor)) | ||
| / U512::from(MAX_BPS), | ||
| ) | ||
| .unwrap_or(U256::MAX); | ||
| } | ||
| } |
There was a problem hiding this comment.
The calculation MAX_BPS - slippage_factor can underflow if slippage_bps is greater than 10000. This would cause an integer underflow in release builds, resulting in an incorrect and potentially very large buy_amount for sell orders. This is a critical issue.
Using saturating_sub prevents this underflow by clamping the result at zero, which is the correct behavior for slippage >= 100%.
match order.kind {
OrderKind::Sell => {
// buyAmount = buyAmount * (10000 - slippageBps) / 10000
order.buy_amount = U256::uint_try_from(
order
.buy_amount
.widening_mul(U256::from(MAX_BPS.saturating_sub(slippage_factor)))
/ U512::from(MAX_BPS),
)
.unwrap_or(U256::MAX);
}
OrderKind::Buy => {
// sellAmount = sellAmount * (10000 + slippageBps) / 10000
order.sell_amount = U256::uint_try_from(
order
.sell_amount
.widening_mul(U256::from(MAX_BPS + slippage_factor))
/ U512::from(MAX_BPS),
)
.unwrap_or(U256::MAX);
}
}References
- This comment focuses on a logic error (integer underflow) which is a critical issue. (link)
There was a problem hiding this comment.
great catch!
i should probably also add validation to reject unreasonable slippage:
if request.slippage_bps > 1000 { // 10% max slippage
return Err(OrderQuoteError::InvalidSlippage);
}
Can you elaborate on this? |
|
Hello, I would like to share this document which outlines the potential features of the v2. I want to be part of verifying the new interface proposal, and also front-end as creator of the SDK. Here is a write up about the feature https://www.notion.so/cownation/Quote-API-v2-2f18da5f04ca8032b09de7e2f409e46b?source=copy_link |
here: It does compile with just: And tests still pass. Making the fix here would be outside the scope of this PR, hence why I ignored. |
|
@theghostmac it compiles just fine in Regardless, that case should always be ok because Rust knows the size of all parties involved, LHS would be the |
|
true. i would go ahead and give @anxolin's document a read. although i do understand that some of it is internal. |
|
Hei @theghostmac, sorry there was probably a misunderstanding on my part. By reading the PR title "add ergonomic /quote v2 endpoint" I thought you were going to work on the evolution of the API we envisioned for easy integrations. The document I shared is not complete, but would give you an overview of the problem. Our SDK solves most of it, so its a great source for inspiration. The screenshots at the end try to show the 3 main prices UIs show and therefore, its handy to return in the endpoint DTO. I understand now you were not intended to solve exactly this problem and your PR had a very different scope. |
|
Hi @anxolin, Yes, I would love to work on the full spec. I paused because I thought it was internal. I can continue now. |
…amount helper func
… conversion limitation, and partner fee currency handling, then add unit tests for all methods
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
|
hi @MartinquaXD, @anxolin, anybody... is it possible to get feedback about this before it gets closed by GA bot? |
3 participants
Description
This PR introduces a new
/api/v2/quoteendpoint that simplifies order creation for frontend applications by providing a complete, ready-to-sign response.The existing
/quoteendpoint returns raw price and fee data, requiring frontends to manually calculate slippage protection and apply fees. This has been a common source of confusion and errors.The new
v2endpoint solves this by returning:OrderCreationobject ready to sign (withfeeAmountset to0per the solver-competition model.beforeAllFees,afterNetworkCosts, andafterSlippageChanges
OrderQuoteRequestV2,OrderQuoteV2,QuoteBreakdown,CostBreakdowntypes incrates/model/src/quote.rs.calculate_quote_v2inQuoteHandler(crates/orderbook/src/quoter.rs) to handle slippage math and zero out fees.POST /api/v2/quoteendpoint incrates/orderbook/src/api/post_quote.rs.v2/quoteroute in the orderbook API router (crates/orderbook/src/api.rs).How to test
cargo test -p orderbook api::post_quotecargo test -p orderbook quoter::tests/api/v2/quotewith a payload containingslippageBps.Architectural notes:
calculate_quote_internal()now feedscalculate_quote()andcalculate_quote_v2()unadjusted_quote(), so i avoided double-counting by reusing that instead.buy_token_pricein price feed for the exact conversionFuture work
setPReSignatureRelated Issues
Fixes # #4058