chore: address review

libp2p · Oct 28, 2020 · c69f7d2 · c69f7d2
1 parent 3240570
commit c69f7d2
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 139 deletions.
diff --git a/doc/API.md b/doc/API.md
@@ -13,8 +13,7 @@
   * [`ping`](#ping)
   * [`multiaddrs`](#multiaddrs)
   * [`addressManager.getListenAddrs`](#addressmanagergetlistenaddrs)
-  * [`addressmger.getAnnounceAddrs`](#addressmanagergetannounceaddrs)
-  * [`addressManager.getNoAnnounceAddrs`](#addressmanagergetnoannounceaddrs)
+  * [`addressManager.getAnnounceAddrs`](#addressmanagergetannounceaddrs)
   * [`contentRouting.findProviders`](#contentroutingfindproviders)
   * [`contentRouting.provide`](#contentroutingprovide)
   * [`contentRouting.put`](#contentroutingput)
@@ -91,7 +90,7 @@ Creates an instance of Libp2p.
 |------|------|-------------|
 | options | `object` | libp2p options |
 | options.modules | [`Array<object>`](./CONFIGURATION.md#modules) | libp2p [modules](./CONFIGURATION.md#modules) to use |
-| [options.addresses] | `{ listen: Array<string>, announce: Array<string>, noAnnounce: Array<string> }` | Addresses for transport listening and to advertise to the network |
+| [options.addresses] | `{ listen: Array<string>, announce: Array<string>, announceFilter: (ma: Array<multiaddr>) => Array<multiaddr> }` | Addresses for transport listening and to advertise to the network |
 | [options.config] | `object` | libp2p modules configuration and core configuration |
 | [options.connectionManager] | [`object`](./CONFIGURATION.md#configuring-connection-manager) | libp2p Connection Manager [configuration](./CONFIGURATION.md#configuring-connection-manager) |
 | [options.transportManager] | [`object`](./CONFIGURATION.md#configuring-transport-manager) | libp2p transport manager [configuration](./CONFIGURATION.md#configuring-transport-manager) |
@@ -483,26 +482,6 @@ const announceMa = libp2p.addressManager.getAnnounceAddrs()
 // [ <Multiaddr 047f00000106f9ba - /dns4/peer.io/...> ]
 ```
 
-### addressManager.getNoAnnounceAddrs
-
-Get the multiaddrs that were provided to not announce to the network.
-
-`libp2p.addressManager.getNoAnnounceAddrs()`
-
-#### Returns
-
-| Type | Description |
-|------|-------------|
-| `Array<Multiaddr>` | Provided noAnnounce multiaddrs |
-
-#### Example
-
-```js
-// ...
-const noAnnounceMa = libp2p.addressManager.getNoAnnounceAddrs()
-// [ <Multiaddr 047f00000106f9ba - /ip4/127.0.0.1/tcp/63930> ]
-```
-
 ### transportManager.getAddrs
 
 Get the multiaddrs that libp2p transports are using to listen on.

diff --git a/doc/CONFIGURATION.md b/doc/CONFIGURATION.md
@@ -211,11 +211,10 @@ Besides the `modules` and `config`, libp2p allows other internal options and con
   - This is used in modules such as the DHT. If it is not provided, `js-libp2p` will use an in memory datastore.
 - `peerId`: the identity of the node, an instance of [libp2p/js-peer-id](https://github.com/libp2p/js-peer-id).
   - This is particularly useful if you want to reuse the same `peer-id`, as well as for modules like `libp2p-delegated-content-routing`, which need a `peer-id` in their instantiation.
-- `addresses`: an object containing `listen`, `announce` and `noAnnounce` properties with `Array<string>`:
+- `addresses`: an object containing `listen`, `announce` and `announceFilter`:
   - `listen` addresses will be provided to the libp2p underlying transports for listening on them.
   - `announce` addresses will be used to compute the advertises that the node should advertise to the network.
-  - `noAnnounce` addresses will be used as a filter to compute the advertises that the node should advertise to the network.
-  - `announceFilter`: filter function used to filter announced addresses programmatically: `(ma: Array<multiaddr>) => Array<multiaddr>`. Default: bypass all addresses. [`libp2p-utils`](https://github.com/libp2p/js-libp2p-utils) provides useful [multiaddr utilities](https://github.com/libp2p/js-libp2p-utils/blob/master/API.md#multiaddr-isloopbackma) to create your filters.
+  - `announceFilter`: filter function used to filter announced addresses programmatically: `(ma: Array<multiaddr>) => Array<multiaddr>`. Default: returns all addresses. [`libp2p-utils`](https://github.com/libp2p/js-libp2p-utils) provides useful [multiaddr utilities](https://github.com/libp2p/js-libp2p-utils/blob/master/API.md#multiaddr-isloopbackma) to create your filters.
 
 ### Examples
 

diff --git a/src/address-manager/index.js b/src/address-manager/index.js
@@ -7,24 +7,21 @@ log.error = debug('libp2p:addresses:error')
 const multiaddr = require('multiaddr')
 
 /**
- * Responsible for managing this peers addresses.
- * Peers can specify their listen, announce and noAnnounce addresses.
+ * Responsible for managing the peer addresses.
+ * Peers can specify their listen and announce addresses.
  * The listen addresses will be used by the libp2p transports to listen for new connections,
- * while the announce an noAnnounce addresses will be combined with the listen addresses for
- * address adverstising to other peers in the network.
+ * while the announce addresses will be used for the peer addresses' to other peers in the network.
  */
 class AddressManager {
   /**
    * @class
    * @param {object} [options]
    * @param {Array<string>} [options.listen = []] - list of multiaddrs string representation to listen.
    * @param {Array<string>} [options.announce = []] - list of multiaddrs string representation to announce.
-   * @param {Array<string>} [options.noAnnounce = []] - list of multiaddrs string representation to not announce.
    */
-  constructor ({ listen = [], announce = [], noAnnounce = [] } = {}) {
+  constructor ({ listen = [], announce = [] } = {}) {
     this.listen = new Set(listen)
     this.announce = new Set(announce)
-    this.noAnnounce = new Set(noAnnounce)
   }
 
   /**
@@ -44,15 +41,6 @@ class AddressManager {
   getAnnounceAddrs () {
     return Array.from(this.announce).map((a) => multiaddr(a))
   }
-
-  /**
-   * Get peer noAnnouncing multiaddrs.
-   *
-   * @returns {Array<Multiaddr>}
-   */
-  getNoAnnounceAddrs () {
-    return Array.from(this.noAnnounce).map((a) => multiaddr(a))
-  }
 }
 
 module.exports = AddressManager
diff --git a/src/index.js b/src/index.js
@@ -361,27 +361,15 @@ class Libp2p extends EventEmitter {
    * @returns {Array<Multiaddr>}
    */
   get multiaddrs () {
-    const announceFilter = this._options.addresses.announceFilter || ((multiaddrs) => multiaddrs)
+    const announceAddrs = this.addressManager.getAnnounceAddrs()
+    if (announceAddrs.length) {
+      return announceAddrs
+    }
 
-    // Filter noAnnounce multiaddrs
-    const filterMa = this.addressManager.getNoAnnounceAddrs()
+    const announceFilter = this._options.addresses.announceFilter || ((multiaddrs) => multiaddrs)
 
     // Create advertising list
-    return announceFilter(this.transportManager.getAddrs()
-      .concat(this.addressManager.getAnnounceAddrs())
-      .filter((ma, index, array) => {
-        // Filter out if repeated
-        if (array.findIndex((otherMa) => otherMa.equals(ma)) !== index) {
-          return false
-        }
-
-        // Filter out if in noAnnounceMultiaddrs
-        if (filterMa.find((fm) => fm.equals(ma))) {
-          return false
-        }
-
-        return true
-      }))
+    return announceFilter(this.transportManager.getAddrs())
   }
 
   /**

diff --git a/test/addresses/address-manager.spec.js b/test/addresses/address-manager.spec.js
@@ -16,7 +16,6 @@ describe('Address Manager', () => {
 
     expect(am.listen.size).to.equal(0)
     expect(am.announce.size).to.equal(0)
-    expect(am.noAnnounce.size).to.equal(0)
   })
 
   it('should return listen multiaddrs on get', () => {
@@ -26,7 +25,6 @@ describe('Address Manager', () => {
 
     expect(am.listen.size).to.equal(listenAddresses.length)
     expect(am.announce.size).to.equal(0)
-    expect(am.noAnnounce.size).to.equal(0)
 
     const listenMultiaddrs = am.getListenAddrs()
     expect(listenMultiaddrs.length).to.equal(2)
@@ -42,28 +40,11 @@ describe('Address Manager', () => {
 
     expect(am.listen.size).to.equal(listenAddresses.length)
     expect(am.announce.size).to.equal(announceAddreses.length)
-    expect(am.noAnnounce.size).to.equal(0)
 
     const announceMultiaddrs = am.getAnnounceAddrs()
     expect(announceMultiaddrs.length).to.equal(1)
     expect(announceMultiaddrs[0].equals(multiaddr(announceAddreses[0]))).to.equal(true)
   })
-
-  it('should return noAnnounce multiaddrs on get', () => {
-    const am = new AddressManager({
-      listen: listenAddresses,
-      noAnnounce: listenAddresses
-    })
-
-    expect(am.listen.size).to.equal(listenAddresses.length)
-    expect(am.announce.size).to.equal(0)
-    expect(am.noAnnounce.size).to.equal(listenAddresses.length)
-
-    const noAnnounceMultiaddrs = am.getNoAnnounceAddrs()
-    expect(noAnnounceMultiaddrs.length).to.equal(2)
-    expect(noAnnounceMultiaddrs[0].equals(multiaddr(listenAddresses[0]))).to.equal(true)
-    expect(noAnnounceMultiaddrs[1].equals(multiaddr(listenAddresses[1]))).to.equal(true)
-  })
 })
 
 describe('libp2p.addressManager', () => {
@@ -76,14 +57,12 @@ describe('libp2p.addressManager', () => {
       config: {
         addresses: {
           listen: listenAddresses,
-          announce: announceAddreses,
-          noAnnounce: listenAddresses
+          announce: announceAddreses
         }
       }
     })
 
     expect(libp2p.addressManager.listen.size).to.equal(listenAddresses.length)
     expect(libp2p.addressManager.announce.size).to.equal(announceAddreses.length)
-    expect(libp2p.addressManager.noAnnounce.size).to.equal(listenAddresses.length)
   })
 })
diff --git a/test/addresses/addresses.node.js b/test/addresses/addresses.node.js
@@ -4,6 +4,7 @@
 const { expect } = require('aegir/utils/chai')
 const sinon = require('sinon')
 
+const multiaddr = require('multiaddr')
 const isLoopback = require('libp2p-utils/src/multiaddr/is-loopback')
 
 const { AddressesOptions } = require('./utils')
@@ -44,108 +45,80 @@ describe('libp2p.multiaddrs', () => {
     expect(listenAddrs.has(listenAddresses[1])).to.equal(true)
   })
 
-  it('should advertise all addresses if noAnnounce addresses are not provided, but with correct ports', async () => {
+  it('should announce transport listen addresses if announce addresses are not provided', async () => {
     [libp2p] = await peerUtils.createPeer({
+      started: false,
       config: {
         ...AddressesOptions,
         addresses: {
-          listen: listenAddresses,
-          announce: announceAddreses
+          listen: listenAddresses
         }
       }
     })
 
-    const tmListen = libp2p.transportManager.getAddrs().map((ma) => ma.toString())
+    await libp2p.start()
 
-    const spyAnnounce = sinon.spy(libp2p.addressManager, 'getAnnounceAddrs')
-    const spyNoAnnounce = sinon.spy(libp2p.addressManager, 'getNoAnnounceAddrs')
-    const spyListen = sinon.spy(libp2p.addressManager, 'getListenAddrs')
-    const spyTranspMgr = sinon.spy(libp2p.transportManager, 'getAddrs')
+    const tmListen = libp2p.transportManager.getAddrs().map((ma) => ma.toString())
 
+    // Announce 2 listen (transport)
     const advertiseMultiaddrs = libp2p.multiaddrs.map((ma) => ma.toString())
-
-    expect(spyAnnounce).to.have.property('callCount', 1)
-    expect(spyNoAnnounce).to.have.property('callCount', 1)
-    expect(spyListen).to.have.property('callCount', 0) // Listen addr should not be used
-    expect(spyTranspMgr).to.have.property('callCount', 1)
-
-    // Announce 2 listen (transport) + 1 announce
-    expect(advertiseMultiaddrs.length).to.equal(3)
+    expect(advertiseMultiaddrs.length).to.equal(2)
     tmListen.forEach((m) => {
       expect(advertiseMultiaddrs).to.include(m)
     })
-    announceAddreses.forEach((m) => {
-      expect(advertiseMultiaddrs).to.include(m)
-    })
     expect(advertiseMultiaddrs).to.not.include(listenAddresses[0]) // Random Port switch
   })
 
-  it('should remove replicated addresses', async () => {
+  it('should only announce the given announce addresses when provided', async () => {
     [libp2p] = await peerUtils.createPeer({
+      started: false,
       config: {
         ...AddressesOptions,
         addresses: {
           listen: listenAddresses,
-          announce: [listenAddresses[1]]
+          announce: announceAddreses
         }
       }
     })
 
-    const advertiseMultiaddrs = libp2p.multiaddrs.map((ma) => ma.toString())
-
-    // Announce 2 listen (transport), ignoring duplicated in announce
-    expect(advertiseMultiaddrs.length).to.equal(2)
-  })
+    await libp2p.start()
 
-  it('should not advertise noAnnounce addresses', async () => {
-    const noAnnounce = [listenAddresses[1]]
-    ;[libp2p] = await peerUtils.createPeer({
-      config: {
-        ...AddressesOptions,
-        addresses: {
-          listen: listenAddresses,
-          announce: announceAddreses,
-          noAnnounce
-        }
-      }
-    })
+    const tmListen = libp2p.transportManager.getAddrs().map((ma) => ma.toString())
 
+    // Announce 2 listen (transport)
     const advertiseMultiaddrs = libp2p.multiaddrs.map((ma) => ma.toString())
-
-    // Announce 1 listen (transport) not in the noAnnounce and the announce
-    expect(advertiseMultiaddrs.length).to.equal(2)
-
-    announceAddreses.forEach((m) => {
-      expect(advertiseMultiaddrs).to.include(m)
-    })
-    noAnnounce.forEach((m) => {
-      expect(advertiseMultiaddrs).to.not.include(m)
+    expect(advertiseMultiaddrs.length).to.equal(announceAddreses.length)
+    advertiseMultiaddrs.forEach((m) => {
+      expect(tmListen).to.not.include(m)
+      expect(announceAddreses).to.include(m)
     })
   })
 
-  it('can filter out loopback addresses to announced by the announce filter', async () => {
+  it('can filter out loopback addresses by the announce filter', async () => {
     [libp2p] = await peerUtils.createPeer({
       started: false,
       config: {
         ...AddressesOptions,
         addresses: {
           listen: listenAddresses,
-          announce: announceAddreses,
           announceFilter: (multiaddrs) => multiaddrs.filter(m => !isLoopback(m))
         }
       }
     })
 
-    const listenAddrs = libp2p.addressManager.listen
-    expect(listenAddrs.size).to.equal(listenAddresses.length)
-    expect(listenAddrs.has(listenAddresses[0])).to.equal(true)
-    expect(listenAddrs.has(listenAddresses[1])).to.equal(true)
-
     await libp2p.start()
 
+    expect(libp2p.multiaddrs.length).to.equal(0)
+
+    // Stub transportManager addresses to add a public address
+    const stubMa = multiaddr('/ip4/120.220.10.1/tcp/1000')
+    sinon.stub(libp2p.transportManager, 'getAddrs').returns([
+      ...listenAddresses.map((a) => multiaddr(a)),
+      stubMa
+    ])
+
     const multiaddrs = libp2p.multiaddrs
-    expect(multiaddrs.length).to.equal(announceAddreses.length)
-    expect(multiaddrs.includes(listenAddresses[0])).to.equal(false)
-    expect(multiaddrs.includes(listenAddresses[1])).to.equal(false)
+    expect(multiaddrs.length).to.equal(1)
+    expect(multiaddrs[0].equals(stubMa)).to.eql(true)
   })
 })