feat: implement canonical filter for findDomains#1654
Open
Conversation
…ter in Queyr.domains and Account.domains. When specified, the resulting set of Domains is composed exclusively of Canonical Domains.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 5682d37 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new optional boolean Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as GraphQL API
participant DB as Postgres DB
Client->>API: Query Query.domains(where: {..., canonical: true})
API->>API: coerce input -> onlyCanonical
API->>DB: Execute SQL with canonical_registries CTE (seed = ENSv2 root registry id)
DB->>DB: Recursive CTE traverses registries and filters reachable v2 domains
DB-->>API: Return canonical domain rows (plus v1 rows)
API-->>Client: Return filtered domain results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
Greptile SummaryThis PR adds a Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[findDomains called with canonical filter] --> B{canonical === true?}
B -->|No| C[Return all domains]
B -->|Yes| D[Build recursive CTE]
D --> E[Base case: Start with ENSv2 Root Registry]
E --> F[Recursive: Find domains in canonical registries]
F --> G[Get subregistries of those domains via registry_canonical_domains]
G --> H{More registries found?}
H -->|Yes| F
H -->|No| I[Return set of canonical registries]
I --> J[Filter v2Domains by registry IN canonical set]
J --> K[Union with all v1Domains]
K --> L[Return filtered domains]
Last reviewed commit: d6a57fa |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a canonical filter to findDomains and the GraphQL where inputs, enabling callers to restrict ENSv2 results to registries reachable from the ENSv2 root via a recursive CTE.
Changes:
- Added optional
canonical: booleantoDomainsWhereInputandAccountDomainsWhereInput. - Extended
findDomainsto acceptcanonicaland apply an ENSv2 canonical-registry recursive CTE filter. - Added a changeset documenting the new filter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/ensapi/src/graphql-api/schema/domain.ts | Exposes canonical in GraphQL inputs for domain filtering. |
| apps/ensapi/src/graphql-api/lib/find-domains/types.ts | Adds canonical to the TypeScript where-arg type with doc comment. |
| apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts | Implements canonical filtering for ENSv2 domains via a recursive CTE. |
| .changeset/wide-trains-camp.md | Documents the API change in release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts (2)
136-164: 🧹 Nitpick | 🔵 TrivialPerformance: unbenchmarked recursive CTE on hot query path
The recursive
canonical_registriesCTE walksregistry_canonical_domain+v2Domainon everyfindDomainscall whereonlyCanonical: true. Without:
- An index on
registry_canonical_domain(registry_id)(the JOIN predicatecr.registry_id = parent.registry_id)- An index on
v2Domain(registry_id)for the outerIN (...)filter- Knowledge of how deep the registry tree can grow
…this could become a sequential-scan-heavy query at production scale. The PR explicitly acknowledges "No performance testing has been performed."
Ensure relevant columns are indexed to speed up join operations in recursive queries.
Before shipping to production, consider:
- Confirming indexes exist on
registry_canonical_domain.registry_idandv2_domain.registry_id.- Running
EXPLAIN ANALYZEwith a realistic dataset to check whether PostgreSQL materializes the CTE once or re-evaluates it.- Optionally adding a depth-counter column and
WHERE depth < Nguard in the recursive term (thoughUNIONdeduplication already prevents cycles, it doesn't bound traversal depth on an unexpectedly deep tree).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts` around lines 136 - 164, The recursive CTE canonical_registries in the onlyCanonical path can cause sequential scans at scale; ensure you add a b-tree index on registry_canonical_domain(registry_id) and on v2_domain(registry_id) (referenced by schema.registryCanonicalDomain and schema.v2Domain in the canonicalRegistryFilter), re-run EXPLAIN ANALYZE against a realistic dataset to confirm the CTE is materialized and performant, and optionally add a depth guard in the recursive term of canonical_registries (or other cycle/size protection) to bound traversal if the registry tree can be deep (see getENSv2RootRegistryId usage and canonicalRegistryFilter in find-domains.ts).
136-164: 🧹 Nitpick | 🔵 TrivialRefactor CTE to use schema-based column references instead of hardcoded SQL names for maintainability
The hardcoded column names in the CTE (
rcd.registry_id,rcd.domain_id,parent.id,parent.registry_id) currently match the schema's default camelCase-to-snake_case conversion, so the query works as intended. However, this approach is fragile: if anyone adds explicit SQL column name overrides (e.g.,text('registry_table_id')) to theregistryCanonicalDomainorv2Domainschema definitions, the CTE would silently produce wrong results or fail at runtime without compile-time detection.Consider refactoring to use schema-based column references (similar to how the table names are already interpolated via
${schema.registryCanonicalDomain}) to ensure type safety and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts` around lines 136 - 164, The CTE in canonicalRegistryFilter uses hardcoded column names (rcd.registry_id, rcd.domain_id, parent.id, parent.registry_id); change it to interpolate schema-based column references so column renames are respected—use ${schema.registryCanonicalDomain.<column>} and ${schema.v2Domain.<column>} (e.g., ${schema.registryCanonicalDomain.registryId}, ${schema.registryCanonicalDomain.domainId}, ${schema.v2Domain.id}, ${schema.v2Domain.registryId}) inside the sql template for canonicalRegistryFilter and retain the existing getENSv2RootRegistryId(config.namespace)::text cast; update canonicalRegistryFilter (and any alias refs like rcd/parent) to reference those interpolated symbols instead of hardcoded names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/wide-trains-camp.md:
- Line 5: Typo in the changelog: change "Queyr.domains" to "Query.domains" in
the .changeset description so it correctly references the Query.domains filter;
ensure the text mentions the added canonical?: boolean filter and
Account.domains consistently.
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Around line 138-150: Add tests in find-domains.test.ts that exercise the
onlyCanonical=true path: create test fixtures for schema.registryCanonicalDomain
and schema.v2Domain (including a root registry id from
getENSv2RootRegistryId(config.namespace)), insert parent/child registry
relationships that require the WITH RECURSIVE CTE to traverse, then call the
findDomains resolver (the code that builds canonicalRegistryFilter in
find-domains.ts) with onlyCanonical enabled and assert only domains from the
recursive canonical registry set are returned; ensure the test uses the
project's DB test helpers/transaction rollback so it runs as an integration test
and validates the IN( WITH RECURSIVE ... ) behavior end-to-end.
---
Outside diff comments:
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Around line 136-164: The recursive CTE canonical_registries in the
onlyCanonical path can cause sequential scans at scale; ensure you add a b-tree
index on registry_canonical_domain(registry_id) and on v2_domain(registry_id)
(referenced by schema.registryCanonicalDomain and schema.v2Domain in the
canonicalRegistryFilter), re-run EXPLAIN ANALYZE against a realistic dataset to
confirm the CTE is materialized and performant, and optionally add a depth guard
in the recursive term of canonical_registries (or other cycle/size protection)
to bound traversal if the registry tree can be deep (see getENSv2RootRegistryId
usage and canonicalRegistryFilter in find-domains.ts).
- Around line 136-164: The CTE in canonicalRegistryFilter uses hardcoded column
names (rcd.registry_id, rcd.domain_id, parent.id, parent.registry_id); change it
to interpolate schema-based column references so column renames are
respected—use ${schema.registryCanonicalDomain.<column>} and
${schema.v2Domain.<column>} (e.g., ${schema.registryCanonicalDomain.registryId},
${schema.registryCanonicalDomain.domainId}, ${schema.v2Domain.id},
${schema.v2Domain.registryId}) inside the sql template for
canonicalRegistryFilter and retain the existing
getENSv2RootRegistryId(config.namespace)::text cast; update
canonicalRegistryFilter (and any alias refs like rcd/parent) to reference those
interpolated symbols instead of hardcoded names.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/ensapi/src/graphql-api/lib/find-domains/find-domains-by-labelhash-path.ts`:
- Around line 140-141: The v2Domain schema is missing an index on subregistryId
causing sequential scans during the recursive JOINs; update the v2Domain index
block in ensv2.schema.ts (the schema that defines v2Domain) to add an index
entry named like bySubregistry that indexes t.subregistryId (e.g., add
bySubregistry: index().on(t.subregistryId) to the v2Domain indexes), then run
migrations/seed as appropriate so queries using v2Domain.subregistry_id use the
new index.
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Around line 136-156: The CTE built in the canonicalRegistries query uses UNION
which already deduplicates, so remove the redundant DISTINCT in the final
SELECT; update the SQL in the canonicalRegistries construction (the SELECT
DISTINCT registry_id FROM canonical_registries part) to just SELECT registry_id,
keeping the rest of the CTE and joins in the canonicalRegistries variable and
preserving getENSv2RootRegistryId(config.namespace) and schema references.
apps/ensapi/src/graphql-api/lib/find-domains/find-domains-by-labelhash-path.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/graphql-api/lib/find-domains/find-domains.test.ts`:
- Around line 3-5: Remove the unused import ENSNamespaceIds and the
vi.mock("@/config", ...) call from find-domains.test.ts because isEffectiveDesc
only depends on its parameters (orderDir, inverted) and does not read config;
update the test file by deleting the line importing ENSNamespaceIds and the
vi.mock(...) line so only the relevant test code and imports for isEffectiveDesc
remain.
apps/ensapi/src/graphql-api/lib/find-domains/find-domains.test.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts
Outdated
Show resolved
Hide resolved
apps/ensapi/src/graphql-api/lib/find-domains/find-domains.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/graphql-api/lib/find-domains/find-domains.test.ts`:
- Around line 3-5: The current test mocks "@/config" using the imported
ENSNamespaceIds inside the vi.mock factory which violates Vitest hoisting rules;
update the mock in find-domains.test.ts to avoid referencing imported bindings
inside vi.mock: either inline the namespace string value (remove the
ENSNamespaceIds import) or use vi.hoisted to require ENSNamespaceIds before
calling vi.mock so the factory can safely reference ENSNamespaceIds.Mainnet;
ensure the mock still returns { default: { namespace: ... } } to satisfy
find-domains.ts module-level import.
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Around line 136-156: Add a depth guard to the recursive CTE that builds
canonicalRegistries: modify the CTE in find-domains.ts (the SQL block used to
create canonicalRegistries) to include a depth column (start depth = 0 for the
root row, increment depth + 1 in the recursive SELECT) and add a WHERE clause
like "WHERE depth < FIND_DOMAINS_MAX_DEPTH" on the recursive branch (or on the
final selection) using the existing FIND_DOMAINS_MAX_DEPTH constant referenced
earlier; ensure the outer Drizzle select still projects only registry_id (drop
or ignore the depth column there) so
canonicalRegistries.as("canonical_registries") returns the same shape while
preventing unbounded recursion.
In `@packages/ensnode-schema/src/schemas/ensv2.schema.ts`:
- Line 224: The current index definition bySubregistry indexes nullable
subregistryId leading to many NULL entries; change it to a partial index that
only includes non-NULL values by using a WHERE clause (e.g., use
sql`${t.subregistryId} IS NOT NULL`) on the index definition for bySubregistry
and import sql from drizzle-orm; update the index declaration in ensv2.schema.ts
(reference: bySubregistry and subregistryId) to use the .where(...)
partial-index approach so NULL subregistryId rows are excluded.
apps/ensapi/src/graphql-api/lib/find-domains/find-domains.test.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… in favor of `Query.domains` with `owner` specified.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/dull-rabbits-take.md:
- Line 2: The changeset currently labels "ensapi" as minor while the body states
a BREAKING change (removal of Account.domains); update the bump to major or
explicitly confirm rationale if this package is pre-1.0.0: either change the
changeset entry from "ensapi": minor to "ensapi": major to reflect the breaking
API change (removal of Account.domains) or add a clear justification in the
changeset body stating that ensapi is <1.0.0 and you intend to use minor for
breaking changes; ensure the change references the removed symbol
Account.domains and that the final changeset accurately matches semver intent.
In `@apps/ensapi/src/graphql-api/lib/canonical-registries-cte.ts`:
- Around line 30-40: The CTE currently selects (registry_id, depth) so UNION
dedupes on the pair and does not provide true cycle protection; change the
recursive CTE canonical_registries to only track registry_id: define
canonical_registries(registry_id) and have both the initial SELECT (use
getENSv2RootRegistryId(config.namespace)::text) and the recursive SELECT from
schema.registryCanonicalDomain JOIN schema.v2Domain return only rcd.registry_id,
and remove references to depth and the WHERE cr.depth <
CANONICAL_REGISTRIES_MAX_DEPTH clause so UNION can terminate on registry_id
alone (alternatively, if you want to keep depth as a cap, replace UNION with
UNION ALL and apply DISTINCT on the final SELECT).
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Around line 147-154: getCanonicalRegistriesCTE() is being called
unconditionally which causes getENSv2RootRegistryId(...) to run even when
onlyCanonical is false; change the code so the CTE is only created when needed
by moving the call into the conditional branch (i.e., call
getCanonicalRegistriesCTE() inside the true branch where v2DomainsBaseQuery is
innerJoined) or lazily assign canonicalRegistries = onlyCanonical ?
getCanonicalRegistriesCTE() : undefined and then use it in the ternary for
v2DomainsBase, referencing the existing symbols getCanonicalRegistriesCTE,
onlyCanonical, v2DomainsBaseQuery, and v2DomainsBase.
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 332-335: The schema's `canonical` field in domain.ts documents
"defaults to false" but is nullable with no default; update the GraphQL input
field `canonical` (in the domain.ts Pothos object) to include defaultValue:
false so the schema matches the description and callers receive false when
omitted; alternatively, if you prefer nullable behavior, change the field
description to remove "defaults to false" to match current semantics (note: the
find-domains logic that uses `canonical === true` already handles nulls).
---
Duplicate comments:
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Around line 82-84: Tests do not cover the findDomains({ canonical: true })
path (the onlyCanonical branch), so add a test that calls findDomains with
canonical set to true and asserts the returned query/result respects the
canonical-only filter; locate the logic in findDomains (variable onlyCanonical)
and either mock the underlying data layer or provide a fixture with both
canonical and non-canonical domains, then verify only canonical entries are
returned, ensuring the XOR tests remain unchanged while exercising the
canonical=true path.
In `@packages/ensnode-schema/src/schemas/ensv2.schema.ts`:
- Line 224: The partial index definition using bySubregistry with
index().on(t.subregistryId).where(sql`${t.subregistryId} IS NOT NULL`) is
correct and requires no change; keep the current line (bySubregistry:
index().on(t.subregistryId).where(sql`${t.subregistryId} IS NOT NULL`)) as it
properly excludes rows with NULL subregistryId.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
1 participant
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.
Summary
canonical: booleantoDomainsWhereInputGraphQL inputcanonical: true, v2 domains are filtered to those whose registry is reachable from the ENSv2 root registry via a recursive CTE overregistry_canonical_domainsWhy
findDomainsshould support thisTesting
Notes for Reviewer
IN (WITH RECURSIVE ...)was generated by claudeChecklist