wallet+db: add initial version of wallet.Ark focused on boarding address/intents - #15
Conversation
Pull Request Review: Boarding Wallet ImplementationOverviewThis PR adds the initial version of Critical Issues1. JSON Serialization in Database (Acknowledged TODO)Location: The PR description explicitly mentions removing JSON from the database. This is correct - using JSON serialization for
2. Missing Context PropagationLocations: Multiple functions in All database methods use func (b *BoardingWalletStore) InsertBoardingAddress(
addr *round.BoardingAddress) error {
ctx := context.Background() // ❌ Should accept ctx as parameter
writeTxOpts := WriteTxOption()Impact:
Fix: Add 3. Error Log Level MisuseLocation: a.log.WarnS(ctx, "Failed to list unspent UTXOs", err,
slog.Int("height", int(epoch.Height)))Issue: According to CLAUDE.md, RPC failures should use lower log levels. This is correct as Recommendation: Ensure error is logged with proper key: 4. Integer Overflow RisksLocation: amount := int32(0)
intent.ChainInfo.WhenSome(func(info round.BoardingChainInfo) {
amount = int32(info.Amt) // ❌ btcutil.Amount is int64, truncates to int32
})Impact: Bitcoin amounts up to 21M BTC * 10^8 sats = 2.1 quadrillion sats exceed int32 max (2.1 billion). This will truncate large amounts. Fix: Change database schema to use 5. Incomplete Test Coverage for Edge CasesLocation: The tests are comprehensive but missing some critical scenarios:
Major Issues6. Missing NetworkParams in Address ParsingLocation: address, err := btcutil.DecodeAddress(
dbAddr.AddressString, nil, // ❌ nil network params
)Issue: Passing Fix: Store or inject 7. Unbounded Memory Growth in seenUtxosLocation: seenUtxos fn.Set[UtxoKey]Issue: The Impact: Long-running nodes could accumulate millions of entries Recommendation:
8. Insufficient Status Transition ValidationLocation: The status conversion functions use default fallback to "derived": default:
return "derived"Issue: Silently converting invalid statuses to "derived" hides bugs. Invalid status should be an error. Fix: Return error for unknown statuses or panic in development builds 9. Missing Transaction Isolation DocumentationLocation: The comment mentions "automatic retry on serialization errors" but doesn't document what isolation level is used. Concern: The UPSERT logic in Fix: Document the isolation level (likely SERIALIZABLE) and test concurrent scenarios 10. Hardcoded Constants Without ConfigurationLocation: MinBoardingConfs = 1
MaxConfsForListUnspent = 9999999Issue: These should be configurable, especially Recommendation: Move to configuration struct Minor Issues & Style Violations11. Comment Style - Missing Function Name PrefixLocation: Per CLAUDE.md: "Every function must have a comment starting with the function name" This is correctly followed throughout, but check consistency with multi-line comments. 12. Inconsistent Error WrappingLocations: Multiple Some errors use 13. TODO Comments for Production CodeLocation: Several TODO comments exist for incomplete features:
Concern: Some TODOs reference critical functionality. Clarify which are required before merge vs. future enhancements. 14. Redundant Comment in MigrationLocation: The documentation says "REMOVE THIS" above the JSON serialization section. Ensure this is cleaned up. 15. Missing Index on Creation TimeLocation: Both tables ORDER BY CREATE INDEX idx_boarding_addresses_creation ON boarding_addresses(creation_time DESC);
CREATE INDEX idx_boarding_intents_creation ON boarding_intents(creation_time DESC);16. Potential SQL Injection (Probably Safe)Location: The COALESCE logic in the UPSERT is correct, but ensure sqlc generates parameterized queries. (This is likely fine since sqlc handles it, but worth verifying.) Performance Considerations17. N+1 Query PatternLocation:
dbAddr, err := q.GetBoardingAddress(ctx, dbIntent.PkScript)Impact: If fetching 100 intents, this results in 100 separate address lookups Optimization:
18. Polling ListUnspent on Every BlockLocation: utxos, err := a.backend.ListUnspent(
ctx, MinBoardingConfs, MaxConfsForListUnspent,
)Concern: Calling Recommendation:
Security Concerns19. No Validation of Operator KeyLocation: The
20. Missing Tapscript ValidationLocation: When deserializing tapscripts from JSON, there's no validation that the tapscript matches the stored keys/parameters. Risk: Corrupt database could lead to invalid tapscripts being used Fix: Add validation in Positive Aspects✅ Excellent Documentation: The ✅ Proper Use of BatchedTx Pattern: Transaction handling follows the established pattern correctly ✅ Good Test Coverage: The test file has comprehensive scenarios for the happy path ✅ Structured Logging: Most logging uses structured fields correctly ( ✅ Idempotent Operations: UPSERT logic allows safe retries ✅ Actor Pattern: Clean separation of concerns with message-based communication RecommendationsMust Fix Before Merge:
Should Fix Before Merge:
Future Enhancements:
Testing RecommendationsBefore merge, ensure:
ConclusionThis is a solid foundation for the boarding wallet with excellent documentation and architecture. The main blockers are the context propagation, amount overflow, and clarification of the JSON removal plan. Once these are addressed, this will be ready for integration. The 60% slop mentioned in the PR description is appropriate - this establishes the core pipeline well. Great work on the structured approach! 🚀 Reviewed against: |
7a099e4 to
ea36590
Compare
|
@codex review /gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces the initial implementation of wallet.Ark along with the necessary database schema and persistence layer for managing boarding addresses and intents. The overall structure is well-designed, using an actor model for the wallet logic and a transactional database layer. The code is clean and includes a comprehensive set of tests.
My review focuses on improving context propagation, ensuring network parameter safety, making the code more robust against unexpected states, and aligning documentation with the implementation. Key suggestions include adding context.Context to store interface methods, properly handling network parameters during address decoding, and making status conversion functions stricter.
The changes are a great step towards the boarding wallet functionality.
| address, err := btcutil.DecodeAddress( | ||
| dbAddr.AddressString, nil, | ||
| ) |
There was a problem hiding this comment.
btcutil.DecodeAddress is called with nil for the network parameters, which causes it to default to chaincfg.MainNetParams. This can lead to incorrect address decoding if the application is running on another network like testnet or regtest. The network parameters should be configurable and passed to this function.
To fix this, you could add the *chaincfg.Params to the BoardingWalletStore, make dbAddrToDomainAddr a method on the store, and use the stored params. For example:
// In BoardingWalletStore struct:
// params *chaincfg.Params
// In dbAddrToDomainAddr method on BoardingWalletStore:
address, err := btcutil.DecodeAddress(
dbAddr.AddressString, b.params,
)| type BoardingStore interface { | ||
| // InsertBoardingAddress persists a boarding address when it is first | ||
| // created. This method is idempotent. | ||
| InsertBoardingAddress(addr *BoardingAddress) error | ||
|
|
||
| // LookupBoardingAddress retrieves a boarding address by its pkScript. | ||
| // Returns an error if the address is not found. | ||
| LookupBoardingAddress(pkScript []byte) (*BoardingAddress, error) | ||
|
|
||
| // ListAllBoardingAddresses returns all persisted boarding addresses. | ||
| ListAllBoardingAddresses() ([]*BoardingAddress, error) | ||
|
|
||
| // InsertBoardingIntents persists one or more boarding intents. | ||
| InsertBoardingIntents(intents ...BoardingIntent) error | ||
|
|
||
| // FetchBoardingIntents returns all boarding intents that are currently | ||
| // in progress (not yet completed). | ||
| FetchBoardingIntents() ([]BoardingIntent, error) | ||
|
|
||
| // FetchBoardingIntentsByStatus returns all boarding intents matching | ||
| // the given status. | ||
| FetchBoardingIntentsByStatus( | ||
| status BoardingStatus, | ||
| ) ([]BoardingIntent, error) | ||
|
|
||
| // GetIntent retrieves a boarding intent by its outpoint (primary key). | ||
| GetIntent(outpoint wire.OutPoint) (*BoardingIntent, error) | ||
|
|
||
| // LookupIntentByScript returns the stored intent associated with a | ||
| // boarding pkScript. | ||
| LookupIntentByScript(pkScript []byte) (*BoardingIntent, error) | ||
| } |
There was a problem hiding this comment.
The methods in the BoardingStore interface perform database I/O and should accept a context.Context as their first argument. This allows for proper cancellation and deadline propagation from the caller (e.g., the Ark actor) down to the database. The current implementation in db/boarding_wallet.go is forced to use context.Background(), which is an anti-pattern for request-scoped operations.
| default: | ||
| return "confirmed" | ||
| } |
There was a problem hiding this comment.
The default case in statusToString returns "confirmed". This can mask programming errors, for instance if a new BoardingStatus is added but this function is not updated. It would be safer to panic to fail fast in such a case.
| default: | |
| return "confirmed" | |
| } | |
| default: | |
| panic(fmt.Sprintf("unknown boarding status: %v", status)) |
| default: | ||
| return wallet.BoardingStatusConfirmed | ||
| } |
There was a problem hiding this comment.
The default case in stringToStatus returns wallet.BoardingStatusConfirmed. This can mask database corruption or inconsistent data, where an invalid status string would be silently treated as "confirmed". It would be safer to panic to fail fast.
| default: | |
| return wallet.BoardingStatusConfirmed | |
| } | |
| default: | |
| panic(fmt.Sprintf("unknown boarding status string: %s", status)) |
| ```mermaid | ||
| erDiagram | ||
| boarding_statuses ||--o{ boarding_intents : "enforces" | ||
| boarding_addresses ||--o{ boarding_intents : "has" | ||
|
|
||
| boarding_statuses { | ||
| BIGINT id "PK" | ||
| TEXT status_name "UK" | ||
| } | ||
|
|
||
| boarding_addresses { | ||
| BLOB pk_script "PK" | ||
| TEXT address_string | ||
| BLOB client_pubkey | ||
| INTEGER client_key_family | ||
| INTEGER client_key_index | ||
| BLOB operator_pubkey | ||
| INTEGER exit_delay | ||
| BLOB tapscript_data "JSON" | ||
| INTEGER last_confirmed_height "Idx, Default=0" | ||
| BIGINT creation_time | ||
| } | ||
|
|
||
| boarding_intents { | ||
| BLOB outpoint_hash "PK" | ||
| INTEGER outpoint_index "PK" | ||
| BLOB pk_script "FK, Idx" | ||
| INTEGER amount | ||
| INTEGER conf_height "Idx" | ||
| BLOB conf_hash | ||
| BLOB conf_tx | ||
| TEXT status "FK, Idx" | ||
| TEXT round_id "Idx" | ||
| BLOB vtxo_template_data "JSON" | ||
| BIGINT creation_time | ||
| BIGINT last_update_time | ||
| } | ||
| ``` | ||
|
|
||
| ## Table Details | ||
|
|
||
| ### boarding_statuses | ||
|
|
||
| An enumeration table enforcing valid boarding intent lifecycle states through a | ||
| foreign key constraint. | ||
|
|
||
| **Lifecycle states**: | ||
| - `derived` (0): Boarding address created, no on-chain funding yet | ||
| - `waiting` (1): On-chain funding detected, awaiting confirmations | ||
| - `confirmed` (2): Sufficient confirmations received, ready for round | ||
| - `adopted` (3): Included in a round that has been checkpointed | ||
| - `failed` (4): Boarding attempt failed (server rejection, timeout, etc.) | ||
| - `complete` (5): Successfully boarded, VTXOs created | ||
|
|
||
| This table is static after migration and enforces type safety at the database | ||
| level. | ||
|
|
||
| ### boarding_addresses | ||
|
|
||
| Stores boarding addresses that have been created and imported into the LND | ||
| wallet. Each address represents a unique 2-of-2 taproot script between the | ||
| client and operator with a CSV timelock for client recovery. | ||
|
|
||
| **Key fields**: | ||
| - `pk_script`: The raw P2TR output script, serves as primary key since it | ||
| uniquely identifies an address | ||
|
|
||
| - `client_pubkey`, `client_key_family`, `client_key_index`: The client's key | ||
| and its BIP32 derivation path for later signing | ||
|
|
||
| - `operator_pubkey`: The operator's public key used in collaborative spends | ||
|
|
||
| - `exit_delay`: CSV delay in blocks for the client's unilateral timeout path | ||
|
|
||
| - `tapscript_data`: JSON-serialized `waddrmgr.Tapscript` containing the full | ||
| taproot script tree | ||
|
|
||
| - `last_confirmed_height`: The most recent block height at which we detected a | ||
| UTXO at this address, used for restart recovery to resume monitoring from the | ||
| last known checkpoint | ||
|
|
||
| **Indexes**: | ||
| - `idx_boarding_addresses_last_confirmed`: Enables efficient queries during | ||
| startup to identify addresses needing monitoring from specific heights | ||
|
|
||
| **Usage pattern**: Addresses are created once and reused. Multiple boarding | ||
| intents can reference the same address if a user sends funds to it multiple | ||
| times. | ||
|
|
||
| ### boarding_intents | ||
|
|
||
| Tracks individual boarding attempts from on-chain confirmation through | ||
| completion. Each intent represents one boarding UTXO's journey through the | ||
| round coordination process. | ||
|
|
||
| **Key fields**: | ||
| - `outpoint_hash`, `outpoint_index`: Composite primary key uniquely identifying | ||
| the boarding UTXO | ||
|
|
||
| - `pk_script`: Foreign key to `boarding_addresses`, linking this intent to its | ||
| address | ||
|
|
||
| - `amount`: Value of the boarding UTXO in satoshis (learned from on-chain | ||
| detection) | ||
|
|
||
| - `conf_height`, `conf_hash`, `conf_tx`: Confirmation details, NULL until the | ||
| UTXO is detected on-chain | ||
| - `status`: Current lifecycle state (foreign key to `boarding_statuses`) | ||
|
|
||
| - `round_id`: Round identifier, NULL until the intent is assigned to a round | ||
|
|
||
| - `vtxo_template_data`: JSON-serialized array of `types.VTXORequest` structs | ||
| specifying the desired VTXO outputs | ||
|
|
||
| - `creation_time`: Unix epoch timestamp when intent was first created | ||
|
|
||
| - `last_update_time`: Unix epoch timestamp of the last status change | ||
|
|
||
| **Indexes**: | ||
| - `idx_boarding_intents_pk_script`: Enables efficient lookup of all intents for | ||
| a given address | ||
|
|
||
| - `idx_boarding_intents_status`: Supports queries like "fetch all confirmed | ||
| intents" | ||
|
|
||
| - `idx_boarding_intents_conf_height`: Enables backlog delivery by height range | ||
|
|
||
| - `idx_boarding_intents_round_id`: Allows querying all intents in a specific | ||
| round | ||
|
|
||
| **Upsert semantics**: Intents are inserted with `ON CONFLICT` handling to | ||
| enable progressive updates. When an intent is re-inserted: | ||
|
|
||
| - `status` is always updated (allows progression through lifecycle) | ||
| - `amount` uses COALESCE to preserve non-zero values | ||
| - `conf_height`, `conf_hash`, `conf_tx` use COALESCE to preserve once set | ||
| - `round_id` uses COALESCE to preserve once assigned | ||
| - `last_update_time` is always updated to track modifications | ||
|
|
||
| ## Operational Logic | ||
|
|
||
| ### Address Creation Flow | ||
|
|
||
| 1. Wallet actor derives a new key using `DeriveNextKey(family=42)` | ||
| 2. Constructs 2-of-2 tapscript with operator key and CSV timelock | ||
| 3. Imports tapscript into LND via `ImportTaprootScript` | ||
| 4. Persists to `boarding_addresses` with `last_confirmed_height=0` | ||
| 5. Returns address to caller | ||
|
|
||
| **Restart recovery**: On startup, `ListAllBoardingAddresses()` retrieves all | ||
| addresses and their `last_confirmed_height` values, enabling the actor to | ||
| resume monitoring from known checkpoints. | ||
|
|
||
| ### UTXO Detection Flow | ||
|
|
||
| 1. On each new block, wallet actor calls LND's `ListUnspent` | ||
|
|
||
| 2. Filters results to only UTXOs paying to boarding addresses (checks | ||
| `pk_script` against `boarding_addresses`) | ||
|
|
||
| 3. Deduplicates using in-memory `fn.Set[UtxoKey]` (loaded from existing intents | ||
| at startup) | ||
|
|
||
| 4. For new UTXOs, inserts `boarding_intent` with status=`confirmed` and full | ||
| chain info | ||
|
|
||
| 5. Notifies registered actors (e.g., round actor) via `BoardingUtxoConfirmedEvent` | ||
|
|
||
| 6. Updates address's `last_confirmed_height` to the confirmation block height | ||
|
|
||
| **Deduplication**: The in-memory `seenUtxos` set prevents duplicate | ||
| notifications. On restart, this set is repopulated by loading all existing | ||
| intents. | ||
|
|
||
| ### Intent Lifecycle Progression | ||
|
|
||
| Typical lifecycle: `derived → waiting → confirmed → adopted → complete` | ||
|
|
||
| - **derived**: Address exists but no funding detected (not currently used by | ||
| wallet actor; reserved for future) | ||
|
|
||
| - **waiting**: On-chain funding detected but insufficient confirmations (not | ||
| currently used; wallet waits for MinBoardingConfs) | ||
|
|
||
| - **confirmed**: UTXO has sufficient confirmations, ready for round inclusion | ||
| (wallet actor creates intents in this state) | ||
|
|
||
| - **adopted**: Round actor has included this intent in a round and checkpointed | ||
| the FSM state | ||
|
|
||
| - **complete**: Round confirmed, VTXOs created, boarding successful | ||
|
|
||
| - **failed**: Error occurred (server rejection, timeout, etc.), may be | ||
| recoverable via CSV path | ||
|
|
||
| ### Round Assignment | ||
|
|
||
| When the round actor receives a `BoardingUtxoConfirmedEvent`: | ||
|
|
||
| 1. Queries `FetchBoardingIntentsByStatus(confirmed)` to get available intents | ||
|
|
||
| 2. Selects intent(s) to include in the next round | ||
|
|
||
| 3. Updates status to `adopted` and sets `round_id` via `InsertBoardingIntents` | ||
| (upsert) | ||
|
|
||
| 4. Proceeds with MuSig2 signing ceremonies | ||
|
|
||
| 5. On round confirmation, updates status to `complete` | ||
|
|
||
| ### Query Patterns | ||
|
|
||
| **Balance calculation**: Query `SumBoardingIntentAmountsByStatus(confirmed)` to | ||
| get total funds ready for boarding. | ||
|
|
||
| **Active intents**: Use `FetchBoardingIntents()` to get all non-complete | ||
| intents for UI display. | ||
|
|
||
| **Backlog delivery**: When an actor registers late, query | ||
| `ListBoardingIntentsByConfHeight(fromHeight)` to send recent confirmations, | ||
| enabling the actor to catch up after restart. | ||
|
|
||
| ## JSON Serialization | ||
|
|
||
| Two fields use JSON serialization for complex types: | ||
|
|
||
| 1. **tapscript_data**: Stores `waddrmgr.Tapscript` struct including control | ||
| blocks, leaves, and root hashes | ||
|
|
||
| 2. **vtxo_template_data**: Stores `[]types.VTXORequest` array with amounts, | ||
| pkScripts, expiries, and signing keys | ||
|
|
||
| JSON is used because these types have nested structures (public keys, key | ||
| locators) that don't map cleanly to flat database columns. The serialization is | ||
| handled by Go's standard `encoding/json` package. | ||
|
|
||
| **REMOVE THIS** |
There was a problem hiding this comment.
The database schema documentation in this file appears to be out of sync with the actual implementation in db/sqlc/migrations/000002_boarding_tables.up.sql in several places:
- The ER diagram and "Table Details" section mention fields like
tapscript_data,round_id, andvtxo_template_datawhich are not present in the SQL schema. - The "Lifecycle states" section lists states (
derived,waiting,complete) that are not in theBoardingStatusenum or theboarding_statusestable, and omits states that are present (expired,swept). - There is a
**REMOVE THIS**marker on line 263.
Please update the documentation to accurately reflect the current schema and remove the marker.
| // TODO(roasbeef): Use actual tapscript construction from lib/scripts. | ||
| // For now, this is a placeholder that constructs a simple 2-of-2 | ||
| // tapscript with CSV timeout. The real implementation should use | ||
| // lib/scripts/vtxo.go or similar to build proper boarding scripts. |
There was a problem hiding this comment.
This TODO comment appears to be outdated. The code calls buildBoardingTapscript, which in turn correctly uses scripts.VTXOTapScript to construct the tapscript. The implementation seems to be complete. Please remove the TODO.
|
@claude review this |
|
Claude finished @Roasbeef's task —— View job Code Review: Boarding Wallet ImplementationI've completed a comprehensive review of this PR. Overall, this is a well-structured implementation with excellent documentation and good use of the actor pattern. However, there are several critical issues that must be addressed before merging. 🔴 Critical Issues (Must Fix)1. Missing Context Propagation in Database LayerLocation: All Impact:
Fix Required: Add InsertBoardingAddress(ctx context.Context, addr *BoardingAddress) error
LookupBoardingAddress(ctx context.Context, pkScript []byte) (*BoardingAddress, error)
// ... etc for all methodsThen update 2. Missing Network Parameters in Address DecodingLocation: address, err := btcutil.DecodeAddress(
dbAddr.AddressString, nil, // ❌ nil network params
)Impact: Passing Fix Required:
3. Critical Data Loss: UTXOs Not PersistedLocation: The func (a *Ark) processUtxo(ctx context.Context,
epoch chainsource.BlockEpoch, utxo *Utxo) {
// ... checks if UTXO is new and belongs to boarding address ...
// Marks as seen in memory
a.seenUtxos.Add(key)
// Notifies actors
for _, notifier := range a.notifiers {
notifier.Tell(ctx, event)
}
// ❌ NEVER CALLS store.InsertBoardingIntents()
}Impact:
This is flagged by the chatgpt-codex-connector review comment and is a critical bug. Fix Required: Add 4. Incorrect Default in Status Conversion FunctionsLocations: Both default:
return "confirmed" // ❌ Masks programming errorsImpact:
Fix Required: Change to 5. Outdated TODO CommentLocation: // TODO(roasbeef): Use actual tapscript construction from lib/scripts.
// For now, this is a placeholder that constructs a simple 2-of-2
// tapscript with CSV timeout.Issue: This TODO is outdated. Lines 179-181 correctly call Fix Required: Remove the TODO comment entirely.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, notifier := range a.notifiers { | ||
| notifier.Tell(ctx, event) | ||
| } | ||
| } |
There was a problem hiding this comment.
In processUtxo the actor marks a detected boarding output as seen and notifies tell-only refs, then returns without ever writing the intent to the BoardingStore (lines 390-393). Because the only persistence hook (InsertBoardingIntents) is never invoked, confirmed boarding outputs are only kept in memory: sendBacklog and GetBoardingBalance will always read an empty set after startup, and a restart will resend the same UTXOs because seenUtxos is rebuilt solely from stored intents. The detected UTXOs need to be inserted into the store when first observed to make balance/backlog/restart flows work.
Useful? React with 👍 / 👎.
ea36590 to
a934fe0
Compare
b0e00ca to
5954e2a
Compare
PR Review: Boarding Wallet ImplementationExcellent work on this initial implementation! The code is well-structured, follows good patterns, and has comprehensive test coverage. Below is my detailed review covering code quality, potential issues, performance, security, and test coverage. Overall AssessmentStrengths:
Areas for Improvement: Code Quality & Best Practices✅ Good Patterns Observed
🔧 Issues & Suggestions1. CRITICAL: Missing Root Hash Population in VTXOTapScript ✅ FIXEDLocation: lib/scripts/vtxo.go:104-128 The original code had: tapscript := input.TapscriptFullTree(
&ARKNUMSKey, collabLeaf, timeoutLeaf,
)The fix: c901d09 properly sets the root hash: tree := txscript.AssembleTaprootScriptTree(tapscript.Leaves...)
rootHash := tree.RootNode.TapHash()
tapscript.RootHash = rootHash[:]✅ This issue has been addressed in the first commit of the PR. 2. Race Condition: Concurrent Map AccessLocation: wallet/wallet.go:404 for _, notifier := range a.notifiers {
notifier.Tell(ctx, event)
}Issue: The Recommendation: Either:
3. Silent Error Handling in processUtxoLocation: wallet/wallet.go:360-364 addr, err := a.store.LookupBoardingAddress(ctx, utxo.PkScript)
if err != nil {
// Not a boarding address, ignore.
return
}Issue: This silently ignores ALL errors, not just "not found". Database errors will be hidden. Recommendation: addr, err := a.store.LookupBoardingAddress(ctx, utxo.PkScript)
if err != nil {
if err == sql.ErrNoRows {
// Not a boarding address, ignore.
return
}
a.log.WarnS(ctx, "Failed to lookup boarding address", err,
btclog.Hex("pk_script", utxo.PkScript))
return
}4. Panic on Invalid Status StringsLocation: db/boarding_wallet.go:499, 518 func stringToStatus(status string) wallet.BoardingStatus {
switch status {
// ...
default:
panic(fmt.Sprintf("unknown boarding status string: %q", status))
}
}Issue: Panicking on database read can crash the entire application. If the database is corrupted or has unexpected data, this is fatal. Recommendation: Return an error instead and handle gracefully: func stringToStatus(status string) (wallet.BoardingStatus, error) {
switch status {
// ...
default:
return 0, fmt.Errorf("unknown boarding status string: %q", status)
}
}Then propagate the error through the call chain. 5. Incomplete TODOLocation: wallet/wallet.go:255 // TODO(roasbeef): Add support for querying all statuses.Issue: The Recommendation: Track this in an issue if not addressing in this PR. Performance Considerations✅ Good Performance Patterns
💡 Performance Suggestions1. Polling ListUnspent on Every BlockLocation: wallet/wallet.go:327-329 Concern: For wallets with many imported scripts, Mitigation Options:
Current verdict: Acceptable for initial implementation, but monitor performance. 2. Backlog Delivery Loads All Confirmed IntentsLocation: wallet/wallet.go:418-419 intents, err := a.store.FetchBoardingIntentsByStatus(
ctx, BoardingStatusConfirmed,
)Issue: If there are thousands of confirmed intents, this loads them all into memory then filters by height. Recommendation: Add a database query that filters by both status AND height: -- name: ListBoardingIntentsByStatusAndHeight :many
SELECT * FROM boarding_intents
WHERE status = $1 AND conf_height >= $2
ORDER BY conf_height DESC;Security Concerns✅ Security Strengths
🔒 Security Recommendations1. Validation: Exit Delay BoundsLocation: wallet/wallet.go:164-176 Issue: No validation that Recommendation: const (
MinBoardingExitDelay = 144 // ~1 day
MaxBoardingExitDelay = 2016 // ~2 weeks
)
// In handleCreateBoardingAddress:
if req.ExitDelay < MinBoardingExitDelay || req.ExitDelay > MaxBoardingExitDelay {
return fn.Err[WalletResp](fmt.Errorf("exit delay %d outside valid range [%d, %d]",
req.ExitDelay, MinBoardingExitDelay, MaxBoardingExitDelay))
}2. Validation: Operator KeyLocation: wallet/wallet.go:164 Issue: No validation that Recommendation: if req.OperatorKey == nil {
return fn.Err[WalletResp](fmt.Errorf("operator key is required"))
}
if req.OperatorKey.IsEqual(zeroKey) {
return fn.Err[WalletResp](fmt.Errorf("operator key cannot be zero"))
}3. Amount Overflow CheckLocation: wallet/wallet.go:266 var totalBalance btcutil.Amount
for _, intent := range intents {
totalBalance += intent.ChainInfo.Amount
}Issue: If there are enough UTXOs, Recommendation: Check for overflow or use a safer type: var totalBalance int64
for _, intent := range intents {
newTotal := totalBalance + int64(intent.ChainInfo.Amount)
if newTotal < totalBalance {
return fn.Err[WalletResp](fmt.Errorf("balance overflow"))
}
totalBalance = newTotal
}Test Coverage✅ Excellent Test Coverage
💡 Test Coverage Suggestions1. Missing: Concurrent Notification TestAdd a test that registers/unregisters notifiers while UTXOs are being processed to catch potential race conditions. 2. Missing: Large Dataset TestTest behavior with 1000+ boarding intents to validate performance assumptions. 3. Missing: Error Recovery TestTest what happens when the database transaction fails partway through (rollback behavior). 4. Missing: Edge Cases
Database Schema✅ Schema Strengths
🔧 Schema Suggestions1. Consider: Add ConstraintsLocation: migrations/000002_boarding_tables.up.sql Currently there are no CHECK constraints. Consider adding: ALTER TABLE boarding_addresses ADD CONSTRAINT check_exit_delay_positive
CHECK (exit_delay > 0);
ALTER TABLE boarding_intents ADD CONSTRAINT check_amount_positive
CHECK (amount > 0);
ALTER TABLE boarding_intents ADD CONSTRAINT check_conf_height_nonnegative
CHECK (conf_height >= 0);2. Consider: Address ExpiryAs mentioned in boarding_wallet.md:258, consider adding Documentation✅ Excellent DocumentationThe
Recommendation: Consider also adding:
Commit Quality✅ Commit Message ExcellenceAll commits follow the project's conventions:
Example of excellent commit: Summary of Action ItemsHigh Priority
Medium Priority
Low Priority (Future Enhancements)
Final RecommendationAPPROVE with minor changes recommended This is a solid foundation for the boarding wallet system. The architecture is sound, the code quality is high, and the test coverage is comprehensive. The critical bug in VTXOTapScript has already been fixed. The main concerns are:
These are relatively minor issues that can be addressed in follow-up commits or as feedback is incorporated. Great work overall! 🚀 |
5954e2a to
25521a0
Compare
a934fe0 to
4540c2a
Compare
Code Review for PR #15: wallet+db Initial Boarding ImplementationI've completed a thorough review of this PR. Overall, this is excellent work with strong architecture, comprehensive testing, and good adherence to the project's style guidelines. The code is production-ready with only minor suggestions for improvement. ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Issues Found1. Minor: Potential Race Condition (wallet/wallet.go:289-293)The sendBacklog goroutine runs asynchronously and accesses a.store, but the actor's Receive method is not designed to be called concurrently. This could cause race conditions if sendBacklog runs while another message is being processed. Recommendation: Either document that BoardingStore must be thread-safe (it is, via BatchedTx) OR queue backlog delivery as an actor message instead of spawning a goroutine 2. Minor: Missing Validation (wallet/wallet.go:176-179)The CreateBoardingAddressRequest doesn't validate that OperatorKey is non-nil or ExitDelay meets minimum requirements. Recommendation: Add validation at the message handler level for these fields. 3. Minor: Inefficient Query (wallet/wallet.go:245-249)The code fetches all intents from the database and sums in Go. The SQL already has SumBoardingIntentAmountsByStatus which does this efficiently in the database. Recommendation: Either use the existing query or remove it if unused. 4. Style: 80-character Line Limit (Multiple locations)Several lines exceed 80 characters. While the guidelines say "best effort," consider wrapping these for consistency:
5. Error Handling Observation (wallet/wallet.go:334)✅ Correct usage! The code properly uses warn level for external errors (LND RPC failures), not error level. This follows CLAUDE.md guidelines. Good job! 🛡️ Security Considerations✅ Good Practices Observed
|
| Guideline | Status | Notes |
|---|---|---|
| Structured logging | ✅ Pass | Uses InfoS/WarnS/DebugS correctly |
| Error log levels | ✅ Pass | External errors use warn, not error |
| Function comments | ✅ Pass | All functions have proper comments |
| 8-space tabs | Assumes .editorconfig is used | |
| 80-char lines | A few lines exceed limit | |
| Error wrapping | ✅ Pass | Consistent use of fmt.Errorf with %w |
| Code generation | ✅ Pass | sqlc-generated code committed separately |
🎯 Recommendations
Before Merge
- Fix: Add input validation for CreateBoardingAddressRequest
- Consider: Change sendBacklog to use actor message instead of goroutine
- Consider: Use SumBoardingIntentAmountsByStatus query or remove it
Follow-up PRs (Non-blocking)
- Add MinBoardingAmount constant and validation
- Implement pruning for seenUtxos set
- Add integration test for actor restart recovery
- Document address reuse behavior and operator trust model
- Consider adding metrics/instrumentation for monitoring
📝 Minor Nits
- db/boarding_wallet.go:63: Type alias comment could clarify "This enables atomic operations across multiple queries" → "This enables transaction-safe atomic operations"
- wallet/wallet.go:56: Consider renaming seenUtxos to processedUtxos for clarity
- wallet/messages.go:230: The embedded *BoardingIntent is clever but can make nil checks tricky - ensure callers never pass nil
🏁 Conclusion
Recommendation: APPROVE with minor changes
This PR demonstrates excellent software engineering practices:
- Clean architecture with proper separation of concerns
- Comprehensive testing approaching 90% coverage goal
- Strong adherence to project style guidelines
- Thoughtful database design with performance optimization
- Excellent documentation
The issues identified are minor and mostly non-blocking. The code is production-ready with the suggested input validation added.
Great work! 🚀
Reviewed with reference to: docs/development_guidelines.md, CLAUDE.md, and repository conventions.
4540c2a to
1c1bf2d
Compare
Code Review - PR #15: Boarding Wallet ImplementationThis is a solid initial implementation with good separation of concerns and proper database patterns. StrengthsArchitecture & Design
Database Schema
Code Quality
Issues & SuggestionsCritical Issues1. Missing Transaction Fetch (wallet/wallet.go:379-389) 2. Log Level Violation (wallet/wallet.go:334) 3. Missing Address Update (wallet/wallet.go:363-403) Code Quality Issues4. Context in Goroutine (wallet/wallet.go:290-293) 5. Inconsistent Error Handling (wallet/wallet.go:390-394) 6. Magic Number (wallet/wallet.go:22) Performance7. Inefficient Address Lookup (wallet/wallet.go:363) 8. Unbounded Notifier Map (wallet/wallet.go:52) Testing9. Missing Integration Tests 10. No UPSERT Test Documentation11. Incomplete Function Comments 12. Missing Package Comment Security13. No Input Validation SummaryOverall: High-quality work demonstrating strong understanding of Go idioms, database patterns, and actor model. Priority Fixes:
Recommended:
Great work! The PR aligns well with codebase conventions. |
| tree := txscript.AssembleTaprootScriptTree(tapscript.Leaves...) | ||
| rootHash := tree.RootNode.TapHash() | ||
| tapscript.RootHash = rootHash[:] |
There was a problem hiding this comment.
is this something that should be fixed upstream in AssembleTaprootScriptTree?
There was a problem hiding this comment.
So The issue isn't AssembleTaprootScriptTree, it's actually the lnd utility used here input.TapscriptFullTree, it just doesn't set RootHash at all: https://github.com/lightningnetwork/lnd/blob/c746aeed31c7f528e9cff3f57d5044892b2a02c9/input/taproot.go#L102-L111
This migration introduces three tables for managing boarding addresses and tracking the boarding intent lifecycle from on-chain confirmation through VTXO creation. The boarding_statuses table enforces valid state transitions at the database level using foreign key constraints. The six lifecycle states (derived, waiting, confirmed, adopted, failed, complete) are populated during migration and remain static thereafter. The boarding_addresses table stores generated taproot addresses with their cryptographic material (client/operator keys, tapscript data) and monitoring metadata. Addresses are created once and can be reused for multiple boarding attempts. The boarding_intents table tracks individual boarding UTXOs through their lifecycle, including confirmation data, round assignments, and VTXO templates. Intents use ON CONFLICT DO UPDATE semantics to enable progressive status updates while preserving once-set fields like confirmation data. All timestamps use BIGINT storing unix epoch seconds to avoid timezone and serialization issues across different database backends. The gen_sqlc_docker.sh script automatically patches these to INTEGER for SQLite compatibility while maintaining int64 types in generated Go code.
These queries provide type-safe database access for the boarding wallet store implementation. The queries support the full lifecycle of boarding operations including address creation, intent tracking, status filtering, and backlog delivery. Insert operations use ON CONFLICT DO UPDATE to enable idempotent upserts as intents progress through their lifecycle. The COALESCE pattern preserves already-set fields like confirmation data and round assignments while allowing status updates. Query operations support various filters including status-based lookups for finding confirmed intents ready for boarding, height- based queries for delivering backlogs to late-registering actors, and pk_script-based lookups for associating UTXOs with their addresses.
Generated type-safe Go code from the boarding wallet schema and queries using sqlc 1.29.0. The generated code includes proper handling of BIGINT timestamps as int64 fields and the boarding status enum table with foreign key relationships. The Querier interface now includes all boarding-related methods for address and intent management, enabling the BatchedTx pattern in the store implementation.
1c1bf2d to
3b52fa6
Compare
Code Review: PR #15 - Boarding Wallet ImplementationI've reviewed this PR focusing on code quality, architecture, testing, and adherence to the project's development guidelines. Overall, this is a solid implementation with excellent test coverage and documentation. Here are my findings: ✅ StrengthsArchitecture & Design
Database Design
Testing
Code Quality
🔍 Issues & RecommendationsCritical Issues1. Missing // In VTXOTapScript (lib/scripts/vtxo.go:120-126)
tapscript := input.TapscriptFullTree(
&ARKNUMSKey, collabLeaf, timeoutLeaf,
)
// Compute and set the root hash since TapscriptFullTree doesn't populate it
tree := txscript.AssembleTaprootScriptTree(tapscript.Leaves...)
rootHash := tree.RootNode.TapHash()
tapscript.RootHash = rootHash[:]✅ This is correct - 2. Error Log Level Usage (wallet/wallet.go:365-370) if err != nil {
a.log.WarnS(ctx, "Failed to list unspent UTXOs", err,
slog.Int("height", int(epoch.Height)))
// Return success to avoid disrupting the actor
return fn.Ok[WalletResp](nil)
}✅ Correct: This properly uses 3. Database Intent Persistence Error Handling (wallet/wallet.go:421-425) err = a.store.InsertBoardingIntents(ctx, intent)
if err != nil {
a.log.WarnS(ctx, "Failed to persist boarding intent", err,
btclog.Fmt("outpoint", "%v", utxo.Outpoint))
}
// Continues to notify even if persist fails
Recommendation: Either:
Current behavior may cause duplicate notifications after restart. Code Style Issues4. Switch/Select Case Spacing (wallet/wallet.go:167-189) switch m := msg.(type) {
case *CreateBoardingAddressRequest:
return a.handleCreateBoardingAddress(ctx, m)
case *GetActiveBoardingAddressesRequest:
return a.handleGetActiveBoardingAddresses(ctx, m)
// ... more cases
}❌ Style violation: According to CLAUDE.md and 5. Struct Literal Formatting (db/boarding_wallet.go:103-119) 6. Line Length (Multiple Files) Minor Issues7. Magic Numbers const (
BoardingKeyFamily = 42 // Good: Named constant
MinBoardingConfs = 1
MaxConfsForListUnspent = 9999999 // Consider: Could be MaxInt32?
)Minor: 8. BIGINT vs INTEGER in Migration 9. Missing TODO Completion Verification
Both TODOs appear completed based on code review. The store does use JSON for VTXO templates (mentioned in commit message for ff81837), but this is for "nested structures that don't map cleanly to flat columns" which is acceptable. 🔒 Security Considerations✅ No security issues identified:
📊 Test CoverageDatabase tests (
Wallet actor tests (
Estimated coverage: Near 90% for new code ✅ 🎯 RecommendationsBefore Merge:
Nice-to-Have:
📝 Commit QualityCommits follow the project's guidelines well:
SummaryThis is high-quality work with excellent architecture, comprehensive testing, and thorough documentation. The main issues are:
Once the ast-grep style issues are fixed and the DB error handling is addressed, this PR should be ready to merge. Great job on the implementation! The message-driven architecture with the actor pattern is clean, the database schema is well-designed, and the documentation is exemplary. 🎉 |
3b52fa6 to
88a41ec
Compare
Code Review - PR #15: wallet.Ark Initial ImplementationOverviewThis PR introduces the initial boarding wallet infrastructure with database persistence, address management, and UTXO monitoring. The implementation is well-structured with strong separation of concerns and comprehensive documentation. Overall quality is high with excellent adherence to repository conventions. ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Issues & RecommendationsCritical Issues1. Missing validation in address creation (db/boarding_wallet.go:108-121)The Recommendation: Add validation to verify the reconstructed tapscript matches the provided pkScript: // After line 108, before params creation:
reconstructedTapscript, err := scripts.VTXOTapScript(
addr.KeyDesc.PubKey, addr.OperatorKey, addr.ExitDelay,
)
if err != nil {
return fmt.Errorf("validate tapscript: %w", err)
}
reconstructedPkScript, err := txscript.PayToAddrScript(addr.Address)
if err != nil {
return fmt.Errorf("reconstruct pk script: %w", err)
}
if !bytes.Equal(reconstructedPkScript, pkScript) {
return fmt.Errorf("pkScript mismatch: address inconsistent with keys")
}2. Race condition in seenUtxos access (wallet/wallet.go:67, 388-390, 430)The Recommendation: Either verify // Add to Ark struct:
seenUtxosMu sync.RWMutex
// Wrap accesses:
a.seenUtxosMu.RLock()
exists := a.seenUtxos.Contains(key)
a.seenUtxosMu.RUnlock()3. Database transaction deadlock risk (wallet/wallet.go:320-324)The Recommendation: Consider rate limiting or using a worker pool for backlog delivery: // Use a semaphore to limit concurrent backlog operations
backlogSem := make(chan struct{}, 5) // Max 5 concurrent backlogsHigh Priority Issues4. Missing index on composite status + conf_height query (db/sqlc/migrations/000002_boarding_tables.up.sql:119-121)The query Recommendation: Add a composite index: CREATE INDEX IF NOT EXISTS idx_boarding_intents_status_conf_height
ON boarding_intents(status, conf_height DESC);5. Log level violation in handleBlockEpoch (wallet/wallet.go:365-366)Per CLAUDE.md guidelines, external failures (LND RPC errors) should use Recommendation: Change to info level: a.log.InfoS(ctx, "Failed to list unspent UTXOs, will retry next block",
slog.Int("height", int(epoch.Height)))6. Potential nil pointer dereference (wallet/wallet.go:434-435)
Recommendation: Use value type instead of pointer embedding or add nil checks: // In messages.go:229-236
type BoardingUtxoConfirmedEvent struct {
actor.BaseMessage
BoardingIntent BoardingIntent // Value type instead of pointer
}Medium Priority Issues7. Incomplete error context in dbIntentToDomainIntent (db/boarding_wallet.go:501-510)The error returned from Recommendation: return nil, fmt.Errorf("lookup boarding address for pk_script %x: %w",
dbIntent.PkScript, err)8. Missing bounds check in processUtxo (wallet/wallet.go:437)Cast Recommendation: if utxo.Confirmations < 0 {
a.log.WarnS(ctx, "Negative confirmation count",
btclog.Fmt("outpoint", "%v", utxo.Outpoint))
return
}9. Magic number in MaxConfsForListUnspent (wallet/wallet.go:30)The constant Recommendation: Use a more reasonable value like // MaxConfsForListUnspent is effectively "no upper bound" for ListUnspent.
// Limited to 9999999 to avoid potential integer overflow in SQLite/Postgres.
MaxConfsForListUnspent = 9_999_99910. Missing test for status transitions (wallet/wallet_test.go)Tests verify insert/fetch but don't test invalid status transitions or status enum validation. Recommendation: Add a test case: func TestBoardingIntentStatusTransitions(t *testing.T) {
// Test invalid status string rejection
// Test valid lifecycle: confirmed -> adopted -> swept
// Test error paths: confirmed -> failed -> expired
}Low Priority / Style11. Inconsistent comment style in SQL schema (db/sqlc/migrations/000002_boarding_tables.up.sql:28-29)Some table comments use inline style, others use block comments above the CREATE statement. Recommendation: Standardize on block comments above tables per Go style guide. 12. Missing MessageType godoc (wallet/messages.go:43-45)All message types implement Recommendation: Add package-level documentation explaining the message type pattern. 🔒 Security AssessmentStrengths
Concerns
📊 Performance ConsiderationsPositive
Concerns
📝 Test Coverage NotesWell Covered
Missing Coverage
🎯 Recommendations SummaryBefore Merge (Required)
Follow-up Issues (Can be addressed post-merge)
📚 Style Compliance✅ Follows Repository Guidelines
|
This store implements the round.BoardingStore interface using the BatchedTx pattern for transaction-safe database operations. All methods execute within database transactions with automatic retry on serialization errors. The implementation defines BoardingQueries and BatchedBoardingQueries interfaces following the established pattern from taproot-assets. The store wraps a BatchedBoardingQueries instance and uses ExecTx for all operations, ensuring atomicity and proper error handling. Serialization handles complex types including btcec public keys, keychain descriptors, waddrmgr tapscripts, and wire transactions. JSON encoding is used for nested structures like VTXO templates and tapscript data that don't map cleanly to flat columns. The status conversion functions translate between round.BoardingStatus enum values and their database string representations, maintaining type safety at the boundary between domain types and persistence.
These tests verify the BoardingWalletStore implementation using the TransactionExecutor pattern with isolated test databases. All ten tests use the newBoardingStoreForTest helper which creates a fresh SQLite database and wraps it with the proper transaction executor. The test coverage includes address round-trip persistence with tapscript serialization, intent lifecycle management from derived through complete states, status-based filtering for coordinator queries, multi-VTXO template handling, batch insert atomicity, and error path validation. Tests verify that upsert semantics work correctly, preserving already-set fields like confirmation data while allowing status progression. The tests also validate complex type serialization including public keys, key descriptors, transactions, and JSON- encoded VTXO templates.
This documentation provides a comprehensive reference for the boarding wallet database schema including table purposes, field descriptions, and operational patterns. The mermaid ERD diagram illustrates the relationships between the three tables and their foreign key constraints. The boarding_statuses enum table enforces valid lifecycle states, while boarding_addresses and boarding_intents form a one-to-many relationship allowing address reuse across multiple boarding attempts. The operational logic section explains key flows including address creation with restart recovery, UTXO detection with deduplication, intent lifecycle progression, and round assignment coordination. The documentation describes how the in-memory seenUtxos set works with database persistence to prevent duplicate notifications after restarts. Performance considerations cover index strategy for common query patterns, upsert optimization using COALESCE, and read-heavy transaction optimization. Future enhancements are documented for archival, pruning, and monitoring improvements.
The BoardingBackend interface abstracts LND wallet operations needed for boarding address management including key derivation, taproot script import, and UTXO enumeration. This abstraction enables testing with mock implementations while the production code uses the LND backend. The sealed message protocol defines six request/response pairs for actor communication: creating boarding addresses, querying active addresses and balances, and registering/unregistering for confirmation notifications. The sealed interface pattern using unexported marker methods prevents external packages from implementing the interfaces, ensuring type safety and exhaustive pattern matching in the actor's receive method. The BlockEpochNotification message wraps chainsource.BlockEpoch to make it compatible with the WalletMsg sealed interface. This enables the actor to receive block notifications directly via the message system using the NotifyActor pattern rather than spawning a separate polling goroutine. The BoardingUtxoConfirmedEvent is sent to registered actors when new boarding UTXOs are detected, providing all context needed to construct a boarding intent including the outpoint, amount, confirmation data, and the boarding address with its keys and tapscript.
The LndBoardingBackend provides the concrete implementation of the BoardingBackend interface by wrapping lndclient.WalletKitClient. This integration layer enables the boarding wallet actor to interact with an LND node's wallet functionality. The DeriveNextKey method delegates to LND's hierarchical deterministic key derivation using the specified key family. The ImportTaprootScript method imports constructed taproot scripts into the LND wallet, enabling LND to track UTXOs paying to boarding addresses and include them in ListUnspent results. The ListUnspent method queries all wallet UTXOs and converts from lnwallet.Utxo types to the wallet package's simplified Utxo type, extracting only the fields needed for boarding UTXO detection.
The BoardingWalletActor manages the full lifecycle of boarding addresses from creation through UTXO confirmation notification. The actor uses a message-driven architecture rather than spawning background goroutines, receiving block epoch notifications as messages via the NotifyActor pattern. On startup, the actor loads existing addresses and intents from the database to populate the seenUtxos deduplication set, then subscribes to block epochs from the chainsource actor using MapBlockEpoch to transform chainsource.BlockEpoch into BlockEpochNotification messages. When block epoch messages arrive, the actor calls ListUnspent on the LND backend and compares results against seenUtxos to identify new boarding UTXOs. For each new UTXO, it verifies the pk_script matches a boarding address, adds it to seenUtxos, and notifies all registered actors by sending BoardingUtxoConfirmedEvent messages. The address creation handler derives a new client key from LND, constructs a proper VTXO tapscript using scripts.VTXOTapScript with 2-of-2 collaborative and CSV timeout paths, imports it into the LND wallet, and persists the address with its cryptographic material. Notifier registration enables other actors like the round coordinator to receive confirmation events with optional backlog delivery for crash recovery scenarios.
These tests verify the boarding wallet actor's message handling using mock implementations of BoardingBackend, BoardingStore, and the chainsource actor. The mock pattern allows testing actor logic in isolation without requiring real LND connections or database transactions. The test suite covers address creation with tapscript construction and import verification, notifier registration and unregistration with proper map management, UTXO detection and notification delivery with deduplication, and query operations for addresses and balances. The MockChainSourceActor implements the ActorRef interface using actor.Promise for async completion, enabling realistic testing of the subscription pattern without spawning real actors. The mockTellOnlyRef captures notification events in a channel for verification. Tests verify that duplicate UTXOs are properly deduplicated, notifications are sent to all registered actors, and the seenUtxos set is maintained correctly across multiple processing cycles.
The gen_sqlc_docker.sh script patches INTEGER PRIMARY KEY to BIGINT PRIMARY KEY before running sqlc to ensure int64 types in generated Go code. This change makes the init migration consistent with that pattern, preventing the script from needing to patch this file on every sqlc run. While SQLite treats INTEGER PRIMARY KEY specially for autoincrement, the script handles the conversion automatically and the generated Go code correctly uses int64 regardless of the underlying database type.
88a41ec to
7a26ed7
Compare
Pull Request Review: wallet.Ark Initial ImplementationSummaryThis PR adds a comprehensive boarding wallet implementation focused on creating boarding addresses and tracking boarding intents. The implementation follows solid architectural patterns with proper separation of concerns, thorough testing, and excellent documentation. Strengths
Critical Issues1. Race condition: Missing mutex for notifiers map (wallet/wallet.go:63) The notifiers map is accessed in multiple methods and could be accessed concurrently. Consider adding explicit synchronization or documenting the concurrency guarantees. 2. Missing validation: CSV delay bounds (wallet/wallet.go:194-208) The ExitDelay parameter is uint32 but cast to int32 in the database. Add validation for min/max bounds and overflow prevention. Major Issues3. Resource leak: Backlog delivery context (wallet/wallet.go:321-323) The context passed to sendBacklog may be canceled before delivery completes. Consider using background context. 4. Infinite loop: ListUnspent polling on errors (wallet/wallet.go:361-371) If ListUnspent fails repeatedly, the actor continues indefinitely. Consider circuit breaker pattern or exponential backoff. 5. Silent error handling (wallet/wallet.go:394-398) Address reconstruction silently ignores all errors including database failures. Log database errors at debug level. Performance Issues6. N+1 query problem (db/boarding_wallet.go:238-242) Each intent loads its address via separate query. Consider JOIN query for batch loading. 7. Unbounded memory growth (wallet/wallet.go:86) seenUtxos set grows indefinitely. Consider pruning or LRU cache. Minor Issues
Security Review
Compliance with CLAUDE.md
VerdictApprove with recommendations for follow-up This is high-quality code with strong architecture, comprehensive testing, and excellent documentation. Critical issues should be addressed before merge. The boarding wallet foundation is solid and ready for round coordinator integration. Great work! 🎉 |
2 participants
In this PR, we add an initial version of
wallet.Ark. This partially fixes: https://github.com/lightninglabs/darepo/issues/24.The main focus of this PR is to better carve out the pipe line of creating boarding addresses, and then boarding intents (confirmed addr UTXOs). The chain notifier by default will only notify for the first instance of an address, so that won't work here. Instead, we import the addrs into the wallet (we should pick another wallet account here), then register for a block epoch, and poll
listunspenteach time a block is mined. Once we find newly confirmed UTXOs, we import them into the wallet.An initial version of the DB is in place as well.
PR is 60% slop to be refined.
The main TODOs here are: