-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
@ddimaria Still waiting on resolution for protocol name and code libp2p/specs#497 (comment) |
@ckousik do I understand correctly that these discussions are not blocking this pull request? Or is all other work done? |
@mxinden Chinmay is out for the rest of the week. I can confirm that the spec-related decisions are not blocking this PR. I plan on adding a browser-to-browser example to the |
src/peer_transport/transport.ts
Outdated
|
||
rawStream.close() | ||
void connection.close() | ||
// TODO(ckousik): Remove this delay. This is required because |
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.
@achingbrain For further clarification, consider a case where A
is connecting to B
using webrtc-direct
. A
and B
both complete SDP exchange and their PeerConnections transition to the connected
state. For the ICE connection, A
is the controlled peer and B
is the controlling peer. With reference to the description here, A
moves to the connected state before B
. This allows A
to create a stream on the new connection before B
has a chance to set up the datachannel callbacks.
|
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.
Minor nits, but overall looks good to me. I'll leave the final approval to @achingbrain
Great work @ckousik :)
examples/browser-to-browser/index.js
Outdated
appendOutput(`Dialing '${ma}'`) | ||
const connection = await node.dial(ma) | ||
|
||
if (!ma.protoCodes().includes(276)) { |
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.
276? Let's not forget to change this.
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.
276 is the protoCode for circuit-relay. This is just used to differentiate between dialing a relay peer to start listening vs dialing a peer over relay to create an echo stream.
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.
is there a library where we can ensure the protocode is more self documenting? (similar to codes in dag-json, dag-cbor, etc.. from ipld) we shouldn't be throwing magic numbers around
) | ||
|
||
func main() { | ||
makeRelayV1() |
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.
Why not relayV2?
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.
We haven't landed support for circuit v2 in js yet
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.
For the record, tracking issue libp2p/js-libp2p#1029 and pull request libp2p/js-libp2p#1533.
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.
These are in now, can we update this?
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.
bump: please update to relay v2 (and the latest go-libp2p release)
src/peer_transport/listener.ts
Outdated
} | ||
|
||
private listeningAddrs: Multiaddr[] = [] | ||
async listen (ma: Multiaddr): Promise<void> { |
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 need this? Aren't we listening already via some relay?
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.
Probably not, but the rationale to add this is as follows:
- Since webrtc-direct is a protocol, we can advertise support over any transport.
- For running on node, we can use
node-webrtc
to replace all instances ofRTCPeerConnection
(webrtc-star and webrtc-direct currently do this).
Although, an argument can be made that any listener can support webrtc-direct since it is only implemented as a protocol.
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.
Some small nits. I'll fix up anything left and merge this soon. 🎉
examples/browser-to-browser/index.js
Outdated
import { noise } from "@chainsafe/libp2p-noise" | ||
|
||
// singletons | ||
let outgoing_stream |
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.
Sorry, I should have been more clear. This shouldn't be a singleton because
there's no benefit of it being so. I'm not sure you could use this in a correct
way as a singleton anyways, since once you pass it into pipe
you don't own
this stream. Since this is an example, the risk that people copy this verbatim
is pretty high. so we should remove this. I'll push a change.
return | ||
} | ||
|
||
node.getMultiaddrs().forEach((ma) => { |
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.
Why do you need to remove an address?
Merging this since I've given it a review. I'm sure there's more things to fix, but I'd rather do those incrementally from now on. @achingbrain feel to review this at a later time |
## [1.1.0](v1.0.5...v1.1.0) (2023-04-07) ### Features * Browser to Browser ([#90](#90)) ([add5c46](add5c46))
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adds the browser to browser transport. Supersedes #88