feat(ica): allow unordered ica channels (backport #5633) (#5647)

* feat(ica): allow unordered ica channels (#5633) * Remove order check for ICA host and controller upgrade callbacks (#5561) * imp: remove the channel type = ordered checks from both host and controller (#5578) * rm checks and tests, amend docustring * rm unnecessary test * When a channel reopens the ordering and metadata must not change (#5562) * chore: require active channel be CLOSED before re-opening. (#5601) * docs: Update ICA documentation with support for unordered channels (#5607) * Allow specifying order when registering ICA account (#5608) * proto: Add Order to MsgRegisterInterchainAccount. * chore: apply proto changes to go files. * Add ordering to cli tx for Register. * Add documentation line for tx now accepting ordering. * Address feedback review. Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Address Cian's feedback; spacing. --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * docs: ICA register CLI (#5625) * imp(ica/host): removed previous version validation check (#5613) * imp: removed validation check * test: updated icahost test * docs: added godocs * docs: added godocs * chore(ica/host): require active channel be CLOSED before re-opening (#5630) * chore(ica/host): require active channel be CLOSED before re-opening * Update modules/apps/27-interchain-accounts/host/keeper/handshake_test.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * e2e: ordered ica channel is upgraded to unordered (#5616) * E2E test where unordered channel is used with ICA (#5566) * test: add test to use an unordered ICA channel * chore: add hard coded UNORDERED channel Order * proto: Add Order to MsgRegisterInterchainAccount. * chore: apply proto changes to go files. * chore: apply proto changes to go files. * chore: e2e test passing with hard coded ordered value * Add ordering to cli tx for Register. * Add documentation line for tx now accepting ordering. * Address feedback review. Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * Address Cian's feedback; spacing. * Update e2e/tests/interchain_accounts/base_test.go Co-authored-by: Charly <charly@interchain.io> --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Charly <charly@interchain.io> * fix: host chan open try test (#5632) * chore: fix linter and merge main * chore: doc lint issue fix * docs: add extra information about ICA channel reopening (#5631) * docs: add extra information about ICA channel reopening * add link to active channels section * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * Update 01-overview.md --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> * chore: rm order checks reintroduced after merge conflict. * e2e: comment out failing e2e * chore: lintertroubles --------- Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: Charly <charly@interchain.io> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: chatton <github.qpeyb@simplelogin.fr> Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com> (cherry picked from commit 6174822) # Conflicts: # docs/docs/02-apps/02-interchain-accounts/01-overview.md # docs/docs/02-apps/02-interchain-accounts/05-messages.md # docs/docs/02-apps/02-interchain-accounts/08-client.md # docs/docs/02-apps/02-interchain-accounts/09-active-channels.md # e2e/tests/core/04-channel/upgrades_test.go # e2e/tests/interchain_accounts/base_test.go # e2e/tests/interchain_accounts/gov_test.go # e2e/tests/interchain_accounts/groups_test.go # e2e/tests/interchain_accounts/incentivized_test.go # e2e/tests/interchain_accounts/localhost_test.go # e2e/tests/interchain_accounts/params_test.go # e2e/tests/upgrades/genesis_test.go # e2e/testsuite/testconfig.go # modules/apps/27-interchain-accounts/controller/types/msgs_test.go * chore: rm docs, e2e folders. * chore: Fix conflicts. Conflicts in controller's msgs_tests.go due to 8.1 using old approach for getting Signers. * fix: Remove API breaks on backport. Introduce NewMsgRegisterInterchainAccountWithOrder to allow Order when creating a new instance of the message and remove Order argument from NewMsgRegisterInterchainAccount. Amend testing function to use old function since they all hard-coded ORDERED as the argument for order. * Update msgs.go * add changelog --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
cosmos · Jan 19, 2024 · 3c95be2 · 3c95be2
1 parent 24be0e1
commit 3c95be2
Show file tree

Hide file tree

Showing 13 changed files with 277 additions and 179 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Features
 
 * (apps/transfer) [\#5280](https://github.com/cosmos/ibc-go/pull/5280) Add list of allowed packet data keys to `Allocation` of `TransferAuthorization`.
+* (apps/27-interchain-accounts) [\#5633](https://github.com/cosmos/ibc-go/pull/5633) Allow setting new and upgrading existing ICA (ordered) channels to use unordered ordering.
 
 ### Bug Fixes
 

diff --git a/modules/apps/27-interchain-accounts/controller/client/cli/tx.go b/modules/apps/27-interchain-accounts/controller/client/cli/tx.go
@@ -14,11 +14,15 @@ import (
 
 	"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/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"
+	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
 )
 
 const (
 	// The controller chain channel version
-	flagVersion               = "version"
+	flagVersion = "version"
+	// The channel ordering
+	flagOrdering              = "ordering"
 	flagRelativePacketTimeout = "relative-packet-timeout"
 )
 
@@ -29,8 +33,8 @@ func newRegisterInterchainAccountCmd() *cobra.Command {
 		Long: strings.TrimSpace(`Register an account on the counterparty chain via the 
 connection id from the source chain. Connection identifier should be for the source chain 
 and the interchain account will be created on the counterparty chain. Callers are expected to 
-provide the appropriate application version string via {version} flag. Generates a new 
-port identifier using the provided owner string, binds to the port identifier and claims 
+provide the appropriate application version string via {version} flag and the desired ordering
+via the {ordering} flag. Generates a new port identifier using the provided owner string, binds to the port identifier and claims 
 the associated capability.`),
 		Args: cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -46,13 +50,19 @@ the associated capability.`),
 				return err
 			}
 
-			msg := types.NewMsgRegisterInterchainAccount(connectionID, owner, version)
+			order, err := parseOrder(cmd)
+			if err != nil {
+				return err
+			}
+
+			msg := types.NewMsgRegisterInterchainAccountWithOrder(connectionID, owner, version, order)
 
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
 	}
 
 	cmd.Flags().String(flagVersion, "", "Controller chain channel version")
+	cmd.Flags().String(flagOrdering, channeltypes.ORDERED.String(), fmt.Sprintf("Channel ordering, can be one of: %s", strings.Join(connectiontypes.SupportedOrderings, ", ")))
 	flags.AddTxFlagsToCmd(cmd)
 
 	return cmd
@@ -107,3 +117,18 @@ appropriate relative timeoutTimestamp must be provided with flag {relative-packe
 
 	return cmd
 }
+
+// parseOrder gets the channel ordering from the flags.
+func parseOrder(cmd *cobra.Command) (channeltypes.Order, error) {
+	orderString, err := cmd.Flags().GetString(flagOrdering)
+	if err != nil {
+		return channeltypes.NONE, err
+	}
+
+	order, found := channeltypes.Order_value[strings.ToUpper(orderString)]
+	if !found {
+		return channeltypes.NONE, fmt.Errorf("invalid channel ordering: %s", orderString)
+	}
+
+	return channeltypes.Order(order), nil
+}
diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
@@ -160,11 +160,6 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 				suite.chainA.GetSimApp().ICAControllerKeeper.SetParams(suite.chainA.GetContext(), types.NewParams(false))
 			}, false,
 		},
-		{
-			"ICA OnChanOpenInit fails - UNORDERED channel", func() {
-				channel.Ordering = channeltypes.UNORDERED
-			}, false,
-		},
 		{
 			"ICA auth module callback fails", func() {
 				suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string,
@@ -767,9 +762,10 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() {
 
 func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() {
 	var (
-		path     *ibctesting.Path
-		isNilApp bool
-		version  string
+		path         *ibctesting.Path
+		isNilApp     bool
+		version      string
+		channelOrder channeltypes.Order
 	)
 
 	testCases := []struct {
@@ -778,7 +774,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() {
 		expError error
 	}{
 		{
-			"success", func() {}, nil,
+			"success w/ ORDERED channel", func() {}, nil,
+		},
+		{
+			"success w/ UNORDERED channel", func() {
+				channelOrder = channeltypes.UNORDERED
+			}, nil,
 		},
 		{
 			"success: nil underlying app",
@@ -844,11 +845,13 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeInit() {
 				cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
 			}
 
+			channelOrder = channeltypes.ORDERED
+
 			version, err = cbs.OnChanUpgradeInit(
 				suite.chainA.GetContext(),
 				path.EndpointA.ChannelConfig.PortID,
 				path.EndpointA.ChannelID,
-				channeltypes.ORDERED,
+				channelOrder,
 				[]string{path.EndpointA.ConnectionID},
 				version,
 			)
@@ -989,15 +992,20 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() {
 		path                *ibctesting.Path
 		isNilApp            bool
 		counterpartyVersion string
+		channelOrder        channeltypes.Order
 	)
 
 	testCases := []struct {
 		name     string
 		malleate func()
 	}{
 		{
-			"success",
-			func() {},
+			"success w/ ORDERED channel", func() {},
+		},
+		{
+			"success w/ UNORDERED channel", func() {
+				channelOrder = channeltypes.UNORDERED
+			},
 		},
 		{
 			"success: nil app",
@@ -1044,11 +1052,13 @@ func (suite *InterchainAccountsTestSuite) TestOnChanUpgradeOpen() {
 				cbs = controller.NewIBCMiddleware(nil, suite.chainA.GetSimApp().ICAControllerKeeper)
 			}
 
+			channelOrder = channeltypes.ORDERED
+
 			cbs.OnChanUpgradeOpen(
 				suite.chainA.GetContext(),
 				path.EndpointA.ChannelConfig.PortID,
 				path.EndpointA.ChannelID,
-				channeltypes.ORDERED,
+				channelOrder,
 				[]string{path.EndpointA.ConnectionID},
 				counterpartyVersion,
 			)

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/account.go b/modules/apps/27-interchain-accounts/controller/keeper/account.go
@@ -41,7 +41,7 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner,
 
 	k.SetMiddlewareEnabled(ctx, portID, connectionID)
 
-	_, err = k.registerInterchainAccount(ctx, connectionID, portID, version)
+	_, err = k.registerInterchainAccount(ctx, connectionID, portID, version, channeltypes.ORDERED)
 	if err != nil {
 		return err
 	}
@@ -51,7 +51,7 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner,
 
 // 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) {
+func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, portID, version string, order channeltypes.Order) (string, error) {
 	// if there is an active channel for this portID / connectionID return an error
 	activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionID, portID)
 	if found {
@@ -69,7 +69,7 @@ func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, portID,
 		}
 	}
 
-	msg := channeltypes.NewMsgChannelOpenInit(portID, version, channeltypes.ORDERED, []string{connectionID}, icatypes.HostPortID, authtypes.NewModuleAddress(icatypes.ModuleName).String())
+	msg := channeltypes.NewMsgChannelOpenInit(portID, version, order, []string{connectionID}, icatypes.HostPortID, authtypes.NewModuleAddress(icatypes.ModuleName).String())
 	handler := k.msgRouter.Handler(msg)
 	res, err := handler(ctx, msg)
 	if err != nil {

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/handshake.go b/modules/apps/27-interchain-accounts/controller/keeper/handshake.go
@@ -15,8 +15,7 @@ import (
 )
 
 // OnChanOpenInit performs basic validation of channel initialization.
-// The channel order must be ORDERED, the counterparty port identifier
-// must be the host chain representation as defined in the types package,
+// The counterparty port identifier must be the host chain representation as defined in the types package,
 // the channel version must be equal to the version in the types package,
 // there must not be an active channel for the specfied port identifier,
 // and the interchain accounts module must be able to claim the channel
@@ -31,10 +30,6 @@ func (k Keeper) OnChanOpenInit(
 	counterparty channeltypes.Counterparty,
 	version string,
 ) (string, error) {
-	if order != channeltypes.ORDERED {
-		return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
-	}
-
 	if !strings.HasPrefix(portID, icatypes.ControllerPortPrefix) {
 		return "", errorsmod.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.ControllerPortPrefix, portID)
 	}
@@ -72,8 +67,12 @@ func (k Keeper) OnChanOpenInit(
 			panic(fmt.Errorf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID))
 		}
 
-		if channel.IsOpen() {
-			return "", errorsmod.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)
+		if channel.State != channeltypes.CLOSED {
+			return "", errorsmod.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s must be %s", activeChannelID, portID, channeltypes.CLOSED)
+		}
+
+		if channel.Ordering != order {
+			return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "order cannot change when reopening a channel expected %s, got %s", channel.Ordering, order)
 		}
 
 		appVersion, found := k.GetAppVersion(ctx, portID, activeChannelID)
@@ -149,19 +148,13 @@ func (Keeper) OnChanCloseConfirm(
 // The following may be changed:
 // - tx type (must be supported)
 // - encoding (must be supported)
+// - order
 //
 // The following may not be changed:
-// - order
 // - connectionHops (and subsequently host/controller connectionIDs)
 // - interchain account address
 // - ICS27 protocol version
 func (k Keeper) OnChanUpgradeInit(ctx sdk.Context, portID, channelID string, proposedOrder channeltypes.Order, proposedConnectionHops []string, proposedversion string) (string, error) {
-	// verify order has not changed
-	// support for unordered ICA channels is not implemented yet
-	if proposedOrder != channeltypes.ORDERED {
-		return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, proposedOrder)
-	}
-
 	// verify connection hops has not changed
 	connectionID, err := k.GetConnectionID(ctx, portID, channelID)
 	if err != nil {