feat: POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making #2522

gobengo · 2024-03-19T22:05:23Z

Motivation:

Note

many files I modified only because the upgrade to new typescript compiler or eslint required it
w3up-client requires node 18 so this drops support for node 16 from the api package
- I think the client will still work on node 16, but I couldn't find a way of getting the ci for client to, using node 16, install just the client package deps and not the api package deps (which tries to install w3up-client, and then yarn errored because that requies node 18)
w3up-client requires node 18, but everything had been running on node 16
- if we can't find a way of continuing to use v16, then when this merges to main, we probably need to go into cloudflare workers settings and change NODE_VERSION from 16 to 18+

Co-authored-by: Vasco Santos <santos.vasco10@gmail.com>

…y meant to configure a temporary feature switch

…-0007

…quest to w3up

This reverts commit 5fb898a.

cloudflare-workers-and-pages · 2024-03-19T22:06:09Z

Deploying nft-storage with Cloudflare Pages

Latest commit:	`4cea9a4`
Status:	✅ Deploy successful!
Preview URL:	https://18e68f1d.nft-storage-1at.pages.dev
Branch Preview URL:	https://w3s-0007-impl.nft-storage-1at.pages.dev

View logs

…ose)" This reverts commit 972a5bd.

…t isn't compatible with new deps. Use v18 and v20 instead

…fail on installing things that require w3up-client and node 18

…nless node 18

codecov-commenter · 2024-03-20T22:59:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4526369) to head (b1270cf).
Report is 3 commits behind head on main.

❗ Current head b1270cf differs from pull request most recent head fc15939. Consider uploading reports for the commit fc15939 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #2522   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1324      1324           
=========================================
  Hits          1324      1324

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…nless node 18

alanshaw · 2024-03-27T19:57:51Z

decisions/20240313-try-w3up.md

+
+###### Should write to w3up happen sync or async?
+
+i.e. if the write to w3up fails, should the POST /uploads request that triggered it also fail ('sync'=yes)? Or should we only do the write to w3up after a successful response to POST /uploads ('async')?


How would a user know that an upload had failed in this case? I think we should stick with sync.

packages/api/src/utils/w3up.js

alanshaw · 2024-03-27T20:12:53Z

packages/api/src/routes/nfts-upload.js

+  if (ctx.w3up) {
+    if (!w3upFeatureSwitchEnabled(ctx, { user })) {
+      console.warn(`skipping w3up upload. Feature switch does not allow it.`)
+    } else {
+      try {
+        await ctx.w3up.uploadCAR(carBlobForW3up)
+      } catch (error) {
+        // explicitly log so we can debug w/ cause
+        console.warn('error with w3up.uploadCAR', {
+          error,
+          cause: /** @type any */ (error)?.cause,
+        })
+        throw error
+      }
+    }
+  } else {
+    console.warn(
+      'nftUpload route skipping w3up storage due to missing ctx.w3up'
+    )
+  }


This needs to be pushed into uploadCar and the upload to w3up should happen instead of upload to s3/r2. There is more than one route that calls this. e.g. https://github.com/nftstorage/nft.storage/blob/main/packages/api/src/routes/nfts-store.js

ah yea - I think this was intentional and intended to limit the potential scope of changes - is there any downside to pushing the data through both upload paths for now, aside from some extra computation and associated $ costs? If the only problem is that doing both uploads will result in extra infra costs for the next couple months, and we can limit potentially disruptive changes then I'm in favor of just having this code do the upload both ways and then potentially removing the "classic" upload path once we're confident things are working... what do you think?

oh but I think your point about moving this inside uploadCar is correct either way!

I'm probbaly more pro replacing it. For one it makes it easier to test if everything is working or not - no false positives since it's not going through the old pipeline as well. It also saves money. I think it mostly boils down to a conditional over

nft.storage/packages/api/src/routes/nfts-upload.js

Lines 152 to 155 in 06712ad

const [s3Backup, r2Backup] = await Promise.all([

ctx.s3Uploader.uploadCar(stat.carBytes, carCid, user.id, metadata),

ctx.r2Uploader.uploadCar(stat.carBytes, carCid, user.id, metadata),

])

...but then linkdex API will not be able to determine completeness since it won't be put in S3. I reckon we could quite easily deploy an equivalent service that works on cloudflare and looks at R2.

ok! I started to look into this but I think it's a decent chunk of work and I wouldn't be able to finish before the end of the day, so I'll leave this one for you tomorrow!

this addresses most of the feedback: - fail to start if w3up client principal is not passed - move function to test helpers - make w3up service DID configurable

🤖 I have created a release *beep* *boop* --- ## [4.5.0](api-v4.4.3...api-v4.5.0) (2024-04-02) ### Features * POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making ([#2522](#2522)) ([0c77cb3](0c77cb3)) * uploadCarWithStat avoids copying stat.carBytes via Blob construction ([#2543](#2543)) ([bcb67a3](bcb67a3)) ### Bug Fixes * add a fake piece hasher to try to fix staging upload issues ([#2542](#2542)) ([542983c](542983c)) * avoid car repack ([#2551](#2551)) ([b0d622e](b0d622e)) * avoid car repack ([#2552](#2552)) ([b4ff571](b4ff571)) * change to post method ([#2547](#2547)) ([65e20fb](65e20fb)) * enable `url_standard` compatbility flag ([#2540](#2540)) ([bdde10a](bdde10a)) * enable the `url_standard` compatibility flag ([#2537](#2537)) ([e822bab](e822bab)) * enabled nodejs compat ([#2545](#2545)) ([654cde7](654cde7)) * log out proof ([225cf28](225cf28)) * log proof for debugging ([#2535](#2535)) ([0ecace3](0ecace3)) * more logging ([#2536](#2536)) ([11ae646](11ae646)) * remove debugging console.logs and nodejs_compat flag ([#2553](#2553)) ([6ceb299](6ceb299)) * Revert "fix: try older w3up client" ([#2549](#2549)) ([b31847b](b31847b)) * revert debugging prs ([#2539](#2539)) ([ba22d01](ba22d01)) * try older w3up client ([#2548](#2548)) ([5d94ec5](5d94ec5)) * upgrade @ipld/dag-cbor ([#2534](#2534)) ([964a12c](964a12c)) ### Other Changes * add a bunch of console.logs ([#2544](#2544)) ([2fc34f3](2fc34f3)) * expose simple route to debug ([#2546](#2546)) ([8a9aef5](8a9aef5)) * simple test case to check limits ([#2550](#2550)) ([6a50a70](6a50a70)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [2.9.0](website-v2.8.5...website-v2.9.0) (2024-04-04) ### Features * POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making ([#2522](#2522)) ([0c77cb3](0c77cb3)) ### Bug Fixes * pin tailwind to version used in production ([#2528](#2528)) ([64239e7](64239e7)) * use correct API URL for classic.nft.storage ([#2558](#2558)) ([cca7d5b](cca7d5b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [3.6.0](cron-v3.5.0...cron-v3.6.0) (2024-04-03) ### Features * POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making ([#2522](#2522)) ([0c77cb3](0c77cb3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [7.2.0](nft.storage-v7.1.1...nft.storage-v7.2.0) (2024-07-01) ### Features * POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making ([#2522](#2522)) ([0c77cb3](0c77cb3)) * **website:** update website to remove upload functionality ([#2704](#2704)) ([060dfd5](060dfd5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

gobengo and others added 10 commits March 13, 2024 16:08

add decisions/20240313-try-w3up.md

b77350e

Update decisions/20240313-try-w3up.md

4db52ef

Co-authored-by: Vasco Santos <santos.vasco10@gmail.com>

clarify NFTSTORAGE_W3S_ALLOW_ACCOUNTS may be removed later and is onl…

8481203

…y meant to configure a temporary feature switch

Merge branch 'w3s-0007' of github.com:nftstorage/nft.storage into w3s…

fd5a02e

…-0007

add packages/api/test.md so trigger api CI

5fb898a

update decisions/20240313-try-w3up.md with steps

21d602b

mention dagcargo in try-w3up problem statement

dc2bf27

add test for write to w3up, W3UP_URL env var, and placeholder http re…

987f940

…quest to w3up

mergeback from main

0d9b492

Revert "add packages/api/test.md so trigger api CI"

1565762

This reverts commit 5fb898a.

gobengo mentioned this pull request Mar 19, 2024

implement replacement for dagcargo as it is deprecating storacha/project-tracking#7

Closed

8 tasks

gobengo added 15 commits March 19, 2024 15:40

dont warn about missing optional env vars in CI (its too verbose)

972a5bd

Revert "dont warn about missing optional env vars in CI (its too verb…

5b8df19

…ose)" This reverts commit 972a5bd.

add w3up utils for parsing proof/principal

a16127b

upgrade typescript in packages/api to fix errors with new deps

c779311

fix tsc errors in packages/client

2ba345c

ci: .github/workflows/website.yml no longer uses node v16 because tha…

bd4d6fa

…t isn't compatible with new deps. Use v18 and v20 instead

upgrading eslint plugins to work with ts 5.0

7d4c838

ci: anything using node-version 16 now uses 18

e3fc4f6

fix ts errors in cron

0dde373

ci: client ci only does install in client package, so node 16 doesnt …

6ea03bc

…fail on installing things that require w3up-client and node 18

try differently for client ci to only install in packages/client

522d0db

try differently for client ci to only install in packages/client

00af17d

try differently for client ci to only install in packages/client

a6f3f70

try differently for client ci to only install in packages/client

672eac4

ci: client no longer tests using node 16 because yarn install fails u…

d94cbb3

…nless node 18

gobengo added 2 commits March 20, 2024 16:07

try differently for client ci to only install in packages/client

6d8cc30

ci: client no longer tests using node 16 because yarn install fails u…

0b22f78

…nless node 18

gobengo added 4 commits March 21, 2024 16:34

fix how test creates W3_NFTSTORAGE_PRINCIPAL value

fa84813

parseW3Proof allows base32

96fe60f

proof cid for test uses base64 multibase

2171390

rm unused import that breaks ci

05b65cb

gobengo force-pushed the w3s-0007-impl branch from e18d709 to 05b65cb Compare March 22, 2024 17:32

gobengo added 4 commits March 22, 2024 13:32

mock w3up uses ucanto. POST /upload/ handler does w3up.uploadCAR

67665f7

fix test and dont log secrets

cdc9426

fix tsc error

3a66d40

add feature switch for w3up upload

9d46b27

gobengo marked this pull request as ready for review March 22, 2024 23:29

gobengo requested a review from vasco-santos March 22, 2024 23:29

gobengo self-assigned this Mar 22, 2024

remove .only from test

9462e07

alanshaw requested changes Mar 27, 2024

View reviewed changes

travis and others added 5 commits March 27, 2024 16:32

fix: updates from Alan's review

fc15939

this addresses most of the feedback: - fail to start if w3up client principal is not passed - move function to test helpers - make w3up service DID configurable

refactor: move w3up uploading to uploadCar

11897aa

chore: move to classic.nft.storage

403ff99

fix: un remove ts-expect-errors

847c282

feat: get DAG structure for w3up uploads

4cea9a4

alanshaw approved these changes Mar 28, 2024

View reviewed changes

alanshaw merged commit 0c77cb3 into main Mar 28, 2024
22 checks passed

alanshaw deleted the w3s-0007-impl branch March 28, 2024 14:05

This was referenced Mar 28, 2024

chore(main): release website 2.9.0 #2529

Merged

chore(main): release cron 3.6.0 #2530

Merged

chore(main): release api 4.5.0 #2531

Merged

chore(main): release nft.storage 7.2.0 #2532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making #2522

feat: POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making #2522

gobengo commented Mar 19, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 19, 2024 •

edited

Loading

codecov-commenter commented Mar 20, 2024 •

edited

Loading

alanshaw Mar 27, 2024

alanshaw Mar 27, 2024

travis Mar 27, 2024

travis Mar 27, 2024

alanshaw Mar 27, 2024

travis Mar 27, 2024


		###### Should write to w3up happen sync or async?

		i.e. if the write to w3up fails, should the POST /uploads request that triggered it also fail ('sync'=yes)? Or should we only do the write to w3up after a successful response to POST /uploads ('async')?

	const [s3Backup, r2Backup] = await Promise.all([
	ctx.s3Uploader.uploadCar(stat.carBytes, carCid, user.id, metadata),
	ctx.r2Uploader.uploadCar(stat.carBytes, carCid, user.id, metadata),
	])

feat: POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making #2522

feat: POST /uploads can write uploads to w3up for web3 serving and dagcargoless filecoin deal-making #2522

Conversation

gobengo commented Mar 19, 2024 • edited Loading

cloudflare-workers-and-pages bot commented Mar 19, 2024 • edited Loading

Deploying nft-storage with Cloudflare Pages

codecov-commenter commented Mar 20, 2024 • edited Loading

Codecov Report

alanshaw Mar 27, 2024

Choose a reason for hiding this comment

alanshaw Mar 27, 2024

Choose a reason for hiding this comment

travis Mar 27, 2024

Choose a reason for hiding this comment

travis Mar 27, 2024

Choose a reason for hiding this comment

alanshaw Mar 27, 2024

Choose a reason for hiding this comment

travis Mar 27, 2024

Choose a reason for hiding this comment

gobengo commented Mar 19, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Mar 19, 2024 •

edited

Loading

codecov-commenter commented Mar 20, 2024 •

edited

Loading