draft: suggestion to add Virtual Domain#2269
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 📝 WalkthroughWalkthroughThis PR extends ENS domain resolution to support names that have on-chain resolvers but remain unindexed by introducing virtual domain representations, on-chain resolver verification, and query orchestration to serve these unindexed-but-valid domains alongside indexed ones. ChangesVirtual Domains for Unindexed ENS Names
Sequence DiagramsequenceDiagram
participant Client
participant QueryResolver as Query.domain Resolver
participant DomainIndex as Domain Index
participant UniversalResolver
participant VirtualDomain as Virtual Domain Factory
Client->>QueryResolver: domain(by: { name })
QueryResolver->>DomainIndex: getDomainIdByInterpretedName(name)
alt Domain indexed
DomainIndex-->>QueryResolver: domainId
QueryResolver-->>Client: indexed Domain
else Domain not indexed
DomainIndex-->>QueryResolver: null
QueryResolver->>UniversalResolver: findResolver(packetToBytes(name))
alt Resolver exists on-chain
UniversalResolver-->>QueryResolver: resolverAddress
QueryResolver->>VirtualDomain: makeVirtualDomain(name)
VirtualDomain-->>QueryResolver: VirtualDomain
QueryResolver-->>Client: VirtualDomain
else No resolver on-chain
UniversalResolver-->>QueryResolver: zeroAddress
QueryResolver-->>Client: null
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| // TODO: implement this | ||
| return true; |
There was a problem hiding this comment.
| // TODO: implement this | |
| return true; | |
| // VirtualDomains are unindexed names without canonical metadata, so they are never canonical. | |
| // All canonical metadata fields (canonicalDepth, canonicalPath, canonicalNode, etc.) are set | |
| // to null for VirtualDomains, indicating they are not in the canonical nametree. | |
| return false; |
The isCanonicalName function incorrectly returns true for all VirtualDomains, marking unindexed names as canonical when they should never be canonical.
| // small hack for testing since I cannot connect to production database :( | ||
| if (name === "offchaindemo.eth") { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
| // small hack for testing since I cannot connect to production database :( | |
| if (name === "offchaindemo.eth") { | |
| return true; | |
| } |
Hardcoded test data in production code causes incorrect resolver check for "offchaindemo.eth"
| export function makeVirtualDomain(name: InterpretedName): Domain { | ||
| const firstLabel = interpretedNameToInterpretedLabels(name)[0]; | ||
| const labelHash = labelhashInterpretedLabel(firstLabel); | ||
| const virtualDomain: VirtualDomain = { |
There was a problem hiding this comment.
Unsafe type casting in makeVirtualDomain bypasses TypeScript type safety for ENSv1DomainId construction
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts`:
- Around line 161-165: The function nameHasResolver contains a hardcoded test
bypass returning true for "offchaindemo.eth"; remove that conditional so the
function always performs the real resolver check (i.e., eliminate the
special-case branch in nameHasResolver and rely on the existing chain lookup
logic), or if needed for local tests move the bypass behind a proper test
seam/flag checked only in test code (e.g., use a test-only environment variable
or dependency-injected mock) so the production code path for nameHasResolver and
Query.domain remains accurate.
In `@apps/ensapi/src/omnigraph-api/schema/domain.ts`:
- Around line 384-405: The virtual domain code is incorrectly marking every
virtual domain as canonical because isCanonicalName(name) always returns true;
change isCanonicalName to return false until you can correctly derive
canonicality (or implement the real logic), so that DomainInterfaceRef.canonical
(the canonical field set when creating virtualDomain) remains false/null when
canonical path/node metadata (canonicalPath, canonicalNode,
canonicalLabelHashPath, __canonicalNamePrefix) are not populated; update the
isCanonicalName function to return false by default or explicitly check for the
presence of canonical metadata before returning true.
In `@apps/ensapi/src/omnigraph-api/schema/query.ts`:
- Around line 131-139: The resolver currently returns
makeVirtualDomain(args.by.name) for names without indexed IDs but still treats
args.by.id as a normal DomainId—causing virtual IDs to not reload. Fix by
detecting virtual domain IDs and materializing them instead of passing them to
the standard loader: either (A) in the resolve function add a check for a
virtual-id pattern (or use a small helper isVirtualDomainId) and if true return
makeVirtualDomain(extractNameFromVirtualId(args.by.id)), otherwise continue to
use args.by.id; or (B) update DomainInterfaceRef.load to recognize the
virtual-id format and return makeVirtualDomain(...) when encountering it.
Reference functions/types: resolve, makeVirtualDomain,
getDomainIdByInterpretedName, nameHasResolver, DomainInterfaceRef.load, and
ensDb.query.domain so the change targets the correct code paths.
🪄 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: 5937e228-10f5-4e4f-86a8-3fa3a41380ad
⛔ 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/lib/get-domain-by-interpreted-name.tsapps/ensapi/src/omnigraph-api/schema/account.tsapps/ensapi/src/omnigraph-api/schema/domain.tsapps/ensapi/src/omnigraph-api/schema/query.ts
| export async function nameHasResolver(name: InterpretedName): Promise<boolean> { | ||
| // small hack for testing since I cannot connect to production database :( | ||
| if (name === "offchaindemo.eth") { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Remove the hardcoded resolver bypass.
Line 163 makes nameHasResolver() return true without consulting the chain, so Query.domain(by: { name: "offchaindemo.eth" }) can surface a VirtualDomain even when the real resolver lookup would fail in the active namespace/environment. If this is only needed for local testing, it should live behind a test seam, not in the shipped code.
Suggested change
export async function nameHasResolver(name: InterpretedName): Promise<boolean> {
- // small hack for testing since I cannot connect to production database :(
- if (name === "offchaindemo.eth") {
- return true;
- }
const {
contracts: {
UniversalResolver: { address, abi },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function nameHasResolver(name: InterpretedName): Promise<boolean> { | |
| // small hack for testing since I cannot connect to production database :( | |
| if (name === "offchaindemo.eth") { | |
| return true; | |
| } | |
| export async function nameHasResolver(name: InterpretedName): Promise<boolean> { | |
| const { | |
| contracts: { | |
| UniversalResolver: { address, abi }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts` around
lines 161 - 165, The function nameHasResolver contains a hardcoded test bypass
returning true for "offchaindemo.eth"; remove that conditional so the function
always performs the real resolver check (i.e., eliminate the special-case branch
in nameHasResolver and rely on the existing chain lookup logic), or if needed
for local tests move the bypass behind a proper test seam/flag checked only in
test code (e.g., use a test-only environment variable or dependency-injected
mock) so the production code path for nameHasResolver and Query.domain remains
accurate.
| export function makeVirtualDomain(name: InterpretedName): Domain { | ||
| const firstLabel = interpretedNameToInterpretedLabels(name)[0]; | ||
| const labelHash = labelhashInterpretedLabel(firstLabel); | ||
| const virtualDomain: VirtualDomain = { | ||
| type: "VirtualDomain", | ||
| // probably not correct | ||
| id: namehashInterpretedName(name) as unknown as ENSv1DomainId, | ||
| label: { labelHash, interpreted: firstLabel }, | ||
| labelHash, | ||
| canonical: isCanonicalName(name), | ||
| registryId: getRootRegistryId(di.context.namespace), | ||
| subregistryId: null, |
There was a problem hiding this comment.
The synthetic registry is wrong for nested virtual names.
Line 385 always assigns getRootRegistryId(...), but this file defines Domain.registry as the registry under which the domain exists. For foo.bar.eth, the leaf label is foo (Line 376), so its registry is bar.eth’s subregistry, not the ENS root. As written, registry, parent, and any registry-scoped follow-up queries are only correct for depth-1 names.
| canonical: isCanonicalName(name), | ||
| registryId: getRootRegistryId(di.context.namespace), | ||
| subregistryId: null, | ||
| tokenId: null, | ||
| node: null, | ||
| ownerId: null, | ||
| rootRegistryOwnerId: null, | ||
| canonicalName: name, | ||
| canonicalDepth: null, | ||
| canonicalPath: null, | ||
| canonicalNode: null, | ||
| canonicalLabelHashPath: null, | ||
| __canonicalNamePrefix: null, | ||
| __latestRegistrationExpiry: 2n ** 256n - 1n, | ||
| __latestRegistrationStart: 2n ** 256n - 1n, | ||
| }; | ||
| return virtualDomain as unknown as Domain; | ||
| } | ||
|
|
||
| function isCanonicalName(name: InterpretedName): boolean { | ||
| // TODO: implement this | ||
| return true; |
There was a problem hiding this comment.
Do not mark every virtual domain as canonical.
DomainInterfaceRef.canonical at Lines 132-137 becomes non-null whenever domain.canonical is truthy. Here isCanonicalName() always returns true, but Lines 392-395 leave the canonical path/node metadata null, so virtual domains claim canonicality without any canonical data to support it. Until canonicality can be derived, this should stay false/null.
Suggested change
const virtualDomain: VirtualDomain = {
type: "VirtualDomain",
@@
- canonical: isCanonicalName(name),
+ canonical: false,
@@
-}
-
-function isCanonicalName(name: InterpretedName): boolean {
- // TODO: implement this
- return true;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ensapi/src/omnigraph-api/schema/domain.ts` around lines 384 - 405, The
virtual domain code is incorrectly marking every virtual domain as canonical
because isCanonicalName(name) always returns true; change isCanonicalName to
return false until you can correctly derive canonicality (or implement the real
logic), so that DomainInterfaceRef.canonical (the canonical field set when
creating virtualDomain) remains false/null when canonical path/node metadata
(canonicalPath, canonicalNode, canonicalLabelHashPath, __canonicalNamePrefix)
are not populated; update the isCanonicalName function to return false by
default or explicitly check for the presence of canonical metadata before
returning true.
| resolve: async (parent, args, ctx, info) => { | ||
| if (args.by.id !== undefined) return args.by.id; | ||
| return getDomainIdByInterpretedName(args.by.name); | ||
| const domainId = await getDomainIdByInterpretedName(args.by.name); | ||
| if (domainId !== null) return domainId; | ||
| // Name is not indexed. Verify it actually has a resolver on-chain (via UniversalResolver) | ||
| // before returning a VirtualDomain — prevents surfacing a VirtualDomain for names that | ||
| // don't exist (e.g. random-string.eth with no resolver). | ||
| if (!(await nameHasResolver(args.by.name))) return null; | ||
| return makeVirtualDomain(args.by.name); |
There was a problem hiding this comment.
Virtual domain IDs are not reloadable through this API.
This resolver can now return makeVirtualDomain(args.by.name), but Line 132 still treats by.id as an indexed DomainId and hands it to the normal DomainInterfaceRef.load path. That loader only hits ensDb.query.domain in apps/ensapi/src/omnigraph-api/schema/domain.ts Lines 73-80, so echoing back the id from a virtual domain will not reconstruct the same object. Right now the new VirtualDomain.id is not a stable API reference in practice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ensapi/src/omnigraph-api/schema/query.ts` around lines 131 - 139, The
resolver currently returns makeVirtualDomain(args.by.name) for names without
indexed IDs but still treats args.by.id as a normal DomainId—causing virtual IDs
to not reload. Fix by detecting virtual domain IDs and materializing them
instead of passing them to the standard loader: either (A) in the resolve
function add a check for a virtual-id pattern (or use a small helper
isVirtualDomainId) and if true return
makeVirtualDomain(extractNameFromVirtualId(args.by.id)), otherwise continue to
use args.by.id; or (B) update DomainInterfaceRef.load to recognize the
virtual-id format and return makeVirtualDomain(...) when encountering it.
Reference functions/types: resolve, makeVirtualDomain,
getDomainIdByInterpretedName, nameHasResolver, DomainInterfaceRef.load, and
ensDb.query.domain so the change targets the correct code paths.
|
closing in favor of #2271 |
2 participants
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)