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

ADR 009: ICS 27 simplication #2218

Merged
merged 5 commits into from
Sep 28, 2022
Merged
Changes from 1 commit
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
110 changes: 110 additions & 0 deletions docs/architecture/adr-009-v6-ics27-msgserver.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
# ADR 009: ICS27 message server addition
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# ADR 009: ICS27 message server addition
# ADR 009: ICS27 Message Server Addition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we follow google style recommendations which only capitalizes the first word in a title cc @crodriguezvega should I capitalize Message?


## Changelog
* 2022/09/07: Initial draft

## Status

Accepted, implemented in v6 of ibc-go

## Context

ICS 27 (Interchain Accounts), brought a cross-chain account management protocol built upon IBC.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
It enabled chains to programatically create accounts on behalf of counterparty chains which may enable a variety of authentication methods for this interchain account.
The initial release of ICS 27 focused on enabling authentication schemes which may not require signing with a private key, such as via on-chain mechanisms like governance.

Following the initial release ICS 27 it became evident that:
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
- a default authentication module would enable more usage of ICS 27
- generic authentication modules should be capable of authenticating an interchain account registration
- application logic which wraps ics27 packet sends do not need to be associated with the authentication logic
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

## Decision

The controller module should be simplified to remove the correlation between the authentication logic for an interchain account and the application logic for an interchain account.
To minimize disruption to developers working on the original design of the ICS 27 controller module, all changes will be made in a backwards compatible fashion.

### Msg server

To acheive this, as stated by [@damiannolan](https://github.com/cosmos/ibc-go/issues/2026#issue-1341640594), it was proposed to:

> Add a new `MsgServer` to `27-interchain-accounts` which exposes two distinct rpc endpoints:
>
> - `RegisterAccount`
Copy link
Contributor

Choose a reason for hiding this comment

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

> - `SubmitTx`
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

This will enable any SDK (authentication) module to register interchain accounts and send transactions on their behalf.
Examples of existing SDK modules which would benefit from this change include:
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
- x/auth
- x/gov
- x/group

The existing go functions: `RegisterInterchainAccount()` and `SendTx()` will remain unchanged.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### Allow nil underlying applications
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

Authentication modules should interact with the controller module via the message server and should not be associated with application logic.
For now, it will be allowed to set a nil underlying application.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
A future version may remove the underlying application entirely.

See issue [#2040](https://github.com/cosmos/ibc-go/issues/2040)

### Channel capability claiming

The controller module will now claim the channel capability in `OnChanOpenInit`.
Underlying applications will be passed a nil capability in `OnChanOpenInit`
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

Channel capability migrations will be added in two steps:
- Upgrade handler migration which modifies the channel capability owner from the underlying app to the controller module
- ICS 27 module automatic migrations which assert the upgrade handler channel capability migration has been performed successfully
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

See issue [#2033](https://github.com/cosmos/ibc-go/issues/2033)

### Middleware enabled channels

In order to maintain backwards compatibility and avoid requiring underlying application developers to account for interchain accounts they did not register, a boolean mapping has been added to track the behaviour of how an account was created.

If the account was created via the legacy API, then the underlying application callbacks will be executed
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

If the account was created with the new API (message server), then the underlying application callbacks will not be executed.

See issue [#2145](https://github.com/cosmos/ibc-go/issues/2145)

### Future considerations

ADR 008 proposes the creation of a middleware which enables callers of an IBC packet send to perform application logic in conjunction with the IBC application.
The underlying application can be removed at the availablity of such a middleware as that will be the preferred method for executing application logic upon a ICS 27 packet send.

crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
### Miscellanous

In order to avoid import cycles, the genesis types have been moved to their own directory.
A new protobuf package has been created for the genesis types.

See pr [#2133](https://github.com/cosmos/ibc-go/pull/2133)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

An additional field has been added to the `ActiveChannel` type to store the `IsMiddlewareEnabled` field upon genesis import/export.

See issue [#2165](https://github.com/cosmos/ibc-go/issues/2165)

## Consequences

### Positive

- default authentications module are provided (x/auth, x/group, x/gov)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
- any SDK authentication module may now be used with ICS 27
- separation of authentication from appplication logic in relation to ICS 27
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
- minimized disruption to existing development around ICS 27 controller module
- underlying applications no longer have to handle capabilities
- removal of the underlying application upon the creation of ADR 008 may be done in a minimally disruptive fashion
- only underlying applications which registered the interchain account will perform application logic for that account (underlying applications do not need to be cogniscent of accounts it did not register)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### Negative

- the security model has been reduced to that of the SDK. SDK modules may send packets for any interchain account.
- additional maintence of the messages added and the middleware enabled flag
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
- underlying applications which will become ADR 008 module are not required to be aware of accounts they did not register
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
- calling legacy API vs the new API results in different behaviour for ICS 27 application stacks which have an underlying application

### Neutral

- A major release is required