chore: apply suggestions from code review

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>
libp2p · Apr 2, 2020 · b06040b · b06040b
1 parent eb27b8d
commit b06040b
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 143 deletions.
diff --git a/doc/API.md b/doc/API.md
@@ -24,7 +24,6 @@
   * [`peerStore.protoBook.delete`](#peerstoreprotobookdelete)
   * [`peerStore.protoBook.get`](#peerstoreprotobookget)
   * [`peerStore.protoBook.set`](#peerstoreprotobookset)
-  * [`peerStore.protoBook.supports`](#peerstoreprotobooksupports)
   * [`peerStore.delete`](#peerstoredelete)
   * [`peerStore.find`](#peerstorefind)
   * [`peerStore.peers`](#peerstorepeers)
@@ -227,7 +226,7 @@ Dials to another peer in the network and selects a protocol to communicate with
 | Name | Type | Description |
 |------|------|-------------|
 | peer | [`PeerInfo`][peer-info]\|[`PeerId`][peer-id]\|[`Multiaddr`][multiaddr]\|`string` | The peer to dial. If a [`Multiaddr`][multiaddr] or its string is provided, it **must** include the peer id |
-| protocols | `String|Array<String>` |  A list of protocols (or single protocol) to negotiate with. Protocols are attempted in order until a match is made. (e.g '/ipfs/bitswap/1.1.0') |
+| protocols | `string|Array<string>` |  A list of protocols (or single protocol) to negotiate with. Protocols are attempted in order until a match is made. (e.g '/ipfs/bitswap/1.1.0') |
 | [options] | `Object` | dial options |
 | [options.signal] | [`AbortSignal`](https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal) | An `AbortSignal` instance obtained from an [`AbortController`](https://developer.mozilla.org/en-US/docs/Web/API/AbortController) that can be used to abort the connection before it completes |
 
@@ -286,7 +285,7 @@ In the event of a new handler for the same protocol being added, the first one i
 
 | Name | Type | Description |
 |------|------|-------------|
-| protocols | `Array<String>|String` | protocols to register |
+| protocols | `Array<string>|string` | protocols to register |
 | handler | `function({ connection:*, stream:*, protocol:string })` | handler to call |
 
 
@@ -311,7 +310,7 @@ Unregisters all handlers with the given protocols
 
 | Name | Type | Description |
 |------|------|-------------|
-| protocols | `Array<String>|String` | protocols to unregister |
+| protocols | `Array<string>|string` | protocols to unregister |
 
 #### Example
 
@@ -438,7 +437,7 @@ Writes a value to a key in the DHT.
 
 | Name | Type | Description |
 |------|------|-------------|
-| key | `String` | key to add to the dht |
+| key | `string` | key to add to the dht |
 | value | `Buffer` | value to add to the dht |
 | [options] | `Object` | put options |
 | [options.minPeers] | `number` | minimum number of peers required to successfully put (default: closestPeers.length) |
@@ -469,7 +468,7 @@ Queries the DHT for a value stored for a given key.
 
 | Name | Type | Description |
 |------|------|-------------|
-| key | `String` | key to get from the dht |
+| key | `string` | key to get from the dht |
 | [options] | `Object` | get options |
 | [options.timeout] | `number` | maximum time the query should run |
 
@@ -498,7 +497,7 @@ Queries the DHT for the n values stored for the given key (without sorting).
 
 | Name | Type | Description |
 |------|------|-------------|
-| key | `String` | key to get from the dht |
+| key | `string` | key to get from the dht |
 | nvals | `number` | number of values aimed |
 | [options] | `Object` | get options |
 | [options.timeout] | `number` | maximum time the query should run |
@@ -528,7 +527,7 @@ Delete the provided peer from the book.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to remove |
+| peerId | [`PeerId`][peer-id] | peerId to remove |
 
 #### Returns
 
@@ -556,7 +555,7 @@ Get the known `multiaddrInfos` of a provided peer.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to get |
+| peerId | [`PeerId`][peer-id] | peerId to get |
 
 #### Returns
 
@@ -569,7 +568,7 @@ Get the known `multiaddrInfos` of a provided peer.
 ```js
 peerStore.addressBook.get(peerId)
 // undefined
-peerStore.addressBook.set(peerId, discoveredMultiaddr)
+peerStore.addressBook.set(peerId, multiaddr)
 peerStore.addressBook.get(peerId)
 // [
 // {
@@ -593,20 +592,20 @@ Get the known `multiaddr` of a provided peer. All returned multiaddrs will inclu
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to get |
+| peerId | [`PeerId`][peer-id] | peerId to get |
 
 #### Returns
 
 | Type | Description |
 |------|-------------|
-| `Array<multiaddr>` | Array of peer's multiaddr |
+| `Array<Multiaddr>` | Array of peer's multiaddr |
 
 #### Example
 
 ```js
 peerStore.addressBook.getMultiaddrsForPeer(peerId)
 // undefined
-peerStore.addressBook.set(peerId, discoveredMultiaddr)
+peerStore.addressBook.set(peerId, multiaddr)
 peerStore.addressBook.getMultiaddrsForPeer(peerId)
 // [
 // /ip4/140.10.2.1/tcp/8000/p2p/QmW8rAgaaA6sRydK1k6vonShQME47aDxaFidbtMevWs73t
@@ -624,9 +623,9 @@ Set known `multiaddrs` of a given peer.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to set |
-| multiaddrs | `multiaddr|Array<multiaddr>` | multiaddrs to store |
-| [options] | `object` | options to set |
+| peerId | [`PeerId`][peer-id] | peerId to set |
+| multiaddrs | `Multiaddr|Array<Multiaddr>` | multiaddrs to store |
+| [options] | `Object` | options to set |
 | [options.replace] | `Object` | replace stored data (if exists) or unique union (default: true) |
 
 #### Returns
@@ -638,7 +637,7 @@ Set known `multiaddrs` of a given peer.
 #### Example
 
 ```js
-peerStore.addressBook.set(peerId, discoveredMultiaddr)
+peerStore.addressBook.set(peerId, multiaddr)
 // [
 // {
 //   multiaddr: /ip4/140.10.2.1/tcp/8000,
@@ -661,7 +660,7 @@ Delete the provided peer from the book.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to remove |
+| peerId | [`PeerId`][peer-id] | peerId to remove |
 
 #### Returns
 
@@ -674,7 +673,7 @@ Delete the provided peer from the book.
 ```js
 peerStore.protoBook.delete(peerId)
 // false
-peerStore.protoBook.set(peerId, supportedProtocols)
+peerStore.protoBook.set(peerId, protocols)
 peerStore.protoBook.delete(peerId)
 // true
 ```
@@ -689,7 +688,7 @@ Get the known `protocols` of a provided peer.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to get |
+| peerId | [`PeerId`][peer-id] | peerId to get |
 
 #### Returns
 
@@ -702,7 +701,7 @@ Get the known `protocols` of a provided peer.
 ```js
 peerStore.protoBook.get(peerId)
 // undefined
-peerStore.protoBook.set(peerId, supportedProtocols)
+peerStore.protoBook.set(peerId, [ '/proto/1.0.0', '/proto/1.1.0' ])
 peerStore.protoBook.get(peerId)
 // [ '/proto/1.0.0', '/proto/1.1.0' ]
 ```
@@ -717,9 +716,9 @@ Set known `protocols` of a given peer.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to set |
+| peerId | [`PeerId`][peer-id] | peerId to set |
 | protocols | `string|Array<string>` | protocols to store |
-| [options] | `object` | options to set |
+| [options] | `Object` | options to set |
 | [options.replace] | `Object` | replace stored data (if exists) or unique union (default: true) |
 
 #### Returns
@@ -735,40 +734,6 @@ peerStore.protoBook.set(peerId, supportedProtocols)
 // [ '/proto/1.0.0', '/proto/1.1.0' ]
 ```
 
-### peerStore.protoBook.supports
-
-Verify if the provided peer supports the given `protocols`.
-
-`peerStore.protoBook.supports(peerId, protocols)`
-
-#### Parameters
-
-| Name | Type | Description |
-|------|------|-------------|
-| peerId | `peerid` | peerId to get |
-| protocols | `string|Array<string>` | protocols to verify |
-
-#### Returns
-
-| Type | Description |
-|------|-------------|
-| `boolean` | true if found and removed |
-
-#### Example
-
-```js
-const supportedProtocols = [ '/proto/1.0.0', '/proto/1.1.0' ]
-peerStore.protoBook.supports(peerId, supportedProtocols)
-// false
-peerStore.protoBook.supports(peerId, supportedProtocols[0])
-// false
-peerStore.protoBook.set(peerId, supportedProtocols)
-peerStore.protoBook.supports(peerId, supportedProtocols)
-// true
-peerStore.protoBook.supports(peerId, supportedProtocols[0])
-// true
-```
-
 ### peerStore.delete
 
 Delete the provided peer from every book.
@@ -779,7 +744,7 @@ Delete the provided peer from every book.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to remove |
+| peerId | [`PeerId`][peer-id] | peerId to remove |
 
 #### Returns
 
@@ -813,7 +778,7 @@ Find the stored information of a given peer.
 
 | Name | Type | Description |
 |------|------|-------------|
-| peerId | `peerid` | peerId to find |
+| peerId | [`PeerId`][peer-id] | peerId to find |
 
 #### Returns
 
@@ -852,7 +817,7 @@ TODO: change when `peer-info` is deprecated (breaking change)
 #### Example
 
 ```js
-for (peer of peerStore.peers().values()) {
+for (let [peerIdString, peerInfo] of peerStore.peers.entries()) {
   // peerInfo instance
 }
 ```
@@ -873,7 +838,7 @@ Gets a list of the peer-ids that are subscribed to one topic.
 
 | Type | Description |
 |------|-------------|
-| `Array<String>` | peer-id subscribed to the topic |
+| `Array<string>` | peer-id subscribed to the topic |
 
 #### Example
 
@@ -891,7 +856,7 @@ Gets a list of topics the node is subscribed to.
 
 | Type | Description |
 |------|-------------|
-| `Array<String>` | topics the node is subscribed to |
+| `Array<string>` | topics the node is subscribed to |
 
 #### Example
 
@@ -938,7 +903,7 @@ Subscribes the given handler to a pubsub topic.
 | Name | Type | Description |
 |------|------|-------------|
 | topic | `string` | topic to subscribe |
-| handler | `function({ from: String, data: Buffer, seqno: Buffer, topicIDs: Array<String>, signature: Buffer, key: Buffer })` | handler for new data on topic |
+| handler | `function({ from: string, data: Buffer, seqno: Buffer, topicIDs: Array<string>, signature: Buffer, key: Buffer })` | handler for new data on topic |
 
 #### Returns
 
@@ -967,7 +932,7 @@ Unsubscribes the given handler from a pubsub topic. If no handler is provided, a
 
 | Name | Type | Description |
 |------|------|-------------|
-| topic | `string` | topic to unsubscribe |
+| topic | `String` | topic to unsubscribe |
 | handler | `function(<Object>)` | handler subscribed |
 
 #### Returns

diff --git a/src/identify/index.js b/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(),

diff --git a/src/peer-store/README.md b/src/peer-store/README.md
@@ -113,7 +113,6 @@ A `peerId.toString()` identifier mapping to a `Set` of protocol identifier strin
 - [`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
 

diff --git a/src/peer-store/proto-book.js b/src/peer-store/proto-book.js
@@ -144,34 +144,6 @@ class ProtoBook extends Book {
 
     return protocols
   }
-
-  /**
-   * Verify if the provided peer supports the given protocols.
-   * @param {PeerId} peerId
-   * @param {Array<string>|string} protocols
-   * @returns {boolean}
-   */
-  supports (peerId, protocols) {
-    if (!PeerId.isPeerId(peerId)) {
-      throw errcode(new Error('peerId must be an instance of peer-id'), ERR_INVALID_PARAMETERS)
-    }
-
-    if (!protocols) {
-      throw errcode(new Error('protocols must be provided'), ERR_INVALID_PARAMETERS)
-    }
-
-    if (!Array.isArray(protocols)) {
-      protocols = [protocols]
-    }
-
-    const recSet = this.data.get(peerId.toString())
-
-    if (!recSet) {
-      return false
-    }
-
-    return [...recSet].filter((p) => protocols.includes(p)).length === protocols.length
-  }
 }
 
 module.exports = ProtoBook
diff --git a/test/peer-store/proto-book.spec.js b/test/peer-store/proto-book.spec.js
@@ -238,53 +238,6 @@ describe('protoBook', () => {
     })
   })
 
-  describe('protoBook.supports', () => {
-    let ee, pb
-
-    beforeEach(() => {
-      ee = new EventEmitter()
-      pb = new ProtoBook(ee)
-    })
-
-    it('throwns invalid parameters error if invalid PeerId is provided', () => {
-      expect(() => {
-        pb.supports('invalid peerId')
-      }).to.throw(ERR_INVALID_PARAMETERS)
-    })
-
-    it('throwns invalid parameters error if no protocols provided', () => {
-      expect(() => {
-        pb.supports(peerId)
-      }).to.throw(ERR_INVALID_PARAMETERS)
-    })
-
-    it('returns false if no records exist for the peer', () => {
-      const supportedProtocols = ['protocol1']
-      const supports = pb.supports(peerId, supportedProtocols[0])
-
-      expect(supports).to.equal(false)
-    })
-
-    it('returns true if the protocol is supported', () => {
-      const supportedProtocols = ['protocol1', 'protocol2']
-
-      pb.set(peerId, supportedProtocols)
-
-      const supports = pb.supports(peerId, supportedProtocols[1])
-      expect(supports).to.equal(true)
-    })
-
-    it('returns false if part of the protocols is supported', () => {
-      const supportedProtocols = ['protocol1', 'protocol2']
-      const otherProtocols = ['protocol3', 'protocol4']
-
-      pb.set(peerId, supportedProtocols)
-
-      const supports = pb.supports(peerId, [supportedProtocols[0], ...otherProtocols])
-      expect(supports).to.equal(false)
-    })
-  })
-
   describe('protoBook.delete', () => {
     let ee, pb