refactor: explicit checks in upgrade init and upgrade ack for ica con…

…troller Updated UpgradeInit logic Updated UpgradeAck logic Refactored UpgradeInit tests
cosmos · Dec 21, 2023 · 6fe19e1 · 6fe19e1
1 parent 947b15e
commit 6fe19e1
Show file tree

Hide file tree

Showing 2 changed files with 128 additions and 39 deletions.
diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go
@@ -142,17 +142,42 @@ func (Keeper) OnChanCloseConfirm(
 }
 
 // OnChanUpgradeInit performs the upgrade init step of the channel upgrade handshake.
+// The upgrade init callback must verify the proposed changes to the order, connectionHops, and version.
+// Within the version we have the tx type, encoding, interchain account address, host/controller connectionID's
+// and the interchain account version.
+//
+// The following may be changed:
+// - tx type (must be supported)
+// - encoding (must be supported)
+//
+// The following may not be changed:
+// - order
+// - connectionHops (and subsequently host/controller connectionIDs)
+// - interchain account address
+// - interchain account version
 func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, order channeltypes.Order, connectionHops []string, version string) (string, error) {
-	if strings.TrimSpace(version) == "" {
-		return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
-	}
-
+	// verify order has not changed
 	// support for unordered ICA channels is not implemented yet
 	if order != channeltypes.ORDERED {
 		return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
 	}
 
-	metadata, err := icatypes.MetadataFromVersion(version)
+	// verify connection hops has not changed
+	connectionID, err := k.GetConnectionID(ctx, portID, channelID)
+	if err != nil {
+		return "", err
+	}
+
+	if len(connectionHops) != 1 || connectionHops[0] != connectionID {
+		return "", errorsmod.Wrapf(channeltypes.ErrInvalidUpgrade, "expected connection hops %s, got %s", []string{connectionID}, connectionHops)
+	}
+
+	// verify proposed version only modifies tx type or encoding
+	if strings.TrimSpace(version) == "" {
+		return "", errorsmod.Wrap(icatypes.ErrInvalidVersion, "version cannot be empty")
+	}
+
+	proposedMetadata, err := icatypes.MetadataFromVersion(version)
 	if err != nil {
 		return "", err
 	}
@@ -162,30 +187,49 @@ func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, ord
 		return "", err
 	}
 
-	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, metadata); err != nil {
-		return "", errorsmod.Wrap(err, "invalid metadata")
+	// ValidateControllerMetadata will ensure the interchain account version has not changed and that the
+	// tx type and encoding are supported
+	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, connectionHops, proposedMetadata); err != nil {
+		return "", errorsmod.Wrap(err, "invalid upgrade metadata")
 	}
 
 	// the interchain account address on the host chain
 	// must remain the same after the upgrade.
-	if currentMetadata.Address != metadata.Address {
+	if currentMetadata.Address != proposedMetadata.Address {
 		return "", errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "interchain account address cannot be changed")
 	}
 
-	if currentMetadata.ControllerConnectionId != connectionHops[0] {
-		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed connection hop must not change")
+	if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
+		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed controller connection ID must not change")
+	}
+
+	if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
+		return "", errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed host connection ID must not change")
 	}
 
 	return version, nil
 }
 
 // OnChanUpgradeAck implements the ack setup of the channel upgrade handshake.
+// The upgrade ack callback must verify the proposed changes to the channel version.
+// Within the channel version we have the tx type, encoding, interchain account address, host/controller connectionID's
+// and the interchain account version.
+//
+// The following may be changed:
+// - tx type (must be supported)
+// - encoding (must be supported)
+//
+// The following may not be changed:
+// - controller connectionID
+// - host connectionID
+// - interchain account address
+// - interchain account version
 func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpartyVersion string) error {
 	if strings.TrimSpace(counterpartyVersion) == "" {
 		return errorsmod.Wrap(channeltypes.ErrInvalidChannelVersion, "counterparty version cannot be empty")
 	}
 
-	metadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
+	proposedMetadata, err := icatypes.MetadataFromVersion(counterpartyVersion)
 	if err != nil {
 		return err
 	}
@@ -195,28 +239,31 @@ func (k Keeper) OnChanUpgradeAck(ctx sdk.Context, portID, channelID, counterpart
 		return err
 	}
 
+	channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
+	if !found {
+		return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
+	}
+
+	// ValidateControllerMetadata will ensure the interchain account version has not changed and that the
+	// tx type and encoding are supported. Note, we pass in the current channel connection hops. The upgrade init
+	// step will verify that the proposed connection hops will not change.
+	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, proposedMetadata); err != nil {
+		return errorsmod.Wrap(err, "invalid upgrade metadata")
+	}
+
 	// the interchain account address on the host chain
 	// must remain the same after the upgrade.
