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

ICS 28: WASM Client #506

Closed
wants to merge 302 commits into from
Closed

ICS 28: WASM Client #506

wants to merge 302 commits into from

Conversation

ParthDesai
Copy link
Contributor

No description provided.

cwgoes and others added 30 commits June 29, 2019 14:27
* Fix 24/getConsensusState

Under `getConsensusState`,

> `getConsensusState` MUST return the consensus state for the consensus algorithm of the host chain at the specified height, for all heights greater than zero and less than or equal to the current height.

For some cases `getConsensusState` will not be able to return the consensus state with height of `0 < _ < current`. For efficiency, the chains might choose to prune the states and (if the pruning happens for past enough states) the impl will still work, so the restriction should be RECOMMENDED.

* Update README.md
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice proposal. Given my focus, I would love to see more detail on the actual interface expected of the wasm verifier bytecode.

height: Height,
proof: CommitmentPrefix) {
codeHandle = clientState.codeHandle()
codeHandle.verifyNewClientState(oldClientState, newClientState, height)
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally like this proposal and level of detail.

However, I am trying to visualize what the full API of the contract is, and it is rather hard to do with the codeHandle.xxx() calls scattered around. Could you add a section listing the API with argument types of the wasm verifier? Also whether it is a pure function or whether we assume it can read local state / query chain state.

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 have added clarification on how code handle will be implemented, as well as WASM interface that need to be supported by every WASM based light client contract.

It should be pure function, there should not be any need to read local state/query chain state.

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! See my comment on the corresponding ADR - cosmos/cosmos-sdk#8035 (review) - this security model should be detailed in this spec document explicitly and followed by the implementation.

@cwgoes
Copy link
Contributor

cwgoes commented Mar 12, 2021

Can you move this to https://github.com/cosmos/ibc ?

@hxrts
Copy link

hxrts commented Apr 29, 2021

Should this still be open?

@cwgoes
Copy link
Contributor

cwgoes commented Apr 30, 2021

Yes, it can be reopened now, sorry for all the confusion. Alas I do not seem to be able to do this, but feel free to.

@hxrts
Copy link

hxrts commented Apr 30, 2021

I can't re-open, maybe @milosevic or @AdityaSripal ?

@AdityaSripal
Copy link
Member

Can't reopen because the branch has no history in common with master. Maybe Parth can manually open a new PR? Sorry for the inconvenience

@ParthDesai
Copy link
Contributor Author

ParthDesai commented Apr 30, 2021 via email

@ParthDesai ParthDesai mentioned this pull request May 7, 2021
@ParthDesai
Copy link
Contributor Author

ParthDesai commented May 7, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.