-
Notifications
You must be signed in to change notification settings - Fork 153
Add solver margin configuration to protect against negative slippage #4037
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
Changes from all commits
20dcd9a
62307f2
465755c
c5dccc7
3d38ad0
a9b0725
f4a007f
7c4a907
0ae7cd8
b81f378
95c9d43
446f26b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ pub fn new( | |
| flashloan_hints: &HashMap<order::Uid, eth::Flashloan>, | ||
| wrappers: &WrapperCalls, | ||
| deadline: chrono::DateTime<chrono::Utc>, | ||
| margin_bps: u32, | ||
| ) -> solvers_dto::auction::Auction { | ||
| let mut tokens: HashMap<eth::Address, _> = auction | ||
| .tokens() | ||
|
|
@@ -110,6 +111,44 @@ pub fn new( | |
| } | ||
| }) | ||
| } | ||
| // Apply margin by adjusting order limits (similar to volume fees). | ||
| // This forces solvers to find solutions with enough surplus to cover | ||
| // the margin applied during competition scoring. | ||
| if margin_bps > 0 { | ||
| let factor = margin_bps as f64 / 10_000.0; | ||
| match order.side { | ||
| Side::Buy => { | ||
| // For buy orders: reduce maximum sell amount | ||
| if let Some(adjusted) = | ||
| available.sell.amount.apply_factor(1.0 / (1.0 + factor)) | ||
| { | ||
| available.sell.amount = adjusted; | ||
| } else { | ||
| tracing::warn!( | ||
| "applying margin bps {} led to sell amount underflow for \ | ||
| order {:?}", | ||
| margin_bps, | ||
| order.uid | ||
| ); | ||
| } | ||
| } | ||
| Side::Sell => { | ||
| // For sell orders: increase minimum buy amount requirement | ||
| if let Some(adjusted) = | ||
| available.buy.amount.apply_factor(1.0 / (1.0 - factor)) | ||
| { | ||
| available.buy.amount = adjusted; | ||
|
Comment on lines
+137
to
+140
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The calculation Consider adding a check to ensure if margin_bps >= 10000 { // Handle 100% margin as an impossible order
tracing::warn!(
"applying margin bps {} led to infinite buy amount for order \n {:?}, treating as unfillable",
margin_bps,
order.uid
);
available.buy.amount = eth::U256::MAX; // Or some other indicator of unfillable
} else if let Some(adjusted) =
available.buy.amount.apply_factor(1.0 / (1.0 - factor))
{
available.buy.amount = adjusted;
} else {
tracing::warn!(
"applying margin bps {} led to buy amount overflow for order \n {:?}",
margin_bps,
order.uid
);
} |
||
| } else { | ||
| tracing::warn!( | ||
| "applying margin bps {} led to buy amount overflow for order \ | ||
| {:?}", | ||
| margin_bps, | ||
| order.uid | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| solvers_dto::auction::Order { | ||
| uid: order.uid.into(), | ||
| sell_token: available.sell.token.0.0, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| //! Tests that verify margin is correctly applied to order limits before | ||
| //! sending to solvers. | ||
|
|
||
| use crate::{ | ||
| domain::{competition::order, eth}, | ||
| tests::{ | ||
| self, | ||
| setup::{ab_order, ab_pool, ab_solution, test_solver}, | ||
| }, | ||
| }; | ||
|
|
||
| /// Test that verifies the solver receives orders with adjusted limits when | ||
| /// margin is applied. | ||
| /// | ||
| /// For sell orders: the minimum buy amount is increased by the margin factor | ||
| /// For buy orders: the maximum sell amount is reduced by the margin factor | ||
| /// | ||
| /// The test works by setting up a solver mock that expects the adjusted amounts | ||
| /// and will fail the assertion if the received amounts don't match. | ||
| #[tokio::test] | ||
| #[ignore] | ||
squadgazzz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| async fn margin_adjusts_order_limits() { | ||
| // Test with 1% margin (100 basis points) | ||
| let margin_bps = 100u32; | ||
|
|
||
| for side in [order::Side::Sell, order::Side::Buy] { | ||
| // Limit orders require solver-determined fees | ||
| let order = ab_order() | ||
| .kind(order::Kind::Limit) | ||
| .side(side) | ||
| .solver_fee(Some(eth::U256::from(500))); | ||
|
|
||
| let test = tests::setup() | ||
| .name(format!("Margin: {side:?}")) | ||
| .solvers(vec![test_solver().margin_bps(margin_bps)]) | ||
| .pool(ab_pool()) | ||
| .order(order.clone()) | ||
| .solution(ab_solution()) | ||
| .done() | ||
| .await; | ||
|
|
||
| // The solver mock will verify that the order limits are adjusted | ||
| // according to the margin. If the limits are not correctly adjusted, | ||
| // the mock will fail with an assertion error. | ||
| test.solve().await.ok().orders(&[order]); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,6 +45,8 @@ pub struct Config<'a> { | |||||||||
| pub private_key: PrivateKeySigner, | ||||||||||
| pub expected_surplus_capturing_jit_order_owners: Vec<Address>, | ||||||||||
| pub allow_multiple_solve_requests: bool, | ||||||||||
| /// Margin in basis points (0-10000) for conservative bidding. | ||||||||||
| pub margin_bps: u32, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| impl Solver { | ||||||||||
|
|
@@ -89,6 +91,14 @@ impl Solver { | |||||||||
| _ => {} | ||||||||||
| } | ||||||||||
| } | ||||||||||
| // Apply margin: reduce sell amount for buy orders | ||||||||||
| if config.margin_bps > 0 { | ||||||||||
| let factor = config.margin_bps as f64 / 10_000.0; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the issue identified in
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||||||||||
| current_sell_amount = eth::TokenAmount(current_sell_amount) | ||||||||||
| .apply_factor(1.0 / (1.0 + factor)) | ||||||||||
| .unwrap() | ||||||||||
| .0; | ||||||||||
| } | ||||||||||
| current_sell_amount.to_string() | ||||||||||
| } | ||||||||||
| _ => quote.sell_amount().to_string(), | ||||||||||
|
|
@@ -115,6 +125,14 @@ impl Solver { | |||||||||
| _ => {} | ||||||||||
| } | ||||||||||
| } | ||||||||||
| // Apply margin: increase buy amount for sell orders | ||||||||||
| if config.margin_bps > 0 { | ||||||||||
| let factor = config.margin_bps as f64 / 10_000.0; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, using
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||||||||||
| current_buy_amount = eth::TokenAmount(current_buy_amount) | ||||||||||
| .apply_factor(1.0 / (1.0 - factor)) | ||||||||||
| .unwrap() | ||||||||||
| .0; | ||||||||||
| } | ||||||||||
| current_buy_amount.to_string() | ||||||||||
| } | ||||||||||
| _ => quote.buy_amount().to_string(), | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
f64for financial calculations likefactor = margin_bps as f64 / 10_000.0can introduce floating-point precision errors. It's generally safer and more accurate to perform these calculations using integer arithmetic withU256orBigRationalto avoid unexpected rounding issues, especially when dealing with token amounts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
apply_factoraccepts f64, so it should be fine I guess.