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

* 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/apps/interchain-accounts/active-channels.md # docs/apps/interchain-accounts/overview.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/client/cli/tx.go # modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go # modules/apps/27-interchain-accounts/controller/keeper/handshake.go # modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go # modules/apps/27-interchain-accounts/controller/keeper/msg_server_test.go # modules/apps/27-interchain-accounts/controller/types/msgs.go # modules/apps/27-interchain-accounts/controller/types/msgs_test.go # modules/apps/27-interchain-accounts/controller/types/tx.pb.go # modules/apps/27-interchain-accounts/host/keeper/handshake.go # modules/apps/27-interchain-accounts/host/keeper/handshake_test.go # modules/apps/callbacks/callbacks_test.go # proto/ibc/applications/interchain_accounts/controller/v1/tx.proto * fix conflicts * rename order to ordering (backport #5674) * add changelog * Update messages.md --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
cosmos · Apr 30, 2024 · 47cf563 · 47cf563
1 parent 63d2e40
commit 47cf563
Show file tree

Hide file tree

Showing 18 changed files with 252 additions and 147 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -48,6 +48,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Features
 
+* (apps/27-interchain-accounts) [\#5633](https://github.com/cosmos/ibc-go/pull/5633) Allow new ICA channels to use unordered ordering.
+
 ### Bug Fixes
 
 ## [v7.4.0](https://github.com/cosmos/ibc-go/releases/tag/v7.4.0) - 2024-04-05

diff --git a/docs/apps/interchain-accounts/active-channels.md b/docs/apps/interchain-accounts/active-channels.md
@@ -4,7 +4,13 @@ order: 9
 
 # Understanding Active Channels 
 
-The Interchain Accounts module uses [ORDERED channels](https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#ordering) to maintain the order of transactions when sending packets from a controller to a host chain. A limitation when using ORDERED channels is that when a packet times out the channel will be closed. 
+The Interchain Accounts module uses either [ORDERED or UNORDERED](https://github.com/cosmos/ibc/tree/master/spec/core/ics-004-channel-and-packet-semantics#ordering) channels. 
+
+When using `ORDERED` channels, the order of transactions when sending packets from a controller to a host chain is maintained.
+
+When using `UNORDERED` channels, there is no guarantee that the order of transactions when sending packets from the controller to the host chain is maintained.
+
+> A limitation when using ORDERED channels is that when a packet times out the channel will be closed.
 
 In the case of a channel closing, a controller chain needs to be able to regain access to the interchain account registered on this channel. `Active Channels` enable this functionality. 
 

diff --git a/docs/apps/interchain-accounts/client.md b/docs/apps/interchain-accounts/client.md
@@ -34,6 +34,25 @@ The `tx` commands allow users to interact with the controller submodule.
 simd tx interchain-accounts controller --help
 ```
 
+#### `register`
+
+The `register` command allows users to register an interchain account on a host chain on the provided connection.
+
+```shell
+simd tx interchain-accounts controller register [connection-id] [flags]
+```
+
+During registration a new channel is set up between controller and host. There are two flags available that influence the channel that is created:
+
+- `--version` to specify the (JSON-formatted) version string of the channel. For example: `{\"version\":\"ics27-1\",\"encoding\":\"proto3\",\"tx_type\":\"sdk_multi_msg\",\"controller_connection_id\":\"connection-0\",\"host_connection_id\":\"connection-0\"}`. Passing a custom version string is useful if you want to specify, for example, the encoding format of the interchain accounts packet data (either `proto3` or `proto3json`). If not specified the controller submodule will generate a default version string.
+- `--ordering` to specify the ordering of the channel. Available options are `order_ordered` (default if not specified) and `order_unordered`.
+
+Example:
+
+```shell
+simd tx interchain-accounts controller register connection-0 --ordering order_unordered --from cosmos1..
+```
+
 #### `send-tx`
 
 The `send-tx` command allows users to send a transaction on the provided connection to be executed using an interchain account on the host chain.

diff --git a/docs/apps/interchain-accounts/messages.md b/docs/apps/interchain-accounts/messages.md
@@ -13,6 +13,7 @@ type MsgRegisterInterchainAccount struct {
   Owner        string
   ConnectionID string
   Version      string
+  Ordering     channeltypes.Order
 }
 ```
 

diff --git a/docs/apps/interchain-accounts/overview.md b/docs/apps/interchain-accounts/overview.md
@@ -30,4 +30,13 @@ SDK modules on a chain are assumed to be trustworthy. For example, there are no
 
 The implementation of ICS-27 in ibc-go uses this assumption in its security considerations.
 
-The implementation assumes other IBC application modules will not bind to ports within the ICS-27 namespace. 
+The implementation assumes other IBC application modules will not bind to ports within the ICS-27 namespace.
+
+## Channel Closure
+
+The provided interchain account host and controller implementations do not support `ChanCloseInit`. However, they do support `ChanCloseConfirm`.
+This means that the host and controller modules cannot close channels, but they will confirm channel closures initiated by other implementations of ICS-27.
+
+In the event of a channel closing (due to a packet timeout in an ordered channel, for example), the interchain account associated with that channel can become accessible again if a new channel is created with a (JSON-formatted) version string that encodes the exact same `Metadata` information of the previous channel. The channel can be reopened using either [`MsgRegisterInterchainAccount`](./05-messages.md#msgregisterinterchainaccount) or `MsgChannelOpenInit`. If `MsgRegisterInterchainAccount` is used, then it is possible to leave the `version` field of the message empty, since it will be filled in by the controller submodule. If `MsgChannelOpenInit` is used, then the `version` field must be provided with the correct JSON-encoded `Metadata` string. See section [Understanding Active Channels](./09-active-channels.md#understanding-active-channels) for more information.
+
+When reopening a channel with the default controller submodule, the ordering of the channel cannot be changed. In order to change the ordering of the channel, the channel has to go through a [channel upgrade handshake](../../01-ibc/06-channel-upgrades.md) or reopen the channel with a custom controller implementation.
diff --git a/modules/apps/27-interchain-accounts/controller/client/cli/tx.go b/modules/apps/27-interchain-accounts/controller/client/cli/tx.go
@@ -13,11 +13,15 @@ import (
 
 	"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
 	icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
+	connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
+	channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
 )
 
 const (
 	// The controller chain channel version
-	flagVersion               = "version"
+	flagVersion = "version"
+	// The channel ordering
+	flagOrdering              = "ordering"
 	flagRelativePacketTimeout = "relative-packet-timeout"
 )
 
@@ -28,8 +32,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 {
@@ -45,13 +49,19 @@ the associated capability.`),
 				return err
 			}
 
-			msg := types.NewMsgRegisterInterchainAccount(connectionID, owner, version)
+			ordering, err := parseOrdering(cmd)
+			if err != nil {
+				return err
+			}
+
+			msg := types.NewMsgRegisterInterchainAccountWithOrdering(connectionID, owner, version, ordering)
 
 			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
 }
+
+// parseOrdering gets the channel ordering from the flags.
+func parseOrdering(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,

diff --git a/modules/apps/27-interchain-accounts/controller/keeper/account.go b/modules/apps/27-interchain-accounts/controller/keeper/account.go
@@ -39,7 +39,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
 	}
@@ -49,7 +49,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, ordering 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 {
@@ -66,7 +66,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, ordering, []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
@@ -13,8 +13,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
@@ -29,10 +28,6 @@ func (k Keeper) OnChanOpenInit(
 	counterparty channeltypes.Counterparty,
 	version string,
 ) (string, error) {
-	if order != channeltypes.ORDERED {
-		return "", sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
-	}
-
 	if !strings.HasPrefix(portID, icatypes.ControllerPortPrefix) {
 		return "", sdkerrors.Wrapf(icatypes.ErrInvalidControllerPort, "expected %s{owner-account-address}, got %s", icatypes.ControllerPortPrefix, portID)
 	}
@@ -66,8 +61,12 @@ func (k Keeper) OnChanOpenInit(
 			panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID))
 		}
 
-		if channel.State == channeltypes.OPEN {
-			return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)
+		if channel.State != channeltypes.CLOSED {
+			return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s must be %s", activeChannelID, portID, channeltypes.CLOSED)
+		}
+
+		if channel.Ordering != order {
+			return "", sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "order cannot change when reopening a channel expected %s, got %s", channel.Ordering, order)
 		}
 
 		appVersion, found := k.GetAppVersion(ctx, portID, activeChannelID)