Exchanges: Migrate cool exchanges to multi-connection#2167
Exchanges: Migrate cool exchanges to multi-connection#2167gloriousCode wants to merge 31 commits intothrasher-corp:masterfrom
Conversation
gloriousCode
requested review from
a team,
cranktakular,
samuael,
shazbert and
thrasher-
![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 migrates several exchanges to a new multi-connection websocket manager, which is a significant and valuable refactoring. The implementation looks mostly solid, with consistent changes across the affected exchanges.
My main concern is the removal of several websocket subscription tests from kraken_test.go and huobi_test.go. These tests covered important scenarios that should still be validated with the new architecture. I've left a comment with more details.
I also found a minor opportunity for code deduplication in bitfinex_test.go.
|
@codex please review |
There was a problem hiding this comment.
Pull request overview
This PR migrates several exchange websocket implementations to the multi-connection websocket manager, updating connectors/subscription handlers to be connection-aware and adjusting tests accordingly.
Changes:
- Updated exchange websocket setup to use
UseMultiConnectionManagementand per-connectionConnectionSetup(public/private connections, subscription scaling limits). - Refactored websocket connect/subscribe/unsubscribe/data-handler functions to accept
(ctx, conn, payload)signatures. - Updated and reworked websocket tests/fixtures to match the new connection-aware APIs.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| exchanges/okx/okx_test.go | Updates WS subscription setup in tests to align with connection-aware subscription store usage. |
| exchanges/kraken/kraken_wrapper.go | Migrates Kraken WS setup to multi-connection manager and adds public/private connection setups. |
| exchanges/kraken/kraken_websocket.go | Refactors Kraken WS connector/handler/subscription management for per-connection routing. |
| exchanges/kraken/kraken_test.go | Updates Kraken websocket tests and adds a new mock WS instance helper. |
| exchanges/huobi/huobi_wrapper.go | Adds supplementary WS endpoint and migrates Huobi WS setup to multi-connection manager. |
| exchanges/huobi/huobi_websocket.go | Refactors Huobi WS connector/auth/handler/subscription management for per-connection routing. |
| exchanges/huobi/huobi_test.go | Updates Huobi websocket fixture tests to match new handler signature. |
| exchanges/gemini/gemini_wrapper.go | Migrates Gemini WS setup to multi-connection manager with separate v2 public and v1 auth endpoints. |
| exchanges/gemini/gemini_websocket.go | Refactors Gemini WS connector/auth/subscription functions to be connection-aware. |
| exchanges/gemini/gemini_test.go | Updates Gemini websocket tests to use the connection-aware handler signature and connection lifecycle. |
| exchanges/deribit/deribit_ws_endpoints.go | Routes WS requests via Websocket.GetConnection() instead of a single global connection. |
| exchanges/deribit/deribit_wrapper.go | Migrates Deribit WS setup to multi-connection manager and enables subscription scaling. |
| exchanges/deribit/deribit_websocket.go | Refactors Deribit WS connector/auth/handler/subscription functions to be connection-aware. |
| exchanges/deribit/deribit_test.go | Updates Deribit websocket fixture tests and connect path for the new manager. |
| exchanges/coinbase/coinbase_wrapper.go | Migrates Coinbase WS setup to multi-connection manager and adapts handler signature. |
| exchanges/coinbase/coinbase_websocket.go | Refactors Coinbase WS connector/subscription management and updates handler signature. |
| exchanges/coinbase/coinbase_test.go | Updates Coinbase websocket tests and introduces helper functions for connection + subscription. |
| exchanges/bitfinex/bitfinex_wrapper.go | Migrates Bitfinex WS setup to multi-connection manager with subscription scaling limits. |
| exchanges/bitfinex/bitfinex_websocket.go | Refactors Bitfinex WS connector/handler/subscription logic to be connection-aware. |
| exchanges/bitfinex/bitfinex_test.go | Updates Bitfinex websocket tests for the new connection-aware APIs. |
| exchange/websocket/manager.go | Minor comment/formatting cleanup in multi-connection connect flow. |
![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: 9c26628631
ℹ️ 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".
gloriousCode
added
Artficial Inveigh
and removed
review me
samuael
left a comment
samuael
left a comment
There was a problem hiding this comment.
Nice work migrating the implementation to multisocket. The WebSocket Connect methods are/should be now well tested and correctly integrated with the wrapper.
thrasher-
added
review me
samuael
left a comment
samuael
left a comment
There was a problem hiding this comment.
Added another round or review comments.
shazbert
left a comment
shazbert
left a comment
There was a problem hiding this comment.
Impressive! 🚀 Just some minor things. looking good 💃
| if conn == nil { | ||
| return fmt.Errorf("%w: nil connection for %s id %d", websocket.ErrSubscriptionFailure, channelName, int64(id)) | ||
| } |
There was a problem hiding this comment.
Suggestion: Conn on every handle should always be supplied from the reader so you can rm this and let it panic. But whatevs. You do you my dude.
There was a problem hiding this comment.
🤖 I think it was inspired by all the other additional uses of it eg gateio. I have removed every instance except in manager.go
| Handler: e.wsHandleData, | ||
| ResponseCheckTimeout: exch.WebsocketResponseCheckTimeout, | ||
| ResponseMaxLimit: exch.WebsocketResponseMaxLimit, | ||
| URL: publicBitfinexWebsocketEndpoint, |
There was a problem hiding this comment.
Need to use e.API.Endpoints.GetURL(exchange.WebsocketSpot) so that the config can override this and for the auth below.
There was a problem hiding this comment.
I use gateio as an inspiration since you use it so much and I'm mislead by your implementation. This will not stand!
(I will update all implementations)
There was a problem hiding this comment.
It seems like the standard is GetURL is used for URL and is also the MessageFilter, I've rolled that out too, but let me know if that makes you mad and/or sad, especially gateio. The only exceptional circumstance is Bybit since we have 2 assets using the same connection
| Connector: e.WsConnectSpot, | ||
| Authenticate: e.authenticateSpot, | ||
| MessageFilter: asset.Spot, | ||
| MessageFilter: wsSpotURL, |
There was a problem hiding this comment.
So what is the main reason for swapping from a static key value to something potentially dynamic? I don't mind the design choice just curious. We could even drop message filter altogether and just match against the conn url.
There was a problem hiding this comment.
I'm just trying to follow your standard and make more consistent. Happy to revert! MessageFilter drop can be something in another PR with a proper look
As said earlier, there is another exchange that uses the same URL twice for a different asset, so connURL there doesnt work
There was a problem hiding this comment.
🤷 I dunno, it works. I am not fussed, if we change the URL by RPC then things could get cooked. So would need to ensure that the underlying system gets some loving on that, if we even have that support? I haven't checked. Will defer reversion until @thrasher- has a looksy. 🕺
There was a problem hiding this comment.
Was mostly in response to your other nit here. To use the GetURL, wasn't going to have a static URL be the MessageFilter and the dynamic URL as the URL. That would have been weird. Will look at this later. Just trying to do what you're after. So let me know what you want for it
PR Description
Hello programmers, my name is @gloriousCode and I'm here to say, that this work migrates some exchanges to use multi-connection and subscription scaling functions, okay!
Updated the following exchanges to use multi-connection and sub scaling:
Type of change
How has this been tested
go run .