Add support for CIDv1 and Base32 #9

lidel · 2018-08-16T00:24:14Z

Closes #8, see it for details.

Added CID support, added resolver.cid (resolver.multihash still works)
Basic support for HAMD sharded directory
- not real support, but returns potential HTML page with index
Return data from raw dag without resolv step
Add tests for CIDv1 in Base32
- CIDv1 tests failing due to ipfs.files.add returns a CIDv1 of an empty file for pp.txt and testfile.txt
- filled upstream bug: ipfs.files.add with {cidVersion: 1} returns CID of an empty file js-ipfs#1518 (this PR is blocked until that is resolved)
  - fixed in fix: import with CID version 1 ipfs-inactive/js-ipfs-unixfs-engine#227 and chore: update ipfs-unixfs-engine dependency js-ipfs#1523
Proactively ensure directory listing in js-ipfs works with these changes
Proactively ensure directory listing in ipfs-companion+libdweb works with these changes
Switch to new js-ipfs after chore: update ipfs-unixfs-engine dependency js-ipfs#1523 is released

- feat: CID support, added resolver.cid - feat: basic support for HAMD sharded directory - not real support, we need ipfs.resolve for that - fix: return data from raw dag without resolv step License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel · 2018-08-22T11:46:21Z

src/resolver.js

+          if (dagDataObj.type === 'directory' || dagDataObj.type === 'hamt-sharded-directory') {
+            let isDirErr = new Error('This dag node is a directory')
+            // store memo of last multihash so it can be used by directory
+            isDirErr.cid = isDirErr.fileName = cid


Backward compatibility: js-ipfs uses fileName field, so we are keeping it here for now.
When ipfs-http-response is released with these changes, will PR js-ipfs to switch from fileName to cid

Context: ipfs/js-ipfs#1518 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel · 2018-09-13T11:05:41Z

@vasco-santos I switched to latest release of js-ipfs (e320884), it includes the fix for broken tests when files.add is used with cidv1 (ipfs/js-ipfs#1518).

Tests pass locally and it is ready to merge.
Please review when you have a moment.

vasco-santos · 2018-09-13T13:48:28Z

Thanks for this PR @lidel

The CI is not happy and I tested it locally and I had the same tests failing 🤔
Meanwhile, I will have a look in the code

lidel · 2018-09-13T14:38:43Z

Indeed something is fishy, I'll look into this.

Old CIDs were invalid as noted in: ipfs-inactive/js-ipfs-unixfs-engine#227 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel · 2018-09-22T00:10:36Z

@vasco-santos check now, ci/jenkins/linux/10.4.1/test is all green 🍏

(I've finally found a moment to fix it: CIDs used in tests were invalid because I generated them with js-ipfs without fix from ipfs-inactive/js-ipfs-unixfs-engine#227 🤦‍♂️)

vasco-santos

Thanks @lidel for the complete PR and for a general improvement on documentation. Left just a few observations before merging this.

Meanwhile I will test this with js-ipfs and service-worker-gateway

test/index.spec.js

vasco-santos · 2018-09-24T14:59:53Z

test/index.spec.js

+        // console.log('CIDv1', filesAdded)
+        const retrievedFile = filesAdded[0]
+        expect(new CID(retrievedFile.hash)).to.deep.equal(new CID(file.cid))
+        // expect(retrievedFile.size, 'ipfs.files.add result size should not be smaller than input buffer').greaterThan(file.data.length)


Don't we receive the file size with CIDv1?

Nope :) there is a bug and size is undefined – i reported it in ipfs/js-ipfs#1585
It does not impact what this lib does, data is added correctly by js-ipfs, so I commented it out for now.

test/index.spec.js

License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

vasco-santos · 2018-09-25T10:50:29Z

@lidel Now the changes look good to me, but there are some problems with the tests. I cannot have the tests running again and they are failing in the before hook, while adding files. Do you know what the problem is?

lidel · 2018-09-25T13:43:59Z

This PR is officially haunted 👻 I get the same issue locally as well 👀

I am quite sure it is related to ipfs-shipyard/is-ipfs#21 and changes in cids v0.5.4-v0.5.3, namely:

migrate to class-is for instance comparise, fixes #53 (6b6873b)

It can be fixed in yarn via:

+  "resolutions": {
+    "cids": "^0.5.5"
+  },

~~But for regular npm we probably need to wait for bugfix release in js-ipfs, then we will bump version here and it will solve the issue.~~

cids@0.5.4 now uses class-is and checks for CIDs were failing because tests were using the old version. License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

Our Jenkins setup changed recently and `test` is ignored License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel · 2018-09-27T20:08:39Z

@vasco-santos I switched to latest cids v0.5.5 and the issue is gone. All green again. 🤞

ps. I had to add test:node because our Jenkins no longer runs test, but expects test:*

vasco-santos · 2018-09-28T15:32:13Z

Nice @lidel

Thanks for the PR!

lidel mentioned this pull request Aug 16, 2018

[PoC] libdweb experiment: protocol handler API ipfs/ipfs-companion#533

Closed

27 tasks

lidel force-pushed the feat/cidv1b32 branch from 475aead to aedaaae Compare August 19, 2018 15:58

lidel mentioned this pull request Aug 19, 2018

ipfs.files.add with {cidVersion: 1} returns CID of an empty file ipfs/js-ipfs#1518

Closed

lidel force-pushed the feat/cidv1b32 branch from aedaaae to 9198c23 Compare August 22, 2018 10:19

feat: support cidv1b32 in resolver

c733266

- feat: CID support, added resolver.cid - feat: basic support for HAMD sharded directory - not real support, we need ipfs.resolve for that - fix: return data from raw dag without resolv step License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel force-pushed the feat/cidv1b32 branch from 9198c23 to c733266 Compare August 22, 2018 11:40

lidel commented Aug 22, 2018

View reviewed changes

lidel mentioned this pull request Sep 4, 2018

feat(gateway): X-Ipfs-Path, Etag, Cache-Control, Suborigin ipfs/js-ipfs#1537

Merged

fix: tests with files.add and cidv1

e320884

Context: ipfs/js-ipfs#1518 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel changed the title ~~[WIP] Add support for CIDv1 and Base32~~ Add support for CIDv1 and Base32 Sep 13, 2018

lidel changed the title ~~Add support for CIDv1 and Base32~~ [WIP] Add support for CIDv1 and Base32 Sep 15, 2018

fix: update tests with valid CIDv1

116255e

Old CIDs were invalid as noted in: ipfs-inactive/js-ipfs-unixfs-engine#227 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel changed the title ~~[WIP] Add support for CIDv1 and Base32~~ Add support for CIDv1 and Base32 Sep 22, 2018

lidel mentioned this pull request Sep 22, 2018

ipfs.files.add with {cidVersion: 1} returns valid hash with undefined size ipfs/js-ipfs#1585

Closed

vasco-santos suggested changes Sep 24, 2018

View reviewed changes

style: remove unused console.log

d673593

License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel added 2 commits September 27, 2018 20:49

fix: update js-cid to v0.5.5

826f032

cids@0.5.4 now uses class-is and checks for CIDs were failing because tests were using the old version. License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

fix(ci): add test:node

17cf177

Our Jenkins setup changed recently and `test` is ignored License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>

lidel force-pushed the feat/cidv1b32 branch from b33a02e to 17cf177 Compare September 27, 2018 18:59

vasco-santos approved these changes Sep 28, 2018

View reviewed changes

vasco-santos merged commit 34a2f68 into ipfs:master Sep 28, 2018

lidel deleted the feat/cidv1b32 branch September 28, 2018 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for CIDv1 and Base32 #9

Add support for CIDv1 and Base32 #9

lidel commented Aug 16, 2018 •

edited

Loading

lidel Aug 22, 2018

lidel commented Sep 13, 2018 •

edited

Loading

vasco-santos commented Sep 13, 2018 •

edited

Loading

lidel commented Sep 13, 2018

lidel commented Sep 22, 2018

vasco-santos left a comment •

edited

Loading

vasco-santos Sep 24, 2018

lidel Sep 24, 2018

vasco-santos commented Sep 25, 2018

lidel commented Sep 25, 2018 •

edited

Loading

lidel commented Sep 27, 2018 •

edited

Loading

vasco-santos commented Sep 28, 2018

Add support for CIDv1 and Base32 #9

Add support for CIDv1 and Base32 #9

Conversation

lidel commented Aug 16, 2018 • edited Loading

lidel Aug 22, 2018

Choose a reason for hiding this comment

lidel commented Sep 13, 2018 • edited Loading

vasco-santos commented Sep 13, 2018 • edited Loading

lidel commented Sep 13, 2018

lidel commented Sep 22, 2018

vasco-santos left a comment • edited Loading

Choose a reason for hiding this comment

vasco-santos Sep 24, 2018

Choose a reason for hiding this comment

lidel Sep 24, 2018

Choose a reason for hiding this comment

vasco-santos commented Sep 25, 2018

lidel commented Sep 25, 2018 • edited Loading

lidel commented Sep 27, 2018 • edited Loading

vasco-santos commented Sep 28, 2018

lidel commented Aug 16, 2018 •

edited

Loading

lidel commented Sep 13, 2018 •

edited

Loading

vasco-santos commented Sep 13, 2018 •

edited

Loading

vasco-santos left a comment •

edited

Loading

lidel commented Sep 25, 2018 •

edited

Loading

lidel commented Sep 27, 2018 •

edited

Loading