PriceImprovement fee policy #2277

squadgazzz · 2024-01-12T18:10:33Z

Description

Partly closes #2287

How to test

TBD

squadgazzz · 2024-01-12T18:11:07Z

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

+                max_volume_factor: _,
+                quote: _,
+            }) => {
+                todo!()


How the quote should be used here?

I think the cleanest solution would probably to adjust fee_from_surplus() such that it computes the surplus compared to a given reference price. Then you could pass in the user signed limit prices for FeePolicy::Surplus or the quoted amounts for FeePolicy::PriceImprovement.

Let me know if I got it wrong 4a0e5ff

Yes, this is exactly what I had in mind. I would just call the new arguments sth like reference_(buy|sell)_amount and add a doc comment explaining the meaning of the reference amounts.

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

crates/autopilot/src/domain/fee/mod.rs

crates/autopilot/src/arguments.rs

crates/autopilot/src/domain/fee/mod.rs

crates/driver/openapi.yml

MartinquaXD · 2024-01-15T08:16:01Z

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

+                max_volume_factor: _,
+                quote: _,
+            }) => {
+                todo!()


I think the cleanest solution would probably to adjust fee_from_surplus() such that it computes the surplus compared to a given reference price. Then you could pass in the user signed limit prices for FeePolicy::Surplus or the quoted amounts for FeePolicy::PriceImprovement.

fleupold

Good first step!

Note, that there is an ongoing discussion whether it should be called PriceImprovement or QuoteDeviation (https://cowservices.slack.com/archives/C05N1FNP3MX/p1704971316558629)

crates/autopilot/src/arguments.rs

fleupold · 2024-01-15T09:54:38Z

crates/autopilot/src/infra/persistence/dto/order.rs

+    PriceImprovement {
+        factor: f64,
+        max_volume_factor: f64,
+        quote: Quote,


I really like that the quote becomes part of the fee policy enum. This makes it very clear how to use it.

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

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

crates/autopilot/src/domain/fee/mod.rs

crates/driver/openapi.yml

sunce86 · 2024-01-22T22:37:45Z

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

-            }
+            }) => self.calculate_fee(
+                self.order().sell.amount.0,
+                self.order().buy.amount.0,


I am not sure this works for scaling partially fillable sell orders. Will need to go over it once more tomorrow, with a fresh mind.

database/sql/V058__add_price_improvement_fee_policy_kind.sql

# Conflicts: # crates/autopilot/src/domain/fee/mod.rs

fleupold

I think it would actually be great if we could get driver based integration tests running before adding more fee policies.

fleupold · 2024-01-24T12:57:27Z

crates/autopilot/src/domain/fee/mod.rs

@@ -90,3 +95,48 @@ pub enum Policy {
        factor: f64,
    },
 }
+
+#[derive(Debug)]
+pub enum PolicyRaw {


I'm a bit surprised by this type. I don't find the word "raw" very descriptive nor appealing and it's missing documentation for why it exists (on top of FeePolicy). It looks like the only difference is the quote (which in only part of the other struct).

Is there any way we can get around defining two almost identical domain types or can we differentiate better between the two?

Maybe PolicyRaw is more like an incomplete Policy (maybe a PolicyBuilder), that can be converted into a full policy .with(quote)

Yeah, totally makes sense. Renamed both the struct and the function.

fleupold · 2024-01-24T12:59:20Z

crates/autopilot/src/domain/fee/mod.rs

+}
+
+impl PolicyRaw {
+    pub fn to_domain(&self, quote: &domain::Quote) -> Policy {


isn't PolicyRaw already a domain object?

#2277 (comment)

fleupold · 2024-01-24T13:07:20Z

crates/autopilot/src/solvable_orders.rs

+                        let protocol_fees = self.protocol_fee.get(&order, quote);
+                        Some(boundary::order::to_domain(order, protocol_fees))


I think it would read slightly nicer if ProtocolFee would return the domain order type.

fleupold · 2024-01-24T13:08:24Z

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

+                quote.sell_amount,
+                quote.buy_amount,


I think here we need to use whatever is better (quote or limit price)

fleupold · 2024-01-24T13:10:47Z

database/README.md

+ sell_amount              | numeric                   |          | quote's sell amount
+ buy_amount               | numeric                   |          | quote's buy amount


Don't we already store this information in order_quotes? Do we want to duplicate it?

I think these will be different values defining user price exchange rate, compared to order_quotes where the amm price exchange rate is saved.
The user price exchange rate takes fee into account.

Here I wrote about an example.

sunce86 · 2024-01-25T14:14:14Z

I think we are somewhat close to finishing this PR (at least roughly we know what needs to be done). However, it's hard to review it with high confidence that it's correct and that it doesn't break the existing functionality (since it tackles most of the tasks from #2287).

Do you think it makes sense to split this PR into multiple PRs and try to write tests for each of them?

fleupold · 2024-01-25T14:51:00Z

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);


Isn't this turned around? We want to take what's better for the trader between quote and limit (where I have to sell less and where I get to buy more)? I really think we should do a test case to cover this.

fleupold · 2024-01-25T14:53:13Z

crates/autopilot/src/domain/fee/mod.rs

    fee_policy_skip_market_orders: bool,
 }

 impl ProtocolFee {
-    pub fn new(policy: Policy, fee_policy_skip_market_orders: bool) -> Self {
+    pub fn new(policy: PolicyBuilder, fee_policy_skip_market_orders: bool) -> Self {


Sorry for being flip floppy here, but now that I look at it isn't this more like a config object (without any need for logic on it)? This way we could leave all logic inside the ProtocolFee component?

sunce86

Can we merge the changes from main branch, so we can make sure the quote_fee_amount is included in the Quote we send to the driver (it should be added to the quote_sell_amount)?

# Conflicts: # crates/autopilot/src/domain/fee/mod.rs # crates/autopilot/src/solvable_orders.rs

github-actions · 2024-01-29T19:35:21Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

1 out of 2 committers have signed the CLA.
✅ @squadgazzz
❌ @Ilia
ilia seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request}

github-actions · 2024-02-06T00:06:13Z

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

# Conflicts: # crates/autopilot/src/domain/fee/mod.rs

github-actions · 2024-02-06T15:32:25Z

Reminder: Please update the DB Readme.

Caused by:

database/sql/V062__add_price_improvement_fee_policy_kind.sql

squadgazzz · 2024-02-06T18:48:41Z

Splitting the PR into parts to properly follow #2287

squadgazzz added 2 commits January 12, 2024 17:49

PriceImprovement policy in autopilot

c1a60c0

PriceImprovement policy in driver

d68cfbd

squadgazzz commented Jan 12, 2024

View reviewed changes

todo

ce00a5b

squadgazzz marked this pull request as ready for review January 12, 2024 18:21

squadgazzz requested a review from a team as a code owner January 12, 2024 18:21

squadgazzz commented Jan 12, 2024

View reviewed changes

crates/driver/src/domain/competition/order/fees.rs Outdated Show resolved Hide resolved

squadgazzz commented Jan 12, 2024

View reviewed changes

crates/autopilot/src/domain/fee/mod.rs Show resolved Hide resolved

squadgazzz changed the title ~~Price improvement policy~~ PriceImprovement fee policy Jan 12, 2024

MartinquaXD reviewed Jan 15, 2024

View reviewed changes

fleupold approved these changes Jan 15, 2024

View reviewed changes

squadgazzz and others added 12 commits January 15, 2024 19:09

Update crates/autopilot/src/arguments.rs

e901812

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

Update crates/driver/openapi.yml

5406807

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

Minor review fixes

ab00441

Formatting

ebf8dd9

Implement price improvement fee calculation

4a0e5ff

Merge branch 'main' into price-improvement-policy

5a153aa

DB persistence

0cbbb00

Orderbook OpenAPI

6bc3de6

Missing files

5d4e32d

Test fix

b4e4b5a

Warn about skipped order

69a2297

PolicyRaw dto

a1444b1

sunce86 reviewed Jan 22, 2024

View reviewed changes

squadgazzz added 3 commits January 23, 2024 12:28

Make quote mandatory

7f1a7c5

Naming and docs

50520ae

Merge branch 'main' into price-improvement-policy

2376f79

# Conflicts: # crates/autopilot/src/domain/fee/mod.rs

fleupold reviewed Jan 24, 2024

View reviewed changes

squadgazzz added 2 commits January 24, 2024 18:52

PolicyRaw -> PolicyBuilder

eb08cf4

Fee calculation fix

f629e61

Construct domain::Order inside the ProtocolFee

e3a27a0

fleupold reviewed Jan 25, 2024

View reviewed changes

sunce86 reviewed Jan 27, 2024

View reviewed changes

ilia and others added 2 commits January 29, 2024 19:11

Merge branch 'main' into price-improvement-policy

ed9ff89

# Conflicts: # crates/autopilot/src/domain/fee/mod.rs # crates/autopilot/src/solvable_orders.rs

Imports

b48ecdc

github-actions bot added the stale label Feb 6, 2024

sunce86 removed the stale label Feb 6, 2024

squadgazzz added 2 commits February 6, 2024 15:24

Merge branch 'main' into price-improvement-policy

8fd95e4

# Conflicts: # crates/autopilot/src/domain/fee/mod.rs

Fix after merge

c582f74

squadgazzz added 3 commits February 6, 2024 15:33

Update migration version

8ca5b05

e2e

ba9a399

Docs

603eb8b

This was referenced Feb 6, 2024

Price improvement fee policy in driver #2375

Merged

Price improvement fee policy DB changes #2376

Closed

squadgazzz closed this Feb 6, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PriceImprovement fee policy #2277

PriceImprovement fee policy #2277

squadgazzz commented Jan 12, 2024 •

edited

Loading

squadgazzz Jan 12, 2024 •

edited

Loading

MartinquaXD Jan 15, 2024

squadgazzz Jan 15, 2024

MartinquaXD Jan 15, 2024

MartinquaXD Jan 15, 2024

fleupold left a comment

fleupold Jan 15, 2024

sunce86 Jan 22, 2024

fleupold left a comment

fleupold Jan 24, 2024

squadgazzz Jan 24, 2024

fleupold Jan 24, 2024

squadgazzz Jan 24, 2024

fleupold Jan 24, 2024

squadgazzz Jan 24, 2024

fleupold Jan 24, 2024

squadgazzz Jan 24, 2024

fleupold Jan 24, 2024

sunce86 Jan 24, 2024

sunce86 commented Jan 25, 2024 •

edited

Loading

fleupold Jan 25, 2024

fleupold Jan 25, 2024

sunce86 left a comment •

edited

Loading

github-actions bot commented Jan 29, 2024

github-actions bot commented Feb 6, 2024

github-actions bot commented Feb 6, 2024 •

edited

Loading

squadgazzz commented Feb 6, 2024

		let protocol_fees = self.protocol_fee.get(&order, quote);
		Some(boundary::order::to_domain(order, protocol_fees))

		sell_amount \| numeric \| \| quote's sell amount
		buy_amount \| numeric \| \| quote's buy amount

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

PriceImprovement fee policy #2277

PriceImprovement fee policy #2277

Conversation

squadgazzz commented Jan 12, 2024 • edited Loading

Description

How to test

squadgazzz Jan 12, 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

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

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 commented Jan 25, 2024 • edited Loading

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

github-actions bot commented Jan 29, 2024

github-actions bot commented Feb 6, 2024

github-actions bot commented Feb 6, 2024 • edited Loading

squadgazzz commented Feb 6, 2024

squadgazzz commented Jan 12, 2024 •

edited

Loading

squadgazzz Jan 12, 2024 •

edited

Loading

sunce86 commented Jan 25, 2024 •

edited

Loading

sunce86 left a comment •

edited

Loading

github-actions bot commented Feb 6, 2024 •

edited

Loading