chore: upgrade to new multiformats module #98
Conversation
BREAKING CHANGE: Uses new CID class
achingbrain
force-pushed
the
chore/upgrade-multiformats
branch
from
66299e2 to
e22501b
Compare
| await pinset.storeSet(blockstore, PinTypes.recursive, recursivePins) | ||
| ] | ||
| } | ||
| const buf = dagPb.encode(pinRoot) |
There was a problem hiding this comment.
const block = Block.encode({ value: pinRoot, codec: dagPb, hasher: sha256 }) is available to shorten this slightly, then you can pick block.cid.bytes out of it.
There was a problem hiding this comment.
It looks like Block.encode will only ever give me a v1 CID back and we need a v0 CID here?
There was a problem hiding this comment.
Could just pull the multihash bytes out of the v1 CID, I guess..
There was a problem hiding this comment.
mmm, block.cid.multihash should do it. Alternatives include block.cid.toV0().bytes (which is more explicit but a little wasteful) or adding a cidVersion argument to Block.encode.
There was a problem hiding this comment.
I don't think Block.encode will save that much, and probably it's best to not add support for v0 there if we eventually want to break from v0. I would personally leave it as is.
There was a problem hiding this comment.
This repo will always have to handle v0 as it needs to be able to migrate repos from before v1 was a thing.
| cidVersion: 0, | ||
| hashAlg: multihash.names['sha2-256'] | ||
| }) | ||
| const buf = dagPb.encode(child) |
| await blockstore.put(cidToKey(cid), buf) | ||
|
|
||
| fanoutLinks[binIdx] = new DAGLink('', child.size, cid) | ||
| let size = child.Links.reduce((acc, curr) => acc + (curr?.Tsize || 0), 0) + buf.length |
There was a problem hiding this comment.
repetitive pattern suggests we might need to pull up a utility or two to dagPb to deal with Tsize
There was a problem hiding this comment.
and, is curr?.Tsize safe to use these days in the platforms we care about? I haven't even bothered touching it, assuming it'll be a year or two away for general use
There was a problem hiding this comment.
repetitive pattern suggests
Yes, this was previously handled by the ipld-dag-pb module, but that feature wasn't ported over so a utility would be helpful.
and, is
curr?.Tsizesafe to use these days in the platforms we care about?
I thought yes, but it looks like the version of acorn used by the current create-react-app release doesn't support it. A later version does but it'll require them to upgrade to webpack 5 and who knows when that will land.
It's caused a few problems with the last js-ipfs release so we should probably back out our use of optional chaining for the time being.
| cidVersion: 0, | ||
| hashAlg: multihash.names['sha2-256'] | ||
| }) | ||
| const buf = dagPb.encode(rootNode) |
Co-authored-by: Rod Vagg <[email protected]>
| await pinset.storeSet(blockstore, PinTypes.recursive, recursivePins) | ||
| ] | ||
| } | ||
| const buf = dagPb.encode(pinRoot) |
There was a problem hiding this comment.
I don't think Block.encode will save that much, and probably it's best to not add support for v0 there if we eventually want to break from v0. I would personally leave it as is.
| @@ -86,15 +82,15 @@ function hash (seed, key) { | |||
| const buffer = new Uint8Array(4) | |||
| const dataView = new DataView(buffer.buffer) | |||
| dataView.setUint32(0, seed, true) | |||
| const encodedKey = uint8ArrayFromString(toB58String(key)) | |||
| const encodedKey = uint8ArrayFromString(key.toString()) | |||
There was a problem hiding this comment.
| @@ -164,14 +160,22 @@ function storeItems (blockstore, items) { | |||
| const fanoutLinks = [] | |||
|
|
|||
| for (let i = 0; i < DEFAULT_FANOUT; i++) { | |||
| fanoutLinks.push(new DAGLink('', 1, EMPTY_KEY)) | |||
| fanoutLinks.push({ | |||
There was a problem hiding this comment.
💭 Seems like having an utility function to create links would have been still useful.
There was a problem hiding this comment.
simple factories that match the old DAGLink and DAGNode constructors would probably be good enough to make this pain decrease a little eh?
There was a problem hiding this comment.
ok, since this one is simple I've drafted it up but would like feedback: ipld/js-dag-pb#20
These 4 lines would become fanoutLinks.push(createLink('', 1, EMPTY_KEY)). Same below, and then the return at the end would be return createNode(rootData, rootLinks).
Is this enough of an improvement to warrant API surface area increase?
There was a problem hiding this comment.
What I really meant is something like link({ cid: EMPTY_KEY, size: 1, name: '' }) just as a sugar.
BREAKING CHANGE: Uses new CID class