client: fix libp2p sync and browser example #1588
Conversation
* remove premature PEER_CONNECTED * parse bootnodes, set consistent chainDB * tidy libp2p files and other small fixes
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Hi there,
nice that you are tackling this 🙂, have left some first comments.
There is still 1 client test failing here, not sure if you have seen this.
I would be interested to do a production test on this along the browser example lines, not sure if I'll be getting to that though, just as some note.
| import PeerId from 'peer-id' | ||
| import { Multiaddr } from 'multiaddr' | ||
| // types currently unavailable for below libp2p deps, | ||
| // tracking issue: https://github.com/libp2p/js-libp2p/issues/659 | ||
| const LibP2pWebsockets = require('libp2p-websockets') |
There was a problem hiding this comment.
Just out of interest (without further digging in): the referenced issue here libp2p/js-libp2p#659 is still open, could this nevertheless be resolved?
There was a problem hiding this comment.
I was able to convert a few of the modules to import, the rest are still on require, I think it's obvious at this point that any modules using require are due to missing types, so I thought I'd clean up/remove the comment.
refactor some cli logic to own functions (helprpc, inputAccounts, setupDev)
|
ok this should be ready for final review/merge unless there are any other suggested changes |
| @@ -33,11 +37,6 @@ const args = yargs(hideBin(process.argv)) | |||
| choices: networks.map((n) => parseInt(n[0])), | |||
| default: undefined, | |||
| }) | |||
| .option('network-id', { | |||
There was a problem hiding this comment.
'network-id' option was defined twice here
|
@ryanio after I spent the time writing them, I'm not so certain the new tests I added for the |
|
I think that's fine, thanks for adding them! definitely helped with the coverage :) |
|
And I think we finally hit the code coverage requirements too. I've spent the last while trying to figure out how to grab the debugger output but can't for the life of me figure out how to redirect it from stderr to something I can actually check. I'm gonna add one more small set of tests around the fetcher since we don't have much coverage there anyway and will then be done. |
| const client = await runNode(config) | ||
| const servers = args.rpc || args.rpcEngine ? runRpcServers(client, config, args) : [] | ||
| const client = await startClient(config) | ||
| const servers = args.rpc || args.rpcEngine ? startRPCServers(client) : [] | ||
|
|
||
| process.on('SIGINT', async () => { | ||
| config.logger.info('Caught interrupt signal. Shutting down...') |
There was a problem hiding this comment.
Whew.
That's a really great and much needed clean-up on the CLI script!!! 😄 👍
Nice.
🎉
| st.pass('store() emitted SYNC_FETCHER_FETCHED event on putting blocks') | ||
| ) | ||
| await fetcher.store([]) | ||
| }) |
There was a problem hiding this comment.
Nice, all these new test cases are really great as well! 😄
Especially the Fetcher is a beasty thing, so that's nice if we get to better coverage there, also for reproducing things and having certain functionality parts more directly called.
This PR:
Buffer is not definedfrom webpack by specifying the polyfillPEER_CONNECTEDinpeer:connectedbrowser_sync.pngimage location and updates to the latest expected outputTo help test this PR, please try running through the steps in
examples/light-browser-sync.md