From c9bead8ff3f2551d348f8cb6d8059ea1738f2c05 Mon Sep 17 00:00:00 2001 From: Romano <6548898+romanornr@users.noreply.github.com> Date: Wed, 7 May 2025 04:26:51 +0200 Subject: [PATCH] exchanges/credentials: Improve handling and error reporting (#1886) * Improve credential handling and error reporting Swapped out empty credential returns with more specific errors to make things clearer and avoid weird behavior. Added safeguards to handle nil credentials in WebSocket data processing. This gives us better error handling and more reliable fallback options when managing credentials. * Rewrite gctwrapper test assertions to use substring matching instead of exact string matching This prevents test failures caused by minor error message formatting changes * Correct mutex use in GetCredentials Move mutex from local credentials copy to shares resource access We were locking the mutex when modifying a local copy instead of when reading from shared credentials Moved RLock/RUnlock to wrap b.API.credentials access remoived the unnecessary locks when changing the local copy Reduce lock contention and fix potential race conditions * Replace test assertions with testify/require package in gctwrapper_test.go * remove unnecesarry mutex locks in GetCredentials and fix test * credentials test use require.ErrorIs * Fix: improve credential handling in BTCMarkets websocket * Refactor error messages and improve test assertions * Update exchanges/credentials.go * Update gctscript/wrappers/gct/gctwrapper_test.go * credentials: Remove redundant comment for sub account override --------- Co-authored-by: Adrian Gallagher --- exchanges/btcmarkets/btcmarkets_websocket.go | 8 ++-- .../coinbasepro/coinbasepro_websocket.go | 7 +++- exchanges/credentials.go | 37 +++++++++---------- exchanges/credentials_test.go | 6 +-- gctscript/wrappers/gct/gctwrapper_test.go | 36 ++++++------------ 5 files changed, 43 insertions(+), 51 deletions(-) diff --git a/exchanges/btcmarkets/btcmarkets_websocket.go b/exchanges/btcmarkets/btcmarkets_websocket.go index 21eaeff9..8e9fd56d 100644 --- a/exchanges/btcmarkets/btcmarkets_websocket.go +++ b/exchanges/btcmarkets/btcmarkets_websocket.go @@ -320,13 +320,15 @@ func (b *BTCMarkets) wsHandleData(respRaw []byte) error { } } - creds, err := b.GetCredentials(context.TODO()) - if err != nil { + clientID := "" + if creds, err := b.GetCredentials(context.TODO()); err != nil { b.Websocket.DataHandler <- order.ClassificationError{ Exchange: b.Name, OrderID: orderID, Err: err, } + } else if creds != nil { + clientID = creds.ClientID } b.Websocket.DataHandler <- &order.Detail{ @@ -335,7 +337,7 @@ func (b *BTCMarkets) wsHandleData(respRaw []byte) error { RemainingAmount: orderData.OpenVolume, Exchange: b.Name, OrderID: orderID, - ClientID: creds.ClientID, + ClientID: clientID, Type: oType, Side: oSide, Status: oStatus, diff --git a/exchanges/coinbasepro/coinbasepro_websocket.go b/exchanges/coinbasepro/coinbasepro_websocket.go index 3c4258da..98438a60 100644 --- a/exchanges/coinbasepro/coinbasepro_websocket.go +++ b/exchanges/coinbasepro/coinbasepro_websocket.go @@ -173,6 +173,11 @@ func (c *CoinbasePro) wsHandleData(respRaw []byte) error { } } + clientID := "" + if creds != nil { + clientID = creds.ClientID + } + if wsOrder.UserID != "" { var p currency.Pair var a asset.Item @@ -191,7 +196,7 @@ func (c *CoinbasePro) wsHandleData(respRaw []byte) error { Exchange: c.Name, OrderID: wsOrder.OrderID, AccountID: wsOrder.ProfileID, - ClientID: creds.ClientID, + ClientID: clientID, Type: oType, Side: oSide, Status: oStatus, diff --git a/exchanges/credentials.go b/exchanges/credentials.go index 56c9d80d..03e19495 100644 --- a/exchanges/credentials.go +++ b/exchanges/credentials.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" + "github.com/thrasher-corp/gocryptotrader/common" "github.com/thrasher-corp/gocryptotrader/common/crypto" "github.com/thrasher-corp/gocryptotrader/config" "github.com/thrasher-corp/gocryptotrader/exchanges/account" @@ -22,12 +23,11 @@ var ( // undertake an authenticated HTTP request. ErrCredentialsAreEmpty = errors.New("credentials are empty") // Errors related to API requirements and failures - errRequiresAPIKey = errors.New("requires API key but default/empty one set") - errRequiresAPISecret = errors.New("requires API secret but default/empty one set") - errRequiresAPIPEMKey = errors.New("requires API PEM key but default/empty one set") - errRequiresAPIClientID = errors.New("requires API Client ID but default/empty one set") - errBase64DecodeFailure = errors.New("base64 decode has failed") - errContextCredentialsFailure = errors.New("context credentials type assertion failure") + errRequiresAPIKey = errors.New("requires API key but default/empty one set") + errRequiresAPISecret = errors.New("requires API secret but default/empty one set") + errRequiresAPIPEMKey = errors.New("requires API PEM key but default/empty one set") + errRequiresAPIClientID = errors.New("requires API Client ID but default/empty one set") + errBase64DecodeFailure = errors.New("base64 decode has failed") ) // SetKey sets new key for the default credentials @@ -115,31 +115,28 @@ func (b *Base) GetCredentials(ctx context.Context) (*account.Credentials, error) if value != nil { ctxCredStore, ok := value.(*account.ContextCredentialsStore) if !ok { - // NOTE: Return empty credentials on error to limit panic on - // websocket handling. - return &account.Credentials{}, errContextCredentialsFailure + return nil, common.GetTypeAssertError("*account.ContextCredentialsStore", value) } creds := ctxCredStore.Get() if err := b.CheckCredentials(creds, true); err != nil { - return creds, fmt.Errorf("context credentials issue: %w", err) + return nil, fmt.Errorf("error checking credentials from context: %w", err) } return creds, nil } - creds := b.API.credentials - err := b.CheckCredentials(&creds, false) - if err != nil { - // NOTE: Return empty credentials on error to limit panic on websocket - // handling. - return &account.Credentials{}, err - } - subAccountOverride, ok := ctx.Value(account.ContextSubAccountFlag).(string) + // Fallback to exchange loaded credentials b.API.credMu.RLock() - defer b.API.credMu.RUnlock() - if ok { + creds := b.API.credentials + b.API.credMu.RUnlock() + if err := b.CheckCredentials(&creds, false); err != nil { + return nil, fmt.Errorf("error checking credentials: %w", err) + } + + if subAccountOverride, ok := ctx.Value(account.ContextSubAccountFlag).(string); ok { creds.SubAccount = subAccountOverride } + return &creds, nil } diff --git a/exchanges/credentials_test.go b/exchanges/credentials_test.go index 5da6deed..e85bbb49 100644 --- a/exchanges/credentials_test.go +++ b/exchanges/credentials_test.go @@ -5,6 +5,8 @@ import ( "errors" "testing" + "github.com/stretchr/testify/require" + "github.com/thrasher-corp/gocryptotrader/common" "github.com/thrasher-corp/gocryptotrader/config" "github.com/thrasher-corp/gocryptotrader/exchanges/account" ) @@ -55,9 +57,7 @@ func TestGetCredentials(t *testing.T) { ctx = context.WithValue(t.Context(), account.ContextCredentialsFlag, "pewpew") _, err = b.GetCredentials(ctx) - if !errors.Is(err, errContextCredentialsFailure) { - t.Fatalf("received: %v but expected: %v", err, errContextCredentialsFailure) - } + require.ErrorIs(t, err, common.ErrTypeAssertFailure) b.API.CredentialsValidator.RequiresBase64DecodeSecret = false fullCred := &account.Credentials{ diff --git a/gctscript/wrappers/gct/gctwrapper_test.go b/gctscript/wrappers/gct/gctwrapper_test.go index 60f98978..5cc38b63 100644 --- a/gctscript/wrappers/gct/gctwrapper_test.go +++ b/gctscript/wrappers/gct/gctwrapper_test.go @@ -10,6 +10,7 @@ import ( "testing" objects "github.com/d5/tengo/v2" + "github.com/stretchr/testify/require" "github.com/thrasher-corp/gocryptotrader/common" "github.com/thrasher-corp/gocryptotrader/config" "github.com/thrasher-corp/gocryptotrader/engine" @@ -183,21 +184,15 @@ func TestExchangePairs(t *testing.T) { } } -func TestAccountInfo(t *testing.T) { +func TestExchangeAccountInfo(t *testing.T) { t.Parallel() _, err := gct.ExchangeAccountInfo() - if !errors.Is(err, objects.ErrWrongNumArguments) { - t.Fatal(err) - } + require.ErrorIs(t, err, objects.ErrWrongNumArguments) obj, err := gct.ExchangeAccountInfo(ctx, exch, assetType) - if err != nil { - t.Fatalf("received: %v but expected: %v", err, nil) - } - rString, _ := objects.ToString(obj) - if rString != `error: "Bitstamp REST or Websocket authentication support is not enabled"` { - t.Errorf("received: %v but expected: %v", - rString, `error: "Bitstamp REST or Websocket authentication support is not enabled"`) - } + require.NoError(t, err) + rString, ok := objects.ToString(obj) + require.True(t, ok, "ExchangeAccountInfo return value must return correctly from objects.ToString") + require.Contains(t, rString, "Bitstamp REST or Websocket authentication support is not enabled") } func TestExchangeOrderQuery(t *testing.T) { @@ -229,9 +224,7 @@ func TestExchangeOrderCancel(t *testing.T) { func TestExchangeOrderSubmit(t *testing.T) { t.Parallel() _, err := gct.ExchangeOrderSubmit() - if !errors.Is(err, objects.ErrWrongNumArguments) { - t.Fatal(err) - } + require.ErrorIs(t, err, objects.ErrWrongNumArguments) orderSide := &objects.String{Value: "ASK"} orderType := &objects.String{Value: "LIMIT"} @@ -249,16 +242,11 @@ func TestExchangeOrderSubmit(t *testing.T) { orderAmount, orderID, orderAsset) - if err != nil { - t.Fatalf("received: %v but expected: %v", err, nil) - } + require.NoError(t, err) - rString, _ := objects.ToString(obj) - if rString != `error: "Bitstamp REST or Websocket authentication support is not enabled"` { - t.Errorf("received: [%v] but expected: %v", - rString, - `error: "Bitstamp REST or Websocket authentication support is not enabled"`) - } + rString, ok := objects.ToString(obj) + require.True(t, ok, "ExchangeOrderSubmit return value must return correctly from objects.ToString") + require.Contains(t, rString, "Bitstamp REST or Websocket authentication support is not enabled") } func TestAllModuleNames(t *testing.T) {