refactor: replace ack interface with result type for OnRecvPacket (#7093

)

* refactor: update OnRecvPacket to use result type in favour of ack interface

* refactor: update recv packet handling in interchain accounts, update event func arg to use concrete type

* api! remove Acknowledgement interface from exported pkg

* refactor: modifications to test code for api removal

* lint: fix duplicate imports

* refactor: use concrete acknowledgement in callbacks test

* doc: update inline godocs for OnRecvPacket in ibccallbacks

* fix: remove wrapping of result ack in callbacks recv packet

* test: refactor writeAck tests to use expected error format

* chore: add godocs and stringer impl for new recv result types

* chore: add issue link for async acknowledgements in 29-fee

* doc: update godocs for RecvPacketResult and status enums

* chore: address pr comments, test updates and inline docs

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -195,11 +195,14 @@ func (IBCMiddleware) OnRecvPacket(
 	_ string,
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
-) ibcexported.Acknowledgement {
+) ibcexported.RecvPacketResult {
 	err := errorsmod.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot receive packet on controller chain")
 	ack := channeltypes.NewErrorAcknowledgement(err)
 	keeper.EmitAcknowledgementEvent(ctx, packet, ack, err)
-	return ack
+	return ibcexported.RecvPacketResult{
+		Status:          ibcexported.Failure,
+		Acknowledgement: ack.Acknowledgement(),
+	}
 }
 
 // OnAcknowledgementPacket implements the IBCMiddleware interface
@@ -285,7 +288,7 @@ func (IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string
 func (IBCMiddleware) WriteAcknowledgement(
 	ctx sdk.Context,
 	packet ibcexported.PacketI,
-	ack ibcexported.Acknowledgement,
+	ack []byte,
 ) error {
 	panic(errors.New("WriteAcknowledgement not supported for ICA controller module"))
 }

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go

@@ -18,6 +18,7 @@ import (
 	porttypes "github.com/cosmos/ibc-go/v9/modules/core/05-port/types"
 	host "github.com/cosmos/ibc-go/v9/modules/core/24-host"
 	ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
+	"github.com/cosmos/ibc-go/v9/modules/core/exported"
 	ibctesting "github.com/cosmos/ibc-go/v9/testing"
 	ibcmock "github.com/cosmos/ibc-go/v9/testing/mock"
 )
@@ -450,12 +451,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanCloseConfirm() {
 
 func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 	testCases := []struct {
-		name     string
-		malleate func()
-		expPass  bool
+		name      string
+		malleate  func()
+		expStatus exported.RecvPacketStatus
 	}{
 		{
-			"ICA OnRecvPacket fails with ErrInvalidChannelFlow", func() {}, false,
+			"ICA OnRecvPacket fails with ErrInvalidChannelFlow", func() {}, exported.Failure,
 		},
 	}
 
@@ -492,8 +493,8 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 				)
 
 				ctx := suite.chainA.GetContext()
-				ack := cbs.OnRecvPacket(ctx, path.EndpointA.GetChannel().Version, packet, nil)
-				suite.Require().Equal(tc.expPass, ack.Success())
+				res := cbs.OnRecvPacket(ctx, path.EndpointA.GetChannel().Version, packet, nil)
+				suite.Require().Equal(tc.expStatus, res.Status, "expected %s but got %s", tc.expStatus, res.Status)
 
 				expectedEvents := sdk.Events{
 					sdk.NewEvent(

modules/apps/27-interchain-accounts/controller/keeper/events.go

@@ -7,12 +7,11 @@ import (
 
 	icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
 	channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
-	"github.com/cosmos/ibc-go/v9/modules/core/exported"
 )
 
 // EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error
 // details if any.
-func EmitAcknowledgementEvent(ctx sdk.Context, packet channeltypes.Packet, ack exported.Acknowledgement, err error) {
+func EmitAcknowledgementEvent(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement, err error) {
 	attributes := []sdk.Attribute{
 		sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
 		sdk.NewAttribute(icatypes.AttributeKeyControllerChannelID, packet.GetDestChannel()),

modules/apps/27-interchain-accounts/host/ibc_module.go

@@ -128,27 +128,41 @@ func (im IBCModule) OnRecvPacket(
 	_ string,
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
-) ibcexported.Acknowledgement {
+) ibcexported.RecvPacketResult {
 	if !im.keeper.GetParams(ctx).HostEnabled {
 		im.keeper.Logger(ctx).Info("host submodule is disabled")
 		keeper.EmitHostDisabledEvent(ctx, packet)
-		return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled)
+		return ibcexported.RecvPacketResult{
+			Status:          ibcexported.Failure,
+			Acknowledgement: channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled).Acknowledgement(),
+		}
 	}
 
 	txResponse, err := im.keeper.OnRecvPacket(ctx, packet)
-	ack := channeltypes.NewResultAcknowledgement(txResponse)
 	if err != nil {
-		ack = channeltypes.NewErrorAcknowledgement(err)
+		ack := channeltypes.NewErrorAcknowledgement(err)
+
+		// Emit an event indicating a successful or failed acknowledgement.
+		keeper.EmitAcknowledgementEvent(ctx, packet, ack, err)
 		im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", err.Error(), packet.Sequence))
-	} else {
-		im.keeper.Logger(ctx).Info("successfully handled packet", "sequence", packet.Sequence)
+
+		return ibcexported.RecvPacketResult{
+			Status:          ibcexported.Failure,
+			Acknowledgement: ack.Acknowledgement(),
+		}
 	}
 
+	ack := channeltypes.NewResultAcknowledgement(txResponse)
+
 	// Emit an event indicating a successful or failed acknowledgement.
 	keeper.EmitAcknowledgementEvent(ctx, packet, ack, err)
+	im.keeper.Logger(ctx).Info("successfully handled packet", "sequence", packet.Sequence)
 
 	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
-	return ack
+	return ibcexported.RecvPacketResult{
+		Status:          ibcexported.Success,
+		Acknowledgement: ack.Acknowledgement(),
+	}
 }
 
 // OnAcknowledgementPacket implements the IBCModule interface

modules/apps/27-interchain-accounts/host/ibc_module_test.go

@@ -409,8 +409,11 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 			"success with ICA auth module callback failure", func() {
 				suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnRecvPacket = func(
 					ctx sdk.Context, channelVersion string, packet channeltypes.Packet, relayer sdk.AccAddress,
-				) exported.Acknowledgement {
-					return channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed OnRecvPacket mock callback"))
+				) exported.RecvPacketResult {
+					return exported.RecvPacketResult{
+						Status:          exported.Failure,
+						Acknowledgement: channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed OnRecvPacket mock callback")).Acknowledgement(),
+					}
 				}
 			}, true,
 			"failed OnRecvPacket mock callback",
@@ -487,17 +490,17 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 				suite.Require().True(ok)
 
 				ctx := suite.chainB.GetContext()
-				ack := cbs.OnRecvPacket(ctx, path.EndpointB.GetChannel().Version, packet, nil)
+				res := cbs.OnRecvPacket(ctx, path.EndpointB.GetChannel().Version, packet, nil)
 
 				expectedAttributes := []sdk.Attribute{
 					sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
 					sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()),
-					sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, strconv.FormatBool(ack.Success())),
+					sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, strconv.FormatBool(res.Status == exported.Success)),
 				}
 
 				if tc.expAckSuccess {
-					suite.Require().True(ack.Success())
-					suite.Require().Equal(expectedAck, ack)
+					suite.Require().Equal(exported.Success, res.Status)
+					suite.Require().Equal(expectedAck.Acknowledgement(), res.Acknowledgement)
 
 					expectedEvents := sdk.Events{
 						sdk.NewEvent(
@@ -510,7 +513,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 					ibctesting.AssertEvents(&suite.Suite, expectedEvents, ctx.EventManager().Events().ToABCIEvents())
 
 				} else {
-					suite.Require().False(ack.Success())
+					suite.Require().Equal(exported.Failure, res.Status)
 
 					expectedAttributes = append(expectedAttributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, tc.eventErrorMsg))
 					expectedEvents := sdk.Events{

modules/apps/27-interchain-accounts/host/keeper/events.go

@@ -8,12 +8,11 @@ import (
 	"github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v9/modules/apps/27-interchain-accounts/types"
 	channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
-	"github.com/cosmos/ibc-go/v9/modules/core/exported"
 )
 
 // EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error
 // details if any.
-func EmitAcknowledgementEvent(ctx sdk.Context, packet channeltypes.Packet, ack exported.Acknowledgement, err error) {
+func EmitAcknowledgementEvent(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement, err error) {
 	attributes := []sdk.Attribute{
 		sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
 		sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()),

modules/apps/29-fee/ibc_middleware.go

@@ -163,24 +163,27 @@ func (im IBCMiddleware) OnRecvPacket(
 	channelVersion string,
 	packet channeltypes.Packet,
 	relayer sdk.AccAddress,
-) exported.Acknowledgement {
+) exported.RecvPacketResult {
 	if !im.keeper.IsFeeEnabled(ctx, packet.DestinationPort, packet.DestinationChannel) {
 		return im.app.OnRecvPacket(ctx, channelVersion, packet, relayer)
 	}
 
 	appVersion := unwrapAppVersion(channelVersion)
-	ack := im.app.OnRecvPacket(ctx, appVersion, packet, relayer)
+	res := im.app.OnRecvPacket(ctx, appVersion, packet, relayer)
 
-	// in case of async acknowledgement (ack == nil) store the relayer address for use later during async WriteAcknowledgement
-	if ack == nil {
+	// in case of async result status store the relayer address for use later during async WriteAcknowledgement
+	if res.Status == exported.Async {
 		im.keeper.SetRelayerAddressForAsyncAck(ctx, channeltypes.NewPacketID(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()), relayer.String())
-		return nil
+		return res
 	}
 
 	// if forwardRelayer is not found we refund recv_fee
 	forwardRelayer, _ := im.keeper.GetCounterpartyPayeeAddress(ctx, relayer.String(), packet.GetDestChannel())
 
-	return types.NewIncentivizedAcknowledgement(forwardRelayer, ack.Acknowledgement(), ack.Success())
+	return exported.RecvPacketResult{
+		Status:          res.Status,
+		Acknowledgement: types.NewIncentivizedAcknowledgement(forwardRelayer, res.Acknowledgement, res.Status == exported.Success).Acknowledgement(),
+	}
 }
 
 // OnAcknowledgementPacket implements the IBCMiddleware interface
@@ -331,7 +334,7 @@ func (im IBCMiddleware) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID str
 func (im IBCMiddleware) WriteAcknowledgement(
 	ctx sdk.Context,
 	packet exported.PacketI,
-	ack exported.Acknowledgement,
+	ack []byte,
 ) error {
 	return im.keeper.WriteAcknowledgement(ctx, packet, ack)
 }

modules/apps/29-fee/ibc_middleware_test.go

@@ -421,8 +421,10 @@ func (suite *FeeTestSuite) TestOnRecvPacket() {
 					channelVersion string,
 					packet channeltypes.Packet,
 					relayer sdk.AccAddress,
-				) exported.Acknowledgement {
-					return nil
+				) exported.RecvPacketResult {
+					return exported.RecvPacketResult{
+						Status: exported.Async,
+					}
 				}
 			},
 			true,
@@ -482,13 +484,13 @@ func (suite *FeeTestSuite) TestOnRecvPacket() {
 					ForwardRelayerAddress: forwardAddr,
 					UnderlyingAppSuccess:  true,
 				}
-				suite.Require().Equal(expectedAck, result)
+				suite.Require().Equal(expectedAck.Acknowledgement(), result.Acknowledgement)
 
 			case !tc.feeEnabled:
-				suite.Require().Equal(ibcmock.MockAcknowledgement, result)
+				suite.Require().Equal(ibcmock.MockAcknowledgement.Acknowledgement(), result.Acknowledgement)
 
-			case tc.forwardRelayer && result == nil:
-				suite.Require().Equal(nil, result)
+			case tc.forwardRelayer && result.Status == exported.Async:
+				suite.Require().Nil(result.Acknowledgement)
 				packetID := channeltypes.NewPacketID(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
 
 				// retrieve the forward relayer that was stored in `onRecvPacket`
@@ -501,7 +503,7 @@ func (suite *FeeTestSuite) TestOnRecvPacket() {
 					ForwardRelayerAddress: "",
 					UnderlyingAppSuccess:  true,
 				}
-				suite.Require().Equal(expectedAck, result)
+				suite.Require().Equal(expectedAck.Acknowledgement(), result.Acknowledgement)
 			}
 		})
 	}

modules/apps/29-fee/keeper/relay.go

@@ -14,7 +14,7 @@ import (
 
 // WriteAcknowledgement wraps IBC ChannelKeeper's WriteAcknowledgement function
 // ICS29 WriteAcknowledgement is used for asynchronous acknowledgements
-func (k Keeper) WriteAcknowledgement(ctx sdk.Context, packet ibcexported.PacketI, acknowledgement ibcexported.Acknowledgement) error {
+func (k Keeper) WriteAcknowledgement(ctx sdk.Context, packet ibcexported.PacketI, acknowledgement []byte) error {
 	if !k.IsFeeEnabled(ctx, packet.GetDestPort(), packet.GetDestChannel()) {
 		// ics4Wrapper may be core IBC or higher-level middleware
 		return k.ics4Wrapper.WriteAcknowledgement(ctx, packet, acknowledgement)
@@ -32,12 +32,14 @@ func (k Keeper) WriteAcknowledgement(ctx sdk.Context, packet ibcexported.PacketI
 	// if there is no registered counterparty address then write acknowledgement with empty relayer address and refund recv_fee.
 	forwardRelayer, _ := k.GetCounterpartyPayeeAddress(ctx, relayer, packet.GetDestChannel())
 
-	ack := types.NewIncentivizedAcknowledgement(forwardRelayer, acknowledgement.Acknowledgement(), acknowledgement.Success())
+	// TODO(https://github.com/cosmos/ibc-go/issues/7044):
+	// The underlying app success bool is temporarily hardcoded to true! This should be revisited for the issue linked above.
+	ack := types.NewIncentivizedAcknowledgement(forwardRelayer, acknowledgement, true)
 
 	k.DeleteForwardRelayerAddress(ctx, packetID)
 
 	// ics4Wrapper may be core IBC or higher-level middleware
-	return k.ics4Wrapper.WriteAcknowledgement(ctx, packet, ack)
+	return k.ics4Wrapper.WriteAcknowledgement(ctx, packet, ack.Acknowledgement())
 }
 
 // GetAppVersion returns the underlying application version.

modules/apps/29-fee/keeper/relay_test.go

@@ -58,7 +58,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgementAsync() {
 			// malleate test case
 			tc.malleate()
 
-			err := suite.chainB.GetSimApp().IBCFeeKeeper.WriteAcknowledgement(suite.chainB.GetContext(), packet, ack)
+			err := suite.chainB.GetSimApp().IBCFeeKeeper.WriteAcknowledgement(suite.chainB.GetContext(), packet, ack.Acknowledgement())
 
 			if tc.expPass {
 				suite.Require().NoError(err)
@@ -95,7 +95,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgementAsyncFeeDisabled() {
 
 	ack := channeltypes.NewResultAcknowledgement([]byte("success"))
 
-	err := suite.chainB.GetSimApp().IBCFeeKeeper.WriteAcknowledgement(suite.chainB.GetContext(), packet, ack)
+	err := suite.chainB.GetSimApp().IBCFeeKeeper.WriteAcknowledgement(suite.chainB.GetContext(), packet, ack.Acknowledgement())
 	suite.Require().NoError(err)
 
 	packetAck, _ := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetPacketAcknowledgement(suite.chainB.GetContext(), packet.DestinationPort, packet.DestinationChannel, 1)

modules/apps/callbacks/ibc_middleware.go

@@ -188,23 +188,24 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx sdk.Context, channelVersion string,
 // It defers to the underlying application and then calls the contract callback.
 // If the contract callback runs out of gas and may be retried with a higher gas limit then the state changes are
 // reverted via a panic.
-func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, packet channeltypes.Packet, relayer sdk.AccAddress) ibcexported.Acknowledgement {
-	ack := im.app.OnRecvPacket(ctx, channelVersion, packet, relayer)
-	// if ack is nil (asynchronous acknowledgements), then the callback will be handled in WriteAcknowledgement
-	// if ack is not successful, all state changes are reverted. If a packet cannot be received, then there is
-	// no need to execute a callback on the receiving chain.
-	if ack == nil || !ack.Success() {
-		return ack
+func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, packet channeltypes.Packet, relayer sdk.AccAddress) ibcexported.RecvPacketResult {
+	res := im.app.OnRecvPacket(ctx, channelVersion, packet, relayer)
+	// if result status is asynchronous, then the callback will be handled in WriteAcknowledgement
+	// if result status is failed, then all state changes are reverted.
+	// if a packet cannot be received, then there is no need to execute a callback on the receiving chain,
+	// thus we only proceed with the contract keeper callback if the result status is successful.
+	if res.Status != ibcexported.Success {
+		return res
 	}
 
 	// OnRecvPacket is not blocked if the packet does not opt-in to callbacks
 	callbackData, err := types.GetDestCallbackData(ctx, im.app, packet, im.maxCallbackGas)
 	if err != nil {
-		return ack
+		return res
 	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
-		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress, callbackData.ApplicationVersion)
+		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, res.Acknowledgement, callbackData.CallbackAddress, callbackData.ApplicationVersion)
 	}
 
 	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
@@ -214,7 +215,7 @@ func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, pac
 		types.CallbackTypeReceivePacket, callbackData, err,
 	)
 
-	return ack
+	return res
 }
 
 // WriteAcknowledgement implements the ReceivePacket destination callbacks for the ibc-callbacks middleware
@@ -225,7 +226,7 @@ func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, pac
 func (im IBCMiddleware) WriteAcknowledgement(
 	ctx sdk.Context,
 	packet ibcexported.PacketI,
-	ack ibcexported.Acknowledgement,
+	ack []byte,
 ) error {
 	err := im.ics4Wrapper.WriteAcknowledgement(ctx, packet, ack)
 	if err != nil {