use new port router with OnAcknowledgementPacket

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

@@ -21,6 +21,7 @@ var (
 	_ porttypes.Middleware            = (*IBCMiddleware)(nil)
 	_ porttypes.PacketDataUnmarshaler = (*IBCMiddleware)(nil)
 	_ porttypes.UpgradableModule      = (*IBCMiddleware)(nil)
+	_ porttypes.VersionWrapper        = (*IBCMiddleware)(nil)
 )
 
 // IBCMiddleware implements the ICS26 callbacks for the fee middleware given the
@@ -217,16 +218,6 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 		return types.ErrControllerSubModuleDisabled
 	}
 
-	connectionID, err := im.keeper.GetConnectionID(ctx, packet.GetSourcePort(), packet.GetSourceChannel())
-	if err != nil {
-		return err
-	}
-
-	// call underlying app's OnAcknowledgementPacket callback.
-	if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, packet.GetSourcePort(), connectionID) {
-		return im.app.OnAcknowledgementPacket(ctx, channelVersion, packet, acknowledgement, relayer)
-	}
-
 	return nil
 }
 
@@ -329,3 +320,9 @@ func (IBCMiddleware) UnwrapVersionUnsafe(version string) (string, string, error)
 	// ignore underlying app version
 	return version, "", nil
 }
+
+// UnwrapVersionSafe returns the version. Interchain accounts does not wrap versions.
+func (IBCMiddleware) UnwrapVersionSafe(ctx sdk.Context, portID, channelID, version string) (string, string) {
+	// ignore underlying app version
+	return version, ""
+}

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

@@ -514,50 +514,22 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 }
 
 func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
-	var (
-		path     *ibctesting.Path
-		isNilApp bool
-	)
+	var path *ibctesting.Path
 
 	testCases := []struct {
 		msg      string
 		malleate func()
-		expPass  bool
+		expError error
 	}{
 		{
 			"success",
 			func() {},
-			true,
+			nil,
 		},
 		{
 			"controller submodule disabled", func() {
 				suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false))
-			}, false,
-		},
-		{
-			"ICA auth module callback fails", func() {
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnAcknowledgementPacket = func(
-					ctx sdk.Context, channelVersion string, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress,
-				) error {
-					return fmt.Errorf("mock ica auth fails")
-				}
-			}, false,
-		},
-		{
-			"nil underlying app", func() {
-				isNilApp = true
-			}, true,
-		},
-		{
-			"middleware disabled", func() {
-				suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ConnectionID)
-
-				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnAcknowledgementPacket = func(
-					ctx sdk.Context, channelVersion string, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress,
-				) error {
-					return fmt.Errorf("error should be unreachable")
-				}
-			}, true,
+			}, types.ErrControllerSubModuleDisabled,
 		},
 	}
 
@@ -567,7 +539,6 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
 
 			suite.Run(tc.msg, func() {
 				suite.SetupTest() // reset
-				isNilApp = false
 
 				path = NewICAPath(suite.chainA, suite.chainB, ordering)
 				path.SetupConnections()
@@ -588,22 +559,22 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
 
 				tc.malleate() // malleate mutates test data
 
-				module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID)
-				suite.Require().NoError(err)
-
-				cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
+				cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.PacketRoute(path.EndpointA.ChannelConfig.PortID)
 				suite.Require().True(ok)
 
-				if isNilApp {
-					cbs = controller.NewIBCMiddleware(suite.chainA.GetSimApp().ICAControllerKeeper)
-				}
+				legacyModule, ok := cbs[0].(*porttypes.LegacyIBCModule)
+				suite.Require().True(ok, "expected there to be a single legacy ibc module")
+
+				legacyModuleCbs := legacyModule.GetCallbacks()
+				controllerModule, ok := legacyModuleCbs[1].(controller.IBCMiddleware) // controller module is routed second
+				suite.Require().True(ok)
 
-				err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), path.EndpointA.GetChannel().Version, packet, []byte("ack"), nil)
+				err = controllerModule.OnAcknowledgementPacket(suite.chainA.GetContext(), path.EndpointA.GetChannel().Version, packet, []byte("ack"), nil)
 
-				if tc.expPass {
+				if tc.expError == nil {
 					suite.Require().NoError(err)
 				} else {
-					suite.Require().Error(err)
+					suite.Require().ErrorIs(err, tc.expError)
 				}
 			})
 		}

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

@@ -305,7 +305,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenConfirm() {
 				suite.Require().True(ok, "expected there to be a single legacy ibc module")
 
 				legacyModuleCbs := legacyModule.GetCallbacks()
-				hostModule, ok := legacyModuleCbs[0].(icahost.IBCModule) // fee module is routed second
+				hostModule, ok := legacyModuleCbs[0].(icahost.IBCModule)
 				suite.Require().True(ok)
 
 				err = hostModule.OnChanOpenConfirm(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
@@ -557,10 +557,14 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
 
 				tc.malleate() // malleate mutates test data
 
-				module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
-				suite.Require().NoError(err)
+				cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.AppRouter.PacketRoute(path.EndpointB.ChannelConfig.PortID)
+				suite.Require().True(ok)
 
-				cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
+				legacyModule, ok := cbs[0].(*porttypes.LegacyIBCModule)
+				suite.Require().True(ok, "expected there to be a single legacy ibc module")
+
+				legacyModuleCbs := legacyModule.GetCallbacks()
+				hostModule, ok := legacyModuleCbs[0].(icahost.IBCModule)
 				suite.Require().True(ok)
 
 				packet := channeltypes.NewPacket(
@@ -574,7 +578,7 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
 					0,
 				)
 
-				err = cbs.OnAcknowledgementPacket(suite.chainB.GetContext(), path.EndpointB.GetChannel().Version, packet, []byte("ackBytes"), nil)
+				err = hostModule.OnAcknowledgementPacket(suite.chainB.GetContext(), path.EndpointB.GetChannel().Version, packet, []byte("ackBytes"), nil)
 
 				if tc.expPass {
 					suite.Require().NoError(err)

modules/apps/29-fee/ibc_middleware.go

@@ -19,10 +19,11 @@ import (
 )
 
 var (
-	_ porttypes.Middleware            = (*IBCMiddleware)(nil)
-	_ porttypes.PacketDataUnmarshaler = (*IBCMiddleware)(nil)
-	_ porttypes.UpgradableModule      = (*IBCMiddleware)(nil)
-	_ porttypes.VersionWrapper        = (*IBCMiddleware)(nil)
+	_ porttypes.Middleware             = (*IBCMiddleware)(nil)
+	_ porttypes.PacketDataUnmarshaler  = (*IBCMiddleware)(nil)
+	_ porttypes.UpgradableModule       = (*IBCMiddleware)(nil)
+	_ porttypes.VersionWrapper         = (*IBCMiddleware)(nil)
+	_ porttypes.AcknowledgementWrapper = (*IBCMiddleware)(nil)
 )
 
 // IBCMiddleware implements the ICS26 callbacks for the fee middleware given the
@@ -196,11 +197,9 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	relayer sdk.AccAddress,
 ) error {
 	if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) {
-		return im.app.OnAcknowledgementPacket(ctx, channelVersion, packet, acknowledgement, relayer)
+		return nil
 	}
 
-	appVersion := unwrapAppVersion(channelVersion)
-
 	var ack types.IncentivizedAcknowledgement
 	if err := json.Unmarshal(acknowledgement, &ack); err != nil {
 		return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS-29 incentivized packet acknowledgement %v: %s", ack, err)
@@ -215,14 +214,13 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 		// for fee enabled channels
 		//
 		// Please see ADR 004 for more information.
-		return im.app.OnAcknowledgementPacket(ctx, appVersion, packet, ack.AppAcknowledgement, relayer)
+		return nil
 	}
 
 	packetID := channeltypes.NewPacketID(packet.SourcePort, packet.SourceChannel, packet.Sequence)
 	feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
 	if !found {
-		// call underlying callback
-		return im.app.OnAcknowledgementPacket(ctx, appVersion, packet, ack.AppAcknowledgement, relayer)
+		return nil
 	}
 
 	payee, found := im.keeper.GetPayeeAddress(ctx, relayer.String(), packet.SourceChannel)
@@ -236,9 +234,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	}
 
 	im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, payeeAddr, feesInEscrow.PacketFees, packetID)
-
-	// call underlying callback
-	return im.app.OnAcknowledgementPacket(ctx, appVersion, packet, ack.AppAcknowledgement, relayer)
+	return nil
 }
 
 // OnTimeoutPacket implements the IBCMiddleware interface
@@ -392,3 +388,30 @@ func (IBCMiddleware) UnwrapVersionUnsafe(version string) (string, string, error)
 
 	return metadata.FeeVersion, metadata.AppVersion, nil
 }
+
+// UnwrapVersionSafe unwraps a version contextually by relying on storage and the given portID and channelID.
+func (im IBCMiddleware) UnwrapVersionSafe(ctx sdk.Context, portID, channelID, version string) (string, string) {
+	if !im.keeper.IsFeeEnabled(ctx, portID, channelID) {
+		return "", version
+	}
+	metadata, err := types.MetadataFromVersion(version)
+	if err != nil {
+		// This should not happen, as it would mean that the channel is broken. Only a severe bug would cause this.
+		panic(errorsmod.Wrap(err, "failed to unwrap app version from channel version"))
+	}
+	return metadata.FeeVersion, metadata.AppVersion
+}
+
+// UnwrapAcknowledgement unwraps an acnkowledgement contextually by relying on storage and the given portID and channelID.
+func (im IBCMiddleware) UnwrapAcknowledgement(ctx sdk.Context, portID, channelID string, ack []byte) ([]byte, []byte) {
+	if !im.keeper.IsFeeEnabled(ctx, portID, channelID) {
+		return nil, ack
+	}
+
+	var incentivizedAck types.IncentivizedAcknowledgement
+	if err := json.Unmarshal(ack, &incentivizedAck); err != nil {
+		panic(errorsmod.Wrap(err, "failed to unwrap acknowledgement"))
+	}
+
+	return ack, incentivizedAck.AppAcknowledgement
+}

modules/apps/29-fee/ibc_middleware_test.go

@@ -703,16 +703,6 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 			false,
 			func() {},
 		},
-		{
-			"application callback fails",
-			func() {
-				suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnAcknowledgementPacket = func(_ sdk.Context, _ string, _ channeltypes.Packet, _ []byte, _ sdk.AccAddress) error {
-					return fmt.Errorf("mock fee app callback fails")
-				}
-			},
-			false,
-			func() {},
-		},
 	}
 
 	for _, tc := range testCases {

modules/apps/callbacks/ibc_middleware.go

@@ -125,12 +125,6 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	acknowledgement []byte,
 	relayer sdk.AccAddress,
 ) error {
-	// we first call the underlying app to handle the acknowledgement
-	err := im.app.OnAcknowledgementPacket(ctx, channelVersion, packet, acknowledgement, relayer)
-	if err != nil {
-		return err
-	}
-
 	// OnAcknowledgementPacket is not blocked if the packet does not opt-in to callbacks
 	callbackData, err := types.GetSourceCallbackData(ctx, im.app, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetData(), im.maxCallbackGas)
 	if err != nil {

modules/apps/callbacks/ibc_middleware_test.go

@@ -258,14 +258,6 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 			callbackSuccess,
 			nil,
 		},
-		{
-			"failure: underlying app OnAcknolwedgePacket fails",
-			func() {
-				ack = []byte("invalid ack")
-			},
-			noExecution,
-			ibcerrors.ErrUnknownRequest,
-		},
 		{
 			"success: no-op on callback data is not valid",
 			func() {
@@ -345,11 +337,19 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 			tc.malleate()
 
 			// callbacks module is routed as top level middleware
-			transferStack, ok := s.chainA.App.GetIBCKeeper().PortKeeper.Route(transfertypes.ModuleName)
+			cbs, ok := s.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.PacketRoute(transfertypes.ModuleName)
+			s.Require().True(ok)
+
+			legacyModule, ok := cbs[0].(*porttypes.LegacyIBCModule)
+			s.Require().True(ok, "expected there to be a single legacy ibc module")
+
+			legacyModuleCbs := legacyModule.GetCallbacks()
+
+			callbacksModule, ok := legacyModuleCbs[1].(ibccallbacks.IBCMiddleware) // callbacks module is routed second
 			s.Require().True(ok)
 
 			onAcknowledgementPacket := func() error {
-				return transferStack.OnAcknowledgementPacket(ctx, s.path.EndpointA.GetChannel().Version, packet, ack, s.chainA.SenderAccount.GetAddress())
+				return callbacksModule.OnAcknowledgementPacket(ctx, s.path.EndpointA.GetChannel().Version, packet, ack, s.chainA.SenderAccount.GetAddress())
 			}
 
 			switch tc.expError {
@@ -388,7 +388,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 				s.Require().Equal(uint8(1), sourceStatefulCounter)
 
 				expEvent, exists := GetExpectedEvent(
-					ctx, transferStack.(porttypes.PacketDataUnmarshaler), gasLimit, packet.Data, packet.SourcePort,
+					ctx, callbacksModule, gasLimit, packet.Data, packet.SourcePort,
 					packet.SourcePort, packet.SourceChannel, packet.Sequence, types.CallbackTypeAcknowledgementPacket, nil,
 				)
 				s.Require().True(exists)

modules/core/05-port/types/ibc_legacy_module.go

@@ -239,13 +239,32 @@ func (LegacyIBCModule) OnRecvPacket(
 }
 
 // OnAcknowledgementPacket implements the IBCModule interface
-func (LegacyIBCModule) OnAcknowledgementPacket(
+func (im *LegacyIBCModule) OnAcknowledgementPacket(
 	ctx sdk.Context,
 	channelVersion string,
 	packet channeltypes.Packet,
 	acknowledgement []byte,
 	relayer sdk.AccAddress,
 ) error {
+	for i := len(im.cbs) - 1; i >= 0; i-- {
+		var (
+			cbVersion = channelVersion
+			cbAck     = acknowledgement
+		)
+
+		if wrapper, ok := im.cbs[i].(VersionWrapper); ok {
+			cbVersion, channelVersion = wrapper.UnwrapVersionSafe(ctx, packet.SourcePort, packet.SourceChannel, cbVersion)
+		}
+
+		if wrapper, ok := im.cbs[i].(AcknowledgementWrapper); ok {
+			cbAck, acknowledgement = wrapper.UnwrapAcknowledgement(ctx, packet.SourcePort, packet.SourceChannel, cbAck)
+		}
+
+		err := im.cbs[i].OnAcknowledgementPacket(ctx, cbVersion, packet, cbAck, relayer)
+		if err != nil {
+			return errorsmod.Wrap(err, "acknowledge packet callback failed")
+		}
+	}
 	return nil
 }
 

modules/core/05-port/types/module.go

@@ -131,12 +131,29 @@ type VersionWrapper interface {
 	WrapVersion(cbVersion, underlyingAppVersion string) string
 	// UnwrapVersionUnsafe is required in order to remove middleware wiring and the ICS4Wrapper
 	// while maintaining backwards compatibility. It will be removed in the future.
-	// Applications should unwrap the provided version with into their  application version.
+	// Applications should unwrap the provided version with into their application version.
 	// and the underlying application version. If they are unsuccessful they should return an error.
 	// UnwrapVersionUnsafe will be used during opening handshakes and channel upgrades when the version
 	// is still being negotiated.
 	UnwrapVersionUnsafe(string) (cbVersion, underlyingAppVersion string, err error)
-	// UnwrapVersionSafe(ctx sdk.Context, portID, channelID, version string) (appVersion, version string)
+	// UnwrapVersionSafe is required in order to remove middleware wiring and the ICS4Wrapper
+	// while maintaining backwards compatibility. It will be removed in the future.
+	// Applications should unwrap the provided version into their application version.
+	// They should use the context and associated portID and channelID to safely do so.
+	// UnwrapVersionSafe will be used during packet processing to provide callbacks
+	// their application version.
+	UnwrapVersionSafe(ctx sdk.Context, portID, channelID, version string) (cbVersion, underlyingAppVersion string)
+}
+
+// AcknowledgementWrapper is an optional interface which should be implemented by middlewares which wrap the acknowledgement
+// to ensure backwards compatibility.
+type AcknowledgementWrapper interface {
+	// UnwrapAcknowledgement is required in order to remove middleware wiring and the ICS4Wrapper
+	// while maintaining backwards compatibility. It will be removed in the future.
+	// Applications should unwrap the underlying app acknowledgement using the context
+	// and the given portID and channelID. They should return their application acknowledgement
+	// as the bytes it expects to decode in OnAcknowledgement.
+	UnwrapAcknowledgement(ctx sdk.Context, portID, channelID string, acknowledgment []byte) (cbAcknowledgement, underlyingAppAcknowledgement []byte)
 }
 
 // UpgradableModule defines the callbacks required to perform a channel upgrade.