-
Notifications
You must be signed in to change notification settings - Fork 634
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
docs: ica tx atomicity docs and code snippet updates #719
Changes from 9 commits
11d2729
9ec49f8
7652585
8aaf1c4
52e8c68
7fe8570
c15e66c
e330389
7c1ae19
dd537db
15fd539
0700e80
54eb032
94ba9bd
032a04c
6ea3b4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
<!-- | ||
order: 3 | ||
order: 2 | ||
--> | ||
|
||
# Building an ICA authentication module | ||
# Authentication Modules | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this as I thought headings should be fairly concise and to the point, and my main goal here was to remove "ICA" from the heading, as its already a subpage of Interchain Accounts docs and essentially implicit. What about a happy medium of "Building an Authentication Module"? |
||
|
||
The controller module is used for account registration and packet sending. | ||
Authentication modules play the role of the `Base Application` as described in [ICS30 IBC Middleware](https://github.com/cosmos/ibc/tree/master/spec/app/ics-030-middleware), and enable application developers to perform custom logic when working with the Interchain Accounts controller API. {synopsis} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking that maybe we should give a concrete example of what an authentication module would do. Probably governance is the easiest to understand. wdyt? I feel like our explanations of I can try and think of something to write. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, something with governance would be great. I think that could be added in a separate PR |
||
|
||
The controller submodule is used for account registration and packet sending. | ||
It executes only logic required of all controllers of interchain accounts. | ||
The type of authentication used to manage the interchain accounts remains unspecified. | ||
There may exist many different types of authentication which are desirable for different use cases. | ||
|
@@ -108,9 +110,8 @@ func (im IBCModule) OnChanOpenTry( | |
channelID string, | ||
chanCap *capabilitytypes.Capability, | ||
counterparty channeltypes.Counterparty, | ||
version, | ||
counterpartyVersion string, | ||
) error { | ||
) (string, error) { | ||
panic("UNIMPLEMENTED") | ||
} | ||
|
||
|
@@ -142,26 +143,14 @@ func (im IBCModule) OnRecvPacket( | |
) ibcexported.Acknowledgement { | ||
panic("UNIMPLEMENTED") | ||
} | ||
|
||
// NegotiateAppVersion implements the IBCModule interface | ||
func (im IBCModule) NegotiateAppVersion( | ||
ctx sdk.Context, | ||
order channeltypes.Order, | ||
connectionID string, | ||
portID string, | ||
counterparty channeltypes.Counterparty, | ||
proposedVersion string, | ||
) (string, error) { | ||
panic("UNIMPLEMENTED") | ||
} | ||
``` | ||
|
||
## `InitInterchainAccount` | ||
|
||
The authentication module can begin registering interchain accounts by calling `InitInterchainAccount`: | ||
|
||
```go | ||
if err := keeper.icaControllerKeeper.InitInterchainAccount(ctx, connectionID, counterpartyConnectionID, owner.String()); err != nil { | ||
if err := keeper.icaControllerKeeper.InitInterchainAccount(ctx, connectionID, owner.String()); err != nil { | ||
return err | ||
} | ||
|
||
|
@@ -176,19 +165,18 @@ The authentication module can attempt to send a packet by calling `TrySendTx`: | |
// Authenticate owner | ||
// perform custom logic | ||
|
||
// Lookup portID based on interchain account owner address | ||
portID, err := icatypes.GeneratePortID(owner.String(), connectionID, counterpartyConnectionID) | ||
// Construct controller portID based on interchain account owner address | ||
portID, err := icatypes.NewControllerPortID(owner.String()) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
channelID, found := keeper.icaControllerKeeper.GetActiveChannelID(ctx, portID) | ||
if !found { | ||
return sdkerrors.Wrapf(icatypes.ErrActiveChannelNotFound, "failed to retrieve active channel for port %s", portId) | ||
return sdkerrors.Wrapf(icatypes.ErrActiveChannelNotFound, "failed to retrieve active channel for port %s", portID) | ||
} | ||
|
||
// Obtain the channel capability. | ||
// The channel capability should have been claimed by the authentication module in OnChanOpenInit | ||
// Obtain the channel capability, claimed in OnChanOpenInit | ||
chanCap, found := keeper.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(portID, channelID)) | ||
if !found { | ||
return sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability") | ||
|
@@ -215,7 +203,8 @@ packetData := icatypes.InterchainAccountPacketData{ | |
// If the packet times out, the channel will be closed requiring a new channel to be created | ||
timeoutTimestamp := obtainTimeoutTimestamp() | ||
|
||
_, err = keeper.icaControllerKeeper.TrySendTx(ctx, chanCap, p, packetData, timeoutTimestamp) | ||
// Send the interchain accounts packet, returning the packet sequence | ||
seq, err = keeper.icaControllerKeeper.TrySendTx(ctx, chanCap, portID, packetData, timeoutTimestamp) | ||
``` | ||
|
||
The data within an `InterchainAccountPacketData` must be serialized using a format supported by the host chain. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ order: 1 | |
Learn about what the Interchain Accounts module is, and how to build custom modules that utilize Interchain Accounts functionality {synopsis} | ||
|
||
|
||
# What is the Interchain Accounts module? | ||
## What is the Interchain Accounts module? | ||
|
||
Interchain Accounts is the Cosmos SDK implementation of the ICS-27 protocol, which enables cross-chain account management built upon IBC. Chains using the Interchain Accounts module can programmatically create accounts on other chains and control these accounts via IBC transactions. | ||
|
||
|
@@ -19,14 +19,14 @@ Developers looking to build upon Interchain Accounts must write custom logic in | |
|
||
Regular accounts use a private key to sign transactions on-chain. Interchain Accounts are instead controlled programmatically by separate chains via IBC transactions. Interchain Accounts are implemented as sub-accounts of the interchain accounts module account. | ||
|
||
# Concepts | ||
## Definitions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aligns with same pattern used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should keep it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no preference There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a problem naming the heading as "Concepts" but I do think we should be consistent throughout the docs. So that would mean renaming "Definitions" heading used within the middleware docs to "Concepts". These kind of sections should, in my opinion, act as a sort of glossary of words/terminology to give context on the language used for a particular topic. Either heading is fine but we should be consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we use |
||
|
||
*Host Chain*: The chain where the interchain account is registered. The host chain listens for IBC packets from a controller chain which should contain instructions (e.g. cosmos SDK messages) for which the interchain account will execute. | ||
`Host Chain`: The chain where the interchain account is registered. The host chain listens for IBC packets from a controller chain which should contain instructions (e.g. cosmos SDK messages) for which the interchain account will execute. | ||
|
||
*Controller Chain*: The chain registering and controlling an account on a host chain. The controller chain sends IBC packets to the host chain to control the account. A controller chain must have at least one interchain accounts authentication module in order to act as a controller chain. | ||
`Controller Chain`: The chain registering and controlling an account on a host chain. The controller chain sends IBC packets to the host chain to control the account. A controller chain must have at least one interchain accounts authentication module in order to act as a controller chain. | ||
|
||
*Authentication Module*: A custom IBC application module on the controller chain that uses the Interchain Accounts module API to build custom logic for the creation & management of interchain accounts. For a controller chain to utilize the interchain accounts module functionality, an authentication module is required. | ||
`Authentication Module`: A custom IBC application module on the controller chain that uses the Interchain Accounts module API to build custom logic for the creation & management of interchain accounts. For a controller chain to utilize the interchain accounts module functionality, an authentication module is required. | ||
|
||
*Interchain Account*: An account on a host chain. An interchain account has all the capabilities of a normal account. However, rather than signing transactions with a private key, a controller chain's authentication module will send IBC packets to the host chain which signals what transactions the interchain account should execute. | ||
`Interchain Account`: An account on a host chain. An interchain account has all the capabilities of a normal account. However, rather than signing transactions with a private key, a controller chain's authentication module will send IBC packets to the host chain which signals what transactions the interchain account should execute. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
<!-- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added separate page for parameters. More easy to navigate and more explicit! |
||
order: 4 | ||
--> | ||
|
||
# Parameters | ||
|
||
The Interchain Accounts module contains the following on-chain parameters, logically separated for each distinct submodule: | ||
|
||
### Controller Submodule Parameters | ||
|
||
| Key | Type | Default Value | | ||
|------------------------|------|---------------| | ||
| `ControllerEnabled` | bool | `true` | | ||
|
||
#### ControllerEnabled | ||
|
||
The `ControllerEnabled` parameter controls a chains ability to service ICS-27 controller specific logic. This includes the sending of Interchain Accounts packet data as well as the following ICS-26 callback handlers: | ||
- `OnChanOpenInit` | ||
- `OnChanOpenAck` | ||
- `OnChanCloseConfirm` | ||
- `OnAcknowledgementPacket` | ||
- `OnTimeoutPacket` | ||
|
||
### Host Submodule Parameters | ||
|
||
| Key | Type | Default Value | | ||
|------------------------|----------|---------------| | ||
| `HostEnabled` | bool | `true` | | ||
| `AllowMessages` | []string | `[]` | | ||
|
||
#### HostEnabled | ||
|
||
The `HostEnabled` parameter controls a chains ability to service ICS27 host specific logic. This includes the following ICS-26 callback handlers: | ||
- `OnChanOpenTry` | ||
- `OnChanOpenConfirm` | ||
- `OnChanCloseConfirm` | ||
- `OnRecvPacket` | ||
|
||
#### AllowMessages | ||
|
||
The `AllowMessages` parameter provides the ability for a chain to limit the types of messages or transactions that it chooses to facilitate by defining an allowlist using the Protobuf message TypeURL format. | ||
damiannolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
For example, a Cosmos SDK based chain that elects to provide hosted Interchain Accounts with the ability of governance voting and staking delegations will define its parameters as follows: | ||
|
||
``` | ||
"params": { | ||
"host_enabled": true, | ||
"allow_messages": ["/cosmos.staking.v1beta1.MsgDelegate", "/cosmos.gov.v1beta1.MsgVote"] | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,48 @@ | ||||
<!-- | ||||
order: 5 | ||||
--> | ||||
|
||||
# Transactions | ||||
|
||||
Learn about Interchain Accounts transaction execution {synopsis} | ||||
|
||||
## Executing a transaction | ||||
|
||||
As described in [Authentication Modules](./auth-modules.md#trysendtx) transactions are executed using the interchain accounts controller API and require a `Base Application` as outlined in [ICS30 IBC Middleware](https://github.com/cosmos/ibc/tree/master/spec/app/ics-030-middleware) to facilitate authentication. The method of authentication remains unspecified to provide flexibility for the authentication module developer. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worked when I was running a local server with docs. Link checker is happy too |
||||
|
||||
Transactions are executed via the ICS27 [`TrySendTx` API](./auth-modules.md#trysendtx). This must be invoked through the Interchain Accounts authentication module, and follows the outlined path of execution below. Packet relaying semantics provided by the IBC core transport, authentication and ordering (IBC/TAO) layer are omitted for brevity. | ||||
damiannolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
![send-tx-flow](../../assets/send-interchain-tx.png "Transaction Execution") | ||||
|
||||
## Atomicity | ||||
|
||||
As the Interchain Accounts module supports the execution of multiple transactions using the Cosmos SDK `Msg` interface, it provides the same atomicity guarantees as Cosmos SDK-based applications, leveraging the [`CacheMultiStore`](https://docs.cosmos.network/master/core/store.html#cachemultistore) architecture provided by the `Context` type. | ||||
|
||||
When a host chain receives an Interchain Accounts packet and successfully deserializes its message content, each `Msg` is authenticated and executed using a cached storage object. A new `CacheMultiStore` is obtained via the Cosmos SDK `ctx.CacheContext()` method. State changes are then performed within the context of a branched key-value store and commited when `writeCache()` is invoked. This storage mechanism ensures that state changes are committed if and only if all transactions succeed. | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should go into this level of detail, personally. I think we can probably remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My reasoning for this is that I feel we shouldn't overcomplicate things for developers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add this into the godoc of executeTx. Agree with keeping these docs simple. I'd just say that the transaction is atomic and the state changes are only written if all the messages succeed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, we can remove this section. We already have something similar as inline comments within
I essentially had just paraphrased the above to flesh out the docs content some more as I wasn't sure if it would be too sparse without. |
||||
|
||||
```go | ||||
func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel string, msgs []sdk.Msg) error { | ||||
if err := k.AuthenticateTx(ctx, msgs, sourcePort); err != nil { | ||||
return err | ||||
} | ||||
|
||||
// CacheContext returns a new context with the multi-store branched into a cached storage object | ||||
// writeCache is called only if all msgs succeed, performing state transitions atomically | ||||
cacheCtx, writeCache := ctx.CacheContext() | ||||
for _, msg := range msgs { | ||||
if err := msg.ValidateBasic(); err != nil { | ||||
return err | ||||
} | ||||
|
||||
if err := k.executeMsg(cacheCtx, msg); err != nil { | ||||
return err | ||||
} | ||||
} | ||||
|
||||
// NOTE: The context returned by CacheContext() creates a new EventManager, so events must be correctly propagated back to the current context | ||||
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) | ||||
writeCache() | ||||
|
||||
return nil | ||||
} | ||||
``` | ||||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to dashed casing to align with other directories and files. E.g. ADRs or
upgrades/developer-guide.md
..etc