Fee Policies API #2809

squadgazzz · 2024-07-12T08:38:53Z

Description

Includes policy fees in the /trades API associated with the trade/order.

Changes

More details are in the PR comments.

How to test

Existing and new tests.

Related Issues

Fixes #2642

crates/model/src/fee_policy.rs

squadgazzz · 2024-07-12T08:41:21Z

crates/model/src/trade.rs

    num::BigUint,
    primitive_types::{H160, H256},
    serde::{Deserialize, Serialize},
    serde_with::{serde_as, DisplayFromStr},
 };

 #[serde_as]
-#[derive(Eq, PartialEq, Clone, Debug, Default, Deserialize, Serialize)]
+#[derive(PartialEq, Clone, Debug, Default, Deserialize, Serialize)]


Eq was removed since FeePolicy contains f64 fields that don't derive this trait. Also, couldn't find a reason why this was required.

crates/database/src/trades.rs

crates/database/src/byte_array.rs

sunce86

I think this is a good approach.

fleupold

Despite having additional comments in the PR, I think the description should contain a clear overview for the reviewer and future reader of the change summarising of the core challenge faced here, the different options considered and why the one that was chosen prevailed.

IMO, we should keep our queries very lean and basic (ie pretty straight forward "fetch raw data") and do joins and filtering logic in the rust code (unless there is very big performance considerations, which I don't believe is the case here).
Otherwise we will stand in front of a monumental challenge the day we need to move away from postgres (which will likely come at some point as we scale and receive more and more orders).

Concretely, here this would mean to first fetch the trades and then fetch the fee policies in a second roundtrip by using the order and auction ids.

squadgazzz · 2024-07-15T07:41:18Z

IMO, we should keep our queries very lean and basic (ie pretty straight forward "fetch raw data") and do joins and filtering logic in the rust code (unless there is very big performance considerations, which I don't believe is the case here).

Makes sense. Initially, it looked like a tiny change, but due to sqlx limitations, it is now more complex than expected. Will separate the queries.

crates/database/src/trades.rs

crates/database/src/byte_array.rs

squadgazzz · 2024-07-15T16:30:09Z

Got rid of the additional complexity, as Felix suggested. Now it uses 3 SQL queries instead of a single one. There is an option to include the quote fetching right into the trades SQL query, but semantically, the quote is required for the policy fee where the latter is no longer a part of the trades SQL.

fleupold

Approach looks good. Some questions around inconsistencies and combining the third roundtrip to the db (fetching all required quotes at once)

crates/database/src/trades.rs

fleupold · 2024-07-16T07:51:58Z

crates/orderbook/src/database/fee_policy.rs

+};
+
+#[async_trait::async_trait]
+pub trait FeePolicyRetrieving: Send + Sync {


Why do we need this trait if there is only a single impl?

Not required, removed.

fleupold · 2024-07-16T08:03:26Z

crates/orderbook/src/database/fee_policy.rs

+                    .with_label_values(&["order_quote"])
+                    .start_timer();
+
+                let order_quote = database::orders::read_quote(&mut ex, &order_uid)


These should be collected and then fetched in a single roundtrip imo (can use existing read_quotes).

Makes sense, reworked.

crates/model/src/fee_policy.rs

fleupold · 2024-07-16T08:06:37Z

crates/orderbook/src/database.rs

@@ -1,5 +1,6 @@
 pub mod app_data;
 pub mod auctions;
+mod fee_policy;


Any reason this is the only module that's using singular (c.f auctions, trades, etc).

fleupold · 2024-07-16T10:09:30Z

crates/orderbook/src/database/trades.rs

+                    async move {
+                        match trade.auction_id {
+                            Some(auction_id) => {
+                                self.fee_policies(auction_id, trade.order_uid, quote).await


Can this be a single roundtrip (fetch fee policies for a list of auction/order pairs) and then only for the ones that require a quote we fetch the quote (in another roundtrip)?

Replaced with a single roundtrip.

m-lord-renkse · 2024-07-16T14:57:07Z

crates/database/src/fee_policies.rs

+        if i > 0 {
+            query_builder.push(" OR ");
+        }
+        query_builder


why not to use format!() instead? it has many string reallocation...

We use QueryBuilder for such cases as the most convenient option, which should be safe since it uses write! macro under the hood.

m-lord-renkse · 2024-07-16T15:05:06Z

crates/orderbook/src/database/fee_policies.rs

+    ) -> anyhow::Result<HashMap<(AuctionId, OrderUid), Vec<FeePolicy>>> {
+        let mut ex = self.pool.acquire().await?;
+
+        let _timer = super::Metrics::get()


isn't it this timer stopped?

Thanks, forgot to stop it after refactoring.

MartinquaXD

Mostly nits

crates/orderbook/openapi.yml

MartinquaXD · 2024-07-17T07:09:14Z

crates/orderbook/src/database/fee_policies.rs

+                "missing price improvement quote for order '{:?}'",
+                order_uid
+            ))?;
+            let fee = quote.gas_amount * quote.gas_price / quote.sell_token_price;


function already supports error handling so I would suggest to use checked_div() here just to be safe.

Fair point. Used BigRational.

crates/database/src/fee_policies.rs

crates/model/src/fee_policy.rs

crates/orderbook/src/database/trades.rs

alfetopito · 2024-07-31T10:29:39Z

crates/model/src/fee_policy.rs

+    PriceImprovement {
+        factor: f64,
+        max_volume_factor: f64,
+        quote: Quote,


FYI, this is not documented in the openapi.yml definition.

Fee Policy API

5342eb1

squadgazzz commented Jul 12, 2024

View reviewed changes

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

squadgazzz commented Jul 12, 2024

View reviewed changes

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

squadgazzz marked this pull request as ready for review July 12, 2024 08:50

squadgazzz requested a review from a team as a code owner July 12, 2024 08:50

squadgazzz commented Jul 12, 2024

View reviewed changes

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

squadgazzz commented Jul 12, 2024

View reviewed changes

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

sunce86 approved these changes Jul 12, 2024

View reviewed changes

fleupold reviewed Jul 13, 2024

View reviewed changes

m-lord-renkse reviewed Jul 15, 2024

View reviewed changes

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

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

squadgazzz marked this pull request as draft July 15, 2024 11:14

Use simpler SQL queries

1dd1956

squadgazzz marked this pull request as ready for review July 15, 2024 16:26

squadgazzz requested review from fleupold, sunce86 and m-lord-renkse July 15, 2024 16:30

squadgazzz added 2 commits July 15, 2024 21:29

Optional auction id

72d36e8

Proper timer

368b534

squadgazzz force-pushed the fee-policy-api branch from 4f044a5 to 368b534 Compare July 15, 2024 18:33

fleupold reviewed Jul 16, 2024

View reviewed changes

squadgazzz added 3 commits July 16, 2024 11:10

Order test results

619121d

Review comments

014e2e4

Read quotes at once

4ee3cde

squadgazzz force-pushed the fee-policy-api branch from 80778b5 to 4ee3cde Compare July 16, 2024 09:25

Typo

74934a1

squadgazzz mentioned this pull request Jul 16, 2024

Fetch Auction from database #2734

Merged

2 tasks

fleupold reviewed Jul 16, 2024

View reviewed changes

squadgazzz added 2 commits July 16, 2024 17:16

Fetch fee policies with a single query

e5a33c8

Single quote query

f47f8cd

squadgazzz force-pushed the fee-policy-api branch from c275939 to f47f8cd Compare July 16, 2024 14:42

m-lord-renkse reviewed Jul 16, 2024

View reviewed changes

Stop timer

ae18776

squadgazzz force-pushed the fee-policy-api branch 2 times, most recently from a6bebad to 22bac33 Compare July 16, 2024 16:46

Check empty filter

22bac33

MartinquaXD reviewed Jul 17, 2024

View reviewed changes

squadgazzz added 2 commits July 17, 2024 12:55

Review comments

eecf45a

Deserialize for tests only

1b95290

MartinquaXD approved these changes Jul 17, 2024

View reviewed changes

squadgazzz changed the title ~~Fee Policy API~~ Fee Policies API Jul 17, 2024

Merge branch 'main' into fee-policy-api

7f6f2d1

squadgazzz requested review from fleupold and m-lord-renkse July 17, 2024 16:07

fleupold approved these changes Jul 17, 2024

View reviewed changes

m-lord-renkse approved these changes Jul 18, 2024

View reviewed changes

sunce86 approved these changes Jul 18, 2024

View reviewed changes

Merge branch 'main' into fee-policy-api

ba23428

sunce86 merged commit 9473850 into main Jul 18, 2024
10 checks passed

sunce86 deleted the fee-policy-api branch July 18, 2024 09:36

github-actions bot locked and limited conversation to collaborators Jul 18, 2024

alfetopito reviewed Jul 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fee Policies API #2809

Fee Policies API #2809

squadgazzz commented Jul 12, 2024 •

edited

Loading

squadgazzz Jul 12, 2024 •

edited

Loading

sunce86 left a comment

fleupold left a comment •

edited

Loading

squadgazzz commented Jul 15, 2024

squadgazzz commented Jul 15, 2024

fleupold left a comment

fleupold Jul 16, 2024

squadgazzz Jul 16, 2024

fleupold Jul 16, 2024

squadgazzz Jul 16, 2024

fleupold Jul 16, 2024

squadgazzz Jul 16, 2024

fleupold Jul 16, 2024

squadgazzz Jul 16, 2024

m-lord-renkse Jul 16, 2024

squadgazzz Jul 16, 2024

m-lord-renkse Jul 16, 2024

squadgazzz Jul 16, 2024

MartinquaXD left a comment

MartinquaXD Jul 17, 2024

squadgazzz Jul 17, 2024

alfetopito Jul 31, 2024

squadgazzz Jul 31, 2024

Fee Policies API #2809

Fee Policies API #2809

Conversation

squadgazzz commented Jul 12, 2024 • edited Loading

Description

Changes

How to test

Related Issues

squadgazzz Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

sunce86 left a comment

Choose a reason for hiding this comment

fleupold left a comment • edited Loading

Choose a reason for hiding this comment

squadgazzz commented Jul 15, 2024

squadgazzz commented Jul 15, 2024

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

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

MartinquaXD 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

squadgazzz commented Jul 12, 2024 •

edited

Loading

squadgazzz Jul 12, 2024 •

edited

Loading

fleupold left a comment •

edited

Loading