fix: remove NonrefundableKey feature (by codex)

Author: srdtrk

modules/apps/packet-forward-middleware/ibc_middleware.go

@@ -100,18 +100,6 @@ func getDenomForThisChain(port, channel, counterpartyPort, counterpartyChannel s
 	return denom.IBCDenom()
 }
 
-// getBoolFromAny returns the bool value is any is a valid bool, otherwise false.
-func getBoolFromAny(value any) bool {
-	if value == nil {
-		return false
-	}
-	boolVal, ok := value.(bool)
-	if !ok {
-		return false
-	}
-	return boolVal
-}
-
 // GetReceiver returns the receiver address for a given channel and original sender.
 // it overrides the receiver address to be a hash of the channel/origSender so that
 // the receiver address is deterministic and can be used to identify the sender on the
@@ -176,9 +164,6 @@ func (im *IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, pa
 
 	metadata := packetMetadata.Forward
 
-	goCtx := ctx.Context()
-	nonrefundable := getBoolFromAny(goCtx.Value(types.NonrefundableKey{}))
-
 	if err := metadata.Validate(); err != nil {
 		logger.Error("packetForwardMiddleware OnRecvPacket forward metadata is invalid", "error", err)
 		return newErrorAcknowledgement(err)
@@ -221,7 +206,7 @@ func (im *IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, pa
 		retries = im.retriesOnTimeout
 	}
 
-	err = im.keeper.ForwardTransferPacket(ctx, nil, packet, data.Sender, overrideReceiver, metadata, token, retries, timeout, []metrics.Label{}, nonrefundable)
+	err = im.keeper.ForwardTransferPacket(ctx, nil, packet, data.Sender, overrideReceiver, metadata, token, retries, timeout, []metrics.Label{})
 	if err != nil {
 		logger.Error("packetForwardMiddleware OnRecvPacket error forwarding packet", "error", err)
 		return newErrorAcknowledgement(err)

modules/apps/packet-forward-middleware/ibc_middleware_test.go

@@ -1,9 +1,8 @@
 package packetforward_test
 
 import (
-	"testing"
-
 	"github.com/stretchr/testify/suite"
+	"testing"
 
 	packetforward "github.com/cosmos/ibc-go/v11/modules/apps/packet-forward-middleware"
 	packetforwardkeeper "github.com/cosmos/ibc-go/v11/modules/apps/packet-forward-middleware/keeper"

modules/apps/packet-forward-middleware/keeper/genesis_test.go

@@ -17,7 +17,6 @@ func (s *KeeperTestSuite) TestGenesis() {
 
 		RetriesRemaining: 2,
 		Timeout:          10101010101,
-		Nonrefundable:    false,
 	}
 
 	key := types.RefundPacketKey(sampleInflight.PacketSrcChannelId, sampleInflight.PacketSrcPortId, sampleInflight.RefundSequence)

modules/apps/packet-forward-middleware/keeper/keeper.go

@@ -16,7 +16,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/cosmos/cosmos-sdk/types/bech32"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 
 	"github.com/cosmos/ibc-go/v11/modules/apps/packet-forward-middleware/types"
@@ -94,69 +93,6 @@ func (*Keeper) Logger(ctx sdk.Context) log.Logger {
 	return ctx.Logger().With("module", "x/"+ibcexported.ModuleName+"-"+types.ModuleName)
 }
 
-// moveFundsToUserRecoverableAccount will move the funds from the escrow account to the user recoverable account
-// this is only used when the maximum timeouts have been reached or there is an acknowledgement error and the packet is nonrefundable,
-// i.e. an operation has occurred to make the original packet funds inaccessible to the user, e.g. a swap.
-// We cannot refund the funds back to the original chain, so we move them to an account on this chain that the user can access.
-func (k *Keeper) moveFundsToUserRecoverableAccount(ctx sdk.Context, packet channeltypes.Packet, token transfertypes.Token, inFlightPacket *types.InFlightPacket) error {
-	amount, ok := sdkmath.NewIntFromString(token.GetAmount())
-	if !ok {
-		return fmt.Errorf("failed to parse amount from packet data for forward recovery: %s", token.GetAmount())
-	}
-	denom := token.GetDenom()
-	coin := sdk.NewCoin(denom.IBCDenom(), amount)
-
-	userAccount, err := k.userRecoverableAccount(inFlightPacket)
-	if err != nil {
-		return fmt.Errorf("failed to get user recoverable account: %w", err)
-	}
-
-	if !denom.HasPrefix(packet.SourcePort, packet.SourceChannel) {
-		// mint vouchers back to sender
-		if err := k.bankKeeper.MintCoins(ctx, transfertypes.ModuleName, sdk.NewCoins(coin)); err != nil {
-			return err
-		}
-
-		if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, transfertypes.ModuleName, userAccount, sdk.NewCoins(coin)); err != nil {
-			panic(fmt.Sprintf("unable to send coins from module to account despite previously minting coins to module account: %v", err))
-		}
-		return nil
-	}
-
-	escrowAddress := transfertypes.GetEscrowAddress(packet.SourcePort, packet.SourceChannel)
-
-	if err := k.bankKeeper.SendCoins(ctx, escrowAddress, userAccount, sdk.NewCoins(coin)); err != nil {
-		return fmt.Errorf("failed to send coins from escrow account to user recoverable account: %w", err)
-	}
-
-	// update the total escrow amount for the denom.
-	k.unescrowToken(ctx, coin)
-
-	return nil
-}
-
-// userRecoverableAccount finds an account on this chain that the original sender of the packet can recover funds from.
-// If the destination receiver of the original packet is a valid bech32 address for this chain, we use that address.
-// Otherwise, if the sender of the original packet is a valid bech32 address for another chain, we translate that address to this chain.
-// Note that for the fallback, the coin type of the source chain sender account must be compatible with this chain.
-func (k *Keeper) userRecoverableAccount(inFlightPacket *types.InFlightPacket) (sdk.AccAddress, error) {
-	var originalData transfertypes.FungibleTokenPacketData
-	err := transfertypes.ModuleCdc.UnmarshalJSON(inFlightPacket.PacketData, &originalData)
-	if err == nil { // if NO error
-		sender, err := k.addressCodec.StringToBytes(originalData.Receiver)
-		if err == nil { // if NO error
-			return sender, nil
-		}
-	}
-
-	_, sender, fallbackErr := bech32.DecodeAndConvert(inFlightPacket.OriginalSenderAddress)
-	if fallbackErr == nil { // if NO error
-		return sender, nil
-	}
-
-	return nil, fmt.Errorf("failed to decode bech32 addresses: %w", errors.Join(err, fallbackErr))
-}
-
 func (k *Keeper) WriteAcknowledgementForForwardedPacket(ctx sdk.Context, packet channeltypes.Packet, transferDetail transfertypes.InternalTransferRepresentation, inFlightPacket *types.InFlightPacket, ack channeltypes.Acknowledgement) error {
 	// Lookup module by channel capability
 	_, found := k.channelKeeper.GetChannel(ctx, inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId)
@@ -172,21 +108,6 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket(ctx sdk.Context, packet
 	// On an ack error or timeout on a forwarded packet, the funds in the escrow account
 	// should be moved to the other escrow account on the other side or burned.
 
-	// If this packet is non-refundable due to some action that took place between the initial ibc transfer and the forward
-	// we write a successful ack containing details on what happened regardless of ack error or timeout
-	if inFlightPacket.Nonrefundable {
-		// We are not allowed to refund back to the source chain.
-		// attempt to move funds to user recoverable account on this chain.
-		if err := k.moveFundsToUserRecoverableAccount(ctx, packet, transferDetail.Token, inFlightPacket); err != nil {
-			return err
-		}
-
-		ackResult := fmt.Sprintf("packet forward failed after point of no return: %s", ack.GetError())
-		newAck := channeltypes.NewResultAcknowledgement([]byte(ackResult))
-
-		return k.ics4Wrapper.WriteAcknowledgement(ctx, inFlightPacket.ChannelPacket(), newAck)
-	}
-
 	amount, ok := sdkmath.NewIntFromString(transferDetail.Token.GetAmount())
 	if !ok {
 		return fmt.Errorf("failed to parse amount from packet data for forward refund: %s", transferDetail.Token.GetAmount())
@@ -252,7 +173,7 @@ func (k *Keeper) unescrowToken(ctx sdk.Context, token sdk.Coin) {
 	k.transferKeeper.SetTotalEscrowForDenom(ctx, newTotalEscrow)
 }
 
-func (k *Keeper) ForwardTransferPacket(ctx sdk.Context, inFlightPacket *types.InFlightPacket, srcPacket channeltypes.Packet, srcPacketSender, receiver string, metadata types.ForwardMetadata, token sdk.Coin, maxRetries uint8, timeoutDelta time.Duration, labels []metrics.Label, nonrefundable bool) error {
+func (k *Keeper) ForwardTransferPacket(ctx sdk.Context, inFlightPacket *types.InFlightPacket, srcPacket channeltypes.Packet, srcPacketSender, receiver string, metadata types.ForwardMetadata, token sdk.Coin, maxRetries uint8, timeoutDelta time.Duration, labels []metrics.Label) error {
 	memo := ""
 
 	// set memo for next transfer with next from this transfer.
@@ -308,7 +229,6 @@ func (k *Keeper) ForwardTransferPacket(ctx sdk.Context, inFlightPacket *types.In
 
 			RetriesRemaining: int32(maxRetries),
 			Timeout:          uint64(timeoutDelta.Nanoseconds()),
-			Nonrefundable:    nonrefundable,
 		}
 	} else {
 		inFlightPacket.RetriesRemaining--
@@ -390,7 +310,7 @@ func (k *Keeper) RetryTimeout(ctx sdk.Context, channel, port string, transferDet
 	token := sdk.NewCoin(ibcDenom, amount)
 
 	// srcPacket and srcPacketSender are empty because inFlightPacket is non-nil.
-	return k.ForwardTransferPacket(ctx, inFlightPacket, channeltypes.Packet{}, "", transferDetail.Sender, metadata, token, uint8(inFlightPacket.RetriesRemaining), time.Duration(inFlightPacket.Timeout)*time.Nanosecond, nil, inFlightPacket.Nonrefundable)
+	return k.ForwardTransferPacket(ctx, inFlightPacket, channeltypes.Packet{}, "", transferDetail.Sender, metadata, token, uint8(inFlightPacket.RetriesRemaining), time.Duration(inFlightPacket.Timeout)*time.Nanosecond, nil)
 }
 
 func (k *Keeper) SetInflightPacket(ctx sdk.Context, channel, port string, sequence uint64, packet *types.InFlightPacket) error {

modules/apps/packet-forward-middleware/keeper/keeper_test.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"encoding/hex"
-	"fmt"
 	"testing"
 	"time"
 
@@ -59,31 +58,17 @@ func (s *KeeperTestSuite) TestWriteAcknowledgementForForwardedPacket() {
 	var expectedAckBz []byte
 
 	tests := []struct {
-		name          string
-		ack           channeltypes.Acknowledgement
-		malleate      func()
-		nonRefundable bool
+		name     string
+		ack      channeltypes.Acknowledgement
+		malleate func()
 	}{
 		{
-			name:          "Ack success -> propagated to ics4 wrapper",
-			ack:           channeltypes.NewResultAcknowledgement([]byte{1}),
-			nonRefundable: false,
+			name: "Ack success -> propagated to ics4 wrapper",
+			ack:  channeltypes.NewResultAcknowledgement([]byte{1}),
 		},
 		{
-			name: "Ack error + Non refundable -> Asset moved to recoverable account then propagate ack to ics4 wrapper",
+			name: "Ack error + Refundable -> Escrow coin then propagate ack to ics4 wrapper",
 			ack:  channeltypes.NewErrorAcknowledgement(nil),
-			malleate: func() {
-				ack := channeltypes.NewErrorAcknowledgement(nil)
-				ackResult := fmt.Sprintf("packet forward failed after point of no return: %s", ack.GetError())
-				newAck := channeltypes.NewResultAcknowledgement([]byte(ackResult))
-				expectedAckBz = channeltypes.CommitAcknowledgement(newAck.Acknowledgement())
-			},
-			nonRefundable: true,
-		},
-		{
-			name:          "Ack error + Refundable -> Escrow coin then propagate ack to ics4 wrapper",
-			ack:           channeltypes.NewErrorAcknowledgement(nil),
-			nonRefundable: false,
 		},
 	}
 
@@ -133,7 +118,7 @@ func (s *KeeperTestSuite) TestWriteAcknowledgementForForwardedPacket() {
 
 			fundAcc(ctxB, s.chainB.GetSimApp().BankKeeper, intermediateAcc)
 
-			err := pfmKeeperB.ForwardTransferPacket(ctxB, nil, srcPacket, initialSender.String(), intermediateAcc.String(), metadata, ibctesting.TestCoin, 2, timeout, nil, tc.nonRefundable)
+			err := pfmKeeperB.ForwardTransferPacket(ctxB, nil, srcPacket, initialSender.String(), intermediateAcc.String(), metadata, ibctesting.TestCoin, 2, timeout, nil)
 			s.Require().NoError(err)
 
 			inflightPacket, err := pfmKeeperB.GetInflightPacket(ctxB, srcPacket)
@@ -212,8 +197,6 @@ func (s *KeeperTestSuite) TestForwardTransferPacket() {
 
 			retries := uint8(2)
 			timeout := time.Duration(1010101010)
-			nonRefundable := false
-
 			metadata := pfmtypes.ForwardMetadata{
 				Receiver: "first-receiver",
 				Port:     path.EndpointA.ChannelConfig.PortID,
@@ -228,7 +211,7 @@ func (s *KeeperTestSuite) TestForwardTransferPacket() {
 
 			tc.malleate()
 
-			err := pfmKeeper.ForwardTransferPacket(ctx, nil, srcPacket, initialSender, finalReceiver, metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil, nonRefundable)
+			err := pfmKeeper.ForwardTransferPacket(ctx, nil, srcPacket, initialSender, finalReceiver, metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil)
 			s.Require().NoError(err)
 
 			// Get the inflight packer
@@ -238,7 +221,7 @@ func (s *KeeperTestSuite) TestForwardTransferPacket() {
 			s.Require().Equal(inflightPacket.RetriesRemaining, int32(retries))
 
 			// Call the same function again with inflight packet. Num retries should decrease.
-			err = pfmKeeper.ForwardTransferPacket(ctx, inflightPacket, srcPacket, initialSender, finalReceiver, metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil, nonRefundable)
+			err = pfmKeeper.ForwardTransferPacket(ctx, inflightPacket, srcPacket, initialSender, finalReceiver, metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil)
 			s.Require().NoError(err)
 
 			// Get the inflight packer
@@ -274,8 +257,6 @@ func (s *KeeperTestSuite) TestForwardTransferPacketWithNext() {
 
 	retries := uint8(2)
 	timeout := time.Duration(1010101010)
-	nonRefundable := false
-
 	// Test with valid metadata.Next - it should be a *PacketMetadata
 	nextPacketMetadata := &pfmtypes.PacketMetadata{
 		Forward: pfmtypes.ForwardMetadata{
@@ -300,7 +281,7 @@ func (s *KeeperTestSuite) TestForwardTransferPacketWithNext() {
 	initialSender := s.chainA.SenderAccount.GetAddress()
 	finalReceiver := s.chainB.SenderAccount.GetAddress()
 
-	err := pfmKeeper.ForwardTransferPacket(ctx, nil, srcPacket, initialSender.String(), finalReceiver.String(), metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil, nonRefundable)
+	err := pfmKeeper.ForwardTransferPacket(ctx, nil, srcPacket, initialSender.String(), finalReceiver.String(), metadata, sdk.NewInt64Coin("denom", 1000), 2, timeout, nil)
 	s.Require().NoError(err)
 
 	// Verify the inflight packet was created
@@ -332,7 +313,6 @@ func (s *KeeperTestSuite) TestRetryTimeoutErrorGettingNext() {
 	inFlightPacket := &pfmtypes.InFlightPacket{
 		RetriesRemaining: 1,
 		Timeout:          1000,
-		Nonrefundable:    false,
 	}
 
 	err := pfmKeeper.RetryTimeout(ctx, path.EndpointA.ChannelID, path.EndpointA.ChannelConfig.PortID, transferDetail, inFlightPacket)

modules/apps/packet-forward-middleware/keeper/migrator.go

@@ -4,6 +4,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v11/modules/apps/packet-forward-middleware/migrations/v3"
+	"github.com/cosmos/ibc-go/v11/modules/apps/packet-forward-middleware/migrations/v4"
 )
 
 // Migrator is a struct for handling in-place state migrations.
@@ -22,3 +23,9 @@ func NewMigrator(k *Keeper) Migrator {
 func (m Migrator) Migrate2to3(ctx sdk.Context) error {
 	return v3.Migrate(ctx, m.keeper.bankKeeper, m.keeper.channelKeeper, m.keeper.transferKeeper)
 }
+
+// Migrate3to4 migrates the module state from the consensus version 3 to
+// version 4.
+func (m Migrator) Migrate3to4(ctx sdk.Context) error {
+	return v4.Migrate(ctx, m.keeper.storeService, m.keeper.cdc)
+}