chore: apply suggestions from code review

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

src/dialer/index.js

@@ -111,7 +111,7 @@ class Dialer {
       }
     }
 
-    dialable.multiaddrs && this.peerStore.addressBook.set(dialable.id, Array.from(dialable.multiaddrs), { replace: false })
+    dialable.multiaddrs && this.peerStore.addressBook.add(dialable.id, Array.from(dialable.multiaddrs))
     const addrs = this.peerStore.addressBook.getMultiaddrsForPeer(dialable.id)
 
     return {

src/get-peer-info.js

@@ -39,8 +39,8 @@ function getPeerInfo (peer, peerStore) {
   addr && peer.multiaddrs.add(addr)
 
   if (peerStore) {
-    peerStore.addressBook.set(peer.id, peer.multiaddrs.toArray(), { replace: false })
-    peerStore.protoBook.set(peer.id, Array.from(peer.protocols), { replace: false })
+    peerStore.addressBook.add(peer.id, peer.multiaddrs.toArray())
+    peerStore.protoBook.add(peer.id, Array.from(peer.protocols))
   }
 
   return peer

src/identify/index.js

@@ -206,9 +206,6 @@ class IdentifyService {
       protocols: Array.from(this._protocols.keys())
     })
 
-    // TODO: should we add to peerStore.addressBook.set() here?
-    // We can have an inbound connection from an unkwown peer
-
     pipe(
       [message],
       lp.encode(),

src/index.js

@@ -292,7 +292,7 @@ class Libp2p extends EventEmitter {
       // TODO Inconsistency from: getDialable adds a set, while regular peerInfo uses a Multiaddr set
       // This should be handled on `peer-info` removal
       const multiaddrs = dialable.multiaddrs.toArray ? dialable.multiaddrs.toArray() : Array.from(dialable.multiaddrs)
-      this.peerStore.addressBook.set(dialable.id, multiaddrs, { replace: false })
+      this.peerStore.addressBook.add(dialable.id, multiaddrs)
 
       connection = this.registrar.getConnection(dialable)
     }
@@ -436,8 +436,8 @@ class Libp2p extends EventEmitter {
     }
 
     // TODO: once we deprecate peer-info, we should only set if we have data
-    this.peerStore.addressBook.set(peerInfo.id, peerInfo.multiaddrs.toArray(), { replace: false })
-    this.peerStore.protoBook.set(peerInfo.id, Array.from(peerInfo.protocols), { replace: false })
+    this.peerStore.addressBook.add(peerInfo.id, peerInfo.multiaddrs.toArray())
+    this.peerStore.protoBook.set(peerInfo.id, Array.from(peerInfo.protocols))
   }
 
   /**

src/peer-store/README.md

@@ -51,9 +51,9 @@ High level operations:
 
 Deletes the provided peer from every book.
 
-- [`peerStore.find(peerId)`](../../doc/API.md#peerstorefind)
+- [`peerStore.get(peerId)`](../../doc/API.md#peerstoreget)
 
-Finds the stored information of a given peer.
+Gets the stored information of a given peer.
 
 - [`peerStore.peers()`](../../doc/API.md#peerstorepeers)
 
@@ -86,6 +86,7 @@ A `peerId.toString()` identifier mapping to a `multiaddrInfo` object, which shou
 **Note:** except for multiaddr naming, the other properties are placeholders for now and might not be as described in the future milestones.
 
 - `addressBook.data`
+- [`addressBook.add()`](../../doc/API.md#peerstoreaddressbookadd)
 - [`addressBook.delete()`](../../doc/API.md#peerstoreaddressbookdelete)
 - [`addressBook.get()`](../../doc/API.md#peerstoreaddressbookget)
 - [`addressBook.getMultiaddrsForPeer()`](../../doc/API.md#peerstoreaddressbookgetmultiaddrsforpeer)
@@ -110,10 +111,10 @@ The `protoBook` holds the identifiers of the protocols supported by each peer. T
 A `peerId.toString()` identifier mapping to a `Set` of protocol identifier strings.
 
 - `protoBook.data`
+- [`protoBook.add()`](../../doc/API.md#peerstoreprotobookadd)
 - [`protoBook.delete()`](../../doc/API.md#peerstoreprotobookdelete)
 - [`protoBook.get()`](../../doc/API.md#peerstoreprotobookget)
 - [`protoBook.set()`](../../doc/API.md#peerstoreprotobookset)
-- [`protoBook.supports()`](../../doc/API.md#peerstoreprotobooksupports)
 
 #### Metadata Book
 

src/peer-store/address-book.js

@@ -22,89 +22,49 @@ const {
 class AddressBook extends Book {
   /**
    * MultiaddrInfo object
-   * @typedef {Object} multiaddrInfo
+   * @typedef {Object} MultiaddrInfo
    * @property {Multiaddr} multiaddr peer multiaddr.
-   * @property {number} validity NOT USED YET
-   * @property {number} confidence NOT USED YET
    */
 
   /**
   * @constructor
   * @param {EventEmitter} peerStore
   */
   constructor (peerStore) {
-    super(peerStore, 'change:multiaddrs', 'multiaddrs')
     /**
      * PeerStore Event emitter, used by the AddressBook to emit:
      * "peer" - emitted when a peer is discovered by the node.
      * "change:multiaddrs" - emitted when the known multiaddrs of a peer change.
      */
-    this._ps = peerStore
+    super(peerStore, 'change:multiaddrs', 'multiaddrs')
 
     /**
      * Map known peers to their known multiaddrs.
-     * @type {Map<string, Array<multiaddrInfo>}
+     * @type {Map<string, Array<MultiaddrInfo>>}
      */
     this.data = new Map()
   }
 
   /**
    * Set known addresses of a provided peer.
+   * @override
    * @param {PeerId} peerId
    * @param {Array<Multiaddr>|Multiaddr} addresses
-   * @param {Object} [options]
-   * @param {boolean} [options.replace = true] whether addresses received replace stored ones or a unique union is performed.
-   * @returns {Array<multiaddrInfo>}
+   * @returns {Map<string, Array<MultiaddrInfo>>}
    */
-  set (peerId, addresses, { replace = true } = {}) {
+  set (peerId, addresses) {
     if (!PeerId.isPeerId(peerId)) {
       log.error('peerId must be an instance of peer-id to store data')
       throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
     }
 
-    if (!addresses) {
-      log.error('addresses must be provided to store data')
-      throw errcode(new Error('addresses must be provided'), ERR_INVALID_PARAMETERS)
-    }
-
-    if (!Array.isArray(addresses)) {
-      addresses = [addresses]
-    }
-
-    // create multiaddrInfo for each address
-    const multiaddrInfos = []
-    addresses.forEach((addr) => {
-      if (!multiaddr.isMultiaddr(addr)) {
-        log.error(`multiaddr ${addr} must be an instance of multiaddr`)
-        throw errcode(new Error(`multiaddr ${addr} must be an instance of multiaddr`), ERR_INVALID_PARAMETERS)
-      }
-
-      multiaddrInfos.push({
-        multiaddr: addr
-      })
-    })
-
-    if (replace) {
-      return this._replace(peerId, multiaddrInfos)
-    }
-
-    return this._add(peerId, multiaddrInfos)
-  }
-
-  /**
-   * Replace known addresses of a provided peer.
-   * If the peer is not known, it is set with the given addresses.
-   * @param {PeerId} peerId
-   * @param {Array<multiaddrInfo>} multiaddrInfos
-   * @returns {Array<multiaddrInfo>}
-   */
-  _replace (peerId, multiaddrInfos) {
+    const multiaddrInfos = this._toMultiaddrInfos(addresses)
     const id = peerId.toString()
     const rec = this.data.get(id)
 
     // Not replace multiaddrs
     if (!multiaddrInfos.length) {
-      return rec ? [...rec] : []
+      return this.data
     }
 
     // Already knows the peer
@@ -115,7 +75,7 @@ class AddressBook extends Book {
       // If yes, no changes needed!
       if (intersection.length === rec.length) {
         log(`the addresses provided to store are equal to the already stored for ${id}`)
-        return [...multiaddrInfos]
+        return this.data
       }
     }
 
@@ -138,17 +98,24 @@ class AddressBook extends Book {
       multiaddrs: multiaddrInfos.map((mi) => mi.multiaddr)
     })
 
-    return [...multiaddrInfos]
+    return this.data
   }
 
   /**
-   * Add new known addresses to a provided peer.
+   * Add known addresses of a provided peer.
    * If the peer is not known, it is set with the given addresses.
+   * @override
    * @param {PeerId} peerId
-   * @param {Array<multiaddrInfo>} multiaddrInfos
-   * @returns {Array<multiaddrInfo>}
+   * @param {Array<Multiaddr>|Multiaddr} addresses
+   * @returns {Map<string, Array<MultiaddrInfo>>}
    */
-  _add (peerId, multiaddrInfos) {
+  add (peerId, addresses) {
+    if (!PeerId.isPeerId(peerId)) {
+      log.error('peerId must be an instance of peer-id to store data')
+      throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
+    }
+
+    const multiaddrInfos = this._toMultiaddrInfos(addresses)
     const id = peerId.toString()
     const rec = this.data.get(id)
 
@@ -163,7 +130,7 @@ class AddressBook extends Book {
     // The content is the same, no need to update.
     if (rec && rec.length === multiaddrInfos.length) {
       log(`the addresses provided to store are already stored for ${id}`)
-      return [...multiaddrInfos]
+      return this.data
     }
 
     this.data.set(id, multiaddrInfos)
@@ -186,7 +153,38 @@ class AddressBook extends Book {
       this._ps.emit('peer', peerInfo)
     }
 
-    return [...multiaddrInfos]
+    return this.data
+  }
+
+  /**
+   * Transforms received multiaddrs into MultiaddrInfo.
+   * @param {Array<Multiaddr>|Multiaddr} addresses
+   * @returns {Array<MultiaddrInfo>}
+   */
+  _toMultiaddrInfos (addresses) {
+    if (!addresses) {
+      log.error('addresses must be provided to store data')
+      throw errcode(new Error('addresses must be provided'), ERR_INVALID_PARAMETERS)
+    }
+
+    if (!Array.isArray(addresses)) {
+      addresses = [addresses]
+    }
+
+    // create MultiaddrInfo for each address
+    const multiaddrInfos = []
+    addresses.forEach((addr) => {
+      if (!multiaddr.isMultiaddr(addr)) {
+        log.error(`multiaddr ${addr} must be an instance of multiaddr`)
+        throw errcode(new Error(`multiaddr ${addr} must be an instance of multiaddr`), ERR_INVALID_PARAMETERS)
+      }
+
+      multiaddrInfos.push({
+        multiaddr: addr
+      })
+    })
+
+    return multiaddrInfos
   }
 
   /**

src/peer-store/book.js

@@ -12,8 +12,8 @@ const {
  * The Book is the skeleton for the PeerStore books.
  */
 class Book {
-  constructor (eventEmitter, eventName, eventProperty) {
-    this.eventEmitter = eventEmitter
+  constructor (peerStore, eventName, eventProperty) {
+    this._ps = peerStore
     this.eventName = eventName
     this.eventProperty = eventProperty
 
@@ -28,10 +28,17 @@ class Book {
    * Set known data of a provided peer.
    * @param {PeerId} peerId
    * @param {Array<Data>|Data} data
-   * @param {Object} [options]
-   * @param {boolean} [options.replace = true] wether data received replace stored the one or a unique union is performed.
    */
-  set (peerId, data, options) {
+  set (peerId, data) {
+    throw errcode(new Error('set must be implemented by the subclass'), 'ERR_NOT_IMPLEMENTED')
+  }
+
+  /**
+   * Add known data of a provided peer.
+   * @param {PeerId} peerId
+   * @param {Array<Data>|Data} data
+   */
+  add (peerId, data) {
     throw errcode(new Error('set must be implemented by the subclass'), 'ERR_NOT_IMPLEMENTED')
   }
 
@@ -67,7 +74,7 @@ class Book {
     // TODO: Remove peerInfo and its usage on peer-info deprecate
     const peerInfo = new PeerInfo(peerId)
 
-    this.eventEmitter.emit(this.eventName, {
+    this._ps.emit(this.eventName, {
       peerId,
       peerInfo,
       [this.eventProperty]: []