feat: ipns locally by vasco-santos · Pull Request #1400 · ipfs/js-ipfs

vasco-santos · 2018-06-19T15:58:24Z

A working version of IPNS working locally is here! 🚀 😎 💪

Steps:

Some notes, regarding the previous steps:

There is an optional parameter not implemented in this PR, which is ttl, since it is still experimental, we can add it in a separate PR.

Finally, thanks @Stebalien for your help and time answering all my questions regarding the IPNS implementation in GO.

Moreover, since there are no specs, and not that much documentation, I have been writing a document with all the IPNS workflow. It is a WIP by now, but it is currently available here.

Related PRs:

hugomrdias · 2018-06-19T16:17:02Z

src/core/ipns/pb/ipnsEntry.js

+    return ipnsEntryProto.decode(marsheled)
+  },
+  validityType: ipnsEntryProto.ValidityType
+}


module.exports = { create, marshal : ipnsEntryProto.encode, unmarshal : ipnsEntryProto.decode, validityType: ipnsEntryProto.ValidityType }

hugomrdias · 2018-06-19T16:18:39Z

src/core/components/name.js

+const human = require('human-to-milliseconds')
+const path = require('../ipns/path')
+
+const OFFLINE_ERROR = require('../utils').OFFLINE_ERROR


const { OFFLINE_ERROR } = require('../utils')

Actually, I think it's better to:

const ERRORS = require('../utils')

so when you use it bellow is more meaningful:

return callback(new Error(ERROR.OFFLINE_ERROR))

It is really strange to me that the errors are in a utils.js file, making those imports really strange. In the future, all errors should be easily accessible from all the project, and we should access them in the same way as @fsdiogo suggested.

hugomrdias · 2018-06-19T16:19:01Z

src/core/ipns/index.js

@@ -0,0 +1,54 @@
+'use strict'
+
+const peerId = require('peer-id')


const { createFromPrivKey } = require('peer-id')

hugomrdias · 2018-06-19T16:28:04Z

src/core/ipns/validator.js

+    }
+
+    // Validate EOL
+    const validityDate = Date.parse(validity.toString())


consider using https://date-fns.org/

FYI, the only valid time format is RFC3339 with nanosecond precision. That is:

2006-01-02T15:04:05.999999999Z07:00

Note: It's extremely important that you only except strings of this exact form. Not validating this has unfortunate security implications.

This also needs to check the validity type.

I will support milliseconds precision at first here and I added a task to improve the precision to nanosecond later (in the list above).

Basically, JS does not support nanoseconds precision natively and all the libs that I managed to find don't support summing date times with nanosecond precision.

When you say that I must validate if the string has this exact form, you say that it should be compliant with RFC3339, right?

fsdiogo · 2018-06-19T16:49:56Z

src/core/components/name.js

+const human = require('human-to-milliseconds')
+const path = require('../ipns/path')
+
+const OFFLINE_ERROR = require('../utils').OFFLINE_ERROR


Actually, I think it's better to:

const ERRORS = require('../utils')

so when you use it bellow is more meaningful:

return callback(new Error(ERROR.OFFLINE_ERROR))

Stebalien · 2018-06-19T19:22:02Z

src/core/components/name.js

+
+      // Parse ipfs path value
+      try {
+        value = path.parsePath(value)


Note: the value can be any arbitrary path.

Stebalien · 2018-06-19T19:40:45Z

src/core/ipns/resolver.js

+  }
+
+  // https://github.com/ipfs/go-ipfs-routing/blob/master/offline/offline.go
+  resolveLocal (name, publicKey, callback) {


Eventually, this should be pushed into a non-IPNS specific routing layer.

I had been thinking about the go-ipfs implementation for the resolver.

For one side, I agree with your approach. That is, you decide which router the IPNS resolver should use, and this router is completely transparent for the IPNS resolver. However, as we are fetching from an offline and local source (datastore), it also seems strange to me to have the OfflineRouter.

Anyway, I am not sure about where this code should live on. @diasdavid any opinion here?

Basically, it means that we can swap out the router at will. However, given the fact that you're only implementing offline routing, it doesn't really make a difference.

Being a pluggable router is a considerable advantage. But yeah, that's what I thought while putting the resolveLocal in there.

However, when I integrate the routing I will have to understand if it still makes sense to have the resolveLocal inside the IPNS layer.

Stebalien · 2018-06-19T19:42:00Z

src/core/ipns/validator.js

+    }
+
+    // Validate EOL
+    const validityDate = Date.parse(validity.toString())


Note: It's extremely important that you only except strings of this exact form. Not validating this has unfortunate security implications.

This also needs to check the validity type.

Stebalien · 2018-06-21T21:50:09Z

src/core/ipns/validator.js


    // Validate EOL
-    const validityDate = Date.parse(validity.toString())
+    if (validityType === IpnsEntry.validityType.EOL) {


We should fail by default if we can't validate the record.

already done in js-ipns, more precisely here

Stebalien · 2018-06-21T21:53:13Z

src/core/ipns/validator.js

    // Validate EOL
-    const validityDate = Date.parse(validity.toString())
+    if (validityType === IpnsEntry.validityType.EOL) {
+      const validityDate = Date.parse(validity.toString())


So it doesn't get lost in the buried review comments, this needs to explicitly check the date format.

Once js-ipns initial implementation PR is merged, I will create an issue in there for tackling this with priority. I have been thinking a lot earlier and reading some stuff about bigint for js and I think that I reached a way to add the nanoseconds precision here. I also need to find a way to explicitly check the date format according to the RFC 3339

I wouldn't feel comfortable merging this until this is fixed. Unfortunately, due to some mistakes in how we make signatures in IPNS, we rely on precisely validating the date for security.

@Stebalien just added it to js-ipns PR!

daviddias · 2018-06-21T22:44:35Z

@Stebalien why go-ipns and not go-libp2p-ipns? Did I miss any important convo?

Stebalien · 2018-06-21T23:43:47Z

why go-ipns and not go-libp2p-ipns? Did I miss any important convo?

As far as I know, IPNS falls under the umbrella of IPFS, not libp2p.

vasco-santos · 2018-06-22T11:00:55Z

why go-ipns and not go-libp2p-ipns? Did I miss any important convo?

As far as I know, IPNS falls under the umbrella of IPFS, not libp2p.

I see IPNS as a different piece than IPFS and libp2p. But as we don't have an IPNS umbrella, I don't know which would suit best here.

By the way @Stebalien and @diasdavid , if one day we decide to completely move IPNS (publish, resolve) to a new repo, this repo should be called js-ipns / go-ipns. This way, I think the current go-ipns and js-ipns should be ipns-record. What do you think?

daviddias · 2018-06-22T14:20:44Z

I know that there is no spec, but IPNS has been libp2p all along -- https://github.com/libp2p/specs/blob/master/4-architecture.md#46-naming

Anyway. @vasco-santos, don't forget about the interop tests! You should be able to publish stuff with a go daemon and then read it from the repo with a js-ipfs daemon.

vasco-santos · 2018-06-22T14:55:36Z

@diasdavid I will change to the libp2porganization then. Do you think it should be js-libp2p-ipns or js-libp2p-ipns-record? I think that the second one suits better because this module does not have the IPNS core code (publish and resolve), but utilities for creating, parsing, and validating IPNS records, which are used for the publish and resolve. The go-ipfs implementation has the IPNS core code, but I think that we could have the js-libp2p-ipns with the core implementation of publish and resolve. Unless @Stebalien have other ideas in here.

Yeah @diasdavid ! I have been testing everything to guarantee the interoperability between both. There are two fields which I am not sure right now, which are the record signature and the validity.
The signature is really difficult to test now, since it is stored in bytes, and I believe that I don't have a clean way to test this, while not having IPNS routing (I tried to copy the go-ipfs repo content and use it in js for validating the signature, but it seems that the repos are not compatible). Anyway, I have the interoperability task written in the first comment of this PR and once the PR initial implementation of the "js-libp2p-ipns-record" gets merged, I will create an issue to confirm the interoperabillity in that repo.

daviddias · 2018-06-22T15:04:14Z

@vasco-santos mind doing a drawing of the IPNS architecture first as you see it so that we clearly sync what are the pieces and their respective names?

vasco-santos · 2018-06-22T15:41:24Z

Here I have a small diagram with the interactions that I was describing. Or do you want a more low-level diagram with something like "how ipns works?"

Stebalien · 2018-06-22T19:34:38Z

I know that there is no spec, but IPNS has been libp2p all along

That's a bit odd. I guess one could, e.g., make an IPNS record point to a multiaddr but I still don't feel like it fits in libp2p. It really does feel like a separate thing. I can move it, I just don't want to shuffle things back and forth.

As for lang-ipns versus lang-ipns-record: ipfs/go-ipns#1

However, I held off on that as IPNS is record-store independent. That is, I'd almost expect:

{go,js}-ipns-interface: the publisher/resolver interfaces.
{go,js}-ipns-value-store: an implementation of the publisher/resolver interfaces that, internally, uses a valuestore (or a record store, depending on what you want to call it).
{go,js}-ipns-record: the actual IPNS record logic (this package).

vasco-santos · 2018-06-25T10:30:12Z

@Stebalien I would go with that package naming proposal!

daviddias · 2018-06-26T18:00:25Z

My claim (and others) is that IPNS is just a specialized version of IPRS and that naming is essential piece to Networking and therefore it should be part of libp2p primitives. But you know what, I'll let you paint the bikeshed the color you want as long as we keep making progress here and we having IPNS working locally, over PubSub and DHT as soon as possible :)

alanshaw

All green apart from commitlint! 💚

ghost assigned vasco-santos Jun 19, 2018

ghost added the status/in-progress In progress label Jun 19, 2018

vasco-santos requested review from Stebalien, alanshaw, daviddias, fsdiogo and hugomrdias June 19, 2018 15:58

vasco-santos mentioned this pull request Jun 19, 2018

Implement IPNS #209

Closed

vasco-santos force-pushed the feat/ipns-locally branch from 53e7b95 to b9ed2f0 Compare June 19, 2018 16:15

hugomrdias previously requested changes Jun 19, 2018

View reviewed changes

fsdiogo previously requested changes Jun 19, 2018

View reviewed changes

Stebalien reviewed Jun 19, 2018

View reviewed changes

vasco-santos mentioned this pull request Jun 21, 2018

ipfs.name.publish and ipfs.name.resolve is error ipfs/js-ipfsd-ctl#273

Closed

alanshaw changed the title ~~WIP: ipns locally~~ [WIP] feat: ipns locally Jun 21, 2018

vasco-santos mentioned this pull request Jun 21, 2018

feat: initial implementation ipfs/js-ipns#1

Merged

Stebalien reviewed Jun 21, 2018

View reviewed changes

This was referenced Jun 25, 2018

Consider renaming to go-ipns-record ipfs/go-ipns#1

Closed

Taking control of Error Handling, reducing js-ipfs Daemon crashes #1406

Closed

Seeking Presentations on These Topics libp2p/developer-meetings#10

Open

vasco-santos added 25 commits August 29, 2018 17:06

updated tests

7bcc648

fix: updated core arguments to an options object

fab0b76

fix: core returns and refactor tests

bc4a9ee

fix: core with camelCase responses

7f2aa3d

fix: add initializeKeyspace to init

fdf3b90

wip

8204f85

wip

61f4d4e

added republisher

08a952b

chore: updated readme

ba7585d

chore: added tests

6e3c435

chore: upgrade dependency of ipns

b2253e1

fix: lint

17bcdcb

fix: code review

1523579

fix: tests

04bf892

fix: republish cancelable

0515553

chore: added tests for path

36e51fb

fix: lint

87e9d91

fix: code review

df4a276

chore: refactored errors

49725ae

chore: upgrade interface-ipfs-core

ca25516

fix: initial rebroadcast delay

23b5fbc

fix: initial rebroadcast delay

6bb6c97

chore: upgrade ipns dependency

7cd8fc5

fix: added not implemented error for routing on ipns getPublished

84ee2c8

fix: republish working for starting and stopping multiple times

77caec1

vasco-santos force-pushed the feat/ipns-locally branch from 734b4b9 to 77caec1 Compare August 29, 2018 16:07

alanshaw approved these changes Aug 29, 2018

View reviewed changes

alanshaw merged commit 1110e96 into master Aug 29, 2018

ghost removed the status/in-progress In progress label Aug 29, 2018

alanshaw deleted the feat/ipns-locally branch August 29, 2018 17:02

		@@ -0,0 +1,54 @@
		'use strict'

		const peerId = require('peer-id')

Conversation

vasco-santos commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviddias commented Jun 21, 2018

Uh oh!

Stebalien commented Jun 21, 2018

Uh oh!

vasco-santos commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daviddias commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasco-santos commented Jun 22, 2018

Uh oh!

daviddias commented Jun 22, 2018

Uh oh!

vasco-santos commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stebalien commented Jun 22, 2018

Uh oh!

vasco-santos commented Jun 25, 2018

Uh oh!

daviddias commented Jun 26, 2018

Uh oh!

alanshaw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

vasco-santos commented Jun 19, 2018 •

edited

Loading

vasco-santos commented Jun 22, 2018 •

edited

Loading

daviddias commented Jun 22, 2018 •

edited

Loading

vasco-santos commented Jun 22, 2018 •

edited

Loading