From 013f883ea8fa5030e0a8b726fe9e5cc3a3e1b6e8 Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 13 Jun 2024 11:25:32 +0100 Subject: [PATCH 01/28] Create ontimeoutpacket test for forwarding --- .../transfer/keeper/relay_forwarding_test.go | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 4647e87b554..3028da0fdbc 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -1,6 +1,7 @@ package keeper_test import ( + "errors" "fmt" sdkmath "cosmossdk.io/math" @@ -612,3 +613,109 @@ Test async ack is properly relayed to middle hop after forwarding transfer compl // TODO Tiemout during forwarding after middle hop execution reverts properly the state changes */ + +func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { + path1 := ibctesting.NewTransferPath(suite.chainA, suite.chainB) + path1.Setup() + + path2 := ibctesting.NewTransferPath(suite.chainB, suite.chainC) + path2.Setup() + + amount := sdkmath.NewInt(100) + coin := sdk.NewCoin(sdk.DefaultBondDenom, amount) + sender := suite.chainA.SenderAccounts[0].SenderAccount + receiver := suite.chainC.SenderAccounts[0].SenderAccount // Conferm whether it's B or C + + transferMsg := types.NewMsgTransfer( + path1.EndpointA.ChannelConfig.PortID, + path1.EndpointA.ChannelID, + sdk.NewCoins(coin), + sender.GetAddress().String(), + receiver.GetAddress().String(), + suite.chainA.GetTimeoutHeight(), + 0, "", + &types.ForwardingInfo{ + Hops: []types.Hop{ + { + PortId: path2.EndpointA.ChannelConfig.PortID, + ChannelId: path2.EndpointA.ChannelID, + }, + }, + }, + ) + + result, err := suite.chainA.SendMsgs(transferMsg) + suite.Require().NoError(err) // message committed + + // parse the packet from result events and recv packet on chainB + packet, err := ibctesting.ParsePacketFromEvents(result.Events) + suite.Require().NoError(err) + suite.Require().NotNil(packet) + + err = path1.EndpointB.UpdateClient() + suite.Require().NoError(err) + + result, err = path1.EndpointB.RecvPacketWithResult(packet) + suite.Require().NoError(err) + suite.Require().NotNil(result) + + err = path2.EndpointA.UpdateClient() + suite.Require().NoError(err) + + err = path2.EndpointB.UpdateClient() + suite.Require().NoError(err) + + // packet, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, 1) + // suite.Require().True(found) + + // bytes := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetPacketCommitment(suite.chainB.GetContext(), path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, 1) + + // packet.SourceChannel = path2.EndpointA.ChannelID + + // bytes2 := channeltypes.CommitPacket(suite.chainB.Codec, packet) + // suite.Require().Equal(bytes, bytes2) + // + // [{Denom:{Base:stake Trace:[]} Amount:100}] + + address := types.GetForwardAddress(packet.DestinationPort, packet.DestinationChannel).String() + data := types.NewFungibleTokenPacketDataV2( + []types.Token{ + { + Denom: types.NewDenom("stake", types.NewTrace(path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID)), + Amount: "100", + }, + }, + address, + suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), + "", nil, + ).GetBytes() + + packet = channeltypes.NewPacket( + data, + 1, + path2.EndpointA.ChannelConfig.PortID, + path2.EndpointA.ChannelID, + path2.EndpointB.ChannelConfig.PortID, + path2.EndpointB.ChannelID, + packet.TimeoutHeight, + packet.TimeoutTimestamp) + + // retrieve module callbacks + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path2.EndpointA.ChannelConfig.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module) + suite.Require().True(ok) + + err = cbs.OnTimeoutPacket(suite.chainB.GetContext(), packet, nil) + suite.Require().NoError(err) + + res := suite.chainB.GetAcknowledgement(packet) + suite.Require().NotNil(res, "chainB does not have an ack") + + ack := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out")) + ackbytes := channeltypes.CommitAcknowledgement(ack.Acknowledgement()) + suite.Require().Equal(ackbytes, res) +} + +//Test that fails verification if no forwarding address From 83f4582724c664733e5b7ce9ea4e37540057c197 Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 13 Jun 2024 11:43:26 +0100 Subject: [PATCH 02/28] Propagate ack on A --- .../transfer/keeper/relay_forwarding_test.go | 47 +++++++++++++++---- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 3028da0fdbc..4bb840092e6 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -626,6 +626,15 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { sender := suite.chainA.SenderAccounts[0].SenderAccount receiver := suite.chainC.SenderAccounts[0].SenderAccount // Conferm whether it's B or C + hops := &types.ForwardingInfo{ + Hops: []types.Hop{ + { + PortId: path2.EndpointA.ChannelConfig.PortID, + ChannelId: path2.EndpointA.ChannelID, + }, + }, + } + transferMsg := types.NewMsgTransfer( path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, @@ -634,14 +643,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { receiver.GetAddress().String(), suite.chainA.GetTimeoutHeight(), 0, "", - &types.ForwardingInfo{ - Hops: []types.Hop{ - { - PortId: path2.EndpointA.ChannelConfig.PortID, - ChannelId: path2.EndpointA.ChannelID, - }, - }, - }, + hops, ) result, err := suite.chainA.SendMsgs(transferMsg) @@ -688,10 +690,10 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { address, suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), "", nil, - ).GetBytes() + ) packet = channeltypes.NewPacket( - data, + data.GetBytes(), 1, path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, @@ -716,6 +718,31 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { ack := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out")) ackbytes := channeltypes.CommitAcknowledgement(ack.Acknowledgement()) suite.Require().Equal(ackbytes, res) + + data = types.NewFungibleTokenPacketDataV2( + []types.Token{ + { + Denom: types.NewDenom("stake"), + Amount: "100", + }, + }, + suite.chainA.SenderAccounts[0].SenderAccount.GetAddress().String(), + suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), + "", hops, + ) + + packet = channeltypes.NewPacket( + data.GetBytes(), + 1, + path1.EndpointA.ChannelConfig.PortID, + path1.EndpointA.ChannelID, + path1.EndpointB.ChannelConfig.PortID, + path1.EndpointB.ChannelID, + packet.TimeoutHeight, + packet.TimeoutTimestamp) + + err = suite.chainA.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, data, ack) + suite.Require().NoError(err) } //Test that fails verification if no forwarding address From 59b93ed32ca39ba367de207c7937bc78d98b1b25 Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 13 Jun 2024 15:15:39 +0100 Subject: [PATCH 03/28] Refactoring --- .../transfer/keeper/relay_forwarding_test.go | 94 ++++++++++++------- 1 file changed, 61 insertions(+), 33 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 4bb840092e6..54353545ee0 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -614,30 +614,61 @@ Test async ack is properly relayed to middle hop after forwarding transfer compl Tiemout during forwarding after middle hop execution reverts properly the state changes */ -func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { - path1 := ibctesting.NewTransferPath(suite.chainA, suite.chainB) +func (suite *KeeperTestSuite) setUpForwardingPaths() (path1, path2 *ibctesting.Path) { + path1 = ibctesting.NewTransferPath(suite.chainA, suite.chainB) + path2 = ibctesting.NewTransferPath(suite.chainB, suite.chainC) path1.Setup() - - path2 := ibctesting.NewTransferPath(suite.chainB, suite.chainC) path2.Setup() + return path1, path2 +} + +type amountType int + +const ( + escrow amountType = iota + balance +) + +func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, balanceType amountType, amount sdkmath.Int, denom string, errorMsg string) { + var total sdk.Coin + switch balanceType { + case escrow: + total = chain.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(chain.GetContext(), denom) + case balance: + total = chain.GetSimApp().BankKeeper.GetBalance(chain.GetContext(), chain.SenderAccounts[0].SenderAccount.GetAddress(), denom) + default: + suite.Fail("invalid amountType %s", balanceType) + } + suite.Require().Equal(amount, total.Amount, errorMsg) +} + +func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { + + pathAtoB, pathBtoC := suite.setUpForwardingPaths() amount := sdkmath.NewInt(100) coin := sdk.NewCoin(sdk.DefaultBondDenom, amount) sender := suite.chainA.SenderAccounts[0].SenderAccount receiver := suite.chainC.SenderAccounts[0].SenderAccount // Conferm whether it's B or C + denomA := types.NewDenom(coin.Denom) + denomAB := types.NewDenom(coin.Denom, types.NewTrace(pathAtoB.EndpointB.ChannelConfig.PortID, pathAtoB.EndpointB.ChannelID)) + denomABC := types.NewDenom(coin.Denom, types.NewTrace(pathAtoB.EndpointA.ChannelConfig.PortID, pathAtoB.EndpointA.ChannelID), types.NewTrace(pathBtoC.EndpointB.ChannelConfig.PortID, pathBtoC.EndpointB.ChannelID)) + + originalABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), sender.GetAddress(), coin.Denom) + hops := &types.ForwardingInfo{ Hops: []types.Hop{ { - PortId: path2.EndpointA.ChannelConfig.PortID, - ChannelId: path2.EndpointA.ChannelID, + PortId: pathBtoC.EndpointA.ChannelConfig.PortID, + ChannelId: pathBtoC.EndpointA.ChannelID, }, }, } transferMsg := types.NewMsgTransfer( - path1.EndpointA.ChannelConfig.PortID, - path1.EndpointA.ChannelID, + pathAtoB.EndpointA.ChannelConfig.PortID, + pathAtoB.EndpointA.ChannelID, sdk.NewCoins(coin), sender.GetAddress().String(), receiver.GetAddress().String(), @@ -654,36 +685,27 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { suite.Require().NoError(err) suite.Require().NotNil(packet) - err = path1.EndpointB.UpdateClient() + err = pathAtoB.EndpointB.UpdateClient() suite.Require().NoError(err) - result, err = path1.EndpointB.RecvPacketWithResult(packet) + result, err = pathAtoB.EndpointB.RecvPacketWithResult(packet) suite.Require().NoError(err) suite.Require().NotNil(result) - err = path2.EndpointA.UpdateClient() + err = pathBtoC.EndpointA.UpdateClient() suite.Require().NoError(err) - err = path2.EndpointB.UpdateClient() + err = pathBtoC.EndpointB.UpdateClient() suite.Require().NoError(err) - // packet, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, 1) - // suite.Require().True(found) - - // bytes := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetPacketCommitment(suite.chainB.GetContext(), path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, 1) - - // packet.SourceChannel = path2.EndpointA.ChannelID - - // bytes2 := channeltypes.CommitPacket(suite.chainB.Codec, packet) - // suite.Require().Equal(bytes, bytes2) - // - // [{Denom:{Base:stake Trace:[]} Amount:100}] + suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount.Sub(amount), denomA.IBCDenom(), "Chain A should have less funds") + suite.assertAmountOnChain(suite.chainB, escrow, amount, denomAB.IBCDenom(), "Chain B's forwarding address should have coins") address := types.GetForwardAddress(packet.DestinationPort, packet.DestinationChannel).String() data := types.NewFungibleTokenPacketDataV2( []types.Token{ { - Denom: types.NewDenom("stake", types.NewTrace(path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID)), + Denom: types.NewDenom(sdk.DefaultBondDenom, types.NewTrace(pathAtoB.EndpointA.ChannelConfig.PortID, pathAtoB.EndpointA.ChannelID)), Amount: "100", }, }, @@ -695,15 +717,15 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { packet = channeltypes.NewPacket( data.GetBytes(), 1, - path2.EndpointA.ChannelConfig.PortID, - path2.EndpointA.ChannelID, - path2.EndpointB.ChannelConfig.PortID, - path2.EndpointB.ChannelID, + pathBtoC.EndpointA.ChannelConfig.PortID, + pathBtoC.EndpointA.ChannelID, + pathBtoC.EndpointB.ChannelConfig.PortID, + pathBtoC.EndpointB.ChannelID, packet.TimeoutHeight, packet.TimeoutTimestamp) // retrieve module callbacks - module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path2.EndpointA.ChannelConfig.PortID) + module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), pathBtoC.EndpointA.ChannelConfig.PortID) suite.Require().NoError(err) cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module) @@ -734,15 +756,21 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { packet = channeltypes.NewPacket( data.GetBytes(), 1, - path1.EndpointA.ChannelConfig.PortID, - path1.EndpointA.ChannelID, - path1.EndpointB.ChannelConfig.PortID, - path1.EndpointB.ChannelID, + pathAtoB.EndpointA.ChannelConfig.PortID, + pathAtoB.EndpointA.ChannelID, + pathAtoB.EndpointB.ChannelConfig.PortID, + pathAtoB.EndpointB.ChannelID, packet.TimeoutHeight, packet.TimeoutTimestamp) err = suite.chainA.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, data, ack) suite.Require().NoError(err) + + suite.assertAmountOnChain(suite.chainB, escrow, sdkmath.NewInt(0), denomAB.IBCDenom(), "Escrow account for chain B should be empty") + suite.assertAmountOnChain(suite.chainC, escrow, sdkmath.NewInt(0), denomABC.IBCDenom(), "Escrow account for chain C should be empty") + suite.assertAmountOnChain(suite.chainA, escrow, sdkmath.NewInt(0), denomA.IBCDenom(), "Escrow account for chain a should be empty") + + suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount, coin.Denom, "Chain A should have their funds back") } //Test that fails verification if no forwarding address From ff35ada5d84b416368055ed69295a15d206f305a Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 13 Jun 2024 15:21:53 +0100 Subject: [PATCH 04/28] Minor changes --- modules/apps/transfer/keeper/relay_forwarding_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 54353545ee0..2636f74a09e 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -649,7 +649,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { amount := sdkmath.NewInt(100) coin := sdk.NewCoin(sdk.DefaultBondDenom, amount) sender := suite.chainA.SenderAccounts[0].SenderAccount - receiver := suite.chainC.SenderAccounts[0].SenderAccount // Conferm whether it's B or C + receiver := suite.chainC.SenderAccounts[0].SenderAccount denomA := types.NewDenom(coin.Denom) denomAB := types.NewDenom(coin.Denom, types.NewTrace(pathAtoB.EndpointB.ChannelConfig.PortID, pathAtoB.EndpointB.ChannelID)) @@ -688,6 +688,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { err = pathAtoB.EndpointB.UpdateClient() suite.Require().NoError(err) + // Receive packet on B. result, err = pathAtoB.EndpointB.RecvPacketWithResult(packet) suite.Require().NoError(err) suite.Require().NotNil(result) @@ -698,8 +699,9 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { err = pathBtoC.EndpointB.UpdateClient() suite.Require().NoError(err) + // Make sure founds went from A to B's escrow account. suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount.Sub(amount), denomA.IBCDenom(), "Chain A should have less funds") - suite.assertAmountOnChain(suite.chainB, escrow, amount, denomAB.IBCDenom(), "Chain B's forwarding address should have coins") + suite.assertAmountOnChain(suite.chainB, escrow, amount, denomAB.IBCDenom(), "Chain B's escrow should have coins") address := types.GetForwardAddress(packet.DestinationPort, packet.DestinationChannel).String() data := types.NewFungibleTokenPacketDataV2( @@ -772,5 +774,3 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount, coin.Denom, "Chain A should have their funds back") } - -//Test that fails verification if no forwarding address From 926d8da8bf23e573efe030b4b64e79c1efbeec33 Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 13 Jun 2024 15:29:38 +0100 Subject: [PATCH 05/28] Added comments --- modules/apps/transfer/keeper/relay_forwarding_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 2636f74a09e..79fb2954669 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -733,12 +733,15 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module) suite.Require().True(ok) + // Trigger OnTimeoutPacket for chainB err = cbs.OnTimeoutPacket(suite.chainB.GetContext(), packet, nil) suite.Require().NoError(err) + // Ensure that chainB has an ack. res := suite.chainB.GetAcknowledgement(packet) suite.Require().NotNil(res, "chainB does not have an ack") + // And that this ack is of the type we expect (Error due to time out) ack := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out")) ackbytes := channeltypes.CommitAcknowledgement(ack.Acknowledgement()) suite.Require().Equal(ackbytes, res) @@ -765,12 +768,15 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { packet.TimeoutHeight, packet.TimeoutTimestamp) + // Send the ack to chain A. err = suite.chainA.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, data, ack) suite.Require().NoError(err) + // Finally, check that A,B, and C escrow accounts do not have fund. suite.assertAmountOnChain(suite.chainB, escrow, sdkmath.NewInt(0), denomAB.IBCDenom(), "Escrow account for chain B should be empty") suite.assertAmountOnChain(suite.chainC, escrow, sdkmath.NewInt(0), denomABC.IBCDenom(), "Escrow account for chain C should be empty") suite.assertAmountOnChain(suite.chainA, escrow, sdkmath.NewInt(0), denomA.IBCDenom(), "Escrow account for chain a should be empty") + // And that A hsa its original balance back. suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount, coin.Denom, "Chain A should have their funds back") } From 3f5dc6ca932775b45be8b85f75f0f7d0df00baf6 Mon Sep 17 00:00:00 2001 From: bznein Date: Fri, 14 Jun 2024 09:08:41 +0100 Subject: [PATCH 06/28] Fix type name. --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index a3cb78202c3..e2da8e169b1 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -657,7 +657,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { originalABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), sender.GetAddress(), coin.Denom) - hops := &types.ForwardingInfo{ + hops := &types.Forwarding{ Hops: []types.Hop{ { PortId: pathBtoC.EndpointA.ChannelConfig.PortID, From 10592e1e23702c992dbc82c679cca1241a3de7ec Mon Sep 17 00:00:00 2001 From: bznein Date: Fri, 14 Jun 2024 09:40:41 +0100 Subject: [PATCH 07/28] Gofumpt --- modules/apps/transfer/keeper/relay_forwarding_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index e2da8e169b1..6e5846df761 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -643,7 +643,6 @@ func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, b } func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { - pathAtoB, pathBtoC := suite.setUpForwardingPaths() amount := sdkmath.NewInt(100) From 6787c74e4fde562f04b45fcf3e634a9e6d3e6340 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Fri, 14 Jun 2024 15:18:12 +0100 Subject: [PATCH 08/28] Update modules/apps/transfer/keeper/relay_forwarding_test.go Co-authored-by: Carlos Rodriguez --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 6e5846df761..d41125a2edc 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -776,6 +776,6 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { suite.assertAmountOnChain(suite.chainC, escrow, sdkmath.NewInt(0), denomABC.IBCDenom(), "Escrow account for chain C should be empty") suite.assertAmountOnChain(suite.chainA, escrow, sdkmath.NewInt(0), denomA.IBCDenom(), "Escrow account for chain a should be empty") - // And that A hsa its original balance back. + // And that A has its original balance back. suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount, coin.Denom, "Chain A should have their funds back") } From 37375ba31d45b94f2c472adf849351d0c8a1a4b8 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Fri, 14 Jun 2024 15:18:26 +0100 Subject: [PATCH 09/28] Update modules/apps/transfer/keeper/relay_forwarding_test.go Co-authored-by: Carlos Rodriguez --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index d41125a2edc..c117dc815b0 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -698,7 +698,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { err = pathBtoC.EndpointB.UpdateClient() suite.Require().NoError(err) - // Make sure founds went from A to B's escrow account. + // Make sure funds went from A to B's escrow account. suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount.Sub(amount), denomA.IBCDenom(), "Chain A should have less funds") suite.assertAmountOnChain(suite.chainB, escrow, amount, denomAB.IBCDenom(), "Chain B's escrow should have coins") From 65a0d0ab0793e45896faa2df9bff71fcdb43d5db Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Fri, 14 Jun 2024 15:18:34 +0100 Subject: [PATCH 10/28] Update modules/apps/transfer/keeper/relay_forwarding_test.go Co-authored-by: Carlos Rodriguez --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index c117dc815b0..b9b10e4bd7d 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -748,7 +748,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { data = types.NewFungibleTokenPacketDataV2( []types.Token{ { - Denom: types.NewDenom("stake"), + Denom: types.NewDenom(sdk.DefaultBondDenom), Amount: "100", }, }, From ca59adf91d41966670332dda0a9c14c2a15500d2 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Fri, 14 Jun 2024 15:18:49 +0100 Subject: [PATCH 11/28] Update modules/apps/transfer/keeper/relay_forwarding_test.go Co-authored-by: Carlos Rodriguez --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index b9b10e4bd7d..392775bef4d 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -772,8 +772,8 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { suite.Require().NoError(err) // Finally, check that A,B, and C escrow accounts do not have fund. - suite.assertAmountOnChain(suite.chainB, escrow, sdkmath.NewInt(0), denomAB.IBCDenom(), "Escrow account for chain B should be empty") suite.assertAmountOnChain(suite.chainC, escrow, sdkmath.NewInt(0), denomABC.IBCDenom(), "Escrow account for chain C should be empty") + suite.assertAmountOnChain(suite.chainB, escrow, sdkmath.NewInt(0), denomAB.IBCDenom(), "Escrow account for chain B should be empty") suite.assertAmountOnChain(suite.chainA, escrow, sdkmath.NewInt(0), denomA.IBCDenom(), "Escrow account for chain a should be empty") // And that A has its original balance back. From 1492378ff98d42d9bb3f5011c2a7f4928cffb671 Mon Sep 17 00:00:00 2001 From: bznein Date: Fri, 14 Jun 2024 15:32:14 +0100 Subject: [PATCH 12/28] Add godoc to test. --- modules/apps/transfer/keeper/relay_forwarding_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 392775bef4d..bac814c1b5a 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -642,6 +642,9 @@ func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, b suite.Require().Equal(amount, total.Amount, errorMsg) } +// TestOnTimeoutPacketForwarding tests the scenario in which a packet goes from +// A to C, using B as a forwarding hop. The packet times out when going to B +// from C and we verify that funds are properly returned to A. func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { pathAtoB, pathBtoC := suite.setUpForwardingPaths() @@ -656,7 +659,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { originalABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), sender.GetAddress(), coin.Denom) - hops := &types.Forwarding{ + forwarding := &types.Forwarding{ Hops: []types.Hop{ { PortId: pathBtoC.EndpointA.ChannelConfig.PortID, @@ -673,7 +676,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { receiver.GetAddress().String(), suite.chainA.GetTimeoutHeight(), 0, "", - hops, + forwarding, ) result, err := suite.chainA.SendMsgs(transferMsg) @@ -754,7 +757,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { }, suite.chainA.SenderAccounts[0].SenderAccount.GetAddress().String(), suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), - "", hops, + "", forwarding, ) packet = channeltypes.NewPacket( From 288f5f6d10cdcef44034de84ac3bdaccb5623321 Mon Sep 17 00:00:00 2001 From: bznein Date: Fri, 14 Jun 2024 15:47:33 +0100 Subject: [PATCH 13/28] Changed trace construction --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index bac814c1b5a..b8e1ff9bef7 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -655,7 +655,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { denomA := types.NewDenom(coin.Denom) denomAB := types.NewDenom(coin.Denom, types.NewTrace(pathAtoB.EndpointB.ChannelConfig.PortID, pathAtoB.EndpointB.ChannelID)) - denomABC := types.NewDenom(coin.Denom, types.NewTrace(pathAtoB.EndpointA.ChannelConfig.PortID, pathAtoB.EndpointA.ChannelID), types.NewTrace(pathBtoC.EndpointB.ChannelConfig.PortID, pathBtoC.EndpointB.ChannelID)) + denomABC := types.NewDenom(coin.Denom, append(denomAB.Trace, types.NewTrace(pathBtoC.EndpointB.ChannelConfig.PortID, pathBtoC.EndpointB.ChannelID))...) originalABalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), sender.GetAddress(), coin.Denom) From a70e56d8ca5f89f0d3b952c61afac41937b8b615 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Mon, 17 Jun 2024 09:21:12 +0100 Subject: [PATCH 14/28] Update modules/apps/transfer/keeper/relay_forwarding_test.go Co-authored-by: Carlos Rodriguez --- modules/apps/transfer/keeper/relay_forwarding_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 30b6d2ece3a..ce42cf870f3 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -640,8 +640,8 @@ func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, b } // TestOnTimeoutPacketForwarding tests the scenario in which a packet goes from -// A to C, using B as a forwarding hop. The packet times out when going to B -// from C and we verify that funds are properly returned to A. +// A to C, using B as a forwarding hop. The packet times out when going to C +// from B and we verify that funds are properly returned to A. func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { pathAtoB, pathBtoC := suite.setUpForwardingPaths() From 6a33a57b1071894d90173688b64c6ee58c8c9311 Mon Sep 17 00:00:00 2001 From: bznein Date: Fri, 14 Jun 2024 16:07:37 +0100 Subject: [PATCH 15/28] remove error msg parameter from helper function --- .../transfer/keeper/relay_forwarding_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index ce42cf870f3..ddf534257ef 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -626,7 +626,7 @@ const ( balance ) -func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, balanceType amountType, amount sdkmath.Int, denom string, errorMsg string) { +func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, balanceType amountType, amount sdkmath.Int, denom string) { var total sdk.Coin switch balanceType { case escrow: @@ -636,7 +636,7 @@ func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, b default: suite.Fail("invalid amountType %s", balanceType) } - suite.Require().Equal(amount, total.Amount, errorMsg) + suite.Require().Equal(amount, total.Amount, fmt.Sprintf("Chain %s: got balance of %d, wanted %d", chain.Name(), total.Amount, amount)) } // TestOnTimeoutPacketForwarding tests the scenario in which a packet goes from @@ -699,8 +699,8 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { suite.Require().NoError(err) // Make sure funds went from A to B's escrow account. - suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount.Sub(amount), denomA.IBCDenom(), "Chain A should have less funds") - suite.assertAmountOnChain(suite.chainB, escrow, amount, denomAB.IBCDenom(), "Chain B's escrow should have coins") + suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount.Sub(amount), denomA.IBCDenom()) + suite.assertAmountOnChain(suite.chainB, escrow, amount, denomAB.IBCDenom()) address := types.GetForwardAddress(packet.DestinationPort, packet.DestinationChannel).String() data := types.NewFungibleTokenPacketDataV2( @@ -772,10 +772,10 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { suite.Require().NoError(err) // Finally, check that A,B, and C escrow accounts do not have fund. - suite.assertAmountOnChain(suite.chainC, escrow, sdkmath.NewInt(0), denomABC.IBCDenom(), "Escrow account for chain C should be empty") - suite.assertAmountOnChain(suite.chainB, escrow, sdkmath.NewInt(0), denomAB.IBCDenom(), "Escrow account for chain B should be empty") - suite.assertAmountOnChain(suite.chainA, escrow, sdkmath.NewInt(0), denomA.IBCDenom(), "Escrow account for chain a should be empty") + suite.assertAmountOnChain(suite.chainC, escrow, sdkmath.NewInt(0), denomABC.IBCDenom()) + suite.assertAmountOnChain(suite.chainB, escrow, sdkmath.NewInt(0), denomAB.IBCDenom()) + suite.assertAmountOnChain(suite.chainA, escrow, sdkmath.NewInt(0), denomA.IBCDenom()) // And that A has its original balance back. - suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount, coin.Denom, "Chain A should have their funds back") + suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount, coin.Denom) } From 7fb55699fcdd988fef302117ac402fe5f0409252 Mon Sep 17 00:00:00 2001 From: bznein Date: Mon, 17 Jun 2024 12:18:56 +0100 Subject: [PATCH 16/28] Add test for forwarded packet --- .../transfer/keeper/relay_forwarding_test.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index ddf534257ef..3f9c3593b5b 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -611,12 +611,12 @@ Test async ack is properly relayed to middle hop after forwarding transfer compl Tiemout during forwarding after middle hop execution reverts properly the state changes */ -func (suite *KeeperTestSuite) setUpForwardingPaths() (path1, path2 *ibctesting.Path) { - path1 = ibctesting.NewTransferPath(suite.chainA, suite.chainB) - path2 = ibctesting.NewTransferPath(suite.chainB, suite.chainC) - path1.Setup() - path2.Setup() - return path1, path2 +func (suite *KeeperTestSuite) setUpForwardingPaths() (pathAtoB, pathBtoC *ibctesting.Path) { + pathAtoB = ibctesting.NewTransferPath(suite.chainA, suite.chainB) + pathBtoC = ibctesting.NewTransferPath(suite.chainB, suite.chainC) + pathAtoB.Setup() + pathBtoC.Setup() + return pathAtoB, pathBtoC } type amountType int @@ -702,6 +702,11 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { suite.assertAmountOnChain(suite.chainA, balance, originalABalance.Amount.Sub(amount), denomA.IBCDenom()) suite.assertAmountOnChain(suite.chainB, escrow, amount, denomAB.IBCDenom()) + // Check that forwarded packet exists + forwardedPacket, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), pathBtoC.EndpointA.ChannelConfig.PortID, pathBtoC.EndpointA.ChannelID, packet.Sequence) + suite.Require().True(found, "Chain B has no forwarded packet") + suite.Require().Equal(packet, forwardedPacket, "ForwardedPacket stored in ChainB is not the same that was sent") + address := types.GetForwardAddress(packet.DestinationPort, packet.DestinationChannel).String() data := types.NewFungibleTokenPacketDataV2( []types.Token{ From d1736dc72c76dfdc570d843d2bf5da7b7281f132 Mon Sep 17 00:00:00 2001 From: bznein Date: Mon, 17 Jun 2024 14:39:09 +0100 Subject: [PATCH 17/28] Delete packet when not needed anymore. --- modules/apps/transfer/keeper/keeper.go | 13 +++++++++++++ modules/apps/transfer/keeper/relay.go | 17 ++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 5973d241359..40ab572a038 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -328,3 +328,16 @@ func (k Keeper) GetForwardedPacket(ctx sdk.Context, portID, channelID string, se return storedPacket, true } + +// DeleteForwardedPacket deletes the forwarded packet from the store. +func (k Keeper) DeleteForwardedPacket(ctx sdk.Context, portID, channelID string, sequence uint64) error { + store := ctx.KVStore(k.storeKey) + packetKey := types.PacketForwardKey(portID, channelID, sequence) + if !store.Has(packetKey) { + return fmt.Errorf("packet with PortID %s, channelID %s and sequence %d not found in the store", portID, channelID, sequence) + } + + store.Delete(packetKey) + + return nil +} diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 8b58787170d..45370976e03 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -286,7 +286,11 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac switch ack.Response.(type) { case *channeltypes.Acknowledgement_Result: if isForwarded { - return k.ackForwardPacketSuccess(ctx, prevPacket) + if err := k.ackForwardPacketSuccess(ctx, prevPacket); err != nil { + return err + } + // Delete the previous packet. + return k.DeleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) } // the acknowledgement succeeded on the receiving chain so nothing @@ -298,7 +302,10 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac return err } if isForwarded { - return k.ackForwardPacketError(ctx, prevPacket, data) + if err := k.ackForwardPacketError(ctx, prevPacket, data); err != nil { + // Delete the previous packet. + return k.DeleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + } } return nil @@ -317,7 +324,11 @@ func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, dat prevPacket, isForwarded := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) if isForwarded { - return k.ackForwardPacketTimeout(ctx, prevPacket, data) + if err := k.ackForwardPacketTimeout(ctx, prevPacket, data); err != nil { + return err + } + // Delete the previous packet. + return k.DeleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) } return nil From 8c8feeb8d106d288e7c1a916bcb0c803e58da55b Mon Sep 17 00:00:00 2001 From: bznein Date: Mon, 17 Jun 2024 14:59:46 +0100 Subject: [PATCH 18/28] Move deletion of packet in a single place. --- modules/apps/transfer/keeper/forwarding.go | 7 ++++++- modules/apps/transfer/keeper/keeper.go | 4 ++-- modules/apps/transfer/keeper/relay.go | 18 +++--------------- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index dab78bb32a5..264f7013f6c 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -50,7 +50,12 @@ func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes. return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") } - return k.ics4Wrapper.WriteAcknowledgement(ctx, capability, packet, ack) + if err := k.ics4Wrapper.WriteAcknowledgement(ctx, capability, packet, ack); err != nil { + return err + } + + k.deleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + return nil } // revertForwardedPacket reverts the logic of receive packet that occurs in the middle chains during a packet forwarding. diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index 40ab572a038..ddaf468c9a1 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -329,8 +329,8 @@ func (k Keeper) GetForwardedPacket(ctx sdk.Context, portID, channelID string, se return storedPacket, true } -// DeleteForwardedPacket deletes the forwarded packet from the store. -func (k Keeper) DeleteForwardedPacket(ctx sdk.Context, portID, channelID string, sequence uint64) error { +// deleteForwardedPacket deletes the forwarded packet from the store. +func (k Keeper) deleteForwardedPacket(ctx sdk.Context, portID, channelID string, sequence uint64) error { store := ctx.KVStore(k.storeKey) packetKey := types.PacketForwardKey(portID, channelID, sequence) if !store.Has(packetKey) { diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 45370976e03..0417f108b08 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -286,11 +286,7 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac switch ack.Response.(type) { case *channeltypes.Acknowledgement_Result: if isForwarded { - if err := k.ackForwardPacketSuccess(ctx, prevPacket); err != nil { - return err - } - // Delete the previous packet. - return k.DeleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + return k.ackForwardPacketSuccess(ctx, prevPacket) } // the acknowledgement succeeded on the receiving chain so nothing @@ -302,12 +298,8 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac return err } if isForwarded { - if err := k.ackForwardPacketError(ctx, prevPacket, data); err != nil { - // Delete the previous packet. - return k.DeleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) - } + return k.ackForwardPacketError(ctx, prevPacket, data) } - return nil default: return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response) @@ -324,11 +316,7 @@ func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, dat prevPacket, isForwarded := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) if isForwarded { - if err := k.ackForwardPacketTimeout(ctx, prevPacket, data); err != nil { - return err - } - // Delete the previous packet. - return k.DeleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + return k.ackForwardPacketTimeout(ctx, prevPacket, data) } return nil From 24f82d6945254cdd62a658c03b6b76e1e0382aca Mon Sep 17 00:00:00 2001 From: bznein Date: Mon, 17 Jun 2024 15:00:36 +0100 Subject: [PATCH 19/28] Reintroduce newline --- modules/apps/transfer/keeper/relay.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 0417f108b08..8b58787170d 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -300,6 +300,7 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac if isForwarded { return k.ackForwardPacketError(ctx, prevPacket, data) } + return nil default: return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response) From 6528515346df835519aa4fdc34b3d59e17070b93 Mon Sep 17 00:00:00 2001 From: bznein Date: Mon, 17 Jun 2024 15:04:12 +0100 Subject: [PATCH 20/28] Do not ignore error. --- modules/apps/transfer/keeper/forwarding.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index 264f7013f6c..463680576fa 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -54,8 +54,7 @@ func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes. return err } - k.deleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) - return nil + return k.deleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) } // revertForwardedPacket reverts the logic of receive packet that occurs in the middle chains during a packet forwarding. From bec616f3aeee2f970076bcff4d75aa39e014d581 Mon Sep 17 00:00:00 2001 From: bznein Date: Tue, 18 Jun 2024 08:39:36 +0100 Subject: [PATCH 21/28] PR feedback. --- modules/apps/transfer/keeper/forwarding.go | 5 ++++- modules/apps/transfer/keeper/keeper.go | 7 +------ modules/apps/transfer/keeper/relay_forwarding_test.go | 4 ++++ 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index cac53c3829d..9601351fe70 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -53,7 +53,10 @@ func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes. return err } - return k.deleteForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + // During a middlehop, we make a mapping creating a pointer in the "reverse" direction with PortID and ChannelID. + // Thus, when processing the ack, we need to delete the packet based on destination port and channel. + k.deleteForwardedPacket(ctx, packet.DestinationPort, packet.DestinationChannel, packet.Sequence) + return nil } // revertForwardedPacket reverts the logic of receive packet that occurs in the middle chains during a packet forwarding. diff --git a/modules/apps/transfer/keeper/keeper.go b/modules/apps/transfer/keeper/keeper.go index ddaf468c9a1..988e599b4af 100644 --- a/modules/apps/transfer/keeper/keeper.go +++ b/modules/apps/transfer/keeper/keeper.go @@ -330,14 +330,9 @@ func (k Keeper) GetForwardedPacket(ctx sdk.Context, portID, channelID string, se } // deleteForwardedPacket deletes the forwarded packet from the store. -func (k Keeper) deleteForwardedPacket(ctx sdk.Context, portID, channelID string, sequence uint64) error { +func (k Keeper) deleteForwardedPacket(ctx sdk.Context, portID, channelID string, sequence uint64) { store := ctx.KVStore(k.storeKey) packetKey := types.PacketForwardKey(portID, channelID, sequence) - if !store.Has(packetKey) { - return fmt.Errorf("packet with PortID %s, channelID %s and sequence %d not found in the store", portID, channelID, sequence) - } store.Delete(packetKey) - - return nil } diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index ac4698003e1..00f97ff8548 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -535,6 +535,10 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, data, ack) suite.Require().NoError(err) + // Check that B deleted the forwarded packet. + _, found = suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), forwardedPacket.DestinationPort, forwardedPacket.DestinationChannel, packet.Sequence) + suite.Require().False(found, "chain B should have deleted the packet") + // Check that Escrow B has been refunded amount coin = sdk.NewCoin(denomAB.IBCDenom(), amount) totalEscrowChainB = suite.chainB.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainB.GetContext(), coin.GetDenom()) From ad4e6f61a36f72047b52995aa40e3f6b88fdbfa0 Mon Sep 17 00:00:00 2001 From: bznein Date: Tue, 18 Jun 2024 10:35:59 +0100 Subject: [PATCH 22/28] Construct packet for B ack check. --- .../transfer/keeper/relay_forwarding_test.go | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 9b69e952c13..a46715ab07a 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -716,8 +716,31 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { err = cbs.OnTimeoutPacket(suite.chainB.GetContext(), packet, nil) suite.Require().NoError(err) + // Now we construct the packet for which B should have an ack. + ackData := types.NewFungibleTokenPacketDataV2( + []types.Token{ + { + Denom: types.NewDenom(sdk.DefaultBondDenom), + Amount: "100", + }, + }, + address, + suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), + "", nil, + ) + + ackPacket := channeltypes.NewPacket( + ackData.GetBytes(), + 1, + pathAtoB.EndpointA.ChannelConfig.PortID, + pathAtoB.EndpointA.ChannelID, + pathAtoB.EndpointB.ChannelConfig.PortID, + pathAtoB.EndpointB.ChannelID, + packet.TimeoutHeight, + packet.TimeoutTimestamp) + // Ensure that chainB has an ack. - res := suite.chainB.GetAcknowledgement(packet) + res := suite.chainB.GetAcknowledgement(ackPacket) suite.Require().NotNil(res, "chainB does not have an ack") // And that this ack is of the type we expect (Error due to time out) From 139facfd9f33a80295fd68141688519ba733202e Mon Sep 17 00:00:00 2001 From: bznein Date: Tue, 18 Jun 2024 15:51:54 +0100 Subject: [PATCH 23/28] PR feedback --- .../transfer/keeper/relay_forwarding_test.go | 45 +++++-------------- 1 file changed, 12 insertions(+), 33 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 0ac698e51e9..4b2b79b605e 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -3,6 +3,7 @@ package keeper_test import ( "errors" "fmt" + "time" sdkmath "cosmossdk.io/math" @@ -611,7 +612,7 @@ Test async ack is properly relayed to middle hop after forwarding transfer compl Tiemout during forwarding after middle hop execution reverts properly the state changes */ -func (suite *KeeperTestSuite) setUpForwardingPaths() (pathAtoB, pathBtoC *ibctesting.Path) { +func (suite *KeeperTestSuite) setupForwardingPaths() (pathAtoB, pathBtoC *ibctesting.Path) { pathAtoB = ibctesting.NewTransferPath(suite.chainA, suite.chainB) pathBtoC = ibctesting.NewTransferPath(suite.chainB, suite.chainC) pathAtoB.Setup() @@ -643,7 +644,7 @@ func (suite *KeeperTestSuite) assertAmountOnChain(chain *ibctesting.TestChain, b // A to C, using B as a forwarding hop. The packet times out when going to C // from B and we verify that funds are properly returned to A. func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { - pathAtoB, pathBtoC := suite.setUpForwardingPaths() + pathAtoB, pathBtoC := suite.setupForwardingPaths() amount := sdkmath.NewInt(100) coin := sdk.NewCoin(sdk.DefaultBondDenom, amount) @@ -671,8 +672,9 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { sdk.NewCoins(coin), sender.GetAddress().String(), receiver.GetAddress().String(), - suite.chainA.GetTimeoutHeight(), - 0, "", + clienttypes.ZeroHeight(), + uint64(suite.chainA.GetContext().BlockTime().Add(time.Minute*5).UnixNano()), + "", forwarding, ) @@ -716,7 +718,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { }, }, address, - suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), + receiver.GetAddress().String(), "", nil, ) @@ -741,37 +743,14 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { err = cbs.OnTimeoutPacket(suite.chainB.GetContext(), packet, nil) suite.Require().NoError(err) - // Now we construct the packet for which B should have an ack. - ackData := types.NewFungibleTokenPacketDataV2( - []types.Token{ - { - Denom: types.NewDenom(sdk.DefaultBondDenom), - Amount: "100", - }, - }, - address, - suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), - "", nil, - ) - - ackPacket := channeltypes.NewPacket( - ackData.GetBytes(), - 1, - pathAtoB.EndpointA.ChannelConfig.PortID, - pathAtoB.EndpointA.ChannelID, - pathAtoB.EndpointB.ChannelConfig.PortID, - pathAtoB.EndpointB.ChannelID, - packet.TimeoutHeight, - packet.TimeoutTimestamp) - // Ensure that chainB has an ack. - res := suite.chainB.GetAcknowledgement(ackPacket) - suite.Require().NotNil(res, "chainB does not have an ack") + storedAck, found := suite.chainB.App.GetIBCKeeper().ChannelKeeper.GetPacketAcknowledgement(suite.chainB.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) + suite.Require().True(found, "chainB does not have an ack") // And that this ack is of the type we expect (Error due to time out) ack := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out")) ackbytes := channeltypes.CommitAcknowledgement(ack.Acknowledgement()) - suite.Require().Equal(ackbytes, res) + suite.Require().Equal(ackbytes, storedAck) data = types.NewFungibleTokenPacketDataV2( []types.Token{ @@ -780,8 +759,8 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { Amount: "100", }, }, - suite.chainA.SenderAccounts[0].SenderAccount.GetAddress().String(), - suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), + sender.GetAddress().String(), + receiver.GetAddress().String(), "", forwarding, ) From fdd769f71d4fb9899fd2ef890cb85be5df6a3fbe Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 20 Jun 2024 09:33:15 +0100 Subject: [PATCH 24/28] Pass packet to acknowledgeforwardedpacket --- modules/apps/transfer/keeper/forwarding.go | 26 ++++++++++------------ modules/apps/transfer/keeper/relay.go | 6 ++--- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index 759e11eceb6..20addcb769f 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -48,14 +48,8 @@ func (k Keeper) forwardPacket(ctx sdk.Context, data types.FungibleTokenPacketDat return nil } -// ackForwardPacketSuccess writes a successful async acknowledgement for the prevPacket -func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket channeltypes.Packet) error { - forwardAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck) -} - // ackForwardPacketError reverts the receive packet logic that occurs in the middle chain and writes the async ack for the prevPacket -func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error { +func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error { // the forwarded packet has failed, thus the funds have been refunded to the intermediate address. // we must revert the changes that came from successfully receiving the tokens on our chain // before propagating the error acknowledgement back to original sender chain @@ -64,21 +58,27 @@ func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.P } forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet failed")) - return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck) + return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) +} + +// ackForwardPacketSuccess writes a successful async acknowledgement for the prevPacket +func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet) error { + forwardAck := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded")) + return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) } // ackForwardPacketTimeout reverts the receive packet logic that occurs in the middle chain and writes a failed async ack for the prevPacket -func (k Keeper) ackForwardPacketTimeout(ctx sdk.Context, prevPacket channeltypes.Packet, timeoutPacketData types.FungibleTokenPacketDataV2) error { +func (k Keeper) ackForwardPacketTimeout(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet, timeoutPacketData types.FungibleTokenPacketDataV2) error { if err := k.revertForwardedPacket(ctx, prevPacket, timeoutPacketData); err != nil { return err } forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out")) - return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck) + return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) } // acknowledgeForwardedPacket writes the async acknowledgement for packet -func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error { +func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes.Packet, forwardedPacket channeltypes.Packet, ack channeltypes.Acknowledgement) error { capability, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.DestinationPort, packet.DestinationChannel)) if !ok { return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") @@ -88,9 +88,7 @@ func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes. return err } - // During a middlehop, we make a mapping creating a pointer in the "reverse" direction with PortID and ChannelID. - // Thus, when processing the ack, we need to delete the packet based on destination port and channel. - k.deleteForwardedPacket(ctx, packet.DestinationPort, packet.DestinationChannel, packet.Sequence) + k.deleteForwardedPacket(ctx, forwardedPacket.DestinationPort, forwardedPacket.DestinationChannel, forwardedPacket.Sequence) return nil } diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 2bfedcc0493..65df15c5d43 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -285,7 +285,7 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac switch ack.Response.(type) { case *channeltypes.Acknowledgement_Result: if isForwarded { - return k.ackForwardPacketSuccess(ctx, prevPacket) + return k.ackForwardPacketSuccess(ctx, prevPacket, packet) } // the acknowledgement succeeded on the receiving chain so nothing @@ -297,7 +297,7 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac return err } if isForwarded { - return k.ackForwardPacketError(ctx, prevPacket, data) + return k.ackForwardPacketError(ctx, prevPacket, packet, data) } return nil @@ -316,7 +316,7 @@ func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, dat prevPacket, isForwarded := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) if isForwarded { - return k.ackForwardPacketTimeout(ctx, prevPacket, data) + return k.ackForwardPacketTimeout(ctx, prevPacket, packet, data) } return nil From d5320075048cf1072452a1ed43dd1f08a754d4e7 Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 20 Jun 2024 09:36:18 +0100 Subject: [PATCH 25/28] revert unwanted change --- modules/apps/transfer/keeper/forwarding.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index 20addcb769f..e8058c06ad3 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -48,6 +48,12 @@ func (k Keeper) forwardPacket(ctx sdk.Context, data types.FungibleTokenPacketDat return nil } +// ackForwardPacketSuccess writes a successful async acknowledgement for the prevPacket +func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet) error { + forwardAck := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded")) + return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) +} + // ackForwardPacketError reverts the receive packet logic that occurs in the middle chain and writes the async ack for the prevPacket func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error { // the forwarded packet has failed, thus the funds have been refunded to the intermediate address. @@ -61,12 +67,6 @@ func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.P return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) } -// ackForwardPacketSuccess writes a successful async acknowledgement for the prevPacket -func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet) error { - forwardAck := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded")) - return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) -} - // ackForwardPacketTimeout reverts the receive packet logic that occurs in the middle chain and writes a failed async ack for the prevPacket func (k Keeper) ackForwardPacketTimeout(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet, timeoutPacketData types.FungibleTokenPacketDataV2) error { if err := k.revertForwardedPacket(ctx, prevPacket, timeoutPacketData); err != nil { From db52296400c2389400dd26367f17b61b6745bee7 Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 20 Jun 2024 09:37:41 +0100 Subject: [PATCH 26/28] Another unwanted change --- modules/apps/transfer/keeper/forwarding.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index e8058c06ad3..50f7c940bcb 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -50,7 +50,7 @@ func (k Keeper) forwardPacket(ctx sdk.Context, data types.FungibleTokenPacketDat // ackForwardPacketSuccess writes a successful async acknowledgement for the prevPacket func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet) error { - forwardAck := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded")) + forwardAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) } From b6684da88a7cfd8386949a06ac89c69ab53bd33a Mon Sep 17 00:00:00 2001 From: bznein Date: Thu, 20 Jun 2024 11:14:06 +0100 Subject: [PATCH 27/28] Better signature and fix source/dest --- modules/apps/transfer/keeper/forwarding.go | 13 ++++++------- .../apps/transfer/keeper/relay_forwarding_test.go | 4 ++-- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index 6bdf0ca3549..705fbc0f5e4 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -48,13 +48,13 @@ func (k Keeper) forwardPacket(ctx sdk.Context, data types.FungibleTokenPacketDat } // ackForwardPacketSuccess writes a successful async acknowledgement for the prevPacket -func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet) error { +func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket, forwardedPacket channeltypes.Packet) error { forwardAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) } // ackForwardPacketError reverts the receive packet logic that occurs in the middle chain and writes the async ack for the prevPacket -func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error { +func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket, forwardedPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error { // the forwarded packet has failed, thus the funds have been refunded to the intermediate address. // we must revert the changes that came from successfully receiving the tokens on our chain // before propagating the error acknowledgement back to original sender chain @@ -63,22 +63,21 @@ func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.P } forwardAck := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketFailed) - return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) + return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) } // ackForwardPacketTimeout reverts the receive packet logic that occurs in the middle chain and writes a failed async ack for the prevPacket -func (k Keeper) ackForwardPacketTimeout(ctx sdk.Context, prevPacket channeltypes.Packet, forwardedPacket channeltypes.Packet, timeoutPacketData types.FungibleTokenPacketDataV2) error { +func (k Keeper) ackForwardPacketTimeout(ctx sdk.Context, prevPacket, forwardedPacket channeltypes.Packet, timeoutPacketData types.FungibleTokenPacketDataV2) error { if err := k.revertForwardedPacket(ctx, prevPacket, timeoutPacketData); err != nil { return err } - forwardAck := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketTimedOut) return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardedPacket, forwardAck) } // acknowledgeForwardedPacket writes the async acknowledgement for packet -func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes.Packet, forwardedPacket channeltypes.Packet, ack channeltypes.Acknowledgement) error { +func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet, forwardedPacket channeltypes.Packet, ack channeltypes.Acknowledgement) error { capability, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.DestinationPort, packet.DestinationChannel)) if !ok { return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") @@ -88,7 +87,7 @@ func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet channeltypes. return err } - k.deleteForwardedPacket(ctx, forwardedPacket.DestinationPort, forwardedPacket.DestinationChannel, forwardedPacket.Sequence) + k.deleteForwardedPacket(ctx, forwardedPacket.SourcePort, forwardedPacket.SourceChannel, forwardedPacket.Sequence) return nil } diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 4895d201521..069f9848be1 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -562,8 +562,8 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { suite.Require().NoError(err) // Check that B deleted the forwarded packet. - _, found = suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), forwardedPacket.DestinationPort, forwardedPacket.DestinationChannel, packet.Sequence) - suite.Require().False(found, "chain B should have deleted the packet") + _, found = suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), forwardedPacket.SourcePort, forwardedPacket.SourceChannel, forwardedPacket.Sequence) + suite.Require().False(found, "chain B should have deleted the forwarded packet mapping") // Check that Escrow B has been refunded amount coin = sdk.NewCoin(denomAB.IBCDenom(), amount) From 432fd1f1ab1d26f82b75ce2e81e39d82ef240b9a Mon Sep 17 00:00:00 2001 From: bznein Date: Fri, 21 Jun 2024 09:11:41 +0100 Subject: [PATCH 28/28] Added one more test. --- modules/apps/transfer/keeper/relay_forwarding_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 7f6b9102d07..b36f6e28a49 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -317,6 +317,10 @@ func (suite *KeeperTestSuite) TestSimplifiedHappyPathForwarding() { err = path2.EndpointB.UpdateClient() suite.Require().NoError(err) + // B should now have deleted the forwarded packet. + _, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), packetFromAtoB.DestinationPort, packetFromAtoB.DestinationChannel, packetFromAtoB.Sequence) + suite.Require().False(found, "Chain B should have deleted its forwarded packet") + result, err = path2.EndpointB.RecvPacketWithResult(packetFromBtoC) suite.Require().NoError(err) suite.Require().NotNil(result)