[IBC] Implement the ICS-02 Client interfaces #933

h5law · 2023-07-24T10:18:25Z

Description

Summary generated by Reviewpad on 26 Jul 23 10:09 UTC

This pull request introduces various changes across multiple files in the codebase.

Here is a summary of the changes:

The file ibc/main_test.go has undergone changes related to the constructor method call from "newProvableStore" to "NewProvableStore".
The file ibc/main_test.go has changes in the function newTestP2PModule, bus.RegisterModule(p2pMock) line removal, and prepareEnvironment function modification.
The file queries.go in the ibc/client/types package includes the addition of new functions for retrieving and storing client and consensus states.
The file submodule.go in the ibc/client package adds the implementation of a client manager module for managing the creation, update, and upgrade of clients.
The file ibc/client/types/validate_test.go contains new test cases for validating various types in the types package.
The file keys_ics02.go in the ibc/path package includes changes related to store keys and paths for client and consensus states.
The file provable_store_test.go has changes in the test cases and the constructor method call, replacing newProvableStore with NewProvableStore.
The file queries.go in the ibc/client package introduces new functions for retrieving client and consensus states.
The file upgrade.go adds new types and functions related to upgrading the client state.
The file events.go includes implementations related to emitting events for the IBC client.
The file event_keys.go defines constants and variables related to the events emitted by the Client submodule.
The file misbehaviour.go adds a new struct and methods related to handling misbehavior by clients.
The file header.go adds a new file with types and methods related to the Wasm client consensus algorithm.
The file consensus_state.go includes the implementation of the ConsensusState struct and its associated methods.
The file ibc/docs/README.md has additions related to the ICS-02 implementation and links to relevant resources.
The file fraction.go introduces a new file with types and methods for comparing and performing operations on fractions.
The file errors.go includes changes related to error codes and a new error.
The file update.go adds types and methods for verifying and updating client states.
The file introspect.go introduces new functions for retrieving and verifying client and consensus states.
The file types.go adds implementations related to the IBC client for a Wasm module.
The file ibc/host/submodule.go includes changes related to imports and function modifications.

Please review these changes carefully to ensure they adhere to the code's functionality and requirements.

Issue

Fixes #894

Type of change

Please mark the relevant option(s):

List of changes

Implement the generic WASM messages according to their interfaces
Implement the ClientManager interface in the client submodule
Finish ICS-24 related host introspection methods
Leave placeholders for [IBC] Implement ICS-08 WASM light clients for Pocket V1 #912 WASM implementations
Add unit tests
Update documentation

Testing

make develop_test; if any code changes were made
make test_e2e on k8s LocalNet; if any code changes were made
e2e-devnet-test passes tests on DevNet; if any code was changed
Docker Compose LocalNet; if any major functionality was changed or introduced
k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
I have tested my changes using the available tooling
I have updated the corresponding CHANGELOG

If Applicable Checklist

I have updated the corresponding README(s); local and/or global
I have added tests that prove my fix is effective or that my feature works
I have added, or updated, mermaid.js diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

h5law · 2023-07-26T09:49:29Z

References for the reviewer:
[1] ICS-02 spec
[2] Most up to date ICS-08 spec
[3] IBC Light Client explanatory series

CC: @Olshansk

h5law · 2023-07-26T09:51:24Z

@srdtrk if you are able to review this PR when you have some time I would really appreciate your feedback. This is an implementation of ICS-02, specifically for enabling ICS-08 (not implemented here). The code should look and feel similar to ibc-go and the spec, any feedback is appreciated

Olshansk

Did my best to read, understand and review this but I'm sure I missed a bunch of stuff along the way.

That aside, great job! Well structured, easy to follow, some of the most understandable code I've seen, especially given the level of complexity and number of moving parts!

ibc/client/events.go

ibc/client/introspect.go

Olshansk · 2023-08-01T22:32:47Z

ibc/client/introspect.go

+// This function is used to validate the state of a client running on a
+// counterparty chain.


This comment is confusing me.

Is the purpose of this to verify the state of pocket on another chain, or the state of another chain on pocket?

So this function gets the current network state and is used in the verification of a pocket client running on another network. I will try to clarify the comment

Olshansk · 2023-08-01T22:34:19Z

ibc/client/introspect.go

+	if err != nil {
+		return nil, err
+	}
+	// TODO_AFTER(#705): use the actual MinimumBlockTime once set


#705 will likely be done by the time this is merged in

@red-0ne FYI.

Olshansk · 2023-08-02T00:53:02Z

ibc/docs/ics02.md

+type ClientMessage interface
+```
+
+These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client.


Suggested change

These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client.

These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field, which contains a serialised, opaque data structure for use within the WASM client.

Olshansk · 2023-08-02T00:53:31Z

ibc/docs/ics02.md

+
+These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client.
+
+The `data` field is a JSON serialised payload that contains the data required for the client to carry out the desired operation, as well as the operation name to carry out. For example, a verify membership payload is constructed using the following `struct`s:


This is a good instance of embedding the structure in the code: it's an example.

Olshansk · 2023-08-02T00:54:03Z

ibc/docs/ics02.md

+)
+```
+
+By utilising this pattern of JSON payloads the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState` for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage.


Suggested change

By utilising this pattern of JSON payloads the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState` for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage.

By utilising this pattern of JSON payloads, the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState`, for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage.

Olshansk · 2023-08-02T00:56:49Z

ibc/docs/ics02.md

+
+## Provable Stores
+
+ICS-02 requires a lot of data to be stored in the IBC stores (defined in [ICS-24](./ics24.md)). In order to do this the provable stores must be initialised on a per client ID basis. This means that any operation using the provable store does not require the use of the `clientID`. This is done as follows:


Suggested change

ICS-02 requires a lot of data to be stored in the IBC stores (defined in [ICS-24](./ics24.md)). In order to do this the provable stores must be initialised on a per client ID basis. This means that any operation using the provable store does not require the use of the `clientID`. This is done as follows:

ICS-02 requires a lot of data to be stored in the IBC stores (defined in [ICS-24](./ics24.md)). In order to do this, the provable stores must be initialised on a per-client ID basis. This means that any operation using the provable store does not require using the `clientID`. This is done as follows:

Olshansk · 2023-08-02T00:57:45Z

ibc/docs/ics02.md

+1. `ClientState`
+   - `ClientState` is an opaque data structure defined by a client type. It may keep arbitrary internal state to track verified roots and past misbehaviours.
+2. `ConsensusState`
+   - `ConsensusState` is an opaque data structure defined by a client type, used by the
+     validity predicate to verify new commits & state roots. Likely the structure will contain the last commit produced by the consensus process, including signatures and validator set metadata.
+3. `ClientMessage`
+   - `ClientMessage` is an opaque data structure defined by a client type which provides information to update the client. `ClientMessage`s can be submitted to an associated client to add new `ConsensusState`(s) and/or update the `ClientState`. They likely contain a height, a proof, a commitment root, and possibly updates to the validity predicate.
+4. `Height`
+   - `Height` is an interface that defines the methods required by a clients implementation of their own height object `Height`s usually have two components: revision number and revision height.


Maybe add these to the definitions seciton above?

It's good context as one starts reading everything.

Suggested change

1. `ClientState`

- `ClientState` is an opaque data structure defined by a client type. It may keep arbitrary internal state to track verified roots and past misbehaviours.

2. `ConsensusState`

- `ConsensusState` is an opaque data structure defined by a client type, used by the

validity predicate to verify new commits & state roots. Likely the structure will contain the last commit produced by the consensus process, including signatures and validator set metadata.

3. `ClientMessage`

- `ClientMessage` is an opaque data structure defined by a client type which provides information to update the client. `ClientMessage`s can be submitted to an associated client to add new `ConsensusState`(s) and/or update the `ClientState`. They likely contain a height, a proof, a commitment root, and possibly updates to the validity predicate.

4. `Height`

- `Height` is an interface that defines the methods required by a clients implementation of their own height object `Height`s usually have two components: revision number and revision height.

1. `ClientState` - an opaque data structure defined by a client type. It may keep an arbitrary internal state to track verified roots and past misbehaviours.

2. `ConsensusState` - an opaque data structure defined by a client type, used by the validity predicate to verify new commits & state roots. Likely the structure will contain the last commit produced by the consensus process, including signatures and validator set metadata.

3. `ClientMessage` - an opaque data structure defined by a client type which provides information to update the client. `ClientMessage`s can be submitted to an associated client to add new `ConsensusState`(s) and/or update the `ClientState`. They likely contain a height, a proof, a commitment root, and possibly updates to the validity predicate.

4. `Height` - an interface that defines the methods required by a clients implementation of their own height object `Height`s usually have two components: revision number and revision height.

Olshansk · 2023-08-02T01:01:38Z

@srdtrk Let us know if/when you can take a look at this as well!

h5law added 9 commits July 21, 2023 10:02

Add cosmos/ics23 proto into local proto with conversion

672e202

Add makefile auto download and sed on proto

513005c

add missing conversion field

0ce694a

pick: conversion funcs

ac50e06

unexport methods

06cb90e

remove unused functions

064f69c

Remove Height field from Events, use uint64 height in queries

61fcc38

Add ICS-02 Client Interfaces

1519ad0

Add ICS-02 Client Implementations and Methods

d87cf1c

h5law added e2e-devnet-test Runs E2E tests on devnet ibc IBC specific changes labels Jul 24, 2023

h5law added this to the M7: Pocket NoS (North Star) milestone Jul 24, 2023

h5law requested review from red-0ne, Olshansk and dylanlott July 24, 2023 10:18

h5law self-assigned this Jul 24, 2023

reviewpad bot added large Pull request is large waiting-for-review labels Jul 24, 2023

h5law changed the base branch from main to ibc/event_height July 24, 2023 10:18

h5law changed the base branch from ibc/event_height to ibc/client_semantics_interfaces July 24, 2023 10:19

Base automatically changed from ibc/client_semantics_interfaces to main July 26, 2023 09:58

h5law added 2 commits July 26, 2023 11:00

Merge branch 'main' into ibc/client_semantics_implementations

b5958f8

fixup: flush

b218b27

Olshansk requested changes Aug 2, 2023

View reviewed changes

Olshansk removed the request for review from red-0ne August 2, 2023 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IBC] Implement the ICS-02 Client interfaces #933

[IBC] Implement the ICS-02 Client interfaces #933

h5law commented Jul 24, 2023 •

edited by reviewpad bot

h5law commented Jul 26, 2023

h5law commented Jul 26, 2023

Olshansk left a comment

Olshansk Aug 1, 2023

h5law Aug 9, 2023 •

edited

Olshansk Aug 1, 2023

Olshansk Aug 2, 2023

Olshansk Aug 2, 2023

Olshansk Aug 2, 2023

Olshansk Aug 2, 2023

Olshansk Aug 2, 2023

Olshansk commented Aug 2, 2023

		// This function is used to validate the state of a client running on a
		// counterparty chain.

	These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client.
	These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field, which contains a serialised, opaque data structure for use within the WASM client.


		These interfaces are generic but have unique implementations for each client type. As Pocket utilises WASM light clients each implementation contains a `data []byte` field which contains a serialised, opaque data structure for use within the WASM client.

		The `data` field is a JSON serialised payload that contains the data required for the client to carry out the desired operation, as well as the operation name to carry out. For example, a verify membership payload is constructed using the following `struct`s:

	By utilising this pattern of JSON payloads the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState` for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage.
	By utilising this pattern of JSON payloads, the WASM client itself is able to unmarshal the opaque payload into their own internal protobuf definitions for the implementation of the `ClientState`, for example. This allows them to have a much simpler implementation and focus solely on the logic around verification and utilising simple storage.


		## Provable Stores

		ICS-02 requires a lot of data to be stored in the IBC stores (defined in [ICS-24](./ics24.md)). In order to do this the provable stores must be initialised on a per client ID basis. This means that any operation using the provable store does not require the use of the `clientID`. This is done as follows:

[IBC] Implement the ICS-02 Client interfaces #933

Are you sure you want to change the base?

[IBC] Implement the ICS-02 Client interfaces #933

Conversation

h5law commented Jul 24, 2023 • edited by reviewpad bot

Description

Summary generated by Reviewpad on 26 Jul 23 10:09 UTC

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

h5law commented Jul 26, 2023

h5law commented Jul 26, 2023

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk Aug 1, 2023

Choose a reason for hiding this comment

h5law Aug 9, 2023 • edited

Choose a reason for hiding this comment

Olshansk Aug 1, 2023

Choose a reason for hiding this comment

Olshansk Aug 2, 2023

Choose a reason for hiding this comment

Olshansk Aug 2, 2023

Choose a reason for hiding this comment

Olshansk Aug 2, 2023

Choose a reason for hiding this comment

Olshansk Aug 2, 2023

Choose a reason for hiding this comment

Olshansk Aug 2, 2023

Choose a reason for hiding this comment

Olshansk commented Aug 2, 2023

h5law commented Jul 24, 2023 •

edited by reviewpad bot

h5law Aug 9, 2023 •

edited