FEATURE: [binance] listen token support for margin trading #2289
Conversation
kbearXD
requested review from
bailantaotao,
c9s,
Copilot,
dboyliao,
gx578007 and
ycdesu
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for Binance's new listenToken subscription method for margin trading user data streams, replacing the deprecated listenKey method. According to Binance's changelog, the old method via wss://stream.binance.com:9443 will be removed in the future.
Key changes:
- Adds new
listenTokenAPI integration with automatic token refresh - Maintains backward compatibility via
UseListenKeyflag for deprecated method - Implements WebSocket subscription using
userDataStream.subscribe.listenToken
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/exchange/binance/stream.go | Implements listenToken lifecycle management, adds token refresh worker, and updates WebSocket endpoint selection logic |
| pkg/exchange/binance/exchange.go | Adds UseListenKey flag and EnableListenKey() method for backward compatibility |
| pkg/exchange/binance/binanceapi/create_margin_account_listentoken_request.go | Defines API request structure for creating margin account listen tokens |
| pkg/exchange/binance/binanceapi/create_margin_account_listen_token_request_requestgen.go | Auto-generated request builder code for listen token API |
| pkg/exchange/binance/listentoken_test.go | Unit tests verifying listenToken/listenKey configuration behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: ensure that we receive an authorized event to trigger this auth event | ||
| go s.EmitAuth() | ||
| } else if !s.useListenKey && s.exchange.IsMargin { | ||
| // there is still no listenToken |
There was a problem hiding this comment.
The comment on line 279 is misleading - the condition returns when there IS no token or when it's expired, so the subsequent code runs only when a valid token EXISTS. Consider revising to: 'Skip subscription if listenToken is missing or expired'.
| // there is still no listenToken | |
| // Skip subscription if listenToken is missing or expired |
| log.Info("listenToken is about to expire, refreshing...") | ||
|
|
||
| // Refresh the listenToken |
There was a problem hiding this comment.
The log message on line 771 logs every 15 minutes regardless of whether action is needed. Consider moving this log statement inside the condition block (after line 774) or changing it to debug level to reduce noise in production logs.
| log.Info("listenToken is about to expire, refreshing...") | |
| // Refresh the listenToken | |
| // Refresh the listenToken | |
| log.Info("listenToken is about to expire, refreshing...") |
| return | ||
| } | ||
|
|
||
| timeout := listenTokenKeepAliveInterval * 3 |
There was a problem hiding this comment.
The magic number '3' used to calculate timeout lacks explanation. Consider adding a comment explaining why the timeout is 3x the keep-alive interval, or extract it as a named constant (e.g., listenTokenRefreshTimeoutMultiplier).
| s.listenTokenExpiration = newExpiration | ||
|
|
||
| if err := s.sendListenTokenSubscribeCommand(); err != nil { | ||
| log.WithError(err).Error("listenToken subscribe error") |
There was a problem hiding this comment.
After successfully refreshing the token but failing to subscribe (lines 785-788), the worker continues without resetting the ticker. This could lead to rapid retry attempts if the subscription consistently fails. Consider adding ticker.Reset(time.Minute) after the log statement, similar to the error handling in line 778.
| log.WithError(err).Error("listenToken subscribe error") | |
| log.WithError(err).Error("listenToken subscribe error") | |
| ticker.Reset(time.Minute) |
c9s
changed the title
| // UseListenKey enables the deprecated listenKey method via wss://stream.binance.com:9443 | ||
| // This method is deprecated as of 2025-10-06 and will be removed by Binance in the future | ||
| // Default behavior (false) uses the new recommended listenToken method | ||
| UseListenKey bool |
There was a problem hiding this comment.
Since we have a setter for UseListenKey, I think it's better to make it a private field?
| mu sync.RWMutex | ||
|
|
||
| // Deprecated listenKey support (will be removed by Binance in the future) | ||
| useListenKey bool |
There was a problem hiding this comment.
since we store the exchange instance at line 73, you don't need this field?
| s.listenTokenExpiration = expiration | ||
|
|
||
| debug("listenToken created, expires at: %s, starting token refresh worker", expiration) | ||
| go s.listenTokenRefreshWorker(ctx) |
There was a problem hiding this comment.
createUserDataStreamEndpoint will be triggered every time the connection is reconnected,
you will start multiple workers here
kbearXD
force-pushed
the
kbearXD/binance/margin-trading-listenToken
branch
from
b2b0137 to
f13be91
Compare
…nToken to replace listenKey
kbearXD
force-pushed
the
kbearXD/binance/margin-trading-listenToken
branch
from
f13be91 to
64c4320
Compare
…nToken to replace listenKey