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

Document MultiAddr codecs implemented by py-multiaddr #101

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ntninja
Copy link
Contributor

@ntninja ntninja commented Sep 1, 2019

Inspired by the Python code I wrote with lots of references to relevant specs.

@ntninja
Copy link
Contributor Author

ntninja commented Oct 3, 2019

@Stebalien @lgierth: Bump. What trick would I have to use to realistically get some discussion/feedback on this with everybody being so busy? 🙃

@ShadowJonathan
Copy link

Bump, I believe this is a useful addition to the repo.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

Encodes a libp2p node address.

TBD: Is this really always a base58btc encoded string of at least 5 characters in length!?
Copy link
Member

Choose a reason for hiding this comment

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

It's anything that parses as a peer ID (the canonical encoding is now actually a libp2p key CID): https://github.com/libp2p/specs/blob/master/peer-ids/peer-ids.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least the go implementation does not follow that spec at all but just expects everything to be base58btc: https://github.com/multiformats/go-multiaddr/blob/master/transcoders.go#L293-L315

If your saying that is just a bug to be fixed, I'll be happy to update this text and py-multiaddr accordingly. 🙂

Choose a reason for hiding this comment

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

That spec also fails to mention that some IDs are still encoded with 1 as first character, which is also CIDv0

Copy link
Member

Choose a reason for hiding this comment

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

@Alexander255 correct (well, not a bug so much as something we need to implement).

@ShadowJonathan also correct. I'll take a stab at fixing the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien: I just tried to implement that spec in Python and there is no guidance in there what to do with binary CIDv0 IDs. I assume they are detected (similar to their string format) using len(buf) == 34 and buf.startswith(b"\x12\x20")?

README.md Outdated Show resolved Hide resolved
@ntninja ntninja mentioned this pull request Jan 12, 2020
…ecified and add wording about path delimiter conversion for Windows
@ntninja
Copy link
Contributor Author

ntninja commented Aug 14, 2020

@Stebalien: I've updated the description of fspath to leave the binary encoding completely unspecified. Can we merge this now?

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.

None yet

3 participants