Skip to content

Conversation

@ovitrif
Copy link
Collaborator

@ovitrif ovitrif commented Nov 7, 2025

Completes Roadmap item: Backup & Restore

Backup & Restore System Polish across all Core Flows

Description

This PR delivers significant architectural improvements and UX polish across three major areas of the app:

  1. Backup & Restore System
  2. Wallet Wipe Feature
  3. Restore Wallet Screen
  4. Activity List & Filtering

1. Backup & Restore System

Changes: Robustness optimisations, various fixes, UI polish

  • Prevent backup triggers during wipe or restore
  • Prevent timed sheets popping-up before settings are restored
  • Restore caches early so home screen can display correct balances
  • Added relative time subtitles, like "2 hours ago", for backup categories status
  • Preserve backup timestamps after restore
  • Adopted payload models for all backups
  • Improved thread safety for state management during wipe & restore
  • Fixed race conditions in backup and restore operations

2. Wallet Wipe Feature

Changes: Refactor to domain use-case, various fixes & performance optimization

  • Extracted wipe logic to use case: WipeWalletUseCase
  • Reordered wipe to make sure it clears data in right oder
  • Included wipe for core database, blocktank data, and logs
  • Added unit tests

3. Restore Wallet Screen

Changes: Full refactor to MVVM, UI/UX polish, paste UX overhaul, unit tests

  • Refactored screen to MVVM, introducing viewModel
  • Replaced custom BIP39 utils with Bip39Service which uses the bitkit-core APIs
  • UX improvements: keyboard nav, focus states, validation timing and paste handler

4. Activity List & Filtering

Changes: Full refactor of lists & filters state management, fixes and improvements

  • Added activity state reset on wallet wipe
  • Improved performance by using reactive flows for filtering and state scoping
  • Pub-Sub mechanism for real-time sync of in-memory lists with bitkit-core activity db
  • Simplified filter state management in viewModel with new data class
  • Refactored activity tag backup & restore to use new bitkit-core metadata models

Preview

Backup Settings

Restored Wallet New Wallet

QA Notes

1️⃣ Main Test

  • New wallet
    • Do enough changes to warrant relevant and easily verified backups
    • Reset & restore
      • optional: when in Restore screen from Onboarding regression test its functionalities
    • Expect no regressions
    • Tap on activity detail - expect detail screen
    • Go to Activity List screen - expect list filled, switch tabs, apply filters
      • optional: Go to backup settings screens to validate new time ago subtitles in 2 or 3 languages

2️⃣ Regression Test

  • Onboarding Restore Screen (all functionalities)
  • Activity List & Filtering (all functionalities)
  • Reset & Restore (major overhaul)

3️⃣ Slop Tests

A. Backup & Restore System

Backup Status Display:

  • Verify backup status shows relative time ("2 hours ago", "5 minutes ago")
  • Verify relative time updates correctly (check after 1+ minute)
  • Verify "never" status for fresh install
  • Check various time ranges: seconds, minutes, hours, days, weeks, months, years

Backup Execution:

  • Manually trigger backup from settings
  • Verify backup success state and timestamp update
  • Trigger multiple rapid backup attempts (race condition test)
  • Verify only one backup runs at a time
  • Verify stale "running" flags are cleared on app restart

Backup Content: (regression)

  • Backup and restore app settings
  • Backup and restore widgets
  • Backup and restore activity metadata (tags, notes)
  • Verify backup timestamps are preserved across restore operations

Backup During Special States:

  • Verify backup is prevented/gated during wallet restore
  • Verify backup is prevented during wallet wipe operation
  • Verify backup can resume normally after restore completes

B. Wallet Wipe

Complete Cleanup:

  • Wipe wallet from settings
  • Verify all activity data is cleared
  • Verify all tags are removed
  • Verify core database is wiped
  • Verify logs are reset
  • Verify blocktank repository data is reset
  • Verify backup state is reset
  • Verify app returns to onboarding screen

Edge Cases:

  • Verify no crashes or data corruption during wipe
  • Verify clean state after wipe (no residual data)
  • Network interruption during backup operation
  • App restart with backup in progress
  • Rapid filter changes in activity list
  • Pasting invalid mnemonic formats (special characters, numbers only, etc.)
  • Very long activity lists (1000+ items)

C. Onboarding Restore Screen

Basic Restore Scenarios: (regression)

  • Restore wallet with valid 12-word mnemonic
  • Restore wallet with valid 24-word mnemonic
  • Restore wallet with passphrase (BIP39 + passphrase)
  • Verify invalid word detection and error messages
  • Verify BIP39 checksum validation catches incorrect word combinations

Paste Functionality:

  • Paste mnemonic with space-separated words (regression)
  • Paste mnemonic with tab-separated words
  • Paste mnemonic with newline-separated words
  • Paste mnemonic with mixed separators
  • Verify keyboard dismisses after valid complete mnemonic paste
  • Verify partial paste fills only available word inputs

Input Navigation & UX:

  • Navigate between word inputs using keyboard next/done
  • Press backspace on empty input to go to previous input
  • Verify focused input shows bold text
  • Verify focused input doesn't show validation errors
  • Verify unfocused inputs show validation errors for invalid words
  • Verify word suggestions appear correctly

State Management: (regression)

  • Verify transaction sheets don't open during restore
  • Verify address rotation occurs on restore
  • Verify activity list & details work after restore

D. Activity List & Filtering (regression)

Display & Navigation:

  • View all activities in activity list (onchain, lightning, transfers)
  • Switch between tabs: All / Lightning / Onchain
  • Verify correct transaction types display in each tab
  • Scroll through long activity lists (performance check)

Search & Filtering: (regression)

  • Search activities by description/notes
  • Filter by single tag
  • Filter by multiple tags simultaneously
  • Apply date range filters
  • Clear filters and verify list resets
  • Verify filter state persists across app restarts

Tag Backup & Restore: (regression)

  • Tag activities with custom tags
  • Create backup after tagging activities
  • Wipe wallet completely
  • Restore from backup and verify tags are restored
  • Verify tag filters work after restore

@ovitrif ovitrif self-assigned this Nov 7, 2025
@synonymdev synonymdev deleted a comment from claude bot Nov 14, 2025
@synonymdev synonymdev deleted a comment from claude bot Nov 14, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR delivers comprehensive improvements to the Backup & Restore system across multiple areas including wallet wipe functionality, restore wallet screen refactoring, and activity list management. The changes modernize the codebase by migrating to MVVM patterns, improving state management with reactive flows, and enhancing UX with relative timestamps.

  • Full refactor of RestoreWalletScreen to MVVM with new RestoreWalletViewModel
  • Extraction of wallet wipe logic into WipeWalletUseCase with comprehensive unit tests
  • Activity list filtering refactored to use reactive flows and simplified state management
  • Backup system improvements including payload models, timestamp preservation, and thread safety

Reviewed Changes

Copilot reviewed 47 out of 47 changed files in this pull request and generated no comments.

Show a summary per file
File Description
gradle/libs.versions.toml Updates bitkit-core dependency to version 0.1.27
app/src/test/java/to/bitkit/viewmodels/RestoreWalletViewModelTest.kt Adds comprehensive unit tests for RestoreWalletViewModel (663 lines)
app/src/test/java/to/bitkit/usecases/WipeWalletUseCaseTest.kt Adds unit tests for WipeWalletUseCase covering various scenarios
app/src/main/java/to/bitkit/viewmodels/RestoreWalletViewModel.kt New ViewModel implementing restore wallet logic with state management
app/src/main/java/to/bitkit/viewmodels/ActivityListViewModel.kt Refactored to use reactive flows and simplified filter state
app/src/main/java/to/bitkit/usecases/WipeWalletUseCase.kt Extracted wallet wipe logic into dedicated use case
app/src/main/java/to/bitkit/services/core/Bip39Service.kt New service wrapping BIP39 operations from bitkit-core
app/src/main/java/to/bitkit/repositories/BackupRepo.kt Enhanced with payload models and improved thread safety
app/src/main/java/to/bitkit/repositories/ActivityRepo.kt Added state management and new methods for activity tags
app/src/main/java/to/bitkit/ui/onboarding/RestoreWalletScreen.kt Complete refactor from stateful to MVVM pattern
app/src/main/java/to/bitkit/ext/DateTime.kt Added toRelativeTimeString() for human-readable timestamps
app/src/main/res/values/strings.xml Added new string resources for backup statuses

@ovitrif ovitrif enabled auto-merge November 18, 2025 10:26
@ovitrif ovitrif mentioned this pull request Nov 18, 2025
ben-kaufman
ben-kaufman previously approved these changes Nov 18, 2025
Copy link
Contributor

@ben-kaufman ben-kaufman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

tested backup and restore on Android.
iOS will need to be updated to the new format of settings and widgets backup.

@ovitrif
Copy link
Collaborator Author

ovitrif commented Nov 18, 2025

@ben-kaufman Updated PR with (hopefully) last changes:

  • unit tests to "lock" wipe order by providing a failing test if future updates break the current setup in 1a47b85
  • fix & unit tests fix for Codex review comment in 907a54d

Please do one final re-review of the changes so we can merge it & move on 🙏🏻

@synonymdev synonymdev deleted a comment from claude bot Nov 18, 2025
@synonymdev synonymdev deleted a comment from claude bot Nov 18, 2025
@synonymdev synonymdev deleted a comment from claude bot Nov 18, 2025
@jvsena42
Copy link
Member

Starting review

@jvsena42
Copy link
Member

Note: Once finished, we must remember test creating a wallet on RN app and restoring it on native

@ovitrif
Copy link
Collaborator Author

ovitrif commented Nov 18, 2025

Note: Once finished, we must remember test creating a wallet on RN app and restoring it on native

We have a dedicated work item on the roadmap specifically for this and any additional "Migration Strategy" related work.

Support for it was out of scope in this PR.

@ben-kaufman
Copy link
Contributor

Small issue, seems like on wipe wallet widgets aren't getting cleared @ovitrif

@ovitrif ovitrif disabled auto-merge November 19, 2025 08:37
@ovitrif
Copy link
Collaborator Author

ovitrif commented Nov 19, 2025

Small issue, seems like on wipe wallet widgets aren't getting cleared @ovitrif

Thank you for this! Fixed in c6ed583 fix: clear widgets data on wipe

@claude
Copy link

claude bot commented Nov 19, 2025

Code Review: PR #461 - Backup & Restore System Polish

Summary

This is a substantial and well-architected PR that delivers significant improvements to the backup/restore system, wallet wipe functionality, restore screen UX, and activity management. The changes are thoughtfully designed with good separation of concerns, proper MVVM architecture, and comprehensive test coverage for critical paths.

Overall Assessment: ✅ Approved with minor suggestions


🎯 Strengths

1. Excellent Architecture & Design Patterns

  • WipeWalletUseCase (app/src/main/java/to/bitkit/usecases/WipeWalletUseCase.kt:19): Clean use case pattern with proper dependency injection and error handling
  • RestoreWalletViewModel (app/src/main/java/to/bitkit/viewmodels/RestoreWalletViewModel.kt:18): Well-implemented MVVM with reactive state management using StateFlow
  • Bip39Service (app/src/main/java/to/bitkit/services/core/Bip39Service.kt:8): Good abstraction of BIP39 operations using bitkit-core APIs
  • ActivityListViewModel (app/src/main/java/to/bitkit/viewmodels/ActivityListViewModel.kt:31): Clean filter state management with reactive flows

2. Thread Safety & Concurrency

  • BackupRepo (app/src/main/java/to/bitkit/repositories/BackupRepo.kt:89): Excellent use of ConcurrentHashMap.newKeySet for tracking running backups
  • Proper coroutine scoping throughout with appropriate dispatchers
  • Race condition fixes: Setting wiping/restoring flags to prevent concurrent operations

3. Comprehensive Testing

  • WipeWalletUseCaseTest (app/src/test/java/to/bitkit/usecases/WipeWalletUseCaseTest.kt:63): Excellent test validating operation order using inOrder verifier
  • RestoreWalletViewModelTest: 713 lines of comprehensive unit tests covering edge cases
  • ✅ Tests for error conditions, state management, and paste functionality

4. User Experience Improvements

  • Relative time display (app/src/main/java/to/bitkit/ext/DateTime.kt:62): Professional implementation using ICU RelativeDateTimeFormatter
  • Enhanced paste UX: Support for space, tab, and newline separators (RestoreWalletViewModel.kt:100)
  • Smart validation timing: No errors shown on focused inputs (RestoreWalletViewModel.kt:175)
  • Keyboard navigation: Backspace navigation to previous field

5. Code Quality

  • ✅ Removed unused utility code (Bip39Utils.kt deleted, logic moved to core)
  • ✅ Good use of Kotlin idioms (data classes, sealed classes, extension functions)
  • ✅ Proper error handling with Result types
  • ✅ Well-documented state machine in BackupRepo (lines 54-66)

⚠️ Issues & Recommendations

🔴 High Priority

1. Sensitive Data in Logs 🔐

Location: app/src/main/java/to/bitkit/repositories/BackupRepo.kt:494

Even with cachedRates removed, the cache may contain sensitive data like onchainAddress, backupStatuses, or other user metadata when logging.

Recommendation:

  • Sanitize all potentially sensitive fields before logging
  • Consider using a debug flag to enable detailed logging only in development builds
  • Review all logging statements to ensure no sensitive data leakage

2. Logger.reset() Called Too Late in Wipe Flow

Location: app/src/main/java/to/bitkit/usecases/WipeWalletUseCase.kt:57

Logger.reset() is called after the onSuccess callback and only if lightningRepo.wipeStorage succeeds. If it fails, logs are not reset.

Recommendation: Move Logger.reset() before the user callback and ensure it runs even on failure if needed.


🟡 Medium Priority

3. Backup Payload Models Missing Versioning

Location: app/src/main/java/to/bitkit/models/BackupPayloads.kt:20

The payload models (SettingsBackupV1, WidgetsBackupV1, etc.) have version in their name but no version field in the payload itself for runtime checking.

Recommendation: Add explicit version field for forward compatibility.


4. TODO Comments Should Be Tracked

Locations:

  • app/src/main/java/to/bitkit/repositories/BackupRepo.kt:232
  • app/src/main/java/to/bitkit/repositories/BackupRepo.kt:439
  • app/src/main/java/to/bitkit/repositories/BackupRepo.kt:496

Multiple TODO comments about PreActivityMetadata usage.

Recommendation: Create GitHub issues to track these TODOs rather than leaving them in code comments indefinitely.


5. Magic Number in DateTime Extension

Location: app/src/main/java/to/bitkit/ext/DateTime.kt:205

Using 30 days per month is an approximation that may cause inaccurate relative time strings.

Recommendation: Document this approximation or consider using calendar-aware calculations for month boundaries.


6. Paste Handler Regex Could Be More Specific

Location: app/src/main/java/to/bitkit/viewmodels/RestoreWalletViewModel.kt:100

While the current whitespace regex works well, consider adding validation to reject obviously invalid input early.

Recommendation: Current implementation is acceptable as it catches invalid words in validation phase.


🟢 Low Priority / Nitpicks

7. Inconsistent Companion Object Declaration

Location: app/src/main/java/to/bitkit/usecases/WipeWalletUseCase.kt:67

The explicit Companion name is redundant (though not harmful).

Nitpick: Can be simplified by removing the explicit name.


8. Consider Extract Method for Backup Status Updates

Location: app/src/main/java/to/bitkit/repositories/BackupRepo.kt:569

The pattern of updating backup status appears multiple times. Consider extracting to a helper method for DRY principle.


9. Build Config Locales String Could Be JSON Array

Location: app/build.gradle.kts:56

Consider using a JSON array format for easier parsing if needed in the future.


🔒 Security Review

✅ Good Security Practices:

  1. PIN Removal: Properly strips PIN before backup
  2. Force Address Rotation: Clears onchainAddress on restore
  3. Mnemonic Validation: Proper BIP39 checksum validation before allowing restore
  4. Safe Wipe Order: Critical data (keychain) wiped before database

⚠️ Security Concerns:

  1. Logging: As mentioned above, ensure no sensitive data in logs
  2. Error Messages: Ensure error messages don't leak sensitive information (current implementation looks good)

📊 Performance Considerations

✅ Good Performance Patterns:

  1. Debounced Search: 300ms debounce on search input (ActivityListViewModel.kt:88)
  2. Flow Optimization: Using distinctUntilChanged() and drop(1) to prevent unnecessary emissions
  3. Scoped State: Proper use of StateFlow with stateIn() for UI state management
  4. Background Dispatchers: Appropriate use of IoDispatcher and BgDispatcher

💡 Performance Suggestions:

  1. BackupRepo.kt:98: The TODO about syncing only on specific events is a good optimization to pursue
  2. Consider adding pagination for very large activity lists if not already implemented

🧪 Test Coverage

Excellent test coverage for critical paths:

WipeWalletUseCaseTest: 5 comprehensive tests covering:

  • Order verification
  • Parameter passing
  • Failure handling
  • Cleanup on failure

RestoreWalletViewModelTest: Extensive coverage including:

  • Initial state
  • Word input handling
  • Paste functionality (multiple separators)
  • Focus management
  • Validation logic
  • Checksum error display

Missing Test Coverage:

  • ⚠️ Integration tests for full backup/restore flow
  • ⚠️ Tests for BackupRepo state machine transitions
  • ⚠️ Tests for relative time formatting edge cases

📝 Code Style & Conventions

✅ Follows Kotlin conventions and detekt rules
✅ Good use of suppression annotations where justified
✅ Proper documentation in critical areas (BackupRepo state machine)
✅ Consistent naming conventions


🎨 UX/UI Review

Based on the code changes:

Excellent UX improvements:

  • Relative time display ("2 hours ago") is more user-friendly than timestamps
  • Keyboard dismissal on valid paste improves flow
  • Backspace navigation feels natural
  • Bold text on focused input provides clear visual feedback
  • Validation only on unfocused fields reduces user frustration

🚀 Migration & Compatibility

Good migration strategy:

  • Payload models allow for versioned backups
  • Preserved backup timestamps ensure continuity
  • Address rotation on restore maintains privacy

⚠️ Consider:

  • Migration path from old backup format (if applicable)
  • Backward compatibility if users downgrade

Conclusion

This is high-quality work with thoughtful architecture, good testing, and significant UX improvements. The main concerns are around logging sensitive data and ensuring the wipe flow is fully resilient to failures.

Action Items:

  1. 🔴 High: Audit and sanitize all logging statements for sensitive data
  2. 🔴 High: Move Logger.reset() before user callback in wipe flow
  3. 🟡 Medium: Add version fields to payload models
  4. 🟡 Medium: Create GitHub issues for TODO comments
  5. 🟢 Low: Minor code cleanup opportunities

Recommendation: Merge after addressing high-priority items.


Great work on this PR! The backup & restore system is significantly more robust and user-friendly. 🎉

Generated with Claude Code

@synonymdev synonymdev deleted a comment from claude bot Nov 19, 2025
Copy link
Member

@jvsena42 jvsena42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested:

  1. Perform unchain and LN transactions
  2. Set tags
  3. Wipe app
  4. Set mnemonic
  5. Check if the data was recovered

@ovitrif ovitrif merged commit 912cdd2 into master Nov 19, 2025
35 of 37 checks passed
@ovitrif ovitrif deleted the feat/backup-polish branch November 19, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants