-
Notifications
You must be signed in to change notification settings - Fork 0
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
CARv2 implementation #29
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.
After a high-level overview, I have some questions about the layer at which this implementation should be. Right now, it is under the binary crate file-storage
, but I thought that the car
implementation is abstract enough to be in a more low-level implementation, such as a library, rather than a binary, so we could at some point publish it on the crates.io. Additionally, the public API is not clear to me; it only exposes the generate_multihash
function and the MultihashCode
trait in mod.rs
.
Overall, the layers are mixed and the scope is a bit broken here, as we have a separate task for the polka-storage
crate implementation here.
Perhaps it's my fault that we didn't discuss this properly before implementation. Sorry about that.
Agreed, I just moved it under file-storage because there was no guide under the parachain implementations on division and I err'd on keeping it under one roof. I'll move it out. About the public API, it's still not finished, hence looking a bit quirky. |
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.
Nice 💪 I imagine this took a lot of work. I left some comments and questions :D
some CARv1 fixes depended on the CARv2, others were found during development, since v2 depends on v1, it's hard to keep them decoupled
They may come back for the UnixFS or other features
Previously, the `read_header` function would trust that the caller knew what they were doing. It no longer does that.
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.
Huge 🔥 Thanks @jmg-duarte!
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.
I didn't go through it all, but it's a piece of great work mate! All I have are questions/minor comments.
* overview of crate structure * note on `unixfs.proto`
We can always convert the macro back to a function and use `#[track_caller]`
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.
🚀 🚀
@jmg-duarte please check the CI. |
Description
Add a CARv2 reader and writer along with a basic block store as an higher-level interface to the writer.
Further development is required to make the interfaces more usable but that can, and should, be informed by usage needs.
Important points for reviewers
Specifications
Design based on
Couldn't simply take it into our system as not only the libraries are no longer maintained, they're also dependent on other components that we don't care about.
Checklist
fixes #25