Tests: Various race fixes and move TestFixtureToDataHandler (thrasher…

…-corp#1534)

* Tests: Move and simplify TestFixtureToDataHandler

* Currency: Fix PairsManager.Load breaking matcher

* Tests: Add multi-instance cache to UpdatePairsOnce

* Kraken: Fix TestUpdateTickers race error

Calling StorePairs on global instance can lead to race

* Bitfinex: Fix TestUpdateTickers racing intermittently

* Currency: Fix concurrent access to PM formats

* Currency: Fix SupportsAsset implementation

This should delegate entirely to PairManager's IsAssetSupported

* Okx: Fix PM intrusion, rm GetPairFromInstrumentID

* Exchange: Fix SetGlobalPairsManager to set asset enabled

* Bitflyer: Fix race on set TestGetCurrURL

TestGetCurrencyTradeURL would fail sometimes due to sequencing of
enabling futures but not having pairs for it.

* Tests: Simplify usage pattern for FixtureToDH

backtester/eventhandlers/exchange/slippage/slippage_test.go

@@ -5,6 +5,8 @@ import (
 	"testing"
 
 	"github.com/shopspring/decimal"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/thrasher-corp/gocryptotrader/currency"
 	"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
 	"github.com/thrasher-corp/gocryptotrader/exchanges/bitstamp"
@@ -14,31 +16,23 @@ import (
 func TestRandomSlippage(t *testing.T) {
 	t.Parallel()
 	resp := EstimateSlippagePercentage(decimal.NewFromInt(80), decimal.NewFromInt(100))
-	if resp.LessThan(decimal.NewFromFloat(0.8)) || resp.GreaterThan(decimal.NewFromInt(1)) {
-		t.Error("expected result > 0.8 and < 100")
-	}
+	assert.True(t, resp.GreaterThan(decimal.NewFromFloat(0.8)), "result should be more than 0.8")
+	assert.True(t, resp.LessThan(decimal.NewFromInt(1)), "result should be less than 1")
 }
 
 func TestCalculateSlippageByOrderbook(t *testing.T) {
 	t.Parallel()
 	b := bitstamp.Bitstamp{}
 	b.SetDefaults()
-	err := b.CurrencyPairs.SetAssetEnabled(asset.Spot, true)
-	if err != nil {
-		t.Fatal(err)
-	}
+
 	cp := currency.NewPair(currency.BTC, currency.USD)
 	ob, err := b.FetchOrderbook(context.Background(), cp, asset.Spot)
-	if err != nil {
-		t.Error(err)
-	}
+	require.NoError(t, err, "FetchOrderbook must not error")
+
 	amountOfFunds := decimal.NewFromInt(1000)
 	feeRate := decimal.NewFromFloat(0.03)
 	price, amount, err := CalculateSlippageByOrderbook(ob, gctorder.Buy, amountOfFunds, feeRate)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if price.Mul(amount).Add(price.Mul(amount).Mul(feeRate)).GreaterThan(amountOfFunds) {
-		t.Error("order size must be less than funds")
-	}
+	require.NoError(t, err, "CalculateSlippageByOrderbook must not error")
+	orderSize := price.Mul(amount).Add(price.Mul(amount).Mul(feeRate))
+	assert.True(t, orderSize.LessThan(amountOfFunds), "order size must be less than funds")
 }

currency/manager.go

@@ -4,39 +4,30 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"slices"
 	"strings"
 
 	"github.com/thrasher-corp/gocryptotrader/common"
 	"github.com/thrasher-corp/gocryptotrader/common/convert"
 	"github.com/thrasher-corp/gocryptotrader/exchanges/asset"
 )
 
+// Public errors
 var (
-	// ErrAssetAlreadyEnabled defines an error for the pairs management system
-	// that declares the asset is already enabled.
-	ErrAssetAlreadyEnabled = errors.New("asset already enabled")
-	// ErrPairAlreadyEnabled returns when enabling a pair that is already enabled
-	ErrPairAlreadyEnabled = errors.New("pair already enabled")
-	// ErrPairNotEnabled returns when looking for a pair that is not enabled
-	ErrPairNotEnabled = errors.New("pair not enabled")
-	// ErrPairNotFound is returned when a currency pair is not found
-	ErrPairNotFound = errors.New("pair not found")
-	// ErrAssetIsNil is an error when the asset has not been populated by the
-	// configuration
-	ErrAssetIsNil = errors.New("asset is nil")
-	// ErrPairNotContainedInAvailablePairs defines an error when a pair is not
-	// contained in the available pairs list and is not supported by the
-	// exchange for that asset type.
+	ErrAssetAlreadyEnabled              = errors.New("asset already enabled")
+	ErrAssetIsNil                       = errors.New("asset is nil")
+	ErrAssetNotFound                    = errors.New("asset type not found in pair store")
+	ErrPairAlreadyEnabled               = errors.New("pair already enabled")
+	ErrPairFormatIsNil                  = errors.New("pair format is nil")
+	ErrPairManagerNotInitialised        = errors.New("pair manager not initialised")
 	ErrPairNotContainedInAvailablePairs = errors.New("pair not contained in available pairs")
-	// ErrPairManagerNotInitialised is returned when a pairs manager is requested, but has not been setup
-	ErrPairManagerNotInitialised = errors.New("pair manager not initialised")
-	// ErrAssetNotFound is returned when an asset does not exist in the pairstore
-	ErrAssetNotFound = errors.New("asset type not found in pair store")
-	// ErrSymbolStringEmpty is an error when a symbol string is empty
-	ErrSymbolStringEmpty = errors.New("symbol string is empty")
+	ErrPairNotEnabled                   = errors.New("pair not enabled")
+	ErrPairNotFound                     = errors.New("pair not found")
+	ErrSymbolStringEmpty                = errors.New("symbol string is empty")
+)
 
+var (
 	errPairStoreIsNil      = errors.New("pair store is nil")
-	errPairFormatIsNil     = errors.New("pair format is nil")
 	errPairMatcherIsNil    = errors.New("pair matcher is nil")
 	errPairConfigFormatNil = errors.New("pair config format is nil")
 )
@@ -65,10 +56,9 @@ func (p *PairsManager) Get(a asset.Item) (*PairStore, error) {
 	defer p.mutex.RUnlock()
 	c, ok := p.Pairs[a]
 	if !ok {
-		return nil,
-			fmt.Errorf("cannot get pair store, %v %w", a, asset.ErrNotSupported)
+		return nil, fmt.Errorf("cannot get pair store, %v %w", a, asset.ErrNotSupported)
 	}
-	return c.copy()
+	return c.clone(), nil
 }
 
 // Match returns a currency pair based on the supplied symbol and asset type
@@ -94,24 +84,16 @@ func (p *PairsManager) Store(a asset.Item, ps *PairStore) error {
 	if !a.IsValid() {
 		return fmt.Errorf("%s %w", a, asset.ErrNotSupported)
 	}
-	cpy, err := ps.copy()
-	if err != nil {
-		return err
+	if ps == nil {
+		return errPairStoreIsNil
 	}
 	p.mutex.Lock()
+	defer p.mutex.Unlock()
 	if p.Pairs == nil {
-		p.Pairs = make(map[asset.Item]*PairStore)
+		p.Pairs = FullStore{}
 	}
-	p.Pairs[a] = cpy
-	if p.matcher == nil {
-		p.matcher = make(map[key]*Pair)
-	}
-	for x := range cpy.Available {
-		p.matcher[key{
-			Symbol: cpy.Available[x].Base.Lower().String() + cpy.Available[x].Quote.Lower().String(),
-			Asset:  a}] = &cpy.Available[x]
-	}
-	p.mutex.Unlock()
+	p.Pairs[a] = ps.clone()
+	p.reindex()
 	return nil
 }
 
@@ -145,9 +127,7 @@ func (p *PairsManager) GetPairs(a asset.Item, enabled bool) (Pairs, error) {
 	}
 
 	if !enabled {
-		availPairs := make(Pairs, len(pairStore.Available))
-		copy(availPairs, pairStore.Available)
-		return availPairs, nil
+		return slices.Clone(pairStore.Available), nil
 	}
 
 	lenCheck := len(pairStore.Enabled)
@@ -157,8 +137,7 @@ func (p *PairsManager) GetPairs(a asset.Item, enabled bool) (Pairs, error) {
 
 	// NOTE: enabledPairs is declared before the next check for comparison
 	// reasons within exchange update pairs functionality.
-	enabledPairs := make(Pairs, lenCheck)
-	copy(enabledPairs, pairStore.Enabled)
+	enabledPairs := slices.Clone(pairStore.Enabled)
 
 	err := pairStore.Available.ContainsAll(pairStore.Enabled, true)
 	if err != nil {
@@ -173,11 +152,9 @@ func (p *PairsManager) StoreFormat(a asset.Item, pFmt *PairFormat, config bool)
 		return fmt.Errorf("%s %w", a, asset.ErrNotSupported)
 	}
 	if pFmt == nil {
-		return errPairFormatIsNil
+		return ErrPairFormatIsNil
 	}
 
-	cpy := *pFmt
-
 	p.mutex.Lock()
 	defer p.mutex.Unlock()
 
@@ -192,25 +169,49 @@ func (p *PairsManager) StoreFormat(a asset.Item, pFmt *PairFormat, config bool)
 	}
 
 	if config {
-		pairStore.ConfigFormat = &cpy
+		pairStore.ConfigFormat = pFmt.clone()
 	} else {
-		pairStore.RequestFormat = &cpy
+		pairStore.RequestFormat = pFmt.clone()
 	}
 	return nil
 }
 
+// GetFormat returns the pair format in a concurrent safe manner
+func (p *PairsManager) GetFormat(a asset.Item, request bool) (PairFormat, error) {
+	p.mutex.RLock()
+	defer p.mutex.RUnlock()
+
+	var pFmt *PairFormat
+	if p.UseGlobalFormat {
+		if request {
+			pFmt = p.RequestFormat
+		} else {
+			pFmt = p.ConfigFormat
+		}
+	} else {
+		ps, err := p.Get(a)
+		if err != nil {
+			return EMPTYFORMAT, err
+		}
+		if request {
+			pFmt = ps.RequestFormat
+		} else {
+			pFmt = ps.ConfigFormat
+		}
+	}
+	if pFmt == nil {
+		return EMPTYFORMAT, ErrPairFormatIsNil
+	}
+	return *pFmt, nil
+}
+
 // StorePairs stores a list of pairs based on the asset type and whether
 // they're enabled or not
 func (p *PairsManager) StorePairs(a asset.Item, pairs Pairs, enabled bool) error {
 	if !a.IsValid() {
 		return fmt.Errorf("%s %w", a, asset.ErrNotSupported)
 	}
 
-	// NOTE: Length check not needed in this scenario as it has the ability to
-	// remove the entire stored list if needed.
-	cpy := make(Pairs, len(pairs))
-	copy(cpy, pairs)
-
 	p.mutex.Lock()
 	defer p.mutex.Unlock()
 
@@ -225,18 +226,10 @@ func (p *PairsManager) StorePairs(a asset.Item, pairs Pairs, enabled bool) error
 	}
 
 	if enabled {
-		pairStore.Enabled = cpy
+		pairStore.Enabled = slices.Clone(pairs)
 	} else {
-		pairStore.Available = cpy
-
-		if p.matcher == nil {
-			p.matcher = make(map[key]*Pair)
-		}
-		for x := range pairStore.Available {
-			p.matcher[key{
-				Symbol: pairStore.Available[x].Base.Lower().String() + pairStore.Available[x].Quote.Lower().String(),
-				Asset:  a}] = &pairStore.Available[x]
-		}
+		pairStore.Available = slices.Clone(pairs)
+		p.reindex()
 	}
 
 	return nil
@@ -405,6 +398,15 @@ func (p *PairsManager) IsAssetEnabled(a asset.Item) error {
 	return nil
 }
 
+// IsAssetSupported returns if the asset is supported by an exchange
+// Does not imply that the Asset is enabled
+func (p *PairsManager) IsAssetSupported(a asset.Item) bool {
+	p.mutex.RLock()
+	defer p.mutex.RUnlock()
+	_, ok := p.Pairs[a]
+	return ok
+}
+
 // SetAssetEnabled sets if an asset is enabled or disabled for first run
 func (p *PairsManager) SetAssetEnabled(a asset.Item, enabled bool) error {
 	if !a.IsValid() {
@@ -435,34 +437,31 @@ func (p *PairsManager) SetAssetEnabled(a asset.Item, enabled bool) error {
 }
 
 // Load sets the pair manager from a seed without copying mutexes
-func (p *PairsManager) Load(seed *PairsManager) error {
-	if seed == nil {
-		return fmt.Errorf("%w PairsManager", common.ErrNilPointer)
-	}
+func (p *PairsManager) Load(seed *PairsManager) {
 	p.mutex.Lock()
 	defer p.mutex.Unlock()
 	seed.mutex.RLock()
 	defer seed.mutex.RUnlock()
 
-	var pN PairsManager
-	j, err := json.Marshal(seed)
-	if err != nil {
-		return err
-	}
-	err = json.Unmarshal(j, &pN)
-	if err != nil {
-		return err
-	}
-	p.BypassConfigFormatUpgrades = pN.BypassConfigFormatUpgrades
-	if pN.UseGlobalFormat {
-		p.UseGlobalFormat = pN.UseGlobalFormat
-		p.RequestFormat = pN.RequestFormat
-		p.ConfigFormat = pN.ConfigFormat
-	}
-	p.LastUpdated = pN.LastUpdated
-	p.Pairs = pN.Pairs
+	p.BypassConfigFormatUpgrades = seed.BypassConfigFormatUpgrades
+	p.UseGlobalFormat = seed.UseGlobalFormat
+	p.LastUpdated = seed.LastUpdated
+	p.Pairs = seed.Pairs.clone()
+	p.RequestFormat = seed.RequestFormat.clone()
+	p.ConfigFormat = seed.ConfigFormat.clone()
+	p.reindex()
+}
 
-	return nil
+// reindex re-indexes the matcher for Available pairs and all assets
+// This method does not lock for concurrency
+func (p *PairsManager) reindex() {
+	p.matcher = make(map[key]*Pair)
+	for a, fs := range p.Pairs {
+		for i, pair := range fs.Available {
+			k := key{Symbol: pair.Base.Lower().String() + pair.Quote.Lower().String(), Asset: a}
+			p.matcher[k] = &fs.Available[i]
+		}
+	}
 }
 
 func (p *PairsManager) getPairStoreRequiresLock(a asset.Item) (*PairStore, error) {
@@ -539,39 +538,30 @@ func (fs FullStore) MarshalJSON() ([]byte, error) {
 	return json.Marshal(temp)
 }
 
-// copy copies and segregates pair store from internal and external calls.
-func (ps *PairStore) copy() (*PairStore, error) {
+// clone returns a deep clone of the PairStore
+func (ps *PairStore) clone() *PairStore {
 	if ps == nil {
-		return nil, errPairStoreIsNil
+		return nil
 	}
+
 	var assetEnabled *bool
 	if ps.AssetEnabled != nil {
 		assetEnabled = convert.BoolPtr(*ps.AssetEnabled)
 	}
 
-	enabled := make(Pairs, len(ps.Enabled))
-	copy(enabled, ps.Enabled)
-
-	avail := make(Pairs, len(ps.Available))
-	copy(avail, ps.Available)
-
-	var rFmt *PairFormat
-	if ps.RequestFormat != nil {
-		cpy := *ps.RequestFormat
-		rFmt = &cpy
+	return &PairStore{
+		AssetEnabled:  assetEnabled,
+		Enabled:       slices.Clone(ps.Enabled),
+		Available:     slices.Clone(ps.Available),
+		RequestFormat: ps.RequestFormat.clone(),
+		ConfigFormat:  ps.ConfigFormat.clone(),
 	}
+}
 
-	var cFmt *PairFormat
-	if ps.ConfigFormat != nil {
-		cpy := *ps.ConfigFormat
-		cFmt = &cpy
+func (fs FullStore) clone() FullStore {
+	c := FullStore{}
+	for a, pairStore := range fs {
+		c[a] = pairStore.clone()
 	}
-
-	return &PairStore{
-		AssetEnabled:  assetEnabled,
-		Enabled:       enabled,
-		Available:     avail,
-		RequestFormat: rFmt,
-		ConfigFormat:  cFmt,
-	}, nil
+	return c
 }