-	if currentMetadata.Address != metadata.Address {
+	if currentMetadata.Address != proposedMetadata.Address {
 		return errorsmod.Wrap(icatypes.ErrInvalidAccountAddress, "address cannot be changed")
 	}
 
-	if currentMetadata.ControllerConnectionId != metadata.ControllerConnectionId {
+	if currentMetadata.ControllerConnectionId != proposedMetadata.ControllerConnectionId {
 		return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed controller connection ID must not change")
 	}
 
-	if currentMetadata.HostConnectionId != metadata.HostConnectionId {
+	if currentMetadata.HostConnectionId != proposedMetadata.HostConnectionId {
 		return errorsmod.Wrap(connectiontypes.ErrInvalidConnectionIdentifier, "proposed host connection ID must not change")
 	}
 
-	channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
-	if !found {
-		return errorsmod.Wrapf(channeltypes.ErrChannelNotFound, "failed to retrieve channel %s on port %s", channelID, portID)
-	}
-
-	if err := icatypes.ValidateControllerMetadata(ctx, k.channelKeeper, channel.ConnectionHops, metadata); err != nil {
-		return err
-	}
-
 	return nil
 }
diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go
@@ -1,6 +1,8 @@
 package keeper_test
 
 import (
+	"reflect"
+
 	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 	icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
 	connectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
@@ -495,25 +497,34 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
 			nil,
 		},
 		{
-			name: "failure: change ICA address",
+			name: "failure: invalid order",
 			malleate: func() {
-				metadata.Address = TestOwnerAddress
+				order = channeltypes.UNORDERED
 			},
-			expError: icatypes.ErrInvalidAccountAddress,
+			expError: channeltypes.ErrInvalidChannelOrdering,
 		},
 		{
-			name: "failure: change controller connection id",
+			name: "failure: connectionID not found",
 			malleate: func() {
-				metadata.ControllerConnectionId = differentConnectionID
+				// channelID is provided via the endpoint channelID
+				path.EndpointA.ChannelID = "invalid channel"
 			},
-			expError: connectiontypes.ErrInvalidConnection,
+			expError: channeltypes.ErrChannelNotFound,
 		},
 		{
-			name: "failure: change host connection id",
+			name: "failure: invalid proposed connectionHops",
 			malleate: func() {
-				metadata.HostConnectionId = differentConnectionID
+				// connection hops is provided via endpoint connectionID
+				path.EndpointA.ConnectionID = differentConnectionID
 			},
-			expError: connectiontypes.ErrInvalidConnection,
+			expError: channeltypes.ErrInvalidUpgrade,
+		},
+		{
+			name: "failure: empty version",
+			malleate: func() {
+				metadata = icatypes.Metadata{}
+			},
+			expError: icatypes.ErrInvalidVersion,
 		},
 		{
 			name: "failure: cannot decode version string",
@@ -525,18 +536,46 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
 			expError: icatypes.ErrUnknownDataType,
 		},
 		{
-			name: "failure: invalid connection hops",
+			name: "failure: failed controller metadata validation, invalid encoding",
 			malleate: func() {
-				path.EndpointA.ConnectionID = differentConnectionID
+				metadata.Encoding = "invalid encoding"
 			},
-			expError: connectiontypes.ErrConnectionNotFound,
+			expError: icatypes.ErrInvalidCodec,
 		},
 		{
-			name: "failure: invalid order",
+			name: "failure: failed controller metadata validation, invalid tx type",
 			malleate: func() {
-				order = channeltypes.UNORDERED
+				metadata.TxType = "invalid tx type"
 			},
-			expError: channeltypes.ErrInvalidChannelOrdering,
+			expError: icatypes.ErrUnknownDataType,
+		},
+		{
+			name: "failure: failed controller metadata validation, invalid interchain account version",
+			malleate: func() {
+				metadata.Version = "invalid interchain account version"
+			},
+			expError: icatypes.ErrInvalidVersion,
+		},
+		{
+			name: "failure: change ICA address",
+			malleate: func() {
+				metadata.Address = TestOwnerAddress
+			},
+			expError: icatypes.ErrInvalidAccountAddress,
+		},
+		{
+			name: "failure: change controller connection id",
+			malleate: func() {
+				metadata.ControllerConnectionId = differentConnectionID
+			},
+			expError: connectiontypes.ErrInvalidConnection,
+		},
+		{
+			name: "failure: change host connection id",
+			malleate: func() {
+				metadata.HostConnectionId = differentConnectionID
+			},
+			expError: connectiontypes.ErrInvalidConnection,
 		},
 	}
 
@@ -565,7 +604,10 @@ func (suite *KeeperTestSuite) TestOnChanUpgradeInit() {
 
 			tc.malleate() // malleate mutates test data
 
-			version := string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
+			var version string // allow for testing empty strings
+			if !reflect.DeepEqual(metadata, icatypes.Metadata{}) {
+				version = string(icatypes.ModuleCdc.MustMarshalJSON(&metadata))
+			}
 
 			upgradeVersion, err := path.EndpointA.Chain.GetSimApp().ICAControllerKeeper.OnChanUpgradeInit(
 				path.EndpointA.Chain.GetContext(),