Skip to content
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

unified wireprotocol #348

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

unified wireprotocol #348

wants to merge 1 commit into from

Conversation

mabels
Copy link
Contributor

@mabels mabels commented Nov 11, 2024

pls review and share any ideas

@mabels mabels requested a review from jchris November 11, 2024 19:36
* "tid": "123",
* "payload": {
* "cid": "bafybeib2v"
* "bytes": [0x65, 0x66, 0x67, ...],
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just send the car raw as binary? the CAR get and PUT dont need to be parsed by the server

* "tid": "123",
* "payload": {
* "cid": "bafybeib2v"
* "bytes": [0x65, 0x66, 0x67, ...],
Copy link
Contributor

Choose a reason for hiding this comment

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

the put request to the signed upload URL generator doesnt include the bytes, the bytes are sent raw on the s3 protocol on a subsequent request

readonly cid: CID;
}
/*
* The response to a PutCar request.
Copy link
Contributor

Choose a reason for hiding this comment

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

the signed upload url can be returned as a redirect, but today we put it in a json envelope, either is ok, but the result here should just be a URL in the s3 protocol

* "auth": { ... }
* "meta": { ... }
* }
*/
Copy link
Contributor

@jchris jchris Nov 12, 2024

Choose a reason for hiding this comment

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

we need to be able to serve unencrpyted files directly at eg: <img src="https://example.fireproof.storage/database-uuid/fileCID"/> with correct content type

Copy link
Contributor

Choose a reason for hiding this comment

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

being able to PUT raw files is not required

readonly cid: CID;
}
/*
* A request to get a META file.
Copy link
Contributor

Choose a reason for hiding this comment

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

meta isn't quite a file, its an API response, as it must be composed by the server dynamically depending on current state

* "tid": "123",
* "payload": {
* "cid": "bafybeib2v"
* "bytes": [0x65, 0x66, 0x67, ...],
Copy link
Contributor

@jchris jchris Nov 12, 2024

Choose a reason for hiding this comment

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

this might be worth taking down another layer as JSON, instead of embedding bytes. Ideally you can see the CIDs of the relevant CARs in the JSON here, it would not be a larger serialization.

* {
* "type": "put-meta-req",
* "tid": "123",
* "payload": {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't have a body i dont think

readonly cid: CID;
}
/*
* The response to a PutMeta request.
Copy link
Contributor

Choose a reason for hiding this comment

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

this should respond with the HEAD post-PUT, inclusive of the PUTs effect -- same as a GET.

* "type": "get-wal-req",
* "tid": "123",
* "payload": {
* operations: DbMeta[];
Copy link
Contributor

Choose a reason for hiding this comment

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

right now this means we need a WAL per remote. we could keep high water marks, but that would require a function from (encrypted) CRDT log events to carLog CIDs. Not hard to do ... but wait til after we have the next layer of testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

this might be a good place for @Damienkatz help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants