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

chore: implement interface-store #91

Merged
merged 3 commits into from
May 26, 2021
Merged

Conversation

achingbrain
Copy link
Member

We have datastores and blockstores and blockstores are like datastores just with a different key/value types.

Instead of retyping them, make datastores generic so blockstores can be a specialisation of datastores.

The generics have defaults so this will be a backwards compatible change.

We have datastores and blockstores and blockstores are like datastores
just with a different key/value types.

Instead of retyping them, make datastores generic so blockstores can
be a specialisation of datastores.

The generics have defaults so this will be a backwards compatible change.
@achingbrain
Copy link
Member Author

Bah, not quite so straight forwards: microsoft/TypeScript#29401

@rvagg
Copy link
Member

rvagg commented May 5, 2021

Can you explain the rationale a little more? The problems I run into with these interfaces is that they seem to be attempting to wrap many different uses into a few key concepts that don't actually have much in common. V = Uint8Array being the prime example here, but also the fact that Key in general for this interface can take many forms (string, path, CID). I'm a little concerned that trying to squash them down into more compacted layering is making them less useful because you end up with implementations that do entirely different things and aren't at all swappable.

@achingbrain
Copy link
Member Author

How did I get here.

While waiting for a ts-related build to run I started swapping cids and ipld-block out for multiformats in ipfs-repo to progress the multiformats upgrade in the spirit of "How hard could it be?!".

ipfs-repo instances have a .blocks property which is a Blockstore. Blockstores take CIDs and give you back Blocks.

ipld-block is basically a cid/bytes tuple and seems to want to be replaced by multiformats/block. The multiformats/block requires a value though - deserializing at the repo level doesn't seem right so maybe it's not the correct construct any more. Fair enough, the repo blockstore could just operate on multiformats/cids and Uint8Arrays, seems a bit simpler*.

Our notion of a Blockstore is not really defined anywhere. ipfs-repo, ipfs-bitswap and ipfs-block-service can all be treated as Blockstores and are depended on by ipfs and ipld. ipld is used by ipfs-http-client to implement the ipfs.dag.get API since is uses ipfs.block.get behind the scenes.

Their types are generated from their implementations so leak their constructors and as such the types of their dependencies - for example ipfs-block-service leaks the types of ipfs-repo as it's a constructor arg - suddenly the http client depends on ipfs-repo, which is 👎 (ipfs/js-ipfs#3662 recently removed the ipfs-http-client -> ipld -> ipfs-block-service -> ipfs-repo dep hierarchy, but you can see how easy it is to shoot yourself in the foot with this stuff).

Phew. All the consuming code really cares about is get, getMany, put, putMany, etc.

Datastores have a dependency-free TS interface in this module which they can all implement, I figured to do the same thing for Blockstores, in the spirit of coding to the interface and not the implementation.

This PR should not be merged however, with a bit more thought this is the wrong way to do this - we should have a interface-store or maybe an ipfs-interfaces module for a generic Store<K, V> interface to live, then Datastore and Blockstore could extend that interface.

ipfs-interfaces is tempting as it's a single repo so easy to organise, but bad because a large portion of changes to it will be breaking so if it's widely depended on, orchestrating those upgrades will be painful. It will also be trivial to introduce deps that may not be relevant to a given module.

interface-store and interface-blockstore swerve those problems at the cost of a proliferation of modules which require a release dance though we can automate a lot of that away.


* = it would be nice if ipfs-repo could just depend on the new multiformats CID definition as it doesn't need all the extra stuff in the multiformats module, but that's not for now.

@rvagg
Copy link
Member

rvagg commented May 7, 2021

good background, thanks 👍

re ipld-block and the general concept of a "block" - this is a pattern we've been flailing on quite a bit, not quite happy with any incarnation. Even though it's CID:Bytes, or { cid, bytes }, it actually should be CID:Bytes:Data because there's the decoded form to consider. https://github.com/ipld/js-block was a pretty good go at representing this because you could go both ways and have it handle the encode/decode/verify for you and didn't need to show up with all 3 things to make it work. But that pattern is too dependent on codec and multihash, i.e. you need to have those functions available to do any encoding, decoding or CID creation (hashing). The best answer right now is to just have strongly typed objects with the properties you care about but it's very unsatisfactory.

One of the things I'd like to do while we're considering the foundations of the stack is revisit this and figure out better abstractions that don't tie you too tightly to dependencies to make them work, but also don't require you to have all of the pieces and forms of a "block" to make it work. It's kind of fundamental to IPFS that this is easy, but also important for JS that it doesn't require you have all the things you could possibly need to make it work.

But while you're looking at this, consider multiformats/block an experiment! It's very likely that it's not the right form and needs to be tweaked, augmented, replaced, deleted. We just need more practical experience with it. So don't consider it out of bounds for change if you're seeing obvious deficiencies or better ways to achieve it.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM! I liked how this approach ended up

@achingbrain achingbrain changed the title feat: make datastore type generic chore: implement interface-store May 26, 2021
@achingbrain achingbrain merged commit c257b98 into master May 26, 2021
@achingbrain achingbrain deleted the feat/make-datastore-generic branch May 26, 2021 13:40
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.

3 participants