Monitoring: fail closed on undelivered events - #961
Conversation
47b41c0 to
5b0b0ad
Compare
…errors - Coverage check now skips events where all receivers returned "ignored" (no receiver configured for that event type), allowing deployments that run with only a subset of receivers without halting monitoring. - Coverage check also skips events whose dispatches were rejected, since the dispatch error is already recorded and a second coverage error would be misleading. - Replace switch/case dispatch blocks with if/else for clarity. - Replace async-reduce test runner with a for-of loop for readability. - Document the 10,000-block catch-up cap and the new dispatch coverage failure mode in README.adoc.
751e35a to
f354669
Compare
📝 WalkthroughWalkthroughThis PR tightens monitoring execution and delivery rules, limits scan and retention ranges, adds duplicate acknowledgement handling, updates the monitoring README, and adds a compiled test runner with CI execution. ChangesMonitoring reliability updates
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monitoring/src/system-event.ts`:
- Around line 280-283: Update the acknowledgement-status guard in trigger so an
empty this.receivers collection reports a coverage failure instead of returning
and advancing the checkpoint; only skip coverage when at least one receiver was
attempted and every dispatch was rejected. Add a test covering trigger with no
registered receivers and verify the monitor event is not silently skipped.
- Line 165: Adjust the toBlock calculation in the system event scan so the
inclusive queryFilter range covers at most maxBlockRange blocks by subtracting
one from the upper-bound offset. Add coverage verifying the boundary case scans
exactly 10,000 blocks and preserves the latestBlock cap.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b0b9c7b-c183-45c9-801d-cc995ca3aec0
📒 Files selected for processing (14)
.github/workflows/monitoring.yml.gitignoremonitoring/.eslintignoremonitoring/.prettierignoremonitoring/README.adocmonitoring/package.jsonmonitoring/src/file-persistence.tsmonitoring/src/sentry-receiver.tsmonitoring/src/system-event.tsmonitoring/test/run.tsmonitoring/test/sentry-receiver.test.tsmonitoring/test/system-event-manager.test.tsmonitoring/test/test-runner.tsmonitoring/tsconfig.test.json
| fromBlock - reorgDepthBlocks > 0 ? fromBlock - reorgDepthBlocks : 0 | ||
|
|
||
| const toBlock = latestBlock | ||
| const toBlock = Math.min(latestBlock, fromBlock + maxBlockRange) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)monitoring/src/system-event\.ts$|system-event' || true
echo "== file outline =="
ast-grep outline monitoring/src/system-event.ts || true
echo "== relevant lines =="
sed -n '130,205p' monitoring/src/system-event.ts
echo "== occurrences of maxBlockRange / toBlock / range bounds =="
rg -n "maxBlockRange|toBlock|fromBlock|blockRange|inclusive|range" monitoring/src/system-event.ts monitoring/src || true
echo "== nearby tests/usages =="
rg -n "system-event|monitor block|block range|maxBlockRange|fromBlock|toBlock" monitoring -S || trueRepository: threshold-network/tbtc-v2
Length of output: 18601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deposit-monitor check implementation =="
sed -n '55,80p' monitoring/src/deposit-monitor.ts
echo "== minting / redemption / wallet / supply implementations around range calls =="
for f in monitoring/src/minting-monitor.ts monitoring/src/redemption-monitor.ts monitoring/src/wallet-monitor.ts monitoring/src/supply-monitor.ts; do
echo "--- $f ---"
rg -n "for .*block|blockRange|range.|toBlock|fromBlock|maxRange|loadChainDataCache|query.*block|block.*filter|filter.*block|includes|fromBlock <=" "$f" -C 2 || rg -n "block" "$f" -C 2 || true
done
echo "== Manager loop around check range =="
sed -n '208,235p' monitoring/src/system-event.ts
echo "== tests for system-event manager triggers =="
sed -n '1,220p' monitoring/test/system-event-manager.test.ts
echo "== all README max block range mentions =="
rg -n "maximum block range|10,000|10000|max block range|maxBlockRange|scan" monitoring/README.adoc -C 2Repository: threshold-network/tbtc-v2
Length of output: 12495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find event query helpers =="
rg -n "getDepositRevealedEvents|getRedemptionRequestedEvents|getNewWalletRegisteredEvents|getDkgResultSubmittedEvents|getDkgResultApprovedEvents|getDkgResultChallengedEvents|getOptimisticMintingRequestedEvents|withTopics|events|queryBlocks|eth_fetch|toBlock|fromBlock" . \
-g '*.ts' -g '*.json' -g '*.js' \
-C 3 || true
echo "== package / dependency clues =="
for f in package.json monitoring/package.json package-lock.json pnpm-lock.yaml yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,180p' "$f"
fi
doneRepository: threshold-network/tbtc-v2
Length of output: 50382
🌐 Web query:
ethers.js QueryFilter fromBlock toBlock inclusive range
💡 Result:
In ethers.js, the fromBlock and toBlock parameters used in queryFilter and general Filter objects define an inclusive range for searching blockchain logs [1][2][3]. When using the contract.queryFilter(event, fromBlock, toBlock) method: - fromBlock: Specifies the starting block of the search range. It is inclusive, meaning logs from this block are included [1]. It defaults to 0 if not provided [1][4]. - toBlock: Specifies the ending block of the search range. It is also inclusive, meaning logs up to and including this block are retrieved [1]. It defaults to "latest" if not provided [1][4]. Because both boundaries are inclusive, specifying the same block number for both fromBlock and toBlock will restrict the search to that specific block. These properties are part of the broader Filter interface in ethers.js, which applies to both high-level contract queries and low-level provider.getLogs calls [2][3].
Citations:
- 1: https://docs.ethers.org/v6/api/contract/
- 2: https://github.com/ethers-io/ethers.js/blob/main/src.ts/providers/provider.ts
- 3: https://docs.ethers.org/v6/api/providers/
- 4: https://github.com/ethers-io/ethers.js/blob/c80fcddf50a9023486e9f9acb1848aba4c19f7b6/packages/contracts/src.ts/index.ts
Cap the inclusive scan to 10,000 blocks.
toBlock passes an inclusive ethers.js queryFilter range, so fromBlock + 10000 scans 10,001 blocks. Use fromBlock + maxBlockRange - 1, or define maxBlockRange as exclusive. Add coverage for the boundary case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@monitoring/src/system-event.ts` at line 165, Adjust the toBlock calculation
in the system event scan so the inclusive queryFilter range covers at most
maxBlockRange blocks by subtracting one from the upper-bound offset. Add
coverage verifying the boundary case scans exactly 10,000 blocks and preserves
the latestBlock cap.
| statuses.size === 0 || | ||
| (statuses.size === 1 && statuses.has("ignored")) | ||
| ) | ||
| return |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Report a coverage failure when no receiver is registered.
Line 280 treats zero acknowledgements as rejected dispatches. With this.receivers empty, no dispatch rejects, this branch returns, and trigger advances the checkpoint. The monitor events are then skipped without delivery. Only skip coverage when at least one receiver was attempted and all dispatches rejected. Add a test with an empty receiver list.
Proposed fix
- if (
- statuses.size === 0 ||
- (statuses.size === 1 && statuses.has("ignored"))
- )
+ const allDispatchesRejected =
+ this.receivers.length > 0 && statuses.size === 0
+ const allReceiversIgnored =
+ statuses.size === 1 && statuses.has("ignored")
+ if (allDispatchesRejected || allReceiversIgnored)
return📝 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.
| statuses.size === 0 || | |
| (statuses.size === 1 && statuses.has("ignored")) | |
| ) | |
| return | |
| const allDispatchesRejected = | |
| this.receivers.length > 0 && statuses.size === 0 | |
| const allReceiversIgnored = | |
| statuses.size === 1 && statuses.has("ignored") | |
| if (allDispatchesRejected || allReceiversIgnored) | |
| return |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@monitoring/src/system-event.ts` around lines 280 - 283, Update the
acknowledgement-status guard in trigger so an empty this.receivers collection
reports a coverage failure instead of returning and advancing the checkpoint;
only skip coverage when at least one receiver was attempted and every dispatch
was rejected. Add a test covering trigger with no registered receivers and
verify the monitor event is not silently skipped.
2 participants
Summary
Validation
git diff --checkpassed locally for the safe-fix batchNotes
This is public-safe operational hardening only and does not include any private deployment mitigation.
Summary by CodeRabbit
Reliability
Documentation
Tests