fix(wallet)_: limit max parameter length in cryptocompare price fetches (#5957)

* fix(wallet)_: limit max parameter length in cryptocompare price fetches

* chore(wallet)_: separate package for network-dependant unit tests

Makefile

@@ -362,23 +362,32 @@ docker-test: ##@tests Run tests in a docker container with golang.
 
 test: test-unit ##@tests Run basic, short tests during development
 
-test-unit: generate
-test-unit: export BUILD_TAGS ?=
-test-unit: export UNIT_TEST_DRY_RUN ?= false
-test-unit: export UNIT_TEST_COUNT ?= 1
-test-unit: export UNIT_TEST_FAILFAST ?= true
+test-unit-prep: generate
+test-unit-prep: export BUILD_TAGS ?=
+test-unit-prep: export UNIT_TEST_DRY_RUN ?= false
+test-unit-prep: export UNIT_TEST_COUNT ?= 1
+test-unit-prep: export UNIT_TEST_FAILFAST ?= true
+test-unit-prep: export UNIT_TEST_USE_DEVELOPMENT_LOGGER ?= true
+test-unit-prep: export UNIT_TEST_REPORT_CODECLIMATE ?= false
+test-unit-prep: export UNIT_TEST_REPORT_CODECOV ?= false
+
+test-unit: test-unit-prep
 test-unit: export UNIT_TEST_RERUN_FAILS ?= true
-test-unit: export UNIT_TEST_USE_DEVELOPMENT_LOGGER ?= true
-test-unit: export UNIT_TEST_REPORT_CODECLIMATE ?= false
-test-unit: export UNIT_TEST_REPORT_CODECOV ?= false
 test-unit: export UNIT_TEST_PACKAGES ?= $(call sh, go list ./... | \
 	grep -v /vendor | \
 	grep -v /t/e2e | \
 	grep -v /t/benchmarks | \
-	grep -v /transactions/fake)
+	grep -v /transactions/fake | \
+	grep -v /tests-unit-network)
 test-unit: ##@tests Run unit and integration tests
 	./_assets/scripts/run_unit_tests.sh
 
+test-unit-network: test-unit-prep
+test-unit-network: export UNIT_TEST_RERUN_FAILS ?= false
+test-unit-network: export UNIT_TEST_PACKAGES ?= $(call sh, go list ./tests-unit-network/...)
+test-unit-network: ##@tests Run unit and integration tests with network access
+	./_assets/scripts/run_unit_tests.sh
+
 test-unit-race: export GOTEST_EXTRAFLAGS=-race
 test-unit-race: test-unit ##@tests Run unit and integration tests with -race flag
 

services/wallet/thirdparty/cryptocompare/client.go

@@ -73,7 +73,15 @@ func NewClientWithParams(params Params) *Client {
 }
 
 func (c *Client) FetchPrices(symbols []string, currencies []string) (map[string]map[string]float64, error) {
-	chunks := utils.ChunkSymbols(symbols, 60)
+	const maxFsymsLength = 300
+	chunkSymbolParams := utils.ChunkSymbolsParams{
+		MaxCharsPerChunk:    maxFsymsLength,
+		ExtraCharsPerSymbol: 1, // joined with a comma
+	}
+	chunks, err := utils.ChunkSymbols(symbols, chunkSymbolParams)
+	if err != nil {
+		return nil, err
+	}
 	result := make(map[string]map[string]float64)
 	realCurrencies := utils.RenameSymbols(currencies)
 	for _, smbls := range chunks {
@@ -82,6 +90,7 @@ func (c *Client) FetchPrices(symbols []string, currencies []string) (map[string]
 		params := url.Values{}
 		params.Add("fsyms", strings.Join(realSymbols, ","))
 		params.Add("tsyms", strings.Join(realCurrencies, ","))
+		params.Add("relaxedValidation", "true")
 		params.Add("extraParams", extraParamStatus)
 
 		url := fmt.Sprintf("%s/data/pricemulti", c.baseURL)
@@ -129,7 +138,15 @@ func (c *Client) FetchTokenDetails(symbols []string) (map[string]thirdparty.Toke
 }
 
 func (c *Client) FetchTokenMarketValues(symbols []string, currency string) (map[string]thirdparty.TokenMarketValues, error) {
-	chunks := utils.ChunkSymbols(symbols)
+	const maxFsymsLength = 300
+	chunkSymbolParams := utils.ChunkSymbolsParams{
+		MaxCharsPerChunk:    maxFsymsLength,
+		ExtraCharsPerSymbol: 1, // joined with a comma
+	}
+	chunks, err := utils.ChunkSymbols(symbols, chunkSymbolParams)
+	if err != nil {
+		return nil, err
+	}
 	realCurrency := utils.GetRealSymbol(currency)
 	item := map[string]thirdparty.TokenMarketValues{}
 	for _, smbls := range chunks {
@@ -138,6 +155,7 @@ func (c *Client) FetchTokenMarketValues(symbols []string, currency string) (map[
 		params := url.Values{}
 		params.Add("fsyms", strings.Join(realSymbols, ","))
 		params.Add("tsyms", realCurrency)
+		params.Add("relaxedValidation", "true")
 		params.Add("extraParams", extraParamStatus)
 
 		url := fmt.Sprintf("%s/data/pricemultifull", c.baseURL)

services/wallet/thirdparty/utils/symbols.go

@@ -1,6 +1,9 @@
 package utils
 
-import "strings"
+import (
+	"errors"
+	"strings"
+)
 
 var renameMapping = map[string]string{
 	"STT": "SNT",
@@ -32,22 +35,36 @@ func GetRealSymbol(symbol string) string {
 	return strings.ToUpper(symbol)
 }
 
-func ChunkSymbols(symbols []string, chunkSizeOptional ...int) [][]string {
-	var chunks [][]string
-	chunkSize := 100
-	if len(chunkSizeOptional) > 0 {
-		chunkSize = chunkSizeOptional[0]
-	}
-
-	for i := 0; i < len(symbols); i += chunkSize {
-		end := i + chunkSize
-
-		if end > len(symbols) {
-			end = len(symbols)
-		}
-
-		chunks = append(chunks, symbols[i:end])
-	}
-
-	return chunks
+type ChunkSymbolsParams struct {
+	MaxSymbolsPerChunk  int
+	MaxCharsPerChunk    int
+	ExtraCharsPerSymbol int
+}
+
+func ChunkSymbols(symbols []string, params ChunkSymbolsParams) ([][]string, error) {
+	var chunks [][]string
+	if len(symbols) == 0 {
+		return chunks, nil
+	}
+
+	chunk := make([]string, 0, 100)
+	chunkChars := 0
+	for _, symbol := range symbols {
+		symbolChars := len(symbol) + params.ExtraCharsPerSymbol
+		if params.MaxCharsPerChunk > 0 && symbolChars > params.MaxCharsPerChunk {
+			return nil, errors.New("chunk cannot fit symbol: " + symbol)
+		}
+		if (params.MaxCharsPerChunk > 0 && chunkChars+symbolChars > params.MaxCharsPerChunk) ||
+			(params.MaxSymbolsPerChunk > 0 && len(chunk) >= params.MaxSymbolsPerChunk) {
+			// Max chars/symbols reached, store chunk and start a new one
+			chunks = append(chunks, chunk)
+			chunk = make([]string, 0, 100)
+			chunkChars = 0
+		}
+		chunk = append(chunk, symbol)
+		chunkChars += symbolChars
+	}
+	chunks = append(chunks, chunk)
+
+	return chunks, nil
 }

services/wallet/thirdparty/utils/symbols_test.go

@@ -0,0 +1,61 @@
+package utils
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_RenameSymbols(t *testing.T) {
+	symbols := []string{"STT", "ETH", "BTC"}
+	renames := RenameSymbols(symbols)
+	require.Equal(t, []string{"SNT", "ETH", "BTC"}, renames)
+}
+
+func Test_RemoveDuplicates(t *testing.T) {
+	strings := []string{"STT", "ETH", "BTC", "ETH", "BTC"}
+	uniqueStrings := RemoveDuplicates(strings)
+	require.Equal(t, []string{"STT", "ETH", "BTC"}, uniqueStrings)
+}
+
+func Test_GetRealSymbol(t *testing.T) {
+	require.Equal(t, "SNT", GetRealSymbol("STT"))
+	require.Equal(t, "ETH", GetRealSymbol("ETH"))
+}
+
+func Test_ChunkSymbols(t *testing.T) {
+	symbols := []string{"STT", "ETH", "BTC"}
+	params := ChunkSymbolsParams{MaxCharsPerChunk: 10, ExtraCharsPerSymbol: 1}
+	chunks, err := ChunkSymbols(symbols, params)
+	require.NoError(t, err)
+	require.Equal(t, [][]string{{"STT", "ETH"}, {"BTC"}}, chunks)
+
+	params = ChunkSymbolsParams{MaxCharsPerChunk: 10, ExtraCharsPerSymbol: 2}
+	chunks, err = ChunkSymbols(symbols, params)
+	require.NoError(t, err)
+	require.Equal(t, [][]string{{"STT", "ETH"}, {"BTC"}}, chunks)
+
+	params = ChunkSymbolsParams{MaxCharsPerChunk: 4, ExtraCharsPerSymbol: 1}
+	chunks, err = ChunkSymbols(symbols, params)
+	require.NoError(t, err)
+	require.Equal(t, [][]string{{"STT"}, {"ETH"}, {"BTC"}}, chunks)
+
+	params = ChunkSymbolsParams{MaxSymbolsPerChunk: 1, MaxCharsPerChunk: 10, ExtraCharsPerSymbol: 2}
+	chunks, err = ChunkSymbols(symbols, params)
+	require.NoError(t, err)
+	require.Equal(t, [][]string{{"STT"}, {"ETH"}, {"BTC"}}, chunks)
+
+	params = ChunkSymbolsParams{MaxCharsPerChunk: 9, ExtraCharsPerSymbol: 2}
+	chunks, err = ChunkSymbols(symbols, params)
+	require.NoError(t, err)
+	require.Equal(t, [][]string{{"STT"}, {"ETH"}, {"BTC"}}, chunks)
+
+	params = ChunkSymbolsParams{MaxCharsPerChunk: 2, ExtraCharsPerSymbol: 1}
+	chunks, err = ChunkSymbols([]string{}, params)
+	require.NoError(t, err)
+	require.Len(t, chunks, 0)
+
+	params = ChunkSymbolsParams{MaxCharsPerChunk: 2, ExtraCharsPerSymbol: 1}
+	_, err = ChunkSymbols(symbols, params)
+	require.Error(t, err)
+}

tests-unit-network/README.MD

@@ -0,0 +1,12 @@
+## Overview
+
+Tests for status-go that require access to the network (for example, chain/market/collectibles providers).
+
+## Table of Contents
+
+- [Overview](#overview)
+- [How to Run](#how-to-run)
+
+## How to Run
+
+`make test-unit-network`

tests-unit-network/cryptocompare/cryptocompare_test.go

@@ -0,0 +1,84 @@
+package cryptocompare_tests
+
+import (
+	"testing"
+
+	"github.com/status-im/status-go/appdatabase"
+	"github.com/status-im/status-go/params"
+	mock_network "github.com/status-im/status-go/rpc/network/mock"
+	w_common "github.com/status-im/status-go/services/wallet/common"
+	"github.com/status-im/status-go/services/wallet/thirdparty/cryptocompare"
+	"github.com/status-im/status-go/services/wallet/token"
+	"github.com/status-im/status-go/t/helpers"
+	"github.com/status-im/status-go/walletdatabase"
+
+	"github.com/stretchr/testify/require"
+
+	"go.uber.org/mock/gomock"
+)
+
+func getTokenSymbols(t *testing.T) []string {
+	appDB, err := helpers.SetupTestMemorySQLDB(appdatabase.DbInitializer{})
+	require.NoError(t, err)
+
+	walletDB, err := helpers.SetupTestMemorySQLDB(walletdatabase.DbInitializer{})
+	require.NoError(t, err)
+
+	networksList := []params.Network{
+		{
+			ChainID: w_common.EthereumMainnet,
+		},
+		{
+			ChainID: w_common.OptimismMainnet,
+		},
+		{
+			ChainID: w_common.ArbitrumMainnet,
+		},
+	}
+
+	ptrNetworkList := make([]*params.Network, 0, len(networksList))
+	for i := range networksList {
+		ptrNetworkList = append(ptrNetworkList, &networksList[i])
+	}
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	networkManager := mock_network.NewMockManagerInterface(ctrl)
+	networkManager.EXPECT().Get(gomock.Any()).Return(ptrNetworkList, nil).AnyTimes()
+	networkManager.EXPECT().GetAll().Return(ptrNetworkList, nil).AnyTimes()
+	networkManager.EXPECT().GetConfiguredNetworks().Return(networksList).AnyTimes()
+
+	// Skeleton token store to get full list of tokens
+	tm := token.NewTokenManager(walletDB, nil, nil, networkManager, appDB, nil, nil, nil, nil, nil)
+
+	tokens, err := tm.GetAllTokens()
+	require.NoError(t, err)
+
+	symbolsMap := make(map[string]bool)
+	for _, token := range tokens {
+		symbolsMap[token.Symbol] = true
+	}
+
+	symbols := make([]string, 0, len(symbolsMap))
+	for symbol := range symbolsMap {
+		symbols = append(symbols, symbol)
+	}
+
+	return symbols
+}
+
+func TestFetchPrices(t *testing.T) {
+	symbols := getTokenSymbols(t)
+
+	stdClient := cryptocompare.NewClient()
+	_, err := stdClient.FetchPrices(symbols, []string{"USD"})
+	require.NoError(t, err)
+}
+
+func TestFetchTokenMarketValues(t *testing.T) {
+	symbols := getTokenSymbols(t)
+
+	stdClient := cryptocompare.NewClient()
+	_, err := stdClient.FetchTokenMarketValues(symbols, "USD")
+	require.NoError(t, err)
+}