feat: Add support for multiaddr node with dns #62

vasco-santos · 2018-04-17T14:34:30Z

Aiming to support the js-ipfs-api to connect with a node using a DNS multiaddr, I created this PR. Basically, as the API uses the nodeAddress function, it must support all types of address, instead of supporting only thin waist addresses.

Closes ipfs-inactive/js-ipfs-http-client#712

victorb · 2018-04-17T15:15:03Z

src/index.js

  return {
-    family: (codes[0] === 41) ? 'IPv6' : 'IPv4',
+    family: family,


This should only be set to either the number 4 or 6. See https://nodejs.org/api/http.html#http_http_request_options_callback

Also, adding a test case with using whatever nodeAddress returns can be passed to http.request would be great, as that would have caught this.

I didn't know that. I will modify it, setting IPv6 for ip6, as well as dns6.

victorb · 2018-04-17T15:15:57Z

test/index.spec.js

@@ -443,11 +443,11 @@ describe('helpers', () => {
  })

  describe('.nodeAddress', () => {
-    it('throws on non thinWaistAddress', () => {
+    it('throws on non invalid node address', () => {


This is a double-negative, could be just throws on valid node address but the test title doesn't make sense. Either it's valid and doesn't throw, or it's invalid and throws.

yes, my bad!

victorb · 2018-04-17T15:16:24Z

test/index.spec.js

+
+    it('returns a node friendly address with dns', () => {
+      expect(
+        multiaddr('dns4/wss0.bootstrap.libp2p.io/tcp/443').nodeAddress()


This is not a valid multiaddr, missing the initial /

Not sure how this test can be passing currently? Seems some checks are missing, at least this test should be failing.

I will add the /.

Anyway. it is possible to do the following:

const ipfs = ipfsAPI('ip4/127.0.0.1/tcp/5002')

Even, with the code in master 😕

Let's add a test and checks that it starts with a / as well then

victorb · 2018-04-18T08:44:30Z

src/index.js

  const codes = this.protoCodes()
  const parts = this.toString().split('/').slice(1)
+
+  if (parts.length < 4) {
+    throw new Error('Multiaddr must be a valid address for nodeAddress.')


This validation is not strict enough. For us to be able to generate a nodeAddress (which is address + port + family) we need to have at least /ip4 or /ip6 or /dns4 or /dns6 with /tcp or /udp. Simply checking if it has 4 parts is not enough.

Agreed! Added those validations now

victorb · 2018-04-18T08:45:23Z

src/index.js

  return {
-    family: (codes[0] === 41) ? 'IPv6' : 'IPv4',
+    family: (codes[0] === 41 || codes[0] === 55) ? 'IPv6' : 'IPv4',


This seems still incorrect (and was before your change as well) as https://nodejs.org/api/http.html#http_http_request_options_callback says it has to be a number, not a string.

It is fixed now

victorb · 2018-04-18T08:45:52Z

test/index.spec.js

+        family: 'IPv4',
+        port: '443'
+      })
+    })
  })



Please also add a test case for checking if the multiaddr is missing the prefix with /

Test was added :)

victorb · 2018-04-18T09:46:15Z

src/index.js

@@ -37,6 +37,9 @@ const Multiaddr = withIs.proto(function (addr) {
     */
    this.buffer = codec.fromBuffer(addr)
  } else if (typeof addr === 'string' || addr instanceof String) {
+    if (addr.length > 0 && addr.charAt(0) !== '/') {
+      throw new Error('addr string must have the correct format')


Better error message would be: multiaddr "ip4/127.0.0.1/tcp/4001" must start with a "/"

We might as well tell the user what's wrong. Worth keeping in mind for future errors as well, if we can tell the user what's wrong exactly, we should.

Agreed and fixed

victorb · 2018-04-18T09:47:10Z

src/index.js

+  if (parts.length < 4 ||
+    (codes[0] !== 4 && codes[0] !== 41 && codes[0] !== 54 && codes[0] !== 55) ||
+    (parts[2] !== 'tcp' && parts[2] !== 'udp')) {
+    throw new Error('Multiaddr must be a valid address for nodeAddress.')


Same goes here, should give the user an idea of what's not, not just that it's invalid.

Agreed and fixed

victorb · 2018-04-18T13:54:23Z

src/index.js

@@ -37,6 +37,9 @@ const Multiaddr = withIs.proto(function (addr) {
     */
    this.buffer = codec.fromBuffer(addr)
  } else if (typeof addr === 'string' || addr instanceof String) {
+    if (addr.length > 0 && addr.charAt(0) !== '/') {
+      throw new Error(`multiAddr "${addr}" must start with a "/"`)


Last mile! Typo multiAddr should be multiaddr

victorb · 2018-04-18T13:54:33Z

src/index.js

  const parts = this.toString().split('/').slice(1)
+
+  if (parts.length < 4) {
+    throw new Error('Multiaddr must have a valid format: "{ip4, ip6, dns4, dns6}/{address}/{tcp, udp}/{port}".')


Last mile! Typo Multiaddr should be multiaddr

Oh, and valid format for a multiaddr is to start with / ;)

daviddias

LGTM, thanks for making this @vasco-santos and thanks for reviewing it @victorbjelkholm :)

This is a breaking change (e.g IPv4 -> 4) and so it means that we will have to update pretty much all the modules 😢 I'll start with this one.

vasco-santos mentioned this pull request Apr 17, 2018

Using a multiaddr with DNS fails connect to IPFS node ipfs-inactive/js-ipfs-http-client#712

Closed

victorb suggested changes Apr 17, 2018

View reviewed changes

vasco-santos force-pushed the feat_add-support-for-multiaddr-with-dns branch from ddd3e71 to 65fc669 Compare April 17, 2018 15:33

victorb suggested changes Apr 18, 2018

View reviewed changes

vasco-santos force-pushed the feat_add-support-for-multiaddr-with-dns branch from 65fc669 to 568f018 Compare April 18, 2018 09:18

victorb suggested changes Apr 18, 2018

View reviewed changes

vasco-santos force-pushed the feat_add-support-for-multiaddr-with-dns branch from 568f018 to 53af336 Compare April 18, 2018 11:12

victorb reviewed Apr 18, 2018

View reviewed changes

feat: Add support for multiaddr node with dns

f193d46

vasco-santos force-pushed the feat_add-support-for-multiaddr-with-dns branch from 53af336 to f193d46 Compare April 19, 2018 08:54

daviddias approved these changes Apr 24, 2018

View reviewed changes

daviddias merged commit 5d6b93a into master Apr 24, 2018

daviddias deleted the feat_add-support-for-multiaddr-with-dns branch April 24, 2018 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for multiaddr node with dns #62

feat: Add support for multiaddr node with dns #62

vasco-santos commented Apr 17, 2018

victorb Apr 17, 2018

vasco-santos Apr 17, 2018

victorb Apr 17, 2018

vasco-santos Apr 17, 2018

victorb Apr 17, 2018

victorb Apr 17, 2018

vasco-santos Apr 17, 2018

victorb Apr 17, 2018

victorb Apr 18, 2018

vasco-santos Apr 18, 2018

victorb Apr 18, 2018

vasco-santos Apr 18, 2018

victorb Apr 18, 2018

vasco-santos Apr 18, 2018

victorb Apr 18, 2018

vasco-santos Apr 18, 2018

victorb Apr 18, 2018

vasco-santos Apr 18, 2018

victorb Apr 18, 2018

vasco-santos Apr 19, 2018

victorb Apr 18, 2018

victorb Apr 18, 2018

vasco-santos Apr 19, 2018

daviddias left a comment

feat: Add support for multiaddr node with dns #62

feat: Add support for multiaddr node with dns #62

Conversation

vasco-santos commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment