exchanges: Fix race, improve error handling in ValidateKline and assertify related tests (#1919)

* exchanges: Fix race, improve error handling in ValidateKline and assertify related tests

* exchanges: Refactor ValidateKline error handling and enhance related tests

* refactor: Fix spaghetti bolognese if else error logic

* exchanges: Simplify error assertion in TestGetKlineRequest

exchanges/exchange.go

@@ -1140,21 +1140,23 @@ func (b *Base) FormatExchangeKlineInterval(in kline.Interval) string {
 	return strconv.FormatFloat(in.Duration().Seconds(), 'f', 0, 64)
 }
 
-// ValidateKline confirms that the requested pair, asset & interval are
-// supported and/or enabled by the requested exchange.
-func (b *Base) ValidateKline(pair currency.Pair, a asset.Item, interval kline.Interval) error {
-	var err error
-	if b.CurrencyPairs.IsAssetEnabled(a) != nil {
-		err = common.AppendError(err, fmt.Errorf("%w %v", asset.ErrNotEnabled, a))
-	} else if !b.CurrencyPairs.Pairs[a].Enabled.Contains(pair, true) {
-		err = common.AppendError(err, fmt.Errorf("%w in enabled pairs %v", currency.ErrPairNotFound, pair))
+// verifyKlineParameters verifies whether the pair, asset and interval are enabled on the exchange
+func (b *Base) verifyKlineParameters(pair currency.Pair, a asset.Item, interval kline.Interval) error {
+	if err := b.CurrencyPairs.IsAssetEnabled(a); err != nil {
+		return err
+	}
+
+	if ok, err := b.IsPairEnabled(pair, a); err != nil {
+		return err
+	} else if !ok {
+		return fmt.Errorf("%w: %v", currency.ErrPairNotEnabled, pair)
 	}
 
 	if !b.klineIntervalEnabled(interval) {
-		err = common.AppendError(err, fmt.Errorf("%w %v", kline.ErrInvalidInterval, interval))
+		return fmt.Errorf("%w: %v", kline.ErrInvalidInterval, interval)
 	}
 
-	return err
+	return nil
 }
 
 // AddTradesToBuffer is a helper function that will only
@@ -1495,7 +1497,7 @@ func (b *Base) GetKlineRequest(pair currency.Pair, a asset.Item, interval kline.
 		return nil, err
 	}
 
-	err = b.ValidateKline(pair, a, exchangeInterval)
+	err = b.verifyKlineParameters(pair, a, exchangeInterval)
 	if err != nil {
 		return nil, err
 	}
@@ -1566,7 +1568,7 @@ func (b *Base) GetKlineExtendedRequest(pair currency.Pair, a asset.Item, interva
 		return nil, err
 	}
 
-	err = b.ValidateKline(pair, a, exchangeInterval)
+	err = b.verifyKlineParameters(pair, a, exchangeInterval)
 	if err != nil {
 		return nil, err
 	}

exchanges/exchange_test.go

@@ -1499,7 +1499,7 @@ func Test_FormatExchangeKlineInterval(t *testing.T) {
 	}
 }
 
-func TestBase_ValidateKline(t *testing.T) {
+func TestVerifyKlineParameters(t *testing.T) {
 	pairs := currency.Pairs{
 		currency.Pair{Base: currency.BTC, Quote: currency.USDT},
 	}
@@ -1529,20 +1529,11 @@ func TestBase_ValidateKline(t *testing.T) {
 		},
 	}
 
-	err := b.ValidateKline(availablePairs[0], asset.Spot, kline.OneMin)
-	if err != nil {
-		t.Fatalf("expected validation to pass received error: %v", err)
-	}
-
-	err = b.ValidateKline(availablePairs[1], asset.Spot, kline.OneYear)
-	if err == nil {
-		t.Fatal("expected validation to fail")
-	}
-
-	err = b.ValidateKline(availablePairs[1], asset.Index, kline.OneYear)
-	if err == nil {
-		t.Fatal("expected validation to fail")
-	}
+	assert.ErrorIs(t, b.verifyKlineParameters(availablePairs[0], asset.Index, kline.OneYear), currency.ErrAssetNotFound)
+	assert.ErrorIs(t, b.verifyKlineParameters(currency.EMPTYPAIR, asset.Spot, kline.OneMin), currency.ErrCurrencyPairEmpty)
+	assert.ErrorIs(t, b.verifyKlineParameters(availablePairs[1], asset.Spot, kline.OneYear), currency.ErrPairNotEnabled)
+	assert.ErrorIs(t, b.verifyKlineParameters(availablePairs[0], asset.Spot, kline.OneYear), kline.ErrInvalidInterval)
+	assert.NoError(t, b.verifyKlineParameters(availablePairs[0], asset.Spot, kline.OneMin), "verifyKlineParameters should not error")
 }
 
 func TestCheckTransientError(t *testing.T) {
@@ -2242,266 +2233,148 @@ func TestHasAssetTypeAccountSegregation(t *testing.T) {
 func TestGetKlineRequest(t *testing.T) {
 	t.Parallel()
 	b := Base{Name: "klineTest"}
-
 	_, err := b.GetKlineRequest(currency.EMPTYPAIR, asset.Empty, 0, time.Time{}, time.Time{}, false)
-	if !errors.Is(err, currency.ErrCurrencyPairEmpty) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, currency.ErrCurrencyPairEmpty)
-	}
+	assert.ErrorIs(t, err, currency.ErrCurrencyPairEmpty)
 
-	pair := currency.NewBTCUSDT()
-	_, err = b.GetKlineRequest(pair, asset.Empty, 0, time.Time{}, time.Time{}, false)
-	if !errors.Is(err, asset.ErrNotSupported) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, asset.ErrNotSupported)
-	}
+	p := currency.NewBTCUSDT()
+	_, err = b.GetKlineRequest(p, asset.Empty, 0, time.Time{}, time.Time{}, false)
+	assert.ErrorIs(t, err, asset.ErrNotSupported)
 
-	_, err = b.GetKlineRequest(pair, asset.Spot, 0, time.Time{}, time.Time{}, false)
-	if !errors.Is(err, kline.ErrInvalidInterval) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrInvalidInterval)
-	}
+	_, err = b.GetKlineRequest(p, asset.Spot, 0, time.Time{}, time.Time{}, false)
+	assert.ErrorIs(t, err, kline.ErrInvalidInterval)
 
 	b.Features.Enabled.Kline.Intervals = kline.DeployExchangeIntervals(kline.IntervalCapacity{Interval: kline.OneDay, Capacity: 1439})
 	err = b.CurrencyPairs.Store(asset.Spot, &currency.PairStore{
 		AssetEnabled: true,
-		Enabled:      []currency.Pair{pair},
-		Available:    []currency.Pair{pair},
+		Enabled:      []currency.Pair{p},
+		Available:    []currency.Pair{p},
 	})
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
+	require.NoError(t, err, "CurrencyPairs.Store must not error")
 
-	_, err = b.GetKlineRequest(pair, asset.Spot, 0, time.Time{}, time.Time{}, false)
-	if !errors.Is(err, kline.ErrInvalidInterval) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrInvalidInterval)
-	}
+	_, err = b.GetKlineRequest(p, asset.Spot, 0, time.Time{}, time.Time{}, false)
+	assert.ErrorIs(t, err, kline.ErrInvalidInterval)
 
-	_, err = b.GetKlineRequest(pair, asset.Spot, kline.OneMin, time.Time{}, time.Time{}, false)
-	if !errors.Is(err, kline.ErrCannotConstructInterval) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrCannotConstructInterval)
-	}
+	_, err = b.GetKlineRequest(p, asset.Spot, kline.OneMin, time.Time{}, time.Time{}, false)
+	assert.ErrorIs(t, err, kline.ErrCannotConstructInterval)
 
 	b.Features.Enabled.Kline.Intervals = kline.DeployExchangeIntervals(kline.IntervalCapacity{Interval: kline.OneMin})
 	b.Features.Enabled.Kline.GlobalResultLimit = 1439
-	_, err = b.GetKlineRequest(pair, asset.Spot, kline.OneHour, time.Time{}, time.Time{}, false)
-	assert.ErrorIs(t, err, currency.ErrPairFormatIsNil, "GetKlineRequest should return Format is Nil")
+	_, err = b.GetKlineRequest(p, asset.Spot, kline.OneHour, time.Time{}, time.Time{}, false)
+	assert.ErrorIs(t, err, currency.ErrPairFormatIsNil)
 
 	err = b.CurrencyPairs.Store(asset.Spot, &currency.PairStore{
 		AssetEnabled:  true,
-		Enabled:       []currency.Pair{pair},
-		Available:     []currency.Pair{pair},
+		Enabled:       []currency.Pair{p},
+		Available:     []currency.Pair{p},
 		RequestFormat: &currency.PairFormat{Uppercase: true},
 	})
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
+	require.NoError(t, err, "CurrencyPairs.Store must not error")
 
 	start := time.Date(2020, 12, 1, 0, 0, 0, 0, time.UTC)
 	end := start.AddDate(0, 0, 1)
-	_, err = b.GetKlineRequest(pair, asset.Spot, kline.OneMin, start, end, true)
-	if !errors.Is(err, kline.ErrRequestExceedsExchangeLimits) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrRequestExceedsExchangeLimits)
-	}
+	_, err = b.GetKlineRequest(p, asset.Spot, kline.OneMin, start, end, true)
+	assert.ErrorIs(t, err, kline.ErrRequestExceedsExchangeLimits)
 
-	_, err = b.GetKlineRequest(pair, asset.Spot, kline.OneMin, start, end, false)
-	if !errors.Is(err, kline.ErrRequestExceedsExchangeLimits) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrRequestExceedsExchangeLimits)
-	}
+	_, err = b.GetKlineRequest(p, asset.Spot, kline.OneMin, start, end, false)
+	assert.ErrorIs(t, err, kline.ErrRequestExceedsExchangeLimits)
 
-	_, err = b.GetKlineRequest(pair, asset.Futures, kline.OneHour, start, end, false)
-	if !errors.Is(err, asset.ErrNotEnabled) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, asset.ErrNotEnabled)
-	}
+	_, err = b.GetKlineRequest(p, asset.Futures, kline.OneHour, start, end, false)
+	assert.ErrorIs(t, err, currency.ErrAssetNotFound)
 
 	err = b.CurrencyPairs.Store(asset.Futures, &currency.PairStore{
 		AssetEnabled:  true,
-		Enabled:       []currency.Pair{pair},
-		Available:     []currency.Pair{pair},
+		Enabled:       []currency.Pair{p},
+		Available:     []currency.Pair{p},
 		RequestFormat: &currency.PairFormat{Uppercase: true},
 	})
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
-	_, err = b.GetKlineRequest(pair, asset.Futures, kline.OneHour, start, end, false)
-	if !errors.Is(err, kline.ErrRequestExceedsExchangeLimits) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrRequestExceedsExchangeLimits)
-	}
+	require.NoError(t, err, "CurrencyPairs.Store must not error")
+
+	_, err = b.GetKlineRequest(p, asset.Futures, kline.OneHour, start, end, false)
+	assert.ErrorIs(t, err, kline.ErrRequestExceedsExchangeLimits)
 
 	b.Features.Enabled.Kline.Intervals = kline.DeployExchangeIntervals(kline.IntervalCapacity{Interval: kline.OneHour})
-	r, err := b.GetKlineRequest(pair, asset.Spot, kline.OneHour, start, end, false)
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
+	r, err := b.GetKlineRequest(p, asset.Spot, kline.OneHour, start, end, false)
+	require.NoError(t, err, "GetKlineRequest must not error")
 
-	if r.Exchange != "klineTest" {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Exchange, "klineTest")
-	}
-
-	if !r.Pair.Equal(pair) {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Pair, pair)
-	}
-
-	if r.Asset != asset.Spot {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Asset, asset.Spot)
-	}
-
-	if r.ExchangeInterval != kline.OneHour {
-		t.Fatalf("received: '%v' but expected: '%v'", r.ExchangeInterval, kline.OneHour)
-	}
-
-	if r.ClientRequired != kline.OneHour {
-		t.Fatalf("received: '%v' but expected: '%v'", r.ClientRequired, kline.OneHour)
-	}
-
-	if r.Start != start {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Start, start)
-	}
-
-	if r.End != end {
-		t.Fatalf("received: '%v' but expected: '%v'", r.End, end)
-	}
-
-	if r.RequestFormatted.String() != "BTCUSDT" {
-		t.Fatalf("received: '%v' but expected: '%v'", r.RequestFormatted.String(), "BTCUSDT")
+	exp := &kline.Request{
+		Exchange:         b.Name,
+		Pair:             p,
+		Asset:            asset.Spot,
+		ExchangeInterval: kline.OneHour,
+		ClientRequired:   kline.OneHour,
+		Start:            start,
+		End:              end,
+		RequestFormatted: p,
+		RequestLimit:     1439,
 	}
+	assert.Equal(t, exp, r, "GetKlineRequest should return the expected request result")
 
 	end = time.Now().Truncate(kline.OneHour.Duration()).UTC()
 	start = end.Add(-kline.OneHour.Duration() * 1439)
+	r, err = b.GetKlineRequest(p, asset.Spot, kline.OneHour, start, end, true)
+	require.NoError(t, err, "GetKlineRequest must not error")
 
-	r, err = b.GetKlineRequest(pair, asset.Spot, kline.OneHour, start, end, true)
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
-
-	if r.Exchange != "klineTest" {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Exchange, "klineTest")
-	}
-
-	if !r.Pair.Equal(pair) {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Pair, pair)
-	}
-
-	if r.Asset != asset.Spot {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Asset, asset.Spot)
-	}
-
-	if r.ExchangeInterval != kline.OneHour {
-		t.Fatalf("received: '%v' but expected: '%v'", r.ExchangeInterval, kline.OneHour)
-	}
-
-	if r.ClientRequired != kline.OneHour {
-		t.Fatalf("received: '%v' but expected: '%v'", r.ClientRequired, kline.OneHour)
-	}
-
-	if r.Start != start {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Start, start)
-	}
-
-	if r.End != end {
-		t.Fatalf("received: '%v' but expected: '%v'", r.End, end)
-	}
-
-	if r.RequestFormatted.String() != "BTCUSDT" {
-		t.Fatalf("received: '%v' but expected: '%v'", r.RequestFormatted.String(), "BTCUSDT")
-	}
+	exp.Start = start
+	exp.End = end
+	assert.Equal(t, exp, r, "GetKlineRequest should return the expected request result")
 }
 
 func TestGetKlineExtendedRequest(t *testing.T) {
 	t.Parallel()
 	b := Base{Name: "klineTest"}
 	_, err := b.GetKlineExtendedRequest(currency.EMPTYPAIR, asset.Empty, 0, time.Time{}, time.Time{})
-	if !errors.Is(err, currency.ErrCurrencyPairEmpty) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, currency.ErrCurrencyPairEmpty)
-	}
+	assert.ErrorIs(t, err, currency.ErrCurrencyPairEmpty)
 
-	pair := currency.NewBTCUSDT()
-	_, err = b.GetKlineExtendedRequest(pair, asset.Empty, 0, time.Time{}, time.Time{})
-	if !errors.Is(err, asset.ErrNotSupported) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, asset.ErrNotSupported)
-	}
+	p := currency.NewBTCUSDT()
+	_, err = b.GetKlineExtendedRequest(p, asset.Empty, 0, time.Time{}, time.Time{})
+	assert.ErrorIs(t, err, asset.ErrNotSupported)
 
-	_, err = b.GetKlineExtendedRequest(pair, asset.Spot, 0, time.Time{}, time.Time{})
-	if !errors.Is(err, kline.ErrInvalidInterval) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrInvalidInterval)
-	}
+	_, err = b.GetKlineExtendedRequest(p, asset.Spot, 0, time.Time{}, time.Time{})
+	assert.ErrorIs(t, err, kline.ErrInvalidInterval)
 
-	_, err = b.GetKlineExtendedRequest(pair, asset.Spot, kline.OneHour, time.Time{}, time.Time{})
-	if !errors.Is(err, kline.ErrCannotConstructInterval) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, kline.ErrCannotConstructInterval)
-	}
+	_, err = b.GetKlineExtendedRequest(p, asset.Spot, kline.OneHour, time.Time{}, time.Time{})
+	assert.ErrorIs(t, err, kline.ErrCannotConstructInterval)
 
 	b.Features.Enabled.Kline.Intervals = kline.DeployExchangeIntervals(kline.IntervalCapacity{Interval: kline.OneMin})
 	b.Features.Enabled.Kline.GlobalResultLimit = 100
 	start := time.Date(2020, 12, 1, 0, 0, 0, 0, time.UTC)
 	end := start.AddDate(0, 0, 1)
-	_, err = b.GetKlineExtendedRequest(pair, asset.Spot, kline.OneHour, start, end)
-	if !errors.Is(err, asset.ErrNotEnabled) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, asset.ErrNotEnabled)
-	}
+	_, err = b.GetKlineExtendedRequest(p, asset.Spot, kline.OneHour, start, end)
+	assert.ErrorIs(t, err, currency.ErrPairManagerNotInitialised)
 
 	err = b.CurrencyPairs.Store(asset.Spot, &currency.PairStore{
 		AssetEnabled: true,
-		Enabled:      []currency.Pair{pair},
-		Available:    []currency.Pair{pair},
+		Enabled:      []currency.Pair{p},
+		Available:    []currency.Pair{p},
 	})
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
+	require.NoError(t, err, "CurrencyPairs.Store must not error")
 
-	_, err = b.GetKlineExtendedRequest(pair, asset.Spot, kline.OneHour, start, end)
+	_, err = b.GetKlineExtendedRequest(p, asset.Spot, kline.OneHour, start, end)
 	assert.ErrorIs(t, err, currency.ErrPairFormatIsNil, "GetKlineExtendedRequest should error correctly")
 
 	err = b.CurrencyPairs.Store(asset.Spot, &currency.PairStore{
 		AssetEnabled:  true,
-		Enabled:       []currency.Pair{pair},
-		Available:     []currency.Pair{pair},
+		Enabled:       []currency.Pair{p},
+		Available:     []currency.Pair{p},
 		RequestFormat: &currency.PairFormat{Uppercase: true},
 	})
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
+	require.NoError(t, err, "CurrencyPairs.Store must not error")
 
 	// The one hour interval is not supported by the exchange. This scenario
 	// demonstrates the conversion from the supported 1 minute candles into
 	// one hour candles
-	r, err := b.GetKlineExtendedRequest(pair, asset.Spot, kline.OneHour, start, end)
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
-	}
+	r, err := b.GetKlineExtendedRequest(p, asset.Spot, kline.OneHour, start, end)
+	require.NoError(t, err, "GetKlineExtendedRequest must not error")
 
-	if r.Exchange != "klineTest" {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Exchange, "klineTest")
-	}
-
-	if !r.Pair.Equal(pair) {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Pair, pair)
-	}
-
-	if r.Asset != asset.Spot {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Asset, asset.Spot)
-	}
-
-	if r.ExchangeInterval != kline.OneMin {
-		t.Fatalf("received: '%v' but expected: '%v'", r.ExchangeInterval, kline.OneMin)
-	}
-
-	if r.ClientRequired != kline.OneHour {
-		t.Fatalf("received: '%v' but expected: '%v'", r.ClientRequired, kline.OneHour)
-	}
-
-	if r.Request.Start != start {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Request.Start, start)
-	}
-
-	if r.Request.End != end {
-		t.Fatalf("received: '%v' but expected: '%v'", r.Request.End, end)
-	}
-
-	if r.RequestFormatted.String() != "BTCUSDT" {
-		t.Fatalf("received: '%v' but expected: '%v'", r.RequestFormatted.String(), "BTCUSDT")
-	}
-
-	if len(r.RangeHolder.Ranges) != 15 { // 15 request at max 100 candles == 1440 1 min candles.
-		t.Fatalf("received: '%v' but expected: '%v'", len(r.RangeHolder.Ranges), 15)
-	}
+	assert.Equal(t, "klineTest", r.Exchange, "Exchange name should match")
+	assert.Equal(t, p, r.Pair, "Pair should match")
+	assert.Equal(t, asset.Spot, r.Asset, "Asset should match")
+	assert.Equal(t, kline.OneMin, r.ExchangeInterval, "ExchangeInterval should match")
+	assert.Equal(t, kline.OneHour, r.ClientRequired, "ClientRequired should match")
+	assert.Equal(t, start, r.Request.Start, "Request.Start should match")
+	assert.Equal(t, end, r.Request.End, "Request.End should match")
+	assert.Equal(t, "BTCUSDT", r.RequestFormatted.String(), "RequestFormatted should match")
+	assert.Equal(t, 15, len(r.RangeHolder.Ranges), "RangeHolder.Ranges length should match")
 }
 
 func TestSetCollateralMode(t *testing.T) {