stream/buffer: Improve orderbook verification conditions and fix TestUpdateByIDAndAction test (#1682)

* don't need to verify if checksum available; don't retroactively verify snapshots

* glorious: nits + allow checksum/validation to be done on updateByIDAndAction

* possible slice manipulation issue fix for test

* thrasher: nit

* this should fix it now

* for subsystem usage disallow deploying a book twice

* reverts strict policy using deploy, it's used in a bunch of tests I don't want to touch just yet, left a note

* rm unused variable

* glorious: nits

---------

Co-authored-by: Ryan O'Hara-Reid <ryan.oharareid@thrasher.io>

exchanges/orderbook/orderbook.go

@@ -79,8 +79,7 @@ func (s *Service) Update(b *Base) error {
 	return s.Mux.Publish(book, m1.ID)
 }
 
-// DeployDepth used for subsystem deployment creates a depth item in the struct
-// then returns a ptr to that Depth item
+// DeployDepth used for subsystem deployment creates a depth item in the struct then returns a ptr to that Depth item
 func (s *Service) DeployDepth(exchange string, p currency.Pair, a asset.Item) (*Depth, error) {
 	if exchange == "" {
 		return nil, errExchangeNameUnset
@@ -112,13 +111,15 @@ func (s *Service) DeployDepth(exchange string, p currency.Pair, a asset.Item) (*
 		s.books[strings.ToLower(exchange)] = m1
 	}
 	book, ok := m1.m[mapKey]
-	if !ok {
-		book = NewDepth(m1.ID)
-		book.exchange = exchange
-		book.pair = p
-		book.asset = a
-		m1.m[mapKey] = book
+	if ok {
+		// Maybe in future we should return an error here and be more strict.
+		return book, nil
 	}
+	book = NewDepth(m1.ID)
+	book.exchange = exchange
+	book.pair = p
+	book.asset = a
+	m1.m[mapKey] = book
 	return book, nil
 }
 

exchanges/orderbook/orderbook_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
 	"github.com/thrasher-corp/gocryptotrader/currency"
 	"github.com/thrasher-corp/gocryptotrader/dispatch"
 	"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
@@ -321,28 +322,18 @@ func TestBaseGetDepth(t *testing.T) {
 
 func TestDeployDepth(t *testing.T) {
 	c, err := currency.NewPairFromStrings("BTC", "USD")
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 	_, err = DeployDepth("", c, asset.Spot)
-	if !errors.Is(err, errExchangeNameUnset) {
-		t.Fatalf("expecting %s error but received %v", errExchangeNameUnset, err)
-	}
+	require.ErrorIs(t, err, errExchangeNameUnset)
 	_, err = DeployDepth("test", currency.EMPTYPAIR, asset.Spot)
-	if !errors.Is(err, errPairNotSet) {
-		t.Fatalf("expecting %s error but received %v", errPairNotSet, err)
-	}
+	require.ErrorIs(t, err, errPairNotSet)
 	_, err = DeployDepth("test", c, asset.Empty)
-	if !errors.Is(err, errAssetTypeNotSet) {
-		t.Fatalf("expecting %s error but received %v", errAssetTypeNotSet, err)
-	}
+	require.ErrorIs(t, err, errAssetTypeNotSet)
 	d, err := DeployDepth("test", c, asset.Spot)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if d == nil {
-		t.Fatal("depth ptr shall not be nill")
-	}
+	require.NoError(t, err)
+	require.NotNil(t, d)
+	_, err = DeployDepth("test", c, asset.Spot)
+	require.NoError(t, err)
 }
 
 func TestCreateNewOrderbook(t *testing.T) {

exchanges/stream/buffer/buffer.go

@@ -170,20 +170,6 @@ func (w *Orderbook) Update(u *orderbook.Update) error {
 		}
 	}
 
-	var ret *orderbook.Base
-	if book.ob.VerifyOrderbook() {
-		// This is used here so as to not retrieve book if verification is off.
-		// On every update, this will retrieve and verify orderbook depth.
-		ret, err = book.ob.Retrieve()
-		if err != nil {
-			return err
-		}
-		err = ret.Verify()
-		if err != nil {
-			return book.ob.Invalidate(err)
-		}
-	}
-
 	// Publish all state changes, disregarding verbosity or sync requirements.
 	book.ob.Publish()
 
@@ -240,15 +226,17 @@ func (w *Orderbook) processBufferUpdate(o *orderbookHolder, u *orderbook.Update)
 	return true, nil
 }
 
-// processObUpdate processes updates either by its corresponding id or by
-// price level
+// processObUpdate processes updates either by its corresponding id or by price level
 func (w *Orderbook) processObUpdate(o *orderbookHolder, u *orderbook.Update) error {
+	// Both update methods require post processing to ensure the orderbook is in a valid state.
 	if w.updateEntriesByID {
-		return o.updateByIDAndAction(u)
-	}
-	err := o.updateByPrice(u)
-	if err != nil {
-		return err
+		if err := o.updateByIDAndAction(u); err != nil {
+			return err
+		}
+	} else {
+		if err := o.updateByPrice(u); err != nil {
+			return err
+		}
 	}
 	if w.checksum != nil {
 		compare, err := o.ob.Retrieve()
@@ -260,6 +248,15 @@ func (w *Orderbook) processObUpdate(o *orderbookHolder, u *orderbook.Update) err
 			return o.ob.Invalidate(err)
 		}
 		o.updateID = u.UpdateID
+	} else if o.ob.VerifyOrderbook() {
+		compare, err := o.ob.Retrieve()
+		if err != nil {
+			return err
+		}
+		err = compare.Verify()
+		if err != nil {
+			return o.ob.Invalidate(err)
+		}
 	}
 	return nil
 }
@@ -277,24 +274,24 @@ func (o *orderbookHolder) updateByIDAndAction(updts *orderbook.Update) error {
 	case orderbook.Amend:
 		err := o.ob.UpdateBidAskByID(updts)
 		if err != nil {
-			return fmt.Errorf("%v %w", errAmendFailure, err)
+			return fmt.Errorf("%w %w", errAmendFailure, err)
 		}
 	case orderbook.Delete:
 		// edge case for Bitfinex as their streaming endpoint duplicates deletes
 		bypassErr := o.ob.GetName() == "Bitfinex" && o.ob.IsFundingRate()
 		err := o.ob.DeleteBidAskByID(updts, bypassErr)
 		if err != nil {
-			return fmt.Errorf("%v %w", errDeleteFailure, err)
+			return fmt.Errorf("%w %w", errDeleteFailure, err)
 		}
 	case orderbook.Insert:
 		err := o.ob.InsertBidAskByID(updts)
 		if err != nil {
-			return fmt.Errorf("%v %w", errInsertFailure, err)
+			return fmt.Errorf("%w %w", errInsertFailure, err)
 		}
 	case orderbook.UpdateInsert:
 		err := o.ob.UpdateInsertByID(updts)
 		if err != nil {
-			return fmt.Errorf("%v %w", errUpdateInsertFailure, err)
+			return fmt.Errorf("%w %w", errUpdateInsertFailure, err)
 		}
 	default:
 		return fmt.Errorf("%w [%d]", errInvalidAction, updts.Action)
@@ -338,20 +335,6 @@ func (w *Orderbook) LoadSnapshot(book *orderbook.Base) error {
 		return err
 	}
 
-	if holder.ob.VerifyOrderbook() {
-		// This is used here so as to not retrieve book if verification is off.
-		// Checks to see if orderbook snapshot that was deployed has not been
-		// altered in any way
-		book, err = holder.ob.Retrieve()
-		if err != nil {
-			return err
-		}
-		err = book.Verify()
-		if err != nil {
-			return holder.ob.Invalidate(err)
-		}
-	}
-
 	holder.ob.Publish()
 	w.dataHandler <- holder.ob
 	return nil