Feat/canonical name heuristic #1576
Open
+855
−232
Conversation
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
🦋 Changeset detectedLatest commit: 65d6ef9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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.
|
📝 WalkthroughWalkthroughAdds partial-name domain search across ENSv1/v2, separate v1/v2 canonical-path computation and per-request canonical-path dataloaders; exposes Domain.name/path and domains query filters; introduces interpretation utilities for partial names, schema/indexer/index changes, and registry→canonical-domain mappings. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Query as GraphQL Query
participant FindDomains as findDomains()
participant DB as Database
participant Loader as Dataloader
Client->>Query: domains(where: {name?, owner?})
Query->>FindDomains: findDomains({name, owner})
FindDomains->>FindDomains: parsePartialInterpretedName → interpretedLabelsToLabelHashPath
FindDomains->>DB: execute v1 recursive CTE (labelHashPath)
DB-->>FindDomains: v1 domain IDs
FindDomains->>DB: execute v2 recursive CTE (registryCanonicalDomain)
DB-->>FindDomains: v2 domain IDs
FindDomains->>FindDomains: unionAll(v1, v2) + filters + pagination
FindDomains-->>Query: paginated Domain IDs
Query->>Loader: load DomainInterfaceRef by IDs
Loader->>DB: fetch domain records
DB-->>Loader: domain data
Loader-->>Query: DomainInterfaceRef[]
Query-->>Client: domains connection
sequenceDiagram
participant Resolver as Domain.name Resolver
participant CanonicalPath as getV1CanonicalPath / getV2CanonicalPath
participant DB as Database
participant Loader as Dataloader
participant Interpreter as interpretedLabelsToInterpretedName
Resolver->>CanonicalPath: request canonical path for domainId
CanonicalPath->>DB: recursive CTE (v1 via parent_id, v2 via registryCanonicalDomain)
DB-->>CanonicalPath: ordered domain IDs (leaf→root)
CanonicalPath-->>Resolver: CanonicalPath[]
Resolver->>Loader: load domains for path IDs
Loader->>DB: fetch domain rows
DB-->>Loader: domain rows
Loader-->>Resolver: Domain objects
Resolver->>Interpreter: interpretedLabelsToInterpretedName(labels)
Interpreter-->>Resolver: Name
Resolver-->>Client: domain.name
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 implements a canonical name heuristic for ENS domains to support domain search and autocomplete functionality. Since ENSv2 canonical names are not yet fully implemented by the ENS team, this PR introduces a "last-write-wins" heuristic for tracking canonical domain references.
Changes:
- Updates ponder dependency from 0.16.1 to 0.16.2
- Adds
registryCanonicalDomainschema table to track registry-to-canonical-domain mappings using a heuristic approach - Implements separate canonical path resolution for ENSv1 (tree-based) and ENSv2 (graph-based with canonical domain tracking)
- Adds
Domain.nameandDomain.pathGraphQL fields to expose canonical names - Introduces
findDomainsfunction for domain search/filtering with partial name support (though implementation is incomplete) - Refactors SDK functions for better separation of concerns (e.g.,
interpretedLabelsToLabelHashPath)
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Updates ponder catalog version to 0.16.2 |
| pnpm-lock.yaml | Updates ponder lock entries to version 0.16.2 across all packages and snapshots |
| packages/ensnode-sdk/src/shared/interpretation/interpreted-names-and-labels.ts | Refactors interpretedLabelsToLabelHashPath to accept labels instead of name; adds utility functions for constructing interpreted names, ensuring interpreted labels, and parsing partial interpreted names |
| packages/ensnode-schema/src/schemas/ensv2.schema.ts | Adds registryCanonicalDomain table to track temporary canonical domain heuristic until ENS team implements proper canonical names |
| packages/datasources/src/ens-test-env.ts | Updates test environment contract addresses for ENSv1RegistryOld and ENSv1Registry |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Implements last-write-wins heuristic for canonical domains in SubregistryUpdated handler; removes debug console.log |
| apps/ensapi/src/graphql-api/schema/query.ts | Adds development-only Query.domains connection for testing domain search functionality |
| apps/ensapi/src/graphql-api/schema/domain.ts | Replaces commented-out canonical name fields with implemented Domain.name and Domain.path fields that use canonical path resolution |
| apps/ensapi/src/graphql-api/schema/account.ts | Refactors Account.domains to use new findDomains helper instead of inline query logic; removes unused unionAll import |
| apps/ensapi/src/graphql-api/lib/get-domain-by-fqdn.ts | Updates function call from interpretedNameToLabelHashPath to interpretedLabelsToLabelHashPath with proper parameter conversion |
| apps/ensapi/src/graphql-api/lib/get-canonical-path.ts | Splits canonical path logic into getV1CanonicalPath (tree-based) and getV2CanonicalPath (uses registry_canonical_domains table); adds ROOT_NODE import |
| apps/ensapi/src/graphql-api/lib/find-domains.ts | Adds new domain filtering/search function supporting name-based and owner-based queries (though name filtering for v2 and partial label matching remain unimplemented) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/ensnode-sdk/src/shared/interpretation/interpreted-names-and-labels.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts (1)
192-206: Clean up stale canonical mappings when subregistries change or are removed.If a subregistry is cleared or replaced, the previous
registryCanonicalDomainrow remains, so canonical-path resolution can keep treating the old registry as canonical. Capture the previoussubregistryIdand delete its mapping before updating/clearing.🛠️ Suggested fix (remove old mapping before update)
const registryAccountId = getThisAccountId(context, event); const canonicalId = getCanonicalId(tokenId); const domainId = makeENSv2DomainId(registryAccountId, canonicalId); + const existing = await context.db.find(schema.v2Domain, { id: domainId }); + const previousSubregistryId = existing?.subregistryId ?? null; // update domain's subregistry if (subregistry === null) { + if (previousSubregistryId) { + await context.db.delete(schema.registryCanonicalDomain, { + registryId: previousSubregistryId, + }); + } await context.db.update(schema.v2Domain, { id: domainId }).set({ subregistryId: null }); } else { const subregistryAccountId: AccountId = { chainId: context.chain.id, address: subregistry }; const subregistryId = makeRegistryId(subregistryAccountId); + if (previousSubregistryId && previousSubregistryId !== subregistryId) { + await context.db.delete(schema.registryCanonicalDomain, { + registryId: previousSubregistryId, + }); + } await context.db.update(schema.v2Domain, { id: domainId }).set({ subregistryId }); // TODO(canonical-names): this implements last-write-wins heuristic for a Registry's canonical name, // replace with real logic once ENS Team implements Canonical Names await context.dbpackages/ensnode-sdk/src/shared/interpretation/interpreted-names-and-labels.ts (1)
108-128: Update TypeScript target to ES2023 or use a compatible alternative.The code uses
Array.prototype.toReversed()(ES2023), buttsconfig.lib.jsondeclarestarget: "ES2022". While the project's minimum Node version (24.13.0) natively supports ES2023, this creates a configuration inconsistency. Either update the TypeScript target to ES2023 or replacetoReversed()with.reverse()to maintain ES2022 compatibility.🔧 Alternative for ES2022 compatibility
- .toReversed(); + .reverse();apps/ensapi/src/graphql-api/lib/get-canonical-path.ts (1)
61-63: Update invariant labels to the new helper names.The thrown error still references
getCanonicalPath, which no longer exists, making logs misleading.🛠️ Proposed fix
- if (rows.length === 0) { - throw new Error(`Invariant(getCanonicalPath): DomainId '${domainId}' did not exist.`); - } + if (rows.length === 0) { + throw new Error(`Invariant(getV2CanonicalPath): DomainId '${domainId}' did not exist.`); + } @@ - if (rows.length === 0) { - throw new Error(`Invariant(getCanonicalPath): DomainId '${domainId}' did not exist.`); - } + if (rows.length === 0) { + throw new Error(`Invariant(getV1CanonicalPath): DomainId '${domainId}' did not exist.`); + }Also applies to: 110-112
🤖 Fix all issues with AI agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts`:
- Around line 164-168: findV2Domains currently ignores the DomainFilter.name
(and canonical path), causing unbounded v2 results; update the function to apply
name/canonicalPath filtering (or return no rows when name is provided until
implemented). Specifically, in findV2Domains (which queries schema.v2Domain),
include an additional where clause that filters by eq(schema.v2Domain.name,
name) or eq(schema.v2Domain.canonicalPath, name) when DomainFilter.name is
present, combining with the existing owner filter (e.g., using and(...) or
building an array of conditions) so the query is constrained; alternatively, if
you prefer to fail closed until canonical-path logic exists, have the function
return an empty selection when name is provided.
- Around line 108-160: The function findV1DomainsByName must handle the case
where parsePartialInterpretedName(name) yields an empty concrete array; add an
early-return when concrete.length === 0 that returns all v1 domains (so
owner-only queries still return domains) instead of building the CTE. In
practice, inside findV1DomainsByName, check if concrete.length === 0 and
immediately return a simple query against schema.v1Domain (similar to the
commented-out snippet that selects schema.v1Domain.id), before computing
labelHashPath/rawLabelHashPathArray/pathLength or constructing the recursive
CTE.
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 124-129: The invariant error message inside the canonicalPath
mapping is still labeled "Domain.canonicalName"; update that message to
reference "Domain.name" instead to match the current model. Locate the mapping
over canonicalPath where domains.find((d) => d.id === domainId) is used and
change the thrown Error text (`Invariant(Domain.canonicalName): ...`) to
`Invariant(Domain.name): ...`, preserving the existing Path and DomainId
details.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
packages/ensnode-sdk/src/shared/interpretation/interpreted-names-and-labels.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 11 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/ensnode-sdk/src/shared/interpretation/interpreted-names-and-labels.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts`:
- Around line 121-130: The Promise.all block that runs
db.select().from(v1DomainsByName) and db.select().from(v2DomainsByName) (and
logs v1Domains.toSQL()/v2Domains.toSQL() and full results) should be removed or
gated behind a development/debug flag and protected with error handling; update
the code around the Promise.all(...) block so it only executes when a debug/dev
mode (e.g., process.env.NODE_ENV !== 'production' or a dedicated isDebug flag)
is true, await the queries inside a try/catch (or add .catch) to avoid unhandled
rejections, and avoid logging full result sets—log only the SQL
(v1Domains.toSQL().sql, v2Domains.toSQL().sql) or a small summary instead.
- Around line 93-119: The owner-only queries exclude domains without label rows
because schema.label is inner-joined unconditionally in the v1Domains and
v2Domains query builders; change the join on schema.label to a leftJoin (or
perform the join only when partial is truthy) so that owner-only searches still
return domains with no label row, while keeping the existing where clause using
partial and like(schema.label.value, `${partial}%`) when partial is provided;
update both v1Domains and v2Domains to reference schema.label via leftJoin (or
wrap the join in a conditional based on the partial variable) so unlabeled
domains are not filtered out.
In `@apps/ensapi/src/graphql-api/schema/query.ts`:
- Around line 38-45: The GraphQL input DomainsWhereInput currently uses isOneOf
which enforces exactly one of name or owner, conflicting with the resolver
findDomains that accepts both; update the schema by removing isOneOf from the
builder.inputType call (i.e., adjust the DomainsWhereInput definition so both
name and owner can be provided), or if you need "at least one" semantics add
runtime validation via Pothos Validation plugin in the same DomainsWhereInput to
enforce at-least-one instead of using isOneOf.
Contributor
Greptile OverviewGreptile SummaryThis PR implements query-time canonical name resolution for ENSv1/v2 domains using recursive CTEs, avoiding the complexity of index-time materialization. The core changes include a new Key Changes:
Issues Found:
Confidence Score: 3.5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL as Query.domains
participant FindDomains as findDomains()
participant Parser as parsePartialInterpretedName()
participant V1CTE as v1DomainsByLabelHashPath()
participant V2CTE as v2DomainsByLabelHashPath()
participant DB as PostgreSQL Database
Client->>GraphQL: query domains(where: {name, owner})
GraphQL->>FindDomains: findDomains({name, owner})
FindDomains->>Parser: parsePartialInterpretedName(name)
Parser-->>FindDomains: {concrete: ["sub1", "sub2"], partial: "paren"}
FindDomains->>FindDomains: validate depth <= MAX_DEPTH (16)
FindDomains->>FindDomains: validate name or owner provided
FindDomains->>FindDomains: interpretedLabelsToLabelHashPath(concrete)
par V1 and V2 Domain Queries
FindDomains->>V1CTE: v1DomainsByLabelHashPath(labelHashPath)
V1CTE->>DB: Recursive CTE (leaf→parent via parent_id)
DB-->>V1CTE: {leafId, headId} pairs
FindDomains->>V2CTE: v2DomainsByLabelHashPath(labelHashPath)
V2CTE->>DB: Recursive CTE (leaf→parent via registryCanonicalDomain)
DB-->>V2CTE: {leafId, headId} pairs
end
FindDomains->>DB: JOIN v1Domains on leafId<br/>+ filter by owner on leaf<br/>+ filter by partial LIKE on head.label
FindDomains->>DB: JOIN v2Domains on leafId<br/>+ filter by owner on leaf<br/>+ filter by partial LIKE on head.label
FindDomains->>DB: UNION ALL v1Domains + v2Domains
DB-->>FindDomains: Combined domain IDs
FindDomains-->>GraphQL: domains CTE
GraphQL->>DB: SELECT with pagination (before/after/limit)
DB-->>GraphQL: Domain results
GraphQL->>GraphQL: Load full Domain entities via dataloader
GraphQL-->>Client: Connection with Domain nodes
|
![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
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts`:
- Around line 169-171: The code uses a non-existent drizzle API sql.param() when
building rawLabelHashPathArray and pathLength; replace the
sql.param(labelHashPath) usage by directly interpolating the value into the sql
template (use sql`${labelHashPath}::text[]` for rawLabelHashPathArray and keep
using sql`${rawLabelHashPathArray}` for pathLength) so Drizzle will
auto-parameterize—also apply the same replacement for the other occurrences
referenced around the pathLength duplicate (the second
rawLabelHashPathArray/pathLength pair).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.changeset/eight-beans-behave.md:
- Line 8: Fix the typo in the example for Account.domains by adding the missing
closing quotation mark to the name value; update `Account.domains(where?: {
name: "example.et })` to have a matching quote for the `name` string so it reads
`Account.domains(where?: { name: "example.et" })`, ensuring the
`Account.domains` example is valid.
- Line 5: Replace the misspelled word "experiemental" in the changeset sentence
that mentions the ENSv2 API (the phrase starting "The experiemental ENSv2
API...") with the correct spelling "experimental" so the line reads "The
experimental ENSv2 API now supports the following Domain filters, namely
matching indexed Domains by name prefix."
In `@apps/ensapi/src/graphql-api/context.ts`:
- Around line 8-16: The two DataLoader factories createV1CanonicalPathLoader and
createV2CanonicalPathLoader currently call getV1CanonicalPath/getV2CanonicalPath
per id, causing N separate recursive CTE queries; change them to call a new
batched function (e.g., batchGetV1CanonicalPaths and batchGetV2CanonicalPaths)
that accepts the full domainIds array and performs a single SQL query that
computes canonical paths for all requested ids in one go (using a recursive CTE
with an IN (...) filter, join to a temp table, or a VALUES list), then return
results in the same order as input, mapping missing rows to null so DataLoader
receives an array aligned with domainIds. Ensure the new batch functions are
used inside DataLoader constructors and preserve existing types (CanonicalPath |
null) and error-handling.
- Around line 8-16: The batch loaders createV1CanonicalPathLoader and
createV2CanonicalPathLoader currently use Promise.all over
getV1CanonicalPath/getV2CanonicalPath so a single thrown error rejects the whole
batch; change each batch function to map domainIds to individual async calls
wrapped in try/catch and return either the CanonicalPath value or an Error
instance for that key (preserving input order) so DataLoader receives per-key
results/errors; ensure you return an array of (CanonicalPath | Error | null)
matching domainIds for both createV1CanonicalPathLoader and
createV2CanonicalPathLoader.
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 110-133: The Domain.name resolver currently assumes every loaded
domain has a label and will throw when found.label or found.label.value is
missing; change the labels mapping in the resolve function (the block using
isENSv1Domain, context.loaders.v1CanonicalPath/v2CanonicalPath,
DomainInterfaceRef.getDataloader, and interpretedLabelsToInterpretedName) to
guard against missing labels by checking found and found.label.value for each
domainId and returning null (or a chosen fallback) from the resolver immediately
if any label is absent instead of throwing an Error.
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 #1565
Reviewer Focus (Read This First)
find-domains.ts- the v1/v2 path traversal logic is the core of this PRisInterpetedLabelbug fix - previously fell through toisNormalizedLabelfor encoded labelhashesProblem & Motivation
What Changed (Concrete)
findDomains()function with recursive CTEs for v1/v2 domain path traversalregistryCanonicalDomainschema table for ENSv2 canonical parent trackinggetCanonicalPathintogetV1CanonicalPathandgetV2CanonicalPathv1Domain.byLabelHash,v2Domain.byLabelHash,label.byValueparsePartialInterpretedName,constructSubInterpretedName,ensureInterpretedLabel,interpretedLabelsToLabelHashPathisInterpetedLabelnow returns early for valid encoded labelhashesQuery.domainsendpoint withwhere: { name, owner }filterDesign & Planning
registryCanonicalDomainas interim solution until ENSv2 canonical names are implemented on-chainSelf-Review
changed
innerJointoleftJoinonschema.labelso owner-only queries don't exclude unlabeled domainsadded MAX_DEPTH validation after initial implementation
Bugs caught:
isInterpetedLabelnot returning early for encoded labelhashesLogic simplified: n/a
Naming / terminology improved: renamed
interpretedNameToLabelHashPath→interpretedLabelsToLabelHashPathDead or unnecessary code removed: n/a
Cross-Codebase Alignment
get-canonical-path.tsfor MAX_DEPTH constant alignmentDownstream & Consumer Impact
Query.domains(where: { name, owner })endpoint available (dev-only for now)concretevspartialterminology for parsed name segmentsTesting Evidence
parsePartialInterpretedName(30 cases)constructSubInterpretedName(14 cases)findDomainsitselfScope Reductions
integration tests for
findDomainsLIKE operator input escaping (marked with TODO)
production-ready
Query.domainsendpoint (currently dev-only)Follow-ups: integration tests, input escaping, promote endpoint to production
Why they were deferred: focused on core functionality first
Risk Analysis
registryCanonicalDomainis an interim solution that will need migrationPre-Review Checklist (Blocking)