chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

Author: vasco-santos

src/index.js

@@ -181,10 +181,10 @@ class Libp2p extends EventEmitter {
 
     // Once we start, emit and dial any peers we may have already discovered
     this.state.on('STARTED', () => {
-      this.peerStore.getAllArray().forEach((peerInfo) => {
+      for (const peerInfo of this.peerStore.peers) {
         this.emit('peer:discovery', peerInfo)
         this._maybeConnect(peerInfo)
-      })
+      }
     })
 
     this._peerDiscovered = this._peerDiscovered.bind(this)
@@ -279,17 +279,19 @@ class Libp2p extends EventEmitter {
       connection = await this.dialer.connectToPeer(peer, options)
     }
 
+    const peerInfo = getPeerInfo(connection.remotePeer)
+
     // If a protocol was provided, create a new stream
     if (protocols) {
       const stream = await connection.newStream(protocols)
-      const peerInfo = getPeerInfo(connection.remotePeer)
 
       peerInfo.protocols.add(stream.protocol)
       this.peerStore.put(peerInfo)
 
       return stream
     }
 
+    this.peerStore.put(peerInfo)
     return connection
   }
 

src/peer-store/index.js

@@ -4,7 +4,6 @@ const assert = require('assert')
 const debug = require('debug')
 const log = debug('libp2p:peer-store')
 log.error = debug('libp2p:peer-store:error')
-const errCode = require('err-code')
 
 const { EventEmitter } = require('events')
 
