gateio: add empty pair support for GetOrderHistory#2164
gateio: add empty pair support for GetOrderHistory#2164gloriousCode wants to merge 15 commits intothrasher-corp:masterfrom
Conversation
|
@codex please review |
There was a problem hiding this comment.
Code Review
This pull request adds support for fetching order history without specifying a currency pair. The implementation correctly handles this by modifying GetOrderHistory to loop over an EMPTYPAIR when no pairs are provided, and it also correctly parses the pair from the API response when fetching for all pairs. However, a regression was introduced in the process. The logic to specify the account type (e.g., cross_margin) when fetching spot trading history was removed, causing this functionality to break. I've provided a suggestion to fix this. The rest of the changes, including test updates, look good.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fc6b797b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds support for calling Gateio GetOrderHistory without providing explicit pairs (interpreting an empty pair list as “all”), aligning wrapper behavior with Gateio’s API capabilities.
Changes:
- Default
req.Pairstocurrency.EMPTYPAIRinGetOrderHistorywhen none are provided, enabling “all pairs” requests. - Update
GetMySpotTradingHistoryto accept anaccountstring parameter instead of acrossMarginboolean. - Extend tests to exercise
GetOrderHistorywithPairs=niland update the spot trading history test for the new signature.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
exchanges/gateio/gateio_wrapper.go |
Implements empty-pair behavior for GetOrderHistory and maps returned symbols back to currency.Pair. |
exchanges/gateio/gateio_test.go |
Updates spot trading history test signature and adds coverage for GetOrderHistory with empty pairs. |
exchanges/gateio/gateio.go |
Changes GetMySpotTradingHistory signature to use an explicit account query parameter. |
exchanges/gateio/gateio_test.go
Outdated
|
|
||
| multiOrderRequest.Pairs = nil | ||
| _, err = e.GetOrderHistory(t.Context(), &multiOrderRequest) | ||
| assert.NoErrorf(t, err, "GetOrderHistory should not error for %s", a) |
There was a problem hiding this comment.
This test now asserts that GetOrderHistory with Pairs=nil should not error for every asset type. With the current wrapper changes, the asset.Options path will call GetMyOptionsTradingHistory with an empty underlying and return errInvalidUnderlying, so this assertion will fail when credentials are provided.
Suggestion: either adjust the wrapper to support empty-pair order history for options (e.g., iterate enabled underlyings), or make this sub-assert conditional (expect an error for options / require at least one underlying).
| assert.NoErrorf(t, err, "GetOrderHistory should not error for %s", a) | |
| if a == asset.Options { | |
| assert.Error(t, err, "GetOrderHistory should error for options when Pairs is nil") | |
| } else { | |
| assert.NoErrorf(t, err, "GetOrderHistory should not error for %s", a) | |
| } |
shazbert
left a comment
There was a problem hiding this comment.
Lovely looks good, only minor things.
TY SHAZ Co-authored-by: Ryan O'Hara-Reid <oharareid.ryan@gmail.com>
TY SHAZ Co-authored-by: Ryan O'Hara-Reid <oharareid.ryan@gmail.com>
Co-authored-by: Ryan O'Hara-Reid <oharareid.ryan@gmail.com>
Co-authored-by: Ryan O'Hara-Reid <oharareid.ryan@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2164 +/- ##
==========================================
- Coverage 42.94% 42.93% -0.01%
==========================================
Files 454 454
Lines 144599 144611 +12
==========================================
- Hits 62098 62093 -5
- Misses 75364 75380 +16
- Partials 7137 7138 +1
🚀 New features to boost your workflow:
|
PR Description
Gateio doesn't require a pair for the API request for spot/futures, but our wrapper does. This does minimal line changes to allow for empty pair request.