-
Notifications
You must be signed in to change notification settings - Fork 133
feat: add republish signed ipns records #745
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
Conversation
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.
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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we require string keys to be fully qualified? E.g. /ipns/...
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably strip any leading /ipns/
from the key then?
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.
Will do in a follow up PR
Co-authored-by: Alex Potsides <alex@achingbrain.net>
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.
👍 lgtm...
/** | ||
* Convert a string to a PeerId | ||
*/ | ||
#getPeerIdFromString (peerIdString: string): PeerId { |
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.
Once libp2p/js-libp2p#3042 is released, we can just use peerIdFromString
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 has shipped
* 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