Round protocol fee up in quote computation#4389
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
Co-authored-by: José Duarte <15343819+jmg-duarte@users.noreply.github.com>
| // BPS) | ||
| let scaled_factor = U256::from(factor.to_high_precision()); | ||
| let scale = U512::from(FeeFactor::HIGH_PRECISION_SCALE); | ||
| if scale.is_zero() { |
There was a problem hiding this comment.
Isn't that some kind of dead code? The line above is a static value.
| .ok_or_else(|| anyhow::anyhow!("volume fee calculation division by zero"))?, | ||
| ) | ||
| .map_err(|_| anyhow::anyhow!("volume fee calculation overflow"))?; | ||
| let numerator: Uint<512, 8> = total_sell_volume.widening_mul(scaled_factor); |
There was a problem hiding this comment.
The sell branch (L255) dropped the explicit Uint<512, 8> annotation on numerator (let numerator = ...), but this buy branch kept it. Both come from widening_mul(U256, U256) and have the same inferred type. Pick one style and apply it to both branches.
| if scale.is_zero() { | ||
| return Err(anyhow::anyhow!("volume fee calculation division by zero")); | ||
| } | ||
| // Round the fee up so we never undercharge the user. |
There was a problem hiding this comment.
"so we never undercharge the user" reads naturally for buy orders (user pays slightly more sell) but is slightly off for sell orders (user receives less buy, which is "underpaying the user" rather than "undercharging"). Suggest "Round the fee up so we never overpromise to the user." or "...so the protocol can always cover the advertised amounts."
| } | ||
|
|
||
| #[test] | ||
| fn test_volume_fee_rounds_up_buy_order() { |
There was a problem hiding this comment.
This test uses fee_amount = 0, which keeps total_sell_volume = quote.sell_amount and effectively retests the sell-branch math. The existing test_volume_fee_buy_order_with_network_fee uses exact division (105e18 / 5000), so it doesn't exercise the ceil path either. A single combined case (non-zero fee_amount AND a non-exact division) would tighten coverage for the buy branch's (sell + network_fee) * factor / scale path. Not a blocker.
3 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