feat: materialize canonicality in the index, expose in omnigraph#2061
Open
feat: materialize canonicality in the index, expose in omnigraph#2061
Conversation
🦋 Changeset detectedLatest commit: aad2f1c 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.
3 Skipped Deployments
|
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 Walkthrough📝 Walkthrough🚥 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)
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 replaces runtime recursive CTE traversal from
Confidence Score: 5/5Safe to merge; bidirectional invariant maintenance is thorough, the handler registration order is structurally enforced, and the query-layer simplification is a strict improvement. Re-index required per Ponder semantics. setRegistryCanonicalDomain correctly maintains both halves of the bidirectional edge, ensureDomainInRegistry seeds domain.canonical from the parent registry at insertion time, and updateRegistryCanonicality propagates flips through the registryDomains child list. The ParentUpdated stub-insert handles out-of-order event delivery without stale state. No logic gaps were found after tracing LabelRegistered, ParentUpdated, SubregistryUpdated, and the bridged-resolver attach/detach paths. canonicality-db-helpers.ts is the highest-risk file — the dislodge path in setRegistryCanonicalDomain is load-bearing and the unconditional cascade of already-non-canonical dislodged registries is a minor inefficiency worth monitoring as registry sizes grow. Important Files Changed
Sequence DiagramsequenceDiagram
participant EV as On-chain Event
participant PA as Protocol Acceleration
participant V2 as ENSv2 Plugin
participant NM as NodeMigration
participant CH as canonicality-db-helpers
participant DB as Database
Note over NM,V2: Registration order: NodeMigration → ENSv2 → ProtocolAcceleration
EV->>NM: NewOwner (ENSv1Registry)
NM->>DB: migrateNode(parentNode, label)
EV->>V2: LabelRegistered (ENSv2Registry)
V2->>DB: ensureRegistry(registryId, canonical=isRoot)
V2->>DB: insert domain (onConflictDoUpdate tokenId)
V2->>CH: ensureDomainInRegistry(registryId, domainId)
CH->>DB: read registry.canonical → set domain.canonical
EV->>V2: ParentUpdated (child ENSv2Registry)
V2->>DB: ensureRegistry(thisRegistry, stub)
V2->>DB: ensureRegistry(parentRegistry, stub)
V2->>DB: insert parentDomain stub (onConflictDoNothing)
V2->>CH: ensureDomainInRegistry(parentRegistry, parentDomain)
V2->>CH: setRegistryCanonicalDomain(thisRegistry, parentDomain)
CH->>DB: clear prevCanonicalDomain.canonicalSubregistryId
CH->>DB: dislodge prevRegistryUnderNewDomain (if any)
CH->>DB: registry.canonicalDomainId = parentDomain
CH->>DB: domain.canonicalSubregistryId = thisRegistry
CH->>CH: updateRegistryCanonicality(thisRegistry, newCanonical)
CH->>DB: cascade canonical flag to subtree
EV->>V2: NewResolver (ENSv1Registry, pre-PA)
V2->>CH: handleBridgedResolverChange(registry, domainId, node, resolver)
CH->>DB: read prevDRR (before PA overwrites)
CH->>CH: isBridgedResolver(prevResolver) → detach if needed
CH->>CH: isBridgedResolver(newResolver) → attach if known bridge
EV->>PA: NewResolver (overwrites DRR)
PA->>DB: ensureDomainResolverRelation(...)
Reviews (5): Last reviewed commit: "fix: re-add MAX_DEPTH guard to getCanoni..." | 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.
| /** | ||
| * Idempotently link `domainId` into `registryId`'s child list and inherit `canonical` from the | ||
| * Registry. If the Domain is already linked, no-op (the cascade in | ||
| * {@link updateRegistryCanonicality} keeps existing children's `canonical` consistent). |
Contributor
There was a problem hiding this comment.
Concurrent event processing causes lost updates when adding children to a registry's domain list due to non-atomic read-modify-write pattern in Ponder's in-memory Store API
- Changeset wording: "non-null Boolean!" instead of "nullable Boolean". - domain.integration.test.ts: tighten Domain.canonical test type to boolean. - ENSv2Registry.ts: replace `tokenId as never` with TokenId-typed event arg and direct usage, matching the rest of the file. - register-handlers.ts: gate node-migration on ProtocolAcceleration only; add comment noting that ProtocolAcceleration is a hard requirement of the ENSv2 plugin so the OR was redundant. - get-canonical-path.ts and updateRegistryCanonicality: drop the fixed depth caps. ENS names have no formal depth limit, so a fixed cap would silently truncate or abort indexing on legitimately deep namegraphs. Termination now relies on the canonical-namegraph-is-a-tree invariant; if that invariant is violated, both call sites would loop indefinitely — accepted trade-off, called out in inline comments.
ensapi-layer code keeps depth caps so the API can fail loudly. The cap is detected by allowing the CTE to emit one row beyond MAX_DEPTH and throwing when that row appears, rather than silently truncating.
Member
Author
|
@greptile review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 30 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+41
| const existing = await context.ensDb.find(ensIndexerSchema.registryDomains, { registryId }); | ||
| if (existing?.domainIds.includes(domainId)) return; | ||
|
|
||
| const domainIds = existing ? [...existing.domainIds, domainId] : [domainId]; | ||
| await context.ensDb | ||
| .insert(ensIndexerSchema.registryDomains) | ||
| .values({ registryId, domainIds }) | ||
| .onConflictDoUpdate({ domainIds }); |
Comment on lines
+123
to
+151
| * Recursively flip `canonical` on `registryId` and every Domain in its child list (and their | ||
| * canonical subtrees). | ||
| * | ||
| * The recursion is unbounded by design. ENS names have no formal depth limit, so a fixed cap | ||
| * would abort indexing on legitimately deep namegraphs. Termination relies on the canonical | ||
| * namegraph being a tree (each Registry has at most one canonical parent Domain, enforced by | ||
| * the bidirectional invariant `Registry.canonicalDomainId` ↔ `Domain.canonicalSubregistryId`). | ||
| * If that invariant is ever violated and a cycle is introduced, this function could recurse | ||
| * indefinitely — that is an accepted trade-off for correctness on legitimately deep names. | ||
| */ | ||
| export async function updateRegistryCanonicality( | ||
| context: IndexingEngineContext, | ||
| registryId: RegistryId, | ||
| canonical: boolean, | ||
| ): Promise<void> { | ||
| await context.ensDb.update(ensIndexerSchema.registry, { id: registryId }).set({ canonical }); | ||
|
|
||
| const children = await context.ensDb.find(ensIndexerSchema.registryDomains, { registryId }); | ||
| if (!children) return; | ||
|
|
||
| for (const domainId of children.domainIds) { | ||
| // Read child once to capture its `canonicalSubregistryId` for the recursion (the field is | ||
| // independent of the `canonical` flag we're about to write, so a single PK read suffices). | ||
| const child = await context.ensDb.find(ensIndexerSchema.domain, { id: domainId }); | ||
| await context.ensDb.update(ensIndexerSchema.domain, { id: domainId }).set({ canonical }); | ||
|
|
||
| const childSubregistry = child?.canonicalSubregistryId ?? null; | ||
| if (childSubregistry) { | ||
| await updateRegistryCanonicality(context, childSubregistry, canonical); |
Comment on lines
13
to
55
| @@ -37,21 +41,20 @@ export async function getCanonicalPath(domainId: DomainId): Promise<CanonicalPat | |||
|
|
|||
| UNION ALL | |||
|
|
|||
| -- Step upward: domain -> current registry's canonical domain (parent). | |||
| -- 1. Recursion stops as soon as we reach a Root Registry or there is no parent to traverse. | |||
| -- 2. MAX_DEPTH guards against corrupted state. | |||
| -- 3. The pd.subregistry_id = upward.registry_id clause performs edge authentication. | |||
| -- Step upward: domain → current registry's canonical parent domain. | |||
| -- The bidirectional invariant guarantees consistency, so no edge-auth is needed. | |||
| -- We allow recursion to one row beyond MAX_DEPTH so we can detect (and throw on) a | |||
| -- legitimate path that exceeds the cap, rather than silently truncating it. | |||
| SELECT | |||
| pd.id AS domain_id, | |||
| pd.registry_id, | |||
| upward.depth + 1 | |||
| FROM upward | |||
| JOIN ${ensIndexerSchema.registryCanonicalDomain} rcd | |||
| ON rcd.registry_id = upward.registry_id | |||
| JOIN ${ensIndexerSchema.registry} r | |||
| ON r.id = upward.registry_id | |||
| JOIN ${ensIndexerSchema.domain} pd | |||
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "ensapi": minor | |||
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—Registry.canonicalDomainId↔Domain.canonicalSubregistryId, plus the cascade throughregistryDomains. this is where bugs live.ENSv2Registry.ParentUpdatedhandler. it's emitted from the child registry and may arrive before the parent registry'sLabelRegistered— handled via stub-insert + ensure-registry, but it's worth a second read.handleBridgedResolverChange. detach has to recover the prior target fromDomain.canonicalSubregistryIdbecause protocol-acceleration'sNewResolver/ResolverUpdatedhas already overwritten the DRR by the time we run.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.What Changed (Concrete)
Registry.canonical,Registry.canonicalDomainId,Domain.canonical,Domain.canonicalSubregistryId. dropregistryCanonicalDomain. addregistryDomains(per-registry child list, single row per registry) so the cascade walks children without scanningdomain.apps/ensindexer/src/lib/ensv2/canonicality-db-helpers.ts:setRegistryCanonicalDomain(writes both halves of the bidirectional edge, dislodges prior occupants, cascades),updateRegistryCanonicality(recursive flip overregistryDomains),ensureDomainInRegistry/removeDomainFromRegistry,handleBridgedResolverChange.apps/ensindexer/src/lib/ensv2/registry-db-helpers.ts:ensureRegistryupsert that seedscanonical = trueonly for the namespace's primary root registry.NewOwnerswitches toensureRegistry+setRegistryCanonicalDomain+ensureDomainInRegistry. ENSv1NewResolvernow invokeshandleBridgedResolverChange(event signature gainsresolver).LabelRegisteredswitches toensureRegistry+ensureDomainInRegistry. ENSv2SubregistryUpdatedkeeps the rawsubregistryId, ensures the registry row exists, leaves canonicality toParentUpdated. new ENSv2ParentUpdatedhandler — child registry claims its canonical parent, ensures parent rows exist (stub-insert if needed) before wiring the edge. new ENSv2ResolverUpdatedhandler invokeshandleBridgedResolverChange.getCanonicalPath: short-circuits ondomain.canonical = false, walksregistry.canonicalDomainIdupward with no root-set check and no edge-auth join.find-domains: deletecanonical-registries-cte.ts.filterByCanonicalis nowWHERE base.canonical = TRUE.base-domain-setderivesparentIdfromregistry.canonicalDomainIddirectly.filterByNamewalks via the materialized edge without edge-auth.getDomainByInterpretedName: delete theBridgedResolverrecursion branch — bridged attach is now an ordinary canonical edge thatwalkCanonicalNamegraphfollows.ENSv1Resolverfallback retained.canonical: BooleanonDomainandRegistryinterfaces (and all implementing types). regeneratedschema.graphqlandintrospection.ts.isBridgedResolversignature is now(namespace, resolver, originatingNode) => BridgedResolverRegistry | null. result is a discriminated union (ENSv1VirtualRegistry/ENSv2Registry) carrying everything needed to upsert the bridged registry. callers inforward-resolution.tsand theResolver.bridgesTographql resolver passENS_ROOT_NODEas a sentinel — onlychainId, addressare projected there.getRootRegistryIdsfrom@ensnode/ensnode-sdk; no consumers remain.getRootRegistryId(singular, primary root) retained.RegistryABI to latest (dropApprovalevent, rename anonymoustokenIdoutput).SearchView: drop the canonical-derivation disclaimer.Design & Planning
TODO(canonical-names)markers and the enskit disclaimer. materialization was the obvious next step once the bidirectional invariant could be maintained at index time.Self-Review
setRegistryCanonicalDomaininitially didn't dislodge a prior occupant on the new domain (i.e.Domain.canonicalSubregistryIdalready pointing at a different registry) — added the dislodge-and-cascade path.handleBridgedResolverChangeinitially read the prior target from the DRR but protocol-acceleration overwrites it first; switched to recovering fromDomain.canonicalSubregistryId, which only the bridged path writes for v1 originators.getDomainByInterpretedNamelost theBridgedResolverrecursion branch and the shadow/non-shadowpath.slicedistinction.getCanonicalPathlost the root-set check and theParam-bound array binding.base-domain-setandfilterByNamelost their edge-auth joins.registryCanonicalDomaintable →registry.canonicalDomainIdcolumn reads more directly.BridgedResolverTarget→BridgedResolverRegistryto reflect the richer return type.getRootRegistryIds,canonical-registries-cte.ts, theparentDomainalias, theregistryCanonicalDomaintable, twoTODO(canonical-names)blocks inENSv2Registry.Cross-Codebase Alignment
registryCanonicalDomain,getRootRegistryIds,BridgedResolverTarget,isBridgedResolver,canonical_domain.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: BooleanandRegistry.canonical: Boolean— additive on the omnigraph schema, nullable, safe.BridgedResolverTarget→BridgedResolverRegistry(internal sdk export).isBridgedResolversignature change is breaking for any in-tree caller; all updated. ensindexer schema changes — re-index required (implicit per ponder semantics).Testing Evidence
domain.integration.test.tsforDomain.pathstill passes; the alias-collapse case now relies on the materialized edge and its old comment about edge-auth in the reverse walk was removed (no longer accurate). newDomain.canonicalandRegistry.canonicalintegration tests assert v2-rooted domains/registries are canonical and v1-only fixtures (e.g. addr.reverse, the v1 root registry) are non-canonical. full integration suite + manual omnigraph queries against a freshly-indexed mainnet to confirm canonical filtering matches forward-resolution preference.ParentUpdatedvsSubregistryUpdatedandLabelRegistered. handlers are structured to be order-insensitive (ensure rows, then wire edge), but worth a second read.Scope Reductions
registryDomainsarray representation if.ethregistry scale makes append-rewrite painful (doubly-linked-list-per-edge is the alternative, documented inline); extendisBridgedResolverfor ENSv2 bridges when defined (detach is wired and works today; attach is currently unreachable via the ENSv2ResolverUpdatedpath).Risk Analysis
.eth) — the cascade is recursive and synchronous, hot-path cost grows with subtree size. ordering of v2ParentUpdatedvsLabelRegistered— handler is order-insensitive but the stub-insert path is the load-bearing piece. assumption that the canonical namegraph is a tree (each registry has at most one canonical parent domain) — enforced by the bidirectional invariant + dislodge-on-conflict; no cycle guard in the cascade.NOTE(child-list)documents the array-rewrite tradeoff.setRegistryCanonicalDomaininvariants throw loudly on missing rows.Pre-Review Checklist (Blocking)