-
Notifications
You must be signed in to change notification settings - Fork 448
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
Delegated Peer and Content Routing #242
Conversation
src/index.js
Outdated
this.contentRouting = contentRouting(this) | ||
// If peer or content routing modules have been provided, use those, otherwise use the dht | ||
this.peerRouting = this._modules.peerRouting || peerRouting(this) | ||
this.contentRouting = this._modules.contentRouting || contentRouting(this) |
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.
@diasdavid my original thought was to either use the provided peer and content routing modules, or the dht. After finishing up the core of the work and thinking more about the example and usage I feel like it would be better to use both if they are enabled. The thought being that leveraging a delegate is a good way to bootstrap a node into the network while it grows its own network, but that overtime its network would have a lot of the information. We could also change peer and content routing to allow for multiple modules and in the future potentially introduce some priority logic for ordering the calls to prevent flooding the network, if that becomes an issue.
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.
Think of it as a Stream Muxer or a Transport, you can use one, many, all the same time, disable some, you name it :)
ae3d5b9
to
31df87d
Compare
a0ae173
to
018a571
Compare
Delegated routing is ready for review! The IPFS updates aren't finished or deployed yet, but this is ready for when it is! cc: @lgierth |
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.
@jacobheun the PR seems good! The example rocks 💪
Added just some small notes.
Note, some of them are the same problem but in different files. I reported all of them (I hope), to guarantee that you do not forget any.
import WebRTCStar from 'libp2p-webrtc-star'; | ||
import MPLEX from 'libp2p-mplex'; | ||
import SECIO from 'libp2p-secio'; | ||
import KadDHT from 'libp2p-kad-dht'; |
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.
Not problematic, but since it is disabled, we could not have it has a dependency and have null
in the config.
src/content-routing.js
Outdated
if (typeof options === 'function') { | ||
callback = options | ||
options = {} | ||
} else if (typeof options === 'number') { |
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.
Can you just add a comment here for future deprecation?
src/content-routing.js
Outdated
|
||
// If we don't have any results, we need to provide an error to keep trying | ||
if (!results || Object.keys(results).length === 0) { | ||
return cb(Object.assign(new Error('not found'), { |
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.
use err-code
module here
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.
Good catch, I completely forgot to switch that over!
src/content-routing.js
Outdated
*/ | ||
findProviders: (key, options, callback) => { | ||
if (routers.length === 0) { | ||
return callback(new Error('No content routers available')) |
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.
add err-code
here?
src/content-routing.js
Outdated
* @returns {void} | ||
*/ | ||
findProviders: (key, options, callback) => { | ||
if (routers.length === 0) { |
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.
I would simply use
if (!routers.length)
But it is a detail. You have this in several places if you agree please change them all, otherwise leave like this
src/peer-routing.js
Outdated
if (err && err.code !== 'NOT_FOUND') { | ||
return callback(err) | ||
} | ||
results = results || null |
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.
shouldn't this be?
results = results || []
test/content-routing.node.js
Outdated
}) | ||
|
||
it('nodeE.contentRouting.findProviders for existing record', (done) => { | ||
nodeE.contentRouting.findProviders(cid, 5000, (err, providers) => { |
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.
Use themaxTimeout
instead of 5000
, as we intend to deprecate it in the future.
test/content-routing.node.js
Outdated
it('nodeC.contentRouting.findProviders for non existing record (timeout)', (done) => { | ||
const cid = new CID('QmTp9VkYvnHyrqKQuFPiuZkiX9gPcqj6x5LJ1rmWuSnnnn') | ||
|
||
nodeE.contentRouting.findProviders(cid, 5000, (err, providers) => { |
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.
Use themaxTimeout
instead of 5000
, as we intend to deprecate it in the future.
test/content-routing.node.js
Outdated
expect(err).to.not.exist() | ||
expect(providers).to.have.length(0) | ||
done() | ||
nodeA.contentRouting.findProviders('a cid', 5000, (err, results) => { |
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.
Use themaxTimeout
instead of 5000
, as we intend to deprecate it in the future.
test/content-routing.node.js
Outdated
const dhtStub = sinon.stub(nodeA._dht, 'findProviders').callsArgWith(2, null, []) | ||
const delegateStub = sinon.stub(delegate, 'findProviders').callsArgWith(2, null, results) | ||
|
||
nodeA.contentRouting.findProviders('a cid', 5000, (err, results) => { |
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.
Use themaxTimeout
instead of 5000
, as we intend to deprecate it in the future.
@vasco-santos I pushed updates from your feedback. I also needed to re-work some of the code in the docs to fix linting there. The logic is unchanged though. |
Looks good to me! |
bfb527c
to
f05e612
Compare
f05e612
to
8128ae0
Compare
Replaces [err-code](https://github.com/IndigoUnited/js-err-code/blob/master/index.js) with [CodeError](libp2p/js-libp2p-interfaces#314) Related: [js-libp2p#1269](libp2p#1269) Changes - removes err-code from dependencies - adds @libp2p/interfaces@3.2.0 to dependencies - uses CodeError in place of err-code
Increases default inactivity timeout to 5 minutes. closes libp2p#239
## [6.0.9](libp2p/js-libp2p-tcp@v6.0.8...v6.0.9) (2023-01-17) ### Bug Fixes * increase default socket close timeout ([libp2p#242](libp2p/js-libp2p-tcp#242)) ([a64ba41](libp2p/js-libp2p-tcp@a64ba41)), closes [libp2p#239](libp2p/js-libp2p-tcp#239) ### Trivial Changes * replace err-code with CodeError ([libp2p#240](libp2p/js-libp2p-tcp#240)) ([5c44562](libp2p/js-libp2p-tcp@5c44562)), closes [js-libp2p#1269](libp2p#1269)
## [7.1.2](libp2p/js-libp2p-mplex@v7.1.1...v7.1.2) (2023-03-21) ### Trivial Changes * replace err-code with CodeError ([libp2p#242](libp2p/js-libp2p-mplex#242)) ([8d58a3b](libp2p/js-libp2p-mplex@8d58a3b)), closes [js-libp2p#1269](libp2p#1269) * Update .github/workflows/semantic-pull-request.yml [skip ci] ([54de88d](libp2p/js-libp2p-mplex@54de88d)) * Update .github/workflows/semantic-pull-request.yml [skip ci] ([df03e8d](libp2p/js-libp2p-mplex@df03e8d)) * Update .github/workflows/semantic-pull-request.yml [skip ci] ([9c3f235](libp2p/js-libp2p-mplex@9c3f235)) ### Dependencies * **dev:** bump typescript from 4.9.5 to 5.0.2 ([libp2p#256](libp2p/js-libp2p-mplex#256)) ([a3590af](libp2p/js-libp2p-mplex@a3590af)) * **dev:** Upgrade aegir to 38.1.7 ([libp2p#257](libp2p/js-libp2p-mplex#257)) ([e0bf45a](libp2p/js-libp2p-mplex@e0bf45a))
This is ready!
This PR adds the ability to configure delegate routing modules, #120.
I've included an example of the delegated routing working.
Note: until ipfs/kubo#4595 is released, running a compatible local node for the example requires building go-ipfs from the
feat/delegated-routing
branch and running that locally. This will also not work in production until a Gateway node is deployed with delegated routing support: ipfs/infra#426.This also provides libp2p users with the ability to create and supply their own peer and content routing modules.