feat: external signable wallets#3639
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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
from
b9ad462 to
dc7dafb
Compare
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
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 88.01% 87.84% -0.18%
============================================
Files 2521 2537 +16
Lines 309007 311315 +2308
============================================
+ Hits 271977 273471 +1494
- Misses 37030 37844 +814
🚀 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: "e5bb55681f2182394317fd310371cc905ceb33ed9756aa2edf42cebaabf40b28"
)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)
| fn downgrade_to_external_signable(temp: Wallet) -> Wallet { | ||
| let wallet_id = temp.wallet_id; | ||
| let network = temp.network; | ||
| let accounts = temp.accounts.clone(); | ||
| drop(temp); | ||
| Wallet::new_external_signable(network, wallet_id, accounts) |
There was a problem hiding this comment.
🟡 Suggestion: drop(temp) does not zeroize the xpriv the doc-comment promises to strip
The doc-comment states this helper exists to "strip the in-process xpriv from a freshly-derived wallet," but drop(temp) runs only the default Drop. key_wallet::Wallet defines a manual impl Zeroize (key-wallet/src/wallet/mod.rs:153) and has no Drop or ZeroizeOnDrop impl. As a result, the Mnemonic chain-code material, BIP-39 Seed, and RootExtendedPrivKey bytes are simply deallocated — they linger in freed heap until reused. The PR's stated motivation ("force every transaction to be signed externally", seed kept only in host secure storage) is satisfied only at the type level; the in-process secret residue is identical to before. The upstream key-wallet-manager performs this same downgrade and explicitly calls wallet.zeroize(); drop(wallet); (key-wallet-manager/src/lib.rs:237-238) — mirror that here. secp256k1::SecretKey still can't be wiped from Rust, but the mnemonic and chain-code material are scrubbed by Wallet::zeroize, which is strictly better than the current no-op.
| fn downgrade_to_external_signable(temp: Wallet) -> Wallet { | |
| let wallet_id = temp.wallet_id; | |
| let network = temp.network; | |
| let accounts = temp.accounts.clone(); | |
| drop(temp); | |
| Wallet::new_external_signable(network, wallet_id, accounts) | |
| 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(); | |
| // `Wallet` implements `Zeroize` but not `ZeroizeOnDrop`, so `drop` | |
| // alone leaves the mnemonic/seed/chain-code material in freed heap. | |
| // Mirror the upstream `key-wallet-manager` downgrade path. | |
| temp.zeroize(); | |
| drop(temp); | |
| Wallet::new_external_signable(network, wallet_id, accounts) | |
| } |
source: ['claude', 'codex']
| fn downgrade_to_external_signable(temp: Wallet) -> Wallet { | ||
| let wallet_id = temp.wallet_id; | ||
| let network = temp.network; | ||
| let accounts = temp.accounts.clone(); | ||
| drop(temp); | ||
| Wallet::new_external_signable(network, wallet_id, accounts) |
There was a problem hiding this comment.
🟡 Suggestion: Cloned accounts keep is_watch_only=false while the wallet becomes ExternalSignable
Wallet::from_mnemonic / from_seed_bytes populate accounts via Account::from_xpriv, which sets is_watch_only: false (key-wallet/src/account/mod.rs:97). This helper clones that AccountCollection verbatim and rebuilds the wallet as WalletType::ExternalSignable, so the result has wallet_type == ExternalSignable but every Account inside still reports is_watch_only == false. The mirror code path on restart — rs-platform-wallet-ffi/src/persistence.rs:1775,1788 — reconstructs the logically-equivalent wallet via Account::from_xpub + new_external_signable, which yields is_watch_only == true. A wallet created on first launch and the same wallet restored on the next launch therefore have different per-account flags. key_wallet branches on account.is_watch_only in places (e.g. Account::derive_xpriv_from_master_xpriv returns Error::WatchOnly when set), so the create vs restore divergence is fragile. Map the cloned accounts through Account::to_watch_only() (key-wallet/src/account/mod.rs:133) before passing them to new_external_signable so the two paths converge.
source: ['claude']
| seed_bytes: [u8; 64], | ||
| accounts: WalletAccountCreationOptions, | ||
| ) -> Result<Arc<PlatformWallet>, PlatformWalletError> { | ||
| let wallet = Wallet::from_seed_bytes(seed_bytes, network, accounts).map_err(|e| { | ||
| let temp = Wallet::from_seed_bytes(seed_bytes, network, accounts).map_err(|e| { | ||
| PlatformWalletError::WalletCreation(format!( | ||
| "Failed to create wallet from seed bytes: {}", | ||
| "Failed to derive accounts from seed bytes: {}", | ||
| e | ||
| )) | ||
| })?; | ||
| let wallet = downgrade_to_external_signable(temp); | ||
| self.register_wallet(wallet).await | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: New ExternalSignable invariant has no test
The PR's behavioral guarantee — that create_wallet_from_mnemonic and create_wallet_from_seed_bytes always return an ExternalSignable wallet — is not asserted by any test in the crate. The only existing call site (tests/spv_sync.rs:185) exercises create_wallet_from_seed_bytes but never inspects wallet_type. Without a regression test, a future edit to the helper or to register_wallet can silently restore in-process signing capability. A small unit test asserting matches!(wallet.wallet_type, WalletType::ExternalSignable) on the wallet returned from each manager constructor would lock the invariant in cheaply.
source: ['claude']
2 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