Conversation
The old logic added +1.1 sat/vb offsets between tiers that cascaded, turning API values of (0.1, 0.5, 1.0) into displayed rates of (1.2, 2.3, 3.4). Replace with a 1.0 sat/vb floor and 0.1 minimum tier gaps so low fees correctly show (1.0, 1.1, 1.2) while high fees pass through unchanged.
Use the API-provided minimum_fee as a floor (maxed with MIN_FEE_RATE) when computing slow/derived fee tiers so rates never fall below the API minimum. Update assertions to use to_sat_per_kwu() conversions and add unit tests covering: minimum_fee above the hardcoded floor and minimum_fee lifting the slow tier above the raw midpoint.
* Fix descriptor export for Sparrow compatibility
Export descriptors as a single BIP-389 multipath descriptor with <0;1>
notation and h-hardened notation, matching Coldcard's default format.
BDK's miniscript uses ' (apostrophe) for hardened steps, but Sparrow
normalizes to h before validating the checksum, which causes a mismatch.
The new to_normalized_string() and to_multipath_string() methods on
DescriptorExt produce h-notation strings with correctly recomputed
checksums.
* Add test for descriptor Display omitting checksum
Add a unit test that verifies the alternate Display format ({:#}) for Descriptor omits the checksum. The test parses a wpkh descriptor, compares normal and alternate formatting, and asserts that recomputing the checksum from the alternate body reproduces the normal output. This guards against upstream changes to miniscript/display behavior.
Add IPHONEOS_DEPLOYMENT_TARGET = "18.0" to rust/.cargo/config.toml to set the minimum iOS deployment target. Change reqwest feature from "rustls" to "rustls-no-provider" in rust/Cargo.toml (both direct and workspace entries) and update Cargo.lock accordingly.
Adapt cove-util to the updated rand API by importing rand::RngExt and calling rand::rng().fill(&mut chain_code) directly (removing the intermediate rng binding). Cargo.lock was also updated/regen'd to reflect dependency changes.
Replace rand::Rng with rand::RngExt for methods like random_range, random_bool, random, and fill. Replace rand::RngCore with rand::Rng for next_u32 in deterministic_random_draw.
…leanup - Use loadAndReset instead of pushRoute for import word buttons to prevent infinite alert loop on back press - Swap "Use as Watch Only" and "Use with Hardware Wallet" button order to match iOS - Remove close() call in clearWalletManager to avoid cancelling in-flight reconcile messages, matching iOS behavior - Remove dead code in iOS alertButtons catch-all branch - Regenerate UniFFI Swift bindings
Add rustls dependency to Cargo.toml (including workspace entry) and enable the ring feature so a crypto backend is available for rustls. Initialize the ring provider at startup by calling rustls::crypto::ring::default_provider().install_default().ok() in init_on_start to satisfy reqwest builds that use rustls-no-provider.
Release version bump to 1.2.2 across platforms: Android app versionCode incremented 15->16 and versionName 1.2.1->1.2.2 (android/app/build.gradle.kts); iOS MARKETING_VERSION set to 1.2.2 and CURRENT_PROJECT_VERSION incremented 62->63 (ios/Cove.xcodeproj/project.pbxproj); Rust crate version updated to 1.2.2 in Cargo.toml and reflected in Cargo.lock (rust/). This prepares the codebase for the 1.2.2 release.
Fixes Android crash where rustls-platform-verifier panics without JNI init, poisoning the FIAT_CLIENT/FEE_CLIENT LazyLock singletons. Build reqwest clients with a pre-configured rustls ClientConfig using webpki-roots for cert verification. Also move ring provider install to App::new() so it runs before any LazyLock client initialization.
Replace onScrollGeometryChange (which depends on contentInsets.top that fluctuates during iOS 26 Liquid Glass transitions) with onGeometryChange on the header view, measuring position in stable global coordinates. Add 10pt hysteresis dead zone to prevent threshold oscillation.
The simultaneousGesture on SidebarView could set isDragging=true from a slight finger movement during a button tap, causing the onChange handler to skip the close animation. Also guard onDragEnded against stale gesture data reopening a programmatically closed sidebar.
SwiftUI has a bug where ToolbarPlacementEnvironment.updateValue() enters an infinite loop during _UINavigationParallaxTransition when a .principal toolbar item exists at accessibilityExtraExtraLarge dynamic type size. Bypass .principal entirely by hosting the SwiftUI title content inside a UIHostingController assigned to UIKit's navigationItem.titleView. This preserves the centered title appearance while avoiding the SwiftUI toolbar placement system.
Use .navigationTitle with .toolbarColorScheme(.dark) for screens with
plain static titles on dark backgrounds. Add .navigationTitleView { }
View extension for screens needing custom title content (shield icon,
context menu). Document the iOS 26 .principal toolbar bug in CLAUDE.md.
Switch ML Kit barcode scanning from the play-services thin client to the bundled variant (com.google.mlkit:barcode-scanning) which embeds the ML model in the APK and has zero GMS runtime dependency. Add StrongBox fallback in KeychainAccessor: check device capability via PackageManager before requesting StrongBox, and retry with TEE-only key if StrongBox-backed initialization fails. If StrongBox was not used, the exception propagates unchanged (fail-closed).
Increment CURRENT_PROJECT_VERSION from 65 to 66 in ios/Cove.xcodeproj/project.pbxproj for the affected build configurations to update the app build number.
📝 WalkthroughWalkthroughThis PR delivers version 1.2.2 across iOS, Android, and Rust with iOS 26 SwiftUI compatibility fixes, wallet load state tracking from Rust, updated dependencies, fee client logic refinement, and Android ML Kit barcode scanning dependency migration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
Greptile SummaryThis is the v1.2.2 release PR, bundling a significant set of bug fixes, stability improvements, and dependency upgrades across Rust, iOS, and Android. The changes are well-scoped and clearly motivated by real user-facing issues. Key bug fixes:
Stability improvements:
Notable dependency updates:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant KWM as Kotlin/Swift WalletManager
participant RWM as RustWalletManager (Rust)
participant DB as SQLite/redb Cache
UI->>KWM: init(walletId)
KWM->>RWM: RustWalletManager(id)
RWM->>DB: load cached transactions
DB-->>RWM: Vec<Transaction>
RWM->>RWM: set initial_load_state<br/>(Loading if empty, Scanning(txns) if cached)
KWM->>RWM: initialLoadState()
RWM-->>KWM: WalletLoadState (Loading | Scanning(txns) | Loaded(txns))
KWM->>UI: render with cached state (no spinner flash)
Note over RWM: Background sync begins
RWM-->>KWM: reconcile(StartedInitialFullScan)
KWM->>KWM: if SCANNING & txns non-empty → keep SCANNING<br/>if LOADED → transition to SCANNING(txns)<br/>else → LOADING
KWM->>UI: update UI state
RWM-->>KWM: reconcile(AvailableTransactions)
KWM->>UI: render final LOADED state
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
rust/src/wallet/metadata.rs (1)
51-51:⚠️ Potential issue | 🟡 MinorTypo in comment.
Minor typo: "transactions i the transaction list" should be "transactions in the transaction list".
📝 Suggested fix
- /// Show labels for transactions i the transaction list + /// Show labels for transactions in the transaction listAs per coding guidelines:
rust/**/*.rs: Identify spelling mistakes in comments, string literals, and documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/wallet/metadata.rs` at line 51, Fix the typo in the doc comment in metadata.rs: change the comment string "Show labels for transactions i the transaction list" to "Show labels for transactions in the transaction list" so the documentation/readme for the related item (the doc comment above the wallet metadata declaration) reads correctly.rust/src/keys.rs (1)
337-344:⚠️ Potential issue | 🔴 CriticalFix moved-value usage in
Fromconversion match arm—proposed fix is incorrect.At Line 343,
erroris matched by value, making the subsequenterror.to_string()call invalid. The proposed fixE::NotMatchingPair.to_string()also won't compile sinceNotMatchingPairis a unit variant without associated data.Use one of these approaches instead:
- Match by reference: Change
match errortomatch &errorto borrow rather than consume.- Hardcode the message: Since
NotMatchingPairis a unit variant with a fixed error message, use:E::NotMatchingPair => Self::UnsupportedDescriptor("descriptors are not a matching external/internal pair".to_string()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/keys.rs` around lines 337 - 344, The From conversion currently consumes `error` and then tries to reuse it; change `match error` to `match &error` to borrow the enum and avoid moved-value issues, adjust the arms to clone the borrowed strings for `E::UnsupportedDescriptor(s)` and `E::UnsupportedDescriptorType(s)` (e.g. `Self::UnsupportedDescriptor(s.clone())`), keep `E::NoOrigin => Self::NoOrigin`, and for `E::NotMatchingPair` return a fixed message (e.g. `Self::UnsupportedDescriptor("descriptors are not a matching external/internal pair".to_string())`) instead of calling `to_string()` on a moved unit variant; these changes should be applied inside the `fn from(error: cove_bdk::descriptor_ext::Error) -> Self` match block referencing `cove_bdk::descriptor_ext::Error` and its variants.
🧹 Nitpick comments (3)
ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift (1)
110-112: Avoid showing the same screen title twice.The body already renders a prominent
Recovery Wordsheading, so Lines 110-112 repeat the same label in the navigation bar and burn vertical space on an already dense view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift` around lines 110 - 112, Remove the duplicate navigation bar title by deleting the .navigationTitle("Recovery Words") and .navigationBarTitleDisplayMode(.inline) modifiers in SecretWordsScreen so the prominent in-body "Recovery Words" heading remains the primary label; you may keep .toolbarColorScheme(.dark, for: .navigationBar) if you still want a dark nav appearance, but do not render the same string in both the view body and the navigation bar.ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift (1)
115-115: Drop the duplicated inline title-display modifier.
navigationBarTitleDisplayMode(.inline)appears twice in the same chain; keeping one is clearer.Suggested cleanup
- .navigationBarTitleDisplayMode(.inline) .fileImporter( isPresented: $isImporting, allowedContentTypes: [.plainText, .json] @@ .background(Color.midnightBlue) .navigationTitle("Add New Wallet") .navigationBarTitleDisplayMode(.inline) .toolbarColorScheme(.dark, for: .navigationBar)As per coding guidelines, "Review SwiftUI view code for proper layout, best practices".
Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift` at line 115, Remove the duplicated navigationBarTitleDisplayMode(.inline) modifier in the SwiftUI view chain for NewWalletSelectScreen (the body of struct NewWalletSelectScreen) — leave a single .navigationBarTitleDisplayMode(.inline) and delete the extra occurrence (also remove the duplicate at the other occurrence around lines noted in the review), ensuring the view modifier chain contains only one inline title-display modifier.ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swift (1)
95-95: Remove duplicatenavigationBarTitleDisplayModemodifier.
.navigationBarTitleDisplayMode(.inline)is applied twice; keeping one improves clarity with no behavior change.Suggested cleanup
- .navigationBarTitleDisplayMode(.inline) .frame(maxHeight: .infinity) .background( Image(.newWalletPattern) @@ .background(Color.midnightBlue) .navigationTitle("Add New Wallet") .navigationBarTitleDisplayMode(.inline) .toolbarColorScheme(.dark, for: .navigationBar)As per coding guidelines, "Review SwiftUI view code for proper layout, best practices".
Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swift` at line 95, HotWalletSelectScreen contains a duplicated SwiftUI modifier navigationBarTitleDisplayMode(.inline); remove the redundant occurrence so the view only applies navigationBarTitleDisplayMode once (keep the single modifier on the HotWalletSelectScreen view body), ensuring you update the HotWalletSelectScreen view to eliminate the duplicate instance (also remove the duplicate around the block referenced in the 106-108 area) to improve clarity without changing behavior.
🤖 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/Security.kt`:
- Around line 20-28: The current catch block around createEncryptedPrefs is too
broad and should only handle security-related failures from StrongBox; replace
the catch of Exception with catching GeneralSecurityException (which covers
StrongBoxUnavailableException) in the code path that uses hasStrongBox and
createEncryptedPrefs so only crypto/hardware unavailability triggers the TEE
fallback; keep rethrowing other exceptions when useStrongBox is false and remove
any message-based detection logic so failures unrelated to StrongBox aren’t
silently masked (refer to hasStrongBox, createEncryptedPrefs,
sharedPreferences/KeychainAccessor to locate the block).
In `@android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt`:
- Around line 105-111: The constructor currently subscribes to updates
(listenForUpdates(this)) before setting WalletManager.loadState from
rustManager.initialLoadState(), allowing reconcile updates to overwrite fresher
state; fix by initializing loadState from rustManager.initialLoadState() before
calling listenForUpdates(this) (or otherwise deferring subscription until after
the when-block that maps org.bitcoinppl.cove_core.WalletLoadState to
WalletLoadState), ensuring the mapping logic that sets
WalletLoadState.LOADING/SCANNING/LOADED runs prior to any reconcile callbacks.
In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift`:
- Around line 510-512: The onAppear handler in SelectedWalletScreen.swift
currently sets shouldShowNavBar = false but does not reset the global header
flag app.isPastHeader, causing a transient toolbar mismatch; update the
.onAppear block (where shouldShowNavBar is changed) to also set app.isPastHeader
= false so the global header state is synchronized with the local navbar state
(i.e., set both shouldShowNavBar = false and app.isPastHeader = false in the
same onAppear).
In `@ios/Cove/Views/NavigationTitleView.swift`:
- Around line 61-65: The dismantleUIViewController implementation
unconditionally calls vc.clearTitle(), which can remove a title view installed
by a newer helper; change dismantleUIViewController in NavigationTitleView.swift
so it only clears the navigation title if this helper still owns it—e.g., have
NavigationTitleViewController/clearTitle check ownership by comparing the stored
titleView reference (or an ownership token/id kept on the controller) against
the navigationItem.titleView (or token) and only remove/clear when they match;
update any place that sets titleView to also set that ownership token so
dismantleUIViewController safely no-ops when another helper replaced the title.
In `@rust/src/app.rs`:
- Line 87: The call to
rustls::crypto::ring::default_provider().install_default() currently uses .ok(),
which drops errors silently; change this to call .expect("failed to install
default crypto provider") so the program panics with a clear message on
failure—update the expression where install_default() is invoked to use expect
with that message to surface configuration errors.
In `@rust/src/http_client.rs`:
- Around line 10-13: The HTTP client initializer currently panics via .expect()
and lacks timeouts; change new_client() to return Result<reqwest::Client,
Box<dyn std::error::Error>> (or an appropriate error type) instead of calling
.expect(), and configure explicit timeouts on the builder (e.g., connect_timeout
and overall timeout) before build; update all call sites that construct the
client (the constructors/factories for FeeClient and FiatClient) to propagate or
handle the Result (return Result from those constructors or handle errors) so
the code no longer panics on builder failure and will time out on
slow/unresponsive servers. Ensure you still use
tls_backend_preconfigured(tls_config) on reqwest::ClientBuilder and
map/propagate build errors rather than unwrapping.
---
Outside diff comments:
In `@rust/src/keys.rs`:
- Around line 337-344: The From conversion currently consumes `error` and then
tries to reuse it; change `match error` to `match &error` to borrow the enum and
avoid moved-value issues, adjust the arms to clone the borrowed strings for
`E::UnsupportedDescriptor(s)` and `E::UnsupportedDescriptorType(s)` (e.g.
`Self::UnsupportedDescriptor(s.clone())`), keep `E::NoOrigin => Self::NoOrigin`,
and for `E::NotMatchingPair` return a fixed message (e.g.
`Self::UnsupportedDescriptor("descriptors are not a matching external/internal
pair".to_string())`) instead of calling `to_string()` on a moved unit variant;
these changes should be applied inside the `fn from(error:
cove_bdk::descriptor_ext::Error) -> Self` match block referencing
`cove_bdk::descriptor_ext::Error` and its variants.
In `@rust/src/wallet/metadata.rs`:
- Line 51: Fix the typo in the doc comment in metadata.rs: change the comment
string "Show labels for transactions i the transaction list" to "Show labels for
transactions in the transaction list" so the documentation/readme for the
related item (the doc comment above the wallet metadata declaration) reads
correctly.
---
Nitpick comments:
In `@ios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swift`:
- Line 95: HotWalletSelectScreen contains a duplicated SwiftUI modifier
navigationBarTitleDisplayMode(.inline); remove the redundant occurrence so the
view only applies navigationBarTitleDisplayMode once (keep the single modifier
on the HotWalletSelectScreen view body), ensuring you update the
HotWalletSelectScreen view to eliminate the duplicate instance (also remove the
duplicate around the block referenced in the 106-108 area) to improve clarity
without changing behavior.
In `@ios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swift`:
- Line 115: Remove the duplicated navigationBarTitleDisplayMode(.inline)
modifier in the SwiftUI view chain for NewWalletSelectScreen (the body of struct
NewWalletSelectScreen) — leave a single .navigationBarTitleDisplayMode(.inline)
and delete the extra occurrence (also remove the duplicate at the other
occurrence around lines noted in the review), ensuring the view modifier chain
contains only one inline title-display modifier.
In `@ios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swift`:
- Around line 110-112: Remove the duplicate navigation bar title by deleting the
.navigationTitle("Recovery Words") and .navigationBarTitleDisplayMode(.inline)
modifiers in SecretWordsScreen so the prominent in-body "Recovery Words" heading
remains the primary label; you may keep .toolbarColorScheme(.dark, for:
.navigationBar) if you still want a dark nav appearance, but do not render the
same string in both the view body and the navigation bar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a53771d0-dab7-4639-8da9-2811f40a1ef0
⛔ Files ignored due to path filters (3)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**ios/CoveCore/Sources/CoveCore/generated/cove_device.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
ARCHITECTURE.mdandroid/app/build.gradle.ktsandroid/app/proguard-rules.proandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/MainActivity.ktandroid/app/src/main/java/org/bitcoinppl/cove/Security.ktandroid/app/src/main/java/org/bitcoinppl/cove/WalletManager.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove.xcodeproj/project.pbxprojios/Cove/CoveApp.swiftios/Cove/Extention/WalletColor+Ext.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletCreateScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletImportScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/HotWalletSelectScreen.swiftios/Cove/Flows/NewWalletFlow/HotWallet/VerifyWords/VerifyWordsScreen.swiftios/Cove/Flows/NewWalletFlow/NewWalletSelectScreen.swiftios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swiftios/Cove/Flows/SelectedWalletFlow/SecretWordsScreen.swiftios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swiftios/Cove/Flows/SelectedWalletFlow/WalletBalanceHeaderView.swiftios/Cove/SidebarContainer.swiftios/Cove/Views/NavigationTitleView.swiftios/Cove/WalletManager.swiftrust/.cargo/config.tomlrust/Cargo.tomlrust/crates/cove-bdk/src/coin_selection/deterministic_random_draw.rsrust/crates/cove-bdk/src/descriptor_ext.rsrust/crates/cove-bip39/src/lib.rsrust/crates/cove-types/src/transaction/sent_and_received.rsrust/crates/cove-util/src/lib.rsrust/src/app.rsrust/src/database/historical_price/record.rsrust/src/fee_client.rsrust/src/fiat/client.rsrust/src/http_client.rsrust/src/keys.rsrust/src/lib.rsrust/src/manager/wallet_manager.rsrust/src/manager/wallet_manager/actor.rsrust/src/mnemonic/number_of_bip39_words.rsrust/src/transaction/ffi.rsrust/src/wallet/metadata.rs
💤 Files with no reviewable changes (1)
- android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt
| val useStrongBox = hasStrongBox(context) | ||
| sharedPreferences = | ||
| EncryptedSharedPreferences.create( | ||
| context, | ||
| "cove_secure_storage", | ||
| masterKey, | ||
| EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, | ||
| EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM, | ||
| ) | ||
| try { | ||
| createEncryptedPrefs(context, requestStrongBox = useStrongBox) | ||
| } catch (e: Exception) { | ||
| if (!useStrongBox) throw e // not a StrongBox issue, no fallback available | ||
| Log.w("KeychainAccessor", "StrongBox-backed prefs failed, falling back to TEE", e) | ||
| createEncryptedPrefs(context, requestStrongBox = false) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For AndroidX Security Crypto MasterKey.Builder.setRequestStrongBoxBacked(true), which exception types explicitly indicate StrongBox unavailability, and what fallback handling pattern is recommended?
💡 Result:
The primary exception type that explicitly indicates StrongBox unavailability when using MasterKey.Builder.setRequestStrongBoxBacked(true) is StrongBoxUnavailableException (android.security.keystore.StrongBoxUnavailableException). This is thrown by the underlying Android Keystore system during key generation (via KeyGenerator.generateKey) when StrongBox hardware is requested but unavailable or unsupported for the key parameters. MasterKey.Builder itself does not throw this exception directly during the builder.setRequestStrongBoxBacked(true) call or build. Instead: - It checks for PackageManager.FEATURE_STRONGBOX_KEYSTORE availability before setting KeyGenParameterSpec.Builder.setIsStrongBoxBacked(true). - The exception occurs later, during MasterKey.build when MasterKeys.getOrCreate calls KeyGenerator.generateKey with the StrongBox-backed spec, if the Keystore cannot fulfill the request. Other potential exceptions during key generation include GeneralSecurityException (superclass), ProviderException, or KeyStoreException, but StrongBoxUnavailableException is the specific indicator for StrongBox unavailability. Recommended fallback handling pattern: 1. Check device support proactively: context.getPackageManager.hasSystemFeature(PackageManager.FEATURE_STRONGBOX_KEYSTORE). 2. Attempt StrongBox-backed key creation in try-catch. 3. On StrongBoxUnavailableException (or catch GeneralSecurityException and check instanceof), fallback to non-StrongBox (TEE-backed or software) by recreating the MasterKey without setRequestStrongBoxBacked(true). 4. Verify key properties post-creation using MasterKey.isStrongBoxBacked or KeyInfo.getSecurityLevel == STRONGBOX. Example pattern: boolean hasStrongBox = context.getPackageManager.hasSystemFeature(PackageManager.FEATURE_STRONGBOX_KEYSTORE); MasterKey masterKey; try { MasterKey.Builder builder = new MasterKey.Builder(context).setRequestStrongBoxBacked(hasStrongBox); masterKey = builder.build; } catch (StrongBoxUnavailableException | GeneralSecurityException e) { // Fallback to TEE/software masterKey = new MasterKey.Builder(context).build; // defaults to non-StrongBox } if (masterKey.isStrongBoxBacked) { // Use StrongBox key } else { // Handle TEE fallback, log, or degrade gracefully } Note: androidx.security:crypto:MasterKey is deprecated; migrate to direct javax.crypto.KeyGenerator + AndroidKeyStore for better control and non-deprecated StrongBox handling.
Citations:
- 1: https://android.googlesource.com/platform/frameworks/support/+/f2e05c341382db64d127118a13451dcaa554b702/security/crypto/src/main/java/androidx/security/crypto/MasterKey.java
- 2: https://android.googlesource.com/platform/frameworks/base/+/master/keystore/java/android/security/keystore/StrongBoxUnavailableException.java
- 3: https://developer.android.com/reference/android/security/keystore/StrongBoxUnavailableException
- 4: https://developer.android.com/privacy-and-security/keystore
- 5: https://android.googlesource.com/platform/frameworks/support/+/3fe2f1a8cea643d63e74868772f512373973744c/security/security-crypto/src/main/java/androidx/security/crypto/MasterKey.java
- 6: https://stackoverflow.com/questions/60071961/setisstrongboxbacked-for-doesnt-throw-strongboxunavailableexception-as-expect
- 7: https://stackoverflow.com/questions/58502299/how-to-check-for-strongbox-keymaster-hardware-availability-before-key-generation
- 8: https://medium.com/@dfs.techblog/safeguarding-cryptographic-keys-implementing-tee-and-strongbox-in-android-applications-7894c800e43e
- 9: https://comviva.com/blog/safeguarding-cryptographic-keys-implementing-tee-and-strongbox-in-android-applications
🏁 Script executed:
fd -t f "Security.kt" android/Repository: bitcoinppl/cove
Length of output: 116
🏁 Script executed:
cat -n android/app/src/main/java/org/bitcoinppl/cove/Security.ktRepository: bitcoinppl/cove
Length of output: 2981
🏁 Script executed:
rg -t kotlin "StrongBoxUnavailableException|ProviderException" android/Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
rg -t kotlin "android.security.keystore|StrongBoxUnavailableException" --type kotlin android/Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
python3 << 'EOF'
# Check the Android Keystore exception hierarchy
# ProviderException is a general cryptography exception
# StrongBoxUnavailableException is more specific
print("Android Exception Hierarchy for StrongBox:")
print("- StrongBoxUnavailableException (extends GeneralSecurityException)")
print(" └─ Thrown during MasterKey.build() -> KeyGenerator.generateKey()")
print()
print("- GeneralSecurityException (broader)")
print(" ├─ StrongBoxUnavailableException")
print(" ├─ KeyStoreException")
print(" └─ Others...")
print()
print("- ProviderException")
print(" └─ A generic exception from security providers (may or may not be related to StrongBox)")
print()
print("Recommended catch order:")
print("1. Specific: StrongBoxUnavailableException (if available in androidx.security)")
print("2. Broader: GeneralSecurityException (covers StrongBox and other key generation errors)")
print("3. NOT recommended: Catch Exception or rely on message string matching")
EOFRepository: bitcoinppl/cove
Length of output: 721
Catch the correct exception for StrongBox unavailability, not ProviderException
Line 24 catches Exception, allowing any failure—not just StrongBox-specific ones—to trigger fallback, potentially masking security initialization errors. However, the proposed fix is incorrect: catch GeneralSecurityException instead of ProviderException and do not rely on message string matching. Per Android security guidelines, StrongBoxUnavailableException (which extends GeneralSecurityException) is thrown when StrongBox hardware is unavailable during key generation. Catching the broader GeneralSecurityException is the recommended pattern—message-based detection is unreliable and fragile.
Correct fix
sharedPreferences =
try {
createEncryptedPrefs(context, requestStrongBox = useStrongBox)
- } catch (e: Exception) {
- if (!useStrongBox) throw e // not a StrongBox issue, no fallback available
- Log.w("KeychainAccessor", "StrongBox-backed prefs failed, falling back to TEE", e)
- createEncryptedPrefs(context, requestStrongBox = false)
+ } catch (e: java.security.GeneralSecurityException) {
+ if (!useStrongBox) throw e // not a StrongBox issue, no fallback available
+ Log.w("KeychainAccessor", "StrongBox-backed prefs failed, falling back to TEE", e)
+ createEncryptedPrefs(context, requestStrongBox = false)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/org/bitcoinppl/cove/Security.kt` around lines 20 -
28, The current catch block around createEncryptedPrefs is too broad and should
only handle security-related failures from StrongBox; replace the catch of
Exception with catching GeneralSecurityException (which covers
StrongBoxUnavailableException) in the code path that uses hasStrongBox and
createEncryptedPrefs so only crypto/hardware unavailability triggers the TEE
fallback; keep rethrowing other exceptions when useStrongBox is false and remove
any message-based detection logic so failures unrelated to StrongBox aren’t
silently masked (refer to hasStrongBox, createEncryptedPrefs,
sharedPreferences/KeychainAccessor to locate the block).
|
|
||
| // set initial load state from Rust cached data | ||
| loadState = when (val rustLoadState = rustManager.initialLoadState()) { | ||
| is org.bitcoinppl.cove_core.WalletLoadState.Loading -> WalletLoadState.LOADING | ||
| is org.bitcoinppl.cove_core.WalletLoadState.Scanning -> WalletLoadState.SCANNING(rustLoadState.v1) | ||
| is org.bitcoinppl.cove_core.WalletLoadState.Loaded -> WalletLoadState.LOADED(rustLoadState.v1) | ||
| } |
There was a problem hiding this comment.
Initialize loadState before subscribing to updates.
At Line 105, listenForUpdates(this) can start delivering reconcile messages before the constructor sets loadState, so the assignment at Line 107 can overwrite fresher state.
🔧 Proposed fix
- rustManager.listenForUpdates(this)
-
// set initial load state from Rust cached data
loadState = when (val rustLoadState = rustManager.initialLoadState()) {
is org.bitcoinppl.cove_core.WalletLoadState.Loading -> WalletLoadState.LOADING
is org.bitcoinppl.cove_core.WalletLoadState.Scanning -> WalletLoadState.SCANNING(rustLoadState.v1)
is org.bitcoinppl.cove_core.WalletLoadState.Loaded -> WalletLoadState.LOADED(rustLoadState.v1)
}
+
+ rustManager.listenForUpdates(this)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/org/bitcoinppl/cove/WalletManager.kt` around lines
105 - 111, The constructor currently subscribes to updates
(listenForUpdates(this)) before setting WalletManager.loadState from
rustManager.initialLoadState(), allowing reconcile updates to overwrite fresher
state; fix by initializing loadState from rustManager.initialLoadState() before
calling listenForUpdates(this) (or otherwise deferring subscription until after
the when-block that maps org.bitcoinppl.cove_core.WalletLoadState to
WalletLoadState), ensuring the mapping logic that sets
WalletLoadState.LOADING/SCANNING/LOADED runs prior to any reconcile callbacks.
| .onAppear { | ||
| shouldShowNavBar = false | ||
| } |
There was a problem hiding this comment.
Reset global header state when resetting local navbar state.
Line 510 resets shouldShowNavBar, but app.isPastHeader is not reset here. That can cause a temporary toolbar-style mismatch until geometry updates run.
Suggested fix
.onAppear {
shouldShowNavBar = false
+ app.isPastHeader = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .onAppear { | |
| shouldShowNavBar = false | |
| } | |
| .onAppear { | |
| shouldShowNavBar = false | |
| app.isPastHeader = false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/Flows/SelectedWalletFlow/SelectedWalletScreen.swift` around lines
510 - 512, The onAppear handler in SelectedWalletScreen.swift currently sets
shouldShowNavBar = false but does not reset the global header flag
app.isPastHeader, causing a transient toolbar mismatch; update the .onAppear
block (where shouldShowNavBar is changed) to also set app.isPastHeader = false
so the global header state is synchronized with the local navbar state (i.e.,
set both shouldShowNavBar = false and app.isPastHeader = false in the same
onAppear).
| static func dismantleUIViewController( | ||
| _ vc: NavigationTitleViewController<TitleContent>, coordinator _: Coordinator | ||
| ) { | ||
| vc.clearTitle() | ||
| } |
There was a problem hiding this comment.
Only clear the navigation title if this helper still owns it.
Line 108 clears titleView unconditionally. If SwiftUI replaces this helper, an older instance can dismantle after a newer one has already attached its own title view and blank the navbar.
Suggested fix
func clearTitle() {
- targetNavigationItem?.titleView = nil
+ if let hostingView = hostingController?.view,
+ targetNavigationItem?.titleView === hostingView
+ {
+ targetNavigationItem?.titleView = nil
+ }
targetNavigationItem = nil
}Also applies to: 107-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Cove/Views/NavigationTitleView.swift` around lines 61 - 65, The
dismantleUIViewController implementation unconditionally calls vc.clearTitle(),
which can remove a title view installed by a newer helper; change
dismantleUIViewController in NavigationTitleView.swift so it only clears the
navigation title if this helper still owns it—e.g., have
NavigationTitleViewController/clearTitle check ownership by comparing the stored
titleView reference (or an ownership token/id kept on the controller) against
the navigationItem.titleView (or token) and only remove/clear when they match;
update any place that sets titleView to also set that ownership token so
dismantleUIViewController safely no-ops when another helper replaced the title.
| set_env(); | ||
|
|
||
| // one time init | ||
| rustls::crypto::ring::default_provider().install_default().ok(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rustls 0.23 install_default crypto provider error handling
💡 Result:
In rustls 0.23, the default crypto provider is aws-lc-rs (enabled by the default aws_lc_rs feature). There is no automatic process-wide default CryptoProvider installed. Functions like ClientConfig::builder and ServerConfig::builder require a process-level default to be set, or they panic with "no process-level CryptoProvider available -- call CryptoProvider::install_default before this point". To fix this, call CryptoProvider::install_default early in main, before any TLS usage. This can only be called once per process. For the default aws-lc-rs provider: use rustls::crypto::aws_lc_rs; fn main { aws_lc_rs::default_provider.install_default.expect("should not fail"); // ... rest of app } For ring (enable the ring feature, avoid enabling both aws_lc_rs and ring to prevent ambiguity): use rustls::crypto::ring; fn main { ring::default_provider.install_default.expect("should not fail"); // ... rest of app } The install_default method returns Result<, Arc<dyn std::error::Error + Send + Sync>>. It fails if already called (second call returns the previously installed provider as error), or if the provider is invalid (e.g., mismatched cipher suites and KX groups). If using default-features = false, or custom-provider feature, or ambiguous features (both aws_lc_rs and ring), you must explicitly install a provider. Alternatively, use builder_with_provider(provider) to avoid process-wide default. Cargo.toml examples: - Default (aws-lc-rs): rustls = "0.23" - Ring: rustls = { version = "0.23", features = ["ring"] } - No default deps: rustls = { version = "0.23", default-features = false, features = ["aws_lc_rs", "std"] } then install explicitly. aws-lc-rs requires cmake and nasm (Windows) at build time.
Citations:
- 1: How I use CryptoProvider::install_default() ? rustls/rustls#1938
- 2: 0.23.13 seems to break semantic versions with the lack of default CryptoProvider rustls/rustls#2148
- 3: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html
- 4: Rustls: no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point snapview/tokio-tungstenite#339
- 5: https://docs.rs/rustls/^0.23.18
- 6: Prepare 0.23.0 rustls/rustls#1817
- 7: https://docs.cloud.google.com/rust/switch-rustls-crypto-provider
- 8: https://docs.rs/rustls/latest/rustls/
🏁 Script executed:
# Check Cargo.toml for rustls configuration
cat -n Cargo.toml | grep -A 5 -B 5 rustls
# Check the actual code context in app.rs
fd app.rs --path rust/ --exec cat -n {} \; | head -100Repository: bitcoinppl/cove
Length of output: 317
🏁 Script executed:
# Find Cargo.toml
find . -name "Cargo.toml" -type f
# Find app.rs in rust directory
find . -path "*/rust/src/app.rs" -type fRepository: bitcoinppl/cove
Length of output: 548
🏁 Script executed:
# Check rustls configuration in main Cargo.toml
cat ./rust/Cargo.toml | grep -A 10 -B 2 rustls
# Check app.rs around line 87
sed -n '70,100p' ./rust/src/app.rsRepository: bitcoinppl/cove
Length of output: 2544
🏁 Script executed:
# Verify the exact error type from rustls documentation by checking if there's any documentation in the crate
rg "install_default" rust/Cargo.toml rust/src/app.rs -B 2 -A 2
# Also check if there are other usages of install_default in the codebase
rg "install_default" --type rustRepository: bitcoinppl/cove
Length of output: 359
🏁 Script executed:
# Get full context around both occurrences - read the entire section with line numbers
sed -n '75,105p' ./rust/src/app.rsRepository: bitcoinppl/cove
Length of output: 1032
Change .ok() to .expect() for proper error handling.
Using .ok() silently discards errors from install_default(). According to rustls 0.23 best practices, this should use .expect() instead. While the benign case (provider already installed) won't occur in single-threaded initialization, using .ok() also masks real configuration errors (e.g., invalid crypto provider setup). Change to .expect("failed to install default crypto provider") to fail loudly on actual issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/app.rs` at line 87, The call to
rustls::crypto::ring::default_provider().install_default() currently uses .ok(),
which drops errors silently; change this to call .expect("failed to install
default crypto provider") so the program panics with a clear message on
failure—update the expression where install_default() is invoked to use expect
with that message to surface configuration errors.
| reqwest::ClientBuilder::new() | ||
| .tls_backend_preconfigured(tls_config) | ||
| .build() | ||
| .expect("failed to build http client") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all call sites that consume crate::http_client::new_client()
# Expected: enumerate constructors that need to handle a fallible return.
rg -nP --type=rust -C2 '\bcrate::http_client::new_client\s*\('Repository: bitcoinppl/cove
Length of output: 853
🏁 Script executed:
# Read the http_client.rs file to verify the current implementation
head -40 rust/src/http_client.rsRepository: bitcoinppl/cove
Length of output: 628
🏁 Script executed:
# Search for all calls to new_client() including potential uses with :: prefix or different patterns
rg -n "new_client\s*\(" rust/ --type=rustRepository: bitcoinppl/cove
Length of output: 420
🏁 Script executed:
# Check if eyre is already a dependency in the project
grep -r "eyre" rust/Cargo.tomlRepository: bitcoinppl/cove
Length of output: 99
🏁 Script executed:
# Examine the call sites to understand how they use the client
cat -n rust/src/fee_client.rs | sed -n '40,60p'Repository: bitcoinppl/cove
Length of output: 902
🏁 Script executed:
# Check fiat client usage
cat -n rust/src/fiat/client.rs | sed -n '75,95p'Repository: bitcoinppl/cove
Length of output: 794
🏁 Script executed:
# Search for tests that might use new_client() or instantiate the clients
rg -n "new_client\|FeeClient\|FiatClient" rust/src --type=rust | grep -i testRepository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check for any test files in the rust directory
find rust -name "*test*" -o -name "*tests*" | grep -E "\.(rs|toml)$"Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Examine the struct definitions to see client field types
rg -A5 "pub struct FeeClient" rust/src --type=rustRepository: bitcoinppl/cove
Length of output: 285
🏁 Script executed:
# Check FiatClient struct
rg -A5 "pub struct FiatClient" rust/src --type=rustRepository: bitcoinppl/cove
Length of output: 285
Avoid panic in HTTP client initialization and add explicit timeouts to prevent hangs.
The code panics via .expect() on line 13 if the client builder fails, and the client lacks explicit timeout configuration. While the panic occurs at initialization rather than during requests, panicking on a fallible operation is poor error handling. Missing timeouts can cause the application to hang indefinitely on slow or unresponsive servers.
Change new_client() to return Result<reqwest::Client> instead of panicking, and add explicit connection and request timeouts. This requires updating three call sites in FeeClient and FiatClient constructors, but the scope is localized and manageable.
Proposed refactor
+use eyre::{Context as _, Result};
+use std::time::Duration;
+
/// Build a reqwest Client that uses webpki-roots for TLS cert verification,
/// bypassing rustls-platform-verifier (which requires Android JNI init)
-pub fn new_client() -> reqwest::Client {
+pub fn new_client() -> Result<reqwest::Client> {
let root_store =
rustls::RootCertStore::from_iter(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());
let tls_config =
rustls::ClientConfig::builder().with_root_certificates(root_store).with_no_client_auth();
reqwest::ClientBuilder::new()
+ .connect_timeout(Duration::from_secs(10))
+ .timeout(Duration::from_secs(20))
.tls_backend_preconfigured(tls_config)
.build()
- .expect("failed to build http client")
+ .context("failed to build http client")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/http_client.rs` around lines 10 - 13, The HTTP client initializer
currently panics via .expect() and lacks timeouts; change new_client() to return
Result<reqwest::Client, Box<dyn std::error::Error>> (or an appropriate error
type) instead of calling .expect(), and configure explicit timeouts on the
builder (e.g., connect_timeout and overall timeout) before build; update all
call sites that construct the client (the constructors/factories for FeeClient
and FiatClient) to propagate or handle the Result (return Result from those
constructors or handle errors) so the code no longer panics on builder failure
and will time out on slow/unresponsive servers. Ensure you still use
tls_backend_preconfigured(tls_config) on reqwest::ClientBuilder and
map/propagate build errors rather than unwrapping.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores