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: upgrade storefront api with filecoin offer from bucket event instead of directly compute #343

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Mar 18, 2024

This PR intends to upgrade storefront package. This enables filecoin/offer invocations lead to messages to the submit queue, where message consumer will compute Piece for the content. The main goal of this is to have the client to compute Filecoin Pieces and offer them, instead of pieces being directly computed from bucket event and written into the DB (skipping the submit validation).

Note that:

  • given we will need to deal with client upgrades and old clients still running for the time being, I opted in this PR to still keep a version of the bucket event in place. However, instead of it running computation of piece, it also triggers filecoin/offer so that we can keep old clients running where filecoin/offer is not invoked. This of course means that we may need to run Piece computation twice for the old path. However, looking at our AWS bill, we paid 200 USD in February for lambda execution of whole w3infra. This assumes then that we will not incur a lot of additional cost while making migration much simpler
  • datastore currently implements a composed store with S3+R2, which allows us to move on an iterative approach where we can deploy first step without client upgrade, while we can iterate on writes to anywhere with the overall picture.

Needs:

@vasco-santos vasco-santos marked this pull request as draft March 18, 2024 12:45
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from bbda033 to 1f3b1f3 Compare March 26, 2024 12:07
@vasco-santos vasco-santos marked this pull request as ready for review March 26, 2024 12:08
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch 2 times, most recently from a18f008 to 31a834f Compare March 26, 2024 12:19
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from 31a834f to e9c6d1c Compare March 26, 2024 12:44
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from e9c6d1c to f85dccb Compare March 26, 2024 13:02
@@ -140,7 +140,7 @@ export const createSQS = async (opts = {}) => {
const port = opts.port || 9324

const queue = await pRetry(() =>
new Container('softwaremill/elasticmq')
new Container('softwaremill/elasticmq:1.5.4')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like latest queue docker image is bad 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was failing with a random error, fixed it for now to previous version

@vasco-santos vasco-santos requested a review from alanshaw March 26, 2024 13:19
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from f85dccb to 0ab61f5 Compare March 26, 2024 13:53
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from 0ab61f5 to 4b7fe90 Compare March 26, 2024 14:54
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from 4b7fe90 to 6343e78 Compare March 26, 2024 15:14
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from 6343e78 to 01724a5 Compare March 26, 2024 15:28
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from 01724a5 to 45776db Compare March 26, 2024 16:22
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from 45776db to e40cd0f Compare March 26, 2024 16:49
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from e40cd0f to e33fa3e Compare March 26, 2024 17:22
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from e33fa3e to 0906e7a Compare March 26, 2024 17:37
@vasco-santos vasco-santos force-pushed the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch from 0906e7a to cb735d2 Compare March 26, 2024 17:49
@seed-deploy seed-deploy bot temporarily deployed to pr343 March 26, 2024 17:49 Inactive
Copy link

seed-deploy bot commented Mar 26, 2024

View stack outputs


const { ok, error } = await storefrontEvents.handlePieceInsertToEquivalencyClaim(context, record)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we now use the API 🎉

const context = {
pieceStore: createPieceTable(AWS_REGION, pieceTableName)
pieceStore: createPieceTable(AWS_REGION, pieceTableName),
dataStore: composeDataStoresWithOrderedStream(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want to read from S3 first, fallback to R2 if not there (prep for new world)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this would read more naturally if it was something like

cons s3 = createDataStore(AWS_REGION, s3BucketName)
const r2 = createDataStore(R2_REGION, r2BucketName, {
    endpoint: r2BucketEndpoint,
    credentials: {
      accessKeyId: r2BucketAccessKeyId,
      secretAccessKey: r2BucketSecretAccessKey,
    },
})

const store = S3.or(r2)

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 call, we actually already use this pattern on upload-api https://github.com/web3-storage/w3infra/blob/main/upload-api/functions/ucan-invocation-router.js#L185C52-L186C1 and that is why I kept. But probably we could get more declarative on a follow up

Comment on lines +37 to +57
// Only used for testing storing a CAR
// until we hook up claims to look for data
put: async (bytes) => {
const hash = await sha256.digest(bytes)
const root = CID.create(1, raw.code, hash)

const { writer, out } = CarWriter.create(root)
writer.put({ cid: root, bytes })
writer.close()

const chunks = []
for await (const chunk of out) {
chunks.push(chunk)
}
const blob = new Blob(chunks)
const cid = await CAR.codec.link(new Uint8Array(await blob.arrayBuffer()))

const putCmd = new PutObjectCommand({
Bucket: bucketName,
Key: `${cid.toString()}/${cid.toString()}.car`,
Body: bytes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we only need this for the tests, so that we can put something on the store. Maybe I should change the API to have an augmented store just on the tests interface, but not the actual service store. WDYT?

@@ -140,7 +140,7 @@ export const createSQS = async (opts = {}) => {
const port = opts.port || 9324

const queue = await pRetry(() =>
new Container('softwaremill/elasticmq')
new Container('softwaremill/elasticmq:1.5.4')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was failing with a random error, fixed it for now to previous version

@@ -13,7 +13,7 @@
"lint": "tsc && eslint '**/*.js'",
"clean": "rm -rf dist node_modules package-lock.json ./*/{.cache,dist,node_modules}",
"test": "npm test -w billing -w upload-api -w carpark -w replicator -w satnav -w roundabout -w filecoin",
"test-integration": "ava --verbose --serial --timeout=600s test/*.test.js",
"test-integration": "ava --verbose --serial --timeout=660s test/*.test.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if needed, but now tests look a bit slower with extra queue

if (result === undefined) {
throw new Error('no result received')
}
return result
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: result should probably aggregate errors as opposed to just returning the last error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, given this is a temporary thing I am tempted to keep it, given otherwise would be difficult to align the types

const context = {
pieceStore: createPieceTable(AWS_REGION, pieceTableName)
pieceStore: createPieceTable(AWS_REGION, pieceTableName),
dataStore: composeDataStoresWithOrderedStream(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this would read more naturally if it was something like

cons s3 = createDataStore(AWS_REGION, s3BucketName)
const r2 = createDataStore(R2_REGION, r2BucketName, {
    endpoint: r2BucketEndpoint,
    credentials: {
      accessKeyId: r2BucketAccessKeyId,
      secretAccessKey: r2BucketSecretAccessKey,
    },
})

const store = S3.or(r2)

@vasco-santos vasco-santos merged commit 3fd05a8 into main Apr 2, 2024
3 checks passed
@vasco-santos vasco-santos deleted the feat/upgrade-storefront-api-with-filecoin-offer-from-bucket-event-instead-of-compute branch April 2, 2024 11:01
vasco-santos added a commit to storacha/w3up that referenced this pull request Apr 19, 2024
This is not needed since #1332
got released and used in w3infra
storacha/w3infra#343 and has been deployed
in production without issues for the last few weeks

BREAKING CHANGE: not possible to skip submit queue on storefront service
anymore
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