Breakdown fees protocol 3/3: DB field and returning on /get_trades #2910

sunce86 · 2024-08-19T11:08:59Z

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

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.

github-actions · 2024-08-19T11:09:13Z

Reminder: Please update the DB Readme.

Caused by:

database/sql/V070__executed_protocol_fees.sql

crates/database/src/order_execution.rs

crates/model/src/trade.rs

crates/database/src/order_execution.rs

database/sql/V068__executed_protocol_fees.sql

fleupold

In general, let's please avoid any advanced postgres features unless there is a very strong reason to do so (ideally we keep the db as vanilla as possible so we are not locked into a specific DB implementation)

database/sql/V068__executed_protocol_fees.sql

crates/model/src/trade.rs

crates/database/src/order_execution.rs

github-actions · 2024-08-28T00:07:19Z

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.

squadgazzz · 2024-09-04T07:04:25Z

database/sql/V068__executed_protocol_fees.sql

+ADD COLUMN protocol_fee_tokens bytea[] NOT NULL DEFAULT '{}', 
+ADD COLUMN protocol_fee_amounts numeric(78,0)[] NOT NULL DEFAULT '{}';


Was it considered to use a single JSON column?

yes, but i don't see any significant advantage

Ability not to think about the ordering of 2 arrays.

crates/database/src/order_execution.rs

m-lord-renkse · 2024-09-04T09:55:57Z

crates/database/src/order_execution.rs

+        pub protocol_fee_tokens: Vec<Address>,
+        pub protocol_fee_amounts: Vec<BigDecimal>,


is it guarantee in the query that we are retrieving the token and its corresponding amount in the same order? it could be dangerous in case the first token doesn't correspond with the first amount, etc

Caller is responsible for storing the values in proper order.

fleupold

Let's combine fee policy and executed protocol fee into one type please.

crates/autopilot/src/domain/settlement/trade/mod.rs

crates/autopilot/src/domain/settlement/trade/math.rs

crates/model/src/trade.rs

database/README.md

crates/orderbook/openapi.yml

sunce86 · 2024-09-04T14:06:11Z

crates/autopilot/src/domain/settlement/trade/mod.rs

    /// Breakdown of protocol fees. Executed protocol fees are in the same order
    /// as policies are defined for an order.
-    pub protocol: Vec<(eth::SellTokenAmount, fee::Policy)>,
+    pub protocol: Vec<ExecutedProtocolFee>,


had to fallback to <total, protocol> since now the values are in different tokens.

willing to revert once the "network" is switched to surplus token as well, since this is a final goal and agreement with solver and frontend team. Doing it in this PR increases significantly the scope.

sunce86 · 2024-09-04T15:56:53Z

Ok, ready for another round of code reviews

crates/orderbook/src/database/fee_policies.rs

MartinquaXD

Mostly nits but would like to see the DB test also assert that we recover the correct values not just the expected number of entries.

crates/autopilot/src/domain/settlement/trade/mod.rs

crates/database/src/order_execution.rs

crates/orderbook/src/database/fee_policies.rs

crates/autopilot/src/domain/settlement/trade/mod.rs

fleupold

Code looks good imo, one question I have is if we will also expose this information on the order endpoint (it seems this would be more convenient for the frontend)

crates/database/src/order_execution.rs

crates/autopilot/src/infra/persistence/mod.rs

crates/autopilot/src/domain/settlement/trade/mod.rs

fleupold · 2024-09-05T10:48:44Z

crates/orderbook/openapi.yml

@@ -1192,12 +1192,12 @@ components:
          allOf:
            - $ref: "#/components/schemas/TransactionHash"
          nullable: true
-        feePolicies:
+        executedProtocolFees:


Is it sufficient to expose this information on the trade? Currently, we are returning the executed fees on the order (and the frontend plans to display the fee on the order page). Are we also planning to return it there?

Hm, will ask the frontend. If they want it on order api as well, I think we can add that in a separate PR.

MartinquaXD

Main suggestions were addressed.

sunce86 added 8 commits August 15, 2024 15:21

Initial commit

03d92da

Add executed protocol fees to get_trades

d1b74ad

self cr

12c2cfd

unit tests

c3c059b

update db docs

4734065

update openapi

5062b98

optional executed fees

dac52c7

update tests

92b8929

sunce86 requested a review from a team as a code owner August 19, 2024 11:09

m-lord-renkse reviewed Aug 19, 2024

View reviewed changes

crates/database/src/order_execution.rs Outdated Show resolved Hide resolved

revert optionallity

917522c

m-lord-renkse reviewed Aug 19, 2024

View reviewed changes

crates/model/src/trade.rs Outdated Show resolved Hide resolved

crates/database/src/order_execution.rs Outdated Show resolved Hide resolved

database/sql/V068__executed_protocol_fees.sql Outdated Show resolved Hide resolved

fleupold reviewed Aug 19, 2024

View reviewed changes

database/sql/V068__executed_protocol_fees.sql Outdated Show resolved Hide resolved

crates/model/src/trade.rs Outdated Show resolved Hide resolved

crates/database/src/order_execution.rs Outdated Show resolved Hide resolved

github-actions bot added the stale label Aug 28, 2024

sunce86 removed the stale label Aug 28, 2024

sunce86 added 6 commits September 3, 2024 12:09

Merge branch 'main' into breakdown-fees-protocol

bee9fda

merge conflict

15385d0

refactor scheme

7e22acb

small refactor

6d25a2e

remove option

b6027ed

save protocol fee

c2e15d6

sunce86 changed the title ~~Breakdown fees protocol 2/3: DB field and returning on /get_trades~~ Breakdown fees protocol 3/3: DB field and returning on /get_trades Sep 3, 2024

sunce86 added 2 commits September 3, 2024 15:24

fix tests

2508c1f

revert ignore

6e91f19

squadgazzz reviewed Sep 4, 2024

View reviewed changes

Merge branch 'main' into breakdown-fees-protocol

f2853c6

m-lord-renkse reviewed Sep 4, 2024

View reviewed changes

crates/database/src/order_execution.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed Sep 4, 2024

View reviewed changes

fleupold reviewed Sep 4, 2024

View reviewed changes

sunce86 self-assigned this Sep 4, 2024

sunce86 added 2 commits September 4, 2024 15:48

protocol fees are in surplus token

f8ae556

refactor

0910437

sunce86 commented Sep 4, 2024

View reviewed changes

sunce86 added 6 commits September 4, 2024 17:22

ExecutedProtocolFee object

f3f2f8b

docs update

2dc555d

add comment for ordering

6dcb6ae

fix e2e test

ab183c9

fmt

fa6fe2a

small fixes

0baf80c

sunce86 requested review from fleupold, MartinquaXD, squadgazzz and m-lord-renkse September 4, 2024 15:56

squadgazzz reviewed Sep 4, 2024

View reviewed changes

crates/orderbook/src/database/fee_policies.rs Show resolved Hide resolved

MartinquaXD reviewed Sep 5, 2024

View reviewed changes

fleupold approved these changes Sep 5, 2024

View reviewed changes

martin's review comments

adaf2e0

MartinquaXD approved these changes Sep 5, 2024

View reviewed changes

sunce86 added 3 commits September 6, 2024 09:15

cr fixes

43d809f

remove comment

1dc1960

Merge branch 'main' into breakdown-fees-protocol

24cb713

sunce86 enabled auto-merge (squash) September 6, 2024 07:22

sunce86 added 2 commits September 6, 2024 09:28

fix conflict

1236347

fix conflicts

a2a2c6b

sunce86 merged commit 5fa5c4f into main Sep 6, 2024
10 checks passed

sunce86 deleted the breakdown-fees-protocol branch September 6, 2024 07:37

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breakdown fees protocol 3/3: DB field and returning on /get_trades #2910

Breakdown fees protocol 3/3: DB field and returning on /get_trades #2910

sunce86 commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 19, 2024 •

edited

Loading

fleupold left a comment

github-actions bot commented Aug 28, 2024

squadgazzz Sep 4, 2024

sunce86 Sep 4, 2024

squadgazzz Sep 4, 2024

m-lord-renkse Sep 4, 2024

sunce86 Sep 4, 2024

fleupold left a comment

sunce86 Sep 4, 2024 •

edited

Loading

sunce86 commented Sep 4, 2024

MartinquaXD left a comment

fleupold left a comment

fleupold Sep 5, 2024

sunce86 Sep 6, 2024

MartinquaXD left a comment

		ADD COLUMN protocol_fee_tokens bytea[] NOT NULL DEFAULT '{}',
		ADD COLUMN protocol_fee_amounts numeric(78,0)[] NOT NULL DEFAULT '{}';

		pub protocol_fee_tokens: Vec<Address>,
		pub protocol_fee_amounts: Vec<BigDecimal>,

Breakdown fees protocol 3/3: DB field and returning on /get_trades #2910

Breakdown fees protocol 3/3: DB field and returning on /get_trades #2910

Conversation

sunce86 commented Aug 19, 2024 • edited Loading

Description

Changes

How to test

github-actions bot commented Aug 19, 2024 • edited Loading

fleupold left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 28, 2024

squadgazzz Sep 4, 2024

Choose a reason for hiding this comment

sunce86 Sep 4, 2024

Choose a reason for hiding this comment

squadgazzz Sep 4, 2024

Choose a reason for hiding this comment

m-lord-renkse Sep 4, 2024

Choose a reason for hiding this comment

sunce86 Sep 4, 2024

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

sunce86 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

sunce86 commented Sep 4, 2024

MartinquaXD left a comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

fleupold Sep 5, 2024

Choose a reason for hiding this comment

sunce86 Sep 6, 2024

Choose a reason for hiding this comment

MartinquaXD left a comment

Choose a reason for hiding this comment

sunce86 commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 19, 2024 •

edited

Loading

sunce86 Sep 4, 2024 •

edited

Loading