feat(platform-wallet): external signable wallets#3639
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR downgrades freshly-created Wallets to an external-signable form before registration and updates several rust-dashcore Git dependency revisions in the workspace Cargo.toml. ChangesWallet External-Signable Downgrade & Workspace Rev Bump
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-05-13T17:57:17.078Z |
ZocoLini
force-pushed
the
feat/external-signable-wallets
branch
2 times, most recently
from
dc7dafb to
e3473a5
Compare
ZocoLini
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3639 +/- ##
============================================
- Coverage 87.15% 87.15% -0.01%
============================================
Files 2606 2606
Lines 319221 319221
============================================
- Hits 278216 278215 -1
- Misses 41005 41006 +1
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "8e580a55cf225cceefbeeaf6b332f9ebd711889902b298fa5b0309cbb03c12c2"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
One-file change that ensures create_wallet_from_mnemonic / create_wallet_from_seed_bytes always yield an ExternalSignable wallet. The type-level invariant is correctly enforced, but the helper's documented security guarantee ("strip the in-process xpriv") is not actually delivered: drop(temp) runs no zeroization because key_wallet::Wallet has only a manual Zeroize impl, no Drop/ZeroizeOnDrop. The exact same downgrade path upstream in key-wallet-manager calls wallet.zeroize() before drop. Two secondary issues: the cloned per-account is_watch_only flag diverges from how the restore path reconstructs the same wallet, and the new invariant has no test coverage.
🟡 3 suggestion(s)
ZocoLini
force-pushed
the
feat/external-signable-wallets
branch
from
e3473a5 to
9f28fe6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (2)
427-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExternal-signable wallet may retain non-watch-only account flags.
At Line 427, cloning accounts from an xpriv-derived wallet can preserve
is_watch_only=falsewhile wallet type becomesExternalSignable, which can diverge from restore-path semantics and trigger fragile account behavior.Suggested fix
fn downgrade_to_external_signable(mut temp: Wallet) -> Wallet { use zeroize::Zeroize; let wallet_id = temp.wallet_id; let network = temp.network; - let accounts = temp.accounts.clone(); + let mut accounts = temp.accounts.clone(); + for account in accounts.all_accounts_mut() { + account.to_watch_only(); + } temp.zeroize(); drop(temp); Wallet::new_external_signable(network, wallet_id, accounts) }#!/bin/bash # Verify watch-only semantics across creation and restore paths. set -euo pipefail rg -n -C3 'from_xpriv|from_xpub|is_watch_only|to_watch_only' --type rust rg -n -C3 'new_external_signable|new_watch_only|create_wallet_from_mnemonic|create_wallet_from_seed_bytes' --type rust🤖 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 `@packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs` around lines 427 - 429, Cloning accounts from the temporary xpriv-derived wallet into Wallet::new_external_signable can carry over is_watch_only=false; update the code that prepares accounts (the temp.accounts.clone usage) to convert each account to watch-only semantics before passing them to Wallet::new_external_signable (e.g., map/iterate the cloned accounts and set their is_watch_only flag or call an existing to_watch_only on account structs), ensuring ExternalSignable wallets never retain non-watch-only account flags and align with the restore-path behavior.
424-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
drop(temp)does not scrub wallet secrets before deallocation.At Line 428, the helper drops the temporary wallet without explicit zeroization. If
Walletis notZeroizeOnDrop, mnemonic/seed-derived material can remain in freed heap pages.Suggested fix
-fn downgrade_to_external_signable(temp: Wallet) -> Wallet { +fn downgrade_to_external_signable(mut temp: Wallet) -> Wallet { + use zeroize::Zeroize; let wallet_id = temp.wallet_id; let network = temp.network; let accounts = temp.accounts.clone(); + temp.zeroize(); drop(temp); Wallet::new_external_signable(network, wallet_id, accounts) }#!/bin/bash # Verify Wallet zeroization traits/impls and upstream downgrade behavior. set -euo pipefail rg -n -C3 'impl\s+Zeroize\s+for\s+Wallet|impl\s+Drop\s+for\s+Wallet|ZeroizeOnDrop' --type rust rg -n -C3 'downgrade_to_external_signable|new_external_signable|zeroize\(' --type rust🤖 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 `@packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs` around lines 424 - 429, The helper downgrade_to_external_signable currently calls drop(temp) which does not guarantee zeroization of sensitive fields; before dropping the temporary Wallet, explicitly scrub its secret material (e.g., call temp.zeroize() or temp.wipe_sensitive() if such methods exist and Wallet implements Zeroize) or, if not present, implement Zeroize/ZeroizeOnDrop (or a Drop impl that zeroes mnemonic/seed fields) on Wallet so secrets are overwritten before deallocation; ensure the change happens in downgrade_to_external_signable (replace the drop(temp) call with an explicit zeroization call or rely on Wallet's ZeroizeOnDrop) and keep Wallet::new_external_signable usage unchanged.
🤖 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.
Duplicate comments:
In `@packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- Around line 427-429: Cloning accounts from the temporary xpriv-derived wallet
into Wallet::new_external_signable can carry over is_watch_only=false; update
the code that prepares accounts (the temp.accounts.clone usage) to convert each
account to watch-only semantics before passing them to
Wallet::new_external_signable (e.g., map/iterate the cloned accounts and set
their is_watch_only flag or call an existing to_watch_only on account structs),
ensuring ExternalSignable wallets never retain non-watch-only account flags and
align with the restore-path behavior.
- Around line 424-429: The helper downgrade_to_external_signable currently calls
drop(temp) which does not guarantee zeroization of sensitive fields; before
dropping the temporary Wallet, explicitly scrub its secret material (e.g., call
temp.zeroize() or temp.wipe_sensitive() if such methods exist and Wallet
implements Zeroize) or, if not present, implement Zeroize/ZeroizeOnDrop (or a
Drop impl that zeroes mnemonic/seed fields) on Wallet so secrets are overwritten
before deallocation; ensure the change happens in downgrade_to_external_signable
(replace the drop(temp) call with an explicit zeroization call or rely on
Wallet's ZeroizeOnDrop) and keep Wallet::new_external_signable usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8908f0c-14e6-489b-90c3-ffb9e70fcda0
📒 Files selected for processing (1)
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #3639 makes mnemonic/seed-derived wallets get downgraded to ExternalSignable via a helper that clones the account collection and drops the temporary wallet. Two prior findings remain valid on the current head: drop(temp) does not actually zeroize the xpriv/seed material the doc-comment promises to strip (Wallet has a manual Zeroize impl but no Drop/ZeroizeOnDrop), and there is no test asserting the new ExternalSignable invariant on either creation entry point. The account-level is_watch_only mismatch is contested and not actionable: ExternalSignable semantically differs from WatchOnly (accounts can still sign via an external signer), and signing/zeroize decisions are driven by wallet_type rather than the account flag, so it is reclassified as resolved.
Note: Inline posting failed (command failed (1): gh api /repos/dashpay/platform/pulls/3639/reviews --method POST --input -
STDOUT:
{"message":"Unprocessable Entity","errors":["Variable $threads of type [DraftPullRequestReviewThread] was provided invalid value for 0.commitId (Field is not defined on DraftPullRequestReviewThread)), so I posted the same verified findings as a top-level review body.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: `drop(temp)` does not zeroize the xpriv the doc-comment promises to strip
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 420)
The doc-comment on downgrade_to_external_signable states the helper exists to "Strip the in-process xpriv from a freshly-derived wallet," but the implementation only clones accounts and then calls drop(temp). key_wallet::Wallet defines a manual impl Zeroize (key-wallet/src/wallet/mod.rs:153) which clears the mnemonic, seed, and chain code, but it has no Drop and is not annotated with ZeroizeOnDrop. The default destructor therefore just recursively drops fields — the mnemonic bytes, the RootExtendedPrivKey (including the secp256k1::SecretKey), the BIP39 seed, and the chain code all linger on the heap until something else reuses that allocation. For the threat model the PR is targeting ("seed lives only in the host's secure storage and the resolver vtable is the only way to sign"), the in-process secret is still recoverable from a heap dump, core file, swap, or attached debugger after this helper returns. Either explicitly call temp.zeroize() before/instead of drop(temp), or downgrade the doc-comment to reflect what the code actually does. Note that even Wallet::zeroize itself does not clear the inner secp256k1::SecretKey (key-wallet/src/wallet/mod.rs:168), and the originating Wallet::from_mnemonic/from_seed_bytes paths leave additional ephemeral copies of the seed in their own stack frames — so a complete fix likely also needs upstream cooperation, but temp.zeroize() is still strictly better than drop(temp).
fn downgrade_to_external_signable(mut temp: Wallet) -> Wallet {
use zeroize::Zeroize;
let wallet_id = temp.wallet_id;
let network = temp.network;
let accounts = temp.accounts.clone();
temp.zeroize();
drop(temp);
Wallet::new_external_signable(network, wallet_id, accounts)
}
suggestion: New `ExternalSignable` invariant has no direct regression test
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 73)
The behavioral guarantee at the heart of this PR — that create_wallet_from_mnemonic and create_wallet_from_seed_bytes always return an ExternalSignable wallet rather than a mnemonic/seed-backed one — is not asserted by any test in the crate. The only call site under tests is packages/rs-platform-wallet/tests/spv_sync.rs, which exercises wallet creation purely as setup for SPV sync and never inspects wallet_type, is_external_signable(), or the account material. A grep for wallet_type / is_external_signable / is_watch_only under packages/rs-platform-wallet/tests returns nothing. A small unit test that calls each entry point and asserts wallet.is_external_signable() && !wallet.has_mnemonic() (plus that the per-account xpubs survived the downgrade) would lock in the invariant against a future refactor accidentally removing the downgrade_to_external_signable call or returning the seed-backed wallet again. The same test is the natural place to also verify that birth_height_override lands in the persisted WalletMetadataEntry.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:420-430: `drop(temp)` does not zeroize the xpriv the doc-comment promises to strip
The doc-comment on `downgrade_to_external_signable` states the helper exists to "Strip the in-process xpriv from a freshly-derived wallet," but the implementation only clones `accounts` and then calls `drop(temp)`. `key_wallet::Wallet` defines a manual `impl Zeroize` (key-wallet/src/wallet/mod.rs:153) which clears the mnemonic, seed, and chain code, but it has no `Drop` and is not annotated with `ZeroizeOnDrop`. The default destructor therefore just recursively drops fields — the mnemonic bytes, the `RootExtendedPrivKey` (including the `secp256k1::SecretKey`), the BIP39 `seed`, and the chain code all linger on the heap until something else reuses that allocation. For the threat model the PR is targeting ("seed lives only in the host's secure storage and the resolver vtable is the only way to sign"), the in-process secret is still recoverable from a heap dump, core file, swap, or attached debugger after this helper returns. Either explicitly call `temp.zeroize()` before/instead of `drop(temp)`, or downgrade the doc-comment to reflect what the code actually does. Note that even `Wallet::zeroize` itself does not clear the inner `secp256k1::SecretKey` (key-wallet/src/wallet/mod.rs:168), and the originating `Wallet::from_mnemonic`/`from_seed_bytes` paths leave additional ephemeral copies of the seed in their own stack frames — so a complete fix likely also needs upstream cooperation, but `temp.zeroize()` is still strictly better than `drop(temp)`.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:73-116: New `ExternalSignable` invariant has no direct regression test
The behavioral guarantee at the heart of this PR — that `create_wallet_from_mnemonic` and `create_wallet_from_seed_bytes` always return an `ExternalSignable` wallet rather than a mnemonic/seed-backed one — is not asserted by any test in the crate. The only call site under tests is `packages/rs-platform-wallet/tests/spv_sync.rs`, which exercises wallet creation purely as setup for SPV sync and never inspects `wallet_type`, `is_external_signable()`, or the account material. A grep for `wallet_type` / `is_external_signable` / `is_watch_only` under `packages/rs-platform-wallet/tests` returns nothing. A small unit test that calls each entry point and asserts `wallet.is_external_signable() && !wallet.has_mnemonic()` (plus that the per-account xpubs survived the downgrade) would lock in the invariant against a future refactor accidentally removing the `downgrade_to_external_signable` call or returning the seed-backed wallet again. The same test is the natural place to also verify that `birth_height_override` lands in the persisted `WalletMetadataEntry`.
| e | ||
| )) | ||
| })?; | ||
| let wallet = downgrade_to_external_signable(wallet); |
There was a problem hiding this comment.
Why we aren't doing this in from_mnemonic or from_seed_bytes in rust-dashcore?
There was a problem hiding this comment.
bcs, so far, right now we use and need internal singable wallets for the tests, and I don't know if we want to only have external singable wallets exposed, it is something that can be done, removing the WalletType field in the Wallet struct shouldn't be that hard, tests would need and update but thats it
There was a problem hiding this comment.
This logic is on the rust-dashcore side; we shouldn't patch it here. You'd better make separate methods for testing only purposes that are available only for test builds.
There was a problem hiding this comment.
I mean, is not like we made this for testing, it was already there when I joined the company xd, but yeah, as I said, I can drop it, shouldn't be that big of a deal
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Talked with Ivan, we think it's best to have create_wallet_from_mnemonic take a bool for downgrading here. And add a downgrade method in rust dash core.
|
okay |
After the colleague's MnemonicResolverCoreSigner landed every signing path is signer-driven, so freshly-derived wallets no longer need to keep the root xpriv in-process. `create_wallet_from_mnemonic` and `create_wallet_from_seed_bytes` now derive accounts via `Wallet::from_mnemonic`/`from_seed_bytes` and immediately downgrade to an ExternalSignable wallet with the same wallet_id + account xpubs. The seed stays in the host's secure storage and the resolver vtable remains the only way to sign.
ZocoLini
force-pushed
the
feat/external-signable-wallets
branch
from
9f28fe6 to
f4f4d29
Compare
|
I updated the PR, and honestly, right now it doesn't make sense to me to add that boolean arg to the function if we are always setting it to true |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The actual PR delta is a 6-line change in packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs that calls wallet.downgrade_to_external_signable() after Wallet::from_mnemonic / Wallet::from_seed_bytes so the in-process xpriv is dropped immediately and signing must flow through the external signer vtable. Prior finding prior-1 (drop(temp) doesn't zeroize) is RESOLVED — the local clone+drop helper was replaced with a direct call to upstream Wallet::downgrade_to_external_signable(), which relies on standard Rust Drop chaining into key_wallet's ZeroizeOnDrop impls. Prior finding prior-2 (missing regression test for the ExternalSignable invariant) is STILL VALID and is carried forward as the only material residual issue; a docs-coverage suggestion is added.
Note: Inline posting failed (command failed (1): gh api /repos/dashpay/platform/pulls/3639/reviews --method POST --input -
STDOUT:
{"message":"Unprocessable Entity","errors":["Variable $threads of type [DraftPullRequestReviewThread] was provided invalid value for 0.commitId (Field is not defined on DraftPullRequestReviewThread)), so I posted the same verified findings as a top-level review body.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s)
suggestion: No regression test pins the new ExternalSignable invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 73)
The behavioral guarantee at the heart of this PR — that create_wallet_from_mnemonic and create_wallet_from_seed_bytes always return a wallet whose wallet_type == ExternalSignable and which holds no in-process xpriv — is not asserted by any test in packages/rs-platform-wallet. Grep finds only two call sites for these constructors (tests/spv_sync.rs:184 and examples/basic_usage.rs:59), and neither inspects the returned wallet's type or signing capabilities. The only thing keeping the guarantee alive is the single wallet.downgrade_to_external_signable(); line at wallet_lifecycle.rs:88 and :114; a future refactor that removes, reorders, or skips that call for one of the two constructors will silently regress the security model — the wallet will still work, SPV will still scan, and the only observable difference is that the xpriv now lives in process memory rather than going through the external resolver. Add a unit/integration test for each entry point that asserts wallet.is_external_signable() on the returned/managed wallet (and ideally that signing without an external resolver fails), so the invariant is locked in at the public API boundary. Carried forward from prior review at 9f28fe6 where the same gap was flagged.
nitpick: Doc-comments on the two creation entry points don't mention the ExternalSignable downgrade
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 52)
The doc-comments on create_wallet_from_mnemonic (52–72) and create_wallet_from_seed_bytes (92–100) cover language detection and birth-height semantics but say nothing about the load-bearing behavior introduced by this PR — that the returned wallet is WalletType::ExternalSignable and therefore holds no in-process xpriv. A caller reading the signature and docs would reasonably assume that feeding in a mnemonic produces a wallet that can sign from in-process key material, which is no longer true: the design now requires the host to register a MnemonicResolverCoreSigner (or equivalent) before any signing attempt, otherwise signing will fail at runtime. Document that contract at the public API surface so callers don't waste time debugging a 'wallet won't sign' error.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:73-116: No regression test pins the new ExternalSignable invariant
The behavioral guarantee at the heart of this PR — that `create_wallet_from_mnemonic` and `create_wallet_from_seed_bytes` always return a wallet whose `wallet_type == ExternalSignable` and which holds no in-process xpriv — is not asserted by any test in `packages/rs-platform-wallet`. Grep finds only two call sites for these constructors (`tests/spv_sync.rs:184` and `examples/basic_usage.rs:59`), and neither inspects the returned wallet's type or signing capabilities. The only thing keeping the guarantee alive is the single `wallet.downgrade_to_external_signable();` line at wallet_lifecycle.rs:88 and :114; a future refactor that removes, reorders, or skips that call for one of the two constructors will silently regress the security model — the wallet will still work, SPV will still scan, and the only observable difference is that the xpriv now lives in process memory rather than going through the external resolver. Add a unit/integration test for each entry point that asserts `wallet.is_external_signable()` on the returned/managed wallet (and ideally that signing without an external resolver fails), so the invariant is locked in at the public API boundary. Carried forward from prior review at 9f28fe6e where the same gap was flagged.
- [NITPICK] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:52-107: Doc-comments on the two creation entry points don't mention the ExternalSignable downgrade
The doc-comments on `create_wallet_from_mnemonic` (52–72) and `create_wallet_from_seed_bytes` (92–100) cover language detection and birth-height semantics but say nothing about the load-bearing behavior introduced by this PR — that the returned wallet is `WalletType::ExternalSignable` and therefore holds no in-process xpriv. A caller reading the signature and docs would reasonably assume that feeding in a mnemonic produces a wallet that can sign from in-process key material, which is no longer true: the design now requires the host to register a `MnemonicResolverCoreSigner` (or equivalent) before any signing attempt, otherwise signing will fail at runtime. Document that contract at the public API surface so callers don't waste time debugging a 'wallet won't sign' error.
4 participants
After Wallet::from_mnemonic and Wallet::from_seed I do a wallet downgrade into ExternableSignable, forcing every transaction to be signed externally
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit