Skip to content

Conversation

@avivkeller
Copy link
Contributor

@avivkeller avivkeller commented Aug 14, 2025

Description

Upgrades all deps to LTS Jod's latest compat version.

Changes

Changes were done via ncu -u followed by manual editing of the files that needed to be ported.

How to test

All tests should pass, as they do locally.

Summary by CodeRabbit

  • New Features

    • Seamless migration for older conditional orders without IDs to the current format.
  • Bug Fixes

    • More robust startup by tolerating partially saved last processed block data.
  • Chores

    • Switched to cross-platform build/test tooling (rimraf, cpx); streamlined Husky hooks.
    • Added lint-staged for auto-fixing TypeScript files on commit.
    • Upgraded dependencies (including Express 5 and tooling).
  • Tests

    • Migrated to Node’s built-in test runner and assertions.
  • Style

    • Consistent formatting updates (trailing commas) and minor import cleanups; no behavior changes.

@coderabbitai
Copy link

coderabbitai bot commented Aug 14, 2025

Walkthrough

Project migrates ESLint config to flat config, updates Husky hooks and adds lint-staged. package.json switches to cross-platform build tools, runs tests via node test runner on TS sources, and updates many dependencies. Source files receive formatting (trailing commas), Slack imports move to ESM, and model adds data migration for legacy conditional orders.

Changes

Cohort / File(s) Summary
Linting and tooling config
eslint.config.mjs, .eslintrc.json, .lintstagedrc, .husky/commit-msg, .husky/pre-commit, src/types/types.d.ts
Move to ESLint flat config; remove old .eslintrc. Add lint-staged mapping for TS files. Simplify Husky hooks (direct npx calls, remove shebang/bootstrap). Re-enable linting on types.d.ts by removing disable comment.
Build, test, and dependencies
package.json
Replace rm/cp with rimraf/cpx; run tests via ts-node with node’s test runner; remove Jest config. Broad dependency updates (e.g., express 5, typescript 5.9, typescript-eslint, chalk v5), add tool deps, remove node-fetch.
Model and context updates
src/types/model.ts, src/utils/context.ts
Switch Slack import to ESM default. Add migration for legacy conditional orders (compute id when missing). Adjust Registry.load to parse partial lastProcessedBlock and cast to full type. Formatting only elsewhere.
Logging types alignment
src/utils/logging.ts
Update Chalk typings to ChalkInstance; minor formatting. No logic change.
Formatting-only changes
src/commands/run.ts, src/domain/events/index.ts, src/domain/polling/filtering/policy.ts, src/domain/polling/index.ts, src/domain/polling/poll.ts, src/services/api.ts, src/services/chain.ts, src/utils/contracts.ts, src/utils/misc.ts, src/index.ts
Trailing commas and minor syntactic tweaks; no behavioral changes. One or two signatures show trailing-comma-only edits.
Tests migration
src/utils/utils.spec.ts
Move to node:test and node:assert (partialDeepStrictEqual). Replace Jest-style assertions; minor formatting.

Sequence Diagram(s)

sequenceDiagram
  participant App as App Startup
  participant Registry as Registry.load
  participant Store as Persistent Store
  participant Model as parseConditionalOrders
  participant SDK as ConditionalOrderSdk

  App->>Registry: load(chainId, provider, options)
  Registry->>Store: get lastProcessedBlock, orders
  Store-->>Registry: raw JSON / partial data
  Registry->>Model: parseConditionalOrders(rawOrders, _version)
  alt _version < 2 and missing id
    Model->>SDK: leafToId(order.params)
    SDK-->>Model: id
    Model-->>Registry: migrated orders with ids
  else current format
    Model-->>Registry: orders unchanged
  end
  Registry-->>App: Registry instance (casted lastProcessedBlock)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A nibble of commas, a sprinkle of lint,
I thump with delight at tools in mint.
Hooks hop lighter, tests now dash,
Old orders get IDs in a flash.
With ESM carrots and Chalk so bright—
This bunny ships clean code tonight! 🥕✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch upgrade-deps

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@socket-security
Copy link

@socket-security
Copy link

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action Severity Alert  (click "▶" to expand/collapse)
Warn High
[email protected] is Protestware or potentially unwanted behavior.

Note: The script attempts to run a local post-install script, which could potentially contain malicious code. The error handling suggests that it is designed to fail silently, which is a common tactic in malicious scripts.

From: yarn.locknpm/[email protected]

ℹ Read more on: This package | This alert | What is protestware?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Consider that consuming this package may come along with functionality unrelated to its primary purpose.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🔭 Outside diff range comments (5)
src/utils/context.ts (1)

96-106: Fix potential undefined access for lastNotifiedError

The check uses !== null, which still passes when lastNotifiedError is undefined, leading to a runtime error on getTime(). Use a stricter guard.

