From 428c7c3ac1850a1c7742fad8531c5a0885031a3d Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Mon, 15 May 2023 21:45:20 +0200 Subject: [PATCH 1/5] do not store 0 escrow amout --- modules/apps/transfer/keeper/keeper.go | 12 +++++++++--- modules/apps/transfer/keeper/keeper_test.go | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 63e2847a680..36dacaf0bd1 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -152,7 +152,7 @@ func (k Keeper) IterateDenomTraces(ctx sdk.Context, cb func(denomTrace types.Den 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 { + if len(bz) == 0 { return sdk.NewCoin(denom, sdk.ZeroInt()) } @@ -169,8 +169,14 @@ func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) { } store := ctx.KVStore(k.storeKey) - bz := k.cdc.MustMarshal(&sdk.IntProto{Int: coin.Amount}) - store.Set(types.TotalEscrowForDenomKey(coin.Denom), bz) + key := types.TotalEscrowForDenomKey(coin.Denom) + + if coin.Amount.IsZero() { + store.Delete(key) + } else { + 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. diff --git a/modules/apps/transfer/keeper/keeper_test.go b/modules/apps/transfer/keeper/keeper_test.go index c22489391cc..e8f5b090edb 100644 --- a/modules/apps/transfer/keeper/keeper_test.go +++ b/modules/apps/transfer/keeper/keeper_test.go @@ -71,6 +71,13 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { }, true, }, + { + "success: escrow amount 0 is not stored", + func() { + expAmount = math.ZeroInt() + }, + true, + }, { "failure: setter panics with negative escrow amount", func() { @@ -94,6 +101,15 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(denom, expAmount)) total := suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(ctx, denom) suite.Require().Equal(expAmount, total.Amount) + + storeKey := suite.chainA.GetSimApp().GetKey(types.ModuleName) + store := ctx.KVStore(storeKey) + key := types.TotalEscrowForDenomKey(denom) + if expAmount.IsZero() { + suite.Require().False(store.Has(key)) + } else { + suite.Require().True(store.Has(key)) + } } else { suite.Require().PanicsWithError("negative coin amount: -1", func() { suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(denom, expAmount)) From a8ed946f9902a6d50bac49c2dbf179ea21564023 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Mon, 15 May 2023 22:02:52 +0200 Subject: [PATCH 2/5] adapt success test --- modules/apps/transfer/keeper/keeper_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/transfer/keeper/keeper_test.go b/modules/apps/transfer/keeper/keeper_test.go index e8f5b090edb..235fa9cf9f6 100644 --- a/modules/apps/transfer/keeper/keeper_test.go +++ b/modules/apps/transfer/keeper/keeper_test.go @@ -60,7 +60,7 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { expPass bool }{ { - "success: with 0 escrow amount", + "success: with non-zero escrow amount", func() {}, true, }, @@ -92,7 +92,7 @@ func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() { suite.Run(tc.name, func() { suite.SetupTest() // reset - expAmount = math.ZeroInt() + expAmount = math.NewInt(100) ctx := suite.chainA.GetContext() tc.malleate() From 06d654125891663295b2203b835da6f568238414 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 16 May 2023 08:59:14 +0200 Subject: [PATCH 3/5] Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Jim Fasarakis-Hilliard --- modules/apps/transfer/keeper/keeper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 9ef85f5313f..57c44ff3991 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -175,7 +175,7 @@ func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) { store.Delete(key) } else { bz := k.cdc.MustMarshal(&sdk.IntProto{Int: coin.Amount}) - store.Set(types.TotalEscrowForDenomKey(coin.Denom), bz) + store.Set(key, bz) } } From 8ba66ed06860311eb2d8b081ee75d4f0ea62e473 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 16 May 2023 11:02:57 +0200 Subject: [PATCH 4/5] Update modules/apps/transfer/keeper/keeper.go Co-authored-by: Damian Nolan --- modules/apps/transfer/keeper/keeper.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 57c44ff3991..243b22e991a 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -173,10 +173,11 @@ func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) { if coin.Amount.IsZero() { store.Delete(key) - } else { - bz := k.cdc.MustMarshal(&sdk.IntProto{Int: coin.Amount}) - store.Set(key, bz) + return } + + bz := k.cdc.MustMarshal(&sdk.IntProto{Int: coin.Amount}) + store.Set(key, bz) } // GetAllTotalEscrowed returns the escrow information for all the denominations. From 826319cb78a17997ae6e61a2c43192125bb567a8 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 16 May 2023 11:53:05 +0200 Subject: [PATCH 5/5] add comments --- modules/apps/transfer/keeper/keeper.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 243b22e991a..da2d2a6a05c 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -163,6 +163,8 @@ func (k Keeper) GetTotalEscrowForDenom(ctx sdk.Context, denom string) sdk.Coin { } // SetTotalEscrowForDenom stores the total amount of source chain tokens that are in escrow. +// Amount is stored in state if and only if it is not equal to zero. The function will panic +// if the amount is negative. func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) { if coin.Amount.IsNegative() { panic(fmt.Sprintf("amount cannot be negative: %s", coin.Amount)) @@ -172,10 +174,10 @@ func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) { key := types.TotalEscrowForDenomKey(coin.Denom) if coin.Amount.IsZero() { - store.Delete(key) + store.Delete(key) // delete the key since Cosmos SDK x/bank module will prune any non-zero balances return } - + bz := k.cdc.MustMarshal(&sdk.IntProto{Int: coin.Amount}) store.Set(key, bz) }