multisig-test-vectors #37
Merged
Conversation
This file contains hidden or 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
sstanculeanu
reviewed
Sep 9, 2024
|
|
||
| const blsHashSize = 16 | ||
|
|
||
| //"message to be signed or to be verified is nil" |
Comment on lines
60
to
91
| // KeyPair defines a pair of public key and private key | ||
| type KeyPair struct { | ||
| PublicKey string `json:"publicKey"` | ||
| PrivateKey string `json:"privateKey"` | ||
| } | ||
|
|
||
| // SignaturePair defines a pair of signature and public key | ||
| type SignaturePair struct { | ||
| Signature string `json:"signature"` | ||
| PublicKey string `json:"publicKey"` | ||
| } | ||
|
|
||
| // TestVectorElement defines the data for a test vector | ||
| type TestVectorElement struct { | ||
| Signatures []SignaturePair `json:"signatures"` | ||
| Message string `json:"message"` | ||
| AggregatedSignature string `json:"aggregatedSignature"` | ||
| ErrorMessage string `json:"errorMessage"` | ||
| TestName string `json:"testName"` | ||
| } | ||
|
|
||
| // JSONFileContent defines the data for generating the json file | ||
| type JSONFileContent struct { | ||
| Keys []KeyPair `json:"keys"` | ||
| TestVectors []TestVectorElement `json:"testVectors"` | ||
| } | ||
|
|
||
| // Key defines a tuple of public key and private key | ||
| type Key struct { | ||
| PubKey crypto.PublicKey | ||
| PrivateKey crypto.PrivateKey | ||
| } |
Contributor
There was a problem hiding this comment.
perhaps extract all exported structs to a new file, data.go
|
|
||
| func createKeyPairs(grSize uint16, suite crypto.Suite) []Key { | ||
| kg := signing.NewKeyGenerator(suite) | ||
| var keys []Key |
Contributor
There was a problem hiding this comment.
Suggested change
| var keys []Key | |
| keys := make([]Key, 0, grSize) |
small optimization
|
|
||
| // generateJSONFileKOSKForAggregateSignaturesTests for KOSK AggregateSignaturesTests | ||
| func generateJSONFileKOSKForAggregateSignaturesTests(signer crypto.LowLevelSignerBLS) error { | ||
| return generateJSONFile(signer, predefinedKOSKAggregateSignaturesTests, "multisigKOSKAggSig.json") |
Contributor
There was a problem hiding this comment.
json files should be generated and moved into a special directory, testData
Author
There was a problem hiding this comment.
created a special directory named testData and moved the json file
| func generateJSONFile(lls crypto.LowLevelSignerBLS, predefinedTests []PredefinedTest, filename string) error { | ||
| suite := mcl.NewSuiteBLS12() | ||
|
|
||
| mapKeys := createKeyPairs(uint16(5), suite) |
Contributor
There was a problem hiding this comment.
perhaps extract the number of keys as parameter as well
| ) | ||
|
|
||
| // TestVector defines the data structure used to unmarshal the JSON file | ||
| type TestVector struct { |
Contributor
There was a problem hiding this comment.
why not use the already defined structure from jsonGenerator.go?
Author
There was a problem hiding this comment.
the one from jsonGenerator.go is a struct of strings, and I need all the data to be decoded
signing/mcl/multisig/vectors_test.go
Outdated
Comment on lines
41
to
45
| testName := testVector.testName | ||
| signatures := testVector.signatures | ||
| pubKeys := testVector.publicKeys | ||
| expectedError := testVector.expectedError | ||
| aggregatedSig := testVector.aggregatedSig |
Contributor
There was a problem hiding this comment.
new vars not necessarily needed
valid for all tests
signing/mcl/multisig/vectors_test.go
Outdated
Comment on lines
86
to
89
| lls := &BlsMultiSigner{} | ||
| hasher, err := blake2b.NewBlake2bWithSize(blsHashSize) | ||
| require.Nil(t, err) | ||
| lls.Hasher = hasher |
Contributor
There was a problem hiding this comment.
Suggested change
| lls := &BlsMultiSigner{} | |
| hasher, err := blake2b.NewBlake2bWithSize(blsHashSize) | |
| require.Nil(t, err) | |
| lls.Hasher = hasher | |
| hasher, err := blake2b.NewBlake2bWithSize(blsHashSize) | |
| require.Nil(t, err) | |
| lls := &BlsMultiSigner{ | |
| Hasher: hasher, | |
| } |
signing/mcl/multisig/vectors_test.go
Outdated
Comment on lines
116
to
119
| lls := &BlsMultiSigner{} | ||
| hasher, err := blake2b.NewBlake2bWithSize(blsHashSize) | ||
| require.Nil(t, err) | ||
| lls.Hasher = hasher |
Contributor
There was a problem hiding this comment.
Suggested change
| lls := &BlsMultiSigner{} | |
| hasher, err := blake2b.NewBlake2bWithSize(blsHashSize) | |
| require.Nil(t, err) | |
| lls.Hasher = hasher | |
| hasher, err := blake2b.NewBlake2bWithSize(blsHashSize) | |
| require.Nil(t, err) | |
| lls := &BlsMultiSigner{ | |
| Hasher: hasher, | |
| } |
signing/mcl/multisig/vectors_test.go
Outdated
Comment on lines
168
to
169
| var pubKeys []crypto.PublicKey | ||
| var sigs [][]byte |
Contributor
There was a problem hiding this comment.
Suggested change
| var pubKeys []crypto.PublicKey | |
| var sigs [][]byte | |
| pubKeys := make([]crypto.PublicKey, 0, len(signatures)) | |
| sigs := make([][]byte, 0, len(signatures)) |
AdoAdoAdo
reviewed
Sep 16, 2024
| } | ||
|
|
||
| // predefinedKOSKAggregateSignaturesTests defines the scenarios for testing the AggregateSignatures for KOSK | ||
| var predefinedKOSKAggregateSignaturesTests = []PredefinedTest{ |
Contributor
There was a problem hiding this comment.
these structures at lines 15, 24, 33, 42 are very similar (duplicated). We can keep only one definition (rename it to something more generic) and use it for the different test vectors.
signing/mcl/multisig/vectors_test.go
Outdated
|
|
||
| for _, testVector := range jsonContent.TestVectors { | ||
|
|
||
| testName := testVector.TestName |
Contributor
There was a problem hiding this comment.
can you extract the lines 149 -> 175 into a function that creates a TestVector instance?
AdoAdoAdo
approved these changes
Sep 20, 2024
sstanculeanu
approved these changes
Sep 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.