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

docs: update v9 migration doc for merkle path v2 #6708

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

damiannolan
Copy link
Contributor

@damiannolan damiannolan commented Jun 26, 2024

Description

Migration docs for changes included in #6644

needs: #6644
closes: #6496


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

Copy link
Contributor Author

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Left some notes! Sorry about the messy diff, but the doc needed to cleaned up a little

Comment on lines 27 to 51
### ICS27 - Interchain Accounts

In [#5785](https://github.com/cosmos/ibc-go/pull/5785) the list of arguments of the `NewKeeper` constructor function of the host submodule was extended with an extra argument for the gRPC query router that the submodule uses when executing a [`MsgModuleQuerySafe`](https://github.com/cosmos/ibc-go/blob/eecfa5c09a4c38a5c9f2cc2a322d2286f45911da/proto/ibc/applications/interchain_accounts/host/v1/tx.proto#L41-L51) to perform queries that are module safe:

```diff
func NewKeeper(
cdc codec.Codec, key storetypes.StoreKey, legacySubspace icatypes.ParamSubspace,
ics4Wrapper porttypes.ICS4Wrapper, channelKeeper icatypes.ChannelKeeper,
portKeeper icatypes.PortKeeper, accountKeeper icatypes.AccountKeeper,
scopedKeeper exported.ScopedKeeper, msgRouter icatypes.MessageRouter,
+ queryRouter icatypes.QueryRouter,
authority string,
) Keeper
```

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

```diff
func (k Keeper) RegisterInterchainAccount(
ctx sdk.Context,
connectionID, owner,
version string,
+ ordering channeltypes.Order
) error {
```
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 moved this section from core to apps. Sorry about the diff!

Comment on lines +57 to +66
- The `exported.ChannelI` and `exported.CounterpartyChannelI` interfaces have been removed. Please use the concrete types.
- The `exported.ConnectionI` and `exported.CounterpartyConnectionI` interfaces have been removed. Please use the concrete types.
- The `exported.Proof` interface has been removed. Please use the `MerkleProof` concrete type.
- The functions `GetState()`, `GetOrdering()`, `GetCounterparty()`, `GetConnectionHops()`, `GetVersion()` of the `Channel` type have been removed.
- The functions `GetPortID()`, `GetChannelID()` of the `CounterpartyChannel` type have been removed.
- The functions `GetClientID()`, `GetState()`, `GetCounterparty()`, `GetVersions()`, and `GetDelayPeriod` of the `Connection` type have been removed.
- The functions `GetClientID()`, `GetConnectionID()`, and `GetPrefix()` of the `CounterpartyConnection` type have been removed.
- The utility function `QueryLatestConsensusState` of `04-channel` CLI has been removed.
- `UnmarshalPacketData` now takes in the context, portID, and channelID. This allows the packet data to be unmarshaled based on the channel version.
- `Router` reference has been removed from IBC core keeper: [#6138](https://github.com/cosmos/ibc-go/pull/6138). Please use `PortKeeper.Router` instead.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made bullets


The utility function `QueryLatestConsensusState` of `04-channel` CLI has been removed.
- The `QueryVerifyMembershipRequest` protobuf message has been modified to include `commitment.v2.MerklePath`. The deprecated `commitment.v1.MerklePath` field has been `reserved`. [See 23-commitment](#23-commitment)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entry for change to QueryVerifyMembershipRequest msg


The testing package functions `coordinator.Setup`, `coordinator.SetupClients`, `coordinator.SetupConnections`, `coordinator.CreateConnections`, and `coordinator.CreateChannels` have been deprecated and will be removed in v10.
Please use the new functions `path.Setup`, `path.SetupClients`, `path.SetupConnections`, `path.CreateConnections`, `path.CreateChannels`.
- The `MerklePath` type has been deprecated and a new `commitment.v2.MerklePath` type has been introduced (ref: [#6644](https://github.com/cosmos/ibc-go/pull/6644)). The new `commitment.v2.MerklePath` contains `repeated bytes` in favour of `repeated string`. This allows users to prove values stored under keys which contain non-utf8 encoded symbols. As a result, changes have been made the 02-client `Query` service and 08-wasm contract api messages for JSON blobs. See [02-client](#02-client) and [08-wasm](#08-wasm), respectively.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entry for 23-commitment changes.

@@ -139,5 +144,6 @@ The `IterateConsensusMetadata` function has been removed.

### 08-wasm

The `ExportMetadataMsg` struct has been removed and is no longer required for contracts to implement. Core IBC will handle exporting all key/value's written to the store by a light client contract.
The `ZeroCustomFields` interface function has been removed from the `ClientState` interface. Core IBC only used this function to set tendermint client states when scheduling an IBC software upgrade. The interface function has been replaced by a type assertion.
- The `VerifyMembershipMsg` and `VerifyNonMembershipMsg` payloads for `SudoMsg` have been extended to include a new field, `MerklePath`. The existing `Path` field will remain the same. The new `MerklePath` field is used if and only if the provided key path contains non-utf8 encoded symbols, and as a result will encode the JSON field `merkle_path` as a base64 encoded bytestring. See [23-commitment](#23-commitment).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entry for 08-wasm contract api additions

@damiannolan damiannolan added the priority PRs that need prompt reviews label Jun 26, 2024
@damiannolan damiannolan marked this pull request as ready for review June 26, 2024 08:49
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! ❤️

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you, @damiannolan

@crodriguezvega crodriguezvega added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit 56e526c Jul 2, 2024
26 checks passed
@crodriguezvega crodriguezvega deleted the damian/6496-merklepath-migration-docs branch July 2, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority PRs that need prompt reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change MerklePath to use bytes instead of string
3 participants