exchanges: Update UpdateTickers method so that available pairs are used instead of enabled pairs.#2159
Conversation
…ecks across multiple exchanges
![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 refactors the UpdateTickers method across multiple exchanges to use available pairs instead of enabled pairs, which is a good improvement for efficiency and functionality. The changes are mostly consistent and correct. However, I've identified a recurring critical bug in the error handling for currency.ErrPairNotFound in bitmex_wrapper.go, exmo_wrapper.go, hitbtc_wrapper.go, and kucoin_wrapper.go. This bug could halt all ticker updates for these exchanges. I've included specific comments and code suggestions to address these issues. The other changes, such as removing functionality that involves consecutive REST calls, align well with the stated goals of the PR.
exchanges/bitmex/bitmex_wrapper.go
Outdated
| if err != nil && !errors.Is(err, currency.ErrPairNotFound) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When e.MatchSymbolCheckEnabled returns a currency.ErrPairNotFound error, the loop should continue to the next ticker. The current logic allows execution to proceed with a zero-value pair, which will cause ticker.ProcessTicker to fail and halt all ticker updates for the exchange.
if err != nil {
if errors.Is(err, currency.ErrPairNotFound) {
continue
}
return err
}| if err != nil { | ||
| if !errors.Is(err, currency.ErrPairNotFound) { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
When e.MatchSymbolCheckEnabled returns a currency.ErrPairNotFound error, the loop should continue to the next ticker. Currently, it proceeds with a zero-value pair, which will cause ticker.ProcessTicker to fail and halt all ticker updates for the exchange.
if err != nil {
if !errors.Is(err, currency.ErrPairNotFound) {
return err
}
continue
}| if err != nil { | ||
| if !errors.Is(err, currency.ErrPairNotFound) { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
When e.MatchSymbolCheckEnabled returns a currency.ErrPairNotFound error, the loop should continue to the next ticker. Currently, it proceeds with a zero-value pair, which will cause ticker.ProcessTicker to fail and halt all ticker updates for the exchange.
| if err != nil { | |
| if !errors.Is(err, currency.ErrPairNotFound) { | |
| return err | |
| } | |
| } | |
| if err != nil { | |
| if !errors.Is(err, currency.ErrPairNotFound) { | |
| return err | |
| } | |
| continue | |
| } |
exchanges/kucoin/kucoin_wrapper.go
Outdated
| if err != nil && !errors.Is(err, currency.ErrPairNotFound) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When e.MatchSymbolCheckEnabled returns a currency.ErrPairNotFound error, the loop should continue to the next ticker. The current logic allows execution to proceed with a zero-value pair, which will cause ticker.ProcessTicker to fail and halt all ticker updates for the exchange.
if err != nil {
if !errors.Is(err, currency.ErrPairNotFound) {
return err
}
continue
}
thrasher-
requested review from
cranktakular,
gloriousCode,
samuael and
thrasher-
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5bcb19f1f
ℹ️ 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".
| Last: tickerInfo[j].Ticker.Latest, | ||
| High: tickerInfo[j].Ticker.High, | ||
| Low: tickerInfo[j].Ticker.Low, | ||
| Volume: tickerInfo[j].Ticker.Volume, | ||
| Pair: tickerInfo[j].Symbol, |
There was a problem hiding this comment.
This loop now processes every ticker returned by LBank without checking that the symbol is in the exchange’s available pairs list. If the API includes delisted/unsupported markets (a common case when exchanges keep historical tickers), ticker.ProcessTicker will store prices for pairs that the exchange config explicitly doesn’t support, which can leak into balance pricing and strategy selection. Consider matching tickerInfo[j].Symbol via MatchSymbolWithAvailablePairs (or verifying against GetAvailablePairs) before calling ProcessTicker so only configured markets are stored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adjusts several exchange wrappers’ UpdateTickers implementations to stop filtering by enabled pairs and instead use available/supported pairs (or process all returned tickers), so clients can fetch tickers for supported markets even when not enabled for trading.
Changes:
- Switch ticker updates from
GetEnabledPairs-based filtering to using available pairs and/or symbol-to-available-pair matching. - Simplify ticker update loops by iterating over returned ticker payloads and matching symbols back to available pairs.
- Mark some previously “batch ticker” paths as unsupported (e.g., OKX Spread, Gateio Options) to avoid consecutive REST calls.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| exchanges/yobit/yobit_wrapper.go | Use available pairs for batching, then filter returned symbols via available-pair matching. |
| exchanges/poloniex/poloniex_wrapper.go | Iterate tickers returned by API and match each symbol to available pairs. |
| exchanges/okx/okx_wrapper.go | Remove per-enabled-pair filtering; Spread UpdateTickers now returns unsupported. |
| exchanges/lbank/lbank_wrapper.go | Remove enabled-pair filtering and process all returned tickers. |
| exchanges/kucoin/kucoin_wrapper.go | Remove enabled-pair filtering for futures and ignore “enabled” flag on symbol matching. |
| exchanges/hitbtc/hitbtc_wrapper.go | Remove enabled-pair filtering by ignoring the “enabled” return from symbol matching. |
| exchanges/gateio/gateio_wrapper.go | Options UpdateTickers now returns unsupported instead of per-underlying REST loops. |
| exchanges/exmo/exmo_wrapper.go | Remove enabled-pair filtering by ignoring the “enabled” return from symbol matching. |
| exchanges/bybit/bybit_wrapper.go | Remove enabled-pair membership checks; rely on available-pair symbol matching. |
| exchanges/btcmarkets/btcmarkets_wrapper.go | Use available pairs for ticker fetching; remove strict length mismatch check. |
| exchanges/bitmex/bitmex_wrapper.go | Remove enabled-pair filtering by ignoring the “enabled” return from symbol matching. |
| exchanges/bitfinex/bitfinex_wrapper.go | Remove enabled-pair filtering by ignoring the “enabled” return from symbol matching. |
| exchanges/binance/binance_wrapper.go | Replace nested enabled-pairs scan with symbol-to-available-pair matching per ticker. |
Comments suppressed due to low confidence (1)
exchanges/okx/okx_wrapper.go:430
- For asset.Margin, instrumentType is forced to instTypeSpot and the code processes all returned spot tickers without checking whether each instrument exists in the available pair set for the requested assetType. This can populate margin tickers for instruments that aren't actually available for margin. Filter each ticker through MatchSymbolWithAvailablePairs (or AvailablePairs.Contains) for
assetTypebefore calling ticker.ProcessTicker.
instrumentType := GetInstrumentTypeFromAssetItem(assetType)
if assetType == asset.Margin {
instrumentType = instTypeSpot
}
ticks, err := e.GetTickers(ctx, instrumentType, "", "")
exchanges/gateio/gateio_wrapper.go
Outdated
| } | ||
| } | ||
| } | ||
| return common.ErrFunctionNotSupported |
There was a problem hiding this comment.
UpdateTickers now returns common.ErrFunctionNotSupported for asset.Options, but Gateio's wrapper tests currently assert UpdateTickers does not error for all assets returned by GetAssetTypes(false) (see exchanges/gateio/gateio_test.go:1768-1773). To keep CI green, either maintain Options support here, or update the exchange/test expectations to treat Options UpdateTickers as unsupported.
| return common.ErrFunctionNotSupported | |
| // Options tickers are currently treated as a supported no-op. | |
| // This ensures UpdateTickers does not return an error for all | |
| // assets returned by GetAssetTypes(false), including Options. | |
| return nil |
exchanges/exmo/exmo_wrapper.go
Outdated
| if !errors.Is(err, currency.ErrPairNotFound) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
If MatchSymbolCheckEnabled returns currency.ErrPairNotFound, this code continues and processes a ticker with an empty pair, which causes ticker.ProcessTicker to return an error and stops UpdateTickers. Add a continue when ErrPairNotFound is encountered (and only return non-ErrPairNotFound errors).
| if !errors.Is(err, currency.ErrPairNotFound) { | |
| return err | |
| } | |
| if errors.Is(err, currency.ErrPairNotFound) { | |
| continue | |
| } | |
| return err |
exchanges/kucoin/kucoin_wrapper.go
Outdated
| for t := range ticks.Tickers { | ||
| pair, enabled, err := e.MatchSymbolCheckEnabled(ticks.Tickers[t].Symbol, assetType, true) | ||
| pair, _, err := e.MatchSymbolCheckEnabled(ticks.Tickers[t].Symbol, assetType, true) | ||
| if err != nil && !errors.Is(err, currency.ErrPairNotFound) { |
There was a problem hiding this comment.
ErrPairNotFound from MatchSymbolCheckEnabled is currently ignored, but the code then calls ticker.ProcessTicker with pair (which will be EMPTYPAIR on ErrPairNotFound). This results in a "pair not set" error and may cause UpdateTickers to return errors unexpectedly. Skip processing (continue) when the pair cannot be matched.
| if err != nil && !errors.Is(err, currency.ErrPairNotFound) { | |
| if err != nil { | |
| if errors.Is(err, currency.ErrPairNotFound) { | |
| continue | |
| } |
| case asset.Spread: | ||
| format, err := e.GetPairFormat(asset.Spread, true) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| pairs, err := e.GetEnabledPairs(assetType) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for y := range pairs { | ||
| var spreadTickers []SpreadTicker | ||
| spreadTickers, err = e.GetPublicSpreadTickers(ctx, format.Format(pairs[y])) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for x := range spreadTickers { | ||
| pair, err := currency.NewPairDelimiter(spreadTickers[x].SpreadID, format.Delimiter) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = ticker.ProcessTicker(&ticker.Price{ | ||
| Last: spreadTickers[x].Last.Float64(), | ||
| Bid: spreadTickers[x].BidPrice.Float64(), | ||
| BidSize: spreadTickers[x].BidSize.Float64(), | ||
| Ask: spreadTickers[x].AskPrice.Float64(), | ||
| AskSize: spreadTickers[x].AskSize.Float64(), | ||
| Pair: pair, | ||
| ExchangeName: e.Name, | ||
| AssetType: assetType, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return common.ErrFunctionNotSupported |
There was a problem hiding this comment.
UpdateTickers now returns common.ErrFunctionNotSupported for asset.Spread, but Okx's wrapper tests currently expect UpdateTickers to succeed for all assets returned by GetAssetTypes(false) (see exchanges/okx/okx_test.go:3355-3361). Either restore support for Spread tickers, adjust the exchange's advertised asset set for this operation, or update the tests/callers to allow ErrFunctionNotSupported for Spread.
exchanges/hitbtc/hitbtc_wrapper.go
Outdated
| if !errors.Is(err, currency.ErrPairNotFound) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When MatchSymbolCheckEnabled returns currency.ErrPairNotFound, the loop still falls through and calls ticker.ProcessTicker with an empty pair, which will error ("pair not set") and abort UpdateTickers. Treat ErrPairNotFound as a skip condition (continue) and only return unexpected errors.
| if !errors.Is(err, currency.ErrPairNotFound) { | |
| return err | |
| } | |
| if errors.Is(err, currency.ErrPairNotFound) { | |
| continue | |
| } | |
| return err |
exchanges/bitmex/bitmex_wrapper.go
Outdated
| if err != nil && !errors.Is(err, currency.ErrPairNotFound) { | ||
| return err | ||
| } | ||
| if !enabled { | ||
| continue | ||
| } | ||
|
|
||
| err = ticker.ProcessTicker(&ticker.Price{ |
There was a problem hiding this comment.
If MatchSymbolCheckEnabled returns currency.ErrPairNotFound, UpdateTickers still calls ticker.ProcessTicker with an empty pair, which will error and abort the whole ticker update. Handle ErrPairNotFound by continuing the loop (only returning for non-ErrPairNotFound errors).
| for key, val := range t { | ||
| pair, enabled, err := e.MatchSymbolCheckEnabled(key[1:], a, true) | ||
| pair, _, err := e.MatchSymbolCheckEnabled(key[1:], a, true) | ||
| if err != nil && !errors.Is(err, currency.ErrPairNotFound) { |
There was a problem hiding this comment.
currency.ErrPairNotFound is not treated as a skip: when MatchSymbolCheckEnabled returns ErrPairNotFound, the code still calls ticker.ProcessTicker with an empty pair and then appends that error. Add an explicit continue when ErrPairNotFound occurs to avoid generating avoidable errors for unmatched symbols.
| if err != nil && !errors.Is(err, currency.ErrPairNotFound) { | |
| if err != nil { | |
| if errors.Is(err, currency.ErrPairNotFound) { | |
| // Skip unmatched symbols to avoid processing tickers with an empty pair. | |
| continue | |
| } |
shazbert
added
nomerge
PR Description
Externally run strategies will have enabled pairs set but dynamically changing pairs create potential holes when pricing balances when using get tickers functionality.
note: It's imperative we remove wrapper impediments for REST calls e.g. if an asset is not enabled but supported. A client should be able to call these functions outside of enabled pairs/assets trading scope for hedging and pricing. This also includes returned websocket stream data.
gloriousEdit:
Fixes # (issue)
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist