-
Notifications
You must be signed in to change notification settings - Fork 16
feat: separate ensv2 datasources out more correctly #1566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR restructures ENS datasources by introducing Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors ENSv2 datasource configuration by splitting the previously conflated Namechain datasource into two distinct datasources (ENSv2Root for L1 root chain and ENSv2ETHRegistry for L2 ETH registry), and removes ENSv2-specific contracts from the ENSRoot datasource. It also introduces a new helper function getDatasourcesWithENSv2Contracts and updates plugins to use the new structure.
Changes:
- Split
DatasourceNames.NamechainintoENSv2RootandENSv2ETHRegistryto separate L1 and L2 deployment surfaces - Removed ENSv2 contracts (Registry, EnhancedAccessControl, ETHRegistry, RootRegistry) from ENSRoot datasource in mainnet and sepolia
- Added
getDatasourcesWithENSv2Contractshelper function and updated plugins to use it
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/datasources/src/lib/types.ts | Replaced Namechain datasource name with ENSv2Root and ENSv2ETHRegistry |
| packages/ensnode-sdk/src/shared/datasources-with-ensv2-contracts.ts | New helper function to get datasources with ENSv2 contracts |
| packages/ensnode-sdk/src/shared/datasources-with-resolvers.ts | Updated to include ENSv2 datasources and changed from filter+warn to map+throw for strictness |
| packages/ensnode-sdk/src/internal.ts | Exported new ENSv2 contracts helper |
| packages/datasources/src/ens-test-env.ts | Split Namechain into ENSv2Root (L1) and ENSv2ETHRegistry (L2) datasources |
| packages/datasources/src/sepolia.ts | Removed ENSv2 contracts from ENSRoot datasource |
| packages/datasources/src/mainnet.ts | Removed ENSv2 contracts from ENSRoot datasource |
| apps/ensindexer/src/plugins/protocol-acceleration/plugin.ts | Updated to use getDatasourcesWithENSv2Contracts helper |
| apps/ensindexer/src/plugins/ensv2/plugin.ts | Updated to reference new ENSv2 datasources and use helper function |
| apps/ensapi/src/graphql-api/lib/get-domain-by-fqdn.ts | Updated references from namechain to ENSv2Root and ENSv2ETHRegistry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/ensapi/src/graphql-api/lib/get-domain-by-fqdn.ts`:
- Around line 138-141: The code creates ENS_ROOT_V2_ETH_REGISTRY_ID by directly
accessing ENSv2Root.contracts.ETHRegistry.address; add a guard that checks
ENSv2Root?.contracts?.ETHRegistry?.address before calling makeRegistryId and
handle the missing-contract case (either throw a clear Error indicating
ETHRegistry is missing for this ENSv2Root or skip/continue for that datasource),
updating the logic around ENS_ROOT_V2_ETH_REGISTRY_ID and any downstream use so
you never dereference ETHRegistry when it may be undefined; refer to ENSv2Root,
ENS_ROOT_V2_ETH_REGISTRY_ID, makeRegistryId and getDatasourcesWithENSv2Contracts
when locating where to add this guard.
- Around line 190-193: The code creates NAMECHAIN_V2_ETH_REGISTRY_ID using
ENSv2ETHRegistry.contracts.ETHRegistry.address without verifying the ETHRegistry
contract exists; add an explicit guard before calling makeRegistryId (e.g.,
check ENSv2ETHRegistry and ENSv2ETHRegistry.contracts.ETHRegistry) and handle
the missing contract by throwing a clear error or returning a safe fallback;
update the code paths that reference NAMECHAIN_V2_ETH_REGISTRY_ID (and the
makeRegistryId call) so they only execute after
ENSv2ETHRegistry?.contracts.ETHRegistry is confirmed defined.
In `@apps/ensindexer/src/plugins/ensv2/plugin.ts`:
- Around line 141-154: The ETHRegistrar block uses ENSv2ETHRegistry without
ensuring ENSv2ETHRegistry.contracts.ETHRegistrar exists, risking a runtime
error; update the condition that spreads chainConfigForContract so it only
executes when ENSv2ETHRegistry is truthy AND
ENSv2ETHRegistry.contracts.ETHRegistrar is defined (e.g., change the spread
guard around chainConfigForContract in the namespaceContract(pluginName,
"ETHRegistrar") entry to also check ENSv2ETHRegistry.contracts.ETHRegistrar) and
pass ENSv2ETHRegistry.contracts.ETHRegistrar to chainConfigForContract alongside
config.globalBlockrange and ENSv2ETHRegistry.chain.id.
In `@packages/ensnode-sdk/src/shared/datasources-with-ensv2-contracts.ts`:
- Around line 33-36: The error message in the check that validates
datasource.contracts (the block that throws when !datasource.contracts.Registry
|| !datasource.contracts.EnhancedAccessControl) is missing "and" and reads
awkwardly; update the thrown Error message to read something like: "Invariant:
Datasource does not define the 'Registry' and 'EnhancedAccessControl' contracts:
${JSON.stringify(datasource)}" so it correctly lists both contract names and
fixes the grammar while keeping the existing JSON payload.
- Around line 24-40: The error message thrown in
getDatasourcesWithENSv2Contracts loses the datasource name because
maybeGetDatasource is mapped directly; refactor to first map
DATASOURCE_NAMES_WITH_ENSv2_CONTRACTS to pairs like { name: datasourceName, ds:
maybeGetDatasource(namespace, datasourceName) }, then filter out falsy ds, and
in the subsequent .map validate ds.contracts.Registry and
ds.contracts.EnhancedAccessControl and throw an Error that includes the
datasourceName (from the pair) along with minimal ds info instead of
JSON.stringify(datasource); update references to datasource to use the pair
structure while preserving function names getDatasourcesWithENSv2Contracts,
DATASOURCE_NAMES_WITH_ENSv2_CONTRACTS, and maybeGetDatasource.
Greptile OverviewGreptile SummaryThis PR separates the conflated Key Changes
Impact
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Application Code
participant Helper as getDatasourcesWithENSv2Contracts()
participant DS as Datasources Config
participant Plugin as ENSv2/Protocol Accel Plugin
Note over App,Plugin: Before: Single "Namechain" datasource conflated L1 & L2
Note over App,Plugin: After: Separate ENSv2Root (L1) & ENSv2ETHRegistry (L2)
App->>DS: Request ENSv2Root datasource
DS-->>App: Returns L1 root chain contracts (Registry, ETHRegistry, RootRegistry)
App->>DS: Request ENSv2ETHRegistry datasource
DS-->>App: Returns L2 chain contracts (Registry, ETHRegistry, ETHRegistrar)
Plugin->>Helper: Call getDatasourcesWithENSv2Contracts(namespace)
Helper->>DS: maybeGetDatasource(ENSv2Root)
DS-->>Helper: Return ENSv2Root or null
Helper->>DS: maybeGetDatasource(ENSv2ETHRegistry)
DS-->>Helper: Return ENSv2ETHRegistry or null
Helper->>Helper: Filter out nulls & validate contracts
Helper-->>Plugin: Return array of valid datasources
Plugin->>Plugin: Reduce datasources to chain configs
Plugin->>Plugin: Configure Registry & EnhancedAccessControl contracts
Note over App,Plugin: ENSRoot datasource no longer contains ENSv2 contracts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Suggestion:
getENSv2RootRegistry is trying to retrieve RootRegistry from the ENSRoot datasource, but this contract has been moved to the new ENSv2Root datasource in the refactoring. This will cause the application to crash on mainnet/sepolia where RootRegistry is no longer defined in ENSRoot.
View Details
📝 Patch Details
diff --git a/packages/ensnode-sdk/src/shared/root-registry.ts b/packages/ensnode-sdk/src/shared/root-registry.ts
index 02cc0243..c120f1a3 100644
--- a/packages/ensnode-sdk/src/shared/root-registry.ts
+++ b/packages/ensnode-sdk/src/shared/root-registry.ts
@@ -30,7 +30,7 @@ export const isENSv1Registry = (namespace: ENSNamespaceId, contract: AccountId)
* Gets the AccountId representing the ENSv2 Root Registry in the selected `namespace`.
*/
export const getENSv2RootRegistry = (namespace: ENSNamespaceId) =>
- getDatasourceContract(namespace, DatasourceNames.ENSRoot, "RootRegistry");
+ getDatasourceContract(namespace, DatasourceNames.ENSv2Root, "RootRegistry");
/**
* Gets the RegistryId representing the ENSv2 Root Registry in the selected `namespace`.
Analysis
getENSv2RootRegistry retrieves contract from wrong datasource
What fails: The getENSv2RootRegistry() function in packages/ensnode-sdk/src/shared/root-registry.ts line 33 attempts to retrieve the RootRegistry contract from the DatasourceNames.ENSRoot datasource. However, the RootRegistry contract exists only in the DatasourceNames.ENSv2Root datasource. Calling this function on mainnet or sepolia (where ENSv2Root datasource does not exist) results in the error: Expected contract not found for {namespace} ENSRoot RootRegistry
How to reproduce:
import { getENSv2RootRegistry } from "@ensnode/ensnode-sdk";
// This will throw an error on mainnet or sepolia:
const registry = getENSv2RootRegistry("mainnet");
// Error: Expected contract not found for mainnet ensroot RootRegistryResult: Error thrown with message Expected contract not found for {namespace} ENSRoot RootRegistry
Expected: The function should retrieve RootRegistry from the ENSv2Root datasource (where it is defined), not from the ENSRoot datasource. The contract exists in:
DatasourceNames.ENSv2Rootin ens-test-env (contains RootRegistry)- NOT in
DatasourceNames.ENSRooton mainnet or sepolia
Root cause: During the ENSv2 datasource boundaries refactoring, the RootRegistry contract was moved from the ENSRoot datasource to a new ENSv2Root datasource. The getENSv2RootRegistry() function was not updated to reflect this change.
References:
- Bug location:
packages/ensnode-sdk/src/shared/root-registry.tsline 33 - Datasource definitions:
packages/datasources/src/ens-test-env.ts(shows RootRegistry under ENSv2Root) - Contract getter function:
packages/ensnode-sdk/src/shared/datasource-contract.ts(getDatasourceContract throws on missing contract)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/datasources/src/lib/types.ts`:
- Around line 67-68: The datasource value strings ENSv2Root and ENSv2ETHRegistry
are using PascalCase while the existing datasource values use
lowercase/camelCase; update the string literals for these identifiers (ENSv2Root
and ENSv2ETHRegistry in types.ts) to follow the repository convention (e.g., use
all-lowercase like "ensv2root" and "ensv2ethregistry" or match the established
camelCase pattern used elsewhere) so the casing is consistent with the other
datasource values.
In `@packages/datasources/src/sepolia-v2.ts`:
- Around line 40-41: Update the outdated inline comment that currently reads
"add a namechain" to reference the actual datasource name used in the map;
replace it with a phrase that mentions DatasourceNames.ENSv2ETHRegistry (for
example "add ENSv2ETHRegistry datasource" or "add ENS v2 ETH registry") so the
comment matches the key in the map and the renamed entity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/datasources/src/sepolia-v2.ts`:
- Around line 21-27: The Resolver, Registry and EnhancedAccessControl contract
definitions are duplicated across the ENSv2 datasources; extract a shared
constant (e.g., ENSv2SharedContracts) containing the contract map { Resolver: {
abi: ResolverABI, startBlock: 3702721 }, Registry: { abi: Registry, startBlock:
9770973 }, EnhancedAccessControl: { abi: EnhancedAccessControl, startBlock:
9770973 } } and replace the inline objects in the DatasourceNames.ENSv2Root and
the other ENSv2 datasource with a reference to that constant (preserving the
chain value sepolia), so both datasources import/use the same contract
definition and avoid future drift.
lightwalker-eth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shrugs Looks good! ✅
closes #1563
Summary
DatasourceNames.NamechainintoENSv2RootandENSv2ETHRegistryto model the two distinct ENSv2 deployment surfacesENSRootdatasource configsgetDatasourcesWithENSv2Contractshelper, updated ensv2 and protocol-acceleration plugins to use itWhy
Namechainconflated L1 root chain and L2 ETH registry contracts into one datasource, and ENSv2 contracts were incorrectly bundled intoENSRoot— this split aligns datasource boundaries with actual deployment topologyNotes for Reviewer (Optional)
getDatasourcesWithResolverschanged fromfilter+console.warntomap+throwfor missing Resolver contract — intentional strictness increaseDatasourceNames.Namechainis removed — any external reference will fail at compile timePre-Review Checklist (Blocking)