Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initial implementation #1

Merged
merged 4 commits into from
Jun 29, 2018
Merged

Conversation

vasco-santos
Copy link
Member

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.

README.md Outdated
#### Create record

```js
const ipns = require('./ipns')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be:

const ipns = require('ipns')

README.md Outdated
#### Validate record

```js
const ipns = require('./ipns')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as prev comment.

README.md Outdated
#### Datastore key

```js
const ipns = require('./ipns')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

README.md Outdated
@@ -0,0 +1,129 @@
# ipns
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be all uppercase, no?

src/index.js Outdated
const ERRORS = require('./errors')

/**
* Create creates a new ipns entry and signs it with the given private key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate create(s).

Copy link
Contributor

@fsdiogo fsdiogo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

README.md Outdated

## License

[MIT](LICENSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

@daviddias daviddias requested a review from alanshaw June 26, 2018 07:37
README.md Outdated
- `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).
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Add link to RFC?

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

@vasco-santos vasco-santos Jun 26, 2018

Choose a reason for hiding this comment

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

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

README.md Outdated

- `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.
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind expanding on this description please?

README.md Outdated

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

README.md Outdated

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

same as before 🙂

README.md Outdated
- `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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

src/index.js Outdated
* @param {Object} peerId peer identifier object.
* @param {Object} entry ipns entry record.
* @param {function(Error)} [callback]
* @returns {Promise|void}
Copy link
Member

Choose a reason for hiding this comment

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

Currently does not return a promise...

README.md Outdated
ipns.getDatastoreKey(peerId);
```

Returns a key to be used for storing the ipns entry in the datastore according to the specs, that is:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

src/index.js Outdated
sign(privateKey, value, validityType, validity, (error, signature) => {
if (error) {
log.error(error)
return callback(error)
Copy link
Member

Choose a reason for hiding this comment

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

Add signing error code?


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

Choose a reason for hiding this comment

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

Is equivalent to const eol = new Date()

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed for milliseconds as previously described.

src/index.js Outdated
value: value,
signature: signature, // TODO confirm format compliance with go-ipfs
validityType: validityType,
validity: validity,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.

README.md Outdated
- `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.
Copy link
Member

Choose a reason for hiding this comment

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

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

src/index.js Outdated
@@ -48,7 +60,7 @@ const create = (privateKey, value, seq, eol, callback) => {
* @param {Object} publicKey public key for validating the record.
* @param {Object} entry ipns entry record.
* @param {function(Error)} [callback]
* @returns {Promise|void}
* @returns {function(Error)} callback
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this @return {Void} also?

src/index.js Outdated

/**
* Get key for sharing the record in the routing mechanism.
* Format: ${base32(/ipns/<HASH>)}, ${base32(/pk/<HASH>)}
*
* @param {Buffer} key peer identifier object.
* @returns {string}
* @returns {Object} containgin the `nameKey` and the `ipnsKey`.
Copy link
Member

Choose a reason for hiding this comment

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

Typo "containgin"

@alanshaw
Copy link
Member

@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
Copy link
Member Author

vasco-santos commented Jun 27, 2018

@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!

@vasco-santos vasco-santos merged commit b8eb65f into master Jun 29, 2018
@vasco-santos vasco-santos deleted the feat/initial-implementation branch June 29, 2018 10:55
achingbrain added a commit that referenced this pull request Apr 2, 2024
- 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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: semantic-release-bot <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
Co-authored-by: Daniel Norman <[email protected]>
github-actions bot pushed a commit that referenced this pull request Apr 2, 2024
## [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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants