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.

Sign up for GitHub

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

Price improvement tests #2467

Closed
wants to merge 14 commits into from
Closed

Price improvement tests #2467

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9b3a0ee
Price improvement tests
squadgazzz Mar 4, 2024
dd233e2
Adjusted values
squadgazzz Mar 5, 2024
3f7bf5e
Update crates/driver/src/tests/cases/protocol_fees.rs
squadgazzz Mar 5, 2024
4b70eb7
Update crates/driver/src/tests/cases/protocol_fees.rs
squadgazzz Mar 5, 2024
38135e8
Adjusted values
squadgazzz Mar 5, 2024
287c1f9
Explicit fee amounts
squadgazzz Mar 5, 2024
08c7027
Network fee
squadgazzz Mar 5, 2024
758ebde
Adjusted fees
squadgazzz Mar 6, 2024
3a80fe6
Adjusted values
squadgazzz Mar 6, 2024
75d99c1
Order surplus factor=1
squadgazzz Mar 8, 2024
47dd239
Merge branch 'main' into 2287/price-improvement-tests
squadgazzz Mar 8, 2024
1a47343
Fixed values
squadgazzz Mar 8, 2024
9daf971
Refactoring
squadgazzz Mar 8, 2024
886fc2f
Wrong comment
squadgazzz Mar 8, 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
128 changes: 128 additions & 0 deletions crates/driver/src/tests/cases/protocol_fees.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
ab_solution,
ExpectedOrderAmounts,
FeePolicy,
PriceImprovementQuote,
Test,
},
},
Expand Down Expand Up @@ -187,3 +188,130 @@ async fn volume_protocol_fee_sell_order() {

protocol_fee_test_case(test_case).await;
}

#[tokio::test]
#[ignore]
async fn price_improvement_fee_buy_out_market_order() {
squadgazzz marked this conversation as resolved.
Show resolved Hide resolved
let quote_sell_amount = 50000000000000000000u128;
let quote_buy_amount = 45000000000000000000u128;
let fee_policy = FeePolicy::PriceImprovement {
factor: 0.5,
// high enough so we don't get capped by volume fee
max_volume_factor: 1.0,
quote: PriceImprovementQuote {
sell_amount: quote_sell_amount.into(),
buy_amount: quote_buy_amount.into(),
fee: 1000000000000000000u128.into(),
},
};
let test_case = TestCase {
order_side: order::Side::Buy,
fee_policy,
order_sell_amount: 50000000000000000000u128.into(),
solver_fee: Some(10000000000000000000u128.into()),
quote_sell_amount: quote_sell_amount.into(),
quote_buy_amount: quote_buy_amount.into(),
executed: quote_buy_amount.into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we charge any fee here? The quote amount is equal to the executed amount so there is no price improvement. Doesn't this test framework now need a way of differentiating between the quoted and executed price? Also, are we even using an executed amount that is lower than the full amount in any test case (ie are we testing partial fills somewhere)

Overall, I still find these test cases a bit hard to parse (imagine someone trying to debug a failing test case and trying to reproduce the assumptions and math that is going on here).

I don't think we necessarily have to find the nicest abstraction that leads to the least code (e.g. I think we could have a different TestCase setup for price improvement fee vs. surplus fee if one is structurally more complex), but let's please make those tests easier to comprehend.

Copy link
Contributor Author

@squadgazzz squadgazzz Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before refactoring, now I understand that I am confused about a condition when an order is inside or outside market price.
Can out of market orders be defined as (order.sell + order.fee) * quote.buy < (quote.sell + quote.fee) * order.buy?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe so, yes (looking at @sunce86 to confirm)

// sell amount + quote fee * factor
executed_sell_amount: 50500000000000000000u128.into(),
// executed buy amount should match quote buy amount
executed_buy_amount: quote_buy_amount.into(),
};

protocol_fee_test_case(test_case).await;
}

#[tokio::test]
#[ignore]
async fn price_improvement_fee_sell_out_market_order() {
squadgazzz marked this conversation as resolved.
Show resolved Hide resolved
let quote_sell_amount = 50000000000000000000u128;
let quote_buy_amount = 45000000000000000000u128;
let fee_policy = FeePolicy::PriceImprovement {
factor: 0.5,
// high enough so we don't get capped by volume fee
max_volume_factor: 1.0,
quote: PriceImprovementQuote {
sell_amount: quote_sell_amount.into(),
buy_amount: quote_buy_amount.into(),
fee: 1000000000000000000u128.into(),
},
};
let order_sell_amount = 50000000000000000000u128;
let test_case = TestCase {
order_side: order::Side::Sell,
fee_policy,
order_sell_amount: order_sell_amount.into(),
solver_fee: Some(10000000000000000000u128.into()),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it would be clearer to call this network_fee in this context.

I do see now that the price improvement comes from the network fee being different than the quoted fee. This is very clever but not something I'd expect someone maintaining this code in 3 month to just guess without context, so please let's help your future colleague to understand how this test work more intuitively.

quote_sell_amount: quote_sell_amount.into(),
quote_buy_amount: quote_buy_amount.into(),
executed: 40000000000000000000u128.into(),
// executed sell amount should match order sell amount
executed_sell_amount: order_sell_amount.into(),
executed_buy_amount: 44558823529411764706u128.into(),
};

protocol_fee_test_case(test_case).await;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Partially fillable orders are missing.

Also can we add a test that shows shifting surplus into network fees doesn't change the protocol fee charged? E.g. the following two executions pay the same fee (assuming same underlying orders & quotes)

  1. Sell 0.1 ETH receive 3500 DAI, pay 0.9 ETH in network fees
  2. Sell 1 ETH receive 3500 DAI, pay 0 in network fees

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partially fillable orders require some further refactoring. I will create a separate issue for this.


#[tokio::test]
#[ignore]
async fn price_improvement_fee_buy_in_market_order() {
let quote_sell_amount = 50000000000000000000u128;
let quote_buy_amount = 45000000000000000000u128;
let fee_policy = FeePolicy::PriceImprovement {
factor: 0.5,
// high enough so we don't get capped by volume fee
max_volume_factor: 1.0,
quote: PriceImprovementQuote {
sell_amount: quote_sell_amount.into(),
buy_amount: quote_buy_amount.into(),
fee: 20000000000000000000u128.into(),
},
};
let test_case = TestCase {
order_side: order::Side::Buy,
fee_policy,
order_sell_amount: 50000000000000000000u128.into(),
solver_fee: Some(10000000000000000000u128.into()),
quote_sell_amount: quote_sell_amount.into(),
quote_buy_amount: quote_buy_amount.into(),
executed: quote_buy_amount.into(),
executed_sell_amount: 60000000000000000000u128.into(),
// executed buy amount should match order buy amount
executed_buy_amount: quote_buy_amount.into(),
};

protocol_fee_test_case(test_case).await;
}

#[tokio::test]
#[ignore]
async fn price_improvement_fee_sell_in_market_order() {
let quote_sell_amount = 50000000000000000000u128;
let quote_buy_amount = 45000000000000000000u128;
let fee_policy = FeePolicy::PriceImprovement {
factor: 0.5,
// high enough so we don't get capped by volume fee
max_volume_factor: 1.0,
quote: PriceImprovementQuote {
sell_amount: quote_sell_amount.into(),
buy_amount: quote_buy_amount.into(),
fee: 20000000000000000000u128.into(),
},
};
let order_sell_amount = 50000000000000000000u128;
let test_case = TestCase {
order_side: order::Side::Sell,
fee_policy,
order_sell_amount: order_sell_amount.into(),
solver_fee: Some(10000000000000000000u128.into()),
quote_sell_amount: quote_sell_amount.into(),
quote_buy_amount: quote_buy_amount.into(),
executed: 40000000000000000000u128.into(),
// executed sell amount should match order sell amount
executed_sell_amount: order_sell_amount.into(),
executed_buy_amount: 38571428571428571429u128.into(),
};

protocol_fee_test_case(test_case).await;
}
30 changes: 30 additions & 0 deletions crates/driver/src/tests/setup/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,28 @@ pub enum Score {
RiskAdjusted { success_probability: f64 },
}

#[serde_as]
#[derive(Debug, Clone, PartialEq, serde::Serialize)]
#[serde(rename_all = "camelCase", tag = "kind")]
pub struct PriceImprovementQuote {
pub buy_amount: eth::U256,
pub sell_amount: eth::U256,
pub fee: eth::U256,
squadgazzz marked this conversation as resolved.
Show resolved Hide resolved
}

#[serde_as]
#[derive(Debug, Clone, PartialEq, serde::Serialize)]
#[serde(rename_all = "camelCase", tag = "kind")]
pub enum FeePolicy {
#[serde(rename_all = "camelCase")]
Surplus { factor: f64, max_volume_factor: f64 },
#[serde(rename_all = "camelCase")]
PriceImprovement {
factor: f64,
max_volume_factor: f64,
quote: PriceImprovementQuote,
},
#[serde(rename_all = "camelCase")]
Volume { factor: f64 },
}

Expand All @@ -94,6 +109,21 @@ impl FeePolicy {
"maxVolumeFactor": max_volume_factor
}
}),
FeePolicy::PriceImprovement {
factor,
max_volume_factor,
quote,
} => json!({
"priceImprovement": {
"factor": factor,
"maxVolumeFactor": max_volume_factor,
"quote": {
"sellAmount": quote.sell_amount,
"buyAmount": quote.buy_amount,
"fee": quote.fee,
}
}
}),
FeePolicy::Volume { factor } => json!({
"volume": {
"factor": factor
Expand Down
Loading