Fix haircut mismatch between reported and on-chain amounts#4109
Fix haircut mismatch between reported and on-chain amounts#4109squadgazzz merged 8 commits intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the mismatch between reported and on-chain amounts when a haircut is configured by moving the haircut logic into the sell_amount() and buy_amount() functions. This ensures consistency. The tests have been updated appropriately to validate the new behavior. I have one piece of feedback regarding error handling, specifically the use of the correct error variant for arithmetic underflow.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
MartinquaXD
left a comment
There was a problem hiding this comment.
Change looks reasonable but given that the other changes also looked reasonable and then resulted in the circuit breaker deny-listing the solver could you please also manually test this on staging before merging?
Other than that only minor nits.
| // Base buy amount from executed sell | ||
| let base = self |
There was a problem hiding this comment.
Since both computations do the same logic you could simplify like this, no?:
self.executed.0
.checked_sub(self.haircut_fee)
.ok_or(Math::Negative)?
.checked_mul(prices.sell)
.ok_or(Math::Overflow)?
.checked_div(prices.buy
.ok_or(Math::DivisionByZero)?;There was a problem hiding this comment.
IIUC, the original logic uses different rounding for the base amount vs. the fee:
let base = ceil(executed * sell / buy);
let haircut_in_buy = floor(haircut * sell / buy);
let result = base - haircut_in_buy;The proposed simplification:
let result = (executed - haircut) * sell / buy;These are not mathematically equivalent.
Concrete example:
- executed = 11, haircut = 4, sell = 2, buy = 5
Original:
- base = ceil(11 × 2 / 5) = ceil(22/5) = ceil(4.4) = 5
- haircut_in_buy = floor(4 × 2 / 5) = floor(8/5) = floor(1.6) = 1
- result = 5 - 1 = 4
Simplified (even with ceil):
- result = ceil((11 - 4) × 2 / 5) = ceil(14/5) = ceil(2.8) = 3
Yep, it was already tested: https://cowservices.slack.com/archives/C0361CDD1FZ/p1769791530558949 |
Description
Fixes the mismatch between driver-reported amounts and on-chain executed amounts when the haircut is configured. Previously, the driver reported higher buy amounts than users actually received on-chain (for sell orders), resulting in a discrepancy that matched the configured haircut.
Root cause:
sell_amount()andbuy_amount()did NOT include haircut, butcustom_prices()(used for on-chain encoding) DID. This caused reported amounts to differ from on-chain execution.Changes
Include haircut effects in
sell_amount()andbuy_amount()so that:For sell orders:
sell_amount()→ unchanged (user sells exactly what they signed)buy_amount()→ reduced by haircut (user receives less)For buy orders:
sell_amount()→ increased by haircut (user pays more)buy_amount()→ unchanged (user receives exactly what they signed for)How to test
Adjusted existing tests.