-
Notifications
You must be signed in to change notification settings - Fork 9
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: add direct relay-rtc example #260
Conversation
I'm aware it's just a draft PR but came in to say it's probably best to use a framework for better readability moving forward Excited for the progress on this one! |
examples/relay-chat/index.js
Outdated
c.remotePeer.toString() | ||
); | ||
const isWebRTCOutbound = | ||
outboundStream.rawStream.constructor.name === "WebRTCStream"; |
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.
afaik this is the only way to know what type of stream is established in the mesh
examples/relay-chat/index.js
Outdated
} | ||
|
||
node.relay.gossipSub.streamsOutbound.delete(c.remotePeer.toString()); | ||
await node.relay.gossipSub.createOutboundStream( |
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.
This is the main problem with the current approach, due to how WebRTC
transport is made it is able only to create outbound
connection using WebRTCStream
but inbound
stays MplexStream
which is wrong, and prevents the mesh from functioning over WebRTC
transport.
Anyway we don't intend to limit ourselves to the libp2p
's WebRTC transport and we need a different solution on which I am working: it is to establish WebRTC
connection similarly to how we do it in noise-rtc by using contentTopics
. Main thing that makes everything ugly here is to manually put streams and connections into gossipSub
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.
but inbound stays MplexStream which is wrong, and prevents the mesh from functioning over WebRTC transport.
Is that a libp2p bug?
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.
I would say it is gossipSub
's bug since it is failing to upgrade inbound
connection from mplex to webrtc due to the condition explicitly skipping it and it happens to be that webrtc transport starts as mplex and then get's upgraded
I am looking at how to fix it in js-libp2p-gossipsub
but I consider it to be a low-priority
For this one, I intentionally used chat template that we have since it is pretty straightforward but let's discuss how it can be improved so we can have event better WoW effect for the next example I am preparing
|
examples/relay-chat/index.html
Outdated
<div> | ||
<input | ||
id="remoteNode" | ||
value="/dns4/node-01.ac-cn-hongkong-c.go-waku.prod.statusim.net/tcp/443/wss/p2p/16Uiu2HAm1fVVprL9EaDpDw4frNX3CPfZu5cBqhtk9Ncm6WCvprpv" |
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.
ah so actually we may not need to run our own go-waku node, right?
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.
yes, the main criteria is to have circuit-relay
implemented on nodes and as of now it only works with go-waku
nodes in prod fleet, the guide is to make sure folks know what to do in case it does not work due to low capacity or something
examples/relay-chat/index.js
Outdated
} | ||
|
||
node.relay.gossipSub.streamsOutbound.delete(c.remotePeer.toString()); | ||
await node.relay.gossipSub.createOutboundStream( |
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.
but inbound stays MplexStream which is wrong, and prevents the mesh from functioning over WebRTC transport.
Is that a libp2p bug?
47f2ca8
to
fbc2aa0
Compare
general comment: can we add details of this works in the description/README? there is mention of how to start and navigate the example, but we should add what is happening with this example theoretically:
This would also help with the review process :D |
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.
Note that you can use mermaidsyntax in GitHub comments/description for sequence diagram. Maybe would be better to have Alice, Bob, Carol in the diagram than tab#N.
.github/workflows/ci.yml
Outdated
@@ -20,6 +20,7 @@ jobs: | |||
web-chat, | |||
noise-js, | |||
noise-rtc, | |||
relay-direct-chat |
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.
Hum, maybe relay-webrtc
would be a better name?
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.
Actually, I named it like this first but I hope to push for the second example with gossiping discovery and call it relay-webrtc
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.
thanks for this effort!
did a first review
general comment: code feels hard to read and like a challenge to maintain. imo readability is of bigger importance considering this is an example for end users.
i would incline to refactor it for the following:
- break down
initWakuContext
into functions likesetupMessageHandlers
,configureWaku
, etc - functions like
initUI
mix both UI initialisation and event handling -- consider separating them - we could also go ahead and group similar functionalities together:
- functions related to message handling can be together, UI related can be together, etc
- modularising the code: breaking down components into modules like
ui.js
,waku.js
, etc - adding some comments/docs on functions that might be hard to understand for the user at first like
setRelayMeshInfo
,dropNetworkConnections
,dialWebRTCpeer
,onExit
cc @LordGhostX how do you feel?
examples/relay-direct-chat/index.js
Outdated
const multiaddr = webrtcPeer.value; | ||
|
||
if (!multiaddr) { | ||
throw Error("No multiaddr to dial webrtc"); |
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.
if the input box is empty, we should prompt the user to enter a multiaddr instead of throw
ing
examples/relay-direct-chat/index.js
Outdated
ui.onExit(async () => { | ||
ui.setStatus("disconnecting...", "progress"); | ||
await unsubscribeFromMessages(); | ||
ui.setStatus("disconnected", "terminated"); | ||
ui.resetMessages(); | ||
}); |
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.
during this, let's also make sure to clear attached event listeners to avoid potential memory leaks
examples/relay-direct-chat/index.js
Outdated
}); | ||
} | ||
|
||
async function initWakuContext({ ui, contentTopic }) { |
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.
this seems to be a fairly long function (150 lines); it would help to break it down into multiple functions
also might help keep code more maintainable & readable to break off code/functions into multiple files
examples/relay-direct-chat/index.js
Outdated
} | ||
|
||
// UI adapter | ||
function initUI() { |
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.
it will help to encapsulate this function into a separate file/UI class for better readability & maintainability
I disagree there @danisharora099 . This is a PoC to demonstrate possible usage of WebRTC for Waku Relay. Should we just have a different repo for PoCs? |
I incline towards aiming to have our POCs as readable as our examples as it doesn't take much from our side; and can serve similar purposes |
Why not just move the PoCs to a different folder and then have them as Then they are all in the same repo, but it is clear(er) that they may not be fully fledged examples to use for your app |
As discussed last week - the following improvement will be introduced in root template here #271 As for if we need to split PoC and exmapels I think it's not needed yet. |
FYI, Comms Hubs was keen on the usage of |
Decoupled this conv to separate issue #275 |
c9fea5f
to
2fdd20f
Compare
A new example here is a showcase of how a client that has only WebRTC connection can communicate with a third client through an intermediary by using Waku Relay.
Such an approach enables us to: