Skip to content
This repository was archived by the owner on Jun 2, 2022. It is now read-only.

wip: migrate to latest multiformats #16

Open
mikeal wants to merge 7 commits into
base: master
Choose a base branch
from
Open

wip: migrate to latest multiformats #16

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
062a284
wip: migrate to latest multiformats
mikeal Oct 1, 2020
fed6eb6
wip: refactor constructor
mikeal Oct 2, 2020
b86ea53
fix: better cid
mikeal Oct 5, 2020
f0c2e44
fix: cid reference
mikeal Oct 5, 2020
c8749a5
fix: remove support for string cids
mikeal Oct 5, 2020
22eb756
fix: don’t throw on comparison
mikeal Oct 5, 2020
a94c4d4
feat: separate encoder and decoder classes
mikeal Oct 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions basics.js
View file
Open in desktop

This file was deleted.

7 changes: 4 additions & 3 deletions defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Block from './basics.js'
import dagcbor from '@ipld/dag-cbor'
Block.multiformats.multicodec.add(dagcbor)
import Block from './index.js'
import { codec as multicodec } from 'multiformats'
import * as dagcbor from '@ipld/dag-cbor'
Block.add(multicodec.codec(dagcbor))

export default Block
321 changes: 184 additions & 137 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,174 +1,221 @@
import withIs from 'class-is'
import transform from 'lodash.transform'
import createReader from './reader.js'
import json from 'multiformats/codecs/json'
import raw from 'multiformats/codecs/raw'
import { sha256 } from 'multiformats/hashes/sha2'
import { bytes, CID } from 'multiformats'

const readonly = value => ({ get: () => value, set: () => { throw new Error('Cannot set read-only property') } })

const immutableTypes = new Set(['number', 'string', 'boolean'])

const create = multiformats => {
const { bytes, CID, multihash, multicodec } = multiformats
const { coerce, isBinary } = bytes
const copyBinary = value => {
const b = coerce(value)
return coerce(b.buffer.slice(b.byteOffset, b.byteOffset + b.byteLength))
}
const reader = createReader(multiformats)

const clone = obj => transform(obj, (result, value, key) => {
const cid = CID.asCID(value)
if (cid) {
result[key] = cid
} else if (isBinary(value)) {
result[key] = copyBinary(value)
} else if (typeof value === 'object' && value !== null) {
result[key] = clone(value)
const { coerce, isBinary } = bytes
const copyBinary = value => {
const b = coerce(value)
return coerce(b.buffer.slice(b.byteOffset, b.byteOffset + b.byteLength))
}
const reader = createReader(CID)
Copy link

@Gozala Gozala Oct 1, 2020

Choose a reason for hiding this comment

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

❓Any reason to pass CID in vs just let createReader import it instead ? If there is a the reason it would be good to have a comment explaining.

Copy link
Contributor Author

@mikeal mikeal Oct 1, 2020

Choose a reason for hiding this comment

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

good point, i was just trying to get it working again but there’s no need for the dep injection anymore.


const clone = obj => transform(obj, (result, value, key) => {
if (value && value.asCID === value) {
result[key] = value
} else if (isBinary(value)) {
result[key] = copyBinary(value)
} else if (typeof value === 'object' && value !== null) {
result[key] = clone(value)
} else {
result[key] = value
}
})

class Block {
constructor (opts) {
if (!opts) throw new Error('Block options are required')
if (opts.codec) {
if (typeof opts.codec !== 'object') {
const codec = Block.codecs.get(opts.codec)
if (!opts.codec) throw new Error(`Cannot find codec ${JSON.stringify(opts.codec)}`)
opts.codec = codec
}
} else {
result[key] = value
if (!opts.cid) throw new Error('Cannot create block instance without cid or codec')
opts.codec = Block.codecs.get(opts.cid.code)
}
})

class Block {
constructor (opts) {
if (!opts) throw new Error('Block options are required')
if (opts.cid) opts.cid = CID.asCID(opts.cid)
if (typeof opts.codec === 'number') {
opts.code = opts.codec
opts.codec = multicodec.get(opts.code).name
}
if (typeof opts.source === 'undefined' &&
typeof opts.data === 'undefined') {
throw new Error('Block instances must be created with either an encode source or data')
}
if (typeof opts.source !== 'undefined' && !opts.codec && !opts.code) {
throw new Error('Block instances created from source objects must include desired codec')
}
if (opts.data && !opts.cid && !opts.codec && !opts.code) {
throw new Error('Block instances created from data must include cid or codec')
}
if (!opts.cid && !opts.algo) opts.algo = 'sha2-256'
// Do our best to avoid accidental mutations of the options object after instantiation
// Note: we can't actually freeze the object because we mutate it once per property later
opts = Object.assign({}, opts)
Object.defineProperty(this, 'opts', readonly(opts))
if (typeof opts.source === 'undefined' &&
typeof opts.data === 'undefined') {
throw new Error('Block instances must be created with either an encode source or data')
}

source () {
if (this.opts.cid || this.opts.data ||
this._encoded || this._decoded) return null
if (!this.opts.source) return null
return this.opts.source
if (typeof opts.source !== 'undefined' && !opts.codec && !opts.code) {
throw new Error('Block instances created from source objects must include desired codec')
}

async cid () {
if (this.opts.cid) return this.opts.cid
const hash = await multihash.hash(this.encodeUnsafe(), this.opts.algo)
const cid = CID.create(1, this.code, hash)
this.opts.cid = cid
// https://github.com/bcoe/c8/issues/135
/* c8 ignore next */
return cid
if (opts.data && !opts.cid && !opts.codec && !opts.code) {
throw new Error('Block instances created from data must include cid or codec')
}
opts.hasher = opts.hasher || sha256
// Do our best to avoid accidental mutations of the options object after instantiation
// Note: we can't actually freeze the object because we mutate it once per property later
opts = Object.assign({}, opts)
Object.defineProperty(this, 'opts', readonly(opts))
Object.defineProperty(this, 'asBlock', readonly(this))
}

get codec () {
if (this.opts.code) {
this.opts.codec = multicodec.get(this.opts.code).name
get hasher () {
Copy link

@Gozala Gozala Oct 1, 2020

Choose a reason for hiding this comment

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

💭 I think it would make things a lot cleaner if there were dedicated Block so that when you create it with CID an appropriate hasher would be pulled and set as a property etc... That way if invariants don't hold (nor cid nor codec was provided, or cid is provided and hasher is not available) error is thrown at construction rather than later on.

Copy link
Contributor Author

@mikeal mikeal Oct 1, 2020

Choose a reason for hiding this comment

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

I agree, there’s a bigger refactor that I’m going to want to do to the construct and class methods that i haven’t done yet. I wanted to first see the existing implementation ported over without much API changes, but I’m still debating with myself whether or not i’m going to tackle the bigger constructor refactor before i get this merged or just do it now so that I can combine the breaking changes to all the APIs.

if (this.opts.cid) {
if (!this.opts.hasher) {
this.opts.hasher = Block.codecs.get(this.opts.cid.multihash.code)
} else if (this.opts.hasher.code !== this.opts.cid.multihash.code) {
this.opts.hasher = Block.codecs.get(this.opts.cid.multihash.code)
} else {
return this.opts.hasher || sha256
}
if (this.opts.cid) {
this.opts.codec = multicodec.get(this.opts.cid.code).name
}
return this.opts.codec
if (!this.opts.hasher) throw new Error('Do not have hash implementation')
Copy link

@Gozala Gozala Oct 1, 2020

Choose a reason for hiding this comment

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

💣 I think throwing error from property accessory is a bad pattern, APIs that do that tend to be really painful to work with (e.g. some DOM bindings that do it). I would suggest return null here and and do the throw from the method that needs to use hasher instead.

Copy link
Contributor Author

@mikeal mikeal Oct 1, 2020

Choose a reason for hiding this comment

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

I agree, this is a bit of a hack that I plan on factoring out.

}
return this.opts.hasher || sha256
}

source () {
if (this.opts.cid || this.opts.data ||
this._encoded || this._decoded) return null
if (!this.opts.source) return null
return this.opts.source
}

async cid () {
if (this.opts.cid) return this.opts.cid
const hash = await this.hasher.digest(this.encodeUnsafe())
const cid = CID.create(1, this.opts.codec.code, hash)
this.opts.cid = cid
// https://github.com/bcoe/c8/issues/135
/* c8 ignore next */
return cid
}

get code () {
if (this.opts.cid) return this.opts.cid.code
if (!this.opts.code) {
this.opts.code = multicodec.get(this.codec).code
get codec () {
Copy link

@Gozala Gozala Oct 1, 2020

Choose a reason for hiding this comment

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

❓Why not return codec from here just like hasher returns hasher ? User can lookup .name if that's what the y need.

Copy link
Contributor Author

@mikeal mikeal Oct 1, 2020

Choose a reason for hiding this comment

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

I didn’t want to take that breaking change yet because it’s going to cause a lot of churn in the tests. I also hate to change the value of existing properties rather than just migrating to new properties because the errors during migration tend to be much more painful, but maybe it’s worth it here since codec is really the only good name for this.

if (this.opts.cid) {
if (!this.opts.codec || this.opts.codec.code !== this.opts.cid.code) {
this.opts.codec = Block.codecs.get(this.opts.cid.code)
}
return this.opts.code
} else if (this.opts.code) {
this.opts.codec = Block.codecs.get(this.opts.code)
}
return this.opts.codec.name
}

async validate () {
// if we haven't created a CID yet we know it will be valid :)
if (!this.opts.cid) return true
const cid = await this.cid()
const data = this.encodeUnsafe()
// https://github.com/bcoe/c8/issues/135
/* c8 ignore next */
return multihash.validate(cid.multihash, data)
get code () {
if (this.opts.cid) return this.opts.cid.code
if (!this.opts.code) {
this.opts.code = this.opts.codec.code
}
return this.opts.code
}

_encode () {
this._encoded = this.opts.data || multicodec.get(this.code).encode(this.opts.source)
}
async validate () {
// if we haven't created a CID yet we know it will be valid :)
if (!this.opts.cid) return true
if (!this.opts.hasher) throw new Error('Must have hasher in order to perform comparison')
const cid = await this.cid()
const data = this.encodeUnsafe()
const hash = await this.hasher.digest(data)
if (bytes.equals(cid.multihash.bytes, hash.bytes)) return true
throw new Error('Bytes do not match')
Copy link

@Gozala Gozala Oct 1, 2020

Choose a reason for hiding this comment

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

💭 I think this another case where dedicated constructors would make more sense. E.g. you could have a Block.createUnsafe(cid, bytes) and Block.createSafe(cid, bytes) where later delegates to former after validation. That would also get rid of this in cases where you know no validation is needed like when you create block e.g from JS value.

Copy link
Contributor Author

@mikeal mikeal Oct 1, 2020

Choose a reason for hiding this comment

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

I was planning on moving to dedicated constructors but hadn’t identified these as separate methods, but I like it. Although I think I’m just going to go with create() and createUnsafe() to match the other unsafe API forms.

}

encode () {
if (!this._encoded) this._encode()
return copyBinary(this._encoded)
_encode () {
if (!this.opts.data && !this.opts.codec) {
throw new Error('Do not have codec implemention in this Block interface')
}
this._encoded = this.opts.data || this.opts.codec.encode(this.opts.source)
}

encodeUnsafe () {
if (!this._encoded) this._encode()
return this._encoded
}
encode () {
if (!this._encoded) this._encode()
return copyBinary(this._encoded)
}

_decode () {
if (typeof this.opts.source !== 'undefined') this._decoded = this.opts.source
else {
const { decode } = multicodec.get(this.code)
this._decoded = decode(this._encoded || this.opts.data)
}
return this._decoded
}
encodeUnsafe () {
if (!this._encoded) this._encode()
return this._encoded
}

decode () {
// TODO: once we upgrade to the latest data model version of
// dag-pb that @gozala wrote we should be able to remove this
// and treat it like every other codec.
/* c8 ignore next */
if (this.codec === 'dag-pb') return this._decode()
if (!this._decoded) this._decode()
const tt = typeof this._decoded
if (tt === 'number' || tt === 'boolean') {
// return any immutable types
return this._decoded
}
if (isBinary(this._decoded)) return copyBinary(this._decoded)
if (immutableTypes.has(typeof this._decoded) || this._decoded === null) {
return this._decoded
}
return clone(this._decoded)
_decode () {
if (typeof this.opts.source !== 'undefined') this._decoded = this.opts.source
else {
this._decoded = this.opts.codec.decode(this._encoded || this.opts.data)
}
return this._decoded
}

decodeUnsafe () {
if (!this._decoded) this._decode()
decode () {
// TODO: once we upgrade to the latest data model version of
// dag-pb that @gozala wrote we should be able to remove this
// and treat it like every other codec.
/* c8 ignore next */
if (this.codec === 'dag-pb') return this._decode()
if (!this._decoded) this._decode()
const tt = typeof this._decoded
if (tt === 'number' || tt === 'boolean') {
// return any immutable types
return this._decoded
}

reader () {
return reader(this.decodeUnsafe())
if (isBinary(this._decoded)) return copyBinary(this._decoded)
if (immutableTypes.has(typeof this._decoded) || this._decoded === null) {
return this._decoded
}
return clone(this._decoded)
}

async equals (block) {
if (block === this) return true
const cid = await this.cid()
if (CID.asCID(block)) return cid.equals(CID.asCID(block))
// https://github.com/bcoe/c8/issues/135
/* c8 ignore next */
return cid.equals(await block.cid())
}
decodeUnsafe () {
if (!this._decoded) this._decode()
return this._decoded
}

const BlockWithIs = withIs(Block, { className: 'Block', symbolName: '@ipld/block' })
BlockWithIs.encoder = (source, codec, algo) => new BlockWithIs({ source, codec, algo })
BlockWithIs.decoder = (data, codec, algo) => new BlockWithIs({ data, codec, algo })
BlockWithIs.create = (data, cid) => {
if (typeof cid === 'string') cid = CID.from(cid)
return new BlockWithIs({ data, cid })
reader () {
return reader(this.decodeUnsafe())
}

async equals (block) {
if (block === this) return true
const cid = await this.cid()
if (block.asCID === block) return cid.equals(block)
Copy link

@Gozala Gozala Oct 1, 2020

Choose a reason for hiding this comment

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

💭 I find the choice to return true when comparing block to cid questionable, but it was the already so 🤷‍♂️

Copy link
Contributor Author

@mikeal mikeal Oct 1, 2020

Choose a reason for hiding this comment

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

This can be cleaned up by just renaming the variable. The API is meant to take either a CID or a Block, which you may not like, but that’s the intention and the fact that the variable is named block makes this very confusing.

// https://github.com/bcoe/c8/issues/135
/* c8 ignore next */
return cid.equals(await block.cid())
}
BlockWithIs.multiformats = multiformats
BlockWithIs.CID = CID
return BlockWithIs
}

export default create
Block.codecs = new Map()

Block.add = codec => {
if (codec.name) Block.codecs.set(codec.name, codec)
if (codec.code) Block.codecs.set(codec.code, codec)
}
Block.add(json)
Block.add(raw)

Block.encoder = (source, codec, hasher) => new Block({ source, codec, hasher })
Block.decoder = (data, codec, hasher) => new Block({ data, codec, hasher })
Block.create = (data, cid) => {
if (typeof cid === 'string') cid = CID.parse(cid)
const codec = Block.codecs.get(cid.code)
return new Block({ data, cid, codec })
}
Block.defaults = opts => ({
CID,
defaults: opts,
encoder: (source, codec, hasher) => new Block({
source,
codec: codec || opts.codec,
hasher: hasher || opts.hasher
}),
decoder: (data, codec, hasher) => new Block({
data,
codec: codec || opts.codec,
hasher: hasher || opts.hasher
}),
create: Block.create,
codecs: Block.codecs,
add: Block.add
})
Block.CID = CID

export default Block
7 changes: 2 additions & 5 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@
"standard": "^14.3.4"
},
"dependencies": {
"@ipld/dag-cbor": "1.1.11",
"@ipld/dag-cbor": "2.0.2",
"class-is": "^1.1.0",
"lodash.transform": "^4.6.0",
"multiformats": "^3.0.3"
"multiformats": "^4.0.0"
},
"repository": {
"type": "git",
Expand All @@ -43,9 +43,6 @@
".": {
"import": "./index.js"
},
"./basics": {
"import": "./basics.js"
},
"./defaults": {
"import": "./defaults.js"
}
Expand Down
Loading