refactor(ensapi): apply operation result pattern#1545
refactor(ensapi): apply operation result pattern#1545tk-o wants to merge 40 commits intofeat/ensnode-result-typefrom
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 implements a standardized result pattern across ENSApi, replacing ad-hoc error handling with a type-safe, structured approach. The implementation is well-architected with clear separation of concerns. Key Changes
Implementation QualityThe code demonstrates strong engineering practices:
The changes align well with the stated goals of standardizing responses and preparing for "Explore APIs" requirements. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Hono as Hono Router
participant Middleware as indexingStatusMiddleware
participant Handler as API Handler (v1)
participant Validator as validateApiHandlerPrerequisites
participant Builder as Result Builder
participant Converter as resultIntoHttpResponse
Client->>Hono: GET /api/v1/registrar-actions/:parentNode
Hono->>Middleware: Execute middleware
Middleware->>Middleware: Fetch & cache indexing status
Middleware->>Hono: Set c.var.indexingStatus
Hono->>Handler: Route to handler
Handler->>Validator: validateApiHandlerPrerequisites(indexingStatus, requiredPlugins)
alt Prerequisites not met
Validator->>Validator: Check indexing status exists
Validator->>Validator: Check required plugins active
Validator->>Validator: Check indexing progress sufficient
Validator-->>Handler: Return error result (ServiceUnavailable/InsufficientIndexingProgress)
Handler->>Converter: resultIntoHttpResponse(c, errorResult)
Converter->>Converter: Map result code to HTTP status (503)
Converter-->>Client: Return HTTP 503 with error JSON
else Prerequisites met
Validator-->>Handler: Return ResultOk with indexingStatus
Handler->>Handler: Fetch registrar actions data
Handler->>Builder: buildResultOkRegistrarActions(data, minIndexingCursor)
Builder-->>Handler: Return ResultOkRegistrarActions
Handler->>Handler: serializeResultOkRegistrarActions(result)
Handler->>Converter: resultIntoHttpResponse(c, serializedResult)
Converter->>Converter: Map result code to HTTP status (200)
Converter-->>Client: Return HTTP 200 with success JSON
end
|
![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
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()`.
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.
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?
There was a problem hiding this comment.
I think it's not necessary, what value would it bring vs keeping the data structure as flat as possible?
There was a problem hiding this comment.
@tk-o It's nice if our result data model makes this guarantee for all possible results.
There was a problem hiding this comment.
@tk-o For example: Such a guarantee may be helpful for completely generic tooling related to results.
There was a problem hiding this comment.
If you want for data to be guaranteed to be object, we need the following update:
export interface AbstractResultOk<TDataType extends object> extends AbstractResult<typeof ResultCodes.Ok> {
/**
* The data of the result.
*/
data: TDataType;
}@lightwalker-eth how does that sound?
apps/ensapi/src/index.ts
Outdated
| }); | ||
|
|
||
| // 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.
There was a problem hiding this comment.
Can you please expand on that note?
Looking at formatOmnichainIndexingStatus function definition:
export function formatOmnichainIndexingStatus(status: OmnichainIndexingStatusId): string {
const [, formattedStatus] = status.split("-");
return formattedStatus;
}Did you mean its input parameter type should be string, like so?
export function formatOmnichainIndexingStatus(status: string): string {
const [, formattedStatus] = status.split("-");
return formattedStatus;
}There was a problem hiding this comment.
@tk-o Your question here is about UI-level concerns. However the main idea I'm trying to flag here is about data-model level concerns in relation to change management.
Here's a principle that should influence all our work: avoid breaking changes to our APIs (as soon as we can achieve the first "stable" API release).
Therefore, we must work hard now to design the first "stable" API release so that we can avoid breaking changes as we continue iterating in the future!
For any enum, we must thoughtfully consider future change management:
- Are we ready to commit to NEVER introducing a new value to this enum without it being a breaking change requiring a new incremented
/vX/...in the API route? - Or do we want the flexibility to introduce a new value to this enum without it being a breaking change?
Use of option 1 should be VERY selective and deliberate. It should never be an accidental oversight.
Use of option 2 requires that we explicitly prepare clients for gracefully handling new enum values that they won't recognize because they come from a future server version.
Do you have any questions on the above?
What do you recommend for this OmnichainIndexingStatusId enum? Should it use option 1 or option 2? Why is that your recommendation?
There was a problem hiding this comment.
@lightwalker-eth Yes, it's likely some enums will have to be extended to cover new cases. This may lead to situation where client-side code does not recognize a newly added enum variant included in server response.
I would go ahead with option 1.
As I proposed during our last team call, I think the client should be able to pick which version of the API response schema (i.e. v1, v2, etc.) it expects. This would leave no room for unnoticed breaking changes. The change management would rely on a migration process. This is the option 1 you mentioned above.
For example:
- The client app has been using the
v1data model provided byENSNodeClientV1instance. - Now the client app needs to use the
v2data model.- The client app has to migrate from using
ENSNodeClientV1instance to theENSNodeClientV2instance.
- The client app has to migrate from using
As for option 2, if we wanted to use it just for enums, we would need to make the client app to handle the "unrecognized" enum variants. In other words, the client app would have to know which variants of the given enum are "recognized", so all other variants could be considered "unrecognized".
If we went down this way, we would have to wrap all enum values in some data wrapper type to allow expressing a generic "unrecognized" variant, so the client app could handle it accordingly.
| * @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.
| @@ -28,10 +46,96 @@ app.get( | |||
| 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.
There was a problem hiding this comment.
I agree on the goal, but this PR is not aiming to update the client code. It will be done in a follow-up PR to reduce the PR scope.
Require Result OK to always include an object in `data` field. Also, require all Result errors to include `data` object with mandatory `errorMessage` and `suggestRetry` fields.
Refine naming convention for Result type variants.
This change supports sharing logic that determines if API handler is supported by the current Indexing Status
This will improve readability on the API handlers level
Use `IndexingStatusForSupportedApiResult` object to replace middleware approach and simplify route handler code.
Use newly added `data` property for accessing `errorMessage`
Flatten the structure, refine field names
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
packages/ensnode-sdk/src/shared/result/zod-schemas.ts:1
- The schema name
makeResultErrorInsufficientIndexingProgressDataSchemais inconsistent with the actual typeResultInsufficientIndexingProgressData. The schema includes "Error" but the type does not. Consider renaming tomakeResultInsufficientIndexingProgressDataSchemato match the type name exactly.
packages/ensnode-sdk/src/api/shared/pagination/response.ts:1 - The removal of
startIndexandendIndexfields fromResponsePageContextWithNoRecordsis not documented or explained. These fields were previously defined asundefinedwhen there are no records. Consider adding a comment explaining why these fields are no longer needed for the no-records case, or ensuring this change is intentional and doesn't break existing consumers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use v0 as implicit version for clients, while allowing them to opt-in to using v1.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Extract shared components between v0 and v1
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Make sure Hono is not mixing up handlers between different API versions.
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 56 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensadmin/src/components/registrar-actions/display-registrar-actions-panel.tsx
Show resolved
Hide resolved
tk-o
left a comment
tk-o
left a comment
There was a problem hiding this comment.
Self-review completed
Lite PR
Summary
Registrar Actions API becomes multi-version
/api/registrar-actionsis implicitly versioned asv0/api/v1/registrar-actionsis explicitly versioned asv1v1tag before the API path. This avoids issues where Hono could mix up handlers between API versions.Registrar Actions API V1
validateApiHandlerPrerequisitesfunction call which allows to get indexing status object if all prerequisites were met. Otherwise, thevalidateApiHandlerPrerequisitesfunction will return result error, with appropriate result code.Result type integrated in ENSApi
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 type updates in ENSNode SDK
ResultCodeincludedatafield, which is guaranteed to be anobject. Thedatafield is optional forResultCodes.Loading.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": { "requestedMaxWorstCaseDistance": 22, "slowestChainIndexingCursor": 1769600807, "worstCaseDistance": 12, "serverNow": 1769600819 } }ResultInsufficientIndexingProgress
Request
Response
{ "resultCode": "insufficient-indexing-progress", "data": { "currentIndexingStatus": "omnichain-following", "currentIndexingCursor": 1769600831, "startIndexingCursor": 1489165544, "targetIndexingStatus": "omnichain-following", "targetIndexingCursor": 1769600839, "errorMessage": "Indexing Status 'worstCaseDistance' must be below or equal to the requested 'requestedMaxWorstCaseDistance'; worstCaseDistance = 18; requestedMaxWorstCaseDistance = 10", "suggestRetry": true } }Registrar Actions API
RegistrarActionsResultOk
Request
Response
{ "resultCode": "ok", "data": { "registrarActions": [ { "action": { "id": "176959988300000000000000010000000024333055000000000000003850000000000000184", "type": "registration", "incrementalDuration": 94608000, "registrant": "0xbad9895fa47616022e648ddd846119f4c9661ed8", "registrationLifecycle": { "subregistry": { "subregistryId": { "chainId": 1, "address": "0x57f1887a8bf19b14fc0df6fd9b2acc9af147ea85" }, "node": "0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae" }, "node": "0x381c59a32d3e741a484f93d4e46f8c759a91455de19fa56cb8815c22df58a957", "expiresAt": 1864207883 }, "pricing": { "baseCost": { "currency": "ETH", "amount": "4971302790087598" }, "premium": { "currency": "ETH", "amount": "0" }, "total": { "currency": "ETH", "amount": "4971302790087598" } }, "referral": { "encodedReferrer": "0x000000000000000000000000f919a96d2970380b87917b04f02e6d3d08368b10", "decodedReferrer": "0xf919a96d2970380b87917b04f02e6d3d08368b10" }, "block": { "number": 24333055, "timestamp": 1769599883 }, "transactionHash": "0x14b724b0c90af0dcb2a87c3be527d4396c9ab507b20063852e02982fda047266", "eventIds": [ "176959988300000000000000010000000024333055000000000000003850000000000000184", "176959988300000000000000010000000024333055000000000000003850000000000000188" ] }, "name": "ainrg.eth" }, { "action": { "id": "176959900700000000000000010000000024332983000000000000010350000000000000298", "type": "renewal", "incrementalDuration": 29113321, "registrant": "0x8bca9f3016a33b1e34135d5f536e46ed541f1a28", "registrationLifecycle": { "subregistry": { "subregistryId": { "chainId": 1, "address": "0x57f1887a8bf19b14fc0df6fd9b2acc9af147ea85" }, "node": "0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae" }, "node": "0xb3fb24ad82ecdfc478ccc8aa82a228f6b73adaa084743cf54ca3f219083e535b", "expiresAt": 1924856160 }, "pricing": { "baseCost": { "currency": "ETH", "amount": "1529798050017079" }, "premium": { "currency": "ETH", "amount": "0" }, "total": { "currency": "ETH", "amount": "1529798050017079" } }, "referral": { "encodedReferrer": "0x0000000000000000000000007e491cde0fbf08e51f54c4fb6b9e24afbd18966d", "decodedReferrer": "0x7e491cde0fbf08e51f54c4fb6b9e24afbd18966d" }, "block": { "number": 24332983, "timestamp": 1769599007 }, "transactionHash": "0x03aae4b10035a9f832a83cc3528489a0dbe738b5e97eb7bb6310a53484943ef0", "eventIds": [ "176959900700000000000000010000000024332983000000000000010350000000000000298", "176959900700000000000000010000000024332983000000000000010350000000000000299", "176959900700000000000000010000000024332983000000000000010350000000000000300" ] }, "name": "vendor.eth" }, { "action": { "id": "176959883900000000000000010000000024332969000000000000003250000000000000181", "type": "registration", "incrementalDuration": 126144000, "registrant": "0x8bca9f3016a33b1e34135d5f536e46ed541f1a28", "registrationLifecycle": { "subregistry": { "subregistryId": { "chainId": 1, "address": "0x57f1887a8bf19b14fc0df6fd9b2acc9af147ea85" }, "node": "0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae" }, "node": "0xb3fb24ad82ecdfc478ccc8aa82a228f6b73adaa084743cf54ca3f219083e535b", "expiresAt": 1924856160 }, "pricing": { "baseCost": { "currency": "ETH", "amount": "6628403720116798" }, "premium": { "currency": "ETH", "amount": "378408936502576335" }, "total": { "currency": "ETH", "amount": "385037340222693133" } }, "referral": { "encodedReferrer": "0x0000000000000000000000007e491cde0fbf08e51f54c4fb6b9e24afbd18966d", "decodedReferrer": "0x7e491cde0fbf08e51f54c4fb6b9e24afbd18966d" }, "block": { "number": 24332969, "timestamp": 1769598839 }, "transactionHash": "0xfadfa5dd2602bc5a9a0f6717956406dc0e847f508ef08b88b4ee69675968d8ba", "eventIds": [ "176959883900000000000000010000000024332969000000000000003250000000000000181", "176959883900000000000000010000000024332969000000000000003250000000000000185" ] }, "name": "vendor.eth" } ], "pageContext": { "page": 1, "recordsPerPage": 3, "totalRecords": 23470, "totalPages": 7824, "hasNext": true, "hasPrev": false, "startIndex": 0, "endIndex": 2 } }, "minIndexingCursor": 1769600915 }ResultInsufficientIndexingProgress
Request
Response
{ "resultCode": "insufficient-indexing-progress", "data": { "currentIndexingStatus": "omnichain-backfill", "currentIndexingCursor": 1702676652, "startIndexingCursor": 1686901632, "targetIndexingStatus": "omnichain-following", "targetIndexingCursor": 1769092140, "suggestRetry": true, "errorMessage": "This API is temporarily unavailable for this ENSNode instance. The cached omnichain indexing status of the connected ENSIndexer has insufficient progress." } } }ResultServiceUnavailable
Request
Response
{ "resultCode": "service-unavailable", "data": { "errorMessage": "This API is unavailable for this ENSNode instance. The connected ENSIndexer did not activate all the plugins this API requires. Active plugins: \"subgraph, basenames, lineanames\". Required plugins: \"subgraph, basenames, lineanames, registrars\".", "suggestRetry": false } }Notes for Reviewer (Optional)
z.bigint()schema, it needs to be replaced with thez.string()"serialized" couterpart. That's why we use "serialized" copies of Zod response schemas.Pre-Review Checklist (Blocking)
Resulttype #1539 before PR 1539 is ready for review.Resulttype #1539PR Creation Tips
Related to #1305, #1355, #1368, #1512.