-
Notifications
You must be signed in to change notification settings - Fork 27
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: chunk API module #60
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.
LGTM!
/** | ||
* Upload chunk to a Bee node | ||
* | ||
* The chunk data consists of 8 byte span and up to 4096 bytes of payload data. | ||
* The span stores the length of the payload in uint64 little endian encoding. | ||
* Upload expects the chuck data to be set accordingly. | ||
* | ||
* @param url Bee URL | ||
* @param hash Chunk reference | ||
* @param data Chunk data to be uploaded | ||
* @param options Aditional options like tag, encryption, pinning | ||
*/ | ||
export async function upload( |
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.
Do you think it makes sense to add validation in this function as well? I am talking about checking the span
, length of the payload
and hash
. Or would you add this to some utilities?
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 found this API too low-level for practical usage and probably subject to change anyhow. Because of this I also wouldn't expose this API on the Bee
interface. Instead I plan to implement utilities for creating correct chunks and calculate the BMT hash. We will need that to implement single owner chunks, but this is only necessary because there is no semantic API yet for handling those. Then I would expose only the SOC related functions on the Bee
interface.
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.
The only problem that I noticed, on local test with cluster nodes the chunk (and also file, byte) download attempt on not existing reference throws timeout error.
we should raise the default jest timeout because of that.
* | ||
* The chunk data consists of 8 byte span and up to 4096 bytes of payload data. | ||
* The span stores the length of the payload in uint64 little endian encoding. | ||
* Upload expects the chuck data to be set accordingly. |
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.
typeo
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.
What do you mean exactly? :)
This PR implements the endpoints related to chunks (chunk upload, download and pinning). It only adds at the module level because at the
Bee
class level I plan to add higher level functionality (e.g. Chunk interface instead ofUint8Array
s).The upload API is quite strange that it already requires the hash to be calculated in order to be able to upload. Since this functionality was not yet added, I hardcoded some pre-calculated values in the upload and download test.
The chunk pinning API has a few extra functions compared to the other (bytes, fiels, collection) APIs and testing it is a bit difficult with only having hardcoded values because this way the tests affect each other. I plan to change that in the next iteration when we are able to create (pseudo-)random chunks, in this PR the goal of the tests are to provide coverage for the endpoint wrapper functions.