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

feat: Port x/wasm module #92

Closed
wants to merge 3 commits into from
Closed

feat: Port x/wasm module #92

wants to merge 3 commits into from

Conversation

aleem1314
Copy link

Ref: #25

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

return err
}

endpoint.ClientID, err = ibctesting.ParseClientIDFromEvents(res.GetEvents())

Check warning

Code scanning / Semgrep

Should `$X` be modified when an error could be returned?

Should `endpoint` be modified when an error could be returned?
return err
}

endpoint.ConnectionID, err = ibctesting.ParseConnectionIDFromEvents(res.GetEvents())

Check warning

Code scanning / Semgrep

Should `$X` be modified when an error could be returned?

Should `endpoint` be modified when an error could be returned?
}

if endpoint.ConnectionID == "" {
endpoint.ConnectionID, err = ibctesting.ParseConnectionIDFromEvents(res.GetEvents())

Check warning

Code scanning / Semgrep

Should `$X` be modified when an error could be returned?

Should `endpoint` be modified when an error could be returned?
return err
}

endpoint.ChannelID, err = ibctesting.ParseChannelIDFromEvents(res.GetEvents())

Check warning

Code scanning / Semgrep

Should `$X` be modified when an error could be returned?

Should `endpoint` be modified when an error could be returned?
}

if endpoint.ChannelID == "" {
endpoint.ChannelID, err = ibctesting.ParseChannelIDFromEvents(res.GetEvents())

Check warning

Code scanning / Semgrep

Should `$X` be modified when an error could be returned?

Should `endpoint` be modified when an error could be returned?
Comment on lines +813 to +816
proposalTitle, err := cmd.Flags().GetString(cli.FlagTitle)
if err != nil {
return clientCtx, proposalTitle, "", nil, err
}

Check warning

Code scanning / Semgrep

Variable `$X` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference

Variable `proposalTitle` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference
Comment on lines +818 to +821
proposalDescr, err := cmd.Flags().GetString(cli.FlagDescription)
if err != nil {
return client.Context{}, proposalTitle, proposalDescr, nil, err
}

Check warning

Code scanning / Semgrep

Variable `$X` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference

Variable `proposalDescr` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference
Comment on lines +828 to +831
deposit, err := sdk.ParseCoinsNormalized(depositArg)
if err != nil {
return client.Context{}, proposalTitle, proposalDescr, deposit, err
}

Check warning

Code scanning / Semgrep

Variable `$X` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference

Variable `deposit` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference
Comment on lines +157 to +160
checksum, err = k.wasmVM.Create(wasmCode)
if err != nil {
return 0, checksum, sdkerrors.Wrap(types.ErrCreateFailed, err.Error())
}

Check warning

Code scanning / Semgrep

Variable `$X` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference

Variable `checksum` is likely modified and later used on error. In some cases this could result in panics due to a nil dereference
Comment on lines +72 to +74
for _, chain := range coord.Chains {
coord.UpdateTimeForChain(chain)
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
if err != nil {
return nil, err
}
if rsp == nil || reflect.ValueOf(rsp).IsNil() {

Check warning

Code scanning / CodeQL

Impossible interface nil check

This value can never be nil, since it is a wrapped interface value.
"encoding/hex"
"fmt"
"math"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

import (
"encoding/json"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism

import (
"fmt"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism
Copy link
Contributor

@i-norden i-norden left a comment

Choose a reason for hiding this comment

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

Thanks @aleem1314! I am wondering if we can get away with using WASM tooling as dependencies rather than internalizing a stateful module for them.

Background here is relevant here too. WASM execution environment is another piece necessary to fulfill the goal of "On-chain watcher fraud proof evaluation", it is this environment that we will need to provide access to the Postgres IPLD blockstore through the package discussed there.

Tagging @AFDudley here to discuss too.

Based on my current understanding the WASM execution environment we will use in laconicd will not need to write state into the Tendermint/Cosmos state commitment (IAVL). Service providers will usually run watcher WASM "off-chain", in watcher groups, and only when someone submits a fraud proof to Laconic does the laconicd state machine execute WASM to evaluate the fraud proof by re-running the challenged watcher state transitions to see if the result matches the result provided. But, are the state changes affected during this evaluation committed back to Tendermint? Or are they executed in a sort of speculative manner and the only thing committed is a signed attestation to the result of the evaluation?

@AFDudley
Copy link
Contributor

AFDudley commented Mar 3, 2023

I think the result needs to be written to the chain, but we will need to use IPLD and probably make some other changes so that it's efficient to store.

@aleem1314
Copy link
Author

Based on my current understanding the WASM execution environment we will use in laconicd will not need to write state into the Tendermint/Cosmos state commitment (IAVL).

The CosmWasmVM Instantiate and Execute ref methods requires access to the KVStore. If we keep wasm not as a stateful module, which module store we are going to use in the wasm?

@telackey
Copy link
Contributor

@telackey telackey closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants