From 14d54864cf9642687047d047d1aea08913c8944a Mon Sep 17 00:00:00 2001 From: Gjermund Garaba Date: Wed, 19 Jun 2024 18:38:23 +0200 Subject: [PATCH] feat(transfer): use registered error code for error acks in token forwarding (#6648) * Add typed errors to packet forwarding * Use forward errors in tests * Make ack construction consistent --- modules/apps/transfer/keeper/forwarding.go | 6 +- .../transfer/keeper/relay_forwarding_test.go | 84 +++++++++---------- modules/apps/transfer/types/errors.go | 2 + testing/events.go | 13 +++ 4 files changed, 55 insertions(+), 50 deletions(-) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index c85de4cbcc1..3e5f0d3718a 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -1,8 +1,6 @@ package keeper import ( - "errors" - errorsmod "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" @@ -64,7 +62,7 @@ func (k Keeper) ackForwardPacketError(ctx sdk.Context, prevPacket channeltypes.P return err } - forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet failed")) + forwardAck := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketFailed) return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck) } @@ -74,7 +72,7 @@ func (k Keeper) ackForwardPacketTimeout(ctx sdk.Context, prevPacket channeltypes return err } - forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out")) + forwardAck := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketTimedOut) return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck) } diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 78191ce7f04..3a2a3543195 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -1,7 +1,6 @@ package keeper_test import ( - "errors" "fmt" "time" @@ -9,7 +8,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/ibc-go/v8/modules/apps/transfer/internal" "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" @@ -403,14 +401,14 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { suite.Require().NoError(err) // message committed // parse the packet from result events and recv packet on chainB - packet, err := ibctesting.ParsePacketFromEvents(result.Events) + packetFromAtoB, err := ibctesting.ParsePacketFromEvents(result.Events) suite.Require().NoError(err) - suite.Require().NotNil(packet) + suite.Require().NotNil(packetFromAtoB) err = path1.EndpointB.UpdateClient() suite.Require().NoError(err) - result, err = path1.EndpointB.RecvPacketWithResult(packet) + result, err = path1.EndpointB.RecvPacketWithResult(packetFromAtoB) suite.Require().NoError(err) suite.Require().NotNil(result) @@ -447,14 +445,14 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { suite.Require().NoError(err) // message committed // parse the packet from result events and recv packet on chainB - packet, err = ibctesting.ParsePacketFromEvents(result.Events) + packetFromBtoC, err := ibctesting.ParsePacketFromEvents(result.Events) suite.Require().NoError(err) - suite.Require().NotNil(packet) + suite.Require().NotNil(packetFromBtoC) err = path2.EndpointB.UpdateClient() suite.Require().NoError(err) - result, err = path2.EndpointB.RecvPacketWithResult(packet) + result, err = path2.EndpointB.RecvPacketWithResult(packetFromBtoC) suite.Require().NoError(err) suite.Require().NotNil(result) @@ -503,22 +501,22 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { suite.Require().Equal(sdkmath.NewInt(0), postCoinOnC.Amount, "Vouchers have not been burned") // parse the packet from result events and recv packet on chainB - packet, err = ibctesting.ParsePacketFromEvents(result.Events) + packetFromCtoB, err := ibctesting.ParsePacketFromEvents(result.Events) suite.Require().NoError(err) - suite.Require().NotNil(packet) + suite.Require().NotNil(packetFromCtoB) err = path2.EndpointA.UpdateClient() suite.Require().NoError(err) - result, err = path2.EndpointA.RecvPacketWithResult(packet) + result, err = path2.EndpointA.RecvPacketWithResult(packetFromCtoB) suite.Require().NoError(err) suite.Require().NotNil(result) // We have successfully received the packet on B and forwarded it to A. // Lets try to retrieve it in order to save it - forwardedPacket, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, packet.Sequence) + forwardedPacket, found := suite.chainB.GetSimApp().TransferKeeper.GetForwardedPacket(suite.chainB.GetContext(), path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, packetFromCtoB.Sequence) suite.Require().True(found) - suite.Require().Equal(packet, forwardedPacket) + suite.Require().Equal(packetFromCtoB, forwardedPacket) // Voucher have been burned on chain B coin = sdk.NewCoin(denomAB.IBCDenom(), amount) @@ -530,36 +528,37 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { // of denom // parse the packet from result events and recv packet on chainA - packet, err = ibctesting.ParsePacketFromEvents(result.Events) + packetFromBtoA, err := ibctesting.ParsePacketFromEvents(result.Events) suite.Require().NoError(err) - suite.Require().NotNil(packet) + suite.Require().NotNil(packetFromBtoA) - // manipulate escrow account for denom on chain A - coin = sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(99)) - suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), coin) - totalEscrowChainA = suite.chainA.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainA.GetContext(), coin.GetDenom()) - suite.Require().Equal(sdkmath.NewInt(99), totalEscrowChainA.Amount) + // turn off receive on chain A to trigger an error + suite.chainA.GetSimApp().TransferKeeper.SetParams(suite.chainA.GetContext(), types.Params{ + SendEnabled: true, + ReceiveEnabled: false, + }) err = path1.EndpointA.UpdateClient() suite.Require().NoError(err) - // suite.Require().Equal(packet, forwardedPacket) - result, err = path1.EndpointA.RecvPacketWithResult(packet) - suite.Require().Error(err) - suite.Require().Nil(result) - // In theory now an error ack should have been written on chain A - // NOW WE HAVE TO SEND ACK TO B, PROPAGTE ACK TO C, CHECK FINAL RESULTS + result, err = path1.EndpointA.RecvPacketWithResult(packetFromBtoA) + suite.Require().NoError(err) - // Reconstruct packet data - data, err := internal.UnmarshalPacketData(packet.Data, types.V2) + // An error ack has been written on chainA + // Now we need to propagate it back to chainB and chainC + packetSequenceOnA, err := ibctesting.ParsePacketSequenceFromEvents(result.Events) suite.Require().NoError(err) + errorAckOnA := channeltypes.NewErrorAcknowledgement(types.ErrReceiveDisabled) + errorAckCommitmentOnA := channeltypes.CommitAcknowledgement(errorAckOnA.Acknowledgement()) + ackOnC, found := suite.chainA.GetSimApp().GetIBCKeeper().ChannelKeeper.GetPacketAcknowledgement(suite.chainA.GetContext(), path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, packetSequenceOnA) + suite.Require().True(found) + suite.Require().Equal(errorAckCommitmentOnA, ackOnC) + err = path1.EndpointB.UpdateClient() suite.Require().NoError(err) - ack := channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed packet transfer")) - // err = path1.EndpointA.AcknowledgePacket(packetRecv, ack.Acknowledgement()) - err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, data, ack) + err = path1.EndpointB.AcknowledgePacket(packetFromBtoA, errorAckOnA.Acknowledgement()) suite.Require().NoError(err) // Check that Escrow B has been refunded amount @@ -567,31 +566,24 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { totalEscrowChainB = suite.chainB.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainB.GetContext(), coin.GetDenom()) suite.Require().Equal(sdkmath.NewInt(100), totalEscrowChainB.Amount) - denom := types.ExtractDenomFromPath(denomABC.Path()) - data = types.NewFungibleTokenPacketDataV2( - []types.Token{ - { - Denom: denom, - Amount: amount.String(), - }, - }, suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String(), suite.chainA.SenderAccounts[0].SenderAccount.GetAddress().String(), "", types.Forwarding{}) - // suite.chainC.SenderAccounts[0].SenderAccount.GetAddress().String() This should be forward account of B - packet = channeltypes.NewPacket(data.GetBytes(), 3, path2.EndpointB.ChannelConfig.PortID, path2.EndpointB.ChannelID, path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, clienttypes.NewHeight(1, 100), 0) - err = path2.EndpointB.UpdateClient() suite.Require().NoError(err) + errorAckOnB := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketFailed) + errorAckCommitmentOnB := channeltypes.CommitAcknowledgement(errorAckOnB.Acknowledgement()) + ackOnB := suite.chainB.GetAcknowledgement(packetFromCtoB) + suite.Require().Equal(errorAckCommitmentOnB, ackOnB) + // Check the status of account on chain C before executing ack. coin = sdk.NewCoin(denomABC.IBCDenom(), amount) postCoinOnC = suite.chainC.GetSimApp().BankKeeper.GetBalance(suite.chainC.GetContext(), suite.chainC.SenderAccounts[0].SenderAccount.GetAddress(), coin.GetDenom()) suite.Require().Equal(sdkmath.NewInt(0), postCoinOnC.Amount, "Final Hop balance has been refunded before Ack execution") // Execute ack - err = suite.chainC.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainC.GetContext(), packet, data, ack) - // err = path2.EndpointB.AcknowledgePacket(packet, ack.Acknowledgement()) + err = path2.EndpointB.AcknowledgePacket(packetFromCtoB, errorAckOnB.Acknowledgement()) suite.Require().NoError(err) - // Check that everythig has been reverted + // Check that everything has been reverted // // Check the vouchers transfer/channel-1/transfer/channel-0/denom have been refunded on C coin = sdk.NewCoin(denomABC.IBCDenom(), amount) @@ -748,7 +740,7 @@ func (suite *KeeperTestSuite) TestOnTimeoutPacketForwarding() { 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")) + ack := channeltypes.NewErrorAcknowledgement(types.ErrForwardedPacketTimedOut) ackbytes := channeltypes.CommitAcknowledgement(ack.Acknowledgement()) suite.Require().Equal(ackbytes, storedAck) diff --git a/modules/apps/transfer/types/errors.go b/modules/apps/transfer/types/errors.go index 70bbc6edc45..fc6b0dad5c2 100644 --- a/modules/apps/transfer/types/errors.go +++ b/modules/apps/transfer/types/errors.go @@ -17,4 +17,6 @@ var ( ErrInvalidAuthorization = errorsmod.Register(ModuleName, 10, "invalid transfer authorization") ErrInvalidMemo = errorsmod.Register(ModuleName, 11, "invalid memo") ErrInvalidForwarding = errorsmod.Register(ModuleName, 12, "invalid token forwarding") + ErrForwardedPacketTimedOut = errorsmod.Register(ModuleName, 13, "forwarded packet timed out") + ErrForwardedPacketFailed = errorsmod.Register(ModuleName, 14, "forwarded packet failed") ) diff --git a/testing/events.go b/testing/events.go index af585a87669..5954b5b4b51 100644 --- a/testing/events.go +++ b/testing/events.go @@ -174,6 +174,19 @@ func ParseProposalIDFromEvents(events []abci.Event) (uint64, error) { return 0, fmt.Errorf("proposalID event attribute not found") } +// ParsePacketSequenceFromEvents parses events emitted from MsgRecvPacket and returns the packet sequence +func ParsePacketSequenceFromEvents(events []abci.Event) (uint64, error) { + for _, event := range events { + for _, attribute := range event.Attributes { + if attribute.Key == "packet_sequence" { + return strconv.ParseUint(attribute.Value, 10, 64) + } + } + } + + return 0, fmt.Errorf("packet sequence event attribute not found") +} + // AssertEvents asserts that expected events are present in the actual events. func AssertEvents( suite *testifysuite.Suite,