Conversation
There was a problem hiding this comment.
Code Review
This PR migrates the OKX solver, introducing a significant amount of new code for DEX integration, including API clients, configuration, domain logic for swaps, and testing infrastructure. The changes are well-structured. The identified critical issue in fee calculation, high-severity issue regarding floating-point numbers for financial configuration, and medium-severity robustness issue in request signing remain valid as they do not contradict any provided rules.
| /// When set to 100%, the feature is disabled (default). | ||
| /// Note: OKX API default is 90% if this parameter is NOT sent. | ||
| #[serde(default = "default_price_impact_protection_percent")] | ||
| price_impact_protection_percent: f64, |
There was a problem hiding this comment.
Using f64 for financial values like price_impact_protection_percent can lead to precision issues. It's better to use bigdecimal::BigDecimal directly in the configuration struct and parse it from a string to maintain precision.
You should change the type to bigdecimal::BigDecimal and use serde_with::DisplayFromStr for deserialization. This will require updating the default_price_impact_protection_percent function and the dependent Config struct in crates/solvers/src/infra/dex/okx/mod.rs as well.
#[serde_as(as = "serde_with::DisplayFromStr")]
price_impact_protection_percent: bigdecimal::BigDecimal,|
Please cover what the migration entails in more detail. Since this was added to solvers, does this bring a new code path? Config changes? Does this require infra changes, like the build changes? |
Added this to the PR description. Basically, one line of the infra config change. |
fafk
left a comment
There was a problem hiding this comment.
Given that its a copypaste from another repo and it's been tested in staging I am happy to give thumbs up here. 👌
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the OKX solver, introducing new code for DEX integration, including domain logic, configuration, and infrastructure for interacting with the OKX API. Two high-severity issues were found: a logic error in a numeric conversion affecting partially fillable orders, and an error handling issue leading to lost debugging information. Additionally, a precision issue with f64 in configuration was identified. While the overall structure is sound and other components like the OKX API client, gas simulation, and configuration handling are implemented securely, addressing these identified issues is crucial for the robustness and maintainability of the new solver.
| pub fn bigdecimal_to_u256(d: &BigDecimal) -> Option<U256> { | ||
| let d = d.to_bigint()?; | ||
| bigint_to_u256(&d) | ||
| } |
There was a problem hiding this comment.
The to_bigint() method returns None if the BigDecimal has a fractional part, which will cause the ? operator to propagate a None result. The calculation of smallest_fill in Fills::dex_order is likely to produce a non-integer, which would cause this function to frequently return None, effectively preventing partially fillable orders from being considered. The BigDecimal should be rounded before conversion. Using RoundingMode::Ceiling is a safer approach for a minimum threshold.
| pub fn bigdecimal_to_u256(d: &BigDecimal) -> Option<U256> { | |
| let d = d.to_bigint()?; | |
| bigint_to_u256(&d) | |
| } | |
| pub fn bigdecimal_to_u256(d: &BigDecimal) -> Option<U256> { | |
| let d = d.with_scale(0, bigdecimal::RoundingMode::Ceiling); | |
| let i = d.to_bigint()?; | |
| bigint_to_u256(&i) | |
| } |
| future, | ||
| ) | ||
| .await | ||
| .map_err(|_: std::sync::Arc<Error>| Error::ApproveTransactionRequestFailed(order.sell)) |
There was a problem hiding this comment.
The original error from the failed approve-transaction request is being discarded by using _. This loss of information makes debugging production issues significantly more difficult. The underlying error should be logged to aid in troubleshooting.
.map_err(|e| {
tracing::warn!(?order.sell, error = ?e, "approve-transaction request failed");
Error::ApproveTransactionRequestFailed(order.sell)
})| /// When set to 100%, the feature is disabled (default). | ||
| /// Note: OKX API default is 90% if this parameter is NOT sent. | ||
| #[serde(default = "default_price_impact_protection_percent")] | ||
| price_impact_protection_percent: f64, |
There was a problem hiding this comment.
Using f64 for price_impact_protection_percent can introduce precision issues. It is recommended to use BigDecimal directly in the configuration to maintain precision. This would involve changing the type here to bigdecimal::BigDecimal, annotating it with #[serde_as(as = "serde_with::DisplayFromStr")], updating the default_price_impact_protection_percent function, and adjusting the Config struct and try_new function in crates/solvers/src/infra/dex/okx/mod.rs to handle BigDecimal directly.
MartinquaXD
left a comment
There was a problem hiding this comment.
Approved assuming this is only copying code and adjusting this code base to fit the other file structure.
| flate2 = "1.0.30" | ||
| futures = "0.3.30" | ||
| gas-price-estimation = { path = "crates/gas-price-estimation" } | ||
| hex = "0.4.3" |
There was a problem hiding this comment.
nit: this repo moved from hex to const_hex due to the superior performance. Not needed to be done in this PR but in a follow up it would be nice to migrate the OKX code to use const_hex and drop hex again.
Description
Migrates the OKX solver from the https://github.com/gnosis/solvers repo as per #4234.
How to test
Migrated tests.
How to migrate
1 line config change in the infra, which basically replaces the image.