Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Break down fees per policy #2862

Closed
3 tasks
sunce86 opened this issue Aug 6, 2024 · 13 comments
Closed
3 tasks

feat: Break down fees per policy #2862

sunce86 opened this issue Aug 6, 2024 · 13 comments

Comments

@sunce86
Copy link
Contributor

sunce86 commented Aug 6, 2024

Problem

Some time ago we expanded get_trades API to return fee policies: #2809

Now, we also need, for each fee policy in get_trades response, to return executed fee (in sell token). This would allow frontend to summarize them into total_protocol_fee. Then, since frontend already has executed_surplus_fee, they could calculate gas_fee = executed_surplus_fee - total_protocol_fee. With that, complete breakdown of fees is provided.

Suggested solution

  1. Executed protocol fees per fee policy can be saved into order_execution database table. We would need to add a new field. Important requirement is that ordering of executed protocol fees saved to order_execution needs to be aligned with protocol fees defined in fee_policies database table.
  2. domain::Settlement code needs to be adjusted to not only return total fee but also protocol fee per fee policy.
  3. OnSettlementEventUpdater saves executed protocol fees to order_execution.

Tasks

  • Extend order_execution with a field that contains a list of executed protocol fees in sell token
  • Save executed protocol fees from the OnSettlementEventUpdater
  • Update get_trades to return executed protocol fees in sell token

Acceptance criteria

For a trade that has fee policies attached, executed fees per fee policy should be returned as well.

@fleupold
Copy link
Contributor

fleupold commented Aug 6, 2024

Executed protocol fees per fee policy can be saved into order_execution database table.

Why does it need to be persisted? With the fee policies and the traded amounts don't we have everything to compute this number on the fly.

The reason the frontend doesn't want to do it is because the logic is a big pita and it's easy to get it wrong. The autopilot already has logic to recover the fee amounts to compute the score. Can this be reused (and using one of the two available price vectors be converted into an approximate sell token amount)?

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 6, 2024

Can this be reused

Maybe we can reuse logic but code is not reusable - reimplementing it in the orderbook would be the third place where we calculate the same thing (we also have protocol fee calculation in the driver). Maybe it's time to extract the code?

AFAIS we currently use uniform clearing prices to convert protocol fee to sell token for sell orders. This is not visible in orderbook.

and using one of the two available price vectors be converted into an approximate sell token amount

You mean to use custom prices (that incorporate all fees) instead of uniform clearing prices for conversion to sell token? I don't think this would work for orders where fee represents a significant portion of the order executed amount, as approximate could be way off.

@fleupold
Copy link
Contributor

fleupold commented Aug 6, 2024

reimplementing it in the orderbook would be the third place where we calculate the same thing (we also have protocol fee calculation in the driver)

It's slightly different in the driver and autopilot though. The driver has the "gross solution" without any fee applied and needs to compute the fee so that it can reduce its solution quality by it.
The autopilot gets the "net solution" with the fee already applied, and wants to compute the fee amount, so the logic is different.

Maybe it's time to extract the code?

I think this makes sense (and can also allow for some nice tests that applying fees and recovering fees yield the same values). We need to check how we translate the different data-types though since this logic lives in the un-related domains of driver/autopilot respectively.

Maybe creating own types in the shared module and having a thin shim in boundary of both driver and autopilot can work for that.

You mean to use custom prices (that incorporate all fees) instead of uniform clearing prices for conversion to sell token?

I think we have two choices

  • Uniform clearing prices
  • External prices that are used to normalize surplus in ETH

I think the latter make slightly more sense as they are not subject to solver scrutiny.

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 6, 2024

It's slightly different in the driver and autopilot though.

I was referring to scoring module that is supposed to be 1:1 with autopilot (except with slight difference how the fee policies are provided): https://github.com/cowprotocol/services/blob/main/crates/driver/src/domain/competition/solution/scoring.rs#L34-L38

External prices that are used to normalize surplus in ETH

If so, then we need a separate PR to use external prices here as well?: https://github.com/cowprotocol/services/blob/main/crates/autopilot/src/domain/settlement/solution/trade.rs#L154-L165
So that executed_surplus_fee is also using the same conversion.

@fleupold
Copy link
Contributor

fleupold commented Aug 6, 2024

If it has already been done using the other price vector I don't think it's worth the change.

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 6, 2024

If it has already been done using the other price vector I don't think it's worth the change.

After working on it, I think it's pretty important we align this: #2863

Turned out the change is straight forward.

sunce86 added a commit that referenced this issue Aug 7, 2024
# Description
Related to discussion in the issue:
#2862 (comment)

Use external price vector to convert "fee in surplus token" into "fee in
sell token" (instead of uniform clearing prices).

This is important so that we make sure that it's always true:

`total_fee = executed_surplus_fee + protocol fees`

If we use uniform clearing prices for `executed_surplus_fee` and
external prices for `protocol fees` I think above equation won't stand.

## How to test
Existing unit test
@sunce86
Copy link
Contributor Author

