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

fix: publish location claim to content claims service #1571

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alanshaw
Copy link
Member

Big oversight here, although I think the original intention was to have the client publish these so perhaps not.

Anyways, since no location claims are being published the gateway cannot fetch an index and serve bytes quickly. It has to get a location claim for every single block using the old dynamodb. Being able to use the index will result in a speed increase for serving content via the gateway.

Depends on storacha/content-claims#74 for passing tests.

Copy link
Member

@Peeja Peeja left a comment

Choose a reason for hiding this comment

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

You're not making that claim?

Good catch! ❤️ LGTM!

body: data,
headers: address.headers,
})
assert.equal(httpPut.status, 200, await httpPut.text())
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: await httpPut.text() seems like a less-than-ideal assertion message to me. I see it's copying the pattern from blob/add schedules allocation and returns effects for allocate, accept and put together with their receipts (when stored). Could we give both these assertions more meaningful messages? Maybe something like:

Suggested change
assert.equal(httpPut.status, 200, await httpPut.text())
assert.equal(httpPut.status, 200, `PUT ${address.url} failed (${httpPut.status}): ${await httpPut.text()}`)

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.

3 participants