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

[WIP] Integrating libp2p into Lodestar #154

Merged
merged 32 commits into from
May 2, 2019

Conversation

Mikerah
Copy link
Contributor

@Mikerah Mikerah commented Apr 9, 2019

This is currently a work in progress and breaks tests.

I have started integrating libp2p into Lodestar. A simple libp2p bundle has been created and is subject to change as more components get built out.

Here's a current TODO list:

  • Have a working basic server for managing peers

  • (Optional) Change part of the service API

  • Add RPC over libp2p to start integrating the ETH2.0 Wire Protocol

  • Add BDD tests to make sure networking is fine.

@Mikerah Mikerah requested a review from wemeetagain April 9, 2019 02:49
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2019

CLA assistant check
All committers have signed the CLA.

@GregTheGreek
Copy link
Member

Just quickly glanced, we should be probably importing a libp2p library rather than a bunch of individual ones?

@Mikerah
Copy link
Contributor Author

Mikerah commented Apr 9, 2019

@GregTheGreek The way it works is that if you don't need any specialized transports, multiplexers, etc and you can use the ones provided by libp2p, you need to add them to the project individually. The one that might be optional is FloodSub but I still imported so that when GossipSub is done, I can just replace the import.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

very exciting, starting to take shape :)

src/p2p/index.ts Outdated Show resolved Hide resolved
src/p2p/index.ts Outdated Show resolved Hide resolved
src/p2p/node.ts Outdated Show resolved Hide resolved
src/p2p/node.ts Outdated Show resolved Hide resolved
src/p2p/index.ts Outdated Show resolved Hide resolved
@wemeetagain
Copy link
Member

@GregTheGreek I think this is as it should be re: importing libraries for now. libp2p takes a veeery modular approach, and we need all of these libraries to weave everything together properly. In fact, we probably will be adding even more libp2p libraries in the future!

Its probably easier to start here, monorepo style, and once we've got things more solid, pull out into a separate repo/ repos, if it makes sense.

@GregTheGreek
Copy link
Member

@wemeetagain Fair enough, I iguess we have a lot to trimfrom the package.json anyways.

@wemeetagain
Copy link
Member

@Mikerah would you be willing to do minor cleanup and plan on merging as is?
It would probably be better to just have the code in master and iterate from there.
I think your TODOs could be reintroduced as issues for further PRs.

@Mikerah
Copy link
Contributor Author

Mikerah commented Apr 12, 2019

@wemeetagain I can do minor cleanups and separate the TODO as separate PRs. My main bottleneck right now is that all the tests break due to odd compilation issues. Before merging, I would like to confirm that this is just an issue on my end and not everybody else's.

@wemeetagain
Copy link
Member

I think what you're seeing is a result of our check-types script.
Before running tests, we run the typescript compiler (without output) just for it to do some checks and catch any type errors.
It's very picky and will catch many small issues.
You'd probably want to just go through them, one by one.

If you're using an IDE (or vim w/ plugins like typescript-vim and YouCompleteMe), it may be much easier, because the errors will be called out in the editor and potentially can be auto-fixed.

@Mikerah
Copy link
Contributor Author

Mikerah commented Apr 22, 2019

The basics are in here and I will be opening subsequent PRs. @mpetrunic got the types working.

package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 1, 2019

Codecov Report

Merging #154 into master will decrease coverage by 20.5%.
The diff coverage is 22.66%.

@@             Coverage Diff             @@
##           master     #154       +/-   ##
===========================================
- Coverage   52.79%   32.29%   -20.51%     
===========================================
  Files          85       47       -38     
  Lines        1305      737      -568     
  Branches      123       64       -59     
===========================================
- Hits          689      238      -451     
+ Misses        590      499       -91     
+ Partials       26        0       -26

@Mikerah
Copy link
Contributor Author

Mikerah commented May 1, 2019

The build is now passing. Awaiting approval...

wemeetagain
wemeetagain previously approved these changes May 1, 2019
@Mikerah Mikerah dismissed GregTheGreek’s stale review May 1, 2019 03:50

Updates have been applied

Copy link
Member

@GregTheGreek GregTheGreek left a comment

Choose a reason for hiding this comment

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

Few small async based comments, but also theres a lot of callback hell going on, using promisify would really help

src/p2p/index.ts Outdated Show resolved Hide resolved
src/p2p/index.ts Outdated Show resolved Hide resolved
src/p2p/index.ts Outdated Show resolved Hide resolved
src/p2p/node.ts Outdated Show resolved Hide resolved
@Mikerah
Copy link
Contributor Author

Mikerah commented May 1, 2019

@GregTheGreek I think leaving the waterfall in createNode is fine. This function won't be changing much in the future. I also think that waterfall is more readable than promisify in the way it's currently coded. However, I found this issue that seems like it's a compromise between both ways.

@mpetrunic
Copy link
Member

@GregTheGreek I think leaving the waterfall in createNode is fine. This function won't be changing much in the future. I also think that waterfall is more readable than promisify in the way it's currently coded. However, I found this issue that seems like it's a compromise between both ways.

I think the point is we don't wan't both async an promisify librarys since they both do same job

@wemeetagain
Copy link
Member

It looks like createNode was pulled from floodsub.
In gossipsub, I recently promisifyed it, you can see it here: https://github.com/ChainSafe/gossipsub-js/blob/gossip_implementation/test/utils.js#L18

@Mikerah
Copy link
Contributor Author

Mikerah commented May 1, 2019

@wemeetagain createNode was pulled from one of the libp2p examples. They use that pretty much everywhere. I don't mind promisifying.

@Mikerah Mikerah requested a review from GregTheGreek May 1, 2019 14:25
@wemeetagain
Copy link
Member

wemeetagain commented May 1, 2019

You can remove the async dependency

Edit: you should take a look at the typescript checking too (yarn check-types), causing travis failure.

src/p2p/node.ts Outdated Show resolved Hide resolved
src/p2p/node.ts Outdated Show resolved Hide resolved
@wemeetagain wemeetagain merged commit dd38877 into master May 2, 2019
@wemeetagain wemeetagain deleted the mikerah/libp2p_integration branch May 2, 2019 02:08
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

5 participants