chore: ics27 middleware callback routing (#2157)

* [WIP] add middleware enabled flags and conditional logic

* adapting private registerInterchainAccount func to accept portID in favour of owner

* updating tests

* cleaning up tests

* adding changelog

* updating tests: adding cbs with unreachable error returns for safety

* Update modules/apps/27-interchain-accounts/controller/keeper/keeper.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

CHANGELOG.md

@@ -93,6 +93,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (apps/27-interchain-accounts) [\#2102](https://github.com/cosmos/ibc-go/pull/2102) ICS27 controller middleware now supports a nil underlying application. This allows chains to make use of interchain accounts with existing auth mechanisms such as x/group and x/gov.
 * (apps/27-interchain-accounts) [\#2146](https://github.com/cosmos/ibc-go/pull/2146) ICS27 controller now claims the channel capability passed via ibc core, and passes `nil` to the underlying app callback. The channel capability arg in `SendTx` is now ignored and looked up internally.
 * (apps/27-interchain-accounts) [\#2134](https://github.com/cosmos/ibc-go/pull/2134) Adding upgrade handler to ICS27 `controller` submodule for migration of channel capabilities. This upgrade handler migrates ownership of channel capabilities from the underlying application to the ICS27 `controller` submodule.
+* (apps/27-interchain-accounts) [\#2157](https://github.com/cosmos/ibc-go/pull/2157) Adding `IsMiddlewareEnabled` functionality to enforce calls to ICS27 msg server to *not* route to the underlying application.
 
 ### Features
 

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

@@ -63,7 +63,7 @@ func (im IBCMiddleware) OnChanOpenInit(
 	// call underlying app's OnChanOpenInit callback with the passed in version
 	// the version returned is discarded as the ica-auth module does not have permission to edit the version string.
 	// ics27 will always return the version string containing the Metadata struct which is created during the `RegisterInterchainAccount` call.
-	if im.app != nil {
+	if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, channelID) {
 		if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, nil, counterparty, version); err != nil {
 			return "", err
 		}
@@ -108,7 +108,7 @@ func (im IBCMiddleware) OnChanOpenAck(
 	}
 
 	// call underlying app's OnChanOpenAck callback with the counterparty app version.
-	if im.app != nil {
+	if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, channelID) {
 		return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion)
 	}
 
@@ -167,7 +167,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	}
 
 	// call underlying app's OnAcknowledgementPacket callback.
-	if im.app != nil {
+	if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
 		return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer)
 	}
 
@@ -188,7 +188,7 @@ func (im IBCMiddleware) OnTimeoutPacket(
 		return err
 	}
 
-	if im.app != nil {
+	if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
 		return im.app.OnTimeoutPacket(ctx, packet, relayer)
 	}
 

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

@@ -124,6 +124,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 	var (
 		channel  *channeltypes.Channel
 		isNilApp bool
+		path     *ibctesting.Path
 	)
 
 	testCases := []struct {
@@ -185,6 +186,18 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 				isNilApp = true
 			}, true,
 		},
+		{
+			"middleware disabled", func() {
+				suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+
+				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
+					portID, channelID string, chanCap *capabilitytypes.Capability,
+					counterparty channeltypes.Counterparty, version string,
+				) (string, error) {
+					return "", fmt.Errorf("error should be unreachable")
+				}
+			}, true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -194,7 +207,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 			suite.SetupTest() // reset
 			isNilApp = false
 
-			path := NewICAPath(suite.chainA, suite.chainB)
+			path = NewICAPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupConnections(path)
 
 			// mock init interchain account
@@ -207,6 +220,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 			path.EndpointA.ChannelConfig.PortID = portID
 			path.EndpointA.ChannelID = ibctesting.FirstChannelID
 
+			suite.chainA.GetSimApp().ICAControllerKeeper.SetMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+
 			// default values
 			counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
 			channel = &channeltypes.Channel{
@@ -336,6 +351,17 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() {
 				isNilApp = true
 			}, true,
 		},
+		{
+			"middleware disabled", func() {
+				suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+
+				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenAck = func(
+					ctx sdk.Context, portID, channelID string, counterpartyChannelID string, counterpartyVersion string,
+				) error {
+					return fmt.Errorf("error should be unreachable")
+				}
+			}, true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -572,6 +598,17 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() {
 				isNilApp = true
 			}, true,
 		},
+		{
+			"middleware disabled", func() {
+				suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+
+				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnAcknowledgementPacket = func(
+					ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress,
+				) error {
+					return fmt.Errorf("error should be unreachable")
+				}
+			}, true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -654,6 +691,17 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
 				isNilApp = true
 			}, true,
 		},
+		{
+			"middleware disabled", func() {
+				suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
+
+				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnTimeoutPacket = func(
+					ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress,
+				) error {
+					return fmt.Errorf("error should be unreachable")
+				}
+			}, true,
+		},
 	}
 
 	for _, tc := range testCases {

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

@@ -17,22 +17,28 @@ import (
 // - An error is returned if the port identifier is already in use. Gaining access to interchain accounts whose channels
 // have closed cannot be done with this function. A regular MsgChannelOpenInit must be used.
 func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner, version string) error {
-	_, err := k.registerInterchainAccount(ctx, connectionID, owner, version)
-	return err
-}
-
-// registerInterchainAccount registers an interchain account, returning the channel id of the MsgChannelOpenInitResponse
-// and an error if one occurred.
-func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, owner, version string) (string, error) {
 	portID, err := icatypes.NewControllerPortID(owner)
 	if err != nil {
-		return "", err
+		return err
 	}
 
+	channelID, err := k.registerInterchainAccount(ctx, connectionID, portID, version)
+	if err != nil {
+		return err
+	}
+
+	k.SetMiddlewareEnabled(ctx, portID, channelID)
+
+	return nil
+}
+
+// registerInterchainAccount registers an interchain account, returning the channel id of the MsgChannelOpenInitResponse
+// and an error if one occurred.
+func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, portID, version string) (string, error) {
 	// if there is an active channel for this portID / connectionID return an error
 	activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionID, portID)
 	if found {
-		return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s for owner %s", activeChannelID, portID, connectionID, owner)
+		return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s", activeChannelID, portID, connectionID)
 	}
 
 	switch {

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

@@ -206,3 +206,21 @@ func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, connectionID, portI
 	store := ctx.KVStore(k.storeKey)
 	store.Set(icatypes.KeyOwnerAccount(portID, connectionID), []byte(address))
 }
