M3: Multi-Wallet, Background Sync, In-App Update#80
Merged
Conversation
Room validates that @ColumnInfo(defaultValue) and @Index annotations on entities exactly match the actual DB schema after migration. MIGRATION_2_3 added DEFAULT clauses and indices that the entities were missing, causing IllegalStateException on app startup. - TransactionEntity: add @ColumnInfo(defaultValue="") on walletId, @Index with DESC order on timestamp matching migration - DaoCellEntity: add @ColumnInfo(defaultValue="") on walletId, @Index for idx_dao_wallet_network - BalanceCacheEntity: add @ColumnInfo(defaultValue="") on walletId - HeaderCacheEntity: add @Index for idx_header_network_number - WalletEntity: add @ColumnInfo defaults on accountIndex, addresses, isActive, createdAt matching CREATE TABLE defaults
- Add Wallet icon next to active wallet name in HomeScreen top bar - Replace crypto jargon: "HD" → "Seed Phrase", "Key" → "Imported" - WalletDetailScreen: "HD Wallet (Mnemonic)" → "Seed Phrase Wallet", "HD Sub-Account" → "Sub-Account", "Raw Key" → "Imported Key"
Resolve conflicts keeping M3's multi-wallet additions (walletId columns in Room entities, wallet switcher in HomeScreen top bar) while picking up main's bugfixes (settings icon removal, dark mode, StrongBox fallback, atomic writes).
- Change savePrivateKey() and setMnemonicBackedUp() from apply() to commit() for synchronous, atomic writes that survive process death - Add wallet-scoped key storage: storeKeysForWallet(), getWalletPrefs(), getMnemonicForWallet(), getPrivateKeyForWallet(), deleteWalletKeys() - Add wallet-scoped backup methods: setMnemonicBackedUpForWallet(), hasMnemonicBackupForWallet() - Add multi-wallet scaffolding: WalletDao, WalletRepository, WalletMigrationHelper, WalletManagerViewModel, WalletDetailViewModel - Add wallets table (Room v2->v3 migration), wire WalletDao in AppModule - Fix HomeScreen/HomeViewModel for wallet list support (WalletSwitcherDropdown) - Fix BalanceCacheEntity missing @PrimaryKey (merge artifact) - Fix BalanceCacheDaoTest to use named parameters after entity change
Replace plain SharedPreferences in getWalletPrefs() with EncryptedSharedPreferences using the same 3-tier StrongBox fallback pattern as the global encryptedPrefs: try StrongBox -> delete + retry without StrongBox -> final StrongBox retry. Add createEncryptedPrefsForWallet() helper and TAG constant.
… items 3,6-9) Bump versionCode to 6 and versionName to 1.5.0 for the M3 multi-wallet release. Verified: - MainActivity start destination correctly uses repository.hasWallet() (delegates to KeyManager global prefs, still source of truth) - WalletPreferences has getThemeMode()/setThemeMode() after merge - GatewayRepository.switchNetwork() uses killProcess() + intent relaunch - HomeScreen TopAppBar has no settings icon (just sync chip)
MIGRATION_2_3 only created the wallets table but did not add the walletId column to transactions, balance_cache, and dao_cells tables. This caused a crash on upgrade from v1.4.1 (DB v2) because Room expected the column and indexes but they were missing.
The idx_header_network_number index was added to HeaderCacheEntity in M3 but never created in any migration SQL. Users upgrading from v1.4.1 (DB v2) would crash on Room schema validation.
WalletMigrationHelper existed but was never injected or called. Users upgrading from v1.4.1 would have their mnemonic and keys stuck in global EncryptedSharedPreferences with no migration to wallet-scoped storage, causing empty backup screen. Now injected into GatewayRepository and called before node init.
The StrongBox fallback was deleting the entire ckb_wallet_keys file when EncryptedSharedPreferences failed to open. This permanently destroys the user's private key and mnemonic — unrecoverable. Changed to: try without StrongBox, then try StrongBox again. Never delete the file. Set walletResetDueToCorruption flag only when both attempts fail so the UI can warn the user.
…liance F-Droid scanner flags Firebase as a proprietary dependency. Firebase App Distribution was only used for tester APK delivery, not a runtime feature. Also add scandelete for binary test file in external CKB light client.
…ackup Defines the defense-in-depth architecture for key management: Phase 1 adds PIN-encrypted backup file alongside ESP, two-tier recovery flow, and soft-gate warning system. Phase 2 migrates primary store from ESP to Keystore+Room. Phase 3 removes ESP.
- C1: PIN recovery validates via backup decryption, not PinManager.verifyPin() - C2: Session PIN cached as CharArray (zeroed on clear), not String - C3: reEncryptAll uses .tmp-then-rename for crash safety - I1: 5-byte magic header (PNBK + version) for format detection - I2: Explicit no-PIN gap documented with mitigation via soft-gate - I3: PIN removal blocked/warned if backup files exist - I4: ESP-first write ordering specified - I5: Raw-key wallets handled in Tier 2 recovery - I6: PIN creation hook coordinated at ViewModel level - PinManager StrongBox fallback bug documented as prerequisite
Covers: KeyBackupManager, dual-write, PinManager fixes, recovery flow, soft-gate warnings (banner, receive modal, post-deposit reminder), mnemonic verification quiz, navigation, and DI wiring.
All Room-backed key reads/writes are now suspend functions. Call sites updated across 11 files: - ViewModels: wrap in viewModelScope.launch - Repository methods: marked suspend - MainActivity: uses lifecycleScope.launch for startup - Tests: wrapped with runTest
…licate imports, parent-with-subs delete Defense-in-depth + UX guards discovered during M3 pre-release audit: - WalletRepository.deleteWallet throws IllegalStateException if walletId is active (UI already guarded, repository layer was not). Prevents zombie state where activeWalletId points at a deleted row. - New validateUniqueAddress wired into importWallet and importRawKey so the same mnemonic or raw key cannot be imported twice under different names. Avoids splitting tx history, balance cache and DAO cells across duplicate WalletEntity rows pointing at the same keypair. - WalletSettingsViewModel.requestDelete refuses parent wallet deletion while sub-accounts exist. Parent holds the seed needed to re-derive subs; user must delete subs first. - Added three unit tests covering the new guards.
Before: PIN was optional. A user could create or import a wallet and land on
the main screen with no PIN, leaving the PIN-encrypted KeyBackupManager
recovery path unpopulated.
After: every code path that leads to Main routes through PIN setup first if
no PIN is configured.
- MainActivity startup gate: !pinManager.hasPin() -> PinEntry("setup") so
existing v1.5.0 upgraders without a PIN are prompted once.
- NavGraph introduces destinationAfterWalletReady() used by Onboarding,
MnemonicBackup, MnemonicImport and Recovery callbacks. Picks Main if PIN
exists, else PinEntry("setup").
- PinEntry CONFIRM handler navigates to Main when setup was the start
destination (no origin to pop back to). Existing SecuritySettings and
SecurityChecklist flows still pop back normally.
- SecuritySettings "Remove PIN": blocked at the ViewModel layer while any
wallet exists, and the dialog now surfaces the reason up front instead of
sending the user through PIN verify only to fail with a snackbar.
M3 release: multi-wallet, HD sub-accounts, Room-backed key storage, mandatory PIN. See fastlane changelog 7.txt for user-facing summary.
… syncing history Two follow-ups to the mandatory PIN change in 171a888: 1. After CONFIRM in the forced-setup flow, the previous code called navigate(Main) with popUpTo(0) which does not match any destination in Nav Compose — so the popUpTo silently no-ops and the NavHost ends up in an inconsistent state, rendering a blank screen. Replaced with popBackStack() on setup: if it returns false (setup is the startDestination), navigate to Main with popUpTo(graph.id) inclusive. Leaves the SecuritySettings / SecurityChecklist flow unchanged because popBackStack() succeeds there. 2. A newly-generated wallet has no history, so defaulting its sync mode to RECENT forced the light client to walk back ~200k blocks for nothing. WalletRepository.createWallet and createSubAccount now mark the new walletId with SyncMode.NEW_WALLET (start from tip) for both networks. Imports still use whatever sync mode the import UI selected.
…xplanation The previous attempt reused PinEntryScreen (SETUP -> CONFIRM) as the startDestination when no PIN existed. That fought the NavHost: both popUpTo(0) and popUpTo(graph.id) paths still resulted in a blank screen after CONFIRM because the graph's startDestination route was itself what we were trying to clear, leaving the NavHost with no active composable. New approach: - InitialPinSetupScreen is a standalone composable with three internal phases (INTRO -> SETUP -> CONFIRM) managed by rememberSaveable state. No navigation between phases, so the back stack has exactly one entry for the whole setup flow. - INTRO explains, in plain language, why a PIN is required: it locks the wallet, unlocks the on-device encrypted recovery-phrase backup, and is not a replacement for the recovery phrase itself. - SETUP and CONFIRM phases reuse PinEntryScreen, sharing the same PinViewModel instance scoped to the InitialPinSetup NavBackStackEntry. CONFIRM still saves the PIN via pinManager.setPin when it matches. - On success, the composable navigates to Main and clears the graph (popUpTo(graph.id) inclusive) — now safe because the start destination being cleared is InitialPinSetup, not the destination we're adding. MainActivity startup gate and NavGraph.destinationAfterWalletReady() now return Screen.InitialPinSetup.route. The PinEntry CONFIRM handler reverts to its original "pop confirm + pop setup" since forced PIN setup no longer goes through PinEntry directly — only SecuritySettings / SecurityChecklist do, and both have a real origin behind setup.
No behavior change — the shared PinEntry flow used by SecuritySettings and SecurityChecklist is byte-identical to pre-session state.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
android/app/src/main/java/com/rjnr/pocketnode/data/gateway/DaoSyncManager.kt (1)
43-56:⚠️ Potential issue | 🟠 MajorRemove empty string defaults and require non-blank wallet IDs to prevent silent fallback to unscoped rows.
The methods
getActiveDeposits,getCompletedDeposits, andinsertPendingDepositare wallet-scoped but defaultwalletIdto an empty string. This enables callers to silently use an unintended scope, potentially reading/writing rows that belong to no wallet. The production call site inGatewayRepository.kt:1529correctly passeswalletId = activeWalletId, but the default creates risk for future call sites.Remove the defaults and add validation:
- Remove
walletId: String = ""from all three method signatures- Add
require(walletId.isNotBlank()) { "walletId is required" }at method entry- Update test calls to explicitly pass a wallet ID (e.g., an empty test wallet constant if empty scope is still needed)
Suggested diff
- suspend fun getActiveDeposits(network: String, walletId: String = ""): List<DaoCellEntity> { + suspend fun getActiveDeposits(network: String, walletId: String): List<DaoCellEntity> { + require(walletId.isNotBlank()) { "walletId is required" } return try { daoCellDao.getActiveByWalletAndNetwork(walletId, network) - suspend fun getCompletedDeposits(network: String, walletId: String = ""): List<DaoCellEntity> { + suspend fun getCompletedDeposits(network: String, walletId: String): List<DaoCellEntity> { + require(walletId.isNotBlank()) { "walletId is required" } return try { daoCellDao.getCompletedByWalletAndNetwork(walletId, network) - suspend fun insertPendingDeposit(txHash: String, capacity: Long, network: String, index: String = "0x0", walletId: String = "") { + suspend fun insertPendingDeposit(txHash: String, capacity: Long, network: String, index: String = "0x0", walletId: String) { + require(walletId.isNotBlank()) { "walletId is required" } try { daoCellDao.upsert(Also applies to: 106-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/gateway/DaoSyncManager.kt` around lines 43 - 56, The wallet-scoped methods getActiveDeposits, getCompletedDeposits, and insertPendingDeposit currently default walletId to "", allowing accidental unscoped access; remove the default from each signature (no "= \"\""), add a guard at the start of each method: require(walletId.isNotBlank()) { "walletId is required" }, and update any tests or callers to pass an explicit walletId (use a test constant for an empty-scope case if needed) so no call silently falls back to an empty walletId.android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt (2)
880-901:⚠️ Potential issue | 🟠 MajorCapture the sender wallet before the delayed rescan.
The delayed coroutine reads
_walletInfo.valuefive seconds later. If the user switches wallets in that window, the partial re-register runs for the new wallet instead of the wallet that sent the transaction.🐛 Proposed fix
// After sending, nudge the light client to rescan from a few blocks back // so it picks up the new change output when the tx confirms. + val senderInfo = _walletInfo.value scope.launch { try { delay(5000) // Wait a bit for tx to propagate @@ - val info = _walletInfo.value - if (info != null) { + if (senderInfo != null) { val blockNumberHex = "0x${rescanFrom.toString(16)}" // Only register lock script (not DAO type) with PARTIAL mode val lockStatus = JniScriptStatus( - script = info.script, + script = senderInfo.script, scriptType = "lock", blockNumber = blockNumberHex )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt` around lines 880 - 901, Capture the current sender wallet details before the delayed coroutine so the rescan uses the wallet that initiated the tx rather than whatever _walletInfo.value becomes after 5s: read and store _walletInfo.value (e.g., into a local val like senderInfo) before calling delay(5000) inside the scope.launch block, then use that captured senderInfo when building JniScriptStatus and calling LightClientNative.nativeSetScripts; ensure you still null-check the captured value and avoid reading _walletInfo.value after the delay.
374-427:⚠️ Potential issue | 🟠 MajorRefresh active wallet context after create/import.
These paths create or activate a wallet through
KeyManager, butGatewayRepository.activeWalletIdandactiveWalletTyperemain stale. Subsequent sync prefs, cache writes, backup gating, and key reads can be scoped to""or the previous wallet.🐛 Proposed fix
Log.d(TAG, "🆕 Creating brand new wallet...") val info = keyManager.generateWallet() + activeWalletId = walletPreferences.getActiveWalletId() ?: "" + activeWalletType = KeyManager.WALLET_TYPE_RAW_KEY _walletInfo.value = info @@ Log.d(TAG, "📥 Importing existing wallet...") val info = keyManager.importWallet(privateKeyHex) + activeWalletId = walletPreferences.getActiveWalletId() ?: "" + activeWalletType = KeyManager.WALLET_TYPE_RAW_KEY _walletInfo.value = info @@ Log.d(TAG, "Creating mnemonic wallet...") val (info, words) = keyManager.generateWalletWithMnemonic() + activeWalletId = walletPreferences.getActiveWalletId() ?: "" + activeWalletType = KeyManager.WALLET_TYPE_MNEMONIC _walletInfo.value = info @@ Log.d(TAG, "Importing wallet from mnemonic...") val info = keyManager.importWalletFromMnemonic(words, passphrase) + activeWalletId = walletPreferences.getActiveWalletId() ?: "" + activeWalletType = KeyManager.WALLET_TYPE_MNEMONIC _walletInfo.value = info🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt` around lines 374 - 427, The create/import flows (createNewWallet, importExistingWallet, importWallet, createWalletWithMnemonic, importFromMnemonic) update _walletInfo but do not refresh GatewayRepository.activeWalletId and activeWalletType, leaving wallet-scoped prefs/caches referencing the old id/type; after each successful wallet creation/import (after _walletInfo.value = info and before calling registerAccount), assign the repository's activeWalletId and activeWalletType from the returned WalletInfo (e.g., info.id and info.type or the actual identifier fields on WalletInfo) so subsequent sync prefs, cache writes, backup gating, and key reads use the newly active wallet context.
♻️ Duplicate comments (3)
android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt (1)
290-291:⚠️ Potential issue | 🔴 CriticalDelete the database row before destroying wallet keys.
deleteWalletKeys(walletId)is irreversible. If the DAO delete or later cleanup fails, Room can still contain a wallet whose key material has already been destroyed.Suggested direction
- keyManager.deleteWalletKeys(walletId) walletDao.delete(walletId) + keyManager.deleteWalletKeys(walletId)Prefer wrapping the row/cache cleanup in a transaction, then deleting keys after the transaction succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt` around lines 290 - 291, The deletion order reverses risk: don't call keyManager.deleteWalletKeys(walletId) before removing the DB row; instead perform the database removal inside a transaction (use walletDao.transaction or RoomDatabase.runInTransaction) to delete the wallet row and any related caches first (the code that calls walletDao.delete(walletId)), check the transaction succeeded/committed, and only after that call keyManager.deleteWalletKeys(walletId); update the function in WalletRepository that currently calls keyManager.deleteWalletKeys then walletDao.delete to follow this transactional-then-key-delete flow.android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt (2)
36-56:⚠️ Potential issue | 🔴 CriticalVerify that
KeyManager.setSessionPin()is wired to the PIN source; otherwise backups are silently disabled.
KeyManagerkeeps its ownsessionPinseparate fromAuthManager's cached PIN. Unless something explicitly callskeyManager.setSessionPin(...)whenever the user enters/sets a PIN (e.g., right afterauthManager.setSessionPin(...)inSecuritySettingsViewModel, as well as on PIN unlock / initial setup / wallet import flows), everywriteBackupIfPinAvailable(...)call returns early at line 597 and no encrypted backup is ever written — which defeats the wholeKeyBackupManagerintegration.Either inject
AuthManagerintoKeyManagerand read the session PIN from there, or ensure all PIN entry sites also propagate tokeyManager.setSessionPin(...)(andclearSessionPin()on logout/timeout).#!/bin/bash # Confirm whether KeyManager.setSessionPin is actually called from production code # (not just tests) anywhere other than KeyManager itself. rg -nP -C3 '\bkeyManager\s*\.\s*setSessionPin\s*\(' android/app/src/main/java android/app/src/main/kotlin 2>/dev/null rg -nP -C3 '\bkeyManager\s*\.\s*clearSessionPin\s*\(' android/app/src/main/java android/app/src/main/kotlin 2>/dev/null # Any references at all (incl. tests) — for scope context rg -nP -C2 '\b(setSessionPin|clearSessionPin)\b' android/app 2>/dev/null # Check if AuthManager is injected into KeyManager somewhere rg -nP -C3 'class\s+KeyManager\b|KeyManager\s*\(' android/app/src/main/java 2>/dev/null | head -60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt` around lines 36 - 56, KeyManager currently stores a separate sessionPin and writeBackupIfPinAvailable returns early if it's null; ensure the PIN is actually propagated so backups aren't silently disabled: either inject AuthManager into KeyManager and have writeBackupIfPinAvailable/readPin call AuthManager.getSessionPin() (or a provided accessor) instead of relying on KeyManager.sessionPin, or make all PIN entry/clear sites call KeyManager.setSessionPin(...) and KeyManager.clearSessionPin() (e.g., immediately after AuthManager.setSessionPin(...) in SecuritySettingsViewModel and in PIN unlock / wallet import / logout flows). Update references to KeyManager.setSessionPin, KeyManager.clearSessionPin, writeBackupIfPinAvailable, AuthManager, and SecuritySettingsViewModel accordingly so the backup path always sees the active PIN.
381-395:⚠️ Potential issue | 🟡 Minor
getWalletPrefs"last-resort" retry is a no-op that will just re-throw.Line 384 already tried
createEncryptedPrefsForWallet(fileName, useStrongBox = true)and failed; retrying the same call at line 392 after a second failure is not meaningfully different (StrongBox state doesn't change between a few microseconds) and will propagate the exception to the caller, crashing whatever forced lazy init (see previous review comment on initialization order — same problem applies here, and there's nowalletResetDueToCorruptionequivalent for per-wallet prefs so there is no recovery path).Consider either (a) returning the non-StrongBox prefs result or a
Result<SharedPreferences>/nullable fromgetWalletPrefsand letting callers surface the recovery UI, or (b) dropping the redundant retry and at minimum setting a per-wallet corruption flag so the Recovery screen can handle it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt` around lines 381 - 395, getWalletPrefs currently retries a StrongBox creation a second time (createEncryptedPrefsForWallet(fileName, useStrongBox = true)) which is a no-op and will just rethrow; replace that final retry with a real recovery path: after the non-StrongBox attempt in getWalletPrefs's inner catch, do not call createEncryptedPrefsForWallet(..., useStrongBox = true) again — instead set a per-wallet corruption flag (e.g. walletResetDueToCorruption[walletId] = true or similar) and return a nullable/Result from getWalletPrefs (or return the non-StrongBox SharedPreferences if it succeeded) so callers can surface recovery UI; update callers of getWalletPrefs to handle the nullable/Result accordingly and remove the redundant StrongBox retry.
🟡 Minor comments (18)
website/claim.html-433-433 (1)
433-433:⚠️ Potential issue | 🟡 MinorAnnounce validation and server errors to assistive tech.
#formErroris visually updated, but it has no live region/alert semantics, so screen-reader users may miss validation and claim failures.Proposed fix
- <div class="form-error" id="formError"></div> + <div class="form-error" id="formError" role="alert" aria-live="polite"></div>Optionally link the inputs to the error container:
id="emailInput" name="email" + aria-describedby="formError" @@ id="addressInput" name="address" + aria-describedby="formError"Also applies to: 497-510, 531-541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/claim.html` at line 433, The error container with id "formError" needs ARIA live/alert semantics so screen readers announce validation and server errors: update the element to include aria-live="assertive" and aria-atomic="true" (or role="alert") and ensure your client-side error update focuses or sets keyboard focus to the container when an error is rendered; also add aria-describedby="formError" to the related form inputs (or toggle that attribute when an error is present) so input fields are programmatically linked to the error message; apply the same changes to the other error containers mentioned (the similar blocks at the other form error spots).website/claim.html-475-475 (1)
475-475:⚠️ Potential issue | 🟡 MinorHarden the external GitHub link.
Add
rel="noopener noreferrer"to thetarget="_blank"link to avoid exposing the opener and referrer unnecessarily.Proposed fix
- <p>© 2026 Pocket Node · <a href="/">Home</a> · <a href="/privacy">Privacy</a> · <a href="https://github.com/RaheemJnr/pocket-node" target="_blank">GitHub</a></p> + <p>© 2026 Pocket Node · <a href="/">Home</a> · <a href="/privacy">Privacy</a> · <a href="https://github.com/RaheemJnr/pocket-node" target="_blank" rel="noopener noreferrer">GitHub</a></p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/claim.html` at line 475, The external GitHub anchor (the <a> tag with href "https://github.com/RaheemJnr/pocket-node" that uses target="_blank") should be hardened by adding rel="noopener noreferrer" to the tag to prevent exposing window.opener and the referrer; update that anchor element to include rel="noopener noreferrer".scripts/batch-send/send.mjs-70-78 (1)
70-78:⚠️ Potential issue | 🟡 MinorBalance check omits the transaction fee.
The pre-check only compares the sum of outputs against the faucet balance; it doesn't account for the fee added later by
payFeeByFeeRate(line 97) nor for cell-capacity rules on the change output. If the balance is only marginally abovetotalAmount, this check passes butpayFeeByFeeRate(or sealing) will fail mid-build with a harder-to-interpret error.Consider adding a small headroom (e.g. estimate
~1 CKBfee slack and the 61 CKB min-capacity for the change cell) before the check, or move the "insufficient" diagnostic into a catch aroundpayFeeByFeeRate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/batch-send/send.mjs` around lines 70 - 78, The balance pre-check using getBalance(indexer, fromLock) currently compares balance to totalAmount but omits transaction fees and potential change-cell minimum capacity, causing late failures in payFeeByFeeRate; update the check by estimating and adding a headroom before exiting (e.g., estimate fee slack ~1 CKB in shannons plus the 61 CKB min change cell capacity) and compare balance.lt(totalAmount.add(headroom)), or alternatively wrap the payFeeByFeeRate(...) call in a try/catch and convert insufficient-funds errors into the same “ERROR: Insufficient balance” diagnostic (include balanceCKB and totalCKB) so failures surface with a clear message; reference symbols: getBalance, balance, totalAmount, totalCKB, balanceCKB, payFeeByFeeRate.scripts/batch-send/send.mjs-34-39 (1)
34-39:⚠️ Potential issue | 🟡 MinorTighten PRIVATE_KEY validation to check hex characters.
The current check only verifies the
0xprefix and total length of 66. A 66-char value like0xZZZ...would pass but fail opaquely insidehd.key.privateKeyToBlake160. A regex check surfaces the problem at the usage step.🛡️ Proposed fix
- const privateKey = process.env.PRIVATE_KEY; - if (!privateKey || !privateKey.startsWith("0x") || privateKey.length !== 66) { + const privateKey = process.env.PRIVATE_KEY; + if (!privateKey || !/^0x[0-9a-fA-F]{64}$/.test(privateKey)) { console.error("ERROR: Set PRIVATE_KEY env var (0x-prefixed, 64 hex chars)"); console.error("Usage: PRIVATE_KEY=0x... node send.mjs"); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/batch-send/send.mjs` around lines 34 - 39, The PRIVATE_KEY validation currently only checks prefix and length; update the check around the privateKey variable in send.mjs to also ensure the 64 characters after "0x" are valid hex (0-9a-fA-F) using a regex, and if it fails, log the same error/usage messages and exit so invalid keys (e.g., 0xZZZ...) are caught before calling hd.key.privateKeyToBlake160.android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsDialog.kt-47-52 (1)
47-52:⚠️ Potential issue | 🟡 MinorNormalize the initial mode against
availableModesand key the remembered state.With the new
availableModesparameter,currentModecan be hidden from the available options. In that case the dialog opens with a hidden selected mode but can still apply it. Also key the remembered initial state so updated parent inputs are reflected when the dialog is reused.Suggested fix
availableModes: List<SyncMode> = SyncMode.entries.toList(), savedCustomBlockHeight: Long? = null ) { - var selectedMode by remember { mutableStateOf(currentMode) } - var customBlockHeight by remember { mutableStateOf(savedCustomBlockHeight?.toString() ?: "") } - var showCustomInput by remember { mutableStateOf(currentMode == SyncMode.CUSTOM) } + val initialMode = remember(currentMode, availableModes) { + currentMode.takeIf { it in availableModes } ?: availableModes.firstOrNull() ?: currentMode + } + var selectedMode by remember(initialMode) { mutableStateOf(initialMode) } + var customBlockHeight by remember(savedCustomBlockHeight) { + mutableStateOf(savedCustomBlockHeight?.toString() ?: "") + } + var showCustomInput by remember(initialMode) { mutableStateOf(initialMode == SyncMode.CUSTOM) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsDialog.kt` around lines 47 - 52, Normalize the initial selection inside SyncOptionsDialog by checking availableModes: if currentMode is not contained in availableModes, initialize selectedMode to a safe default (e.g., availableModes.first()) rather than currentMode; likewise initialize showCustomInput based on that normalized selectedMode. Also key the remembered state so updates from the parent are reflected when the dialog is reused: use remember(...) with appropriate keys (e.g., currentMode and availableModes) for selectedMode, customBlockHeight and showCustomInput so their initial values update when inputs change.android/app/src/main/java/com/rjnr/pocketnode/ui/screens/send/SendScreen.kt-319-343 (1)
319-343:⚠️ Potential issue | 🟡 MinorDisable wallet recipient selection while sending.
The recipient text field is disabled during
uiState.isLoading, but the new “My Wallets” shortcut can still callupdateRecipient(address)during an in-flight send. Gate the shortcut the same way to avoid changing form state mid-transaction.Suggested fix
- TextButton(onClick = { showMyWallets = true }) { + TextButton( + onClick = { showMyWallets = true }, + enabled = !uiState.isLoading + ) { Text("My Wallets", style = MaterialTheme.typography.labelSmall) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/send/SendScreen.kt` around lines 319 - 343, The "My Wallets" shortcut can still call updateRecipient(address) while a send is in progress; prevent this by gating both the menu opener and the items on uiState.isLoading: change the TextButton onClick to only set showMyWallets = true when uiState.isLoading is false (or set the TextButton's enabled property to !uiState.isLoading) and set each DropdownMenuItem's enabled property to !uiState.isLoading (or skip calling updateRecipient when uiState.isLoading). Update references: uiState.otherWallets, showMyWallets, TextButton onClick, DropdownMenuItem, and updateRecipient to ensure no recipient updates occur while uiState.isLoading is true.docs/superpowers/specs/2026-04-13-m3-multi-wallet-fix-design.md-438-443 (1)
438-443:⚠️ Potential issue | 🟡 MinorAdd language identifiers to diagram fences.
markdownlintflagged these fenced blocks with MD040. Usetextfor ASCII diagrams to keep docs lint-clean.Example fix
-``` +```text Wallet Sync Strategy ├── Active Only — "Only syncs the wallet you're using" ├── All Wallets — "Keeps all wallets synced (up to 3)" [default] └── Balanced — "Active wallet real-time, others every 15 min" ```Also applies to: 468-481, 485-488, 497-519, 530-545, 555-574
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-13-m3-multi-wallet-fix-design.md` around lines 438 - 443, Update the ASCII diagram fenced code blocks in the specs document to include a language identifier "text" (e.g., replace ``` with ```text) so markdownlint MD040 is satisfied; target the fenced blocks that contain the "Wallet Sync Strategy" ASCII diagram and the other similar diagram blocks noted in the review and change their opening fence to ```text while keeping the contents unchanged.docs/superpowers/specs/2026-04-13-m3-multi-wallet-fix-design.md-146-146 (1)
146-146:⚠️ Potential issue | 🟡 MinorUpdate stale migration and utility references.
This spec still says the DB bumps to v4 and shows
DatabaseMaintenanceUtilacceptingSupportSQLiteDatabase, but the current PR includeskey_material/KeyMaterialEntityand the utility acceptsAppDatabase. Please update the spec so migration guidance matches the implementation.Proposed doc direction
-**Database version**: Bump to v4. AppModule adds both `MIGRATION_2_3` (corrected) and `MIGRATION_3_4` to the builder. Room handles routing: fresh v2 users get v2→v3→v4, existing v3 users get v3→v4. +**Database version**: Bump through the current schema version. `MIGRATION_2_3` restores wallet scoping, `MIGRATION_3_4` fixes existing v3 `balance_cache` schemas and wallet UI columns, and the key-material migration creates `key_material`. @@ object DatabaseMaintenanceUtil { - fun vacuum(db: SupportSQLiteDatabase) - fun getDatabaseSizeBytes(db: SupportSQLiteDatabase): Long + fun vacuum(db: AppDatabase) + fun getDatabaseSizeBytes(db: AppDatabase): Long }Also applies to: 325-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-13-m3-multi-wallet-fix-design.md` at line 146, Update the spec to match the implementation: change the database target and migration details to reflect bump to v4 with AppModule registering both MIGRATION_2_3 and MIGRATION_3_4, and replace references to DatabaseMaintenanceUtil accepting SupportSQLiteDatabase with the concrete AppDatabase type (and mention handling of key_material/KeyMaterialEntity). Also update any stale lines (around lines 325-331) that describe v2→v3→v4 routing so the migration guidance and utility API in the doc align with the actual classes/method names used in the PR (DatabaseMaintenanceUtil, AppDatabase, KeyMaterialEntity, MIGRATION_2_3, MIGRATION_3_4, AppModule).android/app/src/main/java/com/rjnr/pocketnode/data/database/entity/KeyMaterialEntity.kt-16-26 (1)
16-26:⚠️ Potential issue | 🟡 MinorFix nullable
ByteArrayequality.Line 21 treats
byteArrayOf()as equal tonull. Keep null distinct from an empty encrypted mnemonic.Proposed equality fix
return walletId == other.walletId && encryptedPrivateKey.contentEquals(other.encryptedPrivateKey) && - (encryptedMnemonic?.contentEquals(other.encryptedMnemonic ?: byteArrayOf()) ?: (other.encryptedMnemonic == null)) && + ( + encryptedMnemonic == null && other.encryptedMnemonic == null || + encryptedMnemonic != null && + other.encryptedMnemonic != null && + encryptedMnemonic.contentEquals(other.encryptedMnemonic) + ) && iv.contentEquals(other.iv) && walletType == other.walletType && mnemonicBackedUp == other.mnemonicBackedUp &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/database/entity/KeyMaterialEntity.kt` around lines 16 - 26, The equals implementation in KeyMaterialEntity currently treats a null encryptedMnemonic as equal to an empty byteArray (using encryptedMnemonic?.contentEquals(encryptedMnemonic ?: byteArrayOf())), which is incorrect; update the encryptedMnemonic comparison in equals to first check null equality (i.e., return false if one is null and the other isn't) and only call encryptedMnemonic.contentEquals(other.encryptedMnemonic) when both are non-null so null remains distinct from an empty array while preserving safe use of contentEquals.android/app/src/main/java/com/rjnr/pocketnode/ui/components/WalletAvatar.kt-36-51 (1)
36-51:⚠️ Potential issue | 🟡 MinorUse dynamic foreground colors for avatar initials to ensure readability.
White text fails WCAG AA contrast requirements on 6 of 8 palette colors (Amber: 2.15, Teal: 2.49, Emerald: 2.54, Sky: 2.77, Pink: 3.53, Red: 3.76). Switch to black text on light backgrounds using a luminance threshold.
Proposed contrast-aware foreground
import androidx.compose.ui.graphics.Color +import androidx.compose.ui.graphics.luminance @@ val color = WALLET_COLORS[colorIndex.coerceIn(0, WALLET_COLORS.lastIndex)] + val textColor = if (color.luminance() > 0.179f) Color.Black else Color.White val initial = name.firstOrNull()?.uppercase() ?: "?" @@ Text( text = initial, - color = Color.White, + color = textColor, fontWeight = FontWeight.SemiBold, fontSize = fontSize )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/components/WalletAvatar.kt` around lines 36 - 51, The avatar always uses white text which fails contrast on several WALLET_COLORS; compute a contrast-aware foreground color and use it in Text. Inside WalletAvatar (where you set val color, initial, fontSize and render Box/Text), add e.g. val foreground = if (color.luminance() > 0.5f) Color.Black else Color.White (or adjust the threshold to match your WCAG target) and replace Text(color = Color.White) with Text(color = foreground). Ensure you import/use the Color.luminance() extension available in Compose.docs/superpowers/plans/2026-04-13-key-storage-redesign-phase2.md-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorInconsistent target DB version.
The table row says "bump version to 4", but the rest of the plan (line 7, 9, 381, 541) targets DB version 5 via
MIGRATION_4_5. Fix to avoid confusing readers/agents executing the plan.Proposed fix
-| `data/database/AppDatabase.kt` | Add `KeyMaterialEntity` to entities, add `keyMaterialDao()`, bump version to 4 | +| `data/database/AppDatabase.kt` | Add `KeyMaterialEntity` to entities, add `keyMaterialDao()`, bump version to 5 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-13-key-storage-redesign-phase2.md` at line 37, The table row is inconsistent: change the "bump version to 4" text to "bump version to 5" so it matches the rest of the plan that references MIGRATION_4_5; also verify the entry mentions adding KeyMaterialEntity and keyMaterialDao() and that the AppDatabase migration/versioning references MIGRATION_4_5 (i.e., bump DB version to 5) to keep the plan and migration identifiers consistent.android/app/src/test/java/com/rjnr/pocketnode/data/wallet/KeyManagerMultiWalletTest.kt-21-45 (1)
21-45:⚠️ Potential issue | 🟡 MinorAdd
@Config(sdk = [28], manifest = Config.NONE)annotation to test class.The test is missing the Robolectric SDK pinning that all other tests in this suite use (e.g.,
KeyManagerTest,KeystoreEncryptionManagerTest,KeyStoreMigrationHelperTest). Without it, test behavior may differ across CI agents and local environments.Note: The
KeyManagerconstructor signature on line 40 is correct as-is—it matches the production constructor which only requirescontextandmnemonicManager.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/test/java/com/rjnr/pocketnode/data/wallet/KeyManagerMultiWalletTest.kt` around lines 21 - 45, Add the Robolectric SDK pinning to the test class by annotating the KeyManagerMultiWalletTest class with `@Config`(sdk = [28], manifest = Config.NONE); locate the class declaration "class KeyManagerMultiWalletTest" and place the `@Config` annotation directly above it (alongside the existing `@RunWith`(RobolectricTestRunner::class)) so the test runs consistently across environments.android/app/src/main/java/com/rjnr/pocketnode/ui/screens/activity/ActivityScreen.kt-85-105 (1)
85-105:⚠️ Potential issue | 🟡 MinorMinor: clear
pendingCsvContenton cancellation, and consider a UTF-8 BOM.Two small issues in the export flow:
- If the user dismisses the SAF picker (
uri == null),pendingCsvContentis left populated in memory until the next export. Not a memory leak per se, but it leaks the previously-emitted CSV (which contains wallet transaction data) across export sessions.toByteArray()writes UTF-8 without a BOM. Excel on Windows defaults to locale encoding for CSV and will mojibake any non-ASCII characters (e.g., if a future wallet name or memo field contains them). Prepending\uFEFFresolves this.Proposed fix
val exportLauncher = rememberLauncherForActivityResult( contract = ActivityResultContracts.CreateDocument("text/csv") ) { uri -> - if (uri != null && pendingCsvContent != null) { - context.contentResolver.openOutputStream(uri)?.use { stream -> - stream.write(pendingCsvContent!!.toByteArray()) - } - pendingCsvContent = null - } + val csv = pendingCsvContent + if (uri != null && csv != null) { + context.contentResolver.openOutputStream(uri)?.use { stream -> + // UTF-8 BOM so Excel renders non-ASCII correctly + stream.write("\uFEFF".toByteArray(Charsets.UTF_8)) + stream.write(csv.toByteArray(Charsets.UTF_8)) + } + } + pendingCsvContent = null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/activity/ActivityScreen.kt` around lines 85 - 105, The export flow leaves sensitive CSV data in pendingCsvContent if the SAF picker is cancelled and also writes UTF-8 without a BOM; update the exportLauncher result lambda (the ActivityResultContracts.CreateDocument callback) so that when uri == null it clears pendingCsvContent, and when writing to the stream write a UTF-8 BOM before the CSV bytes (e.g., prepend "\uFEFF" to the CSV content when converting to bytes) so Excel won't mojibake non-ASCII; ensure the LaunchedEffect that sets pendingCsvContent and calls exportLauncher remains unchanged except for producing CSV that will be written with the BOM.docs/superpowers/plans/2026-04-13-key-storage-redesign-phase2.md-930-947 (1)
930-947:⚠️ Potential issue | 🟡 Minor
helper.keyMaterialDaoaccess won't compile.
keyMaterialDaois aprivate valconstructor parameter ofKeyStoreMigrationHelper(see lines 693-697); it is not exposed as a public property, sohelper.keyMaterialDao.count()is inaccessible fromKeyManager. The note on line 946 mentions this caveat, but the snippet above it will fail to build if followed as-is. Recommend resolving the snippet to match the resolution path (addsuspend fun hasAnyKeys(): Boolean = keyMaterialDao.count() > 0toKeyStoreMigrationHelper) so agents don't first write code that fails to compile.Proposed fix
fun hasWallet(): Boolean { val helper = keyStoreMigrationHelper if (helper != null) { - val count = kotlinx.coroutines.runBlocking { - helper.keyMaterialDao.count() - } - if (count > 0) return true + val hasKeys = kotlinx.coroutines.runBlocking { helper.hasAnyKeys() } + if (hasKeys) return true } // Fallback to ESP for pre-migration return prefs.contains(KEY_PRIVATE_KEY) }And in
KeyStoreMigrationHelper:suspend fun hasAnyKeys(): Boolean = keyMaterialDao.count() > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-13-key-storage-redesign-phase2.md` around lines 930 - 947, The hasWallet() snippet fails to compile because keyMaterialDao is a private ctor property of KeyStoreMigrationHelper; instead add a public suspend function on KeyStoreMigrationHelper (e.g. suspend fun hasAnyKeys(): Boolean = keyMaterialDao.count() > 0) and update KeyManager.hasWallet() to call helper.hasAnyKeys() inside the runBlocking block (or otherwise invoke the suspend helper method) rather than referencing helper.keyMaterialDao directly.android/app/src/test/java/com/rjnr/pocketnode/data/database/dao/TransactionDaoWalletTest.kt-83-87 (1)
83-87:⚠️ Potential issue | 🟡 MinorAssert that the target wallet was actually deleted.
This test currently only proves wallet-B survived; it would still pass if
deleteByWalletAndNetwork()deleted nothing.Suggested test assertion
dao.deleteByWalletAndNetwork("wallet-A", "MAINNET") + val deleted = dao.getByWalletAndNetwork("wallet-A", "MAINNET") + assertTrue(deleted.isEmpty()) + val remaining = dao.getByWalletAndNetwork("wallet-B", "MAINNET") assertEquals(1, remaining.size)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/test/java/com/rjnr/pocketnode/data/database/dao/TransactionDaoWalletTest.kt` around lines 83 - 87, The test currently only verifies that "wallet-B" remains but doesn't assert that deleteByWalletAndNetwork("wallet-A", "MAINNET") actually removed wallet-A rows; update the test to call dao.getByWalletAndNetwork("wallet-A", "MAINNET") (or similar retrieval used in this file) after dao.deleteByWalletAndNetwork and assert that the result is empty (e.g., size 0) or null so the deletion is verified; reference the dao.deleteByWalletAndNetwork and dao.getByWalletAndNetwork calls in the TransactionDaoWalletTest to add this assertion.android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt-276-278 (1)
276-278:⚠️ Potential issue | 🟡 MinorPreserve unique wallet names when renaming.
Creates/imports reject duplicate names, but
renameWalletbypasses that constraint. Renaming a wallet to an existing name can leave indistinguishable wallets in management and switcher UI.Suggested adjustment
suspend fun renameWallet(walletId: String, newName: String) { + val existing = walletDao.getAll() + if (existing.any { it.walletId != walletId && it.name.equals(newName, ignoreCase = true) }) { + throw IllegalArgumentException("A wallet named \"$newName\" already exists") + } walletDao.updateName(walletId, newName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt` around lines 276 - 278, renameWallet currently calls walletDao.updateName(walletId, newName) without checking for name collisions; update renameWallet to first check if any existing wallet already uses newName (e.g., via walletDao.getByName or walletDao.existsByName) and if so return/throw a meaningful error or result indicating duplicate-name; otherwise proceed to call walletDao.updateName(walletId, newName). Ensure you reference and reuse existing walletDao methods (or add a simple existsByName/getByName) and keep behavior consistent with create/import flows that enforce unique names.android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt-520-557 (1)
520-557:⚠️ Potential issue | 🟡 MinorMigration infers
walletTypefrom mnemonic presence — read the stored ESP value instead.Line 539 reconstructs
walletTypeasif (mnemonic != null) WALLET_TYPE_MNEMONIC else WALLET_TYPE_RAW_KEY, but the ESP store already has the authoritativeKEY_WALLET_TYPEvalue written bysavePrivateKey/storeKeysForWallet. If an older build ever wrote a different/unexpected value, or a future type is added (e.g. HD sub-account), this lossy inference silently rewrites it during migration. Read the actual value fromgetWalletPrefs(wallet.walletId)(falling back to the inference only when absent).🔧 Proposed fix
val mnemonic = getMnemonicForWallet(wallet.walletId)?.joinToString(" ") - val walletType = if (mnemonic != null) WALLET_TYPE_MNEMONIC else WALLET_TYPE_RAW_KEY + val walletType = getWalletPrefs(wallet.walletId) + .getString(KEY_WALLET_TYPE, null) + ?: if (mnemonic != null) WALLET_TYPE_MNEMONIC else WALLET_TYPE_RAW_KEY val backed = hasMnemonicBackupForWallet(wallet.walletId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt` around lines 520 - 557, The migration currently infers walletType from mnemonic presence in migrateEspToRoomIfNeeded, which can overwrite the authoritative ESP-stored KEY_WALLET_TYPE; instead, read the stored type from getWalletPrefs(wallet.walletId) and use that value when present, falling back to the existing mnemonic-based inference (if prefs missing or KEY_WALLET_TYPE absent). Update the walletType assignment in migrateEspToRoomIfNeeded to first call getWalletPrefs(wallet.walletId), check for KEY_WALLET_TYPE, and only use WALLET_TYPE_MNEMONIC/WALLET_TYPE_RAW_KEY as a fallback.android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt-362-378 (1)
362-378:⚠️ Potential issue | 🟡 MinorAdd a
mnemonicBackedUp: Boolean = falseparameter or document that this API always resets backup state tofalse.The hardcoded
mnemonicBackedUp = falseis intentional and currently handled correctly by all callers (they callsetMnemonicBackedUpForWalletafterwards when needed). However, this design is fragile—it relies on every future caller remembering to manage the backup state separately. A parameter or explicit documentation would prevent accidental regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt` around lines 362 - 378, The storeKeysForWallet function currently hardcodes mnemonicBackedUp = false which can lead to accidental regressions; add a new parameter mnemonicBackedUp: Boolean = false to storeKeysForWallet and pass that value into both writeToRoom (where the backup state is stored) and the KeyMaterial created inside writeBackupIfPinAvailable, so callers can override the default if needed; keep the default as false to preserve current behavior and ensure existing callers (e.g., callers that call setMnemonicBackedUpForWallet later) continue to work.
🧹 Nitpick comments (6)
scripts/batch-send/send.mjs (1)
174-174:parseCSVLinedoesn't handle newlines inside quoted fields.
csvText.split("\n")on line 174 splits the CSV beforeparseCSVLineruns, so any quoted field containing a newline is pre-truncated and both halves will parse as malformed rows. CKB addresses themselves shouldn't contain newlines, but if the Google Sheet gains a "notes" or similar free-text column before theCKB Addresscolumn, the index alignment on line 199 can silently land on the wrong column.Low priority given the current sheet schema — flagging for awareness. If you want hardening, either switch to a streaming CSV parser (e.g.
csv-parse) or tokenize the full text tracking quote state across line boundaries.Also applies to: 226-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/batch-send/send.mjs` at line 174, The current split on newlines via the lines variable (const lines = csvText.split("\n")...) breaks quoted fields that contain embedded newlines before parseCSVLine runs, causing malformed rows and mis-aligned indexing later (see where you extract the CKB column around the index usage near the parse loop). Fix by replacing the naive split strategy: either switch to a robust CSV parser (e.g., csv-parse) and feed the full csvText so quoted newlines are handled, or implement a tokenizer that iterates csvText and tracks quote state to produce logical CSV records before calling parseCSVLine; update any code referencing parseCSVLine, the lines variable, and the CKB address column index to consume those logical records instead of line-split strings.android/app/src/main/java/com/rjnr/pocketnode/ui/screens/settings/SettingsViewModel.kt (1)
25-25: Nit: use an import instead of the fully-qualified type.The other wallet-package types are already imported at the top (
SyncStrategy,ThemeMode,WalletPreferences). Inliningcom.rjnr.pocketnode.data.wallet.WalletRepositoryin the primary constructor is inconsistent.Proposed fix
import com.rjnr.pocketnode.data.wallet.SyncStrategy import com.rjnr.pocketnode.data.wallet.ThemeMode import com.rjnr.pocketnode.data.wallet.WalletPreferences +import com.rjnr.pocketnode.data.wallet.WalletRepository @@ - private val walletRepository: com.rjnr.pocketnode.data.wallet.WalletRepository + private val walletRepository: WalletRepository🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/settings/SettingsViewModel.kt` at line 25, The primary-constructor parameter uses a fully-qualified type for WalletRepository; update SettingsViewModel by adding an import for com.rjnr.pocketnode.data.wallet.WalletRepository at the top and change the constructor parameter type from com.rjnr.pocketnode.data.wallet.WalletRepository to the simple WalletRepository identifier (the parameter name is walletRepository) so it matches the other imported wallet-package types.android/app/src/test/java/com/rjnr/pocketnode/data/crypto/KeystoreEncryptionManagerTest.kt (1)
42-54: PreferassertThrowsover empty try/catch.The empty
catch (e: Exception)swallows the exception (detekt: SwallowedException) and accepts any exception type — a bug that throwsIllegalStateExceptionfromdecryptwould falsely pass. UseassertThrowsto assert on the expected AEAD failure type (or at minimumGeneralSecurityException/AEADBadTagException).Proposed fix
`@Test` fun `decrypt with wrong IV fails`() { val plaintext = "secret".toByteArray() val (ciphertext, _) = manager.encrypt(plaintext) val wrongIv = ByteArray(12) { 0xFF.toByte() } - try { - manager.decrypt(ciphertext, wrongIv) - fail("Should throw on wrong IV") - } catch (e: Exception) { - // Expected - } + assertThrows(javax.crypto.AEADBadTagException::class.java) { + manager.decrypt(ciphertext, wrongIv) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/test/java/com/rjnr/pocketnode/data/crypto/KeystoreEncryptionManagerTest.kt` around lines 42 - 54, Replace the empty try/catch in the test `decrypt with wrong IV fails` with an assertion that the expected exception is thrown: use JUnit's assertThrows (or an equivalent) to call manager.decrypt(ciphertext, wrongIv) and assert it throws a narrow crypto-related exception (prefer AEADBadTagException or at least GeneralSecurityException) instead of catching Exception; update the test `fun decrypt with wrong IV fails()` to assert the specific failure from manager.decrypt rather than swallowing any exception.android/app/src/main/java/com/rjnr/pocketnode/ui/screens/status/NodeStatusViewModel.kt (1)
64-93: Optional: decouple DB size refresh from 3s status tick.
DatabaseMaintenanceUtil.getDatabaseSizeBytes(appDatabase)runs on every 3-second refresh. DB size changes on the order of seconds-to-minutes (writes via sync), so polling it this frequently is wasteful IO. Consider caching with a longer interval (e.g., every 30s or on sync completion), or moving it to a one-shot refresh when the Storage card becomes visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/status/NodeStatusViewModel.kt` around lines 64 - 93, The updateStatus() method in NodeStatusViewModel is calling DatabaseMaintenanceUtil.getDatabaseSizeBytes(appDatabase) on every 3s tick, causing unnecessary IO; change this by decoupling DB size reads: introduce a cached timestamp+value inside NodeStatusViewModel (e.g., dbSizeCache and dbSizeLastRefreshMs) and in updateStatus() only refresh dbSize by calling DatabaseMaintenanceUtil.getDatabaseSizeBytes(appDatabase) when the cache is older than a longer interval (e.g., 30_000 ms) or when an explicit trigger occurs (e.g., onStorageVisible() or a sync completion callback), otherwise reuse cached dbSizeBytes when updating _uiState; ensure the IO call remains on Dispatchers.IO and update the cached value and timestamp when refreshed.android/app/src/test/java/com/rjnr/pocketnode/data/wallet/WalletRepositoryTest.kt (1)
36-39: Add a real migration test for the upgraded schemas.This in-memory database starts at the latest schema, so the registered migrations are not actually exercised. Given the PR includes Room migration fixes, add a
MigrationTestHelpertest that creates an older schema DB, runsMIGRATION_2_3throughMIGRATION_4_5, and validates wallet/key columns and indexes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/test/java/com/rjnr/pocketnode/data/wallet/WalletRepositoryTest.kt` around lines 36 - 39, The current test builds an in-memory DB at the latest schema so the migration objects (MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5) are never exercised; replace or add a migration test in WalletRepositoryTest that uses android.arch.persistence.db.framework.MigrationTestHelper (or androidx.room.testing.MigrationTestHelper) to create a on-disk DB at an older version (e.g., version 2), then run the migrations MIGRATION_2_3 through MIGRATION_4_5 via helper.runMigrationsAndValidate and assert the final schema and data: verify wallet and key columns exist, indexes are present, and any expected column types/defaults are correct; reference the existing MIGRATION_* constants and the db creation logic to wire the helper and validations.android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt (1)
596-604: Pass a copy ofsessionPintowriteBackupto protect against defensive zeroing.
manager.writeBackup(walletId, buildMaterial(), pin)hands the class-heldsessionPinarray toKeyBackupManagerby reference. IfwriteBackup(or anything it delegates to) ever zeroes the array defensively after use — a reasonable thing to do with PIN material — subsequent backup writes in the same session will silently no-op becausesessionPinis now all-zero (still non-null, so the early-return check at line 597 doesn't catch it either). Passpin.copyOf()to isolate the caller's copy.🔧 Proposed diff
private fun writeBackupIfPinAvailable(walletId: String, buildMaterial: () -> KeyMaterial) { val pin = sessionPin ?: return val manager = keyBackupManager ?: return try { - manager.writeBackup(walletId, buildMaterial(), pin) + manager.writeBackup(walletId, buildMaterial(), pin.copyOf()) } catch (e: Exception) { Log.w(TAG, "Backup write failed for $walletId", e) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt` around lines 596 - 604, The writeBackupIfPinAvailable function currently passes the class-held sessionPin array by reference into KeyBackupManager.writeBackup which risks the sessionPin being defensively zeroed; change the call in writeBackupIfPinAvailable to pass a copy of the pin (e.g., sessionPin.copyOf()) instead of the original sessionPin so KeyBackupManager.writeBackup receives an isolated array; keep the early-return checks for sessionPin and keyBackupManager and ensure only the argument to manager.writeBackup is altered.
Responding to CodeRabbit review on PR #80. Critical - GatewayRepository startup: wrap the scope.launch migration sequence in try/catch. Before, any exception in walletMigrationHelper.migrateIfNeeded, keyManager.migrateEspToRoomIfNeeded, or initializeNode left _nodeReady null forever and awaitNodeReady() suspended indefinitely. Now set _nodeReady.value = false on failure so callers see an error. - WalletRepository.deleteWallet: destroy keys AFTER the Room row + cache deletes commit, inside a withTransaction block. Previous ordering could leave an orphaned wallet row with no recoverable key material if the DAO delete threw. - KeyManager.writeBackupIfPinAvailable was silently a no-op because sessionPin was never set from production code (only AuthManager.sessionPin was). Inject AuthManager into KeyManager, prefer authManager.getSessionPin() and fall back to the local field for tests. Wire via provideKeyManager. Also propagate the PIN to AuthManager in PinViewModel on CONFIRM and VERIFY so mandatory-PIN setup, SecuritySettings setup, and auth-unlock flows all populate the session PIN in one place. Major - DaoSyncManager.getActiveDeposits / getCompletedDeposits / insertPendingDeposit: drop the walletId = "" default, add require(walletId.isNotBlank()). Prevents future call sites silently reading/writing unscoped rows. Existing production call site already passes activeWalletId. Updated DaoSyncManagerTest accordingly. - GatewayRepository.sendTransaction: capture _walletInfo.value before the 5-second delayed rescan. If the user switches wallets during the delay, the partial re-register now correctly targets the sender's script rather than the new active wallet. Skipped: Major finding about create/import flows in legacy GatewayRepository.createNewWallet/importExistingWallet leaving activeWalletId stale. That path is not reached from M3 (WalletRepository owns creation); untangling it is a separate cleanup, not a release blocker.
Fix the minor findings that were real bugs or trivial polish wins. The remaining 15 items (docs, off-chain scripts, deprecated ESP fallback code, test-infra polish, post-release a11y/perf) were triaged out of this PR. Real bugs - KeyMaterialEntity.equals: null encryptedMnemonic was being compared equal to an empty ByteArray via the `?: byteArrayOf()` short-circuit. Keeps null distinct from empty now. - WalletRepository.renameWallet bypassed the unique-name check that create/import enforce. Added the same validation so two wallets can't share a name via rename. - KeyManager.migrateEspToRoomIfNeeded was reconstructing walletType from mnemonic presence, silently overwriting the authoritative ESP-stored KEY_WALLET_TYPE. Read the ESP pref first, fall back to the inference only when absent. UX / correctness - ActivityScreen CSV export: always clear pendingCsvContent (not only on success) so a dismissed SAF picker doesn't leave wallet tx data in memory for the next export. Prepend a UTF-8 BOM so Excel on Windows doesn't mojibake non-ASCII fields. - SendScreen "My Wallets" shortcut: gate the TextButton and each DropdownMenuItem on !uiState.isLoading so recipient can't change mid-transaction. - SyncOptionsDialog: normalize initial selectedMode against availableModes so the dialog never opens with a hidden selection. Key the remembered state on inputs so parent updates flow through. - WalletAvatar: pick Color.Black on high-luminance palette entries so the initial passes WCAG AA contrast (Amber, Teal, Emerald, Sky, Pink, Red were all failing with white text). Defensive security - KeyManager.writeBackupIfPinAvailable passes pin.copyOf() to KeyBackupManager.writeBackup so any defensive zeroing by the callee doesn't silently clear the cached session PIN. Style - SettingsViewModel: replace fully-qualified WalletRepository type with an import to match the other wallet-package types in the file.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/com/rjnr/pocketnode/ui/screens/auth/PinViewModel.kt (1)
100-116:⚠️ Potential issue | 🟡 MinorSession PIN should be explicitly cleared on app backgrounding and sign-out.
Seeding
authManager.setSessionPin(...)on successful CONFIRM and VERIFY is correct, andAuthManager.setSessionPindefensively zeroes the previous buffer viaclearSession(), making repeated calls safe.However, the session PIN persists in memory beyond app backgrounding. While
MainActivity.onStop()exists and sets_requireReauth, it does not callauthManager.clearSession(). Additionally,KeyManager.clearSessionPin()exists but is never invoked. For the threat model to hold, both should be called explicitly:
authManager.clearSession()onMainActivity.onStop()(or similar lifecycle hook)keyManager.clearSessionPin()during sign-out or wallet switchTwo secondary notes (not blocking, but worth addressing separately):
pinis aString(immutable), sopin.toCharArray()cannot guarantee zeroability — the originalStringcopy persists on the heap until GC. True zeroization would requireCharArrayend-to-end throughenteredDigitsand all parameters.- Verify that wallet switching clears both session PIN instances.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/auth/PinViewModel.kt` around lines 100 - 116, Call authManager.clearSession() from the app lifecycle stop handler (e.g., MainActivity.onStop()) so the seeded session PIN is explicitly wiped when the app backgrounds, and invoke keyManager.clearSessionPin() during sign-out and wallet-switch flows (wherever signOut or walletSwitch logic lives) to ensure both KeyManager and AuthManager session PINs are cleared; update the places that currently call authManager.setSessionPin(...) (PinViewModel CONFIRM and VERIFY) only to seed the session, and add the corresponding clearSession()/clearSessionPin() calls on lifecycle stop and in sign-out/wallet-switch code paths respectively.
🧹 Nitpick comments (3)
android/app/src/test/java/com/rjnr/pocketnode/data/gateway/DaoSyncManagerTest.kt (1)
96-101: ReplacerunBlockinginsiderunTestwithassertFailsWith.
assertThrows { runBlocking { … } }inside arunTest { … }coroutine block works but nests two coroutine runners and sidesteps the test dispatcher. Usekotlin.test.assertFailsWith(which supports suspend lambdas) for a cleaner assertion:- `@Test` - fun `getActiveDeposits rejects blank walletId`() = runTest { - assertThrows(IllegalArgumentException::class.java) { - kotlinx.coroutines.runBlocking { manager.getActiveDeposits("MAINNET", walletId = "") } - } - } + `@Test` + fun `getActiveDeposits rejects blank walletId`() = runTest { + kotlin.test.assertFailsWith<IllegalArgumentException> { + manager.getActiveDeposits("MAINNET", walletId = "") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/test/java/com/rjnr/pocketnode/data/gateway/DaoSyncManagerTest.kt` around lines 96 - 101, The test `getActiveDeposits rejects blank walletId` nests runBlocking inside runTest and uses JUnit's assertThrows; replace that pattern with kotlin.test.assertFailsWith to assert the suspend function throws without nesting coroutine runners: inside the runTest block call assertFailsWith<IllegalArgumentException> and invoke the suspend function manager.getActiveDeposits("MAINNET", walletId = "") directly (no runBlocking or assertThrows) so the test uses the test dispatcher and supports suspend lambdas correctly.android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt (1)
289-309:deleteWalletordering looks good; consider a brief note that VACUUM can't run in a txn.Transactional row+cache delete followed by key destruction addresses the earlier concern about orphaned key material on DAO failure.
DatabaseMaintenanceUtil.vacuum(appDatabase)correctly runs outside thewithTransactionblock (SQLite forbidsVACUUMinside a txn), which looks intentional. A one‑line comment would help future readers not accidentally fold it inside the block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt` around lines 289 - 309, Add a short explanatory comment in deleteWallet explaining why DatabaseMaintenanceUtil.vacuum(appDatabase) must run outside the transaction (SQLite forbids VACUUM inside a txn) so future maintainers don't accidentally move it into the appDatabase.withTransaction block; place the one-line comment immediately above the vacuum call (after keyManager.deleteWalletKeys()) and keep the vacuum invocation as-is.android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt (1)
1669-1734:registerAllWalletScriptssilently caps at 3 wallets — make the drop observable.The
.take(3)cap is reasonable for bounding native resource usage, but wallets ranked 4+ bylastActiveAtare dropped with only a file‑local// sync capcomment. InALL_WALLETSmode a user with 4+ wallets will see balance/tx tracking "just stop working" for the dropped wallets with no feedback. Consider:
- Logging at
Log.iwhich wallets were dropped (by name or id) so users/support can diagnose.- Exposing the cap as a constant (e.g.
MAX_CONCURRENT_WALLET_SCRIPTS) so it's greppable and documentable in the Settings UI that advertises the strategy.- val wallets = walletDao.getAll() - .sortedByDescending { it.lastActiveAt } - .take(3) // sync cap + val allWallets = walletDao.getAll().sortedByDescending { it.lastActiveAt } + val wallets = allWallets.take(MAX_CONCURRENT_WALLET_SCRIPTS) + if (allWallets.size > wallets.size) { + Log.i(TAG, "ALL_WALLETS: only top-${wallets.size} of ${allWallets.size} wallets will sync " + + "(dropped: ${allWallets.drop(wallets.size).map { it.walletId }})") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt` around lines 1669 - 1734, registerAllWalletScripts currently silently limits scripts via wallets.take(3) causing wallets beyond the cap to be dropped without visibility; replace the hardcoded .take(3) with a named constant (e.g. MAX_CONCURRENT_WALLET_SCRIPTS) and before truncating compute the dropped set from wallets.drop(MAX_CONCURRENT_WALLET_SCRIPTS), then emit an informative Log.i listing the dropped wallet identifiers (wallet.walletId or wallet.name) so support/users can diagnose; update any documentation/comments to reference MAX_CONCURRENT_WALLET_SCRIPTS and ensure registerAllWalletScripts and the wallets variable use the constant.
🤖 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/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt`:
- Around line 118-153: When switching wallets in onActiveWalletChanged, reset
the sync progress tracker so prior wallet samples don't bias the new wallet; add
a call to clear or reset syncProgressTracker (e.g., syncProgressTracker.reset()
or syncProgressTracker.clearSamples()/recreate the tracker instance) immediately
after updating activeWalletId/activeWalletType (before starting
registration/polling) so the polling loop starts with a clean baseline for
percentage, etaDisplay, wasSyncing and justReachedTip.
In `@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.kt`:
- Around line 530-572: The issue is that calling getWalletPrefs(wallet.walletId)
lazily creates an ESP file for Room-only wallets when helper.readDecryptedKey
returns null; to avoid that side-effect, only consult ESP prefs when an ESP
private key was actually recovered. In migrateEspToRoomIfNeeded, move the
getWalletPrefs(...) .getString(KEY_WALLET_TYPE, ...) call so it happens after
you successfully obtain privKeyHex (i.e., after the getPrivateKeyForWallet
try/?: continue), and only then compute walletType using the pref fallback to
mnemonic/RAW; keep helper.migrateWallet, helper.readDecryptedKey verification,
and the continue behavior unchanged.
In
`@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt`:
- Around line 86-204: The legacy KeyManager APIs
(keyManager.generateWalletWithMnemonic, importWalletFromMnemonic, importWallet)
are currently used only to get WalletInfo but they persist keys to the "default"
wallet; instead derive WalletInfo locally and avoid the legacy write paths: in
createWallet call mnemonicManager.generateMnemonic /
mnemonicManager.mnemonicToPrivateKey(words) to get the private key and then call
keyManager.deriveWalletInfo(privateKeyBytes) to build info (remove
keyManager.generateWalletWithMnemonic), in importWallet convert the provided
words to privateKey via mnemonicManager.mnemonicToPrivateKey(words, passphrase)
then call keyManager.deriveWalletInfo(privateKeyBytes) (remove
keyManager.importWalletFromMnemonic), and in importRawKey convert privateKeyHex
to bytes and call keyManager.deriveWalletInfo(privateKeyBytes) (replace
keyManager.importWallet), keeping storeKeysForWallet(walletId, privateKeyBytes,
words/null) and other logic unchanged.
In `@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/send/SendScreen.kt`:
- Around line 333-343: Selecting a wallet may call updateRecipient with an empty
string because WalletEntity defaults addresses to "", so add a guard in the
DropdownMenuItem onClick inside SendScreen (the block iterating
uiState.otherWallets) to check the resolved address (use uiState.networkType ?
wallet.mainnetAddress : wallet.testnetAddress) is not blank before calling
updateRecipient and hiding the menu; if the address is blank, do not call
updateRecipient (optionally surface a user message) and keep or close the menu
appropriately to avoid clearing an existing recipient.
---
Outside diff comments:
In
`@android/app/src/main/java/com/rjnr/pocketnode/ui/screens/auth/PinViewModel.kt`:
- Around line 100-116: Call authManager.clearSession() from the app lifecycle
stop handler (e.g., MainActivity.onStop()) so the seeded session PIN is
explicitly wiped when the app backgrounds, and invoke
keyManager.clearSessionPin() during sign-out and wallet-switch flows (wherever
signOut or walletSwitch logic lives) to ensure both KeyManager and AuthManager
session PINs are cleared; update the places that currently call
authManager.setSessionPin(...) (PinViewModel CONFIRM and VERIFY) only to seed
the session, and add the corresponding clearSession()/clearSessionPin() calls on
lifecycle stop and in sign-out/wallet-switch code paths respectively.
---
Nitpick comments:
In
`@android/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.kt`:
- Around line 1669-1734: registerAllWalletScripts currently silently limits
scripts via wallets.take(3) causing wallets beyond the cap to be dropped without
visibility; replace the hardcoded .take(3) with a named constant (e.g.
MAX_CONCURRENT_WALLET_SCRIPTS) and before truncating compute the dropped set
from wallets.drop(MAX_CONCURRENT_WALLET_SCRIPTS), then emit an informative Log.i
listing the dropped wallet identifiers (wallet.walletId or wallet.name) so
support/users can diagnose; update any documentation/comments to reference
MAX_CONCURRENT_WALLET_SCRIPTS and ensure registerAllWalletScripts and the
wallets variable use the constant.
In
`@android/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.kt`:
- Around line 289-309: Add a short explanatory comment in deleteWallet
explaining why DatabaseMaintenanceUtil.vacuum(appDatabase) must run outside the
transaction (SQLite forbids VACUUM inside a txn) so future maintainers don't
accidentally move it into the appDatabase.withTransaction block; place the
one-line comment immediately above the vacuum call (after
keyManager.deleteWalletKeys()) and keep the vacuum invocation as-is.
In
`@android/app/src/test/java/com/rjnr/pocketnode/data/gateway/DaoSyncManagerTest.kt`:
- Around line 96-101: The test `getActiveDeposits rejects blank walletId` nests
runBlocking inside runTest and uses JUnit's assertThrows; replace that pattern
with kotlin.test.assertFailsWith to assert the suspend function throws without
nesting coroutine runners: inside the runTest block call
assertFailsWith<IllegalArgumentException> and invoke the suspend function
manager.getActiveDeposits("MAINNET", walletId = "") directly (no runBlocking or
assertThrows) so the test uses the test dispatcher and supports suspend lambdas
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f051382-e210-41fb-a6a8-76036700eb81
📒 Files selected for processing (13)
android/app/src/main/java/com/rjnr/pocketnode/data/database/entity/KeyMaterialEntity.ktandroid/app/src/main/java/com/rjnr/pocketnode/data/gateway/DaoSyncManager.ktandroid/app/src/main/java/com/rjnr/pocketnode/data/gateway/GatewayRepository.ktandroid/app/src/main/java/com/rjnr/pocketnode/data/wallet/KeyManager.ktandroid/app/src/main/java/com/rjnr/pocketnode/data/wallet/WalletRepository.ktandroid/app/src/main/java/com/rjnr/pocketnode/di/AppModule.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsDialog.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/components/WalletAvatar.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/activity/ActivityScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/auth/PinViewModel.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/send/SendScreen.ktandroid/app/src/main/java/com/rjnr/pocketnode/ui/screens/settings/SettingsViewModel.ktandroid/app/src/test/java/com/rjnr/pocketnode/data/gateway/DaoSyncManagerTest.kt
✅ Files skipped from review due to trivial changes (1)
- android/app/src/main/java/com/rjnr/pocketnode/data/database/entity/KeyMaterialEntity.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- android/app/src/main/java/com/rjnr/pocketnode/ui/components/SyncOptionsDialog.kt
- android/app/src/main/java/com/rjnr/pocketnode/ui/components/WalletAvatar.kt
- android/app/src/main/java/com/rjnr/pocketnode/ui/screens/activity/ActivityScreen.kt
- android/app/src/main/java/com/rjnr/pocketnode/ui/screens/settings/SettingsViewModel.kt
CodeRabbit round 2 on PR #80. Real bugs (actionable inline) - WalletRepository.createWallet / importWallet / importRawKey were routing through legacy keyManager.generateWalletWithMnemonic / importWalletFromMnemonic / importWallet(hex) just to obtain WalletInfo. Those legacy APIs persist key material under the "default" walletId slot (and fire a PIN backup under "default") on every call, so the M3 "default" Room row was being overwritten to whichever wallet was last created/imported, breaking the legacy single-wallet upgrade fallback in GatewayRepository and leaving an orphan backup file pointing at the most recent wallet. Now derive WalletInfo locally via mnemonicManager + keyManager.deriveWalletInfo so only the correct walletId slot is written. - GatewayRepository.onActiveWalletChanged now resets syncProgressTracker, wasSyncing, and _syncProgress on every wallet switch. Otherwise the new wallet's polling loop appends samples onto the previous wallet's baseline, reporting incorrect percentage / ETA and occasionally firing justReachedTip spuriously right after a switch. - KeyManager.migrateEspToRoomIfNeeded: skip wallets whose ESP shared_prefs file doesn't exist on disk (Room-only wallets). Prevents lazily creating an empty ESP file just to check the walletType, which deleteEspFilesIfSafe would then have to clean up. Also hoists the prefs access out of the check and into the authoritative-ESP branch. - SendScreen "My Wallets" dropdown: guard against wallets whose address for the current network is blank (WalletEntity defaults addresses to ""). Selecting such a wallet would otherwise clear the recipient field. Defensive security (outside-diff) - MainActivity.onStop clears both authManager.clearSession() and keyManager.clearSessionPin() when the app backgrounds with a PIN set, so the cached session PIN doesn't persist across foregrounding. The re-auth gate at onStop already forced the user through Auth; this aligns the in-memory state with that. Small polish - WalletRepository.deleteWallet: one-line comment explaining why VACUUM must run outside the withTransaction block. - GatewayRepository.registerAllWalletScripts: replace the anonymous .take(3) with a named MAX_CONCURRENT_WALLET_SCRIPTS constant and log which wallets were dropped so support can diagnose "wallet X isn't syncing" questions. Skipped: nitpick about assertFailsWith in DaoSyncManagerTest (runBlocking-inside-runTest works, just slightly less idiomatic); outside- diff suggestion to clear session PIN on wallet switch (would force a PIN re-entry per switch and break backup writes — backgrounding is the right boundary for the cache lifetime).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deleteSharedPreferencesfrom StrongBox fallback — never delete key material on encryption failureCloses
Closes #55, #56, #57, #58, #59, #60, #61, #62, #63, #64, #65, #66, #67, #68, #69, #70, #71, #72, #73, #74, #78, #79
Test plan
./gradlew testDebugUnitTestpasses (SyncProgressTracker, UpdateRepository, Room migrations, etc.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes & Improvements