-
Notifications
You must be signed in to change notification settings - Fork 624
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
refactor: solomachine generic verification methods and signbytes simplification #1687
refactor: solomachine generic verification methods and signbytes simplification #1687
Conversation
modules/light-clients/06-solomachine/types/client_state_test.go
Outdated
Show resolved
Hide resolved
* adding SignatureAndDataV2 proto message type * updating misbehaviour checking * removing dead solomachine code (#1716)
* adding verification of non-membership with tests * refactor common code to produceVerificationArgs * removing unused produce args func * Update modules/light-clients/06-solomachine/client_state_test.go Co-authored-by: colin axnér <[email protected]> Co-authored-by: colin axnér <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love these changes so much ❤️ Cleans up a lot of unnecessary code. Thanks for handling these changes and splitting them up in a very digestible way. The code is very clean!
Everything LGTM, the only change I'd block merge on is I believe we need to bump the proto version for the structs which have changed
* add test for successful new diversifier * add changelog entry * fix tests
… solomachine/v3 protobuf defs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wahoo! Excellent!
ConsensusState consensus_state = 3 [(gogoproto.moretags) = "yaml:\"consensus_state\""]; | ||
// when set to true, will allow governance to update a solo machine client. | ||
// The client will be unfrozen if it is frozen. | ||
bool allow_update_after_proposal = 4 [(gogoproto.moretags) = "yaml:\"allow_update_after_proposal\""]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we actually checking this bool anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colin-axner and I actually spoke about potentially removing this yesterday. It could be done in a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are. I think we should just remove it for the same reasoning we did in 07-tendermint (gov always can do a migration if they really want to). Agreed?
I can do it in a followup if we agree
…lification (cosmos#1687) * adding new SignBytes type, generic membership verification implementation and tests * adding protodocs * updating comment * refactor: solomachine misbehaviour checking (cosmos#1715) * adding SignatureAndDataV2 proto message type * updating misbehaviour checking * removing dead solomachine code (cosmos#1716) * updating tests with concrete ibc core types * refactor: solomachine generic VerifyNonMembership (cosmos#1720) * adding verification of non-membership with tests * refactor common code to produceVerificationArgs * removing unused produce args func * Update modules/light-clients/06-solomachine/client_state_test.go Co-authored-by: colin axnér <[email protected]> Co-authored-by: colin axnér <[email protected]> * removing V2 suffix from SignBytes and SignatureAndData types * use current diversifier when verifying header details * Add test for new diversifier for solomachine (cosmos#1860) * add test for successful new diversifier * add changelog entry * fix tests * restoring solomachine/v2 protos, updadting v2 codegen path and adding solomachine/v3 protobuf defs * adding changelog entries Co-authored-by: colin axnér <[email protected]>
Description
SignBytes
message type ( noteV2
suffix will be dropped when PRs are upstreamed to this branch)VerifyMembership
implementationneeds:
closes: #1292 #1295 #1141
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes