Skip to content
This repository has been archived by the owner on Feb 26, 2021. It is now read-only.

Added distinctMultiaddr method #29

Merged
merged 5 commits into from
Nov 22, 2016
Merged

Added distinctMultiaddr method #29

merged 5 commits into from
Nov 22, 2016

Conversation

SidHarder
Copy link
Contributor

This method can be called from libp2p-swarm/lib/transport.js just before the createListeners method is called and may be a solution to issue ipfs/js-ipfs#228. This method returns an array of multiaddr which are distinct by port. Transport is not considered, if there are two transports with the same port the first multiaddr will be returned.

Question:

  1. Does this seem like a resonable solution?
  2. Should both port and transport be used to determine what is distinct?

@coveralls
Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage decreased (-4.5%) to 93.846% when pulling 0c40408 on SidHarder:master into 465ffab on libp2p:master.

@daviddias
Copy link
Member

Almost! :) Unfortunately (or fortunately, I still don't know if there is a valid reason), TCP and UDP ports are different and some kernels also offer SCTP, which I'm not 100% sure if they share the same port space as UDP, that being said, making it unique by port would filter out valid addresses.

You also might want to use multiple network interfaces with the same port

@SidHarder
Copy link
Contributor Author

I added to the transport object in js-multiaddr so that I could filter on it here.

If you have this:
"Addresses": {
"Swarm": [
"/ip4/0.0.0.0/tcp/4001",
"/ip6/::/tcp/4001",
"/ip4/0.0.0.0/udp/4001",
"/ip6/::/udp/4001"
],
"API": "/ip4/127.0.0.1/tcp/5001",
"Gateway": "/ip4/127.0.0.1/tcp/8080"
},

The filter will return this:
"/ip4/0.0.0.0/tcp/4001"
"/ip4/0.0.0.0/udp/4001"

@coveralls
Copy link

coveralls commented Nov 14, 2016

Coverage Status

Coverage decreased (-4.5%) to 93.846% when pulling e1bc4a4 on SidHarder:master into 465ffab on libp2p:master.

@daviddias
Copy link
Member

Looking good! Can you also add tests?

@SidHarder
Copy link
Contributor Author

Yes, I will work on adding some tests.

@SidHarder
Copy link
Contributor Author

diasdavid: There are two test that are failing for the repo now which I don't believe has anything to do with what I have been working on. After looking at the 2 tests I don't see how the test would ever pass. The 2 test are:

  1. add multiaddr that are buffers
  2. peer-info add multiaddr that are buffers:

Do you have any insight on this?

@@ -2,6 +2,7 @@

const Id = require('peer-id')
const multiaddr = require('multiaddr')
const _ = require('lodash')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const uniqBy = require('lodash.uniqBy') if that is the only feature you are going to use, it saves in bundle size :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

David, I have not seen a require like this: require('lodash.uniqBy'), did you mean like this: require('lodash').uniqBy? Or am I missing something?

@daviddias
Copy link
Member

CI looks fine to me, with the exception that SauceLabs is being wonky. where do you see those tests failing specifically @SidHarder ?

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 98.462% when pulling c6121ce on SidHarder:master into 64c1dc0 on libp2p:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 98.462% when pulling c6121ce on SidHarder:master into 64c1dc0 on libp2p:master.

@daviddias daviddias merged commit 8a6cc98 into libp2p:master Nov 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants