Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughThis PR integrates ENS record resolution into the Omnigraph GraphQL schema by introducing a new Changes
Sequence DiagramsequenceDiagram
actor Client
participant GQL as GraphQL Resolver<br/>(Domain.records)
participant DL as DataLoaders
participant ENS as ENS Resolution
Client->>GQL: Query Domain.records with selection
GQL->>DL: Load canonical path for domain
DL-->>GQL: Canonical path with domain IDs
GQL->>GQL: Map to interpreted labels
alt Canonical Path Exists
GQL->>ENS: resolveForward(name, selection)
ENS-->>GQL: Text & Address records
GQL-->>Client: ResolvedRecordsResponse
else Non-Canonical
GQL-->>Client: null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 adds a
Confidence Score: 4/5Safe to merge after addressing the encoded-label guard; the throw surfaces as a GraphQL error for a valid subset of canonical domains. One P1 defect: canonical domains with encoded labelhash labels trigger an unhandled throw from resolveForward instead of returning null, violating the documented API contract. The fix is a single-line isNormalizedName guard before the resolveForward call. apps/ensapi/src/omnigraph-api/schema/domain.ts — the records resolver needs an isNormalizedName guard before calling resolveForward. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL
participant domain.ts
participant resolution.ts
participant forward-resolution.ts
Client->>GraphQL: query { domain(by: {name}) { records(selection) { addresses texts } } }
GraphQL->>domain.ts: resolve Domain.records
domain.ts->>domain.ts: getDomainInterpretedName(domain, context)
domain.ts->>domain.ts: load v1/v2CanonicalPath via DataLoader
alt domain is not canonical
domain.ts-->>GraphQL: null
else domain has encoded label (e.g. [abc].eth)
domain.ts->>forward-resolution.ts: resolveForward(name, selection)
forward-resolution.ts-->>GraphQL: throws Error (not normalized)
else domain is canonical and normalized
domain.ts->>forward-resolution.ts: resolveForward(name, selection)
forward-resolution.ts-->>domain.ts: ResolverRecordsResponseBase
domain.ts->>resolution.ts: map to ResolvedRecords
resolution.ts-->>GraphQL: { addresses, texts, reverseName }
GraphQL-->>Client: records response
end
Reviews (1): Last reviewed commit: "add records resolution" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| resolve: async (domain, { selection }, context) => { | ||
| const name = await getDomainInterpretedName(domain, context); | ||
| if (!name) return null; | ||
|
|
||
| const { result } = await runWithTrace(() => | ||
| resolveForward( | ||
| name, | ||
| { | ||
| name: selection.reverseName ?? undefined, | ||
| texts: selection.texts ?? undefined, | ||
| addresses: selection.addresses ?? undefined, | ||
| }, | ||
| { accelerate: false, canAccelerate: false }, | ||
| ), | ||
| ); | ||
|
|
||
| return result as ResolverRecordsResponseBase; | ||
| }, |
There was a problem hiding this comment.
getDomainInterpretedName can return an InterpretedName that contains encoded labelhash segments (e.g. [abc123].eth). When that name is passed to resolveForward, it immediately throws "'[abc123].eth' must be normalized to be resolvable." (see forward-resolution.ts line 149). This error propagates as a GraphQL field error rather than returning null, violating the stated API contract: "Returns null if the domain is not canonical." Canonical domains with unknown labels would surface an unexpected 500-level error to the client instead of a clean null.
A simple guard before calling resolveForward would fix this:
import { isNormalizedName } from "enssdk";
resolve: async (domain, { selection }, context) => {
const name = await getDomainInterpretedName(domain, context);
if (!name) return null;
if (!isNormalizedName(name)) return null; // add this guard
const { result } = await runWithTrace(() =>
resolveForward(...)
);
...
}| required: false, | ||
| }), | ||
| texts: t.stringList({ | ||
| description: "Text record keys to resolve (e.g. `avatar`, `description`, `com.).", |
There was a problem hiding this comment.
The description for the texts field has an unclosed backtick before com., which will render incorrectly in GraphQL API documentation.
| description: "Text record keys to resolve (e.g. `avatar`, `description`, `com.).", | |
| description: "Text record keys to resolve (e.g. `avatar`, `description`, `com.`).", |
There was a problem hiding this comment.
oops, should be com.github
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/omnigraph-api/schema/domain.ts`:
- Around line 224-249: Domain.records currently lacks an accelerate arg and
always calls resolveForward with accelerate: false; add a GraphQL arg
"accelerate" to the Domain.records field (the field defined at records: t.field)
and forward its value along with context.canAccelerate into resolveForward
(i.e., call resolveForward(name, {...}, { accelerate: args.accelerate,
canAccelerate: context.canAccelerate })). Also wire up the acceleration
middleware chain to the omnigraph-api router by applying
makeIsRealtimeMiddleware, indexingStatusMiddleware and canAccelerateMiddleware
(same order used in resolution-api) and update the omnigraph-api context factory
to accept the initial context param that contains canAccelerate from the
middleware so context.canAccelerate is available in the resolver; keep
getDomainInterpretedName usage unchanged.
In `@apps/ensapi/src/omnigraph-api/schema/resolution.ts`:
- Around line 16-18: Update the description string for the texts field in
resolution.ts to fix the malformed example; locate the texts: t.stringList({ ...
}) declaration and replace the example snippet "Text record keys to resolve
(e.g. `avatar`, `description`, `com.)." with a correctly punctuated example such
as "Text record keys to resolve (e.g. `avatar`, `description`, `com`)." ensuring
matching backticks and proper parentheses/period placement.
🪄 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: 909769d3-67ea-4cf4-873d-e0d220e4bdd9
⛔ Files ignored due to path filters (1)
packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (4)
apps/ensapi/src/omnigraph-api/schema.tsapps/ensapi/src/omnigraph-api/schema/domain.integration.test.tsapps/ensapi/src/omnigraph-api/schema/domain.tsapps/ensapi/src/omnigraph-api/schema/resolution.ts
| records: t.field({ | ||
| description: | ||
| "Resolve ENS records for this Domain via the ENS protocol. Only canonical domains can be resolved. Returns null if the domain is not canonical.", | ||
| type: ResolvedRecordsRef, | ||
| nullable: true, | ||
| args: { | ||
| selection: t.arg({ | ||
| type: ResolveSelectionInput, | ||
| required: true, | ||
| description: "Which records to resolve.", | ||
| }), | ||
| }, | ||
| resolve: async (domain, { selection }, context) => { | ||
| const name = await getDomainInterpretedName(domain, context); | ||
| if (!name) return null; | ||
|
|
||
| const { result } = await runWithTrace(() => | ||
| resolveForward( | ||
| name, | ||
| { | ||
| name: selection.reverseName ?? undefined, | ||
| texts: selection.texts ?? undefined, | ||
| addresses: selection.addresses ?? undefined, | ||
| }, | ||
| { accelerate: false, canAccelerate: false }, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify context acceleration wiring and Domain.records schema args.
rg -nP -C3 '\bcanAccelerate\b|records:\s*t\.field|accelerate:\s*t\.arg' --iglob '*.ts'Repository: namehash/ensnode
Length of output: 17937
🏁 Script executed:
# Check omnigraph context configuration and middleware wiring
cd apps/ensapi
# Find omnigraph context type definition
rg -nP 'type.*Context|interface.*Context' --iglob '*omnigraph*.ts' -B2 -A5
# Check if omnigraph routes/schema uses canAccelerateMiddleware
rg -nP 'canAccelerateMiddleware|omnigraph-api' --iglob '*.ts' -B2 -A2Repository: namehash/ensnode
Length of output: 33798
🏁 Script executed:
# Check omnigraph context definition and yoga setup
cat -n apps/ensapi/src/omnigraph-api/context.ts
# Check omnigraph yoga handler
cat -n apps/ensapi/src/omnigraph-api/yoga.tsRepository: namehash/ensnode
Length of output: 3243
🏁 Script executed:
# Check omnigraph router mounting and middleware setup
rg -nP 'omnigraph|canAccelerateMiddleware' apps/ensapi/src/handlers/api/router.ts -B3 -A3
# Check app.ts for middleware chain
rg -nP 'app\.use|middleware|canAccelerateMiddleware' apps/ensapi/src/app.ts -B2 -A2Repository: namehash/ensnode
Length of output: 971
🏁 Script executed:
# Check omnigraph-api handler definition
cat -n apps/ensapi/src/handlers/api/omnigraph/omnigraph-api.tsRepository: namehash/ensnode
Length of output: 1068
🏁 Script executed:
# Check if there's any middleware applied globally before omnigraph routes
cat -n apps/ensapi/src/app.ts | head -50
# Check if omnigraph has any acceleration-related code
rg -nP 'accelerate|canAccelerate' apps/ensapi/src/omnigraph-api/ --iglob '*.ts'Repository: namehash/ensnode
Length of output: 1990
🏁 Script executed:
# Check how graphql-yoga context is supposed to work with Hono
# Look at yoga creation and how context parameter is passed
rg -nP 'createYoga|context' apps/ensapi/src/omnigraph-api/yoga.ts -B5 -A5
# Check if there are other GraphQL resolvers that access Hono context variables
rg -nP 'context\.' apps/ensapi/src/omnigraph-api/schema/ --iglob '*.ts' | head -20Repository: namehash/ensnode
Length of output: 1604
🏁 Script executed:
# Check graphql-yoga documentation or types for context parameter
# Look for how yoga.fetch uses the second parameter (c.var)
rg -nP 'fetch.*c\.var|context.*function' apps/ensapi/ --iglob '*.ts' -B2 -A2
# Check if there are examples of graphql-yoga receiving context from request
rg -nP 'createYoga.*context|yoga.*context' apps/ensapi/ --iglob '*.ts' -B5 -A5Repository: namehash/ensnode
Length of output: 2707
🏁 Script executed:
# Verify graphql-yoga context function signature and how it receives request context
# Check if context can be a function that receives request info
cat -n apps/ensapi/src/omnigraph-api/yoga.ts | grep -A20 "createYoga"
# Look at subgraph-api to see if it shows context wiring pattern
cat -n apps/ensapi/src/handlers/subgraph/subgraph-api.ts | grep -B5 -A10 "context"Repository: namehash/ensnode
Length of output: 1883
🏁 Script executed:
# Check graphql-yoga version and context handling
# Look at package.json for graphql-yoga version
cd apps/ensapi && cat package.json | grep -A2 -B2 "graphql-yoga"
# Look at how subgraph or other yoga instances handle context from serverContext
rg -nP 'context.*serverContext|createYoga.*context' apps/ensapi/ --iglob '*.ts' -B3 -A10Repository: namehash/ensnode
Length of output: 267
🏁 Script executed:
# Check graphql-yoga version and context patterns
grep -A2 -B2 "graphql-yoga" apps/ensapi/package.json
# Look for context function patterns with parameters in the codebase
rg -nP 'context\s*=.*\(\s*\w+\s*\)' apps/ensapi/src/omnigraph-api/ --iglob '*.ts' -B2 -A5Repository: namehash/ensnode
Length of output: 197
🌐 Web query:
graphql-yoga 5.16 context function serverContext Hono variables fetch
💡 Result:
In GraphQL Yoga 5.16 (released around early 2026 per changelog), the context function and serverContext work with Hono via the WHATWG Fetch API compatibility, as Hono exposes c.req.raw (a standard Request) and app.fetch. Default context includes: request (Fetch Request), params, query (DocumentNode), operationName, variables (query variables object), extensions. For Hono integration (no official docs, but community examples from 2023+ still valid for v5): import { Hono } from 'hono' import { createYoga, createSchema } from 'graphql-yoga' import { serve } from '@hono/node-server' const schema = createSchema({...}) const yoga = createYoga({ schema }) const app = new Hono app.all('/graphql*', async (c) => yoga.fetch(c.req.raw, { /* serverContext if needed */ })) serve({ fetch: app.fetch, port: 3000 }) Or app.use('/graphql', async (c) => yoga.handle({ request: c.req.raw }, {}) ) // older handle In resolvers, access context.variables (object from client query), context.request (for headers/body), context.fetch (server fetch utility). To add Hono-specific serverContext (e.g., Hono c object), pass as second arg to handle/fetch: yoga.fetch(request, { honoC: c }) Then in user context factory: context: async (initialContext) => { const { honoC } = initialContext; return { honoVars: honoC.env?.VAR || c.req.param('id') } } For Cloudflare Workers/Deno (Hono common): serverContext auto-includes FetchEvent with request, waitUntil, etc. No explicit serverContext needed usually. Examples access context.request.headers.get('auth'), context.variables.userId in resolvers. Use context.fetch(url) for internal server calls.
Citations:
- 1: https://the-guild.dev/graphql/yoga-server/docs/features/context
- 2: https://the-guild.dev/graphql/yoga-server/changelogs/graphql-yoga
- 3: https://lorefnon.me/2023/12/17/integrating-hono-and-graphql-yoga/
- 4: https://gist.github.com/xorus/1e63571cfb6040c876c303f8ccdd0b66
🏁 Script executed:
# Check if canAccelerateMiddleware could be applied to omnigraph
# Look at isRealtimeMiddleware to see if it's applied to omnigraph
rg -nP 'isRealtimeMiddleware|makeIsRealtimeMiddleware' apps/ensapi/src/ --iglob '*.ts' -B2 -A2
# Check omnigraph-api router to see all middlewares applied
cat -n apps/ensapi/src/handlers/api/omnigraph/omnigraph-api.tsRepository: namehash/ensnode
Length of output: 5546
🏁 Script executed:
# Final verification: check if context function in yoga can be updated to receive serverContext
# Look at graphql-yoga documentation patterns in the code
rg -nP 'context.*async|context.*=>|serverContext' apps/ensapi/src/omnigraph-api/ --iglob '*.ts' -B3 -A3Repository: namehash/ensnode
Length of output: 10109
🏁 Script executed:
# Check if omnigraph is expected to support acceleration or if it's REST-only
rg -nP 'omnigraph.*accelerat|accelerat.*omnigraph' apps/ensapi/src/ --iglob '*.ts'
# Check if there are any tests or documentation about Domain.records with accelerate
rg -nP 'Domain.*records|records.*accelerate' apps/ensapi/src/omnigraph-api/ --iglob '*.ts' -B2 -A2Repository: namehash/ensnode
Length of output: 836
Expose and honor the accelerate argument.
Domain.records currently has no accelerate GraphQL arg and always passes { accelerate: false, canAccelerate: false }, so callers cannot use the default accelerated path required by the API contract.
To implement this, wire the acceleration middleware to the Omnigraph API router and update the context factory to receive canAccelerate from the middleware:
- Apply
canAccelerateMiddlewareand its prerequisitemakeIsRealtimeMiddleware+indexingStatusMiddlewareto the omnigraph-api router (similar to resolution-api) - Update
omnigraph-api/context.tsto accept an initial context parameter containingcanAcceleratefrom the middleware - Add the
accelerateGraphQL arg toDomain.recordsand pass bothaccelerateandcontext.canAcceleratetoresolveForward()
Proposed fix
records: t.field({
description:
"Resolve ENS records for this Domain via the ENS protocol. Only canonical domains can be resolved. Returns null if the domain is not canonical.",
type: ResolvedRecordsRef,
nullable: true,
args: {
+ accelerate: t.arg.boolean({
+ required: false,
+ defaultValue: true,
+ description: "Whether to use accelerated resolution when available.",
+ }),
selection: t.arg({
type: ResolveSelectionInput,
required: true,
description: "Which records to resolve.",
}),
},
- resolve: async (domain, { selection }, context) => {
+ resolve: async (domain, { accelerate, selection }, context) => {
const name = await getDomainInterpretedName(domain, context);
if (!name) return null;
const { result } = await runWithTrace(() =>
resolveForward(
name,
{
texts: selection.texts ?? undefined,
addresses: selection.addresses ?? undefined,
},
- { accelerate: false, canAccelerate: false },
+ { accelerate: accelerate ?? true, canAccelerate: context.canAccelerate ?? false },
),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/omnigraph-api/schema/domain.ts` around lines 224 - 249,
Domain.records currently lacks an accelerate arg and always calls resolveForward
with accelerate: false; add a GraphQL arg "accelerate" to the Domain.records
field (the field defined at records: t.field) and forward its value along with
context.canAccelerate into resolveForward (i.e., call resolveForward(name,
{...}, { accelerate: args.accelerate, canAccelerate: context.canAccelerate })).
Also wire up the acceleration middleware chain to the omnigraph-api router by
applying makeIsRealtimeMiddleware, indexingStatusMiddleware and
canAccelerateMiddleware (same order used in resolution-api) and update the
omnigraph-api context factory to accept the initial context param that contains
canAccelerate from the middleware so context.canAccelerate is available in the
resolver; keep getDomainInterpretedName usage unchanged.
shrugs
left a comment
shrugs
left a comment
There was a problem hiding this comment.
So i mentioned this in the original issue, which is that i'm not sure we want to allow people to query the trace and other meta information, like whether it was accelerated or not via the graphql api because i'm not convinced the ergonomics of it makes sense. but if you find a format that feels good, we can totally do so. For example, we could make the signature
query GetRecords {
domain(by: { name: "eth" }) {
resolve([accelerate: True]) {
records { # equivalent to a ResolverRecordsSelection
name
texts(keys: ["description"]) { key value }
addresses(coinType: [60]) { coinType address }
}
acceleration { requested attempted }
trace
}
}
}
query GetPrimaryNames {
account(by: { address: "0xabcd" }) {
resolve(accelerate: False) {
primaryNames(coinTypes: [60]) { coinType name }
acceleration { requested attempted }
trace
}
}
}then that feels good; let's do that! then the pattern is clear; Domain.resolve.records and Account.resolve.primaryNames — feels good to me!
Note that within enskit, we need to update the cache client to mark these fields as EMBEDDED_DATA because they don't have a global key. Also, feel free to update the example app to render these records. So maybe add a new component that fetches a name's records by Domain.id or (name from the params, whatever) and then plug it into the name page.
Overall, this is definitely the correct direction and the data loader usage is correct. Let's move that helper into its own library.
to access the pothos context type, let's
- define a
type Context = ReturnType<typeof createContext>inapps/ensapi/src/omnigraph-api/context.ts - use that everywhere we currently use
context: ReturnType<typeof createContext> - make sure the helper you extracted here uses said type as well.
no need to alias it to graphqlContext; within the omnigraph graphql server module, "context" is always the graphql context
| type DomainRecordsResult = { | ||
| domain: { | ||
| records: { | ||
| addresses: Array<{ coinType: number; address: string | null }>; |
There was a problem hiding this comment.
nit: you can use the enssdk#CoinType type here
| const DomainRecords = gql` | ||
| query DomainRecords($name: InterpretedName!, $addresses: [CoinType!], $texts: [String!]) { | ||
| domain(by: { name: $name }) { | ||
| records(selection: { addresses: $addresses, texts: $texts }) { |
There was a problem hiding this comment.
in the future i want to move the addresses/texts selections into the fields themselves (which will require some gql query introspection, which we do elsewhere) but good call to not do that immediately
| texts: [], | ||
| }); | ||
|
|
||
| expect(result.domain.records?.addresses).toEqual([{ coinType: 60, address: DEVNET_OWNER }]); |
There was a problem hiding this comment.
nit:
await expect().resolves.toMatchObject({
domain: { records: { texts: [], addresses: [{coinType: 60, address: DEVNET_OWNER }] } }
})There was a problem hiding this comment.
ditto below, consolidate into a single object match and use the await expect(...).resolves syntax. we should add that preference to our AGENT.md.
There was a problem hiding this comment.
Actually, remove this case in favor of the below case which tests both at the same time.
| /////////////////// | ||
| records: t.field({ | ||
| description: | ||
| "Resolve ENS records for this Domain via the ENS protocol. Only canonical domains can be resolved. Returns null if the domain is not canonical.", |
There was a problem hiding this comment.
additional: only canonical, normalized names can be resolved; so names with unknown labels can't be resolved actually (which is crazy to me but that's what the contracts say)
| // ResolvedRecords | ||
| //////////////////// | ||
| export const ResolvedRecordsRef = builder | ||
| .objectRef<{ |
There was a problem hiding this comment.
the types used here can likely be inferred (or already exist) within the resolution module likely using ResolverRecordsResponse or ResolverRecordsResponseBase, make sure to use that type instead of re-defining
2 participants
Summary
Domain.recordsfield to the Omnigraph GraphQL APIgetDomainInterpretedNameas a shared local helper in domain.ts to avoid duplicating canonical path resolution logicWhy
Closes #1802
Testing
Pre-Review Checklist (Blocking)
[x] This PR does not introduce significant changes and is low-risk to review quickly.
[ ] Relevant changesets are included (or are not required)