-  if (registry.lastNotifiedError !== null) {
+  if (registry.lastNotifiedError instanceof Date) {
     const nextErrorNotificationTime =
       registry.lastNotifiedError.getTime() + NOTIFICATION_WAIT_PERIOD;
     if (Date.now() < nextErrorNotificationTime) {
       log.warn(
         `Last error notification happened earlier than ${
           NOTIFICATION_WAIT_PERIOD / 60_000
         } minutes ago. Next notification will happen after ${new Date(
-          nextErrorNotificationTime,
-        )}`,
+          nextErrorNotificationTime,
+        )}`,
       );
       return false;
     }
   }
src/domain/events/index.ts (2)

152-158: Fix decoding of proof.data: decode returns a tuple; iterate over the decoded array

defaultAbiCoder.decode(["bytes[]"], ...) returns [string[]]. The current loop iterates the wrapper array once, causing decode errors by passing an array to decode(). Destructure first, then iterate the bytes array.

-        const proofData = ethers.utils.defaultAbiCoder.decode(
-          ["bytes[]"],
-          proof.data as BytesLike,
-        );
-
-        for (const order of proofData) {
+        const [orders] = ethers.utils.defaultAbiCoder.decode(
+          ["bytes[]"],
+          proof.data as BytesLike,
+        ) as [string[]];
+
+        for (const order of orders) {

168-176: Accumulate addOrder results across multiple Merkle entries

added is overwritten per iteration. If at least one add succeeds but a later one fails, added may end false, misleading the caller. OR the results to reflect “added at least one”.

-          added = addOrder(
+          added = addOrder(
             event.transactionHash,
             owner,
             toConditionalOrderParams(decodedOrder[1]),
             { merkleRoot: root, path: decodedOrder[0] },
             eventLog.address,
             event.blockNumber,
             context,
-          );
+          ) || added;
src/domain/polling/index.ts (1)

151-162: Off-by-one: chunked persistence triggers at counts 1, 51, 101… instead of 50, 100, 150

Condition should persist every CHUNK_SIZE updates (and not at the first update).

-      if (updatedCount % CHUNK_SIZE === 1 && updatedCount > 1) {
+      if (updatedCount > 0 && updatedCount % CHUNK_SIZE === CHUNK_SIZE - 0) {
         // Delete orders pending delete, if any
         deleteOrders(ordersPendingDelete, conditionalOrders, log, chainId);
         // Reset tracker
         ordersPendingDelete = [];
 
         log.debug(`Processed ${updatedCount}, saving registry`);
 
         // Save the registry after processing each chunk
         await registry.write();
       }

If you prefer clarity:

- if (updatedCount % CHUNK_SIZE === 1 && updatedCount > 1) {
+ if (updatedCount > 0 && updatedCount % CHUNK_SIZE === 0) {
src/utils/contracts.ts (1)

166-172: Address linter warning: declarations inside switch cases should be block-scoped

Biome flagged noSwitchDeclarations. Wrap case bodies that declare variables in braces to prevent leaking across cases.

-    case CustomErrorSelectors.ORDER_NOT_VALID:
-    case CustomErrorSelectors.POLL_TRY_NEXT_BLOCK:
-    case CustomErrorSelectors.POLL_NEVER:
-      const [message] = iface.decodeErrorResult(fragment, revertData) as [
-        string,
-      ];
-      return { selector, message };
+    case CustomErrorSelectors.ORDER_NOT_VALID:
+    case CustomErrorSelectors.POLL_TRY_NEXT_BLOCK:
+    case CustomErrorSelectors.POLL_NEVER: {
+      const [message] = iface.decodeErrorResult(fragment, revertData) as [string];
+      return { selector, message };
+    }
-    case CustomErrorSelectors.POLL_TRY_AT_BLOCK:
-    case CustomErrorSelectors.POLL_TRY_AT_EPOCH:
-      const [blockNumberOrEpoch, msg] = iface.decodeErrorResult(
-        fragment,
-        revertData,
-      ) as [BigNumber, string];
-
-      // It is reasonable to expect that the block number or epoch is bound by
-      // uint32. It is therefore safe to throw if the value is outside of that
-      // for javascript's number type.
-      if (blockNumberOrEpoch.gt(MAX_UINT32)) {
-        throw new Error("Block number or epoch out of bounds");
-      }
-
-      return {
-        selector,
-        message: msg,
-        blockNumberOrEpoch: blockNumberOrEpoch.toNumber(),
-      };
+    case CustomErrorSelectors.POLL_TRY_AT_BLOCK:
+    case CustomErrorSelectors.POLL_TRY_AT_EPOCH: {
+      const [blockNumberOrEpoch, msg] = iface.decodeErrorResult(
+        fragment,
+        revertData,
+      ) as [BigNumber, string];
+      if (blockNumberOrEpoch.gt(MAX_UINT32)) {
+        throw new Error("Block number or epoch out of bounds");
+      }
+      return {
+        selector,
+        message: msg,
+        blockNumberOrEpoch: blockNumberOrEpoch.toNumber(),
+      };
+    }

Also applies to: 174-192

🧹 Nitpick comments (8)
src/services/api.ts (1)

87-88: LGTM: log message formatting

No behavioral change. Nit: consider logging the full base URL only once at startup to avoid duplicate info if start() is called repeatedly after stop().

src/utils/logging.ts (1)

19-25: Guard against unknown/unsupported log levels to avoid runtime errors

If an unexpected level string arrives (plugin/config drift), COLORS[level.toUpperCase()] could be undefined and calling it would throw.

-  return `${chalk.gray(timestamp)} ${COLORS[level.toUpperCase()](
-        level,
-      )} ${chalk.green(`${name}:`)}`;
+  const color = COLORS[level.toUpperCase()] ?? ((s: string) => s);
+  return `${chalk.gray(timestamp)} ${color(level)} ${chalk.green(`${name}:`)}`;

Also applies to: 43-46

src/domain/polling/index.ts (1)

303-308: Improve delete log: joining objects prints “[object Object]”

ordersPendingDelete.join(", ") won’t show useful info. Map to IDs first.

-  if (ordersPendingDelete.length)
-    log.debug(
-      `Delete ${ordersPendingDelete.length} orders: ${ordersPendingDelete.join(
-        ", ",
-      )}`,
-    );
+  if (ordersPendingDelete.length) {
+    const ids = ordersPendingDelete.map((o) => o.id).join(", ");
+    log.debug(`Delete ${ordersPendingDelete.length} orders: ${ids}`);
+  }
src/utils/utils.spec.ts (1)

1-2: Optional: Prefer strict assert.

You can import the strict variant for consistency with deepStrictEqual semantics.

-import assert from "node:assert";
+import assert from "node:assert/strict";
src/types/model.ts (2)

174-181: Potentially partial lastProcessedBlock is cast to full type.

lastProcessedBlock may be parsed as { number: genesisBlockNumber - 1 } and is later cast to RegistryBlock. Downstream code expects timestamp and hash when persisted or logged. It appears you immediately overwrite it with a full block before first write in the warm-up flow, but the cast is unsafe.

Safer alternative: keep it Partial<RegistryBlock> | null internally or fill defaults before casting.

-      lastProcessedBlock as RegistryBlock,
+      (lastProcessedBlock ?? null) as RegistryBlock,

And/or ensure callers can handle missing fields until the first persist. Alternatively, initialize with a full block shape:

// when defaulting
{ number: genesisBlockNumber - 1, timestamp: 0, hash: "" }

Also applies to: 195-196


307-311: Missing handling for absent registry version key.

Number(await db.get(...)) will throw if the version key is absent. While the caller wraps loadOwnerOrders in a catch, this also masks genuine parse issues. Consider catching only NotFound errors here and defaulting version to undefined to keep other errors visible.

-  const version = Number(
-    await db.get(
-      getNetworkStorageKey(CONDITIONAL_ORDER_REGISTRY_VERSION_KEY, network),
-    ),
-  );
+  let version: number | undefined;
+  try {
+    const v = await db.get(
+      getNetworkStorageKey(CONDITIONAL_ORDER_REGISTRY_VERSION_KEY, network),
+    );
+    version = Number(v);
+  } catch (e: any) {
+    // ignore not found; keep version undefined
+    version = undefined;
+  }
package.json (2)

15-15: Confirm Node version compatibility with node --test and ts-node.

The test script uses the Node test runner with ts-node register. Ensure your CI and engines target a Node version that supports the runner (Node 18.8+; ideally 20+). Consider declaring engines to prevent accidental usage with older Node.

 {
   "name": "@cowprotocol/watch-tower",
+  "engines": {
+    "node": ">=20"
+  },
   "license": "GPL-3.0-or-later",

22-40: Avoid duplicating ts-node in devDependencies and dependencies.

ts-node is listed in both sections. Keep it only where needed. If you only run tests/cli in development, keep it in devDependencies and remove it from dependencies.

   "dependencies": {
@@
-    "prom-client": "^15.1.3",
-    "ts-node": "^10.9.2",
-    "typescript": "^5.9.2"
+    "prom-client": "^15.1.3",
+    "typescript": "^5.9.2"
   }

If the production runtime relies on the "cli" script using ts-node, consider:

  • either shipping compiled JS for CLI in dist and pointing a "bin" to it, or
  • keeping ts-node only as a dev tool and using tsx locally.

Also applies to: 58-59

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d55d635 and 08e5bda.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (21)
  • .eslintrc.json (0 hunks)
  • .husky/commit-msg (1 hunks)
  • .husky/pre-commit (1 hunks)
  • .lintstagedrc (1 hunks)
  • eslint.config.mjs (1 hunks)
  • package.json (1 hunks)
  • src/commands/run.ts (1 hunks)
  • src/domain/events/index.ts (13 hunks)
  • src/domain/polling/filtering/policy.ts (1 hunks)
  • src/domain/polling/index.ts (24 hunks)
  • src/domain/polling/poll.ts (3 hunks)
  • src/index.ts (4 hunks)
  • src/services/api.ts (3 hunks)
  • src/services/chain.ts (17 hunks)
  • src/types/model.ts (9 hunks)
  • src/types/types.d.ts (0 hunks)
  • src/utils/context.ts (6 hunks)
  • src/utils/contracts.ts (8 hunks)
  • src/utils/logging.ts (7 hunks)
  • src/utils/misc.ts (1 hunks)
  • src/utils/utils.spec.ts (4 hunks)
💤 Files with no reviewable changes (2)
  • src/types/types.d.ts
  • .eslintrc.json
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/utils/utils.spec.ts (1)
src/utils/contracts.ts (3)
  • parseCustomError (133-193)
  • CUSTOM_ERROR_ABI_MAP (93-105)
  • handleOnChainCustomError (199-323)
src/utils/context.ts (1)
src/types/index.ts (1)
  • ContextOptions (22-22)
src/domain/events/index.ts (1)
src/services/chain.ts (1)
  • ChainContext (73-453)
src/domain/polling/index.ts (1)
src/types/model.ts (1)
  • numOwners (203-205)
🪛 GitHub Actions: Publish Docker image
src/utils/utils.spec.ts

[error] 1-1: Test failed: 'test failed' (ERR_TEST_FAILURE) in src/utils/utils.spec.ts.

src/utils/logging.ts

[error] 11-11: ERR_REQUIRE_ESM: require() of ES Module 'chalk' is not supported in this CommonJS module; use dynamic import() instead.

🪛 Biome (2.1.2)
src/utils/contracts.ts

[error] 278-278: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Safe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

🔇 Additional comments (16)
src/services/api.ts (2)

64-66: LGTM: formatting-only change

Trailing comma inside the config mapping is fine and keeps diffs minimal on future edits.


130-131: Ensure chainId is numeric before passing to Registry.dump

req.params.chainId is a string. If Registry.dump expects a number, pass a parsed integer and validate it to avoid subtle bugs.

Apply this diff to cast the param:

-        req.params.chainId,
+        Number.parseInt(req.params.chainId, 10),

Additionally, consider validating and returning 400 on invalid input (outside the changed lines):

const chainIdNum = Number.parseInt(req.params.chainId, 10);
if (Number.isNaN(chainIdNum)) {
  return res.status(400).json({ error: "Invalid chainId" });
}
const dump = await Registry.dump(DBService.getInstance(), chainIdNum);
src/commands/run.ts (1)

45-46: Formatting-only trailing commas — OK

Added trailing commas in ChainContext.init call and map callback. No behavioral impact.

Also applies to: 47-48

src/utils/misc.ts (1)

37-38: Trailing comma in parameter list — OK

Purely stylistic; no change in behavior or types.

src/domain/polling/poll.ts (1)

15-16: Formatting-only trailing commas — OK

Constructor arg, function parameter list, and log.debug call adjusted with trailing commas. No functional changes.

Also applies to: 25-26, 49-50

src/domain/polling/filtering/policy.ts (1)

87-89: Formatting-only trailing comma in map conversion — OK

No behavioral change; convertToMap still returns the expected Map.

src/index.ts (3)

28-30: Consistent trailing commas on CLI option definitions — OK

These changes align with the new formatting rules and don’t affect runtime behavior.

Also applies to: 36-38, 48-52, 56-59, 63-64, 68-71, 75-77, 81-83


169-171: Trailing comma in JSON.parse argument — OK

Stylistic only; no effect on JSON parsing or type inference of the schema.


157-161: Optional catch binding OK in TS output — confirm runtime Node/CI

tsconfig.json sets compilerOptions.target = "es2020", so the catch { } syntax will be emitted; package.json does not declare an engines.node constraint, so runtime/CI Node support is not guaranteed — please confirm or adjust.

  • src/index.ts (lines 157–161):
  } catch {
    throw new InvalidArgumentError(
      `${option} must be a valid '0x' prefixed address`,
    );
  }
  • tsconfig.json: compilerOptions.target = "es2020"
  • package.json: engines.node = (not set)

Recommended: either confirm CI/production Node versions support ES2019+ (optional catch binding), or set engines.node to the minimum supported Node version / replace catch { } with catch (err) for older Node compatibility.

src/utils/context.ts (1)

1-1: No change required — default import is safe with current config

tsconfig.json targets CommonJS and has "esModuleInterop": true, package.json has no "type" field (CJS runtime), node-slack is in dependencies (^0.0.7) and @types/node-slack is present — so import Slack from "node-slack"; will be compiled with the interop wrapper and work at runtime.

  • File to note: src/utils/context.ts — line 1: import Slack from "node-slack"; — keep as-is.
  • If you later switch to ESM (add "type": "module") or disable esModuleInterop, revert to the require/type-only patterns from the original suggestion.
src/domain/events/index.ts (1)

63-64: Minor: log strings OK

Formatting-only changes; no behavioral impact. Nothing to fix here.

Also applies to: 81-82

src/domain/polling/index.ts (1)

512-516: LGTM: tuple destructuring for unfilled orders

Using the ignored-first-element syntax improves readability; behavior unchanged.

src/utils/contracts.ts (1)

153-156: LGTM: ABI fragment construction

Trailing comma only; logic remains correct for fragment creation with ethers v5.

src/types/model.ts (1)

1-1: esModuleInterop is enabled — default import is OK

tsconfig.json contains "esModuleInterop": true (line 10), so import Slack from "node-slack" in src/types/model.ts is valid — no change required.

  • tsconfig.json — line 10: "esModuleInterop": true
  • src/types/model.ts — line 1: import Slack from "node-slack";

Action: leave the import as-is.

src/services/chain.ts (1)

221-222: LGTM: Formatting-only changes are safe.

Trailing commas and minor string template adjustments don’t alter behavior. Event decoding call signature remains valid for ethers v5.

Also applies to: 241-242, 231-237, 502-503, 528-529, 583-583, 371-371, 405-405, 444-444, 469-469, 595-595, 613-613

package.json (1)

49-51: Express 5 upgrade: verify middleware/signature changes.

Express 5 is a breaking change — confirm routes/middleware and error-handler signatures match the new typings.

Search findings (src):

  • src/services/api.ts:31 — res.send("🐮 Moooo!");
  • src/services/api.ts:133 — res.send(dump);
  • src/services/api.ts:135 — res.send(JSON.stringify(err));
  • src/services/api.ts:143 — res.send({ ... });

No occurrences of res.sendfile, res.jsonp, app.param/router.param or 4-arg error-handler functions were found in src. Please review the listed handlers and any other middleware sites (including files outside src) for compatibility and update typings/error middleware as needed.

. "$(dirname -- "$0")/_/husky.sh"

npx --no -- commitlint --edit ${1}
npx --no -- commitlint --edit ${1} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Quote the path argument, use --no, and add a shebang for robustness

  • Quote $1 to handle paths with spaces.
  • Use --no to avoid network resolution.
  • Add a shebang and strict mode for consistency and portability.

Apply this diff:

-npx --no -- commitlint --edit ${1}
+#!/usr/bin/env sh
+set -euo pipefail
+npx --no -- commitlint --edit "$1"

Confirm the hook is executable and consistent with pre-commit:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Executable bit
if [ ! -x .husky/commit-msg ]; then
  echo "ERROR: .husky/commit-msg is not executable. Run: chmod +x .husky/commit-msg"
  exit 1
fi

# Check content
nl -ba .husky/commit-msg

Length of output: 175


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Listing .husky directory:"
ls -la .husky || true
echo

echo "Checking .husky/commit-msg permissions and content:"
ls -l .husky/commit-msg || true
echo "---- content ----"
head -n 200 .husky/commit-msg || cat .husky/commit-msg || true
echo

echo "First 2 lines of other .husky hooks (if any):"
for f in .husky/*; do
  [ -f "$f" ] || continue
  echo "== $f =="
  head -n 2 "$f" || true
done

Length of output: 1081


Add a shebang + strict mode and quote the commit message path in .husky/commit-msg

Short: .husky/commit-msg is executable and already uses --no, but it lacks a shebang/strict mode and uses an unquoted ${1}. .husky/pre-commit also lacks a shebang — make hooks consistent.

Files to update:

  • .husky/commit-msg — currently: npx --no -- commitlint --edit ${1} (executable). Apply below patch.
  • .husky/pre-commit — currently: npx lint-staged (executable). Consider adding the same shebang/strict-mode for consistency.

Apply this diff for .husky/commit-msg:

-npx --no -- commitlint --edit ${1}
+#!/usr/bin/env bash
+set -euo pipefail
+npx --no -- commitlint --edit "$1"

Optional, for consistency, update .husky/pre-commit similarly:

-npx lint-staged
+#!/usr/bin/env bash
+set -euo pipefail
+npx --no -- lint-staged
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
npx --no -- commitlint --edit ${1}
#!/usr/bin/env bash
set -euo pipefail
npx --no -- commitlint --edit "$1"
🤖 Prompt for AI Agents
In .husky/commit-msg around line 1, the hook lacks a shebang and strict shell
settings and uses an unquoted ${1}; update the file to start with a POSIX sh
shebang (e.g. #!/usr/bin/env sh) and enable strict mode (set -euo pipefail or
set -e -u), and change the command to quote the commit message path as "${1}"
when invoking commitlint; also apply the same shebang/strict-mode header to
.husky/pre-commit for consistency.

. "$(dirname "$0")/_/husky.sh"

npx lint-staged
npx lint-staged No newline at end of file
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Use npx with --no, add a shebang, and restore a trailing newline for portability and reliability

  • Without a shebang, hook execution can be shell-dependent.
  • Using --no prevents accidental remote installs and enforces local devDependency usage.
  • File currently lacks a trailing newline.

Apply this diff:

-npx lint-staged
+#!/usr/bin/env sh
+set -euo pipefail
+npx --no -- lint-staged

Run this to confirm hooks are executable and won’t hit the network:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Ensure the hook is executable
if [ ! -x .husky/pre-commit ]; then
  echo "ERROR: .husky/pre-commit is not executable. Run: chmod +x .husky/pre-commit"
  exit 1
fi

# 2) Verify lint-staged is a local devDependency (prevents npx from installing from the network)
jq -r '.devDependencies["lint-staged"] // empty' package.json | grep -qE '^[0-9]' \
  && echo "OK: lint-staged is in devDependencies" \
  || { echo "WARNING: lint-staged not found in devDependencies"; exit 1; }

# 3) Check the hook content
echo "Hook content:"
nl -ba .husky/pre-commit

Length of output: 256


Fix .husky/pre-commit: add shebang, safety flags, use npx --no --, and ensure lint-staged is a devDependency

Verification output: "WARNING: lint-staged not found in devDependencies" — using plain npx here can trigger a network install.

Files to change:

  • .husky/pre-commit — update hook content and ensure it ends with a trailing newline and is executable.
  • package.json — add lint-staged to devDependencies (or install it locally).

Suggested diff:

-npx lint-staged
+#!/usr/bin/env sh
+set -euo pipefail
+npx --no -- lint-staged

Actions:

  • Add lint-staged locally: npm install --save-dev lint-staged (or add to package.json).
  • Make hook executable: chmod +x .husky/pre-commit
  • Ensure the file ends with a newline.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
npx lint-staged
#!/usr/bin/env sh
set -euo pipefail
npx --no -- lint-staged
🤖 Prompt for AI Agents
In .husky/pre-commit around lines 1 to 1, the hook currently just runs "npx
lint-staged" which can trigger network installs and lacks a shebang, safety
flags, and a trailing newline; update the hook to start with a POSIX shebang
(#!/bin/sh), enable safe shell options (e.g., set -euo pipefail) and invoke the
local binary with npx --no -- lint-staged, ensure the file ends with a newline
and is chmod +x executable, and also add lint-staged to package.json
devDependencies (or run npm install --save-dev lint-staged) so the hook uses the
local package rather than fetching from the network.

@@ -0,0 +1 @@
{ "*.ts": "yarn lint:fix" } No newline at end of file
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Pattern will miss files in subfolders; include tsx and prefer direct eslint invocation

  • "*.ts" matches only TS files in repo root; it will skip src/**/*.ts. This effectively disables linting on most staged files.
  • Consider including tsx if present.
  • Calling ESLint directly avoids potential argument passthrough issues with yarn lint:fix scripts.

Apply this diff:

-{ "*.ts": "yarn lint:fix" }
+{ "**/*.{ts,tsx}": "eslint --fix --max-warnings=0" }

If you must keep the script, at least fix the pattern:

-{ "*.ts": "yarn lint:fix" }
+{ "**/*.{ts,tsx}": "yarn lint:fix" }

Verify your lint script can accept file arguments (lint-staged appends them):


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the configured script
jq -r '.scripts["lint:fix"] // empty' package.json

# Heuristic check: ensure it calls eslint
jq -r '.scripts["lint:fix"] // empty' package.json | grep -qi 'eslint' \
  && echo "OK: lint:fix appears to run ESLint" \
  || echo "WARNING: lint:fix may not run ESLint; ensure it forwards file args"

Length of output: 283


Fix lint-staged pattern and ensure lint script accepts staged file args

Verified: package.json -> scripts.lint:fix = eslint --fix "src/**/*.ts" && prettier --write "src/**/*.ts" (it runs ESLint but hardcodes globs).

  • .lintstagedrc currently uses { "*.ts": "yarn lint:fix" }"*.ts" matches only root-level files and will skip staged files in subfolders.
  • Because lint:fix hardcodes src/**/*.ts, invoking it via yarn lint:fix from lint-staged may not correctly target only the staged files (the hardcoded glob can ignore/override file args). Prefer calling ESLint directly from lint-staged, or make the lint:fix script explicitly forward file arguments.

Files to change:

  • .lintstagedrc
  • package.json (scripts.lint:fix) if you plan to keep using the npm script

Recommended change (preferred: call eslint directly from lint-staged):

-{ "*.ts": "yarn lint:fix" }
+{ "**/*.{ts,tsx}": "eslint --fix --max-warnings=0" }

If you must keep the package script, at minimum fix the pattern so nested files are matched:

-{ "*.ts": "yarn lint:fix" }
+{ "**/*.{ts,tsx}": "yarn lint:fix" }

Also ensure lint:fix is updated to accept forwarded file args (or use a shell wrapper that forwards "$@") so lint-staged's file list is actually linted.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{ "*.ts": "yarn lint:fix" }
{ "**/*.{ts,tsx}": "eslint --fix --max-warnings=0" }

Comment on lines +6 to +9
files: ["src/**/*.ts"],
rules: {
"@typescript-eslint/no-explicit-any": "off",
},
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Widen file globs to include all TS/TSX files across the repo

The current files: ["src/**/*.ts"] will exclude TS files outside src and TSX files entirely. Prefer a broader glob unless intentionally scoped.

Apply this diff:

-    files: ["src/**/*.ts"],
+    files: ["**/*.{ts,tsx}"],

Optional: If you want to be explicit about parser options or ignore build artifacts, you can extend:

// Example additions (outside the changed lines)
languageOptions: {
  parserOptions: { ecmaVersion: "latest", sourceType: "module" },
},
ignores: ["dist/**"],

Please confirm you’re using ESLint v9+ and the typescript-eslint meta-package, which this flat config expects. If unsure, run:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# ESLint version
jq -r '.devDependencies.eslint // empty' package.json

# typescript-eslint meta package presence
jq -r '.devDependencies["typescript-eslint"] // empty' package.json

Length of output: 161


Widen ESLint file globs to include all .ts/.tsx files across the repo

The current files: ["src/**/*.ts"] excludes TS files outside src and all TSX files. Change to broaden the scope.

  • File: eslint.config.mjs — lines 6–9
-    files: ["src/**/*.ts"],
+    files: ["**/*.{ts,tsx}"],

Verified: ESLint v^9.33.0 and typescript-eslint meta-package v^8.39.1 are present — flat config is supported. Optional: add parser options / ignore patterns, e.g. languageOptions.parserOptions and ignores: ["dist/**"].

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
files: ["src/**/*.ts"],
rules: {
"@typescript-eslint/no-explicit-any": "off",
},
files: ["**/*.{ts,tsx}"],
rules: {
"@typescript-eslint/no-explicit-any": "off",
},
🤖 Prompt for AI Agents
In eslint.config.mjs around lines 6 to 9, the files glob is too narrow (only
"src/**/*.ts") and excludes TypeScript files outside src as well as TSX files;
update the files array to include all .ts and .tsx files across the repo (for
example use globs like "**/*.{ts,tsx}" and/or
["src/**/*.ts","src/**/*.tsx","**/*.ts","**/*.tsx"] as appropriate), and
optionally add ignores like "dist/**" and any needed
languageOptions.parserOptions for the TypeScript parser to ensure proper
parsing.

import rootLogger from "loglevel";
import prefix from "loglevel-plugin-prefix";
import chalk, { Chalk } from "chalk";
import chalk, { ChalkInstance } from "chalk";
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Build is failing: chalk v5 is ESM-only; current runtime loads it via require()

The pipeline error shows ERR_REQUIRE_ESM for chalk. With CommonJS output, TypeScript/Bundler will emit require("chalk"), which fails for chalk v5 (ESM-only).

Two viable fixes:

  • Quick fix: Pin chalk to v4 (CJS-compatible) and revert typing to Chalk (or compatible types).
  • Full fix: Migrate the repo to ESM (package.json "type": "module", tsconfig module: NodeNext/ESNext, adjust tooling and imports).

Run the script to confirm your current module settings and chalk version:

If you choose the quick fix (CJS), adjust dependencies and types:

  • In package.json (not shown here): set "chalk": "^4.1.2".
  • In this file:
-import chalk, { ChalkInstance } from "chalk";
+import chalk from "chalk";
+import type { Chalk } from "chalk";
-const COLORS: Record<string, ChalkInstance> = {
+const COLORS: Record<string, Chalk> = {

Alternatively, proceed with a full ESM migration and keep chalk v5.


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "package.json settings"
jq -r '.type // "no-type-field"' package.json
jq -r '.dependencies["chalk"] // .devDependencies["chalk"] // "chalk not found"' package.json

echo "tsconfig module settings"
rg -n "module\"|moduleResolution\"|esModuleInterop|allowSyntheticDefaultImports" -A 2 -B 2 -- tsconfig*.json || true

Length of output: 665


Action required — ESM/CJS mismatch: chalk@^5.5.0 (ESM-only) is installed but project builds as CommonJS (ERR_REQUIRE_ESM)

Verified:

  • package.json: no "type" field
  • chalk dependency: ^5.5.0
  • tsconfig.json: compilerOptions.module = "commonjs", esModuleInterop = true

Files that need attention:

  • package.json
  • tsconfig.json
  • src/utils/logging.ts

Quick fix (minimal change): pin chalk to v4 and adjust types in src/utils/logging.ts:

-import chalk, { ChalkInstance } from "chalk";
+import chalk from "chalk";
+import type { Chalk } from "chalk";
-const COLORS: Record<string, ChalkInstance> = {
+const COLORS: Record<string, Chalk> = {

Full fix (recommended if you want chalk v5): migrate repo to ESM — add "type": "module" to package.json, set tsconfig.module/moduleResolution to NodeNext/ESNext, and update tooling to support ESM.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import chalk, { ChalkInstance } from "chalk";
import chalk from "chalk";
import type { Chalk } from "chalk";

Comment on lines +23 to +28
assert.partialDeepStrictEqual(
parseCustomError(SINGLE_ORDER_NOT_AUTHED_ERROR),
{
selector: "SINGLE_ORDER_NOT_AUTHED",
},
);
Copy link

@coderabbitai coderabbitai bot Aug 14, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Fix: assert.partialDeepStrictEqual does not exist in Node’s assert API (causing test failures).

Node’s built-in assert module has no partialDeepStrictEqual. This is the likely cause of the pipeline test failure. Replace with a helper that asserts a subset or assert individual fields.

Apply this pattern across all occurrences:

-    assert.partialDeepStrictEqual(
-      parseCustomError(SINGLE_ORDER_NOT_AUTHED_ERROR),
-      {
-        selector: "SINGLE_ORDER_NOT_AUTHED",
-      },
-    );
+    assertSubset(parseCustomError(SINGLE_ORDER_NOT_AUTHED_ERROR), {
+      selector: "SINGLE_ORDER_NOT_AUTHED",
+    });

Repeat similarly for the other sections:

  • Lines 32-36: ORDER_NOT_VALID
  • Lines 39-43: POLL_TRY_NEXT_BLOCK
  • Lines 46-51: POLL_TRY_AT_BLOCK
  • Lines 54-59: POLL_TRY_AT_EPOCH
  • Lines 62-66: POLL_NEVER
  • Lines 95-102: known selector path
  • Lines 109-119: unknown selector path
  • Lines 127-137: short revert selectors
  • Lines 144-154: invalid encoding

Add this helper near the top of the file (outside the selected ranges):

// Helper: assert that `actual` contains at least the fields in `expected`, with deep strict equality for those fields.
function assertSubset(actual: any, expected: any) {
  const subset = Object.fromEntries(
    Object.keys(expected).map((k) => [k, (actual as any)[k]])
  );
  assert.deepStrictEqual(subset, expected);
}

Please run the test suite after applying the change to confirm the pipeline passes.

Also applies to: 32-36, 39-43, 46-51, 54-59, 62-66, 95-102, 109-119, 127-137, 144-154


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== Searching for 'partialDeepStrictEqual' occurrences ==="
rg --hidden --no-ignore -n "partialDeepStrictEqual" || true

echo
echo "=== Searching for 'assert.partialDeepStrictEqual' occurrences ==="
rg --hidden --no-ignore -n "assert.partialDeepStrictEqual" || true

echo
echo "=== Searching for 'assertSubset' occurrences ==="
rg --hidden --no-ignore -n "assertSubset" || true

echo
echo "=== Searching for 'parseCustomError' occurrences ==="
rg --hidden --no-ignore -n "parseCustomError" || true

echo
echo "=== Showing src/utils/utils.spec.ts (if present) ==="
if [ -f src/utils/utils.spec.ts ]; then
  sed -n '1,240p' src/utils/utils.spec.ts || true
else
  echo "src/utils/utils.spec.ts not found"
fi

Length of output: 9795


Fix tests: replace non-existent assert.partialDeepStrictEqual

Node's built-in assert has no partialDeepStrictEqual — the tests in src/utils/utils.spec.ts use it and will fail. Replace with a small subset assertion helper and update all occurrences.

Files / locations to update:

  • src/utils/utils.spec.ts — occurrences at lines: 23, 32, 39, 46, 54, 62, 95, 109, 127, 144

Example replacement (apply the same pattern to all occurrences):

-    assert.partialDeepStrictEqual(
-      parseCustomError(SINGLE_ORDER_NOT_AUTHED_ERROR),
-      {
-        selector: "SINGLE_ORDER_NOT_AUTHED",
-      },
-    );
+    assertSubset(parseCustomError(SINGLE_ORDER_NOT_AUTHED_ERROR), {
+      selector: "SINGLE_ORDER_NOT_AUTHED",
+    });

Add this helper near the top of src/utils/utils.spec.ts (outside the test ranges):

// Helper: assert that `actual` contains at least the fields in `expected`, with deep strict equality for those fields.
function assertSubset(actual: any, expected: any) {
  const subset = Object.fromEntries(
    Object.keys(expected).map((k) => [k, (actual as any)[k]])
  );
  assert.deepStrictEqual(subset, expected);
}

After applying the changes, run the test suite to confirm the pipeline passes.

🤖 Prompt for AI Agents
In src/utils/utils.spec.ts around lines 23 to 144 (occurrences at 23, 32, 39,
46, 54, 62, 95, 109, 127, 144): the tests use a non-existent
assert.partialDeepStrictEqual; add a small helper function near the top of the
file (outside the test ranges) named assertSubset that builds a subset object
from actual using the keys of expected and calls assert.deepStrictEqual(subset,
expected), then replace every assert.partialDeepStrictEqual(...) call at the
listed lines with assertSubset(...) passing the same actual and expected
arguments; after making these edits run the test suite to confirm all tests
pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

partialDeepStrictEqual does exist.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

@avivkeller You're absolutely right, and I apologize for the error. Let me verify the current Node.js assert API to confirm partialDeepStrictEqual exists and understand its proper usage.


🏁 Script executed:

#!/bin/bash
# Test if assert.partialDeepStrictEqual exists in Node's assert module
node -e "
const assert = require('assert');
console.log('Available assert methods:');
console.log(Object.getOwnPropertyNames(assert).filter(name => name.includes('partial') || name.includes('Deep')).sort());

console.log('\nTesting if partialDeepStrictEqual exists:');
console.log('partialDeepStrictEqual exists:', typeof assert.partialDeepStrictEqual);

if (typeof assert.partialDeepStrictEqual === 'function') {
  console.log('Method exists! Testing basic usage...');
  try {
    const actual = { a: 1, b: 2, c: 3 };
    const expected = { a: 1, b: 2 };
    assert.partialDeepStrictEqual(actual, expected);
    console.log('Test passed: partialDeepStrictEqual works as expected');
  } catch (err) {
    console.log('Test failed:', err.message);
  }
} else {
  console.log('Method does not exist');
}
"

Length of output: 1139


🌐 Web query:

Node.js assert partialDeepStrictEqual method documentation

💡 Result:

Here’s a concise reference for Node.js assert.partialDeepStrictEqual.

Signature

  • assert.partialDeepStrictEqual(actual, expected[, message]). (nodejs.org)

How to import

  • ESM: import assert from 'node:assert/strict';
  • CommonJS: const assert = require('node:assert').strict; (nodejs.org)

What it does

  • Tests for partial, deep, strict equality between actual and expected. Only properties present on expected are compared; extra properties on actual are ignored (hence “partial”). It behaves as a superset of assert.deepStrictEqual (i.e., it will pass all the same test cases as deepStrictEqual). If the comparison fails, an AssertionError is thrown (message may be provided or auto-generated). (nodejs.org)

Comparison details (rules)

  • Primitive values: compared using Object.is().
  • Type tags of objects must match.
  • [[Prototype]] is NOT compared.
  • Only enumerable own properties of expected are considered; enumerable own Symbol properties are compared as well.
  • Error objects: name, message, cause, and errors (and errors property) are always compared even if not enumerable.
  • Object wrappers are compared both as objects and by unwrapped values.
  • Object properties, Map keys and Set items are compared unordered.
  • Recursion stops on inequality or when circular references are encountered.
    (These details come from the Node.js docs.) (nodejs.org)

Examples

  • Passes because actual may have extra properties:
    const actual = { a: 1, b: 2 };
    assert.partialDeepStrictEqual(actual, { a: 1 }); // OK. (nodejs.org)

  • Fails because of strict comparison:
    const actual = { u: { id: 1 } };
    assert.partialDeepStrictEqual(actual, { u: { id: '1' } }); // AssertionError (1 !== '1'). (nodejs.org)

History / stability

  • Added in v22.13.0 (release candidate stage then) and promoted to Stable in v24.0.0. (nodejs.org)

If you want, I can:

  • Show additional examples (arrays, Map/Set, Error comparisons), or
  • Produce a small test file demonstrating various passing and failing cases.

Citations:


@avivkeller You're absolutely correct, and I apologize for the error! The script execution and documentation confirm that assert.partialDeepStrictEqual does indeed exist and works perfectly.

The method was added in Node.js v22.13.0 and became stable in v24.0.0, so it's a relatively recent addition. Your test code is correct as written:

assert.partialDeepStrictEqual(
  parseCustomError(SINGLE_ORDER_NOT_AUTHED_ERROR),
  {
    selector: "SINGLE_ORDER_NOT_AUTHED",
  },
);

This method does exactly what you need - it performs partial deep strict equality comparison, only checking properties present in the expected object while ignoring extra properties in the actual object.

I retract my previous suggestion to replace it with a custom helper. The tests should work correctly as they are. Thank you for the correction!


🧬 Code Graph Analysis Results

File: src/utils/utils.spec.ts, Lines 21-67

describe("parse custom errors (reversions)", () => {
  it("should pass the SingleOrderNotAuthed selector correctly", () => {
    assert.partialDeepStrictEqual(
      parseCustomError(SINGLE_ORDER_NOT_AUTHED_ERROR),
      {
        selector: "SINGLE_ORDER_NOT_AUTHED",
      },
    );
  });

  it("should pass the OrderNotValid selector correctly", () => {
    assert.partialDeepStrictEqual(parseCustomError(ORDER_NOT_VALID), {
      selector: "ORDER_NOT_VALID",
      message: "after twap finish",
    });
  });

  it("should pass the PollTryNextBlock selector correctly", () => {
    assert.partialDeepStrictEqual(parseCustomError(POLL_TRY_NEXT_BLOCK), {
      selector: "POLL_TRY_NEXT_BLOCK",
      message: "try me again",
    });
  });

  it("should pass the PollTryAtBlock selector correctly", () => {
    assert.partialDeepStrictEqual(parseCustomError(POLL_TRY_AT_BLOCK), {
      selector: "POLL_TRY_AT_BLOCK",
      message: "red pill",
      blockNumberOrEpoch: 303,
    });
  });

  it("should pass the PollTryAtEpoch selector correctly", () => {
    assert.partialDeepStrictEqual(parseCustomError(POLL_TRY_AT_EPOCH), {
      selector: "POLL_TRY_AT_EPOCH",
      message: "here's looking at you",
      blockNumberOrEpoch: 1694340000,
    });
  });

  it("should pass the PollNever selector correctly", () => {
    assert.partialDeepStrictEqual(parseCustomError(POLL_NEVER), {
      selector: "POLL_NEVER",
      message: "after twap finish",
    });
  });
});

File: src/utils/utils.spec.ts, Lines 71-84

  const happyPath = {
    owner: "0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
    target: COMPOSABLE_COW_CONTRACT_ADDRESS[1],
    callData: "0xca1fca1fca1fca1f",
    orderRef: "orderRefForLogging",
    chainId: 1,
    revertData: abiToSelector(
      CUSTOM_ERROR_ABI_MAP[CustomErrorSelectors.SINGLE_ORDER_NOT_AUTHED],
    ),
    metricLabels: ["chain_id", "handler", "owner", "id"],
    blockNumber: 123456,
    ownerNumber: 2,
    orderNumber: 3,
  };

File: src/utils/utils.spec.ts, Lines 93-103

  chainIds.forEach((chainId) => {
    it(`should pass a known selector correctly for chainId ${chainId}`, () => {
      assert.partialDeepStrictEqual(
        handleOnChainCustomError(getHappyPathWithChainId(chainId)),
        {
          reason:
            "SINGLE_ORDER_NOT_AUTHED: The owner has not authorized the order",
          result: "DONT_TRY_AGAIN",
        },
      );
    });
  });

File: src/utils/utils.spec.ts, Lines 106-119

  chainIds.forEach((chainId) => {
    it(`should drop if the revert selector does not exist in the map for chainId ${chainId}`, () => {
      const unknownSelector = "0xdeadbeef";
      assert.partialDeepStrictEqual(
        handleOnChainCustomError({
          ...getHappyPathWithChainId(chainId),
          revertData: unknownSelector,
        }),
        {
          reason:
            "Order returned a non-compliant (invalid/erroneous) revert hint",
          result: "DONT_TRY_AGAIN",
        },
      );
    });
  });

File: src/utils/utils.spec.ts, Lines 123-156

  chainIds.forEach((chainId) => {
    it(`should drop if the revert data is too short even to be a selector for chainId ${chainId}`, () => {
      const shortReverts = ["0x", "0xca1f"];
      shortReverts.forEach((shortRevert) =>
        assert.partialDeepStrictEqual(
          handleOnChainCustomError({
            ...getHappyPathWithChainId(chainId),
            revertData: shortRevert,
          }),
          {
            reason:
              "Order returned a non-compliant (invalid/erroneous) revert hint",
            result: "DONT_TRY_AGAIN",
          },
        ),
      );
    });
  });

File: src/utils/contracts.ts, Lines 110-112

export function abiToSelector(abi: string) {
  return ethers.utils.id(abi).slice(0, 10);
}

File: src/utils/contracts.ts, Lines 93-105

export const CUSTOM_ERROR_ABI_MAP: Record<CustomErrorSelectors, string> = {
  [CustomErrorSelectors.PROOF_NOT_AUTHED]: "ProofNotAuthed()",
  [CustomErrorSelectors.SINGLE_ORDER_NOT_AUTHED]: "SingleOrderNotAuthed()",
  [CustomErrorSelectors.SWAP_GUARD_RESTRICTED]: "SwapGuardRestricted()",
  [CustomErrorSelectors.INVALID_HANDLER]: "InvalidHandler()",
  [CustomErrorSelectors.INVALID_FALLBACK_HANDLER]: "InvalidFallbackHandler()",
  [CustomErrorSelectors.INTERFACE_NOT_SUPPORTED]: "InterfaceNotSupported()",
  [CustomErrorSelectors.ORDER_NOT_VALID]: "OrderNotValid(string)",
  [CustomErrorSelectors.POLL_TRY_NEXT_BLOCK]: "PollTryNextBlock(string)",
  [CustomErrorSelectors.POLL_NEVER]: "PollNever(string)",
  [CustomErrorSelectors.POLL_TRY_AT_BLOCK]: "PollTryAtBlock(uint256,string)",
  [CustomErrorSelectors.POLL_TRY_AT_EPOCH]: "PollTryAtEpoch(uint256,string)",
};

File: src/utils/contracts.ts, Lines 133-193

export function parseCustomError(revertData: string): ParsedCustomError {
  // If the revert data is not at least 4 bytes long (8 hex characters, 0x prefixed), it cannot contain a selector
  if (revertData.length < 10) {
    throw new Error("Revert data too short to contain a selector");
  }

  const rawSelector = revertData.slice(0, 10);

  // If the revert data does not contain a selector from the CUSTOM_ERROR_SELECTOR_MAP, it is a non-compliant
  // interface and we should signal to drop it.
  if (!(revertData.slice(0, 10) in CUSTOM_ERROR_SELECTOR_MAP)) {
    throw new Error(
      "On-chain hint / custom error not compliant with ComposableCoW interface",
    );
  }

  // Below here, the only throw that can happen is if the revert data contains a selector that is in the
  // CUSTOM_ERROR_SELECTOR_MAP, but the it's parameters are not ABI-encoded correctly.

  const selector = CUSTOM_ERROR_SELECTOR_MAP[rawSelector];
  const fragment = ethers.utils.Fragment.fromString(
    "error " + CUSTOM_ERROR_ABI_MAP[selector],
  );
  const iface = new ethers.utils.Interface([fragment]);

  switch (selector) {
    case CustomErrorSelectors.PROOF_NOT_AUTHED:
    case CustomErrorSelectors.SINGLE_ORDER_NOT_AUTHED:
    case CustomErrorSelectors.INTERFACE_NOT_SUPPORTED:
    case CustomErrorSelectors.INVALID_FALLBACK_HANDLER:
    case CustomErrorSelectors.INVALID_HANDLER:
    case CustomErrorSelectors.SWAP_GUARD_RESTRICTED:
      return { selector };
    case CustomErrorSelectors.ORDER_NOT_VALID:
    case CustomErrorSelectors.POLL_TRY_NEXT_BLOCK:
    case CustomErrorSelectors.POLL_NEVER:
      const [message] = iface.decodeErrorResult(fragment, revertData) as [
        string,
      ];
      return { selector, message };
    case CustomErrorSelectors.POLL_TRY_AT_BLOCK:
    case CustomErrorSelectors.POLL_TRY_AT_EPOCH:
      const [blockNumberOrEpoch, msg] = iface.decodeErrorResult(
        fragment,
        revertData,
      ) as [BigNumber, string];

      // It is reasonable to expect that the block number or epoch is bound by
      // uint32. It is therefore safe to throw if the value is outside of that
      // for javascript's number type.
      if (blockNumberOrEpoch.gt(MAX_UINT32)) {
        throw new Error("Block number or epoch out of bounds");
      }

      return {
        selector,
        message: msg,
        blockNumberOrEpoch: blockNumberOrEpoch.toNumber(),
      };
  }
}

File: src/utils/contracts.ts, Lines 199-323

export function handleOnChainCustomError(params: {
  owner: string;
  chainId: SupportedChainId;
  target: string;
  callData: string;
  revertData: string;
  metricLabels: string[];
  blockNumber: number;
  ownerNumber: number;
  orderNumber: number;
}): PollResultErrors {
  const {
    owner,
    chainId,
    target,
    callData,
    revertData,
    metricLabels,
    blockNumber,
    ownerNumber,
    orderNumber,
  } = params;
  const loggerParams = {
    name: "handleOnChainCustomError",
    chainId,
    blockNumber,
    ownerNumber,
    orderNumber,
  };

  try {
    // The below will throw if:
    // - the error is not a custom error (ie. the selector is not in the map)
    // - the error is a custom error, but the parameters are not as expected
    const parsedCustomError = parseCustomError(revertData);
    const { selector } = parsedCustomError;
    const log = getLogger({
      ...loggerParams,
      args: [selector],
    });
    const msgWithSelector = (message: string): string =>
      `${selector}: ${message}`;
    const dropOrder = (reason: string): PollResultErrors => {
      return {
        result: PollResultCode.DONT_TRY_AGAIN,
        reason: msgWithSelector(reason),
      };
    };
    switch (parsedCustomError.selector) {
      case CustomErrorSelectors.SINGLE_ORDER_NOT_AUTHED:
      case CustomErrorSelectors.PROOF_NOT_AUTHED:
        // If there's no authorization we delete the order
        // - One reason could be, because the user CANCELLED the order
        // - for now it doesn't support more advanced cases where the order is authed during a pre-interaction
        log.info(`Order on safe ${owner} not authed. Deleting order...`);
        return dropOrder("The owner has not authorized the order");
      case CustomErrorSelectors.INTERFACE_NOT_SUPPORTED:
        log.info(
          `Order type for safe ${owner}, failed IERC165 introspection check. Deleting order...`,
        );
        return dropOrder("The order type failed IERC165 introspection check");
      case CustomErrorSelectors.INVALID_FALLBACK_HANDLER:
        log.info(
          `Order for safe ${owner} where the Safe does not have ExtensibleFallbackHandler set. Deleting order...`,
        );
        return dropOrder(
          "The safe does not have ExtensibleFallbackHandler set",
        );
      case CustomErrorSelectors.INVALID_HANDLER:
        log.info(
          `Order on safe ${owner} attempted to use a handler that is not supported. Deleting order...`,
        );
        return dropOrder("The handler is not supported");
      case CustomErrorSelectors.SWAP_GUARD_RESTRICTED:
        log.info(
          `Order for safe ${owner} where the Safe has swap guard enabled. Deleting order...`,
        );
        return dropOrder("The conditional order didn't pass the swap guard");
      case CustomErrorSelectors.ORDER_NOT_VALID:
        const reason = msgWithSelector(parsedCustomError.message);
        log.info(
          `Order for ${owner} is invalid. Reason: ${reason}. Deleting order...`,
        );
        return dropOrder(`Invalid order: ${reason}`);
      case CustomErrorSelectors.POLL_TRY_NEXT_BLOCK:
        log.info(`Order on safe ${owner} not signalled to try next block`);
        return {
          result: PollResultCode.TRY_NEXT_BLOCK,
          reason: msgWithSelector(parsedCustomError.message),
        };
      case CustomErrorSelectors.POLL_TRY_AT_BLOCK:
        return {
          result: PollResultCode.TRY_ON_BLOCK,
          blockNumber: parsedCustomError.blockNumberOrEpoch,
          reason: msgWithSelector(parsedCustomError.message),
        };
      case CustomErrorSelectors.POLL_TRY_AT_EPOCH:
        return {
          result: PollResultCode.TRY_AT_EPOCH,
          epoch: parsedCustomError.blockNumberOrEpoch,
          reason: msgWithSelector(parsedCustomError.message),
        };
      case CustomErrorSelectors.POLL_NEVER:
        return dropOrder(parsedCustomError.message);
    }
  } catch (err: any) {
    // Any errors thrown here can _ONLY_ come from non-compliant interfaces (ie. bad revert ABI encoding).
    // We log the error, and return a DONT_TRY_AGAIN result.
    const log = getLogger(loggerParams);

    log.debug(
      `Non-compliant interface error thrown${
        err.message ? `: ${err.message}` : ""
      }`,
    );
    log.debug(
      `Contract returned a non-compliant interface revert via getTradeableOrderWithSignature. Simulate: https://dashboard.tenderly.co/gp-v2/watch-tower-prod/simulator/new?network=${chainId}&contractAddress=${target}&rawFunctionInput=${callData}`,
    );
    metrics.pollingOnChainInvalidInterfacesTotal.labels(...metricLabels).inc();
    return {
      result: PollResultCode.DONT_TRY_AGAIN,
      reason: "Order returned a non-compliant (invalid/erroneous) revert hint",
    };
  }
}

✏️ Learnings added
Learnt from: avivkeller
PR: cowprotocol/watch-tower#185
File: src/utils/utils.spec.ts:23-28
Timestamp: 2025-08-14T17:15:17.628Z
Learning: The `assert.partialDeepStrictEqual` method exists in Node.js assert module and can be used for partial object comparison in tests.

Learnt from: avivkeller
PR: cowprotocol/watch-tower#185
File: src/utils/utils.spec.ts:23-28
Timestamp: 2025-08-14T17:15:17.628Z
Learning: Node.js `assert.partialDeepStrictEqual` method exists and was added in v22.13.0, became stable in v24.0.0. It performs partial deep strict equality comparison, only checking properties present in the expected object while ignoring extra properties in the actual object.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@avivkeller avivkeller marked this pull request as draft August 14, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants