[ON HOLD] Refactor order_execution table #2104

sunce86 · 2023-11-30T18:34:35Z

Description

Reviewing should be started by checking the changes in the order_execution table.
This table contains two columns for fees: surplus_fee and solver_fee, which are populated in a several, very non-understandable ways.

Surplus fee

surplus_fee refers to the fee determined by solvers at the time of solving, exclusively for limit orders. Then we adjust the clearing prices of the settlement to take into consideration this fee (so this is something observable from the calldata itself, we don't need to store at it the time of solving).
Before colocation, order_execution::surplus_fee was populated from solver crate at the time of execution, but we later override it in autopilot (which has a background task for updating the surplus_fee by observing onchain settlement).
After colocation, autopilot set's surplus_fee to None, during execution, but populates it after in the mentioned background task.

Solver fee

solver_fee was added to fix the bug we had for limit orders, where clients of our database were reading the surplus_fee field for limit orders when the surplus_fee was not calculated by solvers but the autopilot, iteratively, and was known upfront (before solving). In the meantime we removed that functionality, and continued using this field as a fee for market orders (which is known upfront, constant and already readable from orders table).

What now?

This PR suggests completely dropping the terminology surplus_fee and solver_fee and merging those into executed_fee.
We rename the order_execution::surplus_fee to order_execution::executed_fee (so we keep the data in that column) and we drop order_execution::solver_fee, but:

For future trades, we can store the fee for market orders into executed_fee, for completness.
For history trades, for backward compatibility, we fallback to reading from orders table. (I might consider executing the script on database to copy that data so we can drop the backward compatibility hack).

How to test

TODO! Since the change is tedious, will need to triple check everything works, but for now the idea seems legit.

fleupold

This is indeed very confusing (great job working through all the code paths). I like the proposal of a combined value!

I'd like to explore the idea of dropping fee reporting entirely from the autopilot/driver and have it be purely based on settlement even indexing. Do you think that would be possible and simplify things further?

fleupold · 2023-12-01T16:22:14Z

crates/autopilot/src/database/competition.rs

@@ -17,19 +17,19 @@ use {

 #[derive(Clone, Debug)]
 pub enum ExecutedFee {
-    /// Fee is calculated by the solver and known upfront (before the settlement
+    /// Fee is determined by user and known upfront (before the settlement


Is this struct even needed in the co-located world? Can't all fees (fixed order fee as well at the surplus fee) be determined by parsing the settlement in the end? Would this simplify the code paths?

fleupold · 2023-12-01T16:25:54Z

crates/autopilot/src/database/competition.rs

    /// is finalized).
-    Solver(U256),
+    UserFee(U256),


nit: it's not really UserDetermined, but rather OrderDetermined?

How about ExecutedFee:Signed vs ExecutedFee::Surplus ?

fleupold · 2023-12-01T16:28:37Z

crates/autopilot/src/decoded_settlement.rs

@@ -312,10 +305,10 @@ impl DecodedSettlement {
                // use every `OrderExecution` exactly once.
                let order = orders.swap_remove(i);

-                // Update fee only for orders with solver computed fees (limit orders)
-                if !order.solver_determines_fee {
+                // Update fee for orders for which is not calculated yet.


Suggested change

// Update fee for orders for which is not calculated yet.

// Update fee for orders for which it is not calculated yet.

fleupold · 2023-12-01T16:29:37Z

crates/autopilot/src/decoded_settlement.rs

@@ -138,15 +137,13 @@ impl From<(Address, U256, Bytes<Vec<u8>>)> for DecodedInteraction {
 #[derive(Debug, Clone)]
 pub struct OrderExecution {
    pub order_uid: OrderUid,
-    pub executed_solver_fee: Option<U256>,
+    pub executed_fee: Option<U256>,


Why does this need to be Option?

Looks like on_settlement_event_updater (which updates the required DB entries when a settlement gets indexed) requires 2 pieces of information:

the full (signed) fee amount collected in total by the settlement

all OrderExecutions that don't have a fee assigned to them yet

These pieces of information can currently be served by the same function Postgres::order_executions_for_tx() which returns all OrderExecutions (with and without a signed fee). For 1 we currently sum all the fees which are already populated and for 2 we use only the executions where executed_fee.is_none().

So refactoring this field to be non-optional might make other code weirder.

While looking into that I stumbled over this code. Could somebody give that a sanity check. My current understanding is that this code is correct because it should only contain the fees that were signed upfront by the user because those are the only fees that are part of the objective value computation. But because fees have become so complicated since we introduced fees that were not signed upfront I think checking that logic once more might be good.

fleupold · 2023-12-01T16:32:22Z

crates/database/src/orders.rs

@@ -586,8 +584,8 @@ WHERE
 #[derive(Debug, Clone, Default, PartialEq, sqlx::FromRow)]
 pub struct OrderExecution {
    pub order_uid: OrderUid,
-    /// The `solver_fee` that got executed for this specific fill.
-    pub executed_solver_fee: Option<BigDecimal>,
+    // Uknown for limit orders before the setlement is finalized.


Suggested change

// Uknown for limit orders before the setlement is finalized.

// Unknown for limit orders before the settlement is finalized.

fleupold · 2023-12-01T16:37:39Z

crates/shared/src/db_order_conversions.rs

+    // Executed fees should be read from the order_execution table, but for
+    // backwards compatibility, for non-limit orders we use the sum of fees from
+    // the order table.


You can make the migration of values part of the flyway migration below.

fleupold · 2023-12-01T16:39:13Z

database/sql/V057__update_order_execution.sql

+ALTER TABLE order_execution 
+  DROP COLUMN solver_fee;
+
+-- Rename surplus_fee to executed_fee as it will represent the general fee paid to the solvers due to network fees.
+-- For market orders, this fee can be fetched from the orders table (but we can keep the duplicate here for completness).
+-- For limit orders, fee is expected to be calculated after the transaction mined, and then this field will be updated from autopilot.
+
+ALTER TABLE order_execution
+  RENAME surplus_fee TO executed_fee;


In case we need to roll back this release, these changes would not be backwards compatible. Would it be possible to create a now column and move the values from the two previous columns into it, and then follow up with another migration a few weeks later, when things have stabilized?

We could do that, but that would require the change in the storing/retrieving functions for this table right? Since we need to also continue populating this new column after the migration. Need to think about it.

sunce86 · 2023-12-02T11:41:26Z

I'd like to explore the idea of dropping fee reporting entirely from the autopilot/driver and have it be purely based on settlement even indexing. Do you think that would be possible and simplify things further?

Yes, that would be the ultimate goal. Based on the types of fees we currently have and how they are calculated, order_execution table should be able to support reindexing and not depend on some specific data at the time of execution.

MartinquaXD · 2023-12-05T08:58:26Z

crates/model/src/solver_competition.rs

-    pub surplus_fee: Option<U256>,
-    #[serde_as(as = "HexOrDecimalU256")]
-    pub solver_fee: U256,


Is there anybody consuming these fields? I.e. is this a relevant breaking change?

MartinquaXD · 2023-12-05T09:03:41Z

database/sql/V057__update_order_execution.sql

+ALTER TABLE order_execution 
+  DROP COLUMN solver_fee;


Don't we have to add the data that currently exists in this column in the surplus_fee column (that gets renamed to executed_fee)?
Otherwise we'd lose data which does not seem right.

sunce86 · 2023-12-29T20:43:54Z

Closing in favor #2190

This only change this PR has different from #2190 is dropping the field in the database, which I will do at the end of fee cleanup.

Refactor order_execution table

0b57e53

sunce86 added the fee_cleanup_temp label Nov 30, 2023

sunce86 requested a review from a team as a code owner November 30, 2023 18:34

fleupold reviewed Dec 1, 2023

View reviewed changes

MartinquaXD reviewed Dec 5, 2023

View reviewed changes

sunce86 changed the title ~~Refactor order_execution table~~ [ON HOLD] Refactor order_execution table Dec 16, 2023

mfw78 removed the fee_cleanup_temp label Dec 18, 2023

sunce86 closed this Dec 29, 2023

github-actions bot locked and limited conversation to collaborators Dec 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ON HOLD] Refactor order_execution table #2104

[ON HOLD] Refactor order_execution table #2104

sunce86 commented Nov 30, 2023

fleupold left a comment

fleupold Dec 1, 2023

fleupold Dec 1, 2023

fleupold Dec 1, 2023

fleupold Dec 1, 2023

MartinquaXD Dec 5, 2023 •

edited

Loading

fleupold Dec 1, 2023

fleupold Dec 1, 2023

fleupold Dec 1, 2023

sunce86 Dec 2, 2023

sunce86 commented Dec 2, 2023

MartinquaXD Dec 5, 2023

MartinquaXD Dec 5, 2023

sunce86 commented Dec 29, 2023

	// Update fee for orders for which is not calculated yet.
	// Update fee for orders for which it is not calculated yet.

	// Uknown for limit orders before the setlement is finalized.
	// Unknown for limit orders before the settlement is finalized.

[ON HOLD] Refactor order_execution table #2104

[ON HOLD] Refactor order_execution table #2104

Conversation

sunce86 commented Nov 30, 2023

Description

Surplus fee

Solver fee

What now?

How to test

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

MartinquaXD Dec 5, 2023 • edited Loading

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

sunce86 commented Dec 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 commented Dec 29, 2023

MartinquaXD Dec 5, 2023 •

edited

Loading