Build ENSRainbow config #1425
Build ENSRainbow config #1425
Conversation
…NSRainbow application
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🦋 Changeset detectedLatest commit: b9a41e9 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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCentralizes ENSRainbow configuration with Zod schemas and defaults, validates environment-to-config mapping (including DB schema invariant), moves CLI defaults to the new config, tightens port validation, adds a GET /v1/config endpoint and SDK client method, and expands tests for config and CLI isolation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "CLI"
participant ConfigBuilder as "buildConfigFromEnvironment"
participant Zod as "Zod schema"
participant Process as "process"
CLI->>ConfigBuilder: request config from env
ConfigBuilder->>Zod: parse & validate environment -> ENSRainbowConfig
Zod-->>ConfigBuilder: validation result
alt valid
ConfigBuilder-->>CLI: return ENSRainbowConfig
CLI->>CLI: continue startup (serve/ingest/validate)
else invalid
ConfigBuilder->>Process: log prettified errors
Process->>Process: process.exit(1)
end
sequenceDiagram
autonumber
participant SDK as "SDK Client"
participant Server as "ENSRainbow API"
participant DB as "Label store / DB"
participant Builder as "buildENSRainbowPublicConfig"
SDK->>Server: GET /v1/config
Server->>DB: fetch server label set & count
DB-->>Server: labelSet, count
Server->>Builder: assemble public config (version, labelSet, count)
Builder-->>Server: ENSRainbowPublicConfig
Server-->>SDK: 200 + ENSRainbowPublicConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/ensrainbow/src/config/config.schema.ts`:
- Around line 65-71: The current ternary for labelSet uses a truthy check
(env.LABEL_SET_ID || env.LABEL_SET_VERSION) which treats empty strings as
missing; change the condition to explicit undefined checks so an empty string is
treated as a provided value and validation will run — e.g. replace the condition
with (env.LABEL_SET_ID !== undefined || env.LABEL_SET_VERSION !== undefined) and
still return the object with labelSetId: env.LABEL_SET_ID and labelSetVersion:
env.LABEL_SET_VERSION when true; keep the symbol name labelSet and the env keys
env.LABEL_SET_ID / env.LABEL_SET_VERSION so locators remain obvious.
- Around line 33-36: The schema currently calls getDefaultDataDir() at module
load in ENSRainbowConfigSchema (dataDir:
DataDirSchema.default(getDefaultDataDir())), capturing process.cwd() too early;
remove the eager default from ENSRainbowConfigSchema and instead handle lazy
evaluation in buildConfigFromEnvironment by supplying dataDir: env.DATA_DIR ??
getDefaultDataDir() when parsing/building the config, keeping
ENSRainbowConfigSchema (and DataDirSchema/PortSchema) purely declarative and
ensuring getDefaultDataDir() runs only at build time.
- Around line 18-24: The path transform in the config schema currently treats
paths starting with "/" as absolute; update the transform used on the config
field to use Node's path.isAbsolute(path) instead of path.startsWith("/"), and
ensure the Node "path" module is imported (or isAbsolute is referenced)
alongside the existing join and process.cwd() usage in the transform callback so
Windows absolute paths like "C:\..." are detected correctly and returned
unchanged.
- Around line 73-83: Replace the terminal process.exit(1) in the catch block
with throwing a descriptive error so callers can handle failures; specifically,
inside the catch for buildConfigFromEnvironment (or whatever function constructs
ENSRainbowConfig) throw a custom error (e.g., ConfigBuildError) or rethrow the
existing Error with context including the prettified ZodError output and the
message "Failed to build ENSRainbowConfig", while keeping the existing logger
calls for ZodError and generic Error; move any process.exit(1) behavior out to
the CLI/entrypoint so tests can catch the thrown error and decide whether to
exit.
In `@apps/ensrainbow/src/config/validations.ts`:
- Around line 7-10: The current type ZodCheckFnInput<T> uses the internal
z.core.ParsePayload<T>; change it to rely on Zod's documented types or a simple
explicit input shape instead: remove z.core.ParsePayload and either use the
public helper z.input with a Zod type (e.g., z.input<z.ZodType<T>>) or replace
ZodCheckFnInput<T> with a small explicit interface/alias (e.g., unknown or
Record<string, any> or a narrow shape your check expects) so the code no longer
depends on the unstable z.core namespace; update any usages of ZodCheckFnInput
to match the new public type.
In `@apps/ensrainbow/src/lib/env.ts`:
- Around line 7-10: The getEnvPort function unsafely asserts process.env as
ENSRainbowEnvironment and rebuilds the full config on every call; remove the
type assertion and instead import the ENSRainbowConfig type (import type {
ENSRainbowConfig } ...) and let buildConfigFromEnvironment validate process.env
at runtime, receiving an ENSRainbowConfig result; then read and return
config.port. Also memoize the built config in a module-level variable so
getEnvPort calls reuse the same config instead of reconstructing it each time
(references: getEnvPort, buildConfigFromEnvironment, ENSRainbowEnvironment,
ENSRainbowConfig).
| const ENSRainbowConfigSchema = z | ||
| .object({ | ||
| port: PortSchema.default(ENSRAINBOW_DEFAULT_PORT), | ||
| dataDir: DataDirSchema.default(getDefaultDataDir()), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Default data directory is evaluated at module load time.
getDefaultDataDir() is called once when the module loads, capturing process.cwd() at that moment. If the working directory changes before buildConfigFromEnvironment is called, the default will be stale.
Consider using a getter function for lazy evaluation:
♻️ Suggested lazy evaluation
const ENSRainbowConfigSchema = z
.object({
port: PortSchema.default(ENSRAINBOW_DEFAULT_PORT),
- dataDir: DataDirSchema.default(getDefaultDataDir()),
+ dataDir: DataDirSchema.optional(),
dbSchemaVersion: DbSchemaVersionSchema,
labelSet: LabelSetSchema.optional(),
})Then handle the default in buildConfigFromEnvironment:
return ENSRainbowConfigSchema.parse({
port: env.PORT,
dataDir: env.DATA_DIR ?? getDefaultDataDir(),
// ...
});🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.ts` around lines 33 - 36, The schema
currently calls getDefaultDataDir() at module load in ENSRainbowConfigSchema
(dataDir: DataDirSchema.default(getDefaultDataDir())), capturing process.cwd()
too early; remove the eager default from ENSRainbowConfigSchema and instead
handle lazy evaluation in buildConfigFromEnvironment by supplying dataDir:
env.DATA_DIR ?? getDefaultDataDir() when parsing/building the config, keeping
ENSRainbowConfigSchema (and DataDirSchema/PortSchema) purely declarative and
ensuring getDefaultDataDir() runs only at build time.
There was a problem hiding this comment.
@djstrong Suggest reviewing this feedback from coderabbit
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } catch (error) { | ||
| if (error instanceof ZodError) { | ||
| logger.error(`Failed to parse environment configuration: \n${prettifyError(error)}\n`); | ||
| } else if (error instanceof Error) { | ||
| logger.error(error, `Failed to build ENSRainbowConfig`); | ||
| } else { | ||
| logger.error(`Unknown Error`); | ||
| } | ||
|
|
||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
process.exit(1) prevents testability and graceful error handling.
Calling process.exit(1) terminates the process immediately, making this function difficult to test and preventing callers from handling errors gracefully. Consider throwing a custom error and letting the caller decide how to handle it.
🔧 Suggested refactor for better error handling
+export class ConfigurationError extends Error {
+ constructor(message: string) {
+ super(message);
+ this.name = "ConfigurationError";
+ }
+}
+
export function buildConfigFromEnvironment(env: ENSRainbowEnvironment): ENSRainbowConfig {
try {
return ENSRainbowConfigSchema.parse({
// ... parsing logic
});
} catch (error) {
if (error instanceof ZodError) {
logger.error(`Failed to parse environment configuration: \n${prettifyError(error)}\n`);
+ throw new ConfigurationError(`Invalid configuration: ${prettifyError(error)}`);
} else if (error instanceof Error) {
logger.error(error, `Failed to build ENSRainbowConfig`);
+ throw error;
} else {
logger.error(`Unknown Error`);
+ throw new ConfigurationError("Unknown configuration error");
}
-
- process.exit(1);
}
}Then handle the exit at the call site (e.g., in CLI entry points):
try {
const config = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment);
} catch (error) {
process.exit(1);
}🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.ts` around lines 73 - 83, Replace
the terminal process.exit(1) in the catch block with throwing a descriptive
error so callers can handle failures; specifically, inside the catch for
buildConfigFromEnvironment (or whatever function constructs ENSRainbowConfig)
throw a custom error (e.g., ConfigBuildError) or rethrow the existing Error with
context including the prettified ZodError output and the message "Failed to
build ENSRainbowConfig", while keeping the existing logger calls for ZodError
and generic Error; move any process.exit(1) behavior out to the CLI/entrypoint
so tests can catch the thrown error and decide whether to exit.
| export function getEnvPort(): number { | ||
| const envPort = process.env.PORT; | ||
| if (!envPort) { | ||
| return DEFAULT_PORT; | ||
| } | ||
|
|
||
| try { | ||
| const port = parseNonNegativeInteger(envPort); | ||
| return port; | ||
| } catch (_error: unknown) { | ||
| const errorMessage = `Invalid PORT value "${envPort}": must be a non-negative integer`; | ||
| logger.error(errorMessage); | ||
| throw new Error(errorMessage); | ||
| } | ||
| const config = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment); | ||
| return config.port; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unsafe type assertion may mask configuration errors.
Casting process.env as ENSRainbowEnvironment bypasses TypeScript's type checking. While buildConfigFromEnvironment validates at runtime, the assertion could hide type mismatches during development.
Additionally, this function rebuilds the entire configuration on each call. If called frequently, consider memoizing or caching the result.
♻️ Suggested improvement
+let cachedConfig: ENSRainbowConfig | null = null;
+
/**
* Gets the port from environment variables.
*/
export function getEnvPort(): number {
- const config = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment);
- return config.port;
+ if (!cachedConfig) {
+ cachedConfig = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment);
+ }
+ return cachedConfig.port;
}You'll also need to import the config type:
import type { ENSRainbowConfig } from "@/config/config.schema";🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/lib/env.ts` around lines 7 - 10, The getEnvPort function
unsafely asserts process.env as ENSRainbowEnvironment and rebuilds the full
config on every call; remove the type assertion and instead import the
ENSRainbowConfig type (import type { ENSRainbowConfig } ...) and let
buildConfigFromEnvironment validate process.env at runtime, receiving an
ENSRainbowConfig result; then read and return config.port. Also memoize the
built config in a module-level variable so getEnvPort calls reuse the same
config instead of reconstructing it each time (references: getEnvPort,
buildConfigFromEnvironment, ENSRainbowEnvironment, ENSRainbowConfig).
There was a problem hiding this comment.
Pull request overview
Introduces a Zod-based, centralized environment configuration builder for the ENSRainbow app, aligning it with the configuration patterns used in other apps in the monorepo.
Changes:
- Added ENSRainbow config schema, environment types, defaults, and cross-field validations.
- Updated ENSRainbow CLI/env port handling to use the new config builder and centralized defaults.
- Tightened shared
PortSchemavalidation to require integer ports; addedzodas a direct ENSRainbow dependency.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds zod to the ENSRainbow importer lock entry. |
| packages/ensnode-sdk/src/shared/config/zod-schemas.ts | Updates shared PortSchema to require integer ports. |
| apps/ensrainbow/src/lib/env.ts | Switches env port resolution to buildConfigFromEnvironment(...). |
| apps/ensrainbow/src/config/validations.ts | Adds ENSRainbow-specific invariant validation for schema version. |
| apps/ensrainbow/src/config/types.ts | Re-exports ENSRainbow config type. |
| apps/ensrainbow/src/config/index.ts | Adds a config module entrypoint exporting types/functions/defaults. |
| apps/ensrainbow/src/config/environment.ts | Defines typed raw environment shape for ENSRainbow. |
| apps/ensrainbow/src/config/defaults.ts | Centralizes ENSRainbow default port and data dir. |
| apps/ensrainbow/src/config/config.schema.ts | Adds ENSRainbow Zod schema + config builder with logging/exit-on-failure behavior. |
| apps/ensrainbow/src/cli.ts | Uses new defaults module for data dir default; continues using env-derived port. |
| apps/ensrainbow/src/cli.test.ts | Updates port tests to reflect process-exit behavior on invalid PORT values. |
| apps/ensrainbow/package.json | Adds zod as an explicit dependency for ENSRainbow. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (config.dbSchemaVersion !== undefined && config.dbSchemaVersion !== DB_SCHEMA_VERSION) { | ||
| throw new Error( | ||
| `DB_SCHEMA_VERSION mismatch! Expected version ${DB_SCHEMA_VERSION} from code, but found ${config.dbSchemaVersion} in environment variables.`, | ||
| ); |
There was a problem hiding this comment.
This .check() invariant throws an Error directly. In most config invariants in this repo, the check adds a custom issue to ctx.issues so the failure is reported via ZodError and formatted by prettifyError (see apps/ensapi/src/config/validations.ts:18-24). Consider pushing a custom issue (with path: ["dbSchemaVersion"]) instead of throwing, so users get consistent, nicely formatted config errors.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensrainbow/src/lib/env.ts
Outdated
| @@ -1,24 +1,10 @@ | |||
| import { join } from "node:path"; | |||
| import { buildConfigFromEnvironment } from "@/config/config.schema"; | |||
There was a problem hiding this comment.
The refactored getEnvPort() function now calls process.exit(1) on validation failures instead of throwing an error, which breaks error handling in code paths that expect a catchable error. This causes the application to exit unexpectedly when validatePortConfiguration() is called with an invalid PORT environment variable.
View Details
📝 Patch Details
diff --git a/apps/ensrainbow/src/cli.test.ts b/apps/ensrainbow/src/cli.test.ts
index a8d674a4..b0159c00 100644
--- a/apps/ensrainbow/src/cli.test.ts
+++ b/apps/ensrainbow/src/cli.test.ts
@@ -51,20 +51,12 @@ describe("CLI", () => {
it("should throw error for invalid port number", () => {
process.env.PORT = "invalid";
- const exitSpy = vi.spyOn(process, "exit").mockImplementation((() => {
- throw new Error("process.exit called");
- }) as never);
expect(() => getEnvPort()).toThrow();
- expect(exitSpy).toHaveBeenCalledWith(1);
});
it("should throw error for negative port number", () => {
process.env.PORT = "-1";
- const exitSpy = vi.spyOn(process, "exit").mockImplementation((() => {
- throw new Error("process.exit called");
- }) as never);
expect(() => getEnvPort()).toThrow();
- expect(exitSpy).toHaveBeenCalledWith(1);
});
});
diff --git a/apps/ensrainbow/src/config/config.schema.ts b/apps/ensrainbow/src/config/config.schema.ts
index e74f166c..6e079dc5 100644
--- a/apps/ensrainbow/src/config/config.schema.ts
+++ b/apps/ensrainbow/src/config/config.schema.ts
@@ -71,14 +71,15 @@ export function buildConfigFromEnvironment(env: ENSRainbowEnvironment): ENSRainb
: undefined,
});
} catch (error) {
+ let errorMessage = `Failed to parse environment configuration`;
+
if (error instanceof ZodError) {
- logger.error(`Failed to parse environment configuration: \n${prettifyError(error)}\n`);
+ errorMessage = `Failed to parse environment configuration: \n${prettifyError(error)}\n`;
} else if (error instanceof Error) {
- logger.error(error, `Failed to build ENSRainbowConfig`);
- } else {
- logger.error(`Unknown Error`);
+ errorMessage = error.message;
}
- process.exit(1);
+ logger.error(errorMessage);
+ throw new Error(errorMessage);
}
}
Analysis
Invalid PORT environment variable prevents CLI port override
What fails: The refactored getEnvPort() function calls buildConfigFromEnvironment() which invokes process.exit(1) on validation failure. When an invalid PORT environment variable is set, the process exits immediately during CLI setup (via yargs default: option evaluation), preventing users from overriding it with --port CLI flag.
How to reproduce:
PORT=invalid node cli.ts serve --port 5000The process exits with validation error instead of using the provided --port 5000.
Expected vs Actual:
- Expected: Accept the
--port 5000CLI argument and use that port, either ignoring or properly validating the invalid PORT env var - Actual: Process exits immediately during yargs option setup before any command validation occurs
Root cause: In commit c34c0bd, getEnvPort() was refactored to call buildConfigFromEnvironment(), which catches validation errors and calls process.exit(1) instead of throwing a catchable error. The old implementation (commit b20b546) threw Error on validation failure, allowing calling code to catch and handle the error.
Fix applied: Changed buildConfigFromEnvironment() to throw Error instead of calling process.exit(1), allowing validation errors to propagate and be caught by calling code in validatePortConfiguration() and yargs error handlers.
… for environment variable handling
…d of environment variable
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 9 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.
| export function buildENSRainbowPublicConfig( | ||
| config: ENSRainbowConfig, | ||
| labelSet: EnsRainbowServerLabelSet, | ||
| recordsCount: number, | ||
| ): EnsRainbow.ENSRainbowPublicConfig { | ||
| return { | ||
| version: packageJson.version, | ||
| labelSet, | ||
| recordsCount, | ||
| }; |
There was a problem hiding this comment.
The config parameter is not used in the function body. While this might be intentional (similar to how ENSApi's buildEnsApiPublicConfig uses config), if there are no plans to use it, consider removing it. If it's kept for future extensibility or API consistency with other services, consider adding a comment explaining why it's present but unused, or using a TypeScript underscore prefix convention (e.g., _config) to indicate it's intentionally unused.
… handling and successful responses
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 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.
pnpm-lock.yaml
Outdated
| version: 17.7.2 | ||
| zod: | ||
| specifier: 'catalog:' | ||
| version: 3.25.76 |
There was a problem hiding this comment.
The zod version specified here (3.25.76) doesn't match the catalog version (^4.3.6) or what other apps in the monorepo use. The code imports from "zod/v4" which is specific to zod v4. This will cause import errors since zod v3 doesn't support the "/v4" import path. The specifier should remain as 'catalog:' to use zod 4.3.6 as defined in pnpm-workspace.yaml.
| version: 3.25.76 | |
| version: 4.3.6 |
| export function buildENSRainbowPublicConfig( | ||
| config: ENSRainbowConfig, | ||
| labelSet: EnsRainbowServerLabelSet, | ||
| recordsCount: number, | ||
| ): EnsRainbow.ENSRainbowPublicConfig { |
There was a problem hiding this comment.
The config parameter is passed to this function but never used. The function only uses labelSet and recordsCount from the other parameters. Consider removing the unused parameter or documenting why it's kept for future use.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/ensrainbow/src/config/config.schema.ts`:
- Around line 74-88: The TypeScript error TS2345 occurs because
invariant_dbSchemaVersionMatch is typed to accept only Pick<ENSRainbowConfig,
"dbSchemaVersion"> but ENSRainbowConfigSchema.check expects a predicate taking
the full ENSRainbowConfig; fix by widening the invariant function signature to
accept the full ENSRainbowConfig (update invariant_dbSchemaVersionMatch to (cfg:
ENSRainbowConfig) => boolean or throw) so it matches
ENSRainbowConfigSchema.check, or alternatively wrap it with a small adapter
passed to ENSRainbowConfigSchema.check that accepts the full config and calls
invariant_dbSchemaVersionMatch({ dbSchemaVersion: cfg.dbSchemaVersion }); ensure
you update the type annotation on invariant_dbSchemaVersionMatch (or the
adapter) accordingly.
♻️ Duplicate comments (1)
apps/ensrainbow/src/config/config.schema.ts (1)
25-49: Return trimmed LABEL_SET values after trimming checks.You validate trimmed values but return raw strings (Line 47-48), which can fail downstream validation for inputs with leading/trailing whitespace. Consider returning trimmed values.
🧩 Suggested fix
- const hasLabelSetId = labelSetId !== undefined && labelSetId.trim() !== ""; - const hasLabelSetVersion = labelSetVersion !== undefined && labelSetVersion.trim() !== ""; + const trimmedLabelSetId = labelSetId?.trim(); + const trimmedLabelSetVersion = labelSetVersion?.trim(); + const hasLabelSetId = trimmedLabelSetId !== undefined && trimmedLabelSetId !== ""; + const hasLabelSetVersion = trimmedLabelSetVersion !== undefined && trimmedLabelSetVersion !== ""; - return hasLabelSetId && hasLabelSetVersion - ? { - labelSetId, - labelSetVersion, - } + return hasLabelSetId && hasLabelSetVersion + ? { + labelSetId: trimmedLabelSetId!, + labelSetVersion: trimmedLabelSetVersion!, + } : undefined;
…on signature for improved clarity
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Builds the ENSRainbow public configuration from an ENSRainbowConfig object and server state. | ||
| * | ||
| * @param config - The validated ENSRainbowConfig object |
There was a problem hiding this comment.
The JSDoc comment references a parameter "config" that doesn't exist in the function signature. The function takes "labelSet" and "recordsCount" as parameters, not a config object. The @param tag for "config" should be removed.
| * @param config - The validated ENSRainbowConfig object |
There was a problem hiding this comment.
This is valid feedback. Please see my other related comment for how the private config really should be passed into this function.
| * Builds the ENSRainbow public configuration from an ENSRainbowConfig object and server state. | ||
| * | ||
| * @param config - The validated ENSRainbowConfig object |
There was a problem hiding this comment.
| * Builds the ENSRainbow public configuration from an ENSRainbowConfig object and server state. | |
| * | |
| * @param config - The validated ENSRainbowConfig object | |
| * Builds the ENSRainbow public configuration from server state. | |
| * |
The JSDoc comment for buildENSRainbowPublicConfig contains an incorrect parameter description that doesn't match the actual function signature.
View Details
Analysis
Incorrect JSDoc parameters for buildENSRainbowPublicConfig()
What fails: The JSDoc comment for buildENSRainbowPublicConfig() in apps/ensrainbow/src/config/config.schema.ts documents a non-existent config parameter and incorrectly describes the function as taking "an ENSRainbowConfig object", while the actual function signature only takes labelSet and recordsCount parameters.
How to reproduce:
# View the JSDoc and function signature:
cat apps/ensrainbow/src/config/config.schema.ts | sed -n '126,136p'Expected vs Actual:
- Before fix: JSDoc referenced
@param config - The validated ENSRainbowConfig objectand first line said "from an ENSRainbowConfig object and server state" - After fix: JSDoc only documents
@param labelSetand@param recordsCount, matching the actual function signature which takes only those two parameters - Result: IDE tooltips and autocomplete now show correct parameter documentation
There was a problem hiding this comment.
This is valid feedback. Please see my other related comment for how the private config really should be passed into this function.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@djstrong Hey great to see this. Thanks for your updates. Reviewed and shared feedback 👍
| }); | ||
|
|
||
| /** | ||
| * @deprecated Use GET /v1/config instead. This endpoint will be removed in a future version. |
There was a problem hiding this comment.
Can you please create a follow-up issue on GitHub to remind us to take this action? Thanks
| }); | ||
|
|
||
| api.get("/v1/config", async (c: HonoContext) => { | ||
| logger.debug("Config request"); |
There was a problem hiding this comment.
Suggest we remove these before merging. Can you verify it works locally as expected?
There was a problem hiding this comment.
We have logger.debug in every endpoint should we remove all of them?
| */ | ||
| .check(invariant_dbSchemaVersionMatch); | ||
|
|
||
| export type ENSRainbowConfig = z.infer<typeof ENSRainbowConfigSchema>; |
There was a problem hiding this comment.
Prefer that we not use this strategy for implicitly defining what an ENSRainbowConfig is. This makes it more difficult for devs to figure out what this very important object is. It also means we don't have nice JSDocs on each of the fields on the config object.
Instead, it will improve our DX moving forward if you explicitly define what an ENSRainbowConfig is, similar to what we did here for ENSIndexerConfig: https://github.com/namehash/ensnode/blob/main/apps/ensindexer/src/config/types.ts
|
|
||
| const config = buildConfigFromEnvironment(env); | ||
|
|
||
| expect(config.dbSchemaVersion).toBeUndefined(); |
There was a problem hiding this comment.
Please see my related comments on this.
The config object should reflect defaults and not pass on the complexity across our code that a key config value might be undefined.
|
|
||
| const config = buildConfigFromEnvironment(env); | ||
|
|
||
| expect(config.dbSchemaVersion).toBeUndefined(); |
There was a problem hiding this comment.
This also should never be allowed to be undefined. The config object needs to trap certain complexity within itself, such as applying defaults. These complexities should not leak out to other layers of our code.
| * @returns The validated label set configuration object, or undefined if neither is set | ||
| * @throws Error if only one of the label set variables is provided | ||
| */ | ||
| function validateLabelSetConfiguration( |
There was a problem hiding this comment.
it looks like this should be converted into an invariant check such as these: https://github.com/namehash/ensnode/blob/main/apps/ensindexer/src/config/validations.ts
| default: getDefaultDataDir(), | ||
| }); | ||
| }, | ||
| async (argv: ArgumentsCamelCase<ServeArgs>) => { |
There was a problem hiding this comment.
For the case of the serve command, what do you think about logging to the console the config that ENSRainbow is running with? Similar to https://github.com/namehash/ensnode/blob/main/apps/ensindexer/ponder/ponder.config.ts#L8-L13 except for ENSRainbow I assume there's no need to "redact" the config.
Related to #1407
Lite PR
Summary
/v1/configendpoint returning public config (version, label set, records count) and deprecated/v1/versionWhy
Testing
config.schema.test.ts) covering success cases, validation errors, invariants, and edge cases/v1/configendpoint in server command testsNotes for Reviewer (Optional)
/v1/versionendpoint is deprecated but still functional for backward compatibilityPre-Review Checklist (Blocking)