Price improvemenet fee policy autopilot domain #2378

squadgazzz · 2024-02-06T18:29:15Z

Description

Closes task n2 from #2287

fleupold

I'm still not entirely happy about the factoring here (it seems somewhat convoluted and not very straightforward to me).

Here is my mental model:

As far as the domain is concerned there is one component responsible for all logic of how fees are applied to orders (fee.apply(order, quote) -> OrderWithFees)
This fee component itself can apply many different types of fee (policies) to a single order (UI integration fee, price improvement fee, etc) based on what the order specifies (ie in its AppData, how much CoW tokens the user holds, etc.) and what the autopilot has been configured with.
For this, each fee policy (builder) can apply itself to an order policy.with(order) -> Option<Fee>
The fee component collects all fees into the order and returns it.

I'm not entirely sure if factoring 3 out into a builder is overkill (we can remove it for now), but I'm worried this logic will explode in complexity as soon as we add more advanced fees (like a volume fee that is specified by the interface or fee discounts based on your CoW token holdings)

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

fleupold · 2024-02-07T10:42:05Z

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

            boundary::OrderClass::Market => {
                if self.fee_policy_skip_market_orders {
                    vec![]
                } else {
-                    vec![self.policy]
+                    vec![self.policy_builder.build_with(quote)]


Shouldn't the information whether or not to apply a fee for market order be part of the fee policy (builder) and not part of this component?

squadgazzz · 2024-02-07T13:18:35Z

Not fully following the intention and how the current structure should be changed.

I'm still not entirely happy about the factoring here (it seems somewhat convoluted and not very straightforward to me).

Here is my mental model:

As far as the domain is concerned there is one component responsible for all logic of how fees are applied to orders (fee.apply(order, quote) -> OrderWithFees)

I thought that domain::fee::ProtocolFee is that component which is serving the intended functionality.

This fee component itself can apply many different types of fee (policies) to a single order (UI integration fee, price improvement fee, etc) based on what the order specifies (ie in its AppData, how much CoW tokens the user holds, etc.) and what the autopilot has been configured with.

ProtocolFee::apply function does something similar, doesn't it?

For this, each fee policy (builder) can apply itself to an order policy.with(order) -> Option<Fee>

The fee component collects all fees into the order and returns it.

Made an attempt(c519fb8) to follow this logic. Not sure it addresses all the intentions.

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

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

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

fleupold

Not fully following the intention and how the current structure should be changed.

Right, I realize that I asked for a bunch of refactorings and am now not happy with it (not really constructive review performance on my end). Feel free to propose something completely different (I just feel we now have a very complex control flow for not even yet very complex fee models, but those are around the corner).

As long as we keep the details private to the domain::fee component we can also refine it later.

fleupold · 2024-02-07T14:51:15Z

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

-    policy: Policy,
-    fee_policy_skip_market_orders: bool,
+    policy_builder: PolicyBuilder,
+    skip_market_orders: bool,


This boolean is bothering me here. Skip market orders is something specific to the Surplus fee (we currently charge a surplus fee but skip market orders) and should thus be specified there and considered when we are trying to apply a Surplus FeePolicy to an order.

So, should this function be also changed? Currently, it checks for this flag regardless of the policy kind.

Yes, I think conceptually this flag applies only to the Surplus policy. Once price improvement is implemented, I think we are likely going to use PriceImprovement over Surplus for all order types. I'm not sure though how we would configure things if price improvement for swap orders has different volume factors than for out of market limit orders (I think such a config may be desirable, to be checked with @olgafetisova).

Moved to Surplus

fleupold · 2024-02-07T15:00:14Z

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

+}
+
+#[derive(Debug)]
+pub enum PolicyBuilder {


I think we should make this class private and create it in the main Fee component from the infra config. It might also be nicer to have every variant be there own sub-struct so that we can implement slightly different build behaviors (e.g. surplus and volume don't need the quote).

Then the main apply function would become something like

self.fee_variants.filter_map(|variant| { match variant { FeeVariant::Surplus(variant) => variant.apply(order), FeeVariant::PriceImprovement(variant) => variant.apply(order, quote), ... } }

# Conflicts: # crates/autopilot/src/arguments.rs

fleupold

Looks good to me. Some remaining comments on how to split the code into files (to allow for succinct and nice to read naming), reducing visibility of internal structs as much as possible and avoiding to share logic which is likely not correlated, but nothing blocking.

fleupold · 2024-02-09T07:52:49Z

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

+}
+
+#[derive(Debug)]
+struct SurplusPolicy {


nit: we are trying to keep struct names short for lexicographic appeal. I think they could just be called Surplus (without policy) but if we want the Policy part on all subtypes, I think it would be prettier to create a new private submodule policy and reference them as

policy::Surplus, policy::PriceImprovement etc

fleupold · 2024-02-09T07:57:22Z

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

+    }
+}
+
+fn can_policy_apply(order: &boundary::Order, quote: &domain::Quote) -> bool {


I wouldn't share this method across fee policies (it seems coincidental that there is some shared logic between).

If I understand the current logic correctly, volume fees can only be applied to out of market limit orders, which doesn't seem correct to me.

Also for PriceImprovement, shouldn't this be applied mainly for in market price orders?

Let's just make this logic explicit for every fee policy.

fleupold · 2024-02-09T08:01:30Z

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

+}
+
+impl SurplusPolicy {
+    pub fn apply(&self, order: &boundary::Order, quote: &domain::Quote) -> Option<Policy> {


no need to be pub

Since it was moved to its own private submodule, this requires the function to be public.

fleupold · 2024-02-09T08:06:04Z

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

 pub struct ProtocolFee {
-    policy: Policy,
-    fee_policy_skip_market_orders: bool,
+    policy_builder: PolicyBuilder,


Since this is a private type now, if we move it to a policy submodule we could even call it Policy without having a name collision (the way it turned out now it's not really following the Builder pattern anymore)

sunce86

Lookin' nice!

sunce86 · 2024-02-09T12:22:33Z

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

+pub struct Quote {
+    pub sell_amount: U256,
+    pub buy_amount: U256,
+    pub fee: U256,


nit: maybe add a comment that the fee is always expressed in the sell token

squadgazzz added 2 commits February 6, 2024 18:13

Price improvemenet fee policy autopilot argument

6be7367

Price improvemenet fee autopilot domain

5360135

squadgazzz changed the base branch from main to 2287/autopilot-argument February 6, 2024 18:29

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

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

squadgazzz changed the title ~~2287/autopilot domain~~ Price improvemenet fee policy autopilot domain Feb 6, 2024

MartinquaXD approved these changes Feb 7, 2024

View reviewed changes

fleupold reviewed Feb 7, 2024

View reviewed changes

squadgazzz added 2 commits February 7, 2024 11:53

Naming

bf7b8db

Refactoring

c519fb8

Base automatically changed from 2287/autopilot-argument to main February 7, 2024 13:39

sunce86 reviewed Feb 7, 2024

View reviewed changes

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

crates/autopilot/src/infra/persistence/dto/fee_policy.rs Outdated Show resolved Hide resolved

Wrong kind fix

3e53f6b

sunce86 mentioned this pull request Feb 7, 2024

Price improvement fee policy in driver #2375

Merged

sunce86 reviewed Feb 7, 2024

View reviewed changes

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

fleupold reviewed Feb 7, 2024

View reviewed changes

squadgazzz added 7 commits February 7, 2024 16:11

Review comments

e2adf86

Make PolicyBuilder private

cc74e36

Merge branch 'main' into 2287/autopilot-domain

021e3b6

# Conflicts: # crates/autopilot/src/arguments.rs

Use skip market orders flag for surplus only

a7352d4

Logic fix

bd72dcd

Refactoring

284e892

Fix

0016bb9

fleupold approved these changes Feb 9, 2024

View reviewed changes

squadgazzz added 2 commits February 9, 2024 08:38

policy mod

cf8c7c3

Fixed policy logic

0bb745f

sunce86 approved these changes Feb 9, 2024

View reviewed changes

squadgazzz added 2 commits February 9, 2024 13:03

Comments

ed216d5

Merge branch 'main' into 2287/autopilot-domain

0cfbc2e

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

squadgazzz merged commit 96f4f7b into main Feb 12, 2024
9 checks passed

squadgazzz deleted the 2287/autopilot-domain branch February 12, 2024 11:32

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 improvemenet fee policy autopilot domain #2378

Price improvemenet fee policy autopilot domain #2378

squadgazzz commented Feb 6, 2024

fleupold left a comment

fleupold Feb 7, 2024

squadgazzz commented Feb 7, 2024 •

edited

Loading

fleupold left a comment

fleupold Feb 7, 2024

squadgazzz Feb 7, 2024

fleupold Feb 7, 2024

squadgazzz Feb 8, 2024

fleupold Feb 7, 2024

squadgazzz Feb 8, 2024

fleupold left a comment

fleupold Feb 9, 2024

squadgazzz Feb 9, 2024

fleupold Feb 9, 2024

squadgazzz Feb 9, 2024

fleupold Feb 9, 2024

squadgazzz Feb 9, 2024

fleupold Feb 9, 2024

squadgazzz Feb 9, 2024

sunce86 left a comment

sunce86 Feb 9, 2024

Price improvemenet fee policy autopilot domain #2378

Price improvemenet fee policy autopilot domain #2378

Conversation

squadgazzz commented Feb 6, 2024

Description

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz commented Feb 7, 2024 • edited Loading

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

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

sunce86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz commented Feb 7, 2024 •

edited

Loading