feat: add provider command with info and ping subcommands #295
Conversation
beck-8
commented
Dec 22, 2025
beck-8
commented
|
@beck-8 : sorry this got missed. It wasn't added to the board, which is why I hadn't seen it. It has been now. @rvagg : it would be good to get your input on whether you want to see this functionality in filecoin-pin vs. in a Synapse CLI. @SgtPooki : I have assigned this to you, but feel free to assign to someone else or note if we shouldn't get distracted by this currently. |
BigLep
added
team/fs-wg
There was a problem hiding this comment.
Pull request overview
Adds a new provider CLI command to inspect registered storage providers and ping their PDP endpoints.
Changes:
- Introduces
provider infoto list approved/active providers or show details for a specific provider. - Introduces
provider pingto probe provider PDP/pdp/pingendpoints (approved/active or specific provider). - Adds unit tests for the new provider command behaviors and wires the command into the main CLI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/commands/provider.ts | Implements provider info/provider ping, formatting and output logic, and auth defaulting. |
| src/test/unit/provider.test.ts | Adds unit tests covering info/ping behaviors, including --all handling. |
| src/cli.ts | Registers the new provider command in the CLI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (data.minPieceSizeInBytes) console.log(` Min Piece Size: ${formatFileSize(data.minPieceSizeInBytes)}`) | ||
| if (data.maxPieceSizeInBytes) console.log(` Max Piece Size: ${formatFileSize(data.maxPieceSizeInBytes)}`) | ||
| if (data.storagePricePerTibPerDay) console.log(` Storage Price: ${formatUSDFC(BigInt(data.storagePricePerTibPerDay))} USDFC/TiB/Day`) | ||
| if (data.minProvingPeriodInEpochs) console.log(` Min Proving Period: ${data.minProvingPeriodInEpochs} epochs`) |
There was a problem hiding this comment.
These checks use truthiness to decide whether to print values. If a field can legally be 0 / 0n, it will be skipped. Prefer explicit null/undefined checks (e.g., != null) so zero values are still displayed.
| if (data.minPieceSizeInBytes) console.log(` Min Piece Size: ${formatFileSize(data.minPieceSizeInBytes)}`) | |
| if (data.maxPieceSizeInBytes) console.log(` Max Piece Size: ${formatFileSize(data.maxPieceSizeInBytes)}`) | |
| if (data.storagePricePerTibPerDay) console.log(` Storage Price: ${formatUSDFC(BigInt(data.storagePricePerTibPerDay))} USDFC/TiB/Day`) | |
| if (data.minProvingPeriodInEpochs) console.log(` Min Proving Period: ${data.minProvingPeriodInEpochs} epochs`) | |
| if (data.minPieceSizeInBytes != null) console.log(` Min Piece Size: ${formatFileSize(data.minPieceSizeInBytes)}`) | |
| if (data.maxPieceSizeInBytes != null) console.log(` Max Piece Size: ${formatFileSize(data.maxPieceSizeInBytes)}`) | |
| if (data.storagePricePerTibPerDay != null) console.log(` Storage Price: ${formatUSDFC(BigInt(data.storagePricePerTibPerDay))} USDFC/TiB/Day`) | |
| if (data.minProvingPeriodInEpochs != null) console.log(` Min Proving Period: ${data.minProvingPeriodInEpochs} epochs`) |
| export const providerCommand = new Command('provider') | ||
| .description('Inspect and interact with storage providers') | ||
|
|
||
| const infoCommand = new Command('info') | ||
| .description('View provider info. Lists all approved providers if no ID/Address specified.') | ||
| .argument('[provider]', 'Provider ID or Address') | ||
| .option('--all', 'List all active providers (ignoring approval status)') | ||
| .action(async (providerIdOrAddr, options) => { | ||
| try { | ||
| ensurePublicAuth(options) | ||
|
|
There was a problem hiding this comment.
Formatting/indentation in this file uses 4-space indents in many blocks/chains, but the repo’s Biome config enforces 2-space indentation (see biome.json formatter.indentWidth: 2). Running Biome formatting (or adjusting indentation) will avoid CI/lint failures.
| // Fix hoisting issue: define mocks in hoisted block | ||
| const { mockWarmStorage, mockSynapse, mockGetProvider, mockGetAllActiveProviders } = vi.hoisted(() => { | ||
| const mockGetProvider = vi.fn() | ||
| const mockGetAllActiveProviders = vi.fn() | ||
| const mockWarmStorage = { | ||
| getServiceProviderRegistryAddress: vi.fn(), | ||
| getApprovedProviderIds: vi.fn(), | ||
| getProvider: vi.fn() | ||
| } | ||
| const mockSynapse = { | ||
| getProvider: vi.fn(), | ||
| storage: { | ||
| _warmStorageService: mockWarmStorage | ||
| } | ||
| } | ||
| return { mockWarmStorage, mockSynapse, mockGetProvider, mockGetAllActiveProviders } | ||
| }) |
There was a problem hiding this comment.
Formatting/indentation in this test file uses 4-space indents, but the repo’s Biome config enforces 2-space indentation (biome.json formatter.indentWidth: 2). Run Biome formatting (or adjust indentation) to prevent formatter/lint failures in CI.
| // Construct ping URL: append /pdp/ping | ||
| // Ensure serviceUrl doesn't end with / to avoid double slashes when simple appending, | ||
| // or use URL API. | ||
| const baseUrl = serviceUrl.endsWith('/') ? serviceUrl.slice(0, -1) : serviceUrl | ||
| const pingUrl = `${baseUrl}/pdp/ping` |
There was a problem hiding this comment.
Ping URL construction will produce a duplicated "/pdp" segment when the provider's serviceURL already includes the PDP base path (e.g., .../pdp), resulting in .../pdp/pdp/ping. Consider normalizing with the URL API and handling both forms (base host vs PDP base path), e.g., if the pathname already ends with /pdp then append /ping, otherwise append /pdp/ping.
| // Construct ping URL: append /pdp/ping | |
| // Ensure serviceUrl doesn't end with / to avoid double slashes when simple appending, | |
| // or use URL API. | |
| const baseUrl = serviceUrl.endsWith('/') ? serviceUrl.slice(0, -1) : serviceUrl | |
| const pingUrl = `${baseUrl}/pdp/ping` | |
| // Construct ping URL, handling both base host and PDP base path forms. | |
| // If the service URL path already ends with `/pdp`, append `/ping`, | |
| // otherwise append `/pdp/ping`. | |
| const url = new URL(serviceUrl) | |
| const normalizedPath = url.pathname.replace(/\/+$/, '') | |
| if (normalizedPath.endsWith('/pdp')) { | |
| url.pathname = `${normalizedPath}/ping` | |
| } else { | |
| url.pathname = `${normalizedPath}/pdp/ping` | |
| } | |
| const pingUrl = url.toString() |
There was a problem hiding this comment.
idk how valid this is.. I don't remember if serviceUrl includes /pdp, but I dont think so
| // Use GET for specific ping endpoint | ||
| const res = await fetch(pingUrl, { method: 'GET', signal: controller.signal }).catch(async () => { | ||
| if (controller.signal.aborted) throw new Error('Timeout') | ||
| throw new Error('Network Error') | ||
| }) | ||
| clearTimeout(timeout) |
There was a problem hiding this comment.
clearTimeout(timeout) is only reached when fetch() resolves successfully; if fetch() rejects (network error/abort), the timer remains scheduled and can fire later, causing unnecessary work and potential flaky behavior. Use a try/finally around the fetch() so the timeout is always cleared.
| // Use GET for specific ping endpoint | |
| const res = await fetch(pingUrl, { method: 'GET', signal: controller.signal }).catch(async () => { | |
| if (controller.signal.aborted) throw new Error('Timeout') | |
| throw new Error('Network Error') | |
| }) | |
| clearTimeout(timeout) | |
| let res: Response | |
| try { | |
| // Use GET for specific ping endpoint | |
| res = await fetch(pingUrl, { method: 'GET', signal: controller.signal }).catch(async () => { | |
| if (controller.signal.aborted) throw new Error('Timeout') | |
| throw new Error('Network Error') | |
| }) | |
| } finally { | |
| clearTimeout(timeout) | |
| } |
| if (!provider) { | ||
| console.error(pc.red(`Provider ${providerIdOrAddr} not found or invalid.`)) | ||
| process.exit(1) | ||
| } | ||
| printProvider(provider) | ||
| } else { | ||
| let providers = [] | ||
| if (options.all) { | ||
| providers = await spRegistry.getAllActiveProviders() | ||
| console.log(pc.bold(`Found ${providers.length} active providers (all):`)) | ||
| } else { | ||
| const approvedIds = await warmStorage.getApprovedProviderIds() | ||
| console.log(pc.bold(`Found ${approvedIds.length} approved providers:`)) | ||
| const providersOrNull = await Promise.all(approvedIds.map((id: number) => spRegistry.getProvider(id))) | ||
| providers = providersOrNull.filter(p => p !== null) | ||
| } | ||
|
|
||
| if (providers.length > 0) { | ||
| printTable(providers) | ||
| } | ||
| } | ||
|
|
||
| await cleanupSynapseService() | ||
| } catch (error) { | ||
| console.error('Failed to get provider info:', error instanceof Error ? error.message : error) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
cleanupSynapseService() is only called on the success path. In error branches where process.exit(1) is invoked (including the !provider case and the outer catch), telemetry/provider cleanup is skipped, which can drop pending telemetry flushes and leaves cleanup behavior inconsistent across outcomes. Consider moving cleanupSynapseService() to a finally block and avoiding early process.exit inside the action (prefer throw + single exit point) so cleanup runs reliably.
There was a problem hiding this comment.
please be sure you're following #254.. look to other commands for the pattern
| const infoCommand = new Command('info') | ||
| .description('View provider info. Lists all approved providers if no ID/Address specified.') | ||
| .argument('[provider]', 'Provider ID or Address') | ||
| .option('--all', 'List all active providers (ignoring approval status)') |
There was a problem hiding this comment.
The help text advertises "Provider ID or Address", but the implementation rejects non-numeric inputs and does not support address lookup. Either implement address-based lookup or update the description/argument help to only mention numeric IDs to avoid misleading CLI UX.
| // adjust widths based on content (optional, but good for table) | ||
| rows.forEach(r => { | ||
| columns.forEach(c => { | ||
| const val = (r as any)[c.key] || '' | ||
| if (val.length > c.width) c.width = Math.min(val.length, 60) // cap max width | ||
| }) |
There was a problem hiding this comment.
Table width capping sets c.width = 60 for long values, but row rendering never truncates the value to c.width (so very long strings will still overflow and break alignment). If you want capped widths, truncate values (optionally with an ellipsis) to c.width when rendering rows/headers.
As a user who uploads data, I prefer to use this function in a tool with complete addition, deletion, modification and query functions. Instead of putting it in Synapse utils |
Well ... I'm not too fussed to be honest. Synapse doesn't technically have a CLI, it's got a set of "examples", so we're probably not going to be recommending that as a go-to in our documentation if we need to tell people how to find something. If we were publishing it to npm as a CLI then maybe. There's a package there, it's named In lieu of that, we have filecoin-pin, which is a really nice CLI already, easy to install and run and useful beyond just pinning IPFS data. So I'm fine making it even nicer. Then users who care about the IPFS use-case have a more full-featured CLI client, and we have a legit place to point people to if we want to tell them to run commands. It's not a perfect fit because of the IPFS bias, but even the name "filecoin-pin" doesn't strictly mean IPFS and CARs, it's just a brand. |
|
Ok, sweet. So we're aligned that functionality like this can live in Should we maybe give optionality and display on whether SPs are approved vs. not approved? Side: I assume this could be another way to satisfy FilOzone/filecoin-services#359 |
|
The command I added only displays "approved" by default; all SPs are only displayed when using "-all". |
Ack, got it - I see that now. I dont see for the "provider detail command" (e.g., |
| const infoCommand = new Command('info') | ||
| .description('View provider info. Lists all approved providers if no ID/Address specified.') | ||
| .argument('[provider]', 'Provider ID or Address') | ||
| .option('--all', 'List all active providers (ignoring approval status)') |
There was a problem hiding this comment.
we discussed this previously with data-set --ls in order to list and --id to specify a specific one. Please follow the patterns there
| export const providerCommand = new Command('provider') | ||
| .description('Inspect and interact with storage providers') | ||
|
|
||
| const infoCommand = new Command('info') | ||
| .description('View provider info. Lists all approved providers if no ID/Address specified.') | ||
| .argument('[provider]', 'Provider ID or Address') | ||
| .option('--all', 'List all active providers (ignoring approval status)') | ||
| .action(async (providerIdOrAddr, options) => { | ||
| try { | ||
| ensurePublicAuth(options) | ||
|
|
| if (!provider) { | ||
| console.error(pc.red(`Provider ${providerIdOrAddr} not found or invalid.`)) | ||
| process.exit(1) | ||
| } | ||
| printProvider(provider) | ||
| } else { | ||
| let providers = [] | ||
| if (options.all) { | ||
| providers = await spRegistry.getAllActiveProviders() | ||
| console.log(pc.bold(`Found ${providers.length} active providers (all):`)) | ||
| } else { | ||
| const approvedIds = await warmStorage.getApprovedProviderIds() | ||
| console.log(pc.bold(`Found ${approvedIds.length} approved providers:`)) | ||
| const providersOrNull = await Promise.all(approvedIds.map((id: number) => spRegistry.getProvider(id))) | ||
| providers = providersOrNull.filter(p => p !== null) | ||
| } | ||
|
|
||
| if (providers.length > 0) { | ||
| printTable(providers) | ||
| } | ||
| } | ||
|
|
||
| await cleanupSynapseService() | ||
| } catch (error) { | ||
| console.error('Failed to get provider info:', error instanceof Error ? error.message : error) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
please be sure you're following #254.. look to other commands for the pattern
| if (data.minPieceSizeInBytes) console.log(` Min Piece Size: ${formatFileSize(data.minPieceSizeInBytes)}`) | ||
| if (data.maxPieceSizeInBytes) console.log(` Max Piece Size: ${formatFileSize(data.maxPieceSizeInBytes)}`) | ||
| if (data.storagePricePerTibPerDay) console.log(` Storage Price: ${formatUSDFC(BigInt(data.storagePricePerTibPerDay))} USDFC/TiB/Day`) | ||
| if (data.minProvingPeriodInEpochs) console.log(` Min Proving Period: ${data.minProvingPeriodInEpochs} epochs`) |
| // Use GET for specific ping endpoint | ||
| const res = await fetch(pingUrl, { method: 'GET', signal: controller.signal }).catch(async () => { | ||
| if (controller.signal.aborted) throw new Error('Timeout') | ||
| throw new Error('Network Error') | ||
| }) | ||
| clearTimeout(timeout) |
| // Construct ping URL: append /pdp/ping | ||
| // Ensure serviceUrl doesn't end with / to avoid double slashes when simple appending, | ||
| // or use URL API. | ||
| const baseUrl = serviceUrl.endsWith('/') ? serviceUrl.slice(0, -1) : serviceUrl | ||
| const pingUrl = `${baseUrl}/pdp/ping` |
There was a problem hiding this comment.
idk how valid this is.. I don't remember if serviceUrl includes /pdp, but I dont think so
4 participants
