Skip to content

Commit

Permalink
Fix Timeout Handling (cosmos#6118)
Browse files Browse the repository at this point in the history 
* fix timeout

* remove print statement

* fix tests

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Alexander Bezobchuk <[email protected]>
  • Loading branch information
@AdityaSripal @fedekunze @alexanderbez
3 people authored May 5, 2020
1 parent 6804204 commit f2a331f
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 27 deletions.
10 changes: 5 additions & 5 deletions x/ibc/04-channel/keeper/timeout.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,6 @@ func (k Keeper) TimeoutPacket(
return nil, sdkerrors.Wrap(types.ErrPacketTimeout, "packet timeout has not been reached for height or timestamp")
}

// check that packet has not been received
if nextSequenceRecv >= packet.GetSequence() {
return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet already received")
}

commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

// verify we sent the packet and haven't cleared it out yet
Expand All @@ -91,6 +86,11 @@ func (k Keeper) TimeoutPacket(

switch channel.Ordering {
case exported.ORDERED:
// check that packet has not been received
if nextSequenceRecv > packet.GetSequence() {
return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet already received")
}

// check that the recv sequence is as claimed
err = k.connectionKeeper.VerifyNextSequenceRecv(
ctx, connectionEnd, proofHeight, proof,
Expand Down
4 changes: 4 additions & 0 deletions x/ibc/20-transfer/keeper/keeper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ func (suite *KeeperTestSuite) SetupTest() {
suite.chainA = NewTestChain(testClientIDA)
suite.chainB = NewTestChain(testClientIDB)

// reset prefixCoins at each setup
prefixCoins = sdk.NewCoins(sdk.NewCoin("bank/firstchannel/atom", sdk.NewInt(100)))
prefixCoins2 = sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100)))

suite.cdc = suite.chainA.App.Codec()
}

Expand Down
4 changes: 2 additions & 2 deletions x/ibc/20-transfer/keeper/relay.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ func (k Keeper) refundPacketAmount(ctx sdk.Context, packet channel.Packet, data
}

// check the denom prefix
prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel())
prefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel())
source := strings.HasPrefix(data.Amount[0].Denom, prefix)

// decode the sender address
Expand All @@ -229,7 +229,7 @@ func (k Keeper) refundPacketAmount(ctx sdk.Context, packet channel.Packet, data
}

// unescrow tokens back to sender
escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel())
escrowAddress := types.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel())
return k.bankKeeper.SendCoins(ctx, escrowAddress, sender, coins)
}

Expand Down
37 changes: 18 additions & 19 deletions x/ibc/20-transfer/keeper/relay_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
// - receiving chain
{"incorrect dest prefix on coin denom",
func() {
data.Amount = prefixCoins2
data.Amount = prefixCoins
}, false},
{"success receive from external chain",
func() {
Expand Down Expand Up @@ -164,8 +164,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
// TestOnAcknowledgementPacket tests that successful acknowledgement is a no-op
// and failure acknowledment leads to refund
func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() {
data := types.NewFungibleTokenPacketData(prefixCoins, testAddr1.String(), testAddr2.String())
testCoins2 := sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100)))
data := types.NewFungibleTokenPacketData(prefixCoins2, testAddr1.String(), testAddr2.String())

successAck := types.FungibleTokenPacketAcknowledgement{
Success: true,
Expand All @@ -186,13 +185,13 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() {
func() {}, true, true},
{"successful refund from source chain", failedAck,
func() {
escrow := types.GetEscrowAddress(testPort2, testChannel2)
escrow := types.GetEscrowAddress(testPort1, testChannel1)
_, err := suite.chainA.App.BankKeeper.AddCoins(suite.chainA.GetContext(), escrow, sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(100))))
suite.Require().NoError(err)
}, true, false},
{"successful refund from external chain", failedAck,
func() {
data.Amount = testCoins2
data.Amount = prefixCoins
}, false, false},
}

Expand All @@ -204,18 +203,18 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() {
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.SetupTest() // reset

preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, prefixCoins[0].Denom)

tc.malleate()

var denom string
if tc.source {
prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel())
denom = prefixCoins[0].Denom[len(prefix):]
prefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel())
denom = prefixCoins2[0].Denom[len(prefix):]
} else {
denom = data.Amount[0].Denom
}

preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, denom)

err := suite.chainA.App.TransferKeeper.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, data, tc.ack)
suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg)

Expand All @@ -225,16 +224,16 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() {
if tc.success {
suite.Require().Equal(sdk.ZeroInt(), deltaAmount, "successful ack changed balance")
} else {
suite.Require().Equal(prefixCoins[0].Amount, deltaAmount, "failed ack did not trigger refund")
suite.Require().Equal(prefixCoins2[0].Amount, deltaAmount, "failed ack did not trigger refund")
}
})
}
}

// TestOnTimeoutPacket test private refundPacket function since it is a simple wrapper over it
func (suite *KeeperTestSuite) TestOnTimeoutPacket() {
data := types.NewFungibleTokenPacketData(prefixCoins, testAddr1.String(), testAddr2.String())
testCoins2 := sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100)))
data := types.NewFungibleTokenPacketData(prefixCoins2, testAddr1.String(), testAddr2.String())
testCoins2 := sdk.NewCoins(sdk.NewCoin("bank/firstchannel/atom", sdk.NewInt(100)))

testCases := []struct {
msg string
Expand All @@ -244,7 +243,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() {
}{
{"successful timeout from source chain",
func() {
escrow := types.GetEscrowAddress(testPort2, testChannel2)
escrow := types.GetEscrowAddress(testPort1, testChannel1)
_, err := suite.chainA.App.BankKeeper.AddCoins(suite.chainA.GetContext(), escrow, sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(100))))
suite.Require().NoError(err)
}, true, true},
Expand All @@ -261,7 +260,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() {
}, true, false},
{"mint failed",
func() {
data.Amount[0].Denom = prefixCoins[0].Denom
data.Amount[0].Denom = prefixCoins2[0].Denom
data.Amount[0].Amount = sdk.ZeroInt()
}, true, false},
}
Expand All @@ -274,26 +273,26 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacket() {
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.SetupTest() // reset

preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, prefixCoins[0].Denom)

tc.malleate()

var denom string
if tc.source {
prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel())
denom = prefixCoins[0].Denom[len(prefix):]
prefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel())
denom = prefixCoins2[0].Denom[len(prefix):]
} else {
denom = data.Amount[0].Denom
}

preCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, denom)

err := suite.chainA.App.TransferKeeper.OnTimeoutPacket(suite.chainA.GetContext(), packet, data)

postCoin := suite.chainA.App.BankKeeper.GetBalance(suite.chainA.GetContext(), testAddr1, denom)
deltaAmount := postCoin.Amount.Sub(preCoin.Amount)

if tc.expPass {
suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg)
suite.Require().Equal(prefixCoins[0].Amount.Int64(), deltaAmount.Int64(), "failed ack did not trigger refund")
suite.Require().Equal(prefixCoins2[0].Amount.Int64(), deltaAmount.Int64(), "successful timeout did not trigger refund")
} else {
suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg)
}
Expand Down
2 changes: 1 addition & 1 deletion x/ibc/20-transfer/module.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ func (am AppModule) OnTimeoutPacket(
packet channeltypes.Packet,
) (*sdk.Result, error) {
var data FungibleTokenPacketData
if err := types.ModuleCdc.UnmarshalBinaryBare(packet.GetData(), &data); err != nil {
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
}
// refund tokens
Expand Down

0 comments on commit f2a331f

Please sign in to comment.