feat: deprecate gdk#671
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRemoves the GDK submodule and all GDK-based on-chain wallet code; replaces it with descriptor-based wallets (BDK for Bitcoin, LWK for Liquid), adds automatic migration of legacy GDK credentials to descriptor credentials, updates build/test/docs, and removes subaccount RPCs/CLI. ChangesGDK removal, descriptor wallets, and migration
Sequence DiagramsequenceDiagram
participant Server as Server Startup
participant DB as Database
participant Migration as Migration Logic
participant Backend as Wallet Backend
participant Persist as DB Persist
Server->>DB: readWalletCredentials(encrypted)
DB-->>Server: WalletCredentials{Legacy: true, Subaccount: X, Mnemonic: M}
Server->>Migration: migrateWalletCredentials()
rect rgba(100, 200, 100, 0.5)
Migration->>Migration: validate subaccount and mnemonic present
end
rect rgba(100, 150, 200, 0.5)
Migration->>Backend: DeriveDefaultDescriptor(network, mnemonic)
Backend-->>Migration: descriptor string
end
rect rgba(200, 150, 100, 0.5)
Migration->>Migration: clear Subaccount
Migration->>Migration: set CoreDescriptor
Migration->>Migration: set Legacy=false
end
Migration->>Persist: Update wallet record
Persist-->>DB: persist migrated credentials
Migration-->>Server: return warnings (if any)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
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 docstrings
🧪 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 |
jackstar12
force-pushed
the
feat/deprecate-gdk
branch
2 times, most recently
from
c97af5e to
a31ec52
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
internal/rpcserver/router.go (2)
1974-1994:⚠️ Potential issue | 🟠 MajorDon’t treat empty mnemonics as proof of partial encryption.
The current check marks
partialEncryptionwheneverdecrypted.Mnemonic == creds.Mnemonic. For encrypted watch-only wallets both values are expected to be empty, so startup will falsely log partial encryption and re-encrypt every wallet on every boot. This detector needs to ignore the empty-mnemonic case or compare the fields that are actually expected to be encrypted for the wallet type.Also applies to: 2001-2021
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/rpcserver/router.go` around lines 1974 - 1994, The partialEncryption flag is being set when decrypted.Mnemonic == creds.Mnemonic which treats empty mnemonics (e.g., encrypted watch-only wallets) as a change; update readWalletCredentials so it ignores empty mnemonics when deciding partialEncryption: only set partialEncryption if decrypted.Mnemonic is non-empty and differs from creds.Mnemonic (or, better, compare the specific fields that should be encrypted for the wallet type), using the same WalletCredentials.Encrypted() / Decrypt() flow to locate the code paths.
256-266:⚠️ Potential issue | 🟠 MajorScope startup warnings to admin or redact wallet identifiers.
server.getWalletMigrationWarnings()returns a global list, and those warning strings include wallet names/currencies frominternal/rpcserver/wallet_migration.go. Exposing them through tenant-scopedGetInfolets one tenant learn about another tenant’s wallets. Either only return these warnings to admin callers, or store/filter them per tenant, or remove wallet-identifying details from the RPC payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/rpcserver/router.go` around lines 256 - 266, The GetInfo handler is including global wallet-identifying warnings via server.getWalletMigrationWarnings() into the tenant-scoped GetInfoResponse, which can leak other tenants' wallet names; update the response construction in the handler that builds GetInfoResponse to only include warnings when the caller is an admin (check tenant role via macaroons.TenantFromContext/serializeTenant) or to filter/redact wallet identifiers by changing server.getWalletMigrationWarnings() to accept a tenant ID and return only tenant-scoped (or redacted) messages; ensure the change is applied where GetInfoResponse is assembled (replace server.getWalletMigrationWarnings() call) and update getWalletMigrationWarnings signature/logic accordingly so non-admin tenants never receive other wallets' names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/onchain/bitcoin-wallet/descriptor.go`:
- Around line 8-10: DeriveDefaultDescriptor currently calls
convertNetwork(network) which will panic if network is nil; add an explicit
guard at the start of DeriveDefaultDescriptor to check for nil network and
return a descriptive error (e.g., "network is required") instead of allowing
convertNetwork to dereference a nil pointer, then proceed to call
convertNetwork(network) and bdk.DeriveDefaultXpub as before; reference the
DeriveDefaultDescriptor, convertNetwork, and bdk.DeriveDefaultXpub symbols when
making the change.
In `@internal/test/test.go`:
- Line 59: The fixture currently swallows errors from
bitcoin_wallet.DeriveDefaultDescriptor (assigned with a blank error) which can
leave WalletCredentials.CoreDescriptor empty; update the fixture so the error is
handled: either change WalletCredentials construction to accept *testing.T and
call require.NoError(t, err) after DeriveDefaultDescriptor, or propagate the
error from the setup function up the call chain (returning (WalletCredentials,
error)) and fail the test immediately when DeriveDefaultDescriptor returns an
error; reference the call to bitcoin_wallet.DeriveDefaultDescriptor and the
WalletCredentials type to locate the change.
In `@pkg/boltzrpc/boltzrpc.proto`:
- Around line 112-119: Remove the literal "Deprecated." prefix from the doc
comments attached to the deprecated RPCs so the description no longer repeats
the deprecation signal; specifically edit the comment blocks for the RPCs
SetSubaccount and GetSubaccounts to begin with the actual behavior text (or an
empty description) rather than "Deprecated. No longer supported.", and apply the
same cleanup to the other deprecated comment blocks mentioned (e.g., the nearby
deprecated comment block referenced in the review) so deprecation is conveyed
only via the proto deprecated option.
---
Outside diff comments:
In `@internal/rpcserver/router.go`:
- Around line 1974-1994: The partialEncryption flag is being set when
decrypted.Mnemonic == creds.Mnemonic which treats empty mnemonics (e.g.,
encrypted watch-only wallets) as a change; update readWalletCredentials so it
ignores empty mnemonics when deciding partialEncryption: only set
partialEncryption if decrypted.Mnemonic is non-empty and differs from
creds.Mnemonic (or, better, compare the specific fields that should be encrypted
for the wallet type), using the same WalletCredentials.Encrypted() / Decrypt()
flow to locate the code paths.
- Around line 256-266: The GetInfo handler is including global
wallet-identifying warnings via server.getWalletMigrationWarnings() into the
tenant-scoped GetInfoResponse, which can leak other tenants' wallet names;
update the response construction in the handler that builds GetInfoResponse to
only include warnings when the caller is an admin (check tenant role via
macaroons.TenantFromContext/serializeTenant) or to filter/redact wallet
identifiers by changing server.getWalletMigrationWarnings() to accept a tenant
ID and return only tenant-scoped (or redacted) messages; ensure the change is
applied where GetInfoResponse is assembled (replace
server.getWalletMigrationWarnings() call) and update getWalletMigrationWarnings
signature/logic accordingly so non-admin tenants never receive other wallets'
names.
🪄 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: 9cb36450-0592-47ed-b989-53b2c537927a
⛔ Files ignored due to path filters (2)
pkg/boltzrpc/boltzrpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (47)
.gitmodulesDockerfileMakefilecmd/boltzcli/commands.gocmd/boltzcli/utils.godocs/gdk.mddocs/grpc.mddocs/index.mddocs/wallets.mdgdkgdk.Dockerfileinternal/database/wallet.gointernal/lightning/static.gointernal/nursery/chain.gointernal/nursery/nursery.gointernal/nursery/reverse.gointernal/onchain/bitcoin-wallet/bdk/static.gointernal/onchain/bitcoin-wallet/descriptor.gointernal/onchain/bitcoin-wallet/wallet.gointernal/onchain/liquid-wallet/descriptor.gointernal/onchain/liquid-wallet/lwk/static.gointernal/onchain/liquid-wallet/wallet.gointernal/onchain/mnemonic.gointernal/onchain/wallet/dummy.cppinternal/onchain/wallet/dynamic.gointernal/onchain/wallet/handler.gointernal/onchain/wallet/include/gdk.hinternal/onchain/wallet/include/gdk_export.hinternal/onchain/wallet/lib/.dockerenvinternal/onchain/wallet/lib/.gitkeepinternal/onchain/wallet/lib/dev/consoleinternal/onchain/wallet/lib/etc/hostnameinternal/onchain/wallet/lib/etc/hostsinternal/onchain/wallet/lib/etc/mtabinternal/onchain/wallet/lib/etc/resolv.confinternal/onchain/wallet/static.gointernal/onchain/wallet/wallet.gointernal/onchain/wallet/wallet_test.gointernal/rpcserver/reverse_swap_test.gointernal/rpcserver/router.gointernal/rpcserver/serializer.gointernal/rpcserver/server.gointernal/rpcserver/wallet_migration.gointernal/rpcserver/wallet_migration_test.gointernal/rpcserver/wallet_test.gointernal/test/test.gopkg/boltzrpc/boltzrpc.proto
💤 Files with no reviewable changes (16)
- internal/onchain/wallet/lib/etc/mtab
- .gitmodules
- Dockerfile
- internal/onchain/wallet/static.go
- docs/gdk.md
- internal/onchain/wallet/include/gdk_export.h
- gdk.Dockerfile
- cmd/boltzcli/utils.go
- internal/onchain/liquid-wallet/wallet.go
- gdk
- internal/onchain/wallet/include/gdk.h
- internal/onchain/wallet/wallet_test.go
- internal/onchain/wallet/dynamic.go
- internal/onchain/wallet/handler.go
- cmd/boltzcli/commands.go
- internal/onchain/wallet/wallet.go
Contributor
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)
256-265:⚠️ Potential issue | 🟠 MajorDo not expose global migration warnings to every tenant.
decryptWalletCredentialsForStartup()buildswalletMigrationWarningsfrom all rows returned byQueryWalletCredentials(), butGetInfo()now returns that shared slice verbatim for any caller. In multi-tenant mode this leaks other tenants’ migration state, and possibly wallet identifiers depending on the warning text. Please scope these warnings by tenant or gate them to admin callers only.🔒 Minimal safe fix
+ warnings := []string(nil) + if isAdmin(ctx) { + warnings = server.getWalletMigrationWarnings() + } + response := &boltzrpc.GetInfoResponse{ Version: build.GetVersion(), Network: server.network.Name, BlockHeights: blockHeights, Tenant: serializeTenant(macaroons.TenantFromContext(ctx)), PendingSwaps: pendingSwapIds, PendingReverseSwaps: pendingReverseSwapIds, RefundableSwaps: refundableSwapIds, ClaimableSwaps: claimableSwapIds, - Warnings: server.getWalletMigrationWarnings(), + Warnings: warnings,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/rpcserver/router.go` around lines 256 - 265, GetInfo() is currently returning server.getWalletMigrationWarnings() verbatim which leaks migration warnings across tenants because decryptWalletCredentialsForStartup() builds a shared slice from QueryWalletCredentials(); update the flow to scope warnings by tenant (or only include for admin callers) by changing server.getWalletMigrationWarnings() to accept the caller identity (e.g., macaroons.TenantFromContext(ctx) or a Tenant/IsAdmin flag) and have decryptWalletCredentialsForStartup() store warnings per-tenant (or return tenant-scoped warnings) instead of a global slice; in GetInfo() call the new method with the tenant/context, filter or clone the warnings to only include those belonging to that tenant (or omit unless tenant.IsAdmin), and ensure you do not return the original shared slice reference to avoid cross-tenant leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/rpcserver/router.go`:
- Around line 1921-1923: The current fallback hides real DB errors by returning
originalErr when server.database.GetWallet(id) returns an error; change the
logic so that if server.database.GetWallet(id) returns a non-nil error you
propagate that error (return nil, err) unless it is a “not found” sentinel (use
your project's NotFound check, e.g. errors.Is(err, sql.ErrNoRows) or the DB
layer’s IsNotFound helper); only when GetWallet succeeds (dbWallet != nil) do
you apply the legacy checks (dbWallet.Legacy and dbWallet.NodePubkey) and fall
back to originalErr if appropriate.
---
Outside diff comments:
In `@internal/rpcserver/router.go`:
- Around line 256-265: GetInfo() is currently returning
server.getWalletMigrationWarnings() verbatim which leaks migration warnings
across tenants because decryptWalletCredentialsForStartup() builds a shared
slice from QueryWalletCredentials(); update the flow to scope warnings by tenant
(or only include for admin callers) by changing
server.getWalletMigrationWarnings() to accept the caller identity (e.g.,
macaroons.TenantFromContext(ctx) or a Tenant/IsAdmin flag) and have
decryptWalletCredentialsForStartup() store warnings per-tenant (or return
tenant-scoped warnings) instead of a global slice; in GetInfo() call the new
method with the tenant/context, filter or clone the warnings to only include
those belonging to that tenant (or omit unless tenant.IsAdmin), and ensure you
do not return the original shared slice reference to avoid cross-tenant leakage.
🪄 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: 18cc7875-1207-452a-9511-2088f4909024
📒 Files selected for processing (2)
internal/rpcserver/router.gointernal/rpcserver/wallet_migration_test.go
jackstar12
force-pushed
the
feat/deprecate-gdk
branch
from
030a9e2 to
7ed3cc5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/test/test.go (1)
52-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast instead of returning partially initialized test credentials.
This helper still discards mnemonic/descriptor derivation errors. When one of those calls fails, the fixture returns invalid credentials and the real cause only surfaces later in unrelated test steps.
🧪 One way to make the fixture fail at the source
-func WalletCredentials(currency boltz.Currency) *onchain.WalletCredentials { - mnemonic, _ := liquid_wallet.GenerateMnemonic(boltz.Regtest) +func WalletCredentials(t *testing.T, currency boltz.Currency) *onchain.WalletCredentials { + mnemonic, err := liquid_wallet.GenerateMnemonic(boltz.Regtest) + require.NoError(t, err) creds := &onchain.WalletCredentials{ WalletInfo: WalletInfo(currency), Mnemonic: mnemonic, } if currency == boltz.CurrencyBtc { - creds.CoreDescriptor, _ = bitcoin_wallet.DeriveDefaultDescriptor(boltz.Regtest, mnemonic) + creds.CoreDescriptor, err = bitcoin_wallet.DeriveDefaultDescriptor(boltz.Regtest, mnemonic) + require.NoError(t, err) } if currency == boltz.CurrencyLiquid { - creds.CoreDescriptor, _ = liquid_wallet.DeriveDefaultDescriptor(boltz.Regtest, mnemonic) + creds.CoreDescriptor, err = liquid_wallet.DeriveDefaultDescriptor(boltz.Regtest, mnemonic) + require.NoError(t, err) } return creds }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/test/test.go` around lines 52 - 63, The helper WalletCredentials currently ignores errors from liquid_wallet.GenerateMnemonic and DeriveDefaultDescriptor, returning partially initialized credentials; update WalletCredentials to fail fast by checking the returned errors from liquid_wallet.GenerateMnemonic and both bitcoin_wallet.DeriveDefaultDescriptor and liquid_wallet.DeriveDefaultDescriptor and aborting immediately (e.g., panic or test fatal) when an error occurs so invalid creds are never returned; locate the error checks around the calls to GenerateMnemonic and the two DeriveDefaultDescriptor calls and handle their error results instead of discarding them (or alternatively change WalletCredentials to return (*onchain.WalletCredentials, error) and propagate the error to callers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/boltzrpc/boltzrpc.proto`:
- Around line 127-129: The RPC comment for GetWallet is misleading: it says
"Returns the current balance of a wallet" but the rpc
GetWallet(GetWalletRequest) returns the full Wallet message. Update the proto
service comment for rpc GetWallet to describe that it returns information about
a wallet (e.g., "Returns information about a wallet, including its current
balance"), so generated docs reflect that GetWallet returns the Wallet message
rather than only a balance; edit the comment immediately above the rpc GetWallet
declaration referencing GetWalletRequest and Wallet.
---
Duplicate comments:
In `@internal/test/test.go`:
- Around line 52-63: The helper WalletCredentials currently ignores errors from
liquid_wallet.GenerateMnemonic and DeriveDefaultDescriptor, returning partially
initialized credentials; update WalletCredentials to fail fast by checking the
returned errors from liquid_wallet.GenerateMnemonic and both
bitcoin_wallet.DeriveDefaultDescriptor and liquid_wallet.DeriveDefaultDescriptor
and aborting immediately (e.g., panic or test fatal) when an error occurs so
invalid creds are never returned; locate the error checks around the calls to
GenerateMnemonic and the two DeriveDefaultDescriptor calls and handle their
error results instead of discarding them (or alternatively change
WalletCredentials to return (*onchain.WalletCredentials, error) and propagate
the error to callers).
🪄 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: e1bab35d-d402-4b9b-bc2d-913da1ac79e3
⛔ Files ignored due to path filters (2)
pkg/boltzrpc/boltzrpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (45)
.gitmodulesDockerfileMakefilecmd/boltzcli/commands.gocmd/boltzcli/utils.godocs/grpc.mddocs/index.mddocs/wallets.mdgdkgdk.Dockerfileinternal/database/wallet.gointernal/lightning/static.gointernal/nursery/nursery.gointernal/nursery/reverse.gointernal/onchain/bitcoin-wallet/bdk/static.gointernal/onchain/bitcoin-wallet/descriptor.gointernal/onchain/bitcoin-wallet/wallet.gointernal/onchain/liquid-wallet/descriptor.gointernal/onchain/liquid-wallet/lwk/static.gointernal/onchain/liquid-wallet/wallet.gointernal/onchain/mnemonic.gointernal/onchain/wallet/dummy.cppinternal/onchain/wallet/dynamic.gointernal/onchain/wallet/handler.gointernal/onchain/wallet/include/gdk.hinternal/onchain/wallet/include/gdk_export.hinternal/onchain/wallet/lib/.dockerenvinternal/onchain/wallet/lib/.gitkeepinternal/onchain/wallet/lib/dev/consoleinternal/onchain/wallet/lib/etc/hostnameinternal/onchain/wallet/lib/etc/hostsinternal/onchain/wallet/lib/etc/mtabinternal/onchain/wallet/lib/etc/resolv.confinternal/onchain/wallet/static.gointernal/onchain/wallet/wallet.gointernal/onchain/wallet/wallet_test.gointernal/rpcserver/reverse_swap_test.gointernal/rpcserver/router.gointernal/rpcserver/serializer.gointernal/rpcserver/server.gointernal/rpcserver/wallet_migration.gointernal/rpcserver/wallet_migration_test.gointernal/rpcserver/wallet_test.gointernal/test/test.gopkg/boltzrpc/boltzrpc.proto
💤 Files with no reviewable changes (15)
- internal/onchain/wallet/lib/etc/mtab
- gdk
- cmd/boltzcli/utils.go
- .gitmodules
- internal/onchain/wallet/static.go
- Dockerfile
- internal/onchain/wallet/dynamic.go
- internal/onchain/liquid-wallet/wallet.go
- internal/onchain/wallet/include/gdk_export.h
- cmd/boltzcli/commands.go
- gdk.Dockerfile
- internal/onchain/wallet/wallet_test.go
- internal/onchain/wallet/include/gdk.h
- internal/onchain/wallet/wallet.go
- internal/onchain/wallet/handler.go
✅ Files skipped from review due to trivial changes (7)
- internal/onchain/bitcoin-wallet/bdk/static.go
- internal/lightning/static.go
- internal/onchain/liquid-wallet/lwk/static.go
- internal/onchain/mnemonic.go
- internal/nursery/reverse.go
- internal/onchain/liquid-wallet/descriptor.go
- internal/rpcserver/server.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/onchain/bitcoin-wallet/descriptor.go
- docs/wallets.md
- internal/rpcserver/wallet_test.go
- internal/rpcserver/wallet_migration.go
- Makefile
- internal/rpcserver/wallet_migration_test.go
- internal/rpcserver/router.go
michael1011
requested changes
May 2, 2026
michael1011
left a comment
michael1011
left a comment
Member
There was a problem hiding this comment.
This is not deprecation. This is flat out removal. Which is fine, but we should make sure to communicate that correctly
Comment on lines
-1
to
-3
| --- | ||
| description: Full gRPC and REST API reference for boltzd covering all swap, wallet, and autoswap methods | ||
| --- |
| @@ -229,16 +227,6 @@ func serializeChainPair(pair boltz.Pair, chainPair *boltz.ChainPair) *boltzrpc.P | |||
| } | |||
|
|
|||
| //nolint:staticcheck | |||
Member
There was a problem hiding this comment.
Suggested change
| //nolint:staticcheck |
Comment on lines
-475
to
-490
| if !wallet.Initialized() { | ||
| if cfg.AutoConsolidateThreshold == nil { | ||
| threshold := wallet.DefaultAutoConsolidateThreshold | ||
| cfg.AutoConsolidateThreshold = &threshold | ||
| } | ||
| err := wallet.Init(wallet.Config{ | ||
| DataDir: cfg.DataDir, | ||
| Network: network, | ||
| Debug: false, | ||
| Electrum: electrumConfig, | ||
| AutoConsolidateThreshold: *cfg.AutoConsolidateThreshold, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not init wallet: %v", err) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Why do we not need that anymore? How does LWK handle it?
Member
Author
There was a problem hiding this comment.
This is GDK specific configuration. The LWK backend is initialized here:
boltz-client/internal/rpcserver/server.go
Lines 235 to 246 in d20135c
Although I just noticed that it doesn't pass on the configuration override.
| } | ||
| } | ||
| migrated, warnings := server.migrateWalletCredentials(credentials) | ||
| server.walletMigrationWarnings = warnings |
Member
There was a problem hiding this comment.
These warnings are not updated after the wallets are removed
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/rpcserver/wallet_migration.go (2)
96-107: 💤 Low valueIn-place filter mutates the backing array while iterating.
filtered := server.walletMigrationWarnings[:0]reuses the original backing array; the loop both reads from and writes into it. It works because writes never outpace reads (filtered length ≤ loop index), but it's a subtle pattern and any later refactor that adds work between iterations could break it. Allocating a new slice would be marginally less efficient but easier to reason about.Proposed refactor
- filtered := server.walletMigrationWarnings[:0] - for _, warning := range server.walletMigrationWarnings { - if warning.walletId != walletId { - filtered = append(filtered, warning) - } - } - server.walletMigrationWarnings = filtered + filtered := make([]walletMigrationWarning, 0, len(server.walletMigrationWarnings)) + for _, warning := range server.walletMigrationWarnings { + if warning.walletId != walletId { + filtered = append(filtered, warning) + } + } + server.walletMigrationWarnings = filtered🤖 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/rpcserver/wallet_migration.go` around lines 96 - 107, The in-place filter in removeWalletMigrationWarning reuses the original backing array (filtered := server.walletMigrationWarnings[:0]) which mutates the slice while iterating and is fragile; change it to allocate a new slice (e.g., make([]Type, 0, len(server.walletMigrationWarnings))) then append non-matching entries to that new slice and finally assign it back to server.walletMigrationWarnings inside the existing walletMigrationWarningsLock/unlock block so iteration no longer writes into the backing array being read.
33-62: 💤 Low valueMinor: misleading "Migrated" log when descriptor is already present.
If a credential reaches
migrateLegacyWalletwithLegacy=truebut a non-emptyCoreDescriptor, the function skips the derivation block and returns(true, ""), which causes the caller to log"Migrated legacy GDK wallet ... to descriptor credentials"even though onlySubaccount/Legacywere cleared. Not a correctness bug, but the log message will mislead operators investigating migrations. Consider distinguishing the two paths (e.g., return a status enum, or only log on actual descriptor derivation).🤖 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/rpcserver/wallet_migration.go` around lines 33 - 62, The function migrateLegacyWallet currently returns (bool,string) and treats clearing Subaccount/Legacy the same as deriving a new CoreDescriptor, causing misleading "Migrated ..." logs; change migrateLegacyWallet to report whether a descriptor was actually derived (e.g., add a third return value or a small enum/status) so callers can tell the two paths apart: inspect CoreDescriptor presence, set derivedDescriptor=true when deriveDescriptor(...) succeeds and false when only Subaccount/Legacy are cleared, and update the caller's logging to emit the "Migrated legacy GDK wallet ... to descriptor credentials" message only when derivedDescriptor is true; keep error paths using manualWalletMigrationWarning unchanged.internal/rpcserver/router.go (1)
2087-2095: 💤 Low valueMigration warnings are persisted to in-memory state before DB persistence is confirmed.
setWalletMigrationWarningsruns at line 2088 unconditionally, but if the subsequentRunTxat lines 2090-2094 fails,unlockreturns an error while warnings remain visible to callers (e.g.,GetInfo). Subsequent unlock attempts will re-run migration and re-set warnings, so it's eventually consistent, but in the failed-persist window callers can observe migration messages for changes that weren't actually committed. Consider deferringsetWalletMigrationWarningsuntil after persistence succeeds.Proposed reorder
migrated, warnings := server.migrateWalletCredentials(credentials) - server.setWalletMigrationWarnings(warnings) if migrated { if err := server.database.RunTx(func(tx *database.Transaction) error { return server.encryptWalletCredentials(tx, password, credentials) }); err != nil { return fmt.Errorf("failed to persist wallet credential migration: %w", err) } } + server.setWalletMigrationWarnings(warnings)🤖 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/rpcserver/router.go` around lines 2087 - 2095, The code sets in-memory migration warnings via server.setWalletMigrationWarnings(warnings) before persisting the migrated credentials, causing callers to see warnings even if the subsequent server.database.RunTx(...) / server.encryptWalletCredentials(...) fails; move the call to server.setWalletMigrationWarnings so it only runs after the RunTx completes successfully (i.e., call setWalletMigrationWarnings(warnings) after the RunTx returns nil), or alternatively set the warnings inside the successful transaction callback after encryptWalletCredentials returns, ensuring warnings reflect only persisted state; references: server.migrateWalletCredentials, server.setWalletMigrationWarnings, server.database.RunTx, server.encryptWalletCredentials.
🤖 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.
Nitpick comments:
In `@internal/rpcserver/router.go`:
- Around line 2087-2095: The code sets in-memory migration warnings via
server.setWalletMigrationWarnings(warnings) before persisting the migrated
credentials, causing callers to see warnings even if the subsequent
server.database.RunTx(...) / server.encryptWalletCredentials(...) fails; move
the call to server.setWalletMigrationWarnings so it only runs after the RunTx
completes successfully (i.e., call setWalletMigrationWarnings(warnings) after
the RunTx returns nil), or alternatively set the warnings inside the successful
transaction callback after encryptWalletCredentials returns, ensuring warnings
reflect only persisted state; references: server.migrateWalletCredentials,
server.setWalletMigrationWarnings, server.database.RunTx,
server.encryptWalletCredentials.
In `@internal/rpcserver/wallet_migration.go`:
- Around line 96-107: The in-place filter in removeWalletMigrationWarning reuses
the original backing array (filtered := server.walletMigrationWarnings[:0])
which mutates the slice while iterating and is fragile; change it to allocate a
new slice (e.g., make([]Type, 0, len(server.walletMigrationWarnings))) then
append non-matching entries to that new slice and finally assign it back to
server.walletMigrationWarnings inside the existing
walletMigrationWarningsLock/unlock block so iteration no longer writes into the
backing array being read.
- Around line 33-62: The function migrateLegacyWallet currently returns
(bool,string) and treats clearing Subaccount/Legacy the same as deriving a new
CoreDescriptor, causing misleading "Migrated ..." logs; change
migrateLegacyWallet to report whether a descriptor was actually derived (e.g.,
add a third return value or a small enum/status) so callers can tell the two
paths apart: inspect CoreDescriptor presence, set derivedDescriptor=true when
deriveDescriptor(...) succeeds and false when only Subaccount/Legacy are
cleared, and update the caller's logging to emit the "Migrated legacy GDK wallet
... to descriptor credentials" message only when derivedDescriptor is true; keep
error paths using manualWalletMigrationWarning unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e0c2e02-f6dd-4076-93de-074443065cce
⛔ Files ignored due to path filters (2)
pkg/boltzrpc/boltzrpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
docs/gdk.mddocs/grpc.mddocs/wallets.mdinternal/database/wallet.gointernal/macaroons/permissions.gointernal/onchain/liquid-wallet/descriptor.gointernal/onchain/liquid-wallet/wallet.gointernal/onchain/liquid-wallet/wallet_test.gointernal/rpcserver/router.gointernal/rpcserver/serializer.gointernal/rpcserver/server.gointernal/rpcserver/wallet_migration.gointernal/rpcserver/wallet_migration_test.gointernal/test/test.gopkg/boltzrpc/boltzrpc.protopkg/boltzrpc/client/client.gopkg/boltzrpc/grpc_docs.template
💤 Files with no reviewable changes (2)
- internal/macaroons/permissions.go
- pkg/boltzrpc/client/client.go
michael1011
approved these changes
May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Deprecated / Removed
Documentation