[ON HOLD] Don't take protocol fee for in-market limit orders #2188

narcis96 · 2023-12-19T15:19:00Z

ON HOLD - will be split into several PRs:

Add autopilot domain auction #2246
Skip fee policies for in-market orders #2250
Move driver_api to infra #2278
todo [move driver model]

Description

We don't want to take protocol fees for limit orders with in-market price.

Changes

We read the quote from the db and compare the order price with the quote price.

Fixes

#2092

crates/autopilot/src/run_loop.rs

crates/autopilot/src/database/quotes.rs

crates/autopilot/src/run_loop.rs

MartinquaXD · 2023-12-19T16:15:25Z

crates/autopilot/src/shadow.rs

@@ -201,6 +201,7 @@ impl RunLoop {
            self.score_cap,
            self.solve_deadline,
            self.fee_policy.clone(),
+            HashMap::new(),


Would be cool if we could get the correct fee policies as part of the shadow competition as well but that is probably not straight forward since we don't have a DB connection in this case.

@MartinquaXD do you maybe know why we don't have the access to db? for security reasons so we don't end up changing something?
I am worried that onboarding new solvers and checking the protocol fee mechanism will be imposible without db connection. OTOH, we do have staging if necesarry.

I think when the shadow autopilot was written there was no need to store anything in the DB.
And over all I like that we don't need any DB connection for it to run.

so do you think is it worth to modify the code of the shadow to be able to read the order and compute the fee policies or maybe is not worth the investment ?

I think for testing fee policies we might have to add the original quote to the order (or some other indicator that fees should be applied) in /auction. However, if we decide to go that route we should do it in a separate PR.
With that taken care of the only reason the shadow autopilot would need a DB is to store the data we need before calling /settle which I think is outside of the scope of what the shadow environment was initially envisioned to do. Originally it was only supposed to be a test bed to onboard new solvers without risk.

fleupold

We are adding a lot of logic and dependencies to the autopilot runloop due to fees (we are using the run_loops db access to fetch quotes which we pass down into the solve request, we instantiate the run_loop with the fee policies that need to be applied, we have a specific function to compute the fee policies all inside the run_loop).

I wonder how good this will scale. What are the trade-offs of having some fee component which knows about its dependencies (ie a database to look up quotes, the fee policy arguments) and can apply the required fee policy logic to an order?

How do we build this in a way that scales for the other types of fees we want to take eventually? How do we prevent the run-loop from becoming our next "SettlementEncoder" or "SolutionSubmissionStrategy" (ie pieces of code no-one wants to touch anymore because they are so brittle )?

#2157 has some attempt and more context on this discussion I believe.

fleupold · 2023-12-20T06:46:14Z

The other alternative I see could be to reuse is_order_outside_market_price where it is defined today and keep the market order order class label Market for "inside market price" limit orders. I'm not sure about the implications, maybe @sunce86 has some thoughts.

sunce86 · 2023-12-20T09:31:48Z

We are adding a lot of logic and dependencies to the autopilot runloop due to fees (we are using the run_loops db access to fetch quotes which we pass down into the solve request, we instantiate the run_loop with the fee policies that need to be applied, we have a specific function to compute the fee policies all inside the run_loop).

#2157 has some attempt and more context on this discussion I believe.

Yes, we should first merge #2157.

The other alternative I see could be to reuse is_order_outside_market_price where it is defined today and keep the market > order order class label Market for "inside market price" limit orders. I'm not sure about the implications, maybe @sunce86 > has some thoughts.

With regards to network fees, it is too early to do this because we would have Market order with fee_amount = 0. For now, I think it's fine to have is_order_outside_market_price logic in the fee::Policies link and replace it later with order class label.

fleupold · 2023-12-20T09:38:09Z

because we would have Market order with fee_amount = 0

What's the issue with this? I remember the frontend team asking us to provide the labels. Are we depending on the assumption class::Market <=> fee_amount=0 elsewhere in the codebase?

sunce86 · 2023-12-20T10:04:30Z

Wouldn't we end up charging 0 fee in that case?

sunce86

Logic lgtm. Let's just reorganize the code according to code review comments so it's more aligned with the ddd approach. Also, it needs to rebase after #2157 is merged.

crates/autopilot/src/run_loop.rs

fleupold

Concern about the failability of the fee application and that it's part of the critical path of sending the auction rather than building the auction.

crates/autopilot/src/database/quotes.rs

crates/autopilot/src/database/onchain_order_events.rs

crates/autopilot/src/protocol/fee.rs

crates/autopilot/src/run_loop.rs

crates/autopilot/src/protocol/fee.rs

crates/autopilot/src/run_loop.rs

fleupold · 2023-12-27T11:30:50Z

crates/model/src/order.rs

@@ -350,6 +350,11 @@ impl OrderData {
            self.valid_to,
        )
    }
+
+    /// Checks if the order is a market order.
+    pub fn within_market(&self, quote_sell_amount: &U256, quote_buy_amount: &U256) -> bool {


nit: you can extend OrderData inside the order_quoting.rs module to allow using the quote type as an argument here which makes this nicer to read.

pub trait OrderPricing { fn within_market(&self, quote: &Quote) -> bool; } impl OrderPricing for OrderData { /// Checks if the order is a market order. fn within_market(&self, quote: &Quote) -> bool { self.sell_amount.full_mul(quote.buy_amount) >= quote.sell_amount.full_mul(self.buy_amount) } }

This reverts commit b96f18d.

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

crates/autopilot/src/run_loop.rs

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

crates/autopilot/src/infra/database/auction/postgres/mod.rs

fleupold

I think this is a great step in the right direction! Could you please create a separate PR for the refactoring that changes SolvableOrdersCache.current_auction to return the domain model (without any logic changes)?

And then add the fee policy and application logic to the domain model in a separate PR?

Moving driver_model into the new structure should also be a separate PR imo.

crates/autopilot/src/solvable_orders.rs

crates/autopilot/src/boundary/order.rs

crates/autopilot/src/domain/auction/order.rs

crates/autopilot/src/infra/database/auction/mod.rs

fleupold · 2024-01-04T09:39:15Z

crates/autopilot/src/infra/solvers/dto/mod.rs

@@ -0,0 +1,4 @@
+pub mod quote;


Why are all these new files? Can't they be moved from existing dtos? Can we maybe leave that to a different PR (to keep the size of this one manageable)?

fleupold · 2024-01-04T09:42:55Z

crates/autopilot/src/infra/database/auction/mod.rs

+    crate::domain::{self},
+};
+
+pub mod postgres;


I think the module structure should be infra::persistence::postgres::dto::{auction, order, fee, quote}

crates/autopilot/src/database/auction.rs

# Description Subset of changes from #2188 Orderbook's `get_auction()` now returns a new Auction type (let's call it `dto::Auction`). `dto::Auction` is a struct that contains the orders in a format that is used between `autopilot` and `drivers`. Note that it is allowed for `dto::Auction` to contain fields which type belongs to `model`. This is to make sure no bugs were introduced due to serialization/deserialization and to reuse the code. If we want to decouple these dependencies we can do it in a separate PR for easier review. Other than that, I introduced a `domain::Auction` that is built from `dto::Auction` and used within autopilot's `RunLoop`. The idea is to gradually eliminate all type dependencies from `RunLoop` so that it works only with `domain` types, which would make it trivial to move it to domain at the end. ## How to test Existing tests. #TODO Update e2e tests.

# Description A subset of changes from #2188 (part 2) # Changes  - [ ] Move fee policies to `domain` - [ ] No fee policies for in-market limit orders ## How to test Existing tests.

# Description Third part of the #2188 Moves driver_api to a separate `solvers` module in `infra`. Next up, moving the driver dtos into `infra`.

sunce86 · 2024-01-15T15:57:31Z

Closing as the last 4th part of this PR is tracked here: #2286

narcis96 requested a review from a team as a code owner December 19, 2023 15:19

narcis96 force-pushed the skip_in_money_protocol_fee branch from 72f4210 to 31aef39 Compare December 19, 2023 15:26

Don't take protocol fee for in-market limit orders

05181dd

narcis96 force-pushed the skip_in_money_protocol_fee branch from 31aef39 to 05181dd Compare December 19, 2023 15:31

MartinquaXD reviewed Dec 19, 2023

View reviewed changes

fleupold reviewed Dec 19, 2023

View reviewed changes

sunce86 reviewed Dec 20, 2023

View reviewed changes

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved

sunce86 mentioned this pull request Dec 21, 2023

Move out protocol fee into separate file #2157

Merged

sunce86 self-assigned this Dec 23, 2023

sunce86 added 9 commits December 23, 2023 02:21

Merge branch 'main' into skip_in_money_protocol_fee

b9364da

refactor fee

4a6ef01

policy pretty format

851bcfb

fix error handling

b2e1606

remove old function

467f653

is_order_outside_market_price refactor

ae890b3

fix

e4cf9a6

sequential calls

196037f

Merge branch 'main' into skip_in_money_protocol_fee

f7ee21a

fleupold reviewed Dec 26, 2023

View reviewed changes

fixed error handling

c3f2039

sunce86 requested review from fleupold and MartinquaXD December 27, 2023 09:55

fleupold reviewed Dec 27, 2023

View reviewed changes

sunce86 added 3 commits December 29, 2023 13:31

Merge branch 'main' into skip_in_money_protocol_fee

8a7dea5

merge

46e8801

Revert "removed domain"

c229377

This reverts commit b96f18d.

sunce86 added the E:4.2 Protocol Fee Implementation See https://github.com/cowprotocol/pm/issues/29 for details label Dec 29, 2023

fleupold reviewed Dec 30, 2023

View reviewed changes

crates/autopilot/src/domain/fee/policy.rs Outdated Show resolved Hide resolved

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved

removed the caching feature

05a2f8d

fleupold reviewed Dec 30, 2023

View reviewed changes

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved

crates/autopilot/src/domain/fee/policy.rs Outdated Show resolved Hide resolved

sunce86 added 2 commits January 3, 2024 17:03

solvable orders cache works with domain auction

8dcc06d

fix paths

cc31cfa

sunce86 mentioned this pull request Jan 3, 2024

Store fee policy #2118

Merged

2 tasks

squadgazzz reviewed Jan 3, 2024

View reviewed changes

crates/autopilot/src/infra/database/auction/postgres/mod.rs Show resolved Hide resolved

sunce86 added 6 commits January 3, 2024 20:20

refactor optional fields

a415fb2

try fix e2e tests

90c55ab

fix imports

ff9f89c

Merge branch 'main' into skip_in_money_protocol_fee

92d5a9b

fix e2e

70c6827

fix

ebd5847

fleupold reviewed Jan 4, 2024

View reviewed changes

crates/autopilot/src/database/auction.rs Show resolved Hide resolved

camelcase

fea2fda

sunce86 mentioned this pull request Jan 4, 2024

Add autopilot domain auction #2246

Merged

sunce86 changed the title ~~Don't take protocol fee for in-market limit orders~~ [ON HOLD] Don't take protocol fee for in-market limit orders Jan 5, 2024

sunce86 mentioned this pull request Jan 6, 2024

Skip fee policies for in-market orders #2250

Merged

2 tasks

sunce86 mentioned this pull request Jan 12, 2024

Move driver_api to infra #2278

Merged

sunce86 added a commit that referenced this pull request Jan 15, 2024

Move driver_api to infra (#2278)

c86b374

# Description Third part of the #2188 Moves driver_api to a separate `solvers` module in `infra`. Next up, moving the driver dtos into `infra`.

sunce86 mentioned this pull request Jan 15, 2024

chore: Move driver_model into infra #2286

Closed

sunce86 closed this Jan 15, 2024

github-actions bot locked and limited conversation to collaborators Jan 15, 2024

sunce86 deleted the skip_in_money_protocol_fee branch January 20, 2024 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ON HOLD] Don't take protocol fee for in-market limit orders #2188

[ON HOLD] Don't take protocol fee for in-market limit orders #2188

narcis96 commented Dec 19, 2023 •

edited by sunce86

Loading

MartinquaXD Dec 19, 2023

narcis96 Dec 20, 2023

sunce86 Dec 20, 2023

MartinquaXD Dec 20, 2023

narcis96 Dec 21, 2023

MartinquaXD Dec 21, 2023

fleupold left a comment

fleupold commented Dec 20, 2023

sunce86 commented Dec 20, 2023

fleupold commented Dec 20, 2023

sunce86 commented Dec 20, 2023

sunce86 left a comment

fleupold left a comment

fleupold Dec 27, 2023

fleupold left a comment

fleupold Jan 4, 2024

fleupold Jan 4, 2024

sunce86 commented Jan 15, 2024

[ON HOLD] Don't take protocol fee for in-market limit orders #2188

[ON HOLD] Don't take protocol fee for in-market limit orders #2188

Conversation

narcis96 commented Dec 19, 2023 • edited by sunce86 Loading

Description

Changes

Fixes

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

fleupold commented Dec 20, 2023

sunce86 commented Dec 20, 2023

fleupold commented Dec 20, 2023

sunce86 commented Dec 20, 2023

sunce86 left a 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

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

sunce86 commented Jan 15, 2024

narcis96 commented Dec 19, 2023 •

edited by sunce86

Loading