Fix and simplify reverts of forwarding state (#6574)

srdtrk · web-flow · commit 6b3b5aa38f42 · 2024-06-13T12:52:45.000+03:00
* refactor: initial simplification attempt

* refactor: further organize

* fix: all tests fixed

* docs: improved godocs

* fix: logic and testing error

* style: self suggestion

* docs: suggestion

* docs: spellcheck

* style: suggestions

* style: renamed to revertForwardedPacket

* style: suggestion

* docs: remove docs

* docs: godoc suggestion

* style: suggestion

* docs: colin suggestions
diff --git a/modules/apps/transfer/keeper/forwarding.go 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
@@ -1,7 +1,6 @@
 package keeper
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 
@@ -320,67 +319,46 @@ 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)
 	}
 }
 
 // OnTimeoutPacket either reverts the state changes executed in receive and send
 // 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
@@ -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{
 			{
