feat: ipns working locally #327

vasco-santos · 2018-07-16T14:20:03Z

PR in the context of 1400 - IPNS working locally

alanshaw · 2018-07-20T10:13:49Z

js/src/name/publish.js

+          expect(err).to.not.exist()
+
+          ipfs = node
+          ipfs.id().then((res) => {


Please use spawnNodeWithId to simplify this!

I didn't know that function! Thanks, I will use it :)

alanshaw · 2018-07-20T10:15:09Z

js/src/name/publish.js

+
+    after((done) => common.teardown(done))
+
+    it('name publish should publish correctly', (done) => {


"name publish should publish correctly" => "should publish correctly"

Instead of writing "correctly" we should write what we're expecting with the test cases. "correctly" is up to the eye of the beholder :)

Also, if should is something we're adding before every title, we should just remove it. So this case ends up being "Publish correctly", which gives me the hint that the test is either trying to span too many things or is simply irrelevant.

Note: irrelevant as in the other tests will tell us exactly what "correctly" means, so might be enough with the rest of the tests.

I added some information, in order to be more understandable:

should publish an IPNS record with the default params

I believe this way is better 😄

alanshaw · 2018-07-20T10:15:46Z

js/src/name/publish.js

+      })
+    })
+
+    it('name publish should publish correctly when the file was not added but resolve is disabled', (done) => {


"name publish should publish correctly when the file was not added but resolve is disabled" => "should publish correctly when the file was not added but resolve is disabled"

alanshaw · 2018-07-20T10:16:03Z

js/src/name/publish.js

+      })
+    })
+
+    it('name publish should publish correctly when a new key is used', (done) => {


"name publish should publish correctly when a new key is used" => "should publish correctly when a new key is used"

alanshaw · 2018-07-20T10:18:07Z

js/src/name/publish.js

+    it('name publish should publish correctly', (done) => {
+      const value = fixtures.files[0].cid
+
+      ipfs.name.publish(value, true, '1m', '10s', 'self', (err, res) => {


This doesn't follow the signature described in https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/NAME.md#namepublish - should the parameters other than value and the callback be in an options object?

true! I will modify it

alanshaw · 2018-07-20T10:29:30Z

js/src/name/resolve.js

+          ipfs.files.add(file.data, { pin: false }, cb)
+        }, done)
+      }
+    })


Please see comments for the before block in name.publish - I think they mostly all apply here too.

alanshaw · 2018-07-20T10:30:06Z

js/src/name/resolve.js

+
+    after((done) => common.teardown(done))
+
+    it('name resolve should resolve correctly after a publish', (done) => {


Please remove the "name resolve " prefix from all of these test names so that they start with "should" :)

Disagree, no need to add "should" before every test case, it's just mindless repetition for no gain.

I prefer having "should" as you can the really read it and check if it makes sense. I've seen too many test descriptions in my past lives where it wasn't really clear if it's the test or the error condition it's describing.

All the tests are defining what the test "should" do, that's the point of the tests. Now it's not a super big deal, so I'm fine with that if that's what we end up deciding.

where it wasn't really clear if it's the test or the error condition it's describing.

This is a separate issue though, and has nothing to do with if the test name is prefixed with "should" or not.

I also prefer to have should prefixing the test description, as it seems more readable. But I can also remove it if we decide that.

All the tests are defining what the test "should" do

...or what they should not do 😉

I think it's good practice to encourage people to express tests in terms of what they should or should not do. I've seen tests here who's description was "it buffer" or "it big buffer" - that's not helpful to me when I look at the output. By having a convention of naming tests like "it should..." or "it should not..." I find that it encourages people to consider the rest of the sentence more carefully and we get better quality descriptions.

alanshaw · 2018-07-20T10:31:32Z

js/src/name/resolve.js

+        expect(err).to.not.exist()
+        expect(res).to.exist()
+
+        ipfs.name.resolve(nodeId, false, false, (err, res) => {


This signature doesn't follow the spec document https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/NAME.md#nameresolve can false, false be an options object instead? I think this is more readable.

Yah, I agree. I did it only on the top level (CLI, HTTP), but I agree with having this as well here

alanshaw · 2018-07-20T10:33:29Z

js/src/name/resolve.js

+        expect(err).to.not.exist()
+        expect(res).to.exist()
+
+        setTimeout(function () {


Is this required? There might be a bug if we can't immediately resolve a value after publishing it.

The setTimeout here aims to guarantee that the record has an expired validity. This way, when we try to resolve it, its validity has expired and we will receive that error, instead of the value.

alanshaw · 2018-07-20T10:35:19Z

js/src/name/resolve.js

+      })
+    })
+
+    it('name resolve should should go recursively until finding an ipfs hash', (done) => {


Could we re-word this one?

How about "should recursively resolve to an IPFS hash"?

alanshaw · 2018-07-20T13:29:52Z

Also @vasco-santos for the future, I have an open PR for adding a (non-local) name.* test, which you might want to pull over the finish line if you implement non-local IPNS later ;) https://github.com/ipfs/interface-ipfs-core/pull/248/files

vasco-santos · 2018-07-21T13:38:48Z

Also @vasco-santos for the future, I have an open PR for adding a (non-local) name.* test, which you might want to pull over the finish line if you implement non-local IPNS later ;) https://github.com/ipfs/interface-ipfs-core/pull/248/files

Cool @alanshaw . I will look at it when implementing the non-local IPNS!

vasco-santos · 2018-07-21T15:23:20Z

@alanshaw your review was entirely addressed, and the PR for js-ipfs #1400 was also updated accordingly.

Particularly, I would like you to have a look here

alanshaw

The setTimeout here aims to guarantee that the record has an expired validity. This way, when we try to resolve it, its validity has expired and we will receive that error, instead of the value.

Could you add a comment to explain that please?

alanshaw · 2018-07-23T12:59:59Z

js/src/name/publish.js

+        expect(err).to.not.exist()
+        expect(res).to.exist()
+        expect(res.Name).to.equal(nodeId)
+        expect(res.Value).to.equal(`/ipfs/${value}`)


The HTTP API in js-ipfs sends Capitalised property names to maintain compatibility with the go-ipfs HTTP API.

js-ipfs core should respond with camelCase property names and the js-ipfs HTTP API should convert them to Capitalised form e.g.

https://github.com/ipfs/js-ipfs/blob/f4344b0e397639df6f26d904be9a015b763e4338/src/http/api/resources/files.js#L273-L277

js-ipfs-api then takes the response from the HTTP API and converts to camelCase. e.g.

https://github.com/ipfs/js-ipfs-api/blob/8536ee42ba59185b71723ae1abde4b722e9fa254/src/utils/file-result-stream-converter.js#L37-L39

Done! And also updated on js-ipfs#1400

vasco-santos · 2018-07-23T17:44:31Z

@alanshaw your new comments were fixed, and a comment was left for the timeout

alanshaw

This is looking great. I'm happy to approve once these last feedback points are addressed.

alanshaw · 2018-07-24T08:07:36Z

js/src/name/publish.js

+    after((done) => common.teardown(done))
+
+    it('should publish an IPNS record with the default params', (done) => {
+      this.timeout(50 * 1000)


Apologies I've just noticed that a bunch of these tests are using this.timeout in an arrow function - you'll need to change it to function (done) {/*...*/} for it to be referring to the correct this (otherwise you're referring to this from the describe block)

alanshaw · 2018-07-24T08:12:44Z

js/src/name/publish.js

+      })
+    })
+
+    it('should recursively resolve to an IPFS hash', (done) => {


This test might need to be renamed or rewritten. Was the intention to ensure that publishing /ipfs/rootHash/path/to/file will actually result in /ipfs/fileHash being published as the value? There doesn't seem to be anything recursive going on here.

Recursive: Resolve until the result is not an IPNS name. Default: “false”. Required: no. (according to the docs at https://ipfs.io/docs/api/#apiv0namepublish)

This test aims to do the following:

Publish an ipfs path with the ID of the peer as the key
Result: /ipns/{PeerID} => /ipfs/{CID}

Publish the previously ipns path with the ID of a new key created
Result: /ipns/{KeyID} => /ipns/{PeerID}

Resolve the last recorder IPNS entry recursively:
Result: /ipfs/{CID}

In this test, the resolve happens recursively until the stop condition (obtaining an ipfs path or reaching a maximum number of tries constant). This way, the first resolve of /ipns/{KeyID} will result in /ipns/{PeerID}, which is not an ipfs path. Accordingly, the resolve continues, and resolving /ipns/{PeerID} results in /ipfs/{CID}. As we got an ipfs path, this is returned.

Maybe the test description is not the most understandable, but for the people who know what the recursive parameter aims to, will understand the logic. If you have any suggestion for renaming it, I will happily change it 🙂

Wrong test name in fact, this was for name/resolve

alanshaw · 2018-07-24T08:25:41Z

js/src/name/resolve.js

+        // guarantee that the record has an expired validity.
+        setTimeout(function () {
+          ipfs.name.resolve(nodeId, (err, res) => {
+            expect(err).to.exist()


Is there an error message we can assert on here (would have to be the same for go-ipfs and js-ipfs)?

yeah, I am using the same message as go-ipfs and I will add an assertion for it.

alanshaw · 2018-07-24T08:28:47Z

js/src/name/resolve.js

+
+      const value = fixture.cid
+      const publishOptions = {
+        resolve: true,


Shouldn't this be false for this test?

It will be the same having it enabled or disabled. In fact, resolve is true by default. Accordingly, when a name publish is performed, it is verified that the ipfs path / ipns path that we want to publish already was added. Otherwise, we may create a ipns path for a unavailable file.

However, as I have other tests with resolve: true I will switch it for false here, which will result in decreasing the time of the test.

@Stebalien

A working version of **IPNS working locally** is here! 🚀 😎 💪 Steps: - [x] Create a new repo (**js-ipns**) like it was made with go in the last week ([go-ipns](https://github.com/ipfs/go-ipns)) and port the related code from this PR to there - [x] Resolve IPNS names in publish, in order to verify if they exist (as it is being done for regular files) before being published - [x] Handle remaining parameters in publish and resolve (except ttl). - [x] Test interface core spec [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327) - [x] Test interoperability with go. [interop#26](ipfs/interop#26) - [x] Integrate logging - [x] Write unit tests - [x] Add support for the lifetime with nanoseconds precision - [x] Add Cache - [x] Add initializeKeyspace - [x] Republish 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](ipfs/specs#184). Related PRs: - [x] [js-ipns#4](ipfs/js-ipns#4) - [x] [js-ipfs-repo#173](ipfs/js-ipfs-repo#173) - [x] [js-ipfs#1496](#1496) - [x] [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327) - [x] enable `interface-ipfs-core` tests for IPNS in `js-ipfs`

vasco-santos requested a review from alanshaw July 16, 2018 14:20

vasco-santos requested review from travisperson and victorb as code owners July 16, 2018 14:20

ghost assigned vasco-santos Jul 16, 2018

ghost added the in progress label Jul 16, 2018

alanshaw suggested changes Jul 20, 2018

View reviewed changes

feat: ipns working locally

a94d80b

vasco-santos force-pushed the feat/ipns-locally branch from e7350df to 441a3f7 Compare July 21, 2018 14:48

fix: code review

29d9e48

vasco-santos force-pushed the feat/ipns-locally branch from 441a3f7 to 29d9e48 Compare July 21, 2018 14:54

alanshaw suggested changes Jul 23, 2018

View reviewed changes

fix: core with camelCase responses

7113139

alanshaw suggested changes Jul 24, 2018

View reviewed changes

vasco-santos added 2 commits July 24, 2018 11:20

fix: code review

71371c8

fix: code review

f429519

alanshaw approved these changes Jul 24, 2018

View reviewed changes

vasco-santos mentioned this pull request Jul 24, 2018

feat: ipns locally ipfs/js-ipfs#1400

Merged

16 tasks

alanshaw merged commit 49a4827 into master Aug 10, 2018

alanshaw deleted the feat/ipns-locally branch August 10, 2018 12:33

ghost removed the in progress label Aug 10, 2018


		after((done) => common.teardown(done))

		it('name publish should publish correctly', (done) => {


		after((done) => common.teardown(done))

		it('name resolve should resolve correctly after a publish', (done) => {

feat: ipns working locally #327

feat: ipns working locally #327

Conversation

vasco-santos commented Jul 16, 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

alanshaw commented Jul 20, 2018

vasco-santos commented Jul 21, 2018

vasco-santos commented Jul 21, 2018 • edited Loading

alanshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Jul 23, 2018

alanshaw left a 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

vasco-santos commented Jul 21, 2018 •

edited

Loading