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

Implement cbor ipld nodes and a first pass at the 'dag' command #3325

Merged
merged 4 commits into from
Oct 30, 2016

Conversation

whyrusleeping
Copy link
Member

@whyrusleeping whyrusleeping commented Oct 24, 2016

Alright, this is ready for some review now

License: MIT
Signed-off-by: Jeromy [email protected]

@whyrusleeping whyrusleeping added the status/in-progress In progress label Oct 24, 2016
@whyrusleeping whyrusleeping force-pushed the feat/cbor-ipld branch 3 times, most recently from 771fae6 to 6b797f1 Compare October 25, 2016 19:07
@whyrusleeping whyrusleeping added need/review Needs a review and removed status/in-progress In progress labels Oct 25, 2016
@whyrusleeping whyrusleeping added this to the ipld integration milestone Oct 25, 2016
@whyrusleeping
Copy link
Member Author

@Kubuxu @lgierth @kevina @hsanjuan Could you guys give me a little review on this one?

Also worth taking a look at the cbor ipld package here: https://github.com/ipfs/go-ipld-cbor

@kevina
Copy link
Contributor

kevina commented Oct 26, 2016

@whyrusleeping I'm afraid I have not been following the IPLD discussion very closely. What exactly is a "cbor ipld node"? In particular what does "cbor" stand for or at least mean.

Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

This could use some test cases. I would to implement CBOR support for the filestore before this gets merged. I will probably be able find more bugs that way. Filestore integration should be easy, but be a few days before I can get to it.

@@ -277,9 +277,10 @@
"version": "0.3.2"
},
{
"hash": "QmQKEgGgYCDyk8VNY6A65FpuE4YwbspvjXHco1rdb75PVc",
"name": "go-libp2p-routing",
"version": "2.2.2"
Copy link
Contributor

@kevina kevina Oct 26, 2016

Choose a reason for hiding this comment

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

So "go-libp2p-routing" is no longer used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, i guess it still is. But its imported indirectly via the go-libp2p-kad-dht, meaning that me accidentally removing this didnt break anything.

@kevina
Copy link
Contributor

kevina commented Oct 26, 2016

@whyrusleeping okay so scratch my filestore notes, it doesn't look like there is enough implemented yet for this node type to make its way to the filestore. I discovered that CBOR = Concise Binary Object Representation (RFC 7049), but I am even more confused on how this all fits in. I ping you on IRC tomorrow. (Sorry for all the noise).

@hsanjuan
Copy link
Contributor

@kevina @whyrusleeping actually I appreciate the noise because I have similar questions. Did you discuss on IRC already? It would be good if I can read it too :)

@whyrusleeping
Copy link
Member Author

@hsanjuan What questions do you have? I can provide answers here as its a better place for others to find the info

@hsanjuan
Copy link
Contributor

@whyrusleeping so CBOR is just another serializing format and this would be used to store IPLD nodes locally, where now Protobuf is used?

Since you asked for review, I'll tell you that you forgot to document most things in go-ipld-cbor (https://godoc.org/github.com/ipfs/go-ipld-cbor) :)

Shouldn't the JSON->CBOR functions be part of go-ipld-cbor too? Since that seems to offer CBOR->JSON already?

@Kubuxu
Copy link
Member

Kubuxu commented Oct 28, 2016

@hsanjuan protobuf is only for what is used currently which is unixfs, in future we might upgrade unixfs to IPLD but so far it is not planned as far as I know.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 28, 2016

LGTM, the conversion functions are a bit not nice but it is to be expected.

License: MIT
Signed-off-by: Jeromy <[email protected]>
@ghost
Copy link

ghost commented Oct 28, 2016

LGTM, just one question: why two formats for each of cbor or and protobuf, and the possible inconsistency between "dag-pb" and "protobuf"?

@whyrusleeping
Copy link
Member Author

@lgierth both dag-pb and protobuf are the same thing. Just different names for it. I've traditionally called it just protobuf, but @diasdavid insists on calling it dag-pb.

If you want we can just pick one, but i'm a fan of 'being liberal in what you accept'

License: MIT
Signed-off-by: Jeromy <[email protected]>
@ghost
Copy link

ghost commented Oct 30, 2016

If you want we can just pick one, but i'm a fan of 'being liberal in what you accept'

cool I was just wondering 👍

@whyrusleeping
Copy link
Member Author

Well, if i merge this now, i will have successfully met the goal we set for ipld integration. I've got a few LGTMs and this command is marked as experimental. Lets do this.

@whyrusleeping whyrusleeping merged commit 8d4fd80 into master Oct 30, 2016
@whyrusleeping whyrusleeping deleted the feat/cbor-ipld branch October 30, 2016 04:59
@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants