-
Notifications
You must be signed in to change notification settings - Fork 473
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
Bring libp2p-websocket-star to the Transports family! 🌟 #122
Changes from 2 commits
614ec03
0e2793f
daf5b69
fe776a5
47fdbdd
baa560a
0807954
724fe5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,9 @@ const PeerId = require('peer-id') | |
const pull = require('pull-stream') | ||
|
||
const sigServer = require('libp2p-webrtc-star/src/sig-server') | ||
const wsRendezvous = require('libp2p-websocket-star-rendezvous') | ||
let server | ||
let server2 | ||
|
||
let node | ||
const rawPeer = require('./test/browser-bundle/peer.json') | ||
|
@@ -15,14 +17,28 @@ const before = (done) => { | |
let count = 0 | ||
const ready = () => ++count === 2 ? done() : null | ||
|
||
sigServer.start({ port: 15555 }, (err, _server) => { | ||
sigServer.start({ | ||
port: 15555 | ||
}, (err, _server) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary style change. |
||
if (err) { | ||
throw err | ||
} | ||
server = _server | ||
ready() | ||
}) | ||
|
||
wsRendezvous.start({ | ||
port: 14444, | ||
refreshPeerListIntervalMS: 1000, | ||
strictMultiaddr: false | ||
}, (err, _server) => { | ||
if (err) { | ||
throw err | ||
} | ||
server2 = _server | ||
ready() | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mkg20001 mind putting the two tasks, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are already parallel... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, to make it more structured with Parallel. I also see Ready being called 3 times and only checked for 2 |
||
|
||
PeerId.createFromJSON(rawPeer, (err, peerId) => { | ||
if (err) { | ||
return done(err) | ||
|
@@ -38,12 +54,7 @@ const before = (done) => { | |
} | ||
|
||
const after = (done) => { | ||
setTimeout(() => node.stop((err) => { | ||
if (err) { | ||
return done(err) | ||
} | ||
server.stop(done) | ||
}), 2000) | ||
setTimeout(() => require('async/each')([node, server, server2], (s, n) => s.stop(n), done), 2000) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a) Please do not require modules throughout the codebase, always require it at the top. |
||
} | ||
|
||
module.exports = { | ||
|
@@ -52,4 +63,3 @@ module.exports = { | |
post: after | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,8 @@ | |
"libp2p-tcp": "~0.11.1", | ||
"libp2p-webrtc-star": "~0.13.2", | ||
"libp2p-websockets": "~0.10.4", | ||
"libp2p-websocket-star": "~0.5.0", | ||
"libp2p-websocket-star-rendezvous": "~0.2.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for keeping the |
||
"lodash.times": "^4.3.2", | ||
"pre-commit": "^1.2.2", | ||
"pull-goodbye": "0.0.2", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
const WS = require('libp2p-websockets') | ||
const WebRTCStar = require('libp2p-webrtc-star') | ||
const WebSocketStar = require('libp2p-websocket-star') | ||
const spdy = require('libp2p-spdy') | ||
const multiplex = require('libp2p-multiplex') | ||
const secio = require('libp2p-secio') | ||
|
@@ -36,11 +37,13 @@ class Node extends libp2p { | |
constructor (peerInfo, peerBook, options) { | ||
options = options || {} | ||
const webRTCStar = new WebRTCStar() | ||
const wsStar = new WebSocketStar({ id: peerInfo.id }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it still require that the PeerInfo gets passed on the constructor? Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the README of ws-star it says: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this requierment to have the
because the peer id is not available at that point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How else can the transport get the id? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first question should be "why does this Id exist in the first place", if:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two things here:
b) is a bit more complicated because you want to peers to prove who they are with the rendezvous point. I'm torn that one the best solution is through PeerId because that really breaks the transport interface expectation. We also want to move away from centralized rendezvous points and just make any node be able to do that, which kind of means that a transport will need access to libp2p itself. Once we end up doing this, it might lead to changing how modules are loaded to a DI scheme. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are now 5 possible solutions:
Which one would be the best? (I prefer the first one) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @diasdavid The peerId IS accesible at the time of transport creation. No need to use the insecure version. https://github.com/ipfs/js-ipfs/compare/master...mkg20001:master.diff There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @diasdavid Insecure server running at |
||
|
||
const modules = { | ||
transport: [ | ||
new WS(), | ||
webRTCStar | ||
webRTCStar, | ||
wsStar | ||
], | ||
connection: { | ||
muxer: getMuxers(options.muxer), | ||
|
@@ -55,6 +58,10 @@ class Node extends libp2p { | |
modules.discovery.push(webRTCStar.discovery) | ||
} | ||
|
||
if (options.wsStar) { | ||
modules.discovery.push(wsStar.discovery) | ||
} | ||
|
||
if (options.bootstrap) { | ||
const r = new Railing(options.bootstrap) | ||
modules.discovery.push(r) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
/* eslint-env mocha */ | ||
'use strict' | ||
|
||
const chai = require('chai') | ||
chai.use(require('dirty-chai')) | ||
const expect = chai.expect | ||
const PeerInfo = require('peer-info') | ||
const PeerId = require('peer-id') | ||
const parallel = require('async/parallel') | ||
const pull = require('pull-stream') | ||
|
||
const Node = require('./browser-bundle') | ||
|
||
describe('libp2p-ipfs-browser (websocket-star only)', () => { | ||
let peer1 | ||
let peer2 | ||
let node1 | ||
let node2 | ||
|
||
it('create two peerInfo with websocket-star addrs', (done) => { | ||
parallel([ | ||
(cb) => PeerId.create({ bits: 1024 }, cb), | ||
(cb) => PeerId.create({ bits: 1024 }, cb) | ||
], (err, ids) => { | ||
expect(err).to.not.exist() | ||
|
||
peer1 = new PeerInfo(ids[0]) | ||
const ma1 = '/ip4/127.0.0.1/tcp/14444/ws/p2p-websocket-star/' | ||
peer1.multiaddrs.add(ma1) | ||
|
||
peer2 = new PeerInfo(ids[1]) | ||
const ma2 = '/ip4/127.0.0.1/tcp/14444/ws/p2p-websocket-star/' | ||
peer2.multiaddrs.add(ma2) | ||
|
||
done() | ||
}) | ||
}) | ||
|
||
it('create two libp2p nodes with those peers', (done) => { | ||
node1 = new Node(peer1, null, { wsStar: true }) | ||
node2 = new Node(peer2, null, { wsStar: true }) | ||
done() | ||
}) | ||
|
||
it('listen on the two libp2p nodes', (done) => { | ||
parallel([ | ||
(cb) => node1.start(cb), | ||
(cb) => node2.start(cb) | ||
], done) | ||
}) | ||
|
||
it('handle a protocol on the first node', () => { | ||
node2.handle('/echo/1.0.0', (protocol, conn) => pull(conn, conn)) | ||
}) | ||
|
||
it('dial from the second node to the first node', (done) => { | ||
node1.dial(peer2, '/echo/1.0.0', (err, conn) => { | ||
expect(err).to.not.exist() | ||
setTimeout(check, 500) | ||
|
||
function check () { | ||
const text = 'hello' | ||
const peers1 = node1.peerBook.getAll() | ||
expect(Object.keys(peers1)).to.have.length(1) | ||
|
||
const peers2 = node2.peerBook.getAll() | ||
expect(Object.keys(peers2)).to.have.length(1) | ||
|
||
pull( | ||
pull.values([Buffer.from(text)]), | ||
conn, | ||
pull.collect((err, data) => { | ||
expect(err).to.not.exist() | ||
expect(data[0].toString()).to.equal(text) | ||
done() | ||
}) | ||
) | ||
} | ||
}) | ||
}) | ||
|
||
it('node1 hangUp node2', (done) => { | ||
node1.hangUp(peer2, (err) => { | ||
expect(err).to.not.exist() | ||
setTimeout(check, 500) | ||
|
||
function check () { | ||
const peers = node1.peerBook.getAll() | ||
expect(Object.keys(peers)).to.have.length(1) | ||
expect(Object.keys(node1.swarm.muxedConns)).to.have.length(0) | ||
done() | ||
} | ||
}) | ||
}) | ||
|
||
it('create a third node and check that discovery works', (done) => { | ||
let counter = 0 | ||
|
||
function check () { | ||
if (++counter === 3) { | ||
expect(Object.keys(node1.swarm.muxedConns).length).to.equal(1) | ||
expect(Object.keys(node2.swarm.muxedConns).length).to.equal(1) | ||
done() | ||
} | ||
} | ||
|
||
PeerId.create((err, id3) => { | ||
expect(err).to.not.exist() | ||
|
||
const peer3 = new PeerInfo(id3) | ||
const ma3 = '/ip4/127.0.0.1/tcp/14444/ws/p2p-websocket-star/ipfs/' + id3.toB58String() | ||
peer3.multiaddrs.add(ma3) | ||
|
||
node1.on('peer:discovery', (peerInfo) => node1.dial(peerInfo, check)) | ||
node2.on('peer:discovery', (peerInfo) => node2.dial(peerInfo, check)) | ||
|
||
const node3 = new Node(peer3, null, { wsStar: true }) | ||
node3.start(check) | ||
}) | ||
}) | ||
}) |
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.
Since we now have two "servers", let's use qualifying names: