chore: address review

src/connection-manager/topology.js

@@ -17,7 +17,8 @@ class Topology {
     onConnect,
     onDisconnect,
     multicodecs,
-    registrar
+    registrar,
+    peerStore
   }) {
     this.multicodecs = multicodecs
     this.registrar = registrar
@@ -32,14 +33,6 @@ class Topology {
     this._onProtocolChange = this._onProtocolChange.bind(this)
 
     // Set by the registrar
-    this._peerStore = null
-  }
-
-  /**
-   * Set peerstore to the topology.
-   * @param {PeerStore} peerStore
-   */
-  set peerStore (peerStore) {
     this._peerStore = peerStore
 
     this._peerStore.on('change:protocols', this._onProtocolChange)
@@ -92,7 +85,7 @@ class Topology {
     // New to protocol support
     for (const protocol of protocols) {
       if (this.multicodecs.includes(protocol)) {
-        this.tryToConnect(peerInfo, this.registrar.getPeerConnection(peerInfo))
+        this.tryToConnect(peerInfo, this.registrar.getConnection(peerInfo))
         return
       }
     }

src/get-peer-info.js

@@ -7,14 +7,14 @@ const errCode = require('err-code')
 
 /**
  * Converts the given `peer` to a `PeerInfo` instance.
- * The `PeerBook` will be checked for the resulting peer, and
- * the peer will be updated in the `PeerBook`.
+ * The `PeerStore` will be checked for the resulting peer, and
+ * the peer will be updated in the `PeerStore`.
  *
  * @param {PeerInfo|PeerId|Multiaddr|string} peer
- * @param {PeerBook} peerBook
+ * @param {PeerStore} peerStore
  * @returns {PeerInfo}
  */
-function getPeerInfo (peer, peerBook) {
+function getPeerInfo (peer, peerStore) {
   if (typeof peer === 'string') {
     peer = multiaddr(peer)
   }
@@ -38,7 +38,7 @@ function getPeerInfo (peer, peerBook) {
 
   addr && peer.multiaddrs.add(addr)
 
-  return peerBook ? peerBook.put(peer) : peer
+  return peerStore ? peerStore.put(peer) : peer
 }
 
 /**
@@ -54,7 +54,7 @@ function getPeerInfoRemote (peer, libp2p) {
   let peerInfo
 
   try {
-    peerInfo = getPeerInfo(peer, libp2p.peerBook)
+    peerInfo = getPeerInfo(peer, libp2p.peerStore)
   } catch (err) {
     return Promise.reject(errCode(
       new Error(`${peer} is not a valid peer type`),

src/index.js

@@ -112,7 +112,7 @@ class Libp2p extends EventEmitter {
       transportManager: this.transportManager
     })
 
-    this.registrar = new Registrar(this.peerStore)
+    this.registrar = new Registrar({ peerStore: this.peerStore })
     this.handle = this.handle.bind(this)
     this.registrar.handle = this.handle
 

src/peer-store/index.js

@@ -148,8 +148,6 @@ class PeerStore extends EventEmitter {
     if (!recorded.id.pubKey && peerInfo.id.pubKey) {
       recorded.id.pubKey = peerInfo.id.pubKey
     }
-
-    // this.peers.set(id, recorded)
   }
 
   /**

src/registrar.js

@@ -8,17 +8,18 @@ const errCode = require('err-code')
 
 const { Connection } = require('libp2p-interfaces/src/connection')
 const PeerInfo = require('peer-info')
-const MulticodecTopology = require('./connection-manager/topology')
+const Toplogy = require('./connection-manager/topology')
 
 /**
  * Responsible for notifying registered protocols of events in the network.
  */
 class Registrar {
   /**
-   * @param {PeerStore} peerStore
+   * @param {Object} props
+   * @param {PeerStore} props.peerStore
    * @constructor
    */
-  constructor (peerStore) {
+  constructor ({ peerStore }) {
     this.peerStore = peerStore
 
     /**
@@ -29,11 +30,11 @@ class Registrar {
     this.connections = new Map()
 
     /**
-     * Map of topologies per multicodec
+     * Map of topologies
      *
      * @type {Map<string, object>}
      */
-    this.multicodecTopologies = new Map()
+    this.topologies = new Map()
 
     this._handle = undefined
   }
@@ -70,7 +71,7 @@ class Registrar {
   onDisconnect (peerInfo, error) {
     assert(PeerInfo.isPeerInfo(peerInfo), 'peerInfo must be an instance of peer-info')
 
-    for (const [, topology] of this.multicodecTopologies) {
+    for (const [, topology] of this.topologies) {
       topology.disconnect(peerInfo, error)
     }
 
@@ -82,7 +83,7 @@ class Registrar {
    * @param {PeerInfo} peerInfo
    * @returns {Connection}
    */
-  getPeerConnection (peerInfo) {
+  getConnection (peerInfo) {
     assert(PeerInfo.isPeerInfo(peerInfo), 'peerInfo must be an instance of peer-info')
 
     return this.connections.get(peerInfo.id.toB58String())
@@ -94,10 +95,10 @@ class Registrar {
    * @param {object} handlers
    * @param {function} handlers.onConnect
    * @param {function} handlers.onDisconnect
-   * @param {object} topology properties for topology
+   * @param {object} topologyProps properties for topology
    * @return {string} registrar identifier
    */
-  register (multicodecs, handlers, topology = {}) {
+  register (multicodecs, handlers, topologyProps = {}) {
     if (!multicodecs) {
       throw errCode(new Error('one or more multicodec should be provided'), 'ERR_NO_MULTICODECS')
     } else if (!Array.isArray(multicodecs)) {
@@ -113,52 +114,50 @@ class Registrar {
     }
 
     // Create multicodec topology
-    const multicodecTopology = new MulticodecTopology({
+    const topology = new Toplogy({
       onConnect: handlers.onConnect,
       onDisconnect: handlers.onDisconnect,
       registrar: this,
       multicodecs,
-      ...topology
+      peerStore: this.peerStore,
+      ...topologyProps
     })
 
-    multicodecTopology.peerStore = this.peerStore
-
     const id = (parseInt(Math.random() * 1e9)).toString(36) + Date.now()
-    this.multicodecTopologies.set(id, multicodecTopology)
+    this.topologies.set(id, topology)
 
-    this._addConnectedPeers(multicodecs, multicodecTopology)
+    this._addConnectedPeers(multicodecs, topology)
 
     // TODO: try to connect to peers-store peers according to current topology
 
     return id
   }
 
-  _addConnectedPeers (multicodecs, multicodecTopology) {
-    const knownPeers = this.peerStore.getAllArray()
-      .filter((peerInfo) => multicodecs.filter(multicodec => peerInfo.protocols.has(multicodec)))
+  _addConnectedPeers (multicodecs, topology) {
+    const knownPeers = []
+
+    for (const [, peer] of this.peerStore.peers) {
+      if (multicodecs.filter(multicodec => peer.protocols.has(multicodec))) {
+        knownPeers.push(peer)
+      }
+    }
 
     for (const [id, conn] of this.connections.entries()) {
       const targetPeer = knownPeers.find((peerInfo) => peerInfo.id.toB58String() === id)
 
       if (targetPeer) {
-        multicodecTopology.tryToConnect(targetPeer, conn)
+        topology.tryToConnect(targetPeer, conn)
       }
     }
   }
 
   /**
    * Unregister topology.
    * @param {string} id registrar identifier
-   * @return {void}
+   * @return {boolean} unregistered successfully
    */
   unregister (id) {
-    const topology = this.multicodecTopologies.get(id)
-
-    if (!topology) {
-      throw errCode(new Error('no registrar found for the provided id'), 'ERR_NO_REGISTRAR')
-    }
-
-    this.multicodecTopologies.delete(id)
+    return this.topologies.delete(id)
   }
 }
 

test/registrar/registrar.node.js

@@ -52,13 +52,13 @@ describe('registrar on dial', () => {
     expect(libp2p.registrar.onDisconnect.callCount).to.equal(0)
 
     // TODO: needs crypto PR fix
-    // const libp2pConn = libp2p.registrar.getPeerConnection(remotePeerInfo)
-    const libp2pConn = libp2p.registrar.getPeerConnection(peerInfo)
+    // const libp2pConn = libp2p.registrar.getConnection(remotePeerInfo)
+    const libp2pConn = libp2p.registrar.getConnection(peerInfo)
     expect(libp2pConn).to.exist()
 
     // TODO: needs crypto PR fix
-    // const conn = libp2p.registrar.getPeerConnection(peerInfo)
-    const remoteConn = remoteLibp2p.registrar.getPeerConnection(remotePeerInfo)
+    // const conn = libp2p.registrar.getConnection(peerInfo)
+    const remoteConn = remoteLibp2p.registrar.getConnection(remotePeerInfo)
     expect(remoteConn).to.exist()
 
     await connection.close()

test/registrar/registrar.spec.js

@@ -20,7 +20,7 @@ describe('registrar', () => {
   describe('erros', () => {
     beforeEach(() => {
       peerStore = new PeerStore()
-      registrar = new Registrar(peerStore)
+      registrar = new Registrar({ peerStore })
     })
 
     it('should fail to register a protocol if no multicodec is provided', () => {
@@ -79,7 +79,7 @@ describe('registrar', () => {
   describe('registration', () => {
     beforeEach(() => {
       peerStore = new PeerStore()
-      registrar = new Registrar(peerStore)
+      registrar = new Registrar({ peerStore })
     })
 
     it('should be able to register a protocol', () => {
@@ -100,19 +100,15 @@ describe('registrar', () => {
       }
 
       const identifier = registrar.register(multicodec, handlers)
+      const success = registrar.unregister(identifier)
 
-      registrar.unregister(identifier)
+      expect(success).to.eql(true)
     })
 
     it('should fail to unregister if no register was made', () => {
-      try {
-        registrar.unregister('bad-identifier')
-      } catch (err) {
-        expect(err).to.exist()
-        expect(err.code).to.eql('ERR_NO_REGISTRAR')
-        return
-      }
-      throw new Error('should fail to unregister if no register was made')
+      const success = registrar.unregister('bad-identifier')
+
+      expect(success).to.eql(false)
     })
 
     it('should call onConnect handler for connected peers after register', async () => {
@@ -147,7 +143,7 @@ describe('registrar', () => {
 
       // Register protocol
       const identifier = registrar.register(multicodec, handlers)
-      const topology = registrar.multicodecTopologies.get(identifier)
+      const topology = registrar.topologies.get(identifier)
 
       // Topology created
       expect(topology).to.exist()
@@ -179,7 +175,7 @@ describe('registrar', () => {
 
       // Register protocol
       const identifier = registrar.register(multicodec, handlers)
-      const topology = registrar.multicodecTopologies.get(identifier)
+      const topology = registrar.topologies.get(identifier)
 
       // Topology created
       expect(topology).to.exist()
@@ -188,24 +184,22 @@ describe('registrar', () => {
 
       // Setup connections before registrar
       const conn = await createMockConnection()
-      const initialRemotePeerInfo = await PeerInfo.create(conn.remotePeer)
+      const peerInfo = await PeerInfo.create(conn.remotePeer)
 
       // Add connected peer to peerStore and registrar
-      peerStore.put(initialRemotePeerInfo)
-      registrar.onConnect(initialRemotePeerInfo, conn)
+      peerStore.put(peerInfo)
+      registrar.onConnect(peerInfo, conn)
 
       // Add protocol to peer and update it
-      const withProtocolRemotePeerInfo = await PeerInfo.create(conn.remotePeer)
-      withProtocolRemotePeerInfo.protocols.add(multicodec)
-      peerStore.put(withProtocolRemotePeerInfo)
+      peerInfo.protocols.add(multicodec)
+      peerStore.put(peerInfo)
 
       await onConnectDefer.promise
       expect(topology.peers.size).to.eql(1)
 
       // Remove protocol to peer and update it
-      const withoutProtocolRemotePeerInfo = await PeerInfo.create(conn.remotePeer)
-      withoutProtocolRemotePeerInfo.protocols.delete(multicodec)
-      peerStore.put(withoutProtocolRemotePeerInfo)
+      peerInfo.protocols.delete(multicodec)
+      peerStore.put(peerInfo)
 
       await onDisconnectDefer.promise
     })