orderbook: consolidate slice array types to orderbook package #1992

shazbert · 2025-08-05T06:36:37Z

PR Description

Refactor orderbook type processing for [][2]types.Number

Replaced them with a unified approach using orderbook.LevelsArrayPriceAmount type
Handles unmarshalling

Fixes # (issue)

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

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.

go test ./... -race
golangci-lint run
Test X

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Github Actions with my changes
Any dependent changes have been merged and published in downstream modules

Copilot

Pull Request Overview

This PR consolidates orderbook slice array types across multiple exchanges to use a unified orderbook.LevelsArrayPriceAmount type. The refactor replaces scattered usage of [][2]types.Number with a centralized approach that includes JSON unmarshaling functionality, reducing code duplication and improving maintainability.

Introduces orderbook.LevelsArrayPriceAmount type with unified JSON unmarshaling
Replaces manual orderbook level conversion loops with direct method calls
Simplifies orderbook processing code across 10+ exchange implementations

Reviewed Changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
exchanges/orderbook/orderbook_types.go	Adds new `LevelsArrayPriceAmount` type with JSON unmarshaling and conversion methods
exchanges/poloniex/poloniex_types.go	Updates orderbook response types to use new unified type
exchanges/poloniex/poloniex.go	Removes manual conversion loops, uses new `Levels()` method
exchanges/lbank/lbank_*.go	Converts from manual slice processing to unified type usage
exchanges/kucoin/kucoin_*.go	Replaces custom conversion functions with unified approach
exchanges/gateio/gateio_*.go	Updates orderbook handling to use new type system
exchanges/bybit/bybit_*.go	Removes custom conversion code, adopts unified type
exchanges/btcmarkets/btcmarkets_*.go	Simplifies orderbook processing with new type
exchanges/bitstamp/bitstamp_*.go	Updates websocket and REST orderbook handling
exchanges/binance/binance_*.go	Comprehensive update across Binance family exchanges

Comments suppressed due to low confidence (2)

exchanges/kucoin/kucoin_futures.go:156

The function name unifyFuturesOrderbook is unclear. Consider a more descriptive name like convertFuturesOrderbookResponse or buildOrderbookFromResponse.

func unifyFuturesOrderbook(o *futuresOrderbookResponse) *Orderbook {

exchanges/kucoin/kucoin.go:142

The function name unifySpotOrderbook is unclear. Consider a more descriptive name like convertSpotOrderbookResponse or buildOrderbookFromResponse.

func unifySpotOrderbook(o *orderbookResponse) *Orderbook {

exchanges/bybit/bybit_types.go

Copilot · 2025-08-05T06:37:33Z

exchanges/kucoin/kucoin_types.go

+	Asks     orderbook.LevelsArrayPriceAmount `json:"asks"`
+	Bids     orderbook.LevelsArrayPriceAmount `json:"bids"`
+	Time     types.Time                       `json:"time"`
+	Sequence types.Number                     `json:"sequence"`


The change from string to types.Number for the Sequence field appears unrelated to the main orderbook consolidation purpose and should be in a separate commit or PR.

Suggested change

Sequence types.Number `json:"sequence"`

Sequence string `json:"sequence"`

Copilot · 2025-08-05T06:37:34Z

exchanges/gateio/gateio_types.go

-	for x := range a.Asks {
-		asks[x].Price = a.Asks[x][0]
-		asks[x].Amount = a.Asks[x][1]
+	return &Orderbook{ID: a.ID, Current: a.Current, Update: a.Update, Asks: OrderbookLevels(a.Asks.Levels()), Bids: OrderbookLevels(a.Bids.Levels())}


[nitpick] The introduction of OrderbookLevels type and its conversion logic appears to add unnecessary complexity. Consider whether this intermediate type is needed or if the orderbook could use orderbook.Levels directly.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

codecov · 2025-08-05T06:43:09Z

Codecov Report

❌ Patch coverage is 80.42328% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.58%. Comparing base (d5b2cf1) to head (303ff84).

Files with missing lines	Patch %	Lines
exchanges/binanceus/binanceus_wrapper.go	44.44%	3 Missing and 2 partials ⚠️
exchanges/poloniex/poloniex.go	37.50%	4 Missing and 1 partial ⚠️
exchanges/btcmarkets/btcmarkets_wrapper.go	69.23%	3 Missing and 1 partial ⚠️
exchanges/binance/binance_websocket.go	76.92%	2 Missing and 1 partial ⚠️
exchanges/binance/binance_wrapper.go	76.92%	2 Missing and 1 partial ⚠️
exchanges/kucoin/kucoin.go	62.50%	2 Missing and 1 partial ⚠️
exchanges/kucoin/kucoin_futures.go	72.72%	0 Missing and 3 partials ⚠️
exchanges/gateio/gateio_wrapper.go	71.42%	1 Missing and 1 partial ⚠️
exchanges/kucoin/kucoin_websocket.go	88.88%	0 Missing and 2 partials ⚠️
exchanges/kucoin/kucoin_wrapper.go	33.33%	1 Missing and 1 partial ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1992      +/-   ##
==========================================
- Coverage   39.70%   39.58%   -0.12%     
==========================================
  Files         434      434              
  Lines      171748   171368     -380     
==========================================
- Hits        68185    67829     -356     
+ Misses      96417    96395      -22     
+ Partials     7146     7144       -2

Files with missing lines	Coverage Δ
exchanges/binance/binance.go	`45.98% <100.00%> (-0.58%)`	⬇️
exchanges/binance/binance_types.go	`100.00% <ø> (ø)`
exchanges/binanceus/binanceus.go	`22.04% <100.00%> (-0.50%)`	⬇️
exchanges/binanceus/binanceus_types.go	`100.00% <ø> (ø)`
exchanges/binanceus/binanceus_websocket.go	`43.12% <100.00%> (-1.85%)`	⬇️
exchanges/bitstamp/bitstamp_websocket.go	`64.80% <100.00%> (-1.08%)`	⬇️
exchanges/btcmarkets/btcmarkets.go	`18.76% <100.00%> (-2.82%)`	⬇️
exchanges/btcmarkets/btcmarkets_types.go	`100.00% <ø> (ø)`
exchanges/bybit/bybit_types.go	`100.00% <ø> (ø)`
exchanges/bybit/bybit_websocket.go	`73.84% <100.00%> (-0.43%)`	⬇️
... and 20 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cranktakular

Got a variety of simple/minor changes I want. With those fixed, I could tACK this.

cranktakular · 2025-08-26T02:36:19Z

exchanges/orderbook/orderbook_test.go

+	t.Parallel()
+
+	var asks LevelsArrayPriceAmount
+	err := asks.UnmarshalJSON([]byte(`[[1,2],[3,4]]`))


Super optional, but if we want to be more robust in testing the functionality this should cover, you could use the byte slice [[1,2],["3","4"]], covering both supported ways of representing numbers without making a significant change to the test.

cranktakular · 2025-08-26T02:40:36Z

exchanges/binance/binance.go

@@ -121,36 +121,20 @@ func (e *Exchange) GetExchangeInfo(ctx context.Context) (ExchangeInfo, error) {
 // OrderBookDataRequestParams contains the following members
 // symbol: string of currency pair
 // limit: returned limit amount


With OrderBookDataRequestParams being removed, these comments explaining it should be too.

cranktakular · 2025-08-26T02:47:16Z

exchanges/binance/binance_websocket.go

+func (e *Exchange) SeedLocalCacheWithBook(p currency.Pair, orderbookNew *OrderBookResponse) error {
+	t := orderbookNew.Timestamp.Time()
+	if t.IsZero() {
+		t = time.Now() // Time not provided for this REST book.


Is this handling still necessary? Are there ever books which don't return their time?

Yeah it seems like all spot orderbooks don't return a time value.

Huh, but GetOrderBook() is in the non-futures file, and has a timestamp in its response struct. If that can just receive futures responses too, don't worry, but if it's spot-only, I think including a timestamp field there if one is never returned is bad.

Yeah you are right I was most likely thinking when I convert this over to manage other assets as well when I do a full ob pass that this will auto utilise the shared field. I can drop it if you really want but it might be missed in a future upgrade pass.

You're better informed on the tradeoffs than me, so use your own judgement.

Will leave this unresolved for other peoples perspective. Either way when somebody is doing some hectic trading and they see a latency discrepancy they will be all like 👻 and open a holy PR that rectifies this issue. 🙏

cranktakular · 2025-08-26T02:54:43Z

exchanges/binanceus/binanceus_wrapper.go

+	if err != nil {
+		return nil, err
+	}
+


Some unnecessary newlines here 💢

cranktakular · 2025-08-26T03:12:02Z

exchanges/btcmarkets/btcmarkets_types.go

+	MarketID   currency.Pair                    `json:"marketId"`
+	SnapshotID int64                            `json:"snapshotId"`
+	Asks       orderbook.LevelsArrayPriceAmount `json:"asks"`
+	Bids       orderbook.LevelsArrayPriceAmount `json:"bids"`
 }

 // OBData stores orderbook data


This type doesn't seem to be used any more.

cranktakular · 2025-08-26T03:22:40Z

exchanges/gateio/gateio_wrapper.go

-		return book, err
-	}
-	return orderbook.Get(e.Name, book.Pair, a)
+	return orderbook.Get(e.Name, p, a)


The orderbook is processed with p.Upper(); shouldn't the same be used here, just in case?

Nope doesn't matter in the storage it just splits it for the *currency.Item I can drop the upper when processing with no harm as well.

cranktakular · 2025-08-26T03:24:27Z

exchanges/gateio/gateio_types.go

+func (o *OrderbookLevels) UnmarshalJSON(data []byte) error {
+	var levels []OrderbookItem
+	if err := json.Unmarshal(data, &levels); err != nil {
+		return err


This new function misses coverage on this line, consider adding a test to get some.

cranktakular · 2025-08-26T03:30:25Z

exchanges/kucoin/kucoin.go

+	return unifySpotOrderbook(o), nil
+}
+
+func unifySpotOrderbook(o *orderbookResponse) *Orderbook {


Given the goal of unifying spot with futures, would you have any interest in adding a Symbol field to Orderbook, which would be fulfilled by passing in the symbol parameter from the original function calls? That'd stop this little data loss.

Rather not; The caller inputs the symbol field param, it already has the pair upstream and the responses don't return the symbol field back. If it had it I would then add it as well.

cranktakular · 2025-08-26T03:39:35Z

exchanges/poloniex/poloniex_types.go

+	Bids     orderbook.LevelsArrayPriceAmount `json:"bids"`
+	IsFrozen string                           `json:"isFrozen"`
+	Error    string                           `json:"error"`
+	Seq      int64                            `json:"seq"`
 }

 // OrderbookItem holds data on an individual item


This PR seems to leave this type unused.

cranktakular · 2025-08-26T03:43:25Z

exchanges/binanceus/binanceus_types.go

-	Symbol currency.Pair `json:"symbol"` // Required field; example LTCBTC,BTCUSDT
-	Limit  int64         `json:"limit"`  // Default 100; max 5000. If limit > 5000, then the response will truncate to 5000
-}
-
 // OrderbookItem stores an individual orderbook item
 type OrderbookItem struct {


This PR seems to render this type unused.

orderbook: consolidate slice array types to orderbook package

aef286a

shazbert requested a review from a team August 5, 2025 06:36

shazbert self-assigned this Aug 5, 2025

Copilot AI review requested due to automatic review settings August 5, 2025 06:36

shazbert added the review me This pull request is ready for review label Aug 5, 2025

Copilot AI reviewed Aug 5, 2025

View reviewed changes

Update exchanges/bybit/bybit_types.go

83d1483

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

linter: fix and add test

8941bcb

shazbert requested review from gbjk, thrasher-, gloriousCode, samuael and cranktakular August 18, 2025 03:29

shazbert mentioned this pull request Aug 26, 2025

bybit: Add convert functions #1993

Open

15 tasks

cranktakular requested changes Aug 26, 2025

View reviewed changes

shazbert added 2 commits August 26, 2025 14:07

Merge branch 'master' into ob_array_type

82fbbbd

cranktakular: nits

303ff84

shazbert requested a review from cranktakular August 26, 2025 04:43

	Sequence types.Number `json:"sequence"`
	Sequence string `json:"sequence"`

orderbook: consolidate slice array types to orderbook package #1992

Are you sure you want to change the base?

orderbook: consolidate slice array types to orderbook package #1992

Conversation

shazbert commented Aug 5, 2025

PR Description

Type of change

How has this been tested

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cranktakular left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 5, 2025 •

edited

Loading