Conversation
📝 WalkthroughWalkthroughThis PR adds a complete Bark wallet integration as a new Lightning node backend. It introduces encrypted backup/restore with AES-256-GCM, BOLT11 invoice and payment operations, wallet configuration and initialization, and module-level exports for cross-language bindings via UniFFI. ChangesBark Wallet Integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant BarkNode
participant Wallet
participant Storage
Client->>BarkNode: BarkNode::new(config)
BarkNode->>Storage: create/validate directory
BarkNode->>Wallet: open or create wallet
Wallet-->>BarkNode: Arc<Wallet>
BarkNode-->>Client: initialized BarkNode
Client->>BarkNode: create_invoice(amount)
BarkNode->>Wallet: create Bolt11
Wallet-->>BarkNode: invoice + hash
BarkNode-->>Client: Transaction
Client->>BarkNode: pay_invoice(invoice)
BarkNode->>Wallet: pay_lightning_invoice
Wallet-->>BarkNode: send state + preimage
BarkNode-->>Client: PayInvoiceResponse
Client->>BarkNode: create_backup(secret)
BarkNode->>Storage: collect + checksum files
BarkNode->>Storage: encrypt (AES-256-GCM + PBKDF2)
BarkNode-->>Client: BarkBackup (Base64 envelope)
Client->>BarkNode: restore_from_backup(backup, secret)
BarkNode->>Storage: decrypt + validate envelope
BarkNode->>Storage: staging write + verify
BarkNode->>Storage: atomic replace + reopen wallet
BarkNode-->>Client: restored BarkNode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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.
Reviewed commit: cbd29a07bc
ℹ️ 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".
| let invoice = wallet | ||
| .pay_lightning_invoice(params.invoice, amount, true) | ||
| .await | ||
| .map_err(|e| ApiError::Api { | ||
| reason: e.to_string(), |
There was a problem hiding this comment.
When PayInvoiceParams includes fee_limit_msat or fee_limit_percentage, this path still calls Bark's payment API without reading either cap, so a caller that set a maximum fee can still have the invoice paid with whatever fee Bark selects. This affects Bark payments where the app relies on LNI's shared fee-limit fields to prevent unexpectedly expensive sends; either pass an equivalent limit if the SDK supports it or reject unsupported fee-limit parameters instead of silently ignoring them.
Useful? React with 👍 / 👎.
| reason: e.to_string(), | ||
| })?; | ||
|
|
||
| if let Some(search) = params.search { |
There was a problem hiding this comment.
ListTransactionsParams has a dedicated payment_hash filter, but Bark only applies search, so list_transactions with payment_hash set and search unset returns the entire wallet history. This breaks callers that use the shared API to fetch transactions for one payment hash and can expose unrelated transactions in the response; include params.payment_hash in the retain predicate before paginating.
Useful? React with 👍 / 👎.
| )); | ||
| } | ||
|
|
||
| let sats = u64::try_from(msats / 1000) |
There was a problem hiding this comment.
For Bark, bitcoin::Amount is sat-denominated, but this conversion truncates amount_msats / 1000; for example, amount_msats = 1999 creates or pays only 1 sat while the returned Transaction still reports 1999 msat. This affects any caller that supplies a millisatoshi amount not divisible by 1000, leading to undercharged invoices or underpaid zero-amount invoices instead of an explicit unsupported-precision error.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (2)
crates/lni/bark/backup.rs (1)
395-408: ⚖️ Poor tradeoffNon-atomic directory replacement has a data-loss window.
Between
remove_dir_all(storage_dir)on line 396 andrename(&staging_dir, storage_dir)on line 403, a crash or power failure would leave the user without wallet data. On POSIX systems, this could be mitigated by renaming the old directory aside before the final rename, then removing it afterward; on Windows, atomic directory swaps are harder.If this is an acceptable trade-off for the target platforms, consider documenting it.
🤖 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 `@crates/lni/bark/backup.rs` around lines 395 - 408, Current replace logic deletes storage_dir then renames staging_dir into its place, leaving a data-loss window; change to a safe three-step swap: first rename existing storage_dir to a unique backup name (e.g., storage_dir_bak with timestamp/UUID) rather than remove it, then rename staging_dir to storage_dir, and only after successful rename remove the backup; ensure all operations that can fail (renaming old, installing staging, removing backup) map errors to ApiError and attempt best-effort cleanup of staging_dir and the backup on failure (references: storage_dir, staging_dir and the rename/remove_dir_all calls), and consider platform-specific fallbacks for Windows if atomic semantics aren’t available.crates/lni/bark/lib.rs (1)
98-100: ⚡ Quick winNormalize network labels before passing them to API methods.
network_label()returns raw user input ("mainnet"or"bitcoin"), while backup/restore paths already canonicalize labels. Canonicalizing here too avoids alias-driven output drift for equivalent network configs.Suggested refactor
- fn network_label(&self) -> &str { - self.network.as_deref().unwrap_or("mainnet") - } + fn network_label(&self) -> Result<&'static str, ApiError> { + Ok(canonical_network_label(self.get_network()?)) + } @@ - crate::bark::api::get_info(self.wallet.clone(), self.config.network_label()).await + crate::bark::api::get_info(self.wallet.clone(), self.config.network_label()?).awaitAlso applies to: 255-257
🤖 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 `@crates/lni/bark/lib.rs` around lines 98 - 100, network_label() currently returns raw user input and should return a canonicalized network label like the backup/restore code does; update fn network_label(&self) -> &str to return the canonicalized form (call the same canonicalization helper used by backup/restore, e.g., canonicalize_network_label/normalize_network_label) so aliases (e.g., "bitcoin") map to the canonical "mainnet", and apply the same change to the other occurrences noted around lines 255-257 to ensure all API calls receive normalized labels.
🤖 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.
Inline comments:
In `@crates/lni/bark/lib.rs`:
- Around line 150-167: The current match falls back to Wallet::create for any
Wallet::open error; change this so you only call Wallet::create when the open
error indicates an uninitialized/missing wallet (the “Wallet is not initialised”
path) and config.create_if_missing is true. Concretely, in the match on
Wallet::open, restrict the Err arm to the specific error variant or condition
(e.g., WalletError::NotInitialised or matching the “Wallet is not initialised”
message) and leave all other Err(open_error) cases to return or propagate the
original error so you don't attempt create on unrelated failures; keep the call
to Wallet::create exactly as currently structured when the uninitialized
condition is met.
---
Nitpick comments:
In `@crates/lni/bark/backup.rs`:
- Around line 395-408: Current replace logic deletes storage_dir then renames
staging_dir into its place, leaving a data-loss window; change to a safe
three-step swap: first rename existing storage_dir to a unique backup name
(e.g., storage_dir_bak with timestamp/UUID) rather than remove it, then rename
staging_dir to storage_dir, and only after successful rename remove the backup;
ensure all operations that can fail (renaming old, installing staging, removing
backup) map errors to ApiError and attempt best-effort cleanup of staging_dir
and the backup on failure (references: storage_dir, staging_dir and the
rename/remove_dir_all calls), and consider platform-specific fallbacks for
Windows if atomic semantics aren’t available.
In `@crates/lni/bark/lib.rs`:
- Around line 98-100: network_label() currently returns raw user input and
should return a canonicalized network label like the backup/restore code does;
update fn network_label(&self) -> &str to return the canonicalized form (call
the same canonicalization helper used by backup/restore, e.g.,
canonicalize_network_label/normalize_network_label) so aliases (e.g., "bitcoin")
map to the canonical "mainnet", and apply the same change to the other
occurrences noted around lines 255-257 to ensure all API calls receive
normalized labels.
🪄 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: 064c12cd-f48f-4266-8f9b-93e7a486f819
📒 Files selected for processing (6)
crates/lni/Cargo.tomlcrates/lni/bark/api.rscrates/lni/bark/backup.rscrates/lni/bark/lib.rscrates/lni/bark/types.rscrates/lni/lib.rs
| let wallet = match Wallet::open( | ||
| &mnemonic, | ||
| db.clone(), | ||
| bark_config.clone(), | ||
| Box::new(MemoryLockManager::new()), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(wallet) => wallet, | ||
| Err(open_error) if config.create_if_missing.unwrap_or(true) => Wallet::create( | ||
| &mnemonic, | ||
| network, | ||
| bark_config, | ||
| db, | ||
| Box::new(MemoryLockManager::new()), | ||
| config.force_create.unwrap_or(false), | ||
| ) | ||
| .await |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For bark-wallet v0.2.3, which Wallet::open error variant specifically means "wallet missing/uninitialized", and under what conditions can Wallet::create reinitialize or overwrite existing wallet state?
💡 Result:
In the bark-wallet library, the Wallet::open method does not return a specific error variant for "wallet missing" or "uninitialized." Instead, it uses the anyhow::Result type to handle errors [1][2][3]. When opening a database that contains no wallet properties, the method explicitly returns an error context message: "Wallet is not initialised" [1][2][3]. Regarding Wallet::create, the implementation includes a check to see if properties already exist in the provided database [3]. If db.read_properties returns existing data, the method will return an error (bail!) stating that it "cannot overwrite already existing config" [3]. Therefore, under standard conditions, Wallet::create cannot reinitialize or overwrite an existing wallet state; it is designed to fail if a wallet is already present [3]. There is a force parameter (often used in the CLI or specific internal flows) which may be used to override this behavior, but the standard Wallet::create function prevents accidental destruction of existing wallet state [3].
Citations:
- 1: https://gitlab.com/ark-bitcoin/bark/-/merge_requests/1297.diff
- 2: https://gitlab.com/ark-bitcoin/bark/-/merge_requests/1452.diff
- 3: https://gitlab.com/ark-bitcoin/bark/-/merge_requests/1485.diff
🌐 Web query:
bark-wallet v0.2.3 Wallet::create implementation: what happens when db.read_properties returns an error (e.g., corruption/unreadable), and how does the force parameter change overwrite/reinitialize behavior?
💡 Result:
In bark-wallet v0.2.3, Wallet::create loads/initializes wallet state based on what’s already in the Bark persister database; Wallet::open explicitly shows how failures in db.read_properties are handled, and the create force flag affects only the “server connection” part of the workflow. 1) What happens if db.read_properties returns an error (corruption/unreadable) - The wallet-open path calls db.read_properties and immediately errors out if properties can’t be read/parsed. - Concretely, Wallet::open does: let properties = db.read_properties?.context("Wallet is not initialised")?; so any error returned by db.read_properties (including unreadable/corrupt DB) is propagated as a failing anyhow::Result (it does not silently fall back to creating a new wallet from scratch). 2) How the force parameter changes overwrite/reinitialize behavior - The docs comment/implementation text around Wallet::create states: “The force flag will allow you to create the wallet even if a connection to the Ark server cannot be established, it will not overwrite a wallet which has already been created.” [1] - That means force affects whether Wallet::create can proceed when the Ark server is unreachable (e.g., offline/connection failure). It does not change behavior to “overwrite/reinitialize” an already-initialized wallet in the DB. Practical implication combining both points - If the database exists but db.read_properties fails (e.g., corruption/unreadable), Wallet::open will fail rather than reinitializing. Wallet::create’s force does not promise overwrite/reinitialize semantics for an already-created wallet; its purpose is avoiding server-connection failure gating, not ignoring/repairing a broken persister state. [1] If you want, tell me whether you mean Wallet::create or Wallet::open in your scenario (some apps “create if missing, otherwise open”), and I can trace the exact control flow for that specific call chain in your version.
Citations:
🌐 Web query:
bark-wallet v0.2.3 Wallet::create control flow when db.read_properties returns an Err (corrupt/unreadable DB): does Wallet::create fail, or does it treat it as "not initialised" and proceed to create/write wallet state anyway?
💡 Result:
In bark-wallet version 0.2.3 (and generally within the library's design), the Wallet::create function does not silently treat an error from db.read_properties as an "uninitialized" state; instead, it propagates the error, causing the function to fail [1][2]. When Wallet::create is called, it relies on the database client to manage state [3]. If the database is corrupt or unreadable, the call to db.read_properties will return an Err (e.g., a database error from the underlying SQL driver). Because Wallet::create uses the Rust? operator or explicit error handling contexts (such as.context("...") in the codebase), any failure at the database level results in the immediate termination of the Wallet::create execution flow, returning an anyhow::Result::Err [1][2]. The library makes a clear distinction between an "uninitialized" wallet (where the DB might be empty or missing specific properties) and a system or database error (where the DB exists but cannot be read due to corruption) [2][4]. If an error occurs during database interaction, the library does not attempt to proceed with writing a new wallet state, as doing so could potentially overwrite or further corrupt existing data [2]. To successfully create or open a wallet, the database must be accessible and readable [2][5].
Citations:
- 1: https://gitlab.com/ark-bitcoin/bark/-/merge_requests/1485.diff
- 2: https://gitlab.com/ark-bitcoin/bark/-/merge_requests/1297.diff
- 3: https://docs.rs/bark-wallet/latest/bark/persist/sqlite/index.html
- 4: https://gitlab.com/ark-bitcoin/bark/-/merge_requests/1297.patch
- 5: https://second.tech/docs/bark-sdk/rust
Only fall back to Wallet::create when Wallet::open fails due to an uninitialized/missing wallet.
crates/lni/bark/lib.rs currently calls Wallet::create for any Wallet::open error when config.create_if_missing is true. In bark-wallet, “uninitialized/missing” is represented by the “Wallet is not initialised” path, while Wallet::create won’t overwrite an already-created wallet and will fail fast if the DB can’t be read. Narrowing the fallback to the uninitialized/missing error preserves non-missing error reporting and avoids unnecessary create attempts.
🤖 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 `@crates/lni/bark/lib.rs` around lines 150 - 167, The current match falls back
to Wallet::create for any Wallet::open error; change this so you only call
Wallet::create when the open error indicates an uninitialized/missing wallet
(the “Wallet is not initialised” path) and config.create_if_missing is true.
Concretely, in the match on Wallet::open, restrict the Err arm to the specific
error variant or condition (e.g., WalletError::NotInitialised or matching the
“Wallet is not initialised” message) and leave all other Err(open_error) cases
to return or propagate the original error so you don't attempt create on
unrelated failures; keep the call to Wallet::create exactly as currently
structured when the uninitialized condition is met.
1 participant
Add Bark node.
see:
Summary by CodeRabbit