-
Notifications
You must be signed in to change notification settings - Fork 52
fix: replace node buffers with uint8arrays #180
Conversation
All usage of node buffers have been replaced with uint8arrays. BREAKING CHANGES: - Where node Buffers were returned, now Uint8Arrays are
Tagging @Gozala too as he's not in the eligible reviewers list. |
@@ -16,7 +16,7 @@ exports.generateKeyFromSeed = async function (seed) { // eslint-disable-line req | |||
|
|||
exports.hashAndSign = async function (key, msg) { // eslint-disable-line require-await | |||
return forge.pki.ed25519.sign({ message: msg, privateKey: key }) | |||
// return Buffer.from(nacl.sign.detached(msg, key)) | |||
// return Uint8Array.from(nacl.sign.detached(msg, key)) |
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.
Forge is going to create an api inconsistency whenever it's returned. Forge isn't very consistent so some of the returned types may be Buffer
in node.js. This is the case for ed25519. We likely need to add more checks in the tests for api types.
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.
It's ok to return Buffers as they are still Uint8Arrays. I think it's only a problem if it returns the weird ascii-byte-array-string it seems to use internally.
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.
Though we could just pass the output to new Uint8Array
for safety. As long as it's not a that string type.
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.
It should be ok. I believe the API does convert from the forge buffer when returning.
All usage of node buffers have been replaced with uint8arrays.
BREAKING CHANGES: