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

gs1.1 direct connect #99

Merged
merged 1 commit into from
Jul 2, 2020
Merged

gs1.1 direct connect #99

merged 1 commit into from
Jul 2, 2020

Conversation

wemeetagain
Copy link
Member

@wemeetagain wemeetagain commented Jun 30, 2020

@vasco-santos
Copy link
Collaborator

All js-libp2p interfaces collected into ts/interfaces.ts (This should likely be refactored away into a separate library eventually, eg: into js-libp2p directly or chainsafe/libp2p-ts. But for now, its extremely convenient to tweak the interfaces as needed all in one place.)

I am ok on having this for now. But I would like to have us to generate the types declarations via js docs with aegir

gossipsub constructor signature changed significantly, now (libp2p: Libp2p, options: object)
This will require a significant rewrite of all existing tests, at least the sections that create new gossipsub objects. I've begun to do this, adding libp2p, libp2p-websockets, libp2p-noise, libp2p-mplex as dev dependencies. Unfortunately, I've run into issues with browser compatability, so just decided to push what I have for now. Hopefully we can discuss and figure out how to proceed.

You will need to have a signal server for the tests. Check it out this example:

This example allows us to test browser considering that a browser node will connect to it directly. I would recommend that we setup here this kind of server but with the server being a circuit relay. We would spin up n nodes, that would listen on the circuit relay peer for new connections from the other peers. Then, we can have configuration files for browser and node like here: https://github.com/libp2p/js-libp2p/tree/master/test/utils , but I would also have a function there to compute the listen multiaddrs to use. That is, we do not need to connect via the relay in node, just when running browser tests.
What do you think?

Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Maybe a new PR just adding libp2p as a parameter to have the tests good before getting this in?

ts/pubsub.js Outdated
peerId,
registrar,
peerId: libp2p.peerId,
registrar: libp2p.registrar,
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we refactor pubsub, I think we should also send libp2p to it. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

for sure 👍

@wemeetagain
Copy link
Member Author

Maybe a new PR just adding libp2p as a parameter to have the tests good before getting this in?

Yeah, I'll split this up into two PRs:

first collecting interfaces and adding libp2p as a parameter, including passing tests,
second adding direct peer connections

@vasco-santos
Copy link
Collaborator

vasco-santos commented Jul 1, 2020

@wemeetagain
Copy link
Member Author

Ok, awesome, I'll give it all a shot. Thanks for the examples, super helpful.

@wemeetagain wemeetagain mentioned this pull request Jul 2, 2020
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2020

Codecov Report

Merging #99 into gsv1.1 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           gsv1.1      #99   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           2        2           
  Lines          24       24           
=======================================
  Hits           22       22           
  Misses          2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b040dc8...7103b83. Read the comment docs.

Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM!

@wemeetagain wemeetagain merged commit b77e2f3 into gsv1.1 Jul 2, 2020
@wemeetagain wemeetagain deleted the cayman/gs1.1-direct-connect branch July 2, 2020 18:07
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.

None yet

3 participants