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

fix(put): pass CID options to resolver #133

Merged
merged 5 commits into from
Jul 17, 2018
Merged

fix(put): pass CID options to resolver #133

merged 5 commits into from
Jul 17, 2018

Conversation

richardschneider
Copy link
Contributor

Creating the CID allows version and hashAlg to be specified; see #82.

@richardschneider
Copy link
Contributor Author

This PR is blocked on ipld/interface-ipld-format#40. Once it is merged and released then update the dependencies.

@richardschneider
Copy link
Contributor Author

@vmx All IPLD formats (dag-cbor, dag-pb, raw, git, bitcoin, ethereum and zcash) are updated to support cid-options. Can you release the packages to NPM?

@vmx
Copy link
Member

vmx commented Jun 27, 2018

@richardschneider When looking at e.g. js-ipld-bitcoin, I see that the options are ignored. I thought that's just a first step. I think silently ignoring options is a bad idea. It should either support hashAlg, or throw an error if it doesn't.

@richardschneider
Copy link
Contributor Author

richardschneider commented Jun 27, 2018

I agree, if the options.hashAlg != defaultHashAlg (for bitcoin, zcash, ethereum and git), then an error should be returned.

On second thought, I might be wrong. Perhaps a Bitcoin CID can have a non-default hashAlg.

@vmx
Copy link
Member

vmx commented Jul 2, 2018

@richardschneider I was about to do a js-ipld-ethereum release. But then I looked at the past commit again. It's missing dealing with options. They can be passed in, but they will silently do nothing. Can you please fix it as you did fix the other IPLD Format implementations?

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

There needs to be an js-ipld-ethereum release before this should be merged.

@richardschneider
Copy link
Contributor Author

@vms ethereum is ready to go. Can we get a new release?

0x-r4bbit added a commit to 0x-r4bbit/interface-ipfs-core that referenced this pull request Jul 4, 2018
At the time of this commit `dag.put()` basically ignores the `hashAlg`
option, as it passes it down to `ipld.put()`, which won't honor it until
ipld/js-ipld#133 is merged.

Once ipld/js-ipld#133 is merged, this test verifies
that e.g.

```
dag.put(cborNode, {
  format: 'dag-cbor',
  hashAlg: 'sha3-512'
}, (err, cid) => {
  ...
})
```

Actually results in a `CID` instance that decodes to `sha3-512` and not
the `sha2-256` default.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit added a commit to 0x-r4bbit/interface-ipfs-core that referenced this pull request Jul 4, 2018
As discussed in ipfs/js-ipfs#1419 (comment)
at the time of this commit, `dag.put()` basically ignores the `hashAlg`
option, as it passes it down to `ipld.put()`, which won't honor it until
ipld/js-ipld#133 is merged.

Once ipld/js-ipld#133 is merged, this test verifies
that e.g.

```
dag.put(cborNode, {
  format: 'dag-cbor',
  hashAlg: 'sha3-512'
}, (err, cid) => {
  ...
})
```

Actually results in a `CID` instance that decodes to `sha3-512` and not
the `sha2-256` default.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit added a commit to 0x-r4bbit/interface-ipfs-core that referenced this pull request Jul 4, 2018
As discussed in ipfs/js-ipfs#1419 (comment)
at the time of this commit, `dag.put()` basically ignores the `hashAlg`
option, as it passes it down to `ipld.put()`, which won't honor it until
ipld/js-ipld#133 is merged.

Once ipld/js-ipld#133 is merged, this test verifies
that e.g.

```
dag.put(cborNode, {
  format: 'dag-cbor',
  hashAlg: 'sha3-512'
}, (err, cid) => {
  ...
})
```

Actually results in a `CID` instance that decodes to `sha3-512` and not
the `sha2-256` default.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit added a commit to 0x-r4bbit/js-ipfs that referenced this pull request Jul 4, 2018
Over at ipfs-inactive/interface-js-ipfs-core#323 we introduce a
test spec that ensures `dag.put()` honors the `hashAlg` option, which at
the time of this commit is ignored by the underlying `ipld.put()` API
(more info: ipfs@1a36375).

This commit skips that spec in `js-ipfs` as ipld/js-ipld#133 has to
land first to fulfill the scenario.

License: MIT
Signed-off-by: Pascal Precht <pascal.precht@gmail.com>
0x-r4bbit added a commit to 0x-r4bbit/js-ipfs that referenced this pull request Jul 5, 2018
Over at ipfs-inactive/interface-js-ipfs-core#323 we introduce a
test spec that ensures `dag.put()` honors the `hashAlg` option, which at
the time of this commit is ignored by the underlying `ipld.put()` API
(more info: ipfs@1a36375).

This commit skips that spec in `js-ipfs` as ipld/js-ipld#133 has to
land first to fulfill the scenario.

License: MIT
Signed-off-by: Pascal Precht <pascal.precht@gmail.com>
alanshaw pushed a commit to ipfs-inactive/interface-js-ipfs-core that referenced this pull request Jul 5, 2018
As discussed in ipfs/js-ipfs#1419 (comment)
at the time of this commit, `dag.put()` basically ignores the `hashAlg`
option, as it passes it down to `ipld.put()`, which won't honor it until
ipld/js-ipld#133 is merged.

Once ipld/js-ipld#133 is merged, this test verifies
that e.g.

```
dag.put(cborNode, {
  format: 'dag-cbor',
  hashAlg: 'sha3-512'
}, (err, cid) => {
  ...
})
```

Actually results in a `CID` instance that decodes to `sha3-512` and not
the `sha2-256` default.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
alanshaw pushed a commit to ipfs/js-ipfs that referenced this pull request Jul 5, 2018
Over at ipfs-inactive/interface-js-ipfs-core#323 we introduce a
test spec that ensures `dag.put()` honors the `hashAlg` option, which at
the time of this commit is ignored by the underlying `ipld.put()` API
(more info: 1a36375).

This commit skips that spec in `js-ipfs` as ipld/js-ipld#133 has to
land first to fulfill the scenario.

License: MIT
Signed-off-by: Pascal Precht <pascal.precht@gmail.com>
@richardschneider
Copy link
Contributor Author

@vmx why iis circle CI failing? Otherwise, ready for a merge and release.

W: GPG error: https://apt.dockerproject.org ubuntu-precise Release: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY F76221572C52609D
W: Failed to fetch http://ppa.launchpad.net/pdoes/ppa/ubuntu/dists/precise/main/source/Sources  404  Not Found

W: Failed to fetch http://ppa.launchpad.net/pdoes/ppa/ubuntu/dists/precise/main/binary-amd64/Packages  404  Not Found

W: Failed to fetch http://ppa.launchpad.net/pdoes/ppa/ubuntu/dists/precise/main/binary-i386/Packages  404  Not Found

E: Some index files failed to download. They have been ignored, or old ones used instead.

sudo apt-get update returned exit code 100

Add tests for passing in `format` and `hashAlg` options into `put()`
to the eth-block format.
@richardschneider
Copy link
Contributor Author

@vmx Yeah team! are we now ready for a merge and release?

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

I take @richardschneider comment as an approval for my change. I'll wait for CI to be green (the current issue is hopefully fixed soon) and then it's ready to be merged and released!

@vmx vmx merged commit 10f3020 into master Jul 17, 2018
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.

2 participants