kucoin: migrate to multi-connection websocket handling #2119
Conversation
- Modified the websocket connection methods across multiple exchanges to include an additional parameter for query values in the Dial function. - This change ensures that the connection can be established with necessary tokens or parameters as required by the respective exchange APIs. - Affected files include binance, binanceus, bitfinex, bithumb, bitmex, bitstamp, btcmarkets, btse, bybit, coinbase, coinut, deribit, gateio, gemini, hitbtc, huobi, kraken, kucoin, okx, and poloniex.
shazbert
requested review from
Copilot,
cranktakular,
gbjk,
gloriousCode,
samuael 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 introduces a significant architectural improvement by refactoring the websocket connection logic to support query parameters, which is then applied across multiple exchanges. The main focus is the migration of the Kucoin exchange to use the new multi-connection websocket manager, which is a substantial and well-executed change. Additionally, it includes an important bug fix for the Kucoin futures orderbook handling, correctly using LoadSnapshot instead of Update for snapshot data. The code is cleaner and more robust. I've found one critical issue in the Kucoin websocket connection logic that needs to be addressed.
There was a problem hiding this comment.
Pull request overview
This pull request migrates KuCoin to a multi-connection websocket architecture with subscription load balancing capabilities, fixes a critical bug in the futures orderbook handling, and updates the websocket Dial signature across all exchanges to support URL query parameters without overriding the internal URL.
- Refactored KuCoin websocket to use multi-connection management with a 400 subscription per connection limit
- Fixed futures orderbook depth channels (5/50) to use
LoadSnapshotinstead of incorrectly callingUpdateafter fetching snapshots - Updated websocket
Dialinterface across 20+ exchanges to accepturl.Valuesas a fourth parameter for query string handling
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| exchange/websocket/connection.go | Updated Connection interface Dial method to accept url.Values parameter and use EncodeURLValues for proper query string handling |
| exchange/websocket/manager_test.go | Updated all test calls to Dial to include nil url.Values parameter |
| exchanges/kucoin/kucoin_wrapper.go | Migrated to multi-connection setup with 400 max subscriptions per connection, removed buffer config |
| exchanges/kucoin/kucoin_websocket.go | Refactored WsConnect to accept Connection parameter, added marketOrderbookDepth1Channel, fixed futures orderbook bug, updated Subscribe/Unsubscribe signatures |
| exchanges/kucoin/kucoin_types.go | Renamed WsOrderbookLevel5Response to WsFuturesOrderbookLevelResponse for better clarity |
| exchanges/kucoin/kucoin_test.go | Added ConnectionFixture mock, updated test calls to new signatures, added GreaterOrEqual assertion |
| exchanges/poloniex/poloniex_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/poloniex/poloniex_test.go | Updated Dial call to include nil url.Values parameter |
| exchanges/okx/okx_websocket.go | Updated Dial calls to include nil url.Values parameter |
| exchanges/okx/okx_business_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/kraken/kraken_websocket.go | Updated Dial calls to include nil url.Values parameter |
| exchanges/huobi/huobi_websocket.go | Updated Dial calls to include nil url.Values parameter |
| exchanges/hitbtc/hitbtc_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/hitbtc/hitbtc_test.go | Updated Dial call to include nil url.Values parameter |
| exchanges/gemini/gemini_websocket.go | Updated Dial calls to include nil url.Values parameter |
| exchanges/gateio/gateio_websocket*.go | Updated Dial calls across spot, futures, delivery futures, and options to include nil url.Values parameter |
| exchanges/deribit/deribit_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/coinut/coinut_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/coinut/coinut_test.go | Updated Dial call to include nil url.Values parameter |
| exchanges/coinbase/coinbase_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/coinbase/coinbase_test.go | Updated Dial call to include nil url.Values parameter |
| exchanges/bybit/bybit_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/bybit/bybit_test.go | Updated FixtureConnection.Dial signature to include url.Values parameter |
| exchanges/btse/btse_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/btcmarkets/btcmarkets_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/bitstamp/bitstamp_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/bitmex/bitmex_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/bitmex/bitmex_test.go | Updated Dial call to include nil url.Values parameter |
| exchanges/bithumb/bithumb_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/bitfinex/bitfinex_websocket.go | Updated Dial calls to include nil url.Values parameter |
| exchanges/binanceus/binanceus_websocket.go | Updated Dial call to include nil url.Values parameter |
| exchanges/binance/binance_websocket.go | Updated Dial call to include nil url.Values parameter |
| docs/ADD_NEW_EXCHANGE.md | Updated example code to show new Dial signature with nil url.Values parameter |
| cmd/exchange_template/websocket.tmpl | Updated template to show new Dial signature with nil url.Values parameter |
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to the websocket handling, migrating KuCoin to a multi-connection setup and modifying the Dial function across multiple exchanges. This is a substantial refactoring that improves future scalability and fixes a bug in the futures orderbook handling. My review focuses on the correctness of these changes, particularly in the KuCoin implementation.
Overall, the changes are well-implemented. The move to a multi-connection manager for KuCoin is a great step forward. I've identified a potential compilation error due to a missing field in a struct, which should be addressed. The rest of the changes, including the bug fix for the futures orderbook and the widespread update to the Dial method, look solid.
|
@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.
ℹ️ 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".
samuael
left a comment
samuael
left a comment
There was a problem hiding this comment.
Great that you added a support for Kucoin. I've left few comments on the changes of kucoin files.
gloriousCode
left a comment
gloriousCode
left a comment
There was a problem hiding this comment.
I dont really have much to add. Once what Sam has raised is addressed I think this is good to go
shazbert
added
reconstructing
|
have to investigate some subscribing issues |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2119 +/- ##
==========================================
- Coverage 42.06% 42.03% -0.03%
==========================================
Files 446 446
Lines 142919 142891 -28
==========================================
- Hits 60120 60066 -54
- Misses 75650 75680 +30
+ Partials 7149 7145 -4
🚀 New features to boost your workflow:
|
Not getting issues anymore. 🚀 |
shazbert
added
review me
gloriousCode
left a comment
gloriousCode
left a comment
There was a problem hiding this comment.
tackeroo mr shazoo
4 participants
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