[BLOCKED] Save executed fee in surplus token #3184

sunce86 · 2024-12-26T11:35:26Z

Blocked by frontend team. I need a confirmation that frontend team switched to using executed_fee field introduced in #2966

Description

This PR implements item (3) from #2963

Previously, executed fees were saved and displayed in the sell token. With this change, executed fees will now be saved and expressed in the surplus token.

This change applies to all orders settled after this PR is merged.

Historical entries will remain expressed in the sell token.
From now on, all fees (e.g., gas fees, protocol fees) will be consistently saved and expressed in the surplus token. 🥳

Follow-Up Tasks

Migrate Historical Fees to Surplus Token

I will manually execute a migration immediately after this PR is merged.

This migration will convert existing fee values from the sell token to the surplus token using UCP prices from solver competition.
The migration is safe because it effectively reverses the logic used to save fees in the sell token in the current implementation.

Current version can be observed here (under function convert_executed_fee): cowprotocol/data-migration@76dc7bd#diff-1d06da761111802c793c6e5ca704bfa0d6336d0becf87fddff02d81548a838abR129

Fix Remaining Issue

After the migration, we need to update the following code reference:
https://github.com/cowprotocol/services/blob/main/crates/database/src/orders.rs#L575

Currently, the system will have a mix of fees saved in the sell token and surplus token.
Once all fees are migrated to the surplus token, we can finalize this fix

Changes

Save executed_fee in surplus token instead of in sell token

How to test

Expanded one unit test.
Existing e2e tests.

squadgazzz · 2024-12-26T19:19:42Z

I will manually execute a migration immediately after this PR is merged.

Is this script ready? It would be worth posting it in the description under a spoiler. Also, I am not sure I understand how the old entries will be identified; the script will probably explain it.

sunce86 · 2024-12-26T19:59:16Z

I will manually execute a migration immediately after this PR is merged.

Is this script ready? It would be worth posting it in the description under a spoiler. Also, I am not sure I understand how the old entries will be identified; the script will probably explain it.

Not yet ready, currently implementing it.

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

squadgazzz · 2024-12-27T11:04:31Z

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

+/// Fee per trade in a solution. These fees are taken for the execution of the
+/// trade.
+#[derive(Debug, Clone)]
+pub struct FeeBreakdown {
+    /// Fee reported by solvers, representing difference between UCP and custom
+    /// prices.
+    pub executed: eth::TokenAmount,
+    /// Breakdown of protocol fees.
+    pub protocol: Vec<ExecutedProtocolFee>,
+    /// Surplus token in which all the fees were taken.
+    pub token: eth::TokenAddress,
+}


It would be helpful to include details from your comment somewhere here.

squadgazzz

LGTM, but I would still first look into the script before approving this PR.

sunce86 · 2024-12-27T13:42:50Z

Current version of migration code can be observed here (under function convert_executed_fee): cowprotocol/data-migration@76dc7bd#diff-1d06da761111802c793c6e5ca704bfa0d6336d0becf87fddff02d81548a838abR129

The code is not clean since we want to primarily focus on logic and the code is practically used one time.

squadgazzz · 2024-12-30T12:33:20Z

Current version of migration code can be observed here (under function convert_executed_fee): cowprotocol/data-migration@76dc7bd#diff-1d06da761111802c793c6e5ca704bfa0d6336d0becf87fddff02d81548a838abR129

Could you make a PR there so it will be possible to ask questions about the migration?

MartinquaXD

I will manually execute a migration immediately after this PR is merged.

Is it important that this happens after the PR is merged? I assume this needs to be synchronized with the frontend to not show nonsensical values?
Asking because this comment implies to me that there should be 2 batches of DB updates (one set for staging and one set for prod).

MartinquaXD · 2024-12-31T12:01:20Z

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

+    /// Fee reported by solvers, representing difference between UCP and custom
+    /// prices.
+    pub executed: eth::TokenAmount,


Just by reading the struct and comments it's not really clear to me what this field is.
It sounds like this is just the sum of all protocol fees but I think since we need to account for the gas in the breakdown this is all protocol fees + gas fees.
In that case I think it would be nicer to have a field like gas_costs and a public member function total_fees(). WDYT?

This is how I implemented it initially, but I had to revert those changes. I commented here on the reasons why.

MartinquaXD · 2024-12-31T12:02:55Z

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

+    /// Breakdown of protocol fees.
+    pub protocol: Vec<ExecutedProtocolFee>,


The order of items also indicates the order in which the fee policies got applied, right?
Seems important enough to point out in a comment.

MartinquaXD · 2024-12-31T12:05:37Z

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

    },
    bigdecimal::Zero,
 };

 mod math;

+pub use math::FeeBreakdown;


nit: Seems like the original location of FeeBreakdown makes more sense (i.e. in trade/mod.rs).

MartinquaXD · 2024-12-31T12:08:23Z

crates/e2e/tests/e2e/protocol_fee.rs

-fn fee_in_buy_token(order: &Order, quote: &OrderQuote) -> U256 {
-    order.metadata.executed_fee * quote.buy_amount / quote.sell_amount
+fn fee_in_buy_token(order: &Order) -> U256 {
+    order.metadata.executed_fee


Isn't this now only correct when order is a buy order given that fees should now always be computed in the surplus token?
In that case I would add an assert for that.

mstrug · 2025-01-02T13:28:04Z

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

@@ -182,7 +160,7 @@ impl Trade {
                policy: *policy,
                fee,
            });
-            total += fee.amount;
+            total += fee;


Safer would be to use checked_add().

mstrug · 2025-01-02T13:46:23Z

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

+    fn surplus_fee(
+        &self,
+        surplus: eth::TokenAmount,
+        factor: f64,


Maybe use FeeFactor here instead of f64 so we can ensure on type-level that factor cannot have value of 1 (so there will be no division by 0).

Same for volume_fee().

mstrug · 2025-01-02T13:56:07Z

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

-            Self::Jit(trade) => trade.sell.token,
+            Self::Fulfillment(trade) => match trade.side {
+                order::Side::Buy => trade.sell.token,
+                order::Side::Sell => trade.buy.token,


Adding new function to implementation of Fulfillment type like get_token_by_side() (or similar), will remove the code duplication (here and Self::Jit() case below).

sunce86 added 4 commits December 26, 2024 11:21

Initial commit

6371811

fix tests

c256f56

fix queries

07c0ea6

revert query

8a87c3f

sunce86 self-assigned this Dec 26, 2024

sunce86 requested a review from a team as a code owner December 26, 2024 11:35

sunce86 added 2 commits December 26, 2024 13:57

fix e2e test

0deac2b

fmt fix

4ef3c3b

squadgazzz reviewed Dec 26, 2024

View reviewed changes

crates/autopilot/src/domain/settlement/trade/math.rs Show resolved Hide resolved

revert changes around gas fee

8ddb90b

squadgazzz reviewed Dec 27, 2024

View reviewed changes

alfetopito mentioned this pull request Dec 30, 2024

feat: executedSurplusFee removal cowprotocol/cowswap#5262

Merged

MartinquaXD reviewed Dec 31, 2024

View reviewed changes

mstrug reviewed Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BLOCKED] Save executed fee in surplus token #3184

[BLOCKED] Save executed fee in surplus token #3184

sunce86 commented Dec 26, 2024 •

edited

Loading

squadgazzz commented Dec 26, 2024

sunce86 commented Dec 26, 2024

squadgazzz Dec 27, 2024

squadgazzz left a comment

sunce86 commented Dec 27, 2024 •

edited

Loading

squadgazzz commented Dec 30, 2024 •

edited

Loading

MartinquaXD left a comment

MartinquaXD Dec 31, 2024

sunce86 Jan 3, 2025

MartinquaXD Dec 31, 2024

MartinquaXD Dec 31, 2024

MartinquaXD Dec 31, 2024

mstrug Jan 2, 2025

mstrug Jan 2, 2025 •

edited

Loading

mstrug Jan 2, 2025

		/// Breakdown of protocol fees.
		pub protocol: Vec<ExecutedProtocolFee>,

[BLOCKED] Save executed fee in surplus token #3184

Are you sure you want to change the base?

[BLOCKED] Save executed fee in surplus token #3184

Conversation

sunce86 commented Dec 26, 2024 • edited Loading

Description

Follow-Up Tasks

Migrate Historical Fees to Surplus Token

Fix Remaining Issue

Changes

How to test

squadgazzz commented Dec 26, 2024

sunce86 commented Dec 26, 2024

Choose a reason for hiding this comment

squadgazzz left a comment

Choose a reason for hiding this comment

sunce86 commented Dec 27, 2024 • edited Loading

squadgazzz commented Dec 30, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstrug Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 commented Dec 26, 2024 •

edited

Loading

sunce86 commented Dec 27, 2024 •

edited

Loading

squadgazzz commented Dec 30, 2024 •

edited

Loading

mstrug Jan 2, 2025 •

edited

Loading