refactor(ensapi): apply operation result pattern #1545
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
This PR applies the operation result pattern from ENSNode SDK to standardize response handling across ENSApi endpoints. The changes introduce new result types and builder functions, and refactor error handling to use a consistent pattern.
Changes:
- Introduced new result pattern infrastructure in the SDK with
buildResultOk,buildResultOkTimestamped, andbuildResultServiceUnavailablebuilders - Added
ServiceUnavailableresult code and updated result type definitions - Refactored ENSApi endpoints (
/amirealtime,/api/registrar-actions) to use the result pattern with newresultIntoHttpResponseutility - Removed legacy
errorResponsefunction in favor of standardized result handling
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/shared/result/result-common.ts | Added buildResultOk, buildResultOkTimestamped, buildResultServiceUnavailable builders and new result type definitions |
| packages/ensnode-sdk/src/shared/result/result-code.ts | Added ServiceUnavailable result code |
| packages/ensnode-sdk/src/shared/result/result-base.ts | Added AbstractResultOkTimestamped type with indexing cursor support |
| packages/ensnode-sdk/src/api/registrar-actions/serialize.ts | Added serializeNamedRegistrarActions helper function |
| packages/ensnode-sdk/src/api/registrar-actions/response.ts | Added RegistrarActionsResultOkData and RegistrarActionsResult types |
| apps/ensapi/src/middleware/registrar-actions.middleware.ts | Updated to use result pattern for error handling |
| apps/ensapi/src/lib/result/result-into-http-response.ts | New utility to convert operation results to HTTP responses |
| apps/ensapi/src/lib/result/result-into-http-response.test.ts | Comprehensive test coverage for result-to-HTTP conversion |
| apps/ensapi/src/lib/handlers/validate.ts | Updated validation error handling to use result pattern |
| apps/ensapi/src/lib/handlers/error-response.ts | Removed legacy error response handler |
| apps/ensapi/src/index.ts | Updated error handlers and notFound handler to use result pattern |
| apps/ensapi/src/handlers/registrar-actions-api.ts | Refactored to use result pattern for responses |
| apps/ensapi/src/handlers/amirealtime-api.ts | Refactored to use result pattern for responses |
| apps/ensapi/src/handlers/amirealtime-api.test.ts | Updated tests to verify new result pattern behavior |
Comments suppressed due to low confidence (1)
apps/ensapi/src/lib/handlers/validate.ts:14
- The documentation comment still references the old
errorResponsefunction which has been replaced with the result pattern. Update the comment to reflect that it now usesbuildResultInvalidRequestandresultIntoHttpResponsefor consistent error formatting.
/**
* Creates a Hono validation middleware with custom error formatting.
*
* Wraps the Hono validator with custom error handling that uses the
* errorResponse function for consistent error formatting across the API.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile OverviewGreptile SummaryThis PR successfully refactors ENSApi to adopt a standardized result pattern for HTTP responses, replacing ad-hoc error handling with a consistent data model across all endpoints. Key Changes
Architecture BenefitsThe result pattern provides several improvements:
All previous review comments about field name mismatches have been addressed - the codebase now consistently uses Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant HonoRouter
participant ValidationMiddleware
participant RegistrarActionsMiddleware
participant RouteHandler
participant resultIntoHttpResponse
participant ENSIndexer
Client->>HonoRouter: GET /api/registrar-actions/:parentNode
HonoRouter->>ValidationMiddleware: validate(param, schema)
alt validation fails
ValidationMiddleware->>ValidationMiddleware: buildResultInvalidRequest(errorMessage)
ValidationMiddleware->>resultIntoHttpResponse: resultIntoHttpResponse(c, result)
resultIntoHttpResponse->>resultIntoHttpResponse: resultCodeToHttpStatusCode(400)
resultIntoHttpResponse->>Client: Response 400 (InvalidRequest)
end
ValidationMiddleware->>RegistrarActionsMiddleware: check prerequisites
alt insufficient indexing progress
RegistrarActionsMiddleware->>RegistrarActionsMiddleware: buildResultInsufficientIndexingProgress(...)
RegistrarActionsMiddleware->>resultIntoHttpResponse: resultIntoHttpResponse(c, result)
resultIntoHttpResponse->>Client: Response 503 (InsufficientIndexingProgress)
else service unavailable
RegistrarActionsMiddleware->>RegistrarActionsMiddleware: buildResultServiceUnavailable(...)
RegistrarActionsMiddleware->>resultIntoHttpResponse: resultIntoHttpResponse(c, result)
resultIntoHttpResponse->>Client: Response 503 (ServiceUnavailable)
end
RegistrarActionsMiddleware->>RouteHandler: next()
RouteHandler->>ENSIndexer: findRegistrarActions(filters, orderBy, limit, offset)
ENSIndexer-->>RouteHandler: {registrarActions, totalRecords}
RouteHandler->>RouteHandler: buildPageContext(page, recordsPerPage, totalRecords)
RouteHandler->>RouteHandler: buildRegistrarActionsResultOk(data, minIndexingCursor)
RouteHandler->>RouteHandler: serializeRegistrarActionsResultOk(result)
RouteHandler->>resultIntoHttpResponse: resultIntoHttpResponse(c, serializedResult)
resultIntoHttpResponse->>resultIntoHttpResponse: resultCodeToHttpStatusCode(200)
resultIntoHttpResponse->>Client: Response 200 (Ok with data and minIndexingCursor)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tk-o
force-pushed
the
ensnode-result-type/registrar-actions-api
branch
from
745bf8b to
902d556
Compare
|
@greptile re-review |
tk-o
force-pushed
the
ensnode-result-type/registrar-actions-api
branch
from
e626f17 to
412e722
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ive HTTP endpoint docs
tk-o
left a comment
tk-o
left a comment
There was a problem hiding this comment.
Self-review completed
| const priceAmountSchemaSerializable = z.string(); | ||
| const makePriceAmountSchemaNative = (valueLabel: string = "Price Amount") => | ||
| z.preprocess( | ||
| (v) => (typeof v === "string" ? BigInt(v) : v), | ||
| z | ||
| .bigint({ | ||
| error: `${valueLabel} must represent a bigint.`, | ||
| }) | ||
| .nonnegative({ | ||
| error: `${valueLabel} must not be negative.`, | ||
| }), | ||
| ); | ||
|
|
||
| export const makePriceCurrencySchema = ( | ||
| export function makePriceAmountSchema<const SerializableType extends boolean>( | ||
| valueLabel: string, | ||
| serializable: SerializableType, | ||
| ): SerializableType extends true | ||
| ? typeof priceAmountSchemaSerializable | ||
| : ReturnType<typeof makePriceAmountSchemaNative>; | ||
| export function makePriceAmountSchema( | ||
| valueLabel: string = "Price Amount Schema", | ||
| serializable: true | false = false, | ||
| ): typeof priceAmountSchemaSerializable | ReturnType<typeof makePriceAmountSchemaNative> { | ||
| if (serializable) { | ||
| return priceAmountSchemaSerializable; | ||
| } else { | ||
| return makePriceAmountSchemaNative(valueLabel); | ||
| } | ||
| } |
There was a problem hiding this comment.
Applied a pattern of making Zod schema "serializable" for the purpose of generating OpenAPI spec. Without that conditional approach, the OpenAPI spec generation process would fail due to unsupported z.bigint() schema.
| makeRegistrarActionRegistrationSchema(`${valueLabel} Registration`, serializable), | ||
| makeRegistrarActionRenewalSchema(`${valueLabel} Renewal`, serializable), |
There was a problem hiding this comment.
Applied a pattern of making Zod schema "serializable" for the purpose of generating OpenAPI spec. All updates in this file follow this one.
| z.union([ | ||
| makeResponsePageContextSchemaWithNoRecords(valueLabel), | ||
| makeResponsePageContextSchemaWithRecords(valueLabel), | ||
| makeResponsePageContextSchemaWithNoRecords(valueLabel), |
There was a problem hiding this comment.
Changes in this file were required to make the response page context schema "serializable" for the purpose of generating OpenAPI spec.
|
|
||
| /** | ||
| * The start index of the records on the page (0-indexed) | ||
| */ | ||
| startIndex: undefined; | ||
|
|
||
| /** | ||
| * The end index of the records on the page (0-indexed) | ||
| */ | ||
| endIndex: undefined; |
There was a problem hiding this comment.
Changes in this file were required to make the response page context schema "serializable" for the purpose of generating OpenAPI spec.
| startIndex: undefined, | ||
| endIndex: undefined, |
There was a problem hiding this comment.
Changes in this file were required to make the response page context schema "serializable" for the purpose of generating OpenAPI spec.
|
|
||
| // fetching is temporarily not possible due to indexing status being not advanced enough | ||
| if (!hasIndexingStatusSupport(omnichainSnapshot.omnichainStatus)) { | ||
| if (!hasIndexingStatusSupport(configTypeId, omnichainSnapshot.omnichainStatus)) { |
There was a problem hiding this comment.
Checking indexing status support now requires providing the chains configuration type ID.
apps/ensadmin/src/components/registrar-actions/use-stateful-fetch-registrar-actions.ts
Show resolved
Hide resolved
| supportedIndexingStatusId: | ||
| | typeof OmnichainIndexingStatusIds.Completed | ||
| | typeof OmnichainIndexingStatusIds.Following; |
There was a problem hiding this comment.
There can be just one supported indexing status at a time.
There was a problem hiding this comment.
Suggest to define a new type in the indexing status file for the idea of an "indexing status end state" that would be a type union of completed and following.
We should document how these are "end states" because once indexing reaches one of these statuses, it will continue in that status forever.
Then you can reference that newly defined type union here and anywhere else where it is relevant.
apps/ensadmin/src/components/registrar-actions/display-registrar-actions-panel.tsx
Show resolved
Hide resolved
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
OpenAPI spec generator requires a Zod schema not to include any fields that cannot be serialized, for example, `z.bigint()`, and replace such fields with a serializable schema counterpart, for example, `z.string()`.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { | ||
| type CurrencyId, | ||
| CurrencyIds, | ||
| Price, | ||
| type PriceEth, | ||
| SerializedPrice, | ||
| type SerializedPriceEth, | ||
| } from "./currencies"; |
There was a problem hiding this comment.
Unused imports Price, SerializedPrice.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks. Reviewed and shared feedback.
| /** | ||
| * Successful result data for Health API requests. | ||
| */ | ||
| export type HealthResultOkData = string; |
There was a problem hiding this comment.
I'm thinking we want to guarantee that data is always an object? What do you think?
| }); | ||
|
|
||
| // log hono errors to console | ||
| app.onError((error, ctx) => { |
There was a problem hiding this comment.
Suggest we use a consistent name for the context variable across all Hono callbacks
| supportedIndexingStatusId: | ||
| | typeof OmnichainIndexingStatusIds.Completed | ||
| | typeof OmnichainIndexingStatusIds.Following; |
There was a problem hiding this comment.
Suggest to define a new type in the indexing status file for the idea of an "indexing status end state" that would be a type union of completed and following.
We should document how these are "end states" because once indexing reaches one of these statuses, it will continue in that status forever.
Then you can reference that newly defined type union here and anywhere else where it is relevant.
| one of the following: | ||
| The Registrar Actions API will be available once the omnichain indexing status becomes{" "} | ||
| <Badge variant="secondary"> | ||
| {formatOmnichainIndexingStatus(registrarActions.supportedIndexingStatusId)} |
There was a problem hiding this comment.
I'm worried that our client-code is interpreting this value as a strict enum, rather than as an arbitrary string.
It's super important we put a high priority on these distinctions.
| * @returns Corresponding HTTP status code | ||
| */ | ||
| export function resultCodeToHttpStatusCode(resultCode: ServerResultCode): ContentfulStatusCode { | ||
| switch (resultCode) { |
There was a problem hiding this comment.
For optimized maintainability and type-checking, suggest defining a Record that maps from ServerResultCode -> ContentfulStatusCode. Then this function can just:
return recordName[resultCode];
And it gets nice type safety, etc.
| const errorMessage = [ | ||
| `Registrar Actions API is not available.`, | ||
| `Indexing status is currently unavailable to this ENSApi instance.`, | ||
| ].join(" "); |
There was a problem hiding this comment.
| const errorMessage = [ | |
| `Registrar Actions API is not available.`, | |
| `Indexing status is currently unavailable to this ENSApi instance.`, | |
| ].join(" "); | |
| const errorMessage = [ | |
| `This API is temporarily unavailable for this ENSNode instance.`, | |
| `The indexing status has not been loaded by ENSApi yet.`, | |
| ].join(" "); |
| return resultIntoHttpResponse(c, result); | ||
| } | ||
|
|
||
| if (c.var.indexingStatus instanceof Error) { |
There was a problem hiding this comment.
I don't understand why we designed the indexing status middleware this way. It's a big pain for all consumers of the middleware to perform this check.
Is there a special reason why the indexing status middleware doesn't internally ensure that the indexing status will be available to downstream consumers, and if not, to return this error?
| const targetIndexingStatus = | ||
| registrarActionsPrerequisites.getSupportedIndexingStatus(configTypeId); | ||
|
|
||
| const errorMessage = [ |
There was a problem hiding this comment.
We're going to need to repeat this error logic in many places. We should define it in a more general way that's not specific to registrar actions. Please see my other related comments.
| description: | ||
| "Checks if the indexing progress is guaranteed to be within a requested worst-case distance of realtime", | ||
| responses: { | ||
| 200: { |
There was a problem hiding this comment.
Please see my other feedback about these massive objects.
| getSupportedIndexingStatus, | ||
| } = registrarActionsPrerequisites; | ||
|
|
||
| /** |
There was a problem hiding this comment.
A key goal of our work here is to fully remove the need for the whole "use stateful registrar actions" concept.
We should just have a "use registrar actions" that makes a single stateless API request. The client can then appropriately handle errors associated with insufficient indexing status or a config that won't support the API request.
Lite PR
Summary
resultIntoHttpResponsefunction, that produces a HTTPResponseobject based on the provided operation result.EnsNodeResponse<T>JSON response schema #1305, Streamline response codes across API modules #1355onError,notFound)GET /healthendpointHealthServerResultresult typeGET /amirealtimeendpointAmIRealtimeServerResultresult typeRegistrarActionsServerResultresult typeminIndexingCursor(as per MoveaccurateAsOffield into generic response data model / middleware #1512). We useAbstractResultOkTimestampedtype to achieve outcome. SeeRegistrarActionsResultOkresult type.RESULT_CODE_SERVER_ERROR_CODESwithResultCodes.ServiceUnavailableResultCodes.InsufficientIndexingProgressWhy
minIndexingCursorfield in their response data model to indicate to which point in time the response data is guaranteed to be up to.Testing
/amirealtimeand/api/registrar-actionsendpoints locally, simulating conditions where both, ok and error results were returned from ENSApi.Please find most important HTTP API test cases below.
HTTP API test cases
"Am I Realtime?" API
AmIRealtimeResultOk
Request
Response
{ "resultCode": "ok", "data": { "maxWorstCaseDistance": 22, "slowestChainIndexingCursor": 1769091731, "worstCaseDistance": 9 } }ResultInsufficientIndexingProgress
Request
Response
{ "resultCode": "insufficient-indexing-progress", "suggestRetry": true, "errorMessage": "Indexing Status 'worstCaseDistance' must be below or equal to the requested 'maxWorstCaseDistance'; worstCaseDistance = 14; maxWorstCaseDistance = 10", "data": { "indexingStatus": "omnichain-following", "slowestChainIndexingCursor": 1769091851, "earliestChainIndexingCursor": 1489165544, "progressSufficientFrom": { "indexingStatus": "omnichain-following", "chainIndexingCursor": 1769091855 } } }Registrar Actions API
RegistrarActionsResultOk
Request
Response
{ "resultCode": "ok", "data": { "registrarActions": [ { "action": { "id": "176909197100000000000000010000000024290909000000000000004950000000000000273", "type": "registration", "incrementalDuration": 20736000, "registrant": "0xeb087f37d87bf6d855717b056519c18e6761e68c", "registrationLifecycle": { "subregistry": { "subregistryId": { "chainId": 1, "address": "0x57f1887a8bf19b14fc0df6fd9b2acc9af147ea85" }, "node": "0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae" }, "node": "0xc2ab751f24da3537432be8a0b51086bdb2e55945d89bd3ff488d5ca2f450bf00", "expiresAt": 1789827971 }, "pricing": { "baseCost": { "currency": "ETH", "amount": "1104307319092065" }, "premium": { "currency": "ETH", "amount": "0" }, "total": { "currency": "ETH", "amount": "1104307319092065" } }, "referral": { "encodedReferrer": "0x0000000000000000000000007e491cde0fbf08e51f54c4fb6b9e24afbd18966d", "decodedReferrer": "0x7e491cde0fbf08e51f54c4fb6b9e24afbd18966d" }, "block": { "number": 24290909, "timestamp": 1769091971 }, "transactionHash": "0xc0a1f83ad441f430db99b87bd1d48d9eede5afa12e1723f8edc21850e86e2341", "eventIds": [ "176909197100000000000000010000000024290909000000000000004950000000000000273", "176909197100000000000000010000000024290909000000000000004950000000000000277" ] }, "name": "qualifiedinvestor.eth" }, { "action": { "id": "176909081900000000000000010000000024290813000000000000005050000000000000170", "type": "registration", "incrementalDuration": 31536000, "registrant": "0x718f6a369239c6963fd7807c62face162f8a31bf", "registrationLifecycle": { "subregistry": { "subregistryId": { "chainId": 1, "address": "0x57f1887a8bf19b14fc0df6fd9b2acc9af147ea85" }, "node": "0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae" }, "node": "0x5bbab16b388d2b32deb7d0fafefdd3997ba775dbd2c195b760d165c8bc5be8ff", "expiresAt": 1800626819 }, "pricing": { "baseCost": { "currency": "ETH", "amount": "1688390191103095" }, "premium": { "currency": "ETH", "amount": "0" }, "total": { "currency": "ETH", "amount": "1688390191103095" } }, "referral": { "encodedReferrer": "0x0000000000000000000000007e491cde0fbf08e51f54c4fb6b9e24afbd18966d", "decodedReferrer": "0x7e491cde0fbf08e51f54c4fb6b9e24afbd18966d" }, "block": { "number": 24290813, "timestamp": 1769090819 }, "transactionHash": "0xccd876afe2b4391cc85df58b0f784cf6404359c8ce170fd7608e23b971b058d4", "eventIds": [ "176909081900000000000000010000000024290813000000000000005050000000000000170", "176909081900000000000000010000000024290813000000000000005050000000000000173" ] }, "name": "techfund.eth" }, { "action": { "id": "176909075900000000000000010000000024290808000000000000006650000000000000217", "type": "renewal", "incrementalDuration": 5184000, "registrant": "0x8b24b1686832757e2f6d640e11e88e7f0064594a", "registrationLifecycle": { "subregistry": { "subregistryId": { "chainId": 1, "address": "0x57f1887a8bf19b14fc0df6fd9b2acc9af147ea85" }, "node": "0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae" }, "node": "0x45477e804caf0ed4048bdb5602eb9e4f85ac4c341745ab301ec5be0ca5e3aff5", "expiresAt": 1770530400 }, "pricing": { "baseCost": { "currency": "ETH", "amount": "8881394977846876" }, "premium": { "currency": "ETH", "amount": "0" }, "total": { "currency": "ETH", "amount": "8881394977846876" } }, "referral": { "encodedReferrer": "0x0000000000000000000000007e491cde0fbf08e51f54c4fb6b9e24afbd18966d", "decodedReferrer": "0x7e491cde0fbf08e51f54c4fb6b9e24afbd18966d" }, "block": { "number": 24290808, "timestamp": 1769090759 }, "transactionHash": "0xa050c6977805dccd03831ecd8ec9d002c2fafe15b662cf74d5d23783bf37aa9a", "eventIds": [ "176909075900000000000000010000000024290808000000000000006650000000000000217", "176909075900000000000000010000000024290808000000000000006650000000000000218", "176909075900000000000000010000000024290808000000000000006650000000000000219" ] }, "name": "5585.eth" } ], "pageContext": { "page": 1, "recordsPerPage": 3, "totalRecords": 19714, "totalPages": 6572, "hasNext": true, "hasPrev": false, "startIndex": 0, "endIndex": 2 } }, "minIndexingCursor": 1769092355 }ResultInsufficientIndexingProgress
Request
Response
{ "resultCode": "insufficient-indexing-progress", "suggestRetry": true, "errorMessage": "Registrar Actions API is not available. The cached omnichain indexing status of the connected ENSIndexer has insufficient progress.", "data": { "indexingStatus": "omnichain-backfill", "slowestChainIndexingCursor": 1702676652, "earliestChainIndexingCursor": 1686901632, "progressSufficientFrom": { "indexingStatus": "omnichain-following", "chainIndexingCursor": 1769092140 } } }ResultServiceUnavailable
Request
Response
{ "resultCode": "service-unavailable", "errorMessage": "Registrar Actions API is not available. Connected ENSIndexer configuration does not have all required plugins active. Current plugins: \"subgraph\". Required plugins: \"subgraph, basenames, lineanames, registrars\".", "suggestRetry": true }Notes for Reviewer (Optional)
z.bigint()schema, it needs to be replaced with thez.string()"serialized" couterpart. That's why we use<const SerializableType extends boolean>parameter type to keep using single Zod schema definition, but with conditional serialization.Pre-Review Checklist (Blocking)
Resulttype #1539 before PR 1539 is ready for review.Resulttype #1539PR Creation Tips
Related to #1305, #1355, #1368, #1512.