Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix escrow proto E2E #3517

Merged
merged 13 commits into from
May 3, 2023
Merged

Fix escrow proto E2E #3517

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
01b343f
wip: experiement with string value for proto
chatton Apr 24, 2023
b1bf9b7
apply PR suggestions and fix unit tests
chatton Apr 25, 2023
2e026cd
added extension method to gRPC response
chatton Apr 25, 2023
9cda050
refactor: change response return type to sdk Coin
chatton Apr 26, 2023
555659a
Merge branch 'main' into escrow-proto-fix
chatton Apr 26, 2023
8c0dc24
Merge branch 'main' into escrow-proto-fix
damiannolan May 1, 2023
fa096ca
rm commented out proto import
damiannolan May 2, 2023
ed5f1e5
remove empty sdk.Coin literal in e2e/grpc_query
damiannolan May 2, 2023
102e2de
reference same type in relay.go in favour of creating new sdk.Coin
damiannolan May 2, 2023
15dc59a
Merge branch 'main' into escrow-proto-fix
damiannolan May 3, 2023
b5f831e
adding code comment regarding zero value coin return
damiannolan May 3, 2023
8a155c7
Merge branch 'main' into escrow-proto-fix
damiannolan May 3, 2023
fb063c2
fix typo, linter
damiannolan May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions e2e/tests/transfer/base_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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")
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions e2e/testsuite/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.NewCoin(denom, sdk.NewInt(0)), err
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

return res.Amount, nil
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/transfer/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to return a string, we can just do denomAmount.String(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this would be a bit nicer, I will play with this idea

Amount: amount,
}, nil
}
8 changes: 4 additions & 4 deletions modules/apps/transfer/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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(),
Expand Down Expand Up @@ -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)
}
Expand Down
18 changes: 9 additions & 9 deletions modules/apps/transfer/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -146,28 +146,28 @@ 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 {
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())
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

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.
Expand Down
10 changes: 5 additions & 5 deletions modules/apps/transfer/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the panic now happens in the sdk.NewCoin function instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the original behaviour is desirable, we can create a coin literal instead of using the NewCoin function.

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)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/migrations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
12 changes: 6 additions & 6 deletions modules/apps/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ func (k Keeper) sendTransfer(

// 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, sdk.NewCoin(token.GetDenom(), newTotalEscrow.Amount))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
} else {
labels = append(labels, telemetry.NewLabel(coretypes.LabelSource, "false"))

Expand Down Expand Up @@ -236,8 +236,8 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t

// 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, sdk.NewCoin(token.GetDenom(), newTotalEscrow.Amount))

defer func() {
if transferAmount.IsInt64() {
Expand Down Expand Up @@ -377,8 +377,8 @@ func (k Keeper) refundPacketToken(ctx sdk.Context, packet channeltypes.Packet, d

// 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, sdk.NewCoin(token.GetDenom(), newTotalEscrow.Amount))

return nil
}
Expand Down
Loading