diff --git a/docs/architecture/adr-001-coin-source-tracing.md b/docs/architecture/adr-001-coin-source-tracing.md index 17b905adca7..b5ff247926c 100644 --- a/docs/architecture/adr-001-coin-source-tracing.md +++ b/docs/architecture/adr-001-coin-source-tracing.md @@ -102,10 +102,10 @@ specification. ## Decision The issues outlined above, are applicable only to SDK-based chains, and thus the proposed solution -are do not require specification changes that would result in modification to other implementations +are not required specification changes that would result in modifications to other implementations of the ICS20 spec. -Instead of adding the identifiers on the coin denomination directly, the proposed solution hashes +Instead of adding the identifiers to the coin denomination directly, the proposed solution hashes the denomination prefix in order to get a consistent length for all the cross-chain fungible tokens. This will be used for internal storage only, and when transferred via IBC to a different chain, the @@ -154,7 +154,7 @@ func (dt DenomTrace) IBCDenom() string { ### `x/ibc-transfer` Changes In order to retrieve the trace information from an IBC denomination, a lookup table needs to be -added to the `ibc-transfer` module. These values need to also be persisted between upgrades, meaning +added to the `ibc-transfer` module. These values also need to be persisted between upgrades, meaning that a new `[]DenomTrace` `GenesisState` field state needs to be added to the module: ```go @@ -171,7 +171,7 @@ func (k Keeper) GetDenomTrace(ctx Context, denomTraceHash []byte) (DenomTrace, b return denomTrace, true } -// HasDenomTrace checks if a the key with the given trace hash exists on the store. +// HasDenomTrace checks if the key with the given trace hash exists on the store. func (k Keeper) HasDenomTrace(ctx Context, denomTraceHash []byte) bool { store := ctx.KVStore(k.storeKey) return store.Has(types.KeyTrace(denomTraceHash)) @@ -274,7 +274,7 @@ func (k Keeper) DenomPathFromHash(ctx sdk.Context, denom string) (string, error) // receiving chain. // // NOTE: We use SourcePort and SourceChannel here, because the counterparty -// chain would have prefixed with DestPort and DestChannel when originally +// chain would have been prefixed with DestPort and DestChannel when originally // receiving this coin as seen in the "sender chain is the source" condition. if ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data.Denom) { // sender chain is not the source, unescrow tokens @@ -346,7 +346,7 @@ function will now: - Bump the maximum character length to 128, as the hex representation used by Tendermint's `HexBytes` type contains 64 characters. -Additional validation logic, such as verifying the length of the hash, the may be added to the bank module in the future if the [custom base denomination validation](https://github.com/cosmos/cosmos-sdk/pull/6755) is integrated into the SDK. +Additional validation logic, such as verifying the length of the hash, may be added to the bank module in the future if the [custom base denomination validation](https://github.com/cosmos/cosmos-sdk/pull/6755) is integrated into the SDK. ### Positive diff --git a/docs/architecture/adr-002-go-module-versioning.md b/docs/architecture/adr-002-go-module-versioning.md index 20959896bb7..d169eac63ab 100644 --- a/docs/architecture/adr-002-go-module-versioning.md +++ b/docs/architecture/adr-002-go-module-versioning.md @@ -17,14 +17,14 @@ v1.0.0 was decided to be used instead of v0.1.0 primarily for the following reas - Maintaining compatibility with the IBC specification v1 requires stronger support/guarantees. - Using the major, minor, and patch numbers allows for easier communication of what breaking changes are included in a release. -- The IBC module is being used by numerous high value projects which require stability. +- The IBC module is being used by numerous high value projects that require stability. ### Problems #### Go module version must be incremented When a Go module is released under v1.0.0, all following releases must follow Go semantic versioning. -Thus when the go API is broken, the Go module major version **must** be incremented. +Thus when the Go API is broken, the Go module major version **must** be incremented. For example, changing the go package version from `v2` to `v3` bumps the import from `github.com/cosmos/ibc-go/v2` to `github.com/cosmos/ibc-go/v3`. If the Go module version is not incremented then attempting to go get a module @v3.0.0 without the suffix results in: @@ -44,7 +44,7 @@ The more concerning problem is that protobuf definitions will also reach a names ibc-go and the Cosmos SDK in general rely heavily on using extended functions for go structs generated from protobuf definitions. This requires the go structs to be defined in the same package as the extended functions. Thus, bumping the import versioning causes the protobuf definitions to be generated in two places (in v2 and v3). -When registering these types at compile time, the go compiler will panic. +When registering these types at compile time, the Go compiler will panic. The generated types need to be registered against the proto codec, but there exist two definitions for the same name. The protobuf conflict policy can be overridden via the environment variable `GOLANG_PROTOBUF_REGISTRATION_CONFLICT`, but it is possible this could lead to various runtime errors or unexpected behaviour (see [here](https://github.com/protocolbuffers/protobuf-go/blob/master/reflect/protoregistry/registry.go#L46)). @@ -55,21 +55,21 @@ More information [here](https://developers.google.com/protocol-buffers/docs/refe #### Changing the protobuf definition version The protobuf definitions all have a type URL containing the protobuf version for this type. -Changing the protobuf version would solve the namespace collision which arise from importing multiple versions of ibc-go, but it leads to new issues. +Changing the protobuf version would solve the namespace collision that arises from importing multiple versions of ibc-go, but it leads to new issues. In the Cosmos SDK, `Any`s are unpacked and decoded using the type URL. Changing the type URL thus is creating a distinctly different type. The same registration on the proto codec cannot be used to unpack the new type. For example: -All Cosmos SDK messages are packed into `Any`s. If we incremented the protobuf version for our IBC messages, clients which submitted the v1 of our Cosmos SDK messages would now be rejected since the old type is not registered on the codec. +All Cosmos SDK messages are packed into `Any`s. If we increment the protobuf version for our IBC messages, clients that submitted the v1 of our Cosmos SDK messages would now be rejected since the old type is not registered on the codec. The clients must know to submit the v2 of these messages. This pushes the burden of versioning onto relayers and wallets. A more serious problem is that the `ClientState` and `ConsensusState` are packed as `Any`s. Changing the protobuf versioning of these types would break compatibility with IBC specification v1. #### Moving protobuf definitions to their own go module -The protobuf definitions could be moved to their own go module which uses 0.x versioning and will never go to 1.0. +The protobuf definitions could be moved to their own Go module which uses 0.x versioning and will never Go to 1.0. This prevents the Go module version from being incremented with breaking changes. It also requires all extended functions to live in the same Go module, disrupting the existing code structure. @@ -91,7 +91,7 @@ It is unclear when a user of the ibc-go code would need multiple versions of ibc Until there is an overwhelming reason to support importing multiple versions of ibc-go: **Major releases cannot be imported simultaneously**. -Releases should focus on keeping backwards compatibility for go code clients, within reason. +Releases should focus on keeping backwards compatibility for Go code clients, within reason. Old functionality should be marked as deprecated and there should exist upgrade paths between major versions. Deprecated functionality may be removed when no clients rely on that functionality. How this is determined is to be decided. diff --git a/docs/architecture/adr-003-ics27-acknowledgement.md b/docs/architecture/adr-003-ics27-acknowledgement.md index 38da73cac21..2a2d5b6928a 100644 --- a/docs/architecture/adr-003-ics27-acknowledgement.md +++ b/docs/architecture/adr-003-ics27-acknowledgement.md @@ -11,11 +11,11 @@ Accepted ## Context Upon receiving an IBC packet, an IBC application can optionally return an acknowledgement. -This acknowledgement will be hashed and written into state. Thus any changes to the information included in an acknowledgement are state machine breaking. +This acknowledgement will be hashed and written into state. Thus any changes to the information included in an acknowledgement are state machine-breaking. ICS27 executes transactions on behalf of a controller chain. Information such as the message result or message error may be returned from other SDK modules outside the control of the ICS27 module. It might be very valuable to return message execution information inside the ICS27 acknowledgement so that controller chain interchain account auth modules can act upon this information. -Only deterministic information returned from the message execution is allowed to be returned in the packet acknowledgement otherwise the network will halt due to a fork in the expected app hash. +Only deterministic information returned from the message execution is allowed to be returned in the packet acknowledgement otherwise, the network will halt due to a fork in the expected app hash. ## Decision @@ -72,7 +72,7 @@ Where `msgResponses` is a slice of the `MsgResponse`s packed into `Any`s. #### Forwards compatible approach -A forwards compatible approach was deemed infeasible. +A forward-compatible approach was deemed infeasible. The `handler` provided by the `MsgServiceRouter` will only include the `*sdk.Result` and an error (if one occurred). In v0.45 of the SDK, the `*sdk.Result.Data` will contain the MsgResponse marshaled data. However, the MsgResponse is not packed and marshaled as a `*codectypes.Any`, thus making it impossible from a generalized point of view to unmarshal the bytes. @@ -81,7 +81,7 @@ If the bytes could be unmarshaled, then they could be packed into an `*codectype Intercepting the MsgResponse before it becomes marshaled requires replicating this [code](https://github.com/cosmos/cosmos-sdk/blob/dfd47f5b449f558a855da284a9a7eabbfbad435d/baseapp/msg_service_router.go#L109-#L128). It may not even be possible to replicate the linked code. The method handler would need to be accessed somehow. -For these reasons it is deemed infeasible to attempt a forwards compatible approach. +For these reasons it is deemed infeasible to attempt a forward-compatible approach. ICA auth developers can interpret which format was used when constructing the transaction response by checking if the `sdk.TxMsgData.Data` field is non-empty. If the `sdk.TxMsgData.Data` field is not empty then the format for v0.45 was used, otherwise ICA auth developers can assume the transaction response uses the newer format. @@ -103,7 +103,7 @@ A test has been [written](https://github.com/cosmos/ibc-go/blob/v3.0.0/modules/a ## Consequences -> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. +> This section describes the consequences of applying the decision. All consequences should be summarized here, not just the "positive" ones. ### Positive diff --git a/docs/architecture/adr-004-ics29-lock-fee-module.md b/docs/architecture/adr-004-ics29-lock-fee-module.md index 5e6a0e927d8..6b7822361d8 100644 --- a/docs/architecture/adr-004-ics29-lock-fee-module.md +++ b/docs/architecture/adr-004-ics29-lock-fee-module.md @@ -12,7 +12,7 @@ Accepted The fee module maintains an escrow account for all fees escrowed to incentivize packet relays. It also tracks each packet fee escrowed separately from the escrow account. This is because the escrow account only maintains a total balance. It has no reference for which coins belonged to which packet fee. -In the presence of a severe bug, it is possible the escrow balance will become out of sync with the packet fees marked as escrowed. +In the presence of a severe bug, it is possible that the escrow balance will become out of sync with the packet fees marked as escrowed. The ICS29 module should be capable of elegantly handling such a scenario. ## Decision @@ -24,9 +24,9 @@ Manual intervention will be needed to unlock the fee module. ### Sending side -Special behaviour will have to be accounted for in `OnAcknowledgementPacket`. Since the counterparty will continue to send incentivized acknowledgements for fee enabled channels, the acknowledgement will still need to be unmarshalled into an incentivized acknowledgement before calling the underlying application `OnAcknowledgePacket` callback. +Special behaviour will have to be accounted for in `OnAcknowledgementPacket`. Since the counterparty will continue to send incentivized acknowledgements for fee-enabled channels, the acknowledgement will still need to be unmarshalled into an incentivized acknowledgement before calling the underlying application `OnAcknowledgePacket` callback. -When distributing fees, a cached context should be used. If the escrow account balance would become negative, the current state changes should be discarded and the fee module should be locked using the uncached context. This prevents fees from being partially distributed for a given packetID. +When distributing fees, a cached context should be used. If the escrow account balance becomes negative, the current state changes should be discarded and the fee module should be locked using the uncached context. This prevents fees from being partially distributed for a given packetID. ### Receiving side diff --git a/docs/architecture/adr-005-consensus-height-events.md b/docs/architecture/adr-005-consensus-height-events.md index eb0527b4532..b4bc3aacae4 100644 --- a/docs/architecture/adr-005-consensus-height-events.md +++ b/docs/architecture/adr-005-consensus-height-events.md @@ -32,7 +32,7 @@ To complement this flexibility, the `UpdateClient` handler will now support the ```go // ClientMessage is an interface used to update an IBC client. -// The update may be done by a single header, a batch of headers, misbehaviour, or any type which when verified produces +// The update may be done by a single header, a batch of headers, misbehaviour, or any type that when verified produces // a change to state of the IBC client type ClientMessage interface { proto.Message @@ -60,9 +60,9 @@ The following decisions have been made in order to provide flexibility to consum UpdateState(sdk.Context, codec.BinaryCodec, sdk.KVStore, ClientMessage) []Height ``` -2. Maintain the `consensus_height` event attribute emitted from the `02-client` update handler, but mark as deprecated for future removal. For example, with tendermint lightclients this will simply be `consensusHeights[0]` following a successful update using a single *Header*. +2. Maintain the `consensus_height` event attribute emitted from the `02-client` update handler, but mark it as deprecated for future removal. For example, with tendermint lightclients this will simply be `consensusHeights[0]` following a successful update using a single *Header*. -3. Add an additional `consensus_heights` event attribute, containing a comma separated list of updated heights. This provides flexibility for emitting a single consensus height or multiple consensus heights in the example use-case of batched header updates. +3. Add an additional `consensus_heights` event attribute, containing a comma-separated list of updated heights. This provides flexibility for emitting a single consensus height or multiple consensus heights in the example use-case of batched header updates. ## Consequences diff --git a/docs/architecture/adr-006-02-client-refactor.md b/docs/architecture/adr-006-02-client-refactor.md index 22721029405..8f8c2722f3f 100644 --- a/docs/architecture/adr-006-02-client-refactor.md +++ b/docs/architecture/adr-006-02-client-refactor.md @@ -34,19 +34,19 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, header exported.H ``` To add additional light clients, code would need to be added directly to the 02-client submodule. -Evidently, this would likely become problematic as IBC scaled to many chains using consensus mechanisms beyond the initial supported light clients. +Evidently, this would likely become problematic as IBC scaled to many chains using consensus mechanisms beyond the initially supported light clients. Issue [#6064](https://github.com/cosmos/cosmos-sdk/issues/6064) on the SDK addressed this problem by creating a more modular 02-client submodule. The 02-client submodule would now interact with each light client via an interface. While, this change was positive in development, increasing the flexibility and adoptability of IBC, it also opened the door to new problems. The difficulty of generalizing light clients became apparent once changes to those light clients were required. Each light client represents a different consensus algorithm which may contain a host of complexity and nuances. -Here are some examples of issues which arose for light clients that are not applicable to all the light clients supported (06-solomachine, 07-tendermint, 09-localhost): +Here are some examples of issues that arose for light clients that are not applicable to all the light clients supported (06-solomachine, 07-tendermint, 09-localhost): ### Tendermint non-zero height upgrades Before the launch of IBC, it was determined that the golang implementation of [tendermint](https://github.com/tendermint/tendermint) would not be capable of supporting non-zero height upgrades. -This implies that any upgrade would require changing of the chain ID and resetting the height to 0. +This implies that any upgrade would require changing the chain ID and resetting the height to 0. A chain is uniquely identified by its chain-id and validator set. Two different chain ID's can be viewed as different chains and thus a normal update produced by a validator set cannot change the chain ID. To work around the lack of support for non-zero height upgrades, an abstract height type was created along with an upgrade mechanism. @@ -93,7 +93,7 @@ As @seunlanlege [states](https://github.com/cosmos/ibc-go/issues/284#issuecommen > > - This would allow light clients to handle batch header updates in CheckHeaderAndUpdateState, for the special case of 11-beefy proving the finality for a batch of headers is much more space and time efficient than the space/time complexity of proving each individual headers in that batch, combined. > -> - This also allows for a single light client instance of 11-beefy be used to prove finality for every parachain connected to the relay chain (Polkadot/Kusama). We achieve this by setting the appropriate ConsensusState for individual parachain headers in CheckHeaderAndUpdateState +> - This also allows for a single light client instance of 11-beefy to be used to prove finality for every parachain connected to the relay chain (Polkadot/Kusama). We achieve this by setting the appropriate ConsensusState for individual parachain headers in CheckHeaderAndUpdateState ## Decision @@ -103,8 +103,8 @@ The IBC specification states: > If the provided header was valid, the client MUST also mutate internal state to store now-finalised consensus roots and update any necessary signature authority tracking (e.g. changes to the validator set) for future calls to the validity predicate. -The initial version of the IBC go SDK based module did not fulfill this requirement. -Instead, the 02-client submodule required each light client to return the client and consensus state which should be updated in the client prefixed store. +The initial version of the IBC go SDK-based module did not fulfill this requirement. +Instead, the 02-client submodule required each light client to return the client and consensus state which should be updated in the client-prefixed store. This decision lead to the issues "Solomachine doesn't set consensus states" and "New clients may want to do batch updates". Each light client should be required to set its own client and consensus states on any update necessary. @@ -115,7 +115,7 @@ This will allow more flexibility for light clients to manage their own internal Remove `GetHeight()` from the header interface (as light clients now set the client/consensus states). This results in the `Header`/`Misbehaviour` interfaces being the same. -To reduce complexity of the codebase, the `Header`/`Misbehaviour` interfaces should be merged into `ClientMessage`. +To reduce the complexity of the codebase, the `Header`/`Misbehaviour` interfaces should be merged into `ClientMessage`. `ClientMessage` will provide the client with some authenticated information which may result in regular updates, misbehaviour detection, batch updates, or other custom functionality a light client requires. ### Split `CheckHeaderAndUpdateState` into 4 functions @@ -129,7 +129,7 @@ Split `CheckHeaderAndUpdateState` into 4 functions: - `UpdateStateOnMisbehaviour` - `UpdateState` -`VerifyClientMessage` checks the that the structure of a `ClientMessage` is correct and that all authentication data provided is valid. +`VerifyClientMessage` checks that the structure of a `ClientMessage` is correct and that all authentication data provided is valid. `CheckForMisbehaviour` checks to see if a `ClientMessage` is evidence of misbehaviour. @@ -167,7 +167,7 @@ This fixes the issues outlined for the solo machine client. ### Add generic verification functions -As the complexity and the functionality grows, new verification functions will be required for additional paths. +As the complexity and functionality grows, new verification functions will be required for additional paths. This was explained in [#684](https://github.com/cosmos/ibc/issues/684) on the specification repo. These generic verification functions would be immediately useful for the new paths added in connection/channel upgradability as well as for custom paths defined by IBC applications such as Interchain Queries. The old verification functions (`VerifyClientState`, `VerifyConnection`, etc) should be removed in favor of the generic verification functions. @@ -177,7 +177,7 @@ The old verification functions (`VerifyClientState`, `VerifyConnection`, etc) sh ### Positive - Flexibility for light client implementations -- Well defined interfaces and their required functionality +- Well-defined interfaces and their required functionality - Generic verification functions - Applies changes necessary for future client/connection/channel upgrabability features - Timeout processing for solo machines diff --git a/docs/architecture/adr-008-app-caller-cbs.md b/docs/architecture/adr-008-app-caller-cbs.md index 585cd144c76..1b18afdfaac 100644 --- a/docs/architecture/adr-008-app-caller-cbs.md +++ b/docs/architecture/adr-008-app-caller-cbs.md @@ -13,13 +13,13 @@ Accepted, middleware implemented ## Context -IBC was designed with callbacks between core IBC and IBC applications. IBC apps would send a packet to core IBC. When the result of the packet lifecycle eventually resolved into either an acknowledgement or a timeout, core IBC called a callback on the IBC application so that the IBC application could take action on the basis of the result (e.g. unescrow tokens for ICS-20). +IBC was designed with callbacks between core IBC and IBC applications. IBC apps would send a packet to the core IBC. When the result of the packet lifecycle is eventually resolved into either an acknowledgement or a timeout, core IBC called a callback on the IBC application so that the IBC application could take action on the basis of the result (e.g. unescrow tokens for ICS-20). This setup worked well for off-chain users interacting with IBC applications. We are now seeing the desire for secondary applications (e.g. smart contracts, modules) to call into IBC apps as part of their state machine logic and then do some actions on the basis of the packet result. Or to receive a packet from IBC and do some logic upon receipt. -Example Usecases: +Example Use Cases: - Send an ICS-20 packet, and if it is successful, then send an ICA-packet to swap tokens on LP and return funds to sender - Execute some logic upon receipt of token transfer to a smart contract address @@ -36,7 +36,7 @@ Create a middleware that can interface between IBC applications and smart contra ## Data structures -The `CallbackPacketData` struct will get constructed from custom callback data in the application packet. The `CallbackAddress` is the IBC Actor address on which the callback should be called on. The `SenderAddress` is also provided to optionally allow a VM to ensure that the sender is the same as the callback address. +The `CallbackPacketData` struct will get constructed from custom callback data in the application packet. The `CallbackAddress` is the IBC Actor address on which the callback should be called. The `SenderAddress` is also provided to optionally allow a VM to ensure that the sender is the same as the callback address. The struct also defines a `CommitGasLimit` which is the maximum gas a callback is allowed to use. If the callback exceeds this limit, the callback will panic and the tx will commit without the callback's state changes. @@ -91,7 +91,7 @@ type CallbacksCompatibleModule interface { porttypes.PacketDataUnmarshaler } -// PacketDataUnmarshaler defines an optional interface which allows a middleware to +// PacketDataUnmarshaler defines an optional interface that allows a middleware to // request the packet data to be unmarshaled by the base application. type PacketDataUnmarshaler interface { // UnmarshalPacketData unmarshals the packet data into a concrete type @@ -105,7 +105,7 @@ type PacketDataUnmarshaler interface { The application's packet data must additionally implement the following interfaces: ```go -// PacketData defines an optional interface which an application's packet data structure may implement. +// PacketData defines an optional interface that an application's packet data structure may implement. type PacketData interface { // GetPacketSender returns the sender address of the packet data. // If the packet sender is unknown or undefined, an empty string should be returned. @@ -124,7 +124,7 @@ type PacketDataProvider interface { } ``` -The callback data can be embedded in an application packet by providing custom packet data for source and destination callback in the custom packet data under the appropriate key. +The callback data can be embedded in an application packet by providing custom packet data for the source and destination callback in the custom packet data under the appropriate key. ```jsonc // Custom Packet data embedded as a JSON object in the packet data @@ -176,8 +176,8 @@ It may also disable certain callback methods by simply performing a no-op. type ContractKeeper interface { // IBCSendPacketCallback is called in the source chain when a PacketSend is executed. The // packetSenderAddress is determined by the underlying module, and may be empty if the sender is - // unknown or undefined. The contract is expected to handle the callback within the user defined - // gas limit, and handle any errors, or panics gracefully. + // unknown or undefined. The contract is expected to handle the callback within the user-defined + // gas limit, and handle any errors or panics gracefully. // This entry point is called with a cached context. If an error is returned, then the changes in // this context will not be persisted, and the error will be propagated to the underlying IBC // application, resulting in a packet send failure. @@ -409,7 +409,7 @@ func (im IBCMiddleware) OnRecvPacket( // Call the IBCActor acknowledgementPacket callback after processing the packet // if the ackPacket callback exists and returns an error -// DO NOT return the error upstream. The acknowledgement must complete for the packet +// DO NOT return the error upstream. The acknowledgement must be completed for the packet // lifecycle to end, so the custom callback cannot block completion. // Instead we emit error events and set the error in state // so that users and on-chain logic can handle this appropriately @@ -494,13 +494,13 @@ func (im IBCModule) OnTimeoutPacket( // processCallback executes the callbackExecutor and reverts contract changes if the callbackExecutor fails. // // Error Precedence and Returns: -// - oogErr: Takes the highest precedence. If the callback runs out of gas, an error wrapped with types.ErrCallbackOutOfGas is returned. +// - oogErr: Takes the highest precedence. If the callback runs out of gas, an error is wrapped with types.ErrCallbackOutOfGas is returned. // - panicErr: Takes the second-highest precedence. If a panic occurs and it is not propagated, an error wrapped with types.ErrCallbackPanic is returned. // - callbackErr: If the callbackExecutor returns an error, it is returned as-is. // // panics if // - the contractExecutor panics for any reason, and the callbackType is SendPacket, or -// - the contractExecutor runs out of gas and the relayer has not reserved gas grater than or equal to +// - the contractExecutor runs out of gas and the relayer has not reserved gas greater than or equal to // CommitGasLimit. func (IBCMiddleware) processCallback( ctx sdk.Context, callbackType types.CallbackType, diff --git a/docs/architecture/adr-009-v6-ics27-msgserver.md b/docs/architecture/adr-009-v6-ics27-msgserver.md index 61d229e5d78..2f1bf84188e 100644 --- a/docs/architecture/adr-009-v6-ics27-msgserver.md +++ b/docs/architecture/adr-009-v6-ics27-msgserver.md @@ -35,7 +35,7 @@ To achieve this, as stated by [@damiannolan](https://github.com/cosmos/ibc-go/is > - `SendTx` 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: +Examples of existing SDK modules that would benefit from this change include: - x/auth - x/gov @@ -61,7 +61,7 @@ Underlying applications will be passed a `nil` capability in `OnChanOpenInit`. 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 migration which asserts the upgrade handler channel capability migration has been performed successfully +- ICS 27 module automatic migration which asserts that the upgrade handler channel capability migration has been performed successfully See issue [#2033](https://github.com/cosmos/ibc-go/issues/2033) @@ -77,8 +77,8 @@ See issue [#2145](https://github.com/cosmos/ibc-go/issues/2145) ### Future considerations -[ADR 008](https://github.com/cosmos/ibc-go/pull/1976) 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 availability of such a middleware as that will be the preferred method for executing application logic upon a ICS 27 packet send. +[ADR 008](https://github.com/cosmos/ibc-go/pull/1976) proposes the creation of a middleware that 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 availability of such a middleware as that will be the preferred method for executing application logic upon an ICS 27 packet send. ### Miscellaneous @@ -101,12 +101,12 @@ See issue [#2165](https://github.com/cosmos/ibc-go/issues/2165) - 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 aware of accounts they did not register) +- only underlying applications that registered the interchain account will perform application logic for that account (underlying applications do not need to be aware of accounts they did not register) ### Negative - the security model has been reduced to that of the SDK. SDK modules may send packets for any interchain account. -- additional maintenance of the messages added and the middleware enabled flag +- additional maintenance of the messages added and the middleware-enabled flag - underlying applications which will become ADR 008 modules are not required to be aware of accounts they did not register - calling legacy API vs the new API results in different behaviour for ICS 27 application stacks which have an underlying application diff --git a/docs/architecture/adr-010-light-clients-as-sdk-modules.md b/docs/architecture/adr-010-light-clients-as-sdk-modules.md index b2459fce722..4153d63f3f5 100644 --- a/docs/architecture/adr-010-light-clients-as-sdk-modules.md +++ b/docs/architecture/adr-010-light-clients-as-sdk-modules.md @@ -17,7 +17,7 @@ ibc-go has 3 main consumers: - relayers Relayers listen and respond to events emitted by ibc-go while IBC light clients and applications are invoked by core IBC. -Currently there exists two different approaches to callbacks being invoked by core IBC. +Currently there exist two different approaches to callbacks being invoked by the core IBC. IBC light clients currently are invoked by a `ClientState` and `ConsensusState` interface as defined by [core IBC](https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/exported/client.go#L36). The 02-client submodule will retrieve the `ClientState` or `ConsensusState` from the IBC store in order to perform callbacks to the light client. @@ -29,7 +29,7 @@ In addition, without increasing the size of the defined `ClientState` interface, The other approach used to perform callback logic is via registered SDK modules. This approach is used by core IBC to interact with IBC applications. IBC applications will register their callbacks on the IBC router at compile time. -When a packet comes in, core IBC will use the IBC router to lookup the registered callback functions for the provided packet. +When a packet comes in, core IBC will use the IBC router to look up the registered callback functions for the provided packet. The benefit of registered callbacks opposed to interface functions is that additional information may be accessed via external keepers. Because the IBC applications are also SDK modules, they additionally get access to a host of functionality provided by the SDK. This includes: genesis import/export, migrations, query/transaction CLI commands, type registration, gRPC query registration, and message server registration. @@ -77,9 +77,9 @@ type ClientState interface { } ``` -For the most part, any functions which require access to the client store should likely not be an interface function of the `ClientState`. +For the most part, any functions that require access to the client store should likely not be an interface function of the `ClientState`. -`ExportMetadata` should eventually be replaced by a light client's ability to import/export it's own genesis information. +`ExportMetadata` should eventually be replaced by a light client's ability to import/export its own genesis information. ### Intermodule communication diff --git a/docs/architecture/adr-015-ibc-packet-receiver.md b/docs/architecture/adr-015-ibc-packet-receiver.md index 3fbe090ce94..c8e956a3dfe 100644 --- a/docs/architecture/adr-015-ibc-packet-receiver.md +++ b/docs/architecture/adr-015-ibc-packet-receiver.md @@ -12,7 +12,7 @@ In ICS 26, the routing module is defined as a layer above each application modul which verifies and routes messages to the destination modules. It is possible to implement it as a separate module, however, we already have the functionality to route messages upon the destination identifiers in the baseapp. This ADR suggests -to utilize existing `baseapp.router` to route packets to application modules. +utilizing the existing `baseapp.router` to route packets to application modules. Generally, routing module callbacks have two separate steps in them, verification and execution. This corresponds to the `AnteHandler`-`Handler` @@ -20,9 +20,9 @@ model inside the SDK. We can do the verification inside the `AnteHandler` in order to increase developer ergonomics by reducing boilerplate verification code. -For atomic multi-message transaction, we want to keep the IBC related +For an atomic multi-message transaction, we want to keep the IBC-related state modification to be preserved even the application side state change -reverts. One of the example might be IBC token sending message following with +reverts. One of the examples might be IBC token sending message followed by stake delegation which uses the tokens received by the previous packet message. If the token receiving fails for any reason, we might not want to keep executing the transaction, but we also don't want to abort the transaction @@ -33,7 +33,7 @@ This ADR suggests new `CodeType`, `CodeTxBreak`, to fix this problem. `PortKeeper` will have the capability key that is able to access only the channels bound to the port. Entities that hold a `PortKeeper` will be -able to call the methods on it which are corresponding with the methods with +able to call the methods on it which are corresponding to the methods with the same names on the `ChannelKeeper`, but only with the allowed port. `ChannelKeeper.Port(string, ChannelChecker)` will be defined to easily construct a capability-safe `PortKeeper`. This will be addressed in @@ -121,15 +121,15 @@ are `sdk.Msg` types correspond to `handleUpdateClient`, `handleRecvPacket`, respectively. The side effects of `RecvPacket`, `VerifyAcknowledgement`, -`VerifyTimeout` will be extracted out into separated functions, +`VerifyTimeout` will be extracted into separate functions, `WriteAcknowledgement`, `DeleteCommitment`, `DeleteCommitmentTimeout`, respectively, which will be called by the application handlers after the execution. `WriteAcknowledgement` writes the acknowledgement to the state that can be -verified by the counter-party chain and increments the sequence to prevent +verified by the counterparty chain and increments the sequence to prevent double execution. `DeleteCommitment` will delete the commitment stored, `DeleteCommitmentTimeout` will delete the commitment and close channel in case -of ordered channel. +of the ordered channel. ```go func (keeper ChannelKeeper) WriteAcknowledgement(ctx Context, packet Packet, ack []byte) { @@ -156,22 +156,22 @@ in order to increase sequence (in case of packet) or remove the commitment (in case of acknowledgement and timeout). Calling those functions implies that the application logic has successfully executed. However, the handlers can return `Result` with `CodeTxBreak` after calling those methods -which will persist the state changes that has been already done but prevent any further -messages to be executed in case of semantically invalid packet. This will keep the sequence +which will persist the state changes that have already been done but prevent any further +messages to be executed in case of a semantically invalid packet. This will keep the sequence increased in the previous IBC packets(thus preventing double execution) without proceeding to the following messages. In any case the application modules should never return state reverting result, which will make the channel unable to proceed. `ChannelKeeper.CheckOpen` method will be introduced. This will replace `onChanOpen*` defined -under the routing module specification. Instead of define each channel handshake callback +under the routing module specification. Instead of defining each channel handshake callback functions, application modules can provide `ChannelChecker` function with the `AppModule` which will be injected to `ChannelKeeper.Port()` at the top level application. `CheckOpen` will find the correct `ChannelChecker` using the `PortID` and call it, which will return an error if it is unacceptable by the application. The `ProofVerificationDecorator` will be inserted to the top level application. -It is not safe to make each module responsible to call proof verification +It is not safe to make each module responsible for calling proof verification logic, whereas application can misbehave(in terms of IBC protocol) by mistake. @@ -259,7 +259,7 @@ func handlePacketDataTransfer(ctx Context, k Keeper, packet Packet, data PacketD func handleCustomTimeoutPacket(ctx Context, k Keeper, packet CustomPacket) Result { err := k.RecoverTransfer(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetDestinationPort(), packet.GetDestinationChannel(), data) if err != nil { - // This chain sent invalid packet or cannot recover the funds + // This chain sent an invalid packet or cannot recover the funds panic(err) } k.ChannelKeeper.DeleteCommitmentTimeout(ctx, packet) @@ -291,7 +291,7 @@ Proposed ### Neutral - Introduces new `AnteHandler` decorator. -- Dynamic ports can be supported using hierarchical port identifier, see #5290 for detail +- Dynamic ports can be supported using a hierarchical port identifier, see #5290 for details ## References diff --git a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md index efad623b5c9..1c6b44f5c41 100644 --- a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md +++ b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md @@ -18,16 +18,16 @@ ### Summary -At launch, IBC will be a novel protocol, without an experienced user-base. At the protocol layer, it is not possible to distinguish between client expiry or misbehaviour due to genuine faults (Byzantine behaviour) and client expiry or misbehaviour due to user mistakes (failing to update a client, or accidentally double-signing). In the base IBC protocol and ICS 20 fungible token transfer implementation, if a client can no longer be updated, funds in that channel will be permanently locked and can no longer be transferred. To the degree that it is safe to do so, it would be preferable to provide users with a recovery mechanism which can be utilised in these exceptional cases. +At launch, IBC will be a novel protocol, without an experienced user-base. At the protocol layer, it is not possible to distinguish between client expiry or misbehaviour due to genuine faults (Byzantine behaviour) and client expiry or misbehaviour due to user mistakes (failing to update a client, or accidentally double-signing). In the base IBC protocol and ICS 20 fungible token transfer implementation, if a client can no longer be updated, funds in that channel will be permanently locked and can no longer be transferred. To the degree that it is safe to do so, it would be preferable to provide users with a recovery mechanism that can be utilised in these exceptional cases. ### Exceptional cases The state of concern is where a client associated with connection(s) and channel(s) can no longer be updated. This can happen for several reasons: -1. The chain which the client is following has halted and is no longer producing blocks/headers, so no updates can be made to the client -1. The chain which the client is following has continued to operate, but no relayer has submitted a new header within the unbonding period, and the client has expired +1. The chain that the client is following has halted and is no longer producing blocks/headers, so no updates can be made to the client +1. The chain that the client is following has continued to operate, but no relayer has submitted a new header within the unbonding period, and the client has expired 1. This could be due to real misbehaviour (intentional Byzantine behaviour) or merely a mistake by validators, but the client cannot distinguish these two cases -1. The chain which the client is following has experienced a misbehaviour event, and the client has been frozen & thus can no longer be updated +1. The chain that the client is following has experienced a misbehaviour event, and the client has been frozen & thus can no longer be updated ### Security model @@ -35,7 +35,7 @@ Two-thirds of the validator set (the quorum for governance, module participation ## Decision -We elect not to deal with chains which have actually halted, which is necessarily Byzantine behaviour and in which case token recovery is not likely possible anyways (in-flight packets cannot be timed-out, but the relative impact of that is minor). +We elect not to deal with chains that have actually halted, which is necessarily Byzantine behaviour and in which case token recovery is not likely possible anyway (in-flight packets cannot be timed out, but the relative impact of that is minor). 1. Require Tendermint light clients (ICS 07) to be created with the following additional flags 1. `allow_update_after_expiry` (boolean, default true). Note that this flag has been deprecated, it remains to signal intent but checks against this value will not be enforced. @@ -70,7 +70,7 @@ This ADR does not address planned upgrades, which are handled separately as per - Establishes a mechanism for client recovery in the case of expiry - Establishes a mechanism for client recovery in the case of misbehaviour -- Constructing an ClientUpdate Proposal is as difficult as creating a new client +- Constructing a ClientUpdate Proposal is as difficult as creating a new client ### Negative diff --git a/docs/architecture/adr-027-ibc-wasm.md b/docs/architecture/adr-027-ibc-wasm.md index c8fa15fdd32..20ba79f3134 100644 --- a/docs/architecture/adr-027-ibc-wasm.md +++ b/docs/architecture/adr-027-ibc-wasm.md @@ -1,4 +1,4 @@ -# ADR 27: Add support for Wasm based light client +# ADR 27: Add support for Wasm-based light client ## Changelog @@ -13,7 +13,7 @@ ## Abstract In the Cosmos SDK light clients are currently hardcoded in Go. This makes upgrading existing IBC light clients or -adding support for new light client a multi step process involving on-chain governance which is time-consuming. +adding support for new light client a multi-step process involving on-chain governance which is time-consuming. To remedy this, we are proposing a Wasm VM to host light client bytecode, which allows easier upgrading of existing IBC light clients as well as adding support for new IBC light clients without requiring a code release and @@ -23,7 +23,7 @@ corresponding hard-fork event. Currently in ibc-go light clients are defined as part of the codebase and are implemented as modules under `modules/light-clients`. Adding support for new light clients or updating an existing light client in the event -of a security issue or consensus update is a multi-step process which is both time-consuming and error-prone. +of a security issue or consensus update is a multi-step process that is both time-consuming and error-prone. In order to enable new IBC light client implementations it is necessary to modify the codebase of ibc-go (if the light client is part of its codebase), re-build chains' binaries, pass a governance proposal and validators upgrade their nodes. @@ -37,7 +37,7 @@ new light clients a simple governance-gated transaction. The light client byteco runs inside a Wasm VM. The Wasm light client submodule exposes a proxy light client interface that routes incoming messages to the appropriate handler function, inside the Wasm VM for execution. -With the Wasm light client module, anybody can add new IBC light client in the form of Wasm bytecode (provided they are +With the Wasm light client module, anybody can add a new IBC light client in the form of Wasm bytecode (provided they are able to submit the governance proposal transaction and that it passes) as well as instantiate clients using any created client type. This allows any chain to update its own light client in other chains without going through the steps outlined above. @@ -103,7 +103,7 @@ returned to the caller. Consider the example of the `VerifyClientMessage` function of `ClientState` interface. Incoming arguments are packaged inside a payload object that is then JSON serialized and passed to `queryContract`, which executes `WasmVm.Query` -and returns the slice of bytes returned by the smart contract. This data is deserialized and passed as return argument. +and returns the slice of bytes returned by the smart contract. This data is deserialized and passed as a return argument. ```go type QueryMsg struct { @@ -157,11 +157,11 @@ the `ClientState` functions. ### Positive -- Adding support for new light client or upgrading existing light client is way easier than before and only requires single transaction instead of a hard-fork. +- Adding support for a new light client or upgrading existing light client is way easier than before and only requires single transaction instead of a hard-fork. - Improves maintainability of ibc-go, since no change in codebase is required to support new client or upgrade it. - The existence of support for Rust dependencies in light clients which may not exist in Go. ### Negative -- Light clients written in Rust need to be written in a subset of Rust which could compile in Wasm. +- Light clients written in Rust need to be written in a subset of Rust that could compile in Wasm. - Introspecting light client code is difficult as only compiled bytecode exists in the blockchain.