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

feat: support parsing base32 and base36 libp2p-key CIDs in peerIdFromString #3042

Merged
merged 5 commits into from
Mar 12, 2025

Conversation

2color
Copy link
Contributor

@2color 2color commented Mar 11, 2025

Title

Given the prevalence of both CID encoded PeerIDs and base58mh encoded ones, I found myself defining this function in multiple places.

This PR introduces this as a helper (as was supported in an earlier version of js-libp2p)

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@2color 2color requested a review from a team as a code owner March 11, 2025 12:23
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

Looking at the utility functions exported by this module, if I have a string and I want a PeerId it would not be obvious to me whether to call peerIdFromString or this new getPeerId function.

Given that the spec mentions base32 (not base36 as in the comment here) encoded libp2p-key CIDs as a canonical string encoding, let's check form that prefix in peerIdFromString instead.

@2color
Copy link
Contributor Author

2color commented Mar 11, 2025

Given that the spec mentions base32 (not base36 as in the comment here) encoded libp2p-key CIDs as a canonical string encoding, let's check form that prefix in peerIdFromString instead.

Do you mean checking the multibase prefix in the string for b and decoding it as a CID if it is in the peerIdFromString function?

Regarding base36 cids, I realise it's not in the peerIDs spec, but it's the standard string form for ipns names. Do you feel strongly about not including it here?

@achingbrain
Copy link
Member

Do you mean checking the multibase prefix in the string for b

Yes, but with few more characters to include the libp2p-key codec in the check so we are more sure it's a CID.

it's the standard string form for ipns names. Do you feel strongly about not including it here?

Groan. The usual reason not to include lots of different base encoder/decoders is because they bloat bundle sizes but I suppose here we have a dependency on the CID class so base36 is in there anyway.

@2color
Copy link
Contributor Author

2color commented Mar 11, 2025

Yes, but with few more characters to include the libp2p-key codec in the check so we are more sure it's a CID.

This turned out to be rather confusing. I took a look at the string encoding of cids and I'm not sure I got the right prefix characters for base36/base32 CID encoded public keys.

@2color 2color requested a review from achingbrain March 11, 2025 16:04
@achingbrain achingbrain changed the title feat: add utility function to parse peer ids feat: support parsing base32 and base36 string encoded CIDs Mar 12, 2025
@2color 2color changed the title feat: support parsing base32 and base36 string encoded CIDs feat: support parsing base32 and base36 string encoded CIDs in peerIdFromString Mar 12, 2025
@2color 2color changed the title feat: support parsing base32 and base36 string encoded CIDs in peerIdFromString feat: support parsing base32 and base36 libp2p-key CIDs in peerIdFromString Mar 12, 2025
@2color 2color merged commit 0699fb7 into main Mar 12, 2025
47 of 53 checks passed
@2color 2color deleted the peer-cid-utility branch March 12, 2025 11:46
@achingbrain achingbrain mentioned this pull request Mar 12, 2025
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