feat: topology #7
Conversation
vasco-santos
force-pushed
the
feat/topology
branch
from
ae08096 to
f197d99
Compare
|
|
||
| const assert = require('assert') | ||
|
|
||
| class Topology { |
There was a problem hiding this comment.
This really isn't a base Topology, it's a MulticodecTopology which should extend a base. Not all topologies are going to care about multicodecs or protocol changes. Two immediate use cases that come to mind for Topologies are Priority Peers and Bootstrap Nodes, which I would categorize as a PeerSet Topology.
Let's say a js-ipfs browser node would like to stay connected to a Preload Node, QmPreload. Currently, libp2p doesn't handle that. What could happen, is a new PeerSet Topology could be created, which we should be able to achieve with the Base Topology.
const priorityTopology = new Topology()
priorityTopology.peers.set('QmPreload', preloadPeerInfo) // We could iterate over several peers.
priorityTopology.min = priorityTopology.peers.size // We want to be connected to all our peers
// ... registrationThis same setup could be used for Bootstrap nodes, instead of it being a "discovery" service. The only difference is that the min can be 0, because we shouldn't really care if we're connected to them, as long as our node has at least ConnectionManager.min connections, which will be handled elsewhere.
There was a problem hiding this comment.
Added multicodec-topology
src/topology/index.js
Outdated
| * @param {Error} [error] | ||
| * @returns {void} | ||
| */ | ||
| disconnect (peerInfo, error) { |
There was a problem hiding this comment.
What's your thought behind passing error here? I don't think we're propagating disconnect errors to the Registrar and I'm not sure they're needed.
There was a problem hiding this comment.
That's for identifying if the disconnect was due to an error or not. I guess we can remove it until it can be useful
There was a problem hiding this comment.
I think if we remove this and fill in the readme this should be gtg!
src/topology/index.js
Outdated
| max = Infinity, | ||
| handlers | ||
| }) { | ||
| assert(handlers, 'the handlers should be provided') |
There was a problem hiding this comment.
I'm thinking we shouldn't require the handlers. Some topologies might not care, like the PeerSet Topologies. Multicodec needs them though. Maybe they should be required there and default to noop functions here?
There was a problem hiding this comment.
I think you are right, I will do it
| protocols: Array.from(topology.multicodecs) | ||
| }) | ||
|
|
||
| await delay(500) |
There was a problem hiding this comment.
Are the delays in this suite actually needed? I don't think the event emitter is async. If I am mistaken then delay(1) should be sufficient.
Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
vasco-santos
force-pushed
the
feat/topology
branch
from
547de08 to
bea364d
Compare
src/topology/README.md
Outdated
| - `onConnect` is a `function` called everytime a peer is connected in the topology context. | ||
| - `onDisconnect` is a `function` called everytime a peer is disconnected in the topology context. | ||
|
|
||
| #### Set the registrar |
There was a problem hiding this comment.
Let's delete this section, I think the property overview is sufficient for now. Also, the Registrar is going to set topology.registrar on the base Topology as well during registration, every Topology will have that property once they've registered, so it might be good to add that to the Topology
There was a problem hiding this comment.
Removed the section, added the set to the Topology and overwrite it in the MulticodecTopology
Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
vasco-santos
force-pushed
the
feat/topology
branch
from
0c0cb61 to
8d509fb
Compare
Added topology interface