Conversation
achingbrain
left a comment
There was a problem hiding this comment.
LGTM - just needs @SgtPooki's comments addressing and it's ready.
|
I've made some changes to address @achingbrain's feedback from #745 (comment), taking in a key first and the value second. I also added the ability to pass in a string |
achingbrain
left a comment
There was a problem hiding this comment.
Looking good, final few nits
| if (typeof key === 'string') { | ||
| // Convert string key to MultihashDigest | ||
| try { | ||
| mh = this.#getPeerIdFromString(key).toMultihash() |
There was a problem hiding this comment.
Should we require string keys to be fully qualified? E.g. /ipns/...
There was a problem hiding this comment.
mmmmm. According to the spec that's good practice but not required. I'd rather not, since the current APIs don't expect it, so this would be a bit contrived.
There was a problem hiding this comment.
We should probably strip any leading /ipns/ from the key then?
There was a problem hiding this comment.
Will do in a follow up PR
Co-authored-by: Alex Potsides <alex@achingbrain.net>
| /** | ||
| * Convert a string to a PeerId | ||
| */ | ||
| #getPeerIdFromString (peerIdString: string): PeerId { |
There was a problem hiding this comment.
Once libp2p/js-libp2p#3042 is released, we can just use peerIdFromString
* origin/main: chore: use peer id parsing function from libp2p (#762) feat: add republish signed ipns records (#745) fix: use bytestream methods to add byte streams (#758) chore: bump codecov/codecov-action from 5.3.1 to 5.4.0 (#752) feat: allow modifying trustless-gateway fetch (#751) fix: align implicit default ttl with specs (#749) docs: add spell checker to ci (#743) chore: Update FUNDING.json for Optimism RPF (#746)
Fixes #745 (comment) --------- Co-authored-by: Daniel N <2color@users.noreply.github.com>
What
Fixes #744
Change checklist