feat!(cli): default egress to none and reuse existing data sets - #658
feat!(cli): default egress to none and reuse existing data sets#658SgtPooki wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the filecoin-pin CLI upload flows to make FilBeam egress opt-in by default and to reuse existing filecoin-pin data sets when the user provides no explicit targeting, reducing unnecessary new data set creation and associated lockups.
Changes:
- Default
--egress-providertononeforadd/import, keeping FilBeam routing explicitly opt-in. - Add default data set reuse logic to target existing live
source=filecoin-pindata sets (and CDN-enabled ones when egress is requested), with deterministic selection when more match than requested copies. - Update documentation and unit tests to reflect the new defaults and reuse behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| upload-action/README.md | Updates Action docs to reflect egress-off-by-default alignment with CLI. |
| upload-action/examples/cli-recipe/README.md | Updates CLI recipe docs for new egress default and opt-in behavior. |
| src/utils/cli-options-egress.ts | Updates CLI egress option documentation/help text for default none. |
| src/test/unit/upload-flow.test.ts | Adds unit coverage for data set reuse resolution and reuse selection logic. |
| src/test/unit/import.test.ts | Updates import tests for egress default and adds reuse “glue” coverage. |
| src/test/unit/add.test.ts | Updates add tests for egress default and adds reuse “glue” coverage. |
| src/import/import.ts | Defaults egress to none and wires in default data set reuse when untargeted. |
| src/core/data-set/resolve-by-metadata.ts | Adds requireKeys support and excludes terminating data sets from matches. |
| src/common/upload-flow.ts | Introduces pickDataSetsForReuse + resolveDefaultDataSetReuse. |
| src/add/add.ts | Defaults egress to none and wires in default data set reuse when untargeted. |
| documentation/glossary.md | Updates FilBeam egress glossary entry for opt-in default and network notes. |
| AGENTS.md | Updates contributor-facing docs to reflect new egress default. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9652a09 to
a0dfbfe
Compare
FilBeam CDN egress is now opt-in for add and import. Pass --egress-provider beam (or EGRESS_PROVIDER=beam) to route piece/CAR retrieval through FilBeam; the default no longer creates CDN-enabled data sets or locks the extra 1 USDFC. This also aligns the CLI default with the upload-action, which already defaulted egressProvider to none.
6ddcf35 to
1bf7c7d
Compare
When add/import run without --data-set-id, --provider-id, or --data-set-metadata, match any live, active data set whose metadata carries source=filecoin-pin and upload into it instead of relying on the SDK's smart-select. The SDK requires exact metadata equality, so data sets carrying extra keys (e.g. withCDN from when FilBeam egress was the default) were skipped and a new data set was created per copy. When more data sets match than copies requested, pick the ones storing the most pieces, spread across distinct providers. When FilBeam egress is requested, only CDN-enabled data sets qualify so the egress request is never silently dropped. With fewer matches than requested copies, or none, the SDK resolves or creates data sets as before. The subset matcher now also skips data sets scheduled for termination (pdpEndEpoch set), which applies to --data-set-metadata resolution too.
1bf7c7d to
698a649
Compare
SgtPooki
left a comment
There was a problem hiding this comment.
self review. Note that this removes FilBeam as default for add/import operations, and fixes data-set matching so filecoin-pin focuses only on finding data-sets where source: filecoin-pin, so we don't force existing users who are adding data to create new data-sets when this goes through.
I think defaulting to just source matching for data-sets is a net improvement for filecoin-pin users.
beck-8
left a comment
There was a problem hiding this comment.
Although I don't know what the background of the change is. But there seems to be no problem with logic.
| spinner.stop( | ||
| `${pc.gray('•')} Found ${resolution.matchedIds.length} existing data set(s) but need ${expectedCopies}; the SDK will resolve or create data sets` | ||
| ) | ||
| return undefined | ||
| } | ||
|
|
||
| spinner.stop(`${pc.gray('•')} No existing filecoin-pin data sets found; the SDK will create new ones`) |
There was a problem hiding this comment.
"the SDK" - this is leaky detail the user doesn't need to see
| if (resolution.kind === 'too-many-matches') { | ||
| const chosen = pickDataSetsForReuse(resolution.matchedDataSets, expectedCopies) | ||
| spinner.stop( | ||
| `${pc.green('✓')} Reusing existing data sets ${chosen.join(', ')} (${resolution.matchedIds.length} matched, picked the ${expectedCopies} storing the most data)` |
There was a problem hiding this comment.
"most data" maybe not, but "most pieces"
but, I don't know you really need to be too wordy in these messages, terseness is nice so you don't overload the user with detail they may not want to try to understand
| for (const ds of sorted) { | ||
| if (picked.length >= count) break | ||
| if (!picked.includes(ds)) picked.push(ds) | ||
| } |
There was a problem hiding this comment.
I think you need to remove "before doubling up on one" variation in this method, as you say: "copies on the same provider add no redundancy" so why do it? if you can't find unique providers, then it's time to set up a new data set, but becasuse this function is assumed to pass you're going to need to handle a failure case in resolveDefaultDataSetReuse to fall-through to the "let the SDK handle it"
What changed
Two isolated commits:
feat!(cli): default --egress-provider to none. FilBeam CDN egress is now opt-in foraddandimport(--egress-provider beamorEGRESS_PROVIDER=beam). The default no longer creates CDN-enabled data sets or locks the extra 1 USDFC, and the CLI now matches the upload-action, which already defaulted tonone. Reverts the default introduced in Add --egress-provider flag with beam as default #464.feat(cli): reuse existing filecoin-pin data sets by default. Without--data-set-id,--provider-id, or--data-set-metadata,add/importnow match any live, active data set withsource=filecoin-pinand upload into it. The SDK's smart-select requires exact metadata equality, so after the default flip every upload from an existing user would have skipped theirwithCDN-tagged data sets and created new ones per copy. The subset match reuses them instead. When more data sets match than copies requested, it picks the ones storing the most pieces across distinct providers (as documented in the glossary's Data Set entry). With--egress-provider beam, only CDN-enabled data sets qualify.Calibration test runs
Wallet
0x44f0...3759(972 data sets: 970 non-CDN, 2 CDN from the oldbeamdefault).Old flow: v1.3.0, no flags (beam default)
Every upload defaults into CDN-tagged data sets (egress billed to the owner). For a wallet without CDN data sets, this creates 2 new ones and locks 2 USDFC, even when hundreds of non-CDN filecoin-pin data sets exist: exact metadata matching cannot see them.
New flow: this branch, no flags (none default)
No FilBeam notice, no CDN lockup, and the upload lands in the wallet's own long-lived data sets (231: 595 pieces on provider 7, 54: 215 pieces on provider 4). Data set 84 (541 pieces) did not qualify: its PDP payments have ended (
pdpEndEpochset).New flow with --egress-provider beam: reuses only CDN data sets, confirmed on-chain
With
beamrequested, only the 2 CDN-enabled data sets qualify (not the 966 non-CDN ones), so the egress request is never silently dropped.How to verify
pnpm run build && npx vitest run. New coverage:resolveDefaultDataSetReuse/pickDataSetsForReuseinupload-flow.test.ts, glue tests inadd.test.ts/import.test.ts, and the flipped egress-default tests.Notes / risks
--provider-idoverrides. Candidate follow-up: skip unreachable providers during the pick.--egress-provider none, CDN-tagged data sets still qualify for reuse:nonemeans "do not request or create CDN", not "never add to a CDN data set". Excluding them would recreate the new-set churn for wallets whose only data sets came from the oldbeamdefault. Documented in theresolveDefaultDataSetReusedocblock.withCDN, so its data sets already match exactly.pdpEndEpochset); this applies to--data-set-metadataresolution too.generated with claude, peer-reviewed with three other models (codex, cursor-agent, gemini)