From bb304ae48643c68e02af801579f6b4d2d62625d7 Mon Sep 17 00:00:00 2001 From: Andrey Bocharnikov Date: Mon, 16 Sep 2024 14:49:25 +0400 Subject: [PATCH] fix(wallet)_: address PR feedback fixes #21071 --- node/status_node_services.go | 9 - rpc/chain/client.go | 27 +-- services/network_utils/network_errors.go | 92 ++++++++++ services/network_utils/network_errors_test.go | 157 ++++++++++++++++++ services/wallet/api_test.go | 2 +- .../wallet/common/mock/online_checker_mock.go | 13 -- .../wallet/common/online_checker_interface.go | 5 - services/wallet/keycard_pairings_test.go | 2 +- services/wallet/market/market.go | 27 ++- services/wallet/market/market_feed_test.go | 47 ++---- services/wallet/market/market_test.go | 7 +- .../wallet/market/mock/mock_price_provider.go | 13 +- services/wallet/service.go | 7 +- 13 files changed, 312 insertions(+), 96 deletions(-) create mode 100644 services/network_utils/network_errors.go create mode 100644 services/network_utils/network_errors_test.go delete mode 100644 services/wallet/common/mock/online_checker_mock.go delete mode 100644 services/wallet/common/online_checker_interface.go diff --git a/node/status_node_services.go b/node/status_node_services.go index b2cdc78872..d45ab1fde8 100644 --- a/node/status_node_services.go +++ b/node/status_node_services.go @@ -584,14 +584,6 @@ func (b *StatusNode) SetWalletCommunityInfoProvider(provider thirdparty.Communit } } -type PeerOnlineChecker struct { - StatusNode *StatusNode -} - -func (f *PeerOnlineChecker) Online() bool { - return f.StatusNode.PeerCount() > 0 -} - func (b *StatusNode) walletService(accountsDB *accounts.Database, appDB *sql.DB, accountsFeed *event.Feed, settingsFeed *event.Feed, walletFeed *event.Feed, statusProxyStageName string) *wallet.Service { if b.walletSrvc == nil { b.walletSrvc = wallet.NewService( @@ -602,7 +594,6 @@ func (b *StatusNode) walletService(accountsDB *accounts.Database, appDB *sql.DB, walletFeed, b.httpServer, statusProxyStageName, - &PeerOnlineChecker{StatusNode: b}, ) } return b.walletSrvc diff --git a/rpc/chain/client.go b/rpc/chain/client.go index da8c24a3e3..82a042c4a4 100644 --- a/rpc/chain/client.go +++ b/rpc/chain/client.go @@ -19,6 +19,7 @@ import ( "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/rpc" "github.com/status-im/status-go/circuitbreaker" + "github.com/status-im/status-go/services/network_utils" "github.com/status-im/status-go/services/rpcstats" "github.com/status-im/status-go/services/wallet/connection" ) @@ -225,17 +226,11 @@ func isVMError(err error) bool { return false } -func isRPSLimitError(err error) bool { - return strings.Contains(err.Error(), "backoff_seconds") || - strings.Contains(err.Error(), "has exceeded its throughput limit") || - strings.Contains(err.Error(), "request rate exceeded") -} - -func (c *ClientWithFallback) SetIsConnected(value bool) { +func (c *ClientWithFallback) setIsConnected(value bool, skipNotification bool) { c.LastCheckedAt = time.Now().Unix() if !value { if c.isConnected.Load() { - if c.WalletNotifier != nil { + if c.WalletNotifier != nil && !skipNotification { c.WalletNotifier(c.ChainID, "down") } c.isConnected.Store(false) @@ -244,13 +239,17 @@ func (c *ClientWithFallback) SetIsConnected(value bool) { } else { if !c.isConnected.Load() { c.isConnected.Store(true) - if c.WalletNotifier != nil { + if c.WalletNotifier != nil && !skipNotification { c.WalletNotifier(c.ChainID, "up") } } } } +func (c *ClientWithFallback) SetIsConnected(value bool) { + c.setIsConnected(value, false) +} + func (c *ClientWithFallback) IsConnected() bool { return c.isConnected.Load() } @@ -279,7 +278,7 @@ func (c *ClientWithFallback) makeCall(ctx context.Context, ethClients []*EthClie res, err := f(provider) if err != nil { - if isRPSLimitError(err) { + if network_utils.IsRPSLimitError(err) { provider.limiter.ReduceLimit() err = provider.limiter.WaitForRequestsAvailability(1) @@ -977,15 +976,19 @@ func (c *ClientWithFallback) SetWalletNotifier(notifier func(chainId uint64, mes func (c *ClientWithFallback) toggleConnectionState(err error) { connected := true + skipNotification := false if err != nil { - if !isNotFoundError(err) && !isVMError(err) && !errors.Is(err, ErrRequestsOverLimit) && !errors.Is(err, context.Canceled) { + if network_utils.IsConnectionError(err) { + connected = false + skipNotification = true + } else if !isNotFoundError(err) && !isVMError(err) && !errors.Is(err, ErrRequestsOverLimit) && !errors.Is(err, context.Canceled) { log.Warn("Error not in chain call", "error", err, "chain", c.ChainID) connected = false } else { log.Warn("Error in chain call", "error", err) } } - c.SetIsConnected(connected) + c.setIsConnected(connected, skipNotification) } func (c *ClientWithFallback) Tag() string { diff --git a/services/network_utils/network_errors.go b/services/network_utils/network_errors.go new file mode 100644 index 0000000000..72ec3b3a65 --- /dev/null +++ b/services/network_utils/network_errors.go @@ -0,0 +1,92 @@ +package network_utils + +import ( + "context" + "crypto/tls" + "errors" + "net" + "net/http" + "strings" +) + +// IsConnectionError checks if the error is related to network issues. +func IsConnectionError(err error) bool { + if err == nil { + return false + } + + // Check for net.Error (timeout or other network errors) + var netErr net.Error + if errors.As(err, &netErr) { + if netErr.Timeout() { + return true + } + } + + // Check for DNS errors + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) { + return true + } + + // Check for network operation errors (e.g., connection refused) + var opErr *net.OpError + if errors.As(err, &opErr) { + return true + } + + // Check for TLS errors + var tlsRecordErr *tls.RecordHeaderError + if errors.As(err, &tlsRecordErr) { + return true + } + + // FIXME: Check for TLS ECH Rejection Error (tls.ECHRejectionError is added in go 1.23) + + // Check for TLS Certificate Verification Error + var certVerifyErr *tls.CertificateVerificationError + if errors.As(err, &certVerifyErr) { + return true + } + + // Check for TLS Alert Error + var alertErr tls.AlertError + if errors.As(err, &alertErr) { + return true + } + + // Check if context deadline exceeded + if errors.Is(err, context.DeadlineExceeded) { + return true + } + + // Check for specific HTTP server closed error + if errors.Is(err, http.ErrServerClosed) { + return true + } + + // Common connection refused or timeout errors + errMsg := strings.ToLower(err.Error()) + if strings.Contains(errMsg, "i/o timeout") || + strings.Contains(errMsg, "connection refused") || + strings.Contains(errMsg, "network is unreachable") || + strings.Contains(errMsg, "no such host") || + strings.Contains(errMsg, "tls handshake timeout") { + return true + } + + return false +} + +func IsRPSLimitError(err error) bool { + if err == nil { + return false + } + errMsg := strings.ToLower(err.Error()) + if strings.Contains(errMsg, "backoff_seconds") || + strings.Contains(errMsg, "has exceeded its throughput limit") || + strings.Contains(errMsg, "request rate exceeded") { + return true + } + return false +} diff --git a/services/network_utils/network_errors_test.go b/services/network_utils/network_errors_test.go new file mode 100644 index 0000000000..172160109b --- /dev/null +++ b/services/network_utils/network_errors_test.go @@ -0,0 +1,157 @@ +package network_utils + +import ( + "context" + "crypto/tls" + "errors" + "net" + "net/http" + "testing" +) + +// TestIsConnectionError tests the isConnectionError function. +func TestIsConnectionError(t *testing.T) { + tests := []struct { + name string + err error + wantResult bool + }{ + { + name: "nil error", + err: nil, + wantResult: false, + }, + { + name: "net.Error timeout", + err: &net.DNSError{IsTimeout: true}, + wantResult: true, + }, + { + name: "DNS error", + err: &net.DNSError{}, + wantResult: true, + }, + { + name: "net.OpError", + err: &net.OpError{}, + wantResult: true, + }, + { + name: "tls.RecordHeaderError", + err: &tls.RecordHeaderError{}, + wantResult: true, + }, + { + name: "tls.CertificateVerificationError", + err: &tls.CertificateVerificationError{}, + wantResult: true, + }, + { + name: "tls.AlertError", + err: tls.AlertError(0), + wantResult: true, + }, + { + name: "context.DeadlineExceeded", + err: context.DeadlineExceeded, + wantResult: true, + }, + { + name: "http.ErrServerClosed", + err: http.ErrServerClosed, + wantResult: true, + }, + { + name: "i/o timeout error message", + err: errors.New("i/o timeout"), + wantResult: true, + }, + { + name: "connection refused error message", + err: errors.New("connection refused"), + wantResult: true, + }, + { + name: "network is unreachable error message", + err: errors.New("network is unreachable"), + wantResult: true, + }, + { + name: "no such host error message", + err: errors.New("no such host"), + wantResult: true, + }, + { + name: "tls handshake timeout error message", + err: errors.New("tls handshake timeout"), + wantResult: true, + }, + { + name: "rps limit error 1", + err: errors.New("backoff_seconds"), + wantResult: false, + }, + { + name: "rps limit error 2", + err: errors.New("has exceeded its throughput limit"), + wantResult: false, + }, + { + name: "rps limit error 3", + err: errors.New("request rate exceeded"), + wantResult: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsConnectionError(tt.err) + if got != tt.wantResult { + t.Errorf("isConnectionError(%v) = %v; want %v", tt.err, got, tt.wantResult) + } + }) + } +} + +func TestIsRPSLimitError(t *testing.T) { + tests := []struct { + name string + err error + wantResult bool + }{ + { + name: "Error contains 'backoff_seconds'", + err: errors.New("Error: backoff_seconds: 30"), + wantResult: true, + }, + { + name: "Error contains 'has exceeded its throughput limit'", + err: errors.New("Your application has exceeded its throughput limit."), + wantResult: true, + }, + { + name: "Error contains 'request rate exceeded'", + err: errors.New("Request rate exceeded. Please try again later."), + wantResult: true, + }, + { + name: "Error does not contain any matching phrases", + err: errors.New("Some other error occurred."), + wantResult: false, + }, + { + name: "Error is nil", + err: nil, + wantResult: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsRPSLimitError(tt.err) + if got != tt.wantResult { + t.Errorf("IsRPSLimitError(%v) = %v; want %v", tt.err, got, tt.wantResult) + } + }) + } +} diff --git a/services/wallet/api_test.go b/services/wallet/api_test.go index 381fd020c3..11817a11f6 100644 --- a/services/wallet/api_test.go +++ b/services/wallet/api_test.go @@ -152,7 +152,7 @@ func TestAPI_GetAddressDetails(t *testing.T) { chainClient.SetWalletNotifier(func(chainID uint64, message string) {}) c.SetWalletNotifier(func(chainID uint64, message string) {}) - service := NewService(db, accountsDb, appDB, c, accountFeed, nil, nil, nil, ¶ms.NodeConfig{}, nil, nil, nil, nil, nil, "", nil) + service := NewService(db, accountsDb, appDB, c, accountFeed, nil, nil, nil, ¶ms.NodeConfig{}, nil, nil, nil, nil, nil, "") api := &API{ s: service, diff --git a/services/wallet/common/mock/online_checker_mock.go b/services/wallet/common/mock/online_checker_mock.go deleted file mode 100644 index 769266b5e2..0000000000 --- a/services/wallet/common/mock/online_checker_mock.go +++ /dev/null @@ -1,13 +0,0 @@ -package mock_common - -type MockOnlineChecker struct { - online bool -} - -func (m *MockOnlineChecker) Online() bool { - return m.online -} - -func (m *MockOnlineChecker) SetOnline(status bool) { - m.online = status -} diff --git a/services/wallet/common/online_checker_interface.go b/services/wallet/common/online_checker_interface.go deleted file mode 100644 index fe0e481954..0000000000 --- a/services/wallet/common/online_checker_interface.go +++ /dev/null @@ -1,5 +0,0 @@ -package common - -type OnlineChecker interface { - Online() bool -} diff --git a/services/wallet/keycard_pairings_test.go b/services/wallet/keycard_pairings_test.go index 6b4fc29003..df1b200ff0 100644 --- a/services/wallet/keycard_pairings_test.go +++ b/services/wallet/keycard_pairings_test.go @@ -29,7 +29,7 @@ func TestKeycardPairingsFile(t *testing.T) { accountFeed := &event.Feed{} - service := NewService(db, accountsDb, appDB, &rpc.Client{NetworkManager: network.NewManager(db)}, accountFeed, nil, nil, nil, ¶ms.NodeConfig{}, nil, nil, nil, nil, nil, "", nil) + service := NewService(db, accountsDb, appDB, &rpc.Client{NetworkManager: network.NewManager(db)}, accountFeed, nil, nil, nil, ¶ms.NodeConfig{}, nil, nil, nil, nil, nil, "") data, err := service.KeycardPairings().GetPairingsJSONFileContent() require.NoError(t, err) diff --git a/services/wallet/market/market.go b/services/wallet/market/market.go index 004de8ac3b..c2e2d3c50e 100644 --- a/services/wallet/market/market.go +++ b/services/wallet/market/market.go @@ -10,7 +10,7 @@ import ( "github.com/ethereum/go-ethereum/log" "github.com/status-im/status-go/circuitbreaker" - walletcommon "github.com/status-im/status-go/services/wallet/common" + "github.com/status-im/status-go/services/network_utils" "github.com/status-im/status-go/services/wallet/thirdparty" "github.com/status-im/status-go/services/wallet/walletevent" ) @@ -35,10 +35,9 @@ type Manager struct { IsConnectedLock sync.RWMutex circuitbreaker *circuitbreaker.CircuitBreaker providers []thirdparty.MarketDataProvider - onlineChecker walletcommon.OnlineChecker } -func NewManager(providers []thirdparty.MarketDataProvider, feed *event.Feed, onlineChecker walletcommon.OnlineChecker) *Manager { +func NewManager(providers []thirdparty.MarketDataProvider, feed *event.Feed) *Manager { cb := circuitbreaker.NewCircuitBreaker(circuitbreaker.Config{ Timeout: 10000, MaxConcurrentRequests: 100, @@ -53,27 +52,24 @@ func NewManager(providers []thirdparty.MarketDataProvider, feed *event.Feed, onl LastCheckedAt: time.Now().Unix(), circuitbreaker: cb, providers: providers, - onlineChecker: onlineChecker, } } -func (pm *Manager) setIsConnected(value bool) { +func (pm *Manager) setIsConnected(value bool, skipNotification bool) { pm.IsConnectedLock.Lock() defer pm.IsConnectedLock.Unlock() pm.LastCheckedAt = time.Now().Unix() - if value != pm.IsConnected { + if value != pm.IsConnected && !skipNotification { message := "down" if value { message = "up" } - if pm.onlineChecker == nil || pm.onlineChecker.Online() { - pm.feed.Send(walletevent.Event{ - Type: EventMarketStatusChanged, - Accounts: []common.Address{}, - Message: message, - At: time.Now().Unix(), - }) - } + pm.feed.Send(walletevent.Event{ + Type: EventMarketStatusChanged, + Accounts: []common.Address{}, + Message: message, + At: time.Now().Unix(), + }) } pm.IsConnected = value } @@ -89,7 +85,8 @@ func (pm *Manager) makeCall(providers []thirdparty.MarketDataProvider, f func(pr } result := pm.circuitbreaker.Execute(cmd) - pm.setIsConnected(result.Error() == nil) + skipNotification := network_utils.IsConnectionError(result.Error()) + pm.setIsConnected(result.Error() == nil, skipNotification) if result.Error() != nil { log.Error("Error fetching prices", "error", result.Error()) diff --git a/services/wallet/market/market_feed_test.go b/services/wallet/market/market_feed_test.go index aea92b5ecc..9e4262dfe8 100644 --- a/services/wallet/market/market_feed_test.go +++ b/services/wallet/market/market_feed_test.go @@ -1,9 +1,11 @@ package market import ( + "errors" "testing" "time" + "github.com/golang/mock/gomock" "github.com/stretchr/testify/suite" "github.com/ethereum/go-ethereum/event" @@ -14,33 +16,30 @@ import ( type MarketTestSuite struct { suite.Suite - manager *Manager - feedSub *mock_common.FeedSubscription - symbols []string - currencies []string - onlineChecker *mock_common.MockOnlineChecker + feedSub *mock_common.FeedSubscription + symbols []string + currencies []string } func (s *MarketTestSuite) SetupTest() { - priceProviderWithError := &mock_market.MockPriceProviderWithError{} - feed := new(event.Feed) s.feedSub = mock_common.NewFeedSubscription(feed) s.symbols = []string{"BTC", "ETH"} s.currencies = []string{"USD", "EUR"} - s.onlineChecker = &mock_common.MockOnlineChecker{} - s.manager = NewManager([]thirdparty.MarketDataProvider{priceProviderWithError}, feed, s.onlineChecker) } func (s *MarketTestSuite) TearDownTest() { s.feedSub.Close() } -func (s *MarketTestSuite) TestEventSentWhenOnlineCheckerIsNil() { +func (s *MarketTestSuite) TestEventOnRpsError() { + ctrl := gomock.NewController(s.T()) + defer ctrl.Finish() // GIVEN - priceProviderWithError := &mock_market.MockPriceProviderWithError{} - manager := NewManager([]thirdparty.MarketDataProvider{priceProviderWithError}, s.feedSub.GetFeed(), nil) + customErr := errors.New("request rate exceeded") + priceProviderWithError := mock_market.NewMockPriceProviderWithError(ctrl, customErr) + manager := NewManager([]thirdparty.MarketDataProvider{priceProviderWithError}, s.feedSub.GetFeed()) // WHEN _, err := manager.FetchPrices(s.symbols, s.currencies) @@ -52,26 +51,16 @@ func (s *MarketTestSuite) TestEventSentWhenOnlineCheckerIsNil() { s.Require().Equal(event.Type, EventMarketStatusChanged) } -func (s *MarketTestSuite) TestEventSentWhenOnlineCheckerIsOnline() { - // GIVEN - s.onlineChecker.SetOnline(true) - - // WHEN - _, err := s.manager.FetchPrices(s.symbols, s.currencies) - s.Require().Error(err, "expected error from FetchPrices due to MockPriceProviderWithError") - event, ok := s.feedSub.WaitForEvent(5 * time.Second) - s.Require().True(ok, "expected an event, but none was received") +func (s *MarketTestSuite) TestNoEventOnNetworkError() { + ctrl := gomock.NewController(s.T()) + defer ctrl.Finish() - // THEN - s.Require().Equal(event.Type, EventMarketStatusChanged) -} - -func (s *MarketTestSuite) TestNoEventSentWhenOnlineCheckerIsOffline() { // GIVEN - s.onlineChecker.SetOnline(false) + customErr := errors.New("dial tcp: lookup optimism-goerli.infura.io: no such host") + priceProviderWithError := mock_market.NewMockPriceProviderWithError(ctrl, customErr) + manager := NewManager([]thirdparty.MarketDataProvider{priceProviderWithError}, s.feedSub.GetFeed()) - // WHEN - _, err := s.manager.FetchPrices(s.symbols, s.currencies) + _, err := manager.FetchPrices(s.symbols, s.currencies) s.Require().Error(err, "expected error from FetchPrices due to MockPriceProviderWithError") _, ok := s.feedSub.WaitForEvent(time.Millisecond * 500) diff --git a/services/wallet/market/market_test.go b/services/wallet/market/market_test.go index 1699096e30..2d266ad3be 100644 --- a/services/wallet/market/market_test.go +++ b/services/wallet/market/market_test.go @@ -16,7 +16,7 @@ import ( ) func setupTestPrice(t *testing.T, providers []thirdparty.MarketDataProvider) *Manager { - return NewManager(providers, &event.Feed{}, nil) + return NewManager(providers, &event.Feed{}) } var mockPrices = map[string]map[string]float64{ @@ -90,7 +90,10 @@ func TestFetchPriceErrorFirstProvider(t *testing.T) { defer ctrl.Finish() priceProvider := mock_market.NewMockPriceProvider(ctrl) priceProvider.SetMockPrices(mockPrices) - priceProviderWithError := &mock_market.MockPriceProviderWithError{} + + customErr := errors.New("error") + priceProviderWithError := mock_market.NewMockPriceProviderWithError(ctrl, customErr) + symbols := []string{"BTC", "ETH"} currencies := []string{"USD", "EUR"} diff --git a/services/wallet/market/mock/mock_price_provider.go b/services/wallet/market/mock/mock_price_provider.go index 94af4f9f95..3eb3edf715 100644 --- a/services/wallet/market/mock/mock_price_provider.go +++ b/services/wallet/market/mock/mock_price_provider.go @@ -1,8 +1,6 @@ package mock_market import ( - "errors" - "github.com/golang/mock/gomock" mock_thirdparty "github.com/status-im/status-go/services/wallet/thirdparty/mock" @@ -40,8 +38,17 @@ func (mpp *MockPriceProvider) FetchPrices(symbols []string, currencies []string) type MockPriceProviderWithError struct { MockPriceProvider + err error +} + +// NewMockPriceProviderWithError creates a new MockPriceProviderWithError with the specified error +func NewMockPriceProviderWithError(ctrl *gomock.Controller, err error) *MockPriceProviderWithError { + return &MockPriceProviderWithError{ + MockPriceProvider: *NewMockPriceProvider(ctrl), + err: err, + } } func (mpp *MockPriceProviderWithError) FetchPrices(symbols []string, currencies []string) (map[string]map[string]float64, error) { - return nil, errors.New("error") + return nil, mpp.err } diff --git a/services/wallet/service.go b/services/wallet/service.go index 8cc557e907..9ddeb597bb 100644 --- a/services/wallet/service.go +++ b/services/wallet/service.go @@ -25,7 +25,6 @@ import ( "github.com/status-im/status-go/services/wallet/balance" "github.com/status-im/status-go/services/wallet/blockchainstate" "github.com/status-im/status-go/services/wallet/collectibles" - walletcommon "github.com/status-im/status-go/services/wallet/common" "github.com/status-im/status-go/services/wallet/community" "github.com/status-im/status-go/services/wallet/currency" "github.com/status-im/status-go/services/wallet/history" @@ -64,7 +63,6 @@ func NewService( feed *event.Feed, mediaServer *server.MediaServer, statusProxyStageName string, - onlineChecker walletcommon.OnlineChecker, ) *Service { signals := &walletevent.SignalsTransmitter{ Publisher: feed, @@ -92,9 +90,6 @@ func NewService( return } - if onlineChecker != nil && !onlineChecker.Online() { - return - } feed.Send(walletevent.Event{ Type: EventBlockchainStatusChanged, Accounts: []common.Address{}, @@ -130,7 +125,7 @@ func NewService( User: config.WalletConfig.StatusProxyMarketUser, Password: config.WalletConfig.StatusProxyMarketPassword, }) - marketManager := market.NewManager([]thirdparty.MarketDataProvider{cryptoCompare, coingecko, cryptoCompareProxy}, feed, onlineChecker) + marketManager := market.NewManager([]thirdparty.MarketDataProvider{cryptoCompare, coingecko, cryptoCompareProxy}, feed) reader := NewReader(tokenManager, marketManager, token.NewPersistence(db), feed) history := history.NewService(db, accountsDB, accountFeed, feed, rpcClient, tokenManager, marketManager, balanceCacher.Cache()) currency := currency.NewService(db, feed, tokenManager, marketManager)