Implement 0x liquidity #2218

squadgazzz · 2023-12-27T20:09:59Z

Description

Removes all the 0x liquidity-related todos.

Changes

Some functionality is copied from the solver crate to reduce the dependencies count.

How to test

#2253

Related Issues

Fixes #1666

fleupold · 2023-12-28T07:01:46Z

crates/driver/src/boundary/liquidity/zeroex.rs

+    solver::{
+        liquidity::{zeroex::ZeroExLiquidity, LimitOrder, LimitOrderExecution},
+        liquidity_collector::LiquidityCollecting,
+        settlement::SettlementEncoder,
+    },


How much work would it be to not depend on this code but instead port what we need into the driver?
While not strictly required to achieve what we need here, it's covered by #2215 so we might as well start it here if it's not too much work.

LiquidityCollecting is currently not possible to move only for 0x since this trait is used for all other liquidity providers. LimitOrder is part of it as well too.

ZeroExLiquidity probably can be moved, but it contains a lot of code and dependencies to the solver crate. It would be better to try in a separate PR. SettlementEncoder and LimitOrderExecution are removed.

squadgazzz · 2023-12-28T18:11:36Z

crates/driver/src/boundary/liquidity/zeroex.rs

+    let handler = pool
+        .settlement_handling
+        .as_any()
+        .downcast_ref::<solver::liquidity::zeroex::OrderSettlementHandler>()
+        .ok_or(anyhow!("not a zeroex::OrderSettlementHandler"))?
+        .clone();


I tried to avoid that, but we need to provide the contract and order somehow. Currently, it's not possible to add zeroex-specific fields right to the solver::LimitOrder since it's used with different settlement handlers. To address it, looks like the whole solver::Liquidity should be moved to the driver crate first.

squadgazzz · 2023-12-28T18:12:27Z

crates/solvers/openapi.yml

-        hash:
-          $ref: "#/components/schemas/Digest"


Turned out that is not currently used at all. And also not sure how to populate it.

squadgazzz · 2023-12-28T18:14:55Z

crates/driver/src/domain/liquidity/zeroex.rs

+    pub sell_token: H160,
+    pub buy_token: H160,
+    pub sell_amount: U256,
+    pub buy_amount: U256,
+    pub order: Order,


order already contains sell/buy token/amount, but the values might differ from the LimitOrder's: 1, 2.

How so? I think only [2] is relevant for us (the other one is a CoW native limit order which is represented differently). I think all we need is Order and IZeroEx.

Even full_execution_amount we should be able to pass along in to_interaction (using input/output fields on solution::interaction::Liquidity) since this would allow solvers to use this liquidity partially (if we set the full execution amount in to_domain we commit to the amount that needs to be executed before solving).

If I'm not mistaken (docs), takerTokenFillAmount should be what we call inputs:

services/crates/driver/src/domain/competition/solution/interaction.rs

Lines 24 to 26 in 1647ad8

/// The assets consumed by this interaction. These assets are taken from the

/// settlement contract when the interaction executes.

