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

test: UpdateStateOnMisbehaviour using mock wasmVM #4919

Conversation

charleenfei
Copy link
Contributor

Description

closes: #4822


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.

@charleenfei charleenfei added testing Testing package and unit/integration tests 08-wasm labels Oct 20, 2023
@charleenfei charleenfei changed the title test test: UpdateStateOnMisbehaviour using mock wasmVM Oct 20, 2023
…stateonmisbehaviour-using-mock-vm-for-08-wasm
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

🙏

modules/light-clients/08-wasm/types/update_test.go Outdated Show resolved Hide resolved
modules/light-clients/08-wasm/types/update_test.go Outdated Show resolved Hide resolved
modules/light-clients/08-wasm/types/update_test.go Outdated Show resolved Hide resolved
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Left some initial comments

@@ -21,6 +21,8 @@ var (

// sudoTypes contains all the possible sudo message types.
sudoTypes = [...]any{UpdateStateMsg{}, UpdateStateOnMisbehaviourMsg{}, VerifyUpgradeAndUpdateStateMsg{}, CheckSubstituteAndUpdateStateMsg{}, VerifyMembershipMsg{}, VerifyNonMembershipMsg{}}

MockClientStateBz = []byte("mockClientStateBz")
Copy link
Member

Choose a reason for hiding this comment

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

I'd be happy to just use a []byte() directly in the tests atm instead of adding a mock var (just in case we lose a bunch of places this could be reused).

Maybe we could add one later and replace all mock []byte() occurrences thereafter, or until the linter cries oceans once again 🥲

Also fine if you wanna use this PR to replace all now, you'd just have to comb through all the tests to see, I definitely added []byte("client-state-data") somewhere in verify membership tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can just open an issue to track this mock var and change this for now

modules/light-clients/08-wasm/types/update_test.go Outdated Show resolved Hide resolved
modules/light-clients/08-wasm/types/update_test.go Outdated Show resolved Hide resolved
modules/light-clients/08-wasm/types/update_test.go Outdated Show resolved Hide resolved
modules/light-clients/08-wasm/types/update_test.go Outdated Show resolved Hide resolved
tc.malleate()

if tc.panicErr == nil {
clientState.UpdateStateOnMisbehaviour(suite.ctx, suite.chainA.App.AppCodec(), suite.store, clientMsg)
Copy link
Contributor

Choose a reason for hiding this comment

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

thing we're going towards removing suite.ctx and suite.store so suite.chainA.GetContext() and suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), endpoint.ClientID)

Copy link
Contributor Author

@charleenfei charleenfei Oct 25, 2023

Choose a reason for hiding this comment

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

should we just make one issue/PR for all these instances? i think there are many in the codebase now.

Copy link
Contributor

Choose a reason for hiding this comment

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

aye there's an issue already, I changed it in my PRs and if any are left they'll be caught by that issue when its time to shine comes.

Comment on lines 683 to 684
suite.SetupWasmWithMockVM()
endpoint := wasmtesting.NewWasmEndpoint(suite.chainA)
Copy link
Contributor

Choose a reason for hiding this comment

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

add a space before space man arrives and gently scolds us.

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

lgtm, left minor thingies

…ng-mock-vm-for-08-wasm' of github.com:cosmos/ibc-go into charly/issue#4822-add-testupdatestateonmisbehaviour-using-mock-vm-for-08-wasm
@charleenfei charleenfei enabled auto-merge (squash) October 25, 2023 15:13
@charleenfei charleenfei merged commit 1b4ae46 into feat/wasm-clients Oct 25, 2023
27 checks passed
@charleenfei charleenfei deleted the charly/issue#4822-add-testupdatestateonmisbehaviour-using-mock-vm-for-08-wasm branch October 25, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
08-wasm testing Testing package and unit/integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants