feat(core): preserve non-standard SP registry capabilities in PDPOffering#687
feat(core): preserve non-standard SP registry capabilities in PDPOffering#687
Conversation
…ring There's potentially useful signals in here downstream from our consumption of the capabilities providers publish.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
synapse-dev | 62b69fb | Mar 30 2026, 04:02 PM |
|
CI failing due to ABI changes, fixing that separately #688 |
There was a problem hiding this comment.
Pull request overview
This PR updates PDP capability decoding to preserve non-standard Service Provider Registry capability entries (e.g., serviceStatus) so downstream consumers can access them without needing synapse-core to explicitly model every possible capability.
Changes:
- Allow unknown capability keys through PDP offering schema validation and surface them via
PDPOffering.extraCapabilities. - Add logic in
decodePDPCapabilitiesto separate known/typed capability fields from extra/untyped ones. - Add tests to verify extra capabilities are preserved and omitted when none exist.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/synapse-core/src/utils/pdp-capabilities.ts | Allows unknown capabilities through validation and returns them in extraCapabilities during decode. |
| packages/synapse-core/src/sp-registry/types.ts | Extends PDPOffering with optional extraCapabilities field. |
| packages/synapse-core/test/pdp-capabilities.test.ts | Adds unit tests covering preservation/omission/filtering of extraCapabilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ipniIpfs: zHex.optional(), | ||
| ipniPeerId: zHex.optional(), | ||
| }) | ||
| .passthrough() |
There was a problem hiding this comment.
PDPOfferingSchema uses .passthrough(), which means any non-standard capability values are accepted without validation. Since extraCapabilities is exposed as Record<string, Hex>, consider switching to .catchall(zHex) (or otherwise validating unknown keys) so extra capability values are guaranteed to be valid hex strings.
| .passthrough() | |
| .catchall(zHex) |
| const parsed = PDPOfferingSchema.safeParse(capabilities) | ||
| if (!parsed.success) { | ||
| throw new ZodValidationError(parsed.error) | ||
| } | ||
| return decodePDPCapabilities(parsed.data) | ||
| return decodePDPCapabilities(parsed.data as Record<string, Hex>) |
There was a problem hiding this comment.
parsed.data is being cast to Record<string, Hex> before calling decodePDPCapabilities. With .passthrough() this can mask invalid extra capability values (typed as unknown by Zod) and silently violate the Hex contract. Prefer adjusting the schema typing/validation (e.g., .catchall(zHex)) so this cast can be removed.
| const extraCapabilities: Record<string, Hex> = {} | ||
| let hasExtra = false | ||
| for (const key of Object.keys(capabilities)) { | ||
| if (!KNOWN_CAPABILITY_KEYS.has(key)) { | ||
| extraCapabilities[key] = capabilities[key] | ||
| hasExtra = true |
There was a problem hiding this comment.
extraCapabilities is built with a normal object literal and then assigned arbitrary, provider-controlled keys. Keys like __proto__/constructor can trigger prototype mutation in JS objects. Consider using Object.create(null) for extraCapabilities (or explicitly guarding against these keys) to avoid prototype-pollution style issues when consuming or serializing this map later.
There was a problem hiding this comment.
+1 to the copilot request
|
sorry i didn't get to this. heading on vacation so i wont be able to look until 2026 APR 8+ |
hugomrdias
left a comment
There was a problem hiding this comment.
LGTM but copilot's first suggestion seems to be good
| const extraCapabilities: Record<string, Hex> = {} | ||
| let hasExtra = false | ||
| for (const key of Object.keys(capabilities)) { | ||
| if (!KNOWN_CAPABILITY_KEYS.has(key)) { | ||
| extraCapabilities[key] = capabilities[key] | ||
| hasExtra = true |
There was a problem hiding this comment.
+1 to the copilot request
| let hasExtra = false | ||
| for (const key of Object.keys(capabilities)) { | ||
| if (!KNOWN_CAPABILITY_KEYS.has(key)) { | ||
| extraCapabilities[key] = capabilities[key] | ||
| hasExtra = true | ||
| } | ||
| } | ||
|
|
||
| return { ...required, ...optional, ...(hasExtra ? { extraCapabilities } : {}) } |
There was a problem hiding this comment.
| let hasExtra = false | |
| for (const key of Object.keys(capabilities)) { | |
| if (!KNOWN_CAPABILITY_KEYS.has(key)) { | |
| extraCapabilities[key] = capabilities[key] | |
| hasExtra = true | |
| } | |
| } | |
| return { ...required, ...optional, ...(hasExtra ? { extraCapabilities } : {}) } | |
| for (const key of Object.keys(capabilities)) { | |
| if (!KNOWN_CAPABILITY_KEYS.has(key)) { | |
| extraCapabilities[key] = capabilities[key] | |
| } | |
| } | |
| return { ...required, ...optional, ...({ extraCapabilities }) } |
this produces a more stable schema and is a simpler implementation
|
|
||
| const result = decodePDPCapabilities(capabilities) | ||
|
|
||
| assert.strictEqual(result.extraCapabilities, undefined) |
There was a problem hiding this comment.
| assert.strictEqual(result.extraCapabilities, undefined) | |
| assert.deepStrictEqual(result.extraCapabilities, {}) |
(if you merge above suggestion)
| } | ||
|
|
||
| /** Capability keys that are decoded into typed PDPOffering fields, derived from the schema */ | ||
| const KNOWN_CAPABILITY_KEYS = new Set([...Object.keys(PDPOfferingSchema.shape), CAP_IPNI_PEER_ID_LEGACY]) |
There was a problem hiding this comment.
This array likely becomes a maintenance burden, and is hard to test in integration.
Would it be easier to expose all capabilities in one object, and not differentiate standard and non-standard capabilities?
|
i will rebase and finish this tomorrow |
We shouldn't have been stripping theses, there's potentially useful signals in here downstream from our consumption of the capabilities providers publish. One example is FilOzone/dealbot#364 where the "serviceStatus" capability still has some use, but there's plenty more where clients have needs that align with what service providers want to dump in their capabilities field that we don't know or care about.