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

Move check for upgraded client state height to 02-client layer. #4368

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

DimitrisJim
Copy link
Contributor

@DimitrisJim DimitrisJim commented Aug 16, 2023

Description

closes: #4212, removing this check in the 08-wasm module is additionally required. (Removed check in 08-wasm module)

Commit Message / Changelog Entry

chore: move check for upgraded client state height in 02-client layer.

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.

@DimitrisJim DimitrisJim force-pushed the jim/4212-move-upgraded-client-check-to-02-client branch from c153b23 to 4841ad2 Compare August 16, 2023 21:31
err = suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedConsStateBz)
suite.Require().NoError(err)

// change upgradedClient height to be lower than current client state height
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that I added a test case here but additionally left the others present in 07-tendermint. (which also need tweaking since they fall victim to the whole not checking error returned and thereby not always checking what they are supposed to)

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2023

Codecov Report

Merging #4368 (5620977) into main (3b1e4e2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4368   +/-   ##
=======================================
  Coverage   79.57%   79.57%           
=======================================
  Files         189      189           
  Lines       13261    13266    +5     
=======================================
+ Hits        10552    10557    +5     
  Misses       2288     2288           
  Partials      421      421           
Files Coverage Δ
modules/core/02-client/keeper/client.go 94.30% <100.00%> (+0.22%) ⬆️
modules/light-clients/07-tendermint/upgrade.go 73.56% <100.00%> (-0.31%) ⬇️

@crodriguezvega
Copy link
Contributor

Thanks for picking this up, @DimitrisJim. Since the check needs to be removed in both 07-tendermint and 08-wasm, I think it would be best to wait until we merge 08-wasm to main and then we can fully address the whole issue in a single PR. What do you think?

@DimitrisJim
Copy link
Contributor Author

I think it would be best to wait until we merge 08-wasm to main and then we can fully address the whole issue in a single PR. What do you think?

sounds perfectly fine to me, we can leave as is and I'll just rebase when the time is right

@DimitrisJim DimitrisJim marked this pull request as draft August 17, 2023 10:22
@DimitrisJim DimitrisJim force-pushed the jim/4212-move-upgraded-client-check-to-02-client branch from 4841ad2 to a4afd70 Compare October 24, 2023 08:05
@DimitrisJim DimitrisJim force-pushed the jim/4212-move-upgraded-client-check-to-02-client branch 2 times, most recently from 37a08d5 to 4a6d6b4 Compare November 1, 2023 08:05
@DimitrisJim DimitrisJim force-pushed the jim/4212-move-upgraded-client-check-to-02-client branch from 4a6d6b4 to b8b1a90 Compare November 15, 2023 15:38
@DimitrisJim DimitrisJim marked this pull request as ready for review November 15, 2023 15:38
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.

Looks fine to me. Happy that we can finally merge this one! :)

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM thanks @DimitrisJim

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

LGTM, the 02-client tests should really be refactored some day, seems like some rearrangement could remove a lot of code cov

@crodriguezvega crodriguezvega merged commit bd16994 into main Nov 20, 2023
72 checks passed
@crodriguezvega crodriguezvega deleted the jim/4212-move-upgraded-client-check-to-02-client branch November 20, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move upgraded client height check from light clients to 02-client
6 participants