ICS8: Update the WASM Client spec #901
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Blas Rodriguez Irizar <[email protected]>
Signed-off-by: Eduardo Díaz <[email protected]>
blasrodri
commented
Mar 17, 2023
Signed-off-by: Eduardo Díaz <[email protected]>
add a basic typescript impl
blasrodri
requested review from
mpoke,
AdityaSripal,
cwgoes,
colin-axner and
angbrav
as code owners
Update the spec with the implementation
Signed-off-by: Carlos Rodriguez <[email protected]>
updates to ICS 08
Co-authored-by: Blas Rodriguez Irizar <[email protected]> Signed-off-by: Carlos Rodriguez <[email protected]>
ics 08: updates after recent refactoring
aeryz
reviewed
Sep 8, 2023
| clientStore = provableStore.prefixStore("clients/{clientMsg.identifier}") | ||
|
|
||
| // use underlying wasm contract to verify client message | ||
| assert(callContract(clientStore, clientState, marshalJSON(payload))) |
There was a problem hiding this comment.
NIT: Since the read-only operations are now changed to be queries, I would use queryContract when a read-only query is made to the underlying contract and callContract when an execute/sudo entrypoint is called.
There was a problem hiding this comment.
Thanks for the feedback. Yes, I agree; I will change that.
| } | ||
|
|
||
| // retrieve client identifier-prefixed store | ||
| clientStore = provableStore.prefixStore("clients/{clientIdentifier}") |
There was a problem hiding this comment.
How should it be?
There was a problem hiding this comment.
the assignment to clientStore appears to be indented? (maybe this is githubs UI being all weird)
Co-authored-by: Aditya <[email protected]>
AdityaSripal
approved these changes
Sep 20, 2023
DimitrisJim
reviewed
Sep 20, 2023
|
|
||
| ### Gas costs | ||
|
|
||
| []`wasmd`](https://github.com/CosmWasm/wasmd) has thoroughly benchmarked [gas adjustments for CosmWasm](https://github.com/CosmWasm/wasmd/blob/v0.41.0/x/wasm/keeper/gas_register.go#L13-L56) and the same values are being applied in the Wasm VM used in ibc-go's implementation of ICS 8. |
There was a problem hiding this comment.
Suggested change
| []`wasmd`](https://github.com/CosmWasm/wasmd) has thoroughly benchmarked [gas adjustments for CosmWasm](https://github.com/CosmWasm/wasmd/blob/v0.41.0/x/wasm/keeper/gas_register.go#L13-L56) and the same values are being applied in the Wasm VM used in ibc-go's implementation of ICS 8. | |
| [`wasmd`](https://github.com/CosmWasm/wasmd) has thoroughly benchmarked [gas adjustments for CosmWasm](https://github.com/CosmWasm/wasmd/blob/v0.41.0/x/wasm/keeper/gas_register.go#L13-L56) and the same values are being applied in the Wasm VM used in ibc-go's implementation of ICS 8. |
| Comparison between heights is implemented as follows: | ||
| ### Headers | ||
|
|
||
| Contents of Wasm client headers depend upon Wasm contract. Binary data represented by the `data` field is opaque and only interpreted by the Wasm contract, and will consist either of a valid header or of two conflicting headers, both of which the Wasm contract would have considered valid. In the latter case, the contract will update the consensus state with the valid header; in the former case, the light client may detect misbehaviour and freeze the client (thus preventing further packet flow). |
There was a problem hiding this comment.
appear to be opposite? 😄
Suggested change
| Contents of Wasm client headers depend upon Wasm contract. Binary data represented by the `data` field is opaque and only interpreted by the Wasm contract, and will consist either of a valid header or of two conflicting headers, both of which the Wasm contract would have considered valid. In the latter case, the contract will update the consensus state with the valid header; in the former case, the light client may detect misbehaviour and freeze the client (thus preventing further packet flow). | |
| Contents of Wasm client headers depend upon Wasm contract. Binary data represented by the `data` field is opaque and only interpreted by the Wasm contract, and will consist either of a valid header or of two conflicting headers, both of which the Wasm contract would have considered valid. In the former case, the contract will update the consensus state with the valid header; in the latter case, the light client may detect misbehaviour and freeze the client (thus preventing further packet flow). |
| } | ||
|
|
||
| // retrieve client identifier-prefixed store | ||
| clientStore = provableStore.prefixStore("clients/{clientIdentifier}") |
There was a problem hiding this comment.
the assignment to clientStore appears to be indented? (maybe this is githubs UI being all weird)
| assert(codeHandle.verifyNextSequenceRecv(clientState, height, prefix, proof, portIdentifier, channelIdentifier, nextSequenceRecv)) | ||
| #### Contract instantiation | ||
|
|
||
| Instantiation of a contract is minimal. No data is passed in the message for the contract call, but the |
There was a problem hiding this comment.
consensus & client state is also passed in?
Comment on lines
+522
to
+523
| pub is_valid: bool, // Execute was successful | ||
| pub error_msg: String, // Error message |
There was a problem hiding this comment.
this was brought up in slack discussion at some point, I remember both is_valid and error_msg being unused in contracts. Either way, can be left for another PR after we actually remove em from code.
crodriguezvega
approved these changes
Nov 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.