Skip to content
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 #149

Merged
merged 10 commits into from
Jul 6, 2021

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Jun 2, 2021

Replaces multihashes with the new multiformats module

BREAKING CHANGE: uses the CID class from the new multiformats module and PeerID.createFromCID now only takes CID instances

Replaces multihashes with the new multiformats module

BREAKING CHANGE: uses the CID class from the new multiformats module
src/index.js Outdated
const cryptoKeys = require('libp2p-crypto/src/keys')
const withIs = require('class-is')
const { PeerIdProto } = require('./proto')
const uint8ArrayEquals = require('uint8arrays/equals')
const uint8ArrayFromString = require('uint8arrays/from-string')
const uint8ArrayToString = require('uint8arrays/to-string')

const IDENTITY_CODE = 0x00
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not using multicodec here? I would like to just have these codes in one place as the source of truth

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one of the design goals behind the new multiformats module was to not have the big lookup tables that multicodec is the guardian of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Probably worth adding a reference to the table just to reference where they come from? Probably to https://github.com/multiformats/multicodec/blob/master/table.csv

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my journey through the codebase doing the multiformats stuff it seems that we only really use four or five of them. I think the only weirdness is when bitswap has to mess around with CID prefixes in incoming bitswap 1.1.0 messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just adding a comment above with the multicodec table is enough here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let me know when I should release it

@achingbrain
Copy link
Member Author

I've tightened up the input types for PeerId.createFromCID as a follow on from multiformats/js-multiformats#91 (comment) - it now only takes CID instances.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏼

@vasco-santos
Copy link
Member

@achingbrain FYI #150 should also be ready soon (also breaking change), so I would like to get these shipped together as bubbling up PeerId updates across all libp2p stack is time consuming. This will create some conflicts that will need to be addresses by the second PR to be merged

@achingbrain achingbrain marked this pull request as ready for review July 6, 2021 10:54
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also have the README updated for parse and createFromCID?

src/index.d.ts Outdated Show resolved Hide resolved
@vasco-santos vasco-santos merged commit 69d7a01 into master Jul 6, 2021
@vasco-santos vasco-santos deleted the chore/update-to-new-multiformats branch July 6, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants