Add solver margin configuration to protect against negative slippage #4037
Conversation
Summary of ChangesHello @squadgazzz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a 'quote haircut' mechanism designed to enhance the conservatism of competition bids. By allowing a configurable reduction in solver-reported executed amounts, the system can submit more cautious bids. This adjustment is performed without altering the underlying interaction calldata, ensuring that bids remain within acceptable limits and respect the order's minimum requirements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'quote haircut' feature, applying a configurable basis points reduction to solver-reported economics to make competition bids more conservative. The core logic is implemented in haircut.rs and includes plumbing the quote_haircut_bps configuration. A medium-severity Denial of Service vulnerability has been identified where a misconfiguration of the quote_haircut_bps parameter can cause a panic due to an arithmetic underflow, crashing the driver. It is recommended to use saturating_sub to prevent this. Additionally, a minor redundancy in the code was noted.
squadgazzz
changed the title
squadgazzz
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces a configurable margin-bps parameter per solver to adjust order limits, protecting against negative slippage. This is a valuable addition for making bids more conservative. The changes involve adding the margin_bps field to relevant configuration structs, passing it through the system, and implementing the logic to adjust order amounts in the auction DTO and mock solver. A new test case has been added to verify the functionality. The review comments highlight important considerations regarding floating-point precision in financial calculations and the status of ignored tests, which should be addressed for robustness and comprehensive testing.
| // 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; |
There was a problem hiding this comment.
Using f64 for financial calculations like factor = margin_bps as f64 / 10_000.0 can introduce floating-point precision errors. It's generally safer and more accurate to perform these calculations using integer arithmetic with U256 or BigRational to avoid unexpected rounding issues, especially when dealing with token amounts.
let factor = eth::U256::from(margin_bps);
let denominator = eth::U256::from(10_000);There was a problem hiding this comment.
The apply_factor accepts f64, so it should be fine I guess.
| } | ||
| // Apply margin: reduce sell amount for buy orders | ||
| if config.margin_bps > 0 { | ||
| let factor = config.margin_bps as f64 / 10_000.0; |
There was a problem hiding this comment.
Similar to the issue identified in dto/auction.rs, using f64 for factor calculation in the mock solver can introduce precision problems. For consistency and accuracy in financial simulations, it's best to use integer or BigRational arithmetic.
| let factor = config.margin_bps as f64 / 10_000.0; | |
| let factor = eth::U256::from(config.margin_bps); | |
| let denominator = eth::U256::from(10_000); | |
| } | ||
| // Apply margin: increase buy amount for sell orders | ||
| if config.margin_bps > 0 { | ||
| let factor = config.margin_bps as f64 / 10_000.0; |
There was a problem hiding this comment.
Again, using f64 for factor calculation in the mock solver can lead to precision issues. It's recommended to use integer or BigRational arithmetic for financial calculations.
| let factor = config.margin_bps as f64 / 10_000.0; | |
| let factor = eth::U256::from(config.margin_bps); | |
| let denominator = eth::U256::from(10_000); | |
There was a problem hiding this comment.
Code Review
The pull request introduces a configurable margin-bps parameter, which is a sound approach. However, a correctness issue was identified where a 100% margin could lead to division by zero and incorrect order handling, which needs to be addressed. The overall approach is otherwise well-documented.
| if let Some(adjusted) = | ||
| available.buy.amount.apply_factor(1.0 / (1.0 - factor)) | ||
| { | ||
| available.buy.amount = adjusted; |
There was a problem hiding this comment.
The calculation 1.0 / (1.0 - factor) for sell orders can lead to division by zero if margin_bps is 10000 (i.e., factor is 1.0). In this scenario, apply_factor will return None due to floating-point division by zero resulting in inf, which cannot be converted to U256. The current implementation then logs a warning and proceeds with the original available.buy.amount, effectively ignoring the 100% margin. This is a correctness issue, as a 100% margin should make the order unfillable or require an impossibly large buy amount, not simply bypass the margin.
Consider adding a check to ensure margin_bps is strictly less than 10000 to prevent this edge case, or explicitly handle the None result from apply_factor as an unfillable order rather than silently using the unadjusted amount.
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
);
}
Description
Adds a configurable margin-bps parameter per solver that adjusts order limits sent to solvers. This protects against negative slippage from external pricing APIs (e.g., OKX) that may quote optimistic prices that don't materialize on-chain.
The margin adjusts order amounts before sending to solvers:
This forces solvers to find solutions with enough buffer to absorb expected negative slippage. The difference is handled through the protocol's existing buffer mechanism.
Example
With margin-bps = 500 (5%):
How to test
New driver and e2e tests.