feat: initial implementation by vasco-santos · Pull Request #1 · ipfs/js-ipns

vasco-santos · 2018-06-21T16:49:20Z

This repo aims to be used by js-ipfs for creating, understanding and validating IPNS records.

This is an important part of js-ipfs PR1400 and once merged and released, the previous PR will be modified for using this module instead of having all the IPNS records code in it.

fsdiogo · 2018-06-21T16:53:20Z

README.md

+#### Create record
+
+```js
+const ipns = require('./ipns')


This should be:

const ipns = require('ipns')

fsdiogo · 2018-06-21T16:54:12Z

README.md

+#### Validate record
+
+```js
+const ipns = require('./ipns')


Same as prev comment.

fsdiogo · 2018-06-21T16:54:36Z

README.md

+#### Datastore key
+
+```js
+const ipns = require('./ipns')


fsdiogo · 2018-06-21T16:55:41Z

README.md

@@ -0,0 +1,129 @@
+# ipns


This should be all uppercase, no?

fsdiogo · 2018-06-21T16:56:58Z

src/index.js

+const ERRORS = require('./errors')
+
+/**
+ * Create creates a new ipns entry and signs it with the given private key.


Duplicate create(s).

fsdiogo

LGTM 👍

Mr0grog · 2018-06-25T20:34:29Z

README.md

+
+## License
+
+[MIT](LICENSE)


Would you mind updating this to fit with the IPFS licensing policy? That should just be a slight tweak to this line in the README to add a little info and updating the LICENSE file to make sure copyright is assigned to “Protocol Labs, Inc.” instead of “IPFS” (which is not an entity copyright can be assigned to).

alanshaw · 2018-06-26T07:43:33Z

README.md

+- `privateKey` (`PrivKey` RSA Instance): key to be used for cryptographic operations.
+- `value` (string): ipfs path of the object to be published.
+- `sequenceNumber` (Number): sequence number of the record.
+- `eol` (string): end of life datetime of the record (according to RFC3339).


Why not a Date instance?...oh wait I think the docs are wrong here this needs to be a date as you're calling toISOString on it in create (https://github.com/ipfs/js-ipns/pull/1/files#diff-1fdf421c05c1140f6d71444ea2b27638R23).

Add link to RFC?

Is it easier to pass a TTL? DNS records use TTL, so it'll be familiar to developers - and this is what seems to be stored in the protobuf...

As you understood, eol was standing for a date in the last origin commit. Currently, I have a new approach in here.

Basically, I think that this function should receive a lifetime of the record and it should be responsible for calculating the appropriate validity of the record (RFC3339 with nanoseconds precision). And I consider this to be the best approach, as a consequence of the ipns entry requirement of the RFC, which does not allow the usage of the native date.

I didn't go with the ttl parameter instead of lifetime/ validity because the ttl parameter will also be used in the IPNS for caching purposes, as you can see here

EDIT: I will try to update this PR today with this new approach

alanshaw · 2018-06-26T07:44:31Z

README.md

+
+- `privateKey` (`PrivKey` RSA Instance): key to be used for cryptographic operations.
+- `value` (string): ipfs path of the object to be published.
+- `sequenceNumber` (Number): sequence number of the record.


Would you mind expanding on this description please?

alanshaw · 2018-06-26T07:44:49Z

README.md

+
+Create an IPNS record for being stored in a protocol buffer.
+
+- `privateKey` (`PrivKey` RSA Instance): key to be used for cryptographic operations.


A link the module that would provide this instance would be useful here

Yeah! I agree that it is useful here. Will add it

alanshaw · 2018-06-26T07:45:19Z

README.md

+
+Create an IPNS record for being stored in a protocol buffer.
+
+- `publicKey` (`PubKey` RSA Instance): key to be used for cryptographic operations.


A link the module that would provide this instance would be useful here

same as before 🙂

alanshaw · 2018-06-26T07:48:34Z

README.md

+- `value` (string): ipfs path of the object to be published.
+- `sequenceNumber` (Number): sequence number of the record.
+- `eol` (string): end of life datetime of the record (according to RFC3339).
+- `callback` (function): operation result.


This needs documentation about the shape or class of the object that is returned, it's methods and properties.

haha, still says (string) here, should it be a number now?

alanshaw · 2018-06-26T08:25:45Z

src/index.js

+ * @param {Object} peerId peer identifier object.
+ * @param {Object} entry ipns entry record.
+ * @param {function(Error)} [callback]
+ * @returns {Promise|void}


Currently does not return a promise...

alanshaw · 2018-06-26T08:27:57Z

README.md

+ipns.getDatastoreKey(peerId);
+```
+
+Returns a key to be used for storing the ipns entry in the datastore according to the specs, that is:


Please link to specs you're referring to or reword...

I will have to reword by now! I still didn't create the PR with specs.

alanshaw · 2018-06-26T08:32:09Z

src/index.js

+  sign(privateKey, value, validityType, validity, (error, signature) => {
+    if (error) {
+      log.error(error)
+      return callback(error)


Add signing error code?

alanshaw · 2018-06-26T08:33:22Z

test/index.spec.js

+
+  it('should create an ipns record correctly', () => {
+    const sequence = 0
+    const eol = new Date(Date.now())


Is equivalent to const eol = new Date()

Changed for milliseconds as previously described.

alanshaw · 2018-06-26T08:38:12Z

src/index.js

+      value: value,
+      signature: signature, // TODO confirm format compliance with go-ipfs
+      validityType: validityType,
+      validity: validity,


I'm not sure how this relates to ttl in the protobuf definition - could you explain?

This proto definition has to be the same as go, in order to guarantee the interoperability among them.

The ttl parameter will also be used in the IPNS for caching purposes, as you can see here. But since it is still experimental in go, I decided to leave it with low priority for now.

vasco-santos · 2018-06-26T17:30:16Z

I have migrated this utility file from js-libp2p-record by now for parsing the RFC3339. Then, it will be necessary to create a new repo with the RFC3339 utilities.

alanshaw · 2018-06-27T09:20:48Z

README.md

+- `value` (string): ipfs path of the object to be published.
+- `sequenceNumber` (Number): sequence number of the record.
+- `eol` (string): end of life datetime of the record (according to RFC3339).
+- `callback` (function): operation result.


haha, still says (string) here, should it be a number now?

alanshaw · 2018-06-27T09:25:02Z

src/index.js

 * @param {Object} entry ipns entry record.
 * @param {function(Error)} [callback]
- * @returns {Promise|void}
+ * @returns {function(Error)} callback


Isn't this @return {Void} also?

alanshaw · 2018-06-27T09:25:23Z

src/index.js

 *
 * @param {Buffer} key peer identifier object.
- * @returns {string}
+ * @returns {Object} containgin the `nameKey` and the `ipnsKey`.


Typo "containgin"

alanshaw · 2018-06-27T09:29:27Z

@vasco-santos should there be a test to verify that if I create an IPNS record, marshal it and then unmarshal it that I get the "same" record back? I get the feeling that currently we'd lose the validity field...

vasco-santos · 2018-06-27T09:30:56Z

@vasco-santos should there be a test to verify that if I create an IPNS record, marshal it and then unmarshal it that I get the "same" record back? I get the feeling that currently we'd lose the validity field...

I will add it now, I think it makes sense!

- Changes the Default TTL to the suggested default of 1 hour per the ipns-record [spec](https://specs.ipfs.tech/ipns/ipns-record/#ttl-uint64) - Allows for a custom TTL to be passed Fixes #310 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net> Co-authored-by: Alex Potsides <alex@achingbrain.net> Co-authored-by: Daniel Norman <1992255+2color@users.noreply.github.com>

## [9.1.0](v9.0.0...v9.1.0) (2024-04-02) ### Features * change default TTL and add support for custom TTL ([#1](#1)) ([#308](#308)) ([d647529](d647529)), closes [/specs.ipfs.tech/ipns/ipns-record/#ttl-uint64](https://github.com/ipfs//specs.ipfs.tech/ipns/ipns-record//issues/ttl-uint64) [#310](#310) ### Trivial Changes * add or force update .github/workflows/js-test-and-release.yml ([#311](#311)) ([0c5f3e1](0c5f3e1)) * add or force update .github/workflows/js-test-and-release.yml ([#312](#312)) ([46a2b72](46a2b72)) * add or force update .github/workflows/js-test-and-release.yml ([#313](#313)) ([e933496](e933496)) * Update .github/workflows/stale.yml [skip ci] ([16e0e10](16e0e10))

feat: initial implementation

2d79059

fsdiogo suggested changes Jun 21, 2018

View reviewed changes

fsdiogo approved these changes Jun 21, 2018

View reviewed changes

vasco-santos force-pushed the feat/initial-implementation branch from d39fa74 to 14e53d0 Compare June 21, 2018 17:15

fix: code review

3afa02c

vasco-santos force-pushed the feat/initial-implementation branch from 14e53d0 to 3afa02c Compare June 21, 2018 17:20

vasco-santos requested a review from daviddias June 21, 2018 17:24

vasco-santos mentioned this pull request Jun 22, 2018

feat: ipns locally ipfs/js-ipfs#1400

Merged

16 tasks

Mr0grog reviewed Jun 25, 2018

View reviewed changes

daviddias requested a review from alanshaw June 26, 2018 07:37

alanshaw requested changes Jun 26, 2018

View reviewed changes

added nanoseconds precision to the validity

6295fb6

alanshaw requested changes Jun 27, 2018

View reviewed changes

added test for marshal and unmarshal ipns records

ccd2605

alanshaw approved these changes Jun 27, 2018

View reviewed changes

vasco-santos merged commit b8eb65f into master Jun 29, 2018

vasco-santos deleted the feat/initial-implementation branch June 29, 2018 10:55


		Create an IPNS record for being stored in a protocol buffer.

		- `privateKey` (`PrivKey` RSA Instance): key to be used for cryptographic operations.


		Create an IPNS record for being stored in a protocol buffer.

		- `publicKey` (`PubKey` RSA Instance): key to be used for cryptographic operations.


		## License

		[MIT](LICENSE)

Conversation

vasco-santos commented Jun 21, 2018

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!

fsdiogo left a comment

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!

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

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!

vasco-santos commented Jun 26, 2018

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!

alanshaw commented Jun 27, 2018

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vasco-santos Jun 26, 2018 •

edited

Loading

vasco-santos commented Jun 27, 2018 •

edited

Loading