client/mm/libxc: Binance orderbook diff updates #2627

martonp · 2023-12-05T04:46:40Z

Previously the Binance library was subscribing to a market data stream from Binance that sent the 20 orders closest to the mid-gap in every update. Now, it fetches a snapshot of 1000 orders, this is stored in memory, and is updated based on a stream that sends diffs instead. Other minor cleanup is done in the Binance library as well.

ukane-philemon

Good work!

ukane-philemon · 2023-12-05T09:30:26Z

client/mm/libxc/binance.go

-		bids:           make([]*bookBin, 0),
-		asks:           make([]*bookBin, 0),
-		numSubscribers: 1,
+// convertBinanceBook converts bids an asks in the binance format,


ukane-philemon · 2023-12-05T09:34:40Z

client/mm/libxc/binance.go

+			if i > 0 {
+				time.Sleep(2 * time.Second)
+			}


No specific reason.. this will only be called if there's a bug on the Binance side where they sent an order book update, then in a snapshot response they sent an earlier state than the update that they already sent. It just gives Binance two seconds to recover from their weird state.

ukane-philemon · 2023-12-05T09:38:15Z

client/mm/libxc/binance.go

+			if snapshot.LastUpdateID >= minUpdateID {
+				bids, asks, err := b.convertBinanceBook(snapshot.Bids, snapshot.Asks)
+				if err != nil {
+					b.log.Errorf("Error parsing binance book: %v", err)
+					return 0, false
+				}
+
+				b.log.Debugf("Got %s orderbook snapshot with update ID %d", b.mktID, snapshot.LastUpdateID)
+
+				b.book.clear()
+				b.book.update(bids, asks)
+				return snapshot.LastUpdateID, true
+			} else {
+				b.log.Infof("Snapshot last update ID %d is less than first update ID %d. Getting new snapshot...", snapshot.LastUpdateID, minUpdateID)
+			}


Pls, consider inverting this check. It'll be easier to follow.

if snapshot.LastUpdateID < minUpdateID { // Log continue } // convert orderbook

ukane-philemon · 2023-12-05T09:42:01Z

client/mm/libxc/binance.go

+	defer b.mtx.RUnlock()
+
+	if !b.synced {
+		return 0, 0, filled, fmt.Errorf("orderbook not synced")


errors.New(...)

ukane-philemon · 2023-12-05T09:44:39Z

client/mm/libxc/binance.go

+	// coin is the asset symbol on binance, always upper case.
+	// For a token like USDC, the coin field will be USDC, but
+	// symbol field will be usdc.eth.


I don't think this comment still apply. https://www.circle.com/blog/native-usdc-now-available-on-polygon-pos

Why? It will still be on ETH as well.

ukane-philemon · 2023-12-05T09:46:35Z

client/mm/libxc/binance.go

@@ -277,6 +447,9 @@ func (bnc *binance) getCoinInfo(ctx context.Context) error {
 			return 0, false
 		}
 		for _, netInfo := range nfo.NetworkList {
+			if tokenSymbol == "weth" {
+				bnc.log.Infof("NetInfo %+v", netInfo)


ukane-philemon · 2023-12-05T09:47:03Z

client/mm/libxc/binance.go

+	coins := make([]*binanceCoinInfo, 0)
+	err := bnc.getAPI(ctx, "/sapi/v1/capital/config/getall", nil, true, true, &coins)
+	if err != nil {
+		return fmt.Errorf("error getting binance coin info: %v", err)


ukane-philemon · 2023-12-05T09:57:19Z

client/mm/libxc/binance.go

 	}

-	bnc.stopMarketDataStream(strings.ToLower(baseCfg.coin + quoteCfg.coin))
 	return nil
 }

 // SubscribeMarket subscribes to order book updates on a market. This must
 // be called before calling VWAP.
 func (bnc *binance) SubscribeMarket(ctx context.Context, baseID, quoteID uint32) error {


This method seems unused in this pkg but it implements libxc.CEX. IMO, methods that satisfy libxc.CEX should mention that in their comment.

ukane-philemon · 2023-12-05T09:59:34Z

client/mm/libxc/binance.go

 	fail := func(err error) (uint64, uint64, bool, error) {
 		return 0, 0, false, err
 	}


There are only two error paths below, can remove.

ukane-philemon · 2023-12-05T10:09:29Z

client/mm/libxc/orderbook.go

+	curr := list.Front()
+	for {


we can do for curr := list.Front(); cur != nil; curr == curr.Next() { .... }, yh? Just to be consistent with what we have at L74&79

JoeGruffins · 2023-12-07T06:41:41Z

client/mm/libxc/binance.go

+// that come after the state of the snapshot. A goroutine is started that keeps
+// that keeps the orderbook in sync by repeating the sync process if an update


Suggested change

// that come after the state of the snapshot. A goroutine is started that keeps

// that keeps the orderbook in sync by repeating the sync process if an update

// that come after the state of the snapshot. A goroutine is started that keeps

// the orderbook in sync by repeating the sync process if an update

JoeGruffins · 2023-12-07T06:45:25Z

client/mm/libxc/binance.go

+		const maxTries = 5
+		for i := 0; i < maxTries; i++ {
+			if i > 0 {
+				time.Sleep(2 * time.Second)


untested

Suggested change

time.Sleep(2 * time.Second)

select {

case <- time.After(2 * time.Second):

case <- ctx.Done():

b.log.Error(ctx.Err())

return 0, false

}

It's a good change, but why log an error?

but why log an error

hmm, I guess you don't have to. Don't we usually log error for context canceled if something was in the middle of something? Maybe not.

JoeGruffins · 2023-12-07T06:50:19Z

client/mm/libxc/binance.go

+			snapshot, err := getSnapshot()
+			if err != nil {
+				b.log.Errorf("Error getting orderbook snapshot: %v", err)
+				time.Sleep(time.Second)


You have the sleep at the beginning of the loop so can probably remove.

JoeGruffins · 2023-12-07T06:55:24Z

client/mm/libxc/binance.go

+		b.mtx.Unlock()
+	}
+
+	firstUpdate := <-b.updateQueue


Maybe this should also select with a ctx.Done()?

JoeGruffins · 2023-12-07T07:02:04Z

client/mm/libxc/binance.go

+			}
+
+			if update.FirstUpdateID > latestUpdate+1 {
+				b.log.Errorf("Missed %d updates for %s orderbook. Re-syncing...", update.FirstUpdateID-latestUpdate, b.mktID)


Would log Warn be more appropriate? This is recoverable?

JoeGruffins · 2023-12-07T07:21:01Z

client/mm/libxc/binance.go

+			if update.LastUpdateID <= latestUpdate {
+				continue
+			}
+


Should always setSynced(false) here regardless? Then set synced after synced?

setSynced(false) doesn't need to be called here. This is the case where the local order book is already ahead of the state in the update. This will usually be hit on the initial sync if more than one update was received before getting the snapshot.

JoeGruffins · 2023-12-07T07:22:35Z

client/mm/libxc/binance.go

+
+			bids, asks, err := b.convertBinanceBook(update.Bids, update.Asks)
+			if err != nil {
+				b.log.Errorf("Error parsing binance book: %v", err)


should setSynced(false)?

This just means that an update with an invalid format came. Maybe synced should be set to false if no update arrives in a certain amount of time though

JoeGruffins · 2023-12-07T07:41:28Z

client/mm/libxc/binance.go

+		bnc.log.Errorf("Error unmarshaling user data stream update: %v", err)
+		bnc.log.Errorf("Raw message: %s", string(b))


May be better to combine these with a \n because they can appear lines apart with other logs between.

JoeGruffins · 2023-12-07T07:59:43Z

client/mm/libxc/binance.go

+	getSnapshot := func() (*binanceOrderbookSnapshot, error) {
+		return bnc.getOrderbookSnapshot(ctx, mktID)
+	}
+	go book.sync(ctx, getSnapshot)


May be better to watch the routine and wait for it to stop after ctx.Done() but it's probably fine.

What do you mean by watch the routine?

I mean the go book.sync(ctx, getSnapshot) goroutine. The program will not wait for it to finish on shutdown, probably. It could also be viewed as code bloat to add a waitgroup, so feel free to ignore. Maybe there's no benefit to waiting at all. If it were writing to a file maybe? I don't think it is though.

JoeGruffins · 2023-12-07T10:06:23Z

client/mm/libxc/orderbook.go

+func (ob *orderbook) String() string {
+	ob.mtx.Lock()
+	defer ob.mtx.Unlock()


Can be a read lock?

ukane-philemon suggested changes Dec 5, 2023

View reviewed changes

JoeGruffins approved these changes Dec 7, 2023

View reviewed changes

Review updates

c945b8c

martonp force-pushed the binanceRefactor branch from 9281d25 to c945b8c Compare December 16, 2023 02:03

martonp mentioned this pull request Dec 26, 2023

client/multi: Refactor MM Balance Handling #2641

Merged

buck54321 approved these changes Dec 28, 2023

View reviewed changes

buck54321 merged commit 8d37a2c into decred:master Dec 28, 2023
5 checks passed

martonp deleted the binanceRefactor branch January 20, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/mm/libxc: Binance orderbook diff updates #2627

client/mm/libxc: Binance orderbook diff updates #2627

martonp commented Dec 5, 2023

ukane-philemon left a comment

ukane-philemon Dec 5, 2023

ukane-philemon Dec 5, 2023

martonp Dec 15, 2023

ukane-philemon Dec 15, 2023

ukane-philemon Dec 5, 2023

ukane-philemon Dec 5, 2023

ukane-philemon Dec 5, 2023

martonp Dec 15, 2023

ukane-philemon Dec 5, 2023

ukane-philemon Dec 5, 2023

ukane-philemon Dec 5, 2023

ukane-philemon Dec 5, 2023

ukane-philemon Dec 5, 2023

JoeGruffins Dec 7, 2023

JoeGruffins Dec 7, 2023

martonp Dec 15, 2023

JoeGruffins Dec 19, 2023

JoeGruffins Dec 7, 2023

JoeGruffins Dec 7, 2023

JoeGruffins Dec 7, 2023

JoeGruffins Dec 7, 2023

martonp Dec 15, 2023

JoeGruffins Dec 7, 2023

martonp Dec 15, 2023

JoeGruffins Dec 7, 2023

JoeGruffins Dec 7, 2023

martonp Dec 15, 2023

JoeGruffins Dec 19, 2023

JoeGruffins Dec 7, 2023

		// that come after the state of the snapshot. A goroutine is started that keeps
		// that keeps the orderbook in sync by repeating the sync process if an update

-				time.Sleep(2 * time.Second)
+				select {
+				        case <- time.After(2 * time.Second):
+				        case <- ctx.Done():
+				                b.log.Error(ctx.Err())
+				                return 0, false
+				}

		bnc.log.Errorf("Error unmarshaling user data stream update: %v", err)
		bnc.log.Errorf("Raw message: %s", string(b))

client/mm/libxc: Binance orderbook diff updates #2627

client/mm/libxc: Binance orderbook diff updates #2627

Conversation

martonp commented Dec 5, 2023

ukane-philemon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment