address serdar review

modules/apps/callbacks/ibc_middleware.go

@@ -9,6 +9,7 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
+	"github.com/cosmos/ibc-go/modules/apps/callbacks/internal"
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
@@ -113,7 +114,7 @@ func (im IBCMiddleware) SendPacket(
 		)
 	}
 
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeSendPacket, callbackData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeSendPacket, callbackData, callbackExecutor)
 	// contract keeper is allowed to reject the packet send.
 	if err != nil {
 		return 0, err
@@ -157,7 +158,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeAcknowledgementPacket, callbackData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeAcknowledgementPacket, callbackData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(),
 		types.CallbackTypeAcknowledgementPacket, callbackData, err,
@@ -191,7 +192,7 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx context.Context, channelVersion stri
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeTimeoutPacket, callbackData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeTimeoutPacket, callbackData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(),
 		types.CallbackTypeTimeoutPacket, callbackData, err,
@@ -228,7 +229,7 @@ func (im IBCMiddleware) OnRecvPacket(ctx context.Context, channelVersion string,
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
 		types.CallbackTypeReceivePacket, callbackData, err,
@@ -271,7 +272,7 @@ func (im IBCMiddleware) WriteAcknowledgement(
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
 		types.CallbackTypeReceivePacket, callbackData, err,

modules/apps/callbacks/ibc_middleware_test.go

@@ -11,6 +11,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	ibccallbacks "github.com/cosmos/ibc-go/modules/apps/callbacks"
+	"github.com/cosmos/ibc-go/modules/apps/callbacks/internal"
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp"
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	icacontrollertypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/controller/types"
@@ -954,7 +955,7 @@ func (s *CallbacksTestSuite) TestProcessCallback() {
 			var err error
 
 			processCallback := func() {
-				err = types.ProcessCallback(ctx, callbackType, callbackData, callbackExecutor)
+				err = internal.ProcessCallback(ctx, callbackType, callbackData, callbackExecutor)
 			}
 
 			expPass := tc.expValue == nil

modules/apps/callbacks/internal/process.go

@@ -0,0 +1,61 @@
+package internal
+
+import (
+	"fmt"
+
+	errorsmod "cosmossdk.io/errors"
+	storetypes "cosmossdk.io/store/types"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
+)
+
+// ProcessCallback executes the callbackExecutor and reverts contract changes if the callbackExecutor fails.
+//
+// Error Precedence and Returns:
+//   - oogErr: Takes the highest precedence. If the callback runs out of gas, an error wrapped with types.ErrCallbackOutOfGas is returned.
+//   - panicErr: Takes the second-highest precedence. If a panic occurs and it is not propagated, an error wrapped with types.ErrCallbackPanic is returned.
+//   - callbackErr: If the callbackExecutor returns an error, it is returned as-is.
+//
+// panics if
+//   - the contractExecutor panics for any reason, and the callbackType is SendPacket, or
+//   - the contractExecutor runs out of gas and the relayer has not reserved gas grater than or equal to
+//     CommitGasLimit.
+func ProcessCallback(
+	ctx sdk.Context, callbackType types.CallbackType,
+	callbackData types.CallbackData, callbackExecutor func(sdk.Context) error,
+) (err error) {
+	cachedCtx, writeFn := ctx.CacheContext()
+	cachedCtx = cachedCtx.WithGasMeter(storetypes.NewGasMeter(callbackData.ExecutionGasLimit))
+
+	defer func() {
+		// consume the minimum of g.consumed and g.limit
+		ctx.GasMeter().ConsumeGas(cachedCtx.GasMeter().GasConsumedToLimit(), fmt.Sprintf("ibc %s callback", callbackType))
+
+		// recover from all panics except during SendPacket callbacks
+		if r := recover(); r != nil {
+			if callbackType == types.CallbackTypeSendPacket {
+				panic(r)
+			}
+			err = errorsmod.Wrapf(types.ErrCallbackPanic, "ibc %s callback panicked with: %v", callbackType, r)
+		}
+
+		// if the callback ran out of gas and the relayer has not reserved enough gas, then revert the state
+		if cachedCtx.GasMeter().IsPastLimit() {
+			if callbackData.AllowRetry() {
+				panic(storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)})
+			}
+			err = errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType)
+		}
+
+		// allow the transaction to be committed, continuing the packet lifecycle
+	}()
+
+	err = callbackExecutor(cachedCtx)
+	if err == nil {
+		writeFn()
+	}
+
+	return err
+}

modules/apps/callbacks/testing/simapp/app.go

@@ -486,7 +486,7 @@ func NewSimApp(
 	ibcRouter.AddRoute(MockFeePort, feeWithMockModule)
 
 	// add transfer v2 module wrapped by callbacks v2 middleware
-	cbTransferModulev2 := ibccallbacksv2.NewIBCMiddleware(transferv2.NewIBCModule(app.TransferKeeper), nil, app.MockContractKeeper, app.IBCKeeper.ChannelKeeperV2, maxCallbackGas)
+	cbTransferModulev2 := ibccallbacksv2.NewIBCMiddleware(transferv2.NewIBCModule(app.TransferKeeper), app.IBCKeeper.ChannelKeeperV2, app.MockContractKeeper, app.IBCKeeper.ChannelKeeperV2, maxCallbackGas)
 	ibcRouterV2.AddRoute(ibctransfertypes.PortID, cbTransferModulev2)
 
 	// Seal the IBC Router

modules/apps/callbacks/types/callbacks.go

@@ -1,12 +1,10 @@
 package types
 
 import (
-	"fmt"
 	"strconv"
 	"strings"
 
 	errorsmod "cosmossdk.io/errors"
-	storetypes "cosmossdk.io/store/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
@@ -156,55 +154,6 @@ func GetCallbackData(
 	}, nil
 }
 
-// ProcessCallback executes the callbackExecutor and reverts contract changes if the callbackExecutor fails.
-//
-// Error Precedence and Returns:
-//   - oogErr: Takes the highest precedence. If the callback runs out of gas, an error wrapped with types.ErrCallbackOutOfGas is returned.
-//   - panicErr: Takes the second-highest precedence. If a panic occurs and it is not propagated, an error wrapped with types.ErrCallbackPanic is returned.
-//   - callbackErr: If the callbackExecutor returns an error, it is returned as-is.
-//
-// panics if
-//   - the contractExecutor panics for any reason, and the callbackType is SendPacket, or
-//   - the contractExecutor runs out of gas and the relayer has not reserved gas grater than or equal to
-//     CommitGasLimit.
-func ProcessCallback(
-	ctx sdk.Context, callbackType CallbackType,
-	callbackData CallbackData, callbackExecutor func(sdk.Context) error,
-) (err error) {
-	cachedCtx, writeFn := ctx.CacheContext()
-	cachedCtx = cachedCtx.WithGasMeter(storetypes.NewGasMeter(callbackData.ExecutionGasLimit))
-
-	defer func() {
-		// consume the minimum of g.consumed and g.limit
-		ctx.GasMeter().ConsumeGas(cachedCtx.GasMeter().GasConsumedToLimit(), fmt.Sprintf("ibc %s callback", callbackType))
-
-		// recover from all panics except during SendPacket callbacks
-		if r := recover(); r != nil {
-			if callbackType == CallbackTypeSendPacket {
-				panic(r)
-			}
-			err = errorsmod.Wrapf(ErrCallbackPanic, "ibc %s callback panicked with: %v", callbackType, r)
-		}
-
-		// if the callback ran out of gas and the relayer has not reserved enough gas, then revert the state
-		if cachedCtx.GasMeter().IsPastLimit() {
-			if callbackData.AllowRetry() {
-				panic(storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)})
-			}
-			err = errorsmod.Wrapf(ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType)
-		}
-
-		// allow the transaction to be committed, continuing the packet lifecycle
-	}()
-
-	err = callbackExecutor(cachedCtx)
-	if err == nil {
-		writeFn()
-	}
-
-	return err
-}
-
 func computeExecAndCommitGasLimit(callbackData map[string]interface{}, remainingGas, maxGas uint64) (uint64, uint64) {
 	// get the gas limit from the callback data
 	commitGasLimit := getUserDefinedGasLimit(callbackData)

modules/apps/callbacks/types/expected_keepers.go

@@ -8,7 +8,6 @@ import (
 	clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
 	channeltypesv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
-	"github.com/cosmos/ibc-go/v9/modules/core/api"
 	ibcexported "github.com/cosmos/ibc-go/v9/modules/core/exported"
 )
 
@@ -103,7 +102,6 @@ type ContractKeeper interface {
 }
 
 type ChannelKeeperV2 interface {
-	api.WriteAcknowledgementWrapper
 	GetAsyncPacket(
 		ctx context.Context,
 		clientID string,

modules/apps/callbacks/v2/ibc_middleware.go

@@ -9,6 +9,7 @@ import (
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
+	"github.com/cosmos/ibc-go/modules/apps/callbacks/internal"
 	"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
 	clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
@@ -49,6 +50,14 @@ func NewIBCMiddleware(
 		panic(errors.New("contract keeper cannot be nil"))
 	}
 
+	if writeAckWrapper == nil {
+		panic(errors.New("write acknowledgement wrapper cannot be nil"))
+	}
+
+	if chanKeeperV2 == nil {
+		panic(errors.New("channel keeper v2 cannot be nil"))
+	}
+
 	if maxCallbackGas == 0 {
 		panic(errors.New("maxCallbackGas cannot be zero"))
 	}
@@ -111,7 +120,7 @@ func (im IBCMiddleware) OnSendPacket(
 		)
 	}
 
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeSendPacket, cbData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeSendPacket, cbData, callbackExecutor)
 	// contract keeper is allowed to reject the packet send.
 	if err != nil {
 		return err
@@ -172,17 +181,13 @@ func (im IBCMiddleware) OnRecvPacket(
 			TimeoutTimestamp:   0,
 		}
 		// wrap the individual acknowledgement into the channeltypesv2.Acknowledgement since it implements the exported.Acknowledgement interface
-		var ack channeltypesv2.Acknowledgement
-		if recvResult.Status == channeltypesv2.PacketStatus_Failure {
-			ack = channeltypesv2.NewAcknowledgement(channeltypesv2.ErrorAcknowledgement[:])
-		} else {
-			ack = channeltypesv2.NewAcknowledgement(recvResult.Acknowledgement)
-		}
+		// since we return early on failure, we are guaranteed that the ack is a successful acknowledgement
+		ack := channeltypesv2.NewAcknowledgement(recvResult.Acknowledgement)
 		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packetv1, ack, cbData.CallbackAddress, payload.Version)
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, cbData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, cbData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, payload.DestinationPort, destinationClient, sequence,
 		types.CallbackTypeReceivePacket, cbData, err,
@@ -239,13 +244,18 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 			TimeoutHeight:      clienttypes.Height{},
 			TimeoutTimestamp:   0,
 		}
+		// NOTE: The callback is receiving the acknowledgement that the application received for its particular payload.
+		// In the case of a successful acknowledgement, this will be the acknowledgement sent by the counterparty application for the given payload
+		// In the case of an error acknowledgement, this will be the sentinel error acknowledgement bytes defined by IBC v2 protocol.
+		// Thus, the contract must be aware that the sentinel error acknowledgement signals a failed receive
+		// and the contract must handle this error case and the corresponding success case (ie ack != ErrorAcknowledgement) accordingly.
 		return im.contractKeeper.IBCOnAcknowledgementPacketCallback(
 			cachedCtx, packetv1, acknowledgement, relayer, cbData.CallbackAddress, cbData.SenderAddress, payload.Version,
 		)
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeAcknowledgementPacket, cbData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeAcknowledgementPacket, cbData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, payload.SourcePort, sourceClient, sequence,
 		types.CallbackTypeAcknowledgementPacket, cbData, err,
@@ -306,7 +316,7 @@ func (im IBCMiddleware) OnTimeoutPacket(
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeTimeoutPacket, cbData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeTimeoutPacket, cbData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, payload.SourcePort, sourceClient, sequence,
 		types.CallbackTypeTimeoutPacket, cbData, err,
@@ -331,21 +341,17 @@ func (im IBCMiddleware) WriteAcknowledgement(
 		return errorsmod.Wrapf(channeltypesv2.ErrInvalidAcknowledgement, "async packet not found for clientID (%s) and sequence (%d)", clientID, sequence)
 	}
 
-	// if WriteAckWrapper is wired, use the wrapper to write the acknowledgement
-	// otherwise, use the channel keeper v2 to write the acknowledgement directly
-	var writeAckWrapper api.WriteAcknowledgementWrapper
-	if im.writeAckWrapper != nil {
-		writeAckWrapper = im.writeAckWrapper
-	} else {
-		writeAckWrapper = im.chanKeeperV2
-	}
-	err := writeAckWrapper.WriteAcknowledgement(ctx, clientID, sequence, ack)
+	err := im.writeAckWrapper.WriteAcknowledgement(ctx, clientID, sequence, ack)
 	if err != nil {
 		return err
 	}
 
 	// NOTE: use first payload as the payload that is being handled by callbacks middleware
 	// must reconsider if multipacket data gets supported with async packets
+	// TRACKING ISSUE: https://github.com/cosmos/ibc-go/issues/7950
+	if len(packet.Payloads) != 1 {
+		return errorsmod.Wrapf(channeltypesv2.ErrInvalidAcknowledgement, "async packet has multiple payloads")
+	}
 	payload := packet.Payloads[0]
 
 	packetData, err := im.app.UnmarshalPacketData(payload)
@@ -393,7 +399,7 @@ func (im IBCMiddleware) WriteAcknowledgement(
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
-	err = types.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, cbData, callbackExecutor)
+	err = internal.ProcessCallback(sdkCtx, types.CallbackTypeReceivePacket, cbData, callbackExecutor)
 	types.EmitCallbackEvent(
 		sdkCtx, payload.DestinationPort, clientID, sequence,
 		types.CallbackTypeReceivePacket, cbData, err,