@@ -60,12 +59,37 @@ class PeerStore extends EventEmitter {
   add (peerInfo) {
     assert(PeerInfo.isPeerInfo(peerInfo), 'peerInfo must be an instance of peer-info')
 
-    this.peers.set(peerInfo.id.toB58String(), peerInfo)
+    // Create new instance and add values to it
+    const newPeerInfo = new PeerInfo(peerInfo.id)
+
+    peerInfo.multiaddrs.forEach((ma) => newPeerInfo.multiaddrs.add(ma))
+    peerInfo.protocols.forEach((p) => newPeerInfo.protocols.add(p))
+
+    const connectedMa = peerInfo.isConnected()
+    connectedMa && newPeerInfo.connect(connectedMa)
+
+    const peerProxy = new Proxy(newPeerInfo, {
+      set: (obj, prop, value) => {
+        if (prop === 'multiaddrs') {
+          this.emit('change:multiaddrs', {
+            peerInfo: obj,
+            multiaddrs: value.toArray()
+          })
+        } else if (prop === 'protocols') {
+          this.emit('change:protocols', {
+            peerInfo: obj,
+            protocols: Array.from(value)
+          })
+        }
+        return true
+      }
+    })
+
+    this.peers.set(peerInfo.id.toB58String(), peerProxy)
   }
 
   /**
    * Updates an already known peer.
-   * If already exist, updates ids info if outdated.
    * @param {PeerInfo} peerInfo
    */
   update (peerInfo) {
@@ -81,24 +105,22 @@ class PeerStore extends EventEmitter {
 
     // Verify new multiaddrs
     // TODO: better track added and removed multiaddrs
-    if (peerInfo.multiaddrs.size || recorded.multiaddrs.size) {
-      recorded.multiaddrs = peerInfo.multiaddrs
+    const multiaddrsIntersection = [
+      ...recorded.multiaddrs.toArray()
+    ].filter((m) => peerInfo.multiaddrs.has(m))
 
-      this.emit('change:multiaddrs', {
-        peerInfo: recorded,
-        multiaddrs: Array.from(recorded.multiaddrs)
-      })
+    if (multiaddrsIntersection.length !== peerInfo.multiaddrs.size) {
+      recorded.multiaddrs = peerInfo.multiaddrs
     }
 
     // Update protocols
     // TODO: better track added and removed protocols
-    if (peerInfo.protocols.size || recorded.protocols.size) {
-      recorded.protocols = new Set(peerInfo.protocols)
+    const protocolsIntersection = new Set(
+      [...recorded.protocols].filter((p) => peerInfo.protocols.has(p))
+    )
 
-      this.emit('change:protocols', {
-        peerInfo: recorded,
-        protocols: Array.from(recorded.protocols)
-      })
+    if (protocolsIntersection.size !== peerInfo.protocols.size) {
+      recorded.protocols = peerInfo.protocols
     }
 
     // Add the public key if missing
@@ -119,19 +141,11 @@ class PeerStore extends EventEmitter {
       return peerInfo
     }
 
-    throw errCode(new Error('PeerInfo was not found'), 'ERR_NO_PEER_INFO')
-  }
-
-  /**
-   * Get an array with all peers known.
-   * @returns {Array<PeerInfo>}
-   */
-  getAllArray () {
-    return Array.from(this.peers.values())
+    return undefined
   }
 
   /**
-   * Remove the info of the peer with the given id.
+   * Removes the Peer with the matching `peerId` from the PeerStore
    * @param {string} peerId b58str id
    * @returns {boolean} true if found and removed
    */
@@ -140,7 +154,7 @@ class PeerStore extends EventEmitter {
   }
 
   /**
-   * Replace the info stored of the given peer.
+   * Completely replaces the existing peers metadata with the given `peerInfo`
    * @param {PeerInfo} peerInfo
    * @returns {void}
    */

test/peer-store/peer-store.node.js

@@ -7,11 +7,18 @@ const { expect } = chai
 const sinon = require('sinon')
 
 const pDefer = require('p-defer')
+const mergeOptions = require('merge-options')
+
+const Libp2p = require('../../src')
 const PeerStore = require('../../src/peer-store')
 const multiaddr = require('multiaddr')
 
-const addr = multiaddr('/ip4/127.0.0.1/tcp/8000')
+const baseOptions = require('../utils/base-options')
 const peerUtils = require('../utils/creators/peer')
+const mockConnection = require('../utils/mockConnection')
+
+const addr = multiaddr('/ip4/127.0.0.1/tcp/8000')
+const listenAddr = multiaddr('/ip4/127.0.0.1/tcp/0')
 
 describe('peer-store', () => {
   let peerStore
@@ -75,49 +82,55 @@ describe('peer-store', () => {
 
   it('should emit the "change:multiaddrs" event when a peer has new multiaddrs', async () => {
     const defer = pDefer()
-    const [peerInfo] = await peerUtils.createPeerInfo(1)
+    const [createdPeerInfo] = await peerUtils.createPeerInfo(1)
 
     // Put the peer in the store
-    peerStore.put(peerInfo)
+    peerStore.put(createdPeerInfo)
 
     // When updating, "change:multiaddrs" event must not be emitted
     peerStore.on('change:multiaddrs', ({ peerInfo, multiaddrs }) => {
       expect(peerInfo).to.exist()
+      expect(peerInfo.id).to.eql(createdPeerInfo.id)
+      expect(peerInfo.protocols).to.eql(createdPeerInfo.protocols)
       expect(multiaddrs).to.exist()
+      expect(multiaddrs).to.eql(createdPeerInfo.multiaddrs.toArray())
       defer.resolve()
     })
     // If no protocols change, the event should not be emitted
     peerStore.on('change:protocols', () => {
       throw new Error('should not emit change:protocols')
     })
 
-    peerInfo.multiaddrs.add(addr)
-    peerStore.put(peerInfo)
+    createdPeerInfo.multiaddrs.add(addr)
+    peerStore.put(createdPeerInfo)
 
     // Wait for peerStore to emit the event
     await defer.promise
   })
 
   it('should emit the "change:protocols" event when a peer has new protocols', async () => {
     const defer = pDefer()
-    const [peerInfo] = await peerUtils.createPeerInfo(1)
+    const [createdPeerInfo] = await peerUtils.createPeerInfo(1)
 
     // Put the peer in the store
-    peerStore.put(peerInfo)
+    peerStore.put(createdPeerInfo)
 
     // If no multiaddrs change, the event should not be emitted
     peerStore.on('change:multiaddrs', () => {
       throw new Error('should not emit change:multiaddrs')
     })
-    // When updating, "change:protocols" event must not be emitted
+    // When updating, "change:protocols" event must be emitted
     peerStore.on('change:protocols', ({ peerInfo, protocols }) => {
       expect(peerInfo).to.exist()
+      expect(peerInfo.id).to.eql(createdPeerInfo.id)
+      expect(peerInfo.multiaddrs).to.eql(createdPeerInfo.multiaddrs)
       expect(protocols).to.exist()
+      expect(protocols).to.eql(Array.from(createdPeerInfo.protocols))
       defer.resolve()
     })
 
-    peerInfo.protocols.add('/new-protocol/1.0.0')
-    peerStore.put(peerInfo)
+    createdPeerInfo.protocols.add('/new-protocol/1.0.0')
+    peerStore.put(createdPeerInfo)
 
     // Wait for peerStore to emit the event
     await defer.promise
@@ -127,19 +140,17 @@ describe('peer-store', () => {
     const [peerInfo] = await peerUtils.createPeerInfo(1)
     const id = peerInfo.id.toB58String()
 
-    try {
-      peerStore.get(id)
-      throw new Error('peer should not exist in store')
-    } catch (err) {
-      expect(err).to.exist()
-      expect(err.code).to.eql('ERR_NO_PEER_INFO')
-    }
+    let retrievedPeer = peerStore.get(id)
+    expect(retrievedPeer).to.not.exist()
 
     // Put the peer in the store
     peerStore.put(peerInfo)
 
-    const retrievedPeer = peerStore.get(id)
+    retrievedPeer = peerStore.get(id)
     expect(retrievedPeer).to.exist()
+    expect(retrievedPeer.id).to.equal(peerInfo.id)
+    expect(retrievedPeer.multiaddrs).to.eql(peerInfo.multiaddrs)
+    expect(retrievedPeer.protocols).to.eql(peerInfo.protocols)
   })
 
   it('should be able to remove a peer from store through its b58str id', async () => {
@@ -151,11 +162,59 @@ describe('peer-store', () => {
 
     // Put the peer in the store
     peerStore.put(peerInfo)
-
-    const peers = peerStore.getAllArray()
-    expect(peers).to.have.lengthOf(1)
+    expect(peerStore.peers.size).to.equal(1)
 
     removed = peerStore.remove(id)
     expect(removed).to.eql(true)
+    expect(peerStore.peers.size).to.equal(0)
   })
 })
+
+describe('peer-store on dial', () => {
+  let peerInfo
+  let remotePeerInfo
+  let libp2p
+  let remoteLibp2p
+
+  before(async () => {
+    [peerInfo, remotePeerInfo] = await peerUtils.createPeerInfoFromFixture(2)
+    remoteLibp2p = new Libp2p(mergeOptions(baseOptions, {
+      peerInfo: remotePeerInfo
+    }))
+  })
+
+  after(async () => {
+    sinon.restore()
+    await remoteLibp2p.stop()
+    libp2p && await libp2p.stop()
+  })
+
+  it('should put the remote peerInfo after dial and emit event', async () => {
+    const remoteId = remotePeerInfo.id.toB58String()
+
+    libp2p = new Libp2p(mergeOptions(baseOptions, {
+      peerInfo
+    }))
+
+    sinon.spy(libp2p.peerStore, 'put')
+    sinon.spy(libp2p.peerStore, 'add')
+    sinon.spy(libp2p.peerStore, 'update')
+    sinon.stub(libp2p.dialer, 'connectToMultiaddr').returns(mockConnection({
+      remotePeer: remotePeerInfo.id
+    }))
+
+    const connection = await libp2p.dial(listenAddr)
+    await connection.close()
+
+    expect(libp2p.peerStore.put.callCount).to.equal(1)
+    expect(libp2p.peerStore.add.callCount).to.equal(1)
+    expect(libp2p.peerStore.update.callCount).to.equal(0)
+
+    const storedPeer = libp2p.peerStore.get(remoteId)
+    expect(storedPeer).to.exist()
+  })
+})
+
+describe('peer-store on discovery', () => {
+  // TODO: implement with discovery
+})