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 ibc-transfer receiving denom bug #8119

Merged
merged 4 commits into from
Dec 9, 2020
Merged

fix ibc-transfer receiving denom bug #8119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e2dfdf1
fix receiving denom bug
colin-axner Dec 9, 2020
56a48dc
Update x/ibc/applications/transfer/keeper/relay.go
alexanderbez Dec 9, 2020
b5aa99b
add testing sending 2 hops and back one
colin-axner Dec 9, 2020
00c264f
Merge branch 'colin/hot-fix' of github.com:cosmos/cosmos-sdk into col…
colin-axner Dec 9, 2020
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
60 changes: 45 additions & 15 deletions x/ibc/applications/transfer/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,23 @@ type TransferTestSuite struct {
// testing chains used for convenience and readability
chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain
}

func (suite *TransferTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(2))
}

// constructs a send from chainA to chainB on the established channel/connection
// and sends the same coin back from chainB to chainA.
func (suite *TransferTestSuite) TestHandleMsgTransfer() {
// setup between chainA and chainB
clientA, clientB, connA, connB := suite.coordinator.SetupClientConnections(suite.chainA, suite.chainB, exported.Tendermint)
channelA, channelB := suite.coordinator.CreateTransferChannels(suite.chainA, suite.chainB, connA, connB, channeltypes.UNORDERED)
originalBalance := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)
// originalBalance := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)
timeoutHeight := clienttypes.NewHeight(0, 110)

coinToSendToB := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100))
Expand All @@ -56,35 +59,62 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
voucherDenomTrace := types.ParseDenomTrace(types.GetPrefixedDenom(packet.GetDestPort(), packet.GetDestChannel(), sdk.DefaultBondDenom))
balance := suite.chainB.App.BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), voucherDenomTrace.IBCDenom())

coinToSendBackToA := types.GetTransferCoin(channelB.PortID, channelB.ID, sdk.DefaultBondDenom, 100)
suite.Require().Equal(coinToSendBackToA, balance)
coinSentFromAToB := types.GetTransferCoin(channelB.PortID, channelB.ID, sdk.DefaultBondDenom, 100)
suite.Require().Equal(coinSentFromAToB, balance)

// send from chainB back to chainA
msg = types.NewMsgTransfer(channelB.PortID, channelB.ID, coinToSendBackToA, suite.chainB.SenderAccount.GetAddress(), suite.chainA.SenderAccount.GetAddress().String(), timeoutHeight, 0)
// setup between chainB to chainC
clientOnBForC, clientOnCForB, connOnBForC, connOnCForB := suite.coordinator.SetupClientConnections(suite.chainB, suite.chainC, exported.Tendermint)
channelOnBForC, channelOnCForB := suite.coordinator.CreateTransferChannels(suite.chainB, suite.chainC, connOnBForC, connOnCForB, channeltypes.UNORDERED)

err = suite.coordinator.SendMsg(suite.chainB, suite.chainA, clientA, msg)
// send from chainB to chainC
msg = types.NewMsgTransfer(channelOnBForC.PortID, channelOnBForC.ID, coinSentFromAToB, suite.chainB.SenderAccount.GetAddress(), suite.chainC.SenderAccount.GetAddress().String(), timeoutHeight, 0)

err = suite.coordinator.SendMsg(suite.chainB, suite.chainC, clientOnCForB, msg)
suite.Require().NoError(err) // message committed

// relay send
// NOTE: fungible token is prefixed with the full trace in order to verify the packet commitment
fullDenomPath := types.GetPrefixedDenom(channelOnCForB.PortID, channelOnCForB.ID, voucherDenomTrace.GetFullDenomPath())
fungibleTokenPacket = types.NewFungibleTokenPacketData(voucherDenomTrace.GetFullDenomPath(), coinSentFromAToB.Amount.Uint64(), suite.chainB.SenderAccount.GetAddress().String(), suite.chainC.SenderAccount.GetAddress().String())
packet = channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelOnBForC.PortID, channelOnBForC.ID, channelOnCForB.PortID, channelOnCForB.ID, timeoutHeight, 0)
err = suite.coordinator.RelayPacket(suite.chainB, suite.chainC, clientOnBForC, clientOnCForB, packet, ack.GetBytes())
suite.Require().NoError(err) // relay committed

coinSentFromBToC := sdk.NewInt64Coin(types.ParseDenomTrace(fullDenomPath).IBCDenom(), 100)
balance = suite.chainC.App.BankKeeper.GetBalance(suite.chainC.GetContext(), suite.chainC.SenderAccount.GetAddress(), coinSentFromBToC.Denom)

// check that the balance is updated on chainC
suite.Require().Equal(coinSentFromBToC, balance)

// check that balance on chain B is empty
balance = suite.chainB.App.BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), coinSentFromBToC.Denom)
suite.Require().Zero(balance.Amount.Int64())

// send from chainC back to chainB
msg = types.NewMsgTransfer(channelOnCForB.PortID, channelOnCForB.ID, coinSentFromBToC, suite.chainC.SenderAccount.GetAddress(), suite.chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0)

err = suite.coordinator.SendMsg(suite.chainC, suite.chainB, clientOnBForC, msg)
suite.Require().NoError(err) // message committed

// relay send
// NOTE: fungible token is prefixed with the full trace in order to verify the packet commitment
voucherDenom := voucherDenomTrace.GetPrefix() + voucherDenomTrace.BaseDenom
fungibleTokenPacket = types.NewFungibleTokenPacketData(voucherDenom, coinToSendBackToA.Amount.Uint64(), suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
packet = channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelB.PortID, channelB.ID, channelA.PortID, channelA.ID, timeoutHeight, 0)
err = suite.coordinator.RelayPacket(suite.chainB, suite.chainA, clientB, clientA, packet, ack.GetBytes())
fungibleTokenPacket = types.NewFungibleTokenPacketData(fullDenomPath, coinSentFromBToC.Amount.Uint64(), suite.chainC.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String())
packet = channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelOnCForB.PortID, channelOnCForB.ID, channelOnBForC.PortID, channelOnBForC.ID, timeoutHeight, 0)
err = suite.coordinator.RelayPacket(suite.chainC, suite.chainB, clientOnCForB, clientOnBForC, packet, ack.GetBytes())
suite.Require().NoError(err) // relay committed

balance = suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)
balance = suite.chainB.App.BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), coinSentFromAToB.Denom)

// check that the balance on chainA returned back to the original state
suite.Require().Equal(originalBalance, balance)
suite.Require().Equal(coinSentFromAToB, balance)

// check that module account escrow address is empty
escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel())
balance = suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom)
balance = suite.chainB.App.BankKeeper.GetBalance(suite.chainB.GetContext(), escrowAddress, sdk.DefaultBondDenom)
suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.ZeroInt()), balance)

// check that balance on chain B is empty
balance = suite.chainB.App.BankKeeper.GetBalance(suite.chainB.GetContext(), suite.chainB.SenderAccount.GetAddress(), voucherDenomTrace.IBCDenom())
balance = suite.chainC.App.BankKeeper.GetBalance(suite.chainC.GetContext(), suite.chainC.SenderAccount.GetAddress(), voucherDenomTrace.IBCDenom())
suite.Require().Zero(balance.Amount.Int64())
}

Expand Down
12 changes: 11 additions & 1 deletion x/ibc/applications/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,17 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
// remove prefix added by sender chain
voucherPrefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel())
unprefixedDenom := data.Denom[len(voucherPrefix):]
token := sdk.NewCoin(unprefixedDenom, sdk.NewIntFromUint64(data.Amount))

// coin denomination used in sending from the escrow address
denom := unprefixedDenom

// The denomination used to send the coins is either the native denom or the hash of the path
// if the denomination is not native.
denomTrace := types.ParseDenomTrace(unprefixedDenom)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, we could just check if the denom contains a separator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this happens in ParseDenomTrace?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the data.Denom is already validated by the packet so just a strings.Contains(unprefixedDenom, "/") would suffice for checking is the denom is non native

if denomTrace.Path != "" {
denom = denomTrace.IBCDenom()
}
token := sdk.NewCoin(denom, sdk.NewIntFromUint64(data.Amount))

// unescrow tokens
escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel())
Expand Down