feat: `/pieceCid` support for roundabout redirect api #233

olizilla · 2023-09-19T15:47:20Z

Upgrade roundabout /:cid endpoint to handle v2 PieceCIDs.

When passed a v2 piece cid, try and find an equivalent CAR CID for it from the content-claims api.

Redirect to a pre-signed URL for the car in R2 if we CAR CID that we are storing, (as roundabout does currently), otherwise 404.

This is good place for this logic, as consumers already expect to get a CAR back from requests to roundabout. /piece/:pieceCID was considered, but that would clash with the spec for fetching pieces, where the response body should be the padded piece itself https://github.com/filecoin-project/FIPs/blob/master/FRCs/frc-0066.md (here we return the equivalent, unpadded car, as asserted by an assert/equals content claim.

License: MIT

Add support for piece cids to the roundabout redirect to signed url api License: MIT Signed-off-by: Oli Evans <[email protected]>

License: MIT Signed-off-by: Oli Evans <[email protected]>

todo: currently fails as content-claims `read` fn expects web transform stream to be available globally License: MIT Signed-off-by: Oli Evans <[email protected]>

- CarReadableStream expects global `TransformStream` - ucanto client expects global `fetch` License: MIT Signed-off-by: Oli Evans <[email protected]>

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla · 2023-09-20T13:22:54Z

roundabout/test/piece.test.js

+  const pieceCid = CID.parse('bafkzcibbai3tdo4zvruj6zxo6wlt4suu3imi6to4vzmaojh4n475mdp5jcbtg')
+  const carCid = CID.parse('bagbaieratdhefxxpkhkae2ovil2tcs7pfr2grvabvvoykful7k2maeepox3q')


CIDs are the ones generated by the previous test.

Added to the content-claims api manually

❯ ./bin.js equals bafkzcibbai3tdo4zvruj6zxo6wlt4suu3imi6to4vzmaojh4n475mdp5jcbtg bagbaieratdhefxxpkhkae2ovil2tcs7pfr2grvabvvoykful7k2maeepox3q > equals-demo.claim ❯ ./bin.js write equals-demo.claim { ok: {} }

Note: the claims added using the CLI will expire and then not be retrievable. I will open an issue for setting it.

storacha/content-claims#28

Fixed! a never expiring claim has now been written

❯ ./bin.js equals bafkzcibbai3tdo4zvruj6zxo6wlt4suu3imi6to4vzmaojh4n475mdp5jcbtg bagbaieratdhefxxpkhkae2ovil2tcs7pfr2grvabvvoykful7k2maeepox3q -e never -o equals-demo-2.claim bafyreibb5x5sw2kw6kcr7pjrho5evli7e3n4uohc63ek3vh5dxvw42feeq ❯ ./bin.js inspect equals-demo-2.claim { iss: 'did:web:claims.web3.storage', aud: 'did:web:claims.web3.storage', v: '0.9.1', s: { '/': { bytes: '7aEDQJOpYFb4BilDcbMMK0YuLnkeCqVpFadt/ZGnaVEkb6aiQ2KzXUYkyBxyGhixA6GePjSrf21hDXxd79Wck6gklQQ' } }, exp: null, att: [ { can: 'assert/equals', nb: { content: { '/': 'bafkzcibbai3tdo4zvruj6zxo6wlt4suu3imi6to4vzmaojh4n475mdp5jcbtg' }, equals: { '/': 'bagbaieratdhefxxpkhkae2ovil2tcs7pfr2grvabvvoykful7k2maeepox3q' } }, with: 'did:web:claims.web3.storage' } ], prf: [], '/': 'bafyreibb5x5sw2kw6kcr7pjrho5evli7e3n4uohc63ek3vh5dxvw42feeq' } ❯ ./bin.js write equals-demo-2.claim (node:29766) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time (Use `node --trace-warnings ...` to show where the warning was created) { ok: {} }

seed-deploy · 2023-09-20T13:26:24Z

View stack outputs

pr233-w3infra-CarparkStack

Name Value

BucketName carpark-pr233-0

Region us-east-2
pr233-w3infra-RoundaboutStack

Name Value

ApiEndpoint https://rq2f72sh1c.execute-api.us-east-2.amazonaws.com

CustomDomain https://pr233.roundabout.web3.storage
pr233-w3infra-SatnavStack

Name Value

BucketName satnav-pr233-0

Region us-east-2
pr233-w3infra-UploadApiStack

Name Value

ApiEndpoint https://srv71jtmye.execute-api.us-east-2.amazonaws.com

CustomDomain https://pr233.up.web3.storage
pr233-w3infra-BusStack
pr233-w3infra-FilecoinStack
pr233-w3infra-ReplicatorStack
pr233-w3infra-UcanInvocationStack
pr233-w3infra-UploadDbStack

olizilla · 2023-09-20T13:29:40Z

roundabout/globals.js

+// NOTE: shim globals needed by content-claims client deps that would be present in nodejs v18.
+// TODO: migrate to sst v2 and nodejs v18+
+import { TransformStream, WritableStream, CountQueuingStrategy } from 'node:stream/web'
+import { fetch } from 'undici'
+globalThis.TransformStream = TransformStream
+globalThis.CountQueuingStrategy = CountQueuingStrategy
+globalThis.WritableStream = WritableStream
+globalThis.fetch = fetch


migrating to sst v2 is not trivial, so this seems to be the least worst workaround I can think of for now.

Mentioned in #152 to guarantee we do not forget to take these

vasco-santos

THis looks great 🙌🏼

Wonder if we could extend the integration tests as follow up? 😛

We know a piece has been written into the DB in https://github.com/web3-storage/w3infra/blob/main/test/integration.test.js#L247 . We could do a request to Roundabout for that pieceCID in same way we test roundabout with real infra in https://github.com/web3-storage/w3infra/blob/main/test/roundabout.test.js#L17

vasco-santos · 2023-09-20T13:44:07Z

roundabout/functions/redirect.js

+    const cars = await findEquivalentCarCids(cid)
+    for (const carCid of cars) {
+      // getUrl returns undefined if we don't have that car, so keep trying till we find a good one.
+      const signedUrl = await signer.getUrl(`${carCid}/${carCid}.car`, { expiresIn }) 


Suggested change

const signedUrl = await signer.getUrl(`${carCid}/${carCid}.car`, { expiresIn })

const key = `${carCid}/${carCid}.car`

const signedUrl = await signer.getUrl(key, { expiresIn })

nit

License: MIT Signed-off-by: Oli Evans <[email protected]>

Gozala

Looks great! I made some suggestions on how to make it bit easier for noob to read and understand it, but all those are just optional.

Additionally I would love if we provided support for PieceV1 CIDs, by which I mean either responding with status code Upgrade Required 426 (although not sure if that is right use of the code) or Unsupported Media Type 415 or perhaps Bad Request 400.

Either way above is probably better done as a followup.

Gozala · 2023-09-20T22:17:07Z

roundabout/piece.js

+    if (claim.type !== 'assert/equals') {
+      continue
+    }
+    const carCid = asCarCid(claim.content) ?? asCarCid(claim.equals)


I'm bit lost here, I was expecting that claim.content will be set to piece here, although I might be misinterpreting what fetchClaims supposed to do.

That is to say I would appreciate code comment above this line telling me what's going on so reader does not has to guess. It also may be worth noting what are the expectations in regards to fetchClaims function.

Gozala · 2023-09-20T22:21:59Z

roundabout/functions/redirect.js

-  return toLambdaResponse(signedUrl)
+  const signer = getSigner(s3Client, BUCKET_NAME)
+
+  if (asPieceCid(cid) !== undefined) {


Suggested change

if (asPieceCid(cid) !== undefined) {

// If request CID is of Filecoin Piece we attempt to find a payload CAR

// and redirect to it.

if (asPieceCid(cid) !== undefined) {

Gozala · 2023-09-20T22:35:15Z

roundabout/functions/redirect.js

+      if (signedUrl) {
+        return toLambdaResponse(signedUrl)
+      }
+    }


Nit: I personally prefer more linear logic so I don't have to maintain stack in my head, e.g. in this case return here would make it easier to follow as I don't have to consider next if block.

Better yet would be to refactor logic paths into separate functions e.g.

const resolvePiece = async (cid, { signer, expiresIn }) => { const piece = asPieceCid(cid) if (piece) { const cars = await findEquivalentCarCids(piece) // Attempt to find a CAR that we can resolve for (const car of cars) { const url = await signer.getUrl(`${car}/${car}.car`, { expiresIn }) if (url) { return url } } // We don't have a CAR for this piece CID return null } else { return null } } const resolveContentArcive = async (cid, {signer, expiresIn}) => { // ... } export async function redirectCarGet(request) { // ... return await resolvePiece(cid, {signer, expiresIn}) ?? await resolveContentArcive(cid, { signer, expiresIn }) ?? await notFound(cid) }

Nit: It also might be a good idea to return 404 with some error message explaining that we could not find a CAR for the piece. That way e.g. if user provides PieceV1 CID they'll be able to tell that we simply do not support those versions as opposed to us not having a content for them.

I prefer to have less state to think about too. I've refactored this to be clearer.

License: MIT Signed-off-by: Oli Evans <[email protected]>

vasco-santos

✨

License: MIT Signed-off-by: Oli Evans <[email protected]>

olizilla · 2023-09-21T16:43:55Z

roundabout/test/piece.test.js

+test('asPieceCidv2', t => {
+  const bytes = new Uint8Array(MIN_PAYLOAD_SIZE)
+  const piece = Piece.fromPayload(bytes)
+  const pieceCidV2 = piece.link
+  const pieceCidV1 = CID.createV1(Piece.FilCommitmentUnsealed, Digest.create(Piece.Sha256Trunc254Padded, piece.root))
+  const carCid = CID.createV1(CAR_CODE, sha256.digest(bytes)) 
+  const rawCid = CID.createV1(Raw.code, sha256.digest(bytes))
+  t.is(asPieceCidV2(pieceCidV1), undefined)
+  t.is(asPieceCidV2(pieceCidV2), pieceCidV2)
+  t.is(asPieceCidV2(carCid), undefined)
+  t.is(asPieceCidV2(rawCid), undefined)
+})


@Gozala here is how i'm (ab)using Piece from data-segment to test my checks for v2 and v1 piece cids.

Looks fine to me, although you could do const pieceCidV1 = piece.toInfo().link instead of manually constructing it

feat: /pieceCid support for roundabout redirect api

d2fb32c

Add support for piece cids to the roundabout redirect to signed url api License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 19, 2023 15:47 Failure

chore: fix package-lock and simplify 404 return

35eab85

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 20, 2023 09:30 Failure

feat: tests for pieceCID handling

eebf755

todo: currently fails as content-claims `read` fn expects web transform stream to be available globally License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 20, 2023 10:55 Failure

fix: shim globals for content-claims client

4986c5c

- CarReadableStream expects global `TransformStream` - ucanto client expects global `fetch` License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 20, 2023 12:47 Failure

chore: fix comments

884222b

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 20, 2023 12:53 Failure

chore: e2e test against content claims api

465243f

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 20, 2023 13:09 Failure

olizilla marked this pull request as ready for review September 20, 2023 13:09

chore: lint

1de86f2

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot temporarily deployed to pr233 September 20, 2023 13:21 Inactive

olizilla commented Sep 20, 2023

View reviewed changes

olizilla requested review from alanshaw, vasco-santos and Gozala September 20, 2023 13:25

olizilla commented Sep 20, 2023

View reviewed changes

vasco-santos mentioned this pull request Sep 20, 2023

fix: update sst to v2 #152

Closed

1 task

vasco-santos approved these changes Sep 20, 2023

View reviewed changes

alanshaw approved these changes Sep 20, 2023

View reviewed changes

chore: add integation test for /:pieceCid

8899a7f

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 20, 2023 16:21 Failure

Gozala approved these changes Sep 20, 2023

View reviewed changes

chore: refactor and detect v1 Piece CIDs

4f21db6

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 12:42 Failure

chore: refactor more

e1041a8

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 13:11 Failure

chore: fix e2e test

52fb402

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 13:27 Failure

fix: await carLocation

3048b11

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 13:53 Failure

olizilla added 2 commits September 21, 2023 15:30

Merge remote-tracking branch 'origin/main' into roundabout-piece

80e9734

chore: use staging claims api in staging

35a1be2

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 15:05 Failure

chore: fix test

c7fc29c

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot temporarily deployed to pr233 September 21, 2023 15:29 Inactive

chore: and roundabout check to integration test

72a3a1b

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 15:43 Failure

chore: fix integration test

26dffba

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 15:52 Failure

vasco-santos approved these changes Sep 21, 2023

View reviewed changes

chore: wait longer for integration test

f4fd152

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot had a problem deploying to pr233 September 21, 2023 16:08 Failure

chore: fix integration test, dont follow redirect

c0b5ffe

License: MIT Signed-off-by: Oli Evans <[email protected]>

seed-deploy bot temporarily deployed to pr233 September 21, 2023 16:30 Inactive

olizilla commented Sep 21, 2023

View reviewed changes

olizilla merged commit 44c75cd into main Sep 22, 2023
1 check passed

olizilla deleted the roundabout-piece branch September 22, 2023 08:49

olizilla mentioned this pull request Sep 25, 2023

w3up integration data-preservation-programs/spade#30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `/pieceCid` support for roundabout redirect api #233

feat: `/pieceCid` support for roundabout redirect api #233

olizilla commented Sep 19, 2023 •

edited

Loading

olizilla Sep 20, 2023

alanshaw Sep 20, 2023

alanshaw Sep 20, 2023

olizilla Sep 20, 2023

seed-deploy bot commented Sep 20, 2023

olizilla Sep 20, 2023

vasco-santos Sep 20, 2023

vasco-santos left a comment

vasco-santos Sep 20, 2023

Gozala left a comment

Gozala Sep 20, 2023

Gozala Sep 20, 2023

Gozala Sep 20, 2023

Gozala Sep 20, 2023

olizilla Sep 21, 2023

vasco-santos left a comment

olizilla Sep 21, 2023

Gozala Sep 21, 2023

		const pieceCid = CID.parse('bafkzcibbai3tdo4zvruj6zxo6wlt4suu3imi6to4vzmaojh4n475mdp5jcbtg')
		const carCid = CID.parse('bagbaieratdhefxxpkhkae2ovil2tcs7pfr2grvabvvoykful7k2maeepox3q')

	const signedUrl = await signer.getUrl(`${carCid}/${carCid}.car`, { expiresIn })
	const key = `${carCid}/${carCid}.car`
	const signedUrl = await signer.getUrl(key, { expiresIn })

feat: /pieceCid support for roundabout redirect api #233

feat: /pieceCid support for roundabout redirect api #233

Conversation

olizilla commented Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seed-deploy bot commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: `/pieceCid` support for roundabout redirect api #233

feat: `/pieceCid` support for roundabout redirect api #233

olizilla commented Sep 19, 2023 •

edited

Loading