Partially fillable orders in driver tests #2512

squadgazzz · 2024-03-12T12:26:16Z

Description

Adds support for partially fillable orders in the driver's integration tests

Related Issues

crates/driver/src/tests/cases/protocol_fees.rs

fleupold

Haven't checked the test cases in depth yet. I'm still a bit confused by the changes to the core tests (I'm pretty sure partial is mis-appropriated from reading the comments in the code)

fleupold · 2024-03-14T16:57:49Z

crates/driver/src/tests/setup/mod.rs

+    /// Override the executed target amount of the order. Useful for testing
+    /// liquidity orders. Otherwise [`execution_diff`] is probably more
+    /// suitable.


We are not testing liquidity order though, are we? I wonder if executed is even needed or if we can instead use ab_adjusted_pool(quote) to control how much is executed. Have you checked that?

You mean to pass order's full amount into this function?

services/crates/driver/src/tests/setup/blockchain.rs

Line 64 in 2ac00e7

pub fn out_given_in(&self, input: Asset) -> eth::U256 {

That doesn't work since ab_adjusted_pool function adjusts one of the pool's reserves to align with the quote's ratio. For different inputs, the pool returns different results.

fleupold · 2024-03-14T17:01:26Z

crates/driver/src/tests/setup/blockchain.rs

@@ -570,6 +608,20 @@ impl Blockchain {
                }),
                buy_amount,
            ),
+            // todo: simplify it


Please do. This conditional is now very hard to read. I think we should have two conditional, one checks if there is an explicit amount set (in which case we use it, using in_given_out for buys and out_given_in for sells, otherwise we use the whole sell amount out_given_in. Ideally we would even get rid of this optionality as I feel it's quite hard to understand already.

Applied your suggestion, thanks!

fleupold · 2024-03-14T17:02:51Z

crates/driver/src/tests/cases/protocol_fees.rs

+    let executed = match partial {
+        Partial::Yes { .. } => match test_case.order.side {
+            order::Side::Buy => Some(test_case.execution.solver.buy),
+            order::Side::Sell => Some(test_case.execution.solver.sell - solver_fee),


Why do we subtract the solver fee here? Shouldn't the whole partial sell amount be executed?

Since executed amount is filled manually, the logic replicates this code.

fleupold · 2024-03-14T17:05:29Z

crates/driver/src/tests/cases/protocol_fees.rs

+        Partial::No
+    } else {
+        Partial::Yes {
+            executed: partially_executed,


services/crates/driver/src/domain/competition/order/mod.rs

Lines 240 to 244 in e970e8e

/// The available amount that can be used from the order.

///

/// This amount considers both how much of the order has already been

/// executed as well as the trader's balance.

available: TargetAmount,

This value is meant to say how much of the order has already been filled in previous settlement. This is indeed another test scenario (having an order be filled partially multiple times and check that each fee is correct), but I don't believe you are trying to test this here.

Changed to zero amount

fleupold · 2024-03-14T17:08:39Z

crates/driver/src/tests/setup/blockchain.rs

@@ -131,6 +131,44 @@ impl QuotedOrder {
        }
    }

+    /// Calculates buy amount that should be received from a driver


What does this mean? Why is this necessary and different to e.g. executed?

fleupold · 2024-03-14T17:11:15Z

crates/driver/src/tests/cases/protocol_fees.rs

+            side: order::Side::Buy,
+        },
+        execution: Execution {
+            // 6 ETH surplus in sell token (after network fee), half of which is kept by the


Can we simplify this example by using more round fractions (e.g. have the order be ratiod 1:1 or 1:2 and partially match it 50%)?

This way it's easy to see the surplus (here you need to do some non-trivial math)

Simplified the amounts.

fleupold

See https://github.com/cowprotocol/services/compare/2512 for a concrete suggestion and a single test case.

squadgazzz · 2024-03-14T18:49:53Z

See https://github.com/cowprotocol/services/compare/2512 for a concrete suggestion and a single test case.

Applied the suggestion but with the following changes:

services/crates/driver/src/tests/setup/blockchain.rs

Line 589 in 2ac00e7

.map(|amount| amount + order.solver_fee.unwrap_or_default())

services/crates/driver/src/tests/cases/protocol_fees.rs

Line 65 in 2ac00e7

Some(test_case.execution.solver.sell - solver_fee)

Without this, sell orders don't work properly.

fleupold

LGTM

crates/driver/src/tests/cases/protocol_fees.rs

Co-authored-by: Felix Leupold <felixleupold90@gmail.com>

MartinquaXD

Looks alright to me. Just slightly surprised about the case where we apply a price improvement fee on an out-of-market order. Overall the chosen values help to follow the math in your head. 👍

MartinquaXD · 2024-03-18T11:32:35Z

crates/driver/src/tests/cases/protocol_fees.rs

+        order::Side::Buy => (test_case.order.buy_amount > test_case.execution.solver.buy)
+            .then_some(test_case.execution.solver.buy),
+        order::Side::Sell => {
+            if test_case.order.sell_amount > test_case.execution.solver.sell {


nit: why use .then_some() in one case and if else in another?

Forgot to update it, thanks

MartinquaXD · 2024-03-18T11:45:10Z

crates/driver/src/tests/cases/protocol_fees.rs

+    let test_case = TestCase {
+        fee_policy,
+        order: Order {
+            // Demanding to sell less than quoted (out-market)


Do we expect a price improvement fee to ever apply to an out-of-market order?
In this case we are falling back to taking a fee compared to the limit price, right?

In this case price improvement behaves exactly like Surplus fees (assuming the same factor). Depending on the final fee policy that is decided we may be able to use a single fee policy for all order types.

squadgazzz added 6 commits March 12, 2024 12:24

Partially fillable orders in driver tests

938f21b

Use partially fillable values

c1fa5b5

Better values

0ff04bf

tmp

578ab42

Remove explicit partially fillable values

f176d45

Comments

5075047

sunce86 reviewed Mar 13, 2024

View reviewed changes

crates/driver/src/tests/cases/protocol_fees.rs Outdated Show resolved Hide resolved

Fixed value

257d023

squadgazzz force-pushed the 2480/partially-fillable-driver-tests branch from 0bcd6e3 to 36617c8 Compare March 13, 2024 15:34

Sell order adjustments

9c9bf73

squadgazzz force-pushed the 2480/partially-fillable-driver-tests branch from 36617c8 to 9c9bf73 Compare March 13, 2024 15:53

Capped surplus partially fillable tests

2f844ef

squadgazzz force-pushed the 2480/partially-fillable-driver-tests branch from e196e42 to 2f844ef Compare March 13, 2024 17:23

squadgazzz added 2 commits March 13, 2024 19:32

Volume fee partially fillable orders tests

72ff73a

Price Improvement fee partially fillable orders tests

227db4e

squadgazzz changed the title ~~Draft: Partially fillable orders in driver tests~~ Partially fillable orders in driver tests Mar 13, 2024

squadgazzz marked this pull request as ready for review March 13, 2024 20:24

squadgazzz requested a review from a team as a code owner March 13, 2024 20:24

Minor refactoring

8ebca7a

squadgazzz force-pushed the 2480/partially-fillable-driver-tests branch from c16bd88 to 8ebca7a Compare March 13, 2024 20:52

fleupold requested changes Mar 14, 2024

View reviewed changes

Zero partially executed

8c212ec

fleupold reviewed Mar 14, 2024

View reviewed changes

Applied suggestion

2ac00e7

Redundant change

d685908

squadgazzz force-pushed the 2480/partially-fillable-driver-tests branch from 723515a to 9548109 Compare March 14, 2024 19:23

Better tests values

38469a3

squadgazzz force-pushed the 2480/partially-fillable-driver-tests branch from faba3dc to 38469a3 Compare March 14, 2024 19:33

fleupold approved these changes Mar 15, 2024

View reviewed changes

crates/driver/src/tests/cases/protocol_fees.rs Outdated Show resolved Hide resolved

squadgazzz and others added 2 commits March 15, 2024 15:01

Update crates/driver/src/tests/cases/protocol_fees.rs

e955a1e

Co-authored-by: Felix Leupold <felixleupold90@gmail.com>

Formatting

7929000

MartinquaXD approved these changes Mar 18, 2024

View reviewed changes

squadgazzz added 2 commits March 18, 2024 11:59

Minor fix

20cfba2

Merge branch 'main' into 2480/partially-fillable-driver-tests

9d4bfbb

squadgazzz enabled auto-merge (squash) March 18, 2024 13:15

squadgazzz merged commit 413bc1a into main Mar 18, 2024
9 checks passed

squadgazzz deleted the 2480/partially-fillable-driver-tests branch March 18, 2024 13:19

github-actions bot locked and limited conversation to collaborators Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially fillable orders in driver tests #2512

Partially fillable orders in driver tests #2512

squadgazzz commented Mar 12, 2024 •

edited

Loading

fleupold left a comment

fleupold Mar 14, 2024

squadgazzz Mar 14, 2024 •

edited

Loading

fleupold Mar 14, 2024

squadgazzz Mar 14, 2024

fleupold Mar 14, 2024

squadgazzz Mar 14, 2024

fleupold Mar 14, 2024

squadgazzz Mar 14, 2024

fleupold Mar 14, 2024

squadgazzz Mar 14, 2024

fleupold Mar 14, 2024

squadgazzz Mar 14, 2024

fleupold left a comment

squadgazzz commented Mar 14, 2024 •

edited

Loading

fleupold left a comment

MartinquaXD left a comment

MartinquaXD Mar 18, 2024

squadgazzz Mar 18, 2024

MartinquaXD Mar 18, 2024

fleupold Mar 18, 2024

	/// The available amount that can be used from the order.
	///
	/// This amount considers both how much of the order has already been
	/// executed as well as the trader's balance.
	available: TargetAmount,

Partially fillable orders in driver tests #2512

Partially fillable orders in driver tests #2512

Conversation

squadgazzz commented Mar 12, 2024 • edited Loading

Description

Related Issues

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

squadgazzz commented Mar 14, 2024 • edited Loading

fleupold left a comment

Choose a reason for hiding this comment

MartinquaXD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz commented Mar 12, 2024 •

edited

Loading

squadgazzz Mar 14, 2024 •

edited

Loading

squadgazzz commented Mar 14, 2024 •

edited

Loading