Define protocol fee params and forward to driver

[sunce86]

Description

Tackles #2083 and #2084.

Defines fee policy configuration parameters. Based on them, we can take the protocol fee as a cut from price improvement or as a percent of the order volume (fixed fee in absolute value will be implemented later).
On top of that, we also need two flags, for skipping in-market limit orders and twap orders, since default initial configuration will be to run the fee experiment only for out-of-market limit orders.

Note that the quote is not forwarded to the driver at this moment, since it is not needed for out-of-market limit orders.

Also note that I am missing the cap for absolute value of protocol fee (default value 1000$ or 0.5ETH) but this cap is still discussed (will be added later if needed).

Skipping forwarding protocol fee parameters for in-market limit orders will be done in a separate PR.

Changes

• Defines basic parameters for protocol fee calculation.
• Forwards protocol fee parameters to the driver.

[fleupold]

How will the fee information be propagated for the frontend to consume? Are we planning to put it on the trade records we write into the database?

Alternatively we could have the API set the fee policy whenever orders are created. Curious if you have thought about the trade-offs?

[sunce86]

How will the fee information be propagated for the frontend to consume? Are we planning to put it on the trade records we write into the database?

It will follow the same flow as solver fees, meaning, fees that are unknown before sending the auction to solvers for solving and also not communicated back to the autopilot before settling, are only possible to evaluate once the transaction is mined and on_settlement_event_updater should calculate them and store them into order_execution table.

[sunce86]

Alternatively we could have the API set the fee policy whenever orders are created. Curious if you have thought about the trade-offs?

Once I restart the autopilot with updated fee policies I want all orders to follow them from that point in time. It's easier to debug and evaluate the change.

[fleupold]

are only possible to evaluate once the transaction is mined and on_settlement_event_updater should calculate it and store it into order_execution table.

This requires information about the fee policy though right (the surplus fee can be observed on chain)? What if the fee policy changes between the auction being created and the settlement submission being mined (e.g. on restarts) or if it's different across staging and production?

[sunce86]

