fix: check for blob/accept receipts before blob/add is concluded #1459

joaosa · 2024-05-15T12:22:29Z

Verifies that blob/accept has succeeded before concluding blob/add as per feat: add blob protocol to upload-client #1425 (comment). A location commitment is returned instead of the blob multihash.

vasco-santos · 2024-05-28T07:46:22Z

packages/upload-client/test/helpers/utils.js


 export const validateAuthorization = () => ({ ok: {} })

+// @ts-ignore Parameter
+export const setupGetReceipt = (contentGenFn) => {


I would recommend to simplify this to either have a separate process mocking the getReceipt endpoint, or create a http server within this process instead of mocking the actual fetch implementation.

in w3up-client we are using a mock server as a different process.
run process https://github.com/w3s-project/w3up/blob/604d300dde17521eea7333b949bb3f6796bd80a2/packages/w3up-client/package.json#L115
see that mock runs on npm run test https://github.com/w3s-project/w3up/blob/604d300dde17521eea7333b949bb3f6796bd80a2/packages/w3up-client/package.json#L109
https://github.com/w3s-project/w3up/blob/604d300dde17521eea7333b949bb3f6796bd80a2/packages/w3up-client/test/helpers/receipts-server.js

This is one way to go there, and if you see upload-client already uses this strategy for bucket-server https://github.com/w3s-project/w3up/blob/604d300dde17521eea7333b949bb3f6796bd80a2/packages/upload-client/package.json#L25 to write to a presigned URL

there is an alternative that maybe is more on the direction you are taking that you could also consider. you can rely on node http Server directly, freeway uses this kind of approach here https://github.com/web3-storage/freeway/blob/main/test/helpers/bucket.js. So, instead of a different process to run a mock server, the server runs in same process as the tests, and its URL and port are used by fetch to perfrom requests against.

thank you! I don't think the first approach would work as I need to load dynamic content into the receipt server. the 2nd approach should work and it does make sense, but I'm wondering if it's worth given the client refactor 🤔

if you could make this work I think it is fine. I think there is no impact on the client refactor but this was just to try to help out with not working tests.
However, I feel it would be much more cleaner than all the yield's in the tests below where I don't really understand some of them without going through the entire code path and logic, while a "real" server running would make it more close to what reality actually is. In addition, all the tests now need to change and have way more information being passed, while just relying on the Request to a real server with the path as the CID would simplify everything

joaosa · 2024-05-28T16:54:41Z

packages/upload-client/src/blob.js

+  // Fetch receipt from endpoint
+  const url = new URL(
+    taskCid.toString(),
+    options.receiptsEndpoint ?? receiptsEndpoint


One thing I'm wondering about is if there's a better way to achieve this param config and what kind implications this would have in terms of configuration in a testing environment 🤔 . Also, I'm usually wary of defaulting to production, but I see this is a pattern on the codebase and is, as such, likely to be okay here.

yes, I think this is the pattern here of defaulting to prod. So would say is good

packages/upload-client/package.json

packages/upload-client/src/blob.js

vasco-santos · 2024-05-30T12:52:41Z

packages/upload-client/src/blob.js

+      onFailedAttempt: console.warn,
+      retries: options.retries ?? REQUEST_RETRIES,
+    }


would be great to test that these defaults work nicely in a real environment, as they may exist a delay in between the receipt to be there. Did you try it against staging?

I have not tried this but that is a good point

packages/upload-client/src/index.js

vasco-santos · 2024-05-30T12:56:05Z

packages/upload-client/test/helpers/utils.js


 export const validateAuthorization = () => ({ ok: {} })

+// @ts-ignore Parameter
+export const setupGetReceipt = (contentGenFn) => {


if you could make this work I think it is fine. I think there is no impact on the client refactor but this was just to try to help out with not working tests.
However, I feel it would be much more cleaner than all the yield's in the tests below where I don't really understand some of them without going through the entire code path and logic, while a "real" server running would make it more close to what reality actually is. In addition, all the tests now need to change and have way more information being passed, while just relying on the Request to a real server with the path as the CID would simplify everything

packages/upload-client/test/index.test.js

packages/w3up-client/test/capability/blob.test.js

vasco-santos · 2024-06-03T11:16:24Z

packages/upload-client/test/index.test.js

@@ -453,6 +475,9 @@ describe('uploadDirectory', () => {
        onShardStored: (meta) => {
          carCID = meta.cid
        },
+        fetch: setupGetReceipt(() => {
+          return links[0]


could we by the path from the url used in fetch figure out which one is the CID for this link instead?

Oh yes that would be nicer, and allow it to respond appropriately when not found.

sadly, I don't think we can as we would need to compute the task ID from the initial links. I think it's best I remove this altogether

vasco-santos · 2024-06-03T11:17:15Z

packages/upload-client/test/helpers/utils.js

+    const taskID = url.pathname.replace('/receipt/', '')
+    const issuer = await Signer.generate()
+
+    const content = contentGen()


I think this content gen should pass the CID of the receipt being requested do that caller does what it needs

vasco-santos · 2024-06-03T11:18:56Z

packages/upload-client/src/blob.js

@@ -26,6 +27,40 @@ function createUploadProgressHandler(url, handler) {
  return onUploadProgress
 }

+// FIXME this code was copied over from w3up-client and modified to parameterise receiptsEndpoint


I though I wrote this earlier, but can't find it now. Can we make w3up to use this function from upload-client instead of having same code in both places?

addressed. I think I somehow mixed it with doing the same for the receipts endpoint

vasco-santos · 2024-06-04T10:05:03Z

packages/upload-api/src/blob/add.js

@@ -5,7 +5,6 @@ import * as Blob from '@web3-storage/capabilities/blob'
 import * as W3sBlob from '@web3-storage/capabilities/web3.storage/blob'
 import * as HTTP from '@web3-storage/capabilities/http'
 import * as API from '../types.js'
-


could we take this out to avoid a upload-api release?

vasco-santos

LGTM ❤️

packages/upload-client/src/receipts.js

Co-authored-by: Vasco Santos <[email protected]>

packages/upload-client/src/receipts.js

alanshaw · 2024-06-04T10:57:41Z

packages/upload-client/src/receipts.js

+    this.receiptsEndpoint = options.receiptsEndpoint ?? receiptsEndpoint
+    this.retries = options.retries ?? REQUEST_RETRIES
+    this.fetch = options.fetch ?? globalThis.fetch.bind(globalThis)
+    /* c8 ignore stop */


nit: IMHO it's overkill to use a class here, since there's no state that changes for the lifetime of the instance and in usage you're never storing the instance to a variable (i.e. await new ReceiptPoller(options).poll(...)).

You could just export 2 methods, poll and get, pass them options objects and import like import * as Receipt from './receipts.js'

alright sounds good. addressed as well

alanshaw · 2024-06-04T11:03:33Z

packages/upload-client/src/blob.js

+    {
+      root: /** @type {import('@ucanto/interface').UCANLink} */ (
+        // @ts-ignore Property
+        acceptReceipt.out.ok.site


You should check for error and then you don't need to ts-ignore here.

I don't think I need at all anymore, as previously the receipt acquisition method could return null. Now it's either a receipt or it throws.

alanshaw · 2024-06-04T11:04:39Z

packages/upload-client/src/blob.js

+    null
+  )
+  /* c8 ignore next 5 */
+  if (!site) {


Why would this be falsey?

Oh you need to not pass null above (pass undefined or simply omit) for view(...) to throw in the case where the root does not exist in the passed blocks.

alanshaw · 2024-06-04T11:09:52Z

packages/upload-client/src/index.js

+ * @param {import('./types.js').CARLink[]} shards
+ * @param {Array<Map<import('./types.js').SliceDigest, import('./types.js').Position>>} shardIndexes
+ */
+export async function indexShardedDAG(root, shards, shardIndexes) {


This should be an export from the blob-index utils.

packages/upload-client/src/receipts.js

alanshaw · 2024-06-04T11:16:19Z

packages/upload-client/src/receipts.js

+      headers: {},
+    })
+    // Get receipt from the potential multiple receipts in the message
+    const receipt = agentMessage.receipts.get(taskCid.toString())


nit: It's kinda odd that this endpoint returns an AgentMessage but is not a ucanto endpoint...

I wonder if this should just always return an agent message (but with no receipts in it if not found) and then you wouldn't have to check for HTTP 404 status as well.

This is because we in the past decided to redirect to the workflow object in S3, so there is really no way to know it exists, unless we would do redundant HEAD Request... We will need to UCANify the gets for workflows/receipts and, we may do changes at that point to the endpoint

Ah ok gotcha!

Co-authored-by: Alan Shaw <[email protected]>

alanshaw

LGTM

Tests blob client with storacha/w3up#1459

🤖 I have created a release *beep* *boop* --- ## [1.0.3](blob-index-v1.0.2...blob-index-v1.0.3) (2024-06-04) ### Fixes * check for blob/accept receipts before blob/add is concluded ([#1459](#1459)) ([462518c](462518c)) --- 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* --- ## [16.0.2](upload-client-v16.0.1...upload-client-v16.0.2) (2024-06-04) ### Fixes * check for blob/accept receipts before blob/add is concluded ([#1459](#1459)) ([462518c](462518c)) --- 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> Co-authored-by: Vasco Santos <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [14.0.0](w3up-client-v13.1.1...w3up-client-v14.0.0) (2024-06-04) ### ⚠ BREAKING CHANGES * **upload-api:** integrate agent store for idempotence & invocation/receipt persistence ([#1444](#1444)) * delegated capabilities required to use `uploadFile`, `uploadDirectory` and `uploadCAR` have changed. In order to use these methods your agent will now need to be delegated `blob/add`, `index/add`, `filecoin/offer` and `upload/add` capabilities. Note: no code changes are required. ### Features * generate sharded DAG index on client and invoke w `index/add` ([#1451](#1451)) ([a6d9026](a6d9026)) * **upload-api:** integrate agent store for idempotence & invocation/receipt persistence ([#1444](#1444)) ([c9bf33e](c9bf33e)) ### Fixes * check for blob/accept receipts before blob/add is concluded ([#1459](#1459)) ([462518c](462518c)) * export blob client ([#1485](#1485)) ([7944077](7944077)) * rename blob and index client capabilities ([#1478](#1478)) ([17e3a31](17e3a31)) --- 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> Co-authored-by: Vasco Santos <[email protected]>

joaosa force-pushed the fix/blob-add-cli branch 2 times, most recently from 3d0ebf1 to 7331b22 Compare May 15, 2024 16:57

alanshaw mentioned this pull request May 20, 2024

client work for blob protocol storacha/project-tracking#47

Closed

joaosa added 5 commits May 23, 2024 18:12

fix: check for blob/accept receipts before blob/add is concluded

06e42e9

fix: address passing receipts endpoint

3ac98a3

fix: return the site multihash for blob/add

f186499

feat: return the location commitment for blob/add

8773f33

fix: address tests to return location commitment for blob/add

cbf1bf0

joaosa force-pushed the fix/blob-add-cli branch from e7be8dd to cbf1bf0 Compare May 23, 2024 17:21

fix: address index/add tests

5a55ba2

joaosa force-pushed the fix/blob-add-cli branch from 6a49452 to 5a55ba2 Compare May 24, 2024 00:29

joaosa added 2 commits May 24, 2024 19:30

fix: use generators for getting receipts in upload-client tests

62c32e6

fix: use generators for getting receipts in w3up-client tests

bcca6c7

vasco-santos reviewed May 28, 2024

View reviewed changes

fix: pass w3up-client tests

a089685

joaosa force-pushed the fix/blob-add-cli branch from 624c1c6 to a089685 Compare May 28, 2024 16:30

joaosa requested review from vasco-santos, alanshaw and Gozala May 28, 2024 16:42

joaosa commented May 28, 2024

View reviewed changes

vasco-santos requested changes May 30, 2024

View reviewed changes

joaosa added 6 commits May 30, 2024 14:40

chore: bump content-claims to 5.0.0

9764432

chore: cleanup receipts endpoint setup

8ab1e9d

fix: relock deps

00326be

chore: remove unneeded generator from helper

9b84fbd

feat: wrap blob add response

61280e0

Merge branch 'main' into fix/blob-add-cli

021f353

joaosa requested a review from vasco-santos June 3, 2024 11:15

vasco-santos requested changes Jun 3, 2024

View reviewed changes

chore: reuse getReceipt code

8382376

joaosa added 4 commits June 4, 2024 10:48

fix: filter out getting the receipt not found error

24258da

chore: remove redundant error

7ee6b58

chore: add receipt missing error

06606b1

fix: return blob/add location commitment delegation

74e4c7a

vasco-santos reviewed Jun 4, 2024

View reviewed changes

chore: readd whitespace to avoid release

444dd8c

joaosa requested a review from vasco-santos June 4, 2024 10:28

joaosa mentioned this pull request Jun 4, 2024

Allow site delegation to be made public #1486

Open

fix: test blob/add location commitment

6c650c5

joaosa force-pushed the fix/blob-add-cli branch from a253d37 to 6c650c5 Compare June 4, 2024 10:34

vasco-santos mentioned this pull request Jun 4, 2024

fix: upgrade w3up client rc with receipt poll for accept storacha/w3infra#384

Merged

vasco-santos approved these changes Jun 4, 2024

View reviewed changes

packages/upload-client/src/receipts.js Outdated Show resolved Hide resolved

packages/upload-client/src/receipts.js Outdated Show resolved Hide resolved

joaosa and others added 2 commits June 4, 2024 12:13

chore: cleanup error propagation

7ad41d7

Co-authored-by: Vasco Santos <[email protected]>

chore: improve error desc

581cb82

Co-authored-by: Vasco Santos <[email protected]>

alanshaw requested changes Jun 4, 2024

View reviewed changes

joaosa and others added 4 commits June 4, 2024 12:20

chore: simplify response check

46a11b9

Co-authored-by: Alan Shaw <[email protected]>

chore: remove unneeded error check

1faa3dc

chore: move indexShardedDAG

1f08812

chore: break down the receipt class

c045997

joaosa requested a review from alanshaw June 4, 2024 11:55

alanshaw approved these changes Jun 4, 2024

View reviewed changes

vasco-santos added a commit to storacha/w3infra that referenced this pull request Jun 4, 2024

fix: upgrade w3up client rc with receipt poll for accept (#384)

62eb7c3

Tests blob client with storacha/w3up#1459

vasco-santos merged commit 462518c into main Jun 4, 2024
15 checks passed

vasco-santos deleted the fix/blob-add-cli branch June 4, 2024 12:40

This was referenced Jun 4, 2024

chore(main): release w3up-client 14.0.0 #1464

Merged

chore(main): release upload-client 16.0.2 #1488

Merged

chore(main): release blob-index 1.0.3 #1489

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: check for blob/accept receipts before blob/add is concluded #1459

fix: check for blob/accept receipts before blob/add is concluded #1459

joaosa commented May 15, 2024 •

edited

Loading

vasco-santos May 28, 2024

vasco-santos May 28, 2024

vasco-santos May 28, 2024

joaosa May 28, 2024

vasco-santos May 30, 2024

joaosa May 28, 2024 •

edited

Loading

vasco-santos Jun 3, 2024

vasco-santos May 30, 2024

joaosa May 30, 2024

vasco-santos May 30, 2024

vasco-santos Jun 3, 2024

alanshaw Jun 3, 2024

joaosa Jun 3, 2024

vasco-santos Jun 3, 2024

vasco-santos Jun 3, 2024

joaosa Jun 3, 2024

vasco-santos Jun 4, 2024

joaosa Jun 4, 2024

vasco-santos left a comment

alanshaw Jun 4, 2024

joaosa Jun 4, 2024

alanshaw Jun 4, 2024

joaosa Jun 4, 2024

alanshaw Jun 4, 2024

alanshaw Jun 4, 2024

joaosa Jun 4, 2024

alanshaw Jun 4, 2024

joaosa Jun 4, 2024

alanshaw Jun 4, 2024

vasco-santos Jun 4, 2024 •

edited

Loading

alanshaw Jun 4, 2024

alanshaw left a comment

fix: check for blob/accept receipts before blob/add is concluded #1459

fix: check for blob/accept receipts before blob/add is concluded #1459

Conversation

joaosa commented May 15, 2024 • 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

Choose a reason for hiding this comment

joaosa May 28, 2024 • 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw left a comment

Choose a reason for hiding this comment

joaosa commented May 15, 2024 •

edited

Loading

joaosa May 28, 2024 •

edited

Loading

vasco-santos Jun 4, 2024 •

edited

Loading