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

Implement new cid(blob, ...) #32

Closed
7 tasks done
vmx opened this issue Apr 27, 2018 · 5 comments
Closed
7 tasks done

Implement new cid(blob, ...) #32

vmx opened this issue Apr 27, 2018 · 5 comments
Assignees

Comments

@vmx
Copy link
Member

vmx commented Apr 27, 2018

The IPLD Format spec was changed so that util.cid() now takes the binary blob and not the deserialized DAG Node as argument (#24).

This is a breaking change and needs to be done on all existing formats. So please don't merge before PRs from all formats are approved. This way things can be merged and released in one go.

Instead of opening an issue on every repository, just let people know on this issue that you're working on it and then link to the PR.

@richardschneider richardschneider self-assigned this Jun 25, 2018
@richardschneider richardschneider changed the title Implement new cid() Implement new cid(blob, ...) Jun 25, 2018
richardschneider added a commit to ipld/js-ipld-dag-cbor that referenced this issue Jun 25, 2018
BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32
richardschneider added a commit to ipld/js-ipld-dag-pb that referenced this issue Jun 25, 2018
BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32
richardschneider added a commit to ipld/js-ipld-git that referenced this issue Jun 26, 2018
BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32
@richardschneider
Copy link

No need to change https://github.com/ipld/js-ipld-raw, because util.serialise is a noop,

richardschneider added a commit to ipld/js-ipld-dag-cbor that referenced this issue Jun 26, 2018
BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32
richardschneider added a commit to ipld/js-ipld-dag-pb that referenced this issue Jun 26, 2018
BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32
richardschneider added a commit to ipld/js-ipld-git that referenced this issue Jun 26, 2018
BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32
@richardschneider
Copy link

@vmx It looks likes bitcoin, zcash and ethereum all have issues, see ipld/js-ipld-zcash#13. Until this is resolved, perhaps

This means

  • NO breaking changes
  • you have time to think about the serializer issue
  • a few IPLD issues get to be closed

@vmx
Copy link
Member Author

vmx commented Jun 27, 2018

@richardschneider Sounds like a good plan. Let's do this!

@daviddias
Copy link
Member

@achingbrain I believe this is related to the perf work you have done for dag-pb. Could you confirm?

@achingbrain
Copy link
Member

That's how I came across it, yes - I don't have any hard numbers on the performance increase it would be though because it hasn't been a bottleneck in the profiling I've been doing - it just looked like a weird bit of code that the docs say should be able to be refactored to do less work (though in this case the docs appear to be ahead of the implementation).

@vmx vmx added api-review Tackle during the API review and removed api-review Tackle during the API review labels Nov 21, 2018
@vmx vmx closed this as completed May 8, 2019
@ghost ghost removed the ready label May 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants