Remove OnchainWallet trait - #259
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughChangesWallet and client decoupling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant AnchorSpendDeps
participant Client
participant UnilateralExit
UnilateralExit->>Client: broadcast_next_unilateral_exit_node(branch, deps)
Client->>AnchorSpendDeps: request change address
AnchorSpendDeps->>Wallet: get_onchain_address
Client->>AnchorSpendDeps: select coins
AnchorSpendDeps->>Wallet: select_coins
Client->>AnchorSpendDeps: sign PSBT
AnchorSpendDeps->>Wallet: sign PSBT
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ark-client/src/lib.rs (1)
2069-2099: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate outdated comment.
The closure maps the error type from
ark_client::Errortoark_core::Error; it doesn't convert aCoinSelectionResulttoUtxoCoinSelection. Thedeps.select_coinsdependency already correctly returnsUtxoCoinSelection.♻️ Proposed fix
- // Create a closure that converts CoinSelectionResult to UtxoCoinSelection + // Create a closure that maps the coin selection error let select_coins_fn =🤖 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 `@ark-client/src/lib.rs` around lines 2069 - 2099, Update the comment above select_coins_fn in bump_tx to describe that the closure maps the deps.select_coins error from ark_client::Error to ark_core::Error while preserving its UtxoCoinSelection result; do not describe it as converting CoinSelectionResult.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ark-client/src/lib.rs`:
- Around line 2069-2099: Update the comment above select_coins_fn in bump_tx to
describe that the closure maps the deps.select_coins error from
ark_client::Error to ark_core::Error while preserving its UtxoCoinSelection
result; do not describe it as converting CoinSelectionResult.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f10949a-373e-4806-96f7-5e55412ccb88
📒 Files selected for processing (18)
ark-bdk-wallet/src/lib.rsark-client-sample/src/main.rsark-client/src/asset.rsark-client/src/batch.rsark-client/src/boltz.rsark-client/src/coin_select.rsark-client/src/fee_estimation.rsark-client/src/lib.rsark-client/src/migration.rsark-client/src/send_vtxo.rsark-client/src/unilateral_exit.rsark-client/src/vtxo_watcher.rsark-client/src/wallet.rse2e-tests/tests/boltz_reverse_vhtlc_unilateral_exit.rse2e-tests/tests/common.rse2e-tests/tests/e2e_send_onchain_boarding_output.rse2e-tests/tests/e2e_send_onchain_vtxo_and_boarding_output.rse2e-tests/tests/e2e_signer_rotation.rs
💤 Files with no reviewable changes (1)
- ark-client/src/wallet.rs
|
@bonomat @luckysori, would really appreciate a review here when you get the chance. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
This touches unilateral-exit and anchor fee-bump paths. Refactor looks mechanically correct — but two seams move safety guarantees from the client to the caller without documenting them. Please have a protocol reviewer sign off even after the notes below are addressed.
Findings
1. send_on_chain / create_send_on_chain_transaction: new change_address param is undocumented (money-loss footgun)
ark-client/src/unilateral_exit.rs:207-212, 234-242, 244-249
The new change_address: Address argument has no doc comment. Previously the client fetched this from the internal wallet, so change from a unilateral exit was guaranteed to be self-spendable. Now every caller has to know that (a) the address must be spendable by them, and (b) it must match the correct network — nothing in the client validates either. ark-core::unilateral_exit::create_unilateral_exit_transaction (ark-core/src/unilateral_exit.rs:188-193) just drops it into a TxOut with the leftover value. On a unilateral-exit tx the change can be effectively the entire balance minus to_amount + 1000 sat — silently sending that to a wrong-network or third-party address loses funds unrecoverably.
Please:
- Add a doc paragraph on
send_on_chain,create_send_on_chain_transaction, andcreate_send_on_chain_transaction_innerstating the address MUST be spendable by the caller. - Consider a network sanity check against
self.server_info().await?.networkincreate_send_on_chain_transaction_innerbefore building the tx.
2. AnchorSpendDeps has no documented consistency invariant
ark-client/src/lib.rs:522-533
The three closures (change_address, select_coins, sign) must all be backed by the same on-chain wallet, otherwise select_coins yields UTXOs sign cannot spend, or fee-bump change lands somewhere unspendable. The doc comment describes each field in isolation. Please add a top-level note on the struct: "All three closures MUST be backed by the same underlying on-chain wallet — mixing sources will produce unsignable or unspendable transactions."
3. No AnchorSpendDeps usage example / helper
ark-client/src/lib.rs (doc example around line 300-404), e2e-tests/tests/e2e_send_onchain_vtxo_and_boarding_output.rs:95-108, e2e-tests/tests/boltz_reverse_vhtlc_unilateral_exit.rs:96-109
The lib.rs doc example was trimmed to remove the old MyWallet, but nothing shows how to build AnchorSpendDeps — which is now the only way to fee-bump a unilateral exit. External consumers coming from the old trait have no example. The two e2e tests repeat the same 14-line closure boilerplate verbatim, which is a clear signal a AnchorSpendDeps::from_bdk_wallet(&Arc<Wallet>) -> AnchorSpendDeps<'_> (or equivalent) helper in ark-bdk-wallet would help both correctness and ergonomics.
4. bdk_wallet::Balance now leaks into the ark-bdk-wallet public API
ark-bdk-wallet/src/lib.rs:119
Fine for an adapter crate, but this hard-pins any downstream consumer typing against Balance to your bdk_wallet major. Previously they saw ark_client::wallet::Balance, which was version-decoupled. Worth calling out in the changelog / release notes as a break.
5. Test coverage gap
- No unit test exercises
AnchorSpendDepsclosures (error propagation, mismatched wallet, wrong-network change). Two e2e tests cover the happy path. - No test covers a
send_on_chaincall with a wrong-networkchange_address.
Not blocking, but the "no behavioural change" claim in the PR body only holds if callers follow the (currently undocumented) contract — a couple of guard tests would make the contract enforceable.
Cross-repo impact
Scanned ark-flutter-example, ark-rs-flutter-example, demos, wallet, arkade-*. All consumers of ark-client pin a git rev, so nothing breaks at merge time. Downstream migrations will hit exactly the issues above (undocumented change_address semantics, missing AnchorSpendDeps example) — please fix here so migrators don't rediscover them.
Nit
ark-client/src/boltz.rs:2828 — unrelated &self.inner.boltz_url → self.inner.boltz_url cleanup is fine but out of scope.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Since cbd50bf: 2 commits directly addressing prior findings; 4/5 issues resolved; 1 still open.
Prior findings close-out
Finding #1 — change_address undocumented (money-loss footgun)
Fixed. Commit "docs(client): document change-address ownership requirement" added a doc paragraph to send_on_chain, create_send_on_chain_transaction, and create_send_on_chain_transaction_inner (unilateral_exit.rs:204-214, 242-244, 256-257). Commit "feat(client): add AnchorSpendDeps helper and guard tests" adds a network guard in create_send_on_chain_transaction_inner (unilateral_exit.rs:261-269) that rejects both to_address and change_address when they don't match server_info.network.
Finding #2 — AnchorSpendDeps missing consistency invariant
Fixed. The struct doc now reads: "All three closures MUST be backed by the same underlying on-chain wallet. Mixing sources produces unsignable or unspendable transactions" (lib.rs:525-527). The same commit adds a runtime guard: if sign returns false (PSBT unfinalized), bump_tx now returns a descriptive error naming the invariant violation (lib.rs:2108-2113).
Finding #3 — No AnchorSpendDeps helper / 14-line boilerplate repeated in e2e tests
Fixed. ark-bdk-wallet/src/lib.rs:209-217 adds Wallet::anchor_spend_deps(&self) -> AnchorSpendDeps<'_> that wires all three closures to self. Both e2e tests updated to a one-liner (e2e_boltz_reverse_vhtlc_unilateral_exit.rs:95, e2e_send_onchain_vtxo_and_boarding_output.rs:94).
Finding #4 — bdk_wallet::Balance leaking into ark-bdk-wallet public API
Still open. The full PR diff shows ark-bdk-wallet/src/lib.rs:120:
pub fn balance(&self) -> Result<bdk_wallet::Balance, Error> {
…
Ok(balance) // returns bdk_wallet::Balance directly
}The previous conversion wrapper (ark_client::wallet::Balance { immature, … }) was removed. bdk_wallet::Balance is now a public API type of the crate. Any downstream consumer that types against it is pinned to your bdk_wallet major version. Not made worse by the incremental commits, but not fixed either. Still worth a re-export or newtype before stabilization; a changelog entry at minimum.
Finding #5 — Test coverage gap
Fixed. unilateral_exit.rs:331-639 adds seven unit tests exercising: wrong-network change address, wrong-network destination address, correct-network addresses reaching the dust check, and all four bump_tx error paths (change-address failure, coin-selection failure, sign failure, unfinalized PSBT). Coverage is good.
New finding (incremental diff only)
N1 — Misleading test name
unilateral_exit.rs:427: send_on_chain_accepts_addresses_on_the_server_network expects a dust error, not success. The test is logically correct (valid-network addresses pass the network check and reach the dust guard) but the name implies the call succeeds. A reader skimming by name will assume both addresses and the amount are valid. Rename to e.g. send_on_chain_correct_network_then_rejects_dust_amount.
Incidental changes note
The boltz.rs delta in this range (+3248/-674) is almost entirely from the PR #260 contract manager merge. The vtxo_watcher.rs change passes None for the new subscription filter and handles the new SubscriptionStarted variant — both correct and minimal. No OnchainWallet-removal issues introduced.
Blocking item: Finding #4 (bdk_wallet::Balance in public API). Needs either a newtype/re-export or a changelog entry documenting the break before this lands. Everything else is resolved. N1 is a nit.
|
Will really appreciate a manual review here @luckysori @bonomat when you get a chance |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Incremental diff (29e4c30…6d8123d): 2 commits, SECURITY.md whitespace-only — zero code change. 0 prior issues fixed this pass, 2 remain open.
Prior findings close-out
Finding #1 — change_address undocumented (money-loss footgun)
Fixed (previous pass, unchanged).
Finding #2 — AnchorSpendDeps missing consistency invariant
Fixed (previous pass, unchanged).
Finding #3 — No AnchorSpendDeps helper / boilerplate
Fixed (previous pass, unchanged).
Finding #4 — bdk_wallet::Balance leaks into ark-bdk-wallet public API
Still open. ark-bdk-wallet/src/lib.rs:120 still returns bdk_wallet::Balance directly. The two commits in this range touch only SECURITY.md; this finding is not made worse but has not been addressed. A newtype, re-export, or explicit changelog entry is needed before this stabilises.
Finding #5 — Test coverage gap
Fixed (previous pass, unchanged).
N1 — Misleading test name
Still open. ark-client/src/unilateral_exit.rs:506 still reads send_on_chain_accepts_addresses_on_the_server_network; the test asserts a dust error, not acceptance. Please rename to e.g. send_on_chain_correct_network_then_rejects_dust_amount.
New findings
None — this diff is purely SECURITY.md whitespace formatting. No code paths changed.
Blocking: Finding #4. N1 is a nit. No new issues to report.
2 participants
Closes #137. Removes the
OnchainWalletabstraction and theWgeneric it required. RemovingBoardingwalletwas already addressed prior to this PR. No behavioural change.Summary by CodeRabbit