chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <[email protected]>

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

@@ -1,18 +1,16 @@
-# Peerstore
+# PeerStore
 
-Libp2p's Peerstore is responsible for keeping an updated register with the relevant information of the known peers. It should gather environment changes, be able to take decisions and notice interested parties of relevant changes. The Peerstore comprises four main components: `addressBook`, `keyBook`, `protocolBook` and `metadataBook`. These book components have similar characteristics with the `Javascript Map` implementation.
+Libp2p's PeerStore is responsible for keeping an updated register with the relevant information of the known peers. It should be the single source of truth for all peer data, where a subsystem can learn about peers' data and where someone can listen for updates. The PeerStore comprises four main components: `addressBook`, `keyBook`, `protocolBook` and `metadataBook`.
 
-The PeerStore manages the high level operations on its inner books. Moreover, the peerStore should be responsible for notifying interested parties of relevant events, through its Event Emitter.
-
-(Future considerations: Peerstore should manage a job runner to trigger books runners for data trimming or computations)
+The PeerStore manages the high level operations on its inner books. Moreover, the PeerStore should be responsible for notifying interested parties of relevant events, through its Event Emitter.
 
 ## Data gathering
 
 Several libp2p subsystems will perform operations, which will gather relevant information about peers. Some operations might not have this as an end goal, but can also gather important data.
 
-In a libp2p node's life, it will discover peers through its discovery protocols. In a typical discovery protocol, addresses of the peer are discovered along with its peer id. Once this happens, the `PeerStore` should collect this information for future (or immediate) usage by other subsystems. When the information is stored, the `PeerStore` should inform interested parties of the peer discovered (`peer` event).
+In a libp2p node's life, it will discover peers through its discovery protocols. In a typical discovery protocol, addresses of the peer are discovered along with its peer id. Once this happens, the PeerStore should collect this information for future (or immediate) usage by other subsystems. When the information is stored, the PeerStore should inform interested parties of the peer discovered (`peer` event).
 
-Taking into account a different scenario, a peer might perform/receive a dial request to/from a unkwown peer. In such a scenario, the `PeerStore` must store the peer's multiaddr once a connection is established. 
+Taking into account a different scenario, a peer might perform/receive a dial request to/from a unkwown peer. In such a scenario, the PeerStore must store the peer's multiaddr once a connection is established. 
 
 After a connection is established with a peer, the Identify protocol will run automatically. A stream is created and peers exchange their information (Multiaddrs, running protocols and their public key). Once this information is obtained, it should be added to the PeerStore. In this specific case, as we are speaking to the source of truth, we should ensure the PeerStore is prioritizing these records. If the recorded `multiaddrs` or `protocols` have changed, interested parties must be informed via the `change:multiaddrs` or `change:protocols` events respectively.
 
@@ -22,19 +20,19 @@ In the background, the Identify Service is also waiting for protocol change noti
 
 While it is currently not supported in js-libp2p, future iterations may also support the [IdentifyDelta protocol](https://github.com/libp2p/specs/pull/176).
 
-It is also possible to gather relevant information for peers from other protocols / subsystems. For instance, in `DHT` operations, nodes can exchange peer data as part of the `DHT` operation. In this case, we can learn additional information about a peer we already know. In this scenario the `PeerStore` should not replace the existing data it has, just add it.
+It is also possible to gather relevant information for peers from other protocols / subsystems. For instance, in `DHT` operations, nodes can exchange peer data as part of the `DHT` operation. In this case, we can learn additional information about a peer we already know. In this scenario the PeerStore should not replace the existing data it has, just add it.
 
 ## Data Consumption
 
-When the `PeerStore` data is updated, this information might be important for different parties.
+When the PeerStore data is updated, this information might be important for different parties.
 
 Every time a peer needs to dial another peer, it is essential that it knows the multiaddrs used by the peer, in order to perform a successful dial to it. The same is true for pinging a peer. While the `AddressBook` is going to keep its data updated, it will also emit `change:multiaddrs` events so that subsystems/users interested in knowing these changes can be notifyied instead of pooling the `AddressBook`.
 
 Everytime a peer starts/stops supporting a protocol, libp2p subsystems or users might need to act accordingly. `js-libp2p` registrar orchestrates known peers, established connections and protocol topologies. This way, once a protocol is supported for a peer, the topology of that protocol should be informed that a new peer may be used and the subsystem can decide if it should open a new stream with that peer or not. For these situations, the `ProtoBook` will emit `change:protocols` events whenever supported protocols of a peer change.
 
 ## PeerStore implementation
 
-The Peerstore wraps four main components: `addressBook`, `keyBook`, `protocolBook` and `metadataBook`. Moreover, it provides a high level API for those components, as well as data events.
+The PeerStore wraps four main components: `addressBook`, `keyBook`, `protocolBook` and `metadataBook`. Moreover, it provides a high level API for those components, as well as data events.
 
 ### API
 
@@ -51,9 +49,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)
 
@@ -78,14 +76,13 @@ A `peerId.toString()` identifier mapping to a `multiaddrInfo` object, which shou
 ```js
 {
   multiaddr: ,
-  validity: ,
-  confidence: 
 }
 ```
 
 **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,11 +107,16 @@ 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
 
 **Not Yet Implemented**
+
+## Future Considerations
+
+- If multiaddr TTLs are added, the PeerStore may schedule jobs to delete all addresses that exceed the TTL to prevent AddressBook bloating
+- Further API methods will probably need to be added in the context of multiaddr validity and confidence.

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]: []