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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring libp2p-websocket-star to the Transports family! 馃専 #122

Merged
merged 8 commits into from Nov 27, 2017

Conversation

@mkg20001
Member

mkg20001 commented Sep 6, 2017

Adds tests for libp2p-websocket-star
Ref libp2p/js-libp2p-websocket-star#3

Show outdated Hide outdated circle.yml
@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 11, 2017

Member

Ok, I've gone through and give a big CR (with refactoring in place) to websocket-star. Overall there is a ton of work there and I'm really happy to see it coming together, thank you @mkg20001 馃憦馃徑馃憦馃徑馃憦馃徑馃憦馃徑!

The CR+Refactor can be found in:

To release and integrate websocket-star with confidence, recommending our users to try it out, I still want to see some issues solved and/or improved. I've opened issues in both repos so that it is clear what is missing, lists can be found in:

@mkg20001 is this something you would like to finish? I can continue helping by providing CR and coding time, just not sure how much time I can allocate that soon :)

Thanks!

Member

diasdavid commented Sep 11, 2017

Ok, I've gone through and give a big CR (with refactoring in place) to websocket-star. Overall there is a ton of work there and I'm really happy to see it coming together, thank you @mkg20001 馃憦馃徑馃憦馃徑馃憦馃徑馃憦馃徑!

The CR+Refactor can be found in:

To release and integrate websocket-star with confidence, recommending our users to try it out, I still want to see some issues solved and/or improved. I've opened issues in both repos so that it is clear what is missing, lists can be found in:

@mkg20001 is this something you would like to finish? I can continue helping by providing CR and coding time, just not sure how much time I can allocate that soon :)

Thanks!

@diasdavid diasdavid changed the title from test: add libp2p-websocket-star tests to Bring libp2p-websocket-star to the Transports family! 馃専 Sep 11, 2017

@diasdavid diasdavid referenced this pull request Sep 11, 2017

Closed

鈿★笍 v0.26.0 RELEASE 馃殌 #986

16 of 16 tasks complete
@mkg20001

This comment has been minimized.

Show comment
Hide comment
@mkg20001

mkg20001 Sep 11, 2017

Member

@mkg20001 is this something you would like to finish? I can continue helping by providing CR and coding time, just not sure how much time I can allocate that soon :)

Yes but I'm also working on https://github.com/ZeroNetJS/zeronet-js/ and my last year in school just began so I won't be working on this project that much

Member

mkg20001 commented Sep 11, 2017

@mkg20001 is this something you would like to finish? I can continue helping by providing CR and coding time, just not sure how much time I can allocate that soon :)

Yes but I'm also working on https://github.com/ZeroNetJS/zeronet-js/ and my last year in school just began so I won't be working on this project that much

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Sep 11, 2017

Member

I didn't know about zeronet-js with libp2p! That's rad! :D

Actually, this is more than rad! it is super-duper rad! Wanna show it working today at the IPFS All Hands? ipfs/pm#499

Member

diasdavid commented Sep 11, 2017

I didn't know about zeronet-js with libp2p! That's rad! :D

Actually, this is more than rad! it is super-duper rad! Wanna show it working today at the IPFS All Hands? ipfs/pm#499

@mkg20001

This comment has been minimized.

Show comment
Hide comment
@mkg20001

mkg20001 Sep 11, 2017

Member

Actually, this is more than rad! it is super-duper rad! Wanna show it working today at the IPFS All Hands?

After thinking about that for a while and talking with my parents I decided it would be a good idea. I'm usually a bit shy and this would be the first every meeting/conference/etc where I would participate. But I would like to participate. How can I join? How does that work?

Member

mkg20001 commented Sep 11, 2017

Actually, this is more than rad! it is super-duper rad! Wanna show it working today at the IPFS All Hands?

After thinking about that for a while and talking with my parents I decided it would be a good idea. I'm usually a bit shy and this would be the first every meeting/conference/etc where I would participate. But I would like to participate. How can I join? How does that work?

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Oct 23, 2017

Member

@dryajov @VictorBjelkholm can I get your review here as well?

Member

diasdavid commented Oct 23, 2017

@dryajov @VictorBjelkholm can I get your review here as well?

mkg20001 added some commits Sep 6, 2017

@wafflebot wafflebot bot added in progress and removed ready labels Oct 28, 2017

@mkg20001

This comment has been minimized.

Show comment
Hide comment
@mkg20001

mkg20001 Oct 28, 2017

Member

Rebased all changes against current master and released stable versions of the libp2p-websocket-star* modules.
Merge?

Member

mkg20001 commented Oct 28, 2017

Rebased all changes against current master and released stable versions of the libp2p-websocket-star* modules.
Merge?

@dryajov

LGTM

Show outdated Hide outdated .aegir.js
Show outdated Hide outdated .aegir.js
Show outdated Hide outdated package.json
Show outdated Hide outdated test/browser-bundle/browser-bundle.js
Show outdated Hide outdated test/nodejs-bundle/tcp+websockets+websocket-star.js
Show outdated Hide outdated test/nodejs-bundle/tcp+websockets+websocket-star.js
Show outdated Hide outdated test/nodejs-bundle/tcp+websockets+websocket-star.js
Show outdated Hide outdated test/nodejs-bundle/tcp+websockets+websocket-star.js
@Beanow

This comment has been minimized.

Show comment
Hide comment
@Beanow

Beanow Nov 16, 2017

Can you provide a configuration you've tested this with in js-ipfs?

In my tests to implement this I found some odd issues with the multi addresses.
Using a test server,

config: {
  Addresses: {
    Swarm: [
      '/dns4/ws-star-signal-2.servep2p.com/wss/p2p-websocket-star/'
    ]
  }
}

My logs output:

Swarm listening on /dns4/ws-star-signal-2.servep2p.com/wss/p2p-websocket-star/ipfs/QmcdoJPn4CStGjJd58XQE3GfLZQHSshMovhoGMNFoHd9px/ipfs/QmcdoJPn4CStGjJd58XQE3GfLZQHSshMovhoGMNFoHd9px

And it does not successfully connect with any peers over websocket star.

When I add this to src/core/runtime/libp2p-browser.js

peerInfo.multiaddrs.clear()
peerInfo.multiaddrs.add(multiaddr('/dns4/ws-star-signal-1.servep2p.com/wss/p2p-websocket-star/'))
const wsstar = new WSStar({id: peerInfo.id})

It will do peer discovery, bitswap and pubsub correctly.

Beanow commented Nov 16, 2017

Can you provide a configuration you've tested this with in js-ipfs?

In my tests to implement this I found some odd issues with the multi addresses.
Using a test server,

config: {
  Addresses: {
    Swarm: [
      '/dns4/ws-star-signal-2.servep2p.com/wss/p2p-websocket-star/'
    ]
  }
}

My logs output:

Swarm listening on /dns4/ws-star-signal-2.servep2p.com/wss/p2p-websocket-star/ipfs/QmcdoJPn4CStGjJd58XQE3GfLZQHSshMovhoGMNFoHd9px/ipfs/QmcdoJPn4CStGjJd58XQE3GfLZQHSshMovhoGMNFoHd9px

And it does not successfully connect with any peers over websocket star.

When I add this to src/core/runtime/libp2p-browser.js

peerInfo.multiaddrs.clear()
peerInfo.multiaddrs.add(multiaddr('/dns4/ws-star-signal-1.servep2p.com/wss/p2p-websocket-star/'))
const wsstar = new WSStar({id: peerInfo.id})

It will do peer discovery, bitswap and pubsub correctly.

@mkg20001

This comment has been minimized.

Show comment
Hide comment
@mkg20001

mkg20001 Nov 19, 2017

Member

Try it with https://github.com/mkg20001/js-ipfs/tree/feat-websocketstar or apply this patch and see if it works (edit: works for me)

Member

mkg20001 commented Nov 19, 2017

Try it with https://github.com/mkg20001/js-ipfs/tree/feat-websocketstar or apply this patch and see if it works (edit: works for me)

@patrickdet

This comment has been minimized.

Show comment
Hide comment
@patrickdet

patrickdet Nov 26, 2017

The only annoying thing is that now it is not possible to configure the libp2p transport with the WebSocketStar from the configuration like:

  const ipfs = new Ipfs({
    libp2p: {
      modules: {
        transport: [new WebSocketStar({id: peerInfo.id})],
        discovery: [new WebSocketStar({id: peerInfo.id})]
      }
    }
  })

because in that scenario the peerInfo is not available yet. Not sure if it is critical to have that though.

I'd really like to have the WebSocketStar available in Ipfs though, because the WebRTC keeps crashing my application.

patrickdet commented Nov 26, 2017

The only annoying thing is that now it is not possible to configure the libp2p transport with the WebSocketStar from the configuration like:

  const ipfs = new Ipfs({
    libp2p: {
      modules: {
        transport: [new WebSocketStar({id: peerInfo.id})],
        discovery: [new WebSocketStar({id: peerInfo.id})]
      }
    }
  })

because in that scenario the peerInfo is not available yet. Not sure if it is critical to have that though.

I'd really like to have the WebSocketStar available in Ipfs though, because the WebRTC keeps crashing my application.

@mkg20001

This comment has been minimized.

Show comment
Hide comment
@mkg20001

mkg20001 Nov 26, 2017

Member

You can use it without peer id and use this signalling server instead: /dns/ws-star-signal-4.servep2p.com/wss/p2p-websocket-star/. note that it's not as secure as the other servers (no crypto challenge)

Member

mkg20001 commented Nov 26, 2017

You can use it without peer id and use this signalling server instead: /dns/ws-star-signal-4.servep2p.com/wss/p2p-websocket-star/. note that it's not as secure as the other servers (no crypto challenge)

@diasdavid diasdavid referenced this pull request Nov 26, 2017

Closed

鈿★笍 v0.27.0 RELEASE 馃殌 #1095

24 of 24 tasks complete
@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Nov 26, 2017

Member

Let's focus on figuring out #130 to get that PeerId on the transport. Until then let's move with the not as secure, also not yet audited version.

Member

diasdavid commented Nov 26, 2017

Let's focus on figuring out #130 to get that PeerId on the transport. Until then let's move with the not as secure, also not yet audited version.

Show outdated Hide outdated .aegir.js
Show outdated Hide outdated .aegir.js
Show outdated Hide outdated .aegir.js
Show outdated Hide outdated .aegir.js
Show outdated Hide outdated test/nodejs-bundle/tcp+websockets+webrtc-star.js
Show outdated Hide outdated test/nodejs-bundle/tcp+websockets+webrtc-star.js
Show outdated Hide outdated test/nodejs-bundle/tcp+websockets+websocket-star.js
@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid Nov 26, 2017

Member

@mkg20001 thanks for pushing this :) I've made some comments. I am happy to finish this one tomorrow early morning if you don't have time until then. Thank you!

Member

diasdavid commented Nov 26, 2017

@mkg20001 thanks for pushing this :) I've made some comments. I am happy to finish this one tomorrow early morning if you don't have time until then. Thank you!

diasdavid added some commits Nov 27, 2017

@diasdavid diasdavid merged commit 95f029e into master Nov 27, 2017

2 of 5 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@diasdavid diasdavid deleted the libp2p-websocket-star branch Nov 27, 2017

@wafflebot wafflebot bot removed the in progress label Nov 27, 2017

@mitra42

This comment has been minimized.

Show comment
Hide comment
@mitra42

mitra42 Nov 28, 2017

I see a number of configurations posted as not working above, is there a conclusion of a configuration that works ? Could it be posted here.

mitra42 commented Nov 28, 2017

I see a number of configurations posted as not working above, is there a conclusion of a configuration that works ? Could it be posted here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment