Baseline solver limit orders support #2326

squadgazzz · 2024-01-24T18:06:45Z

Description

Adds support of 0x LimitOrder to Baseline solver in order to test 0x liquidity properly in e2e tests.

How to test

Added unit tests.

Related Issues

#1666

MartinquaXD · 2024-01-25T09:24:30Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+
+impl BaselineSolvable for LimitOrder {
+    fn get_amount_out(&self, out_token: H160, (in_amount, in_token): (U256, H160)) -> Option<U256> {
+        if in_token == self.taker.token.0 && out_token == self.maker.token.0 {


The meaning of in_token and out_token depends on whether we solve a sell or a buy order.
That needs to be taken into account here and in the other code.

Looks like it is already taken into account:

services/crates/solvers/src/boundary/baseline.rs

Lines 47 to 96 in 2e18b36

let (segments, _) = match request.side {

order::Side::Buy => candidates

.iter()

.filter_map(|path| {

let sell = baseline_solver::estimate_sell_amount(

request.buy.amount,

path,

&self.amms,

)?;

let segments =

self.traverse_path(&sell.path, request.sell.token.0, sell.value)?;

let buy = segments.last().map(|segment| segment.output.amount);

if buy.map(|buy| buy >= request.buy.amount) != Some(true) {

tracing::warn!(

?request,

?segments,

"invalid buy estimate does not cover order"

);

return None;

}

(sell.value <= request.sell.amount).then_some((segments, sell))

})

.min_by_key(|(_, sell)| sell.value)?,

order::Side::Sell => candidates

.iter()

.filter_map(|path| {

let buy = baseline_solver::estimate_buy_amount(

request.sell.amount,

path,

&self.amms,

)?;

let segments =

self.traverse_path(&buy.path, request.sell.token.0, request.sell.amount)?;

let sell = segments.first().map(|segment| segment.input.amount);

if sell.map(|sell| sell >= request.sell.amount) != Some(true) {

tracing::warn!(

?request,

?segments,

"invalid sell estimate does not cover order"

);

return None;

}

(buy.value >= request.buy.amount).then_some((segments, buy))

})

.max_by_key(|(_, buy)| buy.value)?,

};

MartinquaXD · 2024-01-25T09:28:30Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+        if out_token == self.maker.token.0 && in_token == self.taker.token.0 {
+            calculate_amount_in(out_amount, self.maker.amount, self.taker.amount, &self.fee)
+        } else {


Looks like these functions don't get reused anywhere else.
Seems a little easier to read if you just have an early return here if the tokens don't match and then hoist the actual logic into this function here.

Refactored a little bit

fleupold

Approach in general looks correct! Excited to get 0x support in baseline...

fleupold · 2024-01-25T09:58:24Z

crates/solvers/src/boundary/baseline.rs

+                    if let Some(token_pair) =
+                        TokenPair::new(limit_order.maker.token.0, limit_order.taker.token.0)
+                    {
+                        amms.entry(token_pair).or_default().push(Amm {


nit: Amm is no longer the right name I believe. More like OnchainLiquidity and source instead of pool maybe?

Makes sense, renamed.

fleupold · 2024-01-25T10:00:56Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+    fee: &TakerAmount,
+) -> Option<U256> {
+    let fee_adjusted_amount = in_amount.checked_sub(fee.0)?;
+    let fee_adjusted_amount_bfp = Bfp::from_wei(fee_adjusted_amount);


I'm a bit surprised we are using balance Bfp logic here. Why is that?

Used it as a faster approach since the initial attempt with U256 ended up with a more complicated implementation. Removed the Bfp completely.

fleupold · 2024-01-25T10:02:03Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+        .div_down(Bfp::from_wei(taker_amount))
+        .ok()?;
+    let scaled_out_amount_bfp = fee_adjusted_amount_bfp.mul_down(scaled_price_ratio).ok()?;
+    scaled_out_amount_bfp


Don't we need to check here if the 0x LimitOrder is partially fillable and if taker_amount >= in_amount?

Is it possible to understand from the current fields if it is partial-fillable or not? https://0x.org/docs/0x-orderbook-api/api-references/overview#signed-order

IIRC all 0x limit orders are partially fillable.

fleupold · 2024-01-25T10:04:54Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+
+    #[test]
+    fn test_amount_out_in_round_trip() {
+        let maker_amount: u32 = 321;


Are limit orders really using these type of amounts? Aren't they expressed in wei? Maybe using an actual example order could help (but I guess we are doing this in the e2e test).

Switched to wei

MartinquaXD · 2024-01-25T20:35:28Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+        }
+
+        let fee_adjusted_amount = in_amount.checked_sub(self.fee.0)?;
+        if self.maker.amount > self.taker.amount {


Why does it make a difference here whether taker.amount or maker.amount amount is bigger?
My understanding is that from our perspective makerToken == buyToken and takerToken == sellToken.
get_amount_out() basically computes given some in_amount how much outToken would the liquidity source be willing to sell.
If I'm not mistaken it should not have any influence on the math whether takerAmount or makerAmount is bigger.

Same thought on the other function.

We do not operate with floating point numbers here. Consider an example where the ratio is 0.5, where in fact, we will get 0. As an alternative to avoid that, we should multiply the amounts that could lead to U256 overflow.

I see. 👍
You could also get around that by not computing the ratio first and instead do all operations in order which is what we usually do:

fee_adjusted_amount * make.amount / taker.amount

If you want to be super careful in order to avoid overflows on the multiplication you could even do a.full_mul(b) which promotes the result to U512.
One thing to note throughout all this is that we should try to mimic what the actual smart contract does as closely as possible. Imagine they have an error in their equation or even some rounding issues. Then it does us no good if we have a theoretically better implementation if a solution generated by using it ends up reverting.

Also let's add a small comment in the code explaining this non trivial rationale...

True, tried to follow the contract's implementation: https://github.com/0xProject/protocol/blob/e66307ba319e8c3e2a456767403298b576abc85e/contracts/zero-ex/contracts/src/features/native_orders/NativeOrdersSettlement.sol#L469-L509

MartinquaXD · 2024-01-25T20:50:46Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+    }
+
+    fn gas_cost(&self) -> usize {
+        0


We have an approximation for that that we could use here. Although conceptually this gas amount should probably be estimated by the driver when it fetches this liquidity. Theoretically we could probably also fetch other limit orders which do not come from 0x and those would have a different execution cost.

Makes sense. Since currently we have liquidity limit orders only from 0x, adding the gas cost fetching could be done as a separate activity, right?

I think we don't need a separate activity for that.
IIRC when we fetch liquidity sources we already assign a cost to each one which refers to their gas cost. For 0x orders we could simply multiply their fee by the current gas price (cached and already updated in the background). Then when the limit order liquidity source could simply return the gas cost previously assigned to it by the liquidity fetching logic.

Edit: Actually we should probably only return gas units here which are not affected by the gas price.

crates/solvers/src/boundary/liquidity/limit_order.rs

MartinquaXD · 2024-01-25T20:58:56Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+            return None;
+        }
+
+        let fee_adjusted_amount = in_amount.checked_sub(self.fee.0)?;


Looks like this fee scales with gas price. I'm not sure we have taken that into account anywhere: https://docs.0xprotocol.org/en/latest/basics/protocol_fees.html#protocol-fees

Ah, does it mean we have to fetch the gas price each time we fetch 0x's limit orders from the orderbook?

Btw, taker's token fee is not scaled AFAIU. And the protocol fee should not be considered while calculating in/out amounts.

If I understand the contract code correctly the takerTokenFeeAmount gets deducted after the order gets partially filled and does not reduce the available fill amount so I think it does not even have any influence on the math here.
Additionally the protocol fee was decided to be 0 according to the page I linked so I believe the logic is actually just in_amount * maker_amount / taker_amount.

That being said I always find solidity code particularly hard to reason about so I might be wrong here. If there is still doubt about this checking out a tenderly simulation for a 0x fill would help. Also if this math is off we should detect that with the e2e tests.

Ok, simplified the formula to align with the one used in the smart contract. Added this temp test with the values from this tx. The second assertion fails due to precision issue, which is expected, I believe.

fleupold · 2024-01-26T08:25:19Z

crates/solvers/src/boundary/baseline.rs

@@ -13,7 +13,7 @@ use {

 pub struct Solver<'a> {
    base_tokens: BaseTokens,
-    amms: HashMap<TokenPair, Vec<Amm>>,
+    amms: HashMap<TokenPair, Vec<OnchainLiquidity>>,


What's the difference between amms and liquidity? Does it make sense to combine the two somehow (maybe in a separate PR)? Also amms should probably be renamed.

Renamed and created an issue #2332

fleupold · 2024-01-26T08:30:03Z

crates/solvers/src/boundary/liquidity/limit_order.rs

+        }
+
+        let fee_adjusted_amount = in_amount.checked_sub(self.fee.0)?;
+        if self.maker.amount > self.taker.amount {


Also let's add a small comment in the code explaining this non trivial rationale...

MartinquaXD

Still not super sure about this code.
If you want to merge it as is I recommend watching the staging baseline simulations closely because an error in this code can lead to failing solutions or wait for the e2e test to 100% confirm the correctness of this code before merging both PRs together.

crates/e2e/src/nodes/forked_node.rs

fleupold · 2024-02-22T17:47:17Z

crates/e2e/tests/e2e/liquidity.rs

+            maker_amount: order_creation.buy_amount.as_u128() * 3,
+            taker_amount: order_creation.sell_amount.as_u128() * 2,


~~🤔 to me it seems that the inequality in the check should be a <= instead of <.~~

Never mind. Is it possible that the fee amount needs to be taken care of here? How big is the discrepancy exactly?

Why even multiply with 2 (maybe add a comment)?

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

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.

…rders-support # Conflicts: # crates/e2e/tests/e2e/liquidity.rs

…rders-support # Conflicts: # Cargo.lock

squadgazzz · 2024-04-02T16:57:03Z

Why even multiply with 2 (maybe add a comment)?

Otherwise, the solution is discarded since the simulation is reverted on-chain. Gonna need to reproduce it on Tenderly.

MartinquaXD

The diff is a bit tricky to review as it is. Maybe it makes sense to have this plus the PR introducing the 0x liquidity as one for reviewing purposes?

crates/e2e/tests/e2e/liquidity.rs

crates/solvers/src/boundary/baseline.rs

MartinquaXD · 2024-04-02T21:23:37Z

crates/e2e/tests/e2e/liquidity.rs

+            maker_amount: order_creation.buy_amount.as_u128() * 3,
+            taker_amount: order_creation.sell_amount.as_u128() * 2,


Maybe instead of adjusting the amount with these factors it's a better idea to make the order partially fillable. That way we don't have to have a comment for all the factors but only when we assert that the traded amounts look good in the end.

crates/e2e/src/setup/colocation.rs

squadgazzz · 2024-04-03T14:49:08Z

Indeed, it's hard to review at the moment, I will merge it into the e2e test original branch.

Baseline LimitOrder support

2e18b36

squadgazzz changed the base branch from main to e2e/0x-liquidity-fetching January 24, 2024 18:06

Precision fix

4b42119

MartinquaXD reviewed Jan 25, 2024

View reviewed changes

fleupold reviewed Jan 25, 2024

View reviewed changes

squadgazzz added 4 commits January 25, 2024 14:16

Bfp -> U256

c3d45a2

Refactoring

2a2a8aa

More tests

cffd9c0

Naming

381416d

squadgazzz force-pushed the e2e/0x-liquidity-fetching branch from 0ae866e to 86be63e Compare January 25, 2024 18:20

squadgazzz marked this pull request as ready for review January 25, 2024 18:20

squadgazzz requested a review from a team as a code owner January 25, 2024 18:20

squadgazzz added 2 commits January 25, 2024 18:25

Redundant function

0eb67d2

Adjusted value

dce04b6

MartinquaXD reviewed Jan 25, 2024

View reviewed changes

Clean-up

a830885

squadgazzz force-pushed the baseline-solver-limit-orders-support branch from f5d0564 to a830885 Compare January 25, 2024 21:45

fleupold reviewed Jan 26, 2024

View reviewed changes

squadgazzz added 2 commits January 26, 2024 09:08

Align calculations with 0x smart contract

621ca08

Naming

ec9887d

squadgazzz mentioned this pull request Jan 26, 2024

chore: Optimize Liquidity and OnchainLiquidity in solvers #2332

Closed

1 task

fleupold approved these changes Jan 26, 2024

View reviewed changes

MartinquaXD approved these changes Jan 26, 2024

View reviewed changes

Adjust formula

67dba54

squadgazzz force-pushed the baseline-solver-limit-orders-support branch from 43e7efe to 67dba54 Compare February 1, 2024 11:34

squadgazzz added 3 commits February 1, 2024 12:58

Minor test fixes

c86ae17

Domain separator fix

b8cfde1

Fixed hash structure

bea923c

squadgazzz force-pushed the baseline-solver-limit-orders-support branch from 3a844d6 to 6534d5c Compare February 5, 2024 13:55

squadgazzz marked this pull request as draft February 9, 2024 11:09

Fork url env name

a3167b1

squadgazzz force-pushed the baseline-solver-limit-orders-support branch from 0920ca5 to a3167b1 Compare February 21, 2024 13:22

Config fix

3a6b13e

fleupold reviewed Feb 22, 2024

View reviewed changes

squadgazzz marked this pull request as draft February 23, 2024 16:15

github-actions bot added the stale label Mar 2, 2024

github-actions bot closed this Mar 10, 2024

squadgazzz reopened this Apr 1, 2024

squadgazzz added 6 commits April 1, 2024 20:12

Merge branch 'e2e/0x-liquidity-fetching' into baseline-solver-limit-o…

de418ea

…rders-support # Conflicts: # crates/e2e/tests/e2e/liquidity.rs

Missing app_data field

e818b6d

Merge branch 'e2e/0x-liquidity-fetching' into baseline-solver-limit-o…

49df1fb

…rders-support # Conflicts: # Cargo.lock

Redundant import

f10fed0

Private field

6bcfbac

Fix after merge

c27296d

github-actions bot removed the stale label Apr 2, 2024

squadgazzz added 3 commits April 2, 2024 15:36

Native 0x liquidity orders

fd7880f

Comments

68e7da3

Redundant mock

4de0d47

squadgazzz marked this pull request as ready for review April 2, 2024 17:00

squadgazzz requested a review from fleupold April 2, 2024 17:42

MartinquaXD reviewed Apr 2, 2024

View reviewed changes

squadgazzz force-pushed the baseline-solver-limit-orders-support branch 2 times, most recently from 5c93888 to 7e061d8 Compare April 3, 2024 14:55

Review fixes

7e061d8

squadgazzz merged commit c37ee3c into e2e/0x-liquidity-fetching Apr 3, 2024
9 checks passed

squadgazzz deleted the baseline-solver-limit-orders-support branch April 3, 2024 15:02

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

	let (segments, _) = match request.side {
	order::Side::Buy => candidates
	.iter()
	.filter_map(\|path\| {
	let sell = baseline_solver::estimate_sell_amount(
	request.buy.amount,
	path,
	&self.amms,
	)?;
	let segments =
	self.traverse_path(&sell.path, request.sell.token.0, sell.value)?;

	let buy = segments.last().map(\|segment\| segment.output.amount);
	if buy.map(\|buy\| buy >= request.buy.amount) != Some(true) {
	tracing::warn!(
	?request,
	?segments,
	"invalid buy estimate does not cover order"
	);
	return None;
	}

	(sell.value <= request.sell.amount).then_some((segments, sell))
	})
	.min_by_key(\|(_, sell)\| sell.value)?,
	order::Side::Sell => candidates
	.iter()
	.filter_map(\|path\| {
	let buy = baseline_solver::estimate_buy_amount(
	request.sell.amount,
	path,
	&self.amms,
	)?;
	let segments =
	self.traverse_path(&buy.path, request.sell.token.0, request.sell.amount)?;

	let sell = segments.first().map(\|segment\| segment.input.amount);
	if sell.map(\|sell\| sell >= request.sell.amount) != Some(true) {
	tracing::warn!(
	?request,
	?segments,
	"invalid sell estimate does not cover order"
	);
	return None;
	}

	(buy.value >= request.buy.amount).then_some((segments, buy))
	})
	.max_by_key(\|(_, buy)\| buy.value)?,
	};

		maker_amount: order_creation.buy_amount.as_u128() * 3,
		taker_amount: order_creation.sell_amount.as_u128() * 2,

Baseline solver limit orders support #2326

Baseline solver limit orders support #2326

Conversation

squadgazzz commented Jan 24, 2024 • edited Loading

Description

How to test

Related Issues

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

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

squadgazzz Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

MartinquaXD Jan 25, 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

MartinquaXD Jan 26, 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

MartinquaXD Jan 26, 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

MartinquaXD left a comment

Choose a reason for hiding this comment

fleupold Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 2, 2024

squadgazzz commented Apr 2, 2024

MartinquaXD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squadgazzz commented Apr 3, 2024

squadgazzz commented Jan 24, 2024 •

edited

Loading

squadgazzz Jan 25, 2024 •

edited

Loading

MartinquaXD Jan 25, 2024 •

edited

Loading

MartinquaXD Jan 26, 2024 •

edited

Loading

MartinquaXD Jan 26, 2024 •

edited

Loading

fleupold Feb 22, 2024 •

edited

Loading