feat: bdk preparation#567
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a pluggable wallet-backend abstraction, refactors Liquid wallet construction and descriptor derivation, moves sync orchestration to a global per-wallet sync loop, updates RPC/server wiring to register backends, and adjusts tests/helpers and electrum test package imports/signatures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RPC as RPC Server
participant WB as Wallet Backend (registry)
participant OnC as Onchain Manager
participant W as Wallet
Client->>RPC: LoginWallet(credentials)
alt backend exists for currency
RPC->>WB: lookup backend by currency
RPC->>WB: NewWallet(credentials)
WB-->>RPC: Wallet
RPC->>OnC: AddWallet(W)
OnC->>OnC: startSyncLoop(W) %% background per-wallet loop
else legacy/no backend
RPC->>RPC: fallback to legacy wallet.Login(credentials)
end
RPC-->>Client: Login result
sequenceDiagram
autonumber
participant OnC as Onchain Manager
participant W as Wallet
participant Timer as Jittered Interval
OnC->>OnC: Init (create syncCtx/cancel, syncWait)
loop while syncCtx active
Timer-->>OnC: sleep(WalletSyncInterval ± jitter)
OnC->>W: Sync()
alt Sync error
OnC->>OnC: log error and continue
end
alt wallet removed
OnC->>OnC: exit loop and mark done
end
end
OnC->>OnC: Disconnect() -> cancel syncCtx, wait for syncWait
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/rpcserver/router.go (1)
239-244: Bug: wrong slice used when appending refundable swap IDs.You append to refundableSwapIds using pendingReverseSwapIds as the base slice; that drops previous values and mixes lists.
Apply this diff:
- for _, refundableSwap := range refundableSwaps { - refundableSwapIds = append(pendingReverseSwapIds, refundableSwap.Id) - } + for _, refundableSwap := range refundableSwaps { + refundableSwapIds = append(refundableSwapIds, refundableSwap.Id) + }
🧹 Nitpick comments (6)
internal/test/test.go (2)
54-61: Avoid hardcoding WalletId in helpers.Hardcoding Id: 1 can drift from DB-assigned IDs and persister expectations. Prefer returning WalletInfo without Id and using the DB-assigned ID (or thread the actual ID from dbPersister).
169-180: Close electrum client in tests to avoid leaks.Add t.Cleanup if the client exposes Close().
Possible change:
electrumClient, err := electrum.NewClient(onchain.RegtestElectrumConfig.Liquid) require.NoError(t, err) +if c, ok := any(electrumClient).(interface{ Close() error }); ok { + t.Cleanup(func() { _ = c.Close() }) +}internal/onchain/liquid-wallet/wallet_test.go (2)
54-68: Mark helper and improve failure context.Call t.Helper() at the start so assertions point to the caller.
Apply this diff:
-func newWallet(t *testing.T, backend *liquid_wallet.BlockchainBackend, credentials *onchain.WalletCredentials) *liquid_wallet.Wallet { +func newWallet(t *testing.T, backend *liquid_wallet.BlockchainBackend, credentials *onchain.WalletCredentials) *liquid_wallet.Wallet { + t.Helper()
71-92: Avoid magic number for wallet ID in persister read.Use the shared constant to keep tests aligned.
Apply this diff:
- idx, err := cfg.Persister.LoadLastIndex(1) + idx, err := cfg.Persister.LoadLastIndex(test.WalletId)internal/onchain/wallet_test.go (2)
224-238: Test marked TODO and skipped.This test is appropriately skipped with clear TODO marker. Consider tracking implementation in an issue.
Would you like me to open an issue to track the implementation of this test?
240-248: Test marked TODO with basic error validation.Appropriate to skip full testing until valid transaction fixtures are available. The invalid input error handling provides minimal coverage.
Would you like me to open an issue to track the implementation of this test with valid transaction fixtures?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/electrum/electrum_test.go(1 hunks)internal/onchain/liquid-wallet/wallet.go(3 hunks)internal/onchain/liquid-wallet/wallet_test.go(5 hunks)internal/onchain/onchain.go(6 hunks)internal/onchain/wallet.go(1 hunks)internal/onchain/wallet_test.go(1 hunks)internal/rpcserver/router.go(3 hunks)internal/rpcserver/server.go(2 hunks)internal/test/test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
internal/rpcserver/router.go (1)
internal/onchain/wallet.go (2)
WalletBackend(126-129)ValidateWalletCredentials(131-146)
internal/onchain/liquid-wallet/wallet_test.go (4)
internal/onchain/liquid-wallet/wallet.go (6)
NewBackend(71-141)BlockchainBackend(52-57)Wallet(25-33)GenerateMnemonic(570-580)Regtest(145-145)Persister(20-23)internal/test/test.go (3)
LiquidBackendConfig(169-180)WalletCredentials(63-74)WalletInfo(54-61)internal/onchain/onchain.go (2)
TxProvider(51-55)FeeProvider(39-41)internal/onchain/wallet.go (4)
WalletCredentials(72-80)Wallet(49-62)WalletInfo(34-40)ValidateWalletCredentials(131-146)
internal/onchain/wallet.go (5)
internal/test/test.go (2)
WalletCredentials(63-74)WalletBackend(182-191)pkg/boltzrpc/boltzrpc.pb.go (6)
WalletCredentials(4339-4352)WalletCredentials(4367-4367)WalletCredentials(4382-4384)Wallet(5813-5824)Wallet(5839-5839)Wallet(5854-5856)internal/onchain/liquid-wallet/wallet.go (2)
Wallet(25-33)DeriveDefaultDescriptor(173-183)internal/onchain/wallet/wallet.go (1)
Wallet(60-67)internal/database/wallet.go (1)
Wallet(43-47)
internal/test/test.go (5)
internal/onchain/wallet.go (4)
WalletInfo(34-40)WalletCredentials(72-80)Wallet(49-62)WalletBackend(126-129)internal/database/entity.go (1)
DefaultTenantId(12-12)internal/onchain/liquid-wallet/wallet.go (6)
DeriveDefaultDescriptor(173-183)Regtest(145-145)Persister(20-23)Wallet(25-33)Config(40-50)NewBackend(71-141)internal/database/wallet.go (2)
Wallet(43-47)NewWalletPersister(16-18)internal/electrum/electrum.go (1)
NewClient(20-52)
internal/rpcserver/server.go (4)
internal/test/test.go (1)
WalletBackend(182-191)internal/onchain/wallet.go (1)
WalletBackend(126-129)pkg/boltz/currency.go (1)
CurrencyLiquid(12-12)internal/onchain/liquid-wallet/wallet.go (1)
NewBackend(71-141)
internal/electrum/electrum_test.go (2)
internal/electrum/electrum.go (1)
NewClient(20-52)internal/onchain/onchain.go (1)
RegtestElectrumConfig(71-74)
internal/onchain/wallet_test.go (3)
internal/test/test.go (10)
InitLogger(193-195)WalletBackend(182-191)WalletCredentials(63-74)FundWallet(76-118)SendToAddress(245-247)GetCli(201-207)GetNewAddress(241-243)MineBlock(227-231)WalletInfo(54-61)WalletMnemonic(50-50)pkg/boltz/currency.go (2)
CurrencyLiquid(12-12)CurrencyBtc(11-11)internal/onchain/wallet.go (1)
WalletSendArgs(42-47)
internal/onchain/liquid-wallet/wallet.go (3)
internal/onchain/liquid-wallet/lwk/lwk.go (3)
Network(5415-5417)SinglesigWpkh(9563-9563)DescriptorBlindingKeySlip77(9389-9389)internal/test/test.go (1)
WalletCredentials(63-74)internal/onchain/wallet.go (2)
WalletCredentials(72-80)Wallet(49-62)
internal/onchain/onchain.go (3)
internal/onchain/wallet/wallet.go (1)
Wallet(60-67)internal/onchain/wallet.go (1)
Wallet(49-62)internal/logger/logger.go (1)
Errorf(97-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (32)
internal/onchain/liquid-wallet/wallet.go (7)
64-66: LGTM: Backend method wrapper for descriptor derivation.Clean delegation to the free function, enabling backend interface compliance.
71-141: LGTM: Backend initialization with multi-client fallback.The constructor properly validates required dependencies and implements a resilient multi-client strategy with Electrum and Esplora fallbacks.
173-183: LGTM: Descriptor derivation implementation.Correctly derives a singlesig WPKH descriptor with SLIP77 blinding keys for Liquid.
185-222: LGTM: Wallet factory method complies with WalletBackend interface.Proper validation, signer initialization, and readonly mode handling.
224-232: LGTM: Simplified synchronous sync method.Clean orchestration of scan and consolidation. The background sync loop is now managed by the onchain package.
234-268: LGTM: Robust full scan with client fallback.Proper locking, index persistence, and multi-client retry logic.
304-306: LGTM: Disconnect is now a no-op.Lifecycle management is handled by the onchain package's sync loop, which calls Disconnect during cancellation (see onchain.go:122-125).
internal/rpcserver/server.go (2)
64-71: LGTM: Backend registry initialization.Clean addition of the walletBackends map to support pluggable wallet backends.
234-248: LGTM: Liquid backend registration.Properly initializes the Liquid wallet backend with required dependencies and registers it in the backend map.
internal/electrum/electrum_test.go (1)
3-23: LGTM: Test package refactored to use public API.Good practice to use
electrum_testpackage and rely on the publicelectrum.NewClientconstructor.internal/onchain/onchain.go (2)
96-99: LGTM: Context initialization for sync lifecycle.Proper context setup for coordinating wallet sync loops.
437-453: LGTM: Graceful shutdown with timeout.Proper cancellation signal and 10-second timeout for wallet disconnection.
internal/onchain/wallet.go (1)
126-129: LGTM: Clean WalletBackend interface.Minimal interface enabling pluggable wallet backends with descriptor derivation and wallet creation.
internal/rpcserver/router.go (2)
71-71: Initialize and treat walletBackends as immutable at runtime.Make sure this map is initialized (non-nil) during server construction and never mutated after start to avoid data races across goroutines reading it. If mutation is needed, protect with a RW lock.
1432-1434: Credential validation gated on backend presence.Good to validate via backend for currencies with a registered backend. Please confirm BTC/legacy paths remain correct without this validation and that Liquid is always registered so imports don’t slip through unvalidated.
internal/test/test.go (2)
112-114: LGTM: explicit sync inside funding loop.Calling wallet.Sync() during balance polling makes the wait deterministic under fast mining.
156-167: dbPersister wallet ID coupling.You insert a wallet but later tests assume wallet ID 1 for persister reads. Confirm the persister doesn’t enforce FK constraints and that wallet writes/read indices keyed by 1 are valid. Otherwise, capture the created wallet’s ID and use it consistently.
internal/onchain/liquid-wallet/wallet_test.go (1)
97-100: LGTM: explicit FeeProvider for consolidation test.Overriding fee provider to a low, stable value makes consolidation deterministic.
internal/onchain/wallet_test.go (14)
1-3: LGTM!Build tag and external test package declaration follow Go best practices for integration tests.
5-16: LGTM!Import declarations are well-organized and all appear to be utilized in the test suite.
18-18: LGTM!Reasonable interval for polling in integration tests.
20-34: LGTM!Well-structured test helper that properly initializes the test environment, handles errors, and enables parallel test execution.
36-48: LGTM!Thorough test of address generation including distinctness validation.
50-73: LGTM!Comprehensive balance testing with proper consistency checks and appropriate async validation pattern.
75-82: LGTM!Appropriate validation of wallet metadata with helpful explanatory comment.
84-89: LGTM!Straightforward validation that wallet is ready after initialization.
91-126: LGTM!Comprehensive test coverage of both specific-amount and send-all scenarios with proper Liquid confidential address handling and transaction verification.
Also applies to: 128-165
167-196: LGTM!Thorough validation of fee calculation for both specific amounts and send-all scenarios.
198-215: LGTM!Good coverage of pagination parameters with appropriate conditional testing based on available data.
217-222: LGTM!Straightforward validation of disconnect functionality.
250-295: LGTM!Well-structured table-driven test that validates both successful credential import and proper error handling for invalid descriptors. The Liquid-specific ct() wrapper validation is particularly valuable.
297-324: LGTM!Excellent coverage of encryption functionality including idempotence checks and round-trip integrity validation.
| } | ||
| require.NoError(t, err) | ||
| require.True(t, slices.ContainsFunc(tx.Outputs, func(o onchain.TransactionOutput) bool { | ||
| fmt.Println(o.Address, searchAddress) |
There was a problem hiding this comment.
Remove debug print statement.
This fmt.Println appears to be a leftover debug statement and should be removed before merging.
Apply this diff:
- fmt.Println(o.Address, searchAddress)📝 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.
| fmt.Println(o.Address, searchAddress) |
🤖 Prompt for AI Agents
In internal/onchain/wallet_test.go at line 127, there's a leftover debug print
using fmt.Println(o.Address, searchAddress); remove this debug statement from
the test to avoid noisy output and keep tests clean, ensuring imports are
adjusted if fmt is no longer used (remove the fmt import).
| func ValidateWalletCredentials(backend WalletBackend, credentials *WalletCredentials) error { | ||
| if credentials.Encrypted() { | ||
| return errors.New("credentials are encrypted") | ||
| } | ||
| if credentials.CoreDescriptor == "" && credentials.Mnemonic == "" { | ||
| return errors.New("core descriptor or mnemonic is required") | ||
| } | ||
| if credentials.CoreDescriptor == "" { | ||
| descriptor, err := backend.DeriveDefaultDescriptor(credentials.Mnemonic) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| credentials.CoreDescriptor = descriptor | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify Xpub credential handling.
The validation function requires either CoreDescriptor or Mnemonic, but WalletCredentials also supports Xpub (line 76). Ensure that Xpub-based credentials are validated elsewhere or clarify if Xpub is deprecated.
Run this script to check if Xpub credentials are still used:
🏁 Script executed:
#!/bin/bash
# Search for Xpub credential usage in wallet creation/validation flows
rg -n 'Xpub.*=|\.Xpub\b' --type=go -g '!*_test.go' -g '!*.pb.go' -C 3Length of output: 148
🏁 Script executed:
#!/bin/bash
# Locate WalletCredentials definition and Xpub field
rg -n 'type WalletCredentials' -C5 --type=go
rg -n 'Xpub' --type=go -C3Length of output: 15832
Support Xpub as a valid credential
Extend ValidateWalletCredentials in internal/onchain/wallet.go (lines 131–146) to require one of CoreDescriptor, Mnemonic or Xpub—i.e. change
if credentials.CoreDescriptor == "" && credentials.Mnemonic == "" {
return errors.New("core descriptor or mnemonic is required")
}to also allow an Xpub (credentials.Xpub != "") and add an explicit branch for Xpub (skip descriptor derivation, mark as watch-only).
🤖 Prompt for AI Agents
In internal/onchain/wallet.go around lines 131 to 146, the validator currently
requires either CoreDescriptor or Mnemonic but ignores Xpub; change the initial
check to require one of CoreDescriptor, Mnemonic, or Xpub (i.e. if
CoreDescriptor == "" && Mnemonic == "" && Xpub == "" return an error). Then add
an explicit branch: if CoreDescriptor == "" && credentials.Xpub != "" then set
the credentials to watch-only (e.g. credentials.WatchOnly = true or the
appropriate field in the struct) and skip descriptor derivation; otherwise keep
the existing branch that derives CoreDescriptor from Mnemonic when
CoreDescriptor is empty. Keep the Encrypted() check unchanged and return errors
from DeriveDefaultDescriptor as before.
| if !credentials.Legacy { | ||
| if backend, ok := server.walletBackends[credentials.Currency]; ok { | ||
| return backend.NewWallet(credentials) | ||
| } |
There was a problem hiding this comment.
Avoid silent fallback to GDK when a non‑legacy currency backend is missing.
If no backend is registered for a non‑legacy wallet, returning a GDK wallet can be wrong (e.g., Liquid). Prefer failing fast.
Apply this diff:
func (server *routedBoltzServer) loginWallet(credentials *onchain.WalletCredentials) (onchain.Wallet, error) {
if !credentials.Legacy {
if backend, ok := server.walletBackends[credentials.Currency]; ok {
return backend.NewWallet(credentials)
}
+ return nil, status.Errorf(codes.InvalidArgument, "no wallet backend registered for currency %s", credentials.Currency)
}
return wallet.Login(credentials)
}🤖 Prompt for AI Agents
In internal/rpcserver/router.go around lines 2481-2484 the code silently falls
back to creating a GDK wallet when a non-legacy currency backend is not
registered; change the logic so that if credentials.Legacy is false and
server.walletBackends does not contain credentials.Currency you return an
explicit error (e.g., "no wallet backend registered for currency X") instead of
constructing a GDK wallet; preserve existing behavior only for legacy wallets
(i.e., allow GDK creation when credentials.Legacy is true) and ensure the error
is propagated to the caller.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/electrum/electrum_test.go (1)
29-33: Do not callrequire.*inside a goroutine.
require.NoErrorcallsFailNow(fatal) and must run in the test goroutine; here it risks a panic or a test that doesn’t fail deterministically.Use an error channel and assert in the main goroutine:
func TestBlockStream(t *testing.T) { client := client(t) blocks := make(chan *onchain.BlockEpoch) ctx, cancel := context.WithCancel(context.Background()) + errCh := make(chan error, 1) go func() { - err := client.RegisterBlockListener(ctx, blocks) - require.NoError(t, err) + errCh <- client.RegisterBlockListener(ctx, blocks) close(blocks) }() @@ cancel() - _, ok := <-blocks - require.False(t, ok) + // Ensure listener exited without error and channel is closed. + _, ok := <-blocks + require.False(t, ok) + require.NoError(t, <-errCh)
♻️ Duplicate comments (6)
internal/onchain/onchain.go (1)
118-129: Fix jitter: current sleep is ~1ns causing a hot loop.The expression multiplies a float by 1ns (time.Duration default), yielding nanoseconds. Use a real base duration with jitter.
Apply one of the following:
Option A (base + jitter):
- // avoid traffic spikes if a lot of wallets are using the same backend - sleep := time.Duration(float64(1) * (0.75 + rand.Float64()*0.5)) + // avoid traffic spikes if many wallets share a backend + base := 1 * time.Second + jitter := 0.75 + rand.Float64()*0.5 // [0.75, 1.25] + sleep := time.Duration(float64(base) * jitter)Option B (min/max window):
- sleep := time.Duration(float64(1) * (0.75 + rand.Float64()*0.5)) + min := 750 * time.Millisecond + max := 1250 * time.Millisecond + sleep := min + time.Duration(rand.Float64()*float64(max-min))internal/onchain/wallet.go (1)
131-146: Consider supporting Xpub as a valid credential.The validation currently requires either
CoreDescriptororMnemonic, butWalletCredentialsalso includes anXpubfield (line 76). Xpub-based (watch-only) credentials should also be validated.Based on learnings, consider extending the validation:
func ValidateWalletCredentials(backend WalletBackend, credentials *WalletCredentials) error { if credentials.Encrypted() { return errors.New("credentials are encrypted") } - if credentials.CoreDescriptor == "" && credentials.Mnemonic == "" { - return errors.New("core descriptor or mnemonic is required") + if credentials.CoreDescriptor == "" && credentials.Mnemonic == "" && credentials.Xpub == "" { + return errors.New("core descriptor, mnemonic, or xpub is required") } - if credentials.CoreDescriptor == "" { + if credentials.CoreDescriptor == "" && credentials.Mnemonic != "" { descriptor, err := backend.DeriveDefaultDescriptor(credentials.Mnemonic) if err != nil { return err } credentials.CoreDescriptor = descriptor + } else if credentials.Xpub != "" && credentials.CoreDescriptor == "" { + // Mark as watch-only when using Xpub + credentials.Readonly = true } return nil }internal/test/test.go (2)
70-72: Don't ignore descriptor derivation errors.If
DeriveDefaultDescriptorfails, tests may proceed with invalid credentials and fail later with confusing errors.Apply this diff:
if currency == boltz.CurrencyLiquid { - creds.CoreDescriptor, _ = liquid_wallet.DeriveDefaultDescriptor(boltz.Regtest, WalletMnemonic) + desc, err := liquid_wallet.DeriveDefaultDescriptor(boltz.Regtest, WalletMnemonic) + if err != nil { + panic("DeriveDefaultDescriptor failed: " + err.Error()) + } + creds.CoreDescriptor = desc }
182-191: Handle unsupported currencies in WalletBackend().Currently returns nil backend with nil error for non-Liquid currencies, risking nil pointer dereference in callers.
Apply this diff:
func WalletBackend(t *testing.T, currency boltz.Currency) onchain.WalletBackend { var backend onchain.WalletBackend var err error switch currency { case boltz.CurrencyLiquid: backend, err = liquid_wallet.NewBackend(LiquidBackendConfig(t)) + default: + t.Skipf("no test backend for currency %s", currency) + return nil } require.NoError(t, err) return backend }internal/rpcserver/router.go (1)
2481-2487: Avoid silent fallback to GDK when backend is missing.For non-legacy wallets, falling back to GDK when no backend is registered can be incorrect (e.g., Liquid would incorrectly use GDK). Prefer failing fast with a clear error.
Apply this diff:
func (server *routedBoltzServer) loginWallet(credentials *onchain.WalletCredentials) (onchain.Wallet, error) { if !credentials.Legacy { if backend, ok := server.walletBackends[credentials.Currency]; ok { return backend.NewWallet(credentials) } + return nil, status.Errorf(codes.InvalidArgument, "no wallet backend registered for currency %s", credentials.Currency) } return wallet.Login(credentials) }internal/onchain/wallet_test.go (1)
127-127: Remove debug print statement.This
fmt.Printlnis a leftover debug statement that creates noisy test output and should be removed before merging.Apply this diff:
- fmt.Println(o.Address, searchAddress)
🧹 Nitpick comments (6)
internal/electrum/electrum_test.go (3)
9-11: Alias the internal test utils to avoid name confusion withtesting.Optional, but clearer to distinguish
testutils from the stdlibtestingpackage.import ( "context" "testing" "github.com/BoltzExchange/boltz-client/v2/internal/electrum" - "github.com/BoltzExchange/boltz-client/v2/internal/test" + itest "github.com/BoltzExchange/boltz-client/v2/internal/test" "github.com/BoltzExchange/boltz-client/v2/internal/onchain" "github.com/stretchr/testify/require" ) @@ - test.MineBlock() + itest.MineBlock()
16-17: Mark helper with t.Helper() and avoid shadowing for readability.Small ergonomics; preserves behavior.
-func client(t *testing.T) *electrum.Client { - client, err := electrum.NewClient(onchain.RegtestElectrumConfig.Btc) +func client(t *testing.T) *electrum.Client { + t.Helper() + c, err := electrum.NewClient(onchain.RegtestElectrumConfig.Btc) require.NoError(t, err) t.Cleanup(func() { - client.Disconnect() + c.Disconnect() }) - return client + return c }
39-41: Guard against a hang when waiting for channel close; add a timeout.If shutdown is slow, the blocking read can deadlock the test.
-import ( +import ( "context" "testing" + "time" @@ - _, ok := <-blocks - require.False(t, ok) + select { + case _, ok := <-blocks: + require.False(t, ok) + case <-time.After(5 * time.Second): + t.Fatal("timeout waiting for blocks channel to close") + }internal/onchain/liquid-wallet/wallet_test.go (3)
42-46: Tighten mock expectation with Once().Broadcast is called once in this test; constraining the expectation reduces false positives.
- txProvider.EXPECT().BroadcastTransaction(mock.Anything).Return("txid", nil) + txProvider.EXPECT().BroadcastTransaction(mock.Anything).Return("txid", nil).Once()
54-68: Mark helper and keep test failure lines accurate.Declare newWallet as a test helper for clearer failure locations.
func newWallet(t *testing.T, backend *liquid_wallet.BlockchainBackend, credentials *onchain.WalletCredentials) *liquid_wallet.Wallet { + t.Helper() if credentials == nil { mnemonic, err := liquid_wallet.GenerateMnemonic(boltz.Regtest) require.NoError(t, err) credentials = &onchain.WalletCredentials{ WalletInfo: test.WalletInfo(boltz.CurrencyLiquid), Mnemonic: mnemonic, } }
73-76: Avoid hardcoded wallet ID in persister checks.Use the known test wallet id to decouple from magic constants.
- idx, err := cfg.Persister.LoadLastIndex(1) + idx, err := cfg.Persister.LoadLastIndex(test.WalletId) ... - idx, err = cfg.Persister.LoadLastIndex(1) + idx, err = cfg.Persister.LoadLastIndex(test.WalletId)Also applies to: 89-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/autoswap/chain_test.go(1 hunks)internal/electrum/electrum_test.go(1 hunks)internal/onchain/liquid-wallet/wallet.go(3 hunks)internal/onchain/liquid-wallet/wallet_test.go(5 hunks)internal/onchain/onchain.go(6 hunks)internal/onchain/wallet.go(1 hunks)internal/onchain/wallet_test.go(1 hunks)internal/rpcserver/router.go(3 hunks)internal/rpcserver/server.go(2 hunks)internal/test/test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/rpcserver/server.go
🧰 Additional context used
🧬 Code graph analysis (8)
internal/onchain/wallet.go (4)
internal/test/test.go (2)
WalletCredentials(63-74)WalletBackend(182-191)pkg/boltzrpc/boltzrpc.pb.go (6)
WalletCredentials(4339-4352)WalletCredentials(4367-4367)WalletCredentials(4382-4384)Wallet(5813-5824)Wallet(5839-5839)Wallet(5854-5856)internal/onchain/liquid-wallet/wallet.go (2)
Wallet(25-33)DeriveDefaultDescriptor(173-183)internal/database/wallet.go (1)
Wallet(43-47)
internal/onchain/onchain.go (3)
internal/utils/channelforwarder.go (1)
ForwardChannel(18-53)internal/onchain/wallet.go (1)
Wallet(49-62)internal/logger/logger.go (1)
Errorf(97-99)
internal/electrum/electrum_test.go (2)
internal/electrum/electrum.go (1)
NewClient(20-52)internal/onchain/onchain.go (1)
RegtestElectrumConfig(71-74)
internal/onchain/liquid-wallet/wallet.go (3)
internal/onchain/liquid-wallet/lwk/lwk.go (3)
Network(5415-5417)SinglesigWpkh(9563-9563)DescriptorBlindingKeySlip77(9389-9389)internal/test/test.go (1)
WalletCredentials(63-74)internal/onchain/wallet.go (2)
WalletCredentials(72-80)Wallet(49-62)
internal/rpcserver/router.go (1)
internal/onchain/wallet.go (2)
WalletBackend(126-129)ValidateWalletCredentials(131-146)
internal/onchain/wallet_test.go (3)
internal/test/test.go (10)
InitLogger(193-195)WalletBackend(182-191)WalletCredentials(63-74)FundWallet(76-118)SendToAddress(245-247)GetCli(201-207)GetNewAddress(241-243)MineBlock(227-231)WalletInfo(54-61)WalletMnemonic(50-50)pkg/boltz/currency.go (2)
CurrencyLiquid(12-12)CurrencyBtc(11-11)internal/onchain/wallet.go (1)
WalletSendArgs(42-47)
internal/test/test.go (6)
internal/onchain/wallet.go (4)
WalletInfo(34-40)WalletCredentials(72-80)Wallet(49-62)WalletBackend(126-129)internal/database/entity.go (1)
DefaultTenantId(12-12)internal/database/database.go (2)
Id(336-336)Database(240-247)internal/onchain/liquid-wallet/wallet.go (6)
DeriveDefaultDescriptor(173-183)Regtest(145-145)Persister(20-23)Wallet(25-33)Config(40-50)NewBackend(71-141)internal/database/wallet.go (2)
Wallet(43-47)NewWalletPersister(16-18)internal/electrum/electrum.go (1)
NewClient(20-52)
internal/onchain/liquid-wallet/wallet_test.go (5)
internal/onchain/liquid-wallet/wallet.go (6)
NewBackend(71-141)BlockchainBackend(52-57)Wallet(25-33)GenerateMnemonic(570-580)Regtest(145-145)Persister(20-23)internal/test/test.go (3)
LiquidBackendConfig(169-180)WalletCredentials(63-74)WalletInfo(54-61)internal/mocks/onchain/TxProvider_mock.go (1)
NewMockTxProvider(13-23)internal/onchain/onchain.go (2)
TxProvider(51-55)FeeProvider(39-41)internal/onchain/wallet.go (4)
WalletCredentials(72-80)Wallet(49-62)WalletInfo(34-40)ValidateWalletCredentials(131-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (18)
internal/electrum/electrum_test.go (1)
3-3: Good move to external package for black‑box testing.Using
package electrum_testenforces public API usage. LGTM.internal/autoswap/chain_test.go (1)
33-34: LGTM: making Sync() expectation optional stabilizes tests across backends.internal/onchain/onchain.go (1)
8-8: No action needed: Go toolchain already ≥1.22
go.mod specifiesgo 1.24.6and CI workflows use Go 1.24.x, satisfying the requirement for math/rand/v2.internal/test/test.go (4)
54-61: LGTM! Clean test helper for wallet metadata.The
WalletInfohelper provides a clean factory for test wallet metadata and aligns well with the newWalletBackendinterface.
112-114: LGTM! Proper sync in polling loop.Adding
Sync()within the polling loop ensures the wallet state is updated while waiting for funds to arrive, which aligns with the new sync model where sync is explicit rather than background.
156-167: LGTM! Proper test database setup.The
dbPersisterhelper correctly sets up an in-memory database with test wallet credentials for Liquid backend testing.
169-180: LGTM! Backend config properly initialized.The
LiquidBackendConfighelper correctly constructs a complete backend configuration for testing, including Electrum client and persister setup.internal/onchain/wallet.go (1)
126-129: LGTM! Clean backend abstraction.The
WalletBackendinterface provides a clean abstraction for currency-specific wallet backends, enabling the pluggable architecture described in the PR objectives.internal/rpcserver/router.go (2)
71-71: LGTM! Pluggable backend registry.Replacing
liquidBackendwith awalletBackendsmap enables the pluggable wallet backend architecture, allowing multiple currency backends to be registered dynamically.
1432-1436: LGTM! Proper credential validation flow.The import flow correctly validates credentials via
onchain.ValidateWalletCredentialswhen a backend is available, falling back gracefully for legacy wallets.internal/onchain/liquid-wallet/wallet.go (6)
64-66: LGTM! Interface implementation.The
DeriveDefaultDescriptormethod properly implements theWalletBackendinterface by delegating to the existing free function.
71-71: LGTM! Cleaner function naming.Renaming to
NewBackendsimplifies the API while maintaining clear intent within theliquid_walletpackage context.
173-183: LGTM! Proper descriptor derivation.The function correctly derives a default Liquid descriptor using
SinglesigWpkhwith SLIP77 blinding keys, with appropriate error handling.
185-185: LGTM! Interface-conforming signature.The method signature correctly implements the
WalletBackend.NewWalletinterface, returning theonchain.Walletinterface for proper abstraction.
224-232: LGTM! Explicit sync model.The refactored
Sync()method shifts from background sync loop management to explicit orchestration, aligning with the PR objective to move sync control to the caller. This provides better control over sync timing and error handling.
234-268: LGTM! Resilient scan implementation.The
fullScanmethod implements a robust fallback pattern by trying each backend client in sequence, ensuring wallet sync succeeds even if individual clients fail.internal/onchain/wallet_test.go (2)
20-34: LGTM! Well-structured test helper.The
walletTesthelper provides clean test setup with currency-based parameterization, optional funding, and proper parallel execution. This enables comprehensive testing across different wallet backends.
36-324: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of the wallet interface, including happy paths, error cases, and credential handling. The TODO markers for
GetOutputsandApplyTransactionare acceptable for incremental development.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
internal/onchain/onchain.go (2)
106-111: RemoveWallet leaks per-wallet sync goroutines.When RemoveWallet removes a wallet, the goroutine spawned by StartSyncLoop continues to run, leaking resources and attempting to sync a removed wallet. Introduce per-wallet cancellation (e.g., a map of cancel functions) to stop the sync loop when a wallet is removed.
Apply this diff to add per-wallet cancellation:
type Onchain struct { Btc *Currency Liquid *Currency Network *boltz.Network Wallets []Wallet OnWalletChange *utils.ChannelForwarder[[]Wallet] syncWait sync.WaitGroup syncCtx context.Context syncCancel func() + syncMu sync.Mutex + syncCancels map[Id]context.CancelFunc } func (onchain *Onchain) Init() { onchain.OnWalletChange = utils.ForwardChannel(make(chan []Wallet), 0, false) onchain.syncCtx, onchain.syncCancel = context.WithCancel(context.Background()) + onchain.syncCancels = make(map[Id]context.CancelFunc) } func (onchain *Onchain) RemoveWallet(id Id) { + onchain.syncMu.Lock() + if cancel, ok := onchain.syncCancels[id]; ok { + cancel() + delete(onchain.syncCancels, id) + } + onchain.syncMu.Unlock() onchain.Wallets = slices.DeleteFunc(onchain.Wallets, func(current Wallet) bool { return current.GetWalletInfo().Id == id }) onchain.OnWalletChange.Send(onchain.Wallets) } func (onchain *Onchain) StartSyncLoop(wallet Wallet, interval time.Duration) { onchain.syncWait.Add(1) + info := wallet.GetWalletInfo() + ctx, cancel := context.WithCancel(onchain.syncCtx) + onchain.syncMu.Lock() + onchain.syncCancels[info.Id] = cancel + onchain.syncMu.Unlock() go func() { + defer onchain.syncWait.Done() for { sleep := time.Duration(float64(interval) * (0.75 + rand.Float64()*0.5)) select { - case <-onchain.syncCtx.Done(): + case <-ctx.Done(): if err := wallet.Disconnect(); err != nil { - info := wallet.GetWalletInfo() logger.Errorf("Error shutting down wallet %s: %s", info.String(), err.Error()) } - onchain.syncWait.Done() + onchain.syncMu.Lock() + delete(onchain.syncCancels, info.Id) + onchain.syncMu.Unlock() return case <-time.After(sleep): if err := wallet.Sync(); err != nil { - info := wallet.GetWalletInfo() logger.Errorf("Sync for wallet %d failed: %v", info.Id, err) } } } }() }
436-452: Guard against nil dereferences and cancel sync first.Disconnect assumes Btc/Liquid and their Block providers are non-nil, which can cause panics. Additionally, cancel the sync context before disconnecting providers to stop per-wallet loops promptly.
Apply this diff:
func (onchain *Onchain) Disconnect() { onchain.OnWalletChange.Close() - onchain.Btc.Blocks.Disconnect() - onchain.Liquid.Blocks.Disconnect() - onchain.syncCancel() + if onchain.syncCancel != nil { + onchain.syncCancel() + } + if onchain.Btc != nil && onchain.Btc.Blocks != nil { + onchain.Btc.Blocks.Disconnect() + } + if onchain.Liquid != nil && onchain.Liquid.Blocks != nil { + onchain.Liquid.Blocks.Disconnect() + } done := make(chan struct{}) go func() { onchain.syncWait.Wait() close(done) }() select { case <-time.After(10 * time.Second): logger.Warnf("Wallet disconnect timed out") case <-done: } }internal/onchain/wallet.go (1)
131-146: Support Xpub as a valid credential.ValidateWalletCredentials requires either CoreDescriptor or Mnemonic, but WalletCredentials also supports Xpub (line 76). Extend the validation to accept Xpub as a third option, and skip descriptor derivation for Xpub-based credentials (treating them as watch-only).
Apply this diff:
func ValidateWalletCredentials(backend WalletBackend, credentials *WalletCredentials) error { if credentials.Encrypted() { return errors.New("credentials are encrypted") } - if credentials.CoreDescriptor == "" && credentials.Mnemonic == "" { - return errors.New("core descriptor or mnemonic is required") + if credentials.CoreDescriptor == "" && credentials.Mnemonic == "" && credentials.Xpub == "" { + return errors.New("core descriptor, mnemonic, or xpub is required") } - if credentials.CoreDescriptor == "" { + if credentials.CoreDescriptor == "" && credentials.Xpub == "" { descriptor, err := backend.DeriveDefaultDescriptor(credentials.Mnemonic) if err != nil { return err } credentials.CoreDescriptor = descriptor + } else if credentials.Xpub != "" { + // Xpub credentials are watch-only; skip descriptor derivation } return nil }internal/rpcserver/router.go (1)
2481-2488: Avoid silent fallback to GDK for non-legacy wallets.If a non-legacy wallet's backend is not registered, loginWallet falls back to GDK wallet.Login, which may be incorrect for currencies like Liquid. Fail fast with an explicit error instead.
Apply this diff:
func (server *routedBoltzServer) loginWallet(credentials *onchain.WalletCredentials) (onchain.Wallet, error) { if !credentials.Legacy { if backend, ok := server.walletBackends[credentials.Currency]; ok { return backend.NewWallet(credentials) } + return nil, status.Errorf(codes.InvalidArgument, "no wallet backend registered for currency %s", credentials.Currency) } return wallet.Login(credentials) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
internal/autoswap/chain_test.go(1 hunks)internal/config/config.go(2 hunks)internal/electrum/electrum_test.go(1 hunks)internal/onchain/liquid-wallet/wallet.go(3 hunks)internal/onchain/liquid-wallet/wallet_test.go(5 hunks)internal/onchain/onchain.go(6 hunks)internal/onchain/onchain_test.go(4 hunks)internal/onchain/wallet.go(1 hunks)internal/onchain/wallet_test.go(1 hunks)internal/rpcserver/router.go(6 hunks)internal/rpcserver/server.go(2 hunks)internal/test/test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/test/test.go
- internal/onchain/wallet_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
internal/onchain/wallet.go (3)
internal/test/test.go (2)
WalletCredentials(63-74)WalletBackend(181-190)pkg/boltzrpc/boltzrpc.pb.go (6)
WalletCredentials(4339-4352)WalletCredentials(4367-4367)WalletCredentials(4382-4384)Wallet(5813-5824)Wallet(5839-5839)Wallet(5854-5856)internal/onchain/liquid-wallet/wallet.go (2)
Wallet(25-33)DeriveDefaultDescriptor(164-174)
internal/rpcserver/router.go (3)
internal/onchain/onchain.go (2)
Onchain(84-94)Currency(76-82)internal/onchain/wallet.go (3)
WalletBackend(126-129)ValidateWalletCredentials(131-146)Wallet(49-62)internal/onchain/wallet/wallet.go (2)
Login(440-507)Wallet(60-67)
internal/onchain/liquid-wallet/wallet_test.go (3)
internal/onchain/liquid-wallet/wallet.go (6)
NewBackend(69-132)BlockchainBackend(51-56)Wallet(25-33)GenerateMnemonic(561-571)Regtest(136-136)Persister(20-23)internal/test/test.go (3)
LiquidBackendConfig(169-179)WalletCredentials(63-74)WalletInfo(54-61)internal/onchain/wallet.go (4)
WalletCredentials(72-80)Wallet(49-62)WalletInfo(34-40)ValidateWalletCredentials(131-146)
internal/onchain/onchain.go (2)
internal/onchain/liquid-wallet/wallet.go (1)
Wallet(25-33)internal/onchain/wallet.go (1)
Wallet(49-62)
internal/onchain/onchain_test.go (3)
internal/onchain/onchain.go (3)
FeeFloor(190-193)Currency(76-82)Onchain(84-94)internal/mocks/onchain/BlockProvider_mock.go (2)
MockBlockProvider(29-31)NewMockBlockProvider(16-26)internal/mocks/onchain/Wallet_mock.go (1)
NewMockWallet(14-24)
internal/electrum/electrum_test.go (2)
internal/electrum/electrum.go (1)
NewClient(20-52)internal/onchain/onchain.go (1)
RegtestElectrumConfig(71-74)
internal/rpcserver/server.go (4)
internal/onchain/wallet.go (1)
WalletBackend(126-129)internal/test/test.go (1)
WalletBackend(181-190)pkg/boltz/currency.go (1)
CurrencyLiquid(12-12)internal/onchain/liquid-wallet/wallet.go (1)
NewBackend(69-132)
internal/onchain/liquid-wallet/wallet.go (4)
internal/onchain/liquid-wallet/lwk/lwk.go (3)
Network(5415-5417)SinglesigWpkh(9563-9563)DescriptorBlindingKeySlip77(9389-9389)internal/config/config.go (1)
Config(55-100)internal/onchain/wallet.go (2)
WalletCredentials(72-80)Wallet(49-62)internal/test/test.go (1)
WalletCredentials(63-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (25)
internal/electrum/electrum_test.go (3)
3-3: LGTM! Black-box testing approach.Converting to
package electrum_testensures tests validate the public API surface.
9-11: LGTM! Import reorganization is correct.The imports are properly updated to support the black-box test pattern.
16-23: LGTM! Helper function correctly updated.The return type and constructor call are properly qualified with the
electrumpackage name.internal/autoswap/chain_test.go (1)
33-33: LGTM!The addition of
Sync().Return(nil).Maybe()correctly reflects the new explicit synchronization API introduced in the wallet refactoring.internal/onchain/liquid-wallet/wallet_test.go (3)
3-3: LGTM!Moving to an external test package (
liquid_wallet_test) ensures tests exercise the public API surface, which is a best practice.
54-68: Verify the type assertion doesn't panic.The type assertion
wallet.(*liquid_wallet.Wallet)on line 67 will panic ifbackend.NewWalletreturns a different concrete type. While this is currently correct, consider whether this test needs to work with the concrete type or if theonchain.Walletinterface would suffice.If the concrete type is required for test-specific methods, ensure this is documented. Otherwise, prefer working with the interface to reduce coupling.
114-114: LGTM!The explicit
wallet.Sync()call correctly reflects the new synchronization model where sync is orchestrated externally rather than via internal goroutines.internal/onchain/liquid-wallet/wallet.go (6)
63-65: LGTM!The
DeriveDefaultDescriptormethod correctly implements theWalletBackendinterface by delegating to the package-level function with the backend's configured network.
69-69: LGTM!Renaming from
NewBlockchainBackendtoNewBackendimproves API clarity and is more idiomatic.
164-174: LGTM!Returning
(string, error)instead of a descriptor object simplifies the public API surface while maintaining the same functionality.
176-213: LGTM!The
NewWalletmethod correctly implements theWalletBackendinterface. The credential validation (checking for empty CoreDescriptor) and readonly wallet detection (when Mnemonic is empty) are appropriate.
215-223: LGTM!The new
Sync()method provides a clean, explicit synchronization entry point that orchestratesfullScanandautoConsolidate. This is a significant improvement over the previous implicit background goroutine approach, making synchronization lifecycle more predictable and testable.
295-297: LGTM!Simplifying
Disconnect()is correct since background goroutines have been removed. The method can now be a no-op or reserved for future cleanup logic.internal/config/config.go (2)
97-97: LGTM!The
WalletSyncIntervalconfiguration field is correctly defined with appropriate tags and description.
164-165: LGTM!The default value of 60 seconds for
WalletSyncIntervalis reasonable for production use while being configurable for different deployment scenarios.internal/rpcserver/server.go (2)
70-70: LGTM!The
walletBackendsregistry enables pluggable wallet backend support per currency, which is a clean architectural improvement over dedicated fields for each backend type.
245-248: LGTM!The changes correctly initialize the liquid backend using the renamed
NewBackendfunction and store it in thewalletBackendsregistry. This aligns with the new pluggable backend architecture.internal/onchain/onchain_test.go (2)
27-103: LGTM!The formatting and naming adjustments improve test readability without changing semantics.
139-143: LGTM!The helper function cleanly constructs mock block providers with flexible disconnect expectations, improving test setup.
internal/onchain/wallet.go (1)
126-129: LGTM!The WalletBackend interface provides a clean abstraction for pluggable wallet backends, supporting both wallet creation and descriptor derivation.
internal/onchain/onchain.go (1)
8-8: LGTM!The import, sync fields, and Init changes properly support the new per-wallet synchronization lifecycle.
Also applies to: 91-94, 96-99
internal/rpcserver/router.go (4)
70-72: LGTM!The walletBackends registry and walletSyncInterval field support pluggable wallet backends and configurable per-wallet synchronization.
2490-2493: LGTM!The addWallet method correctly adds the wallet to the onchain registry and starts the per-wallet synchronization loop with the configured interval.
1433-1437: LGTM!The importWallet flow correctly validates credentials using the pluggable backend registry when a backend is available.
1456-1456: LGTM!Replacing direct onchain.AddWallet calls with server.addWallet correctly integrates the new per-wallet synchronization lifecycle.
Also applies to: 2053-2053, 2075-2075
michael1011
left a comment
michael1011
left a comment
There was a problem hiding this comment.
Untested concept ACK. But rabbit has some valid points
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/onchain/onchain_test.go (1)
195-195: Remove leftover debug print.
printlnwill spam stdout whenever the test runs. Drop it or replace witht.Logif you need visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/onchain/onchain_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/onchain/onchain_test.go (5)
pkg/boltz/currency.go (2)
CurrencyBtc(11-11)CurrencyLiquid(12-12)internal/onchain/onchain.go (4)
FeeFloor(201-204)Currency(78-84)Onchain(86-97)Id(20-20)internal/mocks/onchain/BlockProvider_mock.go (2)
MockBlockProvider(29-31)NewMockBlockProvider(16-26)internal/mocks/onchain/Wallet_mock.go (1)
NewMockWallet(14-24)internal/test/test.go (1)
WalletInfo(54-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/rpcserver/router.go (1)
239-245: Fix wrong slice when collecting refundable swap IDs.Currently appends to pendingReverseSwapIds, producing incorrect results.
- for _, refundableSwap := range refundableSwaps { - refundableSwapIds = append(pendingReverseSwapIds, refundableSwap.Id) - } + for _, refundableSwap := range refundableSwaps { + refundableSwapIds = append(refundableSwapIds, refundableSwap.Id) + }
♻️ Duplicate comments (5)
internal/onchain/onchain.go (2)
447-456: Shutdown order and nil checks in Disconnect.Cancel sync first, and guard BlockProvider calls. Prevent panics and speed shutdown.
func (onchain *Onchain) Disconnect() { onchain.OnWalletChange.Close() - onchain.Btc.Blocks.Disconnect() - onchain.Liquid.Blocks.Disconnect() - onchain.syncCancel() + if onchain.syncCancel != nil { + onchain.syncCancel() + } + if onchain.Btc != nil && onchain.Btc.Blocks != nil { + onchain.Btc.Blocks.Disconnect() + } + if onchain.Liquid != nil && onchain.Liquid.Blocks != nil { + onchain.Liquid.Blocks.Disconnect() + } done := make(chan struct{}) go func() { - onchain.syncWait.Wait() + onchain.syncWait.Wait() close(done) }()
86-97: Per‑wallet sync goroutines can leak and race; add per‑wallet cancel and defer Done().
- Missing WaitGroup.Done() on early return leaks goroutines.
- No per‑wallet cancel; RemoveWallet doesn’t stop the loop.
- Reads on onchain.Wallets without synchronization risk data races.
Add a cancel map, wire cancellation in RemoveWallet, and defer Done in the goroutine.
type Onchain struct { Btc *Currency Liquid *Currency Network *boltz.Network Wallets []Wallet OnWalletChange *utils.ChannelForwarder[[]Wallet] WalletSyncInterval time.Duration - syncWait sync.WaitGroup - syncCtx context.Context - syncCancel func() + syncWait sync.WaitGroup + syncCtx context.Context + syncCancel func() + syncMu sync.Mutex + syncCancels map[Id]context.CancelFunc } func (onchain *Onchain) Init() { onchain.OnWalletChange = utils.ForwardChannel(make(chan []Wallet), 0, false) onchain.syncCtx, onchain.syncCancel = context.WithCancel(context.Background()) if onchain.WalletSyncInterval == 0 { onchain.WalletSyncInterval = DefaultWalletSyncInterval } + onchain.syncCancels = make(map[Id]context.CancelFunc) } func (onchain *Onchain) RemoveWallet(id Id) { + onchain.syncMu.Lock() + if cancel, ok := onchain.syncCancels[id]; ok { + cancel() + delete(onchain.syncCancels, id) + } + onchain.syncMu.Unlock() onchain.Wallets = slices.DeleteFunc(onchain.Wallets, func(current Wallet) bool { return current.GetWalletInfo().Id == id }) onchain.OnWalletChange.Send(onchain.Wallets) } func (onchain *Onchain) startSyncLoop(wallet Wallet) { onchain.syncWait.Add(1) - go func() { + info := wallet.GetWalletInfo() + ctx, cancel := context.WithCancel(onchain.syncCtx) + onchain.syncMu.Lock() + onchain.syncCancels(info.Id) = cancel + onchain.syncMu.Unlock() + go func() { + defer onchain.syncWait.Done() for { // avoid traffic spikes if a lot of wallets are using the same backend sleep := time.Duration(float64(onchain.WalletSyncInterval) * (0.75 + rand.Float64()*0.5)) select { - case <-onchain.syncCtx.Done(): - if err := wallet.Disconnect(); err != nil { - info := wallet.GetWalletInfo() - logger.Errorf("Error shutting down wallet %s: %s", info.String(), err.Error()) - } - onchain.syncWait.Done() - return + case <-ctx.Done(): + if err := wallet.Disconnect(); err != nil { + logger.Errorf("Error shutting down wallet %s: %s", info.String(), err.Error()) + } + onchain.syncMu.Lock() + delete(onchain.syncCancels, info.Id) + onchain.syncMu.Unlock() + return case <-time.After(sleep): - if slices.Contains(onchain.Wallets, wallet) { - if err := wallet.Sync(); err != nil { - info := wallet.GetWalletInfo() - logger.Errorf("Sync for wallet %d failed: %v", info.Id, err) - } - } else { - return - } + if err := wallet.Sync(); err != nil { + logger.Errorf("Sync for wallet %d failed: %v", info.Id, err) + } } } }() }Also applies to: 99-105, 113-118, 120-146
internal/rpcserver/router.go (1)
2481-2487: Do not silently fall back to GDK for non‑legacy wallets without a backend.Return an error instead of creating the wrong wallet type.
func (server *routedBoltzServer) loginWallet(credentials *onchain.WalletCredentials) (onchain.Wallet, error) { if !credentials.Legacy { if backend, ok := server.walletBackends[credentials.Currency]; ok { return backend.NewWallet(credentials) } + return nil, status.Errorf(codes.InvalidArgument, "no wallet backend registered for currency %s", credentials.Currency) } return wallet.Login(credentials) }internal/test/test.go (1)
70-73: Don’t ignore descriptor derivation errors.Bubble up failure immediately to avoid invalid test creds.
- if currency == boltz.CurrencyLiquid { - creds.CoreDescriptor, _ = liquid_wallet.DeriveDefaultDescriptor(boltz.Regtest, WalletMnemonic) - } + if currency == boltz.CurrencyLiquid { + desc, err := liquid_wallet.DeriveDefaultDescriptor(boltz.Regtest, WalletMnemonic) + if err != nil { + panic("DeriveDefaultDescriptor failed: " + err.Error()) + } + creds.CoreDescriptor = desc + }internal/onchain/wallet_test.go (1)
127-127: Remove debug print statement.This debug statement was flagged in a previous review and should be removed before merging.
Apply this diff:
- fmt.Println(o.Address, searchAddress)
🧹 Nitpick comments (2)
internal/onchain/onchain_test.go (1)
194-196: Remove stray debug print.println in tests is noisy; drop it.
- println("disconnect")internal/test/test.go (1)
99-105: Stop ticker to avoid leaks in tests.Ensure the ticker is stopped on exit.
- ticker := time.NewTicker(1 * time.Second) + ticker := time.NewTicker(1 * time.Second) + defer ticker.Stop()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
internal/autoswap/chain_test.go(1 hunks)internal/electrum/electrum_test.go(1 hunks)internal/onchain/liquid-wallet/wallet.go(3 hunks)internal/onchain/liquid-wallet/wallet_test.go(5 hunks)internal/onchain/onchain.go(5 hunks)internal/onchain/onchain_test.go(4 hunks)internal/onchain/wallet.go(1 hunks)internal/onchain/wallet_test.go(1 hunks)internal/rpcserver/router.go(3 hunks)internal/rpcserver/server.go(2 hunks)internal/test/test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/onchain/wallet.go
- internal/autoswap/chain_test.go
- internal/rpcserver/server.go
🧰 Additional context used
🧬 Code graph analysis (8)
internal/onchain/onchain_test.go (4)
pkg/boltz/currency.go (2)
CurrencyBtc(11-11)CurrencyLiquid(12-12)internal/onchain/onchain.go (4)
FeeFloor(201-204)Currency(78-84)Onchain(86-97)Id(20-20)internal/mocks/onchain/BlockProvider_mock.go (2)
MockBlockProvider(29-31)NewMockBlockProvider(16-26)internal/mocks/onchain/Wallet_mock.go (1)
NewMockWallet(14-24)
internal/onchain/onchain.go (3)
internal/onchain/wallet.go (1)
Wallet(49-62)internal/utils/channelforwarder.go (2)
ChannelForwarder(7-16)ForwardChannel(18-53)internal/logger/logger.go (1)
Errorf(97-99)
internal/onchain/liquid-wallet/wallet_test.go (5)
internal/onchain/liquid-wallet/wallet.go (6)
NewBackend(69-132)BlockchainBackend(51-56)Wallet(25-33)GenerateMnemonic(561-571)Regtest(136-136)Persister(20-23)internal/test/test.go (3)
LiquidBackendConfig(169-179)WalletCredentials(63-74)WalletInfo(54-61)internal/mocks/onchain/TxProvider_mock.go (1)
NewMockTxProvider(13-23)internal/onchain/onchain.go (2)
TxProvider(51-55)FeeProvider(39-41)internal/onchain/wallet.go (4)
WalletCredentials(72-80)Wallet(49-62)WalletInfo(34-40)ValidateWalletCredentials(131-146)
internal/electrum/electrum_test.go (2)
internal/electrum/electrum.go (1)
NewClient(20-52)internal/onchain/onchain.go (1)
RegtestElectrumConfig(71-74)
internal/onchain/liquid-wallet/wallet.go (3)
internal/onchain/liquid-wallet/lwk/lwk.go (3)
Network(5415-5417)SinglesigWpkh(9563-9563)DescriptorBlindingKeySlip77(9389-9389)internal/test/test.go (1)
WalletCredentials(63-74)internal/onchain/wallet.go (2)
WalletCredentials(72-80)Wallet(49-62)
internal/test/test.go (8)
internal/onchain/wallet.go (4)
WalletInfo(34-40)WalletCredentials(72-80)Wallet(49-62)WalletBackend(126-129)internal/onchain/onchain.go (5)
Currency(78-84)Id(20-20)RegtestElectrumConfig(71-74)FeeProvider(39-41)TxProvider(51-55)internal/database/entity.go (1)
DefaultTenantId(12-12)internal/onchain/wallet/wallet.go (3)
Subaccount(52-58)Wallet(60-67)Config(69-76)pkg/boltz/currency.go (1)
CurrencyLiquid(12-12)internal/onchain/liquid-wallet/wallet.go (6)
DeriveDefaultDescriptor(164-174)Regtest(136-136)Persister(20-23)Wallet(25-33)Config(40-49)NewBackend(69-132)internal/database/wallet.go (2)
Wallet(43-47)NewWalletPersister(16-18)internal/electrum/electrum.go (1)
NewClient(20-52)
internal/onchain/wallet_test.go (3)
internal/test/test.go (10)
InitLogger(194-196)WalletBackend(181-192)WalletCredentials(63-74)FundWallet(76-118)SendToAddress(246-248)GetCli(202-208)GetNewAddress(242-244)MineBlock(228-232)WalletInfo(54-61)WalletMnemonic(50-50)pkg/boltz/currency.go (2)
CurrencyLiquid(12-12)CurrencyBtc(11-11)internal/onchain/wallet.go (1)
WalletSendArgs(42-47)
internal/rpcserver/router.go (2)
internal/test/test.go (1)
WalletBackend(181-192)internal/onchain/wallet.go (2)
WalletBackend(126-129)ValidateWalletCredentials(131-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (13)
internal/onchain/liquid-wallet/wallet_test.go (2)
3-67: LGTM! Clean refactoring to new backend abstraction.The test file has been successfully refactored to use the new
WalletBackendinterface:
- Backend construction via
liquid_wallet.NewBackend(test.LiquidBackendConfig(t))- Mnemonic generation via
liquid_wallet.GenerateMnemonic- Credential validation via
onchain.ValidateWalletCredentials- Wallet instantiation via
backend.NewWallet(credentials)The changes maintain test coverage while adopting the pluggable backend architecture.
114-114: Explicit sync call improves test clarity.The addition of
wallet.Sync()calls in tests makes the synchronization behavior explicit, which is consistent with the removal of automatic background sync loops from the wallet lifecycle.internal/onchain/wallet_test.go (5)
20-34: Well-structured test helper for cross-currency testing.The
walletTesthelper provides a clean pattern for testing wallet implementations across multiple currencies with optional funding. The parallel execution viat.Parallel()is appropriate for isolated integration tests.
36-48: Comprehensive wallet interface test coverage.The test suite thoroughly exercises the
Walletinterface with well-structured test cases covering:
- Address generation with uniqueness validation
- Balance tracking across confirmed/unconfirmed states
- Transaction sending (both fixed amount and send-all)
- Fee estimation
- Transaction history with pagination
- Wallet lifecycle (ready state, disconnect)
The use of
require.Eventuallyfor async operations and mining blocks demonstrates understanding of blockchain test patterns.Also applies to: 50-73, 75-82, 84-89, 91-165, 167-196, 198-215, 217-222
224-238: TODO tests are acceptable placeholders.The skipped tests for
GetOutputsandApplyTransactionare appropriately marked and serve as documentation of functionality that needs testing coverage in the future.Also applies to: 240-248
250-295: Thorough credential validation testing.The
TestWallet_ImportCredentialseffectively validates descriptor handling, including:
- Valid Liquid descriptor with CT (Confidential Transactions)
- Invalid descriptor without CT wrapper (correctly expected to error)
This ensures the wallet correctly enforces the requirement for Liquid descriptors to use Confidential Transactions.
297-324: Solid encryption workflow validation.The credential encryption/decryption test covers the full lifecycle including edge cases:
- Successful encryption and decryption
- Prevention of double encryption
- Prevention of decrypting non-encrypted credentials
internal/onchain/liquid-wallet/wallet.go (6)
63-65: Clean delegation to standalone descriptor derivation.The
DeriveDefaultDescriptormethod onBlockchainBackendproperly delegates to the standalone function, maintaining a clean separation between the backend instance and descriptor logic.
69-132: Robust backend initialization with sensible defaults.The
NewBackendconstructor:
- Validates required dependencies (Persister, TxProvider)
- Applies sensible defaults (ConsolidationThreshold)
- Supports both Electrum and Esplora client configurations
- Adds a default Electrum backup when using Esplora
- Provides clear logging for each backend type
The fallback to network-specific defaults for Esplora is a good user experience touch.
164-174: Improved descriptor derivation signature.The updated
DeriveDefaultDescriptorfunction now returns the descriptor string instead of mutating a credentials object. This is a cleaner, more functional design that:
- Reduces side effects
- Makes the function easier to test
- Allows callers to decide how to use the descriptor
176-213: Proper wallet construction with validation.The
NewWalletmethod correctly:
- Validates backend instance is not nil
- Requires a core descriptor
- Handles both mnemonic-based and read-only wallet creation
- Sets the readonly flag when no mnemonic is provided
- Constructs the underlying LWK wallet with proper network and data directory
The error handling is appropriate throughout.
215-223: Explicit sync flow improves clarity.The
Syncmethod now explicitly orchestrates the synchronization process by callingfullScanfollowed byautoConsolidate. This makes the sync behavior transparent and easier to understand compared to hidden background goroutines.
225-259: Robust multi-client scanning with fallback.The
fullScanmethod implements a resilient scanning strategy:
- Attempts each configured client in order
- Falls back to the next client on failure
- Logs failures for debugging
- Properly handles concurrent access with syncLock
- Persists address index state
The error handling ensures at least one client must succeed before returning.
| go func() { | ||
| for { | ||
| // avoid traffic spikes if a lot of wallets are using the same backend | ||
| sleep := time.Duration(float64(onchain.WalletSyncInterval) * (0.75 + rand.Float64()*0.5)) |
There was a problem hiding this comment.
On second thought, since we are sleeping and clients won't start at the exact same time, this is not needed
There was a problem hiding this comment.
The idea is for single clients with more wallets where they do start at the same time on startup
abstract wallet creation to allow for more similar implementation across currency
2 participants
WalletBackendinterfaceSummary by CodeRabbit
New Features
Bug Fixes
Tests