-
Notifications
You must be signed in to change notification settings - Fork 541
chore: auto relay multiaddr update push #748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
ec47310
5fe1d8f
2b6c231
c0a01df
4c34155
7ef8044
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ const { IdentifyService, multicodecs } = require('../../src/identify') | |
| const Peers = require('../fixtures/peers') | ||
| const Libp2p = require('../../src') | ||
| const Envelope = require('../../src/record/envelope') | ||
| const PeerRecord = require('../../src/record/peer-record') | ||
| const PeerStore = require('../../src/peer-store') | ||
| const baseOptions = require('../utils/base-options.browser') | ||
| const pkg = require('../../package.json') | ||
|
|
@@ -78,6 +79,9 @@ describe('Identify', () => { | |
| sinon.spy(localIdentify.peerStore.addressBook, 'consumePeerRecord') | ||
| sinon.spy(localIdentify.peerStore.protoBook, 'set') | ||
|
|
||
| // Transport Manager creates signed peer record | ||
| await _createSelfPeerRecord(remoteIdentify._libp2p) | ||
|
|
||
| // Run identify | ||
| await Promise.all([ | ||
| localIdentify.identify(localConnectionMock), | ||
|
|
@@ -239,6 +243,10 @@ describe('Identify', () => { | |
| sinon.spy(remoteIdentify.peerStore.addressBook, 'consumePeerRecord') | ||
| sinon.spy(remoteIdentify.peerStore.protoBook, 'set') | ||
|
|
||
| // Transport Manager creates signed peer record | ||
| await _createSelfPeerRecord(localIdentify._libp2p) | ||
| await _createSelfPeerRecord(remoteIdentify._libp2p) | ||
|
|
||
| // Run identify | ||
| await Promise.all([ | ||
| localIdentify.push([localConnectionMock]), | ||
|
|
@@ -249,7 +257,7 @@ describe('Identify', () => { | |
| }) | ||
| ]) | ||
|
|
||
| expect(remoteIdentify.peerStore.addressBook.consumePeerRecord.callCount).to.equal(1) | ||
| expect(remoteIdentify.peerStore.addressBook.consumePeerRecord.callCount).to.equal(2) | ||
| expect(remoteIdentify.peerStore.protoBook.set.callCount).to.equal(1) | ||
|
|
||
| const addresses = localIdentify.peerStore.addressBook.get(localPeer) | ||
|
|
@@ -359,8 +367,8 @@ describe('Identify', () => { | |
| expect(connection).to.exist() | ||
|
|
||
| // Wait for peer store to be updated | ||
| // Dialer._createDialTarget (add), Identify (consume), Create self (consume) | ||
| await pWaitFor(() => peerStoreSpyConsumeRecord.callCount === 2 && peerStoreSpyAdd.callCount === 1) | ||
| // Dialer._createDialTarget (add), Identify (consume) | ||
| await pWaitFor(() => peerStoreSpyConsumeRecord.callCount === 1 && peerStoreSpyAdd.callCount === 1) | ||
| expect(libp2p.identifyService.identify.callCount).to.equal(1) | ||
|
|
||
| // The connection should have no open streams | ||
|
|
@@ -404,5 +412,53 @@ describe('Identify', () => { | |
| // Verify the streams close | ||
| await pWaitFor(() => connection.streams.length === 0) | ||
| }) | ||
|
|
||
| it('should push multiaddr updates to an already connected peer', async () => { | ||
| libp2p = new Libp2p({ | ||
| ...baseOptions, | ||
| peerId | ||
| }) | ||
|
|
||
| await libp2p.start() | ||
|
|
||
| sinon.spy(libp2p.identifyService, 'identify') | ||
| sinon.spy(libp2p.identifyService, 'push') | ||
|
|
||
| const connection = await libp2p.dialer.connectToPeer(remoteAddr) | ||
| expect(connection).to.exist() | ||
| // Wait for nextTick to trigger the identify call | ||
| await delay(1) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? There is a wait immediately after this for identify.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it was a debugging remaining, just removing it |
||
|
|
||
| // Wait for identify to finish | ||
| await libp2p.identifyService.identify.firstCall.returnValue | ||
| sinon.stub(libp2p, 'isStarted').returns(true) | ||
|
|
||
| libp2p.peerStore.addressBook.add(libp2p.peerId, [multiaddr('/ip4/180.0.0.1/tcp/15001/ws')]) | ||
|
|
||
| // Verify the remote peer is notified of change | ||
| expect(libp2p.identifyService.push.callCount).to.equal(1) | ||
| for (const call of libp2p.identifyService.push.getCalls()) { | ||
| const [connections] = call.args | ||
| expect(connections.length).to.equal(1) | ||
| expect(connections[0].remotePeer.toB58String()).to.equal(remoteAddr.getPeerId()) | ||
| const results = await call.returnValue | ||
| expect(results.length).to.equal(1) | ||
| } | ||
|
|
||
| // Verify the streams close | ||
| await pWaitFor(() => connection.streams.length === 0) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| // Self peer record creating on Transport Manager simulation | ||
| const _createSelfPeerRecord = async (libp2p) => { | ||
| try { | ||
| const peerRecord = new PeerRecord({ | ||
| peerId: libp2p.peerId, | ||
| multiaddrs: libp2p.multiaddrs | ||
| }) | ||
| const envelope = await Envelope.seal(peerRecord, libp2p.peerId) | ||
| libp2p.peerStore.addressBook.consumePeerRecord(envelope) | ||
| } catch (_) {} | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this to a utility file? |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,24 +4,34 @@ | |
| const chai = require('chai') | ||
| chai.use(require('dirty-chai')) | ||
| const { expect } = chai | ||
| const sinon = require('sinon') | ||
|
|
||
| const AddressManager = require('../../src/address-manager') | ||
| const TransportManager = require('../../src/transport-manager') | ||
| const PeerStore = require('../../src/peer-store') | ||
| const Transport = require('libp2p-tcp') | ||
| const PeerId = require('peer-id') | ||
| const multiaddr = require('multiaddr') | ||
| const mockUpgrader = require('../utils/mockUpgrader') | ||
| const Peers = require('../fixtures/peers') | ||
| const addrs = [ | ||
| multiaddr('/ip4/127.0.0.1/tcp/0'), | ||
| multiaddr('/ip4/127.0.0.1/tcp/0') | ||
| ] | ||
|
|
||
| describe('Transport Manager (TCP)', () => { | ||
| let tm | ||
| let localPeer | ||
|
|
||
| before(async () => { | ||
| localPeer = await PeerId.createFromJSON(Peers[0]) | ||
| }) | ||
|
|
||
| before(() => { | ||
| tm = new TransportManager({ | ||
| libp2p: { | ||
| addressManager: new AddressManager({ listen: addrs }) | ||
| addressManager: new AddressManager({ listen: addrs }), | ||
| PeerStore: new PeerStore({ peerId: localPeer }) | ||
| }, | ||
| upgrader: mockUpgrader, | ||
| onConnection: () => {} | ||
|
|
@@ -40,10 +50,16 @@ describe('Transport Manager (TCP)', () => { | |
| }) | ||
|
|
||
| it('should be able to listen', async () => { | ||
| sinon.spy(tm, '_createSelfPeerRecord') | ||
|
|
||
| tm.add(Transport.prototype[Symbol.toStringTag], Transport) | ||
| await tm.listen(addrs) | ||
| expect(tm._listeners).to.have.key(Transport.prototype[Symbol.toStringTag]) | ||
| expect(tm._listeners.get(Transport.prototype[Symbol.toStringTag])).to.have.length(addrs.length) | ||
|
|
||
| // Created Self Peer record on new listen address | ||
| expect(tm._createSelfPeerRecord.callCount).to.equal(addrs.length) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be better to validate the record is being stored in the address book. Validating the correct record is being stored would also be useful. |
||
|
|
||
| // Ephemeral ip addresses may result in multiple listeners | ||
| expect(tm.getAddrs().length).to.equal(addrs.length) | ||
| await tm.close() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to return anything, nothing is consuming the result of this, so we should always return null.
nit: Calling this function
_updateSelfPeerRecordmight be a bit clearer about what it's doing.