-
Notifications
You must be signed in to change notification settings - Fork 129
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
feat(wasmer/crypto): move sig verifier to crypto pkg #2057
Merged
kishansagathiya
merged 43 commits into
ChainSafe:development
from
1garo:1garo/move-sig-verifier-to-crypto
Dec 6, 2021
Merged
Changes from 33 commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
db85f05
chore: delete sig verifier from runtime pkg
1garo eda4236
feat: implement SignVerify for sr25519
1garo ed89c50
feat: implement SignVerify for ed25519
1garo 6f41dfc
feat: implement SignVerify for srcp256k1
1garo 73e8d87
fix: move sig_verifier to crypto pkg
1garo 11b6016
chore: update usages for crypto pkg
1garo 63a3442
fix: resolve lint problem
1garo 969efc2
chore: add comment on exported type
1garo 9146c37
remove spaces between imports
1garo a839018
rename function to improve readability
1garo 43ad637
add err signature err msg and add return for docs purpose
1garo 9531a54
improve naming and error handling
1garo 7a9560e
implement tests for new function
1garo 9c31504
gen by mockery
1garo fd188fc
create tests for sig verifier
1garo 53ca92b
Merge branch 'development' into 1garo/move-sig-verifier-to-crypto
1garo 610e4cd
rename type and refactor VerifyFunc call
1garo 5c45a1f
remove ok bool as return value
1garo 5c15e6f
update tests and add one for sig_verifier
1garo fbe70d8
add signature name when error occurs
1garo c164f4d
parallel tests and rename test case
1garo e1717bc
parallel tests and add log level
1garo f009530
gofmt
1garo b83cc03
Merge branch 'development' into 1garo/move-sig-verifier-to-crypto
1garo 9534e7e
resolve make build erroring
1garo 634ea82
rename type
1garo 973ce6b
remove space between imports
1garo ddba19f
remove useless alias
1garo da14c99
check ok, if false errors
1garo 664aa50
rename typ
1garo 625ca02
solve tests failing
1garo 975b406
use test cases to remove useless code
1garo 75f6080
send empty bytes on error test case
1garo 7a62d06
improve tests cases
1garo 7da55ab
use constants instead of just []byte
1garo 2fdabf8
switch validation order
1garo ae14785
use new err approach validation and add tests
1garo f223d65
make use of constants
1garo 1ee5e82
fix error validation on ed25519
1garo 4012a66
fix error validation on sr25519
1garo f7b81b1
put publicKey in var to avoid golangci-lint error
1garo 52bc4c3
use a new err variable for each parallel test run
kishansagathiya 698d36e
Merge branch 'development' into 1garo/move-sig-verifier-to-crypto
kishansagathiya File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 need to split those two conditions. If error is nil and ok is false (when the verification fails), we should wrap our own error defined in this package (or some shared package) and not wrap the
nil
error returned.Alternatively we could modify
Verify
methods to return an error if the verification fails and not the booleanok
, but that might imply a lot of code changes, so not really a solution here.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.
My suggestion would be:
since when
err != nil
, the cause of it will be explicit onerr
, e.g if one of those happen:and in the case where
err == nill && !ok
, the problem was really the verification step, i.e:The same would apply for
ed25518
.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.
Sure, although don't wrap both errors with
crypto.ErrSignatureVerificationFailed
that looks strange. The parent calling function should wrap the error in this case. Instead you can just have?
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.
looking now it really seems strange that way, changed on 1ee5e82 4012a66