Skip to content

Commit

Permalink
Fix order likelihood logic (#2073)
Browse files Browse the repository at this point in the history
# Description

I noticed that when placing a too optimistically quoted order on Gnosis
Chain (which cannot fill) and later placing another more realistic order
on top, the second order only gets filled once the first one is expired
or cancelled. This is because the driver filters out the second order as
it looks like the user only has balance for the first. However, in
theory we should sort orders by their expected chance of filling (taking
the external auction prices into account), meaning the second order with
a more lax limit price should have priority over the first order:


https://github.com/cowprotocol/services/blob/fd7f12dded72359c087be6f3c04b898241c3eb20/crates/driver/src/domain/competition/auction.rs#L176-L195

The root cause seems to be that likelihood wasn't implemented correctly,
which this PR ixes

# Changes
- Add unit tests for the likelihood method
- Inverse the ratio that is returned by the method to comply with its
documentation (higher value means higher likelihood)

## How to test
Unit tests, also redo the test where we set an order with extremely
tight slippage tolerance that doesn't fill and place a higher slippage
order afterwards.

<!--
## Related Issues

Fixes #
-->
  • Loading branch information
fleupold authored Nov 24, 2023
1 parent 9701fc7 commit 6aee5f6
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 18 deletions.
4 changes: 2 additions & 2 deletions crates/driver/src/domain/competition/order/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,9 @@ impl Order {
(Some(buy_price), Some(sell_price)) => {
let buy = buy_price.apply(self.buy.amount);
let sell = sell_price.apply(self.sell.amount);
buy.0
sell.0
.to_big_rational()
.checked_div(&sell.0.to_big_rational())
.checked_div(&buy.0.to_big_rational())
.unwrap_or_else(num::BigRational::zero)
}
_ => num::BigRational::zero(),
Expand Down
4 changes: 2 additions & 2 deletions crates/driver/src/tests/cases/merge_settlements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ async fn possible() {
let test = setup()
.pool(cd_pool())
.pool(ab_pool())
.order(cd_order())
.order(ab_order())
.order(cd_order())
.solution(cd_solution())
.solution(ab_solution())
.done()
Expand Down Expand Up @@ -40,8 +40,8 @@ async fn possible() {
async fn impossible() {
let test = setup()
.pool(ab_pool())
.order(ab_order().rename("reduced order").reduce_amount(1000000000000000u128.into()))
.order(ab_order())
.order(ab_order().rename("reduced order").reduce_amount(1000000000000000u128.into()))
// These two solutions result in different clearing prices (due to different surplus),
// so they can't be merged.
.solution(ab_solution())
Expand Down
2 changes: 1 addition & 1 deletion crates/driver/src/tests/cases/negative_scores.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ async fn no_valid_solutions() {
async fn one_valid_solution() {
let test = setup()
.pool(ab_pool())
.order(ab_order().rename("no surplus").no_surplus())
.order(ab_order())
.order(ab_order().rename("no surplus").no_surplus())
.solution(ab_solution())
// This solution has no surplus, and hence a negative score, so it gets skipped.
.solution(Solution {
Expand Down
19 changes: 6 additions & 13 deletions crates/driver/src/tests/cases/order_prioritization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,15 @@ async fn sorting() {
let test = setup()
.pool(ab_pool())
// Orders with better price ratios come first.
.order(
ab_order()
.reduce_amount(1000000000000000u128.into()),
)
.order(ab_order().rename("second order"))
.order(ab_order())
.order(ab_order().reduce_amount(1000000000000000u128.into()).rename("second order"))
// Limit orders come after market orders.
.order(
ab_order()
.rename("third order")
.limit()
.reduce_amount(1000000000000000u128.into()),
)
.order(ab_order().rename("fourth order").limit())
.order(ab_order().reduce_amount(1000000000000000u128.into()).rename("fourth order").limit())
.solution(ab_solution())
.done()
.await;
Expand All @@ -40,11 +36,8 @@ async fn filtering() {
let test = setup()
.pool(ab_pool())
// Orders with better price ratios come first.
.order(
ab_order()
.reduce_amount(1000000000000000u128.into()),
)
.order(ab_order().rename("second order"))
.order(ab_order())
.order(ab_order().reduce_amount(1000000000000000u128.into()).rename("second order"))
// Filter out the next order, because the trader doesn't have enough balance to cover it.
.order(
ab_order()
Expand All @@ -56,7 +49,7 @@ async fn filtering() {
// fulfill the previous orders.
.order(
Order {
sell_amount: 4999899999900002000000000000000u128.into(),
sell_amount: 4999999999900002000000000000000u128.into(),
surplus_factor: 1.into(),
..ab_order()
}
Expand Down

0 comments on commit 6aee5f6

Please sign in to comment.