Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp: use UNORDERED as default ordering for new ICA channels #6434

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (core/04-channel) [\#6023](https://github.com/cosmos/ibc-go/pull/6023) Remove emission of non-hexlified event attributes `packet_data` and `packet_ack`.
* (core) [\#6320](https://github.com/cosmos/ibc-go/pull/6320) Remove unnecessary `Proof` interface from `exported` package.
* (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version.
* (apps/27-interchain-accounts) [\#6433](https://github.com/cosmos/ibc-go/pull/6433) Use UNORDERED as the default ordering for new ICA channels.
* (apps/transfer) [\#6440](https://github.com/cosmos/ibc-go/pull/6440) Remove `GetPrefixedDenom`.

### State Machine Breaking
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ The Interchain Accounts module uses either [ORDERED or UNORDERED](https://github

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.
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. If no ordering is specified in `MsgRegisterInterchainAccount`, then the default ordering for new ICA channels is `UNORDERED`.

> A limitation when using ORDERED channels is that when a packet times out the channel will be closed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comma for better readability.

- ...ED channels is that when a packet times out the channel will be closed.  In the cas...
+ ...ED channels is that when a packet times out, the channel will be closed.  In the cas...

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
> A limitation when using ORDERED channels is that when a packet times out the channel will be closed.
> A limitation when using ORDERED channels is that when a packet times out, the channel will be closed.


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ The controller submodule keeper exposes two legacy functions that allow respecti
The authentication module can begin registering interchain accounts by calling `RegisterInterchainAccount`:

```go
if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, connectionID, owner.String(), version); err != nil {
if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, connectionID, owner.String(), version, channeltypes.UNORDERED); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update documentation to reflect new default behavior.

The documentation examples explicitly set the channel ordering to UNORDERED. Consider mentioning the new default behavior in the documentation to inform users that they do not need to explicitly set UNORDERED unless they want to override another specified behavior.

Also applies to: 47-47, 78-78

return err
}

return nil
```

The `version` argument is used to support ICS-29 fee middleware for relayer incentivization of ICS-27 packets. Consumers of the `RegisterInterchainAccount` are expected to build the appropriate JSON encoded version string themselves and pass it accordingly. If an empty string is passed in the `version` argument, then the version will be initialized to a default value in the `OnChanOpenInit` callback of the controller's handler, so that channel handshake can proceed.
The `version` argument is used to support ICS-29 fee middleware for relayer incentivization of ICS-27 packets. The `ordering` argument allows to specify the ordering of the channel that is created; if `NONE` is passed, then the default ordering will be `UNORDERED`. Consumers of the `RegisterInterchainAccount` are expected to build the appropriate JSON encoded version string themselves and pass it accordingly. If an empty string is passed in the `version` argument, then the version will be initialized to a default value in the `OnChanOpenInit` callback of the controller's handler, so that channel handshake can proceed.

The following code snippet illustrates how to construct an appropriate interchain accounts `Metadata` and encode it as a JSON bytestring:

Expand All @@ -44,7 +44,7 @@ if err != nil {
return err
}

if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, controllerConnectionID, owner.String(), string(appVersion)); err != nil {
if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, controllerConnectionID, owner.String(), string(appVersion), channeltypes.UNORDERED); err != nil {
return err
}
```
Expand Down Expand Up @@ -75,7 +75,7 @@ if err != nil {
return err
}

if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, controllerConnectionID, owner.String(), string(feeEnabledVersion)); err != nil {
if err := keeper.icaControllerKeeper.RegisterInterchainAccount(ctx, controllerConnectionID, owner.String(), string(feeEnabledVersion), channeltypes.UNORDERED); err != nil {
return err
}
```
Expand Down
11 changes: 11 additions & 0 deletions docs/docs/05-migrations/13-v8-to-v9.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,17 @@ func NewKeeper(
) Keeper
```

The legacy function `RegisterInterchainAccount` now takes an extra parameter to specify the ordering of new ICA channels:

```diff
func (k Keeper) RegisterInterchainAccount(
ctx sdk.Context,
connectionID, owner,
version string,
+ ordering channeltypes.Order
) error {
```

## Relayers

- Renaming of event attribute keys in [#5603](https://github.com/cosmos/ibc-go/pull/5603).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ the associated capability.`),
}

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, ", ")))
cmd.Flags().String(flagOrdering, channeltypes.UNORDERED.String(), fmt.Sprintf("Channel ordering, can be one of: %s", strings.Join(connectiontypes.SupportedOrderings, ", ")))
flags.AddTxFlagsToCmd(cmd)

return cmd
Expand Down
Loading
Loading