Fix price improvement fee when scoring #2567
Conversation
| .checked_div(self.custom_price.sell)?; | ||
| // since sell price limit can be lower than order sell limit (e.g. price | ||
| // improvement fee with quote as price limit), we don't want to | ||
| // return error in cases when the solution is worse than price limit (quote) |
There was a problem hiding this comment.
Would we want to return an error if we have a negative surplus compared to the actual limit price of the order (not just the quote)?
Also I find the comment quite hard to understand.
IIUC the heart of the problem is that that surplus may be called with different price limits depending on the fee (order limit price vs. quoted price). And there is the option that an execution respects the limit price but does not actually improve over the quoted price. In that case this function would return an error without this patch, right?
There was a problem hiding this comment.
I think this method name is no longer accurate. Maybe we should call it price_improvement. I even wonder if Result<Option<eth::Asset>> would be a better return type (that returns Ok(None) in case of negative improvement). Then for surplus we can still assert that it's not null, whereas of price improvement call sites we can unwrap None into 0.
There was a problem hiding this comment.
Would we want to return an error if we have a negative surplus compared to the actual limit price of the order (not just the quote)?
No. We also do not return error when calculating the protocol fee to be withheld (fee.rs). Here, it is even unexpected to ever happen since scoring is called after solution passed simulation (although we should not assume wider context of function usage to decide on these kind of things).
In that case this function would return an error without this patch, right?
Yes
There was a problem hiding this comment.
Renamed to surplus_over_reference_price as we already have a function with similar functionality.
Introduced surplus_over_limit_price and surplus_over_quote.
Updated the comments.
Can you check if it is clearer now?
| .checked_sub(limit_buy) | ||
| .checked_mul(self.custom_price.sell) | ||
| .ok_or(Math::Overflow)? | ||
| .checked_ceil_div(&self.custom_price.buy) |
There was a problem hiding this comment.
hidden checked_ceil_div here :)
| return Ok(eth::Asset { | ||
| token: self.surplus_token(), | ||
| amount: 0.into(), | ||
| }); |
| .ok_or(Math::Overflow)? | ||
| .checked_div(self.custom_price.sell) | ||
| .ok_or(Math::DivisionByZero)?; | ||
| limit_sell.checked_sub(sold).ok_or(Error::NegativeSurplus( |
There was a problem hiding this comment.
Wondering if this should be Math::Negative (or Math::Underflow) instead?
There was a problem hiding this comment.
Wasn't sure about this one, since it could lead to accidentally adding another case in the future, where Math::Negative is returned, while not being related to negative surplus... but implemented, let's keep the current code clean.
Description
In cases when solvers provide solutions with no improvement over quote, scoring module returned error instead of 0, for protocol fee. Now it returns 0.
Similar thing already exists when the fee is actually calculated: https://github.com/cowprotocol/services/blob/main/crates/driver/src/domain/competition/solution/trade.rs#L200
How to test
Added driver test which now passes.