refactor: solomachine misbehaviour checking #1715
Conversation
damiannolan
requested review from
colin-axner,
fedekunze,
AdityaSripal,
crodriguezvega,
seantking and
charleenfei
as code owners
Codecov Report
@@ Coverage Diff @@
## damian/solomachine-generic-verify #1715 +/- ##
=====================================================================
- Coverage 80.06% 79.85% -0.21%
=====================================================================
Files 166 166
Lines 11808 11821 +13
=====================================================================
- Hits 9454 9440 -14
- Misses 1905 1931 +26
- Partials 449 450 +1
|
| @@ -71,6 +71,24 @@ func (sd SignatureAndData) ValidateBasic() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // ValidateBasic ensures that the signature and data fields are non-empty. | |||
| func (sd SignatureAndDataV2) ValidateBasic() error { | |||
| if len(sd.Signature) == 0 { | |||
There was a problem hiding this comment.
This is a nit, but maybe UX could be slightly improved if we group errors, in case more than one thing is empty. For example, if both signature and data are empty, then you can return one error saying that signature and data cannot be empty.
There was a problem hiding this comment.
i kind of like having the separate errors as they come rather than arbitrarily grouping errors (sometimes two? sometimes 3? what if all four error out?), this way we can keep it consistent across the codebase...
There was a problem hiding this comment.
I think this point has come up a few times before. I tend to agree with the general behaviour of the sdk of "fail fast" where the error is returned immediately rather than grouping. I think it makes for more idiomatic go code. I do see the reasoning for validating the entire message and returning a group of error responses... it is in line with more traditional rest api design in a way. But all things considered I'd prefer to keep the code consistent and stay aligned with the sdk pattern.
If we were to change the behaviour here it should be changed across our entire codebase. And still, it would differ from standard sdk modules then like x/auth,x/bank, x/gov ..etc
…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
SignatureAndDatatype temporarily toSignatureAndDataV2NOTE: branch 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