Skip to content

Conversation

@shrugs
Copy link
Collaborator

@shrugs shrugs commented Dec 29, 2025

Fix Acceleration Mismatch


Reviewer Focus (Read This First)

What reviewers should focus on
  1. Database query logic in find-resolver.ts:203-229 - The or() clause that adds RegistryOld fallback and the sorting logic that ensures Registry records take precedence over RegistryOld records
  2. Simplified control flow in find-resolver.ts:242-271 - Changed from iterating all node-resolver relations to taking just the first (after sorting). Confirm this matches the intended behavior.

Problem & Motivation

Why this exists
  • Problem: Protocol Accelerated Resolution API requests were failing for legacy unmigrated ENS names
  • Root cause: The ENS Root Registry contract is actually ENSRegistryWithFallback, which falls back to RegistryOld storage for nodes that haven't been migrated to the new registry. The acceleration code wasn't implementing this fallback logic.
  • Impact: Any unmigrated name would return NULL_RESULT when queried through the accelerated path, despite having valid resolver data in RegistryOld
  • Why now: Discovered when testing acceleration with older ENS names that predate the registry migration

What Changed (Concrete)

What actually changed
  1. Added RegistryOld datasource contract reference to find-resolver.ts
  2. Modified nodeResolverRelation database query to include records from both Registry and RegistryOld when querying the ENS Root Registry
  3. Added sorting logic to prefer Registry records over RegistryOld records (implementing fallback semantics)
  4. Simplified iteration logic: instead of looping through all node-resolver relations, now takes the first record after sorting
  5. Minor cleanup: removed explicit type annotations from Registry event handler (apps/ensindexer/src/plugins/protocol-acceleration/handlers/Registry.ts:102)

Design & Planning

How this approach was chosen

Approach: Mirror the ENSRegistryWithFallback contract logic in the query layer by fetching from both registries and using sort order to implement precedence.

Alternatives considered:

  • Assigning registry old records directly to the registry, but hacky.

Why this approach:

  • Single query with conditional logic keeps performance acceptable
  • Sorting implements fallback semantics cleanly without branching
  • Minimal changes to existing code paths
  • Planning artifacts: None, straightforward bug fix
  • Reviewed / approved by: N/A

Self-Review

What you caught yourself
  • Logic simplified: Original implementation iterated through all node-resolver relations; realized after review that we only need the first record after proper sorting
  • Type safety: Removed unnecessary function re-wrapping in Registry.ts handler
  • Bugs caught: None beyond the original issue
  • Logic simplified: Changed iteration to take first record after sorting
  • Naming / terminology improved: N/A
  • Dead or unnecessary code removed: Explicit type annotations in Registry.ts

Downstream & Consumer Impact

Who this affects and how

Impact:

  • API consumers making accelerated resolution requests for legacy/unmigrated ENS names will now get correct results instead of null
  • No breaking changes - purely additive fix
  • Performance impact negligible (single query with slightly more complex filter)

Operators:

  • No configuration changes required
  • No migration needed - uses existing indexed data
  • Can deploy just ENSApi to patch production

Terminology:

  • Comments at lines 194-199 explain the ENSRegistryWithFallback contract behavior for future maintainers
  • Public APIs affected: None (behavioral fix, same interface)
  • Docs updated: Inline comments added explaining fallback logic

Testing Evidence

How this was validated

Testing performed:

  • Manual testing with legacy unmigrated ENS names via accelerated resolution API
  • Verified fallback order: Registry records preferred when both exist, RegistryOld used when Registry has no record

Known gaps:

  • No automated test coverage for this specific fallback scenario
  • Integration tests would require larger overhead

If this is wrong:

  • Legacy unmigrated names would return null resolvers (regression to previous bug)
  • Or, if sort order is wrong, could return stale RegistryOld data when migrated data exists in Registry
  • Testing performed: Manual validation with legacy ENS name (d8da6bf26964af9d7eed9e03e53415d37aa96045.addr.reverse)
  • Known gaps: No automated test coverage for registry fallback scenarios
  • What reviewers have to reason about manually: Database query result ordering correctness and the sort comparator logic (lines 226-233)

Scope Reductions

What you intentionally didn't do

Deferred:

  • Adding comprehensive test coverage for registry fallback scenarios (would require significant test infrastructure setup)
  • Applying similar fallback logic to other protocol acceleration queries (only needed for resolver lookups currently)
  • Follow-ups: Consider adding integration tests for registry fallback once test infrastructure matures
  • Why they were deferred: Bug fix is time-sensitive; test infrastructure work is a larger project

Risk Analysis

How this could go wrong

Assumptions:

  • Sort order correctly prioritizes Registry over RegistryOld when nodes match

Failure modes:

  • If sort logic is incorrect, could return stale RegistryOld resolver when migrated resolver exists

Blast radius:

  • Limited to Protocol Accelerated Resolution API
  • Non-accelerated paths unaffected
  • Only affects resolver lookup, not other ENS operations
  • Risk areas: Sort order correctness
  • Mitigations or rollback options: Easy revert via deploying previous ENSApi version
  • Named owner if this causes problems: @shrugs (PR author)

Pre-Review Checklist (Blocking)

  • I reviewed every line of this diff and understand it end-to-end
  • I'm prepared to defend this PR line-by-line in review
  • I'm comfortable being the on-call owner for this change
  • Relevant changesets are included (or explicitly not required) - ✅ Included in .changeset/fast-geese-poke.md

…n API requests and legacy unmigrated names, which should now resolve correctly when accelerated.
@shrugs shrugs requested a review from a team as a code owner December 29, 2025 19:34
@changeset-bot
Copy link

changeset-bot bot commented Dec 29, 2025

🦋 Changeset detected

Latest commit: e405229

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
ensapi Minor
ensindexer Minor
ensadmin Minor
ensrainbow Minor
fallback-ensapi Minor
@ensnode/datasources Minor
@ensnode/ensrainbow-sdk Minor
@ensnode/ponder-metadata Minor
@ensnode/ensnode-schema Minor
@ensnode/ensnode-react Minor
@ensnode/ponder-subgraph Minor
@ensnode/ensnode-sdk Minor
@ensnode/shared-configs Minor
@docs/ensnode Minor
@docs/ensrainbow Minor

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

@vercel
Copy link

vercel bot commented Dec 29, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
admin.ensnode.io Ready Ready Preview, Comment Dec 29, 2025 7:34pm
ensnode.io Ready Ready Preview, Comment Dec 29, 2025 7:34pm
ensrainbow.io Ready Ready Preview, Comment Dec 29, 2025 7:34pm

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shrugs Nice work, thank you. Please take the lead to merge this when ready 🚀

@shrugs shrugs merged commit 05d7481 into main Dec 29, 2025
17 checks passed
@shrugs shrugs deleted the fix/acceleration-mismatch branch December 29, 2025 21:36
@github-actions github-actions bot mentioned this pull request Dec 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants