exchanges: Update Binance #1546
Conversation
… into binance_update
… into binance_update
… into binance_update
… into binance_update
… into binance_update
… into binance_update
There was a problem hiding this comment.
Pull Request Overview
This PR significantly updates the Binance exchange implementation with comprehensive enhancements to WebSocket functionality, new API endpoints for European Options and Portfolio Margin, and introduces new order types. The update includes:
- WebSocket streaming support for USDT-margined and coin-margined futures
- Addition of European Options endpoints and Portfolio Margin functionality
- Support for new order types (LimitMaker, TakeProfitLimit, OTO, SOR)
- Updated configuration format for improved asset type management
Reviewed Changes
Copilot reviewed 41 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/configtest.json | Updated Binance configuration with new asset types and currency pairs structure |
| exchanges/request/limit.go | Changed Weight type from uint8 to uint32 for higher rate limits |
| exchanges/order/orders.go | Added support for new order types and their string conversions |
| exchanges/order/order_types.go | Defined new order type constants and string representations |
| exchanges/order/order_test.go | Added test cases for new order types |
| exchanges/okx/okx_types.go | Removed unused error variables |
| exchanges/okx/okx_test.go | Updated error assertion references |
| exchanges/okx/okx.go | Replaced deprecated error variables with standard order errors |
| exchanges/mock/ | Refactored test code to use assert/require pattern |
| exchanges/mock/recording.go | Improved data filtering and JSON processing with slicing limits |
| exchanges/kraken/kraken_wrapper.go | Added time in force validation function |
| exchanges/kline/ | Added FiveHundredMilliseconds interval support |
| exchanges/exchange.go | Added RestOptions URL type |
| exchanges/binance/ | New type definitions and conversion utilities |
exchanges/binance/type_convert.go
Outdated
| err := json.Unmarshal(data, &singlePriceChange) | ||
| if err != nil { | ||
| var resp []PriceChangeStats | ||
| err = json.Unmarshal(data, a) |
There was a problem hiding this comment.
The unmarshal operation is attempting to decode into the receiver a instead of the temporary variable resp. This should be err = json.Unmarshal(data, &resp) to maintain consistency with the error handling pattern.
| err = json.Unmarshal(data, a) | |
| err = json.Unmarshal(data, &resp) |
| switch v.(type) { | ||
| case map[string]any, []any: | ||
| checkedData, err := CheckJSON(v, excluded) | ||
| for i := range value { |
There was a problem hiding this comment.
The loop processes all elements in the slice but then applies a limit check after appending. Consider applying the limit check before processing to avoid unnecessary iterations: limit := len(value); if limit > 0 && limit > maxLimit { limit = maxLimit }; for i := 0; i < limit; i++ {
| for i := range value { | |
| limitToUse := len(value) | |
| if limit > 0 && limit < limitToUse { | |
| limitToUse = limit | |
| } | |
| for i := 0; i < limitToUse; i++ { |
… into binance_update
|
@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".
| var resp *WithdrawResponse | ||
| return resp.ID, e.SendAuthHTTPRequest(ctx, | ||
| exchange.RestSpot, |
There was a problem hiding this comment.
[P1] Dereferencing nil withdrawal response before HTTP call
The new WithdrawCrypto declares var resp *WithdrawResponse and immediately returns resp.ID alongside the result of SendAuthHTTPRequest. If the HTTP request returns an error, resp stays nil but the expression resp.ID is evaluated first, causing a panic instead of propagating the error. Initialising resp or returning an empty ID before the request succeeds would avoid the nil dereference.
Useful? React with 👍 / 👎.
shazbert
removed
the
nomerge requires dependency
|
This PR is stale because it has been open 30 days with no activity. Please provide an update on the progress of this PR. |
… into binance_update
… into binance_update
|
This PR is stale because it has been open 21 days with no activity. Please provide an update on the progress of this PR. |
… into binance_update
… into binance_update
… into binance_update
… into binance_update
4 participants
PR Description
This PR updates the Binance exchange by addressing WebSocket enhancements, adding or updating endpoints, and implementing unit tests for each endpoint, along with mock records. Additionally, it introduces new endpoints under European Options and Portfolio Margin and extends WebSocket streaming support for futures and spot endpoints. The new WebSocket streaming functionality supports USDT-margined and coin-margined futures assets, establishing connections and managing incoming stream data separately. Furthermore, this PR adds a group of endpoints that utilize endpoint calls through the WebSocket stream, building on the previous Binance WebSocket update.
Requires #1968
Type of change
Please delete options that are not relevant and add an
xin[]as the 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