feat: lwk server integration#462
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce Liquid wallet support to the server, including initialization of a Liquid blockchain backend, new wallet login logic, and transactional improvements for wallet import and credential encryption. Test helpers are refactored to ensure deterministic wallet funding, and subaccount management is now restricted to BTC wallets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant DB
participant BTCBackend
participant LiquidBackend
User->>Server: ImportWallet(credentials)
Server->>DB: Start transaction
Server->>DB: Decrypt & append credentials
Server->>Server: loginWallet(credentials)
alt Currency is BTC
Server->>BTCBackend: Login(credentials)
else Currency is Liquid
Server->>LiquidBackend: Login(credentials)
end
Server->>DB: Encrypt all credentials
Server->>DB: Commit transaction
Server->>User: Return wallet info
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/rpcserver/router.go (1)
1396-1433: 🛠️ Refactor suggestionConsider defensive copying when appending to the credentials slice.
The refactoring to use database transactions is excellent for ensuring atomicity. However, on line 1415, appending to
decryptedCredentialsmodifies the original slice returned bydecryptWalletCredentials. If this slice is cached or used elsewhere, it could lead to unexpected behavior.Consider creating a new slice to avoid modifying the original:
- decryptedCredentials = append(decryptedCredentials, credentials) + allCredentials := make([]*onchain.WalletCredentials, len(decryptedCredentials)+1) + copy(allCredentials, decryptedCredentials) + allCredentials[len(decryptedCredentials)] = credentials if password != "" { - if err := server.encryptWalletCredentials(tx, password, decryptedCredentials); err != nil { + if err := server.encryptWalletCredentials(tx, password, allCredentials); err != nil { return fmt.Errorf("could not encrypt credentials: %w", err) } }
🧹 Nitpick comments (2)
internal/onchain/liquid-wallet/wallet.go (1)
64-82: Consider making Regtest Esplora URL configurableThe enhanced Esplora configuration logic with default settings for Regtest is a good improvement. However, the hardcoded
http://localhost:3003URL might be inflexible for different development setups.Consider making the default Regtest URL configurable through environment variables or configuration:
case boltz.Regtest: + url := os.Getenv("REGTEST_ESPLORA_URL") + if url == "" { + url = "http://localhost:3003" + } cfg.Esplora = &EsploraConfig{ - Url: "http://localhost:3003", + Url: url, Waterfall: false, }The rest of the logic correctly handles client creation and error cases.
internal/rpcserver/router.go (1)
2397-2402: Clean wallet routing implementation.The
loginWalletmethod provides clean routing between Liquid and Bitcoin wallet implementations. The implementation is simple and focused.Consider adding error context to help with debugging:
func (server *routedBoltzServer) loginWallet(credentials *onchain.WalletCredentials) (onchain.Wallet, error) { if credentials.Currency == boltz.CurrencyLiquid { - return liquid_wallet.NewWallet(server.liquidBackend, credentials) + wallet, err := liquid_wallet.NewWallet(server.liquidBackend, credentials) + if err != nil { + return nil, fmt.Errorf("failed to create liquid wallet: %w", err) + } + return wallet, nil } - return wallet.Login(credentials) + wallet, err := wallet.Login(credentials) + if err != nil { + return nil, fmt.Errorf("failed to login to wallet: %w", err) + } + return wallet, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/onchain/liquid-wallet/wallet.go(1 hunks)internal/rpcserver/router.go(11 hunks)internal/rpcserver/rpcserver_test.go(1 hunks)internal/rpcserver/server.go(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
internal/rpcserver/rpcserver_test.go
[error] 288-288: TestDirectReverseSwapPayments/Multiple failed: timed out while waiting for swap EA5UqotPduHR to reach state SUCCESSFUL
[error] 1850-1850: TestDirectReverseSwapPayments/Liquid/ZeroConf failed: rpc error: code = Unknown desc = wallet regtest has insufficient balance for sending 50069 sats
[error] 2793-2793: TestChainSwap/BTC/InternalWallets/EnoughBalance failed: rpc error: code = NotFound desc = readonly wallet with id: 2 currency: L-BTC not found
[error] 2793-2793: TestChainSwap/Liquid/InternalWallets/EnoughBalance failed: rpc error: code = NotFound desc = readonly wallet with id: 2 currency: BTC not found
[error] 2971-2971: TestChainSwap/BTC/Refund/Manual/Wallet/Valid failed: Should not be zero, but was 0
[error] 1859-1859: TestDirectReverseSwapPayments/Liquid/Normal failed: timed out while waiting for swap wqSqI2oSbGg5 to reach state PENDING
🔇 Additional comments (11)
internal/rpcserver/server.go (2)
21-21: LGTM: Clean import additionThe import of the liquid_wallet package follows Go conventions and is properly aliased.
194-200:Details
✅ Verification successful
Verify liquidBackend field declaration in routedBoltzServer struct
The liquid backend initialization logic is correct and follows the same pattern as other backend initializations. However, ensure that the
liquidBackendfield is declared in theroutedBoltzServerstruct.
🏁 Script executed:
#!/bin/bash # Description: Verify that liquidBackend field is declared in routedBoltzServer struct # Expected: Should find the liquidBackend field declaration ast-grep --pattern $'type routedBoltzServer struct { $$$ liquidBackend $_ $$$ }'Length of output: 1207
liquidBackend field is properly declared
The
routedBoltzServerstruct ininternal/rpcserver/router.goalready includes:• Line 73:
liquidBackend *liquid_wallet.BlockchainBackendNo further action required.
internal/onchain/liquid-wallet/wallet.go (1)
53-58: LGTM: Network-specific sync intervals improve development experienceThe network-specific sync interval configuration is a good improvement. Using 1 second for Regtest environments will provide faster feedback during development and testing, while maintaining the 30-second default for production networks.
internal/rpcserver/router.go (8)
29-29: LGTM!The import of the liquid-wallet package with an appropriate alias is clean and follows Go conventions.
73-74: LGTM!The
liquidBackendfield is properly typed and logically placed with other service dependencies in the struct.
1531-1539: Verify that Liquid wallets don't support subaccounts.The code now restricts subaccount creation to BTC wallets only. While the comment indicates this is intentional ("only GDK wallets have subaccounts"), please confirm that:
- Liquid wallets indeed don't support subaccounts in the current implementation
- This won't break any existing functionality for users who might have expected subaccount support for Liquid wallets
1736-1761: LGTM! Proper multi-currency support.The changes correctly handle fee estimation for different wallet currencies by:
- Extracting the currency from the wallet info
- Using the appropriate currency for fee estimation
- Selecting the correct dummy address based on the wallet's currency
1826-1829: LGTM!Using
getAnyWalletwithAllowReadonly: trueis appropriate for wallet removal operations, as both readonly and regular wallets should be removable.
1922-1934: LGTM! Good transaction composition pattern.The signature change to accept a transaction parameter enables better transaction composition, allowing this method to participate in larger atomic operations. The caller is now responsible for transaction management, which is a cleaner design.
2032-2032: LGTM!Using the new
loginWalletmethod provides proper routing between Bitcoin and Liquid wallet implementations while maintaining the concurrent login behavior.
2071-2073: LGTM! Proper transaction usage.Wrapping the credential encryption in a transaction ensures atomicity - if encryption fails for any credential, all changes are rolled back. This prevents partial password updates.
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
2 times, most recently
from
16c3c8f to
c10fd68
Compare
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
c10fd68 to
fa5d3ea
Compare
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
fa5d3ea to
c1e12c4
Compare
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
cdd4bdb to
904749c
Compare
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
904749c to
ab20430
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/rpcserver/rpcserver_test.go (2)
1866-1869: Fix TODO grammar and consider tracking in an issue.The TODO comment has a grammatical error and should be tracked properly.
- // TODO: re-enable this once when we have mrh notifications + // TODO: re-enable this once we have mrh notificationsConsider creating a GitHub issue to track the re-enabling of these Liquid tests when mrh notifications are implemented.
3159-3195: Well-structured test for legacy wallet functionality.The test properly verifies the legacy wallet type switching behavior. Good use of type assertions to verify the wallet implementation changes from
liquid_wallet.Wallettowallet.Walletwhen the legacy flag is set.Consider extracting the direct SQL update into a test helper function for better maintainability:
func setWalletLegacyFlag(t *testing.T, db *database.Database, walletId uint64, legacy bool) { _, err := db.Exec("UPDATE wallets SET legacy = ? WHERE id = ?", legacy, walletId) require.NoError(t, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
pkg/boltzrpc/autoswaprpc/autoswaprpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/autoswaprpc/autoswaprpc_grpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc.pb.gw.gois excluded by!**/*.pb.gw.gopkg/boltzrpc/boltzrpc_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (23)
.gitignore(1 hunks)Makefile(1 hunks)cmd/boltzcli/commands.go(2 hunks)docs/grpc.md(1 hunks)internal/cln/cln.go(1 hunks)internal/database/database.go(1 hunks)internal/database/migration.go(2 hunks)internal/database/wallet.go(1 hunks)internal/lnd/lnd.go(1 hunks)internal/mocks/lightning/LightningNode_mock.go(1 hunks)internal/mocks/onchain/Wallet_mock.go(1 hunks)internal/nursery/reverse.go(1 hunks)internal/onchain/liquid-wallet/wallet.go(5 hunks)internal/onchain/liquid-wallet/wallet_test.go(5 hunks)internal/onchain/wallet.go(2 hunks)internal/onchain/wallet/wallet_test.go(1 hunks)internal/rpcserver/router.go(15 hunks)internal/rpcserver/rpcserver_test.go(5 hunks)internal/rpcserver/serializer.go(1 hunks)internal/rpcserver/server.go(2 hunks)internal/test/test.go(2 hunks)pkg/boltzrpc/boltzrpc.proto(4 hunks)pkg/boltzrpc/client/client.go(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- internal/rpcserver/serializer.go
- internal/database/database.go
- docs/grpc.md
- cmd/boltzcli/commands.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/rpcserver/server.go
- internal/onchain/liquid-wallet/wallet.go
🧰 Additional context used
🧠 Learnings (5)
pkg/boltzrpc/client/client.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
internal/nursery/reverse.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
internal/test/test.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
internal/rpcserver/router.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
internal/rpcserver/rpcserver_test.go (1)
Learnt from: jackstar12
PR: BoltzExchange/boltz-client#444
File: internal/rpcserver/router.go:2036-2052
Timestamp: 2025-05-26T09:32:25.533Z
Learning: In boltz-client, when handling Boltz backend connectivity during startup, the code intentionally uses different error handling strategies: logger.Fatalf for version incompatibility (fail fast approach) and retry logic for availability issues. Version incompatibility requires immediate human intervention and should not be retried.
🧬 Code Graph Analysis (8)
pkg/boltzrpc/client/client.go (2)
pkg/boltzrpc/boltzrpc.pb.go (6)
Subaccount(5992-6001)Subaccount(6016-6016)Subaccount(6031-6033)SetSubaccountRequest(4618-4626)SetSubaccountRequest(4641-4641)SetSubaccountRequest(4656-4658)internal/onchain/wallet/wallet.go (1)
Subaccount(53-59)
internal/onchain/wallet.go (1)
internal/onchain/onchain.go (1)
Output(33-36)
internal/onchain/wallet/wallet_test.go (1)
internal/onchain/wallet/wallet.go (1)
MaxInputs(36-36)
internal/lnd/lnd.go (2)
internal/onchain/onchain.go (1)
Output(33-36)internal/lightning/lightning.go (1)
ErrUnsupported(28-28)
internal/cln/cln.go (2)
internal/onchain/onchain.go (1)
Output(33-36)internal/lightning/lightning.go (1)
ErrUnsupported(28-28)
internal/nursery/reverse.go (4)
internal/lightning/lightning.go (1)
ErrUnsupported(28-28)internal/database/reverse.go (1)
ReverseSwap(17-49)internal/onchain/onchain.go (2)
Output(33-36)Id(19-19)internal/logger/logger.go (1)
Errorf(97-99)
internal/mocks/onchain/Wallet_mock.go (1)
internal/onchain/onchain.go (1)
Output(33-36)
internal/mocks/lightning/LightningNode_mock.go (2)
internal/onchain/onchain.go (1)
Output(33-36)internal/nursery/swap.go (1)
Output(32-39)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (40)
internal/database/wallet.go (1)
59-59: LGTM! Database field scanning correctly updated.The addition of the
Legacyfield scanning aligns with the database schema migration and follows the established pattern for parsing wallet fields.pkg/boltzrpc/client/client.go (1)
118-118: LGTM! Appropriate lint suppressions for deprecated methods.The
//nolint:staticcheckdirectives correctly suppress static analysis warnings for the deprecated subaccount methods while maintaining backward compatibility.Also applies to: 123-123
internal/lnd/lnd.go (1)
400-402: LGTM! Proper stub implementation for unsupported functionality.The
GetOutputsmethod correctly implements theWalletinterface by returninglightning.ErrUnsupported, consistent with other unsupported methods in the LND implementation.internal/cln/cln.go (2)
354-354: LGTM! Syntax fix resolves missing closing brace.The addition of the closing brace fixes the
SendToAddressmethod syntax.
356-358: LGTM! Consistent stub implementation for unsupported functionality.The
GetOutputsmethod properly implements theWalletinterface withlightning.ErrUnsupported, maintaining consistency with the LND implementation and other unsupported methods in CLN.internal/onchain/wallet.go (2)
57-57: LGTM! Clean interface extension for output retrieval.The
GetOutputsmethod is a well-designed addition to theWalletinterface, enabling address-specific output retrieval with an appropriate signature that returns[]*Outputand error.
75-75: LGTM! Legacy field supports wallet migration strategy.The
Legacyboolean field is properly positioned in theWalletCredentialsstruct and aligns with the database migration strategy for distinguishing legacy wallets during the Liquid wallet integration.Makefile (2)
76-78: LGTM! Clean test data management.The new
clear-wallet-datatarget properly removes test data directories for both liquid wallet components, ensuring a clean testing environment.
80-84: LGTM! Automated cleanup integration.Adding the
clear-wallet-datadependency to both regtest targets ensures test data is automatically cleaned before each run, preventing potential test contamination.internal/test/test.go (2)
64-64: LGTM! Function export for broader usage.Exporting
FundWalletenables its usage in other packages, which aligns with the restructured wallet initialization approach.
132-132: LGTM! Consistent function naming.Updated call correctly uses the newly exported
FundWalletfunction.internal/onchain/wallet/wallet_test.go (1)
279-292: LGTM! Simplified test logic with reliable polling.The replacement of complex event notification logic with
require.Eventuallypolling is a good improvement. The test now:
- Polls every 250ms for up to 15 seconds
- Checks for the expected number of transactions (including consolidation)
- Validates consolidation transaction properties correctly
This approach is more reliable and easier to understand than the previous event-based waiting.
internal/database/migration.go (3)
24-24: LGTM! Schema version incremented for new migration.Correctly increments the schema version to support the new legacy wallet column migration.
591-592: LGTM! Proper migration logging.Consistent with the established pattern of logging migration steps.
598-611: LGTM! Well-structured migration for legacy wallet support.The migration correctly:
- Adds the
legacyboolean column with appropriate default value (FALSE)- Sets existing wallets as legacy (TRUE) to maintain backward compatibility
- Includes proper error handling for both steps
This supports the new wallet type differentiation in the Liquid wallet integration.
internal/nursery/reverse.go (1)
339-377: LGTM! Excellent abstraction improvement.The refactoring successfully removes the dependency on the internal wallet package by:
Key improvements:
- Uses the new generic
GetOutputsinterface method instead of type assertion- Handles
ErrUnsupportedgracefully by returning(false, nil)- Implements sophisticated output-to-swap matching with proper sorting by amount
- Includes robust error handling for database queries
Logic validation:
- Correctly sorts both swaps and outputs by amount for optimal matching
- Handles multiple swaps per address with appropriate output selection
- Validates existing claim transactions to prevent conflicts
- Chooses outputs that best match swap amounts while staying within bounds
This abstraction enables the nursery to work generically across different wallet implementations while maintaining all the existing functionality.
pkg/boltzrpc/boltzrpc.proto (4)
108-108: LGTM: Consistent deprecation of subaccount RPC methods.The deprecation of
SetSubaccountandGetSubaccountsRPC methods aligns with the broader changes to restrict subaccount management to BTC wallets only.Also applies to: 113-113
721-721: LGTM: Subaccount field deprecation in WalletCredentials.Marking the
subaccountfield as deprecated is consistent with the overall subaccount deprecation strategy.
746-746: LGTM: Comprehensive deprecation of subaccount message types.All subaccount-related request and response messages are properly marked as deprecated, ensuring a consistent deprecation strategy.
Also applies to: 753-753, 758-758
899-899: LGTM: Core Subaccount message deprecation.Deprecating the main
Subaccountmessage completes the systematic phase-out of subaccount functionality.internal/onchain/liquid-wallet/wallet_test.go (4)
22-28: LGTM: Centralized configuration management.The
defaultConfig()function provides a clean way to centralize test configuration, improving maintainability and consistency across tests.
32-32: LGTM: Improved TestMain initialization.Using
defaultConfig()for backend initialization and adding explicit logger initialization improves test setup consistency.Also applies to: 36-36
121-132: LGTM: Explicit backend parameter in helper function.Making the backend parameter explicit in the
newWallethelper function improves clarity and allows for test-specific backend configurations.
147-152: LGTM: Test-specific backend configuration.Creating a custom backend with a specific
ConsolidationThresholdfor the auto-consolidation test enables deterministic testing behavior.internal/mocks/onchain/Wallet_mock.go (1)
204-264: LGTM: Proper mock implementation for new GetOutputs method.The auto-generated mock for
GetOutputs(address string) ([]*onchain.Output, error)follows the standard testify mock patterns and provides complete testing support for the new wallet interface method.internal/mocks/lightning/LightningNode_mock.go (1)
446-506: LGTM: Consistent GetOutputs mock implementation.The auto-generated mock for
GetOutputs(address string) ([]*onchain.Output, error)on the LightningNode interface matches the wallet implementation and follows proper testify mock conventions.internal/rpcserver/rpcserver_test.go (3)
20-21: LGTM! Import statements properly organized.The new imports for Liquid wallet support are correctly added and follow the project's import organization pattern.
1161-1186: Much improved wallet funding implementation!The refactored
fundedWalletfunction addresses the critical issues from previous reviews:
- ✅ Uses
require.Eventuallywith a 10-second timeout instead of an infinite loop- ✅ Proper error handling without failing the test inside the polling logic
- ✅ Sends multiple transactions before mining to ensure deterministic funding
This implementation is much more robust and test-friendly.
1847-1857: Good improvement to test reliability with polling logic.The change from immediate checks to
require.Eventuallywith block mining makes the test more robust by accounting for asynchronous transaction processing. The 15-second timeout with 3-second intervals provides a good balance between test speed and reliability.internal/rpcserver/router.go (11)
29-29: LGTM: Import added for Liquid wallet supportThe import follows Go naming conventions and aligns with the PR objective of integrating Liquid wallet functionality.
72-72: LGTM: Server struct extended for Liquid backendThe
liquidBackendfield addition is consistent with the server architecture pattern and enables Liquid wallet functionality as mentioned in the AI summary.
1394-1431: Excellent refactoring: Wallet import now runs in database transactionThe wallet import process has been properly refactored to ensure transactional consistency. The implementation correctly:
- Validates credentials before database operations
- Uses
RunTxto wrap all database operations in a transaction- Handles wallet login and onchain addition within the transaction scope
- Provides proper error handling with rollback on failure
This addresses potential data consistency issues that could occur if the process failed partway through.
1533-1542: LGTM: Subaccount creation restricted to BTC walletsThe logic correctly restricts subaccount creation to BTC wallets only, which aligns with the comment "only GDK wallets have subaccounts" and the overall deprecation of subaccount features for other currencies.
1739-1739: Good change: Broadened wallet access for fee estimation and removalChanging from
getOwnWallettogetAnyWalletallows more flexible wallet access patterns:
- Line 1739: Enables fee estimation for any wallet type (not just owned/modifiable wallets)
- Line 1829: Allows removal of any wallet type (including read-only wallets)
This is consistent with the functionality these operations require.
Also applies to: 1829-1829
1744-1752: LGTM: Improved fee estimation logicThe refactoring correctly:
- Extracts currency from the wallet info rather than hardcoding
- Uses the wallet's currency for fee estimation and dummy address lookup
- Maintains the same logic flow while being more generic
This supports multi-currency wallet operations introduced with Liquid support.
1925-1937: LGTM: Password encryption refactored for transaction supportThe
encryptWalletCredentialsfunction now properly accepts a transaction parameter, enabling it to be used within database transactions. The implementation correctly:
- Uses the passed transaction for database operations
- Maintains the same encryption logic
- Provides proper error handling
This supports the transactional wallet import process.
2056-2058: LGTM: Password change operation now transactionalThe password change operation is properly wrapped in a database transaction using
RunTx, ensuring atomicity when updating multiple wallet credentials. This is consistent with the transactional approach used elsewhere in the codebase.
2017-2017: LGTM: Using unified loginWallet methodThe call to
server.loginWallet(creds)uses the new unified method that handles both Liquid and Bitcoin wallets based on the credential type. This maintains consistency with the wallet login logic used elsewhere.
2382-2387: Excellent implementation: Unified wallet login methodThe new
loginWalletmethod provides clean abstraction for wallet creation:
- Correctly routes Liquid wallets (non-legacy) to the Liquid wallet implementation
- Falls back to the standard Bitcoin wallet implementation for legacy wallets
- Maintains type safety and clear separation of concerns
This is a well-designed solution for supporting multiple wallet backends while maintaining backward compatibility.
1462-1487: Deprecation of subaccount RPCs confirmed
BothSetSubaccountandGetSubaccounts(and their request/response messages, plus theSubaccounttype) are markedoption deprecated = trueinpkg/boltzrpc/boltzrpc.proto. The//nolint:staticcheckininternal/rpcserver/router.gois therefore appropriate.Key locations:
- pkg/boltzrpc/boltzrpc.proto:
- rpc SetSubaccount (SetSubaccountRequest) returns (Subaccount) { option deprecated = true; }
- rpc GetSubaccounts (GetSubaccountsRequest) returns (GetSubaccountsResponse) { option deprecated = true; }
- message SetSubaccountRequest { option deprecated = true; … }
- message GetSubaccountsRequest { option deprecated = true; … }
- message GetSubaccountsResponse { option deprecated = true; … }
- message Subaccount { option deprecated = true; … }
No further changes required.
michael1011
left a comment
michael1011
left a comment
There was a problem hiding this comment.
Waterfall working nicely?
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
04cade3 to
c4e6fc4
Compare
| if !wallet.Readonly { | ||
| return wallet, selectSubaccount(ctx, wallet.Id) | ||
| } | ||
| fmt.Printf("Wallet Balance: %s\n", utils.Satoshis(wallet.Balance.Total)) |
There was a problem hiding this comment.
In the GDK model, we had to choose a subaccount after importing to actually discover all accounts and see the balance.
We dont do that in LWK anymore. We import, in case of a mnemonic it derives the default descriptor (wpkh) and syncs to that - done.
The LWK implementation now allows populating both mnemonic and descriptor to allow cases where the default descriptor isn't the one the user wants. That option still has to be built into the CLI
There was a problem hiding this comment.
I mean why print the balance on import
There was a problem hiding this comment.
If the wallet has an existing balance I can see that directly here too
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
b2e3c40 to
a82b4cc
Compare
not needed anymore
important to not broadcast via boltz
this way, we can create the wallet inside of a transaction with the other db operations and have the heavier sync not block the db
want to be consistent with gdk
users will see descriptor in credentials of hot wallets this way
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
831d105 to
15627fa
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| runtime.GC() |
There was a problem hiding this comment.
Why run garbage collection manually?
* feat: implement `transaction.direct` status update * refactor: dont set mrh for external claim addresses this simplifies the handling of direct payments and gets rid of the problem where we cant verify the value of liquid transactions made to these transactions * fix: flaky gdk bump test
debugging leftover
jackstar12
force-pushed
the
feat/lwk-server-integration
branch
from
424cc08 to
ba3eb9b
Compare
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores