-
Notifications
You must be signed in to change notification settings - Fork 22
feat(storage): add dataSetId to piece confirmation callbacks #439
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
base: master
Are you sure you want to change the base?
Conversation
|
@virajbhartiya this needs a rebase |
|
@virajbhartiya is attempting to deploy a commit to the FilOz Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds enhanced callback APIs for storage uploads that expose dataset IDs and piece information when pieces are confirmed on-chain.
- Added
PieceIdentifiersinterface combiningpieceIdandpieceCid - Introduced
onPiecesAddedandonPiecesConfirmedcallbacks with richer data - Deprecated
onPieceAddedandonPieceConfirmedcallbacks
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/synapse-sdk/src/types.ts | Defines new PieceIdentifiers interface and adds onPiecesAdded/onPiecesConfirmed callbacks while deprecating old ones |
| packages/synapse-sdk/src/storage/context.ts | Implements new callbacks in upload flow, passing dataset ID and piece arrays to callbacks |
| packages/synapse-sdk/src/test/storage-upload.test.ts | Adds test coverage for new callback signatures and validates they receive correct arguments |
| utils/example-storage-e2e.js | Updates example to demonstrate new callbacks with piece arrays and dataset IDs |
| examples/cli/src/commands/upload.ts | Migrates CLI upload command to use new callback APIs |
| docs/src/content/docs/guides/storage.mdx | Updates storage guide documentation with new callback examples |
| docs/src/content/docs/developer-guides/storage/storage-context.mdx | Updates developer guide with comprehensive examples of new callback usage and type definitions |
Comments suppressed due to low confidence (2)
packages/synapse-sdk/src/storage/context.ts:1083
- Same issue as lines 1029-1032: the mapping of
confirmedPieceIdstopieceCidsby index could be incorrect if the arrays have different lengths. Validation should occur before firing the callbacks to prevent passing incorrect data toonPiecesConfirmed. Consider adding:
if (confirmedPieceIds.length !== pieceCids.length) {
throw createError('StorageContext', 'addPieces', `Expected ${pieceCids.length} confirmed piece IDs but got ${confirmedPieceIds.length}`)
} }
item.resolve(pieceId)
})
} catch (error) {
packages/synapse-sdk/src/storage/context.ts:1032
- The mapping of
confirmedPieceIdstopieceCidsby index could be incorrect if the arrays have different lengths. This validation happens later (line 1092-1095) but after callbacks are fired, potentially passing incorrect data toonPiecesConfirmed. Consider validating array lengths match before creatingpiecesForConfirmed, or use the same validation pattern here as used later:
if (confirmedPieceIds.length !== pieceCids.length) {
throw createError('StorageContext', 'addPieces', `Expected ${pieceCids.length} confirmed piece IDs but got ${confirmedPieceIds.length}`)
} : baseMetadataObj
// Convert to MetadataEntry[] for PDP operations (requires ordered array)
const finalMetadata = objectToEntries(metadataObj)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@SgtPooki rebased |
SgtPooki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comments from Hugo need looked at.. if we just use this.dataSetId in the checks where it exists we should be able to remove the null check
| const pieceCids: PieceCID[] = batch.map((item) => item.pieceCid) | ||
| const metadataArray: MetadataEntry[][] = batch.map((item) => item.metadata ?? []) | ||
| const confirmedPieceIds: number[] = [] | ||
| const piecesForAdded = pieceCids.map((pieceCid) => ({ pieceCid })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm not sure how large we expect the batches to grow, or how sensitive we need to be to performance, but we're looping over each batch three times here and may want to look (separate from this PR) into a reduce or for...of loop
|
Looking pretty good @virajbhartiya, some suggestions in there that it would be good for you to update. There are some disagreements we're having that it's not up to you to resolve, although you could use |
…ity in upload callbacks and data handling
docs/src/content/docs/developer-guides/storage/storage-context.mdx
Outdated
Show resolved
Hide resolved
|
Looks good! Just that one doc fix and I think this is OK to go. |
onPiecesConfirmed(dataSetId, pieces)callback exposing dataset ID when pieces are confirmedonPiecesAdded(transaction, pieces)callback with piece CIDsonPieceAddedandonPieceConfirmedCloses #409