fix: upgrade filecoin api with content store to rely on roundabout #360
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
View stack outputs
|
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
2e5bb6a
to
3253ccd
Compare
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
3253ccd
to
1f45386
Compare
vasco-santos
added a commit
to storacha/w3up
that referenced
this pull request
Apr 30, 2024
Filecoin api exported tests still using CARs instead of Blobs, which means storacha/w3infra#360 need to have TestContentStore to use CARs to make these tests work https://github.com/w3s-project/w3infra/pull/360/files#diff-cd843762832d0767495ff56f3f8692a15262dce245db89690794a1316868e25aR99 But, we want to use Blobs already, so let's make the tests also want Blobs across Filecoin pipeline :) note that in practise both Blobs and CARs work, but for testing we are only writing one of them and is a CAR today https://github.com/w3s-project/w3up/blob/main/packages/filecoin-api/test/events/storefront.js#L42
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
1f45386
to
ef018d1
Compare
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
ef018d1
to
7164afa
Compare
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
7164afa
to
0907272
Compare
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
0907272
to
f335473
Compare
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
f335473
to
16c788c
Compare
vasco-santos
commented
Apr 30, 2024
| @@ -41,6 +41,7 @@ export default { | |||
|
|
|||
| app.stack(BusStack) | |||
| app.stack(UploadDbStack) | |||
| app.stack(RoundaboutStack) | |||
There was a problem hiding this comment.
FilecoinStack now requires Roundabout endpoint deployed
vasco-santos
force-pushed
the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
from
16c788c
to
5f05460
Compare
vasco-santos
commented
Apr 30, 2024
| // create URL for the link to be fetched | ||
| const getUrl = new URL(`/${cid.toString()}`, storeHttpEndpoint) | ||
|
|
||
| // Retry a few times as it looks like R2 sometimes takes a bit to make it available |
There was a problem hiding this comment.
context: This was difficult to find. It was making integration tests sometimes fail before I added the retry as R2 would give 404...
alanshaw
approved these changes
Apr 30, 2024
| res = await pRetry((async () => { | ||
| const fetchRes = await fetch(getUrl, { | ||
| // Follow potential redirects | ||
| redirect: 'follow', |
filecoin/store/content.js
Outdated
| }) | ||
| if (fetchRes.status === 404) { | ||
| throw new RecordNotFound(`blob ${cid.toString()} not found in store`) | ||
| } else if (fetchRes.status > 299 || !fetchRes.body) { |
There was a problem hiding this comment.
Suggested change
| } else if (fetchRes.status > 299 || !fetchRes.body) { | |
| } else if (!fetchRes.ok || !fetchRes.body) { |
filecoin/store/content.js
Outdated
| } | ||
|
|
||
| // To satisfy typescript | ||
| if (!res.body) { |
There was a problem hiding this comment.
Return fetchRes.body above where you have already checked it is not null and then you don't need this check.
vasco-santos
deleted the
fix/upgrade-filecoin-api-with-content-store-to-use-roundabout
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today Storefront relies directly on a bucket (in S3) to read bytes to derive PieceCID for validation. With blob protocol, we are moving to write to R2 only 🙌🏼 therefore, we aim to make filecoin Storefront able to read from anywhere, but also not increase the infra costs with this change.
This PR changes Storefront to rely on Roundabout to read Blob bytes to derive Piece CID from anywhere instead of
contentStorebeing directly hooked to a bucket. See usageRelying on Roundabout introduces a layer of indirection, but in current implementation where this validation runs in AWS and content will likely be read from CF, makes it cost optimal to read from Roundabout.
Closes storacha/w3up#1349