feat: integrate DCA bot with configuration and deposit notification#64
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 58 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR integrates a DCA bot service that receives deposit notifications via HTTP POST. Configuration defines the bot's wallet username and API endpoint. A new NotifyDeposit function matches incoming usernames against configuration and sends reset-retry-counts requests. Integration points in webhook and transaction handlers trigger notifications on deposit events. ChangesDCA Deposit Notification Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (3)
internal/config.go (1)
25-28: ⚖️ Poor tradeoffAdd URL validation for
ApiUrlininit().Other URL configuration fields (
LNURLServer,WebhookServer, etc.) are parsed and validated in theinit()function (lines 145-154), butDCA.ApiUrlis not. The URL is later concatenated as a string ininternal/dca/dca.goline 18, which could produce malformed URLs if the configuration value is invalid.🔍 Proposed validation addition
Add validation in the
init()function after line 157:Configuration.Bot.LNURLHostUrl = hostname checkLnbitsConfiguration() setAPISendDefaults() setAPIAnalyticsDefaults() + validateDCAConfiguration() } + +// validateDCAConfiguration validates the DCA configuration +func validateDCAConfiguration() { + if Configuration.DCA.ApiUrl == "" { + return // DCA integration is optional + } + _, err := url.Parse(Configuration.DCA.ApiUrl) + if err != nil { + log.Warnf("Invalid DCA api_url '%s': %v. DCA notifications will be disabled.", Configuration.DCA.ApiUrl, err) + Configuration.DCA.ApiUrl = "" // Disable on invalid URL + } +}🤖 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 `@internal/config.go` around lines 25 - 28, The DCAConfiguration.ApiUrl isn't validated in init(), so add URL parsing/validation there: locate the init() function and after the other server URL validations validate DCAConfiguration.ApiUrl by parsing it (use url.Parse or equivalent), ensure a non-empty scheme (http/https) and host, and return/log an error if invalid; reference the DCAConfiguration struct and its ApiUrl field and ensure this prevents malformed concatenation later in the DCA client code (referenced by internal/dca/dca.go using ApiUrl).config.yaml.example (1)
49-49: ⚡ Quick winUse a placeholder URL in the example configuration.
The example configuration file contains a production Railway URL (
https://bitcoindeepa-dca-be-production.up.railway.app). Example/template config files should use placeholder URLs (e.g.,https://your-dca-bot-api.example.com) to prevent accidental API calls to production systems during development and testing.📝 Proposed change
- api_url: "https://bitcoindeepa-dca-be-production.up.railway.app" + api_url: "https://your-dca-bot-api.example.com" # Replace with your DCA bot API URL🤖 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 `@config.yaml.example` at line 49, Replace the hard-coded production Railway URL in the example config by changing the api_url value (the "api_url" key) to a clearly marked placeholder (e.g., "https://your-dca-bot-api.example.com") so the example config cannot accidentally target production; update the api_url entry in config.yaml.example accordingly and ensure any accompanying documentation or comments reflect that this is a placeholder for users to fill in.internal/dca/dca.go (1)
26-27: ⚡ Quick winReuse HTTP client instead of creating one per request.
A new
http.Clientis created on everyNotifyDepositcall. HTTP clients manage connection pooling and should be reused. Creating a new client per request incurs unnecessary overhead and prevents connection reuse.♻️ Proposed refactor
Define a package-level client:
+var httpClient = &http.Client{ + Timeout: 10 * time.Second, +} + // NotifyDeposit checks whether the given recipient username matches the configured // DCA wallet username and, if so, asks the DCA backend to reset its retry counts. func NotifyDeposit(toUsername string) {Then use it:
- client := &http.Client{Timeout: 10 * time.Second} - resp, err := client.Do(req) + resp, err := httpClient.Do(req)🤖 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 `@internal/dca/dca.go` around lines 26 - 27, The NotifyDeposit function currently creates a new http.Client for each call (client := &http.Client{Timeout: 10 * time.Second}), which prevents connection reuse; refactor by adding a package-level reusable client (e.g., var httpClient = &http.Client{Timeout: 10 * time.Second}) and replace the local client creation in NotifyDeposit with that httpClient so all requests reuse the same client and connection pool; ensure you reference the httpClient variable in NotifyDeposit and remove the per-call instantiation.
🤖 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 `@internal/dca/dca.go`:
- Around line 19-24: The request is setting req.Header.Set("Content-Type",
"application/json") while creating the POST with a nil body; remove the
incorrect header or set it only when a non-nil body is provided. Update the code
around the http.NewRequest(...) call in internal/dca/dca.go so that the
Content-Type header is not added when body is nil (e.g., only call
req.Header.Set("Content-Type", "application/json") when the body variable is
non-nil), keeping the rest of the error handling and logging for req creation
intact.
- Line 32: The deferred call defer resp.Body.Close() ignores the error return;
update the function in internal/dca/dca.go to capture and handle the error from
resp.Body.Close() (e.g., assign err := resp.Body.Close() or check the returned
error inside the deferred closure) so any Close() failure is logged or returned;
locate the resp variable and replace the bare defer resp.Body.Close() with a
deferred closure that checks the error and uses the function's logger or error
return to report it.
- Line 18: The code builds the endpoint by concatenating
internal.Configuration.DCA.ApiUrl with "/transaction/reset-retry-counts" which
can produce malformed URLs if ApiUrl has a trailing slash; replace the string
concatenation that assigns url with a proper join using net/url (e.g.,
url.JoinPath) to combine internal.Configuration.DCA.ApiUrl and the endpoint
path, add "net/url" to imports, and update any variable or error handling around
the resulting url variable to use the joined value.
---
Nitpick comments:
In `@config.yaml.example`:
- Line 49: Replace the hard-coded production Railway URL in the example config
by changing the api_url value (the "api_url" key) to a clearly marked
placeholder (e.g., "https://your-dca-bot-api.example.com") so the example config
cannot accidentally target production; update the api_url entry in
config.yaml.example accordingly and ensure any accompanying documentation or
comments reflect that this is a placeholder for users to fill in.
In `@internal/config.go`:
- Around line 25-28: The DCAConfiguration.ApiUrl isn't validated in init(), so
add URL parsing/validation there: locate the init() function and after the other
server URL validations validate DCAConfiguration.ApiUrl by parsing it (use
url.Parse or equivalent), ensure a non-empty scheme (http/https) and host, and
return/log an error if invalid; reference the DCAConfiguration struct and its
ApiUrl field and ensure this prevents malformed concatenation later in the DCA
client code (referenced by internal/dca/dca.go using ApiUrl).
In `@internal/dca/dca.go`:
- Around line 26-27: The NotifyDeposit function currently creates a new
http.Client for each call (client := &http.Client{Timeout: 10 * time.Second}),
which prevents connection reuse; refactor by adding a package-level reusable
client (e.g., var httpClient = &http.Client{Timeout: 10 * time.Second}) and
replace the local client creation in NotifyDeposit with that httpClient so all
requests reuse the same client and connection pool; ensure you reference the
httpClient variable in NotifyDeposit and remove the per-call instantiation.
🪄 Autofix (Beta)
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
Run ID: a4ee353b-1e1d-41b5-aacc-5fa47cea6f5f
📒 Files selected for processing (5)
config.yaml.exampleinternal/config.gointernal/dca/dca.gointernal/lnbits/webhook/webhook.gointernal/telegram/transaction.go
| return | ||
| } | ||
|
|
||
| url := internal.Configuration.DCA.ApiUrl + "/transaction/reset-retry-counts" |
There was a problem hiding this comment.
Use proper URL joining instead of string concatenation.
Directly concatenating the API URL with the endpoint path can produce malformed URLs if Configuration.DCA.ApiUrl has a trailing slash (resulting in https://api.com//transaction/...) or is otherwise malformed.
🔗 Proposed fix using url.JoinPath
- url := internal.Configuration.DCA.ApiUrl + "/transaction/reset-retry-counts"
- req, err := http.NewRequest(http.MethodPost, url, nil)
+ baseURL := internal.Configuration.DCA.ApiUrl
+ fullURL, err := url.JoinPath(baseURL, "/transaction/reset-retry-counts")
+ if err != nil {
+ log.Errorf("[DCA] Error constructing URL: %s", err.Error())
+ return
+ }
+ req, err := http.NewRequest(http.MethodPost, fullURL, nil)Note: Add "net/url" to imports.
🤖 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 `@internal/dca/dca.go` at line 18, The code builds the endpoint by
concatenating internal.Configuration.DCA.ApiUrl with
"/transaction/reset-retry-counts" which can produce malformed URLs if ApiUrl has
a trailing slash; replace the string concatenation that assigns url with a
proper join using net/url (e.g., url.JoinPath) to combine
internal.Configuration.DCA.ApiUrl and the endpoint path, add "net/url" to
imports, and update any variable or error handling around the resulting url
variable to use the joined value.
| req, err := http.NewRequest(http.MethodPost, url, nil) | ||
| if err != nil { | ||
| log.Errorf("[DCA] Error creating reset-retry-counts request: %s", err.Error()) | ||
| return | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") |
There was a problem hiding this comment.
Remove Content-Type header when sending no body.
The request sets Content-Type: application/json (line 24) but the request body is nil (line 19). This is inconsistent. If the DCA API endpoint does not require a request body, the Content-Type header should be omitted.
🧹 Proposed fix
req, err := http.NewRequest(http.MethodPost, url, nil)
if err != nil {
log.Errorf("[DCA] Error creating reset-retry-counts request: %s", err.Error())
return
}
- req.Header.Set("Content-Type", "application/json")📝 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.
| req, err := http.NewRequest(http.MethodPost, url, nil) | |
| if err != nil { | |
| log.Errorf("[DCA] Error creating reset-retry-counts request: %s", err.Error()) | |
| return | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| req, err := http.NewRequest(http.MethodPost, url, nil) | |
| if err != nil { | |
| log.Errorf("[DCA] Error creating reset-retry-counts request: %s", err.Error()) | |
| 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 `@internal/dca/dca.go` around lines 19 - 24, The request is setting
req.Header.Set("Content-Type", "application/json") while creating the POST with
a nil body; remove the incorrect header or set it only when a non-nil body is
provided. Update the code around the http.NewRequest(...) call in
internal/dca/dca.go so that the Content-Type header is not added when body is
nil (e.g., only call req.Header.Set("Content-Type", "application/json") when the
body variable is non-nil), keeping the rest of the error handling and logging
for req creation intact.
| log.Errorf("[DCA] Error calling reset-retry-counts: %s", err.Error()) | ||
| return | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Check error return from resp.Body.Close().
The deferred resp.Body.Close() call does not check for errors. While rare, Close() can fail and may indicate issues with connection cleanup or incomplete response reads.
🔧 Proposed fix
- defer resp.Body.Close()
+ defer func() {
+ if err := resp.Body.Close(); err != nil {
+ log.Errorf("[DCA] Error closing response body: %s", err.Error())
+ }
+ }()Based on learnings from static analysis tool errcheck.
📝 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.
| defer resp.Body.Close() | |
| defer func() { | |
| if err := resp.Body.Close(); err != nil { | |
| log.Errorf("[DCA] Error closing response body: %s", err.Error()) | |
| } | |
| }() |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 32-32: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 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 `@internal/dca/dca.go` at line 32, The deferred call defer resp.Body.Close()
ignores the error return; update the function in internal/dca/dca.go to capture
and handle the error from resp.Body.Close() (e.g., assign err :=
resp.Body.Close() or check the returned error inside the deferred closure) so
any Close() failure is logged or returned; locate the resp variable and replace
the bare defer resp.Body.Close() with a deferred closure that checks the error
and uses the function's logger or error return to report it.
Source: Linters/SAST tools
xbuddhi
left a comment
xbuddhi
left a comment
There was a problem hiding this comment.
Hi @helloscoopa — nice, focused change. The wiring is correct: SendTransaction is only ever called from Transaction.Send(), and every internal send/tip/faucet/shop/batch path routes through Send(), so the single hook there covers all internal transfers. No import cycle, and the goroutine keeps deposit handling non-blocking.
There's one correctness issue that will silently break the feature, plus a couple of robustness items — could you take a look before we merge?
1. Username @-prefix mismatch (blocking). Both call sites pass GetUserStr(), which returns @username (see internal/telegram/users.go:30). So wallet_username only matches if the operator includes the leading @. The config comment and empty example give no hint, so a value like BitcoinDeepaDCA silently never fires with no error to debug. Please normalize both sides (e.g. strings.TrimPrefix(x, "@")) so it works with or without the @.
2. No guard on empty api_url — if wallet_username is set but api_url is empty, every matching deposit POSTs to a relative path and fails. A cheap early-return makes misconfig obvious.
3. Unauthenticated call — reset-retry-counts is POSTed with no auth token. If the backend endpoint is meant to be protected, add a shared secret; otherwise confirm it's intentionally open.
Minor (non-blocking): no trailing-slash handling on api_url, and failures are best-effort (logged, no retry) — fine for this design, just noting it. A small unit test on the match/no-op logic with httptest.Server would also catch #1.
xbuddhi
left a comment
xbuddhi
left a comment
There was a problem hiding this comment.
Thanks @helloscoopa — all three points are resolved, verified against f3abfd6:
- ✅ Username
@mismatch —strings.TrimPrefixon both the configured wallet and the incoming username, so it now matches with or without the leading@. This also makes the config comment a non-issue. - ✅ Empty
api_urlguard — early-return with a clear log line, plusTrimSuffixon the trailing slash (also resolves CodeRabbit's URL-joining note). - ✅ Auth — acknowledged as an intentionally-open, non-sensitive endpoint. Fine.
LGTM 👍
2 participants
Add DCA backend integration for retry-count reset on deposits
Summary
sats are deposited into the configured DCA wallet account.
Why
The DCA bot needs to know when its wallet receives new funds so it can reset its retry counters and resume scheduled purchases.
Config
dca:
wallet_username: "" # Telegram username of the DCA wallet account
api_url: "https://bitcoindeepa-dca-be-production.up.railway.app"
Test plan
reset-retry-counts call (check logs for [DCA] reset-retry-counts called for ...).