refactor: solomachine generic VerifyNonMembership #1720
Conversation
damiannolan
requested review from
colin-axner,
fedekunze and
AdityaSripal
as code owners
damiannolan
requested review from
charleenfei,
chatton,
crodriguezvega and
seantking
Codecov Report
@@ Coverage Diff @@
## damian/solomachine-generic-verify #1720 +/- ##
=====================================================================
+ Coverage 80.06% 80.11% +0.05%
=====================================================================
Files 166 166
Lines 11808 11841 +33
=====================================================================
+ Hits 9454 9487 +33
+ Misses 1905 1904 -1
- Partials 449 450 +1
|
…ine-verify-non-membership
| sdkerrors.ErrInvalidHeight, | ||
| "client state sequence != proof sequence (%d != %d)", latestSequence, sequence, | ||
| ) | ||
| publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof) |
There was a problem hiding this comment.
[nit] I know produceVerificationArgs wasn't changed in this PR, so feel free to dis-regard, but I think 5 values is a bit too many to return. What do you think about wrapping the first 4 values in a struct and having the the method signature return the struct and an error? (unless we can't change the method signature for some reason)
verificationArgs, err := produceVerificationArgs(cdc, cs, height, proof)
There was a problem hiding this comment.
Sounds like a great suggestion to me
I guess my only concern would be constructing a type verificationArgs which are dependent on arguments that needed to be checked rather than the totality of arguments required for verification. The path/diversifier/data being the others. We could also take away some of the return arguments (public key, sequence) can be derived from the client state. Originally this function was made to reduce redundancy of calls by 6-7 functions, now only 2 functions rely on it so redundancy isn't as much of a concern. Thoughts?
There was a problem hiding this comment.
In general I agree, and normally lean against having such a large number of return args. I'd be happy to continue the discussion or explore options for improving the function sig and its usage, maybe we could potentially open an issue for this? At least this an unexported function so whatever we decide should not have any implications on consumers.
I'm going to go ahead and merge this to the [WIP] branch and get it R4R with renaming types to remove the V2 suffixes.
| sdkerrors.ErrInvalidHeight, | ||
| "client state sequence != proof sequence (%d != %d)", latestSequence, sequence, | ||
| ) | ||
| publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof) |
There was a problem hiding this comment.
Sounds like a great suggestion to me
I guess my only concern would be constructing a type verificationArgs which are dependent on arguments that needed to be checked rather than the totality of arguments required for verification. The path/diversifier/data being the others. We could also take away some of the return arguments (public key, sequence) can be derived from the client state. Originally this function was made to reduce redundancy of calls by 6-7 functions, now only 2 functions rely on it so redundancy isn't as much of a concern. Thoughts?
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
…lification (#1687) * adding new SignBytes type, generic membership verification implementation and tests * adding protodocs * updating comment * refactor: solomachine misbehaviour checking (#1715) * adding SignatureAndDataV2 proto message type * updating misbehaviour checking * removing dead solomachine code (#1716) * updating tests with concrete ibc core types * refactor: solomachine generic VerifyNonMembership (#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 <25233464+colin-axner@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * removing V2 suffix from SignBytes and SignatureAndData types * use current diversifier when verifying header details * Add test for new diversifier for solomachine (#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 <25233464+colin-axner@users.noreply.github.com>
…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 <25233464+colin-axner@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * 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 <25233464+colin-axner@users.noreply.github.com>
Description
VerifyNonMembershipimplementationproduceVerificationArgshelper funcNOTE: this PR is targeted for #1687
closes: #XXXX
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/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes