refactor(config): per-network shared-value inheritance + resolve #570 review findings

apps/backend/.env.example

@@ -6,16 +6,28 @@
 # Multi-network primer
 # --------------------
 # `NETWORKS` is a comma-separated list of Filecoin networks the instance should
-# drive (supported: `calibration`, `mainnet`). Every network-scoped variable is
-# namespaced with the UPPERCASE network name:
+# drive (supported: `calibration`, `mainnet`).
 #
 #   NETWORKS=calibration,mainnet
-#   CALIBRATION_WALLET_PRIVATE_KEY=0x...
-#   MAINNET_WALLET_PRIVATE_KEY=0x...
 #
-# Globals (database, jobs, timeouts, HTTP ports, etc.) remain unprefixed and
-# apply to all networks. Variables for INACTIVE networks are ignored; only
-# active networks are validated at startup.
+# Each per-network variable resolves as:
+#   <NETWORK>_<VAR>  (per-network override)
+#     -> <VAR>       (unprefixed shared value, applies to all active networks)
+#       -> default
+#
+# So set a shared value once and override only where a network differs:
+#   DEAL_JOB_TIMEOUT_SECONDS=360            # shared by both networks
+#   MAINNET_DEALS_PER_SP_PER_HOUR=1         # mainnet-only override
+#
+# Chain-specific variables never inherit the unprefixed slot and MUST be
+# prefixed: WALLET_ADDRESS, WALLET_PRIVATE_KEY, SESSION_KEY_PRIVATE_KEY, RPC_URL,
+# PDP_SUBGRAPH_ENDPOINT, SUBGRAPH_ENDPOINT, DEALBOT_DATASET_VERSION, BLOCKED_SP_IDS,
+# BLOCKED_SP_ADDRESSES, DATASET_LIFECYCLE_CHECK_ENABLED (network-dependent default:
+# off on mainnet, so a shared =true must not enable the canary there).
+#
+# Process globals (database, HTTP ports, ClickHouse, pg-boss) have no per-network
+# form. Variables for INACTIVE networks are ignored; only active networks are
+# validated at startup.
 # =============================================================================
 
 # -----------------------------------------------------------------------------

apps/backend/src/config/config-pipeline.spec.ts

@@ -0,0 +1,184 @@
+import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import { validateConfig } from "./env.schema.js";
+import { loadConfig } from "./loader.js";
+
+/**
+ * End-to-end of the config pipeline as NestJS runs it: `validateConfig` first
+ * (legacy/rename translation + Joi), then `loadConfig` reading `process.env`.
+ * Both touch the real `process.env`, so each case snapshots and restores it.
+ */
+const TOUCHED = [
+  "NETWORK",
+  "NETWORKS",
+  "DATABASE_HOST",
+  "DATABASE_USER",
+  "DATABASE_PASSWORD",
+  "DATABASE_NAME",
+  "WALLET_ADDRESS",
+  "SESSION_KEY_PRIVATE_KEY",
+  "WALLET_PRIVATE_KEY",
+  "RPC_URL",
+  "DEALBOT_MAINTENANCE_WINDOWS_UTC",
+  "MAINTENANCE_WINDOWS_UTC",
+  "DEALBOT_MAINTENANCE_WINDOW_MINUTES",
+  "MAINTENANCE_WINDOW_MINUTES",
+  "DEAL_JOB_TIMEOUT_SECONDS",
+  "DATASET_LIFECYCLE_CHECK_ENABLED",
+  "CALIBRATION_WALLET_ADDRESS",
+  "CALIBRATION_SESSION_KEY_PRIVATE_KEY",
+  "CALIBRATION_WALLET_PRIVATE_KEY",
+  "CALIBRATION_RPC_URL",
+  "CALIBRATION_MAINTENANCE_WINDOWS_UTC",
+  "CALIBRATION_DEAL_JOB_TIMEOUT_SECONDS",
+  "CALIBRATION_DATASET_LIFECYCLE_CHECK_ENABLED",
+  "MAINNET_WALLET_PRIVATE_KEY",
+  "MAINNET_WALLET_ADDRESS",
+  "MAINNET_DEAL_JOB_TIMEOUT_SECONDS",
+  "MAINNET_DATASET_LIFECYCLE_CHECK_ENABLED",
+];
+
+const snapshot: Record<string, string | undefined> = {};
+
+beforeEach(() => {
+  for (const key of TOUCHED) {
+    snapshot[key] = process.env[key];
+    delete process.env[key];
+  }
+  process.env.DATABASE_HOST = "localhost";
+  process.env.DATABASE_USER = "test";
+  process.env.DATABASE_PASSWORD = "test";
+  process.env.DATABASE_NAME = "test";
+});
+
+afterEach(() => {
+  for (const key of TOUCHED) {
+    if (snapshot[key] === undefined) delete process.env[key];
+    else process.env[key] = snapshot[key];
+  }
+});
+
+describe("config pipeline (validateConfig -> loadConfig)", () => {
+  it("rolls a legacy single-network deployment forward, including renamed vars", () => {
+    // Staging-style: legacy NETWORK + session-key mode + an old maintenance name.
+    process.env.NETWORK = "calibration";
+    process.env.WALLET_ADDRESS = "0xabc";
+    process.env.SESSION_KEY_PRIVATE_KEY = "0xsession";
+    process.env.RPC_URL = "http://erpc.local/main/evm/314159";
+    process.env.DEALBOT_MAINTENANCE_WINDOWS_UTC = "01:00,13:00";
+
+    expect(() => validateConfig(process.env as Record<string, unknown>)).not.toThrow();
+    const cfg = loadConfig();
+
+    expect(cfg.activeNetworks).toEqual(["calibration"]);
+    expect(cfg.networks.calibration.rpcUrl).toBe("http://erpc.local/main/evm/314159");
+    expect(cfg.networks.calibration.maintenanceWindowsUtc).toEqual(["01:00", "13:00"]);
+    if ("sessionKeyPrivateKey" in cfg.networks.calibration) {
+      expect(cfg.networks.calibration.sessionKeyPrivateKey).toBe("0xsession");
+    } else {
+      throw new Error("expected session-key variant");
+    }
+  });
+
+  it("validates and loads a shared value with a per-network override across both networks", () => {
+    process.env.NETWORKS = "calibration,mainnet";
+    process.env.CALIBRATION_WALLET_PRIVATE_KEY = "0xcal";
+    process.env.CALIBRATION_WALLET_ADDRESS = "0xcaladdr";
+    process.env.MAINNET_WALLET_PRIVATE_KEY = "0xmain";
+    process.env.MAINNET_WALLET_ADDRESS = "0xmainaddr";
+    process.env.DEAL_JOB_TIMEOUT_SECONDS = "500"; // shared, validated once
+    process.env.MAINNET_DEAL_JOB_TIMEOUT_SECONDS = "900"; // override
+
+    expect(() => validateConfig(process.env as Record<string, unknown>)).not.toThrow();
+    const cfg = loadConfig();
+
+    expect(cfg.activeNetworks).toEqual(["calibration", "mainnet"]);
+    expect(cfg.networks.calibration.dealJobTimeoutSeconds).toBe(500);
+    expect(cfg.networks.mainnet.dealJobTimeoutSeconds).toBe(900);
+  });
+
+  it("rejects an out-of-range shared value before load", () => {
+    process.env.NETWORKS = "calibration";
+    process.env.CALIBRATION_WALLET_PRIVATE_KEY = "0xcal";
+    process.env.CALIBRATION_WALLET_ADDRESS = "0xcaladdr";
+    process.env.DEAL_JOB_TIMEOUT_SECONDS = "1"; // below min(120)
+
+    expect(() => validateConfig(process.env as Record<string, unknown>)).toThrow(/DEAL_JOB_TIMEOUT_SECONDS/);
+  });
+
+  it("carries a shared value sourced only from a .env file (not the OS env) through to the loader", () => {
+    // Faithful to @nestjs/config: it validates a merged object, then assigns
+    // ONLY the returned keys back into process.env (and only when not already
+    // present). A `.env`-file value lives in that object, not in process.env, so
+    // the loader sees it only if validateConfig keeps it in its output.
+    const envFile: Record<string, string> = {
+      NETWORKS: "calibration,mainnet",
+      DATABASE_HOST: "localhost",
+      DATABASE_USER: "test",
+      DATABASE_PASSWORD: "test",
+      DATABASE_NAME: "test",
+      CALIBRATION_WALLET_PRIVATE_KEY: "0xcal",
+      CALIBRATION_WALLET_ADDRESS: "0xcaladdr",
+      MAINNET_WALLET_PRIVATE_KEY: "0xmain",
+      MAINNET_WALLET_ADDRESS: "0xmainaddr",
+      DEAL_JOB_TIMEOUT_SECONDS: "500", // shared, file-only
+    };
+
+    const config = { ...envFile, ...process.env };
+    const validated = validateConfig(config) as Record<string, unknown>;
+    for (const key of Object.keys(validated)) {
+      if (!(key in process.env)) process.env[key] = String(validated[key]);
+    }
+
+    const cfg = loadConfig();
+    expect(cfg.networks.calibration.dealJobTimeoutSeconds).toBe(500);
+    expect(cfg.networks.mainnet.dealJobTimeoutSeconds).toBe(500);
+  });
+
+  it("promotes a renamed legacy var through the Nest copy path to both networks", () => {
+    // Multi-network + only the OLD name set. The rename happens on Nest's copy;
+    // the promoted current name must survive validation and be assigned to
+    // process.env so the loader inherits it (regression for the strip bug).
+    const envFile: Record<string, string> = {
+      NETWORKS: "calibration,mainnet",
+      DATABASE_HOST: "localhost",
+      DATABASE_USER: "test",
+      DATABASE_PASSWORD: "test",
+      DATABASE_NAME: "test",
+      CALIBRATION_WALLET_PRIVATE_KEY: "0xcal",
+      CALIBRATION_WALLET_ADDRESS: "0xcaladdr",
+      MAINNET_WALLET_PRIVATE_KEY: "0xmain",
+      MAINNET_WALLET_ADDRESS: "0xmainaddr",
+      DEALBOT_MAINTENANCE_WINDOW_MINUTES: "45", // old name only
+    };
+
+    const config = { ...envFile, ...process.env };
+    const validated = validateConfig(config) as Record<string, unknown>;
+    for (const key of Object.keys(validated)) {
+      if (!(key in process.env)) process.env[key] = String(validated[key]);
+    }
+
+    const cfg = loadConfig();
+    expect(cfg.networks.calibration.maintenanceWindowMinutes).toBe(45);
+    expect(cfg.networks.mainnet.maintenanceWindowMinutes).toBe(45);
+  });
+
+  it("does not inject a prefixed per-network Joi default into process.env", () => {
+    // The core inheritance fix: prefixed keys carry no Joi default, so an absent
+    // override is NOT written back to process.env where it would shadow a shared value.
+    const config = {
+      NETWORKS: "calibration",
+      DATABASE_HOST: "localhost",
+      DATABASE_USER: "test",
+      DATABASE_PASSWORD: "test",
+      DATABASE_NAME: "test",
+      CALIBRATION_WALLET_PRIVATE_KEY: "0xcal",
+      CALIBRATION_WALLET_ADDRESS: "0xcaladdr",
+    };
+    const validated = validateConfig(config) as Record<string, unknown>;
+    for (const key of Object.keys(validated)) {
+      if (!(key in process.env)) process.env[key] = String(validated[key]);
+    }
+
+    expect(process.env.CALIBRATION_DEAL_JOB_TIMEOUT_SECONDS).toBeUndefined();
+  });
+});

apps/backend/src/config/env.helpers.spec.ts

@@ -0,0 +1,36 @@
+import { describe, expect, it } from "vitest";
+import { coerceBoolean, coerceFloat, coerceNumber } from "./env.helpers.js";
+
+describe("coerceBoolean", () => {
+  it("returns the fallback when the value is absent", () => {
+    expect(coerceBoolean(undefined, true)).toBe(true);
+    expect(coerceBoolean(undefined, false)).toBe(false);
+  });
+
+  it("parses case-insensitive true/false", () => {
+    expect(coerceBoolean("true", false)).toBe(true);
+    expect(coerceBoolean("TRUE", false)).toBe(true);
+    expect(coerceBoolean("False", true)).toBe(false);
+    expect(coerceBoolean(" false ", true)).toBe(false);
+  });
+
+  it("does NOT treat 0/no/off as true (the old footgun)", () => {
+    // Old behaviour returned `value !== "false"`, so these were all true.
+    expect(coerceBoolean("0", true)).toBe(true); // unrecognised -> fallback, not forced true
+    expect(coerceBoolean("0", false)).toBe(false);
+    expect(coerceBoolean("no", false)).toBe(false);
+  });
+});
+
+describe("coerceNumber / coerceFloat", () => {
+  it("parses numeric strings and floats", () => {
+    expect(coerceNumber("42", 0)).toBe(42);
+    expect(coerceFloat("0.25", 0)).toBe(0.25);
+  });
+
+  it("returns the fallback for empty/absent values", () => {
+    expect(coerceNumber(undefined, 7)).toBe(7);
+    expect(coerceNumber("", 7)).toBe(7);
+    expect(coerceFloat(undefined, 1.5)).toBe(1.5);
+  });
+});

apps/backend/src/config/env.helpers.ts

@@ -1,24 +1,44 @@
 /**
  * Raw environment variable accessors.
  *
- * These are purely mechanical: they read a key from `process.env` (or any
- * `NodeJS.ProcessEnv` map) and coerce it to the requested primitive type,
- * falling back to a default when the key is absent or empty.
+ * Two layers:
+ *   - `coerce*` take an already-resolved string value and coerce it to a
+ *     primitive, falling back to a default when absent/empty. The per-network
+ *     loader uses these because it resolves the value itself (override → shared).
+ *   - `get*Env` read a single key from a `NodeJS.ProcessEnv` map and coerce it.
+ *     Global (non per-network) config uses these.
  */
 
-export const getStringEnv = (env: NodeJS.ProcessEnv, key: string, fallback: string): string => env[key] || fallback;
+export const coerceNumber = (value: string | undefined, fallback: number): number =>
+  value ? Number.parseInt(value, 10) : fallback;
 
-export const getNumberEnv = (env: NodeJS.ProcessEnv, key: string, fallback: number): number => {
-  const value = env[key];
-  return value ? Number.parseInt(value, 10) : fallback;
-};
+export const coerceFloat = (value: string | undefined, fallback: number): number =>
+  value ? Number.parseFloat(value) : fallback;
 
-export const getFloatEnv = (env: NodeJS.ProcessEnv, key: string, fallback: number): number => {
-  const value = env[key];
-  return value ? Number.parseFloat(value) : fallback;
+/**
+ * Coerces a boolean env value, matching Joi's case-insensitive `true`/`false`
+ * (so `"true"`, `"TRUE"`, `"false"`, `"False"` all parse to the obvious value).
+ * Any other string — `"0"`, `"1"`, `"no"`, `"yes"`, etc. — is unrecognized and
+ * returns the fallback rather than parsing as `false`: for a key whose default
+ * is `true` (e.g. `CHECK_DATASET_CREATION_FEES`), setting `"0"`/`"no"` does NOT
+ * disable it. (Joi rejects unrecognized values at boot for registered keys, so
+ * the loader never sees them there.)
+ */
+export const coerceBoolean = (value: string | undefined, fallback: boolean): boolean => {
+  if (value === undefined) return fallback;
+  const normalized = value.trim().toLowerCase();
+  if (normalized === "true") return true;
+  if (normalized === "false") return false;
+  return fallback;
 };
 
-export const getBooleanEnv = (env: NodeJS.ProcessEnv, key: string, fallback: boolean): boolean => {
-  const value = env[key];
-  return value !== undefined ? value !== "false" : fallback;
-};
+export const getStringEnv = (env: NodeJS.ProcessEnv, key: string, fallback: string): string => env[key] || fallback;
+
+export const getNumberEnv = (env: NodeJS.ProcessEnv, key: string, fallback: number): number =>
+  coerceNumber(env[key], fallback);
+
+export const getFloatEnv = (env: NodeJS.ProcessEnv, key: string, fallback: number): number =>
+  coerceFloat(env[key], fallback);
+
+export const getBooleanEnv = (env: NodeJS.ProcessEnv, key: string, fallback: boolean): boolean =>
+  coerceBoolean(env[key], fallback);

apps/backend/src/config/env.schema.spec.ts

@@ -506,4 +506,70 @@ describe("createConfigValidationSchema", () => {
       expect(a).not.toBe(b);
     });
   });
+
+  describe("shared (unprefixed) override validation", () => {
+    it("accepts an in-range shared value", () => {
+      const { error } = validate(schemaFor("calibration"), {
+        ...baseEnv,
+        NETWORKS: "calibration",
+        ...withWalletKey("CALIBRATION"),
+        DEALS_PER_SP_PER_HOUR: 4,
+      });
+      expect(error).toBeUndefined();
+    });
+
+    it("rejects an out-of-range shared value (not waved through by allowUnknown)", () => {
+      const { error } = validate(schemaFor("calibration"), {
+        ...baseEnv,
+        NETWORKS: "calibration",
+        ...withWalletKey("CALIBRATION"),
+        DEALS_PER_SP_PER_HOUR: 999,
+      });
+      expect(error).toBeDefined();
+      expect(error?.message).toMatch(/DEALS_PER_SP_PER_HOUR/);
+    });
+
+    it("rejects an invalid shared maintenance-window format", () => {
+      const { error } = validate(schemaFor("calibration"), {
+        ...baseEnv,
+        NETWORKS: "calibration",
+        ...withWalletKey("CALIBRATION"),
+        MAINTENANCE_WINDOWS_UTC: "not-a-time",
+      });
+      expect(error).toBeDefined();
+    });
+
+    it("does not register a bare key for a chain-specific var", () => {
+      // BLOCKED_SP_IDS is chain-specific; an unprefixed value is unknown to the
+      // schema and only allowed because allowUnknown is on (never validated as shared).
+      const { error } = validate(schemaFor("calibration"), {
+        ...baseEnv,
+        NETWORKS: "calibration",
+        ...withWalletKey("CALIBRATION"),
+        BLOCKED_SP_IDS: "1,2,3",
+      });
+      expect(error).toBeUndefined();
+    });
+  });
+
+  describe("NETWORKS validation", () => {
+    it("rejects a present-but-empty NETWORKS (whitespace/commas)", () => {
+      const { error } = validate(schemaFor("calibration"), {
+        ...baseEnv,
+        NETWORKS: " , ,",
+        ...withWalletKey("CALIBRATION"),
+      });
+      expect(error).toBeDefined();
+      expect(error?.message).toMatch(/NETWORKS must list at least one supported network/);
+    });
+
+    it("rejects an unsupported network name", () => {
+      const { error } = validate(schemaFor("calibration"), {
+        ...baseEnv,
+        NETWORKS: "calibration,moonbase",
+        ...withWalletKey("CALIBRATION"),
+      });
+      expect(error).toBeDefined();
+    });
+  });
 });