gateio: add websocket resub manager and orderbook update with snapshot functionality [spot] #2045
Conversation
… with snapshot functionality [spot]
shazbert
requested review from
Copilot,
cranktakular,
gbjk,
gloriousCode,
samuael and
thrasher-
There was a problem hiding this comment.
Pull Request Overview
This PR implements a websocket subscription manager and orderbook update functionality with snapshot support for the GateIO exchange. The implementation focuses on spot trading with the ability to handle both incremental updates and full snapshots for orderbook data.
Key changes:
- Adds a new websocket subscription manager to handle resubscriptions and track subscription states
- Implements orderbook update with snapshot functionality for handling both full snapshots and incremental updates
- Updates currency package to expose
ErrCannotCreatePairas a public variable for better error handling
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| exchanges/gateio/ws_ob_sub_manager.go | New subscription manager implementation with resubscription tracking |
| exchanges/gateio/ws_ob_sub_manager_test.go | Comprehensive test suite for the subscription manager |
| exchanges/gateio/gateio_websocket.go | Enhanced websocket handling with new orderbook update with snapshot channel |
| exchanges/gateio/gateio_websocket_test.go | Test cases for the new orderbook update functionality |
| exchanges/gateio/gateio_types.go | New type definition for orderbook update with snapshot data |
| exchanges/gateio/gateio_wrapper.go | Integration of the subscription manager in exchange setup |
| exchanges/gateio/gateio.go | Addition of subscription manager field to Exchange struct |
| currency/pair.go | Export of ErrCannotCreatePair error variable |
| currency/pair_methods.go | Updated to use the exported error variable |
| currency/pair_test.go | Test updates to use the exported error variable |
| currency/pairs_test.go | Test updates to use the exported error variable |
exchanges/gateio/gateio_websocket.go
Outdated
| }); err != nil { | ||
| return err | ||
| } | ||
| e.wsOBSubMgr.CompletedResubscribe(pair, asset.Spot) // asset.Spot used so that all pathways don't compete |
There was a problem hiding this comment.
[nitpick] The hardcoded use of asset.Spot in comments suggests this is a temporary solution. Consider using a more explicit approach or a constant to make this design decision clearer and more maintainable.
exchanges/gateio/gateio_websocket.go
Outdated
| if e.wsOBSubMgr.IsResubscribing(pair, asset.Spot) { // asset.Spot used so that all pathways don't compete | ||
| continue // Drop incremental updates; waiting for a fresh snapshot | ||
| } |
There was a problem hiding this comment.
[nitpick] The hardcoded use of asset.Spot in comments suggests this is a temporary solution. Consider using a more explicit approach or a constant to make this design decision clearer and more maintainable.
exchanges/gateio/gateio_websocket.go
Outdated
| } | ||
|
|
||
| if lastUpdateID, _ := e.Websocket.Orderbook.LastUpdateID(pair, a); lastUpdateID+1 != data.FirstUpdateID { | ||
| errs = common.AppendError(errs, e.wsOBSubMgr.Resubscribe(e, conn, data.Channel, pair, asset.Spot)) // asset.Spot used so that all pathways don't compete |
There was a problem hiding this comment.
[nitpick] The hardcoded use of asset.Spot in comments suggests this is a temporary solution. Consider using a more explicit approach or a constant to make this design decision clearer and more maintainable.
|
@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 suggestions.
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, or 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 fix this CI failure" or "@codex address that feedback".
| // Resubscribe marks a subscription as resubscribing and starts the unsubscribe/resubscribe process | ||
| func (m *wsSubscriptionManager) Resubscribe(e *Exchange, conn websocket.Connection, qualifiedChannel string, pair currency.Pair, a asset.Item) error { | ||
| if err := e.Websocket.Orderbook.InvalidateOrderbook(pair, a); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| m.m.Lock() | ||
| defer m.m.Unlock() | ||
|
|
||
| sub := e.Websocket.GetSubscription(newQualifiedChannelKey(qualifiedChannel)) | ||
| if sub == nil { | ||
| return fmt.Errorf("%w: %q", subscription.ErrNotFound, qualifiedChannel) | ||
| } | ||
|
|
||
| m.lookup[key.PairAsset{Base: pair.Base.Item, Quote: pair.Quote.Item, Asset: a}] = true | ||
|
|
||
| go func() { // Has to be called in routine to not impede websocket throughput | ||
| if err := e.Websocket.ResubscribeToChannel(conn, sub); err != nil { | ||
| log.Errorf(log.ExchangeSys, "Failed to resubscribe to channel %q: %v", qualifiedChannel, err) |
There was a problem hiding this comment.
[P1] Clear resubscribe flag when ResubscribeToChannel fails
The subscription manager marks a pair as resubscribing (lookup[...] = true) before starting the goroutine that calls e.Websocket.ResubscribeToChannel. If that call returns an error, the code only logs it and never clears the flag. Because processOrderbookUpdateWithSnapshot skips every incremental update while IsResubscribing is true, a single failed resubscribe attempt leaves the order book stuck forever, dropping all future updates until the process is restarted. Consider clearing the flag or retrying when ResubscribeToChannel fails so that pairs can recover after a transient websocket error.
Useful? React with 👍 / 👎.
exchanges/gateio/gateio_websocket.go
Outdated
| pair, err := currency.NewPairFromString(strings.Split(data.Channel, ".")[1]) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
[P2] Validate channel format before indexing
In processOrderbookUpdateWithSnapshot the pair is parsed with strings.Split(data.Channel, ".")[1] without checking the slice length. If Gate.io ever sends an unexpected payload (e.g. a malformed s field or an empty string), this will panic the websocket handler and tear down the connection. Guarding against a split shorter than two elements avoids the crash and allows the handler to return a parse error instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
If I have a strategy actively trading this and they cook their values I am okay for this to panic. But if other people want to me to jank around let me know.
There was a problem hiding this comment.
It's not without precedent to assume splits have a len (Kucoin ws topics). It's a bit of a lame spot to panic on though (versus say a Must... func). Not having order book data for a dumb pair sounds better than this dying when you could be doing many other things elsewhere on other exchanges. Plus PR turnaround time to fix it
I'd prefer the check
samuael
left a comment
samuael
left a comment
There was a problem hiding this comment.
Just a quick review. The PR looks clean. Thanks.
Just left a minor suggestion on unit tests
|
|
||
| m := newWSSubscriptionManager() | ||
| require.NotNil(t, m) | ||
| require.NotNil(t, m.lookup) |
There was a problem hiding this comment.
Is it necessary to use require on these last assertify checks? This is below.
| require.NoError(t, err) | ||
| err = m.Resubscribe(e, conn, "ob.BTC_USDT.50", currency.NewBTCUSDT(), asset.Spot) | ||
| require.NoError(t, err) | ||
| require.True(t, m.IsResubscribing(currency.NewBTCUSDT(), asset.Spot)) |
| "github.com/thrasher-corp/gocryptotrader/log" | ||
| ) | ||
|
|
||
| type wsSubscriptionManager struct { |
There was a problem hiding this comment.
Can somebody suggest a better name for this?
There was a problem hiding this comment.
wsOrderbookResubscriptionHandler
wsCanSomeoneSuggestABetterNameForThis
wsObResubManager
wsOrderbooSilverIsMooningOMG
There was a problem hiding this comment.
wsOrderbookSilverIsMooningOMG is my pick thanks!
shazbert
changed the title
samuael
left a comment
samuael
left a comment
There was a problem hiding this comment.
couldn't find much but left two suggestion on minor issues
| return err | ||
| } | ||
|
|
||
| m.m.Lock() |
There was a problem hiding this comment.
The lock can be moved to line 45; below GetSubscription() call
| delete(m.lookup, key.PairAsset{Base: pair.Base.Item, Quote: pair.Quote.Item, Asset: a}) | ||
| } | ||
|
|
||
| func newQualifiedChannelKey(qualifiedChannel string) qualifiedChannelKey { |
There was a problem hiding this comment.
(Optional suggestion) I see no need to declare this as it only used once and the qualifiedChannelKey{} instance creation can also be don inside the Resubscribe function
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2045 +/- ##
==========================================
+ Coverage 41.93% 41.99% +0.05%
==========================================
Files 443 444 +1
Lines 143138 143220 +82
==========================================
+ Hits 60021 60139 +118
+ Misses 75977 75934 -43
- Partials 7140 7147 +7
🚀 New features to boost your workflow:
|
shazbert
added
reconstructing
… handle error from LastUpdateID
Co-authored-by: Gareth Kirwan <gbjkirwan@gmail.com>
shazbert
added
blocked
and removed
review me
|
|
shazbert
added
review me
|
This PR is stale because it has been open 21 days with no activity. Please provide an update on the progress of this PR. |
|
Go away stale tag! Other things are a priority. |
gloriousCode
left a comment
gloriousCode
left a comment
There was a problem hiding this comment.
Looks good!
INFO] | SYNC | 20/01/2026 17:50:30 | GateIO websocket BTC-USDT SPOT ORDERBOOK: Bids len: 50 Amount: 3.457137 BTC. Total value: 314203.20859570004 Asks len: 50 Amount: 2.550006 BTC. Total value: 231906.23394580002
[DEBUG] | WEBSOCKET | 20/01/2026 17:50:30 | GateIO wss://api.gateio.ws/ws/v4/: Message received: {"channel":"spot.obu","result":{"t":1768891830817,"s":"ob.BTC_USDT.50","u":31518294256,"U":31518294200,"b":[["90914.9","0.22685"],["90908.5","0.011006"],["90903","0"]],"a":[["90915","0.116082"],["90916.8","0.005499"],["90924.9","0.011006"],["90920.2","0"],["90967.9","0"]]},"time_ms":1768891830817,"event":"update"}
[DEBUG] | WEBSOCKET | 20/01/2026 17:50:30 | GateIO wss://api.gateio.ws/ws/v4/: Sending message: {"time":1768891830,"id":62,"channel":"spot.obu","event":"unsubscribe","payload":["ob.BTC_USDT.50"]}
[DEBUG] | WEBSOCKET | 20/01/2026 17:50:30 | GateIO wss://api.gateio.ws/ws/v4/: Message received: {"time":1768891830,"time_ms":1768891830927,"id":62,"conn_id":"ba2a016b99263e15","trace_id":"e99fd40bb8d7b4e7044a1e36e2b15d3c","channel":"spot.obu","event":"unsubscribe","payload":["ob.BTC_USDT.50"],"result":{"status":"success"},"requestId":"e99fd40bb8d7b4e7044a1e36e2b15d3c"}
[DEBUG] | WEBSOCKET | 20/01/2026 17:50:30 | GateIO wss://api.gateio.ws/ws/v4/: Sending message: {"time":1768891830,"id":63,"channel":"spot.obu","event":"subscribe","payload":["ob.BTC_USDT.50"]}
[DEBUG] | WEBSOCKET | 20/01/2026 17:50:31 | GateIO wss://api.gateio.ws/ws/v4/: Message received: {"time":1768891831,"time_ms":1768891831036,"id":63,"conn_id":"ba2a016b99263e15","trace_id":"e99fd40bb8d7b4e7044a1e36e2b15d3c","channel":"spot.obu","event":"subscribe","payload":["ob.BTC_USDT.50"],"result":{"status":"success"},
| channelParts := strings.Split(data.Channel, ".") | ||
| if len(channelParts) < 3 { | ||
| return fmt.Errorf("%w: %q", common.ErrMalformedData, data.Channel) | ||
| } |
There was a problem hiding this comment.
Do this before you convert the book plz
| return nil | ||
| } | ||
|
|
||
| // CompletedResubscribe marks a subscription as completed |
There was a problem hiding this comment.
There is no marking. Please update
Co-authored-by: Scott <gloriousCode@users.noreply.github.com>
gloriousCode
left a comment
gloriousCode
left a comment
There was a problem hiding this comment.
tACK! TY4MakinChanges
5 participants
This is only opened for spot right now for design issues than can be expanded across all assets.
resolves #2028
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