feat: materialize canonicality in the index, expose in omnigraph#2061
Merged
feat: materialize canonicality in the index, expose in omnigraph#2061
Conversation
🦋 Changeset detectedLatest commit: e77f7fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 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 |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks canonical-namegraph storage and traversal: adds canonical flags and pointers to schema, removes the canonical-registries CTE, introduces indexer DB helpers to maintain canonical invariants (including bridged-resolver wiring), and updates Omnigraph queries, forward-resolution, and GraphQL schema to be canonical-aware. ChangesCanonicality: schema, indexer helpers, and indexer wiring
Omnigraph queries, forward-resolution, and GraphQL surface
Sequence Diagram(s)sequenceDiagram
participant Indexer as Indexer
participant DB as ENS DB
participant ENSSDK as ENS SDK (isBridgedResolver)
participant ENSApi as ENSApi / Omnigraph
Note over Indexer,DB: Indexer processes on-chain events
Indexer->>DB: ensureRegistry(registryId, ...)
Indexer->>DB: ensureDomainInRegistry(registryId, domainId)
Indexer->>ENSSDK: isBridgedResolver(namespace, resolver)
ENSSDK-->>Indexer: BridgedResolverTarget | null
alt bridged target found
Indexer->>DB: handleBridgedResolverChange(...)\n(set domain.subregistry, set registry.canonicalDomainId)
end
Note over DB,ENSApi: DB stores canonical flags and pointers
ENSApi->>DB: query domainsBase / forwardWalkDisjointNamegraph
DB-->>ENSApi: canonical-aware path rows
ENSApi->>ENSSDK: forward-resolution via bridged registry (registryId/registry)
ENSSDK-->>ENSApi: resolution result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves ENS “canonicality” from query-time recursive traversal into index-time materialization, then exposes the resulting canonical flags and canonical parent/child edges through the Omnigraph GraphQL schema. This reduces query complexity/cost (no canonical registries CTE / edge-auth joins) and enables first-class Domain.canonical / Registry.canonical semantics.
Changes:
- Materializes canonicality in the indexed DB (
Registry.canonical/canonicalDomainId,Domain.canonical/canonicalSubregistryId) plus a per-registry child list (registryDomains) to support cascading canonicality updates. - Updates ENSv1/ENSv2 indexer handlers (incl. new ENSv2
ParentUpdated+ResolverUpdated) to maintain the bidirectional canonical edge invariant and bridged-resolver attachments. - Simplifies Omnigraph canonical path + domain search/name-walk logic to use the materialized edges/flags; adds
canonicalfields to GraphQL types and regenerates SDK schema artifacts.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Adds canonical: Boolean to Domain/Registry interfaces and implementing types. |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Regenerates introspection to include new canonical fields. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Removes getRootRegistryIds; clarifies getRootRegistryId as canonical root. |
| packages/ensnode-sdk/src/shared/protocol-acceleration/is-bridged-resolver.ts | Changes bridged-resolver detection to return a richer discriminated union incl. registry id + metadata; requires originatingNode. |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Adds canonicality columns + introduces registryDomains table; removes registryCanonicalDomain. |
| packages/datasources/src/abis/ensv2/Registry.ts | Updates ENSv2 Registry ABI (drops Approval event; renames an output). |
| examples/enskit-react-example/src/SearchView.tsx | Updates example search UI copy and query to render only canonical domains. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Ensures registries/domains are tracked in registryDomains; adds ParentUpdated and ResolverUpdated canonicality/bridge wiring. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Switches to ensureRegistry + canonicality helpers; wires bridged-resolver changes from ENSv1 NewResolver. |
| apps/ensindexer/src/lib/ensv2/registry-db-helpers.ts | Adds ensureRegistry helper that seeds root registry as canonical. |
| apps/ensindexer/src/lib/ensv2/canonicality-db-helpers.ts | Introduces core canonicality maintenance helpers (bidirectional edge + cascading flips + bridge attach/detach). |
| apps/ensapi/src/omnigraph-api/schema/resolver.ts | Updates Resolver.bridgesTo to new isBridgedResolver signature (uses sentinel node). |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Exposes Registry.canonical field. |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Exposes Domain.canonical field. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Updates test commentary to reflect new canonicality mechanics. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Removes query-time bridged-resolver recursion; walks canonical namegraph using materialized edges/flags. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Simplifies canonical path traversal to materialized edges + short-circuit on domain.canonical. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Updates upward traversal to use registry.canonicalDomainId instead of removed table. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Filters via domain.canonical instead of canonical registries CTE. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Derives parentId from registry.canonicalDomainId and includes canonical in the base shape. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Deletes no-longer-needed canonical registries recursive CTE. |
| apps/ensapi/src/lib/resolution/forward-resolution.ts | Updates forward-resolution recursion to new isBridgedResolver signature/projection. |
Comments suppressed due to low confidence (1)
packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts:290
Domain.canonicalis now a frequently-used filter predicate (e.g.filterByCanonicalbecomesWHERE base.canonical = TRUE, and the enskit example queriescanonical: true), but the schema doesn’t add an index/partial index ondomains.canonical. On large datasets this can turn canonical-only queries into full scans.
Consider adding a partial index like (canonical) WHERE canonical = true (or a composite index that matches your common query patterns) to keep canonical filtering performant.
// Whether this Domain is part of the canonical namegraph. Mirrors the parent Registry's flag.
canonical: t.boolean().notNull().default(false),
// The Subregistry of this Domain that participates in the canonical namegraph (i.e. the
// Registry whose `canonicalDomainId` points back to this Domain). May differ from
// `subregistryId` when a Bridged Resolver attaches a different Registry under this Domain.
canonicalSubregistryId: t.text().$type<RegistryId>(),
// NOTE: Domain-Resolver Relations tracked via Protocol Acceleration plugin
}),
(t) => ({
byType: index().on(t.type),
byRegistry: index().on(t.registryId),
bySubregistry: index().on(t.subregistryId).where(sql`${t.subregistryId} IS NOT NULL`),
byOwner: index().on(t.ownerId),
byLabelHash: index().on(t.labelHash),
}),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile SummaryThis PR materializes
Confidence Score: 3/5Two confirmed defects affect production bridged namegraphs (Basenames, Lineanames) and could corrupt canonicality silently or crash the indexer. The cascadeCanonicality SQL logic and query-layer rewrites are well-designed and internally consistent. However, two defects in the indexer write path are confirmed: (1) every non-TLD ENSv1 NewOwner unconditionally overwrites domain.subregistryId with the L1 virtual registry, silently destroying any bridged canonical edge already set by handleBridgedResolverChange; (2) handleBridgedResolverChange calls handleRegistryCanonicalDomainUpdated which throws if the target virtual registry row does not exist, and for real Basenames/Lineanames deployments the mainnet NewResolver event precedes any L2 NewOwner, so this invariant is violated in practice and would crash the indexer during a fresh index run. apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts (unconditional subregistryId overwrite) and apps/ensindexer/src/lib/ensv2/canonicality-db-helpers.ts (missing registry-existence guard before handleRegistryCanonicalDomainUpdated). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["ENSv1/ENSv2 on-chain event"] --> B{Event type}
B -->|"NewOwner (non-TLD)"| C["handleSubregistryUpdated\n(parentDomain → L1 VR)"]
B -->|"SubregistryUpdated"| D["handleSubregistryUpdated\n(domain → next subregistry)"]
B -->|"ParentUpdated"| E["handleRegistryCanonicalDomainUpdated\n(child registry → parent domain)"]
B -->|"NewResolver / ResolverUpdated"| F["handleBridgedResolverChange\n(detach prev / attach next bridged)"]
C --> G["reconcileRegistryCanonicality"]
D --> G
E --> G
F --> G
G --> H{"flag flipped?"}
H -->|No| I["no-op"]
H -->|"Yes: __hasChildren=false"| J["in-memory PK update\n(ENSv1 fast-path)"]
H -->|"Yes: __hasChildren=true"| K["cascadeCanonicality\nrecursive-CTE batch UPDATE\n(flush Ponder cache)"]
K --> L["Registry.canonical + Domain.canonical\nupdated for entire subtree"]
Reviews (14): Last reviewed commit: "fix: remove unnecessary changeset" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts (1)
130-148:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSilent fall-through when the deepest resolver is an ENSv2Resolver.
The
// TODO: ENSv2Resolverbranch on Line 144 means that whendeepestResolveris set but is not the ENSv1 fallback resolver, control silently flows into thereturn exact ? deepest.domainId : null;line. For a non-exact match with an ENSv2Resolver in the middle of the path, callers will getnullwith no log or error to attribute the miss — making this hard to debug post-merge. Consider at least alogger.debug({ deepestResolver })or a typed error to aid triage until the ENSv2Resolver hop is implemented.Want me to open a tracking issue for the ENSv2Resolver hop and add a debug log here in the meantime?
🤖 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 130 - 148, The deepestResolver branch currently falls through silently for ENSv2 resolvers; update the TODO: ENSv2Resolver branch to log the resolver before returning so non-exact misses are debuggable — e.g., call logger.debug({ deepestResolver, path, depth }) (or logger.warn) when deepestResolver exists and isn't equal to ENSv1Resolver (found via maybeGetDatasourceContract) before the final return; keep the existing ENSv1Resolver fallback logic (resolveCanonicalDomainId/getENSv1RootRegistryId) and then return exact ? deepest.domainId : null as before.
🤖 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/schema/domain.ts`:
- Around line 109-113: The Domain.canonical and Registry.canonical GraphQL field
definitions are missing explicit nullable declarations and default to nullable:
true; update the t.field calls for the Domain.canonical resolver (symbol:
canonical in apps/ensapi/src/omnigraph-api/schema/domain.ts) and the
Registry.canonical resolver (symbol: canonical in
apps/ensapi/src/omnigraph-api/schema/registry.ts) to include nullable: false so
the schema reflects the database not-null boolean semantics and matches file
conventions.
In `@apps/ensindexer/src/lib/ensv2/canonicality-db-helpers.ts`:
- Around line 173-190: handleBridgedResolverChange currently treats
domain.canonicalSubregistryId (prev) as bridge-owned and unconditionally calls
setRegistryCanonicalDomain(prev, null) when a bridged resolver disappears;
instead only detach a canonicalSubregistryId that was created by the bridge for
this originatingNode. Modify handleBridgedResolverChange to determine the set of
bridge-derived registry IDs for the originatingNode (use isBridgedResolver logic
or the same provenance mechanism that creates those registries) and only call
setRegistryCanonicalDomain(context, prev, null) when prev is non-null and
matches one of those bridge-derived IDs; do not clear canonicalSubregistryId for
registries that are normal (non-bridge) parent/child links. Use function
names/vars: handleBridgedResolverChange, canonicalSubregistryId,
setRegistryCanonicalDomain, isBridgedResolver, originatingNode, prev to locate
and implement this conditional check or tracking of bridge provenance.
In `@packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts`:
- Around line 616-619: The registry_domains schema uses a hot-path text[] column
(registryDomains.domainIds) causing full-row rewrites on
ensureDomainInRegistry/removeDomainFromRegistry and expensive reads in
canonicality-db-helpers.ts; replace this with a normalized onchainTable that
stores one row per (registryId, domainId) with a composite primary key, or if
immediate migration is not possible open a follow-up to implement that
normalized table and add instrumentation around cascade write latency and read
hotspots (measure operations in ensureDomainInRegistry/removeDomainFromRegistry
and the cascade walks in canonicality-db-helpers.ts) so we can detect
regressions early.
---
Outside diff comments:
In `@apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts`:
- Around line 130-148: The deepestResolver branch currently falls through
silently for ENSv2 resolvers; update the TODO: ENSv2Resolver branch to log the
resolver before returning so non-exact misses are debuggable — e.g., call
logger.debug({ deepestResolver, path, depth }) (or logger.warn) when
deepestResolver exists and isn't equal to ENSv1Resolver (found via
maybeGetDatasourceContract) before the final return; keep the existing
ENSv1Resolver fallback logic (resolveCanonicalDomainId/getENSv1RootRegistryId)
and then return exact ? deepest.domainId : null as before.
🪄 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: efd22769-1097-48d0-b83a-3042c36ab0d9
⛔ Files ignored due to path filters (2)
packages/enssdk/src/omnigraph/generated/introspection.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (20)
apps/ensapi/src/lib/resolution/forward-resolution.tsapps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.tsapps/ensapi/src/omnigraph-api/lib/get-canonical-path.tsapps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.tsapps/ensapi/src/omnigraph-api/schema/domain.integration.test.tsapps/ensapi/src/omnigraph-api/schema/domain.tsapps/ensapi/src/omnigraph-api/schema/registry.tsapps/ensapi/src/omnigraph-api/schema/resolver.tsapps/ensindexer/src/lib/ensv2/canonicality-db-helpers.tsapps/ensindexer/src/lib/ensv2/registry-db-helpers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.tsexamples/enskit-react-example/src/SearchView.tsxpackages/datasources/src/abis/ensv2/Registry.tspackages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.tspackages/ensnode-sdk/src/shared/protocol-acceleration/is-bridged-resolver.tspackages/ensnode-sdk/src/shared/root-registry.ts
💤 Files with no reviewable changes (2)
- apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts
- apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts
Add integration tests covering Domain.canonical and Registry.canonical: canonical=true for v2-rooted entities, canonical=false for ENSv1 addr.reverse and the v1 root registry (which is non-canonical in ens-test-env where the v2 root is the namespace's canonical root). Add changeset for the omnigraph schema additions.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Reorder handler registration: NodeMigration -> ENSv2 -> ProtocolAcceleration. Lets handleBridgedResolverChange read the previous Domain-Resolver Relation before ProtocolAcceleration overwrites it, removing the brittle canonicalSubregistryId-as-provenance hack that detached normal canonical edges on non-bridged resolver updates. - Extract node migration into its own handlers/node-migration.ts so it can be registered ahead of both plugins. - handleBridgedResolverChange now takes the registry AccountId and reads prev via DRR PK lookup; isBridgedResolver determines bridge provenance. - Drop unused removeDomainFromRegistry. - ensureDomainInRegistry throws when registry row is missing (invariant). - updateRegistryCanonicality: read child once, add MAX_CASCADE_DEPTH=16 guard. - getCanonicalPath: throw on impossible zero-row result for defense in depth. - Domain.canonical and Registry.canonical are nullable: false on the omnigraph schema (matches notNull DB columns). Regenerate schema.graphql + introspection. - ParentUpdated: add TODO comment about whether it should also be a domain event.
Member
Author
|
@greptile review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 27 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts (1)
374-380:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParentUpdated event still not linked to domain history.
The TODO at line 377 acknowledges this gap, and the event row is created at line 378 but never connected to either the child or the parent domain history via
ensureDomainEvent. If you intend to associateParentUpdatedwith the child domain (the registry that emitted it), the most natural place isthisRegistry's root domain (or the domain whose canonical parent just changed). Otherwise, please convert the TODO into an explicit decision in code so reviewers don't continue flagging it.🤖 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/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts` around lines 374 - 380, The ParentUpdated event is created via ensureEvent but not linked to domain history; either attach it to the affected domain or make the TODO an explicit decision. Fix by calling ensureDomainEvent for the domain whose canonical parent changed (use the child/domain identifier from the ParentUpdated payload, e.g. event.node or the registry root for thisRegistry) after ensureEvent so the event is recorded in domain history; alternatively replace the TODO with a clear comment/flag that documents the intentional choice not to link ParentUpdated. Ensure you reference ParentUpdated, ensureEvent and ensureDomainEvent (and thisRegistry or event.node as the domain identifier) when making the change.
🤖 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 @.changeset/canonical-fields-omnigraph.md:
- Line 5: The changeset text incorrectly states that Domain.canonical and
Registry.canonical are nullable; update the release note in .changeset to
reflect that both fields are non-nullable (GraphQL type Boolean!), e.g., change
wording to state they are exposed as non-nullable Boolean! fields and clearly
indicate they always return a boolean value, referencing the symbols
Domain.canonical and Registry.canonical so readers know which schema fields were
changed.
In `@apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts`:
- Around line 38-50: The recursive CTE "upward" currently stops at MAX_DEPTH
(16) and can silently return a truncated canonical path; modify the logic around
the "upward" CTE and the final path/name assembly to detect when recursion hit
MAX_DEPTH (i.e., any row with upward.depth >= MAX_DEPTH) and surface an error
instead of returning partial results. Concretely, either add a SQL guard after
the recursion (e.g., an existence check on upward where depth >= MAX_DEPTH that
causes the query to fail) or perform an explicit check in the calling function
(the get-canonical-path flow) after the query and throw a clear error when
MAX_DEPTH was reached; adjust both the upward CTE and the later assembly used
around the same block (also apply to the similar block at the 59-64 region) so
deep ENS names don't get silently truncated.
In `@apps/ensindexer/src/lib/ensv2/canonicality-db-helpers.ts`:
- Around line 24-29: The MAX_CASCADE_DEPTH constant is set too low and will
falsely abort valid ENS trees because updateRegistryCanonicality() recurses once
per canonical subregistry hop; replace this brittle depth cap with explicit
cycle detection (track visited registry/node identifiers in
updateRegistryCanonicality and bail with a clear error if a repeated id is
encountered) or, if a cap must remain, raise MAX_CASCADE_DEPTH to the protocol's
actual maximum nesting and document it; update any other uses (e.g., the second
occurrence at the other location) to use the new cycle-detection logic or the
revised constant.
- Around line 41-48: The current read-modify-write (existing/domainIds) can lose
concurrent updates; replace it with a single atomic upsert that appends the new
domainId at the DB level instead of reading the full row first. Modify the
insert on ensIndexerSchema.registryDomains via context.ensDb to use
onConflictDoUpdate that sets domainIds =
dedup(array_cat(registry_domains.domainIds, ARRAY[<domainId>]::text[])) (or the
equivalent array_cat/array_append + DISTINCT expression your SQL driver
supports) so the DB merges the new domainId into the existing array atomically;
reference the same registryId conflict key and avoid the prior existing
check/merge in canonicality-db-helpers.ts (remove the existing find/if block and
the client-side domainIds merge). Ensure the expression uses EXCLUDED or the
table column names your query builder exposes to perform the concat+distinct in
the UPDATE clause.
- Around line 41-58: The code inserts/updates registryDomains before confirming
the parent registry exists, which can leave an orphaned child-list on error;
change the order in the function that handles registry/domain linking (the block
using registryId, domainId, context.ensDb and collections
ensIndexerSchema.registryDomains, ensIndexerSchema.registry,
ensIndexerSchema.domain) so you first fetch/validate the parent registry (await
context.ensDb.find(ensIndexerSchema.registry, { id: registryId })) and throw if
missing, and only after that perform the registryDomains insert/update and the
domain update (onConflictDoUpdate for registryDomains and .update(...).set({
canonical: reg.canonical })) to prevent creating the child list when the parent
is absent.
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts`:
- Around line 389-407: The handler in ENSv2Registry uses `tokenId` typed as
`bigint` and force-casts it with `as never`; update the event arg typing to use
the branded TokenId (e.g. EventWithArgs<{ tokenId: TokenId; resolver:
NormalizedAddress }>) and remove the `as never` casts when calling makeStorageId
and interpretTokenIdAsNode so you pass the TokenId directly; ensure the TokenId
type is imported/available and keep the rest of the logic calling
handleBridgedResolverChange(context, registry, domainId, originatingNode,
resolver) unchanged.
---
Duplicate comments:
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts`:
- Around line 374-380: The ParentUpdated event is created via ensureEvent but
not linked to domain history; either attach it to the affected domain or make
the TODO an explicit decision. Fix by calling ensureDomainEvent for the domain
whose canonical parent changed (use the child/domain identifier from the
ParentUpdated payload, e.g. event.node or the registry root for thisRegistry)
after ensureEvent so the event is recorded in domain history; alternatively
replace the TODO with a clear comment/flag that documents the intentional choice
not to link ParentUpdated. Ensure you reference ParentUpdated, ensureEvent and
ensureDomainEvent (and thisRegistry or event.node as the domain identifier) when
making the change.
🪄 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: 809c939b-d98f-4942-a4ae-f4edaba89cc8
⛔ Files ignored due to path filters (2)
packages/enssdk/src/omnigraph/generated/introspection.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (12)
.changeset/canonical-fields-omnigraph.mdapps/ensapi/src/omnigraph-api/lib/get-canonical-path.tsapps/ensapi/src/omnigraph-api/schema/domain.integration.test.tsapps/ensapi/src/omnigraph-api/schema/domain.tsapps/ensapi/src/omnigraph-api/schema/registry.integration.test.tsapps/ensapi/src/omnigraph-api/schema/registry.tsapps/ensindexer/ponder/src/register-handlers.tsapps/ensindexer/src/lib/ensv2/canonicality-db-helpers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/node-migration.ts
The wallet Registry's ParentUpdated claims sub1.sub2.parent.eth as its canonical parent; linked.parent.eth.subregistry was re-pointed to the same Registry without a corresponding ParentUpdated, so wallet.linked.parent.eth is now a non-canonical alias that does not resolve via the canonical-name walk. Update Domain.path tests and DEVNET_NAMES to reflect this. Also: v1 'eth' Domain has a null canonical name in ens-test-env (v2 root is the namespace's canonical root), so update Query.domains > sees .eth domain to assert name: null on the v1 entity. Account.domains expected list flips wallet.linked.parent.eth → wallet.sub1.sub2.parent.eth.
- ENSv2 ParentUpdated handler: note `parent` is interpreted same-chain; cross-chain parentage goes through Bridged Resolvers, not ParentUpdated - handleBridgedResolverChange: document the two implied invariants — 1:1 originating-Domain-per-bridged-Registry, and bridged target indexed before the Bridged Resolver event fires (with the fix sketch if a future Bridged Resolver violates it) - Add changeset for the breaking Resolver.bridged GraphQL type change (AccountId scalar → Registry interface) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- getRootRegistryIds removed (replaced by isRootRegistryId predicate) - BridgedResolverTarget shape: shadow → registryId (in /internal export) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
every nameable Domain is canonical by definition, so the canonical filter on Query.domains was redundant. Query.domains now always scopes to Canonical Domains. consumers reading Domain.canonical directly are unaffected; consumers relying on canonical:false to surface non-canonical Domains via this query should switch to Account.domains / Registry.domains or read Domain.canonical directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- handleRegistryCanonicalDomainUpdated: docstring now matches the single-reconcile body; invariant tag matches the function name - Resolver.bridged changeset: lead with the migration (scalar → Registry interface) instead of the post-change shape Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- canonicality-db-helpers: typo "canoniality"; grammar "may not yet" → "need not exist yet" - get-canonical-path: typo "Canonial" - ensdb/database-schemas docs: align with current canonicality model (boolean flags + on-demand agreement check, no parallel materialization tables) - isBridgedResolver: memoize per-namespace BridgedResolverConfig[] — sits on every NewResolver/ResolverUpdated event during indexing and every forward-resolution hop, configs are pure function of namespace Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
inline the cache check at entry + cache write at exit instead of splitting build/cached-accessor across two functions. one name for callers, one place to edit when adding a new bridge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
102
to
108
| const parentDomainId = makeENSv1DomainId(registry, parentNode); | ||
| // route through handleSubregistryUpdated so any prior subregistry edge (e.g. a bridged | ||
| // attachment) is properly reconciled instead of orphaned by a blind overwrite. | ||
| await handleSubregistryUpdated(context, parentDomainId, parentRegistryId); | ||
|
|
||
| // set the parent Domain's subregistry to said registry | ||
| await context.ensDb | ||
| .update(ensIndexerSchema.domain, { id: parentDomainId }) | ||
| .set({ subregistryId: parentRegistryId }); | ||
|
|
||
| // ensure Canonical Domain reference | ||
| await context.ensDb | ||
| .insert(ensIndexerSchema.registryCanonicalDomain) | ||
| .values({ registryId: parentRegistryId, domainId: parentDomainId }) | ||
| .onConflictDoNothing(); | ||
| await handleRegistryCanonicalDomainUpdated(context, parentRegistryId, parentDomainId); | ||
| } |
Contributor
There was a problem hiding this comment.
NewOwner unconditionally destroys bridged subregistryId
Every non-TLD NewOwner calls handleSubregistryUpdated(context, parentDomainId, parentRegistryId) where parentRegistryId is the L1 virtual registry. handleSubregistryUpdated writes the new value unconditionally (after a prevId === nextId early-exit that won't fire because the bridged L2 registry and the L1 VR are different IDs).
Concrete failure: bridge.linea.eth (or any other ENSv1 subname under linea.eth) gets a NewOwner event on mainnet. handleSubregistryUpdated sets linea_eth_domain.subregistryId = L1_VR_for_linea_eth, overwriting the previously bridged pointer to the Linea L2 registry. reconcileRegistryCanonicality is then called for both the Linea L2 registry (prev) and the L1 VR (next). Because linea_eth_domain.subregistryId now agrees with the L1 VR, the L1 VR becomes canonical and the entire Linea namegraph loses canonicality silently. The comment // route through handleSubregistryUpdated so any prior subregistry edge (e.g. a bridged attachment) is properly reconciled describes canonicality reconciliation, not preservation of the bridged pointer — the bridged pointer is lost.
Comment on lines
+220
to
+228
| // if the next resolver is a Bridged Resolver, we need to update the Domain's Subregistry | ||
| if (nextBridged) { | ||
| // update the domain's indicated subregistry | ||
| await handleSubregistryUpdated(context, domainId, nextBridged.targetRegistryId); | ||
|
|
||
| // only update the Registry's Canonical Domain iff this is the correct originating Domain | ||
| if (nextBridged.originDomainId === domainId) { | ||
| await handleRegistryCanonicalDomainUpdated(context, nextBridged.targetRegistryId, domainId); | ||
| } |
Contributor
There was a problem hiding this comment.
handleBridgedResolverChange crashes when bridged target registry doesn't yet exist
handleRegistryCanonicalDomainUpdated (line 227) throws Invariant(...): Registry does not yet exist if the target registry row is absent. The target VR (makeENSv1VirtualRegistryId(basenamesRegistry, base_eth_node)) is created only when the first ENSv1 NewOwner on Base fires — i.e., when the first Basenames subname is indexed. The mainnet NewResolver(base.eth, BasenamesL1Resolver) event predates Basenames' launch: in real-world timestamp order it is processed by Ponder before any Base NewOwner events exist, meaning the VR row is absent and the indexer crashes here. The code comment in this function acknowledges the invariant but treats it as a property of the specific resolvers rather than verifying it holds.
| } | ||
|
|
||
| const rows = await walkCanonicalNamegraph(registryId, path); | ||
| // walk the disjoint namegraph by indicated by `registryId` through `path` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewer Focus (Read This First)
canonicality-db-helpers.ts. the canonical edge between Registry R and parent Domain D is NOT materialized in a parallel table; it is derived on demand by checking that the unidirectional pointers agree (R.canonicalDomainId = D.id↔D.subregistryId = R.id). reconcile runs at every pointer write and only fires the cascade whenRegistry.canonicalactually flips.Registry.__hasChildren = false(the dominant case for fresh ENSv1 virtual registries on first wire-up), or a single recursive-CTE batch UPDATE that walks the canonical subgraph by agreement check and flipsRegistry.canonical+Domain.canonicalon every visited row. the SQL path goes throughcontext.ensDb.sqlwhich forces a Ponder cache flush + invalidate; we accept that cost because it's bounded to flips on Registries that actually have descendants.ENSv2Registry.ParentUpdatedis emitted from the child registry and may arrive before the parent'sLabelRegistered/SubregistryUpdated. the helpers blindly write the unidirectional pointers and reconcile only on actual flag flips, so order doesn't matter — worth a second read.handleBridgedResolverChangereads the previous resolver from the Domain-Resolver Relation, so it must run before Protocol Acceleration'sNewResolver/ResolverUpdatedhandlers overwrite the DRR row. ordering enforced inapps/ensindexer/ponder/src/register-handlers.ts. two implied invariants are documented in the helper: 1:1 originating-Domain-per-bridged-Registry, and bridged target Registry indexed before its bridged-resolver event fires.Problem & Motivation
getRootRegistryIds(). every canonical filter, every name walk, every canonical-path lookup paid the traversal cost.registryCanonicalDomaintable was a last-write-wins shim with no edge auth on the writer side; the API layer compensated with edge-auth joins everywhere it was consumed. brittle.TODO(canonical-names)markers and a user-facing disclaimer in the enskit example flagged this as a known stopgap. moving canonicality into the index unblocks accurateDomain.canonical/Registry.canonicalexposure on the omnigraph schema.Domain.nameandDomain.path). Forward Resolution still prefers the ENSv2 namegraph when both exist; that preference is encoded in the resolver, not in the canonicality flags.bridge.linea.ethis non-canonical because mainnetlinea.eth.subregistryIdpoints at the bridged Linea Registry, not at the mainnet v1 virtual registry; only the Linea-sidebridge.linea.ethis the resolution-visible canonical record).What Changed (Concrete)
Registry.canonical,Registry.canonicalDomainId,Registry.__hasChildren,Domain.canonical. drop the oldregistryCanonicalDomaintable. the canonical edge is not materialized in a parallel table —Registry.canonicalDomainId(Registry → Domain) andDomain.subregistryId(Domain → Registry) are written blindly when their onchain events fire; the bidirectional canonical edge is derived on demand by checking agreement.apps/ensindexer/src/lib/ensv2/canonicality-db-helpers.ts:ensureDomainInRegistry: inherit parent Registry'scanonicalflag onto the new Domain, and flip the parent's__hasChildrensentinel from false→true on the first child (idempotent thereafter, monotonic).handleRegistryCanonicalDomainUpdated: writes the unidirectionalRegistry.canonicalDomainIdand reconciles canonicality.handleSubregistryUpdated: writes the unidirectionalDomain.subregistryIdand reconciles canonicality on both prev and next subregistries.handleBridgedResolverChange: detaches/attaches both halves of the bridged canonical edge; documents the two implied invariants (1:1 + event ordering) and the fix sketch if either is ever violated.reconcileRegistryCanonicality: recomputeRegistry.canonicalfrom the agreement check (root registries short-circuit to true); callscascadeCanonicalityonly when the flag actually flips.cascadeCanonicality: in-memory PK update whenRegistry.__hasChildren = false; otherwise a single recursive-CTE batch UPDATE that walks the canonical subgraph by agreement check.apps/ensindexer/src/lib/ensv2/registry-db-helpers.ts:ensureRegistryupsert that seedscanonical = trueonly for the namespace's Root Registries (ENSv1 root + ENSv2 root when defined). all other Registries default to false and earn canonicality through reconcile.isRootRegistryId(namespace, registryId)predicate in@ensnode/ensnode-sdk. removesgetRootRegistryIds(plural) — root membership is now a predicate, not an enumeration.NewOwnerswitches toensureRegistry+ensureDomainInRegistry; for non-TLDs also writes the parent'ssubregistryIdand callshandleRegistryCanonicalDomainUpdatedfor the synthesizedENSv1VirtualRegistry. ENSv1NewResolvernow invokeshandleBridgedResolverChange(event signature gainsresolver).LabelRegistered/LabelReservedswitch toensureRegistry+ensureDomainInRegistry. ENSv2SubregistryUpdatedcallshandleSubregistryUpdated. new ENSv2ParentUpdatedhandler — child registry claims its canonical parent viahandleRegistryCanonicalDomainUpdated(parent address interpreted same-chain). new ENSv2ResolverUpdatedhandler invokeshandleBridgedResolverChange.getCanonicalPath: short-circuits ondomain.canonical = false, walks upward via the agreement check at each hop (registries.canonical_domain_id = domains.idANDdomains.subregistry_id = registries.id).find-domains: deletecanonical-registries-cte.ts.filterByCanonicalis nowWHERE base.canonical = TRUE.base-domain-setderivesparentIdvia the same two-join-with-agreement-predicate pattern.filterByNamewalks via the agreement check at each hop in its recursive CTE.getDomainByInterpretedName: walks rawdomain.subregistry_idedges (not the canonical edge) — the comment at the recursive CTE makes this explicit. addressability via alias paths is intentional; canonical name/path comes from the canonical-edge walk ingetCanonicalPath.BridgedResolverrecursion branch retained;ENSv1Resolver/ENSv2Resolverfallback branches retained.canonical: Boolean!onDomainandRegistryinterfaces (and all implementing types).Resolver.bridgedreturn type changed fromAccountIdto the bridged targetRegistry(interface). regeneratedschema.graphqlandintrospection.ts.BridgedResolverTargetin@ensnode/ensnode-sdk/internalsimplified:{ registry: AccountId, shadow: boolean }→{ registryId: RegistryId, ... }.RegistryABI to latest (dropApprovalevent, rename anonymoustokenIdoutput).SearchView: drop the canonical-derivation disclaimer.Design & Planning
TODO(canonical-names)markers and the enskit disclaimer. materializing canonicality into the index was the obvious next step once the bidirectional invariant could be maintained at index time; collapsing the parallel materialization tables (domainCanonicalSubregistry,registryDomains) came later, after measuring the per-NewOwner array-rewrite cost on.ethvirtual-registry scale.Domain.name/Domain.pathpopulated for v1-only entities (e.g. addr.reverse, the v1 root's children) and lets resolution preference live in the resolver, where it belongs.registryDomains.domainIdsarray rewrite (the steady-state mainnet bottleneck) and the paralleldomainCanonicalSubregistryupsert/delete dance. cascade pays a Ponder cache flush only whenRegistry.canonicalactually flips AND the registry has descendants —Registry.__hasChildrenshort-circuits to in-memory PK update for the dominant fresh-virtual-registry case.Self-Review
handleRegistryCanonicalDomainUpdatedinitially didn't dislodge a prior occupant on the new domain — added the dislodge path in reconciliation.handleBridgedResolverChangeinitially read the prior target from a parallel canonical-edge table, which forced a complicated recovery when bridging detaches. switched to reading the prior resolver from the Domain-Resolver Relation (the natural source of truth), and ordered this helper before Protocol Acceleration's DRR-overwriting handlers inregister-handlers.ts.canonical=true) was wrong: it incorrectly canonicalized v1 children of a v1 Domain whose Bridged Resolver had claimed the canonical sub-registry elsewhere (e.g. mainnetbridge.linea.eth). fixed by only seeding root registries and letting the natural reconcile flow handle v1 the same way as v2.domainCanonicalSubregistryandregistryDomainstables entirely.getCanonicalPathandfilterByNamelost their materialized-edge JOINs (the agreement check is two joins inline).base-domain-setlost its parallel-table left-join. v1-specific seeding logic inensureRegistrycollapsed to "is this a root?".setRegistryCanonicalDomain→handleRegistryCanonicalDomainUpdatedfor symmetry withhandleSubregistryUpdated/handleBridgedResolverChange. consolidatedmaterializeAndCascade+updateRegistryCanonicality→reconcileRegistryCanonicality+cascadeCanonicality(one decides whether to flip, the other applies the flip).getRootRegistryIds,canonical-registries-cte.ts, theparentDomainalias in the old query, the oldregistryCanonicalDomain/domainCanonicalSubregistry/registryDomainstables,materializeAndCascade,updateRegistryCanonicality, twoTODO(canonical-names)blocks inENSv2Registry.Cross-Codebase Alignment
registryCanonicalDomain,getRootRegistryIds,canonical_domain,canonicalSubregistry,domainCanonicalSubregistry,registryDomains,isBridgedResolver,materializeAndCascade,updateRegistryCanonicality.find-domainsorder/limit layers (not affected), ensadmin (no consumers of removed helpers), ponder-subgraph (no canonicality references), ensapi resolution forward path (only theisBridgedResolvercall site).Downstream & Consumer Impact
Domain.canonical: Boolean!,Registry.canonical: Boolean!.Resolver.bridgedreturn type changed fromAccountIdto the bridged targetRegistry(interface). consumers selectingbridged { ... }now get theRegistryshape with rich navigation; consumers readingbridgedasAccountIdneed to update their selection.@ensnode/ensnode-sdk(breaking):getRootRegistryIdsremoved in favor of theisRootRegistryId(namespace, registryId)predicate.@ensnode/ensnode-sdk/internal(breaking):BridgedResolverTargetshape —{ registry, shadow }→{ registryId, ... }.canonical = true(previouslyfalseonce ENSv2 was deployed). consumers that filtered oncanonical: trueto reach the v2 namegraph specifically will need to filter on__typenameinstead, or scope by Registry. Forward Resolution preference is unchanged.Testing Evidence
Registry.canonicalasserts both v1 and v2 Root Registries are canonical.Domain.canonicalasserts ENSv1-onlyaddr.reverseis canonical (lookup by id; nameability holds even when resolution prefers v2).Query.domains > sees .eth domainexpects v1 'eth' to surfacename: "eth"(v1 root is canonical, so the canonical-name walk produces a non-null name for its children).Domain.path > returns the canonical path for a Domain looked up via a non-canonical alias—domain(by: { name: "wallet.linked.parent.eth" })resolves to the wallet Domain via raw subregistry edges (addressability), and itspathwalks the canonical edge by agreement check throughwallet.sub1.sub2.parent.eth → ... → eth(nameability). this is the test that locks in the addressability/nameability split.canonical: true; left asit.todo.ParentUpdatedvsSubregistryUpdatedandLabelRegistered. handlers are structured to be order-insensitive (write unidirectional pointers blindly, reconcile only on actual flag flips), but worth a second read.Scope Reductions
isBridgedResolverfor ENSv2 bridges when defined (detach is wired and works today; attach is currently unreachable via the ENSv2ResolverUpdatedpath). consider runtime enforcement of the 1:1 originating-Domain-per-bridged-Registry invariant inhandleBridgedResolverChange(currently documented, not enforced). drop theResolver.bridgedAccountId-shaped read path entirely once consumers migrate.Risk Analysis
__hasChildren = false(the dominant v1 NewOwner case). ordering of ENSv2ParentUpdatedvsLabelRegistered/SubregistryUpdated— handlers are order-insensitive but the reconcile-on-pointer-write pattern is the load-bearing piece. ordering ofhandleBridgedResolverChangevs Protocol Acceleration's DRR overwrite — enforced inregister-handlers.ts. assumption that the canonical namegraph is a tree (each registry has at most one canonical parent domain) — enforced by the bidirectional agreement check; if a cycle is ever introduced the recursive CTE'sUNION(notUNION ALL) prunes duplicates and termination is preserved.handleBridgedResolverChangeinvariants (1:1 + event ordering) are documented inline with a fix sketch if either is ever violated.Pre-Review Checklist (Blocking)