diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go new file mode 100644 index 00000000000..52e31e6d606 --- /dev/null +++ b/modules/apps/transfer/keeper/forwarding.go @@ -0,0 +1,99 @@ +package keeper + +import ( + "errors" + + errorsmod "cosmossdk.io/errors" + sdkmath "cosmossdk.io/math" + + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" + channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" + host "github.com/cosmos/ibc-go/v8/modules/core/24-host" +) + +// 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 { + // 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 + if err := k.revertForwardedPacket(ctx, prevPacket, failedPacketData); err != nil { + return err + } + + forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet failed")) + return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck) +} + +// ackForwardPacketSuccess writes a successful async acknowledgement for the prevPacket +func (k Keeper) ackForwardPacketSuccess(ctx sdk.Context, prevPacket channeltypes.Packet) error { + forwardAck := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded")) + return k.acknowledgeForwardedPacket(ctx, prevPacket, 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 { + if err := k.revertForwardedPacket(ctx, prevPacket, timeoutPacketData); err != nil { + return err + } + + // the timeout is converted into an error acknowledgement in order to propagate the failed packet forwarding + // back to the original sender + forwardAck := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet timed out")) + return k.acknowledgeForwardedPacket(ctx, prevPacket, forwardAck) +} + +// acknowledgeForwardedPacket writes the async acknowledgement for packet +func (k Keeper) acknowledgeForwardedPacket(ctx sdk.Context, packet 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") + } + + return k.ics4Wrapper.WriteAcknowledgement(ctx, capability, packet, ack) +} + +// revertForwardedPacket reverts the logic of receive packet that occurs in the middle chains during a packet forwarding. +// If the packet fails to be forwarded all the way to the final destination, the state changes on this chain must be reverted +// before sending back the error acknowledgement to ensure atomic packet forwarding. +func (k Keeper) revertForwardedPacket(ctx sdk.Context, prevPacket channeltypes.Packet, failedPacketData types.FungibleTokenPacketDataV2) error { + /* + Recall that RecvPacket handles an incoming packet depending on the denom of the received funds: + 1. If the funds are native, then the amount is sent to the receiver from the escrow. + 2. If the funds are foreign, then a voucher token is minted. + We revert it in this function by: + 1. Sending funds back to escrow if the funds are native. + 2. Burning voucher tokens if the funds are foreign + */ + + forwardingAddr := types.GetForwardAddress(prevPacket.DestinationPort, prevPacket.DestinationChannel) + escrow := types.GetEscrowAddress(prevPacket.DestinationPort, prevPacket.DestinationChannel) + + // we can iterate over the received tokens of prevPacket by iterating over the sent tokens of failedPacketData + for _, token := range failedPacketData.Tokens { + // parse the transfer amount + transferAmount, ok := sdkmath.NewIntFromString(token.Amount) + if !ok { + return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount) + } + coin := sdk.NewCoin(token.Denom.IBCDenom(), transferAmount) + + // check if the token we received originated on the sender + // given that the packet is being reversed, we check the DestinationChannel and DestinationPort + // of the prevPacket to see if a hop was added to the trace during the receive step + if token.Denom.SenderChainIsSource(prevPacket.DestinationPort, prevPacket.DestinationChannel) { + // then send it back to the escrow address + if err := k.escrowCoin(ctx, forwardingAddr, escrow, coin); err != nil { + return err + } + + continue + } + + if err := k.burnCoin(ctx, forwardingAddr, coin); err != nil { + return err + } + } + return nil +} diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index c4957aae5e3..1b458408143 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -1,7 +1,6 @@ package keeper import ( - "errors" "fmt" "strings" @@ -320,44 +319,29 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t // acknowledgement was a success then nothing occurs. If the acknowledgement failed, // then the sender is refunded their tokens using the refundPacketToken function. func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) error { - prevPacket, found := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) - if found { - channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.SourcePort, packet.SourceChannel)) - if !ok { - return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") - } + prevPacket, isForwarded := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) - switch ack.Response.(type) { - case *channeltypes.Acknowledgement_Result: - // the acknowledgement succeeded on the receiving chain so - // we write the asynchronous acknowledgement for the sender - // of the previous packet. - fungibleTokenPacketAcknowledgement := channeltypes.NewResultAcknowledgement([]byte("forwarded packet succeeded")) - return k.ics4Wrapper.WriteAcknowledgement(ctx, channelCap, prevPacket, fungibleTokenPacketAcknowledgement) - case *channeltypes.Acknowledgement_Error: - // the forwarded packet has failed, thus the funds have been refunded to the forwarding address. - // we must revert the changes that came from successfully receiving the tokens on our chain - // before propogating the error acknowledgement back to original sender chain - if err := k.revertInFlightChanges(ctx, packet, prevPacket, data); err != nil { - return err - } + switch ack.Response.(type) { + case *channeltypes.Acknowledgement_Result: + if isForwarded { + return k.ackForwardPacketSuccess(ctx, prevPacket) + } - fungibleTokenPacketAcknowledgement := channeltypes.NewErrorAcknowledgement(errors.New("forwarded packet failed")) - return k.ics4Wrapper.WriteAcknowledgement(ctx, channelCap, prevPacket, fungibleTokenPacketAcknowledgement) - default: - return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response) + // the acknowledgement succeeded on the receiving chain so nothing + // needs to be executed and no error needs to be returned + return nil + case *channeltypes.Acknowledgement_Error: + // We refund the tokens from the escrow address to the sender + if err := k.refundPacketTokens(ctx, packet, data); err != nil { + return err } - } else { - switch ack.Response.(type) { - case *channeltypes.Acknowledgement_Result: - // the acknowledgement succeeded on the receiving chain so nothing - // needs to be executed and no error needs to be returned - return nil - case *channeltypes.Acknowledgement_Error: - return k.refundPacketTokens(ctx, packet, data) - default: - return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response) + 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) } } @@ -365,22 +349,16 @@ func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Pac // packet if the chain acted as a middle hop on a multihop transfer; or refunds // the sender if the original packet sent was never received and has been timed out. func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) error { - prevPacket, found := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) - if found { - channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.SourcePort, packet.SourceChannel)) - if !ok { - return errorsmod.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") - } - - if err := k.revertInFlightChanges(ctx, packet, prevPacket, data); err != nil { - return err - } + if err := k.refundPacketTokens(ctx, packet, data); err != nil { + return err + } - fungibleTokenPacketAcknowledgement := channeltypes.NewErrorAcknowledgement(fmt.Errorf("forwarded packet timed out")) - return k.ics4Wrapper.WriteAcknowledgement(ctx, channelCap, prevPacket, fungibleTokenPacketAcknowledgement) + prevPacket, isForwarded := k.GetForwardedPacket(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + if isForwarded { + return k.ackForwardPacketTimeout(ctx, prevPacket, data) } - return k.refundPacketTokens(ctx, packet, data) + return nil } // refundPacketTokens will unescrow and send back the tokens back to sender @@ -429,63 +407,6 @@ func (k Keeper) refundPacketTokens(ctx sdk.Context, packet channeltypes.Packet, return nil } -// revertInFlightChanges reverts the logic of receive packet and send packet -// that occurs in the middle chains during a packet forwarding. If an error -// occurs further down the line, the state changes on this chain must be -// reverted before sending back the error acknowledgement to ensure atomic packet forwarding. -func (k Keeper) revertInFlightChanges(ctx sdk.Context, sentPacket channeltypes.Packet, receivedPacket channeltypes.Packet, sentPacketData types.FungibleTokenPacketDataV2) error { - forwardEscrow := types.GetEscrowAddress(sentPacket.SourcePort, sentPacket.SourceChannel) - reverseEscrow := types.GetEscrowAddress(receivedPacket.DestinationPort, receivedPacket.DestinationChannel) - - // the token on our chain is the token in the sentPacket - for _, token := range sentPacketData.Tokens { - // parse the transfer amount - transferAmount, ok := sdkmath.NewIntFromString(token.Amount) - if !ok { - return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount) - } - coin := sdk.NewCoin(token.Denom.IBCDenom(), transferAmount) - - // check if the packet we sent out was sending as source or not - // if it is source, then we escrowed the outgoing tokens - if token.Denom.SenderChainIsSource(sentPacket.SourcePort, sentPacket.SourceChannel) { - // check if the packet we received was a source token for our chain - // check if here should be ReceiverChainIsSource - if token.Denom.SenderChainIsSource(receivedPacket.DestinationPort, receivedPacket.DestinationChannel) { - // receive sent tokens from the received escrow to the forward escrow account - // so we must send the tokens back from the forward escrow to the original received escrow account - return k.unescrowCoin(ctx, forwardEscrow, reverseEscrow, coin) - } - - // receive minted vouchers and sent to the forward escrow account - // so we must remove the vouchers from the forward escrow account and burn them - if err := k.bankKeeper.BurnCoins( - ctx, types.ModuleName, sdk.NewCoins(coin), - ); err != nil { - return err - } - } else { //nolint:gocritic - // in this case we burned the vouchers of the outgoing packets - // check if the packet we received was a source token for our chain - // in this case, the tokens were unescrowed from the reverse escrow account - if token.Denom.SenderChainIsSource(receivedPacket.DestinationPort, receivedPacket.DestinationChannel) { - // in this case we must mint the burned vouchers and send them back to the escrow account - if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(coin)); err != nil { - return err - } - - if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, reverseEscrow, sdk.NewCoins(coin)); err != nil { - panic(fmt.Errorf("unable to send coins from module to account despite previously minting coins to module account: %v", err)) - } - } - - // if it wasn't a source token on receive, then we simply had minted vouchers and burned them in the receive. - // So no state changes were made, and thus no reversion is necessary - } - } - return nil -} - // escrowCoin will send the given coin from the provided sender to the escrow address. It will also // update the total escrowed amount by adding the escrowed coin's amount to the current total escrow. func (k Keeper) escrowCoin(ctx sdk.Context, sender, escrowAddress sdk.AccAddress, coin sdk.Coin) error { @@ -572,3 +493,20 @@ func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string, return packetDataBytes } + +// burnCoin sends coins from the account to the transfer module account and then burn them. +// We do this because bankKeeper.BurnCoins only works with a module account in SDK v0.50, +// the next version of the SDK will allow burning coins from any account. +// TODO: remove this function once we switch forwarding address to a module account (#6561) +func (k Keeper) burnCoin(ctx sdk.Context, account sdk.AccAddress, coin sdk.Coin) error { + coins := sdk.NewCoins(coin) + if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, account, types.ModuleName, coins); err != nil { + return err + } + + if err := k.bankKeeper.BurnCoins(ctx, types.ModuleName, coins); err != nil { + return err + } + + return nil +} diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 880e9dbcf89..4647e87b554 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -7,6 +7,7 @@ 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" @@ -349,9 +350,6 @@ func (suite *KeeperTestSuite) TestSimplifiedHappyPathForwarding() { } // This test replicates the Acknowledgement Failure Scenario 5 -// Currently seems like the middle hop is not reverting state changes when an error occurs. -// In turn the final hop properly reverts changes. There may be an error in the way async ack are managed -// or in the way i'm trying to activate the OnAck function. func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { amount := sdkmath.NewInt(100) /* @@ -472,7 +470,7 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { // Now we want to trigger C -> B -> A // The coin we want to send out is exactly the one we received on C - // coin = sdk.NewCoin(denomTraceBC.IBCDenom(), amount) + coin = sdk.NewCoin(denomABC.IBCDenom(), amount) sender = suite.chainC.SenderAccounts[0].SenderAccount receiver = suite.chainA.SenderAccounts[0].SenderAccount // Receiver is the A chain account @@ -554,33 +552,23 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { // NOW WE HAVE TO SEND ACK TO B, PROPAGTE ACK TO C, CHECK FINAL RESULTS // Reconstruct packet data - denom := types.ExtractDenomFromPath(denomAB.Path()) - data := types.NewFungibleTokenPacketDataV2( - []types.Token{ - { - Denom: denom, - Amount: amount.String(), - }, - }, types.GetForwardAddress(path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID).String(), suite.chainA.SenderAccounts[0].SenderAccount.GetAddress().String(), "", nil) - packetRecv := channeltypes.NewPacket(data.GetBytes(), 3, path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, clienttypes.NewHeight(1, 100), 0) + data, err := internal.UnmarshalPacketData(packet.Data, types.V2) + suite.Require().NoError(err) 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(), packetRecv, data, ack) + err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket(suite.chainB.GetContext(), packet, data, ack) suite.Require().NoError(err) // Check that Escrow B has been refunded amount - // NOTE This is failing. The revertInFlightsChanges sohuld mint back voucher to chainBescrow - // but this is not happening. It may be a problem related with how we're writing async acks. - // coin = sdk.NewCoin(denomAB.IBCDenom(), amount) totalEscrowChainB = suite.chainB.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainB.GetContext(), coin.GetDenom()) suite.Require().Equal(sdkmath.NewInt(100), totalEscrowChainB.Amount) - denom = types.ExtractDenomFromPath(denomABC.Path()) + denom := types.ExtractDenomFromPath(denomABC.Path()) data = types.NewFungibleTokenPacketDataV2( []types.Token{ {