fix: support uint8arrays in place of node buffers #23
Conversation
Allows passing Uint8Arrays in place of Node Buffers BREAKING CHANGE: takes Uint8Arrays as well as Node Buffers
|
cc @Gozala (can't add you to the reviewers list) |
| @@ -16,7 +18,8 @@ const bestRecord = (selectors, k, records) => { | |||
| throw errcode(new Error(errMsg), 'ERR_NO_RECORDS_RECEIVED') | |||
| } | |||
|
|
|||
| const parts = k.toString().split('/') | |||
| const kStr = utf8Decoder.decode(k) | |||
| const parts = kStr.split('/') | |||
There was a problem hiding this comment.
📝 Seems like factoring out logic to count / char codes would be make sense here, which also could be then improved to avoid string encoding splitting etc...
There was a problem hiding this comment.
I'm going to leave this in place as it's just doing what was being done before, but it's a valid point.
| @@ -12,7 +13,8 @@ const errcode = require('err-code') | |||
| */ | |||
| const verifyRecord = (validators, record) => { | |||
| const key = record.key | |||
| const parts = key.toString().split('/') | |||
| const keyString = utf8Decoder.decode(key) | |||
| const parts = keyString.split('/') | |||
There was a problem hiding this comment.
📝 This is the second place were / char codes are counted.
There was a problem hiding this comment.
I'm going to leave this in place as it's just doing what was being done before, but it's a valid point.
src/validators/public-key.js
Outdated
| throw errcode(new Error('invalid public key record'), 'ERR_INVALID_RECORD_KEY_TOO_SHORT') | ||
| } | ||
|
|
||
| const prefix = key.slice(0, 4).toString() | ||
| const keyString = utf8Decoder.decode(key) |
There was a problem hiding this comment.
📝 Seems like const prefix = utf8Decoder.decode(key.subarray(0, 4)) would have kept same profile.
There was a problem hiding this comment.
I've made this change
src/validators/public-key.js
Outdated
| throw errcode(new Error('key was not prefixed with /pk/'), 'ERR_INVALID_RECORD_KEY_BAD_PREFIX') | ||
| } | ||
|
|
||
| const keyhash = key.slice(4) | ||
|
|
||
| const publicKeyHash = await multihashing(publicKey, 'sha2-256') | ||
|
|
||
| if (!keyhash.equals(publicKeyHash)) { | ||
| if (!keyhash.every((val, i) => val === publicKeyHash[i])) { |
There was a problem hiding this comment.
🚨 This does not take byteLength into account.
📝 Seems like equality check should be factored out into separate function
There was a problem hiding this comment.
These are Uint8Arrays so unless I'm missing something .length should always equal .byteLength so we should be good to use .every and friends?
That said it could be optimised to compare lengths before iterating over the values which would worth pulling into it's own function.
There was a problem hiding this comment.
I've pulled it into it's own function.
There was a problem hiding this comment.
These are
Uint8Arrays so unless I'm missing something.lengthshould always equal.byteLengthso we should be good to use.everyand friends?
They do. I was just trying to say that keyhash and publicKeyHash could have different length which this code was not taking into account.
Pulls in the latest interface-datastore and replaces use of node Buffers with Uint8Arrays Depends on: - [ ] ipfs/interface-datastore#43 - [ ] ipfs/js-datastore-core#27 - [ ] libp2p/js-libp2p-record#23
|
|
Allows passing Uint8Arrays in place of Node Buffers
BREAKING CHANGE: takes Uint8Arrays as well as Node Buffers