JAV-18 [Github #253] Signature Schema Refactoring #254
Conversation
Pull Request Test Coverage Report for Build 664
💛 - Coveralls |
fboucquez
left a comment
fboucquez
left a comment
There was a problem hiding this comment.
Hi Steve, I've added a few comments. Nothing blocker. What do you think about them?
src/core/crypto/KeyPair.ts
Outdated
| secretKey = convert.hexToUint8Reverse(convert.uint8ToHex(secretKey)); | ||
| } | ||
| return Utility.catapult_crypto.deriveSharedKey(salt, secretKey, publicKey, Utility.catapult_hash.func, signSchema); | ||
| return Utility.catapult_crypto.deriveSharedKey(salt, keyPair.privateKey, publicKey, Utility.catapult_hash.func, networkType); |
There was a problem hiding this comment.
I would keep the reverse hooks, maybe create a utility method that reverse if necessary (called every time there was a hexToUint8Reverse). Just in case you want to add the reverse stuff in the future.
There was a problem hiding this comment.
Not sure about this. Let's confirm with core first. If the decision is non-reversal I don't think we should keep it.
The 'hexToUint8Reverse' definitely stays, as will probably need it at some point. E.g. the UInt64 is using Little Endian.
| 'NCBC4VAQBVSB4J5J2PTFM7OUY5CYDL33VW7RKW7K', | ||
| 'NBLW3CQPBGPCFAXG4XM5GDEVLPESCPDPFO7NUCOM', | ||
| 'NA5RDU36TKBTW4KVSSPD7PT5YTUMD7OIJFMMIEEQ', | ||
| 'MDIPRQMB3HT7A6ZKV7HOHJQM7JHX6H3FN5YHHZMD', |
There was a problem hiding this comment.
Maybe you can load the test cases directly from the vector files. I've added that to Java sdk.
There was a problem hiding this comment.
Good point, but the size of the test vector json files is a concern. To keep all unit tests run offline, need to copy the files to the project.
…factoring JAV-2 [Github symbol#226] MosaicDefinition refactoring
…eceipt JAV-12 Added Namespace pruned receipt type
…em2-sdk-typescript-javascript into task/g253_signSchema_refactoring
fix this.createBuilder
…em2-sdk-typescript-javascript into task/g253_signSchema_refactoring
src/core/format/RawAddress.ts
Outdated
| * Determines the validity of a decoded address. | ||
| * @param {Uint8Array} decoded The decoded address. | ||
| * @param {SignSchema} signSchema The Sign Schema. (KECCAK_REVERSED_KEY / SHA3) | ||
| * @param {number} networkIdentifier The network identifier. |
There was a problem hiding this comment.
what is network identifier ? Is it a networkType ? or maybe do we need an added model NetworkIdentifier ?
There was a problem hiding this comment.
I think's it's a typo. should read NetworkType
fboucquez
left a comment
fboucquez
left a comment
There was a problem hiding this comment.
All good. The only thing would be to add more vector tests related to Keccak (not reversed) once the new vectors have been defined. In Java, I'm still supporting reversed Keccak just for the tests although network type resolves the non-reverse Keccak. Just for a confidence boost, if the reverse Keccak (nes1) vector tests passes, new simpler Keccak implementation would pass too.
5 participants
Signature Schema Refactoring
Signature schema (NIS1 vs Catapult) can be identified by
NetworkType(based on confirmation from Core on [Sig-API]. Signatures of methods which previously usedSignSchemahave been refactored to useNetworkTypeinstead.KeyPair generation under NIS1 does NOT reverse the private key bytes anymore. Keys & Address generated / derived under NIS1 schema in the SDK will not be compatible with current public net any more.
Fixed all unit tests (due to key reversal removed)