Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Baseline solver limit orders support #2326

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
2e18b36
Baseline LimitOrder support
squadgazzz Jan 24, 2024
4b42119
Precision fix
squadgazzz Jan 24, 2024
c3d45a2
Bfp -> U256
squadgazzz Jan 25, 2024
2a2a8aa
Refactoring
squadgazzz Jan 25, 2024
cffd9c0
More tests
squadgazzz Jan 25, 2024
381416d
Naming
squadgazzz Jan 25, 2024
0eb67d2
Redundant function
squadgazzz Jan 25, 2024
dce04b6
Adjusted value
squadgazzz Jan 25, 2024
a830885
Clean-up
squadgazzz Jan 25, 2024
621ca08
Align calculations with 0x smart contract
squadgazzz Jan 26, 2024
ec9887d
Naming
squadgazzz Jan 26, 2024
67dba54
Adjust formula
squadgazzz Jan 26, 2024
c86ae17
Minor test fixes
squadgazzz Feb 1, 2024
b8cfde1
Domain separator fix
squadgazzz Feb 1, 2024
bea923c
Fixed hash structure
squadgazzz Feb 2, 2024
f848362
Whitelist solvers and fix values
squadgazzz Feb 21, 2024
fca90f4
Merge branch 'e2e/0x-liquidity-fetching' into baseline-solver-limit-o…
squadgazzz Feb 21, 2024
9ad39cd
Redundant log
squadgazzz Feb 21, 2024
a3167b1
Fork url env name
squadgazzz Feb 21, 2024
3a6b13e
Config fix
squadgazzz Feb 21, 2024
de418ea
Merge branch 'e2e/0x-liquidity-fetching' into baseline-solver-limit-o…
squadgazzz Apr 1, 2024
e818b6d
Missing app_data field
squadgazzz Apr 1, 2024
49df1fb
Merge branch 'e2e/0x-liquidity-fetching' into baseline-solver-limit-o…
squadgazzz Apr 1, 2024
f10fed0
Redundant import
squadgazzz Apr 1, 2024
6bcfbac
Private field
squadgazzz Apr 1, 2024
c27296d
Fix after merge
squadgazzz Apr 1, 2024
fd7880f
Native 0x liquidity orders
squadgazzz Apr 2, 2024
68e7da3
Comments
squadgazzz Apr 2, 2024
4de0d47
Redundant mock
squadgazzz Apr 2, 2024
7e061d8
Review fixes
squadgazzz Apr 3, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/solvers/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,5 @@ tracing = { workspace = true }
[dev-dependencies]
glob = "0.3"
tempfile = "3"
hex-literal = { workspace = true }
ethcontract = { workspace = true }
15 changes: 15 additions & 0 deletions crates/solvers/src/boundary/baseline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,17 @@ fn to_boundary_amms(liquidity: &[liquidity::Liquidity]) -> HashMap<TokenPair, Ve
}
}
}
liquidity::State::LimitOrder(limit_order) => {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, renamed.

id: liquidity.id.clone(),
token_pair,
pool: Pool::LimitOrder(limit_order.clone()),
})
}
}
// The baseline solver does not currently support other AMMs.
_ => {}
};
Expand All @@ -206,6 +217,7 @@ enum Pool {
ConstantProduct(boundary::liquidity::constant_product::Pool),
WeightedProduct(boundary::liquidity::weighted_product::Pool),
Stable(boundary::liquidity::stable::Pool),
LimitOrder(liquidity::limit_order::LimitOrder),
}

impl BaselineSolvable for Amm {
Expand All @@ -214,6 +226,7 @@ impl BaselineSolvable for Amm {
Pool::ConstantProduct(pool) => pool.get_amount_out(out_token, input),
Pool::WeightedProduct(pool) => pool.get_amount_out(out_token, input),
Pool::Stable(pool) => pool.get_amount_out(out_token, input),
Pool::LimitOrder(limit_order) => limit_order.get_amount_out(out_token, input),
}
}

Expand All @@ -222,6 +235,7 @@ impl BaselineSolvable for Amm {
Pool::ConstantProduct(pool) => pool.get_amount_in(in_token, out),
Pool::WeightedProduct(pool) => pool.get_amount_in(in_token, out),
Pool::Stable(pool) => pool.get_amount_in(in_token, out),
Pool::LimitOrder(limit_order) => limit_order.get_amount_in(in_token, out),
}
}

Expand All @@ -230,6 +244,7 @@ impl BaselineSolvable for Amm {
Pool::ConstantProduct(pool) => pool.gas_cost(),
Pool::WeightedProduct(pool) => pool.gas_cost(),
Pool::Stable(pool) => pool.gas_cost(),
Pool::LimitOrder(limit_order) => limit_order.gas_cost(),
}
}
}
Expand Down
125 changes: 125 additions & 0 deletions crates/solvers/src/boundary/liquidity/limit_order.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
use {
crate::domain::liquidity::limit_order::{LimitOrder, TakerAmount},
contracts::ethcontract::{H160, U256},
shared::{baseline_solver::BaselineSolvable, sources::balancer_v2::swap::fixed_point::Bfp},
};

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it is already taken into account:

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)?,
};

calculate_amount_out(in_amount, self.maker.amount, self.taker.amount, &self.fee)
} else {
None
}
}

fn get_amount_in(&self, in_token: H160, (out_amount, out_token): (U256, H160)) -> Option<U256> {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored a little bit

None
}
}

fn gas_cost(&self) -> usize {
0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@MartinquaXD MartinquaXD Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

fn calculate_amount_out(
in_amount: U256,
maker_amount: U256,
taker_amount: U256,
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let scaled_maker_amount = Bfp::from_wei(maker_amount).mul_down(Bfp::exp10(18)).ok()?;
let scaled_price_ratio = scaled_maker_amount
.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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC all 0x limit orders are partially fillable.

.div_down(Bfp::exp10(18))
.ok()
.map(|amount| amount.as_uint256())
}

fn calculate_amount_in(
out_amount: U256,
maker_amount: U256,
taker_amount: U256,
fee: &TakerAmount,
) -> Option<U256> {
let scaled_taker_amount = Bfp::from_wei(taker_amount).mul_down(Bfp::exp10(18)).ok()?;
let maker_bfp = Bfp::from_wei(maker_amount);
let scaled_price_ratio = scaled_taker_amount.div_down(maker_bfp).ok()?;
let required_amount_before_scaling =
Bfp::from_wei(out_amount).mul_up(scaled_price_ratio).ok()?;
let required_amount = required_amount_before_scaling
.div_up(Bfp::exp10(18))
.ok()?
.as_uint256();
required_amount.checked_add(fee.0)
}

#[cfg(test)]
mod tests {
use {super::*, crate::domain::eth, contracts::ethcontract::U256, shared::addr};

fn create_limit_order(maker_amount: u32, taker_amount: u32, fee_amount: u32) -> LimitOrder {
let maker = eth::Asset {
amount: U256::from(maker_amount),
token: eth::TokenAddress(addr!("a0b86991c6218b36c1d19d4a2e9eb0ce3606eb48")),
};
let taker = eth::Asset {
amount: U256::from(taker_amount),
token: eth::TokenAddress(addr!("c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2")),
};
let fee = TakerAmount(U256::from(fee_amount));

LimitOrder { maker, taker, fee }
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to wei

let taker_amount: u32 = 123;
let fee_amount: u32 = 10;
let desired_in_amount: u32 = 50;

let order = create_limit_order(maker_amount, taker_amount, fee_amount);
let out_token = order.maker.token.0;
let in_token = order.taker.token.0;

let amount_out = order
.get_amount_out(out_token, (U256::from(desired_in_amount), in_token))
.unwrap();
let amount_in = order
.get_amount_in(in_token, (amount_out, out_token))
.unwrap();

assert_eq!(amount_in, U256::from(desired_in_amount));
}

#[test]
fn test_amount_in_out_round_trip() {
let maker_amount: u32 = 123;
let taker_amount: u32 = 321;
let fee_amount: u32 = 10;
let desired_out_amount: u32 = 50;

let order = create_limit_order(maker_amount, taker_amount, fee_amount);
let out_token = order.maker.token.0;
let in_token = order.taker.token.0;

let amount_in = order
.get_amount_in(in_token, (U256::from(desired_out_amount), out_token))
.unwrap();
let amount_out = order
.get_amount_out(out_token, (amount_in, in_token))
.unwrap();

assert_eq!(amount_out, U256::from(desired_out_amount));
}
}
1 change: 1 addition & 0 deletions crates/solvers/src/boundary/liquidity/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub mod constant_product;
mod limit_order;
pub mod stable;
pub mod weighted_product;
Loading