-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(storage-provider): upload a file and download CARv2 #115
Conversation
…-to-upload-the-file
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.
v1 looks great ;D
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 was trying this out and this can't be merged like this.
I thought it was a good idea to have both servers run from a single command but I disagree now. And it's very clear, they're separate concerns...
cargo run -- run
doesn't work out of the box, it needs to connect to a substrate server.
The upload server should be a separate command. The change isn't too big and it makes things simpler when handling signals and threads.
The graceful shutdown and timeout still apply, just separate the upload server into another command.
Hmm. I wonder how the storage client will exchange the content with the storage provider. Maybe down the line we won't even need the separate server. |
Filecoin does it over Graphsync and the SP is not necessarily on the same machine. |
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.
Some notes on the docker. I'm not entirely convinced that cargo-chef is really needed since we're going to ship the container to be used, not built from scratch. But if it improves local builds, that works for me
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.
One last question. It's important because if it fails and it simply stays put, the user will be left clueless.
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.
Great job! I learned a bunch about async too
Description
The change implements upload and download functionality. Currently, the axum server starts on a different port, but in the future, I imagine we would like to listen on the same address as the general rpc service. There are still many things to be solved before the functionality can be called finished. But as the POC it works.
How do you try it out?
Check #120 for a guide on how to try it out.
Checklist