Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects missing hot-wallet private keys and adds watch-only import/restore flows across Rust, Android, and iOS: new AppAlertState variants, downgrade/reconcile logic emitting HotWalletKeyMissing, FFI enum/serialization expansions, and a new FFI call to set wallet type. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Mobile UI
participant App as App Manager
participant Keychain
participant DB as Database
participant Rust as FFI/Rust
User->>UI: Open wallet
UI->>App: Load wallet
App->>DB: Fetch wallet metadata
DB-->>App: Metadata (type=Hot)
App->>Keychain: Retrieve private key
alt Key present
Keychain-->>App: Key found
App-->>UI: Wallet usable
else Key missing
Keychain-->>App: Key not found
App->>Rust: downgrade_and_notify_if_needed
Rust->>DB: Update metadata -> WatchOnly (persist)
DB-->>Rust: Persisted
Rust-->>App: Queue HotWalletKeyMissing
App-->>UI: Emit HotWalletKeyMissing alert
UI->>User: Show import/watch-only options
User->>UI: Choose import method (Words/Hardware/QR/NFC/Paste)
UI->>Rust: Trigger import via FFI
Rust->>Keychain: Save private key / descriptors
Rust->>DB: Update metadata (Hot/Cold) & persist
DB-->>Rust: Persisted
Rust-->>App: Notify wallet restored
App-->>UI: Update UI (wallet restored)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR implements CSPP (Cove Security & Privacy Protocol) phase 1, adding wallet type upgrade/downgrade paths, duplicate wallet reconciliation, and backup exclusion for sensitive data.
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A[Wallet Load / Import] --> B{Wallet exists<br/>with same fingerprint?}
B -->|No| C[Create new wallet]
B -->|Yes| D{Current wallet type?}
D -->|Hot + key present| E[Show Duplicate Alert]
D -->|Hot + key missing| F[Restore key from mnemonic<br/>Keep as Hot]
D -->|WatchOnly| G{Import source?}
D -->|Cold / XpubOnly| H{Import source?}
G -->|Mnemonic| I[Save key + descriptors<br/>Upgrade to Hot]
G -->|Pubport/Xpub| J[Save xpub + descriptors<br/>Upgrade to Cold]
H -->|Mnemonic| I
H -->|Pubport/Xpub| K{Type is WatchOnly?}
K -->|Yes| J
K -->|No| E
subgraph "On Wallet Manager Init"
L[Load wallet] --> M{Hot wallet?}
M -->|No| N[Continue normally]
M -->|Yes| O{Private key in keychain?}
O -->|Yes| N
O -->|No| P[Downgrade to WatchOnly<br/>Send HotWalletKeyMissing alert]
end
P --> Q[User sees recovery options]
Q --> R[Import 12/24 words]
Q --> S[Use with Hardware Wallet]
Q --> T[Keep as Watch Only]
Last reviewed commit: e34647b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/src/wallet.rs (1)
266-270:⚠️ Potential issue | 🟠 MajorOrphaned SQLite database files created on watch-only → cold upgrade path.
BdkStore::try_new(&id, network)on line 269 creates a persistent SQLite database file (bdk_wallet_sqlite_{id}.db). When the upgrade path is taken (lines 312-333), this function returns early without using the store, leaving the SQLite file and its auxiliary WAL/SHM files orphaned on disk. These files accumulate on repeated imports of hardware wallets matching existing watch-only wallets.Move the
BdkStorecreation after the fingerprint/upgrade check, or explicitly delete the orphaned store files before returning from the upgrade path usingBdkStore::delete_sqlite_store().rust/src/manager/import_wallet_manager.rs (1)
112-124:⚠️ Potential issue | 🟠 MajorFingerprint lookup mechanisms differ: inconsistent sources could cause duplicate wallets.
Fingerprint::try_new()inimport_wallet_manager.rs(line 119) reads from the keychain viaKeychain::global().get_wallet_xpub(), whilewallet.rs(line 305) reads directly fromwallet_metadata.master_fingerprint. If a wallet's xpub is removed or unavailable in the keychain but the metadata fingerprint persists (e.g., due to corruption or migration), the import flow will not find the existing wallet and create a duplicate instead of upgrading it. The lookup inconsistency compounds this risk—Fingerprint::try_new()silently fails with.ok()?, filtering out wallets that metadata-based checks would find.ios/Cove/CoveApp.swift (1)
226-247:⚠️ Potential issue | 🟡 MinorDead cases:
.hotWalletKeyMissingand.confirmWatchOnlyare already handled above.Lines 242–243 list
.hotWalletKeyMissingand.confirmWatchOnlyin the default "OK" group, but they are already matched by the specificcase let .hotWalletKeyMissing(walletId:)at line 76 andcase .confirmWatchOnlyat line 95. These entries in the default group are unreachable dead code. SwiftLint also flags this asduplicate_conditions.Remove them from the grouped default to silence the warning and avoid confusion:
Proposed fix
case .invalidWordGroup, .errorImportingHotWallet, .importedSuccessfully, .unableToSelectWallet, .errorImportingHardwareWallet, .invalidFileFormat, .importedLabelsSuccessfully, .unableToGetAddress, .failedToScanQr, .noUnsignedTransactionFound, .tapSignerSetupFailed, .tapSignerInvalidAuth, .tapSignerDeriveFailed, .general, .invalidFormat, - .loading, - .hotWalletKeyMissing, - .confirmWatchOnly: + .loading: Button("OK") { app.alertState = .none }
🤖 Fix all issues with AI agents
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 420-452: In the AppAlertState.HotWalletKeyMissing handler, don’t
silently swallow exceptions from app.rust.setWalletType(walletId,
WalletType.COLD); instead catch the exception and either log it (using the same
logging approach used elsewhere in this file) and/or set app.alertState to an
error alert (e.g., TaggedItem(AppAlertState.SomeErrorState) or a toast) so the
user is informed that setWalletType failed; update the try/catch around
app.rust.setWalletType to capture the exception, log the error details, and
surface a user-facing alert via app.alertState rather than leaving the dialog
closed with no feedback.
In `@ios/Cove/CoveApp.swift`:
- Around line 87-90: The Button's action uses try? with
app.rust.setWalletType(id: walletId, walletType: .cold) which silently ignores
failures; replace the silent try? with a do-catch (or use Result) to catch
errors from setWalletType and surface them via app.alertState (e.g., set an
error alert message) and only clear app.alertState on success; locate the Button
action where app.alertState is set and update it to handle the thrown error from
app.rust.setWalletType and present a user-facing error alert.
- Around line 190-202: The Paste button handler currently creates a Wallet via
Wallet.newFromXpub and calls app.rust.selectWallet(id: wallet.id()) but does not
navigate to the newly selected wallet; update the Button action so that after a
successful selectWallet call it also calls
app.resetRoute(.SelectedWallet(wallet.id())) (or the
app.resetRoute(Route.SelectedWallet(id)) equivalent used elsewhere) so the UI
navigates to the imported wallet; keep the existing error handling around
Wallet.newFromXpub and selectWallet.
In `@rust/src/manager/import_wallet_manager.rs`:
- Around line 137-140: The current lookup uses
Database::global().wallets.get(&id, network, mode)? and maps a None to
ImportWalletError::WalletAlreadyExists(id.clone()), which is misleading when the
fingerprint exists but metadata is missing; change this to return a more
accurate error (e.g. add/import ImportWalletError::MissingMetadata(id.clone())
or reuse an appropriate existing variant) and replace the
None-to-WalletAlreadyExists mapping in the import flow (the get call handling in
import_wallet_manager.rs) so callers receive the correct MissingMetadata error;
update any matching/handling sites that expect WalletAlreadyExists accordingly.
In `@rust/src/manager/wallet_manager.rs`:
- Around line 261-278: The current sanity check comparing wallet.metadata with
Database::global().wallets.get(...) can fail if downgrade_and_notify_if_needed
mutates wallet.metadata in-memory then fails to persist; update the flow so the
DB is authoritative: either change downgrade_and_notify_if_needed to return a
Result and propagate persistence errors up from Wallet::try_load_persisted (so
the in-memory mutation only happens when the DB write succeeded), or move the
metadata sanity check (the comparison after Database::global().wallets.get) to
run before calling downgrade_and_notify_if_needed, or re-read the wallet
metadata from Database::global().wallets.get after
downgrade_and_notify_if_needed completes and use that value for the equality
check; reference the functions Wallet::try_load_persisted,
downgrade_and_notify_if_needed, and Database::global().wallets.get when making
the change.
- Around line 1363-1370: The current match on
Keychain::global().get_wallet_key(...) treats any Err as "no key" and sets
has_private_key = false, which can permanently downgrade wallets on transient
keychain failures; update the logic around
Keychain::global().get_wallet_key(&metadata.id) (the match that sets
has_private_key) to distinguish transient keychain errors from a definitive
Ok(None): inspect the returned Err (by error kind, variant, or an
is_locked/is_unavailable helper on the keychain error type), and for transient
errors either propagate the error (return Err from the surrounding function),
schedule/retry the lookup, or leave the wallet in its prior state instead of
setting has_private_key = false; keep the warn log but include the error and use
a different handling path than the Ok(None) branch so only Ok(None) triggers
permanent downgrade to watch-only.
In `@rust/src/wallet.rs`:
- Around line 298-310: The code is silently discarding database errors by
calling .unwrap_or(None) on the Result from database.wallets.get_all; replace
that swallow with proper error propagation so real DB errors are returned
instead of treated as "no wallets". Locate the block that builds existing using
database.wallets.get_all(network, mode) and remove the .unwrap_or(None), instead
propagate the Result (e.g., use ? on the get_all call or map the Err out) and
then perform the filter_map over the successful wallets to set existing; ensure
the containing function's signature permits returning the propagated error so
database failures are not masked.
🧹 Nitpick comments (4)
rust/src/wallet.rs (1)
312-333: No cleanup on partial failure during watch-only → cold upgrade.The upgrade path performs multiple fallible operations (save xpub, set wallet_type, save descriptors, update metadata, select wallet, load wallet). If any step after
save_wallet_xpubfails, previously persisted changes are not rolled back. This mirrors the existing pattern intry_new_persisted_and_selectedwhich does have cleanup logic (lines 177-198).Consider wrapping this in a similar cleanup-on-failure pattern, or at least documenting that partial state is acceptable here.
rust/src/manager/import_wallet_manager.rs (1)
126-157: No rollback on partial failure during wallet reconciliation.This multi-step reconciliation (save key → save xpub → save descriptors → update metadata → select wallet) has no cleanup if an intermediate step fails. For example, if
save_public_descriptor(line 144) fails after the mnemonic was already saved (line 131), the keychain will contain the private key but the wallet won't be updated toHottype, leaving inconsistent state.Consider adding cleanup logic similar to
Wallet::try_new_persisted_and_selectedinwallet.rs.rust/src/app.rs (1)
422-441: No validation on wallet type transitions.
set_wallet_typeallows setting anyWalletType(includingHot) without verifying that the prerequisite state exists (e.g., private key in the keychain). A frontend bug or misuse could set a wallet toHotwhen no mnemonic is available, causing send-flow failures that are harder to diagnose than an upfront error.Consider adding a guard, at minimum for the
Hottransition:Suggested validation
+ if wallet_type == WalletType::Hot { + let has_key = Keychain::global().get_wallet_key(&id).ok().flatten().is_some(); + if !has_key { + return Err(DatabaseError::WalletNotFound); // or a more specific error + } + } + metadata.wallet_type = wallet_type;rust/src/app/alert_state.rs (1)
109-122: OS-specific backup messaging looks good.The
cfg(target_os)approach for tailoring the backup-type wording is clean. Minor nit: on Android the backup system is typically called "Google backup" rather than just "Android" — consider whether "Google" or "Android Auto Backup" would be clearer to end users.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/Cove/CoveApp.swift (1)
228-248:⚠️ Potential issue | 🟡 MinorDead patterns:
.hotWalletKeyMissingand.confirmWatchOnlyare already handled above.These two variants are matched by dedicated
casearms at lines 76 and 95, so they are unreachable here. SwiftLint also flags this asduplicate_conditions. Remove them from this group.Proposed fix
case .invalidWordGroup, .errorImportingHotWallet, .importedSuccessfully, .unableToSelectWallet, .errorImportingHardwareWallet, .invalidFileFormat, .importedLabelsSuccessfully, .unableToGetAddress, .failedToScanQr, .noUnsignedTransactionFound, .tapSignerSetupFailed, .tapSignerInvalidAuth, .tapSignerDeriveFailed, .general, .invalidFormat, - .loading, - .hotWalletKeyMissing, - .confirmWatchOnly: + .loading: Button("OK") { app.alertState = .none }
🤖 Fix all issues with AI agents
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 741-762: The Wallet returned by Wallet.newFromXpub is
AutoCloseable and must be closed after use to avoid UniFFI resource leaks; wrap
creation and use in a Kotlin use block (or ensure wallet.close() is called) so
you obtain the id via the Wallet instance inside the block and then call
app.rust.selectWallet(id) and app.resetRoute(Route.SelectedWallet(id)) before
the wallet is closed; update the Paste button handler to use
Wallet.newFromXpub(...) with wallet.use { ... } (or explicit close) and preserve
the existing error handling via the catch block.
In `@ios/Cove/CoveApp.swift`:
- Around line 95-98: The confirmWatchOnly alert's "I Understand" button only
clears app.alertState and never persists the wallet type change; update the
Button handler in the case .confirmWatchOnly (the Button("I Understand")
closure) to call app.setWalletType(id: walletId, walletType: .watchOnly) (or the
app-equivalent API used for persisting wallet type) before setting
app.alertState = .none so the wallet is actually converted to watch-only and
then the alert dismissed.
|
@greptile-apps re-review |
praveenperera
force-pushed
the
cspp-phase-1
branch
from
01e8b1e to
5ee2efc
Compare
|
@greptile-apps re-review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/src/wallet.rs (2)
266-269:⚠️ Potential issue | 🟠 MajorOrphaned BDK store created when taking the WatchOnly → Cold upgrade path.
When the upgrade branch (line 308–328) is taken, the
BdkStorecreated on line 269 for the newidis never used and its SQLite file is left on disk. Each re-import of a matching pubport export leaks an orphaned file.Move
BdkStore::try_new(and the newWalletId) below the upgrade check so they are only allocated when a new wallet is actually needed, or clean up the unused store in the early-return path.Suggested restructure (move store creation after upgrade check)
let pubport_descriptors = match pubport { // ... (match arms unchanged) }; let fingerprint = pubport_descriptors.fingerprint(); // compute xpub and descriptors early let xpub = pubport_descriptors.xpub()...; let descriptors: Descriptors = pubport_descriptors.into(); - let id = WalletId::new(); - let mut metadata = WalletMetadata::new_for_hardware(id.clone(), "", None); - let mut store = BdkStore::try_new(&id, network).map_err_str(WalletError::LoadError)?; - // check for existing wallet with same fingerprint, upgrade watch-only → cold if let Some(fingerprint) = fingerprint.as_ref() { // ... upgrade path (unchanged) ... } + let id = WalletId::new(); + let mut metadata = WalletMetadata::new_for_hardware(id.clone(), "", None); + let mut store = BdkStore::try_new(&id, network).map_err_str(WalletError::LoadError)?; + // ... rest of new-wallet creation ...Note:
metadatais also mutated inside the fingerprint block (line 296), so you would need to split the metadata initialization or pass the fingerprint into the block differently. But the store allocation is the main concern.#!/bin/bash # Verify that `metadata` fields set inside the fingerprint block are only used in the upgrade path # or also needed for the new-wallet path, to assess how to restructure. rg -n 'metadata\.' rust/src/wallet.rs | head -60Also applies to: 308-329
663-687:⚠️ Potential issue | 🟠 Major
check_for_duplicate_walletstill silently swallows database errors.The same
.unwrap_or_default()pattern that was just fixed intry_new_persisted_from_pubport(line 300) persists here on line 680. A DB failure is treated as "no wallets found," allowing a duplicate wallet to be created silently.Proposed fix: propagate the error
fn check_for_duplicate_wallet( network: Network, mode: metadata::WalletMode, fingerprint: Fingerprint, ) -> Result<(), WalletError> { let all_fingerprints: Vec<(WalletId, Arc<Fingerprint>)> = Database::global() .wallets - .get_all(network, mode) - .map(|wallets| { - wallets - .into_iter() - .filter_map(|wallet_metadata| { - let fingerprint = wallet_metadata.master_fingerprint?; - Some((wallet_metadata.id, fingerprint)) - }) - .collect() - }) - .unwrap_or_default(); + .get_all(network, mode)? + .into_iter() + .filter_map(|wallet_metadata| { + let fingerprint = wallet_metadata.master_fingerprint?; + Some((wallet_metadata.id, fingerprint)) + }) + .collect(); if let Some((id, _)) = all_fingerprints.into_iter().find(|(_, f)| f.as_ref() == &fingerprint) { return Err(WalletError::WalletAlreadyExists(id)); } Ok(()) }ios/Cove/CoveApp.swift (1)
239-259:⚠️ Potential issue | 🟡 MinorDuplicate switch cases:
.hotWalletKeyMissingand.confirmWatchOnlyare unreachable here.Lines 255–256 duplicate cases already handled at Lines 76 and 105 respectively. Swift matches cases top-to-bottom, so these entries are dead code and the generic "OK" button will never be shown for these states. This is also what SwiftLint flagged (
duplicate_conditions).Remove both from the catch-all group:
Proposed fix
case .invalidWordGroup, .errorImportingHotWallet, .importedSuccessfully, .unableToSelectWallet, .errorImportingHardwareWallet, .invalidFileFormat, .importedLabelsSuccessfully, .unableToGetAddress, .failedToScanQr, .noUnsignedTransactionFound, .tapSignerSetupFailed, .tapSignerInvalidAuth, .tapSignerDeriveFailed, .general, .invalidFormat, - .loading, - .hotWalletKeyMissing, - .confirmWatchOnly: + .loading: Button("OK") { app.alertState = .none }
🧹 Nitpick comments (2)
ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportScreen.swift (1)
228-229: MissingMetadata error is silently swallowed — user sees no feedback.Like several other branches here (
.Keychain,.Database,.BdkError), this only logs the error. For a "wallet metadata missing" scenario — which indicates data corruption or loss — the user should arguably be informed so they can take action (e.g., re-import). Consider surfacing this via an alert, at least for this specific case.Proposed improvement
case let .MissingMetadata(walletId): - Log.error("Wallet metadata missing for \(walletId)") + Log.error("Wallet metadata missing for \(walletId)") + app.alertState = TaggedItem(.general( + title: "Import Error", + message: "Wallet metadata is missing. Please try importing again." + ))ios/Cove/CoveApp.swift (1)
200-215: Nit:wallet.id()called twice — store in a local.
wallet.id()is called on both Line 206 and Line 207. While likely inexpensive, extracting it to a localletis cleaner and consistent with howimportColdWallet(Line 329) handles this.Proposed fix
Button("Paste") { app.alertState = .none let text = UIPasteboard.general.string ?? "" if text.isEmpty { return } do { let wallet = try Wallet.newFromXpub(xpub: text) - try app.rust.selectWallet(id: wallet.id()) - app.resetRoute(to: .selectedWallet(wallet.id())) + let id = wallet.id() + try app.rust.selectWallet(id: id) + app.resetRoute(to: .selectedWallet(id)) } catch {
praveenperera
force-pushed
the
cspp-phase-1
branch
from
1cbf2dc to
62a691c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/Cove/CoveApp.swift (1)
239-259:⚠️ Potential issue | 🟡 MinorDuplicate switch cases:
hotWalletKeyMissingandconfirmWatchOnlyare handled twice.Lines 76–108 already handle
.hotWalletKeyMissingand.confirmWatchOnlywith specific buttons. Including them again in the catch-all at lines 255–256 creates dead code branches that SwiftLint flags asduplicate_conditions. Remove them from the catch-all group.Proposed fix
case .invalidWordGroup, .errorImportingHotWallet, .importedSuccessfully, .unableToSelectWallet, .errorImportingHardwareWallet, .invalidFileFormat, .importedLabelsSuccessfully, .unableToGetAddress, .failedToScanQr, .noUnsignedTransactionFound, .tapSignerSetupFailed, .tapSignerInvalidAuth, .tapSignerDeriveFailed, .general, .invalidFormat, - .loading, - .hotWalletKeyMissing, - .confirmWatchOnly: + .loading: Button("OK") { app.alertState = .none }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/CoveApp.swift` around lines 239 - 259, The switch contains duplicate cases: remove .hotWalletKeyMissing and .confirmWatchOnly from the catch-all case group so the earlier specific handlers remain effective; locate the switch handling app.alertState in CoveApp.swift and delete those two enum cases from the grouped list (leaving the Button("OK") fallback for the remaining cases) to eliminate the duplicate_conditions lint warning.
🧹 Nitpick comments (3)
ios/Cove/TaggedItem.swift (1)
18-25: Identity-only equality — reasonable trade-off, but document the intent.Removing the
T: Equatableconstraint is a pragmatic choice to support non-Equatable payloads (e.g., FFI-generated alert state types). The custom==comparing onlyidmeans two independently createdTaggedItems wrapping the same value will never be equal, since each gets a freshUUID.This is fine for the primary use case (SwiftUI optional-binding for sheets/alerts), but could surprise anyone who later uses
TaggedItemin collections (Set,Dictionarykey) or value-based equality checks. A brief doc comment on the==override would help future readers understand the deliberate semantics.📝 Suggested doc comment
struct TaggedItem<T>: Identifiable, Equatable { let id = UUID() let item: T + /// Equality is based solely on `id` (instance identity) rather than the + /// wrapped `item`, because `T` is not constrained to `Equatable`. static func == (lhs: TaggedItem, rhs: TaggedItem) -> Bool { lhs.id == rhs.id }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/TaggedItem.swift` around lines 18 - 25, The equality implementation for TaggedItem intentionally compares only the generated id (UUID) so two instances wrapping the same payload are not equal; add a short doc comment on the TaggedItem type or the static func == (lhs:rhs:) explaining that T is not constrained to Equatable, equality is identity-only (uses id), and this is deliberate for SwiftUI sheet/alert usage to avoid value-based equality surprises when used in Sets/Dictionaries or value comparisons; reference TaggedItem, id, item, and the custom == implementation in your comment.android/app/build.gradle.kts (1)
84-84: Duplicatelifecycle-runtime-ktxdependency.
androidx.lifecycle:lifecycle-runtime-ktx:2.10.0is declared on both Line 84 and Line 107. Remove one to avoid confusion.Proposed fix
- implementation("androidx.lifecycle:lifecycle-runtime-ktx:2.10.0") implementation("androidx.lifecycle:lifecycle-runtime-compose:2.10.0") + implementation("androidx.lifecycle:lifecycle-runtime-ktx:2.10.0")i.e., remove Line 84 and keep the one at Line 107 grouped with other lifecycle dependencies.
Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/build.gradle.kts` at line 84, The Gradle file declares a duplicate dependency implementation("androidx.lifecycle:lifecycle-runtime-ktx:2.10.0"); remove the redundant declaration so the artifact is only declared once (keep the entry that is grouped with the other lifecycle dependencies), i.e., delete the extra implementation("androidx.lifecycle:lifecycle-runtime-ktx:2.10.0") occurrence and ensure only the grouped lifecycle declarations (including lifecycle-runtime-ktx:2.10.0) remain.rust/src/manager/wallet_manager.rs (1)
914-927:set_wallet_typesilently swallows DB persistence failures.The method logs the error but does not return it. If the DB write fails, the in-memory cache and reconciler reflect the new type, but the database retains the old type. On next wallet load, the wallet reverts to the old type. This is consistent with the pattern used in
dispatch(line 1284) andvalidate_metadata(line 950), so it's an existing design choice — just noting it for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/manager/wallet_manager.rs` around lines 914 - 927, set_wallet_type currently applies the new WalletType to the in-memory metadata and sends Message::WalletMetadataChanged before attempting Database::global().wallets.update_wallet_metadata, which causes DB failures to be silently inconsistent; change set_wallet_type to return a Result (e.g., Result<(), WalletManagerError>), attempt the DB update first (call update_wallet_metadata with the new metadata), and only on Ok update the in-memory metadata and call self.reconciler.send(Message::WalletMetadataChanged(...)); on DB error return Err(error) (or map it to your manager error type) so callers can handle persistence failures—refer to set_wallet_type, Database::global().wallets.update_wallet_metadata, self.reconciler.send(Message::WalletMetadataChanged) and follow the error-return pattern used by dispatch and validate_metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 420-458: The HotWalletKeyMissing AlertDialog
(AppAlertState.HotWalletKeyMissing) lacks an explicit dismiss/cancel button; add
a dismissButton parameter to the AlertDialog (alongside the existing
confirmButton) that renders a TextButton (e.g., labeled "Cancel" or "Dismiss")
which calls onDismiss() when clicked, mirroring the pattern used by
CantSendOnWatchOnlyWallet and improving discoverability/accessibility.
In `@android/build.gradle.kts`:
- Around line 3-4: The AGP version string for the Android Gradle plugin is
incorrect—replace the version value in the id("com.android.application")
declaration from "9.0.1" to a valid release (e.g., "9.0.0") or change it to the
intended pre-release like "9.1.0-alpha" if that was desired; update only the
version literal in the id("com.android.application") declaration and then
re-sync/verify the build to ensure compatibility with the Kotlin Compose plugin
already set to "2.3.10".
In `@ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportScreen.swift`:
- Around line 229-230: The .MissingMetadata(walletId) branch currently only logs
the error; update it to also surface a user-facing alert like the other error
branches (Keychain, Database) so the user knows the import failed — keep the
Log.error call but also call the existing UI alert helper used elsewhere in
HotWalletImportScreen (e.g., presentErrorAlert / showImportFailureAlert or the
same method used in the Keychain/Database cases) with a brief message such as
"Import failed: wallet metadata missing", and ensure the alert is presented on
the main thread.
In `@rust/src/app.rs`:
- Around line 422-441: FfiApp::set_wallet_type currently updates DB but leaves
RustWalletManager's in-memory cache stale; update this method to either delegate
to the existing RustWalletManager::set_wallet_type (or equivalent manager
instance) so the manager updates its cached WalletMetadata and triggers the
reconciler/WalletMetadataChanged event, or after persisting call into
RustWalletManager to refresh its metadata and emit the change; ensure you
reference and update the manager instance used by the app (RustWalletManager)
rather than only mutating the DB so the UI sees the new WalletType immediately.
In `@rust/src/manager/import_wallet_manager.rs`:
- Around line 117-123: The code is swallowing DB errors by calling
unwrap_or_default() on Database::global().wallets.get_all, which treats failures
as "no wallets" and can create duplicate wallets; change the call to propagate
the error instead (remove unwrap_or_default and use the Result from get_all,
e.g. return Err or use the ? operator) so import flow can fail fast and
reconcile existing entries; update the surrounding function signature to return
a Result if needed and ensure wallet matching still uses
wallet_metadata.wallet_matches_fingerprint(fingerprint) on the successful
get_all result.
- Around line 67-68: Remove the dead error variant MissingMetadata(WalletId)
from the error enum in import_wallet_manager.rs: delete the enum variant
declaration and any associated use/import of WalletId that only existed for this
variant, and update any pattern matches or references to this variant (if
present) so the enum remains exhaustive; ensure Cargo builds and run tests to
confirm no other code constructs this error.
---
Outside diff comments:
In `@ios/Cove/CoveApp.swift`:
- Around line 239-259: The switch contains duplicate cases: remove
.hotWalletKeyMissing and .confirmWatchOnly from the catch-all case group so the
earlier specific handlers remain effective; locate the switch handling
app.alertState in CoveApp.swift and delete those two enum cases from the grouped
list (leaving the Button("OK") fallback for the remaining cases) to eliminate
the duplicate_conditions lint warning.
---
Duplicate comments:
In `@android/app/src/main/java/org/bitcoinppl/cove/MainActivity.kt`:
- Around line 756-767: The Wallet returned by Wallet.newFromXpub(...) is an
AutoCloseable UniFFI resource that's not closed; wrap its usage to ensure
deterministic cleanup (either use Kotlin's use { ... } or a try/finally that
calls wallet.close()). Specifically, obtain the Wallet from
Wallet.newFromXpub(text.trim()), call wallet.id() inside the scoped block, then
call app.rust.selectWallet(id) and app.resetRoute(Route.SelectedWallet(id))
before the block exits so wallet.close() runs even if exceptions occur; keep the
existing catch handling for AppAlertState.ErrorImportingHardwareWallet.
---
Nitpick comments:
In `@android/app/build.gradle.kts`:
- Line 84: The Gradle file declares a duplicate dependency
implementation("androidx.lifecycle:lifecycle-runtime-ktx:2.10.0"); remove the
redundant declaration so the artifact is only declared once (keep the entry that
is grouped with the other lifecycle dependencies), i.e., delete the extra
implementation("androidx.lifecycle:lifecycle-runtime-ktx:2.10.0") occurrence and
ensure only the grouped lifecycle declarations (including
lifecycle-runtime-ktx:2.10.0) remain.
In `@ios/Cove/TaggedItem.swift`:
- Around line 18-25: The equality implementation for TaggedItem intentionally
compares only the generated id (UUID) so two instances wrapping the same payload
are not equal; add a short doc comment on the TaggedItem type or the static func
== (lhs:rhs:) explaining that T is not constrained to Equatable, equality is
identity-only (uses id), and this is deliberate for SwiftUI sheet/alert usage to
avoid value-based equality surprises when used in Sets/Dictionaries or value
comparisons; reference TaggedItem, id, item, and the custom == implementation in
your comment.
In `@rust/src/manager/wallet_manager.rs`:
- Around line 914-927: set_wallet_type currently applies the new WalletType to
the in-memory metadata and sends Message::WalletMetadataChanged before
attempting Database::global().wallets.update_wallet_metadata, which causes DB
failures to be silently inconsistent; change set_wallet_type to return a Result
(e.g., Result<(), WalletManagerError>), attempt the DB update first (call
update_wallet_metadata with the new metadata), and only on Ok update the
in-memory metadata and call
self.reconciler.send(Message::WalletMetadataChanged(...)); on DB error return
Err(error) (or map it to your manager error type) so callers can handle
persistence failures—refer to set_wallet_type,
Database::global().wallets.update_wallet_metadata,
self.reconciler.send(Message::WalletMetadataChanged) and follow the error-return
pattern used by dispatch and validate_metadata.
Upgrade Gradle 8.13 → 9.3.1, AGP 8.13.1 → 9.0.1, Kotlin 2.2.10 → 2.3.10. Remove kotlin-android plugin (AGP 9 has built-in Kotlin support) and stale composeOptions block. Update dependencies: compose-bom, lifecycle, activity-compose, jna, accompanist, mlkit, zxing, coil, security-crypto, runtime-livedata.
When a hot wallet's private key is missing from the keychain, automatically downgrade it to a cold wallet on load. When the user re-imports the same mnemonic, restore the private key and upgrade the wallet back to hot instead of returning a "wallet already exists" error.
Move the hot-wallet-downgrade notification into the Rust layer so both iOS and Android get it automatically via the reconcile message channel, instead of each frontend querying the DB before/after load.
When importing an xpub for a wallet that already exists as watch-only, upgrade it to a cold wallet instead of returning WalletAlreadyExists. Saves xpub, public descriptors, and updates wallet type/origin metadata, mirroring the existing hot wallet upgrade path for seed word imports.
Watch-only wallets can be upgraded back via xpub or seed word import, while cold wallets cannot. This makes recovery possible when a hot wallet loses its private key from the keychain.
When a watch-only wallet tries to send, present options to upgrade it by importing hardware wallet keys (QR, NFC, paste) or seed words (QR, NFC, 12/24 words). Adds set_wallet_type FFI method, new WatchOnlyImportHardware/WatchOnlyImportWords alert states, and WalletNotFound database error.
Wrap alert-to-alert state changes in DispatchQueue.main.async on iOS to prevent SwiftUI's dismiss binding from overwriting the new alert state. Use pushRoute instead of loadAndReset so users can swipe back to the wallet after starting an import flow, on both iOS and Android.
Surface setWalletType errors to the user on both Android and iOS instead of silently swallowing them. Add navigation after xpub paste import on iOS. In Rust, propagate keychain and DB errors from downgrade check instead of swallowing them, add MissingMetadata error variant for the correct semantic, and propagate get_all errors in duplicate detection.
praveenperera
force-pushed
the
cspp-phase-1
branch
from
357a2db to
370583e
Compare
If the fingerprint matches a wallet that is already hot, select it and return WalletAlreadyExists instead of silently overwriting its private key material with the newly imported mnemonic.
Route "Use with Hardware Wallet" through WalletManager.setWalletType() instead of FfiApp.setWalletType() so the cache and reconcile channel are updated. Delete the now-unused FfiApp.setWalletType() method. Add ClearCachedWalletManager app reconcile message so the frontend drops its stale WalletManager when a watch-only wallet is upgraded to cold via xpub import.
When importing a hardware export that matches an existing watch-only wallet, upgrade the wallet type to cold without navigating away from the current screen. Also adds a db field comment on Wallet struct.
praveenperera
force-pushed
the
cspp-phase-1
branch
from
a2e9242 to
a58ab02
Compare
Screen-level @State alert was racing with the route change from selectWallet — the UIAlertController dismissal animation overlapped with the LoadAndResetContainer delay, causing the "Import Successful" alert to hang on xpub and QR hardware imports. App-level alertState persists across route changes so the alert shows on the destination wallet screen instead.
praveenperera
force-pushed
the
cspp-phase-1
branch
from
a96c0be to
31beada
Compare
|
@greptile-apps re-review |
Additional Comments (1)
When the existing-wallet upgrade path is taken (early return at line 333), the Each time a user re-imports a hardware wallet that matches an existing watch-only wallet, a new orphaned Then defer the |
Only check the wallet to see if fingerprint matches
…nditional cache clear - Defer BdkStore creation past fingerprint duplicate check so the upgrade early-return path no longer orphans a SQLite file on disk - Make set_wallet_type return Result and write DB before updating in-memory state, so failures are surfaced instead of silently reverted - Check walletId before clearing cached WalletManager on Android, matching existing iOS behavior
praveenperera
force-pushed
the
cspp-phase-1
branch
from
ea74d75 to
5aeb1e2
Compare
|
@greptile-apps re-review |
praveenperera
force-pushed
the
cspp-phase-1
branch
from
cf82328 to
7a9c6b2
Compare
The hot upgrade path (importing mnemonic into WatchOnly wallet) was missing the cache invalidation that upgrade_to_cold already had, leaving a stale WatchOnly manager in memory until the user navigated away and back.
Add explanatory comments in AppManager on Android and iOS to document that multiple screens within the same wallet (send, coin control, tx details, settings) should call getWalletManager to reuse the cached walletManager and avoid recreating the actor and reconciler. This makes the intended caching behavior clearer in both platforms.
1 participant
Closes: #554
Closes: #555
Closes: #556
Summary by CodeRabbit
New Features
Bug Fixes
Chores