Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

[WIP] feat(dag): add IPLD to dag.get #755

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alanshaw
Copy link
Contributor

@alanshaw alanshaw commented May 3, 2018

This PR is a proposal to resolve #738 and will also fix #747.

In the dag.get function we create an temporary in-memory IPLD resolver with a custom bitswap exchange hooked up to the block.get API call.

This is kinda awesome!

I've not implemented all the methods for the exchange, but you don't need to because it only uses get.

I created a utility module for extracting the CID and path from the cid arg...the spec for dag.get is flexible in that you can pass it a Qmhash, or a Qmhash/with/path or a CID instance but the IPLD module isn't as flexible and they need to be split into two args: CID(Qmhash) and /with/path.

I've also added explain-error because it's really good at adding context to error messages whilst also retaining the original error & stack. It also means that in tests we can assert on error messages that are generated within this module rather than (possibly) randomly changing messages from dependencies.


BLOCKED

This is currently a breaking change to the way dag-pb nodes are traversed via path.

In go-ipfs you can do this:

ipfs dag get /ipfs/QmWgCbdDVhsXA8UzpaQwFgw71iGaLrAfyyy51XWpHzNu8C/gif

but with this pull request, to get the same object, you'd need to:

jsipfs dag get /ipfs/QmWgCbdDVhsXA8UzpaQwFgw71iGaLrAfyyy51XWpHzNu8C/Links/gif

Note "Links".

@alanshaw alanshaw requested review from vmx and daviddias May 3, 2018 12:00
@ghost ghost assigned alanshaw May 3, 2018
@ghost ghost added the in progress label May 3, 2018
@magik6k
Copy link
Contributor

magik6k commented May 4, 2018

Note that go-ipfs intends to add /ipld namespace which will behave the same way as /ipfs does here. See ipfs/kubo#4672.

I'd also use ipfs dag relolve to resolve the path on the daemon side to limit api cals.

@alanshaw
Copy link
Contributor Author

alanshaw commented May 4, 2018

@magik6k thanks for looking at this!

I'd love to use dag.resolve but js-ipfs daemon doesn't have it. Is dag.resolve going away in go-ipfs or is it worth implementing it in js-ipfs and adding it to the spec?

@daviddias
Copy link
Contributor

dag.resolve should not exist as it stands per all the conversations I, @whyrusleeping and @jbenet had in the past, both in person and async. It has been almost 2 years since the work on this API started, not worth going through all the data points, the tl;dr; is that there wasn't the opportunity to commit to an API for both implementations.

I took a lot of notes along the way here, find them at:

My suggestion is for everyone to get a revisited dag API and implement it for both implementations that respects all the constraints (i.e. we can't just use JSON serialization for responses over HTTP as it messes up with types).

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
})

it('should parse input as string without leading slash', () => {
const input = 'ipfs/QmUmaEnH1uMmvckMZbh3yShaasvELPW4ZLPWnB4entMTEn/path/to'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this actually be supported? I prefer having strict rules and fail early. The rule would be: path with namespaces start with a slash, if there isn't a slash it's a CID.

const input = 42
expect(() => ipfsPath(input)).to.throw('invalid path')
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Additional test cases:

  • what happens if a path has two slashes in a row (/ipfs/CID/some//thing)
  • ipfs namespace without a path (/ipfs/CID)
  • CID-only with a trailing slash, with and without namespace (CID/, /ipfs/CID/)

], callback)
IPLDResolver.inMemory((err, ipld) => {
if (err) return callback(explain(err, 'failed to create IPLD resolver'))
ipld.support.rm(dagPB.resolver.multicodec)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a comment here, why this is needed? It's clear to me, but I don't think it's clear when you read the code for the first time.

@alanshaw alanshaw changed the title [WIP] feat: add IPLD to dag.get [WIP] feat(dag): add IPLD to dag.get May 18, 2018
@alanshaw alanshaw removed the blocked label Jul 23, 2018
@alanshaw
Copy link
Contributor Author

unblocked thanks to ipld/js-ipld-dag-pb#78

Copy link
Contributor

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Check my proposed solution and let me know if you see any issues with it

}
}
], callback)
IPLDResolver.inMemory((err, ipld) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking that this should be done by passing the ipfs.block.{get,put} as the Block Service, rather than doing an inMemory resolver and then monkey patching the Block Service Instance with a fake exchange.

You should be able to do:

const ipld = new IPLD(ipfs.block)

or close

const ipld = new IPLD(shimBS(ipfs.block))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dag.get for cid with codec not dag-pb or dag-cbor never calls callback Use js-ipld for DAG API
4 participants