Price improvement fee policy in driver #2375

squadgazzz · 2024-02-06T17:49:29Z

Description

Closes task n3 from the #2287

How to test

TBD

crates/driver/openapi.yml

MartinquaXD · 2024-02-07T08:22:58Z

crates/driver/src/domain/competition/order/fees.rs

+        factor: f64,
+        max_volume_factor: f64,


Doc comments on each field would be nice.

crates/driver/src/domain/competition/solution/fee.rs

Co-authored-by: Martin Beckmann <[email protected]>

sunce86 · 2024-02-07T14:21:10Z

crates/driver/src/domain/competition/order/fees.rs

+    PriceImprovement {
+        /// Factor of price improvement the protocol charges as a fee.
+        /// Price improvement is the difference between executed price and the
+        /// best quote


Price improvement is the difference between executed price and the best quote or limit price, whichever is better for the user.

sunce86 · 2024-02-07T14:23:26Z

crates/driver/src/domain/competition/order/fees.rs

+        /// E.g. if a user received 2000USDC for 1ETH while having a best quote
+        /// of 1990USDC, their surplus is 10USDC. A factor of 0.1
+        /// requires the solver to pay 1USDC to the protocol for
+        /// settling this order.


E.g. if a user received 2000USDC for 1ETH while having a best quote of 1995USDC and limit price of 1990USDC, their surplus is 10USDC while the price improvement is 5USDC. A factor of 0.1 requires the solver to pay 0.5USDC to the protocol for settling this order. In case the best quote was 1990USDC while the limit price was 1995USDC, the solver should also pay 0.5USDC to the protocol.

crates/driver/src/domain/competition/solution/fee.rs

sunce86 · 2024-02-07T14:42:30Z

crates/driver/src/domain/competition/solution/fee.rs

-                tracing::debug!(uid=?self.order().uid, fee_from_surplus=?fee_from_surplus, fee_from_volume=?fee_from_volume, protocol_fee=?(std::cmp::min(fee_from_surplus, fee_from_volume)), executed=?self.executed(), surplus_fee=?self.surplus_fee(), "calculated protocol fee");
-                Ok(std::cmp::min(fee_from_surplus, fee_from_volume))
+                let sell_amount = quote.sell_amount.max(self.order().sell.amount.0);
+                let buy_amount = quote.buy_amount.min(self.order().buy.amount.0);


I'm super scared of these two lines, because they assume that the quote.sell_amount = order.sell_amount for sell orders and that the quote.buy_amount = order.buy_amount for buy order, which may or may not be the case.

I think the best approach is to make sure this is true is:

Add the fee amount I mentioned in the previous pr Price improvemenet fee policy autopilot domain #2378 (comment) and include it in the quote exchange rate. Example:
quote sell: 10
quote buy : 9
quote fee: 1 (denominated in sell)

exchange rate is 11:9
2. Scale the exchange rate (make the quote.sell_amount = order.sell_amount for sell orders and make quote.buy_amount = order.buy_amount for buy orders).

With that, I believe this might work but needs to be confirmed with a range of e2e tests.

I believe this is my final proposition to fix the sell and buy amounts that are forwarded to function:

// adjust the quote sell amount to include the fee quote.sell = quote.sell + quote.fee // order amounts let order_sell = self.order().sell_amount; let order_buy = self.order().buy_amount; // limit amounts used for surplus calculation let (sell_amount, buy_amount) = match self.order().side { Side:Sell => (order.sell, max(order.buy, quote.buy * (order.sell / quote.sell))), Side:Buy => (min(order.sell, quote.sell * (order.buy / quote.buy)), order.buy) }

Here is an example to understand the scaling:

Order.sell: 20
Order.buy: 19

Quote.sell: 21
Quote: buy:18
Quote.fee: 1

Quote.sell = 21 + 1 = 22

This is now the exchange rate we need to compare the order limit amounts with.
Quote exchange rate: 22:18

Sell order:
We want to scale the quote sell amounts so that the quote.sell and order.sell are equal, so that we can safely compare their buy amounts for surplus calculation. We also need to scale the quote buy amount so that the quote exchange rate remains the same:
Quote.sell = 22 * (20 / 22) = 20 -> now it’s equal to order sell
Quote.buy = 18 * (20 / 22) = 16.364

Buy order:
We want to scale the quote buy amounts so that the quote.buy and order.buy are equal, so that we can safely compare their sell amounts for surplus calculation. We also need to scale the quote sell amount so that the quote exchange rate remains the same:
Quote.sell = 22 * (19 / 18) = 23.222
Quote.buy = 18 * (19 / 18) = 19 -> now it’s equal to order buy

crates/driver/src/domain/competition/order/fees.rs

fleupold · 2024-02-07T11:45:10Z

crates/driver/src/domain/competition/order/fees.rs

+        /// Factor of price improvement the protocol charges as a fee.
+        /// Price improvement is the difference between executed price and the
+        /// best quote
+        ///


Very redundant comment

Updated the comment. Is it still redundant? Tried to align with other policies' comments.

crates/driver/src/domain/competition/solution/fee.rs

fleupold · 2024-02-07T11:54:25Z

crates/driver/src/domain/competition/solution/fee.rs

+                let sell_amount = quote.sell_amount.max(self.order().sell.amount.0);
+                let buy_amount = quote.buy_amount.min(self.order().buy.amount.0);


Is it guaranteed that one of the two is always equal to the order amounts? Otherwise it's possible that we take one value from the quote and another value from the order. We may have to check both ratios and commit to using either buy and sell from quote or buy and sell from order.

I believe this is resolved with #2375 (comment)

sunce86

Nice.
With added unit tests we made sure our idea around reference amounts might work.

Do you think we can now add a test suite for the calculate_fee function? Or maybe we should test that through the driver tests instead.

crates/driver/src/domain/competition/order/fees.rs

crates/driver/src/domain/competition/solution/fee.rs

squadgazzz · 2024-02-08T17:49:56Z

Do you think we can now add a test suite for the calculate_fee function? Or maybe we should test that through the driver tests instead.

Yeah, I'd rather concentrate on the integration tests now.

sunce86

Lg!

sunce86 · 2024-02-09T12:56:29Z

crates/driver/src/domain/competition/solution/fee.rs

@@ -192,6 +230,40 @@ fn apply_factor(amount: eth::U256, factor: f64) -> Result<eth::U256, Error> {
        / 10000)
 }

+fn adjusted_price_improvement_amounts(


nit: Some comments that sumarize what this function does and why, would be nice.
Also, fn name is long but not clear enough.

I would suggest explaining this in terms of limit_amounts, which are taken either from order or quote amounts. Also, explain why we do scaling - because in order to meaningfully compare buy amounts, for example, sell amounts for order and quote need to be equal.

maybe even providing an example or referring to exact unit test. I often get back to this example to remind myself of the logic.

maybe also renaming reference amounts to limit amounts

sunce86 · 2024-02-09T13:04:31Z

crates/driver/src/infra/api/routes/solve/dto/auction.rs

+        competition::order::fees::Quote {
+            sell: eth::Asset {
+                amount: eth::TokenAmount(self.sell_amount),
+                token: eth::TokenAddress(eth::ContractAddress(sell_token)),


nit:

amount: self.sell_amount.into() token: sell_token.into()

sunce86 · 2024-02-12T08:52:01Z

Can we merge this PR as I have a "rank by surplus" code that conflicts with some of the changes of this PR?

We have e2e tests that confirm this change doesn't break existing Surplus protocol fee. For price improvement protocol fee, test can be part of a separate PR.

MartinquaXD · 2024-02-12T09:47:27Z

crates/driver/src/domain/competition/solution/fee.rs

+
+        assert_eq!(
+            sell_amount, order_sell_amount,
+            "Sell amount should be taken from the quote for sell orders in market price."


With this comment I would expect the assertion to be sell_amount == quote_sell_amount.

squadgazzz · 2024-02-12T11:21:41Z

Can we merge this PR as I have a "rank by surplus" code that conflicts with some of the changes of this PR?

We have e2e tests that confirm this change doesn't break existing Surplus protocol fee. For price improvement protocol fee, test can be part of a separate PR.

Merging it, the remaining comments will be addressed in a separate PR.

Price improvement fee policy in driver

c9c50a8

squadgazzz marked this pull request as ready for review February 6, 2024 18:49

squadgazzz requested a review from a team as a code owner February 6, 2024 18:49

MartinquaXD approved these changes Feb 7, 2024

View reviewed changes

squadgazzz and others added 3 commits February 7, 2024 09:35

Update crates/driver/openapi.yml

a2c6c1c

Co-authored-by: Martin Beckmann <[email protected]>

More docs

b0f1954

Nits

a7c8a46

sunce86 reviewed Feb 7, 2024

View reviewed changes

fleupold reviewed Feb 7, 2024

View reviewed changes

squadgazzz added 4 commits February 7, 2024 18:13

Docs fix

5943859

Add quote fee

7311fde

Calculation logic fix

d49c707

Refactoring

368026f

squadgazzz force-pushed the price-improvement-policy-driver branch from 896e6e8 to a154a05 Compare February 8, 2024 13:50

Unit test

dcf40f0

squadgazzz force-pushed the price-improvement-policy-driver branch from a154a05 to dcf40f0 Compare February 8, 2024 13:51

squadgazzz added 3 commits February 8, 2024 13:53

Formatting

c4bff78

Comments

6493f9a

Fix buy branch

a11312c

sunce86 reviewed Feb 8, 2024

View reviewed changes

Tests adjustement

5f2f872

squadgazzz force-pushed the price-improvement-policy-driver branch from afc761c to 5f2f872 Compare February 8, 2024 17:48

Quote asset

8a0cbf6

squadgazzz requested review from fleupold and sunce86 February 9, 2024 12:56

sunce86 approved these changes Feb 9, 2024

View reviewed changes

MartinquaXD reviewed Feb 12, 2024

View reviewed changes

Merge branch 'main' into price-improvement-policy-driver

c5b9887

squadgazzz enabled auto-merge (squash) February 12, 2024 11:22

squadgazzz merged commit 72cac8c into main Feb 12, 2024
9 checks passed

squadgazzz deleted the price-improvement-policy-driver branch February 12, 2024 11:26

github-actions bot locked and limited conversation to collaborators Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Price improvement fee policy in driver #2375

Price improvement fee policy in driver #2375

squadgazzz commented Feb 6, 2024 •

edited

Loading

MartinquaXD Feb 7, 2024

squadgazzz Feb 7, 2024

sunce86 Feb 7, 2024

sunce86 Feb 7, 2024

sunce86 Feb 7, 2024

sunce86 Feb 8, 2024 •

edited

Loading

sunce86 Feb 8, 2024 •

edited

Loading

fleupold Feb 7, 2024

squadgazzz Feb 8, 2024

fleupold Feb 7, 2024

sunce86 Feb 9, 2024

sunce86 left a comment •

edited

Loading

squadgazzz commented Feb 8, 2024

sunce86 left a comment

sunce86 Feb 9, 2024

sunce86 Feb 9, 2024

sunce86 commented Feb 12, 2024

MartinquaXD Feb 12, 2024

squadgazzz commented Feb 12, 2024

		let sell_amount = quote.sell_amount.max(self.order().sell.amount.0);
		let buy_amount = quote.buy_amount.min(self.order().buy.amount.0);

Price improvement fee policy in driver #2375

Price improvement fee policy in driver #2375

Conversation

squadgazzz commented Feb 6, 2024 • edited Loading

Description

How to test

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

sunce86 Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

sunce86 Feb 8, 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

sunce86 left a comment • edited Loading

Choose a reason for hiding this comment

squadgazzz commented Feb 8, 2024

sunce86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 commented Feb 12, 2024

Choose a reason for hiding this comment

squadgazzz commented Feb 12, 2024

squadgazzz commented Feb 6, 2024 •

edited

Loading

sunce86 Feb 8, 2024 •

edited

Loading

sunce86 Feb 8, 2024 •

edited

Loading

sunce86 left a comment •

edited

Loading