Changed accurateAsOf in Registrar Actions from optional to required#1907
Changed accurateAsOf in Registrar Actions from optional to required#1907
accurateAsOf in Registrar Actions from optional to required#1907Conversation
🦋 Changeset detectedLatest commit: c17c8c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMakes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR promotes Confidence Score: 5/5Safe to merge — all changes are consistent, well-tested, and the breaking-change implications are documented and intentional. No P0 or P1 issues found. The type/schema change is correct, the previously-missing No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/ensapi/src/handlers/api/explore/registrar-actions-api.ts | Adds accurateAsOf (from indexingStatus.snapshot.slowestChainIndexingCursor) to the unfiltered handler, mirroring the already-correct by-parent-node handler; no logic issues. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/response.ts | Removes the ? optional modifier and associated backward-compat comment from accurateAsOf in RegistrarActionsResponseOk; straightforward and correct. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.ts | Removes .optional() from the accurateAsOf Zod field, keeping the schema aligned with the updated TypeScript type. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.test.ts | Adds a test asserting that a response missing accurateAsOf is rejected; adequate coverage for the new requirement. |
| .changeset/shy-rabbits-drum.md | Correctly bumps both @ensnode/ensnode-sdk and ensapi as minor for the breaking schema change. |
Sequence Diagram
sequenceDiagram
participant Client
participant indexingStatusMiddleware
participant registrarActionsApiMiddleware
participant getRegistrarActionsHandler
Client->>indexingStatusMiddleware: GET /api/registrar-actions
indexingStatusMiddleware->>indexingStatusMiddleware: set c.var.indexingStatus (RealtimeIndexingStatusProjection | Error)
indexingStatusMiddleware->>registrarActionsApiMiddleware: next()
registrarActionsApiMiddleware->>registrarActionsApiMiddleware: check indexingStatus instanceof Error → 503
registrarActionsApiMiddleware->>registrarActionsApiMiddleware: check config/indexing support → 503
registrarActionsApiMiddleware->>getRegistrarActionsHandler: next()
getRegistrarActionsHandler->>getRegistrarActionsHandler: read indexingStatus.snapshot.slowestChainIndexingCursor (accurateAsOf)
getRegistrarActionsHandler->>getRegistrarActionsHandler: fetchRegistrarActions(undefined, query)
getRegistrarActionsHandler-->>Client: 200 { responseCode, registrarActions, pageContext, accurateAsOf }
Reviews (1): Last reviewed commit: "accurateasof made required" | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
Makes accurateAsOf mandatory again in the Registrar Actions API contract and restores it on the unfiltered GET /api/registrar-actions endpoint so clients can reliably use indexing “accurate as of” metadata.
Changes:
- Require
accurateAsOfinRegistrarActionsResponseOk(TypeScript type + Zod schema). - Add a schema test ensuring responses missing
accurateAsOfare rejected. - Restore
accurateAsOfin the unfiltered registrar-actions handler response.
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/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.ts | Makes accurateAsOf required in the response OK schema. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.test.ts | Adds a regression test rejecting OK responses without accurateAsOf. |
| packages/ensnode-sdk/src/ensapi/api/registrar-actions/response.ts | Makes accurateAsOf required in the public response type and removes the temporary-optional note. |
| apps/ensapi/src/handlers/api/explore/registrar-actions-api.ts | Restores returning accurateAsOf for the unfiltered endpoint using indexing status snapshot data. |
| .changeset/shy-rabbits-drum.md | Declares release notes + version bumps for the contract change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/handlers/api/explore/registrar-actions-api.ts`:
- Around line 104-106: Replace the throw in the indexing-status check with the
same serialized 503 response used elsewhere: when c.var.indexingStatus
instanceof Error, call serializeRegistrarActionsResponse(...) with responseCode
set to 503 and the appropriate body (preserving existing shape) instead of
throwing; update the handler that calls serializeRegistrarActionsResponse (the
registrar-actions-api request/handler) so this branch returns that serialized
result directly, preserving the prior API contract and avoiding a caught throw
that yields a generic 500.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4c961c2-d1a2-46a7-afd5-33a48a9e31dc
📒 Files selected for processing (5)
.changeset/shy-rabbits-drum.mdapps/ensapi/src/handlers/api/explore/registrar-actions-api.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/response.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.test.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
packages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.test.ts
Outdated
Show resolved
Hide resolved
| */ | ||
| app.openapi(getRegistrarActionsRoute, async (c) => { | ||
| try { | ||
| if (c.var.indexingStatus instanceof Error) { |
There was a problem hiding this comment.
Handler throws a generic Error when indexingStatus is unavailable, causing a 500 response inside try-catch instead of returning correct 503 response
There was a problem hiding this comment.
The registrar-actions handlers' if (c.var.indexingStatus instanceof Error) throw … branches are unreachable defensive code for TypeScript narrowing - registrarActionsApiMiddleware already short-circuits that case with a serialized 503 before the handler runs.
There was a problem hiding this comment.
@Goader Thanks. Agreed with your conclusion here but it will be good to add a comment in the code here how this case is unreachable based on the registrarActionsApiMiddleware (which would return a HTTP 503 error) but we add this logic here just as a formality for type narrowing.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@Goader Thanks for this. Reviewed and shared feedback. Please take the lead to merge when ready 👍
| */ | ||
| app.openapi(getRegistrarActionsRoute, async (c) => { | ||
| try { | ||
| if (c.var.indexingStatus instanceof Error) { |
There was a problem hiding this comment.
@Goader Thanks. Agreed with your conclusion here but it will be good to add a comment in the code here how this case is unreachable based on the registrarActionsApiMiddleware (which would return a HTTP 503 error) but we add this logic here just as a formality for type narrowing.
| const { registrarActions, pageContext } = await fetchRegistrarActions(undefined, query); | ||
|
|
||
| // Get the accurateAsOf timestamp from the slowest chain indexing cursor | ||
| const accurateAsOf = c.var.indexingStatus.snapshot.slowestChainIndexingCursor; |
There was a problem hiding this comment.
Let's not use this field for this idea. Instead, let's set accurateAsOf to be equal to the omnichainIndexingCursor field which you should be able to access as as a field somewhere in the indexingStatus object.
| if (c.var.indexingStatus instanceof Error) { | ||
| throw new Error("Indexing status has not been loaded yet"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggest we move the operation that builds accurateAsOf here, before the call to fetchRegistrarActions. We want to error on the side of accurateAsOf being older, rather than newer than the data being returned.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Registrar Actions:
accurateAsOfcloses: #1497
Summary
accurateAsOfis now required inRegistrarActionsResponseOk(type + zod schema).accurateAsOfon theGET /api/registrar-actionshandler.Why
accurateAsOfto the Registrar Actions API #1484 introduced the field as optional to avoid breaking ENS Awards snapshot NPM packages. That workaround can now be removed.Testing
Notes for Reviewer (Optional)
GET /api/registrar-actionsendpoint stopped returningaccurateAsOfafter refactor: use zod-openapi for registrar-actions-api #1673 split the original single/:parentNode?handler into two, keeping the logic only in the by-parent-node one. This PR restores it, but the older versions of ENSApi will not return it, and therefore newer client will throw.Pre-Review Checklist (Blocking)