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.

Sign up for GitHub

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

Jump to bottom

chore: 07-tendermint set client/consensus states in UpdateState #1199

Merged

damiannolan merged 5 commits into 02-client-refactor from damian/1190-tm-set-client-and-consensus-states

Mar 30, 2022

Member

damiannolan commented Mar 30, 2022

Description

Set ClientState and ConsensusState in store within 07-tendermint UpdateState
Updating tests to assert against stored values in favour of return values

closes: #1190

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.
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
Review Codecov Report in the comment section below once CI passes


          adding storage ops to 07-tendermint UpdateState, updating tests

81eee38

damiannolan requested review from colin-axner, fedekunze and AdityaSripal as code owners

March 30, 2022 09:23

damiannolan requested review from charleenfei, crodriguezvega and seantking

March 30, 2022 09:24

damiannolan commented

View reviewed changes

modules/light-clients/07-tendermint/types/store.go

    
              // SetConsensusState stores the consensus state at the given height.

              func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, consensusState *ConsensusState, height exported.Height) {

              // setConsensusState stores the consensus state at the given height.

              func setConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, consensusState *ConsensusState, height exported.Height) {

Member Author

damiannolan Mar 30, 2022

Updated to make private / unexported

damiannolan commented

View reviewed changes

modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved

damiannolan commented

View reviewed changes

modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved

colin-axner reviewed

View reviewed changes

Contributor

colin-axner left a comment

Excellent! Left some suggestions on improving the testing changes

modules/light-clients/07-tendermint/types/update_test.go Outdated

Comment on lines 435 to 437

		bz := clientStore.Get(host.ClientStateKey())
		updatedClientState := clienttypes.MustUnmarshalClientState(suite.chainA.App.AppCodec(), bz)

Contributor

colin-axner Mar 30, 2022

should this go in malleate? I'm not sure these checks perform any actual checks now since GetClientState() performs the same code

Member Author

damiannolan Mar 30, 2022

Right, yeah I get you! GetClientState is essentially just getting from the store and unmarshalling as well. So this is basically asserting true is true.

Could you expand on what you're suggesting for moving to malleate? I'm not sure I follow, malleate will be called before UpdateState right, so the client state retrieved is not yet updated. Are you suggesting to retrieve the not yet updated state in malleate and then in expResult() assert that they are not equal?

Contributor

colin-axner Mar 30, 2022

Are you suggesting to retrieve the not yet updated state in malleate and then in expResult() assert that they are not equal?

These test cases are asserting the client state didn't get updated (since we updated to a previous height). So yes, I am suggesting we construct the expected client state in malleate and then ensure the client state doesn't change after calling UpdateState

Member Author

damiannolan Mar 30, 2022

Ah yes, sorry overlooked that when I wrote the comment. I've updated as you suggested to retrieve the "previous" states before update in malleate and check for equality in expResult! :))

modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved

modules/light-clients/07-tendermint/types/update_test.go Outdated

Comment on lines 453 to 457

+              				bz := clientStore.Get(host.ClientStateKey())
+              				updatedClientState := clienttypes.MustUnmarshalClientState(suite.chainA.App.AppCodec(), bz)
+              				bz = clientStore.Get(host.ConsensusStateKey(clientMessage.GetHeight()))
+              				updatedConsensusState := clienttypes.MustUnmarshalConsensusState(suite.chainA.App.AppCodec(), bz)

Contributor

colin-axner Mar 30, 2022

ditto on moving into malleate

Member Author

damiannolan Mar 30, 2022

Done 👍

modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved


          use clientMessage.GetHeight() as per review suggestions

f98617f

colin-axner approved these changes

View reviewed changes

Contributor

colin-axner left a comment

ACK pending these testing improvements. Wonderful that things are moving forward!

damiannolan added 2 commits

March 30, 2022 14:39


          updating tests to obtain previous client/consensus state in malleate

45bf66a


          Merge branch '02-client-refactor' into damian/1190-tm-set-client-and-…

f601fe8

…consensus-states

seantking approved these changes

View reviewed changes


          Merge branch '02-client-refactor' into damian/1190-tm-set-client-and-…

4212e66

…consensus-states

damiannolan enabled auto-merge (squash)

March 30, 2022 13:13

damiannolan merged commit 2e2bfab into 02-client-refactor

damiannolan deleted the damian/1190-tm-set-client-and-consensus-states branch

March 30, 2022 13:21

seunlanlege pushed a commit to ComposableFi/ibc-go that referenced this pull request


          chore: 07-tendermint set client/consensus states in UpdateState (cosm…

48b505f

…os#1199)

* adding storage ops to 07-tendermint UpdateState, updating tests

* use clientMessage.GetHeight() as per review suggestions

* updating tests to obtain previous client/consensus state in malleate

CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request


          feat: init mdbooks for specs site with place holder files (cosmos#1199)

667cec7

<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->

This PR adds
[mdbook](https://rust-lang.github.io/mdBook/guide/installation.html) for
hosting the specs site. A workflow was added to deploy the specs site to
github pages.

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [ ] Linked issues closed with keywords

---------

Co-authored-by: nashqueue <99758629+nashqueue@users.noreply.github.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

seantking seantking approved these changes

colin-axner colin-axner approved these changes

fedekunze Awaiting requested review from fedekunze

AdityaSripal Awaiting requested review from AdityaSripal

charleenfei Awaiting requested review from charleenfei

crodriguezvega Awaiting requested review from crodriguezvega

Labels

None yet