From 82b63ccf2975321ad50b208a24341c63c19e5b4c Mon Sep 17 00:00:00 2001 From: saledjenic <86303051+saledjenic@users.noreply.github.com> Date: Mon, 29 Jul 2024 19:03:13 +0200 Subject: [PATCH] chore_: needed router changes (#5620) * fix_: setting best route only when we have it * chore_: when returning errors, give precedence to custom (network) errors * chore_: skip flaky tests (#5606) * chore_: skip TestPeerCount * chore_: skip flaky wallet tests * chore_: skip flaky TestMemberReceivesPendingRequestToJoinAfterAfterGettingTokenMasterRole * test_: flaky router tests fixed Closes https://github.com/status-im/status-go/issues/5601 --------- Co-authored-by: Igor Sirotin --- ...es_messenger_shared_member_address_test.go | 3 + services/wallet/router/errors.go | 2 +- .../wallet/router/pathprocessor/errors.go | 27 ++++++++ .../router/pathprocessor/errors_test.go | 35 ++++++++++ services/wallet/router/router_v2.go | 65 +++++++++++++++++-- services/wallet/router/router_v2_test.go | 27 ++++---- services/wallet/router/router_v2_test_data.go | 4 +- telemetry/client_test.go | 2 + 8 files changed, 143 insertions(+), 22 deletions(-) diff --git a/protocol/communities_messenger_shared_member_address_test.go b/protocol/communities_messenger_shared_member_address_test.go index b3a250422..08f5844b6 100644 --- a/protocol/communities_messenger_shared_member_address_test.go +++ b/protocol/communities_messenger_shared_member_address_test.go @@ -10,6 +10,7 @@ import ( gethcommon "github.com/ethereum/go-ethereum/common" hexutil "github.com/ethereum/go-ethereum/common/hexutil" + gethbridge "github.com/status-im/status-go/eth-node/bridge/geth" "github.com/status-im/status-go/eth-node/crypto" "github.com/status-im/status-go/eth-node/types" @@ -819,6 +820,8 @@ func (s *MessengerCommunitiesSharedMemberAddressSuite) TestTokenMasterReceivesAc } func (s *MessengerCommunitiesSharedMemberAddressSuite) TestMemberReceivesPendingRequestToJoinAfterAfterGettingTokenMasterRole() { + s.T().Skip("flaky test") + community, _ := createOnRequestCommunity(&s.Suite, s.owner) advertiseCommunityTo(&s.Suite, community, s.owner, s.alice) diff --git a/services/wallet/router/errors.go b/services/wallet/router/errors.go index 4ff7a9491..fb859115d 100644 --- a/services/wallet/router/errors.go +++ b/services/wallet/router/errors.go @@ -29,6 +29,6 @@ var ( ErrLockedAmountExcludesAllSupported = &errors.ErrorResponse{Code: errors.ErrorCode("WR-021"), Details: "all supported chains are excluded, routing impossible"} ErrTokenNotFound = &errors.ErrorResponse{Code: errors.ErrorCode("WR-022"), Details: "token not found"} ErrNoBestRouteFound = &errors.ErrorResponse{Code: errors.ErrorCode("WR-023"), Details: "no best route found"} - ErrCannotCheckReceiverBalance = &errors.ErrorResponse{Code: errors.ErrorCode("WR-024"), Details: "cannot check receiver balance"} + ErrCannotCheckBalance = &errors.ErrorResponse{Code: errors.ErrorCode("WR-024"), Details: "cannot check balance"} ErrCannotCheckLockedAmounts = &errors.ErrorResponse{Code: errors.ErrorCode("WR-025"), Details: "cannot check locked amounts"} ) diff --git a/services/wallet/router/pathprocessor/errors.go b/services/wallet/router/pathprocessor/errors.go index aa7b08378..5b635b9d6 100644 --- a/services/wallet/router/pathprocessor/errors.go +++ b/services/wallet/router/pathprocessor/errors.go @@ -95,3 +95,30 @@ func createErrorResponse(processorName string, err error) error { customErrResp.Details = genericErrResp.Details return customErrResp } + +func IsCustomError(err error) bool { + if err == nil { + return false + } + + errResp, ok := err.(*errors.ErrorResponse) + if !ok { + return false + } + + switch errResp { + case ErrTransferCustomError, + ErrERC721TransferCustomError, + ErrERC1155TransferCustomError, + ErrBridgeHopCustomError, + ErrBridgeCellerCustomError, + ErrSwapParaswapCustomError, + ErrENSRegisterCustomError, + ErrENSReleaseCustomError, + ErrENSPublicKeyCustomError, + ErrStickersBuyCustomError: + return true + default: + return false + } +} diff --git a/services/wallet/router/pathprocessor/errors_test.go b/services/wallet/router/pathprocessor/errors_test.go index 42236991a..550d25127 100644 --- a/services/wallet/router/pathprocessor/errors_test.go +++ b/services/wallet/router/pathprocessor/errors_test.go @@ -68,3 +68,38 @@ func TestNonGenericErrorResponse(t *testing.T) { require.Equal(t, errResp.Code, castPPErrResp.Code) require.Equal(t, errResp.Details, castPPErrResp.Details) } + +func TestCustomErrors(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + { + name: "no error - nil", + err: nil, + expected: false, + }, + { + name: "not error response error", + err: errors.New("unknown error"), + expected: false, + }, + { + name: "not custom error", + err: ErrFromChainNotSupported, + expected: false, + }, + { + name: "custom error", + err: ErrTransferCustomError, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expected, IsCustomError(tt.err)) + }) + } +} diff --git a/services/wallet/router/router_v2.go b/services/wallet/router/router_v2.go index 4c9c7d3db..24d61a34a 100644 --- a/services/wallet/router/router_v2.go +++ b/services/wallet/router/router_v2.go @@ -175,7 +175,6 @@ func newSuggestedRoutesV2( suggestedRoutes := &SuggestedRoutesV2{ Uuid: uuid, Candidates: candidates, - Best: candidates, TokenPrice: tokenPrice, NativeChainTokenPrice: nativeChainTokenPrice, } @@ -190,6 +189,14 @@ func newSuggestedRoutesV2( allRoutes := node.buildAllRoutesV2() allRoutes = filterRoutesV2(allRoutes, amountIn, fromLockedAmount) + if len(allRoutes) > 0 { + sort.Slice(allRoutes, func(i, j int) bool { + iRoute := getRoutePriority(allRoutes[i]) + jRoute := getRoutePriority(allRoutes[j]) + return iRoute <= jRoute + }) + } + return suggestedRoutes, allRoutes } @@ -507,7 +514,16 @@ func (r *Router) SuggestedRoutesV2(ctx context.Context, input *RouteInputParams) // No best route found, but no error given. if len(processorErrors) > 0 { // Return one of the path processor errors if present. - err = errors.CreateErrorResponseFromError(processorErrors[0].Error) + // Give precedence to the custom error message. + for _, processorError := range processorErrors { + if processorError.Error != nil && pathprocessor.IsCustomError(processorError.Error) { + err = processorError.Error + break + } + } + if err == nil { + err = errors.CreateErrorResponseFromError(processorErrors[0].Error) + } } else { err = ErrNoBestRouteFound } @@ -995,6 +1011,12 @@ func (r *Router) resolveCandidates(ctx context.Context, input *RouteInputParams, }) } + sort.Slice(candidates, func(i, j int) bool { + iChain := getChainPriority(candidates[i].FromChain.ChainID) + jChain := getChainPriority(candidates[j].FromChain.ChainID) + return iChain <= jChain + }) + group.Wait() return candidates, processorErrors, nil } @@ -1004,7 +1026,7 @@ func (r *Router) checkBalancesForTheBestRoute(ctx context.Context, bestRoute []* return new(big.Int).Set(v.(*big.Int)) }).(map[string]*big.Int) if balanceMapCopy == nil { - return ErrCannotCheckReceiverBalance + return ErrCannotCheckBalance } // check the best route for the required balances @@ -1071,6 +1093,27 @@ func removeBestRouteFromAllRouters(allRoutes [][]*PathV2, best []*PathV2) [][]*P return nil } +func getChainPriority(chainID uint64) int { + switch chainID { + case walletCommon.EthereumMainnet, walletCommon.EthereumSepolia: + return 1 + case walletCommon.OptimismMainnet, walletCommon.OptimismSepolia: + return 2 + case walletCommon.ArbitrumMainnet, walletCommon.ArbitrumSepolia: + return 3 + default: + return 0 + } +} + +func getRoutePriority(route []*PathV2) int { + priority := 0 + for _, path := range route { + priority += getChainPriority(path.FromChain.ChainID) + } + return priority +} + func (r *Router) resolveRoutes(ctx context.Context, input *RouteInputParams, candidates []*PathV2, balanceMap map[string]*big.Int) (suggestedRoutes *SuggestedRoutesV2, err error) { var prices map[string]float64 if input.testsMode { @@ -1088,6 +1131,16 @@ func (r *Router) resolveRoutes(ctx context.Context, input *RouteInputParams, can var allRoutes [][]*PathV2 suggestedRoutes, allRoutes = newSuggestedRoutesV2(input.Uuid, input.AmountIn.ToInt(), candidates, input.FromLockedAmount, tokenPrice, nativeTokenPrice) + defer func() { + if suggestedRoutes.Best != nil && len(suggestedRoutes.Best) > 0 { + sort.Slice(suggestedRoutes.Best, func(i, j int) bool { + iChain := getChainPriority(suggestedRoutes.Best[i].FromChain.ChainID) + jChain := getChainPriority(suggestedRoutes.Best[j].FromChain.ChainID) + return iChain <= jChain + }) + } + }() + for len(allRoutes) > 0 { best := findBestV2(allRoutes, tokenPrice, nativeTokenPrice) @@ -1098,18 +1151,16 @@ func (r *Router) resolveRoutes(ctx context.Context, input *RouteInputParams, can if (input.SendType == Transfer || input.SendType == Bridge) && len(allRoutes) > 1 { + allRoutes = removeBestRouteFromAllRouters(allRoutes, best) continue } else { + suggestedRoutes.Best = best return suggestedRoutes, errors.CreateErrorResponseFromError(err) } } if len(best) > 0 { - sort.Slice(best, func(i, j int) bool { - return best[i].AmountInLocked - }) - // At this point we have to do the final check and update the amountIn (subtracting fees) if complete balance is going to be sent for native token (ETH) for _, path := range best { if path.subtractFees && path.FromToken.IsNative() { diff --git a/services/wallet/router/router_v2_test.go b/services/wallet/router/router_v2_test.go index 76fda581b..3ba364b74 100644 --- a/services/wallet/router/router_v2_test.go +++ b/services/wallet/router/router_v2_test.go @@ -230,20 +230,23 @@ func TestNoBalanceForTheBestRouteRouterV2(t *testing.T) { // Test async endpoints for _, tt := range tests { - router.SuggestedRoutesV2Async(tt.input) + t.Run(tt.name, func(t *testing.T) { - select { - case asyncRoutes := <-suggestedRoutesCh: - assert.Equal(t, tt.input.Uuid, asyncRoutes.Uuid) - assert.Equal(t, tt.expectedError, asyncRoutes.ErrorResponse) - assertPathsEqual(t, tt.expectedCandidates, asyncRoutes.Candidates) - if tt.expectedError == nil { - assertPathsEqual(t, tt.expectedBest, asyncRoutes.Best) + router.SuggestedRoutesV2Async(tt.input) + + select { + case asyncRoutes := <-suggestedRoutesCh: + assert.Equal(t, tt.input.Uuid, asyncRoutes.Uuid) + assert.Equal(t, tt.expectedError, asyncRoutes.ErrorResponse) + assertPathsEqual(t, tt.expectedCandidates, asyncRoutes.Candidates) + if tt.expectedError == nil { + assertPathsEqual(t, tt.expectedBest, asyncRoutes.Best) + } + break + case <-time.After(10 * time.Second): + t.FailNow() } - break - case <-time.After(10 * time.Second): - t.FailNow() - } + }) } } diff --git a/services/wallet/router/router_v2_test_data.go b/services/wallet/router/router_v2_test_data.go index 985cd4e8b..d859e4066 100644 --- a/services/wallet/router/router_v2_test_data.go +++ b/services/wallet/router/router_v2_test_data.go @@ -2728,7 +2728,7 @@ func getNoBalanceTestParamsList() []noBalanceTestParams { }, expectedError: &errors.ErrorResponse{ Code: ErrNotEnoughTokenBalance.Code, - Details: fmt.Sprintf(ErrNotEnoughTokenBalance.Details, pathprocessor.UsdcSymbol, walletCommon.EthereumMainnet), + Details: fmt.Sprintf(ErrNotEnoughTokenBalance.Details, pathprocessor.UsdcSymbol, walletCommon.ArbitrumMainnet), }, expectedCandidates: []*PathV2{ { @@ -2788,7 +2788,7 @@ func getNoBalanceTestParamsList() []noBalanceTestParams { }, expectedError: &errors.ErrorResponse{ Code: ErrNotEnoughNativeBalance.Code, - Details: fmt.Sprintf(ErrNotEnoughNativeBalance.Details, pathprocessor.EthSymbol, walletCommon.EthereumMainnet), + Details: fmt.Sprintf(ErrNotEnoughNativeBalance.Details, pathprocessor.EthSymbol, walletCommon.ArbitrumMainnet), }, expectedCandidates: []*PathV2{ { diff --git a/telemetry/client_test.go b/telemetry/client_test.go index 566829315..8eb121c13 100644 --- a/telemetry/client_test.go +++ b/telemetry/client_test.go @@ -352,6 +352,8 @@ func setDefaultConfig(config *wakuv2.Config, lightMode bool) { var testStoreENRBootstrap = "enrtree://AI4W5N5IFEUIHF5LESUAOSMV6TKWF2MB6GU2YK7PU4TYUGUNOCEPW@store.staging.shards.nodes.status.im" func TestPeerCount(t *testing.T) { + t.Skip("flaky test") + expectedCondition := func(received []TelemetryRequest) (shouldSucceed bool, shouldFail bool) { found := slices.ContainsFunc(received, func(req TelemetryRequest) bool { t.Log(req)