Skip to content

fix: singleton SecretManagerServiceClient to prevent gRPC channel leak + OOM#31

Merged
chapati23 merged 2 commits intomainfrom
fix/secret-manager-client-leak
Mar 14, 2026
Merged

fix: singleton SecretManagerServiceClient to prevent gRPC channel leak + OOM#31
chapati23 merged 2 commits intomainfrom
fix/secret-manager-client-leak

Conversation

@gisk0
Copy link
Copy Markdown
Contributor

@gisk0 gisk0 commented Mar 13, 2026

Root Cause

Every call to getSecret() was instantiating a new SecretManagerServiceClient:

// BEFORE — leaked a new gRPC channel on every request
const secretManager = new SecretManagerServiceClient();

getSecret() is called on every QuickNode webhook request (signature verification via isFromQuicknode()). The SecretManagerServiceClient uses gRPC under the hood — each instantiation opens a new channel that wasn't being closed or GC'd, causing steady memory growth.

Evidence: instance started at 16:00 UTC, received ~169 requests over 28 minutes, OOM'd at 16:28 UTC with 488 MiB used. Same pattern seen daily for the past week.

Fix

  1. Singleton clientSecretManagerServiceClient created once at module load time, reused for all requests
  2. Secret caching — secret values cached in memory after first fetch; secrets don't change during an instance's lifetime, so repeated round-trips to Secret Manager were unnecessary
// AFTER — one client, one fetch per secret per instance lifetime
const secretManager = new SecretManagerServiceClient();
const secretCache = new Map<string, string>();

Impact

  • Eliminates gRPC channel leak → no more OOM
  • Reduces Secret Manager API calls from ~N calls/min to 1 call per secret per cold start
  • Faster request handling (no Secret Manager round-trip on warm requests)

Note

Medium Risk
Touches production secret-loading and QuickNode webhook deployment paths; mistakes could break webhook updates or secret retrieval/rotation behavior despite the changes being small and well-covered by new unit tests.

Overview
Prevents Cloud Run memory growth by switching getSecret() to reuse a module-level SecretManagerServiceClient (no per-request gRPC channel creation), while explicitly avoiding secret value caching to preserve versions/latest rotation behavior.

Updates bin/deploy-quicknode-filter.sh to deploy QuickNode template-based (evmAbiFilter) webhooks by parsing abi/contracts from the filter file header and PATCHing /template/evmAbiFilterGo (with safer temp-file cleanup), and trims the ABI headers/bodies in governor.js/sorted-oracles.js to only the handled events.

Adds Vitest (vitest.config.ts, new npm scripts, lockfile updates) plus unit tests for getSecret() covering success, empty payloads, errors, and singleton-client reuse.

Written by Cursor Bugbot for commit f283520. This will update automatically on new commits. Configure here.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR fixes the Secret Manager gRPC leak by reusing a single client and switches the QuickNode webhook deploy flow to template args / trimmed ABIs. The singleton client change is the right direction, but the new global secret cache and the weaker deploy verification both introduce regressions.

src/utils/get-secret.ts

  • 🔴 Critical (19-40): caching versions/latest forever changes this helper from "read the current secret" to "read the first value seen by this container". This helper is used by isFromQuicknode() and hasAuthToken(), so rotating quicknode-security-token or x-auth-token will not take effect on warm instances until Cloud Run happens to recycle them. That can either reject valid requests after rotation or keep accepting an old credential longer than intended.
  • 🟡 Important (19-40): there is no test coverage for the new cache semantics, so the auth-rotation regression above would only be caught in production.

bin/deploy-quicknode-filter.sh

  • 🟡 Important (133-167): the script now deploys directly from the filter-file header, but the repo's Terraform source of truth still contains the old base64 filter_function blobs. A future delete/recreate or fresh-environment bootstrap will recreate the old full-ABI webhooks unless the IaC path is updated from the same data.
  • 🟡 Important (145-167): verification only checks that the webhook is still active; it never asserts that the expected template/templateArgs were actually applied, so a no-op or partial update can still report success.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

);
}

secretCache.set(secretId, secret);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caching the resolved value here silently changes this helper's contract from "read versions/latest" to "read once per container". Because getSecret() is used for request authentication (isFromQuicknode() / hasAuthToken()), rotating either auth secret will leave warm instances validating against a stale credential until Cloud Run restarts them. Keep the singleton SecretManagerServiceClient, but don't permanently cache auth secrets without TTL/invalidation or an explicit restart strategy.


# Update via template endpoint — internal name is evmAbiFilterGo (evmAbiFilter is display name)
# No pause/unpause needed for template updates.
log "Updating template args via /template/evmAbiFilterGo endpoint..."
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This makes the filter-file header the runtime source of truth, but nothing updates infra/quicknode.tf anymore. The checked-in Terraform still contains the old base64 filter_function blobs, so deleting/recreating the webhook or bootstrapping a fresh environment will recreate the old full-ABI filters and reintroduce the event flood. The deployed template args and IaC need to come from the same source.

info "Template: ${template_id}"

if [[ ${live_status} == "active" && ${has_filter} == "yes" ]]; then
if [[ ${live_status} == "active" ]]; then
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The verification step only checks that the webhook is still active. If QuickNode accepts the PATCH but leaves the previous template args in place, this still exits 0 and prints success. Please assert the returned templateId and deployed templateArgs match the parsed ABI/contracts instead of treating active as sufficient proof of a successful update.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Trap overwrite leaks first temp file in "all" deploy
    • Replaced per-call EXIT trap with a single global cleanup trap that deletes all mktemp payload files collected across deploy_webhook invocations.

Create PR

Or push these changes by commenting:

@cursor push c5491183b4
Preview (c5491183b4)
diff --git a/bin/deploy-quicknode-filter.sh b/bin/deploy-quicknode-filter.sh
--- a/bin/deploy-quicknode-filter.sh
+++ b/bin/deploy-quicknode-filter.sh
@@ -32,7 +32,15 @@
 GOVERNOR_WEBHOOK_ID="73a99141-e8cb-411a-9732-c42a031cebe6"
 
 QN_API_BASE="https://api.quicknode.com/webhooks/rest/v1/webhooks"
+TMP_FILES=()
 
+cleanup_temp_files() {
+	if (( ${#TMP_FILES[@]} > 0 )); then
+		rm -f "${TMP_FILES[@]}"
+	fi
+}
+trap cleanup_temp_files EXIT
+
 # ------------------------------------------------------------------------------
 log() { printf '\n\033[1m%s\033[0m\n' "$*"; }
 success() { printf '✅ %s\n' "$*"; }
@@ -96,9 +104,7 @@
 	# The internal template ID for PATCH is "evmAbiFilterGo" (evmAbiFilter is the display name).
 	local payload_file
 	payload_file=$(mktemp /tmp/qn_payload.XXXXXX.json)
-	# Ensure temp file is always cleaned up, even on SIGINT/SIGTERM
-	# shellcheck disable=SC2064
-	trap "rm -f '${payload_file}'" EXIT
+	TMP_FILES+=("${payload_file}")
 
 	# Build templateArgs payload: abiJson must be a raw JSON string (not a parsed object).
 	# Use env vars to avoid shell quoting issues with large ABI strings.

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

gisk0 added a commit that referenced this pull request Mar 13, 2026
…verwrite

Address PR #31 review feedback:

1. Remove secretCache from get-secret.ts
   - Caching versions/latest forever prevents secret rotation from taking
     effect on warm Cloud Run instances (🔴 critical regression per review)
   - The singleton SecretManagerServiceClient alone fixes the gRPC leak;
     per-request round-trips to Secret Manager are acceptable
   - Added explicit comment explaining why we don't cache

2. Fix EXIT trap overwrite in deploy-quicknode-filter.sh
   - Per-call 'trap rm EXIT' in deploy_webhook() overwrote the previous
     trap on repeated calls, leaking the first temp file
   - Replace with global TMP_FILES array + single script-level EXIT trap
     (same fix proposed by Cursor Bugbot)
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR correctly moves SecretManagerServiceClient to a module-level singleton and rewires QuickNode webhook deployment to PATCH template args instead of updating filter_function blobs. The runtime leak fix looks good, but the new QuickNode deployment path introduces a broken request shape and a behavior regression in the healthcheck webhook.

bin/deploy-quicknode-filter.sh

  • 🔴 Critical (130): the script sends templateArgs.abiJson, but QuickNode's public PATCH /webhooks/{webhookId}/template/{templateId} docs for evmAbiFilter accept templateArgs.abi and contracts. Shipping {"templateArgs":{"abiJson":...}} is very likely to 400 or silently ignore the ABI update, which means the new deploy path does not reliably work.
  • 🟡 Important (154-176): verification only checks that the webhook is still active. It never asserts that the expected template args were actually applied, so a no-op or partial update can still print deployed successfully.
  • 🟡 Important (98-107, 140-176): this script now treats the filter-file header as the deployment source of truth, but the repo's existing Terraform/bootstrap workflow still points at the old base64 filter_function blobs in infra/quicknode.tf, bin/update-quicknode-filter.js, README.md, and ADDING_EVENTS.md. A delete/recreate or docs-driven rollout will drift back to the previous filter definition.

infra/quicknode-filter-functions/sorted-oracles.js

  • 🟡 Important (8-11, 19-21, 39-43): the file now explicitly says the JS body is not executed for template-based webhooks. That makes the targetTokenAddress predicate below dead code, so this webhook will now match every MedianUpdated emitted by SortedOracles, not just the CELO/cUSD feed the healthcheck is supposed to track.

src/utils/get-secret.ts

  • 🟡 Important (13-24): this changes auth-path secret retrieval to rely on a process-wide singleton client, but there is no automated coverage proving only the client is memoized and the secret value is still fetched on each call. Because this helper gates request authentication, it needs a small unit test around accessSecretVersion() call behavior.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

print(f"ERROR: ABI is not valid JSON: {e}", file=sys.stderr)
sys.exit(1)
contracts = [c.strip() for c in m.group(2).strip().split(",")]
payload = {"templateArgs": {"abiJson": abi_str, "contracts": contracts}}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

templateArgs.abiJson does not match QuickNode's documented evmAbiFilter template schema here. The public create/update template docs describe templateArgs.abi plus contracts; if we send abiJson instead, this PATCH is likely to fail or be treated as a no-op.

info "Template: ${template_id}"

if [[ ${live_status} == "active" && ${has_filter} == "yes" ]]; then
if [[ ${live_status} == "active" ]]; then
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This success check is too weak for the new deploy path. The script only verifies status == active, so it can report success even if the template update was ignored and the webhook is still serving the previous ABI/contracts. At minimum, verify the returned templateId and template args match the parsed file.

const contracts = ["0xefb84935239dacdecf7c5ba76d8de40b077b7b33"];
const abi = `[{"inputs":[{"internalType":"bool","name":"test","type":"bool"}],"payable":false,"stateMutability":"nonpayable","type":"constructor"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"newBreakerBox","type":"address"}],"name":"BreakerBoxUpdated","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"token","type":"address"},{"indexed":true,"internalType":"address","name":"equivalentToken","type":"address"}],"name":"EquivalentTokenSet","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"token","type":"address"},{"indexed":false,"internalType":"uint256","name":"value","type":"uint256"}],"name":"MedianUpdated","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"token","type":"address"},{"indexed":true,"internalType":"address","name":"oracleAddress","type":"address"}],"name":"OracleAdded","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"token","type":"address"},{"indexed":true,"internalType":"address","name":"oracleAddress","type":"address"}],"name":"OracleRemoved","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"token","type":"address"},{"indexed":true,"internalType":"address","name":"oracle","type":"address"}],"name":"OracleReportRemoved","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"token","type":"address"},{"indexed":true,"internalType":"address","name":"oracle","type":"address"},{"indexed":false,"internalType":"uint256","name":"timestamp","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"value","type":"uint256"}],"name":"OracleReported","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"previousOwner","type":"address"},{"indexed":true,"internalType":"address","name":"newOwner","type":"address"}],"name":"OwnershipTransferred","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"reportExpiry","type":"uint256"}],"name":"ReportExpirySet","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"address","name":"token","type":"address"},{"indexed":false,"internalType":"uint256","name":"reportExpiry","type":"uint256"}],"name":"TokenReportExpirySet","type":"event"},{"constant":false,"inputs":[{"internalType":"address","name":"token","type":"address"},{"internalType":"address","name":"oracleAddress","type":"address"}],"name":"addOracle","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"breakerBox","outputs":[{"internalType":"contract IBreakerBox","name":"","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"deleteEquivalentToken","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"","type":"address"}],"name":"equivalentTokens","outputs":[{"internalType":"address","name":"token","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"getEquivalentToken","outputs":[{"internalType":"address","name":"","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"getExchangeRate","outputs":[{"internalType":"uint256","name":"numerator","type":"uint256"},{"internalType":"uint256","name":"denominator","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"getOracles","outputs":[{"internalType":"address[]","name":"","type":"address[]"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"getRates","outputs":[{"internalType":"address[]","name":"","type":"address[]"},{"internalType":"uint256[]","name":"","type":"uint256[]"},{"internalType":"enum SortedLinkedListWithMedian.MedianRelation[]","name":"","type":"uint8[]"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"getTimestamps","outputs":[{"internalType":"address[]","name":"","type":"address[]"},{"internalType":"uint256[]","name":"","type":"uint256[]"},{"internalType":"enum SortedLinkedListWithMedian.MedianRelation[]","name":"","type":"uint8[]"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"getTokenReportExpirySeconds","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[],"name":"getVersionNumber","outputs":[{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"pure","type":"function"},{"constant":false,"inputs":[{"internalType":"uint256","name":"_reportExpirySeconds","type":"uint256"}],"name":"initialize","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"initialized","outputs":[{"internalType":"bool","name":"","type":"bool"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"isOldestReportExpired","outputs":[{"internalType":"bool","name":"","type":"bool"},{"internalType":"address","name":"","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"","type":"address"},{"internalType":"address","name":"","type":"address"}],"name":"isOracle","outputs":[{"internalType":"bool","name":"","type":"bool"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[],"name":"isOwner","outputs":[{"internalType":"bool","name":"","type":"bool"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"medianRate","outputs":[{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"medianRateWithoutEquivalentMapping","outputs":[{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"medianTimestamp","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"numRates","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"token","type":"address"}],"name":"numTimestamps","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"","type":"address"},{"internalType":"uint256","name":"","type":"uint256"}],"name":"oracles","outputs":[{"internalType":"address","name":"","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":true,"inputs":[],"name":"owner","outputs":[{"internalType":"address","name":"","type":"address"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"token","type":"address"},{"internalType":"uint256","name":"n","type":"uint256"}],"name":"removeExpiredReports","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"token","type":"address"},{"internalType":"address","name":"oracleAddress","type":"address"},{"internalType":"uint256","name":"index","type":"uint256"}],"name":"removeOracle","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[],"name":"renounceOwnership","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"token","type":"address"},{"internalType":"uint256","name":"value","type":"uint256"},{"internalType":"address","name":"lesserKey","type":"address"},{"internalType":"address","name":"greaterKey","type":"address"}],"name":"report","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"reportExpirySeconds","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"contract IBreakerBox","name":"newBreakerBox","type":"address"}],"name":"setBreakerBox","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"token","type":"address"},{"internalType":"address","name":"equivalentToken","type":"address"}],"name":"setEquivalentToken","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"uint256","name":"_reportExpirySeconds","type":"uint256"}],"name":"setReportExpiry","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"_token","type":"address"},{"internalType":"uint256","name":"_reportExpirySeconds","type":"uint256"}],"name":"setTokenReportExpiry","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"internalType":"address","name":"","type":"address"}],"name":"tokenReportExpirySeconds","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"internalType":"address","name":"newOwner","type":"address"}],"name":"transferOwnership","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"}]`;
// Only MedianUpdated — matches the comment header above which the deploy script reads.
// The evmAbiFilter template uses these templateArgs (abi + contracts); this JS body is
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment implies the JS body below no longer runs after deployment. If that's true, the targetTokenAddress guard at 39-43 is dead code, and the webhook will start matching every MedianUpdated for SortedOracles instead of only the CELO/cUSD feed.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR fixes the SecretManagerServiceClient leak by reusing a singleton client in getSecret() and adds unit-test coverage for that behavior. It also rewires QuickNode webhook deployment from patching filter_function to patching template args.

bin/deploy-quicknode-filter.sh

  • 🟡 IMPORTANT (L98-L153): this script now treats the comment header in the filter JS files as the deploy source of truth, but the rest of the repo still treats the JS body / Terraform filter_function as canonical via bin/update-quicknode-filter.js, infra/quicknode.tf, README.md, and ADDING_EVENTS.md. That leaves two divergent definitions of the same webhook. A future edit made through the documented hot-reload/Terraform flow will update the body only, while this script deploys the header only; the next deployment can silently ship different ABI/contracts than what the repo’s Terraform state and docs imply. Consolidate on one representation or update/remove the old path in this PR.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

echo "${pause_response}"
# These are evmAbiFilter template-based webhooks. The .js filter file embeds the
# ABI and contract addresses in the comment header and in the JS code itself.
# Template-based webhooks cannot have filter_function updated directly via PATCH
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This change makes the filter file's block comment the deploy source of truth, but the repo still has a second, conflicting source of truth: the JS body / Terraform filter_function flow (bin/update-quicknode-filter.js, infra/quicknode.tf, README.md, ADDING_EVENTS.md). A future edit done through the documented hot-reload/Terraform path will update the body only, while this script deploys the header only. Please consolidate on one representation or update/remove the old tooling in the same PR.

@cursor
Copy link
Copy Markdown

cursor bot commented Mar 14, 2026

You have used all of your free Bugbot PR reviews.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR replaces per-call Secret Manager client construction with a singleton client, adds a small Vitest unit suite around getSecret, and documents a new QuickNode template-based deployment flow. The Secret Manager change itself looks fine, but the QuickNode deployment changes introduce two operational regressions and leave one important verification gap.

infra/quicknode-filter-functions/governor.js

  • 🔴 Critical — infra/quicknode-filter-functions/governor.js:8-10
    The PR explicitly states the JS body is not executed for template-based webhooks, and README.md:255 states QuickNode ignores the contracts address filter in templateArgs. If both are true, this change removes the only effective governor-contract address filter, but the runtime handlers still do not validate event.address anywhere in src/events/configs.ts. Any other contract on Celo emitting the standard OpenZeppelin ProposalCreated/ProposalQueued/ProposalExecuted/ProposalCanceled signatures can now produce false governance notifications.

bin/deploy-quicknode-filter.sh

  • 🔴 Critical — bin/deploy-quicknode-filter.sh:31-32
    The new deployment flow hardcodes two webhook UUIDs. Those IDs are server-assigned and will change on bootstrap/recreate flows managed by infra/quicknode.tf, so after the next recreate this script will PATCH stale IDs and stop updating the live webhooks.

infra/quicknode.tf

  • 🟡 Important — infra/quicknode.tf:1-13
    These new comments say the base64 filter_function blobs are no longer a deployment source of truth, but Terraform still uses them when it creates or recreates a webhook. Because the blob contents were not updated in this PR, bootstrap/delete-recreate flows will come up with stale filter configuration until someone manually patches them afterward.

package.json

  • 🟡 Important — package.json:22,34-36
    The new get-secret regression tests are not wired into the default npm test path, so contributors following the existing test command still won't execute them. That leaves the new coverage easy to bypass and makes the gRPC-leak regression much easier to reintroduce unnoticed.

Final Verdict

🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

@@ -33,6 +33,17 @@ GOVERNOR_WEBHOOK_ID="73a99141-e8cb-411a-9732-c42a031cebe6"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These UUIDs are now part of the deployment contract, but they are not stable across terraform recreate/bootstrap flows because QuickNode assigns them on creation. After the next destroy/recreate, this script will PATCH the wrong webhook unless it resolves IDs dynamically (for example from Terraform state or by querying the API by name) instead of hardcoding them here.

const contracts = ["0x47036d78bb3169b4f5560dd77bf93f4412a59852"];
const abi = `[{"inputs":[],"name":"Empty","type":"error"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint8","name":"version","type":"uint8"}],"name":"Initialized","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"ProposalCanceled","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"proposalId","type":"uint256"},{"indexed":false,"internalType":"address","name":"proposer","type":"address"},{"indexed":false,"internalType":"address[]","name":"targets","type":"address[]"},{"indexed":false,"internalType":"uint256[]","name":"values","type":"uint256[]"},{"indexed":false,"internalType":"string[]","name":"signatures","type":"string[]"},{"indexed":false,"internalType":"bytes[]","name":"calldatas","type":"bytes[]"},{"indexed":false,"internalType":"uint256","name":"startBlock","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"endBlock","type":"uint256"},{"indexed":false,"internalType":"string","name":"description","type":"string"}],"name":"ProposalCreated","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"ProposalExecuted","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"proposalId","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"eta","type":"uint256"}],"name":"ProposalQueued","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"oldProposalThreshold","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"newProposalThreshold","type":"uint256"}],"name":"ProposalThresholdSet","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"oldQuorumNumerator","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"newQuorumNumerator","type":"uint256"}],"name":"QuorumNumeratorUpdated","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"address","name":"oldTimelock","type":"address"},{"indexed":false,"internalType":"address","name":"newTimelock","type":"address"}],"name":"TimelockChange","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"voter","type":"address"},{"indexed":false,"internalType":"uint256","name":"proposalId","type":"uint256"},{"indexed":false,"internalType":"uint8","name":"support","type":"uint8"},{"indexed":false,"internalType":"uint256","name":"weight","type":"uint256"},{"indexed":false,"internalType":"string","name":"reason","type":"string"}],"name":"VoteCast","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"internalType":"address","name":"voter","type":"address"},{"indexed":false,"internalType":"uint256","name":"proposalId","type":"uint256"},{"indexed":false,"internalType":"uint8","name":"support","type":"uint8"},{"indexed":false,"internalType":"uint256","name":"weight","type":"uint256"},{"indexed":false,"internalType":"string","name":"reason","type":"string"},{"indexed":false,"internalType":"bytes","name":"params","type":"bytes"}],"name":"VoteCastWithParams","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"oldVotingDelay","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"newVotingDelay","type":"uint256"}],"name":"VotingDelaySet","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"oldVotingPeriod","type":"uint256"},{"indexed":false,"internalType":"uint256","name":"newVotingPeriod","type":"uint256"}],"name":"VotingPeriodSet","type":"event"},{"inputs":[],"name":"BALLOT_TYPEHASH","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"COUNTING_MODE","outputs":[{"internalType":"string","name":"","type":"string"}],"stateMutability":"pure","type":"function"},{"inputs":[],"name":"EXTENDED_BALLOT_TYPEHASH","outputs":[{"internalType":"bytes32","name":"","type":"bytes32"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"contract IVotesUpgradeable","name":"veToken","type":"address"},{"internalType":"contract TimelockControllerUpgradeable","name":"timelockController","type":"address"},{"internalType":"uint256","name":"votingDelay_","type":"uint256"},{"internalType":"uint256","name":"votingPeriod_","type":"uint256"},{"internalType":"uint256","name":"threshold_","type":"uint256"},{"internalType":"uint256","name":"quorum_","type":"uint256"}],"name":"__MentoGovernor_init","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"cancel","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"},{"internalType":"uint8","name":"support","type":"uint8"}],"name":"castVote","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"},{"internalType":"uint8","name":"support","type":"uint8"},{"internalType":"uint8","name":"v","type":"uint8"},{"internalType":"bytes32","name":"r","type":"bytes32"},{"internalType":"bytes32","name":"s","type":"bytes32"}],"name":"castVoteBySig","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"},{"internalType":"uint8","name":"support","type":"uint8"},{"internalType":"string","name":"reason","type":"string"}],"name":"castVoteWithReason","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"},{"internalType":"uint8","name":"support","type":"uint8"},{"internalType":"string","name":"reason","type":"string"},{"internalType":"bytes","name":"params","type":"bytes"}],"name":"castVoteWithReasonAndParams","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"},{"internalType":"uint8","name":"support","type":"uint8"},{"internalType":"string","name":"reason","type":"string"},{"internalType":"bytes","name":"params","type":"bytes"},{"internalType":"uint8","name":"v","type":"uint8"},{"internalType":"bytes32","name":"r","type":"bytes32"},{"internalType":"bytes32","name":"s","type":"bytes32"}],"name":"castVoteWithReasonAndParamsBySig","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"address[]","name":"targets","type":"address[]"},{"internalType":"uint256[]","name":"values","type":"uint256[]"},{"internalType":"bytes[]","name":"calldatas","type":"bytes[]"},{"internalType":"bytes32","name":"descriptionHash","type":"bytes32"}],"name":"execute","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"payable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"execute","outputs":[],"stateMutability":"payable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"getActions","outputs":[{"internalType":"address[]","name":"targets","type":"address[]"},{"internalType":"uint256[]","name":"values","type":"uint256[]"},{"internalType":"string[]","name":"signatures","type":"string[]"},{"internalType":"bytes[]","name":"calldatas","type":"bytes[]"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"},{"internalType":"address","name":"voter","type":"address"}],"name":"getReceipt","outputs":[{"components":[{"internalType":"bool","name":"hasVoted","type":"bool"},{"internalType":"uint8","name":"support","type":"uint8"},{"internalType":"uint96","name":"votes","type":"uint96"}],"internalType":"struct IGovernorCompatibilityBravoUpgradeable.Receipt","name":"","type":"tuple"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"address","name":"account","type":"address"},{"internalType":"uint256","name":"blockNumber","type":"uint256"}],"name":"getVotes","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"address","name":"account","type":"address"},{"internalType":"uint256","name":"blockNumber","type":"uint256"},{"internalType":"bytes","name":"params","type":"bytes"}],"name":"getVotesWithParams","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"},{"internalType":"address","name":"account","type":"address"}],"name":"hasVoted","outputs":[{"internalType":"bool","name":"","type":"bool"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"address[]","name":"targets","type":"address[]"},{"internalType":"uint256[]","name":"values","type":"uint256[]"},{"internalType":"bytes[]","name":"calldatas","type":"bytes[]"},{"internalType":"bytes32","name":"descriptionHash","type":"bytes32"}],"name":"hashProposal","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"pure","type":"function"},{"inputs":[],"name":"name","outputs":[{"internalType":"string","name":"","type":"string"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"address","name":"","type":"address"},{"internalType":"address","name":"","type":"address"},{"internalType":"uint256[]","name":"","type":"uint256[]"},{"internalType":"uint256[]","name":"","type":"uint256[]"},{"internalType":"bytes","name":"","type":"bytes"}],"name":"onERC1155BatchReceived","outputs":[{"internalType":"bytes4","name":"","type":"bytes4"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"address","name":"","type":"address"},{"internalType":"address","name":"","type":"address"},{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"bytes","name":"","type":"bytes"}],"name":"onERC1155Received","outputs":[{"internalType":"bytes4","name":"","type":"bytes4"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"address","name":"","type":"address"},{"internalType":"address","name":"","type":"address"},{"internalType":"uint256","name":"","type":"uint256"},{"internalType":"bytes","name":"","type":"bytes"}],"name":"onERC721Received","outputs":[{"internalType":"bytes4","name":"","type":"bytes4"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"proposalDeadline","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"proposalEta","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"proposalSnapshot","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"proposalThreshold","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"proposals","outputs":[{"internalType":"uint256","name":"id","type":"uint256"},{"internalType":"address","name":"proposer","type":"address"},{"internalType":"uint256","name":"eta","type":"uint256"},{"internalType":"uint256","name":"startBlock","type":"uint256"},{"internalType":"uint256","name":"endBlock","type":"uint256"},{"internalType":"uint256","name":"forVotes","type":"uint256"},{"internalType":"uint256","name":"againstVotes","type":"uint256"},{"internalType":"uint256","name":"abstainVotes","type":"uint256"},{"internalType":"bool","name":"canceled","type":"bool"},{"internalType":"bool","name":"executed","type":"bool"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"address[]","name":"targets","type":"address[]"},{"internalType":"uint256[]","name":"values","type":"uint256[]"},{"internalType":"bytes[]","name":"calldatas","type":"bytes[]"},{"internalType":"string","name":"description","type":"string"}],"name":"propose","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"address[]","name":"targets","type":"address[]"},{"internalType":"uint256[]","name":"values","type":"uint256[]"},{"internalType":"string[]","name":"signatures","type":"string[]"},{"internalType":"bytes[]","name":"calldatas","type":"bytes[]"},{"internalType":"string","name":"description","type":"string"}],"name":"propose","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"address[]","name":"targets","type":"address[]"},{"internalType":"uint256[]","name":"values","type":"uint256[]"},{"internalType":"bytes[]","name":"calldatas","type":"bytes[]"},{"internalType":"bytes32","name":"descriptionHash","type":"bytes32"}],"name":"queue","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"queue","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"blockNumber","type":"uint256"}],"name":"quorum","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"quorumDenominator","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"uint256","name":"blockNumber","type":"uint256"}],"name":"quorumNumerator","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"quorumNumerator","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"quorumVotes","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"address","name":"target","type":"address"},{"internalType":"uint256","name":"value","type":"uint256"},{"internalType":"bytes","name":"data","type":"bytes"}],"name":"relay","outputs":[],"stateMutability":"payable","type":"function"},{"inputs":[{"internalType":"uint256","name":"newProposalThreshold","type":"uint256"}],"name":"setProposalThreshold","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"newVotingDelay","type":"uint256"}],"name":"setVotingDelay","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"newVotingPeriod","type":"uint256"}],"name":"setVotingPeriod","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"uint256","name":"proposalId","type":"uint256"}],"name":"state","outputs":[{"internalType":"enum IGovernorUpgradeable.ProposalState","name":"","type":"uint8"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"bytes4","name":"interfaceId","type":"bytes4"}],"name":"supportsInterface","outputs":[{"internalType":"bool","name":"","type":"bool"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"timelock","outputs":[{"internalType":"address","name":"","type":"address"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"token","outputs":[{"internalType":"contract IVotesUpgradeable","name":"","type":"address"}],"stateMutability":"view","type":"function"},{"inputs":[{"internalType":"uint256","name":"newQuorumNumerator","type":"uint256"}],"name":"updateQuorumNumerator","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"contract TimelockControllerUpgradeable","name":"newTimelock","type":"address"}],"name":"updateTimelock","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[],"name":"version","outputs":[{"internalType":"string","name":"","type":"string"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"votingDelay","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"inputs":[],"name":"votingPeriod","outputs":[{"internalType":"uint256","name":"","type":"uint256"}],"stateMutability":"view","type":"function"},{"stateMutability":"payable","type":"receive"}]`;
// Only the 4 events the app handles — matches the comment header above.
// The JS body is not executed by QuickNode for template-based webhooks.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If this comment is accurate, we're dropping the only governor-contract filter. The same PR also documents that QuickNode ignores the contracts filter in templateArgs, and the runtime validators/handlers do not check event.address anywhere in src/events/configs.ts. That means any other contract emitting these standard Governor events can now trigger false governance alerts.

@@ -1,3 +1,17 @@
# ⚠️ IMPORTANT: The `filter_function` blobs in this file are NOT the deployment source of truth.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is only true for updates to existing webhooks. Terraform still POSTs the filter_function blob when it creates or recreates a webhook, so this file is still a bootstrap source of truth. Because the blob contents were not refreshed in this PR, delete/recreate flows will come up with stale filter config until someone manually patches them afterward.

"test:prod:healthcheck": "./bin/test-deployed-function.sh healthcheck",
"todo": "git ls-files -c --exclude-standard | grep -v \"package.json\" | xargs grep -n -i --color \"TODO:\\|FIXME:\""
"todo": "git ls-files -c --exclude-standard | grep -v \"package.json\" | xargs grep -n -i --color \"TODO:\\|FIXME:\"",
"test:unit": "vitest run",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

test:unit is useful, but it's not wired into the existing npm test path above. Right now the default test command still only runs the curl-based local smoke flow, so these new regression tests are easy to skip accidentally. Please make the normal test path execute them as well.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR replaces per-call Secret Manager client construction with a singleton client, introduces Vitest unit tests for secret retrieval behavior, and switches QuickNode filter deployment to template-arg PATCH updates. It also adds runtime governor-address filtering to avoid false positives from non-governor contracts.

src/events/process-event.ts

  • 🟡 IMPORTANT (32-39): New routing-critical logic drops governance events by contract address, but there is no direct test coverage for this behavior. Add tests that explicitly verify: (1) governor events from the canonical address are processed, (2) same event names from non-governor addresses are skipped, and (3) mixed-case addresses are handled correctly.

infra/quicknode.tf

  • 🟢 SUGGESTION (1-14 vs 15-25): The newly added header says filter updates are script-driven and Terraform is creation-only, but the older guidance directly below still instructs pause/update/reactivate via Terraform. Consolidate to one canonical workflow to avoid operator confusion.

Final Verdict

🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

// Guard: governance events must originate from the MentoGovernor contract.
// Other contracts on Celo emit identical OZ Governor event signatures; without
// this check, those would produce false notifications.
if (GOVERNOR_EVENT_TYPES.has(event.name as EventType)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

processEvent now includes routing-critical guard logic for governance events, but this file still has no direct tests. Please add focused tests for: allowed governor address path, rejected non-governor path, and mixed-case address normalization.

# (infra/quicknode-filter-functions/*.js) and run:
# ./bin/deploy-quicknode-filter.sh --webhook <healthcheck|governor>
#
# Terraform is only used here to CREATE webhooks (on initial bootstrap). Updates use the script.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new header says updates are script-driven and Terraform is create-only, but the existing guidance lower in the same file still describes pause/update/reactivate via Terraform. Please reconcile these sections so operators have a single unambiguous workflow.

@gisk0 gisk0 force-pushed the fix/secret-manager-client-leak branch 2 times, most recently from d89d1a0 to b517eee Compare March 14, 2026 10:37
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR introduces two functional improvements: a singleton Secret Manager client to stop gRPC channel leaks/OOM, and stricter governor-event filtering to avoid false positives from non-governor contracts. It also adds unit tests and Vitest wiring.

src/events/process-event.ts

  • 🔴 Critical (line 33): event.address.toLowerCase() assumes address is always a string. The current payload validator only checks field presence ("address" in event), not type, so a malformed payload can trigger a runtime TypeError and fail the whole request with 500. This is a new runtime crash path introduced by the guard. Fix by type-checking before lowercasing and/or tightening payload validation to enforce string types.
  • 🟡 Important (line 12): MENTO_GOVERNOR_ADDRESS is hardcoded here while contract addresses are also maintained in filter files/Terraform. This creates drift risk: a governor address rotation can silently drop all governance events unless this code constant is also updated. Pull the address from config/env (single source of truth).

src/events/__tests__/process-event.test.ts

  • 🟡 Important (lines 44, 53, 62, 71, 80, 93, 110): tests repeatedly call processEvent(event as never), which bypasses type safety and can hide payload-shape regressions. Build typed fixtures (QuicknodeEvent / specific event types) and add a non-string address test to cover the new guard’s non-happy path.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR switches QuickNode webhook filter deployment from Terraform/base64 updates to live template-arg patching, and adds runtime guards/tests for governor-address filtering plus a singleton Secret Manager client. The direction is good, but there are two important regressions that should be fixed before merge.

src/events/process-event.ts

  • 🟡 IMPORTANT (line 33): event.address.toLowerCase() assumes address is a string, but payload validation currently only checks key presence ("address" in event) rather than type. A malformed payload can now throw a runtime TypeError and fail the whole webhook request instead of being handled/rejected cleanly. Add a type guard (typeof event.address === "string") here or tighten request validation upstream.

src/events/__tests__/process-event.test.ts

  • 🟡 IMPORTANT (line 1): this test is placed under src/ and imported in the normal TS project scope. With current build config (tsconfig.build.json includes src/**/*), test files are compiled into production artifacts. That couples build output to test-only code and dependencies.

src/utils/__tests__/get-secret.test.ts

  • 🟡 IMPORTANT (line 1): same issue as above — test files under src/ are included by production build config. Move tests outside src or explicitly exclude **/*.test.ts/**/__tests__/** from build TS config.

bin/deploy-quicknode-filter.sh

  • 🟢 SUGGESTION (lines 8-11): file header comment still describes the old pause → update → reactivate lifecycle, but implementation now does template patching without pause. Update the header to avoid operator confusion.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

// Other contracts on Celo emit identical OZ Governor event signatures; without
// this check, those would produce false notifications.
if (GOVERNOR_EVENT_TYPES.has(event.name as EventType)) {
if (event.address.toLowerCase() !== MENTO_GOVERNOR_ADDRESS) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

event.address.toLowerCase() is unsafe unless address is guaranteed to be a string. Current payload validation only checks presence of the address key, so malformed payloads can now cause a runtime TypeError and fail the full webhook request. Please add a type guard here (or strengthen upstream validation).

@@ -0,0 +1,115 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This new test file lives under src/, which is included by the production TS build (tsconfig.build.json includes src/**/*). That means test code gets emitted into deploy artifacts. Please either move tests outside src or exclude test globs (**/*.test.ts, **/__tests__/**) from the build TS config.

@@ -0,0 +1,91 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same build-scope issue as the other new test file: placing tests under src/ causes them to be part of production build output with current TS config. Recommend moving tests out of src or explicitly excluding test globs from tsconfig.build.json.

@gisk0 gisk0 force-pushed the fix/secret-manager-client-leak branch 2 times, most recently from 7ddaeb3 to b0f1a7f Compare March 14, 2026 10:42
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR hardens runtime behavior by reusing a singleton Secret Manager client and adding a governor-address guard for governance events, plus adds unit tests and updates the QuickNode deployment docs/workflow.

Findings

🔴 CRITICAL

  • osv-scanner.toml:7, osv-scanner.toml:12, osv-scanner.toml:17, osv-scanner.toml:22, osv-scanner.toml:27, osv-scanner.toml:32
    These entries suppress high-severity advisories (flatted / undici) with the reason "No upgrade path available upstream," but the lockfile still pins vulnerable versions (package-lock.json:3761 is flatted@3.3.1, package-lock.json:6694 is undici@6.23.0). This turns real security debt into hidden debt by silencing CI instead of remediating.

    Fix direction: upgrade to patched transitive versions (e.g. undici >= 6.24.0, flatted >= 3.4.0) via overrides/lockfile refresh, then keep scanner checks active. Only suppress with a concrete non-reachability justification.

🟢 SUGGESTION

  • bin/deploy-quicknode-filter.sh:8-bin/deploy-quicknode-filter.sh:10
    The file header comment still says the script performs a pause → update → activate cycle, but the implementation now hot-patches template args directly. Updating this comment avoids operator confusion during incident response.

Final verdict

🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR introduces a good set of fixes: singleton Secret Manager client reuse (to stop gRPC channel churn), governor-address filtering in event processing, and new unit-test coverage plus QuickNode deployment/docs cleanup. I reviewed all changed files in this PR; there is one merge-blocking issue.

src/utils/is-valid-quicknode-payload.ts

  • 🔴 Critical (L13-L17): this change fails CI (trunk-check) with @typescript-eslint/no-unnecessary-condition on event !== null because result is first force-cast to QuicknodePayload, so event is treated as QuicknodeEvent at type level. That makes the null check “impossible” to the linter and blocks merge. It also weakens the type guard by asserting QuicknodePayload before validation.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

"logIndex" in event &&
"name" in event &&
"transactionHash" in event
event !== null &&
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

trunk-check is failing here (@typescript-eslint/no-unnecessary-condition) because the function force-casts requestBody to QuicknodePayload before validation (L13-L14), so event is inferred as QuicknodeEvent and this null check is considered impossible.

Please rewrite this guard without pre-casting to QuicknodePayload (work from Record<string, unknown> / unknown[] first), then narrow step-by-step. That will both fix CI and keep the type guard sound.

@gisk0 gisk0 force-pushed the fix/secret-manager-client-leak branch from b0f1a7f to b649286 Compare March 14, 2026 10:49
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR fixes two real production concerns: it stops per-call Secret Manager client construction (gRPC channel leak/OOM risk) and adds a governor-address guard to prevent false-positive governance notifications. It also modernizes local test setup with Vitest and adds focused unit tests.

osv-scanner.toml

  • 🔴 CRITICAL: lines 7-33 suppress multiple high advisories (flatted, undici) with the claim "No upgrade path available upstream." That claim is not accurate for the current graph: npm audit fix --dry-run resolves flatted to 3.4.1 and undici to 6.24.1. This change hides known vulnerabilities until 2026-09-01 instead of remediating them.

src/utils/is-valid-quicknode-payload.ts

  • 🟡 IMPORTANT: lines 14-24 introduce stricter runtime payload validation but there is no accompanying unit test coverage for this validator. Add tests for malformed payloads (null event, missing keys, non-string values) plus one happy-path case so this guard cannot silently regress.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

osv-scanner.toml Outdated

[[IgnoredVulns]]
id = "GHSA-25h7-pfq9-p65f"
reason = "Transitive dep via discord.js → flatted. No upgrade path available upstream. Dev/runtime impact: none for governance watchdog."
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This suppression is blocking a real remediation path. In the current dependency graph, these vulnerabilities can be reduced (e.g., flatted -> 3.4.1, undici -> 6.24.1) instead of being ignored until 2026. Please prefer dependency remediation first, and keep ignores only for advisories with truly no patch path plus a tracking issue.

"name" in event &&
"transactionHash" in event
event !== null &&
typeof (event as Record<string, unknown>).address === "string" &&
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The validator behavior changed materially here, but there are no direct tests for isValidQuicknodePayload(). Please add a unit test suite that covers reject cases (null event object, missing required fields, wrong field types) and a valid payload case to prevent future regressions in request validation.

…docs cleanup

## Root cause fix
Every call to getSecret() instantiated a new SecretManagerServiceClient,
opening a gRPC channel that was never closed. ~169 requests over 28 minutes
→ 488 MiB OOM. Fix: singleton client at module level.

Secret values are NOT cached (would break rotation on warm instances).

## Governor address guard
QuickNode evmAbiFilter ignores the 'contracts' field in templateArgs
(silently, empirically confirmed). Added MENTO_GOVERNOR_ADDRESS guard in
process-event.ts so events from other OZ Governor contracts on Celo don't
produce false notifications.

## Deploy script fixes
- Global TMP_FILES array + single EXIT trap (fixes trap overwrite on
  multiple deploy_webhook calls)
- Code comment explaining why templateArgs uses 'abiJson' not 'abi'
  (spec says 'abi', live evmAbiFilterGo endpoint requires 'abiJson')
- Comment explaining hardcoded webhook UUIDs and how to update them
- Updated Terraform filter_function blobs to current trimmed ABIs

## Docs & cleanup
- Delete bin/update-quicknode-filter.js (zombie script, never worked)
- Remove dead dev:webhook:* npm scripts
- Consolidate quicknode.tf comment: UPDATE=script, RECREATE=Terraform
- Rewrite README + ADDING_EVENTS.md to reflect actual deploy workflow

## Tests
- 5 unit tests for get-secret.ts (singleton behavior, no caching, errors)
- 7 unit tests for process-event.ts governor address guard
  (canonical address, wrong address, all 4 event types, mixed-case, MedianUpdated)
- npm test now runs vitest before integration tests
@gisk0 gisk0 force-pushed the fix/secret-manager-client-leak branch from b649286 to 166a670 Compare March 14, 2026 10:51
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Stale comment

This PR addresses two key reliability issues: it reuses a singleton Secret Manager client to stop gRPC channel growth, and it adds governor-address filtering to prevent false-positive governance notifications. I reviewed every changed file in this diff; there is one blocking correctness issue left.

src/utils/is-valid-quicknode-payload.ts

  • 🟡 Important (line 16): the new validator still throws on malformed payloads containing null items in result.
    • Current logic checks typeof event === "object", which is true for null, then immediately dereferences event.address on line 17.
    • Repro: payload like {"result":[null]} causes TypeError: Cannot read properties of null instead of returning false.
    • Fix: require event !== null before any property reads.
  • 🟡 Important (lines 16-22): this new validation path has no dedicated unit coverage (e.g. result: [null], non-object entries, wrong field types mixed in arrays). Please add focused tests so future validator edits don’t regress to throw-vs-false behavior.

Final Verdict

🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

@@ -14,12 +14,12 @@ export default function isValidQuicknodePayload(
(requestBody as QuicknodePayload).result.every((event) => {
return (
typeof event === "object" &&
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

typeof event === "object" is also true for null. With the current condition order, result: [null] reaches the next checks and throws when reading event.address.

Please add event !== null before property reads so malformed payloads return false instead of raising a TypeError.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Summary: This PR introduces a good fix direction (singleton Secret Manager client + governor-address guard) and adds meaningful unit tests/docs, but it currently introduces a build-breaking TypeScript regression. There is also a dependency override change that can silently pull breaking major versions.

src/utils/is-valid-quicknode-payload.ts

  • 🔴 Critical: L17-L22 introduces casts that fail strict TypeScript compilation (TS2352), and npm run build now fails. This blocks deploy/build.

package.json

  • 🟡 Important: L41 uses "undici": ">=6.24.0", which is open-ended and already resolves to major undici@7 in the lockfile. That can silently introduce breaking changes when lockfiles are regenerated.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

"logIndex" in event &&
"name" in event &&
"transactionHash" in event
typeof (event as Record<string, unknown>).address === "string" &&
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line introduces a merge blocker: npm run build fails under strict TS with TS2352 (same error repeats on lines 18-22).

event is already inferred from (requestBody as QuicknodePayload).result, so casting directly to Record<string, unknown> is rejected. Please replace this with a safe narrowing approach (e.g., cast through unknown, or assign to a local const e = event as unknown as Record<string, unknown> once after proper object/null guards).

"overrides": {
"undici": "^6.23.0"
"flatted": ">=3.4.0",
"undici": ">=6.24.0"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using an open-ended override (>=6.24.0) allows future major upgrades and has already resolved to undici@7.24.2 in the lockfile while transitive deps request 6.x.

Please pin this to a bounded range (^6.24.0 or an explicit version) unless the team explicitly wants automatic major-version drift in transitive deps.

…docs cleanup

Every call to getSecret() instantiated a new SecretManagerServiceClient,
opening a gRPC channel that was never closed. ~169 requests over 28 minutes
→ 488 MiB OOM. Fix: singleton client at module level.

Secret values are NOT cached (would break rotation on warm instances).

QuickNode evmAbiFilter ignores the 'contracts' field in templateArgs
(silently, empirically confirmed). Added MENTO_GOVERNOR_ADDRESS guard in
process-event.ts so events from other OZ Governor contracts on Celo don't
produce false notifications.

- Global TMP_FILES array + single EXIT trap (fixes trap overwrite on
  multiple deploy_webhook calls)
- Code comment explaining why templateArgs uses 'abiJson' not 'abi'
  (spec says 'abi', live evmAbiFilterGo endpoint requires 'abiJson')
- Comment explaining hardcoded webhook UUIDs and how to update them
- Updated Terraform filter_function blobs to current trimmed ABIs

- Delete bin/update-quicknode-filter.js (zombie script, never worked)
- Remove dead dev:webhook:* npm scripts
- Consolidate quicknode.tf comment: UPDATE=script, RECREATE=Terraform
- Rewrite README + ADDING_EVENTS.md to reflect actual deploy workflow

- 5 unit tests for get-secret.ts (singleton behavior, no caching, errors)
- 7 unit tests for process-event.ts governor address guard
  (canonical address, wrong address, all 4 event types, mixed-case, MedianUpdated)
- npm test now runs vitest before integration tests
@chapati23 chapati23 merged commit 999ce4e into main Mar 14, 2026
2 checks passed
@chapati23 chapati23 deleted the fix/secret-manager-client-leak branch March 14, 2026 11:12
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

This PR hardens event filtering and secret access behavior: it adds a governor-address guard in processEvent, switches Secret Manager usage to a singleton client, introduces unit tests with Vitest, and updates QuickNode filter deployment docs/scripts.

src/utils/is-valid-quicknode-payload.ts

  • 🟡 Important (L15-L19): malformed payloads like { result: [null] } throw a runtime TypeError (Cannot read properties of null) instead of returning false. The validator should never throw on bad input.
    • Suggested fix:
      (requestBody as QuicknodePayload).result.every((event) => {
        if (typeof event !== "object" || event === null) return false;
        const e = event as Record<string, unknown>;
        return (
          typeof e.address === "string" &&
          typeof e.blockHash === "string" &&
          typeof e.blockNumber === "string" &&
          typeof e.logIndex === "string" &&
          typeof e.name === "string" &&
          typeof e.transactionHash === "string"
        );
      })
  • 🟡 Important (L6-L27): this file’s new validation logic has no dedicated unit coverage (especially null/non-object items in result), so the above failure mode is currently untested.

package.json

  • 🟡 Important (L41): overrides.undici is set to ">=6.24.0", which permits major upgrades and currently resolves to undici@7.x. This can break transitive packages pinned to the 6.x API surface.
    • Suggested fix: pin to a safe 6.x range (for example "^6.24.0" or a fixed "6.24.x") and regenerate lockfile.

package-lock.json

  • 🟡 Important (L247, L6695): lockfile shows @discordjs/rest requiring undici: "6.21.3" while install resolves to undici@7.24.2. That mismatch is exactly the class of breakage the override should avoid.

ADDING_EVENTS.md

  • 🟢 Suggestion (L430): summary step still says “Migrate to Terraform”, but this PR’s new workflow explicitly states filter updates are done via deploy-quicknode-filter.sh (Terraform only for creation/bootstrap). Update summary wording to match the new process.

Final verdict: 🚫 Needs changes

Open in Web View Automation 

Sent by Cursor Automation: PR Review

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