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

chore: remove references to AllowUpdateAfterExpiry, AllowUpdateAfterMisbehaviour #1843

Merged
merged 18 commits into from
Aug 9, 2022

Conversation

charleenfei
Copy link
Contributor

Description

closes: #1237


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

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

Merging #1843 (15acb28) into main (8b61fa1) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1843      +/-   ##
==========================================
- Coverage   80.07%   80.07%   -0.01%     
==========================================
  Files         167      167              
  Lines       11750    11747       -3     
==========================================
- Hits         9409     9406       -3     
  Misses       1925     1925              
  Partials      416      416              
Impacted Files Coverage Δ
modules/light-clients/07-tendermint/upgrade.go 73.86% <ø> (-0.30%) ⬇️
...odules/light-clients/07-tendermint/client_state.go 89.30% <100.00%> (-0.12%) ⬇️

@colin-axner
Copy link
Contributor

Sorry about the conflicts ❤️

Looks like most of those conflicts can be deleted -> "07-tendermint/types". Let me know if you need help going through them

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! Thanks for fixing the conflicts!

Sorry I didn't mention this before, but it would be good to leave test cases ensuring we can update a frozen client (might want to just leave a test case updating an expired client as well. The value of the allow booleans of course don't matter in these cases

Comment on lines 125 to 127
if tc.FreezeClient {
subjectClientState.FrozenHeight = frozenHeight
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably leave the freeze client cases in since we unfreeze the client if it is frozen in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yea good point

@charleenfei charleenfei merged commit 088ba19 into main Aug 9, 2022
@charleenfei charleenfei deleted the charly/remove_ref_allow_update branch August 9, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour references
5 participants