Support OKX API buy orders by squadgazzz · Pull Request #175 · gnosis/solvers

squadgazzz · 2025-10-31T18:29:32Z

This PR adds support for buy orders for the OKX API. It turned out that only the V5 API supports this kind of order, so a new optional config was added, which configures the V5 API URL, since the feature support is currently limited to Ethereum, Base, BNB, and Arb1 chains[docs]. The existing one is renamed to align with the new one properly.

Added new tests and adjusted existing ones.

# Conflicts: # config/example.okx.toml # src/infra/config/dex/okx/file.rs # src/infra/dex/okx/dto.rs # src/infra/dex/okx/mod.rs # src/tests/okx/api_calls.rs

Copilot

Pull Request Overview

This PR adds support for buy orders in the OKX DEX integration by introducing separate endpoints for sell orders (V6 API) and buy orders (V5 API). The V5 API is needed specifically for buy orders (exactOut mode) as it supports the required functionality.

Key changes:

Renamed endpoint to sell_orders_endpoint and added optional buy_orders_endpoint configuration
Updated API request routing to use V6 API for sell orders and V5 API for buy orders
Modified allowance calculation to use U256::MAX for buy orders due to slippage on input tokens
Added comprehensive test coverage for both enabled and disabled buy order scenarios

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/infra/dex/okx/mod.rs	Core implementation changes for dual-endpoint support and buy order logic
src/infra/dex/okx/dto.rs	Added V5 API DTOs and updated request building logic
src/infra/config/dex/okx/file.rs	Configuration parsing for new endpoint fields
config/example.okx.toml	Documentation of new configuration options
src/tests/okx/mod.rs	Updated test configuration with new endpoint naming
src/tests/okx/market_order.rs	Added test for buy orders when enabled
src/tests/okx/api_calls.rs	Added integration tests for buy order scenarios

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jmg-duarte

LGTM, just a nit on the docs

MartinquaXD

Did you already test driver this in staging?
If that is verified to be working I would be fine to merge as is to not delay the roll out of this as my concerns are about cleaner architecture and not so much about correctness.
Approved assuming this was tested.

MartinquaXD · 2025-11-07T09:50:47Z

+                        order,
+                    );
+                    let approve_request_v5 =
+                        dto::ApproveTransactionRequestV5::from_v6(&approve_request_v6);


These conversions from v6 to v5 are quite weird to me. Although it's a bit more boiler plate code I think the better way would be to have 1 domain specific request type that can be converted to v5 and v6. The conversion to v6 should then also be fallible in case it's requesting a buy order quote.

Yeah, that makes sense for the long perspective, but the idea was to introduce the V5 request temporarily, since it will be deprecated/disabled sometime later, and then we can easily get back to V6 with minimal changes.

MartinquaXD · 2025-11-07T09:53:19Z

+        // For buy orders (ExactOut mode), we need to use U256::MAX allowance
+        // because the exact input amount is not known in advance.
+        let approve_amount = if matches!(order.side, order::Side::Buy) {
+            U256::MAX
+        } else {
+            order.amount.get()
+        };


Could you measure how much time we'd lose if we don't do the approve and quote request in parallel? Given that this is not doing any complicated computation it should be a few ms at most. In that case I think it would be nicer to restructure the code to build the approve request with the amounts suggested by the quote.

The approve request takes 350ms from my local machine. We can probably afford it.

I just realized that this logic doesn't make any sense:

solvers/src/infra/dex/okx/mod.rs

Lines 257 to 261 in 148a19a

self.dex_approved_addresses

.try_get_with(order.sell, approve_transaction_request_future)

.map_err(

|_: std::sync::Arc<Error>| Error::ApproveTransactionRequestFailed(order.sell)

)

Once a first order with this token gets an approval for an amount X, we continue using the same approval for all the upcoming orders, but if next orders exceed the approved amount X, the tx will fail. Did I miss anything? If not, we should get rid of this cache.

I had to read up on the cache myself but it's only storing which contract we have to give an allowance to for every sell token. It's not caching the approved amounts.
I think now that it's possible to to user 2 APIs at the same time this caching is no longer correct. If the v5 and v6 API use different contracts under the hood we would cache the v6 address when we handle a SELL order token X -> token Y. Then when we handle a BUY order token X -> token Y we'd look up the cached v6 contract address although we'd have to use the v5 address.

The approve request takes 350ms from my local machine. We can probably afford it.

That's a relatively long time TBH. However, given that the OKX solver was not very good in the past chances are it will not find many trades satisfying the order's limit price in the first place. If we only incur the 350ms latency hit if the solver actually finds a feasible trade (i.e. early return if the limit price is not respected) we should probably be okay to not have the parallel requests and also remove the caching.

I had to read up on the cache myself but it's only storing which contract we have to give an allowance to for every sell token. It's not caching the approved amounts.
I think now that it's possible to to user 2 APIs at the same time this caching is no longer correct. If the v5 and v6 API use different contracts under the hood we would cache the v6 address when we handle a SELL order token X -> token Y. Then when we handle a BUY order token X -> token Y we'd look up the cached v6 contract address although we'd have to use the v5 address.

I understand the issue with different API versions, but I'm not sure I understand whether it's been working correctly.

So, if UserA submits an order with X amount of TokenA, we approve that exact X amount, and it's not tied to that user. If the transaction goes through before the cache expires, the allowance gets used. But if another(or even the same) user submits a similar order later, we don’t request a new allowance, so that transaction should fail, right? The same thing would happen if the previous allowance wasn’t used, but the next order needs a higher amount of TokenA.

The cache only stores which account we need to set an approval for. It's not keeping track of the actual approval amount. So it should be okay at the moment.

Ah, sorry, this address is later used in the interaction as an allowance. I missed that. All clear then, thanks!

If we only incur the 350ms latency hit if the solver actually finds a feasible trade (i.e. early return if the limit price is not respected) we should probably be okay to not have the parallel requests

Updated the code. Now it sends swap and approval requests sequentially only for buy orders, since the majority are still sell orders.

squadgazzz added 4 commits October 31, 2025 15:24

Support OKX V6 API

adfeadd

Default url const

28a72ea

Support OKX API buy orders

ea6e008

naming

ca76a49

Base automatically changed from okx/v6 to main November 5, 2025 08:55

squadgazzz added 4 commits November 6, 2025 14:44

Use V5 API

bcf2e4a

Merge branch 'main' into okx/buy-orders

f0c6966

# Conflicts: # config/example.okx.toml # src/infra/config/dex/okx/file.rs # src/infra/dex/okx/dto.rs # src/infra/dex/okx/mod.rs # src/tests/okx/api_calls.rs

Fix after merge

a13db5f

Test fix

1626c77

squadgazzz marked this pull request as ready for review November 6, 2025 14:55

Copilot AI review requested due to automatic review settings November 6, 2025 14:55

Copilot AI reviewed Nov 6, 2025

View reviewed changes

Comment thread src/infra/dex/okx/dto.rs

Comment thread config/example.okx.toml

Comment thread src/infra/dex/okx/mod.rs Outdated

Redundant change

fa99763

fafk reviewed Nov 7, 2025

View reviewed changes

Comment thread src/infra/dex/okx/dto.rs Outdated

jmg-duarte approved these changes Nov 7, 2025

View reviewed changes

Comment thread src/infra/config/dex/okx/file.rs Outdated

Comment thread src/infra/dex/okx/mod.rs Outdated

MartinquaXD approved these changes Nov 7, 2025

View reviewed changes

fafk reviewed Nov 7, 2025

View reviewed changes

Comment thread src/infra/dex/okx/dto.rs Outdated

fafk reviewed Nov 7, 2025

View reviewed changes

Comment thread src/infra/dex/okx/mod.rs Outdated

fafk reviewed Nov 7, 2025

View reviewed changes

Comment thread src/infra/dex/okx/mod.rs Outdated

squadgazzz added 6 commits November 7, 2025 10:31

Impl from

f3ff993

Comments

7a2f557

Wording

6cdb969

Reduce duplication

148a19a

Adjust test

a5f4396

Reduce duplication

728926c

fafk approved these changes Nov 7, 2025

View reviewed changes

squadgazzz added 3 commits November 7, 2025 17:32

Cache by order side

cba3c23

Improve buy orders approval amount

77c1420

Sell orders in parallel

a668a0d

squadgazzz merged commit 7bf6851 into main Nov 10, 2025
4 checks passed

squadgazzz deleted the okx/buy-orders branch November 10, 2025 12:14

	self.dex_approved_addresses
	.try_get_with(order.sell, approve_transaction_request_future)
	.map_err(
	\|_: std::sync::Arc<Error>\| Error::ApproveTransactionRequestFailed(order.sell)
	)

Conversation

squadgazzz commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmg-duarte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MartinquaXD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squadgazzz Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

squadgazzz commented Oct 31, 2025 •

edited

Loading

squadgazzz Nov 7, 2025 •

edited

Loading