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

feat: add blob/get #126

Merged
merged 13 commits into from
Jun 5, 2024
Merged

feat: add blob/get #126

merged 13 commits into from
Jun 5, 2024

Conversation

joaosa
Copy link
Contributor

@joaosa joaosa commented May 30, 2024

Adds the blob/get capability spec which is analogous to #82

w3-blob.md Outdated Show resolved Hide resolved
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

needs origin out though

w3-blob.md Outdated Show resolved Hide resolved
Co-authored-by: Vasco Santos <[email protected]>
w3-blob.md Outdated
}

type GetBlobOk = {
blob: blob
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw in the implementation you add the cause here. And I actually think that is nice, so also should be in the spec response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes perfect sense! adding it

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just some minor fixes needed.

w3-blob.md Outdated Show resolved Hide resolved
w3-blob.md Outdated Show resolved Hide resolved
w3-blob.md Outdated Show resolved Hide resolved
w3-blob.md Outdated Show resolved Hide resolved
w3-blob.md Outdated Show resolved Hide resolved
w3-blob.md Show resolved Hide resolved
w3-blob.md Outdated Show resolved Hide resolved
joaosa and others added 7 commits May 30, 2024 16:58
Co-authored-by: Alan Shaw <[email protected]>
Co-authored-by: Alan Shaw <[email protected]>
Co-authored-by: Alan Shaw <[email protected]>
Co-authored-by: Alan Shaw <[email protected]>
@joaosa joaosa requested a review from alanshaw May 30, 2024 16:01
@Gozala
Copy link
Collaborator

Gozala commented May 30, 2024

This might be controversial, but I wonder if we could simply return return pointer to the receipt and let client fetch all the details. The rational here is that adding a blob is a long running task and has state that moves from initiated to in progress to complete described by individual tasks.

I think it would make most sense to do that. That is not to say we should block shipping blob/get on this, but perhaps it would be better to not spec out in way that requires state checks on the server.

Please note that I'm not advocating for the above solution, it's really more of a question than prescription.

@Gozala
Copy link
Collaborator

Gozala commented May 30, 2024

In the implementation PR I have proposed adding version suffix to the capability storacha/w3up#1484 (review) I think it would make sense to add this changes to the spec with version suffix also maybe with some note that likely we'll deprecate it in the future in favor of suffixes-less version that just returns receipt id.

@joaosa joaosa requested a review from vasco-santos May 31, 2024 16:01
vasco-santos added a commit to storacha/w3up that referenced this pull request Jun 4, 2024
Adds support for `blob/get` as defined in
storacha/specs#126

---------

Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Irakli Gozalishvili <[email protected]>
w3-blob.md Outdated
message: string
}

type Blob = { /* ??? */ }
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to fill this out - it's multihash and size right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. completely missed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanshaw addressed

@joaosa joaosa requested a review from alanshaw June 4, 2024 17:34
@joaosa joaosa merged commit ff36626 into main Jun 5, 2024
2 checks passed
@joaosa joaosa deleted the feat/blob-get branch June 5, 2024 09:27
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.

4 participants