chore: apply suggestions from code review

libp2p · vasco-santos · Jul 23, 2020 · Jun 21, 2020 · Jun 29, 2020 · Jul 15, 2020
commit 5019a60758055588937724e8124b2cbc0ce9e232
diff --git a/src/peer-store/address-book.js b/src/peer-store/address-book.js
@@ -88,7 +88,7 @@ class AddressBook extends Book {
     }
 
     // Verify peerId
-    if (peerRecord.peerId.toB58String() !== envelope.peerId.toB58String()) {
+    if (!peerRecord.peerId.equals(envelope.peerId)) {
       log('signing key does not match PeerId in the PeerRecord')
       return false
     }
@@ -220,10 +220,10 @@ class AddressBook extends Book {
     const id = peerId.toB58String()
 
     const entry = this.data.get(id) || {}
-    const rec = entry.addresses
+    const rec = entry.addresses || []
 
     // Add recorded uniquely to the new array (Union)
-    rec && rec.forEach((addr) => {
+    rec.forEach((addr) => {
       if (!addresses.find(r => r.multiaddr.equals(addr.multiaddr))) {
         addresses.push(addr)
       }
@@ -244,7 +244,7 @@ class AddressBook extends Book {
     log(`added provided multiaddrs for ${id}`)
 
     // Notify the existance of a new peer
-    if (!rec) {
+    if (!entry.addresses) {
       this._ps.emit('peer', peerId)
     }
 

diff --git a/src/peer-store/index.js b/src/peer-store/index.js
@@ -37,7 +37,7 @@ class PeerStore extends EventEmitter {
   /**
    * @constructor
    */
-  constructor ({ peerId } = {}) {
+  constructor ({ peerId }) {
     super()
 
     this._peerId = peerId

diff --git a/src/peer-store/persistent/index.js b/src/peer-store/persistent/index.js
@@ -189,13 +189,15 @@ class PersistentPeerStore extends PeerStore {
 
       const encodedData = Addresses.encode({
         addrs: entry.addresses.map((address) => ({
-          multiaddr: address.multiaddr.buffer
+          multiaddr: address.multiaddr.buffer,
+          isCertified: address.isCertified
         })),
         certified_record: entry.record ? {
           seq: entry.record.seqNumber,
           raw: entry.record.raw
         } : undefined
       })
+
       batch.put(key, encodedData)
     } catch (err) {
       log.error(err)
@@ -302,7 +304,8 @@ class PersistentPeerStore extends PeerStore {
             peerId,
             {
               addresses: decoded.addrs.map((address) => ({
-                multiaddr: multiaddr(address.multiaddr)
+                multiaddr: multiaddr(address.multiaddr),
+                isCertified: Boolean(address.isCertified)
               })),
               record: decoded.certified_record ? {
                 raw: decoded.certified_record.raw,

diff --git a/src/peer-store/persistent/pb/address-book.proto.js b/src/peer-store/persistent/pb/address-book.proto.js
@@ -7,6 +7,9 @@ message Addresses {
   // Address represents a single multiaddr.
   message Address {
     required bytes multiaddr = 1;
+
+    // Flag to indicate if the address comes from a certified source.
+    optional bool isCertified = 2;
   }
 
   // CertifiedRecord contains a serialized signed PeerRecord used to

diff --git a/src/record/envelope/index.js b/src/record/envelope/index.js
@@ -112,7 +112,12 @@ const formatSignaturePayload = (domain, payloadType, payload) => {
   ])
 }
 
-const unmarshalEnvelope = async (data) => {
+/**
+ * Unmarshal a serialized Envelope protobuf message.
+ * @param {Buffer} data
+ * @return {Promise<Envelope>}
+ */
+Envelope.createFromProtobuf = async (data) => {
   const envelopeData = Protobuf.decode(data)
   const peerId = await PeerId.createFromPubKey(envelopeData.public_key)
 
@@ -124,13 +129,6 @@ const unmarshalEnvelope = async (data) => {
   })
 }
 
-/**
- * Unmarshal a serialized Envelope protobuf message.
- * @param {Buffer} data
- * @return {Promise<Envelope>}
- */
-Envelope.createFromProtobuf = unmarshalEnvelope
-
 /**
 * Seal marshals the given Record, places the marshaled bytes inside an Envelope
 * and signs it with the given peerId's private key.
@@ -163,7 +161,7 @@ Envelope.seal = async (record, peerId) => {
  * @return {Envelope}
  */
 Envelope.openAndCertify = async (data, domain) => {
-  const envelope = await unmarshalEnvelope(data)
+  const envelope = await Envelope.createFromProtobuf(data)
   const valid = await envelope.validate(domain)
 
   if (!valid) {

diff --git a/test/dialing/direct.node.js b/test/dialing/direct.node.js
@@ -55,7 +55,7 @@ describe('Dialing (direct, TCP)', () => {
     })
     remoteTM.add(Transport.prototype[Symbol.toStringTag], Transport)
 
-    peerStore = new PeerStore()
+    peerStore = new PeerStore({ peerId: remotePeerId })
     localTM = new TransportManager({
       libp2p: {},
       upgrader: mockUpgrader
@@ -97,13 +97,13 @@ describe('Dialing (direct, TCP)', () => {
   })
 
   it('should be able to connect to a given peer id', async () => {
-    const peerStore = new PeerStore()
+    const peerId = await PeerId.createFromJSON(Peers[0])
+    const peerStore = new PeerStore({ peerId })
     const dialer = new Dialer({
       transportManager: localTM,
       peerStore
     })
 
-    const peerId = await PeerId.createFromJSON(Peers[0])
     peerStore.addressBook.set(peerId, [remoteAddr])
 
     const connection = await dialer.connectToPeer(peerId)

diff --git a/test/dialing/direct.spec.js b/test/dialing/direct.spec.js
@@ -13,7 +13,6 @@ const Transport = require('libp2p-websockets')
 const Muxer = require('libp2p-mplex')
 const { NOISE: Crypto } = require('libp2p-noise')
 const multiaddr = require('multiaddr')
-const PeerId = require('peer-id')
 const AggregateError = require('aggregate-error')
 const { AbortError } = require('libp2p-interfaces/src/transport/errors')
 
@@ -24,7 +23,6 @@ const PeerStore = require('../../src/peer-store')
 const TransportManager = require('../../src/transport-manager')
 const Libp2p = require('../../src')
 
-const Peers = require('../fixtures/peers')
 const { MULTIADDRS_WEBSOCKETS } = require('../fixtures/browser')
 const mockUpgrader = require('../utils/mockUpgrader')
 const createMockConnection = require('../utils/mockConnection')
@@ -35,9 +33,11 @@ const remoteAddr = MULTIADDRS_WEBSOCKETS[0]
 describe('Dialing (direct, WebSockets)', () => {
   let localTM
   let peerStore
+  let peerId
 
-  before(() => {
-    peerStore = new PeerStore()
+  before(async () => {
+    [peerId] = await createPeerId()
+    peerStore = new PeerStore({ peerId })
     localTM = new TransportManager({
       libp2p: {},
       upgrader: mockUpgrader,
@@ -132,7 +132,6 @@ describe('Dialing (direct, WebSockets)', () => {
         }
       }
     })
-    const peerId = await PeerId.createFromJSON(Peers[0])
 
     const connection = await dialer.connectToPeer(peerId)
     expect(connection).to.exist()
@@ -149,7 +148,6 @@ describe('Dialing (direct, WebSockets)', () => {
         }
       }
     })
-    const peerId = await PeerId.createFromJSON(Peers[0])
 
     await expect(dialer.connectToPeer(peerId))
       .to.eventually.be.rejectedWith(AggregateError)
@@ -198,7 +196,6 @@ describe('Dialing (direct, WebSockets)', () => {
     const deferredDial = pDefer()
     sinon.stub(localTM, 'dial').callsFake(() => deferredDial.promise)
 
-    const [peerId] = await createPeerId()
     // Perform 3 multiaddr dials
     dialer.connectToPeer(peerId)
 
@@ -245,7 +242,6 @@ describe('Dialing (direct, WebSockets)', () => {
     })
 
     // Perform 3 multiaddr dials
-    const [peerId] = await createPeerId()
     const dialPromise = dialer.connectToPeer(peerId)
 
     // Let the call stack run
@@ -266,14 +262,9 @@ describe('Dialing (direct, WebSockets)', () => {
   })
 
   describe('libp2p.dialer', () => {
-    let peerId
     let libp2p
     let remoteLibp2p
 
-    before(async () => {
-      peerId = await PeerId.createFromJSON(Peers[0])
-    })
-
     afterEach(async () => {
       sinon.restore()
       libp2p && await libp2p.stop()

diff --git a/test/identify/index.spec.js b/test/identify/index.spec.js
@@ -50,7 +50,7 @@ describe('Identify', () => {
       libp2p: {
         peerId: localPeer,
         connectionManager: new EventEmitter(),
-        peerStore: new PeerStore(),
+        peerStore: new PeerStore({ peerId: localPeer }),
         multiaddrs: listenMaddrs
       },
       protocols
@@ -60,7 +60,7 @@ describe('Identify', () => {
       libp2p: {
         peerId: remotePeer,
         connectionManager: new EventEmitter(),
-        peerStore: new PeerStore(),
+        peerStore: new PeerStore({ peerId: remotePeer }),
         multiaddrs: listenMaddrs
       },
       protocols
@@ -103,7 +103,7 @@ describe('Identify', () => {
       libp2p: {
         peerId: localPeer,
         connectionManager: new EventEmitter(),
-        peerStore: new PeerStore(),
+        peerStore: new PeerStore({ peerId: localPeer }),
         multiaddrs: listenMaddrs
       },
       protocols
@@ -113,7 +113,7 @@ describe('Identify', () => {
       libp2p: {
         peerId: remotePeer,
         connectionManager: new EventEmitter(),
-        peerStore: new PeerStore(),
+        peerStore: new PeerStore({ peerId: remotePeer }),
         multiaddrs: listenMaddrs
       },
       protocols
@@ -156,7 +156,7 @@ describe('Identify', () => {
       libp2p: {
         peerId: localPeer,
         connectionManager: new EventEmitter(),
-        peerStore: new PeerStore(),
+        peerStore: new PeerStore({ peerId: localPeer }),
         multiaddrs: []
       },
       protocols
@@ -165,7 +165,7 @@ describe('Identify', () => {
       libp2p: {
         peerId: remotePeer,
         connectionManager: new EventEmitter(),
-        peerStore: new PeerStore(),
+        peerStore: new PeerStore({ peerId: remotePeer }),
         multiaddrs: []
       },
       protocols
@@ -202,7 +202,7 @@ describe('Identify', () => {
         libp2p: {
           peerId: localPeer,
           connectionManager: new EventEmitter(),
-          peerStore: new PeerStore(),
+          peerStore: new PeerStore({ peerId: localPeer }),
           multiaddrs: listenMaddrs
         },
         protocols: new Map([
@@ -215,7 +215,7 @@ describe('Identify', () => {
         libp2p: {
           peerId: remotePeer,
           connectionManager,
-          peerStore: new PeerStore(),
+          peerStore: new PeerStore({ peerId: remotePeer }),
           multiaddrs: []
         }
       })
@@ -263,7 +263,7 @@ describe('Identify', () => {
         libp2p: {
           peerId: localPeer,
           connectionManager: new EventEmitter(),
-          peerStore: new PeerStore(),
+          peerStore: new PeerStore({ peerId: localPeer }),
           multiaddrs: listenMaddrs
         },
         protocols: new Map([
@@ -276,7 +276,7 @@ describe('Identify', () => {
         libp2p: {
           peerId: remotePeer,
           connectionManager,
-          peerStore: new PeerStore(),
+          peerStore: new PeerStore({ peerId: remotePeer }),
           multiaddrs: []
         }
       })

diff --git a/test/peer-store/address-book.spec.js b/test/peer-store/address-book.spec.js
@@ -36,7 +36,7 @@ describe('addressBook', () => {
     let peerStore, ab
 
     beforeEach(() => {
-      peerStore = new PeerStore()
+      peerStore = new PeerStore({ peerId })
       ab = peerStore.addressBook
     })
 
@@ -150,7 +150,7 @@ describe('addressBook', () => {
     let peerStore, ab
 
     beforeEach(() => {
-      peerStore = new PeerStore()
+      peerStore = new PeerStore({ peerId })
       ab = peerStore.addressBook
     })
 
@@ -278,7 +278,7 @@ describe('addressBook', () => {
     let peerStore, ab
 
     beforeEach(() => {
-      peerStore = new PeerStore()
+      peerStore = new PeerStore({ peerId })
       ab = peerStore.addressBook
     })
 
@@ -313,7 +313,7 @@ describe('addressBook', () => {
     let peerStore, ab
 
     beforeEach(() => {
-      peerStore = new PeerStore()
+      peerStore = new PeerStore({ peerId })
       ab = peerStore.addressBook
     })
 
@@ -349,7 +349,7 @@ describe('addressBook', () => {
     let peerStore, ab
 
     beforeEach(() => {
-      peerStore = new PeerStore()
+      peerStore = new PeerStore({ peerId })
       ab = peerStore.addressBook
     })
 
@@ -405,9 +405,9 @@ describe('addressBook', () => {
   describe('certified records', () => {
     let peerStore, ab
 
-    describe('consumes successfully a valid peer record and stores its data', () => {
+    describe('consumes a valid peer record and stores its data', () => {
       beforeEach(() => {
-        peerStore = new PeerStore()
+        peerStore = new PeerStore({ peerId })
         ab = peerStore.addressBook
       })
 
@@ -600,7 +600,7 @@ describe('addressBook', () => {
 
     describe('fails to consume invalid peer records', () => {
       beforeEach(() => {
-        peerStore = new PeerStore()
+        peerStore = new PeerStore({ peerId })
         ab = peerStore.addressBook
       })
 

diff --git a/test/peer-store/key-book.spec.js b/test/peer-store/key-book.spec.js
@@ -19,7 +19,7 @@ describe('keyBook', () => {
 
   beforeEach(async () => {
     [peerId] = await peerUtils.createPeerId()
-    peerStore = new PeerStore()
+    peerStore = new PeerStore({ peerId })
     kb = peerStore.keyBook
   })