-
Notifications
You must be signed in to change notification settings - Fork 298
Conversation
@diasdavid, could you please share what have to be done to complete the DAG API? you mentioned that different formats have to be supported at #549, is it related with https://github.com/ipld/interface-ipld-format implementations? if the tasks are relatively easy I can try to help with this in the JavaScript part. Thanks |
Right now, we just need these tasks to be done: ipfs/kubo#3771 (comment), essentially, the http-api needs to support what we need in order to complete the JS. Would you like to take a look at that? :) |
@diasdavid, I am afraid I am not familiar enough with Go language to help with the http-api. Is there something else I can do to help pushing forward the solution for this issue? |
@diasdavid, what about implementing it in js-ipfs http API? does it makes sense? https://github.com/ipfs/js-ipfs/tree/master/src/http-api |
So with fixes/additions in ipfs/kubo#4133 current status is that:
@diasdavid Is my thinking here correct or do you think this can be solved in a simpler way? |
8e6d61b
to
1caafb8
Compare
So I rebased this on current master and implemented |
src/dag/get.js
Outdated
dagPB.resolver.resolve(blk, path, cb) | ||
} | ||
} | ||
], callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@magik6k this is an interesting approach. It misses any other format that is not dag-cbor
and or dag-pb
. I also believe that it won't pass every test currently present in https://github.com/ipfs/interface-ipfs-core/blob/master/src/dag.js, which is a requisite to get things merged in js-ipfs-api.
I had understood that you were working on implementing the dag api on go-ipfs, is there anything blocking you there from finishing the implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-ipfs side is done. Now that i think of it, is there any reason the code for put which was here earlier didn't just pass the data as json to go-ipfs? Passing json directly instead of serializing it here would allow to support every encoding go side supports without additional work here.
The same is the case for get
with the difference that using go-ipfs dag get doesn't return remainderPath
(I don't think there is any case where it wouldn't be empty). Does the value passed to callback in get
implement any API or is it much like just like parsing json from ipfs dag get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing json directly instead of serializing it here would allow to support every encoding go side supports without additional work here.
Using json for all the serialization has a big drawback described here: https://github.com/ipfs/interface-ipfs-core/issues/81#issuecomment-277271941
tl;dr; JSON doesn't support Buffers, which makes it really hard to distinguish a Buffer from a String, you can see that we do a cast to Buffer every single time in object get
just because we know that Data in the protobuf is always binary. This is the tl;dr; but please do read the whole thread :)
The same is the case for get with the difference that using go-ipfs dag get doesn't return remainderPath(I don't think there is any case where it wouldn't be empty)
remainderPath
should return any path that the node wasn't able to resolve either because it couldn't find more nodes or because it didn't have the right parser for it.
if (format === 'dag-pb') { | ||
dagPB.util.serialize(dagNode, finalize) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same drawbacks as my comment above https://github.com/ipfs/js-ipfs-api/pull/534/files#r133178508
Do we have any update on this? ETA? :) |
@koalalorenzo I don't have an ETA for this. I currently working on getting the CLI to support the In regards to help: sure, any help is welcome. After a quick look at this PR, I think it shouldn't use the IPLD formats directly, but instead use the API from https://github.com/ipld/js-ipld. This way you needed care format specific things yourself, but have support for all. @diasdavid Do you agree that using |
I got here looking for a way to put an IPLD serialized object into an IPFS node while that IPFS node was running as a daemon. I'm aware that this can now be done with I figured out how to use ipld-js to store the data while the IPFS node is not running: https://gist.github.com/pcardune/411c93eb4dd7d15a642b3750a4bd8cfa. But then you have to start the IPFS node to make that data available publicly. I'm guessing that this PR would solve my problem, but since it's unclear if and when this PR is going to get merged, I'm wondering if there is another way to accomplish this? |
I do :) That was my realization on #534 (comment), that we can use js-ipld and use the ipfsAPI.block as the block service that js-ipld uses. This way we don't have any serialization problems. |
@koalalorenzo @pcardune @jaycenhorton I'm working on this now. I thought it's a piece of cake, but it turned out that it's harder than I thought (too many bugs and outdated code on the way). Though I'll have a look again next week. |
A quick update: I run into more troubles than expected. I'll link the issues here in case someone wants to know more. |
@vmx AFAIU you won't get the nice dag API -- as in https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DAG.md -- from go-ipfs http-api soon due to how things are wired inside go-ipfs (@Stebalien can confirm). You will need to implement the dag API by bringing js-ipld and mounting it on top of the Block API, making the resolve happen clientside. |
@diasdavid ipfs/kubo#4728 (comment) sounds promising. At least it should be possible to do it "manually" in JS land. But currently even that is not really possible. If you request |
* Implement dag.get * Handle dag put response, enable CIDs for dag get and fix dag get request param
The js-ipld formats API changed, they now take a raw data block rather than an IPFS block.
There was a refactoring, now the usual way of doing a HTTP request is through SendOneFile.
Many people have worked on this. I've rebased the commits and added my own ones. This is ready for review, so @diasdavid and @magik6k please review it. |
} | ||
if (format === 'dag-pb') { | ||
dagPB.util.serialize(dagNode, finalize) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about all the other types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use js-ipld here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later. This PR is on for almost a year.
The plan surely is to use js-ipld. But that's not straight forward as js-ipld currently only works with local repositories and not with remote ones (that's what I wanted to mention in the JS Dev call but couldn't remember).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
I'd import git
here too as it's the one other (well) supported codec on go-ipfs side (and would be really handy to have for https://github.com/ipfs-shipyard/IGiS which can't use window.ipfs
from api now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it OK if I I open an issue for it and do it in a separate PR? That' also something newcomers can work on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR won't let users resolve through multiple IPLD formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diasdavid Sounds like a test for that is missing in interface-ipfs-core then.
It will work once js-ipld is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of this thread: everything mentioned here is solved if js-ipld is used. Hence I propose merging this change as is currently is and open a new issue (I'll do that) to use js-ipld instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good my IPLD captain! 🚢
Will need changes that come from ipfs/kubo#3771 (comment)