renaming to align with x/wasm naming #4091

crodriguezvega · 2023-07-14T13:39:28Z

Description

Opening this one to hear opinions if we should do all this renaming to align with x/wasm naming. This causes a change in the store key and will require some changes in the contracts (test failures are expected).

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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 and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

crodriguezvega · 2023-07-14T13:40:07Z

proto/ibc/lightclients/wasm/v1/genesis.proto

-  // metadata value
-  bytes contract_code = 1;
+  // contract byte code
+  bytes code_bytes = 1;


crodriguezvega · 2023-07-14T13:40:23Z

proto/ibc/lightclients/wasm/v1/genesis.proto

@@ -9,12 +9,12 @@ option go_package = "github.com/cosmos/ibc-go/modules/light-clients/08-wasm/type
 // GenesisState defines 08-wasm's keeper genesis state
 message GenesisState {
  // uploaded light client wasm contracts
-  repeated GenesisContract contracts = 1 [(gogoproto.nullable) = false];
+  repeated Contract contracts = 1 [(gogoproto.nullable) = false];


crodriguezvega · 2023-07-14T13:41:17Z

proto/ibc/lightclients/wasm/v1/tx.proto

 }

 // MsgStoreCodeResponse defines the response type for the StoreCode rpc
 message MsgStoreCodeResponse {
-  bytes code_id = 1;
+  // Checksum is the sha256 hash of the stored code
+  bytes checksum = 1;


ditto on protodoc lowercase nit

crodriguezvega · 2023-07-14T13:41:31Z

proto/ibc/lightclients/wasm/v1/tx.proto

@@ -18,10 +19,12 @@ message MsgStoreCode {
  option (cosmos.msg.v1.signer) = "signer";

  string signer = 1;
-  bytes  code   = 2;
+  // WASMByteCode can be raw or gzip compressed
+  bytes wasm_byte_code = 2 [(gogoproto.customname) = "WASMByteCode"];


nit: I know these replicate the wasmd protos but for our protodocs we usually use all lowercase

crodriguezvega · 2023-07-14T13:42:13Z

proto/ibc/lightclients/wasm/v1/query.proto

 }

 // QueryCodeResponse is the response type for the Query/Code RPC method.
 message QueryCodeResponse {
-  bytes code = 1;
+  bytes data = 1;


crodriguezvega · 2023-07-14T13:43:05Z

proto/ibc/lightclients/wasm/v1/query.proto

  // pagination defines an optional pagination for the request.
  cosmos.base.query.v1beta1.PageResponse pagination = 2;
 }

 // QueryCodeRequest is the request type for the Query/Code RPC method.
 message QueryCodeRequest {
-  string code_id = 1;
+  string code_hash = 1;


Code ID has a different meaning in x/wasm so I thought of renaming to code_hash to avoid confusion between x/wasm and 08-wasm.

how is the meaning different? i thought they both referred to stored contract code?

Sorry, "meaning" is probably not the right word... What I wanted to say is that the term code ID in x/wasm is an integer that indeed is used to refer to the contract (this integer is generated when a contract is stored) and the code hash is the hash of the byte code. So I thought that to avoid confusion between the code ID in x/wasm and the code ID we were using here it was better to rename it to code hash, so that we use the same terms for the same things in both modules.

crodriguezvega · 2023-07-14T13:43:30Z

modules/light-clients/08-wasm/types/keys.go

@@ -12,11 +12,11 @@ const (
 	// StoreKey is the store key string for 08-wasm
 	StoreKey = ModuleName

-	KeyCodeIDPrefix = "codeId"
+	KeyCodeHashPrefix = "codeHash"


Unfortunately this changes the store key prefix...

This will change with #4085 anyways tho right? we won't be storing the wasm code anymore

Yes, but I guess we will have a codeHashes key where we store the list of all code hashes.

damiannolan

Really nice clean up! Big++ on maintaining the same terminology/concepts naming conventions as x/wasm!

I just left some nits, but LGTM!

docs/architecture/adr-027-ibc-wasm.md

damiannolan · 2023-07-15T09:58:47Z

modules/light-clients/08-wasm/types/client_state.go

+	lenCodeHash := len(cs.CodeHash)
+	if lenCodeHash == 0 {
+		return errorsmod.Wrap(ErrInvalidCodeHash, "code hash cannot be empty")
 	}
-	if lenCodeID > 32 { // sha256 output is 256 bits long
-		return errorsmod.Wrapf(ErrInvalidCodeID, "expected 32, got %d", lenCodeID)
+	if lenCodeHash > 32 { // sha256 output is 256 bits long
+		return errorsmod.Wrapf(ErrInvalidCodeHash, "expected 32, got %d", lenCodeHash)


is the output expected to be exactly 256 bits long? why doesn't this check for strict equality?

Is my thinking correct here that this could be replaced with a if len(cs.CodeHash) != 32 { return err } ?

Yes, good catch!

damiannolan · 2023-07-15T10:01:23Z

modules/light-clients/08-wasm/types/errors.go

we might want to revisit these errors, ErrInvalid and ErrInvalidData seem almost overly generic to maintain here?

damiannolan · 2023-07-15T10:04:13Z

modules/light-clients/08-wasm/types/keys.go

@@ -12,11 +12,11 @@ const (
 	// StoreKey is the store key string for 08-wasm
 	StoreKey = ModuleName

-	KeyCodeIDPrefix = "codeId"
+	KeyCodeHashPrefix = "codeHash"


This will change with #4085 anyways tho right? we won't be storing the wasm code anymore

damiannolan · 2023-07-15T10:10:09Z

proto/ibc/lightclients/wasm/v1/tx.proto

@@ -18,10 +19,12 @@ message MsgStoreCode {
  option (cosmos.msg.v1.signer) = "signer";

  string signer = 1;
-  bytes  code   = 2;
+  // WASMByteCode can be raw or gzip compressed
+  bytes wasm_byte_code = 2 [(gogoproto.customname) = "WASMByteCode"];


nit: I know these replicate the wasmd protos but for our protodocs we usually use all lowercase

damiannolan · 2023-07-15T10:10:19Z

proto/ibc/lightclients/wasm/v1/tx.proto

 }

 // MsgStoreCodeResponse defines the response type for the StoreCode rpc
 message MsgStoreCodeResponse {
-  bytes code_id = 1;
+  // Checksum is the sha256 hash of the stored code
+  bytes checksum = 1;


ditto on protodoc lowercase nit

damiannolan · 2023-07-15T10:12:07Z

testing/coordinator.go

Changes in here are flagged for removal in #4048 👍

Co-authored-by: Damian Nolan <[email protected]>

DimitrisJim · 2023-07-20T07:01:03Z

modules/light-clients/08-wasm/types/client_state.go

@@ -47,7 +47,7 @@ func (cs ClientState) Validate() error {
 	if lenCodeHash == 0 {
 		return errorsmod.Wrap(ErrInvalidCodeHash, "code hash cannot be empty")
 	}
-	if lenCodeHash > 32 { // sha256 output is 256 bits long
+	if lenCodeHash != 32 { // sha256 output is 256 bits long


only nit pertains to err message used here, doesn't really give indication that 32 refers to # of bytes.

damiannolan · 2023-07-20T08:41:04Z

proto/ibc/lightclients/wasm/v1/tx.proto

-  // WASMByteCode can be raw or gzip compressed
-  bytes wasm_byte_code = 2 [(gogoproto.customname) = "WASMByteCode"];
+  // wasm byte code of light client contract. It can be raw or gzip compressed
+  bytes wasm_byte_code = 2;


didn't you want to keep the gogoproto custom name?

https://github.com/CosmWasm/wasmd/blob/main/proto/cosmwasm/wasm/v1/tx.proto#L83

Oh, I thought you preferred to have the field name as WasmByteCode. I will merge the PR now, but let me know if you'd like me to change this, and I will do in a follow up.

renaming to align with x/wasm naming

29b916d

crodriguezvega requested review from colin-axner, AdityaSripal, charleenfei, damiannolan, chatton, DimitrisJim and srdtrk as code owners July 14, 2023 13:39

crodriguezvega commented Jul 14, 2023

View reviewed changes

crodriguezvega added the 08-wasm label Jul 14, 2023

fix typo

92eda7b

damiannolan approved these changes Jul 15, 2023

View reviewed changes

Update docs/architecture/adr-027-ibc-wasm.md

01b8a46

Co-authored-by: Damian Nolan <[email protected]>

crodriguezvega mentioned this pull request Jul 15, 2023

Do not store wasm byte code in 08-wasm store #4085

Closed

2 tasks

address review comments

3bf8827

crodriguezvega mentioned this pull request Jul 19, 2023

imp: disallow submessage execution #4130

Merged

9 tasks

DimitrisJim approved these changes Jul 20, 2023

View reviewed changes

damiannolan reviewed Jul 20, 2023

View reviewed changes

crodriguezvega added 2 commits July 20, 2023 20:39

review comment

9aa0489

Merge branch 'feat/wasm-clients' into carlos/align-with-wasm-naming

76358c6

crodriguezvega merged commit 59ccad5 into feat/wasm-clients Jul 20, 2023

crodriguezvega deleted the carlos/align-with-wasm-naming branch July 20, 2023 18:58

faddat mentioned this pull request Sep 10, 2023

deps: update wasm-clients to sdk50 #4616

Closed

9 tasks

crodriguezvega mentioned this pull request Oct 7, 2023

Decide on a convention for naming wasm byte code/byte code hash. #4852

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

renaming to align with x/wasm naming #4091

renaming to align with x/wasm naming #4091

crodriguezvega commented Jul 14, 2023 •

edited

Loading

crodriguezvega Jul 14, 2023

crodriguezvega Jul 14, 2023

crodriguezvega Jul 14, 2023

damiannolan Jul 15, 2023

crodriguezvega Jul 14, 2023

damiannolan Jul 15, 2023

crodriguezvega Jul 14, 2023

crodriguezvega Jul 14, 2023

charleenfei Jul 14, 2023

crodriguezvega Jul 14, 2023

crodriguezvega Jul 14, 2023

damiannolan Jul 15, 2023

crodriguezvega Jul 15, 2023

damiannolan left a comment

damiannolan Jul 15, 2023

crodriguezvega Jul 15, 2023

damiannolan Jul 15, 2023

damiannolan Jul 15, 2023

damiannolan Jul 15, 2023

damiannolan Jul 15, 2023

damiannolan Jul 15, 2023

DimitrisJim Jul 20, 2023

damiannolan Jul 20, 2023

crodriguezvega Jul 20, 2023 •

edited

Loading

renaming to align with x/wasm naming #4091

renaming to align with x/wasm naming #4091

Conversation

crodriguezvega commented Jul 14, 2023 • edited Loading

Description

Commit Message / Changelog Entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

crodriguezvega commented Jul 14, 2023 •

edited

Loading

crodriguezvega Jul 20, 2023 •

edited

Loading