-
Notifications
You must be signed in to change notification settings - Fork 12
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
chore: update to new multiformats #46
Conversation
Depends on: - [ ] multiformats/js-multiformats#91 BREAKING CHANGE: uses the CID class from the new multiformats module
src/index.js
Outdated
@@ -116,14 +115,14 @@ class DelegatedPeerRouting { | |||
// Track the addresses, so we can yield them when done | |||
result.responses.forEach(response => { | |||
peers.set(response.id, { | |||
id: PeerId.createFromCID(response.id), | |||
id: PeerId.createFromB58String(response.id), |
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 want to make this change? I remember we wanted to move towards not using b58 peer IDs and this will mean changing this back again
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.
PeerId.createFromCID(response.id)
here was accidentally taking a base58 encoded string so can't be used any more. Annoyingly the DHT result type 2 has reponse.id
as a CID
instance whereas type 1 has it as a string.
I've changed it to use PeerId.parse
so we can add support for extra bases there without having to refactor everything in future.
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
Depends on:
BREAKING CHANGE: uses the CID class from the new multiformats module