feat(dashpay): add historical metadata backend #1408
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces new and refactored transaction metadata management features, including enhanced tracking, merging, and publishing of metadata changes with progress reporting. UI components are updated for improved theming and layout. The build system is updated for new dependencies. Several LiveData-based flows are migrated to Kotlin Flow, and new resource files are added. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant ViewModel
participant PlatformSyncService
participant MetadataDAO
UI->>ViewModel: Trigger publish past transaction metadata
ViewModel->>PlatformSyncService: publishPastTxMetadata(progressListener)
PlatformSyncService->>MetadataDAO: Get unsaved transactions
MetadataDAO-->>PlatformSyncService: Return unsaved transactions
PlatformSyncService->>MetadataDAO: Merge and publish metadata changes
MetadataDAO-->>PlatformSyncService: Confirm metadata saved
PlatformSyncService-->>ViewModel: Return TxMetadataSaveInfo (itemsSaved, itemsToSave)
ViewModel-->>UI: Update progress and completion status
sequenceDiagram
participant UI
participant ViewModel
UI->>ViewModel: Observe publish operation status (Flow)
ViewModel-->>UI: Emit Resource<WorkInfo> updates via Flow
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
HashEngineering
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataWorker.kt (1)
49-87: Ensure transaction metadata remains encrypted before publishingRemoving all password handling and key derivation here is a significant security model change that exposes user metadata (amounts, timestamps, notes, etc.) to linkage, interception, and profiling attacks. Before merging, please confirm you’ve addressed the following:
- Encrypt metadata locally (e.g. AES-256) in
PublishTransactionMetadataWorker.kt(around the call topublishPastTxMetadata()) or withinplatformSynchronizationService.- Securely derive and store encryption keys (using PBKDF2/Argon2 or a hardware-backed keystore), not hard-coded values.
- Limit metadata fields to only what’s strictly necessary.
- Provide users control over which metadata is shared.
- Ensure end-to-end encryption from device to server.
- Document your key management approach and reference any encryption libraries used.
Without these safeguards in place, unencrypted metadata publishing can violate privacy requirements and may run afoul of compliance regulations.
🧹 Nitpick comments (2)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (2)
1143-1182: Consider adding error handling for wallet access.The method correctly handles past transaction metadata publishing with progress reporting. However, it might benefit from null-safety when accessing the wallet.
Consider adding explicit null handling:
- val txes = walletApplication.wallet?.getTransactions(true) + val txes = walletApplication.wallet?.getTransactions(true) ?: return TxMetadataSaveInfo.NONE
1066-1070: Simplify progress callback invocation.The progress callback is already running on the IO dispatcher context, so launching another coroutine is unnecessary.
Simplify the callback invocation:
- ) { progress -> - syncScope.launch(Dispatchers.IO) { - progressListener?.invoke(10 + progress * 90 / 100) - } - } + ) { progress -> + progressListener?.invoke(10 + progress * 90 / 100) + }Note: If the callback truly needs to be on a different coroutine for some reason, consider adding a comment explaining why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
build.gradle(2 hunks)common/src/main/java/org/dash/wallet/common/services/TransactionMetadataProvider.kt(1 hunks)common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt(3 hunks)common/src/main/java/org/dash/wallet/common/ui/components/DashCheckBox.kt(5 hunks)common/src/main/java/org/dash/wallet/common/ui/components/DashRadioButton.kt(9 hunks)common/src/main/java/org/dash/wallet/common/ui/components/MyTheme.kt(1 hunks)common/src/main/java/org/dash/wallet/common/ui/radio_group/RadioGroupAdapter.kt(0 hunks)common/src/main/res/drawable/ic_preview.xml(1 hunks)common/src/main/res/drawable/ic_radio_button_circle.xml(1 hunks)wallet/src/de/schildbach/wallet/Constants.java(1 hunks)wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt(2 hunks)wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataDocumentDao.kt(1 hunks)wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataCacheItem.kt(1 hunks)wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataDocument.kt(1 hunks)wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt(2 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(12 hunks)wallet/src/de/schildbach/wallet/service/platform/TxMetadataSaveInfo.kt(1 hunks)wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataOperation.kt(2 hunks)wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataWorker.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/more/SaveMetadataAndResetDialogFragment.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/more/SecurityViewModel.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/more/SettingsFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsFragment.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsScreen.kt(6 hunks)wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsViewModel.kt(5 hunks)
💤 Files with no reviewable changes (1)
- common/src/main/java/org/dash/wallet/common/ui/radio_group/RadioGroupAdapter.kt
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt (1)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/service/platform/TxMetadataSaveInfo.kt (1)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsFragment.kt (3)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
common/src/main/java/org/dash/wallet/common/services/TransactionMetadataProvider.kt (1)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/Constants.java (2)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1370
File: fastlane/Fastfile:176-176
Timestamp: 2025-04-04T07:11:07.664Z
Learning: The explore feature in the dash-wallet application uses different naming conventions for different types of databases: preloaded and test databases still use the original naming convention (e.g., "explore.db"), while production databases use the v2 naming scheme (e.g., "explore-v2.db"). This inconsistency is intentional.
wallet/src/de/schildbach/wallet/ui/more/SettingsFragment.kt (3)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt (1)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/ui/more/SecurityViewModel.kt (5)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt (2)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
wallet/src/de/schildbach/wallet/ui/more/SaveMetadataAndResetDialogFragment.kt (7)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: common/src/main/java/org/dash/wallet/common/ui/segmented_picker/SegmentedPicker.kt:70-83
Timestamp: 2025-05-12T09:17:22.548Z
Learning: The SegmentedPicker component in the Dash Wallet app intentionally uses a mixed state pattern (both internal state and external parameter) to handle animations internally while still respecting external state changes. This design allows the component to manage animation timing independently without requiring the parent component to handle animation logic.
common/src/main/java/org/dash/wallet/common/ui/components/DashCheckBox.kt (2)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataOperation.kt (5)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
wallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.kt (1)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsScreen.kt (5)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataCacheItem.kt (1)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataWorker.kt (4)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsViewModel.kt (5)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
common/src/main/java/org/dash/wallet/common/ui/components/DashRadioButton.kt (3)
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt (4)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/ExploreDataSource.kt:106-115
Timestamp: 2025-05-07T14:17:15.912Z
Learning: In the Dash Wallet app, MerchantDao interface methods use an "excludeType" parameter (e.g., `MerchantType.ONLINE`) to exclude merchants of a specific type rather than to include them. For example, in `observePhysicalMerchants()`, passing `MerchantType.ONLINE` as the exclude type parameter filters out online-only merchants, effectively returning physical merchants.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (6)
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/model/Merchant.kt:52-52
Timestamp: 2025-05-07T14:19:08.146Z
Learning: In the dash-wallet project, database migrations must account for pre-populated databases. When new columns are added to model classes, empty migrations with incremented version numbers are used instead of ALTER TABLE statements, as the downloaded database will already contain the new columns. This prevents SQL errors when trying to add columns that already exist.
🧬 Code Graph Analysis (3)
wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataCacheItem.kt (1)
common/src/main/java/org/dash/wallet/common/data/entity/TransactionMetadata.kt (1)
isNotEmpty(49-52)
wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataWorker.kt (1)
wallet/src/de/schildbach/wallet/service/work/BaseWorker.kt (1)
setProgress(31-33)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt (1)
min(61-63)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (39)
common/src/main/java/org/dash/wallet/common/ui/components/MyTheme.kt (1)
31-31: LGTM! Good practice to expose theme resources.Changing the visibility of
interRegularfrom private to public enables consistent font usage across UI components. This follows standard theming patterns where core design tokens are made accessible to other components.common/src/main/res/drawable/ic_radio_button_circle.xml (1)
1-11: LGTM! Well-defined radio button icon.The vector drawable properly defines a circular radio button icon with appropriate dimensions (22dp) and uses the consistent blue color from the theme. The thick stroke (5dp) with transparent fill provides good visual contrast for UI interactions.
wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataDocument.kt (1)
24-24: LGTM! Data class conversion enhances functionality.Converting to a data class provides automatic
equals(),hashCode(),toString(), andcopy()implementations, which are beneficial for database entities that require comparison and merging operations. This change aligns well with the enhanced transaction metadata management features.build.gradle (2)
7-7: LGTM! Version bump aligns with PR objectives.The version update from
1.7.6-SNAPSHOTto2.0.0-TX-SNAPSHOTappropriately reflects the major enhancements in transaction metadata functionality introduced in this PR. The "TX-SNAPSHOT" suffix clearly indicates transaction-related improvements.
79-79: Snapshot repository verified as legitimate and required
- The repository URL follows Sonatype’s standard snapshot pattern (
central.sonatype.com/repository/maven-snapshots/).build.gradledefinesdppVersion = "2.0.0-TX-SNAPSHOT", so resolving snapshots is necessary.No further changes required.
common/src/main/res/drawable/ic_preview.xml (1)
1-11: LGTM! Well-structured vector drawable.The icon follows standard Android vector drawable conventions with appropriate dimensions, viewport, and styling for a preview icon.
common/src/main/java/org/dash/wallet/common/services/TransactionMetadataProvider.kt (1)
121-124: LGTM! Clean interface addition.The new
existsmethod is well-documented and appropriately designed as a suspend function for checking transaction metadata existence.wallet/src/de/schildbach/wallet/Constants.java (1)
100-100: Confirm production readiness before enabling transaction metadataEnabling
SUPPORTS_TXMETADATA = trueturns on metadata support in production. Before merging this flag change:• Ensure end-to-end and regression tests covering metadata transactions are in place and green in CI
• Verify that peers lacking metadata support are handled gracefully (fallback paths, error handling)
• Consider rolling out via remote-config or a staged release to limit blast radiuswallet/src/de/schildbach/wallet/ui/more/SettingsFragment.kt (2)
212-212: LGTM! Proper migration from LiveData to Flow.The change from
publishOperationLiveData(workId)toobservePublishOperation(workId)correctly migrates to Flow-based observation, which aligns with modern reactive programming practices in Android.
232-232: Good practice: Explicit UI state initialization.Explicitly calling
setTransactionMetadataText(isSaving = false, saveProgress = -1)ensures proper initial UI state, preventing potential undefined state issues.wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataDocumentDao.kt (1)
178-179: LGTM! Straightforward DAO method addition.The new
getTransactionMetadatamethod correctly queries all metadata documents for a transaction ID and follows Room DAO conventions.wallet/src/de/schildbach/wallet/service/platform/TxMetadataSaveInfo.kt (1)
20-27: LGTM! Well-designed data class.The implementation follows Kotlin best practices with immutable properties and a useful companion object constant. The class serves its purpose well as a standardized result object for transaction metadata save operations.
wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsFragment.kt (3)
68-72: Clean date handling logic.The implementation properly handles both scenarios - using the last save date if available, otherwise falling back to the first unsaved transaction date. This provides a good user experience by showing relevant temporal context.
80-81: Good improvement using dynamic transaction count.Replacing the hardcoded placeholder with
viewModel.unsavedTxCountprovides accurate information to users about how many transactions would be affected.
73-73: Dialog visibility logic is correctThe negated OR (
!(settings.saveToNetwork || settings.savePastTxToNetwork)) ensures the save-metadata prompt only appears when neither option is enabled. In the preview (both flags true) the dialog is correctly suppressed, matching the intended behavior. No changes required.wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt (1)
591-593: LGTM! Clean implementation of exists method.The method properly delegates to the DAO and follows the established pattern used throughout the class. This supports the new interface requirement for existence checks.
wallet/src/de/schildbach/wallet/ui/more/SaveMetadataAndResetDialogFragment.kt (4)
29-29: LGTM! Proper imports for Flow migration.The new imports support the migration from LiveData to Flow-based observation, which aligns with the broader modernization effort in the codebase.
Also applies to: 38-38
84-84: Good migration from LiveData to Flow.The change from
publishOperationLiveData(workId)toobservePublishOperation(workId).observe(viewLifecycleOwner)properly adapts to the new Flow-based API while maintaining lifecycle-aware observation. This follows the pattern established in the retrieved learnings about migrating to Flow in this codebase.
85-85: Clean up unnecessary null check in logging.Removing the null check (
?.toString() ?: "") simplifies the logging since Flow-basedworkInfois likely non-nullable in this context.
166-166: Interface signature updated correctly.The preview implementation properly reflects the new Flow-based method signature, maintaining consistency with the actual implementation.
common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt (3)
56-62: LGTM! Excellent theming consistency improvements.The migration from hardcoded colors to
MyTheme.Colorsreferences centralizes color management and improves maintainability. The comprehensive updates across different button styles ensure consistent theming throughout the component.Also applies to: 66-79
135-135: Font family update aligns with theme refactoring.The change from
MyTheme.InterFonttoMyTheme.interRegularsuggests the font system has been refactored to make specific font variants publicly accessible, which improves the theming architecture.
156-156: New Plain style added appropriately.The addition of the
Plainstyle to the enum expands the available button variants while maintaining the existing order and structure.wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataOperation.kt (1)
69-89: LGTM! Clean Flow migration maintaining existing behavior.The new
operationStatusFlowmethod correctly mirrors the existing LiveData-based approach while leveraging modern Flow APIs. The error handling, validation logic, and filtering are consistent with the original implementation.common/src/main/java/org/dash/wallet/common/ui/components/DashCheckBox.kt (2)
41-42: LGTM! Improved component sizing and layout.Setting a minimum height ensures consistent component sizing across different content configurations, and removing the clipping allows for better visual presentation.
109-110: Good alignment improvement for trailing text.Adding horizontal end alignment ensures trailing text is properly positioned, improving the visual consistency of the component.
wallet/src/de/schildbach/wallet/ui/more/SecurityViewModel.kt (2)
63-63: Good dependency injection for centralized service.Adding
PlatformSyncServiceas a constructor parameter enables access to centralized transaction metadata management logic.
154-154: Excellent refactoring to centralize business logic.Delegating to
platformSyncService.hasPendingTxMetadataToSave()centralizes the logic for determining pending metadata, improving maintainability and ensuring consistent behavior across the application.wallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.kt (1)
52-56: Enhanced flexibility for metadata saving control.The addition of the
saveAllparameter provides better control over metadata saving by allowing external callers to override individual save flags when needed. This maintains backward compatibility while enabling more flexible bulk operations.wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsScreen.kt (3)
72-74: Clean Flow migration with performance optimization.The migration from LiveData to Flow using
collectAsState()is correctly implemented. Caching the current date and extracting theisSavingflag improves performance by avoiding repeated calculations during recomposition.
159-166: Improved UI state management with cached values.Using the cached
currentDateandisSavingflag provides better performance and cleaner state management for the subtitle text and checkbox enabled state.
284-284: Minor UI refinement for better spacing.The padding adjustment improves the visual layout of the card section content.
wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataCacheItem.kt (1)
72-77: SentTimestamp is intentionally excluded from isNotEmpty()
ThesentTimestampfield is used only for caching/timestamping, not as part of the metadata payload.
• Thecompare()method deliberately ignoressentTimestampwhen checking for metadata changes.
• The diff operator (minus) tracks timestamp changes but downstream publishing filters only non‐metadata fields viaisNotEmpty().
No code relies onsentTimestamp != nullto determine non‐emptiness—dropping it fromisNotEmpty()is correct.Likely an incorrect or invalid review comment.
wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt (1)
159-225: Well-implemented DAO enhancementsThe new methods are properly implemented:
observeCachedItemsBeforecorrectly provides reactive observation using Flow- The
hasmethod uses proper SQLIScomparisons for nullable fields in the EXISTS query- The overloaded convenience method effectively delegates to the detailed implementation
common/src/main/java/org/dash/wallet/common/ui/components/DashRadioButton.kt (1)
33-316: Well-structured UI component enhancementsThe refactoring improves the flexibility and usability of the radio button components:
- Support for trailing text/help text enables richer option displays
- The new
DashRadioGroupOptiondata class provides a clean API for complex options- The overloaded
RadioGroupcomposable maintains backward compatibility while adding new functionality- The visual style updates align with modern UI patterns
wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsViewModel.kt (1)
166-199: Well-implemented Flow migration and transaction trackingThe migration from LiveData to Flow is properly executed:
- The combined Flow logic correctly merges multiple data sources
- The new unsaved transaction tracking provides better visibility into pending changes
- The asynchronous fetching of unsaved transactions is appropriately dispatched to IO
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (3)
250-266: Good defensive check added.The addition of
platformRepo.hasIdentity()check for the txMetadataJob shutdown is a good defensive programming practice to ensure we only shut down the job when an identity exists.
1100-1141: Well-implemented unsaved transaction detection.The implementation correctly:
- Filters out CoinJoin transactions (except None and Send types)
- Checks for both missing metadata and differences in existing metadata
- Tracks the earliest unsaved transaction timestamp
- Includes performance logging for monitoring
1184-1285: Excellent refactoring of the publish logic.The updated method properly:
- Supports selective saving based on user preferences or forced save-all mode
- Provides progress feedback for UI updates
- Returns save statistics via
TxMetadataSaveInfo- Updates local transaction metadata after successful publishing
| true, false -> { | ||
| // TODO: Disable Reset Wallet for now | ||
| // doReset() | ||
| doReset() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that wallet reset reactivation is intentional and thoroughly tested.
The doReset() calls have been uncommented, reactivating wallet reset functionality that was previously disabled. This is a critical security operation that affects user data.
Please confirm:
- The issues that caused the reset functionality to be disabled have been resolved
- The reset functionality has been thoroughly tested with the new transaction metadata management
- The reactivation aligns with the improved platform synchronization mentioned in the PR
Also applies to: 216-216
🏁 Script executed:
#!/bin/bash
# Search for any remaining TODOs or FIXMEs related to reset functionality
echo "Searching for reset-related TODOs or issues..."
rg -i "todo.*reset|fixme.*reset|hack.*reset" --type kotlin
echo -e "\nSearching for other disabled reset functionality..."
rg -A 3 -B 3 "doReset.*//|//.*doReset" --type kotlinLength of output: 748
Action Required: Remove stale TODOs and validate wallet reset functionality
• SecurityFragment.kt (around lines 210 & 216):
- Remove or update the lingering
// TODO: Disable Reset Wallet for now
comments now thatdoReset()has been reactivated.
• CreateIdentityService.kt (dashpay, around the blockchain reset logic):
- Address or resolve the
// TODO: handle case when blockchain reset has happened and the cftx was not found yet
to ensure consistency after a wallet reset.
• Testing & Verification:
- Ensure comprehensive unit and integration tests cover the
doReset()flow under the new transaction‐metadata management. - Confirm end-to-end scenarios (e.g., app restart, data sync) behave correctly post‐reset.
Please remove these outdated TODOs, handle any remaining edge cases, and verify that the reset functionality is fully covered by tests.
🤖 Prompt for AI Agents
In wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt at line 210,
remove the outdated TODO comment "// TODO: Disable Reset Wallet for now" since
doReset() is now active. Additionally, in CreateIdentityService.kt near the
blockchain reset logic, implement handling for the case when the blockchain
reset occurs but the cftx is not yet found, resolving the related TODO. Finally,
ensure thorough unit and integration tests cover the doReset() process and
verify that end-to-end scenarios like app restart and data synchronization
function correctly after a wallet reset.
| private suspend fun shouldSaveToCache(): Boolean { | ||
| val currentTime = System.currentTimeMillis() | ||
| val shouldSaveToCache = dashPayConfig.isSavingToNetwork() && currentTime > dashPayConfig.getSaveAfterTimestamp() | ||
| val shouldSaveToCache = true; //dashPayConfig.isSavingToNetwork() && currentTime > dashPayConfig.getSaveAfterTimestamp() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review hardcoded return value in shouldSaveToCache().
The method now always returns true with the original conditional logic commented out. This appears to be a temporary change that might have been made for testing or development purposes.
Ensure this change is intentional for production. If it's temporary, consider using a feature flag or configuration parameter instead of hardcoding.
🏁 Script executed:
#!/bin/bash
# Search for other references to shouldSaveToCache to understand the impact
rg -A 3 -B 3 "shouldSaveToCache"Length of output: 7411
Hardcoded return in shouldSaveToCache(): restore configurable behavior
The helper now always returns true, bypassing both the dashPayConfig.isSavingToNetwork() toggle and the timestamp check. This forces every metadata change to hit the cache regardless of production settings.
• Location:
- wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt
– private suspend fun shouldSaveToCache() (around line 186)
• Recommendation:
- Re-enable the original logic using
dashPayConfig.isSavingToNetwork()andcurrentTime > dashPayConfig.getSaveAfterTimestamp(). - If you need a temporary override for testing, wrap this in a feature flag or configuration parameter rather than hardcoding
true.
🤖 Prompt for AI Agents
In wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt
at line 186, the variable shouldSaveToCache is hardcoded to true, bypassing the
original logic that checks dashPayConfig.isSavingToNetwork() and compares
currentTime to dashPayConfig.getSaveAfterTimestamp(). Restore the original
conditional expression combining these two checks to re-enable configurable
behavior. If a temporary override is needed, implement it using a feature flag
or configuration parameter instead of hardcoding true.
| fun compare(currentItem: TransactionMetadata, giftCard: GiftCard?): Boolean { | ||
| val txData = txId == currentItem.txId && | ||
| (this.memo ?: "") == currentItem.memo && | ||
| this.taxCategory == currentItem.taxCategory && | ||
| this.service == currentItem.service && | ||
| this.currencyCode == currentItem.currencyCode && | ||
| this.rate == currentItem.rate | ||
| val giftCardEquals = giftCard?.let { | ||
| this.giftCardNumber == giftCard.number && | ||
| this.giftCardPin == giftCard.pin && | ||
| this.barcodeValue == giftCard.barcodeValue && | ||
| this.barcodeFormat == giftCard.barcodeFormat.toString() && | ||
| this.merchantName == giftCard.merchantName && | ||
| this.merchantUrl == giftCard.merchantUrl && | ||
| this.originalPrice == giftCard.price | ||
| } | ||
|
|
||
| return txData && if (giftCard != null) giftCardEquals!! else true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid potential NPE with the !! operator
The use of !! on line 123 can throw a NullPointerException if giftCardEquals is somehow null, even though the logic suggests it shouldn't be. Consider using a safer approach.
Apply this diff for a safer implementation:
- return txData && if (giftCard != null) giftCardEquals!! else true
+ return txData && (giftCard == null || giftCardEquals == true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun compare(currentItem: TransactionMetadata, giftCard: GiftCard?): Boolean { | |
| val txData = txId == currentItem.txId && | |
| (this.memo ?: "") == currentItem.memo && | |
| this.taxCategory == currentItem.taxCategory && | |
| this.service == currentItem.service && | |
| this.currencyCode == currentItem.currencyCode && | |
| this.rate == currentItem.rate | |
| val giftCardEquals = giftCard?.let { | |
| this.giftCardNumber == giftCard.number && | |
| this.giftCardPin == giftCard.pin && | |
| this.barcodeValue == giftCard.barcodeValue && | |
| this.barcodeFormat == giftCard.barcodeFormat.toString() && | |
| this.merchantName == giftCard.merchantName && | |
| this.merchantUrl == giftCard.merchantUrl && | |
| this.originalPrice == giftCard.price | |
| } | |
| return txData && if (giftCard != null) giftCardEquals!! else true | |
| } | |
| fun compare(currentItem: TransactionMetadata, giftCard: GiftCard?): Boolean { | |
| val txData = txId == currentItem.txId && | |
| (this.memo ?: "") == currentItem.memo && | |
| this.taxCategory == currentItem.taxCategory && | |
| this.service == currentItem.service && | |
| this.currencyCode == currentItem.currencyCode && | |
| this.rate == currentItem.rate | |
| val giftCardEquals = giftCard?.let { | |
| this.giftCardNumber == giftCard.number && | |
| this.giftCardPin == giftCard.pin && | |
| this.barcodeValue == giftCard.barcodeValue && | |
| this.barcodeFormat == giftCard.barcodeFormat.toString() && | |
| this.merchantName == giftCard.merchantName && | |
| this.merchantUrl == giftCard.merchantUrl && | |
| this.originalPrice == giftCard.price | |
| } | |
| return txData && (giftCard == null || giftCardEquals == true) | |
| } |
🤖 Prompt for AI Agents
In
wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataCacheItem.kt
around lines 106 to 124, the use of the !! operator on giftCardEquals can cause
a NullPointerException if giftCardEquals is null. To fix this, avoid using !! by
restructuring the return statement to safely handle the nullable giftCardEquals,
for example by using a safe call with Elvis operator or explicitly checking for
null before returning, ensuring no forced unwrapping occurs.
wallet/src/de/schildbach/wallet/database/entity/TransactionMetadataCacheItem.kt
Show resolved
Hide resolved
| companion object { | ||
| private val log = LoggerFactory.getLogger(PublishTransactionMetadataWorker::class.java) | ||
| const val KEY_PASSWORD = "PublishTransactionMetadataWorker.PASSWORD" | ||
| // const val KEY_IDENTITY = "PublishTransactionMetadataWorker.IDENTITY" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused KEY_PASSWORD constant
The KEY_PASSWORD constant is no longer used after the refactoring that removed password handling. This dead code should be removed.
Apply this diff to remove the unused constant:
companion object {
private val log = LoggerFactory.getLogger(PublishTransactionMetadataWorker::class.java)
- const val KEY_PASSWORD = "PublishTransactionMetadataWorker.PASSWORD"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| companion object { | |
| private val log = LoggerFactory.getLogger(PublishTransactionMetadataWorker::class.java) | |
| const val KEY_PASSWORD = "PublishTransactionMetadataWorker.PASSWORD" | |
| // const val KEY_IDENTITY = "PublishTransactionMetadataWorker.IDENTITY" | |
| } | |
| companion object { | |
| private val log = LoggerFactory.getLogger(PublishTransactionMetadataWorker::class.java) | |
| } |
🤖 Prompt for AI Agents
In
wallet/src/de/schildbach/wallet/service/platform/work/PublishTransactionMetadataWorker.kt
around lines 44 to 47, the constant KEY_PASSWORD is defined but no longer used
after refactoring. Remove the line declaring KEY_PASSWORD from the companion
object to eliminate this dead code.
| private val _lastSaveDate = MutableStateFlow<Long>(-1) | ||
| override val lastSaveDate = _lastSaveDate.asStateFlow() | ||
| private val _futureSaveDate = MutableStateFlow<Long>(-1) | ||
| override val futureSaveDate = _lastSaveDate.asStateFlow() |
There was a problem hiding this comment.
Fix incorrect StateFlow exposure
The futureSaveDate property is incorrectly returning _lastSaveDate instead of _futureSaveDate. This will cause the UI to display wrong date information.
Apply this diff to fix the property:
- override val futureSaveDate = _lastSaveDate.asStateFlow()
+ override val futureSaveDate = _futureSaveDate.asStateFlow()🤖 Prompt for AI Agents
In
wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataSettingsViewModel.kt
at line 119, the futureSaveDate property is incorrectly exposing _lastSaveDate
instead of _futureSaveDate. Change the property to return
_futureSaveDate.asStateFlow() to ensure the UI displays the correct date
information.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| val PUSH_PERIOD = if (true) 3.minutes else 3.hours // revert this | ||
| val WEEKLY_PUSH_PERIOD = 7.days | ||
| val CUTOFF_MIN = if (BuildConfig.DEBUG) 3.minutes else 3.hours | ||
| val CUTOFF_MAX = if (BuildConfig.DEBUG) 6.minutes else 6.hours | ||
| val CUTOFF_MIN = if (true) 3.minutes else 3.hours // revert this | ||
| val CUTOFF_MAX = if (true) 6.minutes else 6.hours // revert this |
There was a problem hiding this comment.
Critical: Revert temporary timing constant changes before merging.
The PUSH_PERIOD and CUTOFF_MIN/CUTOFF_MAX values have been temporarily reduced from 3 hours to 3 minutes (and 6 hours to 6 minutes). These changes should be reverted before merging to production as they will cause excessive platform API calls.
Apply this diff to revert the timing constants:
- val PUSH_PERIOD = if (true) 3.minutes else 3.hours // revert this
+ val PUSH_PERIOD = 3.hours
val WEEKLY_PUSH_PERIOD = 7.days
- val CUTOFF_MIN = if (true) 3.minutes else 3.hours // revert this
- val CUTOFF_MAX = if (true) 6.minutes else 6.hours // revert this
+ val CUTOFF_MIN = 3.hours
+ val CUTOFF_MAX = 6.hours📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val PUSH_PERIOD = if (true) 3.minutes else 3.hours // revert this | |
| val WEEKLY_PUSH_PERIOD = 7.days | |
| val CUTOFF_MIN = if (BuildConfig.DEBUG) 3.minutes else 3.hours | |
| val CUTOFF_MAX = if (BuildConfig.DEBUG) 6.minutes else 6.hours | |
| val CUTOFF_MIN = if (true) 3.minutes else 3.hours // revert this | |
| val CUTOFF_MAX = if (true) 6.minutes else 6.hours // revert this | |
| val PUSH_PERIOD = 3.hours | |
| val WEEKLY_PUSH_PERIOD = 7.days | |
| val CUTOFF_MIN = 3.hours | |
| val CUTOFF_MAX = 6.hours |
🤖 Prompt for AI Agents
In wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt
around lines 154 to 157, revert the temporary timing constant changes by
replacing the conditions that currently always evaluate to true with false, so
that PUSH_PERIOD is set to 3 hours instead of 3 minutes, CUTOFF_MIN to 3 hours
instead of 3 minutes, and CUTOFF_MAX to 6 hours instead of 6 minutes. This will
prevent excessive platform API calls by restoring the original longer intervals.
| private fun mergeTransactionMetadataDocuments(txId: Sha256Hash, docs: List<TransactionMetadataDocument>): TransactionMetadataCacheItem { | ||
| return TransactionMetadataCacheItem( | ||
| cacheTimestamp = docs.lastOrNull()?.timestamp ?: 0, | ||
| txId = txId, | ||
| sentTimestamp = docs.lastOrNull { it.sentTimestamp != null }?.sentTimestamp, | ||
| taxCategory = docs.lastOrNull { it.taxCategory != null }?.taxCategory, | ||
| currencyCode = docs.lastOrNull { it.currencyCode != null }?.currencyCode, | ||
| rate = docs.lastOrNull { it.rate != null }?.rate.toString(), | ||
| memo = docs.lastOrNull { it.memo != null }?.memo, | ||
| service = docs.lastOrNull { it.service != null }?.service, | ||
| customIconUrl = docs.lastOrNull { it.customIconUrl != null }?.customIconUrl, | ||
| giftCardNumber = docs.lastOrNull { it.giftCardNumber != null }?.giftCardNumber, | ||
| giftCardPin = docs.lastOrNull { it.giftCardPin != null }?.giftCardPin, | ||
| merchantName = docs.lastOrNull { it.merchantName != null }?.merchantName, | ||
| originalPrice = docs.lastOrNull { it.originalPrice != null }?.originalPrice, | ||
| barcodeValue = docs.lastOrNull { it.barcodeValue != null }?.barcodeValue, | ||
| barcodeFormat = docs.lastOrNull { it.barcodeFormat != null }?.barcodeFormat, | ||
| merchantUrl = docs.lastOrNull { it.merchantUrl != null }?.service, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix merchantUrl assignment bug.
There's a copy-paste error where merchantUrl is being assigned from service instead of merchantUrl.
Apply this diff to fix the bug:
- merchantUrl = docs.lastOrNull { it.merchantUrl != null }?.service,
+ merchantUrl = docs.lastOrNull { it.merchantUrl != null }?.merchantUrl,🤖 Prompt for AI Agents
In wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt
around lines 1074 to 1093, the merchantUrl field is incorrectly assigned from
the service property due to a copy-paste error. Fix this by changing the
assignment of merchantUrl to use docs.lastOrNull { it.merchantUrl != null
}?.merchantUrl instead of ?.service.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| suspend fun initSync(runFirstUpdateBlocking: Boolean = false) | ||
| fun resume() | ||
| fun shutdown() | ||
| suspend fun shutdown() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all callers are updated for the API changes.
The interface changes include making shutdown() suspend and adding two new methods. Ensure all implementations and callers are updated accordingly.
Run the following script to verify the interface usage:
Also applies to: 124-125
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of PlatformSyncService to ensure they handle the new suspend shutdown() and new methods
# Check for shutdown() calls that might need to be updated to use coroutines
echo "=== Checking shutdown() calls ==="
rg -A 3 "platformSyncService.*shutdown\(\)"
# Check for interface implementations
echo "=== Checking PlatformSyncService implementations ==="
ast-grep --pattern 'class $_ : PlatformSyncService'Length of output: 731
🏁 Script executed:
#!/bin/bash
# Verify Java implementations of PlatformSyncService and any missed callers
echo "=== Checking Java implementations of PlatformSyncService ==="
rg "implements PlatformSyncService" -n
echo "=== AST search for Java impl classes ==="
ast-grep --pattern $'class $_ implements PlatformSyncService'
echo "=== Full grep for any remaining shutdown() calls ==="
rg -n "PlatformSyncService" -C2Length of output: 28846
🏁 Script executed:
#!/bin/bash
# 1. Locate the Kotlin implementation of PlatformSyncService
impl_file=$(rg -l "class PlatformSynchronizationService" -n)
echo "Implementation file: $impl_file"
# 2. Show the first 200 lines of the implementation to verify overrides of shutdown and new methods
echo "=== Implementation contents ==="
sed -n '1,200p' "$impl_file"
# 3. Grep for the shutdown signature in the implementation
echo "=== Overrides of shutdown in implementation ==="
grep -n "shutdown" "$impl_file"
# 4. Inspect the context around the shutdown() call in BlockchainServiceImpl.kt
echo "=== Context around platformSyncService.shutdown() call ==="
rg -n -C5 "platformSyncService.shutdown" wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.ktLength of output: 11116
Fix coroutine usage for suspend shutdown()
The direct call to platformSyncService.shutdown() in a non-suspend context will no longer compile. Wrap it in a coroutine builder. For example, in wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt around line 1305:
- platformSyncService.shutdown()
+ // Wait for shutdown to complete
+ runBlocking(Dispatchers.IO) {
+ platformSyncService.shutdown()
+ }• Review other callers of shutdown() and new suspend methods—ensure they’re invoked from a coroutine or wrapped with runBlocking/CoroutineScope.launch.
• Update any tests that verify shutdown() to use MockK’s coVerify/coEvery for suspend functions.
🤖 Prompt for AI Agents
In wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt at
line 103, the shutdown() function is now suspend, so all calls to
platformSyncService.shutdown() must be made from a coroutine context. To fix
this, wrap calls to shutdown() in a coroutine builder like runBlocking or
CoroutineScope.launch where they are called outside suspend functions. Also,
update any tests that mock or verify shutdown() to use MockK's coVerify and
coEvery for suspend functions to handle coroutine behavior correctly.
There was a problem hiding this comment.
platformSyncService.shutdown() runs in a coroutine (syncScope)
There was a problem hiding this comment.
@HashEngineering You're absolutely right! Looking at the implementation in BlockchainServiceImpl.kt, I can see that the call to platformSyncService.shutdown() is already properly wrapped in a runBlocking block:
runBlocking {
platformSyncService.shutdown()
}This provides the necessary coroutine context for the suspend function. My previous review comment was inaccurate - the implementation already correctly handles the suspend nature of the shutdown() method. Thank you for the clarification!
🧠 Learnings used
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1399
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:298-303
Timestamp: 2025-06-09T04:15:53.515Z
Learning: In Kotlin suspend functions, when a suspend function calls another suspend function directly, no additional coroutine launching is required. The suspend context is maintained throughout the call chain. Only non-suspend functions need to launch coroutines to call suspend functions.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
There was a problem hiding this comment.
@HashEngineering some issues found by coderabbit need checking
Issue being fixed or feature implemented
Related PR's and Dependencies
dashpay/kotlin-platform#22
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores