This repository has been archived by the owner on Feb 12, 2024. It is now read-only.
feat: pass libp2pOptions to the bundle function #2591
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,73 +8,65 @@ const SECIO = require('libp2p-secio') | |
| const Bootstrap = require('libp2p-bootstrap') | ||
| const KadDHT = require('libp2p-kad-dht') | ||
| const GossipSub = require('libp2p-gossipsub') | ||
| const multiaddr = require('multiaddr') | ||
|
|
||
| module.exports = ({ peerInfo, options }) => { | ||
| const wrtcstar = new WebRTCStar({ id: peerInfo.id }) | ||
|
|
||
| // this can be replaced once optional listening is supported with the below code. ref: https://github.com/libp2p/interface-transport/issues/41 | ||
| // const wsstar = new WebSocketStar({ id: _options.peerInfo.id }) | ||
| const wsstarServers = peerInfo.multiaddrs.toArray().map(String).filter(addr => addr.includes('p2p-websocket-star')) | ||
| peerInfo.multiaddrs.replace(wsstarServers.map(multiaddr), '/p2p-websocket-star') // the ws-star-multi module will replace this with the chosen ws-star servers | ||
| const wsstar = new WebSocketStarMulti({ servers: wsstarServers, id: peerInfo.id, ignore_no_online: !wsstarServers.length || options.wsStarIgnoreErrors }) | ||
|
|
||
| return { | ||
| switch: { | ||
| denyTTL: 2 * 60 * 1e3, // 2 minute base | ||
| denyAttempts: 5, // back off 5 times | ||
| maxParallelDials: 100, | ||
| maxColdCalls: 25, | ||
| dialTimeout: 20e3 | ||
| }, | ||
| modules: { | ||
| transport: [ | ||
| WS, | ||
| wrtcstar, | ||
| wsstar | ||
| ], | ||
| streamMuxer: [ | ||
| Multiplex | ||
| ], | ||
| connEncryption: [ | ||
| SECIO | ||
| ], | ||
| peerDiscovery: [ | ||
| wrtcstar.discovery, | ||
| wsstar.discovery, | ||
| Bootstrap | ||
| ], | ||
| dht: KadDHT, | ||
| pubsub: GossipSub | ||
| }, | ||
| config: { | ||
| peerDiscovery: { | ||
| autoDial: true, | ||
| bootstrap: { | ||
| enabled: true | ||
| }, | ||
| webRTCStar: { | ||
| enabled: true | ||
| }, | ||
| websocketStar: { | ||
| enabled: true | ||
| } | ||
| }, | ||
| dht: { | ||
| enabled: false | ||
| }, | ||
| pubsub: { | ||
| enabled: true, | ||
| emitSelf: true | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,75 +9,67 @@ const KadDHT = require('libp2p-kad-dht') | |
| const GossipSub = require('libp2p-gossipsub') | ||
| const Multiplex = require('pull-mplex') | ||
| const SECIO = require('libp2p-secio') | ||
| const multiaddr = require('multiaddr') | ||
|
|
||
| module.exports = ({ peerInfo, options }) => { | ||
| // this can be replaced once optional listening is supported with the below code. ref: https://github.com/libp2p/interface-transport/issues/41 | ||
| // const wsstar = new WebSocketStar({ id: _options.peerInfo.id }) | ||
| const wsstarServers = peerInfo.multiaddrs.toArray().map(String).filter(addr => addr.includes('p2p-websocket-star')) | ||
| peerInfo.multiaddrs.replace(wsstarServers.map(multiaddr), '/p2p-websocket-star') // the ws-star-multi module will replace this with the chosen ws-star servers | ||
| const wsstar = new WebSocketStarMulti({ servers: wsstarServers, id: peerInfo.id, ignore_no_online: !wsstarServers.length || options.wsStarIgnoreErrors }) | ||
|
|
||
| return { | ||
| switch: { | ||
| denyTTL: 2 * 60 * 1e3, // 2 minute base | ||
| denyAttempts: 5, // back off 5 times | ||
| maxParallelDials: 150, | ||
| maxColdCalls: 50, | ||
| dialTimeout: 10e3 // Be strict with dial time | ||
| }, | ||
| modules: { | ||
| transport: [ | ||
| TCP, | ||
| WS, | ||
| wsstar | ||
| ], | ||
| streamMuxer: [ | ||
| Multiplex | ||
| ], | ||
| connEncryption: [ | ||
| SECIO | ||
| ], | ||
| peerDiscovery: [ | ||
| MulticastDNS, | ||
| Bootstrap, | ||
| wsstar.discovery | ||
| ], | ||
| dht: KadDHT, | ||
| pubsub: GossipSub | ||
| }, | ||
| config: { | ||
| peerDiscovery: { | ||
| autoDial: true, | ||
| mdns: { | ||
| enabled: true | ||
| }, | ||
| bootstrap: { | ||
| enabled: true | ||
| }, | ||
| websocketStar: { | ||
| enabled: true | ||
| } | ||
| }, | ||
| dht: { | ||
| kBucketSize: 20, | ||
| enabled: false, | ||
| randomWalk: { | ||
| enabled: false | ||
| } | ||
| }, | ||
| pubsub: { | ||
| enabled: true, | ||
| emitSelf: true | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,6 +107,39 @@ describe('libp2p customization', function () { | |
| done() | ||
| }) | ||
| }) | ||
|
|
||
| it('should pass libp2p options to libp2p bundle function', (done) => { | ||
| class DummyTransport { | ||
| filter () { | ||
| return [] | ||
| } | ||
| } | ||
|
|
||
| const ipfs = { | ||
| _repo: { | ||
| datastore | ||
| }, | ||
| _peerInfo: peerInfo, | ||
| _peerBook: peerBook, | ||
| // eslint-disable-next-line no-console | ||
| _print: console.log, | ||
| _options: { | ||
| libp2p: ({ libp2pOptions, peerInfo }) => { | ||
| libp2pOptions.modules.transport = [DummyTransport] | ||
| return new Libp2p(libp2pOptions) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| _libp2p = libp2pComponent(ipfs, testConfig) | ||
|
|
||
| _libp2p.start((err) => { | ||
| expect(err).to.not.exist() | ||
| expect(_libp2p._transport).to.have.length(1) | ||
| expect(_libp2p._transport[0] instanceof DummyTransport).to.equal(true) | ||
| done() | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('options', () => { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, options is getting passed into
getLibp2pOptions, but then it is still passed into libp2p creation. ShouldgetLibp2pOptionsjust handle merging all of that so options doesn't need to be passed again?Along those lines, why is
optionsbeing passed here? It feels like the result ofgetLibp2pOptionsshould just be able to get passed directly to bothnew Libp2pandoptions.libp2p. No other params should be needed right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be some other IPFS option you want to use in your custom bundle function. It's "passed again" because that's what currently happens, and I'm trying to avoid a breaking change.
optionsis IPFS options, passed to the IPFS constructor. It's passed here because that's currently what happens when you provide a custom bundle function. If we removed it, we'd have a breaking change.getLibp2pOptionsis passed IPFSoptionsand returns libp2p options (I would call them defaults, but they're not really because they're based off the IPFSoptionsand the IPFS config from the repo).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so
optionsandconfigare for IPFS? If so, would it be easier to understand if in the scope of the custom bundleroptionswere the libp2p options and the IPFS ones were prefixed? If those are going to be exposed I think it might be helpful to make that distinction.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, that would be a breaking change. I think that would be clearer, but since the goal is to avoid the breaking change I think this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I agree!