Oh, you meant the fee policy (I was talking about the executed protocol fee).
Yes, we need to store the fee policy somewhere for each order execution. Since I want to make order_execution table reindexable (to not depend on any data that is lost forever if not saved at the execution time), then will probably store this data somewhere else (I'd like to have a separate table).

[sunce86]

Alternatively we could have the API set the fee policy whenever orders are created. Curious if you have thought about the trade-offs?

Once I restart the autopilot with updated fee policies I want all orders to follow them from that point in time. It's easier to debug and evaluate the change.

The other alternative would be to save the fee policies to Order whenever order is created, but to update this field on each open order on each new auction (so we have this trait I mentioned ☝️ ). Personally I really dislike this approach and would prefer to have a different table for fee policies.
The reason being that these kind of solutions lead to unnecessary complexities in the system, where we update the same struct from different points in the code and in different points in time. This makes this struct scary to change and hard to reason about, while the only benefit is less DRY code.

[MartinquaXD]

Configuration and propagation of current fee policies look alright to me.
I'm worried that we might end up with a lot of additional CLI args in the autopilot due to different fee policies but for the start where we only have a single fee policy this seems sufficient.

[MartinquaXD]

nit: the --help text refers to a percentage when it's actually a factor that is to be understood as a percentage. Slight difference that bit us in the past.

[sunce86]

I think we are covered with the parsing function that makes sure it's a factor. We also have a factor in the variable name. I wanted to keep the comments focused on the meaning.

[MartinquaXD]

It makes sure that the value entered is in [0,1] but it doesn't make sure that what you provided is actually what you intended. Take slippage for example where it makes sense to configure 0.5%. We had a CLI argument in the past that claimed it was a percentage so we passed in 0.5 which then ended up allowing 50% relative slippage. Despite using shared::arguments::parse_percentage_factor it took us a while to notice the error. 😬

We also have a factor in the variable name.

This is true but I think with MFW's recent push to make the repo more contributor friendly it still makes sense to be precise in the --help text.
It could be as short as:
Cap protocol fee with a percentage of the order's volume. (1.0 meaning 100%)

[fleupold]

Only nits, but I think it's important we pay attention to the way we document things and where. E.g. the autopilot is agnostic to the fact that the fee "is taken from surplus". It just warns participants in the protocol that if they match an order they may have to pay the protocol something at the end of the accounting period.

Our implementation of the driver then is the one that decides to hide fees from solver engines and instead "take" those fees in the form of positive slippage.

We should also make sure we use a common language when talking about the different types of fees (that will be consistent throughout or developer docs, marketing, communication with solvers etc).

Lastly, we should make it easy for maintainers to configure these parameters in a way that are intuitive and contextually clear.

[fleupold]

Shouldn't the dto also have documentation (so that it can be generated for the swagger docs)?

[sunce86]

f783d68 is a significant change:

While thinking how to save the fee policies to the database, I figured that I have to design everything from the start, to support all ways of taking fees:

1. quote deviation with a optional volume cap and optional absolute cap
2. volume percent
3. absolute value

Each of these can be modelled with 3 parameters: price_improvement_percent, volume_percent, absolute_fee:

1. price_improvement_percent = Some, volume_percent = Optional, absolute_fee = Optional,
2. price_improvement_percent = None, volume_percent = Some, absolute_fee = None,
3. price_improvement_percent = None, volume_percent = None, absolute_fee = Some

If we have all 3 fees defines like in the case of (1), we will calculate all 3 and take the smallest (which is equivalent to calculating price_improvement with volume cap and absolute cap).

[fleupold]

What would it semantically mean to define multiple entries with the same policy but different parameters? That the protocol has multiple fees for the same order type? This sounds confusing to me.

I don't foresee us using vastly different parameters for in market vs out of market orders (if so we can define them later as two different fields). I therefore think we should just define a struct with

• price improvement factor
• volume % cap
• absolute cap
• whether or not to include in-market priced limit orders

On a per order basis, I think we should then just signal the concrete fee for this order, which can be one of three (maybe 4) in my opinion, potentially chained in case integrators want to charge additional fees.

• % of price improvement (WE ONLY NEED THIS FOR NOW)
• % of volume (e.g. 0.1% of volume)
• Absolute amount (e.g. $1 per trade)
• maybe % of surplus (although the incentives are too obviously bad to even experiment with this IMO)

[sunce86]

What would it semantically mean to define multiple entries with the same policy but different parameters?

It enables us to take a quote deviation fee, and after that another fee as percent of volume (meaning we can chain multiple different fees for a single order). IIUC, this is something we have to support for integrators.

One could argue that we should summarize all of them to a single value and send that single value to the driver, but can't do that since not all types of fees are known upfront (volume percent and absolute fee are, quote deviation not).

Then, since db migrations are painful, I decided to model the db table for fee polices in a way that supports all foreseeable types of policies and not just for price improvement type. That design was created to be memory efficient and decent performance wise (I could have created a separate table for each fee type but then the fetching would be slower).

Why

pub enum FeePolicy {
LimitOrderOutOfMarket(FeePolicyParams),
LimitOrderInMarket(FeePolicyParams),
}

Because I expect to add a third type - TWAP orders.

I hope my reasonings are clearer now. Note that this PR is not yet ready for merging, I just need to get the approval for the last commit that my new logic is ok.

[sunce86]

Also, I hoped that I could support these two types of fees in the first iteration, since it will be trivial with this design:

% of price improvement (WE ONLY NEED THIS FOR NOW)
% of volume (e.g. 0.1% of volume)

[fleupold]

IIUC, this is something we have to support for integrators.

My vision for integrators is that they specify their fee policy as part of the app data and it doesn't become part of the arguments we start the core protocol with.

The core protocol will experiment with different fees (price improvements, volume based, etc) but I foresee its fee config to be relatively concise and not a chain of many different fee compositions. Also, the protocol can only charge fees on order types that are a first class citizen in the core protocol (limit orders).

Because I expect to add a third type - TWAP orders.

TWAP orders are not part of the core CoW Protocol. They are a set of orders that semantically belong together and are orchestrated and placed from a watch tower, but as far as CoW protocol is concerned users are simply placing limit orders in awkwardly consistent periods.

So my goal here is to split what the protocol boots up with as a fee policy from the different types of protocol fees we can attach to each order that is sent to the solvers.

E.g. the protocol may define that we want to take 0.1% volume fee on all orders. This is what the autopilot should be started with.

If an integrator (e.g. CoW Swap's TWAP feature or Metamask) wants to charge 0.85% volume fee on top, then they can create an order specifying this in the app data and what we end up sending to the driver is two fees (0.1% for the protocol, 0.85% for the integrator), both of which will be charged from solvers if they settle this order at the end. Potentially the protocol will demand a cut (cf. Apple charging 30% of all app store purchases) from the integrator fees.

To sum up, I think we should have a FeePolicy struct on the autopilot arguments and the a protocol_fee: Vec<Fee> on the order itself.

[sunce86]

Ok, not using configuration type for autopilot domain makes sense, will split it.

Do you think we should keep the possibility that 2 different policies (for example quote deviation and volume based) can be sent for a single order to the driver, at the same time?

If you don't like having a third enum variant for TWAP orders, how would you model a situation where we set quote deviation policy for limit orders and volume based policy for TWAP orders using:

price improvement factor
volume % cap
absolute cap
whether or not to include in-market priced limit orders

We talked about not supporting this on slack, if so, I can just add another whether to include twap orders flag.

[sunce86]

@fleupold @MartinquaXD re-asking for review, third time's a charm :)

This setup now fits nicely with the db table: #2118
I've updated the PR to also support volume based fees, since they are anyway needed for quote deviation fee calculation (volume fee cap) so why not adding it right away.

[fleupold]

Thanks for all the turn-around I think we are getting there on the naming.

My remaining worry now is about simply inlining the increasingly complex fee policy parsing and mapping logic into the main autopilot run-loop component, which is a recipe for complexity.

[fleupold]

ditto - what currency does the protocol charge solvers the fee in?

[sunce86]

As you mentioned in the comment ☝️ , the protocol collects the protocol fees in different tokens. The question is, whether the conversion to ETH/COW/else should be done in the autopilot or later in the script for accounting.

[sunce86]

The conversion will be done in the script similarly to how we do it in solver rewards script, using the prices table.

[fleupold]

Sure, but it will still be charged in some currency right? Solvers will receive a bill from us at the end of the week saying: order x got 100 USDT price improvement, please pay us y.

y will be some percentage of 100 USDT, but in which currency (USDT, COW, ETH)? This is important for someone consuming this model to know so that they can make sure they convert and keep the fees in the right currency if they want to be neutral.

[fleupold]

Configuration looks good, last concern is about factoring the logic that adds the few on the order struct.

[fleupold]

Shouldn't this somehow take fee_policy_skip_market_orders into account and include the match statement on order.metadata.class?

I'd create a new protocol_fee module and have a struct apply the fee. Note that in the future we will likely have logic that based on app_data adds additional fees here.

[sunce86]

This will be implemented as a separate task: #2092

Let's also move this logic into a separate file as part of ☝️ task, to unblock this PR and merge it.

[MartinquaXD]

Only nits at this point.

[MartinquaXD]

The fees get taken in the opposite token (compared to surplus fee). This is because it would allow us to easily implement the fee model in the driver without solvers having to know about it, right?

[sunce86]

My initial idea was that we need to take the fee as some percent of the executed_amount which is always expressed in the sell token for sell orders and in buy token for buy orders, but this is still open and could change during implementation if we figure out we actually need something else.

[sunce86]

Merging this PR to unblock @narcis96 so that he could work on other tasks in parallel.