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

[WIP] feat: adding p2p-circuit proto #41

Merged
merged 4 commits into from
Mar 27, 2017
Merged

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented Feb 26, 2017

needs tests

@dryajov dryajov changed the title feat: adding p2p-circuit proto [WIP] feat: adding p2p-circuit proto Feb 26, 2017
@daviddias daviddias added the status/ready Ready to be worked label Mar 9, 2017
@@ -42,7 +42,8 @@ Protocols.table = [
[477, 0, 'ws'],
[478, 0, 'wss'],
[275, 0, 'libp2p-webrtc-star'],
[276, 0, 'libp2p-webrtc-direct']
[276, 0, 'libp2p-webrtc-direct'],
[277, 0, 'p2p-circuit']
Copy link

Choose a reason for hiding this comment

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

The length should be V here

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We haven't, so far, considered multiaddrs that are 'pseudo-encapsulate' other multiaddrs as if the other multiaddrs are part of size of the one that 'pseudo-encapsulates'. See the webrtc-star example or even ws.

I can understand the reasoning, it can be open for discussion, but the parsing is not made for that case right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lgierth @diasdavid @whyrusleeping

After making the change to a V in [277, V, 'p2p-circuit'], odd number multiaddrs break, so a valid address such as /p2p-circuit/ipfs/QmDest is not a valid address anymore (at least in JS). What is the reason for making it a variable length addr?

Copy link

Choose a reason for hiding this comment

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

Sorry! For the record: 0 was correct!

@dryajov
Copy link
Member Author

dryajov commented Mar 11, 2017

Ah, great catch! Didn't see the table before. Should the proto name in the table be changed to /p2p-circuit for consistency?

@ghost
Copy link

ghost commented Mar 11, 2017

Oh actually yes, great catch too! Wanna take care of that?

@dryajov
Copy link
Member Author

dryajov commented Mar 11, 2017

Yep


bs58.decode(b58str)
return b58str
}
Copy link
Member

Choose a reason for hiding this comment

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

@daviddias daviddias merged commit 08d5d5d into multiformats:master Mar 27, 2017
@daviddias daviddias removed the status/ready Ready to be worked label Mar 27, 2017
@daviddias daviddias mentioned this pull request Mar 27, 2017
53 tasks
@dryajov dryajov mentioned this pull request Aug 16, 2017
48 tasks
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