+
+// IsMiddlewareEnabled returns true if the underlying application callbacks are enabled for given port and channel identifier pair, otherwise false
+func (k Keeper) IsMiddlewareEnabled(ctx sdk.Context, portID, channelID string) bool {
+	store := ctx.KVStore(k.storeKey)
+	return store.Has(icatypes.KeyIsMiddlewareEnabled(portID, channelID))
+}
+
+// SetMiddlewareEnabled stores a flag to indicate that the underlying application callbacks should be enabled for the given port and channel identifier pair
+func (k Keeper) SetMiddlewareEnabled(ctx sdk.Context, portID, channelID string) {
+	store := ctx.KVStore(k.storeKey)
+	store.Set(icatypes.KeyIsMiddlewareEnabled(portID, channelID), []byte{byte(1)})
+}
+
+// DeleteMiddlewareEnabled deletes the middleware enabled flag stored in state
+func (k Keeper) DeleteMiddlewareEnabled(ctx sdk.Context, portID, channelID string) {
+	store := ctx.KVStore(k.storeKey)
+	store.Delete(icatypes.KeyIsMiddlewareEnabled(portID, channelID))
+}

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

@@ -6,6 +6,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
+	icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types"
 )
 
 var _ types.MsgServer = Keeper{}
@@ -14,7 +15,12 @@ var _ types.MsgServer = Keeper{}
 func (k Keeper) RegisterAccount(goCtx context.Context, msg *types.MsgRegisterAccount) (*types.MsgRegisterAccountResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
-	channelID, err := k.registerInterchainAccount(ctx, msg.ConnectionId, msg.Owner, msg.Version)
+	portID, err := icatypes.NewControllerPortID(msg.Owner)
+	if err != nil {
+		return nil, err
+	}
+
+	channelID, err := k.registerInterchainAccount(ctx, msg.ConnectionId, portID, msg.Version)
 	if err != nil {
 		return nil, err
 	}

modules/apps/27-interchain-accounts/types/keys.go

@@ -36,6 +36,9 @@ var (
 
 	// PortKeyPrefix defines the key prefix used to store ports
 	PortKeyPrefix = "port"
+
+	// IsMiddlewareEnabledPrefix defines the key prefix used to store a flag for legacy API callback routing via ibc middleware
+	IsMiddlewareEnabledPrefix = "isMiddlewareEnabled"
 )
 
 // KeyActiveChannel creates and returns a new key used for active channels store operations
@@ -52,3 +55,8 @@ func KeyOwnerAccount(portID, connectionID string) []byte {
 func KeyPort(portID string) []byte {
 	return []byte(fmt.Sprintf("%s/%s", PortKeyPrefix, portID))
 }
+
+// KeyIsMiddlewareEnabled creates and returns a new key used for signaling legacy API callback routing via ibc middleware
+func KeyIsMiddlewareEnabled(portID, channelID string) []byte {
+	return []byte(fmt.Sprintf("%s/%s/%s", IsMiddlewareEnabledPrefix, portID, channelID))
+}

modules/apps/27-interchain-accounts/types/keys_test.go

@@ -1,7 +1,10 @@
 package types_test
 
 import (
+	fmt "fmt"
+
 	"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types"
+	ibctesting "github.com/cosmos/ibc-go/v5/testing"
 )
 
 func (suite *TypesTestSuite) TestKeyActiveChannel() {
@@ -13,3 +16,8 @@ func (suite *TypesTestSuite) TestKeyOwnerAccount() {
 	key := types.KeyOwnerAccount("port-id", "connection-id")
 	suite.Require().Equal("owner/port-id/connection-id", string(key))
 }
+
+func (suite *TypesTestSuite) TestKeyIsMiddlewareEnabled() {
+	key := types.KeyIsMiddlewareEnabled(ibctesting.MockPort, ibctesting.FirstChannelID)
+	suite.Require().Equal(fmt.Sprintf("%s/%s/%s", types.IsMiddlewareEnabledPrefix, ibctesting.MockPort, ibctesting.FirstChannelID), string(key))
+}