Apply specific variant types for ChainIndexingMetrics#1612
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConvert chain indexing metrics to a discriminated-union model (Queued/Backfill/Realtime/Completed), refactor deserialization to build/validate per-chain variants, update mocks and a client test, add DeepPartial utility, and wrap per-chain checkpointBlock in a new ChainIndexingStatus object. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR refactors the Ponder SDK data model to use a discriminated union for Key changes:
The changes maintain backward compatibility for ENSIndexer use cases and improve type safety through discriminated unions, making invalid states unrepresentable at the type level. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant PonderClient
participant Ponder as Ponder App
participant Deserializer
participant Validator
Client->>PonderClient: metrics()
PonderClient->>Ponder: GET /metrics
Ponder-->>PonderClient: Prometheus metrics text
PonderClient->>Deserializer: deserializePonderIndexingMetrics(text)
Deserializer->>Deserializer: deserializePrometheusMetrics(text)
Deserializer->>Validator: check invariants (required metrics, no conflicts)
alt Validation fails
Validator-->>Client: throw Error
end
Deserializer->>Deserializer: buildUnvalidatedPonderIndexingMetrics()
loop For each chain
Deserializer->>Deserializer: buildUnvalidatedChainIndexingMetrics()
alt ponder_sync_is_complete === 1
Deserializer->>Deserializer: return ChainIndexingMetricsCompleted
else ponder_sync_is_realtime === 1
Deserializer->>Deserializer: return ChainIndexingMetricsRealtime
else
Deserializer->>Deserializer: return ChainIndexingMetricsHistorical
end
end
Deserializer->>Validator: validate with Zod schemas
alt Validation fails
Validator-->>Client: throw Error
end
Validator-->>Client: PonderIndexingMetrics
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Adding If backward compatibility is required, gate this requirement (e.g., treat missing metric as “not supported” and fall back to prior logic), or bump major version / document the minimum Ponder version this SDK now requires. |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the ChainIndexingMetrics type in the Ponder SDK from a single interface with boolean flags to a discriminated union with four distinct states. This improves type safety and makes the state machine more explicit and easier to reason about.
Changes:
- Converted
ChainIndexingMetricsfrom an interface with conditional boolean fields to a discriminated union of four state variants:Queued,Backfill,Realtime, andCompleted - Updated deserialization logic to build the appropriate state based on Prometheus metrics values
- Enhanced invariant checking to validate state-specific constraints at the Prometheus metrics level
- Updated test mocks and test assertions to reflect the new data model
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ponder-sdk/src/indexing-metrics.ts | Defines the new discriminated union type structure with four state variants, replacing the previous single interface with boolean flags |
| packages/ponder-sdk/src/deserialize/utils.ts | Adds DeepPartial utility type for handling partial/unvalidated data structures during deserialization |
| packages/ponder-sdk/src/deserialize/indexing-metrics.ts | Updates deserialization logic to construct appropriate state variants based on Prometheus metrics, with enhanced invariant validation |
| packages/ponder-sdk/src/deserialize/indexing-metrics.mock.ts | Updates mock test data to use the new discriminated union structure with explicit type fields |
| packages/ponder-sdk/src/client.test.ts | Updates test assertion to match the new error message format for conflicting metrics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tk-o
left a comment
tk-o
left a comment
There was a problem hiding this comment.
Self-review completed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/ponder-sdk/src/deserialize/indexing-metrics.ts`:
- Around line 75-141: In buildUnvalidatedChainIndexingMetrics, treat missing
per-chain metric values as "Queued" instead of falling through to Backfill:
change the ponderHistoricalCompletedIndexingSeconds check to treat both 0 and
undefined/null as queued (e.g., if ponderHistoricalCompletedIndexingSeconds ==
null || ponderHistoricalCompletedIndexingSeconds === 0) and ensure the
subsequent checks for ponderSyncIsComplete and ponderSyncIsRealtime only
consider a value of 1 as true (leave them as strict === 1) so undefined/null
won't be misinterpreted; update logic around latestSyncedBlock and
backfillTotalBlocks handling accordingly so missing metric values don't produce
an unintended Backfill result.
In `@packages/ponder-sdk/src/deserialize/utils.ts`:
- Around line 31-37: DeepPartial currently treats any non-primitive as a nested
object (so Map/Set/Date/RegExp/Function get recursed into); update the
conditional in the DeepPartial type to exclude built-in non-plain objects by
adding a guard: after the array branch check add a case like "T[P] extends Date
| Function | Map<any, any> | Set<any> | WeakMap<any, any> | WeakSet<any> |
RegExp ? T[P] : DeepPartial<T[P]>" so that the type alias DeepPartial<T> only
recurses into plain object shapes and returns the original type for those
built-ins.
In `@packages/ponder-sdk/src/indexing-metrics.ts`:
- Around line 116-136: The doc comment for the interface
ChainIndexingMetricsRealtime uses ambiguous phrasing "the backfill phase
transitioned to completed phase"; update the comment above the
ChainIndexingMetricsRealtime declaration to clearly state that the backfill
phase has finished and the chain has entered realtime indexing mode (i.e.,
backfill completed and indexing continues with no target end block), replacing
the ambiguous "transitioned to completed phase" wording with a concise phrasing
like "the backfill phase has finished and the chain entered realtime mode" so
readers of the ChainIndexingMetricsRealtime docs understand the intended state.
|
@greptile review |
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ponder-sdk/src/indexing-status.ts (1)
30-37:⚠️ Potential issue | 🟡 MinorJSDoc on
chainsis now slightly inaccurate.Line 32 still says "Map of indexed chain IDs to their block reference," but the value type is now
ChainIndexingStatus, notBlockRef.📝 Suggested doc fix
/** - * Map of indexed chain IDs to their block reference. + * Map of indexed chain IDs to their indexing status. * * Guarantees: * - Includes entry for at least one indexed chain. */ chains: Map<ChainId, ChainIndexingStatus>;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tk-o
force-pushed
the
refactor/ponder-sdk-indexing-metrics
branch
from
849d4cd to
8feeada
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tk-o
left a comment
tk-o
left a comment
There was a problem hiding this comment.
Self-review completed.
| schemaChainIdString, | ||
| schemaSerializedChainIndexingMetrics, | ||
| ); | ||
| const schemaChainsIndexingMetrics = z.map(schemaChainId, schemaChainIndexingMetrics); |
There was a problem hiding this comment.
We'll use business-level representation of ChainId instead of its string representation.
|
|
||
| function invariant_includesAtLeastOneIndexedChain( | ||
| ctx: ParsePayload<SerializedPonderIndexingMetrics>, | ||
| function invariant_indexingCompletedAndRealtimeAreNotBothTrue( |
There was a problem hiding this comment.
Validating Prometheus Metrics before constructing Unvalidated<PonderIndexingMetrics>.
| } | ||
| } | ||
|
|
||
| function invariant_includesRequiredMetrics(ctx: ParsePayload<PrometheusMetrics>) { |
There was a problem hiding this comment.
Validating Prometheus Metrics before constructing Unvalidated<PonderIndexingMetrics>.
| // Invariant: All values in the 'chain' label of required chain metrics must be valid ChainId strings. | ||
| for (const maybeChainId of metricLabels) { | ||
| const result = schemaChainIdString.safeParse(maybeChainId); | ||
|
|
||
| if (!result.success) { |
There was a problem hiding this comment.
Thanks to this invariant we can later safely call Number(chainIdString) while building Unvalidated<PonderIndexingMetrics> object.
| .pipe(schemaPonderIndexingMetrics); | ||
| .check(invariant_indexingCompletedAndRealtimeAreNotBothTrue); | ||
|
|
||
| function invariant_includesAtLeastOneIndexedChain(ctx: ParsePayload<PonderIndexingMetrics>) { |
There was a problem hiding this comment.
Validating Prometheus Metrics before constructing Unvalidated<PonderIndexingMetrics>.
| * @returns Unvalidated Chain Indexing Metrics | ||
| * to be validated by {@link schemaChainIndexingMetrics}. | ||
| */ | ||
| function buildUnvalidatedChainIndexingMetrics( |
There was a problem hiding this comment.
Logic in this function supports the current way ENSIndexer was determining a concrete Indexing Status type.
| const validation = schemaSerializedPonderIndexingMetrics | ||
| .transform(buildUnvalidatedPonderIndexingMetrics) | ||
| .pipe(schemaPonderIndexingMetrics) |
There was a problem hiding this comment.
During deserialization, we parse raw text with schemaSerializedPonderIndexingMetrics into validated PrometheusMetrics instance, and then use that instance to build Unvalidated<PonderIndexingStatus>, which in turn gets validated with schemaPonderIndexingMetrics.
|
@greptile review |
|
@greptile review |
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thank you 👍 Please take the lead to merge when ready
| * Latest synced block | ||
| * Total count of historical blocks. | ||
| * | ||
| * Each time a Ponder app restarts, if the historical blocks have |
There was a problem hiding this comment.
This comment makes it sound like historicalTotalBlocks keeps increasing across time for a Ponder app instance that hasn't restarted yet. My understanding is it never changes unless the Ponder app restarts?
There was a problem hiding this comment.
I'll refine the comment, your understanding is correct.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
invariant_includesRequiredMetrics validates that each required metric has some chain labels, but later buildUnvalidatedChainIndexingMetrics() assumes that for every chain present in ponder_sync_block there is also a corresponding sample in ponder_sync_block_timestamp (and ponder_historical_total_blocks for historical state). If any chain is missing in one of those metrics, getValue() returns undefined and the error will surface later as a less-actionable schema failure. Consider adding an invariant that the set of chain labels is consistent across required chain metrics (or at least that each chain in ponder_sync_block has non-undefined values for the other required metrics).
| } | |
| } | |
| // Invariant: The set of 'chain' labels must be consistent across required chain metrics. | |
| // In particular, every chain present in 'ponder_sync_block' must also be present in the | |
| // other required chain metrics, so that downstream code can safely assume per-chain values exist. | |
| const syncBlockChains = new Set( | |
| prometheusMetrics.getLabels("ponder_sync_block", "chain"), | |
| ); | |
| if (syncBlockChains.size > 0) { | |
| const otherChainMetricNames = requiredChainMetricNames.filter( | |
| (name) => name !== "ponder_sync_block", | |
| ); | |
| for (const metricName of otherChainMetricNames) { | |
| const metricChains = new Set( | |
| prometheusMetrics.getLabels(metricName, "chain"), | |
| ); | |
| for (const chain of syncBlockChains) { | |
| if (!metricChains.has(chain)) { | |
| ctx.issues.push({ | |
| code: "custom", | |
| input: ctx.value, | |
| message: `Metric '${metricName}' must include a 'chain' label for each chain present in 'ponder_sync_block'. Missing chain '${chain}'.`, | |
| }); | |
| } | |
| } | |
| } | |
| } |
| function invariant_indexingCompletedAndRealtimeAreNotBothTrue( | ||
| ctx: ParsePayload<PrometheusMetrics>, | ||
| ) { | ||
| const { chains } = ctx.value; | ||
| const prometheusMetrics = ctx.value; | ||
| const chainReferences = prometheusMetrics.getLabels("ponder_sync_block", "chain"); | ||
|
|
||
| if (chains.size === 0) { | ||
| ctx.issues.push({ | ||
| code: "custom", | ||
| input: ctx.value, | ||
| message: "Ponder Indexing Metrics must include at least one indexed chain.", | ||
| for (const maybeChainId of chainReferences) { | ||
| const ponderSyncIsComplete = prometheusMetrics.getValue("ponder_sync_is_complete", { | ||
| chain: maybeChainId, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Schema representing settings of a Ponder app. | ||
| */ | ||
| const schemaSerializedApplicationSettings = z.object({ | ||
| command: z.enum(PonderAppCommands), | ||
| ordering: z.enum(PonderIndexingOrderings), | ||
| }); | ||
| const ponderSyncIsRealtime = prometheusMetrics.getValue("ponder_sync_is_realtime", { | ||
| chain: maybeChainId, | ||
| }); | ||
|
|
||
| /** | ||
| * Schema describing the Ponder Indexing Metrics. | ||
| */ | ||
| const schemaPonderIndexingMetrics = z | ||
| .object({ | ||
| appSettings: schemaSerializedApplicationSettings, | ||
| chains: schemaSerializedChainsIndexingMetrics, | ||
| }) | ||
| .check(invariant_includesAtLeastOneIndexedChain); | ||
| if (ponderSyncIsComplete === 1 && ponderSyncIsRealtime === 1) { | ||
| ctx.issues.push({ | ||
| code: "custom", | ||
| input: ctx.value, | ||
| message: `'ponder_sync_is_complete' and 'ponder_sync_is_realtime' metrics cannot both be 1 at the same time for chain ${maybeChainId}`, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The function name invariant_indexingCompletedAndRealtimeAreNotBothTrue no longer matches what it validates (it now checks the Prometheus metrics 'ponder_sync_is_complete'/'ponder_sync_is_realtime', not the removed indexingCompleted/indexingRealtime fields). Renaming it would make the invariant’s intent clearer and avoid confusion for future refactors.
| export function deserializePonderIndexingMetrics(ponderMetricsText: string): PonderIndexingMetrics { | ||
| const validation = schemaSerializedPonderIndexingMetrics | ||
| .transform(buildUnvalidatedPonderIndexingMetrics) | ||
| .pipe(schemaPonderIndexingMetrics) | ||
| .safeParse(ponderMetricsText); |
There was a problem hiding this comment.
deserializePonderIndexingMetrics was narrowed to accept only string, even though the schema uses z.coerce.string() and other deserializers in this package accept unknown. If this is part of the public SDK API, consider keeping the parameter as unknown (or string | unknown) for consistency and to avoid an unnecessary breaking change; otherwise, consider dropping z.coerce and requiring a string end-to-end.
2 participants
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
ChainIndexingMetricstype have been collected as a discriminated union.ChainIndexingMetricsHistoricalincludeshistoricalTotalBlocksfield to support ENSIndexer use case where, at the ENSIndexer app start, all block refs must be fetched from RPC and cached for further use.ChainIndexingMetricsRealtimeincludeslatestSyncedBlockfieldChainIndexingMetricsCompletedincludesfinalIndexedBlockfieldPonderIndexingStatusnow includes thecheckpointBlockfield for each indexed chain.Unvalidated<T>type has been introduced to support creating objects shaped like business-layer data model, but not validated yet.Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)