sunce86 commented Aug 7, 2024

There are two more arguments against calculating protocol fees JIT. Examples:

  1. Order get's filled and get_trades return protocol fee X. After two months we change the logic/math how we calculate protocol fees (for any reason) and then get_trades return protocol fee Y for the same order. What's the truth here?
  2. Once we execute some order with, for example, surplus variant of protocol fee, we are not allowed to never ever delete surplus variant in our code since we need to support historic executions of surplus protocol fees. If, on the other side, these executed protocol fees were persisted in database, we would be able to maintain our code properly and delete surplus variant.

The only drawback of persisting executed protocol fee in database is memory space, if I am not wrong.

@fhenneke
Copy link

fhenneke commented Aug 7, 2024

@harisang is a big fan of persisting protocol fees. (To the point of building and maintaining its own database tables around that). Having that data available in a convenient format (e.g. per fee policy, ideally in the protocol fee token itself) would simplify solver rewards, slippage, partner fee accounting, circuit breaking; in addition to benefits to frontend.

@fleupold
Copy link
Contributor

fleupold commented Aug 7, 2024

ok, then I'm convinced. Where would you store those? One the trades table?

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 7, 2024

I suggested putting it into order_execution table (see issue desc ☝️ ) as it already contains executed_surplus_fee which is a related thing.

@fhenneke
Copy link

fhenneke commented Aug 7, 2024

If I understand correctly, the plan is to store one value per fee policy. So if there are two protocol fees attached to a trade, there would be two values, one for each fee policy. (This is how it is specified in the issue description, but the general introduction only mentions totals.)
This leaves the question about what currency the fee is stored in. Is it the sell token (to make the ui endpoint easier), the token the fee was actually charged in (to make the solver teams lives easier; no conversion before storing; the correct choice), or some numeraire (similar to scores)? If I got the discussion correctly, we can choose to convert in a standard way e.g. using external prices and then convert back and forth at will.

@sunce86
Copy link
Contributor Author

sunce86 commented Aug 7, 2024

If I understand correctly, the plan is to store one value per fee policy. So if there are two protocol fees attached to a trade, there would be two values, one for each fee policy. (This is how it is specified in the issue description, but the general introduction only mentions totals.) This leaves the question about what currency the fee is stored in. Is it the sell token (to make the ui endpoint easier), the token the fee was actually charged in (to make the solver teams lives easier; no conversion before storing; the correct choice), or some numeraire (similar to scores)? If I got the discussion correctly, we can choose to convert in a standard way e.g. using external prices and then convert back and forth at will.

executed_surplus_fee is in sell token, so I initially wanted to stick to executed protocol fee being also in sell token (as this is what frontend needs).

the other option is to save it in it's original form (in SURPLUS token) to the database and then, on get_trades convert it to sell token (using external prices) as required by frontend, while solver team could use database::order_execution.protocol_fees directly.

edit: After some thinking, I am leaning towards saving the executed protocol fees in surplus token (for start), but also save surplus token address. I think this is the most future proof solution. Edited the original post ☝️

sunce86 added a commit that referenced this issue Aug 9, 2024
# Description
Extracts part of https://github.com/cowprotocol/services/pull/2861/files
that can be merged now, and don't have to be part of that PR.

This enables me to have a nice pub interface of `Settlement` and
continue adding functions for
#2862 and not wait for ☝️
PR to be merged.

Also #2861 becomes smaller
and easier to revert if we encounter some issue on weekly release.
sunce86 added a commit that referenced this issue Aug 13, 2024
# Description
Implements task ~2~ 1 from
#2862:

> Extend order_execution with a field that contains a list of executed
protocol fees in surplus token, together with the surplus token address

`autopilot::domain::Settlement` now exposes breakdown of protocol fees
in surplus token.

Applies the same refactoring to driver, to keep the code 1:1 (and also
to use driver tests to confirm no bugs were introduced).

## How to test
Existing tests.
sunce86 added a commit that referenced this issue Sep 6, 2024
…2910)

# Description
Related to #2862

Exposes executed protocol fees from database on /get_trades endpoint.

Database fee field is an array of <token, amount> so that we have the
flexibility now (and also in the future) to save taken fee in whatever
token we want. This was inspired by discussion where even for the same
order we considered defining taken protocol fee in sell token for
"surplus" variant, while, for "volume" variant it's more logical to
define it in non-surplus token (sell token for sell order and buy token
for buy order) so that the taken fee doesn't depend on the amount of
surplus provided in a solution.

# Changes
<!-- List of detailed changes (how the change is accomplished) -->

- [ ] Add new columns `protocol_fee_tokens` and `protocol_fee_amounts`
to `order_execution` database table.
- [ ] Expand `executedProtocolFees` field on `Trade` struct to return
both executed protocol fees and fee policies.

## How to test
Updated db test.
Existing tests.
e2e test updated to prove correctness.
@sunce86
Copy link
Contributor Author

sunce86 commented Sep 9, 2024

Finished.

@sunce86 sunce86 closed this as completed Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants