Skip to content

Migrate OKX solver#4236

Open
squadgazzz wants to merge 7 commits intomainfrom
migrate-okx-solver
Open

Migrate OKX solver#4236
squadgazzz wants to merge 7 commits intomainfrom
migrate-okx-solver

Conversation

@squadgazzz
Copy link
Contributor

@squadgazzz squadgazzz commented Mar 5, 2026

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.

@squadgazzz squadgazzz marked this pull request as ready for review March 5, 2026 20:10
@squadgazzz squadgazzz requested a review from a team as a code owner March 5, 2026 20:10
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

high

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,

@jmg-duarte
Copy link
Contributor

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?

@squadgazzz
Copy link
Contributor Author

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.

Copy link
Contributor

@fafk fafk left a comment

Choose a reason for hiding this comment

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

Given that its a copypaste from another repo and it's been tested in staging I am happy to give thumbs up here. 👌

@squadgazzz
Copy link
Contributor Author

/gemini review

@squadgazzz squadgazzz enabled auto-merge March 6, 2026 15:12
@squadgazzz squadgazzz disabled auto-merge March 6, 2026 15:13
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +57 to +60
pub fn bigdecimal_to_u256(d: &BigDecimal) -> Option<U256> {
let d = d.to_bigint()?;
bigint_to_u256(&d)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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.

Suggested change
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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

Choose a reason for hiding this comment

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

high

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.

Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

@squadgazzz squadgazzz added this pull request to the merge queue Mar 10, 2026
@squadgazzz squadgazzz removed this pull request from the merge queue due to a manual request Mar 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants