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

change FrozenSequence to boolean in solo machine #169

Merged
merged 7 commits into from
May 12, 2021

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented May 10, 2021

Description

This will be a lot easier to do now since the solo machine was broken and we don't have to worry about backwards compatibility

closes: #36


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

return exported.Frozen
}

return exported.Active
}

// IsFrozen returns true if the client is frozen.
func (cs ClientState) IsFrozen() bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should have been deleted in the Status pr

Copy link
Member

Choose a reason for hiding this comment

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

Have you removed it from localhost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, I think this one got added back during a merge of main when I fixed the solo machine bug

// GetFrozenHeight returns the frozen sequence of the client.
// Return exported.Height to satisfy interface
// Revision number is always 0 for a solo-machine
func (cs ClientState) GetFrozenHeight() exported.Height {
return clienttypes.NewHeight(0, cs.FrozenSequence)
return clienttypes.NewHeight(0, 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be removed by #165

@@ -445,10 +440,6 @@ func produceVerificationArgs(
}
// sequence is encoded in the revision height of height struct
sequence := height.GetRevisionHeight()
if cs.IsFrozen() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should have been removed by Status pr, this is checked in 02-client

@@ -16,7 +16,7 @@ message ClientState {
// latest sequence of the client state
uint64 sequence = 1;
// frozen sequence of the solo machine
uint64 frozen_sequence = 2 [(gogoproto.moretags) = "yaml:\"frozen_sequence\""];
bool is_frozen = 2 [(gogoproto.moretags) = "yaml:\"is_frozen\""];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is proto breaking. Should I make a v2? @AdityaSripal @fedekunze

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 could alternatively deprecate the frozen sequence field and add a new field is_frozen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to try to do a v2. I think it is good practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I just changed the package from v1 -> v2

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.91%. Comparing base (4007cfa) to head (e1fb7a2).
Report is 2441 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #169   +/-   ##
=======================================
  Coverage   79.91%   79.91%           
=======================================
  Files         107      107           
  Lines        6417     6409    -8     
=======================================
- Hits         5128     5122    -6     
+ Misses        931      929    -2     
  Partials      358      358           
Files with missing lines Coverage Δ
...light-clients/06-solomachine/types/client_state.go 67.00% <100.00%> (-0.16%) ⬇️
...lients/06-solomachine/types/misbehaviour_handle.go 93.93% <100.00%> (-0.35%) ⬇️
...ht-clients/06-solomachine/types/proposal_handle.go 100.00% <100.00%> (ø)

@colin-axner colin-axner mentioned this pull request May 11, 2021
30 tasks
@colin-axner colin-axner marked this pull request as draft May 11, 2021 10:52
@colin-axner colin-axner marked this pull request as ready for review May 11, 2021 11:05
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK

fixed changelog, removed GetFrozenHeight solo machine function
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

LGTM. Check localhost has been changed correctly after Status PR.

return exported.Frozen
}

return exported.Active
}

// IsFrozen returns true if the client is frozen.
func (cs ClientState) IsFrozen() bool {
Copy link
Member

Choose a reason for hiding this comment

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

Have you removed it from localhost?

@colin-axner colin-axner merged commit f412334 into main May 12, 2021
@colin-axner colin-axner deleted the colin/36-sm-isfrozen-bool branch May 12, 2021 16:18
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.

Replace FrozenSequence with IsFrozen boolean in Solomachine Client
4 participants