chore: address review

libp2p · Jul 15, 2020 · 3dc300d · 3dc300d
1 parent fa45a6b
commit 3dc300d
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 115 deletions.
diff --git a/src/identify/consts.js b/src/identify/consts.js
@@ -6,5 +6,5 @@ module.exports.MULTICODEC_IDENTIFY = '/p2p/id/1.1.0'
 module.exports.MULTICODEC_IDENTIFY_PUSH = '/p2p/id/push/1.1.0'
 
 // Legacy
-module.exports.MULTICODEC_IDENTIFY_LEGACY = '/ipfs/id/1.0.0'
-module.exports.MULTICODEC_IDENTIFY_PUSH_LEGACY = '/ipfs/id/push/1.0.0'
+module.exports.MULTICODEC_IDENTIFY_1_0_0 = '/ipfs/id/1.0.0'
+module.exports.MULTICODEC_IDENTIFY_PUSH_1_0_0 = '/ipfs/id/push/1.0.0'
diff --git a/src/identify/index.js b/src/identify/index.js
@@ -22,9 +22,9 @@ const PeerRecord = require('../record/peer-record')
 
 const {
   MULTICODEC_IDENTIFY,
-  MULTICODEC_IDENTIFY_LEGACY,
+  MULTICODEC_IDENTIFY_1_0_0,
   MULTICODEC_IDENTIFY_PUSH,
-  MULTICODEC_IDENTIFY_PUSH_LEGACY,
+  MULTICODEC_IDENTIFY_PUSH_1_0_0,
   AGENT_VERSION,
   PROTOCOL_VERSION
 } = require('./consts')
@@ -97,26 +97,12 @@ class IdentifyService {
   push (connections) {
     const pushes = connections.map(async connection => {
       try {
-        const { protocol, stream } = await connection.newStream([MULTICODEC_IDENTIFY_PUSH, MULTICODEC_IDENTIFY_PUSH_LEGACY])
-
-        // Handle Legacy
-        if (protocol === MULTICODEC_IDENTIFY_PUSH_LEGACY) {
-          return pipe(
-            [{
-              listenAddrs: this._libp2p.multiaddrs.map((ma) => ma.buffer),
-              protocols: Array.from(this._protocols.keys())
-            }],
-            pb.encode(Message),
-            stream,
-            consume
-          )
-        }
-
-        const envelope = await this._getSelfPeerRecord()
-        const signedPeerRecord = envelope.marshal()
+        const { stream } = await connection.newStream([MULTICODEC_IDENTIFY_PUSH, MULTICODEC_IDENTIFY_PUSH_1_0_0])
+        const signedPeerRecord = await this._getSelfPeerRecord()
 
         await pipe(
           [{
+            listenAddrs: this._libp2p.multiaddrs.map((ma) => ma.buffer),
             signedPeerRecord,
             protocols: Array.from(this._protocols.keys())
           }],
@@ -159,7 +145,7 @@ class IdentifyService {
    * @returns {Promise<void>}
    */
   async identify (connection) {
-    const { protocol, stream } = await connection.newStream([MULTICODEC_IDENTIFY, MULTICODEC_IDENTIFY_LEGACY])
+    const { protocol, stream } = await connection.newStream([MULTICODEC_IDENTIFY, MULTICODEC_IDENTIFY_1_0_0])
     const [data] = await pipe(
       [],
       stream,
@@ -198,7 +184,7 @@ class IdentifyService {
     observedAddr = IdentifyService.getCleanMultiaddr(observedAddr)
 
     // LEGACY: differentiate message with SignedPeerRecord
-    if (protocol === MULTICODEC_IDENTIFY_LEGACY) {
+    if (protocol === MULTICODEC_IDENTIFY_1_0_0) {
       // Update peers data in PeerStore
       this.peerStore.addressBook.set(id, listenAddrs.map((addr) => multiaddr(addr)))
       this.peerStore.protoBook.set(id, protocols)
@@ -249,13 +235,11 @@ class IdentifyService {
   handleMessage ({ connection, stream, protocol }) {
     switch (protocol) {
       case MULTICODEC_IDENTIFY:
+      case MULTICODEC_IDENTIFY_1_0_0:
         return this._handleIdentify({ connection, stream })
-      case MULTICODEC_IDENTIFY_LEGACY:
-        return this._handleIdentifyLegacy({ connection, stream })
       case MULTICODEC_IDENTIFY_PUSH:
+      case MULTICODEC_IDENTIFY_PUSH_1_0_0:
         return this._handlePush({ connection, stream })
-      case MULTICODEC_IDENTIFY_PUSH_LEGACY:
-        return this._handlePushLegacy({ connection, stream })
       default:
         log.error('cannot handle unknown protocol %s', protocol)
     }
@@ -275,45 +259,14 @@ class IdentifyService {
       publicKey = this.peerId.pubKey.bytes
     }
 
-    const envelope = await this._getSelfPeerRecord()
-    const signedPeerRecord = envelope.marshal()
-
-    const message = Message.encode({
-      protocolVersion: PROTOCOL_VERSION,
-      agentVersion: AGENT_VERSION,
-      publicKey,
-      signedPeerRecord,
-      observedAddr: connection.remoteAddr.buffer,
-      protocols: Array.from(this._protocols.keys())
-    })
-
-    pipe(
-      [message],
-      lp.encode(),
-      stream,
-      consume
-    )
-  }
-
-  /**
-   * Sends the `Identify` response with listen addresses (LEGACY)
-   * to the requesting peer over the given `connection`
-   * @private
-   * @param {object} options
-   * @param {*} options.stream
-   * @param {Connection} options.connection
-   */
-  _handleIdentifyLegacy ({ connection, stream }) {
-    let publicKey = Buffer.alloc(0)
-    if (this.peerId.pubKey) {
-      publicKey = this.peerId.pubKey.bytes
-    }
+    const signedPeerRecord = await this._getSelfPeerRecord()
 
     const message = Message.encode({
       protocolVersion: PROTOCOL_VERSION,
       agentVersion: AGENT_VERSION,
       publicKey,
       listenAddrs: this._libp2p.multiaddrs.map((ma) => ma.buffer),
+      signedPeerRecord,
       observedAddr: connection.remoteAddr.buffer,
       protocols: Array.from(this._protocols.keys())
     })
@@ -353,6 +306,22 @@ class IdentifyService {
       return log.error('received invalid message', err)
     }
 
+    const id = connection.remotePeer
+
+    // Legacy
+    if (!message.signedPeerRecord) {
+      try {
+        this.peerStore.addressBook.set(id, message.listenAddrs.map((addr) => multiaddr(addr)))
+      } catch (err) {
+        return log.error('received invalid listen addrs', err)
+      }
+
+      // Update the protocols
+      this.peerStore.protoBook.set(id, message.protocols)
+
+      return
+    }
+
     // Open envelope and verify if is authenticated
     let envelope
     try {
@@ -372,7 +341,6 @@ class IdentifyService {
     }
 
     // Update peers data in PeerStore
-    const id = connection.remotePeer
     try {
       // TODO: Store as certified record
 
@@ -386,45 +354,8 @@ class IdentifyService {
   }
 
   /**
-   * Reads the Identify Push message from the given `connection`
-   * with listen addresses (LEGACY)
-   * @private
-   * @param {object} options
-   * @param {*} options.stream
-   * @param {Connection} options.connection
-   */
-  async _handlePushLegacy ({ connection, stream }) {
-    const [data] = await pipe(
-      [],
-      stream,
-      lp.decode(),
-      take(1),
-      toBuffer,
-      collect
-    )
-
-    let message
-    try {
-      message = Message.decode(data)
-    } catch (err) {
-      return log.error('received invalid message', err)
-    }
-
-    // Update peers data in PeerStore
-    const id = connection.remotePeer
-    try {
-      this.peerStore.addressBook.set(id, message.listenAddrs.map((addr) => multiaddr(addr)))
-    } catch (err) {
-      return log.error('received invalid listen addrs', err)
-    }
-
-    // Update the protocols
-    this.peerStore.protoBook.set(id, message.protocols)
-  }
-
-  /**
-   * Get self signed peer record envelope.
-   * @return {Envelope}
+   * Get self signed peer record raw envelope.
+   * @return {Buffer}
    */
   async _getSelfPeerRecord () {
     // TODO: Verify if updated
@@ -436,7 +367,9 @@ class IdentifyService {
       peerId: this.peerId,
       multiaddrs: this._libp2p.multiaddrs
     })
-    this._selfRecord = await Envelope.seal(peerRecord, this.peerId)
+    const envelope = await Envelope.seal(peerRecord, this.peerId)
+
+    this._selfRecord = envelope.marshal()
 
     return this._selfRecord
   }
@@ -449,8 +382,8 @@ module.exports.IdentifyService = IdentifyService
  */
 module.exports.multicodecs = {
   IDENTIFY: MULTICODEC_IDENTIFY,
-  IDENTIFY_LEGACY: MULTICODEC_IDENTIFY_LEGACY,
+  IDENTIFY_1_0_0: MULTICODEC_IDENTIFY_1_0_0,
   IDENTIFY_PUSH: MULTICODEC_IDENTIFY_PUSH,
-  IDENTIFY_PUSH_LEGACY: MULTICODEC_IDENTIFY_PUSH_LEGACY
+  IDENTIFY_PUSH_1_0_0: MULTICODEC_IDENTIFY_PUSH_1_0_0
 }
 module.exports.Message = Message
diff --git a/test/identify/index.spec.js b/test/identify/index.spec.js
@@ -30,8 +30,8 @@ const protocols = new Map([
 ])
 
 const protocolsLegacy = new Map([
-  [multicodecs.IDENTIFY_LEGACY, () => { }],
-  [multicodecs.IDENTIFY_PUSH_LEGACY, () => { }]
+  [multicodecs.IDENTIFY_1_0_0, () => { }],
+  [multicodecs.IDENTIFY_PUSH_1_0_0, () => { }]
 ])
 
 describe('Identify', () => {
@@ -49,7 +49,7 @@ describe('Identify', () => {
     sinon.restore()
   })
 
-  it('should be able to identify another peer with legacy protocol', async () => {
+  it('should be able to identify another peer with 1.0.0 legacy protocol', async () => {
     const localIdentify = new IdentifyService({
       libp2p: {
         peerId: localPeer,
@@ -81,7 +81,7 @@ describe('Identify', () => {
     const remoteConnectionMock = { remoteAddr: observedAddr }
 
     const [local, remote] = duplexPair()
-    sinon.stub(localConnectionMock, 'newStream').returns({ stream: local, protocol: multicodecs.IDENTIFY_LEGACY })
+    sinon.stub(localConnectionMock, 'newStream').returns({ stream: local, protocol: multicodecs.IDENTIFY_1_0_0 })
 
     sinon.spy(localIdentify.peerStore.addressBook, 'set')
     sinon.spy(localIdentify.peerStore.protoBook, 'set')
@@ -92,7 +92,7 @@ describe('Identify', () => {
       remoteIdentify.handleMessage({
         connection: remoteConnectionMock,
         stream: remote,
-        protocol: multicodecs.IDENTIFY_LEGACY
+        protocol: multicodecs.IDENTIFY_1_0_0
       })
     ])
 
@@ -214,7 +214,7 @@ describe('Identify', () => {
   })
 
   describe('push', () => {
-    it('should be able to push identify updates to another peer with legacy protocol', async () => {
+    it('should be able to push identify updates to another peer with 1.0.0 legacy protocols', async () => {
       const connectionManager = new EventEmitter()
       connectionManager.getConnection = () => {}
 
@@ -225,8 +225,8 @@ describe('Identify', () => {
           multiaddrs: listenMaddrs
         },
         protocols: new Map([
-          [multicodecs.IDENTIFY_LEGACY],
-          [multicodecs.IDENTIFY_PUSH_LEGACY],
+          [multicodecs.IDENTIFY_1_0_0],
+          [multicodecs.IDENTIFY_PUSH_1_0_0],
           ['/echo/1.0.0']
         ])
       })
@@ -247,12 +247,12 @@ describe('Identify', () => {
       })
 
       // Setup peer protocols and multiaddrs
-      const localProtocols = new Set([multicodecs.IDENTIFY_LEGACY, multicodecs.IDENTIFY_PUSH_LEGACY, '/echo/1.0.0'])
+      const localProtocols = new Set([multicodecs.IDENTIFY_1_0_0, multicodecs.IDENTIFY_PUSH_1_0_0, '/echo/1.0.0'])
       const localConnectionMock = { newStream: () => {} }
       const remoteConnectionMock = { remotePeer: localPeer }
 
       const [local, remote] = duplexPair()
-      sinon.stub(localConnectionMock, 'newStream').returns({ stream: local, protocol: multicodecs.IDENTIFY_PUSH_LEGACY })
+      sinon.stub(localConnectionMock, 'newStream').returns({ stream: local, protocol: multicodecs.IDENTIFY_PUSH_1_0_0 })
 
       sinon.spy(remoteIdentify.peerStore.addressBook, 'set')
       sinon.spy(remoteIdentify.peerStore.protoBook, 'set')
@@ -263,7 +263,7 @@ describe('Identify', () => {
         remoteIdentify.handleMessage({
           connection: remoteConnectionMock,
           stream: remote,
-          protocol: multicodecs.IDENTIFY_PUSH_LEGACY
+          protocol: multicodecs.IDENTIFY_PUSH_1_0_0
         })
       ])
 

diff --git a/test/pubsub/operation.node.js b/test/pubsub/operation.node.js
@@ -237,7 +237,6 @@ describe('Pubsub subsystem operates correctly', () => {
       const topic = 'test-topic'
       const data = 'hey!'
       const libp2pId = libp2p.peerId.toB58String()
-
       let counter = 0
       const defer1 = pDefer()
       const defer2 = pDefer()