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.

Sign up for GitHub

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

Adding docs for client state interface #2886

Merged
merged 11 commits into from
Dec 8, 2022
Merged

Adding docs for client state interface #2886

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1169822
docs: adding docs for client state
chatton Dec 5, 2022
44c11cf
docs: updating client-state docs
chatton Dec 5, 2022
d2d5ee9
docs: updating based on PR feedback
chatton Dec 6, 2022
271ad1e
docs: addressing PR feedback
chatton Dec 6, 2022
4f7866d
Merge branch 'main' into cian/issue#1851-light-client-guide-client-st…
chatton Dec 6, 2022
c5fb774
docs: adding backtick to ClientState
chatton Dec 7, 2022
1468c65
docs: addressing PR feedback
chatton Dec 8, 2022
8ad270b
docs: fixing grammar in docs
chatton Dec 8, 2022
b650122
Merge branch 'main' into cian/issue#1851-light-client-guide-client-st…
chatton Dec 8, 2022
ff490ed
docs: addressing PR feedback
chatton Dec 8, 2022
e24d68c
Merge branch 'main' into cian/issue#1851-light-client-guide-client-st…
chatton Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion docs/ibc/light-clients/client-state.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,65 @@
<!--
order: 2
-->
-->

# Implementing the ClientState interface
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Implementing the ClientState interface
# Implementing the `ClientState` interface


Learn how to implement the [Client State](https://github.com/cosmos/ibc-go/blob/main/modules/core/exported/client.go#L36) interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Learn how to implement the [Client State](https://github.com/cosmos/ibc-go/blob/main/modules/core/exported/client.go#L36) interface.
Learn how to implement the [`ClientState`](https://github.com/cosmos/ibc-go/blob/main/modules/core/exported/client.go#L36) interface.


### ClientType
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### ClientType
## `ClientType` method


`ClientType` should return a unique string identifier of the light client.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### GetLatestHeight
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### GetLatestHeight
## `GetLatestHeight` method


`GetLatestHeight` should return the latest block height.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### Validate

`Validate` should validate every client state field and should return an error if any value is invalid. The light client
implementor is in charge of determining which checks are required. See the [tendermint light client implementation](https://github.com/cosmos/ibc-go/blob/main/modules/light-clients/07-tendermint/client_state.go#L111)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
as a reference.

### Status

`Status` must return the status of the client. Only `Active` clients are allowed to process packets. All
possible Status types can be found [here](https://github.com/cosmos/ibc-go/blob/main/modules/core/exported/client.go).
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to explain what Frozen and Expired mean

Copy link
Member

Choose a reason for hiding this comment

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

A bulleted list could be nice, giving a short description of each of the statuses.

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 looked around at our usages of the statues, the only one the connection keeper cares about is Active. Frozen is used in our implementation of CheckSubstituteAndUpdateState, however this is an interface method and so will look different in custom light clients. I didn't see any usage of Expired. My take away is that use of Frozen/Expired are for internal implementation of the client, but it won't have an impact in the same way Active does.

Please let me know if my understanding is incorrect!

Copy link
Contributor

Choose a reason for hiding this comment

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

Frozen, Expired were previously required in the CheckSubstituteAndUpdateState logic that has now been removed. They are useful in determining why a client is not active. A client is Frozen if misbehaviour has successfully been submitted against it. Expired clients are those whose latest consensus state update is too old to process a new update given the client state parameters. In the tendermint case, it is specifically in relation to the trusting period, that is, the next update must be before the lastest consensus state time + the trusting period

I think it is still useful to explain these concepts even if core IBC only cares if the status is Active as it'll be useful for UX purposes

Copy link
Contributor

@colin-axner colin-axner Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here's some development context on how this function came to be, see issue #98 and this thread

Reading back, maybe we can mention that this is also used for the Status gRPC route?


### ZeroCustomFields

`ZeroCustomFields` should return a copy of the light client with all client customizable fields with their zero value. It should not mutate the fields of the light client.
This method is used to [verify upgrades](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/types/proposal.go#L120) and when [scheduling upgrades](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/proposal.go#L82).
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### GetTimestampAtHeight

`GetTimestampAtHeight` must return the timestamp for the consensus state associated with the provided height.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### Initialize

Clients must validate the initial consensus state, and may store any client-specific metadata necessary
for correct light client operations in the `Initialize` function.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

`Initialize` gets called when a [client is created](https://github.com/cosmos/ibc-go/blob/main/modules/core/02-client/keeper/client.go#L32).

### VerifyMembership

`VerifyMembership` must verify the existence of a value at a given CommitmentPath at the specified height.
The caller of this function is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized
path (as defined in ICS 24).
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this looks like wording from the spec (I guess it is the godoc string). Maybe we could explain what some of the arguments to this function mean or how they are obtained constructed? We can state that the path provided must be casted to a merkle path (maybe link to example code). We can also explain when this is used (to verify channel ends, packet commits, acknowledgements, etc)

Copy link
Member

Choose a reason for hiding this comment

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

Agree, but I think we've added a page for Membership/Non-Membership proofs here: https://github.com/cosmos/ibc-go/blob/main/docs/.vuepress/config.js#L316

Maybe the specific information could go under that such as CommitmentPath and CommitmentPrefix, explaining what they mean wrt ics24..etc. And then we link to it from here and underneath in VerifyNonMembership section also?

We could then be concise here and just briefly mention that these methods are used for verification of existence and absence proofs of counterparty state using the ClientState created on-chain.

This is just a suggestion, interested to hear what others think regarding how this should be laid out, but as @colin-axner mentioned it will likely grow organically as we progress!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good point @damiannolan, I think it makes sense to link to the proofs page and give the short version here.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me!


### VerifyNonMembership

`VerifyNonMembership` must verify the absence of a given CommitmentPath at a specified height.
The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

### VerifyClientMessage

VerifyClientMessage must verify a ClientMessage. A ClientMessage could be a Header, Misbehaviour, or batch update.
It must handle each type of ClientMessage appropriately. Calls to CheckForMisbehaviour, UpdateState, and UpdateStateOnMisbehaviour
will assume that the content of the ClientMessage has been verified and can be trusted. An error should be returned
if the ClientMessage fails to verify.

### CheckForMisbehaviour

Checks for evidence of a misbehaviour in Header or Misbehaviour type. It assumes the ClientMessage
has already been verified.
Copy link
Member

Choose a reason for hiding this comment

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

Should these be kept under the "Handling Updates" page?

I'm wondering if this section should group VerifyClientMessage, CheckForMisbehaviour, UpdateStateOnMisbehaviour and UpdateState together, and mention that they are used for handling updates.
The "Handling Updates" page could then detail the entrypoint for updating a client via the msg server and 02-client and what the expectation of these methods is for the light client implementation.

cc. @colin-axner

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I agree! The handling updates code also show the code usage which I think would help make each function more clear