-
Notifications
You must be signed in to change notification settings - Fork 9
fix: pieces can be removed from a data-set #253
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
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 a new rm command to the CLI that enables users to remove pieces from data sets via the Synapse SDK. The implementation follows established patterns from similar commands (add, import) and provides both CLI and library usage patterns through a flexible API design.
Key changes:
- New
rmcommand with options for piece CID, data set ID, and optional transaction confirmation - Core
removePiecefunction supporting both CLI (with dataSetId) and library (with StorageContext) usage patterns - Progress event system for tracking transaction submission and confirmation
- Package export for the new
core/piecemodule
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/commands/rm.ts |
Defines the CLI command interface with piece, data-set, and wait-for-confirmation options |
src/rm/types.ts |
TypeScript interfaces for rm command options and results |
src/rm/remove-piece.ts |
CLI orchestration layer with input validation, Synapse initialization, and progress tracking |
src/rm/index.ts |
Export file for rm module |
src/core/piece/remove-piece.ts |
Reusable core function for removing pieces with flexible API supporting both CLI and library usage |
src/core/piece/index.ts |
Export file for core piece module |
src/cli.ts |
Registers the new rm command with the main CLI program |
package.json |
Adds package export entry for the new core/piece module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Need to look into why we're still calling PDP server for datasets and piece info.. we should use warmstorage for source of truth. discussed in slack: https://filecoinproject.slack.com/archives/C095WFA0QK1/p1763984382967049?thread_ts=1763576428.763319&cid=C095WFA0QK1 |
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
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@SgtPooki : I assume you'll request review from Rod once tests are passing? |
|
there are certain circumstances (should not be possible, but we should show to users) where piece could be known on chain, but not on SP, or vice versa.. some changes have been added to display this. See slack thread for more details status types/**
* Status of the piece, e.g. "pending removal", "active", "orphaned"
*
* - "pending-removal": the piece is scheduled for deletion, but still showing on chain
* - "active": the piece is active, onchain and known by the provider
* - "onchain-orphaned": the piece is not known by the provider, but still on chain
* - "offchain-orphaned": the piece is known by the provider, but not on chain
*
* The orphaned states should not happen, but have been observed and should be logged and displayed to the user.
*/
export enum PieceStatus {
ACTIVE = 'ACTIVE',
PENDING_REMOVAL = 'PENDING_REMOVAL',
ONCHAIN_ORPHANED = 'ONCHAIN_ORPHANED',
OFFCHAIN_ORPHANED = 'OFFCHAIN_ORPHANED',
}output┌ Filecoin Onchain Cloud Data Set Details for #48
│
◇ ━━━ Data Set ━━━
│
│ Network: calibration
│ Client address: 0x2B3d0b827BAded638279C637d7ef21a266994c9E
│
│ #48
│ Status: live
│ CDN add-on: disabled
│
│ Provider
│ ID: 2
│ Address: 0xbCdf1bdc1a97D071a5a8EF03F1F05225b6E2a1Ba
│ Name: ezpdpz-calib2
│ Description: PDP Calibration node based in the UK
│ Service URL: https://calib2.ezpdpz.net
│ Active: yes
│ Location: C=GB;ST=Gloucestershire;L=Cheltenham
│
│ Metadata
│ source: "filecoin-pin"
│ withIPFSIndexing: ""
│
│ Payment
│ PDP rail ID: 89
│ Payer: 0x2B3d0b827BAded638279C637d7ef21a266994c9E
│ Payee: 0xbCdf1bdc1a97D071a5a8EF03F1F05225b6E2a1Ba
│
│ Pieces
│ Total pieces: 9
│ Total size: 900.0 B
│ Unique PieceCIDs: 6
│ Unique IPFS Root CIDs: 6
│
│ #1 (active)
│ PieceCID: bafkzcibcdub2fsyawhoxetnwlwxajaqovmapo6b2tgkuw2uwhnjwdqfo2zt6wmq
│ Size: 225.0 B
│ Metadata
│ ipfsRootCID: "bafybeidifi6iaamwjrohdnsnumattjcraz6rm46mbfzpphkbtgnse2wd54"
│
│ #2 (active)
│ PieceCID: bafkzcibcdub3mf2pi3y523askafb774xgzz5rwma2qgjpi2tgacxevdozo2huja
│ Size: 225.0 B
│ Metadata
│ ipfsRootCID: "bafybeids6gixmju6uwwji332pa7mg2hej2ecojoqppa5let6whxvuw474u"
│
│ #3 (active)
│ PieceCID: bafkzcibcdubu56fv7yais7htphs3j6q6n53sl3osyrwsc45a6zuvv2amdj5qopi
│ Size: 225.0 B
│ Metadata
│ ipfsRootCID: "bafybeieltmtc5xuye5h4jfhjpyujrcloyll5fjdsuop4ulvx6raekxl6zy"
│
│ #4 (offchain orphaned)
│ PieceCID: bafkzcibcdubzlvmhmqgjksynuephe32tz7letfuct6o6ielczcsjkifrpfllobq
│ Size: unknown
│ Metadata
│ ipfsRootCID: "bafybeig5utgrmrfqsvvhq42zopcxcf7cytcm4mpck6suaovlhzjt2hyxxu"
│
│ #5 (onchain orphaned)
│ PieceCID: bafkzcibcdubxifgdf7nmryydlkqbmo5q7a5r7wirrousepemrrkx55hb644awby
│ Size: 225.0 B
│ Metadata
│ ipfsRootCID: "bafybeidjstuje7huztr4mjwbqslwqkt4q4clypi76ijzzkwxvp5ngxzxra"
│
│ #6 (offchain orphaned)
│ PieceCID: bafkzcibcdubuavvokuyremde4qn5lh24rkscmx6onybphbhoeq6ijhwc2fdbcly
│ Size: unknown
│ Metadata
│ ipfsRootCID: "bafybeig577ua2iyo2cyhciu4vcgotxbj3cxsjnmbhbkgzak24ruvtchbxq"
│
│
└ Data set inspection complete |
@BigLep so, the tests are failing here because this depends on synapse-sdk PR: FilOzone/synapse-sdk#464 I can move to draft if we want to wait on that |
|
FilOzone/synapse-sdk#464 was just merged, so i'll update this PR when a new version of synapse-sdk is released |
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
Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.
Files not reviewed (1)
- upload-action/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
self review
| * @returns Transaction hash of the removal operation | ||
| * @throws Error if storage context is not bound to a Data Set | ||
| */ | ||
| export async function removePiece(pieceCid: string, options: RemovePieceOptions): Promise<`0x${string}` | string> { |
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.
do we want removePieceWithDataSetId and removePieceWithStorageContext helper functions or other better names?
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.
nah, in fact, you only seem to be using the dataSetId variant of this so maybe it doesn't need the context form of it? or you could do the opposite and require a context?
| try { | ||
| const warmStorage = await WarmStorageService.create(synapse.getProvider(), synapse.getWarmStorageAddress()) | ||
| const pdpVerifier = new PDPVerifier(synapse.getProvider(), warmStorage.getPDPVerifierAddress()) | ||
| scheduledRemovals = await pdpVerifier.getScheduledRemovals(storageContext.dataSetId) |
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.
in this instance, maybe we should look at adding getScheduledRemovals to StorageContext, this seems like it's going to be a common-enough operation for anyone who wants to do anything similar to this
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 should have done that in the synapse-sdk PR.. 🤦
| pieceCid: pieceCid.toString(), | ||
| status, | ||
| } | ||
| if (status === PieceStatus.ONCHAIN_ORPHANED) { |
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.
ooof, how often are we seeing these instances? I guess we're dealing with filecoin-project/curio#815 still, maybe eventually we can trust the two to be in sync but this is probably a good sanity check to do here.
I think, in the spirit of providing an example of usage, you should document what's going on here at the top of this block, or pull it out into a separate function that does the reconciliation / checking so it's much more clear what's going on and why.
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.
tbh, its not common, but there is a thread in slack where Kubuxu was looking into it, and I believe he opened filecoin-project/curio#825 to help with further investigation.
ACK on pulling the code out and documenting more about what's happening.
| }) | ||
|
|
||
| // Display results | ||
| log.spinnerSection('Results', [ |
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.
oh, I missed spinnerSection, the cli-logger is getting pretty fancy 👌
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 added that when I needed to get consistent spacing with the payments status output!
Add
rmcommand to CLI. This command will remove a piece from a data set.depends on FilOzone/synapse-sdk#464
Usage
Options
--piece <cid>: The CID of the piece to remove.--data-set <id>: The ID of the data set to remove the piece from.--wait-for-confirmation: Wait for transaction confirmation before exiting.still in draft because data-set show and rm are conflicting somewhere. See https://filecoinproject.slack.com/archives/C095WFA0QK1/p1763576428763319