Update wasm light client unit tests and tendermint/grandpa cw contract #4306

misko9 · 2023-08-07T22:43:28Z

update grandpa testing, update TestInitializeTendermint(), remove unnecessary test from TestConsensusStateValidateBasic(), change the update state result's height type.

Description

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.

TestInitializeTendermint(), remove unnecessary test from TestConsensusStateValidateBasic(), change the update state result's height type.

DimitrisJim · 2023-08-10T00:33:16Z

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

@@ -120,5 +120,5 @@ type checkForMisbehaviourResult struct {

 type updateStateResult struct {
 	contractResult
-	Heights []exported.Height `json:"heights"`
+	Heights []clienttypes.Height `json:"heights"`


yeah, I think this is right, I guess we could keep it as an interface and define structs for each different response type we could have and define a custom unmarshaller. It's not entirely clear to me if that would be needed though.

Also noting that Status can also be directly set to string instead of exported.Status.

I think better to use the concrete types when serialising json across the vm with std lib.
Not sure how well encoding/json plays with interfaces in general, its not quite like the proto3 json and the interface support with sdk codec

DimitrisJim · 2023-08-10T00:34:31Z

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

 	ibctm "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint"
 	ibctesting "github.com/cosmos/ibc-go/v7/testing"
 )

-func (suite *TypesTestSuite) TestExportGenesisGrandpa() {
+/*func (suite *TypesTestSuite) TestExportGenesisGrandpa() {


Q: We're waiting for the contract update for Grandpa, right? I'm guessing this is a temp commenting out for that eventuality.

yes, we need updates for grandpa contract and hyperspace supporting the new wasm client changes. Then interchaintest support for any hyperspace cli/config changes (I'm pretty sure there are some). And then, finally, run interchaintest using compatible polkadot and parachain images. Once the e2e test is successful, we can harvest the test data for the wasm client's grandpa tests for the tests that require new test data.

TestStatusGrandpa, and TestExportGenesisGrandpa working again

TestUpdateStateGrandpa, TestVerifyUpgradeGrandpa working again

crodriguezvega

Thank you very much for this PR, @misko9. I had a quick look and left just one question for now. Will review more later.

crodriguezvega · 2023-09-04T08:28:46Z

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

@@ -365,7 +384,7 @@ func (suite *TypesTestSuite) TestVerifyMembershipGrandpa() {
 						ConnectionId: connectionID,
 						Prefix:       suite.chainA.GetPrefix(),
 					},
-					DelayPeriod: 0,
+					DelayPeriod: 1000000000,


Curious: why this needed to be changed?

Since the last test data was generated, a requirement was added to Hyperspace for a non-zero delay in seconds. For generating the new test data using the e2e test, I used 1 second, 1_000_000_000, for that input to fulfill that requirement.

Thanks for the explanation, @misko9. I will add a comment in a follow-up PR, so that we don't lose this information.

DimitrisJim · 2023-09-10T03:54:47Z

modules/light-clients/08-wasm/test_data/README

@@ -0,0 +1,59 @@
+How to regenerate grandpa test data


tysm for the README, haven't tried to run these atm but really great idea you decided to add this

+1 thanks for this doc

crodriguezvega

Great work, @misko9. Thanks a lot for regenerating the test data and updating the tests.

I am happy to work on my comments in a follow-up PR. I will also open an issue to track the work for fixing the misbehaviour tests.

crodriguezvega · 2023-09-14T08:13:43Z

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

@@ -173,17 +173,17 @@ func (suite *TypesTestSuite) TestValidate() {
 	}{
 		{
 			name:        "valid client",
-			clientState: types.NewClientState([]byte{0}, []byte{0}, clienttypes.ZeroHeight()),
+			clientState: types.NewClientState([]byte{0}, []byte("01234567012345670123456701234567"), clienttypes.ZeroHeight()),


We can make this magic 01234567012345670123456701234567 string a constant somewhere. I am happy to do it in a follow-up PR to speed up merging this one.

where does this magic string come from?

Would be nice to move away from hardcoded values in testing in the long term, but I think we can also work on that post-release.

This magic string just needs to be any 32 bytes to pass ClientState's Validate() function.

Aha, makes sense, thanks @misko9

crodriguezvega · 2023-09-14T08:19:28Z

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

@@ -365,7 +384,7 @@ func (suite *TypesTestSuite) TestVerifyMembershipGrandpa() {
 						ConnectionId: connectionID,
 						Prefix:       suite.chainA.GetPrefix(),
 					},
-					DelayPeriod: 0,
+					DelayPeriod: 1000000000,


Thanks for the explanation, @misko9. I will add a comment in a follow-up PR, so that we don't lose this information.

damiannolan

Thank you @misko9! Much appreciated.

damiannolan · 2023-09-14T10:08:30Z

modules/light-clients/08-wasm/test_data/README

@@ -0,0 +1,59 @@
+How to regenerate grandpa test data


+1 thanks for this doc

damiannolan · 2023-09-14T10:13:21Z

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

@@ -120,5 +120,5 @@ type checkForMisbehaviourResult struct {

 type updateStateResult struct {
 	contractResult
-	Heights []exported.Height `json:"heights"`
+	Heights []clienttypes.Height `json:"heights"`


I think better to use the concrete types when serialising json across the vm with std lib.
Not sure how well encoding/json plays with interfaces in general, its not quite like the proto3 json and the interface support with sdk codec

damiannolan · 2023-09-14T10:13:44Z

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

Should we create an issue for these commented out tests?

damiannolan · 2023-09-14T10:16:58Z

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

@@ -173,17 +173,17 @@ func (suite *TypesTestSuite) TestValidate() {
 	}{
 		{
 			name:        "valid client",
-			clientState: types.NewClientState([]byte{0}, []byte{0}, clienttypes.ZeroHeight()),
+			clientState: types.NewClientState([]byte{0}, []byte("01234567012345670123456701234567"), clienttypes.ZeroHeight()),


where does this magic string come from?

Would be nice to move away from hardcoded values in testing in the long term, but I think we can also work on that post-release.

Unit tests: temporarily disable grandpa testing, update

607aea9

TestInitializeTendermint(), remove unnecessary test from TestConsensusStateValidateBasic(), change the update state result's height type.

DimitrisJim added the 08-wasm label Aug 10, 2023

DimitrisJim reviewed Aug 10, 2023

View reviewed changes

misko9 added 5 commits August 30, 2023 16:03

TestVerifyMembershipGrandpa working again

c34fd15

TestVerifyNonMembershipGrandpa, TestInitializeGrandpa,

86dcf72

TestStatusGrandpa, and TestExportGenesisGrandpa working again

TestCheckSubstituteAndUpdateStateGrandpa, TestVerifyHeaderGrandpa,

81abf1e

TestUpdateStateGrandpa, TestVerifyUpgradeGrandpa working again

Run linter

33fa18b

Add instructions for re-creating grandpa test data

25548ba

misko9 marked this pull request as ready for review August 30, 2023 23:51

misko9 requested review from colin-axner, AdityaSripal, damiannolan, crodriguezvega, charleenfei and chatton as code owners August 30, 2023 23:51

Add space after comment delimiter

01757b9

misko9 changed the title ~~Update wasm light client unit tests and tendermint cw contract~~ Update wasm light client unit tests and tendermint/grandpa cw contract Sep 1, 2023

crodriguezvega self-assigned this Sep 4, 2023

crodriguezvega reviewed Sep 4, 2023

View reviewed changes

crodriguezvega added this to the 08-wasm/v1.0.0 milestone Sep 4, 2023

DimitrisJim mentioned this pull request Sep 7, 2023

Duplicate simapp in 08 wasm #4532

Merged

9 tasks

DimitrisJim reviewed Sep 10, 2023

View reviewed changes

crodriguezvega mentioned this pull request Sep 10, 2023

chore: fix wasm client tests #4613

Closed

9 tasks

faddat mentioned this pull request Sep 11, 2023

update wasmVM to v1.4.0 #4603

Closed

9 tasks

crodriguezvega approved these changes Sep 14, 2023

View reviewed changes

damiannolan approved these changes Sep 14, 2023

View reviewed changes

misko9 and others added 2 commits September 14, 2023 17:49

Updated instructions for regenerating test data

aaaab5a

Merge branch 'feat/wasm-clients' into steve/feat/wasm-clients

4f1bfa3

crodriguezvega merged commit 1bc5845 into cosmos:feat/wasm-clients Sep 15, 2023
20 checks passed

crodriguezvega mentioned this pull request Sep 15, 2023

Grandpa misbehaviour tests need to be fixed #4672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wasm light client unit tests and tendermint/grandpa cw contract #4306

Update wasm light client unit tests and tendermint/grandpa cw contract #4306

misko9 commented Aug 7, 2023 •

edited

Loading

DimitrisJim Aug 10, 2023

damiannolan Sep 14, 2023

DimitrisJim Aug 10, 2023

misko9 Aug 18, 2023

crodriguezvega left a comment

crodriguezvega Sep 4, 2023

misko9 Sep 9, 2023

crodriguezvega Sep 14, 2023

DimitrisJim Sep 10, 2023

damiannolan Sep 14, 2023

crodriguezvega left a comment

crodriguezvega Sep 14, 2023

damiannolan Sep 14, 2023

misko9 Sep 14, 2023

damiannolan Sep 18, 2023

crodriguezvega Sep 14, 2023

damiannolan left a comment

damiannolan Sep 14, 2023

damiannolan Sep 14, 2023

damiannolan Sep 14, 2023

damiannolan Sep 14, 2023

Update wasm light client unit tests and tendermint/grandpa cw contract #4306

Update wasm light client unit tests and tendermint/grandpa cw contract #4306

Conversation

misko9 commented Aug 7, 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

crodriguezvega 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

crodriguezvega 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

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

misko9 commented Aug 7, 2023 •

edited

Loading