Skip to content

fix: add error logging to silent catch blocks in API handlers#388

Merged
andreabadesso merged 2 commits intomasterfrom
fix/add-error-logging-to-silent-catch-blocks
Apr 9, 2026
Merged

fix: add error logging to silent catch blocks in API handlers#388
andreabadesso merged 2 commits intomasterfrom
fix/add-error-logging-to-silent-catch-blocks

Conversation

@andreabadesso
Copy link
Copy Markdown
Collaborator

@andreabadesso andreabadesso commented Apr 8, 2026

Motivation

Following up on #387, an audit of the codebase revealed several other API handlers with catch blocks that silently swallow errors without any logging, making it impossible to diagnose failures in production.

Acceptance Criteria

  • walletIdProxyHandler in commons.ts logs errors when authorization context extraction fails
  • txProposalDestroy handler logs errors when the DB transaction (cancel + release UTXOs) fails
  • pushRegister handler logs errors when the DB transaction (remove + register device) fails

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged
  • Make sure either the unit tests and/or the QA tests are capable of testing the new features
  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Chores
    • Enhanced error handling and logging throughout the wallet service for improved system monitoring and diagnostic capabilities.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 8, 2026

📝 Walkthrough

Walkthrough

Three API handlers in the wallet service now include error logging in their exception-handling paths. When operations fail during register, transaction proposal destruction, and wallet ID proxy handling, the code instantiates a default logger and emits error details before returning the same error responses.

Changes

Cohort / File(s) Summary
Error logging enhancement
packages/wallet-service/src/api/pushRegister.ts, packages/wallet-service/src/api/txProposalDestroy.ts, packages/wallet-service/src/commons.ts
Added default logger instantiation and error logging to catch blocks in three handlers. Each logs relevant context (walletId, deviceId, txProposalId, or exception message) before returning existing error responses.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

enhancement

Suggested reviewers

  • r4mmer
  • pedroferreira1

Poem

🐰 Whiskers twitch with logging glee,
Error paths now all can see!
When things go wrong in API land,
Messages logged, oh isn't it grand?

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding error logging to silent catch blocks in API handlers, which aligns perfectly with the three files modified.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/add-error-logging-to-silent-catch-blocks

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@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.

🧹 Nitpick comments (1)
packages/wallet-service/src/commons.ts (1)

555-563: Initialize logger only in the catch path.

Line 555 creates a logger for every invocation, even when walletId extraction succeeds. Move it into catch (same pattern used in pushRegister and txProposalDestroy) to avoid unnecessary work on the hot path.

Proposed diff
 export const walletIdProxyHandler = (handler: WalletProxyHandler): APIGatewayProxyHandler => (
   async (event, context) => {
-    const logger = createDefaultLogger();
     let walletId: string;
     try {
       walletId = event.requestContext.authorizer.principalId;
       // validate walletId?
     } catch (e) {
+      const logger = createDefaultLogger();
       logger.error('Failed to extract walletId from authorization context', {
         error: e.message,
       });
       return {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/wallet-service/src/commons.ts` around lines 555 - 563, The logger is
being created unconditionally which wastes work on the hot path; move the
createDefaultLogger() call into the catch block that handles extraction of
walletId so the logger is only initialized when an error occurs. Locate the
walletId extraction (walletId = event.requestContext.authorizer.principalId) in
commons.ts and replace the eager const logger = createDefaultLogger() with
creating the logger inside the catch where logger.error(...) is called (mirror
the pattern used in pushRegister and txProposalDestroy), and ensure the catch
still logs e.message and any context after creating the logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/wallet-service/src/commons.ts`:
- Around line 555-563: The logger is being created unconditionally which wastes
work on the hot path; move the createDefaultLogger() call into the catch block
that handles extraction of walletId so the logger is only initialized when an
error occurs. Locate the walletId extraction (walletId =
event.requestContext.authorizer.principalId) in commons.ts and replace the eager
const logger = createDefaultLogger() with creating the logger inside the catch
where logger.error(...) is called (mirror the pattern used in pushRegister and
txProposalDestroy), and ensure the catch still logs e.message and any context
after creating the logger.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: edbe1d2a-93fb-4cf1-b002-9f6cb17664f2

📥 Commits

Reviewing files that changed from the base of the PR and between bcfbfa5 and 5949059.

📒 Files selected for processing (3)
  • packages/wallet-service/src/api/pushRegister.ts
  • packages/wallet-service/src/api/txProposalDestroy.ts
  • packages/wallet-service/src/commons.ts

@andreabadesso andreabadesso self-assigned this Apr 9, 2026
@andreabadesso andreabadesso added the enhancement New feature or request label Apr 9, 2026
@andreabadesso andreabadesso moved this from Todo to In Progress (Done) in Hathor Network Apr 9, 2026
@github-project-automation github-project-automation Bot moved this from In Progress (Done) to In Review (WIP) in Hathor Network Apr 9, 2026
@andreabadesso andreabadesso merged commit cc5eea2 into master Apr 9, 2026
3 checks passed
@github-project-automation github-project-automation Bot moved this from In Review (WIP) to Waiting to be deployed in Hathor Network Apr 9, 2026
@andreabadesso andreabadesso deleted the fix/add-error-logging-to-silent-catch-blocks branch April 9, 2026 14:55
@andreabadesso andreabadesso moved this from Waiting to be deployed to Done in Hathor Network Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants