Price improvement tests #2467

squadgazzz · 2024-03-04T18:12:46Z

Description

Fixes #2479

squadgazzz · 2024-03-05T11:45:57Z

Still not quite sure this PR should replace all of the unit tests

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

fleupold · 2024-03-05T14:34:01Z

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

+        quote_sell_amount: quote_sell_amount.into(),
+        quote_buy_amount: quote_buy_amount.into(),
+        executed: quote_buy_amount.into(),


How can we charge any fee here? The quote amount is equal to the executed amount so there is no price improvement. Doesn't this test framework now need a way of differentiating between the quoted and executed price? Also, are we even using an executed amount that is lower than the full amount in any test case (ie are we testing partial fills somewhere)

Overall, I still find these test cases a bit hard to parse (imagine someone trying to debug a failing test case and trying to reproduce the assumptions and math that is going on here).

I don't think we necessarily have to find the nicest abstraction that leads to the least code (e.g. I think we could have a different TestCase setup for price improvement fee vs. surplus fee if one is structurally more complex), but let's please make those tests easier to comprehend.

Before refactoring, now I understand that I am confused about a condition when an order is inside or outside market price.
Can out of market orders be defined as (order.sell + order.fee) * quote.buy < (quote.sell + quote.fee) * order.buy?

I believe so, yes (looking at @sunce86 to confirm)

fleupold · 2024-03-05T14:43:15Z

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

+        order_side: order::Side::Sell,
+        fee_policy,
+        order_sell_amount: order_sell_amount.into(),
+        solver_fee: Some(10000000000000000000u128.into()),


nit: I think it would be clearer to call this network_fee in this context.

I do see now that the price improvement comes from the network fee being different than the quoted fee. This is very clever but not something I'd expect someone maintaining this code in 3 month to just guess without context, so please let's help your future colleague to understand how this test work more intuitively.

fleupold · 2024-03-05T14:45:40Z

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

+    };
+
+    protocol_fee_test_case(test_case).await;
+}


Partially fillable orders are missing.

Also can we add a test that shows shifting surplus into network fees doesn't change the protocol fee charged? E.g. the following two executions pay the same fee (assuming same underlying orders & quotes)

Sell 0.1 ETH receive 3500 DAI, pay 0.9 ETH in network fees

Sell 1 ETH receive 3500 DAI, pay 0 in network fees

Partially fillable orders require some further refactoring. I will create a separate issue for this.

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

Co-authored-by: Felix Leupold <[email protected]>

fleupold

~~Can we capture the follow up item of adding tests for partial fills (I believe this might also be missing for the surplus fee policy)?~~

nvm #2480

fleupold · 2024-03-06T08:55:44Z

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

+        order_side: order::Side::Buy,
+        fee_policy,
+        order_sell_amount,
+        network_fee: Some(2000000000000000000u128.into()),


This will likely be easier to see with #2472, but can you comment in code how the price improvement is achieved (realized network fee lower than quote, leading to X price improvement). This will help understand the expected executed sell amount (compared to the quoted one), but maybe you can add a comment for this expectation as well.

Should be done for each test.

Started writing the comments and found out that the results look pretty weird. Found this code which applies a default surplus factor. Not sure if it needs to be eliminated for the price improvement tests.

Yes, either we need to set it in the test case or make sure it's set to 1. I think your trick with reducing the fee is good enough and allows us to more precisely reason about absolute surplus values (rather than a factor), so I'd slightly prefer setting the factor to 1 (but no strong feelings, as longs as it's clear from the main test how the calculation is carried out)

# Conflicts: # crates/driver/src/tests/cases/protocol_fees.rs # crates/driver/src/tests/setup/blockchain.rs

squadgazzz · 2024-03-08T18:28:28Z

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

+                    } => quote.network_fee,
+                    _ => eth::U256::zero(),
+                };
+                (self.buy * (self.sell - fee) / self.sell) / self.order.surplus_factor


Is it valid?

I think we should always incorporate some network fee here (and not have an if branch just for PriceImprovement fees. That feels way too hacky.

squadgazzz · 2024-03-08T18:28:37Z

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

+                    } => quote.network_fee,
+                    _ => eth::U256::zero(),
+                };
+                (self.sell + fee) * self.order.surplus_factor


Is it valid?

fleupold

I still find the whole setup very hacky and cannot see myself maintaining these tests.

Can we limit our price improvement for now to be only based on the difference in estimted network fee and actually network fee?

Then, a test case just needs a quote (which determines the order amounts and the expected fee), an actual fee amount (which determines the price improvement), a side and the expected amounts.

We shouldn't create if branches deep inside the test setup to cater to one specific test case but instead rethink the setup to make it work with all fee models.

fleupold · 2024-03-09T13:16:06Z

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

+        .sell_amount(test_case.order_sell_amount)
+        .buy_amount(test_case.order_buy_amount);


Why is that set from the order amounts and not from the quote?

fleupold · 2024-03-09T13:18:29Z

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

+        quote,
+        order_sell_amount,
+        order_buy_amount,
+        network_fee: Some(1.ether().into_wei()),


You are still not saying anywhere that this is the little detail that leads to price improvement. This is way to hidden to leave this uncommented.

fleupold · 2024-03-09T13:19:23Z

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

+        network_fee: Some(network_fee),
+        executed: order_sell_amount - network_fee,
+        executed_sell_amount: order_sell_amount,
+        // todo: how to prove the value?


Well how is it calculated? Comment with the math that leads to this value.

Let's maybe start by "where is the price improvement coming from in this test"? I see sell amount and buy amounts and network fees are equal for quote and orders. Unclear why there is any price improvement fee here?

fleupold · 2024-03-09T13:22:47Z

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

+                    } => quote.network_fee,
+                    _ => eth::U256::zero(),
+                };
+                (self.buy * (self.sell - fee) / self.sell) / self.order.surplus_factor


I think we should always incorporate some network fee here (and not have an if branch just for PriceImprovement fees. That feels way too hacky.

fleupold · 2024-03-09T13:22:54Z

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

+                    } => quote.network_fee,
+                    _ => eth::U256::zero(),
+                };
+                (self.sell + fee) * self.order.surplus_factor


fleupold · 2024-03-09T14:02:11Z

Can we limit our price improvement for now to be only based on the difference in estimated network fee and actually network fee?

Actually maybe this wasn't a good idea to begin with (assuming it's the reason for the conditional refactoring). In this case, you can also work with explicit order and quote amounts and just assume 0 network everywhere.

fleupold · 2024-03-10T14:00:07Z

@squadgazzz I created #2510 as a suggestion on how I would implement these tests. Can you take a look, tell me what you think about this approach and if it works and if convert the remaining tests?

squadgazzz · 2024-03-11T17:26:53Z

#2510

Price improvement tests

9b3a0ee

squadgazzz force-pushed the 2287/price-improvement-tests branch from 5042984 to 666f4fe Compare March 5, 2024 09:34

Adjusted values

dd233e2

squadgazzz force-pushed the 2287/price-improvement-tests branch 2 times, most recently from e103df2 to dd233e2 Compare March 5, 2024 11:08

squadgazzz marked this pull request as ready for review March 5, 2024 11:43

squadgazzz requested a review from a team as a code owner March 5, 2024 11:43

fleupold reviewed Mar 5, 2024

View reviewed changes

squadgazzz and others added 2 commits March 5, 2024 17:28

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

3f7bf5e

Co-authored-by: Felix Leupold <[email protected]>

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

4b70eb7

Co-authored-by: Felix Leupold <[email protected]>

squadgazzz force-pushed the 2287/price-improvement-tests branch from 4c68e5c to 4d4ad2f Compare March 5, 2024 19:57

Adjusted values

38135e8

squadgazzz force-pushed the 2287/price-improvement-tests branch from 4d4ad2f to 38135e8 Compare March 5, 2024 20:45

Explicit fee amounts

287c1f9

squadgazzz force-pushed the 2287/price-improvement-tests branch from 2a20508 to 287c1f9 Compare March 5, 2024 20:59

Network fee

08c7027

fleupold approved these changes Mar 6, 2024

View reviewed changes

Adjusted fees

758ebde

m-lord-renkse approved these changes Mar 6, 2024

View reviewed changes

squadgazzz marked this pull request as draft March 6, 2024 17:05

Adjusted values

3a80fe6

squadgazzz force-pushed the 2287/price-improvement-tests branch from 2390320 to 3a80fe6 Compare March 7, 2024 18:59

squadgazzz added 5 commits March 8, 2024 16:49

Order surplus factor=1

75d99c1

Merge branch 'main' into 2287/price-improvement-tests

47dd239

# Conflicts: # crates/driver/src/tests/cases/protocol_fees.rs # crates/driver/src/tests/setup/blockchain.rs

Fixed values

1a47343

Refactoring

9daf971

Wrong comment

886fc2f

squadgazzz marked this pull request as ready for review March 8, 2024 18:25

squadgazzz commented Mar 8, 2024

View reviewed changes

fleupold requested changes Mar 9, 2024

View reviewed changes

fleupold mentioned this pull request Mar 10, 2024

Fee tests #2510

Merged

5 tasks

sunce86 mentioned this pull request Mar 11, 2024

chore: Revert commented out driver test #2511

Closed

squadgazzz closed this Mar 11, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Price improvement tests #2467

Price improvement tests #2467

squadgazzz commented Mar 4, 2024 •

edited

Loading

squadgazzz commented Mar 5, 2024 •

edited

Loading

fleupold Mar 5, 2024

squadgazzz Mar 5, 2024 •

edited

Loading

fleupold Mar 6, 2024

fleupold Mar 5, 2024

fleupold Mar 5, 2024

squadgazzz Mar 5, 2024

fleupold left a comment •

edited

Loading

fleupold Mar 6, 2024

squadgazzz Mar 6, 2024

fleupold Mar 6, 2024

squadgazzz Mar 8, 2024

fleupold Mar 9, 2024

squadgazzz Mar 8, 2024

fleupold Mar 9, 2024

fleupold left a comment

fleupold Mar 9, 2024

fleupold Mar 9, 2024

fleupold Mar 9, 2024

fleupold Mar 9, 2024

fleupold Mar 9, 2024

fleupold commented Mar 9, 2024

fleupold commented Mar 10, 2024

squadgazzz commented Mar 11, 2024

		.sell_amount(test_case.order_sell_amount)
		.buy_amount(test_case.order_buy_amount);

Price improvement tests #2467

Price improvement tests #2467

Conversation

squadgazzz commented Mar 4, 2024 • edited Loading

Description

squadgazzz commented Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

squadgazzz Mar 5, 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

fleupold left a comment • 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

fleupold 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

Choose a reason for hiding this comment

fleupold commented Mar 9, 2024

fleupold commented Mar 10, 2024

squadgazzz commented Mar 11, 2024

squadgazzz commented Mar 4, 2024 •

edited

Loading

squadgazzz commented Mar 5, 2024 •

edited

Loading

squadgazzz Mar 5, 2024 •

edited

Loading

fleupold left a comment •

edited

Loading