Conversation
Make the volume-based protocol fee applied during quoting use ceiling division instead of floor, matching the pessimistic rounding already used for the network fee in `crates/shared/src/fee.rs`. The quote now never undercharges users at the wei level: SELL orders deliver a slightly smaller buy_amount, BUY orders quote a slightly larger sell_amount on inexact divisions.
![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 updates the protocol fee calculation in get_vol_fee_adjusted_quote_data to use ceiling rounding, ensuring fees are rounded up for both sell and buy orders. It also adds an explicit zero-check for the division scale and includes unit tests to verify the new rounding logic. I have no feedback to provide.
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
Overall looks good to me and makes sense, just left some notes for small improvements
| // observe the ceiling. factor 0.0001 -> high_precision = 100, scale = | ||
| // 1_000_000, so fee = buy_amount * 100 / 1_000_000 = buy_amount / 10_000. | ||
| // Use buy_amount = 12345 so 12345 / 10000 = 1.2345 -> ceil = 2. |
There was a problem hiding this comment.
The way this is formatted is a bit hard to read and understand
Especially the factor 0.0001 -> high_precision = 100, I don't understand what you mean
| if scale.is_zero() { | ||
| return Err(anyhow::anyhow!("volume fee calculation division by zero")); | ||
| } |
There was a problem hiding this comment.
Nice, makes the rest of the code much cleaner
Co-authored-by: José Duarte <15343819+jmg-duarte@users.noreply.github.com>
2 participants
Summary
Make fee rounding consistent.
Takes a pessimistic approach, the one who yields a:
buyAmountfor sell orders, which allows solvers to keep all fees and still fulfill the order. If we round it down, we might be promising the user one 1wei more, so when the solver takes the fee, the order might not be fillable.sellAmountfor buy orders should also round fees up, which makes the user to sign 1wei more so we can fill the order after taking the feesContext
I noticed a small divergence between what the quote endpoint returns, and what debug.cow.fi thinks it return based on its own calculations.
See this order:
0xd129e1a85ec79443540264ee8bb37841c9c4d5850c9b1c5ea6e1d764e8aff3e7cfe5660d6c55906ec8c488a466bd4f77f77eec8869fdbaebThe tool calculates the amount at slippage=0, which is basically what the API returns. In this case, returns
63774986, but if you check the logs you see63774987(it overpromises by 1 wei). See https://victorialogs.dev.cow.fi/goto/aflg4t470vz7kc?orgId=1Details
The volume-based protocol fee applied during quote computation in
crates/orderbook/src/quoter.rspreviously used floor division (checked_div), while the network fee incrates/shared/src/fee.rsuses ceiling. This PR aligns both to the same pessimistic policy: always round protocol fee UP.Effect at the wei level on inexact divisions:
buy_amount = quote.buy_amount − ceil(buy * factor / scale)— user receives slightly less buy, never more than the protocol can cover.sell_amount = quote.sell_amount + ceil((sell + network_fee) * factor / scale)— user pays slightly more sell, never less than required.The change touches only quote-time amounts shown to users; settlement-time fee accounting is unchanged.
Test plan
cargo test -p orderbook quoter::tests— 9/9 pass, including two new tests (test_volume_fee_rounds_up_sell_order,test_volume_fee_rounds_up_buy_order) that exercise non-exact divisions where floor and ceil diverge (12345 * 100 / 1_000_000 = 1.2345 → ceil = 2).cargo check -p orderbook