modify total escrow to take in sdk.Coin in function APIs/proto (#3517)

* wip: experiement with string value for proto

* apply PR suggestions and fix unit tests

* added extension method to gRPC response

* refactor, change response return type to sdk Coin

* rm commented out proto import

* remove empty sdk.Coin literal in e2e/grpc_query

* reference same type in relay.go in favour of creating new sdk.Coin

* adding code comment regarding zero value coin return

* fix typo, linter

---------

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

e2e/tests/transfer/base_test.go

@@ -6,7 +6,8 @@ import (
 	"testing"
 	"time"
 
-	// "cosmossdk.io/math"
+	"cosmossdk.io/math"
+	sdk "github.com/cosmos/cosmos-sdk/types"
 	paramsproposaltypes "github.com/cosmos/cosmos-sdk/x/params/types/proposal"
 	"github.com/strangelove-ventures/interchaintest/v7/ibc"
 	test "github.com/strangelove-ventures/interchaintest/v7/testutil"
@@ -90,11 +91,11 @@ func (s *TransferTestSuite) TestMsgTransfer_Succeeds_Nonincentivized() {
 		expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount
 		s.Require().Equal(expected, actualBalance)
 
-		// actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
-		// s.Require().NoError(err)
+		actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
+		s.Require().NoError(err)
 
-		// expectedTotalEscrow := math.NewInt(testvalues.IBCTransferAmount)
-		// s.Require().Equal(expectedTotalEscrow, actualTotalEscrow)
+		expectedTotalEscrow := sdk.NewCoin(chainADenom, math.NewInt(testvalues.IBCTransferAmount))
+		s.Require().Equal(expectedTotalEscrow, actualTotalEscrow)
 	})
 
 	t.Run("start relayer", func(t *testing.T) {
@@ -125,9 +126,9 @@ func (s *TransferTestSuite) TestMsgTransfer_Succeeds_Nonincentivized() {
 
 		s.Require().Equal(int64(0), actualBalance)
 
-		// actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainB, chainBIBCToken.IBCDenom())
-		// s.Require().NoError(err)
-		// s.Require().Equal(math.ZeroInt(), actualTotalEscrow) // total escrow is zero because sending chain is not source for tokens
+		actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainB, chainBIBCToken.IBCDenom())
+		s.Require().NoError(err)
+		s.Require().Equal(sdk.NewCoin(chainBIBCToken.IBCDenom(), sdk.NewInt(0)), actualTotalEscrow) // total escrow is zero because sending chain is not source for tokens
 	})
 
 	s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")
@@ -142,11 +143,11 @@ func (s *TransferTestSuite) TestMsgTransfer_Succeeds_Nonincentivized() {
 		s.Require().Equal(expected, actualBalance)
 	})
 
-	// t.Run("tokens are un-escrowed", func(t *testing.T) {
-	// 	actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
-	// 	s.Require().NoError(err)
-	// 	s.Require().Equal(math.ZeroInt(), actualTotalEscrow) // total escrow is zero because tokens have come back
-	// })
+	t.Run("tokens are un-escrowed", func(t *testing.T) {
+		actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
+		s.Require().NoError(err)
+		s.Require().Equal(sdk.NewCoin(chainADenom, sdk.NewInt(0)), actualTotalEscrow) // total escrow is zero because tokens have come back
+	})
 }
 
 // TestMsgTransfer_Fails_InvalidAddress attempts to send an IBC transfer to an invalid address and ensures

e2e/testsuite/grpc_query.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"sort"
 
-	"cosmossdk.io/math"
 	tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
 	"github.com/cosmos/cosmos-sdk/client/grpc/tmservice"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -163,13 +162,14 @@ func (s *E2ETestSuite) QueryPacketCommitment(ctx context.Context, chain ibc.Chai
 }
 
 // QueryTotalEscrowForDenom queries the total amount of tokens in escrow for a denom
-func (s *E2ETestSuite) QueryTotalEscrowForDenom(ctx context.Context, chain ibc.Chain, denom string) (math.Int, error) {
+func (s *E2ETestSuite) QueryTotalEscrowForDenom(ctx context.Context, chain ibc.Chain, denom string) (sdk.Coin, error) {
 	queryClient := s.GetChainGRCPClients(chain).TransferQueryClient
 	res, err := queryClient.TotalEscrowForDenom(ctx, &transfertypes.QueryTotalEscrowForDenomRequest{
 		Denom: denom,
 	})
+
 	if err != nil {
-		return math.ZeroInt(), err
+		return sdk.Coin{}, err
 	}
 
 	return res.Amount, nil

modules/apps/transfer/keeper/genesis.go

@@ -32,7 +32,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) {
 	// Every denom will have only one total escrow amount, since any
 	// duplicate entry will fail validation in Validate of GenesisState
 	for _, denomEscrow := range state.TotalEscrowed {
-		k.SetTotalEscrowForDenom(ctx, denomEscrow.Denom, denomEscrow.Amount)
+		k.SetTotalEscrowForDenom(ctx, denomEscrow)
 	}
 }
 

modules/apps/transfer/keeper/genesis_test.go

@@ -41,7 +41,7 @@ func (suite *KeeperTestSuite) TestGenesis() {
 		amount, ok := math.NewIntFromString(pathAndEscrowMount.escrow)
 		suite.Require().True(ok)
 		escrows = append(sdk.NewCoins(sdk.NewCoin(denom, amount)), escrows...)
-		suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), denom, amount)
+		suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.NewCoin(denom, amount))
 	}
 
 	genesis := suite.chainA.GetSimApp().TransferKeeper.ExportGenesis(suite.chainA.GetContext())

modules/apps/transfer/keeper/grpc_query.go

@@ -134,9 +134,9 @@ func (k Keeper) TotalEscrowForDenom(c context.Context, req *types.QueryTotalEscr
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
 
-	denomAmount := k.GetTotalEscrowForDenom(ctx, req.Denom)
+	amount := k.GetTotalEscrowForDenom(ctx, req.Denom)
 
 	return &types.QueryTotalEscrowForDenomResponse{
-		Amount: denomAmount,
+		Amount: amount,
 	}, nil
 }

modules/apps/transfer/keeper/grpc_query_test.go

@@ -282,7 +282,7 @@ func (suite *KeeperTestSuite) TestTotalEscrowForDenom() {
 				}
 
 				expEscrowAmount = math.NewInt(100)
-				suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.DefaultBondDenom, expEscrowAmount)
+				suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.NewCoin(sdk.DefaultBondDenom, expEscrowAmount))
 			},
 			true,
 		},
@@ -297,7 +297,7 @@ func (suite *KeeperTestSuite) TestTotalEscrowForDenom() {
 				suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), denomTrace)
 				expEscrowAmount, ok := math.NewIntFromString("100000000000000000000")
 				suite.Require().True(ok)
-				suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.DefaultBondDenom, expEscrowAmount)
+				suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.NewCoin(sdk.DefaultBondDenom, expEscrowAmount))
 
 				req = &types.QueryTotalEscrowForDenomRequest{
 					Denom: denomTrace.IBCDenom(),
@@ -343,15 +343,15 @@ func (suite *KeeperTestSuite) TestTotalEscrowForDenom() {
 		suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
 			suite.SetupTest() // reset
 
-			expEscrowAmount = math.ZeroInt()
+			expEscrowAmount = sdk.ZeroInt()
 			tc.malleate()
 			ctx := sdk.WrapSDKContext(suite.chainA.GetContext())
 
 			res, err := suite.chainA.GetSimApp().TransferKeeper.TotalEscrowForDenom(ctx, req)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
-				suite.Require().Equal(expEscrowAmount, res.Amount)
+				suite.Require().Equal(expEscrowAmount, res.Amount.Amount)
 			} else {
 				suite.Require().Error(err)
 			}

modules/apps/transfer/keeper/keeper.go

@@ -4,14 +4,14 @@ import (
 	"fmt"
 	"strings"
 
-	"cosmossdk.io/math"
 	tmbytes "github.com/cometbft/cometbft/libs/bytes"
 	"github.com/cometbft/cometbft/libs/log"
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
+
 	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 
 	"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
@@ -146,28 +146,31 @@ func (k Keeper) IterateDenomTraces(ctx sdk.Context, cb func(denomTrace types.Den
 
 // GetTotalEscrowForDenom gets the total amount of source chain tokens that
 // are in escrow, keyed by the denomination.
-func (k Keeper) GetTotalEscrowForDenom(ctx sdk.Context, denom string) math.Int {
+//
+// NOTE: if there is no value stored in state for the provided denom then a new Coin is returned for the denom with an initial value of zero.
+// This accommodates callers to simply call `Add()` on the returned Coin as an empty Coin literal (e.g. sdk.Coin{}) will trigger a panic due to the absence of a denom.
+func (k Keeper) GetTotalEscrowForDenom(ctx sdk.Context, denom string) sdk.Coin {
 	store := ctx.KVStore(k.storeKey)
 	bz := store.Get(types.TotalEscrowForDenomKey(denom))
 	if bz == nil {
-		return math.ZeroInt()
+		return sdk.NewCoin(denom, sdk.ZeroInt())
 	}
 
 	amount := sdk.IntProto{}
 	k.cdc.MustUnmarshal(bz, &amount)
 
-	return amount.Int
+	return sdk.NewCoin(denom, amount.Int)
 }
 
 // SetTotalEscrowForDenom stores the total amount of source chain tokens that are in escrow.
-func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, denom string, amount math.Int) {
-	if amount.IsNegative() {
-		panic(fmt.Sprintf("amount cannot be negative: %s", amount))
+func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) {
+	if coin.Amount.IsNegative() {
+		panic(fmt.Sprintf("amount cannot be negative: %s", coin.Amount))
 	}
 
 	store := ctx.KVStore(k.storeKey)
-	bz := k.cdc.MustMarshal(&sdk.IntProto{Int: amount})
-	store.Set(types.TotalEscrowForDenomKey(denom), bz)
+	bz := k.cdc.MustMarshal(&sdk.IntProto{Int: coin.Amount})
+	store.Set(types.TotalEscrowForDenomKey(coin.Denom), bz)
 }
 
 // GetAllTotalEscrowed returns the escrow information for all the denominations.

modules/apps/transfer/keeper/keeper_test.go

@@ -91,15 +91,15 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() {
 			tc.malleate()
 
 			if tc.expPass {
-				suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, denom, expAmount)
+				suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(denom, expAmount))
 				total := suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(ctx, denom)
-				suite.Require().Equal(expAmount, total)
+				suite.Require().Equal(expAmount, total.Amount)
 			} else {
-				suite.Require().PanicsWithValue("amount cannot be negative: -1", func() {
-					suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, denom, expAmount)
+				suite.Require().PanicsWithError("negative coin amount: -1", func() {
+					suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(denom, expAmount))
 				})
 				total := suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(ctx, denom)
-				suite.Require().Equal(math.ZeroInt(), total)
+				suite.Require().Equal(math.ZeroInt(), total.Amount)
 			}
 		})
 	}

modules/apps/transfer/keeper/migrations.go

@@ -67,7 +67,7 @@ func (m Migrator) MigrateTotalEscrowForDenom(ctx sdk.Context) error {
 	}
 
 	for _, totalEscrow := range totalEscrowed {
-		m.keeper.SetTotalEscrowForDenom(ctx, totalEscrow.Denom, totalEscrow.Amount)
+		m.keeper.SetTotalEscrowForDenom(ctx, totalEscrow)
 	}
 
 	return nil

modules/apps/transfer/keeper/migrations_test.go

@@ -197,7 +197,7 @@ func (suite *KeeperTestSuite) TestMigrateTotalEscrowForDenom() {
 
 			// 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)
+			suite.Require().Equal(tc.expectedEscrowAmt, amount.Amount)
 		})
 	}
 }

modules/apps/transfer/keeper/relay.go

@@ -376,8 +376,8 @@ func (k Keeper) escrowToken(ctx sdk.Context, sender, escrowAddress sdk.AccAddres
 
 	// track the total amount in escrow keyed by denomination to allow for efficient iteration
 	currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
-	newTotalEscrow := currentTotalEscrow.Add(token.Amount)
-	k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)
+	newTotalEscrow := currentTotalEscrow.Add(token)
+	k.SetTotalEscrowForDenom(ctx, newTotalEscrow)
 
 	return nil
 }
@@ -395,8 +395,8 @@ func (k Keeper) unescrowToken(ctx sdk.Context, escrowAddress, receiver sdk.AccAd
 
 	// track the total amount in escrow keyed by denomination to allow for efficient iteration
 	currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
-	newTotalEscrow := currentTotalEscrow.Sub(token.Amount)
-	k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)
+	newTotalEscrow := currentTotalEscrow.Sub(token)
+	k.SetTotalEscrowForDenom(ctx, newTotalEscrow)
 
 	return nil
 }