pub fn inputs(&self) -> Vec<eth::Asset> {

If I'm not mistaken (docs), takerTokenFillAmount should be what we call inputs:

Correct me if I got it wrong:

services/crates/driver/src/domain/liquidity/zeroex.rs

Line 58 in 5184697

input.0.amount.0.as_u128(),

And here, should we also use input from Liquidity or order's value?

services/crates/driver/src/infra/solver/dto/auction.rs

Line 212 in 5184697

taker_token_fee_amount: limit_order.order.taker_token_fee_amount.into(),

squadgazzz · 2023-12-28T18:16:10Z

crates/driver/src/infra/solver/dto/auction.rs

+                        id: liquidity.id.0,
+                        address: pool.zeroex.address(),
+                        gas_estimate: liquidity.gas.into(),
+                        maker_token: pool.sell_token,
+                        taker_token: pool.buy_token,
+                        maker_amount: pool.sell_amount,
+                        taker_amount: pool.buy_amount,
+                        taker_token_fee_amount: pool.order.taker_token_fee_amount.into(),


Not 100% sure the proper values are used here.

squadgazzz · 2023-12-28T18:43:06Z

crates/driver/src/domain/liquidity/zeroex.rs

+impl LimitOrder {
+    pub fn to_interaction(&self) -> anyhow::Result<eth::Interaction> {
+        let method = self.zeroex.fill_or_kill_limit_order(
+            (
+                self.order.maker_token,
+                self.order.taker_token,
+                self.order.maker_amount,
+                self.order.taker_amount,
+                self.order.taker_token_fee_amount,
+                self.order.maker,
+                self.order.taker,
+                self.order.sender,
+                self.order.fee_recipient,
+                Bytes(self.order.pool.0),
+                self.order.expiry,
+                self.order.salt,
+            ),
+            (
+                self.order.signature_type,
+                self.order.signature_v,
+                Bytes(self.order.signature_r.0),
+                Bytes(self.order.signature_s.0),
+            ),
+            self.full_execution_amount.as_u128(),
+        );
+        let calldata = method.tx.data.ok_or(anyhow!("no calldata"))?;
+
+        Ok(eth::Interaction {
+            target: self.zeroex.address().into(),
+            value: 0.into(),
+            call_data: calldata.0.into(),
+        })
+    }
+}


Moved from https://github.com/cowprotocol/services/blob/06ae435fed087424edf9e057808d2bb870f0527f/crates/solver/src/interactions/zeroex.rs

fleupold

Looks good! I think we can simplify the LimitOrder type much more and should document the downcasting tradeoff inside the code.

A bit of a bummer this is so hard to test, but I guess we have to mock a ZeroX API in e2e tests and write code to underwrite our own 0x order which sounds very involved.

fleupold · 2023-12-29T09:11:39Z

crates/driver/src/boundary/liquidity/zeroex.rs

+    let handler = limit_order
+        .settlement_handling
+        .as_any()
+        .downcast_ref::<solver::liquidity::zeroex::OrderSettlementHandler>()


😬 hack alert. Can we add a comment in the code on why we are chosing this workaround and how a proper solution would look like?

fleupold · 2023-12-29T09:23:03Z

crates/driver/src/domain/liquidity/zeroex.rs

+    pub sell_token: H160,
+    pub buy_token: H160,
+    pub sell_amount: U256,
+    pub buy_amount: U256,
+    pub order: Order,


How so? I think only [2] is relevant for us (the other one is a CoW native limit order which is represented differently). I think all we need is Order and IZeroEx.

Even full_execution_amount we should be able to pass along in to_interaction (using input/output fields on solution::interaction::Liquidity) since this would allow solvers to use this liquidity partially (if we set the full execution amount in to_domain we commit to the amount that needs to be executed before solving).

If I'm not mistaken (docs), takerTokenFillAmount should be what we call inputs:

services/crates/driver/src/domain/competition/solution/interaction.rs

Lines 24 to 26 in 1647ad8

/// The assets consumed by this interaction. These assets are taken from the

/// settlement contract when the interaction executes.

pub fn inputs(&self) -> Vec<eth::Asset> {

fleupold · 2023-12-29T09:28:43Z

crates/driver/src/domain/liquidity/zeroex.rs

+}
+
+impl LimitOrder {
+    pub fn to_interaction(&self) -> anyhow::Result<eth::Interaction> {


squadgazzz · 2023-12-29T12:21:21Z

A bit of a bummer this is so hard to test, but I guess we have to mock a ZeroX API in e2e tests and write code to underwrite our own 0x order which sounds very involved.

# Conflicts: # crates/solvers/openapi.yml

MartinquaXD · 2024-01-15T07:35:00Z

crates/driver/src/domain/liquidity/zeroex.rs

-pub struct LimitOrder {}
+pub struct LimitOrder {
+    pub order: Order,
+    pub zeroex: IZeroEx,


I believe other liquidity implementations would have the IZeroEx struct via an Arc because it's pretty big and always identical.

MartinquaXD · 2024-01-15T07:35:38Z

crates/driver/src/domain/liquidity/zeroex.rs

+    pub signature_type: u8,
+    pub signature_r: H256,
+    pub signature_s: H256,
+    pub signature_v: u8,


Would be nice to collapse that into a Signature struct.

MartinquaXD · 2024-01-15T07:37:29Z

crates/solver/src/liquidity/zeroex.rs

+#[derive(Clone)]
+pub struct OrderSettlementHandler {
+    pub order: Order,
+    pub zeroex: IZeroEx,


This IZeroEx struct could also be Arced.

MartinquaXD · 2024-01-15T07:42:15Z

crates/driver/src/domain/liquidity/zeroex.rs

+                Bytes(self.order.signature_r.0),
+                Bytes(self.order.signature_s.0),
+            ),
+            input.0.amount.0.as_u128(),


This should probably do .try_into().context("executed amount does not fit into u128") because the liquidity::MaxInput type theoretically allows bigger amounts than 0x can handle.

github-actions · 2024-02-06T00:06:20Z

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.

# Conflicts: # crates/solver/src/interactions/zeroex.rs # crates/solvers/src/api/routes/solve/dto/auction.rs

squadgazzz · 2024-04-03T16:05:44Z

Covered with the test: #2253

crates/driver/src/domain/liquidity/zeroex.rs

crates/solver/src/liquidity/zeroex.rs

Implement 0x liquidity

37facc8

fleupold reviewed Dec 28, 2023

View reviewed changes

squadgazzz force-pushed the fix/1666-2 branch from 2696992 to 8d276bb Compare December 28, 2023 08:52

Minor

7d982bd

squadgazzz force-pushed the fix/1666-2 branch from 8d276bb to 7d982bd Compare December 28, 2023 12:03

squadgazzz added 4 commits December 28, 2023 14:18

Refactoring

f3e38c1

Remaining todos

cb88286

Clean-up

4f23650

Refactoring

3bba068

squadgazzz commented Dec 28, 2023

View reviewed changes

Naming

1647ad8

squadgazzz marked this pull request as ready for review December 28, 2023 18:32

squadgazzz requested a review from a team as a code owner December 28, 2023 18:32

squadgazzz commented Dec 28, 2023

View reviewed changes

fleupold reviewed Dec 29, 2023

View reviewed changes

Review fixes

5184697

squadgazzz marked this pull request as draft January 8, 2024 17:29

Merge branch 'main' into fix/1666-2

a9d7fd5

# Conflicts: # crates/solvers/openapi.yml

MartinquaXD reviewed Jan 15, 2024

View reviewed changes

squadgazzz added 4 commits January 17, 2024 16:14

Merge branch 'main' into fix/1666-2

1fbeba6

IZeroEx via Arc

a459359

Signature field

38de5f9

Safe executed amount conversion

aaf7c29

squadgazzz force-pushed the fix/1666-2 branch from 05e055a to aaf7c29 Compare January 17, 2024 19:20

squadgazzz added the blocked This issue is blocked by some other work label Jan 29, 2024

github-actions bot added stale and removed stale labels Feb 6, 2024

squadgazzz added 3 commits February 21, 2024 12:43

Merge branch 'main' into fix/1666-2

17dd562

Merge branch 'main' into fix/1666-2

5dc0d86

# Conflicts: # crates/solver/src/interactions/zeroex.rs # crates/solvers/src/api/routes/solve/dto/auction.rs

Merge branch 'main' into fix/1666-2

cdef6b6

squadgazzz removed the blocked This issue is blocked by some other work label Apr 3, 2024

Merge branch 'main' into fix/1666-2

8354312

squadgazzz marked this pull request as ready for review April 3, 2024 16:00

squadgazzz requested review from MartinquaXD and fleupold April 3, 2024 16:03

MartinquaXD approved these changes Apr 4, 2024

View reviewed changes

m-lord-renkse reviewed Apr 4, 2024

View reviewed changes

crates/driver/src/domain/liquidity/zeroex.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed Apr 4, 2024

View reviewed changes

crates/driver/src/domain/liquidity/zeroex.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed Apr 4, 2024

View reviewed changes

crates/driver/src/domain/liquidity/zeroex.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed Apr 4, 2024

View reviewed changes

crates/solver/src/liquidity/zeroex.rs Outdated Show resolved Hide resolved

Review fixes

b67d0ee

squadgazzz mentioned this pull request Apr 4, 2024

0x liquidity fetching e2e test #2253

Merged

m-lord-renkse approved these changes Apr 4, 2024

View reviewed changes

Merge branch 'main' into fix/1666-2

5301a49

squadgazzz enabled auto-merge (squash) April 4, 2024 13:54

squadgazzz merged commit 5a6a942 into main Apr 4, 2024
9 checks passed

squadgazzz deleted the fix/1666-2 branch April 4, 2024 13:58

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement 0x liquidity #2218

Implement 0x liquidity #2218

squadgazzz commented Dec 27, 2023 •

edited

Loading

fleupold Dec 28, 2023

squadgazzz Dec 28, 2023 •

edited

Loading

squadgazzz Dec 28, 2023

squadgazzz Dec 28, 2023 •

edited

Loading

squadgazzz Dec 28, 2023 •

edited

Loading

fleupold Dec 29, 2023

squadgazzz Dec 29, 2023 •

edited

Loading

squadgazzz Dec 28, 2023

squadgazzz Dec 28, 2023

fleupold left a comment

fleupold Dec 29, 2023

squadgazzz Dec 29, 2023

fleupold Dec 29, 2023

fleupold Dec 29, 2023

squadgazzz commented Dec 29, 2023

MartinquaXD Jan 15, 2024

MartinquaXD Jan 15, 2024

MartinquaXD Jan 15, 2024

MartinquaXD Jan 15, 2024

github-actions bot commented Feb 6, 2024

squadgazzz commented Apr 3, 2024

	/// The assets consumed by this interaction. These assets are taken from the
	/// settlement contract when the interaction executes.
	pub fn inputs(&self) -> Vec<eth::Asset> {

Implement 0x liquidity #2218

Implement 0x liquidity #2218

Conversation

squadgazzz commented Dec 27, 2023 • edited Loading

Description

Changes

How to test

Related Issues

Choose a reason for hiding this comment

squadgazzz Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

squadgazzz Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz Dec 29, 2023 • edited Loading

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

Choose a reason for hiding this comment

squadgazzz commented Dec 29, 2023

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

github-actions bot commented Feb 6, 2024

squadgazzz commented Apr 3, 2024

squadgazzz commented Dec 27, 2023 •

edited

Loading

squadgazzz Dec 28, 2023 •

edited

Loading

squadgazzz Dec 28, 2023 •

edited

Loading

squadgazzz Dec 28, 2023 •

edited

Loading

squadgazzz Dec 29, 2023 •

edited

Loading