Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: updates ipld-dag-pb dep to version without .cid or .multihash properties #1702

Closed
wants to merge 1 commit into from

Conversation

achingbrain
Copy link
Member

Follows on from ipld/js-ipld-dag-pb#99 and updates this module to not rely on DAGNodes having knowledge of their CIDs.

@ghost ghost assigned achingbrain Nov 8, 2018
@ghost ghost added the status/in-progress In progress label Nov 8, 2018
@achingbrain achingbrain force-pushed the update-dag-pb-to-not-have-cid-property branch from 2b5edc5 to 9b12a1f Compare November 9, 2018 07:22
@achingbrain achingbrain changed the title fix: updates ipld-dag-pb dep to version without .cid properties fix: updates ipld-dag-pb dep to version without .cid or .multihash properties Nov 9, 2018
@achingbrain achingbrain force-pushed the update-dag-pb-to-not-have-cid-property branch 2 times, most recently from 4ecf656 to 5573131 Compare November 9, 2018 14:01
@achingbrain achingbrain force-pushed the update-dag-pb-to-not-have-cid-property branch from 0caf558 to fda2bec Compare November 13, 2018 09:02
Follows on from ipld/js-ipld-dag-pb#99 and
updates this module to not rely on DAGNodes having knowledge of
their CIDs.
@achingbrain achingbrain force-pushed the update-dag-pb-to-not-have-cid-property branch from fda2bec to b0eab93 Compare November 13, 2018 09:06
},
'cid-base': {
default: 'base58btc',
describe: 'CID base to use.'
}
Copy link
Member

Choose a reason for hiding this comment

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

This has a lot of cross over with what's happening in #1552. Does this option need to be implemented in this PR? There's no added tests for it and what we have here has the potential to throw a lot of uncaught errors when requesting a base other than base58btc from a CID version 0 (see https://github.com/ipld/js-cid/blob/5653cbc479bcd8d0f676ccda84b22f5d241b65eb/src/index.js#L188).

Copy link
Member Author

Choose a reason for hiding this comment

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

It just seemed like a natural thing to add. I can add a test or remove it, as you prefer.

@alanshaw alanshaw mentioned this pull request Nov 19, 2018
@ghost ghost assigned alanshaw Nov 19, 2018
@alanshaw alanshaw mentioned this pull request Nov 19, 2018
49 tasks
@ghost ghost removed the status/in-progress In progress label Nov 20, 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