Show JIT orders on COW explorer #2920

sunce86 · 2024-08-22T11:30:46Z

Description

Fixes #2825

Changes

Added jit_orders database table and read/write functions
Refactored domain::Settlement so that user orders and JIT orders can be differentiated using Trade enum.
Saved JIT orders from OnSettlementEventUpdater
Return jit orders on /get_order and /get_trades

Todo in follow up PR: return on get_order_by_tx and get_order_by_owner

How to test

Updated unit tests for surplus/fee calculation
Added one new unit test for settlement with cow amm.
Added unit test for read/write to the new database table.
Jit order expanded to prove that jit orders are returned on endpoints.

I would suggest to review in IDE, since it's not easy to follow the changes as code diff.

crates/database/src/jit_orders.rs

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

fleupold

I have almost only nits. The only launchblocking comment for me is the FullJitOrder type which I think should be removed.

The test looks great!

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

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

fleupold · 2024-09-02T18:28:38Z

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

-    /// Adjusted uniform prices to account for fees (gas cost and protocol fees)
-    pub custom: ClearingPrices,
-}
-


any reason? I personally think clearing prices make more sense in the context of a trade than in the context of a transaction?

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

fleupold · 2024-09-02T18:35:47Z

crates/database/src/jit_orders.rs

+/// Jit order combined with trades table and order_execution table, suitable for
+/// API responses.
+#[derive(Debug, Clone, Default, PartialEq, sqlx::FromRow)]
+pub struct FullJitOrder {


It looks like this type is not needed. The API expects a orders::FullOrder. Instead of creating a new type (that has a very vague "Full" prefix which isn't really expressive) can we just explicitly return Option<orders::FullOrder> here (it's also not easy to find where the From trait is called and there is no strong reason to use this abstraction because the type is never used in a generic Vec or the like)

If you want this then conversion from FullJitOrder to orders::FullOrder is either:

Part of sql query (hardcoded) which is not reusable and not compiler checked.

Directly implemented inside single_full_jit_order to avoid defining FullJitOrder struct, which again in not reusable.

Reusability is important since in a follow up PR I will implement similar function to orders::full_orders_in_tx that will reuse this.

I ended refactoring so that FullJitOrder is now private function and conversion is done inside jit_orders::single_full_jit_order which resolves most of your concerns.

crates/database/src/trades.rs

fleupold · 2024-09-02T18:40:48Z

crates/e2e/src/setup/solver/solution.rs

@@ -45,47 +45,58 @@ impl JitOrder {
        signing_scheme: EcdsaSigningScheme,
        domain: &DomainSeparator,
        key: SecretKeyRef,
-    ) -> solvers_dto::solution::JitOrder {
+    ) -> (solvers_dto::solution::JitOrder, OrderUid) {


nanonit: uid is not dependent on signing and should (it's a pure coincidence that this method is called in the test where it is used, and happens to also depend on DomainSeperator). It makes more sense to have this as a standalone method imo.

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

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

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

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

MartinquaXD · 2024-09-02T18:53:30Z

crates/database/src/jit_orders.rs

+    pub executed_surplus_fee: BigDecimal,
+}
+
+/// 1:1 mapping to the `jit_orders` table, used to store orders.


This type does not have to be leaked.
It could either be declared in the single_full_jit_order() function or probably better you could design the query such that we can directly deserialize the row into the regular full order struct.

this one is used for storing orders as well

answer might be related to #2920 (comment)

MartinquaXD · 2024-09-02T18:56:50Z

crates/database/src/jit_orders.rs

+            .push_bind(jit_order.buy_token_balance);
+    });
+
+    query_builder.push(


Why is a simple insert no sufficient here?

not following? Ilya previously suggested to have a function that inserts batch of jit orders.

I mean why do we need to manage conflicts on inserts here?

I think this is a leftover from when the OnSettlementEventUpdater was doing reindexing and in case of some bug in event indexing logic we might get stuck on a same insert.
I am also fine putting ON CONFLICT DO NOTHING if it seems cleaner in this case?

crates/database/src/settlements.rs

crates/e2e/src/setup/solver/solution.rs

database/README.md

m-lord-renkse · 2024-09-03T07:47:21Z

crates/autopilot/src/domain/eth/mod.rs

@@ -263,7 +263,7 @@ pub struct Event {
 }

 /// Any type of on-chain transaction.
-#[derive(Debug)]
+#[derive(Debug, Clone, Default)]


Why do you need the Default here, and in the structs above?

needed for settlement unit tests

in that case, we could use #[cfg_attr(test, derive(Default))]. In production code, those struct shouldn't have a default 🤔

Ah, I see now that I accidentally resolved this comment without fixing. Will address it in another PR.

fleupold

Nothing blocking from my side, just naming nits.

fleupold · 2024-09-03T18:46:38Z

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

-    /// Adjusted uniform prices to account for fees (gas cost and protocol fees)
-    pub custom: ClearingPrices,
-}
-


I wouldn't have put it in math::Trade but in the highlevel thing that hosts the enum. But also no strong feeling

fleupold · 2024-09-03T18:49:25Z

crates/database/src/jit_orders.rs

+    },
+};
+
+pub async fn single_full_jit_order(


nit: why not

Suggested change

pub async fn single_full_jit_order(

pub async fn fetch(

so it will be called via database::jit_orders::fetch(...) or by_uid or something elegant.

fleupold · 2024-09-03T18:51:40Z

crates/database/src/jit_orders.rs

+/// Jit order combined with trades table and order_execution table, suitable for
+/// API responses.
+#[derive(Debug, Clone, Default, PartialEq, sqlx::FromRow)]
+struct FullJitOrder {


nit

Suggested change

struct FullJitOrder {

struct JitOrderWithExecutions {

Full is a bit vague

squadgazzz

LGTM

sunce86 added 30 commits July 25, 2024 14:31

Initial commit

6db9266

initial commit

a6158db

simplification

ea010ec

update table

79b02ce

Merge branch 'main' into jit-orders-database

5fede7c

delete old

5a90766

update comment

6a12f09

add index on block_number

e7feb38

add read/write functions

79b63e1

saving jit orders

64882aa

fix syntax

2075b9b

rename table

1ebfa38

syntax fix 2

0396d84

fix index

0c010e2

add unique constraint

6d4789e

saving of jit orders

d30901e

Merge branch 'main' into jit-orders-database

419a36f

update jit orders fetching

35e018f

fix tests

9cf4e61

Settlement is not always valid

cda884b

simplify jit

895bd28

fix roundtrip

7778a24

clippy

a572a25

add fee amount

09e23d8

add timestamp

3d7121c

converting jit order to database

29faf16

refactor

5d181d1

forgot to bind

7b03bb7

remove order.rs

2df2620

refactor into trade enum

ab9caa9

sunce86 added 2 commits September 2, 2024 12:49

update test

7ed30d7

small fixes

c1863a5

sunce86 requested review from fleupold, MartinquaXD and squadgazzz September 2, 2024 11:12

squadgazzz reviewed Sep 2, 2024

View reviewed changes

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

check existing jit orders test

8ca1fe6

squadgazzz reviewed Sep 2, 2024

View reviewed changes

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

squadgazzz reviewed Sep 2, 2024

View reviewed changes

crates/autopilot/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved

squadgazzz reviewed Sep 2, 2024

View reviewed changes

crates/autopilot/src/infra/persistence/mod.rs Outdated Show resolved Hide resolved

fleupold reviewed Sep 2, 2024

View reviewed changes

MartinquaXD reviewed Sep 2, 2024

View reviewed changes

m-lord-renkse reviewed Sep 3, 2024

View reviewed changes

sunce86 added 3 commits September 3, 2024 09:59

cr fixes

46cd46e

more cr fixes

24e690a

FullJitOrder is private

37d210d

sunce86 requested review from fleupold, MartinquaXD, squadgazzz and m-lord-renkse September 3, 2024 09:25

fleupold approved these changes Sep 3, 2024

View reviewed changes

MartinquaXD approved these changes Sep 3, 2024

View reviewed changes

squadgazzz approved these changes Sep 4, 2024

View reviewed changes

sunce86 added 3 commits September 4, 2024 09:46

renaming

2b2c1f3

Merge branch 'main' into jit-orders-database-v2

0248821

update db unit test

2d921c2

sunce86 enabled auto-merge (squash) September 4, 2024 07:59

sunce86 merged commit 41334ea into main Sep 4, 2024
10 checks passed

sunce86 deleted the jit-orders-database-v2 branch September 4, 2024 08:03

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show JIT orders on COW explorer #2920

Show JIT orders on COW explorer #2920

sunce86 commented Aug 22, 2024 •

edited

Loading

fleupold left a comment

fleupold Sep 2, 2024

fleupold Sep 2, 2024

sunce86 Sep 3, 2024 •

edited

Loading

fleupold Sep 2, 2024

MartinquaXD Sep 2, 2024

sunce86 Sep 3, 2024

sunce86 Sep 3, 2024

MartinquaXD Sep 2, 2024

sunce86 Sep 3, 2024

MartinquaXD Sep 3, 2024 •

edited

Loading

sunce86 Sep 3, 2024

m-lord-renkse Sep 3, 2024

sunce86 Sep 3, 2024

m-lord-renkse Sep 3, 2024

sunce86 Sep 5, 2024

fleupold left a comment

fleupold Sep 3, 2024

fleupold Sep 3, 2024

fleupold Sep 3, 2024

squadgazzz left a comment

Show JIT orders on COW explorer #2920

Show JIT orders on COW explorer #2920

Conversation

sunce86 commented Aug 22, 2024 • edited Loading

Description

Changes

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

sunce86 Sep 3, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinquaXD Sep 3, 2024 • 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

Choose a reason for hiding this comment

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

squadgazzz left a comment

Choose a reason for hiding this comment

sunce86 commented Aug 22, 2024 •

edited

Loading

sunce86 Sep 3, 2024 •

edited

Loading

MartinquaXD Sep 3, 2024 •

edited

Loading