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 richardschneider mentioned this pull request Jun 27, 2018
Closed
7 tasks
@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 •
edited
Loading

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 vmx mentioned this pull request Jun 28, 2018
Closed
@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?

vmx
vmx suggested changes Jul 2, 2018
View reviewed changes
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?

@alanshaw alanshaw mentioned this pull request Jul 4, 2018
Merged
0x-r4bbit added a commit to 0x-r4bbit/interface-ipfs-core that referenced this pull request Jul 4, 2018
@0x-r4bbit
test(dag): add test to verify put API overrides hash algorithm
84a08ca 
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
@0x-r4bbit
test(dag): add test to verify put API overrides hash algorithm
404b929 
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 0x-r4bbit mentioned this pull request Jul 4, 2018
Merged
0x-r4bbit added a commit to 0x-r4bbit/interface-ipfs-core that referenced this pull request Jul 4, 2018
@0x-r4bbit
test(dag): add test to verify put API overrides hash algorithm
3b33c1a 
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
@0x-r4bbit
test(dag): skip spec for put API
86ead45 
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 0x-r4bbit mentioned this pull request Jul 4, 2018
Merged
0x-r4bbit added a commit to 0x-r4bbit/js-ipfs that referenced this pull request Jul 5, 2018
@0x-r4bbit
test(dag): skip spec for put API
0f93d0d 
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
@0x-r4bbit @alanshaw
test(dag): add test to verify put API overrides hash algorithm (#323)
1c6c9f0 
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
@0x-r4bbit @alanshaw
test(dag): skip spec for put API (#1421)
796bb46 
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

@vmx
test: add additional eth-block put() tests
39b79d4 
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?

vmx
vmx approved these changes Jul 17, 2018
View reviewed changes
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
@vmx vmx deleted the put-options branch July 17, 2018 13:46
@alanshaw alanshaw mentioned this pull request Jul 17, 2018
Merged
@alanshaw alanshaw mentioned this pull request Aug 2, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vmx vmx vmx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@richardschneider @vmx