imp: use bytes in wasm contract api instead of wrapped types by crodriguezvega · Pull Request #5154 · cosmos/ibc-go

crodriguezvega · 2023-11-21T22:52:34Z

Description

Changes for:

Contract changes here.

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.

…(wip)

crodriguezvega · 2023-11-21T22:53:30Z

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

-	ConsensusState *ConsensusState `json:"consensus_state"`
+	ClientState    []byte `json:"client_state"`
+	ConsensusState []byte `json:"consensus_state"`
+	Checksum       []byte `json:"checksum"`


I left the checksum here for now.

What about latest height. Also, I don't see why contracts would need their own checksum. I think we should remove this.

I didn't add it because the contract takes the latest height from the underlying client state (the bytes that we are passing here) and uses that to set the latest height of the 08-wasm wrapping type.

Happy to remove the checksum if you do me the favour to figure out how the contract can calculate that itself. I don't think I will have the time to investigate that. 🙏

Happy to keep. Contracts can only get this through a querier which we are passing nil. I think this was the only way to get it for standard contracts. I still don't see the reason why the contract should be aware of its own codehash.

Also it can get its own code hash during other executions from the clientStore right? Eitherway. Happy to keep.

I think we should assume that the contract cannot get it's own codehash in instantiate if we remove this since we are not passing in a querier. But it can get it during other executions. I think the contract does not need it's own codeHash in instantiate and we could remove this. I'm in favour of removal but happy to hear what others think

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

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

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

srdtrk · 2023-11-22T09:17:33Z

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

-	ConsensusState *ConsensusState `json:"consensus_state"`
+	ClientState    []byte `json:"client_state"`
+	ConsensusState []byte `json:"consensus_state"`
+	Checksum       []byte `json:"checksum"`


What about latest height. Also, I don't see why contracts would need their own checksum. I think we should remove this.

…haviourMsg to use bytes

crodriguezvega · 2023-11-24T09:34:37Z

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

 			clientState := endpoint.GetClientState()

 			clientMsg = &types.ClientMessage{
-				Data: []byte{1},


I could have left this as it was, but I like using tests as some sort of documentation as well, and thus being explicit that we expect a header here it's more helpful for the reader, I think.

modules/light-clients/08-wasm/testing/values.go

srdtrk · 2023-11-24T10:28:20Z

modules/light-clients/08-wasm/testing/wasm_endpoint.go


-	clientState := types.NewClientState(contractClientState, checksum, clienttypes.NewHeight(0, 1))
-	consensusState := types.NewConsensusState(contractConsensusState)
+	wrappedClientStateBz := clienttypes.MustMarshalClientState(endpoint.Chain.App.AppCodec(), CreateMockWrappedClientState(clienttypes.NewHeight(1, 5)))


Suggested change

wrappedClientStateBz := clienttypes.MustMarshalClientState(endpoint.Chain.App.AppCodec(), CreateMockWrappedClientState(clienttypes.NewHeight(1, 5)))

wrappedClientStateBz := clienttypes.MustMarshalClientState(endpoint.Chain.App.AppCodec(), MockWrappedClientState)

Would this work?

I wanted to use a different mock client state with a different height than MockWrappedClientState because there are some tests where the client state is updated with MockWrappedClientState and wanted to verify that the height actually changes.

damiannolan · 2023-11-27T16:18:07Z

CI is now green 👍🏻

I didn't modify the migrate contract files migrate_success.wasm.gz and migrate_error.wasm.gz but the tests are now passing. Is this expected @srdtrk?

crodriguezvega · 2023-11-27T18:31:23Z

I didn't modify the migrate contract files migrate_success.wasm.gz and migrate_error.wasm.gz but the tests are now passing. Is this expected @srdtrk?

Yes, that is expected, since we didn't change anything in the migration flow.

Thanks for getting CI green, @damiannolan @chatton! 🍾

crodriguezvega · 2023-11-29T10:32:38Z

Note for myself: I need to update the docs.

damiannolan

LGTM, thanks everyone who contributed to this PR 🙏🏻

damiannolan

ACK docs, thanks @crodriguezvega 🙏🏻

I left one suggestion for noting the base64 encoded byte strings

docs/docs/03-light-clients/04-wasm/07-contracts.md

Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit f2cc21c) # Conflicts: # docs/docs/03-light-clients/04-wasm/07-contracts.md # e2e/tests/wasm/contracts/ics10_grandpa_cw_expiry.wasm.gz # e2e/tests/wasm/grandpa_test.go # modules/light-clients/08-wasm/testing/values.go # modules/light-clients/08-wasm/testing/wasm_endpoint.go # modules/light-clients/08-wasm/types/client_state_test.go # modules/light-clients/08-wasm/types/misbehaviour_handle_test.go # modules/light-clients/08-wasm/types/proposal_handle_test.go # modules/light-clients/08-wasm/types/update_test.go # modules/light-clients/08-wasm/types/upgrade_test.go

Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit f2cc21c) # Conflicts: # e2e/tests/wasm/contracts/ics10_grandpa_cw_expiry.wasm.gz # e2e/tests/wasm/grandpa_test.go # modules/light-clients/08-wasm/testing/wasm_endpoint.go # modules/light-clients/08-wasm/types/client_state_test.go # modules/light-clients/08-wasm/types/update_test.go

…#5154) (#5275) * imp: use bytes in wasm contract api instead of wrapped types (#5154) Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit f2cc21c) # Conflicts: # docs/docs/03-light-clients/04-wasm/07-contracts.md # e2e/tests/wasm/contracts/ics10_grandpa_cw_expiry.wasm.gz # e2e/tests/wasm/grandpa_test.go # modules/light-clients/08-wasm/testing/values.go # modules/light-clients/08-wasm/testing/wasm_endpoint.go # modules/light-clients/08-wasm/types/client_state_test.go # modules/light-clients/08-wasm/types/misbehaviour_handle_test.go # modules/light-clients/08-wasm/types/proposal_handle_test.go # modules/light-clients/08-wasm/types/update_test.go # modules/light-clients/08-wasm/types/upgrade_test.go * fix conflicts * delete e2e * delete docs file --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

…#5154) (#5276) * imp: use bytes in wasm contract api instead of wrapped types (#5154) Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit f2cc21c) # Conflicts: # e2e/tests/wasm/contracts/ics10_grandpa_cw_expiry.wasm.gz # e2e/tests/wasm/grandpa_test.go # modules/light-clients/08-wasm/testing/wasm_endpoint.go # modules/light-clients/08-wasm/types/client_state_test.go # modules/light-clients/08-wasm/types/update_test.go * fix conflicts * delete e2e --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

refactor: use bytes in instantiate message and verify client message …

60f8166

…(wip)

github-actions bot added the 08-wasm label Nov 21, 2023

crodriguezvega commented Nov 21, 2023

View reviewed changes

modules/light-clients/08-wasm/types/consensus_state.go Outdated Show resolved Hide resolved

style: fixed linter issues

0db7a22

srdtrk reviewed Nov 22, 2023

View reviewed changes

srdtrk mentioned this pull request Nov 22, 2023

nit(08-wasm): removed unused param from 'NewConsensusState' #5159

Merged

9 tasks

crodriguezvega added 2 commits November 22, 2023 20:03

Merge branch 'main' into serdar/carlos/contract-api-bytes

1b70f2a

change CheckForMisbehaviourMsg, UpdateStateMsg and UpdateStateOnMisbe…

6d7dee5

…haviourMsg to use bytes

github-actions bot added core 02-client labels Nov 22, 2023

remove unused function

3069577

github-actions bot removed core 02-client labels Nov 22, 2023

Merge branch 'main' into serdar/carlos/contract-api-bytes

fc0687e

crodriguezvega mentioned this pull request Nov 22, 2023

refactor messages to use bytes misko9/centauri#7

Merged

crodriguezvega changed the title ~~imp: use bytes in wasm contract api instead of wrapped types (WiP)~~ imp: use bytes in wasm contract api instead of wrapped types Nov 24, 2023

crodriguezvega commented Nov 24, 2023

View reviewed changes

srdtrk reviewed Nov 24, 2023

View reviewed changes

crodriguezvega marked this pull request as ready for review November 27, 2023 10:08

crodriguezvega requested review from AdityaSripal, charleenfei, chatton and damiannolan as code owners November 27, 2023 10:08

chore: recompile contracts from PR and push for CI runs

7062231

github-actions bot added the e2e label Nov 27, 2023

damiannolan assigned damiannolan and chatton Nov 27, 2023

damiannolan added 2 commits November 27, 2023 14:53

chore: check-in new contract for happy path transfer

fa3e116

chore: recompile contract for e2e recover client test

5ebdd4f

Merge branch 'main' into serdar/carlos/contract-api-bytes

f628c0e

crodriguezvega added the priority PRs that need prompt reviews label Nov 28, 2023

damiannolan approved these changes Nov 29, 2023

View reviewed changes

damiannolan added backport-wasm-v0.1.x+ibc-go-v7.3.x-wasmvm-v1.4.x labels Nov 29, 2023

crodriguezvega added 2 commits November 29, 2023 22:42

Merge branch 'main' into serdar/carlos/contract-api-bytes

9a0b880

update docs

404b303

github-actions bot added the docs Improvements or additions to documentation label Nov 29, 2023

fix contract file extension

c7f143e

damiannolan approved these changes Nov 30, 2023

View reviewed changes

docs/docs/03-light-clients/04-wasm/07-contracts.md Outdated Show resolved Hide resolved

DimitrisJim approved these changes Nov 30, 2023

View reviewed changes

crodriguezvega added 2 commits November 30, 2023 12:32

review comment

c24c541

rename wrapped -> tendermint

ace5a04

crodriguezvega merged commit f2cc21c into main Nov 30, 2023

crodriguezvega deleted the serdar/carlos/contract-api-bytes branch November 30, 2023 13:04

This was referenced Nov 30, 2023

imp: use bytes in wasm contract api instead of wrapped types (backport #5154) #5275

Merged

imp: use bytes in wasm contract api instead of wrapped types (backport #5154) #5276

Merged

damiannolan added a commit that referenced this pull request Dec 5, 2023

imp: use bytes in wasm contract api instead of wrapped types (#5154)

afd5b52

Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com>

	wrappedClientStateBz := clienttypes.MustMarshalClientState(endpoint.Chain.App.AppCodec(), CreateMockWrappedClientState(clienttypes.NewHeight(1, 5)))
	wrappedClientStateBz := clienttypes.MustMarshalClientState(endpoint.Chain.App.AppCodec(), MockWrappedClientState)

Conversation

crodriguezvega commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Commit Message / Changelog Entry

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damiannolan commented Nov 27, 2023

Uh oh!

crodriguezvega commented Nov 27, 2023

Uh oh!

crodriguezvega commented Nov 29, 2023

Uh oh!

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

damiannolan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

crodriguezvega commented Nov 21, 2023 •

edited

Loading