Conversation
📝 WalkthroughWalkthroughThe pull request adds comprehensive error handling to two database query functions—GetBudgetUsageSat and GetIsolatedBalance—by changing their return types to include error values. All call sites throughout the codebase are updated to capture and handle these errors, ensuring failures in budget usage and balance retrieval operations are properly logged and propagated rather than silently ignored. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nip47/controllers/get_balance_controller.go (1)
46-48:⚠️ Potential issue | 🟠 MajorPre-existing: potential nil dereference if
GetBalancesfails.
balance = balances.Lightning.TotalSpendableon Line 47 executes before the error check on Line 48. IfGetBalancesreturns an error and a nil/zero-valuedbalances, this could panic. Not introduced by this PR, but worth fixing since you're already touching this function.Proposed fix
} else { balances, err := controller.lnClient.GetBalances(ctx, true) - balance = balances.Lightning.TotalSpendable if err != nil { logger.Logger.WithFields(logrus.Fields{ "request_event_id": requestEventId, }).WithError(err).Error("Failed to fetch balance") publishResponse(&models.Response{ ResultType: nip47Request.Method, Error: mapNip47Error(err), }, nostr.Tags{}) return } + balance = balances.Lightning.TotalSpendable }
🤖 Fix all issues with AI agents
In `@api/api.go`:
- Around line 585-592: In ListApps, a failure from queries.GetIsolatedBalance
currently returns the error and aborts the whole listing; change this to match
GetApp's soft-fail behavior: if queries.GetIsolatedBalance(api.db, dbApp.ID)
returns an error, log it with
logger.Logger.WithError(err).WithFields(...).Error("Failed to get isolated app
balance") and do NOT return — instead set apiApp.Balance to a safe default
(e.g., zero or nil depending on the Balance type) and continue processing the
rest of apps so a single isolated balance DB hiccup doesn't fail the entire
ListApps result.
In `@transactions/transactions_service.go`:
- Around line 1057-1060: The error returned from GetIsolatedBalance is
discarded; update the error return in the block that calls
queries.GetIsolatedBalance(tx, appPermission.AppId) to wrap the original err
(e.g., using fmt.Errorf with %w) and include context such as the app id and a
short message—replace the current return errors.New(...) with something like
fmt.Errorf("failed to calculate isolated balance for app %s: %w",
appPermission.AppId, err) so the root cause is preserved when propagated from
functions like the transaction creator.
- Around line 1087-1090: The error returned from queries.GetBudgetUsageSat(tx,
&appPermission) is being discarded; change the return to include the original
err (e.g., use fmt.Errorf or errors.Wrap) so the root cause is preserved when
returning from the current function where GetBudgetUsageSat and appPermission
are used; ensure the returned message still describes the context (e.g., "failed
to calculate budget usage for app: %w", err).
🧹 Nitpick comments (2)
api/api.go (2)
428-433: Inconsistent error handling pattern withinGetApp:BudgetUsageis always set (even on error), butBalanceis conditionally set.On error,
budgetUsagedefaults to0and is unconditionally assigned at line 462. This means a DB failure silently shows zero usage, potentially misleading the caller into thinking the full budget is available. Compare with theBalancehandling at lines 472–479, which only sets the field on success.Consider either returning an error or guarding assignment like you did for
Balance:Proposed fix
budgetUsage, err := queries.GetBudgetUsageSat(api.db, &paySpecificPermission) if err != nil { logger.Logger.WithError(err).WithFields(logrus.Fields{ "app_id": dbApp.ID, }).Error("Failed to get budget usage for app") + } else { + response.BudgetUsage = budgetUsage }And remove the static assignment at line 462:
- BudgetUsage: budgetUsage,
601-608: Same hard-fail concern as the isolated balance query above.Budget usage query failure for a single app also aborts the entire
ListAppsresponse. This is consistent with the pattern at line 590, but both differ from the soft-fail approach inGetApp. Consider aligning the error handling strategy across both methods.
| }, nostr.Tags{}) | ||
| return | ||
| } | ||
| balance = balances.Lightning.TotalSpendable |
There was a problem hiding this comment.
Coderabbit saves the day, yet again 😁
There was a problem hiding this comment.
Could you have a look and see if we can add any missing tests to make sure we do not rely on Coderabbit
2 participants
Fixes #2061
Summary by CodeRabbit