Conversation
This ensures that `EnsNodeStackInfo` object has been resolved correctly before serving any HTTP route that relies on the stack info
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements a split configuration design for ENSApi, separating environment-based config (static import) from ENSDb-derived config (async, middleware-shared). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as API Router<br/>(middleware)
participant StackMiddleware as Stack Info<br/>Middleware
participant ENSDb as ENSDb Client
participant Handler as Handler
participant Config as Config
Client->>Router: HTTP Request
Router->>StackMiddleware: ensureEnsNodeStackInfoAvailable()
alt Stack Info not cached
StackMiddleware->>ENSDb: getEnsNodeStackInfo()
ENSDb-->>StackMiddleware: {ensApi, ensDb, ensIndexer, ensRainbow}
StackMiddleware->>StackMiddleware: Cache in c.var.stackInfo
end
StackMiddleware->>Handler: c.var.stackInfo available
Handler->>Handler: Extract namespace & plugins<br/>from c.var.stackInfo.ensIndexer
Handler->>Config: resolveForward(namespace, plugins, ...)
Config->>Config: Compute using provided namespace
Config-->>Handler: Result
Handler-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
@greptile review |
![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 (9)
apps/ensapi/src/lib/public-client.ts (1)
11-40:⚠️ Potential issue | 🟠 MajorPerf regression: RPC configs are re-parsed on every call, defeating the cache.
buildRpcConfigsFromEnv(config, namespace)+RpcConfigsSchema.parse(...)now runs on every invocation, before the cache check. Since this is called on the request hot-path (e.g.forward-resolution.ts), every request pays a full Zod validation of the RPC config map even when the cachedPublicClientexists. Previously the lookup was effectivelyO(1)for cache hits.Also, the JSDoc is stale — it still says "Gets a viem#PublicClient for the specified
chainId", but the parameter is nownamespace.♻️ Proposed fix: derive `rootChainId` cheaply, serve cached client fast, only parse on miss
-/** - * Gets a viem#PublicClient for the specified `chainId` using the ENSApiConfig's RPCConfig. Caches - * the instance itself to minimize unnecessary allocations. - */ +/** + * Builds a viem#PublicClient for the ENS root chain of the given `namespace`, using the + * ENSApiConfig's RPCConfig. Caches the instance itself keyed by the derived root chain id + * to minimize unnecessary allocations and avoid re-validating RPC configs on every call. + */ export function buildPublicClientForRootChain(namespace: ENSNamespaceId): PublicClient { const rootChainId = getENSRootChainId(namespace); + + const cached = _cache.get(rootChainId); + if (cached) return cached; + const unvalidatedRpcConfigs = buildRpcConfigsFromEnv(config, namespace); const rpcConfigs = RpcConfigsSchema.parse(unvalidatedRpcConfigs); const rpcConfig = rpcConfigs.get(rootChainId); if (!rpcConfig) { throw new Error(`Invariant: ENSApi does not have an RPC to chain id '${rootChainId}'.`); } - if (!_cache.has(rootChainId)) { - _cache.set( - rootChainId, - // Create an viem#PublicClient that uses a fallback() transport with all specified HTTP RPCs - createPublicClient({ - transport: fallback(rpcConfig.httpRPCs.map((url) => http(url.toString()))), - }), - ); - } - - const publicClient = _cache.get(rootChainId); - - // publicClient guaranteed to exist due to cache-setting logic above - if (!publicClient) throw new Error("never"); - + const publicClient = createPublicClient({ + transport: fallback(rpcConfig.httpRPCs.map((url) => http(url.toString()))), + }); + _cache.set(rootChainId, publicClient); return publicClient; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/lib/public-client.ts` around lines 11 - 40, The function buildPublicClientForRootChain currently parses RPC configs on every call causing a perf regression; change the logic so you compute rootChainId using getENSRootChainId(namespace) and check _cache.has(rootChainId) first, returning the cached PublicClient if present, and only call buildRpcConfigsFromEnv(config, namespace) and RpcConfigsSchema.parse(...) when the cache misses to construct and set the client via createPublicClient/fallback/http as before; also update the JSDoc to refer to the `namespace` parameter (not `chainId`) to keep comments accurate.apps/ensapi/src/config/config.schema.ts (2)
46-73:⚠️ Potential issue | 🟡 MinorStale
async+ stale JSDoc onbuildConfigFromEnvironment.With the
ensIndexerPublicConfigfetching removed, there are noawaits left in this function, yet it's still declaredasyncand returnsPromise<EnsApiConfig>. The JSDoc also still claims it's "fetching the EnsIndexerPublicConfig", which is no longer true. Either make it sync (preferable, since the whole point of this PR is that env-based config is statically importable) or keep it async but update the doc.♻️ Proposed fix
/** - * Builds the EnsApiConfig from an EnsApiEnvironment object, fetching the EnsIndexerPublicConfig. + * Builds the EnsApiConfig from an EnsApiEnvironment object. * * `@returns` A validated EnsApiConfig object * `@throws` Error with formatted validation messages if environment parsing fails */ -export async function buildConfigFromEnvironment(env: EnsApiEnvironment): Promise<EnsApiConfig> { +export function buildConfigFromEnvironment(env: EnsApiEnvironment): EnsApiConfig {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/config/config.schema.ts` around lines 46 - 73, The buildConfigFromEnvironment function is declared async and its JSDoc still says it "fetches the EnsIndexerPublicConfig" even though there are no awaits or async work; make it synchronous by removing the async modifier and changing the return type from Promise<EnsApiConfig> to EnsApiConfig, and update the JSDoc to reflect it simply builds/validates the config from the provided EnsApiEnvironment (no fetching). Keep the existing error handling (ZodError branch using prettifyError and logger.error) and ensure callers of buildConfigFromEnvironment are updated to handle a direct EnsApiConfig return instead of awaiting it.
75-98: 🧹 Nitpick | 🔵 TrivialStale JSDoc on
buildEnsApiPublicConfig.
@param configno longer matches the signature — the function now takesensApiConfigandensIndexerPublicConfig. Worth updating to document both params and note thatnamespace/isSubgraphCompatiblecome from the indexer config.📝 Proposed doc fix
/** * Builds the ENSApi public configuration from an EnsApiConfig object. * - * `@param` config - The validated EnsApiConfig object + * `@param` ensApiConfig - The validated EnsApiConfig object (env-sourced) + * `@param` ensIndexerPublicConfig - The ENSIndexer public config (loaded from ENSDb), + * used to derive `namespace` and `isSubgraphCompatible` for TheGraph fallback. * `@returns` A complete ENSApiPublicConfig object */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/config/config.schema.ts` around lines 75 - 98, Update the JSDoc for buildEnsApiPublicConfig to reflect its current signature: document both parameters ensApiConfig (validated EnsApiConfig) and ensIndexerPublicConfig (validated EnsIndexerPublicConfig), and note that namespace and isSubgraphCompatible are read from ensIndexerPublicConfig; also ensure the `@returns` tag correctly describes the returned EnsApiPublicConfig. Reference the function name buildEnsApiPublicConfig and the parameter names ensApiConfig and ensIndexerPublicConfig when making the doc changes.packages/ensnode-sdk/src/stack-info/ensnode-stack-info.ts (1)
34-43: 🧹 Nitpick | 🔵 TrivialStale JSDoc — update to mention ENSIndexer and ENSRainbow inputs.
The comment only mentions ENSApi and ENSDb, but the signature now also takes
ensIndexerPublicConfig(required) andensRainbowPublicConfig(optional). Worth updating so the contract is clear — particularly thatensRainbowis intentionally optional and represents cold-start (which is already documented on the type field).📝 Proposed doc fix
/** - * Build a complete {`@link` EnsNodeStackInfo} object from - * the given public configs of ENSApi and ENSDb. + * Build a complete {`@link` EnsNodeStackInfo} object from the public configs of + * ENSApi, ENSDb, and ENSIndexer, plus an optional ENSRainbow public config + * (omit to represent ENSRainbow cold start). */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensnode-sdk/src/stack-info/ensnode-stack-info.ts` around lines 34 - 43, Update the stale JSDoc for buildEnsNodeStackInfo to mention all inputs: ensApiPublicConfig, ensDbPublicConfig, ensIndexerPublicConfig (required) and ensRainbowPublicConfig (optional); explicitly note that ensRainbowPublicConfig is optional and represents cold-start/disabled state for ENS Rainbow while ensIndexerPublicConfig is now a required input, and ensure the description aligns with the EnsNodeStackInfo type fields.apps/ensapi/src/config/config.schema.test.ts (1)
61-82:⚠️ Potential issue | 🟠 MajorThis assertion contradicts the PR goal of an env-only
EnsApiConfig.Per the commit "Removed from EnsApiConfig any fields not sourced from environment variables",
buildConfigFromEnvironmentshould no longer returnensIndexerPublicConfig,namespace, orrpcConfigs. ThetoStrictEqualon lines 63-81 still asserts all three. Either the code has not yet removed these fields fromEnsApiConfig, or this test needs to be updated to reflect the new shape (mirroring what was done for thebuildEnsApiPublicConfigtests below). Also, given thegetEnsIndexerPublicConfigmock at lines 8-12, it's worth confirming whetherbuildConfigFromEnvironmentstill reads from ENSDb at all under the new design — if not, that mock can be dropped too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/config/config.schema.test.ts` around lines 61 - 82, The test asserts fields that should no longer be returned by buildConfigFromEnvironment; update the expectation to only include environment-sourced fields (e.g., port, ensDbUrl, theGraphApiKey, ensIndexerSchemaName, referralProgramEditionConfigSetUrl) and remove assertions for ensIndexerPublicConfig, namespace, and rpcConfigs from the toStrictEqual payload for buildConfigFromEnvironment; also remove or adjust the getEnsIndexerPublicConfig mock (lines 8-12) if buildConfigFromEnvironment no longer reads ENSDb. Locate buildConfigFromEnvironment and the test block referencing it to mirror the shape used by the buildEnsApiPublicConfig tests below.apps/ensadmin/src/app/mock/config-info/page.tsx (1)
22-49:⚠️ Potential issue | 🟡 MinorThe mock data shape is not accurately represented by the type cast.
Line 22 asserts
Record<string, SerializedENSApiPublicConfig>, butEnsApiPublicConfig(whichSerializedENSApiPublicConfigaliases) contains onlytheGraphFallbackandversionInfo. The actual mock data includes an additionalensIndexerPublicConfigfield, which lines 46–48 depend on when callingdeserializeEnsIndexerPublicConfig.Change the type to accurately reflect the hybrid structure, such as:
type MockConfigData = Record<string, SerializedEnsApiPublicConfig & { ensIndexerPublicConfig: SerializedEnsIndexerPublicConfig }>; const mockConfigData = mockDataJson as unknown as MockConfigData;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensadmin/src/app/mock/config-info/page.tsx` around lines 22 - 49, The mockConfigData cast is too narrow: mockConfigData is typed as Record<string, SerializedENSApiPublicConfig> but the actual entries also include ensIndexerPublicConfig which deserializeEnsIndexerPublicConfig uses; update the type of mockConfigData to reflect the combined shape (e.g., a Record keyed by string whose value is SerializedENSApiPublicConfig & { ensIndexerPublicConfig: SerializedEnsIndexerPublicConfig }) so that accesses in MockConfigPage (selectedConfig, deserializeEnsApiPublicConfig, deserializeEnsIndexerPublicConfig, and usage of ensRainbowPublicConfig) are correctly typed and avoid unsafe casts.apps/ensapi/src/omnigraph-api/yoga.ts (1)
15-57: 🛠️ Refactor suggestion | 🟠 MajorMemoize Yoga by namespace and pass context as a factory function to ensure fresh dataloaders and timestamps per request.
createYogaForNamespaceis invoked on each HTTP request. Instantiating a complete Yoga server (schema compilation, plugin initialization, middleware wiring) per request is wasteful compared to caching by namespace. Additionally, the current context is precomputed once viacreateYogaContextForNamespace(namespace), which captures a fixednowtimestamp and dataloader instances. If Yoga instances are cached without changing the context pattern, those stale values would leak across requests.Yoga supports context as a factory function (invoked per request). Memoize Yoga instances by namespace and pass
contextas a function so dataloaders andnowremain fresh for every operation. This is the standard GraphQL Yoga pattern.♻️ Proposed fix
-export const createYogaForNamespace = (namespace: ENSNamespaceId) => { - const context = createYogaContextForNamespace(namespace); - const yoga = createYoga({ - graphqlEndpoint: "*", - schema, - context, - ... - }); - return yoga; -}; +const yogaByNamespace = new Map<ENSNamespaceId, ReturnType<typeof createYoga>>(); + +export const createYogaForNamespace = (namespace: ENSNamespaceId) => { + const cached = yogaByNamespace.get(namespace); + if (cached) return cached; + + const yoga = createYoga({ + graphqlEndpoint: "*", + schema, + // factory form: invoked per-request so dataloaders/now are fresh + context: () => createYogaContextForNamespace(namespace), + cors: false, + graphiql: { defaultQuery: `...` }, + logging: logger, + plugins: [], + }); + + yogaByNamespace.set(namespace, yoga); + return yoga; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/omnigraph-api/yoga.ts` around lines 15 - 57, createYogaForNamespace currently creates a new Yoga server on every request and calls createYogaContextForNamespace(namespace) once, which captures a fixed now and dataloader instances; instead, memoize the Yoga server per namespace (cache by ENSNamespaceId inside createYogaForNamespace) and change the context option to a factory function that calls createYogaContextForNamespace(namespace) per request so dataloaders and timestamps are fresh for each operation; ensure the cache key is the namespace and that the cached Yoga instance reuses schema, plugins, logging, and graphiql settings but receives context as () => createYogaContextForNamespace(namespace).apps/ensapi/src/lib/subgraph/indexing-status-to-subgraph-meta.ts (1)
10-24: 🧹 Nitpick | 🔵 TrivialUpdate JSDoc to document the new
ensIndexerPublicConfigparameter.The
@paramlist only mentionsindexingStatus, but the signature now also takesensIndexerPublicConfig. Either add a matching@paramentry or drop the@param/@returnsblock entirely to keep it in sync.📝 Proposed doc fix
* `@param` indexingStatus - The indexing context from the indexing status middleware - * `@returns` SubgraphMeta object or null if conversion is not possible + * `@param` ensIndexerPublicConfig - ENSIndexer public config used to derive ENS root chain id and deployment metadata + * `@returns` SubgraphMeta object or null if conversion is not possible */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/lib/subgraph/indexing-status-to-subgraph-meta.ts` around lines 10 - 24, The JSDoc for indexingContextToSubgraphMeta is out of sync: it documents only the indexingStatus parameter but the function signature also accepts ensIndexerPublicConfig; update the comment to include a new `@param` entry describing ensIndexerPublicConfig (type/purpose and how it affects conversion) and ensure the existing `@returns` remains accurate, or if you prefer, remove the whole `@param/`@returns block—make sure the JSDoc and the function signature (indexingContextToSubgraphMeta, indexingStatus, ensIndexerPublicConfig) match.apps/ensapi/src/handlers/api/explore/name-tokens-api.ts (1)
86-127: 🧹 Nitpick | 🔵 TrivialDrop
lazyProxyhere and hoist thenamespacedestructure; the lazy comment is stale.Now that this code runs inside a request handler:
lazyProxy(() => getIndexedSubregistries(...))is constructed and immediately consumed by.find()on the next line, so the indirection only costs a Proxy allocation per request without any deferral benefit.- The comment on lines 89–90 ("so that this module can be imported without env vars being present (e.g. during OpenAPI generation)") no longer applies — there is no module-level evaluation anymore.
{ namespace }is destructured twice (line 87 and line 122) from the samec.var.stackInfo.ensIndexer. Hoist once at the top of the handler.♻️ Proposed refactor
app.openapi(getNameTokensRoute, async (c) => { ensureEnsNodeStackInfoAvailable(c); + const { namespace, plugins } = c.var.stackInfo.ensIndexer; + // Check if Indexing Status resolution failed. if (c.var.indexingStatus instanceof Error) { ... } @@ const parentNode = namehashInterpretedName(parentName); - const { namespace, plugins } = c.var.stackInfo.ensIndexer; - - // lazyProxy defers construction until first use so that this module can be - // imported without env vars being present (e.g. during OpenAPI generation). - const indexedSubregistries = lazyProxy(() => - getIndexedSubregistries(namespace, plugins as PluginName[]), - ); - - const subregistry = indexedSubregistries.find((s) => s.node === parentNode); + const indexedSubregistries = getIndexedSubregistries(namespace, plugins as PluginName[]); + const subregistry = indexedSubregistries.find((s) => s.node === parentNode); @@ - const { namespace } = c.var.stackInfo.ensIndexer; const registeredNameTokens = await findRegisteredNameTokensForDomain( namespace, domainId, accurateAsOf, );If
lazyProxy/PluginName-typed imports become unused after this, drop them too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/handlers/api/explore/name-tokens-api.ts` around lines 86 - 127, Remove the unnecessary lazyProxy indirection and hoist the namespace destructure: extract const { namespace, plugins } = c.var.stackInfo.ensIndexer once at the top of the handler, replace const indexedSubregistries = lazyProxy(() => getIndexedSubregistries(namespace, plugins as PluginName[])) and the subsequent indexedSubregistries.find(...) with a direct call getIndexedSubregistries(namespace, plugins as PluginName[]) and find on its result to avoid allocating a Proxy per request, and delete the stale lazyProxy comment; if PluginName or lazyProxy imports become unused after this, remove them as well.
🤖 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/middleware/stack-info.middleware.ts`:
- Around line 67-72: The current ensureEnsNodeStackInfoAvailable function
wrongly asserts success when c.var.stackInfo is undefined and throws a plain
Error for Error values; instead, add a pure type guard (e.g.,
isEnsNodeStackInfoAvailable(c) that returns boolean by checking c.var.stackInfo
is not undefined and not an Error) and replace/augment the assert function with
a middleware (e.g., requireEnsNodeStackInfo) that calls that guard and, on
failure, returns a 503 using the shared errorResponse helper (from
error-response.ts) rather than throwing; update callers to use the guard when
narrowing is optional or the middleware when a 503 response is desired, and
remove the throw of a plain Error from ensureEnsNodeStackInfoAvailable.
In `@apps/ensapi/src/omnigraph-api/context.ts`:
- Around line 25-37: createYogaContextForNamespace currently constructs
per-request values (loaders, now) but may be invoked only once during Yoga
server construction; ensure it's called per-request by wiring Yoga's context to
a factory that returns a fresh object each request (e.g. in
createYogaForNamespace/yoga.ts use context: () =>
createYogaContextForNamespace(namespace) so dataloaders and now aren't
long‑lived), or if you can't change yoga.ts, tighten the JSDoc on
createYogaContextForNamespace to state it must be invoked per-request and not
reused.
---
Outside diff comments:
In `@apps/ensadmin/src/app/mock/config-info/page.tsx`:
- Around line 22-49: The mockConfigData cast is too narrow: mockConfigData is
typed as Record<string, SerializedENSApiPublicConfig> but the actual entries
also include ensIndexerPublicConfig which deserializeEnsIndexerPublicConfig
uses; update the type of mockConfigData to reflect the combined shape (e.g., a
Record keyed by string whose value is SerializedENSApiPublicConfig & {
ensIndexerPublicConfig: SerializedEnsIndexerPublicConfig }) so that accesses in
MockConfigPage (selectedConfig, deserializeEnsApiPublicConfig,
deserializeEnsIndexerPublicConfig, and usage of ensRainbowPublicConfig) are
correctly typed and avoid unsafe casts.
In `@apps/ensapi/src/config/config.schema.test.ts`:
- Around line 61-82: The test asserts fields that should no longer be returned
by buildConfigFromEnvironment; update the expectation to only include
environment-sourced fields (e.g., port, ensDbUrl, theGraphApiKey,
ensIndexerSchemaName, referralProgramEditionConfigSetUrl) and remove assertions
for ensIndexerPublicConfig, namespace, and rpcConfigs from the toStrictEqual
payload for buildConfigFromEnvironment; also remove or adjust the
getEnsIndexerPublicConfig mock (lines 8-12) if buildConfigFromEnvironment no
longer reads ENSDb. Locate buildConfigFromEnvironment and the test block
referencing it to mirror the shape used by the buildEnsApiPublicConfig tests
below.
In `@apps/ensapi/src/config/config.schema.ts`:
- Around line 46-73: The buildConfigFromEnvironment function is declared async
and its JSDoc still says it "fetches the EnsIndexerPublicConfig" even though
there are no awaits or async work; make it synchronous by removing the async
modifier and changing the return type from Promise<EnsApiConfig> to
EnsApiConfig, and update the JSDoc to reflect it simply builds/validates the
config from the provided EnsApiEnvironment (no fetching). Keep the existing
error handling (ZodError branch using prettifyError and logger.error) and ensure
callers of buildConfigFromEnvironment are updated to handle a direct
EnsApiConfig return instead of awaiting it.
- Around line 75-98: Update the JSDoc for buildEnsApiPublicConfig to reflect its
current signature: document both parameters ensApiConfig (validated
EnsApiConfig) and ensIndexerPublicConfig (validated EnsIndexerPublicConfig), and
note that namespace and isSubgraphCompatible are read from
ensIndexerPublicConfig; also ensure the `@returns` tag correctly describes the
returned EnsApiPublicConfig. Reference the function name buildEnsApiPublicConfig
and the parameter names ensApiConfig and ensIndexerPublicConfig when making the
doc changes.
In `@apps/ensapi/src/handlers/api/explore/name-tokens-api.ts`:
- Around line 86-127: Remove the unnecessary lazyProxy indirection and hoist the
namespace destructure: extract const { namespace, plugins } =
c.var.stackInfo.ensIndexer once at the top of the handler, replace const
indexedSubregistries = lazyProxy(() => getIndexedSubregistries(namespace,
plugins as PluginName[])) and the subsequent indexedSubregistries.find(...) with
a direct call getIndexedSubregistries(namespace, plugins as PluginName[]) and
find on its result to avoid allocating a Proxy per request, and delete the stale
lazyProxy comment; if PluginName or lazyProxy imports become unused after this,
remove them as well.
In `@apps/ensapi/src/lib/public-client.ts`:
- Around line 11-40: The function buildPublicClientForRootChain currently parses
RPC configs on every call causing a perf regression; change the logic so you
compute rootChainId using getENSRootChainId(namespace) and check
_cache.has(rootChainId) first, returning the cached PublicClient if present, and
only call buildRpcConfigsFromEnv(config, namespace) and
RpcConfigsSchema.parse(...) when the cache misses to construct and set the
client via createPublicClient/fallback/http as before; also update the JSDoc to
refer to the `namespace` parameter (not `chainId`) to keep comments accurate.
In `@apps/ensapi/src/lib/subgraph/indexing-status-to-subgraph-meta.ts`:
- Around line 10-24: The JSDoc for indexingContextToSubgraphMeta is out of sync:
it documents only the indexingStatus parameter but the function signature also
accepts ensIndexerPublicConfig; update the comment to include a new `@param` entry
describing ensIndexerPublicConfig (type/purpose and how it affects conversion)
and ensure the existing `@returns` remains accurate, or if you prefer, remove the
whole `@param/`@returns block—make sure the JSDoc and the function signature
(indexingContextToSubgraphMeta, indexingStatus, ensIndexerPublicConfig) match.
In `@apps/ensapi/src/omnigraph-api/yoga.ts`:
- Around line 15-57: createYogaForNamespace currently creates a new Yoga server
on every request and calls createYogaContextForNamespace(namespace) once, which
captures a fixed now and dataloader instances; instead, memoize the Yoga server
per namespace (cache by ENSNamespaceId inside createYogaForNamespace) and change
the context option to a factory function that calls
createYogaContextForNamespace(namespace) per request so dataloaders and
timestamps are fresh for each operation; ensure the cache key is the namespace
and that the cached Yoga instance reuses schema, plugins, logging, and graphiql
settings but receives context as () => createYogaContextForNamespace(namespace).
In `@packages/ensnode-sdk/src/stack-info/ensnode-stack-info.ts`:
- Around line 34-43: Update the stale JSDoc for buildEnsNodeStackInfo to mention
all inputs: ensApiPublicConfig, ensDbPublicConfig, ensIndexerPublicConfig
(required) and ensRainbowPublicConfig (optional); explicitly note that
ensRainbowPublicConfig is optional and represents cold-start/disabled state for
ENS Rainbow while ensIndexerPublicConfig is now a required input, and ensure the
description aligns with the EnsNodeStackInfo type fields.
🪄 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: 105504b5-2034-4736-a3e8-fb48a181e818
📒 Files selected for processing (49)
apps/ensadmin/src/app/mock/config-info/page.tsxapps/ensadmin/src/app/mock/indexing-status-api.mock.tsapps/ensapi/src/cache/stack-info.cache.tsapps/ensapi/src/config/config.schema.test.tsapps/ensapi/src/config/config.schema.tsapps/ensapi/src/config/redact.tsapps/ensapi/src/handlers/api/explore/name-tokens-api.tsapps/ensapi/src/handlers/api/meta/status-api.tsapps/ensapi/src/handlers/api/omnigraph/omnigraph-api.tsapps/ensapi/src/handlers/api/resolution/resolution-api.tsapps/ensapi/src/handlers/api/router.tsapps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.tsapps/ensapi/src/handlers/subgraph/subgraph-api.tsapps/ensapi/src/lib/name-tokens/find-name-tokens-for-domain.tsapps/ensapi/src/lib/protocol-acceleration/find-resolver.tsapps/ensapi/src/lib/protocol-acceleration/get-records-from-index.tsapps/ensapi/src/lib/public-client.tsapps/ensapi/src/lib/resolution/forward-resolution.tsapps/ensapi/src/lib/resolution/multichain-primary-name-resolution.tsapps/ensapi/src/lib/resolution/resolve-with-universal-resolver.integration.test.tsapps/ensapi/src/lib/resolution/resolve-with-universal-resolver.tsapps/ensapi/src/lib/resolution/reverse-resolution.tsapps/ensapi/src/lib/subgraph/indexing-status-to-subgraph-meta.tsapps/ensapi/src/middleware/can-accelerate.middleware.tsapps/ensapi/src/middleware/name-tokens.middleware.tsapps/ensapi/src/middleware/registrar-actions.middleware.tsapps/ensapi/src/middleware/stack-info.middleware.tsapps/ensapi/src/middleware/subgraph-meta.middleware.tsapps/ensapi/src/middleware/thegraph-fallback.middleware.tsapps/ensapi/src/omnigraph-api/builder.tsapps/ensapi/src/omnigraph-api/context.tsapps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.tsapps/ensapi/src/omnigraph-api/lib/find-domains/find-domains-resolver.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.tsapps/ensapi/src/omnigraph-api/lib/get-canonical-path.tsapps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.tsapps/ensapi/src/omnigraph-api/schema/account.tsapps/ensapi/src/omnigraph-api/schema/query.tsapps/ensapi/src/omnigraph-api/schema/resolver.tsapps/ensapi/src/omnigraph-api/yoga.tspackages/ensnode-sdk/src/ensapi/config/conversions.test.tspackages/ensnode-sdk/src/ensapi/config/deserialize.tspackages/ensnode-sdk/src/ensapi/config/serialize.tspackages/ensnode-sdk/src/ensapi/config/serialized-types.tspackages/ensnode-sdk/src/ensapi/config/types.tspackages/ensnode-sdk/src/ensapi/config/zod-schemas.tspackages/ensnode-sdk/src/ensnode/client.test.tspackages/ensnode-sdk/src/stack-info/ensnode-stack-info.tspackages/ensnode-sdk/src/stack-info/serialize/ensnode-stack-info.ts
💤 Files with no reviewable changes (6)
- apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.ts
- apps/ensadmin/src/app/mock/indexing-status-api.mock.ts
- packages/ensnode-sdk/src/ensapi/config/conversions.test.ts
- packages/ensnode-sdk/src/ensapi/config/types.ts
- packages/ensnode-sdk/src/ensapi/config/zod-schemas.ts
- packages/ensnode-sdk/src/ensnode/client.test.ts
| export function ensureEnsNodeStackInfoAvailable<T extends Context>( | ||
| c: T, | ||
| ): asserts c is T & { var: T["var"] & { stackInfo: EnsNodeStackInfo } } { | ||
| if (c.var.stackInfo instanceof Error) { | ||
| throw new Error(`ENSNode Stack Info is not available: ${c.var.stackInfo.message}`); | ||
| } |
There was a problem hiding this comment.
Don’t assert success when stack info is missing or turn availability failures into a 500.
Line 70 only rejects Error, so an omitted/not-yet-run middleware leaves c.var.stackInfo === undefined and this assertion incorrectly narrows it to EnsNodeStackInfo. When it is an Error, throwing a plain Error also bypasses the PR’s intended 503 response path for stack-info-dependent routes. Split this into a boolean type guard plus a route/middleware 503 response, or have a dedicated “require stack info” middleware return the shared ENSApi error response before downstream handlers run.
As per coding guidelines, use the shared errorResponse helper (apps/ensapi/src/lib/handlers/error-response.ts) for all error responses in ENSApi.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/middleware/stack-info.middleware.ts` around lines 67 - 72,
The current ensureEnsNodeStackInfoAvailable function wrongly asserts success
when c.var.stackInfo is undefined and throws a plain Error for Error values;
instead, add a pure type guard (e.g., isEnsNodeStackInfoAvailable(c) that
returns boolean by checking c.var.stackInfo is not undefined and not an Error)
and replace/augment the assert function with a middleware (e.g.,
requireEnsNodeStackInfo) that calls that guard and, on failure, returns a 503
using the shared errorResponse helper (from error-response.ts) rather than
throwing; update callers to use the guard when narrowing is optional or the
middleware when a 503 response is desired, and remove the throw of a plain Error
from ensureEnsNodeStackInfoAvailable.
| /** | ||
| * Constructs a new GraphQL Context per-request. | ||
| * Constructs a new GraphQL Context per-request for the given {@link ENSNamespaceId}. | ||
| * | ||
| * @dev make sure that anything that is per-request (like dataloaders) are newly created in this fn | ||
| */ | ||
| export const context = () => ({ | ||
| export const createYogaContextForNamespace = (namespace: ENSNamespaceId) => ({ | ||
| namespace, | ||
| now: BigInt(getUnixTime(new Date())), | ||
| loaders: { | ||
| v1CanonicalPath: createV1CanonicalPathLoader(), | ||
| v2CanonicalPath: createV2CanonicalPathLoader(), | ||
| v2CanonicalPath: createV2CanonicalPathLoader(namespace), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Context freshness is coupled to Yoga construction, not to requests.
The JSDoc states per-request values (dataloaders, now) are created in this function, but that invariant only holds if the function is invoked per request. As used in yoga.ts today, it's invoked once per Yoga construction — correctness relies on createYogaForNamespace also being called per request (see the related comment). If Yoga is memoized by namespace (recommended), this object becomes long‑lived and dataloaders would batch/cache across unrelated requests, and now would drift.
Recommend wiring this as a per-request factory in yoga.ts (context: () => createYogaContextForNamespace(namespace)). No change required here if the yoga.ts change is adopted, but consider tightening the JSDoc to reflect that this function must be invoked per-request by the Yoga setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/omnigraph-api/context.ts` around lines 25 - 37,
createYogaContextForNamespace currently constructs per-request values (loaders,
now) but may be invoked only once during Yoga server construction; ensure it's
called per-request by wiring Yoga's context to a factory that returns a fresh
object each request (e.g. in createYogaForNamespace/yoga.ts use context: () =>
createYogaContextForNamespace(namespace) so dataloaders and now aren't
long‑lived), or if you can't change yoga.ts, tighten the JSDoc on
createYogaContextForNamespace to state it must be invoked per-request and not
reused.
There was a problem hiding this comment.
Pull request overview
This PR continues the work from #1806 by splitting ENSApi configuration into (1) static env-based config and (2) dynamic, ENSDb-derived config carried via EnsNodeStackInfo and passed through middleware/context.
Changes:
- Removed
ensIndexerPublicConfigfromEnsApiPublicConfigand updated SDK serialization/deserialization + tests accordingly. - Introduced stack-info-driven namespace/config plumbing across ENSApi (middlewares, resolution, omnigraph), replacing direct
config.namespacereads. - Updated stack info building/caching to fetch
EnsIndexerPublicConfigfrom ENSDb and assemble a fullEnsNodeStackInfo.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/stack-info/serialize/ensnode-stack-info.ts | Stops re-serializing ENSApi config; uses stackInfo field directly. |
| packages/ensnode-sdk/src/stack-info/ensnode-stack-info.ts | Splits stack info builder inputs to accept indexer/rainbow configs separately. |
| packages/ensnode-sdk/src/ensnode/client.test.ts | Updates client test fixtures to match new ENSApi public config shape. |
| packages/ensnode-sdk/src/ensapi/config/zod-schemas.ts | Removes indexer-public-config fields from ENSApi config schemas. |
| packages/ensnode-sdk/src/ensapi/config/types.ts | Removes ensIndexerPublicConfig from EnsApiPublicConfig. |
| packages/ensnode-sdk/src/ensapi/config/serialized-types.ts | Simplifies serialized type to alias EnsApiPublicConfig. |
| packages/ensnode-sdk/src/ensapi/config/serialize.ts | Drops indexer serialization from ENSApi config serialization. |
| packages/ensnode-sdk/src/ensapi/config/deserialize.ts | Drops indexer deserialization from ENSApi config deserialization. |
| packages/ensnode-sdk/src/ensapi/config/conversions.test.ts | Updates ENSApi config conversion tests to new shape. |
| apps/ensapi/src/omnigraph-api/yoga.ts | Creates namespace-specific Yoga/context factory. |
| apps/ensapi/src/omnigraph-api/schema/resolver.ts | Switches from global config namespace to request context namespace. |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Switches domain filtering/ID derivation to use context namespace. |
| apps/ensapi/src/omnigraph-api/schema/account.ts | Switches canonical filtering to use context namespace. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Threads namespace into ENSv2 root registry lookup. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Threads namespace into ENSv2 canonical path logic. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Threads namespace into canonical registries CTE. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/find-domains-resolver.ts | Updates context typing to new namespace-aware context creator. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Makes canonical registries CTE namespace-dependent. |
| apps/ensapi/src/omnigraph-api/context.ts | Adds namespace to GraphQL context and loader construction. |
| apps/ensapi/src/omnigraph-api/builder.ts | Updates schema builder context typing. |
| apps/ensapi/src/middleware/thegraph-fallback.middleware.ts | Uses stackInfo-derived namespace/subgraph-compatibility for fallback logic. |
| apps/ensapi/src/middleware/subgraph-meta.middleware.ts | Passes stackInfo indexer config into subgraph meta mapping. |
| apps/ensapi/src/middleware/stack-info.middleware.ts | Adds ensureEnsNodeStackInfoAvailable type guard helper. |
| apps/ensapi/src/middleware/registrar-actions.middleware.ts | Switches registrar actions prerequisites to stackInfo indexer config. |
| apps/ensapi/src/middleware/name-tokens.middleware.ts | Switches name tokens prerequisites to stackInfo indexer config. |
| apps/ensapi/src/middleware/can-accelerate.middleware.ts | Uses stackInfo plugins for acceleration gating (ENSv2 bailout + plugin checks). |
| apps/ensapi/src/lib/subgraph/indexing-status-to-subgraph-meta.ts | Removes global config usage; uses indexer config passed in. |
| apps/ensapi/src/lib/resolution/reverse-resolution.ts | Threads namespace/plugins into reverse resolution and forwards into forward resolution. |
| apps/ensapi/src/lib/resolution/resolve-with-universal-resolver.ts | Threads namespace into datasource contract selection (UR v1/v2). |
| apps/ensapi/src/lib/resolution/resolve-with-universal-resolver.integration.test.ts | Updates integration test to new public client API + namespace passing. |
| apps/ensapi/src/lib/resolution/multichain-primary-name-resolution.ts | Threads namespace/plugins into primary-name resolution; updates chainId selection logic. |
| apps/ensapi/src/lib/resolution/forward-resolution.ts | Threads namespace/plugins into forward resolution; switches to root-chain public client builder. |
| apps/ensapi/src/lib/public-client.ts | Replaces chainId-based client getter with root-chain client builder per namespace. |
| apps/ensapi/src/lib/protocol-acceleration/get-records-from-index.ts | Threads namespace for resolver defaulting detection. |
| apps/ensapi/src/lib/protocol-acceleration/find-resolver.ts | Threads namespace into resolver lookup paths and datasource contract selection. |
| apps/ensapi/src/lib/name-tokens/find-name-tokens-for-domain.ts | Threads namespace into token ownership computation. |
| apps/ensapi/src/handlers/subgraph/subgraph-api.ts | Ensures stackInfo middleware is installed and prerequisites use stackInfo config. |
| apps/ensapi/src/handlers/ensanalytics/ensanalytics-api.test.ts | Removes now-unneeded config mocking for namespace. |
| apps/ensapi/src/handlers/api/router.ts | Adds stackInfoMiddleware at the API router level. |
| apps/ensapi/src/handlers/api/resolution/resolution-api.ts | Uses stackInfo namespace/plugins for resolution calls. |
| apps/ensapi/src/handlers/api/omnigraph/omnigraph-api.ts | Uses stackInfo namespace and creates Yoga instance for that namespace. |
| apps/ensapi/src/handlers/api/meta/status-api.ts | Uses stackInfo guard and simplifies error conditions. |
| apps/ensapi/src/handlers/api/explore/name-tokens-api.ts | Adds stackInfo middleware + uses stackInfo namespace/plugins in routing logic. |
| apps/ensapi/src/config/redact.ts | Removes redaction of fields that no longer exist on env-based config. |
| apps/ensapi/src/config/config.schema.ts | Removes ENSDb-fetched indexer config and rpc configs from env config; makes public config builder accept indexer config input. |
| apps/ensapi/src/config/config.schema.test.ts | Updates tests for new buildEnsApiPublicConfig signature and config shape. |
| apps/ensapi/src/cache/stack-info.cache.ts | Fetches indexer public config from ENSDb and builds full stack info. |
| apps/ensadmin/src/app/mock/indexing-status-api.mock.ts | Updates mock ENSApi public config to new shape. |
| apps/ensadmin/src/app/mock/config-info/page.tsx | Updates mock page to build stack info from split config pieces. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function ensureEnsNodeStackInfoAvailable<T extends Context>( | ||
| c: T, | ||
| ): asserts c is T & { var: T["var"] & { stackInfo: EnsNodeStackInfo } } { | ||
| if (c.var.stackInfo instanceof Error) { | ||
| throw new Error(`ENSNode Stack Info is not available: ${c.var.stackInfo.message}`); | ||
| } |
There was a problem hiding this comment.
ensureEnsNodeStackInfoAvailable only checks for Error, but (1) it doesn’t handle c.var.stackInfo === undefined (e.g. if stackInfoMiddleware wasn’t applied), and (2) throwing here will be caught by app.onError and turned into a 500, even though the PR description indicates stack-info-dependent routes should return 503 when stack info isn’t available.
Consider changing this helper to either (a) return a boolean/EnsNodeStackInfo | null so callers can return a 503 response, or (b) throw a typed error that is mapped to 503 by a centralized error handler, and also explicitly guard against undefined.
| export function buildPublicClientForRootChain(namespace: ENSNamespaceId): PublicClient { | ||
| const rootChainId = getENSRootChainId(namespace); | ||
| const unvalidatedRpcConfigs = buildRpcConfigsFromEnv(config, namespace); | ||
| const rpcConfigs = RpcConfigsSchema.parse(unvalidatedRpcConfigs); | ||
| const rpcConfig = rpcConfigs.get(rootChainId); | ||
|
|
There was a problem hiding this comment.
buildRpcConfigsFromEnv expects a raw RpcEnvironment (e.g. ALCHEMY_API_KEY, RPC_URL_<id>), but this function is being called with the parsed config object. Since EnsApiConfigSchema no longer includes those env vars, this will always yield empty RPC configs (except the hardcoded ens-test-env), causing rpcConfigs.get(rootChainId) to be undefined and resolution to fail at runtime.
Pass the actual runtime environment (e.g. process.env typed as RpcEnvironment/EnsApiEnvironment) or persist the needed RPC env vars/derived rpcConfigs in config at init time, and avoid re-parsing env on every call when the client is already cached.
| const ensApiPublicConfig = deserializeEnsApiPublicConfig(mockConfigData[selectedConfig]); | ||
| const ensIndexerPublicConfig = deserializeEnsIndexerPublicConfig( | ||
| mockConfigData[selectedConfig], | ||
| ); |
There was a problem hiding this comment.
deserializeEnsIndexerPublicConfig expects a SerializedEnsIndexerPublicConfig, but this code passes the entire SerializedENSApiPublicConfig object (which in the current mock JSON contains ensIndexerPublicConfig nested). As a result, deserialization will fail because required indexer fields aren’t at the top level.
Use the nested ensIndexerPublicConfig value from the mock JSON (or update the mock JSON to the new split shape), and update mockConfigData’s type accordingly so TypeScript reflects the actual structure.
| export const createYogaForNamespace = (namespace: ENSNamespaceId) => { | ||
| const context = createYogaContextForNamespace(namespace); | ||
| const yoga = createYoga({ | ||
| graphqlEndpoint: "*", | ||
| schema, | ||
| context, |
There was a problem hiding this comment.
createYogaForNamespace constructs a brand new Yoga server (and a single, precomputed context object with now) each time it’s called. In omnigraph-api.ts this appears to be invoked per request, which is likely very expensive and also makes it hard to ensure per-request context values if you later cache/reuse Yoga.
Consider caching the Yoga instance per namespace (e.g. a module-level Map<ENSNamespaceId, YogaServer>) and passing context as a function (e.g. () => createYogaContextForNamespace(namespace)) so dataloaders/now are recreated per request while reusing the server + schema.
Greptile SummaryThis WIP PR splits the ENSApi config data model: static config (env vars only) is handled by
Confidence Score: 4/5Two P1 defects (failing tests + broken mock page) must be resolved before merging The core refactor (env-only config, EnsNodeStackInfo for dynamic data, SWR cache) is clean and well-structured. However, the test file has concrete assertions that will fail against the current implementation, and the mock admin page will always hit the error branch at runtime due to a mismatched deserialization call. These are present defects on the changed code path, not speculative risks. apps/ensapi/src/config/config.schema.test.ts and apps/ensadmin/src/app/mock/config-info/page.tsx Important Files Changed
Sequence DiagramsequenceDiagram
participant ENV as Env Vars
participant CFG as EnsApiConfig (static)
participant CACHE as stackInfoCache (SWR)
participant DB as ENSDb Client
participant MW as stackInfoMiddleware
participant H as Request Handler
ENV->>CFG: buildConfigFromEnvironment()
Note over CFG: port, theGraphApiKey,<br/>ensDbUrl, etc.
activate CACHE
CACHE->>DB: getEnsIndexerPublicConfig()
DB-->>CACHE: EnsIndexerPublicConfig
CACHE->>DB: buildEnsDbPublicConfig()
DB-->>CACHE: EnsDbPublicConfig
Note over CACHE: Build EnsNodeStackInfo<br/>(TTL=∞, errorTTL=1min)
deactivate CACHE
H->>MW: HTTP Request
MW->>CACHE: stackInfoCache.read()
alt Stack info available
CACHE-->>MW: EnsNodeStackInfo
MW->>H: c.var.stackInfo = EnsNodeStackInfo
H-->>H: ensureEnsNodeStackInfoAvailable(c)
H-->>H: Use stackInfo.ensIndexer, stackInfo.ensApi, etc.
else Stack info unavailable
CACHE-->>MW: Error
MW->>H: c.var.stackInfo = Error
H-->>H: ensureEnsNodeStackInfoAvailable throws
H-->>H: Return 503
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const ensApiPublicConfig = deserializeEnsApiPublicConfig(mockConfigData[selectedConfig]); | ||
| const ensIndexerPublicConfig = deserializeEnsIndexerPublicConfig( | ||
| mockConfigData[selectedConfig], | ||
| ); |
There was a problem hiding this comment.
mockConfigData[selectedConfig] is the outer combined object { versionInfo, theGraphFallback, ensIndexerPublicConfig: {...} }, but deserializeEnsIndexerPublicConfig expects an object with ENSIndexer-level fields (labelSet, indexedChainIds, etc.) at the top level. The ENSIndexer fields are one level deeper inside ensIndexerPublicConfig. As a result, deserializeEnsIndexerPublicConfig will always fail validation here — the page will always display a "Deserialization Error" instead of the intended mock state.
The correct source for the ENSIndexer config is the nested property, e.g. (mockConfigData[selectedConfig] as any).ensIndexerPublicConfig.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestions:
- Test expectations are out of sync with refactored buildConfigFromEnvironment function, expecting removed properties that are no longer part of EnsApiConfig type
- Tests for buildConfigFromEnvironment were not updated after refactoring removed RPC config validation and initialization from the config building phase
| rpcConfigs: new Map([ | ||
| [ | ||
| 1, | ||
| { |
There was a problem hiding this comment.
Test expects buildEnsApiPublicConfig to return ensIndexerPublicConfig field, but the refactored function doesn't include it in the return value
| // Invariant: ENSApi must have an rpcConfig for the requested `chainId` | ||
| const rpcConfig = config.rpcConfigs.get(chainId); | ||
| export function buildPublicClientForRootChain(namespace: ENSNamespaceId): PublicClient { | ||
| const rootChainId = getENSRootChainId(namespace); |
There was a problem hiding this comment.
buildPublicClientForRootChain passes EnsApiConfig object to buildRpcConfigsFromEnv instead of environment variables, causing RPC configuration to fail
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Overall I like where this is going. Shared some preliminary feedback 👍
| }, | ||
| } satisfies SerializedEnsApiPublicConfig; | ||
|
|
||
| const EXAMPLE_ENSDB_PUBLIC_RESPONSE = { |
There was a problem hiding this comment.
| const EXAMPLE_ENSDB_PUBLIC_RESPONSE = { | |
| const EXAMPLE_ENSDB_PUBLIC_CONFIG = { |
| @@ -69,35 +69,6 @@ const EXAMPLE_ENSAPI_CONFIG_RESPONSE = { | |||
| canFallback: false, | |||
There was a problem hiding this comment.
Several lines above we should rename EXAMPLE_ENSAPI_CONFIG_RESPONSE to EXAMPLE_ENSAPI_PUBLIC_CONFIG
| }, | ||
| }, | ||
| } satisfies SerializedEnsApiPublicConfig; | ||
|
|
There was a problem hiding this comment.
I'm interested to also see the following updates. Could we include them in this PR?
- Introduce a new type
EnsIndexerStackInfothat has two fields:ensIndexerandensRainbow, each the relevant public config type. - Use
EnsIndexerStackInfoto splitensRainbowPublicConfigfromEnsIndexerPublicConfig. - Remove the count from
ensRainbowPublicConfigso that ENSRainbow can immediately return its config without waiting. - Guarantee that both
EnsIndexerStackInfoandEnsNodeStackInfohave all their fields non-optional. In other words, there's never a case whereensRainbowis undefined. - Update the data model for
ensRainbowPublicConfigso that it aligns with theversionInfostandards from the other config types. It shouldn't haveversionas a root-level field. Instead it should have aversionInfoobject with a field inside of it forensRainbowthat might be a value such as "0.31.0".
|
|
||
| // lazyProxy defers construction until first use so that this module can be | ||
| // imported without env vars being present (e.g. during OpenAPI generation). | ||
| const ensv1RegistryOld = lazyProxy(() => |
There was a problem hiding this comment.
Can you send me a message in Slack when you have an opportunity to reply to this? I'm curious to understand if the work in this PR enables us to completely remove the use of this lazyProxy stuff? As I remember it was a source of some complexity for us and we had the goal of fully removing it.
| export function getPublicClient(chainId: ChainId): PublicClient { | ||
| // Invariant: ENSApi must have an rpcConfig for the requested `chainId` | ||
| const rpcConfig = config.rpcConfigs.get(chainId); | ||
| export function buildPublicClientForRootChain(namespace: ENSNamespaceId): PublicClient { |
There was a problem hiding this comment.
@tk-o Hey, please ask Matt to review this when you're ready. Please use special care to ensure this is 100% correct.
| */ | ||
| export async function resolveForward<SELECTION extends ResolverRecordsSelection>( | ||
| namespace: ENSNamespaceId, | ||
| plugins: string[], |
There was a problem hiding this comment.
Do you have any other suggestions for how we might pass these values around?
I dislike how we are decomposing our config objects / stack objects into independent fields where invariants are being erased because now they are just random fields that are theoretically independent of each other.
Why can't we still have some singleton object such as stack (rather than config) that would hold the stack. You could implement some getter on it that would throw an error if for some reason the data for the stack hadn't been initialized already.
| * | ||
| * @param c The Hono context | ||
| */ | ||
| export function ensureEnsNodeStackInfoAvailable<T extends Context>( |
There was a problem hiding this comment.
I dislike this approach. If the stack info isn't loaded yet it shouldn't result in a HTTP 500 error. It should result in a HTTP 503 error.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
EnsNodeStackInfoobject that is loaded from cache for every request. If theEnsNodeStackInfoobject is not available, no relevant HTTP routes (the ones that rely on theEnsNodeStackInfoobject) return 503.Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)