transfer (total escrow): some more review comments (#3519)

* some more review comments * Rename pathAndEscrowAMount to pathAndEscrowAmount --------- Co-authored-by: DimitrisJim <[email protected]>
cosmos · May 4, 2023 · 9f99eee · 9f99eee
1 parent a448960
commit 9f99eee
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 29 deletions.
diff --git a/modules/apps/transfer/keeper/genesis_test.go b/modules/apps/transfer/keeper/genesis_test.go
@@ -29,16 +29,16 @@ func (suite *KeeperTestSuite) TestGenesis() {
 		}
 	)
 
-	for _, pathAndEscrowMount := range pathsAndEscrowAmounts {
+	for _, pathAndEscrowAmount := range pathsAndEscrowAmounts {
 		denomTrace := types.DenomTrace{
 			BaseDenom: "uatom",
-			Path:      pathAndEscrowMount.path,
+			Path:      pathAndEscrowAmount.path,
 		}
 		traces = append(types.Traces{denomTrace}, traces...)
 		suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), denomTrace)
 
 		denom := denomTrace.IBCDenom()
-		amount, ok := math.NewIntFromString(pathAndEscrowMount.escrow)
+		amount, ok := math.NewIntFromString(pathAndEscrowAmount.escrow)
 		suite.Require().True(ok)
 		escrows = append(sdk.NewCoins(sdk.NewCoin(denom, amount)), escrows...)
 		suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), denom, amount)

diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go
@@ -190,12 +190,7 @@ func (k Keeper) IterateTokensInEscrow(ctx sdk.Context, prefix []byte, cb func(de
 
 	defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() })
 	for ; iterator.Valid(); iterator.Next() {
-		keySplit := strings.Split(string(iterator.Key()), "/")
-		if len(keySplit) < 2 {
-			continue // key doesn't conform to expected format
-		}
-
-		denom := strings.Join(keySplit[1:], "/")
+		denom := strings.TrimPrefix(string(iterator.Key()), fmt.Sprintf("%s/", types.KeyTotalEscrowPrefix))
 		if strings.TrimSpace(denom) == "" {
 			continue // denom is empty
 		}

diff --git a/modules/apps/transfer/keeper/migrations_test.go b/modules/apps/transfer/keeper/migrations_test.go
@@ -3,7 +3,6 @@ package keeper_test
 import (
 	"fmt"
 
-	"cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil"
 
@@ -127,59 +126,78 @@ func (suite *KeeperTestSuite) TestMigratorMigrateTracesCorruptionDetection() {
 
 func (suite *KeeperTestSuite) TestMigrateTotalEscrowForDenom() {
 	var (
-		path  *ibctesting.Path
-		denom string
+		path             *ibctesting.Path
+		expectedEscrowed sdk.Coins
 	)
 
 	testCases := []struct {
-		msg               string
-		malleate          func()
-		expectedEscrowAmt math.Int
+		msg      string
+		malleate func()
 	}{
 		{
 			"success: one native denom escrowed in one channel",
 			func() {
-				denom = sdk.DefaultBondDenom
+				denom := sdk.DefaultBondDenom
 				escrowAddress := transfertypes.GetEscrowAddress(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
-				coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
+				coin := sdk.NewCoin(denom, sdk.NewInt(100))
 
 				// funds the escrow account to have balance
 				suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress, sdk.NewCoins(coin)))
+
+				expectedEscrowed.Add(coin)
+			},
+		},
+		{
+			"success: two native denom escrowed in one channel",
+			func() {
+				denom1 := "samoleans"
+				denom2 := sdk.DefaultBondDenom
+
+				escrowAddress := transfertypes.GetEscrowAddress(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+				coin1 := sdk.NewCoin(denom1, sdk.NewInt(100))
+				coin2 := sdk.NewCoin(denom2, sdk.NewInt(200))
+
+				// funds the escrow accounts to have balance
+				suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress, sdk.NewCoins(coin1)))
+				suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress, sdk.NewCoins(coin2)))
+
+				expectedEscrowed.Add(coin1, coin2)
 			},
-			math.NewInt(100),
 		},
 		{
 			"success: one native denom escrowed in two channels",
 			func() {
-				denom = sdk.DefaultBondDenom
+				denom := sdk.DefaultBondDenom
 				extraPath := NewTransferPath(suite.chainA, suite.chainB)
 				suite.coordinator.Setup(extraPath)
 
 				escrowAddress1 := transfertypes.GetEscrowAddress(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
 				escrowAddress2 := transfertypes.GetEscrowAddress(extraPath.EndpointA.ChannelConfig.PortID, extraPath.EndpointA.ChannelID)
-				coin1 := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
-				coin2 := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
+				coin1 := sdk.NewCoin(denom, sdk.NewInt(100))
+				coin2 := sdk.NewCoin(denom, sdk.NewInt(100))
 
 				// funds the escrow accounts to have balance
 				suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress1, sdk.NewCoins(coin1)))
 				suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress2, sdk.NewCoins(coin2)))
+
+				expectedEscrowed.Add(coin1, coin2)
 			},
-			math.NewInt(200),
 		},
 		{
 			"success: valid ibc denom escrowed in one channel",
 			func() {
 				escrowAddress := transfertypes.GetEscrowAddress(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
 				trace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, sdk.DefaultBondDenom))
-				coin := sdk.NewCoin(trace.IBCDenom(), sdk.NewInt(100))
-				denom = trace.IBCDenom()
+				denom := trace.IBCDenom()
+				coin := sdk.NewCoin(denom, sdk.NewInt(100))
 
 				suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), trace)
 
 				// funds the escrow account to have balance
 				suite.Require().NoError(banktestutil.FundAccount(suite.chainA.GetSimApp().BankKeeper, suite.chainA.GetContext(), escrowAddress, sdk.NewCoins(coin)))
+
+				expectedEscrowed.Add(coin)
 			},
-			sdk.NewInt(100),
 		},
 	}
 
@@ -196,8 +214,10 @@ func (suite *KeeperTestSuite) TestMigrateTotalEscrowForDenom() {
 			suite.Require().NoError(migrator.MigrateTotalEscrowForDenom(suite.chainA.GetContext()))
 
 			// check that the migration set the expected amount for both native and IBC tokens
-			amount := suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainA.GetContext(), denom)
-			suite.Require().Equal(tc.expectedEscrowAmt, amount)
+			for _, escrow := range expectedEscrowed {
+				amount := suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainA.GetContext(), escrow.GetDenom())
+				suite.Require().Equal(escrow.Amount, amount)
+			}
 		})
 	}
 }
diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go
@@ -384,8 +384,10 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 			err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)
 
 			// check total amount in escrow of received token denom on receiving chain
-			var denom string
-			var totalEscrow math.Int
+			var (
+				denom       string
+				totalEscrow math.Int
+			)
 			if tc.recvIsSource {
 				denom = sdk.DefaultBondDenom
 			} else {