-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use upstream cid #850
Use upstream cid #850
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'll just need to run interop tests and make sure no regressions as well as waiting for a few interop PRs to come in before merging this in though since this is a change that touches pretty much everything
@austinabell what about this plan:
|
Sounds great to me, the PRs I would like to get in before retesting are #847 #849 and #851 and I will let you know when those come in. I do believe your changes should be good as is, as I did some testing of it, I just want to check a couple things first and be completely sure if you don't mind |
Instead of a custom implementation of cid, wrap the upstream version.
An assert was commented out during development and was forgotten to be commented in again.
The `DAG_PB` constant isn't really needed.
8ee7270
to
d05edcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, should be good for release (these two comments are local changes, unless the display format is intended)
Thanks :) |
Instead of a custom implementation of cid, wrap the upstream version.
Summary of changes
Changes introduced in this pull request:
Other information and links
I removed the "default CID" case from the CID serializer as the old default CID won't work with rust-cid as it was an invalid CID.
This PR is intentionally using the rust-cid master branch and not a release as I would like to see if things would work with that version of rust-cid or if any changes would be nice/needed before I do a release (as this release is a huge breaking one, so it's the chance to break more things).
There is a
new_from_cbor()
function which I made into a module's function instead of adding it to theCid
impl. The reason is that I wanted to keep theCid
a pure wrapper around the upstreamCid
without additional methods. Though I'm happy to change that if requested.Feedback in welcome.