refactor: unified polymorphic domain + registry#1983
Open
Conversation
Split `RegistryId` into a union of `ENSv1RegistryId`, `ENSv1VirtualRegistryId`,
and `ENSv2RegistryId`. Add corresponding `makeENSv1RegistryId`,
`makeENSv2RegistryId`, and `makeENSv1VirtualRegistryId` constructors, and keep
`makeRegistryId` as a union-returning helper for callsites that genuinely can't
narrow (e.g. client-side cache key reconstruction).
Reshape `ENSv1DomainId` from `Node` to `${ENSv1RegistryId}/${node}` so ENSv1
domains are addressable in the same namegraph model as ENSv1VirtualRegistry.
`makeENSv1DomainId` now takes `(AccountId, Node)` — breaking change for all
callers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the split `v1_domains` + `v2_domains` tables with a single polymorphic `domains` table keyed by `DomainId` and discriminated by `domainType` enum (`"ENSv1Domain"` | `"ENSv2Domain"`). Drop `domain.parentId`; ENSv1 parent traversal now flows through `registryCanonicalDomain` uniformly with ENSv2. `tokenId` becomes nullable (non-null iff ENSv2). Make `registries` polymorphic: add `registryType` enum (`"ENSv1Registry"` | `"ENSv1VirtualRegistry"` | `"ENSv2Registry"`), add nullable `node` column (non-null iff virtual), replace the unique `(chainId, address)` constraint with a plain index so virtual Registries keyed by node can share (chainId, address) with their concrete parent. Widen `registryCanonicalDomain.domainId` from `ENSv2DomainId` to the unified `DomainId`. Add `getENSv1RootRegistryId` / `maybeGetENSv1RootRegistryId` / `maybeGetENSv1Registry` helpers mirroring the v2 equivalents; narrow v2 helpers to use `makeENSv2RegistryId`. Update the ensdb-sdk drizzle test to reference the unified `domain` export. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend `managed-names.ts`: `CONTRACTS_BY_MANAGED_NAME` now maps `Name` to
`{ registry, contracts }`, `getManagedName(contract)` returns
`{ name, node, registry }` so any Registrar / Controller / NameWrapper handler
can resolve the concrete ENSv1 Registry that governs its namegraph. Add the
ENS Root (`""`) Managed Name group covering the mainnet ENSv1Registry and
ENSv1RegistryOld; include each shadow Registry (Basenames, Lineanames) in its
respective Managed Name group. Groups for namespaces that don't ship a given
shadow Registry are omitted entirely.
ENSv1 `handleNewOwner`: upsert the concrete ENSv1 Registry row, pick
`parentRegistryId` as the concrete Registry when `parentNode` is the Managed
Name and as an `ENSv1VirtualRegistry` keyed by `parentNode` otherwise. When
the parent is virtual, also upsert the virtual Registry row and the
`registryCanonicalDomain` self-link so reverse traversal works uniformly with
ENSv2. Combine domain upsert with `rootRegistryOwner` update into one query
via `onConflictDoUpdate`. Canonicalize ENSv1Registry / ENSv1RegistryOld events
through `getManagedName(...).registry` — ENSRegistryWithFallback proxies
reads, so both contracts face the same logical namegraph and should write into
the same Registry ID.
All remaining v1 handlers (Transfer / NewTTL / NewResolver, BaseRegistrar,
NameWrapper, RegistrarController, protocol-acceleration ENSv1Registry /
ThreeDNSToken) update to the two-arg `makeENSv1DomainId(registry, node)`.
ENSv2 `handleRegistrationOrReservation`: switch `makeRegistryId` to
`makeENSv2RegistryId`, add `type: "ENSv2Registry"` to the Registry insert and
`type: "ENSv2Domain"` to the Domain insert.
Update `domain-db-helpers.materializeENSv1DomainEffectiveOwner` to write
through the unified `domain` table. Update the managed-names test to assert
the new `registry` return field.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Context & loaders
- Drop `v1CanonicalPath` + `v2CanonicalPath` loaders in favour of a single
`canonicalPath` loader backed by `getCanonicalPath(domainId)`.
Canonical path
- Replace `getV1CanonicalPath` + `getV2CanonicalPath` with a single recursive
CTE over `domain` + `registryCanonicalDomain`. Recursion terminates naturally:
roots have no `registryCanonicalDomain` entry, so the JOIN fails when we
reach one. Canonicality is decided by the final `tld.registry_id === root`
check. MAX_DEPTH guards against corrupted state.
Interpreted-name lookup (`get-domain-by-interpreted-name.ts`)
- Collapse the ENSv1 / ENSv2 branches into one `traverseFromRoot(root, name)`
helper. Both lineages hop via `domain.subregistryId` (ENSv1 Domains now set
this to their managed VirtualRegistry, symmetric with ENSv2 domains' declared
subregistries). The starting root picks v1 vs v2 lineage; v1 and v2 registry
IDs are disjoint, so no cross-contamination.
Find-domains layers
- `base-domain-set.ts`: single select over `domain`; `parentId` derived via
`registryCanonicalDomain` uniformly for v1 and v2.
- `filter-by-registry.ts`: simplify comment (no v1/v2 distinction).
- `filter-by-canonical.ts`: all domains have a `registryId` now; canonicality
reduces to `INNER JOIN` against the canonical-registries CTE.
- `filter-by-name.ts`: collapse `v1DomainsByLabelHashPath` +
`v2DomainsByLabelHashPath` into one CTE over `registryCanonicalDomain`.
- `canonical-registries-cte.ts`: union v1 + v2 roots as base cases; recursive
step uses `d.subregistry_id` uniformly.
Schemas
- `schema/domain.ts`: `DomainInterfaceRef` becomes a loadable interface with a
single `ensDb.query.domain.findMany` loader. `DomainInterface = Omit<Domain,
"tokenId" | "node" | "rootRegistryOwnerId">`. Variant types tightened via
`RequiredAndNotNull` / `RequiredAndNull` to encode invariants
(`ENSv1Domain.{node: Node, tokenId: null}`;
`ENSv2Domain.{tokenId: bigint, node: null, rootRegistryOwnerId: null}`).
`parent` moves onto the interface via `ctx.loaders.canonicalPath`; expose
`ENSv1Domain.node` as a first-class GraphQL field.
- `schema/registry.ts`: new `RegistryInterfaceRef` with `ENSv1Registry`,
`ENSv1VirtualRegistry`, `ENSv2Registry` implementations; shared fields
(`id`, `type`, `contract`, `parents`, `domains`, `permissions`). `parents`
uses `eq(domain.subregistryId, parent.id)` for virtual v1 and v2 (both set
`subregistryId`), and `eq(domain.registryId, parent.id)` for concrete v1.
`ENSv1VirtualRegistryRef` exposes `node: Node`.
- `schema/query.ts`: `registry(by: {contract})` does a DB lookup filtered by
`type IN (ENSv1Registry, ENSv2Registry)` — virtual Registries share
`(chainId, address)` with their concrete parent and aren't addressable via
contract alone. Dev-only `v1Domains` / `v2Domains` filter by `d.type`.
- Swap `RegistryRef` → `RegistryInterfaceRef` in `query.ts` and
`registry-permissions-user.ts`.
- `schema/registration.ts`: `WrappedBaseRegistrarRegistration.tokenId` loads
the domain via the `DomainInterfaceRef` dataloader and reads `domain.node`.
Supporting changes
- `ensdb-sdk` schema: add `domain.node: Hex` (non-null iff ENSv1Domain).
- `ensindexer` ENSv1 `handleNewOwner`: write `node` on domain upsert and set
parent domain's `subregistryId` to the VirtualRegistry when upserting it
(so forward traversal + canonical-registries CTE work uniformly with v2).
- `ensnode-sdk`: add `RequiredAndNull<T, K>` helper type (symmetric to
`RequiredAndNotNull`) for encoding "null in this variant" invariants.
Regenerate pothos generated files (`schema.graphql`, `introspection.ts`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: ba61cee The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📝 WalkthroughWalkthroughUnifies ENSv1/ENSv2 domain and registry models, consolidates canonical-path and domain-finding into unified traversals/loaders, updates ID constructors and indexer handlers to be registry-aware, and adapts GraphQL schema/loaders and tests to the polymorphic domain/registry model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL as "Omnigraph API"
participant Loaders as "Context Loaders\n(canonicalPath)"
participant DB as "ensIndexer DB\n(domains, registries)"
Client->>GraphQL: Query domain / root / registry
GraphQL->>Loaders: load canonicalPath(domainId)
Loaders->>DB: recursive traversal on `domains` and registryCanonicalDomain
DB-->>Loaders: canonical path or null
Loaders-->>GraphQL: CanonicalPath | null
GraphQL-->>Client: resolved Domain/Registry response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 refactors the ENS data model and API surface to unify ENSv1 + ENSv2 domains into a single polymorphic domain table, introduces a polymorphic Registry model (including ENSv1 virtual registries), and updates the indexer + ENS API to traverse/query the unified namegraph—targeting correctness for multi-registry conflicts (#205) and improving find-domains architecture/perf (#1877) while advancing DomainId terminology (#1511).
Changes:
- Unifies DB schema from
v1Domain/v2Domainintodomain(typed by enum), and makesregistrypolymorphic (ENSv1 concrete / ENSv1 virtual / ENSv2). - Changes ENSv1 identifiers to be registry-qualified (
ENSv1DomainId = ${ENSv1RegistryId}/${node}) and adds new RegistryId constructors. - Updates indexer handlers + ENS API (GraphQL schema, loaders, find-domains layers, canonical path + interpreted-name traversal) to operate on the unified model and expose new GraphQL interfaces/fields (
Registryinterface,Domain.parent).
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates generated GraphQL schema: Domain.parent, ENSv1Domain.node, Registry becomes an interface with ENSv1/virtual/v2 implementations. |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Updates generated introspection JSON to match new interfaces/types. |
| packages/enssdk/src/lib/types/ensv2.ts | Introduces branded ENSv1RegistryId/ENSv2RegistryId/ENSv1VirtualRegistryId and changes ENSv1 domain id shape. |
| packages/enssdk/src/lib/ids.ts | Adds makeENSv1RegistryId / makeENSv2RegistryId / makeENSv1VirtualRegistryId; changes makeENSv1DomainId signature to include registry. |
| packages/ensnode-sdk/src/shared/types.ts | Adds RequiredAndNull TS helper used for discriminated polymorphic models. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Adds ENSv1 root registry id helpers and switches ENSv2 root helper to makeENSv2RegistryId. |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Updates schema cloning tests to reflect table rename (domain). |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Core schema refactor: new domain + polymorphic registry, new enums, updated relations/indexes, registryCanonicalDomain.domainId to unified DomainId. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Updates ENSv1 domain id creation to include registry. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Canonicalizes ENSv1 registry source and updates domain id creation to include registry. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Writes ENSv2 domains into unified domain table and inserts registries with polymorphic type. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.ts | Updates ENSv1 domain id creation to include registry from managed-name group. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Updates ENSv1 domain id creation to include registry for wrapper events. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Implements ENSv1 virtual registries + unified domain writes; canonicalizes ENSv1RegistryOld vs new registry. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Retargets reads/writes to unified domain table and updates ENSv1 id creation. |
| apps/ensindexer/src/lib/managed-names.ts | Expands managed-name mapping to include concrete registry per group; adds ENS root group and shadow registries. |
| apps/ensindexer/src/lib/managed-names.test.ts | Updates tests for new registry return from getManagedName. |
| apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts | Updates effective-owner materialization to write unified domain table. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Replaces Registry object with Registry interface + ENSv1/virtual/v2 object types; updates parent/domain connections. |
| apps/ensapi/src/omnigraph-api/schema/registry-permissions-user.ts | Switches to RegistryInterfaceRef. |
| apps/ensapi/src/omnigraph-api/schema/registration.ts | Updates wrapped token id resolution to load ENSv1 domain and use domain.node. |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Retargets v1/v2 domain list queries to unified table; updates Query.registry to resolve concrete registries via DB lookup. |
| apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts | Updates fixtures for ENSv1 id format (registry + node). |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Reworks Domain loader to unified domain table, adds Domain.parent, and updates ENSv1/ENSv2 type implementations. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Replaces separate v1/v2 lookup logic with unified forward traversal rooted at v1/v2 root registries. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Unifies canonical-path resolution into a single reverse-traversal CTE over domain + registryCanonicalDomain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Updates docs/semantics for unified registry filtering. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Updates docs to match unified model (behavior unchanged). |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Replaces v1/v2 union traversal with a single traversal over unified domain table. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Switches canonical filtering to require membership in canonical registries set. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Replaces v1/v2 union base set with a single unified domain-based base set. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Replaces ENSv2-only canonical registry traversal with unified traversal rooted at v1 root (+ v2 root if present). |
| apps/ensapi/src/omnigraph-api/context.ts | Consolidates canonical-path loaders into a single canonicalPath loader. |
| SPEC-domain-model.md | Adds/updates design spec for unified polymorphic domain + registry model and migration notes. |
| .changeset/unified-domain-model.md | Declares major bumps and documents breaking changes (schema + id formats + GraphQL). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Member
Author
|
@greptile re-review |
Contributor
Greptile SummaryThis PR refactors the data model to unify Several issues flagged in earlier review rounds remain open (event-ordering gaps with Confidence Score: 3/5The core model change is well-structured but multiple P1 issues from prior review rounds remain open and are load-bearing for the new unified traversal pipeline. Pre-existing P1s (onConflictDoUpdate drops registryId/node/subregistryId; shadow-registry canonical paths broken; filterByCanonical INNER JOIN sensitive to event ordering; ThreeDNS domain keying) are not introduced by this PR but are tightly coupled to the new unified pipeline. New code itself introduces only P2 items. apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts, apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts, apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Roots["Root Registries (getRootRegistryIds)"]
R1["ENSv1RegistryId\n(mainnet ENSv1Registry)"]
R2["ENSv1VirtualRegistryId\n(basenamesRegistry, base.eth.node)"]
R3["ENSv1VirtualRegistryId\n(lineanamesRegistry, linea.eth.node)"]
R4["ENSv2RegistryId\n(RootRegistry)"]
end
subgraph ENSv1["ENSv1 Namegraph (mainnet)"]
D_eth["domain: eth\ntype=ENSv1Domain\nregistryId=ENSv1RegistryId"]
VR_eth["ENSv1VirtualRegistry\n(ensRootRegistry, eth.node)"]
D_fooeth["domain: foo.eth\ntype=ENSv1Domain\nregistryId=VR(eth.node)"]
end
subgraph ENSv2["ENSv2 Namegraph"]
D_v2eth["domain: eth\ntype=ENSv2Domain\nregistryId=ENSv2RegistryId"]
ETHReg["ENSv2Registry\n(ETH subregistry)"]
end
subgraph Basenames["Basenames Namegraph"]
VR_base["ENSv1VirtualRegistry\n(basenamesRegistry, base.eth.node)"]
D_foobase["domain: foo.base.eth\ntype=ENSv1Domain"]
end
R1 --> D_eth
D_eth --> VR_eth
VR_eth --> D_fooeth
R2 --> VR_base
VR_base --> D_foobase
R4 --> D_v2eth
D_v2eth --> ETHReg
Reviews (9): Last reviewed commit: "fix: pr notes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors ENSNode’s domain/registry model into a unified polymorphic representation across ENSv1 + ENSv2, updating DB schema, ID formats, and Omnigraph GraphQL types/resolvers to better handle cross-registry conflicts and canonical traversal.
Changes:
- Unifies
v1Domain+v2Domaininto a singledomaintable (discriminated bytype) and makesRegistrypolymorphic (ENSv1 concrete, ENSv1 virtual, ENSv2). - Updates ID formats and constructors (notably
ENSv1DomainId→${ENSv1RegistryId}/${node}and new registry ID helpers). - Reworks canonical traversal / domain lookup and updates Omnigraph GraphQL schema + generated artifacts accordingly.
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates Omnigraph SDL: Registry becomes an interface; Domain.parent added; Query.allDomains added; Query.root becomes non-null. |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Regenerates introspection JSON to reflect new polymorphic Registry and unified Domain surface. |
| packages/enssdk/src/lib/types/ensv2.ts | Introduces branded ENSv1RegistryId/ENSv2RegistryId/ENSv1VirtualRegistryId and redefines RegistryId union; changes ENSv1DomainId to CAIP-shaped string. |
| packages/enssdk/src/lib/ids.ts | Adds new ID constructors (makeENSv1RegistryId, makeENSv2RegistryId, makeENSv1VirtualRegistryId, makeConcreteRegistryId); updates makeENSv1DomainId signature. |
| packages/ensnode-sdk/src/shared/types.ts | Adds RequiredAndNull<T, K> utility type. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Adds helpers for ENSv1 root IDs and introduces getRootRegistryId + getRootRegistryIds to enumerate canonical roots. |
| packages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.ts | Updates graphcache resolvers to handle Registry as an interface and resolve concrete typenames via cache probing. |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Updates tests for renamed/merged schema table (v1Domain → domain). |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Implements unified domain + polymorphic registry tables; updates relations and indexes accordingly. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Updates ENSv1 domain ID construction to include concrete registry. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Canonicalizes ENSv1 registry (old vs new) when computing ENSv1 domain IDs for resolver relations. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Writes registry.type, migrates inserts/updates/finds from v2Domain to unified domain, and uses ENSv2-specific registry IDs. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.ts | Updates ENSv1 domain ID construction to include concrete registry from managed-name group. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Updates ENSv1 domain ID construction (including wrapped tokenId→node case) to include concrete registry. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Refactors ENSv1 NewOwner/NewResolver/NewTTL flows to unified domain + virtual registry modeling and canonical path metadata. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Migrates lookups from v1Domain to unified domain and updates ENSv1 domain ID construction. |
| apps/ensindexer/src/lib/managed-names.ts | Refactors managed-name mapping to include concrete registry ownership metadata (and includes registry contracts in groups). |
| apps/ensindexer/src/lib/managed-names.test.ts | Updates tests for new registry return value and uses match-object assertions where appropriate. |
| apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts | Updates ENSv1 effective owner materialization to write unified domain table. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Converts Registry from object to loadable interface with ENSv1/ENSv1Virtual/ENSv2 implementations and shared fields. |
| apps/ensapi/src/omnigraph-api/schema/registry-permissions-user.ts | Updates registry field type to the new Registry interface. |
| apps/ensapi/src/omnigraph-api/schema/registration.ts | Fixes wrapped tokenId derivation by loading the ENSv1 domain and reading domain.node (since ENSv1DomainId is no longer the node). |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Replaces v1Domains/v2Domains dev methods with allDomains; makes root non-null and updates registry polymorphism behavior. |
| apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts | Updates tests for Query.root preference + registry polymorphism and ENSv1 domain IDs. |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Refactors to a unified loadable Domain interface backed by domain table; adds parent field via canonical path loader. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Adds/updates integration tests for canonical Domain.path semantics (leaf→root and alias collapsing). |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Rewrites forward traversal to operate over unified domain table and traverse from all configured root registries. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Collapses v1/v2 canonical path logic into a single reverse traversal over unified domain + registryCanonicalDomain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Updates docs/intent to reflect registry filtering over unified domain set. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Cleans up docs now that parent derivation is unified. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Replaces dual v1/v2 traversal queries with a unified reverse-walk via registryCanonicalDomain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Simplifies canonical filtering to an inner-join against the canonical registries CTE (no more v1/v2 split). |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Replaces unioned v1/v2 base sets with a single base set built from domain and canonical-parent derivation. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Rebuilds canonical registries CTE to traverse forward from all configured roots via domain.subregistryId. |
| apps/ensapi/src/omnigraph-api/context.ts | Replaces separate v1/v2 canonical-path dataloaders with a unified canonicalPath loader. |
| AGENTS.md | Adds test assertion style guidance and explicit “run generators” steps for OpenAPI/GraphQL schema changes. |
| .changeset/unified-domain-model.md | Documents breaking changes in IDs and the unified schema/interface redesign. |
| .changeset/query-root-nonnull.md | Documents Query.root becoming non-null and selecting preferred root registry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+125
to
+147
| ...(basenamesRegistry && { | ||
| "base.eth": { | ||
| registry: basenamesRegistry, | ||
| contracts: [ | ||
| basenamesRegistry, | ||
| maybeGetDatasourceContract(config.namespace, DatasourceNames.Basenames, "BaseRegistrar"), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Basenames, | ||
| "EARegistrarController", | ||
| ), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Basenames, | ||
| "RegistrarController", | ||
| ), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Basenames, | ||
| "UpgradeableRegistrarController", | ||
| ), | ||
| ].filter((c) => !!c), | ||
| } satisfies ManagedNameGroup, |
Comment on lines
+149
to
+163
| ...(lineanamesRegistry && { | ||
| "linea.eth": { | ||
| registry: lineanamesRegistry, | ||
| contracts: [ | ||
| lineanamesRegistry, | ||
| maybeGetDatasourceContract(config.namespace, DatasourceNames.Lineanames, "BaseRegistrar"), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Lineanames, | ||
| "EthRegistrarController", | ||
| ), | ||
| lineanamesNameWrapper, | ||
| ].filter((c) => !!c), | ||
| } satisfies ManagedNameGroup, | ||
| }), |
Comment on lines
+62
to
+66
| // Canonicalize ENSv1Registry vs. ENSv1RegistryOld via `getManagedName(...).registry`. Both | ||
| // Registries share a Managed Name (the ENS Root for mainnet) and write into the same | ||
| // namegraph; canonicalizing here ensures Old events that pass `nodeIsMigrated` don't fragment | ||
| // domains across two Registry IDs. | ||
| const { node: managedNode, registry } = getManagedName(getThisAccountId(context, event)); |
…subregistry helpers
- New `packages/ensnode-sdk/src/shared/managed-names.ts` holds the
namespace-parameterized `getManagedName(namespace, contract)` and
`isNameWrapper(namespace, contract)` plus the `CONTRACTS_BY_MANAGED_NAME`
structure (memoized per namespace). `apps/ensindexer/src/lib/managed-names.ts`
becomes a thin wrapper that partially applies `config.namespace`.
- `getRootRegistryIds` now uses `getManagedName` directly (dropping the earlier
mocked cross-package dependency).
- Delete `packages/ensnode-sdk/src/registrars/{basenames,lineanames,ethnames}-subregistry.ts`
and drop their barrel exports. The sole consumer, `get-indexed-subregistries.ts`,
is rewritten to iterate (plugin × datasource) entries and derive managed-name
nodes via `getManagedName` — single source of truth.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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: 7
♻️ Duplicate comments (3)
AGENTS.md (1)
86-87:⚠️ Potential issue | 🟡 MinorPlease make schema/spec regeneration triggers explicit.
These two checklist items are still vague (“were affected”). Spell out concrete triggers (e.g., ENSApi route/request/response changes for OpenAPI; GraphQL schema/resolver/type-shape changes for gqlschema) so contributors know exactly when to run each command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 86 - 87, Update the two checklist items to replace vague "were affected" wording with explicit triggers: for the OpenAPI step (pnpm generate:openapi) list concrete changes that require regeneration such as modifications to ENSApi routes, request/response shapes, new or removed endpoints, or changes to JSDoc/OpenAPI annotations; for the GraphQL step (pnpm generate:gqlschema) list changes like alterations to schema definitions, resolver signatures, type shapes, added/removed queries or mutations, or updates to GraphQL directives—make the items actionable so contributors can determine when to run pnpm generate:openapi and pnpm generate:gqlschema.apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts (1)
40-44:⚠️ Potential issue | 🟡 MinorMinor:
reducewithout seed will throw on an emptyrootsarray.If
getRootRegistryIds(config.namespace)ever returns[](e.g. a misconfigured namespace), thereducewithout an initial value raisesTypeError: Reduce of empty array with no initial valuerather than a clear invariant error. In practice the ENSv1 root is always configured, so this is defensive hardening. Consider seeding the reduce or assertingroots.length > 0up front so the failure mode is explicit.🛡️ Proposed guard
const roots = getRootRegistryIds(config.namespace); + if (roots.length === 0) { + throw new Error( + "Invariant(getCanonicalRegistriesCTE): no Root Registries configured for namespace", + ); + } const rootsUnion = roots .map((root) => sql`SELECT ${root}::text AS registry_id, 0 AS depth`) .reduce((acc, part, i) => (i === 0 ? part : sql`${acc} UNION ALL ${part}`));Based on learnings: "Fail fast and loudly on invalid inputs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts` around lines 40 - 44, The code currently calls getRootRegistryIds(config.namespace) into roots and then reduces without a seed, which throws on an empty array; add a defensive check that roots.length > 0 and throw a clear Error (e.g. "No root registry ids for namespace: {config.namespace}") before building rootsUnion, or alternatively provide a safe seed to the reduce so it handles [] gracefully; reference getRootRegistryIds, roots, rootsUnion and config.namespace when adding the guard.apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts (1)
175-189:⚠️ Potential issue | 🟡 MinorTransfer handler assumes domain already exists; consider defensive checks or upsert pattern for consistency.
The
.update()call silently no-ops if the domain doesn't exist. While ENS v1 Registry guarantees that NewOwner fires before Transfer at the blockchain level, and Ponder'somnichainordering preserves this event order, defensive existence checks are used elsewhere in the codebase (e.g., BaseRegistrar.ts checksif (domain)before updates). For consistency and clarity of intent, either add an existence check before updating, or use an upsert pattern like NewOwner does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts` around lines 175 - 189, The ENSv1 Registry transfer handler uses context.ensDb.update(...).set(...) which no-ops if the Domain row doesn't exist; modify the handler around makeENSv1DomainId/getManagedName so you either (a) read the Domain first (e.g., select by id created with makeENSv1DomainId) and guard the update with if (domain) to match patterns in BaseRegistrar.ts, or (b) replace the update with an upsert pattern (create-or-update) like NewOwner does; ensure you still call ensureAccount(owner) and use the same ownerId/rootRegistryOwnerId fields when performing the guarded update or upsert so behavior remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts`:
- Line 68: The runtime error comes from calling isAddressEqual(zeroAddress,
parentNode) where parentNode is a Node (bytes32); replace this check with a
direct Node equality against ENS_ROOT_NODE (i.e. compare parentNode ===
ENS_ROOT_NODE) to determine isTLD; update the symbol usage in ENSv1Registry
where isTLD is set (replace isAddressEqual with the ENS_ROOT_NODE comparison) so
NewOwner handling no longer throws InvalidAddressError.
In `@packages/ensnode-sdk/src/shared/managed-names.ts`:
- Around line 188-202: The current lookup in getManagedName relies on
Object.entries(getContractsByManagedName(namespace)) and will pick the first
group containing a contract when a contract is accidentally registered in
multiple groups; fix this by enforcing uniqueness when building groups: update
getContractsByManagedName (or the code that constructs
MANAGED_NAME_BY_NAMESPACE) to assert a contract only appears in one managed-name
group (track seen contract identifiers and throw or log an error if a duplicate
is found), or alternatively add a duplicate-detection check at the start of
getManagedName that scans MANAGED_NAME_BY_NAMESPACE for the given contract and
throws if it is present in more than one managedName, so resolution is
deterministic and accidental double-registrations fail fast.
- Around line 129-144: Replace the conditional spread that uses a falsy
short-circuit (...(basenamesRegistry && { "base.eth": {...} })) with an explicit
ternary to avoid spreading undefined at compile-time and clearer intent: use
...(basenamesRegistry ? { "base.eth": { registry: basenamesRegistry, contracts:
[ basenamesRegistry, maybeGetDatasourceContract(namespace,
DatasourceNames.Basenames, "BaseRegistrar"),
maybeGetDatasourceContract(namespace, DatasourceNames.Basenames,
"EARegistrarController"), maybeGetDatasourceContract(namespace,
DatasourceNames.Basenames, "RegistrarController"),
maybeGetDatasourceContract(namespace, DatasourceNames.Basenames,
"UpgradeableRegistrarController"), ].filter((c): c is AccountId => !!c), }
satisfies ManagedNameGroup } : {}); and apply the same pattern for the analogous
lineanamesRegistry block; keep the same properties, filters, and the satisfies
ManagedNameGroup assertion and the maybeGetDatasourceContract calls to preserve
types and runtime behavior.
- Around line 49-54: MANAGED_NAME_BY_NAMESPACE is currently typed
Partial<Record<ENSNamespaceId, Record<Name, Name>>> which forces an explicit
cast to InterpretedName later (see the cast at the code using this map); change
the inner map type to Record<InterpretedName, InterpretedName> so the map
declaration enforces the invariant and you can remove the cast/comment at the
usage site. Update the declaration of MANAGED_NAME_BY_NAMESPACE to
Partial<Record<ENSNamespaceId, Record<InterpretedName, InterpretedName>>>
(keeping ENSNamespaceId) and ensure any entries (e.g., "base.eth"/"linea.eth")
conform to InterpretedName so no downstream cast is needed.
In `@packages/ensnode-sdk/src/shared/root-registry.ts`:
- Around line 146-152: The trailing comment token after v1RootRegistryId in the
returned array is vestigial; remove the dangling "//" following v1RootRegistryId
in the array literal (the return that builds [v1RootRegistryId,
basenamesRegistryId, lineanamesRegistryId, v2RootRegistryId].filter(...)) so the
element is cleanly written as v1RootRegistryId, without changing the surrounding
logic or the .filter((id): id is RegistryId => !!id) call.
- Around line 102-117: Fix the typo "Regsitry" → "Registry" in the docblock in
root-registry.ts and smooth the awkward sentence about Shadow Registries: update
the phrase "negating canonicality for any names under other names managed by
said Shadow Regsitry" to a clearer wording (for example: "thereby removing
canonical status for names managed under a different Shadow Registry") so the
intent is unambiguous; ensure references to the helper/getter remain intact (see
getRootRegistryId mention) and keep the rest of the comment intact.
- Around line 122-144: getRootRegistryIds is producing virtual registry IDs for
DatasourceNames.Basenames and DatasourceNames.Lineanames even though those
datasources never insert registry rows; stop emitting phantom IDs by skipping
creation of the virtual id for those datasources. In the block that calls
maybeGetDatasourceContract + makeENSv1VirtualRegistryId (refer to
getRootRegistryIds, maybeGetDatasourceContract, makeENSv1VirtualRegistryId,
getManagedName and DatasourceNames.Basenames/Lineanames), add a guard to return
null when the datasource is Basenames or Lineanames (or otherwise ensure only
datasources whose handlers create registry rows, e.g., the ENSv1Registry
handleNewOwner flow, produce an id). This ensures canonical-registries-cte
traversal starts only from real registry rows.
---
Duplicate comments:
In `@AGENTS.md`:
- Around line 86-87: Update the two checklist items to replace vague "were
affected" wording with explicit triggers: for the OpenAPI step (pnpm
generate:openapi) list concrete changes that require regeneration such as
modifications to ENSApi routes, request/response shapes, new or removed
endpoints, or changes to JSDoc/OpenAPI annotations; for the GraphQL step (pnpm
generate:gqlschema) list changes like alterations to schema definitions,
resolver signatures, type shapes, added/removed queries or mutations, or updates
to GraphQL directives—make the items actionable so contributors can determine
when to run pnpm generate:openapi and pnpm generate:gqlschema.
In `@apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts`:
- Around line 40-44: The code currently calls
getRootRegistryIds(config.namespace) into roots and then reduces without a seed,
which throws on an empty array; add a defensive check that roots.length > 0 and
throw a clear Error (e.g. "No root registry ids for namespace:
{config.namespace}") before building rootsUnion, or alternatively provide a safe
seed to the reduce so it handles [] gracefully; reference getRootRegistryIds,
roots, rootsUnion and config.namespace when adding the guard.
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts`:
- Around line 175-189: The ENSv1 Registry transfer handler uses
context.ensDb.update(...).set(...) which no-ops if the Domain row doesn't exist;
modify the handler around makeENSv1DomainId/getManagedName so you either (a)
read the Domain first (e.g., select by id created with makeENSv1DomainId) and
guard the update with if (domain) to match patterns in BaseRegistrar.ts, or (b)
replace the update with an upsert pattern (create-or-update) like NewOwner does;
ensure you still call ensureAccount(owner) and use the same
ownerId/rootRegistryOwnerId fields when performing the guarded update or upsert
so behavior remains consistent.
🪄 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: 4d25414e-7db0-4d31-bf7f-f2a617fe1634
⛔ Files ignored due to path filters (1)
packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (16)
AGENTS.mdapps/ensapi/src/lib/name-tokens/get-indexed-subregistries.tsapps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.tsapps/ensapi/src/omnigraph-api/lib/get-canonical-path.tsapps/ensapi/src/omnigraph-api/schema/query.integration.test.tsapps/ensapi/src/omnigraph-api/schema/registry.tsapps/ensindexer/src/lib/managed-names.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tspackages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.tspackages/ensnode-sdk/src/index.tspackages/ensnode-sdk/src/registrars/basenames-subregistry.tspackages/ensnode-sdk/src/registrars/ethnames-subregistry.tspackages/ensnode-sdk/src/registrars/index.tspackages/ensnode-sdk/src/registrars/lineanames-subregistry.tspackages/ensnode-sdk/src/shared/managed-names.tspackages/ensnode-sdk/src/shared/root-registry.ts
💤 Files with no reviewable changes (4)
- packages/ensnode-sdk/src/registrars/lineanames-subregistry.ts
- packages/ensnode-sdk/src/registrars/index.ts
- packages/ensnode-sdk/src/registrars/ethnames-subregistry.ts
- packages/ensnode-sdk/src/registrars/basenames-subregistry.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors ENSNode’s domain + registry model to be polymorphic and unified across ENSv1 and ENSv2, enabling consistent traversal/querying across L1/L2 shadow registries and improving canonical-path handling.
Changes:
- Unifies
v1Domain/v2Domaininto a singledomaintable (typed via enums) and makesRegistrypolymorphic (ENSv1 concrete, ENSv1 virtual-per-parent, ENSv2). - Updates ID formats + constructors (notably CAIP-shaped
ENSv1DomainIdand expandedRegistryIdunion) and threads these changes through indexer + API. - Updates Omnigraph GraphQL schema to reflect new polymorphism (
Domain.parent,Registryinterface,Query.allDomains, non-nullQuery.root).
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates public GraphQL schema for polymorphic Domain/Registry and new fields. |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Regenerates introspection to match the updated GraphQL schema. |
| packages/enssdk/src/lib/types/ensv2.ts | Refactors registry/domain ID brands; makes v1 IDs CAIP-shaped and expands RegistryId. |
| packages/enssdk/src/lib/ids.ts | Adds new ID constructors and updates makeENSv1DomainId signature. |
| packages/ensnode-sdk/src/shared/types.ts | Adds RequiredAndNull typing helper for discriminated polymorphic models. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Adds helpers for preferred root and enumerating all root registries (incl. shadow roots). |
| packages/ensnode-sdk/src/shared/managed-names.ts | Introduces namespace-aware “managed name” resolution shared across consumers. |
| packages/ensnode-sdk/src/registrars/lineanames-subregistry.ts | Removes now-redundant per-registrar helpers (replaced by managed-names). |
| packages/ensnode-sdk/src/registrars/index.ts | Stops exporting removed registrar helpers. |
| packages/ensnode-sdk/src/registrars/ethnames-subregistry.ts | Removes now-redundant per-registrar helpers (replaced by managed-names). |
| packages/ensnode-sdk/src/registrars/basenames-subregistry.ts | Removes now-redundant per-registrar helpers (replaced by managed-names). |
| packages/ensnode-sdk/src/index.ts | Exposes managed-names from SDK entrypoint. |
| packages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.ts | Updates Graphcache lookup logic for polymorphic registries (v1/v2/virtual). |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Updates schema tests for renamed/unified domain table. |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Unifies domain/registry tables and relations; adds type discriminators + v1 virtual registries. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Updates v1 domain ID construction to include registry context. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Canonicalizes v1 registry identity before constructing domain IDs. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Writes to unified domain table and inserts typed ENSv2Registry rows. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.ts | Updates v1 domain ID creation using managed-name registry context. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Updates v1 domain ID creation + registry canonicalization across wrapper events. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Refactors v1 NewOwner/Transfer flows to unified domain+registry model and virtual registries. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Updates v1 domain ID creation + unified domain-table lookups. |
| apps/ensindexer/src/lib/managed-names.ts | Wraps SDK managed-name helpers with indexer’s fixed namespace. |
| apps/ensindexer/src/lib/managed-names.test.ts | Updates managed-name tests to include concrete registry selection. |
| apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts | Updates effective-owner materialization to target unified domain table. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Converts Registry into a loadable interface with v1/v1-virtual/v2 implementations. |
| apps/ensapi/src/omnigraph-api/schema/registry-permissions-user.ts | Updates registry field typing to the new Registry interface. |
| apps/ensapi/src/omnigraph-api/schema/registration.ts | Fixes wrapped tokenId derivation under new CAIP-shaped v1 domain IDs. |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Adds allDomains dev connection, updates registry + makes root non-null using new root helper. |
| apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts | Updates integration tests for new root semantics and v1 domain ID format. |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Refactors Domain to a loadable interface over unified table; adds parent + v1 node field. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Adds integration tests for canonical path behavior and alias collapsing. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Updates forward traversal to search across multiple configured root registries. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Unifies canonical-path computation for v1/v2 via registry→canonical-domain traversal. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Updates registry filter semantics/comments for unified domain model. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Adjusts parent-filter commentary for unified parent derivation logic. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Consolidates v1/v2 name filtering into a single unified-domain recursive CTE. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Updates canonical filtering to rely on canonical registries set for unified domains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Switches base domain set to unified domain table and canonical-parent derivation via rcd. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Builds canonical registries set from all namespace roots (v1 + shadow + v2). |
| apps/ensapi/src/omnigraph-api/context.ts | Consolidates canonical-path dataloader to a single unified loader. |
| apps/ensapi/src/lib/name-tokens/get-indexed-subregistries.ts | Replaces registrar-specific helpers with managed-name based discovery. |
| AGENTS.md | Updates contributor guidance for tests and schema generation steps. |
| .changeset/unified-domain-model.md | Changeset documenting unified domain/registry + ID format breaking changes. |
| .changeset/query-root-nonnull.md | Changeset documenting Query.root becoming non-null. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
@greptile re-review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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)
55-82:⚠️ Potential issue | 🔴 CriticalForward traversal from Basenames/Lineanames virtual roots will not match — the label-path design is incompatible with managed-name seeding.
traverseFromRootrecursively matches labels frominterpretedLabelsToLabelHashPath, which produces root-first traversal:alice.base.eth→[labelHash("eth"), labelHash("base"), labelHash("alice")]. When the root isvirtualRegistry(basenamesRegistry, namehash("base.eth")), the first iteration at depth 1 attempts to matchlabelHash("eth")against domains whoseregistry_idequals that shadow-root id.However, Basenames domains indexed under this root are direct subdomains of
base.eth(e.g.,alice.base.ethhaslabelHash("alice")), not ancestor-traversal paths. The recursion fails on the first join because no domain withlabelHash("eth")exists in that subtree, returning null for any Basenames-only name.The issue applies identically to Lineanames. Since Basenames/Lineanames events are processed by the ensv2 plugin and populate the
domaintable withregistry_id = virtualRegistry(...), thegetDomainIdByInterpretedNamefunction cannot resolve names existing only in shadow registries, contradicting the goal of issue#205.To fix, either (a) trim the label path to exclude ancestors above the managed-name node for shadow roots, (b) pre-populate shadow-root ancestors (eth, base) in the
domaintable before label matching, or (c) use separate seeding logic for shadow roots that skips the ancestor chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts` around lines 55 - 82, getDomainIdByInterpretedName fails to resolve Basenames/Lineanames shadow-root domains because traverseFromRoot uses the full ancestor-first label-hash path (interpretedLabelsToLabelHashPath) which doesn't match virtualRegistry(...) subtrees; detect when a root is a shadow/virtual registry (use maybeGetENSv2RootRegistryId / virtualRegistry marker or inspect getRootRegistryIds' entries) and adjust traversal by trimming the label-hash path to start at the managed-name node (i.e., drop ancestor labels above the shadow root) before calling traverseFromRoot so the first label compared matches the direct children of that shadow root; update traverseFromRoot or the caller (getDomainIdByInterpretedName) to perform this slice and add a small unit test proving resolution for a basenames name like alice.base.eth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts`:
- Around line 73-80: The fallback that picks the first non-null v1 result is
non-deterministic when multiple v1 roots return a domain (variables roots,
results, v2Root, v2Hit in get-domain-by-interpreted-name.ts); implement a
deterministic resolution policy or open a tracked follow-up issue and reference
it here. Concretely, add a resolver function (e.g., chooseCanonicalV1Domain or
resolveBridgedDomain) that inspects candidate DomainId entries in results for
multiple v1 roots and applies Bridged Resolver logic (chain/registry
identifiers, bridged-vs-native preference, or explicit priority list) to pick
the correct v1 domain, then replace the current results.find(...) fallback with
that function; if you cannot implement now, create a follow-up issue describing
the required Bridged Resolver rules and update the TODO with the issue link.
In `@packages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.ts`:
- Around line 43-64: Replace the mixed existence checks in the registry resolver
with a consistent truthiness style: change the initial check "by.id !==
undefined" in the registry() function to use "if (by.id)" (matching the sibling
domain resolver), keep the later "if (id)" check as-is, and ensure
makeConcreteRegistryId is still only invoked when by.contract is truthy; update
only the checks around registry, virtualKey, v1Key, and v2Key to use the unified
truthiness pattern.
In `@packages/ensnode-sdk/src/shared/managed-names.ts`:
- Around line 200-216: isNameWrapper currently hard-codes two datasource paths
(ENSRoot/NameWrapper and Lineanames/NameWrapper) which duplicates the list in
getContractsByManagedName and risks drift; change isNameWrapper to derive its
membership from the managed-name group definition instead. Locate isNameWrapper
and replace the hard-coded checks
(getDatasourceContract/maybeGetDatasourceContract +
DatasourceNames.ENSRoot/Lineanames) with a lookup against the ManagedNameGroup
data returned by getContractsByManagedName (or a new wrappers: AccountId[] on
ManagedNameGroup), then check accountIdEqual against those returned wrapper
AccountIds so future wrapper additions are automatically included. Ensure you
reference the existing functions getContractsByManagedName and accountIdEqual
when implementing the lookup to preserve behavior.
- Around line 63-152: getContractsByManagedName rebuilds the full groups map on
every lookup causing per-event overhead in getManagedName; memoize its result
per ENSNamespaceId (e.g., use a module-level Map<ENSNamespaceId,
Record<Name,ManagedNameGroup>>) so repeated calls return the cached groups, and
optionally build and cache a reverse index Map<AccountId, { managedName,
registry }> to make getManagedName O(1); update getContractsByManagedName to
check the cache first, populate the cache on miss, and ensure any callers
(getManagedName in ENSv1Registry.ts) use the cached structure instead of
reconstructing groups each time.
---
Outside diff comments:
In `@apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts`:
- Around line 55-82: getDomainIdByInterpretedName fails to resolve
Basenames/Lineanames shadow-root domains because traverseFromRoot uses the full
ancestor-first label-hash path (interpretedLabelsToLabelHashPath) which doesn't
match virtualRegistry(...) subtrees; detect when a root is a shadow/virtual
registry (use maybeGetENSv2RootRegistryId / virtualRegistry marker or inspect
getRootRegistryIds' entries) and adjust traversal by trimming the label-hash
path to start at the managed-name node (i.e., drop ancestor labels above the
shadow root) before calling traverseFromRoot so the first label compared matches
the direct children of that shadow root; update traverseFromRoot or the caller
(getDomainIdByInterpretedName) to perform this slice and add a small unit test
proving resolution for a basenames name like alice.base.eth.
🪄 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: 83b2a291-0836-4874-aa73-41213b43dc0f
📒 Files selected for processing (8)
apps/ensapi/src/lib/name-tokens/get-indexed-subregistries.tsapps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.tsapps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.tspackages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.tspackages/ensnode-sdk/src/shared/managed-names.tspackages/ensnode-sdk/src/shared/root-registry.ts
Comment on lines
+73
to
+80
| logger.debug({ roots, results }); | ||
|
|
||
| v1Logger.debug({ domainId, exists }); | ||
|
|
||
| return exists ? domainId : null; | ||
| // prefer the v2 Root's result when present, otherwise the first non-null hit from any v1 root. | ||
| const v2Index = v2Root ? roots.indexOf(v2Root) : -1; | ||
| const v2Hit = v2Index >= 0 ? results[v2Index] : null; | ||
| // TODO: need to implement some resolution logic here to correctly choose which v1 domain ? | ||
| // i.e. differentiating between bridge.linea.eth on L1 and bridge.linea.eth on L2 requires Bridged Resolver logic... | ||
| return v2Hit ?? results.find((r): r is DomainId => r !== null) ?? null; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Resolve the TODO about v1-conflict resolution before merge, or open a follow-up issue.
The inline TODO at Line 78–79 acknowledges that when two ENSv1 roots both return a result for the same name (the bridge.linea.eth L1-vs-L2 case called out in issue #205), the current "first non-null hit" fallback is non-deterministic — it depends on the order of roots returned by getRootRegistryIds. Since issue #205 is one of the objectives this PR claims to close, please either implement the resolution policy, downgrade what #205 promises, or file a tracked follow-up issue and link it here so the TODO has an owner.
Want me to open a follow-up issue for Bridged-Resolver-aware canonical selection?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts` around
lines 73 - 80, The fallback that picks the first non-null v1 result is
non-deterministic when multiple v1 roots return a domain (variables roots,
results, v2Root, v2Hit in get-domain-by-interpreted-name.ts); implement a
deterministic resolution policy or open a tracked follow-up issue and reference
it here. Concretely, add a resolver function (e.g., chooseCanonicalV1Domain or
resolveBridgedDomain) that inspects candidate DomainId entries in results for
multiple v1 roots and applies Bridged Resolver logic (chain/registry
identifiers, bridged-vs-native preference, or explicit priority list) to pick
the correct v1 domain, then replace the current results.find(...) fallback with
that function; if you cannot implement now, create a follow-up issue describing
the required Bridged Resolver rules and update the TODO with the issue link.
Comment on lines
43
to
64
| registry(parent, args, cache, info) { | ||
| const by = args.by as { id?: RegistryId; contract?: AccountId }; | ||
|
|
||
| if (by.id) return { __typename: "Registry", id: by.id }; | ||
| if (by.contract) return { __typename: "Registry", id: makeRegistryId(by.contract) }; | ||
| // see if we have an ENSv1VirtualRegistry by id | ||
| if (by.id !== undefined) { | ||
| const virtualKey = cache.keyOfEntity({ __typename: "ENSv1VirtualRegistry", id: by.id }); | ||
| if (virtualKey && cache.resolve(virtualKey, "id")) return virtualKey; | ||
| } | ||
|
|
||
| // otherwise, fall back to concrete by id or contract | ||
|
|
||
| const id = by.id ?? (by.contract ? makeConcreteRegistryId(by.contract) : undefined); | ||
| if (id) { | ||
| const v1Key = cache.keyOfEntity({ __typename: "ENSv1Registry", id }); | ||
| if (v1Key && cache.resolve(v1Key, "id")) return v1Key; | ||
|
|
||
| const v2Key = cache.keyOfEntity({ __typename: "ENSv2Registry", id }); | ||
| if (v2Key && cache.resolve(v2Key, "id")) return v2Key; | ||
| } | ||
|
|
||
| return passthrough(args, cache, info); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — polymorphic registry lookup mirrors the domain resolver pattern.
Virtual-first, then concrete (ENSv1/ENSv2) lookup is correct: for concrete ids, cache.resolve(virtualKey, "id") returns nullish when no ENSv1VirtualRegistry entity is cached under that key, so we correctly fall through to the concrete branch. makeConcreteRegistryId is only invoked when by.contract is truthy, so no crash on missing inputs, and the terminal passthrough keeps network fallback behavior intact.
Optional nit: this function uses by.id !== undefined on line 47 but if (id) on line 55, while the sibling domain resolver uses if (by.id). Harmonizing to a single style would improve readability, but it's purely cosmetic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.ts` around
lines 43 - 64, Replace the mixed existence checks in the registry resolver with
a consistent truthiness style: change the initial check "by.id !== undefined" in
the registry() function to use "if (by.id)" (matching the sibling domain
resolver), keep the later "if (id)" check as-is, and ensure
makeConcreteRegistryId is still only invoked when by.contract is truthy; update
only the checks around registry, virtualKey, v1Key, and v2Key to use the unified
truthiness pattern.
Comment on lines
+63
to
+152
| const getContractsByManagedName = (namespace: ENSNamespaceId) => { | ||
| const ensRootRegistry = getDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.ENSRoot, | ||
| "ENSv1Registry", | ||
| ); | ||
| const ensRootRegistryOld = getDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.ENSRoot, | ||
| "ENSv1RegistryOld", | ||
| ); | ||
| const ethnamesNameWrapper = getDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.ENSRoot, | ||
| "NameWrapper", | ||
| ); | ||
|
|
||
| const basenamesRegistry = maybeGetDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.Basenames, | ||
| "Registry", | ||
| ); | ||
| const lineanamesRegistry = maybeGetDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.Lineanames, | ||
| "Registry", | ||
| ); | ||
| const lineanamesNameWrapper = maybeGetDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.Lineanames, | ||
| "NameWrapper", | ||
| ); | ||
|
|
||
| return { | ||
| [ENS_ROOT_NAME]: { | ||
| registry: ensRootRegistry, | ||
| contracts: [ensRootRegistry, ensRootRegistryOld], | ||
| }, | ||
| eth: { | ||
| registry: ensRootRegistry, | ||
| contracts: [ | ||
| getDatasourceContract(namespace, DatasourceNames.ENSRoot, "BaseRegistrar"), | ||
| getDatasourceContract(namespace, DatasourceNames.ENSRoot, "LegacyEthRegistrarController"), | ||
| getDatasourceContract(namespace, DatasourceNames.ENSRoot, "WrappedEthRegistrarController"), | ||
| getDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.ENSRoot, | ||
| "UnwrappedEthRegistrarController", | ||
| ), | ||
| getDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.ENSRoot, | ||
| "UniversalRegistrarRenewalWithReferrer", | ||
| ), | ||
| ethnamesNameWrapper, | ||
| ], | ||
| }, | ||
| ...(basenamesRegistry && { | ||
| "base.eth": { | ||
| registry: basenamesRegistry, | ||
| contracts: [ | ||
| basenamesRegistry, | ||
| maybeGetDatasourceContract(namespace, DatasourceNames.Basenames, "BaseRegistrar"), | ||
| maybeGetDatasourceContract(namespace, DatasourceNames.Basenames, "EARegistrarController"), | ||
| maybeGetDatasourceContract(namespace, DatasourceNames.Basenames, "RegistrarController"), | ||
| maybeGetDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.Basenames, | ||
| "UpgradeableRegistrarController", | ||
| ), | ||
| ].filter((c): c is AccountId => !!c), | ||
| }, | ||
| }), | ||
| ...(lineanamesRegistry && { | ||
| "linea.eth": { | ||
| registry: lineanamesRegistry, | ||
| contracts: [ | ||
| lineanamesRegistry, | ||
| maybeGetDatasourceContract(namespace, DatasourceNames.Lineanames, "BaseRegistrar"), | ||
| maybeGetDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.Lineanames, | ||
| "EthRegistrarController", | ||
| ), | ||
| lineanamesNameWrapper, | ||
| ].filter((c): c is AccountId => !!c), | ||
| }, | ||
| }), | ||
| } satisfies Record<Name, ManagedNameGroup>; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Memoize getContractsByManagedName(namespace) to avoid rebuilding the map on every lookup.
getManagedName is called from handlers on every NewOwner/Transfer/NewTTL/NewResolver event in ENSv1Registry.ts, and each call rebuilds the entire groups object — running ~10 getDatasourceContract/maybeGetDatasourceContract lookups plus allocating the group objects — before iterating via Object.entries. The groups map is a pure function of namespace, so caching per ENSNamespaceId (e.g. via a Map<ENSNamespaceId, Record<Name, ManagedNameGroup>>) would eliminate this per-event overhead. An even better shape would be a reverse index Map<AccountId-key, { managedName, registry }> so getManagedName becomes O(1) instead of O(groups × contracts) per event. This ties into the #1877 slowness objective of the PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensnode-sdk/src/shared/managed-names.ts` around lines 63 - 152,
getContractsByManagedName rebuilds the full groups map on every lookup causing
per-event overhead in getManagedName; memoize its result per ENSNamespaceId
(e.g., use a module-level Map<ENSNamespaceId, Record<Name,ManagedNameGroup>>) so
repeated calls return the cached groups, and optionally build and cache a
reverse index Map<AccountId, { managedName, registry }> to make getManagedName
O(1); update getContractsByManagedName to check the cache first, populate the
cache on miss, and ensure any callers (getManagedName in ENSv1Registry.ts) use
the cached structure instead of reconstructing groups each time.
Comment on lines
+200
to
+216
| export const isNameWrapper = (namespace: ENSNamespaceId, contract: AccountId): boolean => { | ||
| const ethnamesNameWrapper = getDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.ENSRoot, | ||
| "NameWrapper", | ||
| ); | ||
| if (accountIdEqual(ethnamesNameWrapper, contract)) return true; | ||
|
|
||
| const lineanamesNameWrapper = maybeGetDatasourceContract( | ||
| namespace, | ||
| DatasourceNames.Lineanames, | ||
| "NameWrapper", | ||
| ); | ||
| if (lineanamesNameWrapper && accountIdEqual(lineanamesNameWrapper, contract)) return true; | ||
|
|
||
| return false; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider deriving isNameWrapper from the managed-name group definition to avoid duplicate enumeration.
The NameWrapper contracts (ENSRoot/NameWrapper, Lineanames/NameWrapper) are already listed inside getContractsByManagedName. Hard-coding the same two datasource paths here means a future third NameWrapper (e.g. Basenames) has to be added in two places. A single source of truth (e.g. a wrappers: AccountId[] field on ManagedNameGroup, or a separate constant consumed by both) would avoid the drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensnode-sdk/src/shared/managed-names.ts` around lines 200 - 216,
isNameWrapper currently hard-codes two datasource paths (ENSRoot/NameWrapper and
Lineanames/NameWrapper) which duplicates the list in getContractsByManagedName
and risks drift; change isNameWrapper to derive its membership from the
managed-name group definition instead. Locate isNameWrapper and replace the
hard-coded checks (getDatasourceContract/maybeGetDatasourceContract +
DatasourceNames.ENSRoot/Lineanames) with a lookup against the ManagedNameGroup
data returned by getContractsByManagedName (or a new wrappers: AccountId[] on
ManagedNameGroup), then check accountIdEqual against those returned wrapper
AccountIds so future wrapper additions are automatically included. Ensure you
reference the existing functions getContractsByManagedName and accountIdEqual
when implementing the lookup to preserve behavior.
| lineanamesRegistry && | ||
| makeENSv1VirtualRegistryId( | ||
| lineanamesRegistry, | ||
| getManagedName(namespace, lineanamesRegistry).node, |
Contributor
There was a problem hiding this comment.
getRootRegistryIds() incorrectly creates virtual registry IDs for Basenames and Lineanames concrete registries
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.
closes #205
closes #1511
closes #1877