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

Put ClientState in provableStore and use it in connOpen{Try,Ack} #802

Merged
merged 13 commits into from
Aug 1, 2022

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Jul 26, 2022

Closes: #764

I wasn't sure where to define the validateSelfClient() function. ibc-go puts it in the ICS-2 client keeper, with the Tendermint implementation. However, validateSelfClient() is a function that should be provided by the host, and so belongs in ICS-24 in my opinion. Since ICS-24 doesn't have any per-consensus-protocol sections, I opted for writing the generic declaration of validateSelfClient() in a new "Client state validation" section, and provide the Tendermint implementation as an example. I'm open to better ideas on how to organize this.

I faced another challenge. In ibc-go, validateSelfClient() takes an sdk.Context as parameter and compares the appropriate fields against the fields of the ClientState passed in as argument. The most elegant way I found to abstract this use of the sdk.Context is to require the hosts to implement

type getHostClientState = (height: Height) => ClientState

which asks the host to "represent itself as a ClientState". In the case of the sdk, this would be implemented by building the ClientState from the appropriate fields in the sdk.Context. My only concern with this abstraction is that it might confuse users, as the ClientState is a light client abstraction, not a host chain one. Hopefully, the example provided is enough to clear up any confusion. Again, I'm open to better ways of doing it!

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Nice work! Agreed that ValidateSelfClient makes sense in 24-host

`verifyClientState` verifies a proof of the client state of the specified client stored on the target machine.

```typescript
type verifyClientState = (
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding this function to the specification, we should transition to using the generic verify membership methods. Though I suppose that can be done in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you would like to open an issue describing specifically what's to be done, I'd be happy to take a stab at it in another PR

@AdityaSripal
Copy link
Member

Please add a changelog entry

@plafer
Copy link
Contributor Author

plafer commented Jul 27, 2022

@AdityaSripal Done! 5a4e0c8

@mpoke
Copy link
Contributor

mpoke commented Jul 29, 2022

@AdityaSripal Done! 5a4e0c8

I think he was referring to add an entry to https://github.com/cosmos/ibc/blob/main/CHANGELOG.md

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Nice work. In the future, please open a PR per modified standard. It helps keeping track of changes.

@plafer
Copy link
Contributor Author

plafer commented Jul 29, 2022

In the future, please open a PR per modified standard.

Sounds good!

I think he was referring to add an entry to https://github.com/cosmos/ibc/blob/main/CHANGELOG.md

928a964

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

ClientState is stored in the privateStore in the standard, but is provable in ibc-go
4 participants