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

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

Merged
merged 2 commits into from
May 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec399ac
some more review comments
Apr 24, 2023
107433e
Rename pathAndEscrowAMount to pathAndEscrowAmount
DimitrisJim May 2, 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
6 changes: 3 additions & 3 deletions modules/apps/transfer/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 1 addition & 6 deletions modules/apps/transfer/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
56 changes: 38 additions & 18 deletions modules/apps/transfer/keeper/migrations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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),
},
}

Expand All @@ -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)
}
})
}
}
6 changes: 4 additions & 2 deletions modules/apps/transfer/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down