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

feat: integrate gossipsub by default #19

Merged
merged 8 commits into from
Aug 12, 2019

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented May 24, 2019

This PR modified the configuration of a libp2p node, in order to allow the selection of the pubsub implementation to use.

Also, Gossipsub was added as the default pubsub implementation.

Needs:

@vasco-santos
Copy link
Member Author

cc @wemeetagain @Mikerah

src/libp2p.js Outdated Show resolved Hide resolved
@jacobheun
Copy link
Contributor

There are some unrelated failures in CI. This should be gtg once deps are updated.

@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch 2 times, most recently from eb94b69 to eeb0f98 Compare July 15, 2019 09:22
@vasco-santos vasco-santos marked this pull request as ready for review July 15, 2019 10:00
@jacobheun jacobheun force-pushed the feat/integrate-gossipsub-by-default branch from eeb0f98 to 18b46df Compare July 24, 2019 09:42
@jacobheun
Copy link
Contributor

@vasco-santos fyi, I rebased with master to get the latest test fixes here.

@vasco-santos
Copy link
Member Author

Thanks @jacobheun

@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from 18b46df to 998fd42 Compare July 30, 2019 19:15
@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from 998fd42 to 46d0c18 Compare July 31, 2019 08:28
@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from 6f728e5 to 8acc4bd Compare July 31, 2019 14:13
@vasco-santos
Copy link
Member Author

@jacobheun updated this with the newest release candidate of js-libp2p.

I could remove the pubsub wrapper for promises, which was great. However, I had to do minor changes to start and dial. Basically, with the initial step for promises with https://github.com/libp2p/js-libp2p/blob/master/src/index.js#L191-L196 was overwriting the start and dial and I replaced them with _start and _ dial for now. Once we fully move to promises, we can remove this, what do you think?

@jacobheun
Copy link
Contributor

@vasco-santos if that is breaking the sub class we need to fix js-libp2p. If other users are extending the libp2p class that's going to be a huge pain for them.

@vasco-santos
Copy link
Member Author

I was thinking about it earlier. I don't know how common it is to extend the libp2p class and override start, dial, ... But anyway, we should avoid causing those problems.

The easier solution I see now is to change all the following functions: 'start', 'stop', 'dial', 'dialProtocol', 'dialFSM', 'hangUp', 'ping'. We need to check if we receive a callback and return a Promise if not. We will need to add a lot of spaghetti code though. What do you think?

@jacobheun
Copy link
Contributor

@vasco-santos I think libp2p/js-libp2p#394 should resolve the problem.

@vasco-santos
Copy link
Member Author

Thanks, I will try that here! But seems to fix the issue

@vasco-santos
Copy link
Member Author

It worked 🎉

@jacobheun
Copy link
Contributor

🎉just working on 1 last PR for libp2p and then I'll do another RC release

@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from 8acc4bd to b257a2a Compare August 1, 2019 14:46
@jacobheun
Copy link
Contributor

v0.26.0-rc.2 is on the web

@vasco-santos vasco-santos force-pushed the feat/integrate-gossipsub-by-default branch from b257a2a to 710c192 Compare August 1, 2019 15:29
@vasco-santos
Copy link
Member Author

Updated here! Thanks @jacobheun

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Updated to the latest release of libp2p, this is gtg!

@jacobheun jacobheun merged commit 2959fc8 into master Aug 12, 2019
@jacobheun jacobheun deleted the feat/integrate-gossipsub-by-default branch August 12, 2019 15:13
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.

2 participants