Skip to content
Open
12 changes: 6 additions & 6 deletions docs/architecture/adr-001-coin-source-tracing.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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))
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
14 changes: 7 additions & 7 deletions docs/architecture/adr-002-go-module-versioning.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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)).
Expand All @@ -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.

Expand All @@ -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.
Expand Down
10 changes: 5 additions & 5 deletions docs/architecture/adr-003-ics27-acknowledgement.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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

Expand Down
6 changes: 3 additions & 3 deletions docs/architecture/adr-004-ics29-lock-fee-module.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
6 changes: 3 additions & 3 deletions docs/architecture/adr-005-consensus-height-events.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
Loading