feat: encrypt xpub for auto swap outs#1973
Conversation
swaps/swaps_service.go
Outdated
| boltzWs *boltz.Websocket | ||
| swapListeners map[string]chan boltz.SwapUpdate | ||
| swapListenersLock sync.Mutex | ||
| autoSwapOutDecryptedXpub string |
There was a problem hiding this comment.
Do we need this? I think we can avoid this and hence GetDecryptedAutoSwapXpub by using ValidateXpub itself in the GetAutoSwapConfig and passing xpub param to getNextUnusedAddressFromXpub instead of using svc.autoSwapOutDecryptedXpub (or leaving it as is before this change)
There was a problem hiding this comment.
ValidateXpub only checks format; it does not decrypt config values,
cfg.Get(key, "") returns ciphertext when the value is encrypted (config/config.go:230), so in GetAutoSwapConfig you cannot recover a real XPUB from config without an encryption key.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAPI, frontend, startup, and swaps service updated to support encrypted auto-swap XPUBs: request payloads accept Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant API
participant SwapsService
participant Storage
User->>Frontend: Enter destination + (optional) unlockPassword
Frontend->>Frontend: If external XPUB and not internal -> open unlock dialog
Frontend->>API: POST /api/autoswap { destination, destinationType, unlockPassword }
API->>SwapsService: Ensure service started
API->>SwapsService: Parse/Validate destination (address or xpub)
alt destinationType == "xpub"
API->>SwapsService: EnableAutoSwapOut(unlockPassword)
SwapsService->>SwapsService: Decrypt XPUB using unlockPassword
SwapsService->>SwapsService: Cache decrypted XPUB (in-memory)
SwapsService->>Storage: Persist encrypted destination/config (with encryptionKey)
else destinationType == "address"
API->>SwapsService: EnableAutoSwapOut(unlockPassword)
SwapsService->>Storage: Persist plain address/config
end
loop Auto-swap loop
SwapsService->>SwapsService: If cached decrypted XPUB -> derive next address
SwapsService->>Storage: Use/record swap transactions
end
SwapsService-->>API: Success / error
API-->>Frontend: Response
Frontend-->>User: Confirmation or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
api/api.go (1)
947-979:⚠️ Potential issue | 🟠 MajorValidate first, then persist autoswap config.
This method writes threshold/amount before checking swaps service availability and XPUB unlock-password validity. On validation failure, config is partially written.
Suggested fix
func (api *api) EnableAutoSwapOut(ctx context.Context, enableAutoSwapsRequest *EnableAutoSwapRequest) error { + if api.svc.GetSwapsService() == nil { + return errors.New("SwapsService not started") + } + + encryptionKey := "" + if enableAutoSwapsRequest.Destination != "" { + if err := api.svc.GetSwapsService().ValidateXpub(enableAutoSwapsRequest.Destination); err == nil { + if enableAutoSwapsRequest.UnlockPassword == "" { + return errors.New("unlock password is required when using an xpub as destination") + } + if !api.cfg.CheckUnlockPassword(enableAutoSwapsRequest.UnlockPassword) { + return errors.New("invalid unlock password") + } + encryptionKey = enableAutoSwapsRequest.UnlockPassword + } + } + err := api.cfg.SetUpdate(config.AutoSwapBalanceThresholdKey, strconv.FormatUint(enableAutoSwapsRequest.BalanceThreshold, 10), "") ... err = api.cfg.SetUpdate(config.AutoSwapAmountKey, strconv.FormatUint(enableAutoSwapsRequest.SwapAmount, 10), "") ... - if api.svc.GetSwapsService() == nil { - return errors.New("SwapsService not started") - } - encryptionKey := "" - ... err = api.cfg.SetUpdate(config.AutoSwapDestinationKey, enableAutoSwapsRequest.Destination, encryptionKey) ... - return api.svc.GetSwapsService().EnableAutoSwapOut(enableAutoSwapsRequest.UnlockPassword) + return api.svc.GetSwapsService().EnableAutoSwapOut(encryptionKey) }Also applies to: 984-984
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 947 - 979, The code persists AutoSwapBalanceThreshold and AutoSwapAmount before validating SwapsService availability and destination/XPUB unlock password, which can leave partial config on failure; in EnableAutoSwapOut reorder logic so you first check api.svc.GetSwapsService() != nil, validate the Destination via api.svc.GetSwapsService().ValidateXpub and verify UnlockPassword with api.cfg.CheckUnlockPassword as required, and only after all validations succeed call api.cfg.SetUpdate for config.AutoSwapBalanceThresholdKey, config.AutoSwapAmountKey and config.AutoSwapDestinationKey (using the derived encryptionKey) so writes are atomic in effect; apply the same reorder/fix to the other place where AutoSwapAmount/Balance are persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@swaps/swaps_service.go`:
- Around line 178-181: The current code calls
svc.cfg.Get(config.AutoSwapDestinationKey, encryptionKey) and ignores its error,
which can hide decryption failures; update the logic in swaps_service.go to
capture and handle the returned error from svc.cfg.Get, and if an error is
non-nil (e.g., wrong encryption key or read failure) return or log and fail fast
instead of treating swapDestination as empty; keep the subsequent validation
using svc.ValidateXpub(swapDestination) and only assign
svc.autoSwapOutDecryptedXpub when Get succeeds and ValidateXpub returns nil.
- Around line 52-53: autoSwapOutDecryptedXpub is unsafely accessed from multiple
goroutines and is never cleared on stop; protect all accesses to this sensitive
string by adding a dedicated mutex (or sync.RWMutex) on the swaps service and
use it to guard writes in EnableAutoSwapOut(), reads in the auto-swap loop and
GetDecryptedAutoSwapXpub(), and clearing in StopAutoSwapOut(); additionally,
when clearing the value overwrite the string (e.g., assign an empty string and,
if using a byte buffer, zero its bytes) before releasing it so plaintext is
removed from memory.
- Around line 1571-1577: The ValidateXpub function currently accepts any
parseable extended key; update it to explicitly reject private extended keys by
parsing the key with hdkeychain.NewKeyFromString (as already done) and then
calling the returned key's IsPrivate() method (or equivalent) to detect private
keys; if IsPrivate() is true return a clear error like "private extended key not
allowed" (instead of nil), otherwise return nil. Ensure you reference the
ValidateXpub function and hdkeychain.NewKeyFromString/IsPrivate when making the
change.
---
Outside diff comments:
In `@api/api.go`:
- Around line 947-979: The code persists AutoSwapBalanceThreshold and
AutoSwapAmount before validating SwapsService availability and destination/XPUB
unlock password, which can leave partial config on failure; in EnableAutoSwapOut
reorder logic so you first check api.svc.GetSwapsService() != nil, validate the
Destination via api.svc.GetSwapsService().ValidateXpub and verify UnlockPassword
with api.cfg.CheckUnlockPassword as required, and only after all validations
succeed call api.cfg.SetUpdate for config.AutoSwapBalanceThresholdKey,
config.AutoSwapAmountKey and config.AutoSwapDestinationKey (using the derived
encryptionKey) so writes are atomic in effect; apply the same reorder/fix to the
other place where AutoSwapAmount/Balance are persisted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b526c926-3b53-4b21-8cbe-f1e3ba8c5c3c
📒 Files selected for processing (5)
api/api.goapi/models.gofrontend/src/screens/wallet/swap/AutoSwap.tsxservice/start.goswaps/swaps_service.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/screens/wallet/swap/AutoSwap.tsx (1)
85-110:⚠️ Potential issue | 🟠 MajorFix numeric validation to avoid lexicographic comparison bugs.
Line 86 compares
swapAmountandbalanceThresholdas strings, causing incorrect validation. For example,"9"is greater than"10"lexicographically, so the check fails when intended values should pass. Parse both to numbers once, validate finitude, and reuse in the request payload.Proposed fix
const submitAutoSwap = async (password?: string) => { - if (swapAmount > balanceThreshold) { + const swapAmountValue = Number(swapAmount); + const balanceThresholdValue = Number(balanceThreshold); + + if ( + !Number.isFinite(swapAmountValue) || + !Number.isFinite(balanceThresholdValue) + ) { + toast.info("Please enter valid numeric amounts"); + return; + } + + if (swapAmountValue > balanceThresholdValue) { toast.info( "Balance threshold must be greater than or equal to swap amount" ); return; } @@ body: JSON.stringify({ - swapAmount: parseInt(swapAmount), - balanceThreshold: parseInt(balanceThreshold), + swapAmount: Math.trunc(swapAmountValue), + balanceThreshold: Math.trunc(balanceThresholdValue), destination, unlockPassword: isXpub ? password : undefined, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/screens/wallet/swap/AutoSwap.tsx` around lines 85 - 110, The comparison in submitAutoSwap uses string values swapAmount and balanceThreshold, causing lexicographic bugs; parse both once to numeric values (e.g., const swapAmountNum = Number(swapAmount), const balanceThresholdNum = Number(balanceThreshold)), validate they are finite numbers (use Number.isFinite or isFinite and handle NaN) and show a toast error if validation fails, then compare swapAmountNum <= balanceThresholdNum and reuse these numeric variables in the request body (replace parseInt(...) calls) and any further logic; update references in submitAutoSwap accordingly.
🧹 Nitpick comments (1)
frontend/src/screens/wallet/swap/AutoSwap.tsx (1)
190-196: Set internal/external mode from selected value, not by toggling.Using
setInternalSwap(!isInternalSwap)can desync if this handler is triggered more than expected. Prefer deriving state directly from the emitted radio value.Proposed refactor
<RadioGroup defaultValue="normal" value={isInternalSwap ? "internal" : "external"} - onValueChange={() => { + onValueChange={(value) => { setDestination(""); - setInternalSwap(!isInternalSwap); + setInternalSwap(value === "internal"); }} className="flex gap-4 flex-row" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/screens/wallet/swap/AutoSwap.tsx` around lines 190 - 196, The onValueChange handler for RadioGroup currently toggles internal mode using setInternalSwap(!isInternalSwap), which can desync; update the handler to accept the emitted value (e.g., onValueChange={(val) => { ... }}) and derive state from it: call setInternalSwap(val === "internal") and clear destination with setDestination(""); keep the RadioGroup value mapping ("internal" vs "external") as-is so state is driven directly from the emitted radio value rather than toggling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/screens/wallet/swap/AutoSwap.tsx`:
- Around line 85-110: The comparison in submitAutoSwap uses string values
swapAmount and balanceThreshold, causing lexicographic bugs; parse both once to
numeric values (e.g., const swapAmountNum = Number(swapAmount), const
balanceThresholdNum = Number(balanceThreshold)), validate they are finite
numbers (use Number.isFinite or isFinite and handle NaN) and show a toast error
if validation fails, then compare swapAmountNum <= balanceThresholdNum and reuse
these numeric variables in the request body (replace parseInt(...) calls) and
any further logic; update references in submitAutoSwap accordingly.
---
Nitpick comments:
In `@frontend/src/screens/wallet/swap/AutoSwap.tsx`:
- Around line 190-196: The onValueChange handler for RadioGroup currently
toggles internal mode using setInternalSwap(!isInternalSwap), which can desync;
update the handler to accept the emitted value (e.g., onValueChange={(val) => {
... }}) and derive state from it: call setInternalSwap(val === "internal") and
clear destination with setDestination(""); keep the RadioGroup value mapping
("internal" vs "external") as-is so state is driven directly from the emitted
radio value rather than toggling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b892171-f4dc-40eb-9bcc-b329e6df3cab
📒 Files selected for processing (1)
frontend/src/screens/wallet/swap/AutoSwap.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/api.go (1)
999-1026:⚠️ Potential issue | 🟠 MajorValidate before writing any autoswap config.
This endpoint writes
AutoSwapBalanceThresholdKeyandAutoSwapAmountKeybefore any destination validation, and the destination path only validates xpub inputs. If the destination is an xprv, the unlock password is wrong, or the final destination write fails, the request returns an error after part of the config is already persisted. On the next startup that can leave autoswaps effectively enabled with the previous or empty destination. Move validation ahead of the first write and persist the three keys as one unit if possible.As per coding guidelines:
{api,http}/**/*.go: Validate all user input at system boundaries; trust internal service calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 999 - 1026, The handler currently writes AutoSwapBalanceThresholdKey and AutoSwapAmountKey via api.cfg.SetUpdate before validating the destination and unlock password, which can leave partial state on error; move the destination validation (api.svc.GetSwapsService().ParseSwapDestination(enableAutoSwapsRequest.Destination)) and the unlock password check (api.cfg.CheckUnlockPassword(enableAutoSwapsRequest.UnlockPassword)) to run before any api.cfg.SetUpdate calls, and only after those validations succeed perform the writes; if the config API supports atomic/batched persistence write all three keys (config.AutoSwapBalanceThresholdKey, config.AutoSwapAmountKey, config.AutoSwapDestinationKey) in one operation or implement a transactional/save-all pattern so the three values are persisted together and rollback on error.
♻️ Duplicate comments (2)
swaps/swaps_service.go (2)
179-184:⚠️ Potential issue | 🟠 MajorFail closed when reading the autoswap destination fails.
The result of
cfg.Get(config.AutoSwapDestinationKey, encryptionKey)decides whether the destination is treated as an xpub, but its error is dropped. A read/decryption failure can silently continue autoswap setup with an invalid destination instead of aborting.Suggested fix
- swapDestination, _ := svc.cfg.Get(config.AutoSwapDestinationKey, encryptionKey) + swapDestination, err := svc.cfg.Get(config.AutoSwapDestinationKey, encryptionKey) + if err != nil { + cancelFn() + return fmt.Errorf("failed to read auto swap destination: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swaps/swaps_service.go` around lines 179 - 184, The code drops the error from svc.cfg.Get(config.AutoSwapDestinationKey, encryptionKey) which can allow autoswap to continue with an invalid destination; update the block around swapDestination to check the returned error and, if err != nil, call cancelFn() and return the error (or a wrapped error) before using swapDestination or calling svc.ParseSwapDestination; reference svc.cfg.Get, config.AutoSwapDestinationKey, encryptionKey, swapDestination, svc.ParseSwapDestination, and cancelFn to locate and modify the logic.
40-52:⚠️ Potential issue | 🟠 MajorProtect and clear the decrypted XPUB cache.
autoSwapOutDecryptedXpubis now shared by the autoswap loop,getNextUnusedAddressFromXpub(), andGetDecryptedAutoSwapXpub(), but it is read/written without synchronization andStopAutoSwapOut()never blanks it. That leaves stale plaintext in memory and, becauseGetAutoSwapConfig()now prefers this cache, disabling autoswaps can still return the old xpub until restart.Suggested fix
type swapsService struct { swapListeners map[string]chan boltz.SwapUpdate swapListenersLock sync.Mutex + autoSwapOutXpubMu sync.RWMutex autoSwapOutDecryptedXpub string } func (svc *swapsService) StopAutoSwapOut() { if svc.autoSwapOutCancelFn != nil { logger.Logger.Info("Stopping auto swap out service...") svc.autoSwapOutCancelFn() logger.Logger.Info("Auto swap out service stopped") } + svc.autoSwapOutXpubMu.Lock() + svc.autoSwapOutDecryptedXpub = "" + svc.autoSwapOutXpubMu.Unlock() } func (svc *swapsService) GetDecryptedAutoSwapXpub() string { + svc.autoSwapOutXpubMu.RLock() + defer svc.autoSwapOutXpubMu.RUnlock() return svc.autoSwapOutDecryptedXpub }Use the same mutex around the writes in
EnableAutoSwapOut()and the read ingetNextUnusedAddressFromXpub().Also applies to: 165-171, 173-189, 233-235, 1508-1511, 1590-1591
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swaps/swaps_service.go` around lines 40 - 52, autoSwapOutDecryptedXpub is accessed without synchronization and is never cleared; add a dedicated mutex (e.g., autoSwapOutXpubLock sync.Mutex) to the struct and use it to protect all reads/writes to autoSwapOutDecryptedXpub (wrap assignments in EnableAutoSwapOut(), reads in getNextUnusedAddressFromXpub() and GetDecryptedAutoSwapXpub(), and any other access points noted in the review), and ensure StopAutoSwapOut() zeroes the cache while holding the same mutex so the plaintext is cleared safely from memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/api.go`:
- Around line 999-1026: The handler currently writes AutoSwapBalanceThresholdKey
and AutoSwapAmountKey via api.cfg.SetUpdate before validating the destination
and unlock password, which can leave partial state on error; move the
destination validation
(api.svc.GetSwapsService().ParseSwapDestination(enableAutoSwapsRequest.Destination))
and the unlock password check
(api.cfg.CheckUnlockPassword(enableAutoSwapsRequest.UnlockPassword)) to run
before any api.cfg.SetUpdate calls, and only after those validations succeed
perform the writes; if the config API supports atomic/batched persistence write
all three keys (config.AutoSwapBalanceThresholdKey, config.AutoSwapAmountKey,
config.AutoSwapDestinationKey) in one operation or implement a
transactional/save-all pattern so the three values are persisted together and
rollback on error.
---
Duplicate comments:
In `@swaps/swaps_service.go`:
- Around line 179-184: The code drops the error from
svc.cfg.Get(config.AutoSwapDestinationKey, encryptionKey) which can allow
autoswap to continue with an invalid destination; update the block around
swapDestination to check the returned error and, if err != nil, call cancelFn()
and return the error (or a wrapped error) before using swapDestination or
calling svc.ParseSwapDestination; reference svc.cfg.Get,
config.AutoSwapDestinationKey, encryptionKey, swapDestination,
svc.ParseSwapDestination, and cancelFn to locate and modify the logic.
- Around line 40-52: autoSwapOutDecryptedXpub is accessed without
synchronization and is never cleared; add a dedicated mutex (e.g.,
autoSwapOutXpubLock sync.Mutex) to the struct and use it to protect all
reads/writes to autoSwapOutDecryptedXpub (wrap assignments in
EnableAutoSwapOut(), reads in getNextUnusedAddressFromXpub() and
GetDecryptedAutoSwapXpub(), and any other access points noted in the review),
and ensure StopAutoSwapOut() zeroes the cache while holding the same mutex so
the plaintext is cleared safely from memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7067ae99-7881-4d1c-b051-e61f0c039e9f
📒 Files selected for processing (5)
api/api.goapi/models.gofrontend/src/screens/wallet/swap/AutoSwap.tsxservice/start.goswaps/swaps_service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/models.go
- frontend/src/screens/wallet/swap/AutoSwap.tsx
|
I made it even cleaner as it was accepting random strings in both address/xpub and added mutex for |
2 participants
When using an xpub for auto swap outs, the xpub was saved in config unencrypted, and this is unsafe.
fixes #1928
Summary by CodeRabbit
New Features
Bug Fixes
UI