From 9f659e71e346de23906f17e274b68284e7139e1b Mon Sep 17 00:00:00 2001 From: Adrian Gallagher Date: Fri, 23 May 2025 17:30:11 +1000 Subject: [PATCH] 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 | 26 ++-- exchanges/exchange_test.go | 291 +++++++++++-------------------------- 2 files changed, 96 insertions(+), 221 deletions(-) diff --git a/exchanges/exchange.go b/exchanges/exchange.go index e73e6bba..e589bc6c 100644 --- a/exchanges/exchange.go +++ b/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 } diff --git a/exchanges/exchange_test.go b/exchanges/exchange_test.go index 3b4c35c3..7ad67d0b 100644 --- a/exchanges/exchange_test.go +++ b/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, ¤cy.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, ¤cy.PairStore{ AssetEnabled: true, - Enabled: []currency.Pair{pair}, - Available: []currency.Pair{pair}, + Enabled: []currency.Pair{p}, + Available: []currency.Pair{p}, RequestFormat: ¤cy.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, ¤cy.PairStore{ AssetEnabled: true, - Enabled: []currency.Pair{pair}, - Available: []currency.Pair{pair}, + Enabled: []currency.Pair{p}, + Available: []currency.Pair{p}, RequestFormat: ¤cy.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, ¤cy.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, ¤cy.PairStore{ AssetEnabled: true, - Enabled: []currency.Pair{pair}, - Available: []currency.Pair{pair}, + Enabled: []currency.Pair{p}, + Available: []currency.Pair{p}, RequestFormat: ¤cy.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) {