chore: small fixes for ensapi and openapi doc generation#1872
chore: small fixes for ensapi and openapi doc generation#1872
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
🦋 Changeset detectedLatest commit: d17133e The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (4)
📝 WalkthroughWalkthroughAdded OpenAPI examples and richer response/content schemas; introduced serialized registrar-action schemas and examples; renamed a request selection param ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Greptile SummaryThis PR makes a focused set of quality-of-life improvements to the ENSApi and its OpenAPI documentation: it fixes a bug where the Key changes:
Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/documentation suggestions that do not block correctness. The bug fix (app.onError), the parameter rename, and the serialized-schema additions are all logically correct and well-tested. The three P2 findings (typo in error message, changeset bump level, and OpenAPI boolean-vs-string annotation) are non-blocking quality items. No logic bugs or data-integrity issues were found. packages/ensdb-sdk/src/client/zod-schemas/ensdb-config.ts (typo) and apps/ensapi/src/app.ts (error message exposure) deserve a quick look before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[HTTP Request] --> B{Route Handler}
B -->|success| C[serializeRegistrarActionsResponse]
C --> D[JSON Response 200]
B -->|ZodError / SchemaError thrown| E[app.onError]
B -->|Other error thrown| E
E --> F[errorResponse ctx error]
F -->|instanceof ZodError| G[400 Invalid Input + details]
F -->|instanceof SchemaError| G
F -->|instanceof Error| H[500 error.message]
F -->|unknown| I[500 Internal Server Error]
subgraph Deserialization [Client SDK: deserializeRegistrarActionsResponse]
J[Wire JSON] --> K[makeSerializedRegistrarActionsResponseSchema validates string amounts]
K -->|error| L[throw Error]
K -->|ok| M[makeRegistrarActionsResponseSchema transforms strings to BigInt]
M -->|error| N[throw Error]
M -->|ok| O[RegistrarActionsResponse domain type]
end
Reviews (1): Last reviewed commit: "add env names" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const rawSelectionParams = z.object({ | ||
| name: z.string().optional(), | ||
| addresses: z.string().optional(), | ||
| texts: z.string().optional(), | ||
| nameRecord: z | ||
| .string() | ||
| .optional() | ||
| .openapi({ | ||
| type: "boolean", | ||
| description: "Whether to include the ENS name record in the response.", | ||
| }), | ||
| addresses: z |
There was a problem hiding this comment.
type: "boolean" on a z.string() field
rawSelectionParams.nameRecord is declared as z.string() but then annotated with .openapi({ type: "boolean" }). This tells OpenAPI code generators and consumers that the parameter is a JSON boolean, yet over HTTP it is still a plain query-string value ("true" / "false"). Some clients may therefore attempt to serialise it as a literal true (no quotes), which is invalid for a query param.
A more accurate OpenAPI annotation would be:
| const rawSelectionParams = z.object({ | |
| name: z.string().optional(), | |
| addresses: z.string().optional(), | |
| texts: z.string().optional(), | |
| nameRecord: z | |
| .string() | |
| .optional() | |
| .openapi({ | |
| type: "boolean", | |
| description: "Whether to include the ENS name record in the response.", | |
| }), | |
| addresses: z | |
| nameRecord: z | |
| .string() | |
| .optional() | |
| .openapi({ | |
| type: "string", | |
| enum: ["true", "false"], | |
| description: "Whether to include the ENS name record in the response.", | |
| }), |
Alternatively, simply reuse boolstring (which already carries the correct .openapi({ type: "boolean" }) hint that @hono/zod-openapi understands from a string-with-coercion context) to stay consistent with the addresses / texts siblings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.routes.ts`:
- Around line 103-113: The 200 response blocks currently use
makeSerializedRegistrarActionsResponseSchema(...) which produces both success
and error variants; replace those 200 blocks with the OK-only serialized schema
(the project’s "registrar actions OK" schema factory / serializer) and keep the
error shape only on the 500 responses; ensure the example
(registrarActionsResponseOkExample) is still attached to the OK-only schema so
the 200 response describes only the success payload and the 500 response
continues to reference the error schema.
In `@apps/ensapi/src/lib/handlers/params.schema.ts`:
- Around line 72-78: The schema change removed the public query key "name"
causing existing ?name=true callers to be dropped; restore backward
compatibility by accepting "name" as an alias to "nameRecord": add an optional
schema entry for the "name" query (same shape/docs as nameRecord) and update the
request-mapping logic that currently reads/sets nameRecord so it also checks for
and maps the legacy "name" key into nameRecord before validation/stripping
(referencing the nameRecord schema entry and the mapping code that assigns
nameRecord).
In `@packages/ensdb-sdk/src/client/zod-schemas/ensdb-config.ts`:
- Around line 4-7: The error message on the EnsDbUrlSchema z.string validator
contains a typo ("variavle"); update the error text for EnsDbUrlSchema to use
the correct spelling "variable" and ensure the message matches the wording used
elsewhere (e.g., other ENV error messages) so it's consistent across schemas.
In `@packages/ensnode-sdk/src/registrars/zod-schemas.ts`:
- Around line 227-245: Duplicate field definitions for serialized registrar
actions should be extracted into a single shared shape builder to avoid schema
drift: create a helper (e.g., makeSerializedRegistrarActionBaseShape or
makeBaseActionShape) that returns the common z.object shape including id
(EventIdSchema), incrementalDuration, registrant (makeLowercaseAddressSchema),
registrationLifecycle, referral (makeRegistrarActionReferralSchema), block
(makeBlockRefSchema), transactionHash (makeTransactionHashSchema), and eventIds
(EventIdsSchema) and apply invariant_eventIdsInitialElementIsTheActionId to that
shape; then update the existing makeBaseSerializedRegistrarActionSchema and the
other duplicated schema to compose that base shape and only add the differing
pricing field (using makeSerializedRegistrarActionPricingSchema) so all common
invariants are defined once.
- Around line 103-117: The serialized registrar action pricing schema
(makeSerializedRegistrarActionPricingSchema) currently accepts objects where
baseCost, premium, and total are individually valid but not checked for
consistency; update makeSerializedRegistrarActionPricingSchema to add the same
validation parity as the domain pricing schema by adding a
refinement/superRefine that, for the non-null object variant produced from
makeSerializedPriceEthSchema, computes baseCost + premium and asserts it equals
total (and still allows the null-object variant), so inconsistent serialized
payloads are rejected early.
🪄 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: fdd3c9b5-dea4-4106-9a75-97cdf1b0350e
📒 Files selected for processing (26)
.changeset/nice-foxes-cheat.md.changeset/smooth-foxes-boil.mdapps/ensapi/src/app.tsapps/ensapi/src/handlers/api/explore/name-tokens-api.routes.tsapps/ensapi/src/handlers/api/explore/registrar-actions-api.routes.tsapps/ensapi/src/handlers/api/explore/registrar-actions-api.tsapps/ensapi/src/handlers/api/meta/realtime-api.routes.tsapps/ensapi/src/handlers/api/meta/status-api.routes.tsapps/ensapi/src/handlers/api/resolution/resolution-api.routes.tsapps/ensapi/src/lib/handlers/params.schema.test.tsapps/ensapi/src/lib/handlers/params.schema.tsdocs/docs.ensnode.io/ensapi-openapi.jsonpackages/ensdb-sdk/src/client/zod-schemas/ensdb-config.tspackages/ensnode-sdk/src/ensapi/api/name-tokens/examples.tspackages/ensnode-sdk/src/ensapi/api/name-tokens/zod-schemas.test.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/deserialize.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/examples.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/serialize.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.test.tspackages/ensnode-sdk/src/ensapi/api/registrar-actions/zod-schemas.tspackages/ensnode-sdk/src/ensapi/api/resolution/examples.tspackages/ensnode-sdk/src/ensapi/api/resolution/zod-schemas.test.tspackages/ensnode-sdk/src/ensapi/api/shared/pagination/response.tspackages/ensnode-sdk/src/ensapi/api/shared/pagination/zod-schemas.tspackages/ensnode-sdk/src/internal.tspackages/ensnode-sdk/src/registrars/zod-schemas.ts
💤 Files with no reviewable changes (3)
- apps/ensapi/src/handlers/api/meta/status-api.routes.ts
- packages/ensnode-sdk/src/ensapi/api/shared/pagination/zod-schemas.ts
- apps/ensapi/src/handlers/api/meta/realtime-api.routes.ts
| const makeSerializedRegistrarActionPricingSchema = ( | ||
| valueLabel: string = "Serialized Registrar Action Pricing", | ||
| ) => | ||
| z.union([ | ||
| z.object({ | ||
| baseCost: makeSerializedPriceEthSchema(`${valueLabel} Base Cost`), | ||
| premium: makeSerializedPriceEthSchema(`${valueLabel} Premium`), | ||
| total: makeSerializedPriceEthSchema(`${valueLabel} Total`), | ||
| }), | ||
| z.object({ | ||
| baseCost: z.null(), | ||
| premium: z.null(), | ||
| total: z.null(), | ||
| }), | ||
| ]); |
There was a problem hiding this comment.
Add serialized pricing total validation parity.
Line [107]-Line [111] validate shape, but unlike the domain pricing schema (Line [77]), this path does not enforce total = baseCost + premium. That lets inconsistent serialized payloads pass this schema and fail later.
Suggested fix
+function invariant_serializedRegistrarActionPricingTotalIsSumOfBaseCostAndPremium(
+ ctx: ParsePayload<{
+ baseCost: { amount: string; currency: "ETH" };
+ premium: { amount: string; currency: "ETH" };
+ total: { amount: string; currency: "ETH" };
+ }>,
+) {
+ const sum = (BigInt(ctx.value.baseCost.amount) + BigInt(ctx.value.premium.amount)).toString();
+ if (ctx.value.total.amount !== sum) {
+ ctx.issues.push({
+ code: "custom",
+ input: ctx.value,
+ message: `'total' must be equal to the sum of 'baseCost' and 'premium'`,
+ });
+ }
+}
+
const makeSerializedRegistrarActionPricingSchema = (
valueLabel: string = "Serialized Registrar Action Pricing",
) =>
z.union([
z.object({
baseCost: makeSerializedPriceEthSchema(`${valueLabel} Base Cost`),
premium: makeSerializedPriceEthSchema(`${valueLabel} Premium`),
total: makeSerializedPriceEthSchema(`${valueLabel} Total`),
- }),
+ }).check(invariant_serializedRegistrarActionPricingTotalIsSumOfBaseCostAndPremium),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensnode-sdk/src/registrars/zod-schemas.ts` around lines 103 - 117,
The serialized registrar action pricing schema
(makeSerializedRegistrarActionPricingSchema) currently accepts objects where
baseCost, premium, and total are individually valid but not checked for
consistency; update makeSerializedRegistrarActionPricingSchema to add the same
validation parity as the domain pricing schema by adding a
refinement/superRefine that, for the non-null object variant produced from
makeSerializedPriceEthSchema, computes baseCost + premium and asserts it equals
total (and still allows the null-object variant), so inconsistent serialized
payloads are rejected early.
| const makeBaseSerializedRegistrarActionSchema = ( | ||
| valueLabel: string = "Serialized Base Registrar Action", | ||
| ) => | ||
| z | ||
| .object({ | ||
| id: EventIdSchema, | ||
| incrementalDuration: makeDurationSchema(`${valueLabel} Incremental Duration`), | ||
| registrant: makeLowercaseAddressSchema(`${valueLabel} Registrant`), | ||
| registrationLifecycle: makeRegistrationLifecycleSchema( | ||
| `${valueLabel} Registration Lifecycle`, | ||
| ), | ||
| pricing: makeSerializedRegistrarActionPricingSchema(`${valueLabel} Pricing`), | ||
| referral: makeRegistrarActionReferralSchema(`${valueLabel} Referral`), | ||
| block: makeBlockRefSchema(`${valueLabel} Block`), | ||
| transactionHash: makeTransactionHashSchema(`${valueLabel} Transaction Hash`), | ||
| eventIds: EventIdsSchema, | ||
| }) | ||
| .check(invariant_eventIdsInitialElementIsTheActionId); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract shared base action shape to prevent schema drift.
Line [191]-Line [206] and Line [231]-Line [243] duplicate the same fields with only pricing differing. Please factor a shared shape builder to keep invariants and field edits in one place.
As per coding guidelines: “Do not duplicate definitions across multiple locations; use type aliases to document invariants, with each invariant documented exactly once on its type alias.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensnode-sdk/src/registrars/zod-schemas.ts` around lines 227 - 245,
Duplicate field definitions for serialized registrar actions should be extracted
into a single shared shape builder to avoid schema drift: create a helper (e.g.,
makeSerializedRegistrarActionBaseShape or makeBaseActionShape) that returns the
common z.object shape including id (EventIdSchema), incrementalDuration,
registrant (makeLowercaseAddressSchema), registrationLifecycle, referral
(makeRegistrarActionReferralSchema), block (makeBlockRefSchema), transactionHash
(makeTransactionHashSchema), and eventIds (EventIdsSchema) and apply
invariant_eventIdsInitialElementIsTheActionId to that shape; then update the
existing makeBaseSerializedRegistrarActionSchema and the other duplicated schema
to compose that base shape and only add the differing pricing field (using
makeSerializedRegistrarActionPricingSchema) so all common invariants are defined
once.
| transactionHash: makeTransactionHashSchema(`${valueLabel} Transaction Hash`), | ||
| eventIds: EventIdsSchema, | ||
| }) | ||
| .check(invariant_eventIdsInitialElementIsTheActionId); |
There was a problem hiding this comment.
not sure if I need to add this check here... I think it will be triggered during makeBaseRegistrarActionSchema, no? i'm still trying to understand how it all works
There was a problem hiding this comment.
@sevenzing Yes it hurts my head too. Suggest to check with @tk-o for his advice as I think he's the most familiar with this existing logic in our code.
There was a problem hiding this comment.
In my mind, defining "serialized" schema has the goal of checking individual fields to ensure we can transform the serialized data model into the business-level data model.
On the other hand, the goal for the "business-level" schema definition is converting the "serialized" data model into the busines-level data model. This includes invariants for various relationships exisitng among fields in the business- data model.
Having that said, there's no need for calling check() method on the "serialized" data model.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/ensapi/src/lib/handlers/params.schema.ts (1)
70-100:⚠️ Potential issue | 🟠 MajorKeep accepting the legacy
namequery key at runtime.This is a runtime breaking change, not just an OpenAPI rename: because the outer query schema now only whitelists
nameRecord, legacy?name=trueis stripped beforeparams.selection.parse(...)runs. Existing callers can therefore lose the name record silently or getSelection cannot be empty.when that was their only selector. Keepnameas a deprecated runtime alias, or remap it tonameRecordbefore validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/lib/handlers/params.schema.ts` around lines 70 - 100, The selection schema currently only accepts nameRecord at runtime so legacy ?name=true is stripped; update the runtime handling to accept the deprecated "name" key and map it to the new field: add name: z.optional(boolstring) to the selection z.object and in the .transform for selection use the combined check (value.name ?? value.nameRecord) when building the ResolverRecordsSelection (i.e., set name: true if either value.name or value.nameRecord is truthy) so existing callers keep working while still supporting nameRecord.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/fair-ghosts-poke.md:
- Line 5: Update the changeset summary to improve grammar, capitalization, and
specificity: change the line to start with a capital letter, use present-tense
"Fixes" (e.g., "Fixes error handling in app.onError to return correct HTTP
status codes and resolves OpenAPI schema generation issues"), and include the
specific components affected (app.onError and OpenAPI schema generation) so the
changeset description is clear and actionable.
In `@docs/docs.ensnode.io/ensapi-openapi.json`:
- Around line 2423-2454: Add concrete example payloads for the 400 and 500
responses so the generated docs show error cases: update the OpenAPI response
objects that define the 400 and 500 schemas (the response entries with keys
"400" and "500") to include an "example" (or an "examples") value under
content.application/json (or directly under the schema) showing a realistic
payload — e.g. for 400 include { "message": "Invalid query", "details": { ... }
} and for 500 include { "responseCode": "error", "error": { "message": "Internal
server error", "details": { ... } } } — ensure the example fields align with the
existing schema properties ("message", "details", "responseCode", "error") so
regeneration will embed these examples in the docs.
---
Duplicate comments:
In `@apps/ensapi/src/lib/handlers/params.schema.ts`:
- Around line 70-100: The selection schema currently only accepts nameRecord at
runtime so legacy ?name=true is stripped; update the runtime handling to accept
the deprecated "name" key and map it to the new field: add name:
z.optional(boolstring) to the selection z.object and in the .transform for
selection use the combined check (value.name ?? value.nameRecord) when building
the ResolverRecordsSelection (i.e., set name: true if either value.name or
value.nameRecord is truthy) so existing callers keep working while still
supporting nameRecord.
🪄 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: 280cfc9e-b312-43c8-9b25-1f939cd1d4aa
📒 Files selected for processing (5)
.changeset/fair-ghosts-poke.mdapps/ensapi/src/lib/handlers/params.schema.tsapps/ensindexer/src/config/config.test.tsdocs/docs.ensnode.io/ensapi-openapi.jsonpackages/ensdb-sdk/src/client/zod-schemas/ensdb-config.ts
| "400": { | ||
| "description": "Invalid query", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { "message": { "type": "string" }, "details": {} }, | ||
| "required": ["message"] | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal server error", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "responseCode": { "type": "string", "enum": ["error"] }, | ||
| "error": { | ||
| "type": "object", | ||
| "properties": { "message": { "type": "string" }, "details": {} }, | ||
| "required": ["message"] | ||
| } | ||
| }, | ||
| "required": ["responseCode", "error"], | ||
| "additionalProperties": false | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add concrete 400/500 examples for the registrar-actions error responses.
These changed error media types now have schemas, but they still omit concrete example payloads, so the generated docs only show a fully rendered success case. Please add error examples at the route/schema source so regeneration emits them here.
Also applies to: 2969-3000
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docs.ensnode.io/ensapi-openapi.json` around lines 2423 - 2454, Add
concrete example payloads for the 400 and 500 responses so the generated docs
show error cases: update the OpenAPI response objects that define the 400 and
500 schemas (the response entries with keys "400" and "500") to include an
"example" (or an "examples") value under content.application/json (or directly
under the schema) showing a realistic payload — e.g. for 400 include {
"message": "Invalid query", "details": { ... } } and for 500 include {
"responseCode": "error", "error": { "message": "Internal server error",
"details": { ... } } } — ensure the example fields align with the existing
schema properties ("message", "details", "responseCode", "error") so
regeneration will embed these examples in the docs.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensapi/src/handlers/api/explore/registrar-actions-api.routes.ts
Outdated
Show resolved
Hide resolved
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@sevenzing Hey good to see the improvements here. Reviewed and shared feedback 👍
| @@ -75,7 +75,7 @@ app.get("/health", async (c) => { | |||
| // log hono errors to console | |||
| app.onError((error, ctx) => { | |||
There was a problem hiding this comment.
For this fallback catch-all error handler, what do you think about adding a prefix to the log messages / `errorResponse that would clearly identify for us that the error went through this fallback catch-all error handler.
Goal: Help us with debugging.
For example, maybe a little prefix such as "Unexpected server error: ..."?
Appreciate your advice.
| if (!url.startsWith("postgresql://") && !url.startsWith("postgres://")) { | ||
| export const EnsDbUrlSchema = z | ||
| .string({ | ||
| error: "ENV variable ENSDB_URL is required.", |
There was a problem hiding this comment.
There's a few updates in this file where we're making specific reference to the ENSDB_URL env variable or the ENSINDEXER_SCHEMA_NAME env variable.
It's really good that the error messages apps like ENSIndexer or ENSApi output in the case these env variables are incorrectly formatted make specific reference back to these env variable names. I really like that! It will help people who run these apps to debug incorrect env variable configurations.
However, we need to separate this logic into different layers of responsibility.
Please note how this code is living in the ensdb-sdk package. It should therefore not be tightly bound to the responsibilities that live within a particular app (ex: environment variable names or reading directly from environment variables). App-level responsibilities should live at the app-level, not at the package-level.
Therefore, suggest something like the following division of responsibilities:
- In the
ensdb-sdkpackage the logic here can output error information about what's invalid about an EnsDbUrl, or an EnsIndexerSchemaName, but it shouldn't make any hardcoded references to environment variable names. - We can then define a wrapper utility function that would extend the above parsing logic specifically for the context that it is related to an environment variable named X.
- It's fine to have this utility function defined in
ensdb-sdkso long as it (a) doesn't read directly from env vars itself; and (b) takes as input the env var name rather than hardcoding it.
- It's fine to have this utility function defined in
Appreciate your advice 👍
| schema: makeSerializedRegistrarActionsResponseSchema( | ||
| "Registrar Actions Response", | ||
| ).openapi({ | ||
| example: registrarActionsResponseOkExample, |
There was a problem hiding this comment.
It would be awesome if you could make it so that we could produce multiple examples in the OpenAPI spec. For example, both an Ok and and Error example.
It's important that we document examples for error cases too.
For some of our APIs, it would also be valuable if we could document various edge cases for success responses too.
What do you think? Please feel welcome to create a separate follow-up issue and PR for this goal.
|
|
||
| const makeSerializedPriceEthSchema = (valueLabel: string = "Serialized Price ETH") => | ||
| z.object({ | ||
| amount: z.string({ error: `${valueLabel} amount must be a string.` }).regex(/^\d+$/, { |
There was a problem hiding this comment.
We should have some other existing Zod schemas you could build upon for non-negative integers so that no regex is needed here.
Goal: Build upon the primitives we've already been creating.
| amount: z.string({ error: `${valueLabel} amount must be a string.` }).regex(/^\d+$/, { | ||
| error: `${valueLabel} amount must be a non-negative integer string.`, | ||
| }), | ||
| currency: z.literal("ETH", { |
There was a problem hiding this comment.
Hmm. I thought we would already have a Zod schema defined for this? Can you please review our existing Zod schemas such as:
makePriceSchemamakePriceEthSchemadeserializePriceEth
Perhaps there's existing code to reuse or refactor?
| */ | ||
| export const resolveRecordsResponseExample = { | ||
| records: { | ||
| name: "vitalik.eth", |
There was a problem hiding this comment.
Suggest we remove the name resolver record from being used in any of our forward-reslution examples. It will be confusing for 99% of devs and therefore create more problems than benefit to highlight in our example docs.
| }, | ||
| }, | ||
| accelerationRequested: false, | ||
| accelerationAttempted: false, |
There was a problem hiding this comment.
This terminology of accelerationAttempted is confusing. Can you please create a follow up issue and PR to update it?
The problem is that "attempted" is ambiguous. Ok.. maybe it was "attempted" but was it actually "completed"?
I assume that this field actually represents if acceleration was completed or not?
Assuming so, suggest to rename this field from accelerationAttempted to just accelerated as that is the more concise way of communicating the idea of "acceleration completed".
There was a problem hiding this comment.
at the moment: accelerationRequested is === the ?accelerate param, and accelerationAttempted is related to whether ensnode has attempted acceleration (for example, it may be lagging realtime, or the protocol-acceleration plugin may not be enabled)
so if we rename the ?accelerate, perhaps it should be to requestAcceleration? OR, since we're defaulting to enabled, perhaps we should have a ?noAccelerate param.
then perhaps we want to rename these fields to
accelerationEnabled and accelerationAttempted? in the future we want to provide an didAccelerate or isAccelerated boolean, but deriving that isn't trivial at the moment
| "ensapi": patch | ||
| --- | ||
|
|
||
| use example requests for openapi doc |
There was a problem hiding this comment.
| use example requests for openapi doc | |
| add example responses to autogenerated openapi doc |
These are example responses, right?
.changeset/fair-ghosts-poke.md
Outdated
| "ensapi": patch | ||
| --- | ||
|
|
||
| fix handling error on latest server step and some minor bug fixes |
There was a problem hiding this comment.
Please update this changeset to make it more clear. I'm struggling to understand it.
| "description": "ENS name to resolve (e.g. 'vitalik.eth'). Must be normalized per ENSIP-15." | ||
| }, | ||
| "required": true, | ||
| "description": "ENS name to resolve (e.g. 'vitalik.eth'). Must be normalized per ENSIP-15.", |
There was a problem hiding this comment.
I note that the description is being duplicated both here and in the schema a few lines above.
Maybe this is totally fine and the right way to do things? I'm not sure. If it is, please ignore and mark this as resolved. Just flagging in case there's something for us to optimize here. Appreciate your advice. Thanks.
| "@ensnode/ensnode-sdk": patch | ||
| --- | ||
|
|
||
| add examples for type system |
There was a problem hiding this comment.
as a reader i'm not sure what this is referring to
| const accelerate = z | ||
| .optional(boolstring) | ||
| .default(false) | ||
| .describe("Attempt accelerated CCIP-Read resolution using L1 data.") |
There was a problem hiding this comment.
yeah maybe just Attempt Protocol Acceleration using indexed data
| default: false, | ||
| }); | ||
| const address = makeLowercaseAddressSchema().describe( | ||
| "EVM wallet address (e.g. '0xd8da6bf26964af9d7eed9e03e53415d37aa96045').", |
There was a problem hiding this comment.
i believe we accept checksummed addresses and the lowercase schema normalizes them
| .transform((value, ctx) => { | ||
| const selection: ResolverRecordsSelection = { | ||
| ...(value.name && { name: true }), | ||
| ...(value.nameRecord && { name: true }), |
There was a problem hiding this comment.
imo we should leave the protocol acceleration lib as-is — so the current change is acceptable — the handler accepts nameRecord (or whatever we end up calling it) and passes the configuration option to the protocol acceleration lib as name: true. since there's no confusion within that module.
finally, if we're going to rename name to nameRecord i might propose we just rename all of the fields for ultimate clarity like resolveNameRecord, resolveTextRecords and resolveAddressRecords
| app.onError((error, ctx) => { | ||
| logger.error(error); | ||
| return errorResponse(ctx, "Internal Server Error"); | ||
| return errorResponse(ctx, error); |
There was a problem hiding this comment.
this is the wrong approach: this would catch any uncaught or unexpected errors and leak internal error messages to the public.
instead, the handlers should catch errors and return an error response, and this function should never be executed. by definition if this fn is executed, there's some unexpected internal server error, and that's what we should return
| }, | ||
| }, | ||
| accelerationRequested: false, | ||
| accelerationAttempted: false, |
There was a problem hiding this comment.
at the moment: accelerationRequested is === the ?accelerate param, and accelerationAttempted is related to whether ensnode has attempted acceleration (for example, it may be lagging realtime, or the protocol-acceleration plugin may not be enabled)
so if we rename the ?accelerate, perhaps it should be to requestAcceleration? OR, since we're defaulting to enabled, perhaps we should have a ?noAccelerate param.
then perhaps we want to rename these fields to
accelerationEnabled and accelerationAttempted? in the future we want to provide an didAccelerate or isAccelerated boolean, but deriving that isn't trivial at the moment
4 participants
Summary
app.onErrorto pass the actual error object instead of a hardcoded string, such thathttps://api.alpha.ensnode.io/api/resolve/records/vitalik.ethwont result in 500! (it will result in 400 but we will fix it soon too 👍 )name→nameRecordquery param in record selection of/api/resolve/records/to avoid collision with the name path paramexamples.tsfiles forname-tokens,registrar-actions, andresolution— typed example constants used in both routes and tests!resolution/registrar-actionsquery and path params; fixbeginTimestampandendTimestampmissing type:"integer"in OpenAPI output was missing for some reason, idk why...makeSerialized...Schema— wire-format schemas with string pricing amounts (separate from the domain bigint schemas) for use in OpenAPI route definitionsz.undefined()frommakeResponsePageContextSchemaWithNoRecords—ZodUndefinedhas no JSON Schema representation and caused UnknownZodTypeError during OpenAPI generationTesting
tested it with
Pre-Review Checklist (Blocking)