Conversation
…rt multiple chat IDs
…_RELEASE_CHAT_IDS) and support threads in Telegram notifications
…letion commands (#57) * feat: implement standing order feature with creation, listing, and deletion commands * feat: add documentation comments for standing order scheduler and management functions * feat: implement cancellable context for standing order scheduler to ensure clean shutdown * feat: enhance executeOrder logic to improve idempotency and error handling * fix: modify error handling in soCreateHandler to prevent returning errors on invalid input * feat: improve error messaging in standing order handlers for better user feedback * feat: add consecutive failure tracking and auto-deactivation for standing orders
Add HMAC-authenticated analytics API for transaction queries and user history. Filter empty/ghost telebot errors from being sent to the TG error log group, and improve LNbits error handling to always return meaningful error details.
📝 WalkthroughWalkthroughThis PR adds GitHub Actions release workflows with Telegram notifications, implements transaction analytics API endpoints featuring HMAC authentication and CSV export, introduces standing orders for recurring scheduled transfers with hourly background scheduling, and extends the cache system with memo-based duplicate prevention for idempotent payments. Changes
Sequence DiagramssequenceDiagram
participant Scheduler as Standing Order<br/>Scheduler
participant DB as Database
participant User as User / Bot
participant Transfer as LNbits<br/>Transfer
participant Telegram as Telegram<br/>Notifications
loop Every Hour
Scheduler->>DB: Fetch active standing orders
DB-->>Scheduler: Active orders list
Scheduler->>DB: Filter due orders by day_of_month<br/>and LastExecutedAt
DB-->>Scheduler: Due orders
loop For each due order
Scheduler->>DB: Update LastExecutedAt (pre-mark)
DB-->>Scheduler: Updated
Scheduler->>User: Load user & wallet
User-->>Scheduler: User details
alt Wallet exists
Scheduler->>Transfer: Execute transfer
alt Transfer succeeds
Transfer-->>Scheduler: Success
Scheduler->>DB: Reset ConsecutiveFailures to 0
Scheduler->>Telegram: Send success notification
else Transfer fails
Transfer-->>Scheduler: Error
Scheduler->>DB: Restore LastExecutedAt
Scheduler->>DB: Increment ConsecutiveFailures
Scheduler->>Telegram: Send failure notification
alt Max failures reached
Scheduler->>DB: Set Active = false
Scheduler->>Telegram: Send deactivation notice
end
end
else No wallet
Scheduler->>Telegram: Skip user notification
end
end
end
sequenceDiagram
participant Client as Analytics API<br/>Client
participant Middleware as AnalyticsHMAC<br/>Middleware
participant Handler as Analytics<br/>Handler
participant DB as Database
participant Formatter as CSV/JSON<br/>Formatter
Client->>Middleware: GET /api/v1/analytics/transactions<br/>+ X-Timestamp + X-HMAC-Signature
alt Timestamp valid & within tolerance
Middleware->>Middleware: Calculate expected HMAC<br/>for each API key
alt HMAC matches configured key
Middleware->>Middleware: Store API key ID in context
Middleware->>Handler: Forward request + context
Handler->>DB: Query transactions<br/>(filters: date, type, user, limit/offset)
DB-->>Handler: Transaction results
Handler->>DB: Aggregate summary statistics
DB-->>Handler: Summary data
alt Output format = CSV
Handler->>Formatter: Convert to CSV<br/>+ injection mitigation
Formatter-->>Handler: CSV bytes
else Output format = JSON
Handler->>Formatter: Convert to JSON
Formatter-->>Handler: JSON bytes
end
Handler-->>Client: 200 OK + formatted data
else HMAC mismatch
Middleware-->>Client: 401 Unauthorized
end
else Invalid/expired timestamp
Middleware-->>Client: 400/401 Bad Request or Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 11
🧹 Nitpick comments (6)
internal/lnbits/types.go (1)
212-223: Minor formatting inconsistency and potential validation gap.
- Line 220 has inconsistent indentation (
ConsecutiveFailureshas extra leading space compared to other fields).DayOfMonthaccepts anyintvalue, but valid days are 1-31. Consider adding a GORM check constraint or documenting the expected range.♻️ Suggested fix for formatting
type StandingOrder struct { ID string `json:"id" gorm:"primaryKey"` UserID string `json:"user_id" gorm:"index"` PotName string `json:"pot_name"` - DayOfMonth int `json:"day_of_month"` + DayOfMonth int `json:"day_of_month" gorm:"check:day_of_month >= 1 AND day_of_month <= 31"` Amount int64 `json:"amount"` Active bool `json:"active" gorm:"default:true"` LastExecutedAt *time.Time `json:"last_executed_at"` - ConsecutiveFailures int `json:"consecutive_failures" gorm:"default:0"` + ConsecutiveFailures int `json:"consecutive_failures" gorm:"default:0"` CreatedAt time.Time `json:"created_at" gorm:"autoCreateTime"` UpdatedAt time.Time `json:"updated_at" gorm:"autoUpdateTime"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/lnbits/types.go` around lines 212 - 223, Fix the formatting and add validation for the StandingOrder struct: align the ConsecutiveFailures field indentation with other fields in the StandingOrder type and add a GORM check constraint (or model validation) to enforce DayOfMonth is within 1..31 (e.g., a gorm tag like check: "day_of_month >= 1 AND day_of_month <= 31" on DayOfMonth or a Validate/BeforeCreate method on StandingOrder that returns an error for out-of-range values). Reference the StandingOrder struct and the DayOfMonth and ConsecutiveFailures fields when making the changes.internal/telegram/error_logger.go (1)
55-59: Consider extracting the sentinel JSON string to a named constant.The hardcoded JSON string
{"message":"","Err":{},"code":0}is a magic value that may need to be referenced elsewhere (e.g., the AI summary mentionsmain.goalso filters the same sentinel). Extracting it to a package-level constant would improve maintainability and ensure consistency.♻️ Suggested refactor
+const emptyGhostErrorPayload = `{"message":"","Err":{},"code":0}` + // LogError logs an error to the configured Telegram group func (el *ErrorLogger) LogError(err error, context string, userInfo ...interface{}) { if !el.enabled || err == nil { return } // Filter out empty/ghost errors and irrelevant messages errorMsg := err.Error() - if errorMsg == "" || errorMsg == `{"message":"","Err":{},"code":0}` { + if errorMsg == "" || errorMsg == emptyGhostErrorPayload { return // Skip empty/meaningless errors }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/telegram/error_logger.go` around lines 55 - 59, Extract the hardcoded sentinel JSON string {"message":"","Err":{},"code":0} into a package-level constant (e.g., sentinelEmptyError) and replace the inline literal used in the error filtering code that assigns errorMsg := err.Error() and checks if errorMsg == "" || errorMsg == ...; update any other places (like main.go) that perform the same check to reference this constant to ensure consistency and maintainability.internal/lnbits/lnbits.go (1)
10-22: Consider handling the JSON unmarshaling error.The
resp.ToJSON(&reqErr)call on line 16 ignores its return value. While the fallback toRawBodyon lines 18-19 provides resilience when bothDetailandMessageare empty, explicitly checking for an unmarshal error would make the intent clearer and could help with debugging malformed responses.♻️ Suggested improvement
func parseLNbitsError(resp *req.Resp) Error { statusCode := resp.Response().StatusCode rawBody := resp.String() var reqErr Error - resp.ToJSON(&reqErr) + _ = resp.ToJSON(&reqErr) // Explicitly ignore: fallback to RawBody if parsing fails reqErr.StatusCode = statusCode if reqErr.Detail == "" && reqErr.Message == "" { reqErr.RawBody = rawBody } return reqErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/lnbits/lnbits.go` around lines 10 - 22, In parseLNbitsError, check and handle the error returned by resp.ToJSON(&reqErr) instead of ignoring it: capture the returned err, and if non-nil set reqErr.RawBody = rawBody (and/or set reqErr.Message or an UnmarshalError field with fmt.Sprintf("failed to unmarshal LNbits response: %v", err)) before populating StatusCode; add fmt import if needed. This makes it explicit when JSON decoding fails and preserves the raw response for debugging while keeping the existing fallback behavior using the resp variable and reqErr struct..github/workflows/release.yml (2)
63-80: Consider adding empty CHAT_ID handling similar to release-dev.yml.The dev workflow (release-dev.yml) includes an empty target check (
if [[ -z "$target" ]]; then continue; fi), but this production workflow doesn't. This could cause curl errors ifRELEASE_CHAT_IDScontains empty entries or trailing commas.♻️ Add empty target check for consistency
IFS=',' read -ra TARGETS <<< "${{ env.CHAT_ID }}" for target in "${TARGETS[@]}"; do target=$(echo "$target" | xargs) + if [[ -z "$target" ]]; then + continue + fi if [[ "$target" == *":"* ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 63 - 80, The for-loop over TARGETS may process empty entries and should skip them like in release-dev.yml; inside the loop in the block that starts with for target in "${TARGETS[@]}"; do (after trimming with target=$(echo "$target" | xargs)), add a guard if [[ -z "$target" ]]; then continue; fi so empty or whitespace-only CHAT_ID entries are ignored before the existing ":" check and curl calls (references: TARGETS, target, the for-loop and the existing if [[ "$target" == *":"* ]] branch).
32-40: Redundant local tag creation after github-tag-action.The
mathieudutour/github-tag-actionalready creates and pushes the tag to the remote repository. Creating a local tag afterward (line 40) is redundant and could potentially cause issues if the action's tag and local tag diverge.♻️ Suggested simplification
- name: Bump version and push tag id: tag_version uses: mathieudutour/github-tag-action@v6.1 with: github_token: ${{ secrets.GITHUB_TOKEN }} default_bump: patch - - name: Create local tag - run: git tag ${{ steps.tag_version.outputs.new_tag }} -If GoReleaser needs the local tag, you can fetch it instead:
- name: Fetch new tag run: git fetch --tags🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 32 - 40, The workflow redundantly creates a local tag after mathieudutour/github-tag-action@v6.1 (step id tag_version) which already creates and pushes the tag; remove the step that runs git tag ${{ steps.tag_version.outputs.new_tag }} and, if a local tag is required later (e.g., by GoReleaser), replace it with a simple fetch of tags (git fetch --tags) after the tag action instead of re-creating the tag locally..github/workflows/release-dev.yml (1)
40-49: Same redundant local tag creation as release.yml.The
github-tag-actionalready pushes the tag. The local tag creation on line 49 is redundant.♻️ Suggested simplification
- name: Tag dev build id: tag_version uses: mathieudutour/github-tag-action@v6.1 with: github_token: ${{ secrets.GITHUB_TOKEN }} custom_tag: v${{ steps.calc_version.outputs.major }}.${{ steps.calc_version.outputs.minor }}.${{ steps.calc_version.outputs.patch }}-${{ steps.vars.outputs.sha_short }} tag_prefix: "" - - name: Create local tag - run: git tag ${{ steps.tag_version.outputs.new_tag }} + - name: Fetch new tag + run: git fetch --tags🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-dev.yml around lines 40 - 49, Remove the redundant "Create local tag" step that runs git tag after the github-tag-action has already created and pushed the tag; locate the workflow step with name "Create local tag" (following the step with name "Tag dev build" and id "tag_version") and delete that run: git tag ${{ steps.tag_version.outputs.new_tag }} entry (or comment it out) so the workflow relies solely on the mathieudutour/github-tag-action output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 12-15: The .gitignore currently lists files that don't exist in
this PR—remove the entries analytics_export.py, analytics_requirements.txt,
ANALYTICS_API.md, and ANALYTICS_QUICKSTART.md from .gitignore so it only ignores
real or generated artifacts; if any are actually generated by the build, instead
add a comment explaining their generation and ensure the build produces them,
otherwise delete those four lines referencing those filenames.
In `@internal/api/analytics.go`:
- Around line 223-226: The analytics handlers currently swallow backend errors
and return 200 OK, making failures indistinguishable from empty results; update
the error handling around s.Bot.Client.PaymentsWithOptions, and similar calls
(also referenced at the other locations), to propagate or surface errors to the
HTTP response (return 5xx) instead of continuing, and ensure
GetUserTransactionHistory does not ignore the internal Find error but returns it
to the caller so the handler can respond with an error status; locate and modify
the blocks that call PaymentsWithOptions and the internal Find (and any database
client Find methods) to log the error and return an appropriate error response
(or propagate the error) rather than continuing as if there were no data.
- Around line 500-535: parseDate currently returns midnight for date-only inputs
which makes inclusive end-date queries drop the entire day; change parseDate to
accept a boolean flag (e.g., parseDate(dateStr string, isEnd bool)) or add a
dedicated helper (e.g., parseEndDate) and when the input matches the date-only
layout "2006-01-02" and isEnd==true set the time to the end of that day (parsed
+ 24h - 1ns or
parsed.Add(23*time.Hour+59*time.Minute+59*time.Second+999999999*time.Nanosecond));
keep the same range checks against minValidTimestamp/maxValidTimestamp and
return the adjusted time so callers using paymentTime.After(endDate) or SQL <=
endDate behave correctly.
- Around line 223-252: The pagination logic incorrectly only enlarges the
per-user fetch but never skips the first offset-matching rows; fix by tracking a
running skipped counter before iterating users and, after applying date/type
filters to each payment, if skipped < offset increment skipped and continue (do
not append); otherwise append to response.ExternalPayments and stop when
len(response.ExternalPayments) >= limit. Adjust per-user fetch size if desired
by subtracting already-searched payments, but the core fix is to use a global
skipped counter when processing payments returned by
s.Bot.Client.PaymentsWithOptions and only append after skipping offset matching
items; refer to PaymentsWithOptions, response.ExternalPayments,
paymentTime/startDate/endDate and paymentType/paymentTypeFilter to locate where
to apply this logic.
In `@internal/api/middleware.go`:
- Around line 248-259: The timestamp validation only rejects old requests;
update the check around now, timestamp and tolerance (using
internal.Configuration.API.Analytics.TimestampTolerance) to also reject
timestamps too far in the future by adding a condition (e.g. if timestamp-now >
tolerance) or by validating abs(now - timestamp) > tolerance, logging a warning
via log.Warnf and returning http.Error as done for expired requests; apply the
same change to WalletHMACMiddleware to mirror the behavior.
In `@internal/api/send.go`:
- Around line 169-171: The memo LIKE query can misinterpret % or _ in req.Memo
as wildcards; before building memoSearch, escape backslashes, percent and
underscore characters in req.Memo (e.g., replace "\" with "\\", "%" with "\%",
and "_" with "\_"), then construct memoSearch from the escaped memo and use that
in the Where call on s.Bot.DB.Transactions.Model(&telegram.Transaction{}) so the
duplicate check matches text literally; also include an ESCAPE '\\' clause in
the LIKE condition if your SQLite driver needs it.
In `@internal/telegram/standing_order_scheduler.go`:
- Around line 95-115: The scheduler currently checks due orders in memory (using
effectiveDayForMonth and shouldExecuteToday) then loads the user and calls
executeOrder, which allows a race where two processes both execute the same
standing order; fix this by atomically claiming the order in the database before
calling executeOrder: perform a conditional UPDATE (or set a claimed flag) on
the standing order row identified by order.ID that only succeeds if
LastExecutedAt (or a dedicated claimed/locked column) still indicates the order
is due (e.g., last_executed_at is NULL or older than the scheduled time), then
check that RowsAffected == 1 before proceeding to load the user and call
executeOrder; apply the same atomic-claim pattern to the other occurrence
mentioned (the block around executeOrder at the other location).
- Around line 163-165: executeOrder() currently calls TransferToPot() directly
which can race with interactive transfers because TransferToPot() performs a
pre-transaction balance check without taking the per-user mutex used by the
Telegram handlers; to fix, ensure the same per-user mutex used around
interactive transfers in pots.go is acquired for the user before calling
TransferToPot() (or move the balance-check+transfer logic into a single DB
transaction inside TransferToPot()), so that executeOrder() serializes with
manual transfers and prevents double-spend races.
- Around line 115-130: The code currently restores the previous LastExecutedAt
on failure so shouldExecuteToday() remains true and the order is retried hourly;
instead, after incrementing ConsecutiveFailures in the error branch of
s.executeOrder, advance the order's LastExecutedAt to the next scheduled period
(e.g. order.LastExecutedAt = order.LastExecutedAt.AddDate(0,1,0) for monthly
semantics, or set to time.Now() if you prefer day-based suppression) before
saving and notifying (references: executeOrder, shouldExecuteToday,
ConsecutiveFailures, LastExecutedAt, notifyFailure, notifyDeactivated,
maxConsecutiveFailures). Ensure the adjusted LastExecutedAt is persisted via
s.bot.DB.Users.Save(&order) so the order won't be retried repeatedly within the
same month/day.
In `@internal/telegram/standing_orders.go`:
- Around line 39-42: The Count call's error is not checked so failures leave
orderCount==0 and bypass the limit; update the logic around
bot.DB.Users.Model(&lnbits.StandingOrder{}).Where("user_id = ? AND active =
true").Count(&orderCount) to capture and handle the returned error, e.g., check
the error and return it (or log and return a wrapped error) before comparing
orderCount to MaxStandingOrdersPerUser, ensuring Create() is only called when
Count succeeded and confirmed below the limit.
- Around line 34-35: The lookup error handling in CreateStandingOrder() and
GetStandingOrderByID() flattens all DB errors into a “not found” message (e.g.
the bot.GetPot(user, potName) call and the standing-order lookup), which hides
real failures; change these sites to only convert the error into a user-facing
"not found" message when the underlying error is the specific NotFound
sentinel/err type returned by the store, and otherwise return/propagate the
original error (or wrap it with context) so timeouts/connection errors are
preserved; reference the bot.GetPot(user, potName) call and the
CreateStandingOrder/ GetStandingOrderByID functions to locate and adjust the
conditional error branches accordingly.
---
Nitpick comments:
In @.github/workflows/release-dev.yml:
- Around line 40-49: Remove the redundant "Create local tag" step that runs git
tag after the github-tag-action has already created and pushed the tag; locate
the workflow step with name "Create local tag" (following the step with name
"Tag dev build" and id "tag_version") and delete that run: git tag ${{
steps.tag_version.outputs.new_tag }} entry (or comment it out) so the workflow
relies solely on the mathieudutour/github-tag-action output.
In @.github/workflows/release.yml:
- Around line 63-80: The for-loop over TARGETS may process empty entries and
should skip them like in release-dev.yml; inside the loop in the block that
starts with for target in "${TARGETS[@]}"; do (after trimming with target=$(echo
"$target" | xargs)), add a guard if [[ -z "$target" ]]; then continue; fi so
empty or whitespace-only CHAT_ID entries are ignored before the existing ":"
check and curl calls (references: TARGETS, target, the for-loop and the existing
if [[ "$target" == *":"* ]] branch).
- Around line 32-40: The workflow redundantly creates a local tag after
mathieudutour/github-tag-action@v6.1 (step id tag_version) which already creates
and pushes the tag; remove the step that runs git tag ${{
steps.tag_version.outputs.new_tag }} and, if a local tag is required later
(e.g., by GoReleaser), replace it with a simple fetch of tags (git fetch --tags)
after the tag action instead of re-creating the tag locally.
In `@internal/lnbits/lnbits.go`:
- Around line 10-22: In parseLNbitsError, check and handle the error returned by
resp.ToJSON(&reqErr) instead of ignoring it: capture the returned err, and if
non-nil set reqErr.RawBody = rawBody (and/or set reqErr.Message or an
UnmarshalError field with fmt.Sprintf("failed to unmarshal LNbits response: %v",
err)) before populating StatusCode; add fmt import if needed. This makes it
explicit when JSON decoding fails and preserves the raw response for debugging
while keeping the existing fallback behavior using the resp variable and reqErr
struct.
In `@internal/lnbits/types.go`:
- Around line 212-223: Fix the formatting and add validation for the
StandingOrder struct: align the ConsecutiveFailures field indentation with other
fields in the StandingOrder type and add a GORM check constraint (or model
validation) to enforce DayOfMonth is within 1..31 (e.g., a gorm tag like check:
"day_of_month >= 1 AND day_of_month <= 31" on DayOfMonth or a
Validate/BeforeCreate method on StandingOrder that returns an error for
out-of-range values). Reference the StandingOrder struct and the DayOfMonth and
ConsecutiveFailures fields when making the changes.
In `@internal/telegram/error_logger.go`:
- Around line 55-59: Extract the hardcoded sentinel JSON string
{"message":"","Err":{},"code":0} into a package-level constant (e.g.,
sentinelEmptyError) and replace the inline literal used in the error filtering
code that assigns errorMsg := err.Error() and checks if errorMsg == "" ||
errorMsg == ...; update any other places (like main.go) that perform the same
check to reference this constant to ensure consistency and maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a439f44d-8c49-4dbb-9872-09888a9e85a9
📒 Files selected for processing (21)
.github/workflows/release-dev.yml.github/workflows/release.yml.gitignore.goreleaser.yamlconfig.yaml.exampleinternal/api/analytics.gointernal/api/lightning.gointernal/api/middleware.gointernal/api/send.gointernal/config.gointernal/lnbits/lnbits.gointernal/lnbits/types.gointernal/telegram/bot.gointernal/telegram/database.gointernal/telegram/error_logger.gointernal/telegram/handler.gointernal/telegram/standing_order_scheduler.gointernal/telegram/standing_orders.gointernal/utils/cache.gomain.gotranslations/en.toml
Comment on lines
+12
to
+15
| analytics_export.py | ||
| analytics_requirements.txt | ||
| ANALYTICS_API.md | ||
| ANALYTICS_QUICKSTART.md |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for analytics documentation in the repository
# Search for analytics-related markdown files that are committed
fd -e md -t f . | rg -i 'analytics'
# Search for references to analytics in README or docs
rg -i 'analytics' README.md docs/ --type md 2>/dev/null || echo "No analytics references found in README or docs/"Repository: CeyLabs/BitcoinDeepaBot
Length of output: 115
🏁 Script executed:
# Check if the analytics files exist in the working directory
ls -la | grep -i analytics
# Look at the git status to see what files are modified/added
git status
# Check the full .gitignore context
head -20 .gitignore
# Search for any analytics-related files in the entire repo
find . -type f -name "*analytics*" 2>/dev/null | head -20Repository: CeyLabs/BitcoinDeepaBot
Length of output: 406
🏁 Script executed:
# Check recent commits
git log --oneline -20
# Check if there are other feature documentation patterns
find . -name "*.md" -type f | head -20
# Look at the structure of internal/api to understand the analytics implementation
ls -la internal/api/ | head -20
# Check git log for this specific .gitignore file
git log --oneline .gitignore | head -10Repository: CeyLabs/BitcoinDeepaBot
Length of output: 1138
🏁 Script executed:
# Check the size and content summary of analytics.go
wc -l internal/api/analytics.go
# Look for documentation comments in analytics.go
head -100 internal/api/analytics.go
# Check if README mentions analytics API
grep -n -A 5 -B 5 -i "analytics" README.md
# Check the commit to see what changed in .gitignore
git show 0562b7f -- .gitignore | head -40Repository: CeyLabs/BitcoinDeepaBot
Length of output: 4926
Remove .gitignore entries for non-existent files.
The analytics feature is being added via analytics.go (620 lines with inline documentation of the API), but the .md documentation files and Python scripts referenced in .gitignore (lines 12-15) are not part of this PR. Adding .gitignore entries for non-existent files is unnecessary and creates maintenance clutter. Remove these entries unless they represent auto-generated or build artifacts that are actually produced by the build process.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 12 - 15, The .gitignore currently lists files that
don't exist in this PR—remove the entries analytics_export.py,
analytics_requirements.txt, ANALYTICS_API.md, and ANALYTICS_QUICKSTART.md from
.gitignore so it only ignores real or generated artifacts; if any are actually
generated by the build, instead add a comment explaining their generation and
ensure the build produces them, otherwise delete those four lines referencing
those filenames.
Comment on lines
+223
to
+226
| payments, err := s.Bot.Client.PaymentsWithOptions(*user.Wallet, limit+offset, 0) | ||
| if err != nil { | ||
| log.Errorf("[Analytics] Error fetching payments for user %d: %s", user.Telegram.ID, err.Error()) | ||
| continue |
There was a problem hiding this comment.
Failed analytics backends are currently indistinguishable from empty results.
Both handlers log LNbits/DB failures and still return 200 OK (and GetUserTransactionHistory ignores the internal Find error entirely). That produces silent partial datasets, which is dangerous for downstream reporting because clients cannot tell “no data” from “backend failed.”
Also applies to: 307-311, 411-413, 457-461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/analytics.go` around lines 223 - 226, The analytics handlers
currently swallow backend errors and return 200 OK, making failures
indistinguishable from empty results; update the error handling around
s.Bot.Client.PaymentsWithOptions, and similar calls (also referenced at the
other locations), to propagate or surface errors to the HTTP response (return
5xx) instead of continuing, and ensure GetUserTransactionHistory does not ignore
the internal Find error but returns it to the caller so the handler can respond
with an error status; locate and modify the blocks that call PaymentsWithOptions
and the internal Find (and any database client Find methods) to log the error
and return an appropriate error response (or propagate the error) rather than
continuing as if there were no data.
Comment on lines
+223
to
+252
| payments, err := s.Bot.Client.PaymentsWithOptions(*user.Wallet, limit+offset, 0) | ||
| if err != nil { | ||
| log.Errorf("[Analytics] Error fetching payments for user %d: %s", user.Telegram.ID, err.Error()) | ||
| continue | ||
| } | ||
|
|
||
| for _, payment := range payments { | ||
| // Apply date filters | ||
| paymentTime := time.Unix(int64(payment.Time), 0) | ||
| if !startDate.IsZero() && paymentTime.Before(startDate) { | ||
| continue | ||
| } | ||
| if !endDate.IsZero() && paymentTime.After(endDate) { | ||
| continue | ||
| } | ||
|
|
||
| // Determine payment type | ||
| paymentType := "outgoing" | ||
| if payment.Amount > 0 { | ||
| paymentType = "incoming" | ||
| } | ||
|
|
||
| // Apply payment type filter | ||
| if paymentTypeFilter != "" && paymentTypeFilter != "all" && paymentTypeFilter != paymentType { | ||
| continue | ||
| } | ||
|
|
||
| // Check limit | ||
| if len(response.ExternalPayments) >= limit { | ||
| break |
There was a problem hiding this comment.
External LNbits pagination is incorrect right now.
offset is only used to enlarge the fetch (limit+offset), but neither handler ever skips the first offset filtered rows before appending. Because filtering happens after the fetch and the global limit breaks inside the per-user loop, page 2 will repeat page 1, early wallets can fill the page before later users are considered, and older matching records beyond that first window are never surfaced.
Also applies to: 411-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/analytics.go` around lines 223 - 252, The pagination logic
incorrectly only enlarges the per-user fetch but never skips the first
offset-matching rows; fix by tracking a running skipped counter before iterating
users and, after applying date/type filters to each payment, if skipped < offset
increment skipped and continue (do not append); otherwise append to
response.ExternalPayments and stop when len(response.ExternalPayments) >= limit.
Adjust per-user fetch size if desired by subtracting already-searched payments,
but the core fix is to use a global skipped counter when processing payments
returned by s.Bot.Client.PaymentsWithOptions and only append after skipping
offset matching items; refer to PaymentsWithOptions, response.ExternalPayments,
paymentTime/startDate/endDate and paymentType/paymentTypeFilter to locate where
to apply this logic.
Comment on lines
+500
to
+535
| // parseDate parses a date string and validates it falls within reasonable bounds. | ||
| func parseDate(dateStr string) (time.Time, error) { | ||
| var t time.Time | ||
|
|
||
| // Try parsing as Unix timestamp first | ||
| if timestamp, err := strconv.ParseInt(dateStr, 10, 64); err == nil { | ||
| if timestamp < minValidTimestamp || timestamp > maxValidTimestamp { | ||
| return time.Time{}, fmt.Errorf("timestamp out of range: %d", timestamp) | ||
| } | ||
| return time.Unix(timestamp, 0), nil | ||
| } | ||
|
|
||
| // Try parsing as date string | ||
| layouts := []string{ | ||
| "2006-01-02", | ||
| "2006-01-02T15:04:05", | ||
| time.RFC3339, | ||
| } | ||
|
|
||
| for _, layout := range layouts { | ||
| if parsed, err := time.Parse(layout, dateStr); err == nil { | ||
| t = parsed | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if t.IsZero() { | ||
| return time.Time{}, fmt.Errorf("unsupported date format: %s", dateStr) | ||
| } | ||
|
|
||
| // Validate parsed date is within reasonable bounds | ||
| if t.Unix() < minValidTimestamp || t.Unix() > maxValidTimestamp { | ||
| return time.Time{}, fmt.Errorf("date out of range: %s", dateStr) | ||
| } | ||
|
|
||
| return t, nil |
There was a problem hiding this comment.
Date-only end_date excludes almost the entire requested day.
parseDate("2026-03-25") returns midnight, and the callers use that as an inclusive upper bound (paymentTime.After(endDate) / time <= ?). That drops everything after 00:00:00 on March 25. Date-only end bounds need to become end-of-day, or be rewritten as < nextDay.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/analytics.go` around lines 500 - 535, parseDate currently
returns midnight for date-only inputs which makes inclusive end-date queries
drop the entire day; change parseDate to accept a boolean flag (e.g.,
parseDate(dateStr string, isEnd bool)) or add a dedicated helper (e.g.,
parseEndDate) and when the input matches the date-only layout "2006-01-02" and
isEnd==true set the time to the end of that day (parsed + 24h - 1ns or
parsed.Add(23*time.Hour+59*time.Minute+59*time.Second+999999999*time.Nanosecond));
keep the same range checks against minValidTimestamp/maxValidTimestamp and
return the adjusted time so callers using paymentTime.After(endDate) or SQL <=
endDate behave correctly.
Comment on lines
+248
to
+259
| // Check if request is not too old (prevent replay attacks) | ||
| now := time.Now().Unix() | ||
| tolerance := internal.Configuration.API.Analytics.TimestampTolerance | ||
| if tolerance == 0 { | ||
| tolerance = 300 | ||
| } | ||
|
|
||
| if now-timestamp > tolerance { | ||
| log.Warnf("[Analytics] Request timestamp too old (age: %d seconds)", now-timestamp) | ||
| http.Error(w, "Request expired", http.StatusUnauthorized) | ||
| return | ||
| } |
There was a problem hiding this comment.
Consider rejecting future timestamps to prevent pre-computed signature attacks.
The current validation only rejects timestamps that are too old (now - timestamp > tolerance), but accepts timestamps arbitrarily far in the future. An attacker who obtains a valid API key could pre-compute signatures with future timestamps that remain valid indefinitely.
🛡️ Proposed fix to validate both past and future timestamps
// Check if request is not too old (prevent replay attacks)
now := time.Now().Unix()
tolerance := internal.Configuration.API.Analytics.TimestampTolerance
if tolerance == 0 {
tolerance = 300
}
- if now-timestamp > tolerance {
- log.Warnf("[Analytics] Request timestamp too old (age: %d seconds)", now-timestamp)
+ if now-timestamp > tolerance || timestamp-now > tolerance {
+ log.Warnf("[Analytics] Request timestamp out of valid range (diff: %d seconds)", now-timestamp)
http.Error(w, "Request expired", http.StatusUnauthorized)
return
}Note: The same issue exists in WalletHMACMiddleware at line 163.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/middleware.go` around lines 248 - 259, The timestamp validation
only rejects old requests; update the check around now, timestamp and tolerance
(using internal.Configuration.API.Analytics.TimestampTolerance) to also reject
timestamps too far in the future by adding a condition (e.g. if timestamp-now >
tolerance) or by validating abs(now - timestamp) > tolerance, logging a warning
via log.Warnf and returning http.Error as done for expired requests; apply the
same change to WalletHMACMiddleware to mirror the behavior.
Comment on lines
+95
to
+115
| for _, order := range orders { | ||
| if effectiveDayForMonth(order.DayOfMonth, now) != today { | ||
| continue | ||
| } | ||
| if !shouldExecuteToday(order, now) { | ||
| continue | ||
| } | ||
|
|
||
| // Load the user | ||
| var user lnbits.User | ||
| if err := s.bot.DB.Users.Where("id = ?", order.UserID).First(&user).Error; err != nil { | ||
| log.Errorf("[StandingOrderScheduler] User not found for order %s: %v", order.ID, err) | ||
| continue | ||
| } | ||
|
|
||
| // Skip banned or wallet-less users silently | ||
| if user.Banned || user.Wallet == nil { | ||
| continue | ||
| } | ||
|
|
||
| if err := s.executeOrder(&order, &user); err != nil { |
There was a problem hiding this comment.
Claim the standing order atomically before executing it.
The “due” check happens in memory before LastExecutedAt is persisted. If two scheduler instances/processes run at the same time, both can read the row as due and both can execute the transfer. Saving LastExecutedAt inside executeOrder() helps only after the race has already been lost.
Also applies to: 157-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_order_scheduler.go` around lines 95 - 115, The
scheduler currently checks due orders in memory (using effectiveDayForMonth and
shouldExecuteToday) then loads the user and calls executeOrder, which allows a
race where two processes both execute the same standing order; fix this by
atomically claiming the order in the database before calling executeOrder:
perform a conditional UPDATE (or set a claimed flag) on the standing order row
identified by order.ID that only succeeds if LastExecutedAt (or a dedicated
claimed/locked column) still indicates the order is due (e.g., last_executed_at
is NULL or older than the scheduled time), then check that RowsAffected == 1
before proceeding to load the user and call executeOrder; apply the same
atomic-claim pattern to the other occurrence mentioned (the block around
executeOrder at the other location).
Comment on lines
+115
to
+130
| if err := s.executeOrder(&order, &user); err != nil { | ||
| // Increment consecutive failure count and deactivate if threshold is reached | ||
| order.ConsecutiveFailures++ | ||
| if order.ConsecutiveFailures >= maxConsecutiveFailures { | ||
| order.Active = false | ||
| log.Warnf("[StandingOrderScheduler] Deactivating order %s after %d consecutive failures", order.ID, order.ConsecutiveFailures) | ||
| if saveErr := s.bot.DB.Users.Save(&order).Error; saveErr != nil { | ||
| log.Errorf("[StandingOrderScheduler] Failed to deactivate order %s: %v", order.ID, saveErr) | ||
| } | ||
| s.notifyDeactivated(&user, order) | ||
| } else { | ||
| if saveErr := s.bot.DB.Users.Save(&order).Error; saveErr != nil { | ||
| log.Errorf("[StandingOrderScheduler] Failed to update failure count for order %s: %v", order.ID, saveErr) | ||
| } | ||
| s.notifyFailure(&user, order, err) | ||
| } |
There was a problem hiding this comment.
A failed order will be retried every hour, not next month.
On failure you restore the old LastExecutedAt, so shouldExecuteToday() stays true on the next tick. That means ConsecutiveFailures can increment multiple times on the same day, even though the comments and user-facing copy describe these as monthly failures. After three hourly retries, the order is auto-deactivated.
Also applies to: 165-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_order_scheduler.go` around lines 115 - 130, The
code currently restores the previous LastExecutedAt on failure so
shouldExecuteToday() remains true and the order is retried hourly; instead,
after incrementing ConsecutiveFailures in the error branch of s.executeOrder,
advance the order's LastExecutedAt to the next scheduled period (e.g.
order.LastExecutedAt = order.LastExecutedAt.AddDate(0,1,0) for monthly
semantics, or set to time.Now() if you prefer day-based suppression) before
saving and notifying (references: executeOrder, shouldExecuteToday,
ConsecutiveFailures, LastExecutedAt, notifyFailure, notifyDeactivated,
maxConsecutiveFailures). Ensure the adjusted LastExecutedAt is persisted via
s.bot.DB.Users.Save(&order) so the order won't be retried repeatedly within the
same month/day.
Comment on lines
+163
to
+165
| // Execute the transfer | ||
| if err := s.bot.TransferToPot(user, order.PotName, order.Amount); err != nil { | ||
| // Transfer failed — reset LastExecutedAt so the order can be retried next month |
There was a problem hiding this comment.
Scheduler transfers can race with interactive transfers for the same user.
executeOrder() calls TransferToPot() directly, but TransferToPot() checks the available balance before its DB transaction and does not take the per-user mutex used by the Telegram handlers (internal/telegram/pots.go:95-131). A manual transfer can interleave here and both operations can spend against the same pre-update balance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_order_scheduler.go` around lines 163 - 165,
executeOrder() currently calls TransferToPot() directly which can race with
interactive transfers because TransferToPot() performs a pre-transaction balance
check without taking the per-user mutex used by the Telegram handlers; to fix,
ensure the same per-user mutex used around interactive transfers in pots.go is
acquired for the user before calling TransferToPot() (or move the
balance-check+transfer logic into a single DB transaction inside
TransferToPot()), so that executeOrder() serializes with manual transfers and
prevents double-spend races.
Comment on lines
+34
to
+35
| if _, err := bot.GetPot(user, potName); err != nil { | ||
| return nil, fmt.Errorf("pot '%s' not found — create it first with /createpot", potName) |
There was a problem hiding this comment.
Preserve real lookup failures instead of flattening them to “not found”.
CreateStandingOrder() and GetStandingOrderByID() both convert every lookup error into a missing resource. internal/telegram/pots.go:82-93 shows GetPot() also returns raw DB errors, so a timeout/connection problem will be reported as “create it first” or “standing order not found”, which is misleading and much harder to diagnose.
Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_orders.go` around lines 34 - 35, The lookup error
handling in CreateStandingOrder() and GetStandingOrderByID() flattens all DB
errors into a “not found” message (e.g. the bot.GetPot(user, potName) call and
the standing-order lookup), which hides real failures; change these sites to
only convert the error into a user-facing "not found" message when the
underlying error is the specific NotFound sentinel/err type returned by the
store, and otherwise return/propagate the original error (or wrap it with
context) so timeouts/connection errors are preserved; reference the
bot.GetPot(user, potName) call and the CreateStandingOrder/ GetStandingOrderByID
functions to locate and adjust the conditional error branches accordingly.
Comment on lines
+39
to
+42
| var orderCount int64 | ||
| bot.DB.Users.Model(&lnbits.StandingOrder{}).Where("user_id = ? AND active = true", user.ID).Count(&orderCount) | ||
| if orderCount >= MaxStandingOrdersPerUser { | ||
| return nil, fmt.Errorf("maximum number of standing orders reached (%d)", MaxStandingOrdersPerUser) |
There was a problem hiding this comment.
Check the count query before enforcing the per-user limit.
If Count() fails, orderCount stays zero and the limit check is bypassed, so the code falls through to Create() and can accept more than MaxStandingOrdersPerUser.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/telegram/standing_orders.go` around lines 39 - 42, The Count call's
error is not checked so failures leave orderCount==0 and bypass the limit;
update the logic around
bot.DB.Users.Model(&lnbits.StandingOrder{}).Where("user_id = ? AND active =
true").Count(&orderCount) to capture and handle the returned error, e.g., check
the error and return it (or log and return a wrapped error) before comparing
orderCount to MaxStandingOrdersPerUser, ensuring Create() is only called when
Count succeeded and confirmed below the limit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
.github/workflows/release.yml/release-dev.yml— new release workflows.goreleaser.yaml— GoReleaser build configurationinternal/api/send.go— memo dedup check + locking to prevent duplicate transactionsinternal/utils/cache.go— cache utility for memo dedupmain.go— register memo cache cleanupTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Infrastructure