Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Awesome IPLD endeavour #60

Merged
merged 15 commits into from Oct 26, 2016
Merged

Awesome IPLD endeavour #60

merged 15 commits into from Oct 26, 2016

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Oct 3, 2016

WIP This PR updates the IPLD Resolver to become aware of multiple IPLD formats. It is also the entry point to track the migration that needed to happen across IPFS and IPLD module in order to support both.

Following up the technical discussions pre and post DEVCON2, this PR (and all the listed PRs) update the interfaces and add support to the panoply of IPLD Formats to live and be resolved through IPFS, without breaking links nor requiring transformations (this is huge).

Introducing interface-ipld-format

The interface-ipld-format is a specification for a data type that enables the IPLD Resolver to manipulate and resolve through different types of Merkle Data Structures. The interface is very simple:

UPDATE to the interface: See: #60 (comment)

  • Class DAGNode
    • .serialize() - returns a serialized version of this instance
    • .deserialize(<data>)- deserializes a binary blob into the instance
    • .cid - returns the CID of this instance
  • resolver - the block level resolver (knows how to resolve paths at the block scope)
    • .resolve(block, path) - resolves a path in block, returns the value and or a link and the partial missing path. This way the IPLD Resolver can fetch the link and continue to resolve.
    • .tree(block, options) - returns all the paths available in this block

A full specification of the interface will be defined at https://github.com/ipld/interface-ipld-format. Meanwhile, you can use https://github.com/ipld/js-ipld-dag-pb/tree/awesome-ipld as an example.

UPDATE to the interface: See: #60 (comment)

Tasks

Moving to IPLD and starting to use IPLD formats, required changes to be applied from IPFS-Repo and up, like full support of CID, multicodec spec update and impl, etc. Here are the list of tasks that need to be performed to make this transition complete, so that is easier to track state.

@daviddias daviddias added the status/in-progress In progress label Oct 3, 2016
@daviddias
Copy link
Member Author

@wanderer o/ here is an update :) This is required me to make more changes than I hoped for, but things are getting ready and I'm feeling pretty happy about this "interface-ipld-format".

If you want to start hacking the js-ipld-eth-block, check ipld/js-ipld-dag-pb#1, it is pretty much there. I added you as a contributor to https://github.com/ipld/js-ipld-eth-block.

I'm going to continue and churn through the other things :)

@dignifiedquire
Copy link
Member

@nicola I don't know how much you have seen of this, but this is important re the discussions we had in tr past two week. It seems this changes everything we planned for, so we have to start over again to a degree.

@daviddias daviddias mentioned this pull request Oct 3, 2016
2 tasks
@nicola
Copy link
Member

nicola commented Oct 3, 2016

I don't think it's a drastic change, @diasdavid can you be in the call today?

@dignifiedquire
Copy link
Member

I don't think it's a drastic change, @diasdavid can you be in the call today?
Add your review

Not in general, but in regards to the abstractions for the implementation I think it changes some things.

@daviddias
Copy link
Member Author

daviddias commented Oct 17, 2016

Update to the interface-ipld-format

By implementing dag-cbor, I realized that we only (and benefit from) having an interface for serialize, deserialize and cid detached from the format class. This enables raw json objects to be passed around like dag-cbor, without having to do an instance of a specific DAGNode cbor class. That being said, the new interface for an IPLD format resumes to:

  • format.util
    • .serialize(dagNode) - returns a serialized version of this instance
    • .deserialize(<data>)- deserializes a binary blob into the instance
    • .cid(dagNode) - returns the CID of this instance
  • format.resolver - the block level resolver (knows how to resolve paths at the block scope)
    • .resolve(block, path) - resolves a path in block, returns the value and or a link and the partial missing path. This way the IPLD Resolver can fetch the link and continue to resolve.
    • .tree(block, options) - returns all the paths available in this block

Another way to view it is that the main resolver doesn't really care what type does the 'deserialize' call return.

}

// Adds support for an IPLD format
// default ones are dag-pb and dag-cbor
support (multicodec, type, resolver, util) {
Copy link
Member

Choose a reason for hiding this comment

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

how do you know now if something is cbor or pb?

Copy link
Member Author

Choose a reason for hiding this comment

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

Through the CID

@wanderer
Copy link
Member

@diasdavid one other consideration (maybe its already been considered, im just still trying to grok) is that merkle links and the DAG topology / traversal should be independent of each other.

So for example, i would want to do something like

rootVertex.get(`/<ethereu, account hash>/`, account => {
   account.get('/balance/', (balanceVertex) => {
      console.log(balanceVertex.value) // how much ether this account has
  })
}) 

but underneath account doesn't have a true merkle link to balance. Its all part of the account blob. So account vertex has the edges balance, nonce, code, state. balance and nonce are just values. And code and state are merkle links.

@daviddias
Copy link
Member Author

@wanderer you can achieve that in two ways:

// resolve the account and get its object
resolver.resolve('/<ethereum account hash>/', '/', (err, result) => {
  const account = result.value
  // then manipulate the account as you already do
})

// or resolve the account balance, using the path notation

resolver.resolve('/<ethereum account hash>/', 'balance', (err, result) => {
  const balance = result.value
  console.log(balance.value) // how much ether this account has
})

we can go through this during today's js-ipfs chat :)

@daviddias
Copy link
Member Author

daviddias commented Oct 21, 2016

Getting close to that IPLD Week 2 goal :) An excellent way to achieve that is by CR the following PR:

PR CR RFM M
#60 ✔️ ✔️ ✔️
ipld/js-ipld-dag-pb#1 ✔️ ✔️ ✔️
ipld/js-ipld-dag-cbor#25 ✔️ ✔️ ✔️
multiformats/js-cid#4 ✔️ ✔️ ✔️
ipfs-inactive/js-ipfs-unixfs-engine#74 ✔️ ✔️
multiformats/multicodec#16
ipld/js-ipld-block#5 ✔️ ✔️ ✔️
ipfs/js-ipfs-block-service#27 ✔️ ✔️ ✔️
ipfs/js-ipfs-repo#89 ✔️ ✔️ ✔️
ipfs-inactive/interface-js-ipfs-core#78 ✔️ ✔️
ipfs/js-ipfs#532 ✔️ ✔️
ipfs-inactive/js-ipfs-http-client#398 ✔️ ✔️

Please, if you review it, remove the 'Needs review' label, so that work doesn't get duplicated.

//cc @victorbjelkholm @dignifiedquire @RichardLitt

MOVED TABLE TO: ipfs/js-ipfs#532 (comment)

}
}

resolve (cid, path, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

why are cid and path two different inputs?
Can we not just pass cid+path?
the "deconcatenation" is something that I will happen to do often

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicola note that CID is a instance of class CID that has several utility methods https://github.com/ipfs/js-cid/tree/feat/complete

What you are proposing is to use cids always as strings, which would assume that we have to be constantly parsing and mounting them again to get their props.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, perfect then.

There must be up in the levels of abstraction a sort of resolver that takes a path and resolve that (by using this, I guess)


// TODO: Consider if we still want this (fact: no one is using it
// right now and this will be just an IPLD selector anyway
getRecursive (cid, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

During the Toronto's IPLD call, we mentioned that this should be a BFS and should have an optional parameter x where if set to false or -1, we explore recursively the entire graph, otherwise, we just move with depth x

Copy link
Member Author

@daviddias daviddias Oct 21, 2016

Choose a reason for hiding this comment

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

What we talked was .tree method inside an IPLD format.

This getRecursive is an old method that comes from the previous "DAGService", which was never used in practice. This method returns the nodes directly, not their values.

What you make me think is that we should have a .tree at the main IPLD Resolver and make that option part of the interface-ipld-format for local resolvers too.

Copy link
Member

Choose a reason for hiding this comment

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

^ perfect


[![](https://img.shields.io/badge/made%20by-Protocol%20Labs-blue.svg?style=flat-square)](http://ipn.io)
[![](https://img.shields.io/badge/project-IPFS-blue.svg?style=flat-square)](http://ipfs.io/)
[![](https://img.shields.io/badge/freenode-%23ipfs-blue.svg?style=flat-square)](http://webchat.freenode.net/?channels=%23ipfs)
[![Coverage Status](https://coveralls.io/repos/github/ipfs/js-ipld-resolver/badge.svg?branch=master)](https://coveralls.io/github/ipfs/js-ipld-resolver?branch=master)
[![Travis CI](https://travis-ci.org/ipfs/js-ipld-resolver.svg?branch=master)](https://travis-ci.org/ipfs/js-ipld-resolver)
Copy link
Member

Choose a reason for hiding this comment

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

let's move this to the ipld organization as part of this effort

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, it's already on the IPLD org, forgot to change the badges. Good catch :)

"description": "IPLD implementation in JavaScript",
"main": "lib/index.js",
"description": "IPLD Resolver Implementation in JavaScript",
"main": "src/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

TODO: remove before merge

const CID = require('cids')
const until = require('async/until')
const IPFSRepo = require('ipfs-repo')
const MemoryStore = require('../node_modules/interface-pull-blob-store/lib/reference.js')
Copy link
Member

Choose a reason for hiding this comment

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

you can do require('interface-pull-blob-store/lib/reference.js') instead, no need for the full path

Copy link
Member Author

Choose a reason for hiding this comment

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

right! (I do remember fighting with this one and this was how I got it to reference to the right one, not sure what was the error without node_modules though).

Could you also check ipfs-inactive/interface-pull-blob-store#6, this way we don't even have to point to a path inside.


// Support by default dag-pb and dag-cbor
this.support(dagPB.resolver.multicodec, dagPB.resolver, dagPB.util)
this.support(dagCBOR.resolver.multicodec, dagCBOR.resolver, dagCBOR.util)
Copy link
Member

Choose a reason for hiding this comment

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

should there be a way to disable these defaults?

Copy link
Member Author

Choose a reason for hiding this comment

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

What about having functions to add and remove, so that:

  • this.support.add
  • this.support.rm

// Adds support for an IPLD format
// default ones are dag-pb and dag-cbor
support (multicodec, resolver, util) {
this.resolvers[multicodec] = {
Copy link
Member

Choose a reason for hiding this comment

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

maybe warn on overriding?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

if (err) {
return cb(err)
}
const result = this.resolvers[cid.codec].resolver.resolve(block, path)
Copy link
Member

Choose a reason for hiding this comment

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

there should be a check with a nice error message for this.resolvers[cid.codec] as this can easily be undefined

)
}

// TODO: Consider if we still want this (fact: no one is using it
Copy link
Member

Choose a reason for hiding this comment

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

lets use this opportunity to drop the recursive methods

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 👍

/*
* Test different types of data structures living together
* &
* Test data made of mixed data structures!
Copy link
Member

Choose a reason for hiding this comment

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

are you planning to fill this before merge?

Copy link
Member Author

@daviddias daviddias Oct 24, 2016

Choose a reason for hiding this comment

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

Yes :) cbor + pb

@@ -0,0 +1,205 @@
/* eslint-env mocha */
'use strict'
Copy link
Member

Choose a reason for hiding this comment

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

the tests in cbor and proto are super similar, maybe you could extract the test methods, and just pass in the type agains it should test?

Copy link
Member Author

@daviddias daviddias Oct 24, 2016

Choose a reason for hiding this comment

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

Similar, but not the same, cbor is way more flexible and needs even more throughout tests. Creating a base tests would make every other format be tested very much like dag-pb, which is the less interesting data format because of its rigidity.

@daviddias daviddias merged commit fc98c00 into master Oct 26, 2016
@daviddias daviddias deleted the awesome-ipld branch October 26, 2016 12:10
@daviddias daviddias removed the status/in-progress In progress label Oct 26, 2016
@daviddias
Copy link
Member Author

@kumavis, @wanderer now you can git clone and npm install this repo without going through all the npm link hurdles, perfect time to test that new eth-block IPLD format :D

@nicola
Copy link
Member

nicola commented Oct 26, 2016

🎉

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

Successfully merging this pull request may close these issues.

None yet

4 participants