-
Notifications
You must be signed in to change notification settings - Fork 25
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
fix: replace node buffers with uint8arrays #67
fix: replace node buffers with uint8arrays #67
Conversation
BREAKING CHANGES: - All deps of this module use Uint8Arrays instead of Buffers - value and validity fields of IPNSEntries are now Uint8Arrays instead of Strings as they are `bytes` in the protobuf definition
src/index.js
Outdated
@@ -193,7 +196,7 @@ const extractPublicKey = (peerId, entry) => { | |||
if (entry.pubKey) { | |||
let pubKey | |||
try { | |||
pubKey = crypto.keys.unmarshalPublicKey(entry.pubKey) | |||
pubKey = crypto.keys.unmarshalPublicKey(Buffer.from(entry.pubKey)) |
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.
Aren't we replacing the Buffers everywhere?
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.
The underlying crypto libraries do not understand Uint8Arrays and require Buffers :(
But! The user should be able to pass Uint8Arrays to our modules without them exploding.
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.
This should be updated to use libp2p-crypto@0.18.0, it does use Uint8Array now
node: { | ||
path: true, | ||
|
||
Buffer: true |
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.
do we need this?
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.
Yes, I've updated the file with comments about which modules require the node libs
BREAKING CHANGES:
of Strings as they are
bytes
in the protobuf definition