feat: finalize txmetadata and align with specification #1436
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 WalkthroughThe pull request restructures Settings UI by migrating SettingsFragment from Changes
Sequence DiagramsequenceDiagram
participant UI as SettingsFragment
participant VM as TransactionMetadataSettingsViewModel
participant DB as TransactionMetadataDao
participant Platform as PlatformSyncService
UI->>VM: setupTransactionMetadataObservers()
VM->>VM: Load last work IDs
VM->>DB: Query last saved timestamp
Platform->>Platform: Trigger metadata publishing
Platform->>DB: Update transaction metadata
Platform->>VM: Emit publish result
VM->>UI: Update subtitle LiveData
UI->>UI: setTransactionMetadataText(isSaving, progress)
UI->>UI: Refresh subtitle display (date or progress)
alt User taps transaction metadata
UI->>VM: Check visibility state
alt Info already shown
UI->>UI: Navigate to settings fragment
else First time
UI->>UI: Show info dialog
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple files with a mix of architectural refactoring (file migration), new UI integration logic (observers, subtitle updates, navigation), hash handling updates in the sync service, and UI state management extensions. While individual cohorts are straightforward, the integration across Settings components and the hash reversal logic require careful review for consistency and correctness. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai please review |
|
@HashEngineering: I'll review the changes in this PR. 🧠 Learnings used✅ 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: 2
🧹 Nitpick comments (8)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (2)
1034-1061: Ensure reversed bytes match platform's expected hash format.When publishing metadata to platform, the code now uses
it.txId.reversedBytes(line 1045). This should be consistent with:
- How platform expects to receive transaction hashes
- The reversal applied when receiving hashes (line 808)
If your internal storage uses little-endian and platform expects big-endian, this is correct. However, this must align with the platform specification and be verified against the changes at line 808.
Consider adding documentation or constants to clarify the byte order convention:
// At the top of the file or in a companion object /** * Platform stores tx hashes in big-endian (display format). * Internal storage uses little-endian (Sha256Hash native format). * Use wrapReversed when receiving from platform. * Use reversedBytes when sending to platform. */
1264-1264: Good refactor: Usevalfor immutable variable.Correctly changed from
vartovalsinceitemsToSaveis never reassigned after initialization.wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataDialog.kt (1)
50-68: Scope and opt-in hygiene
- onViewCreated doesn’t use experimental coroutines APIs; drop @OptIn here to reduce opt-in spread.
- Prefer viewLifecycleOwner.lifecycleScope for view-tied work.
Apply:
- @OptIn(ExperimentalCoroutinesApi::class) override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) ... - lifecycleScope.launch { + viewLifecycleOwner.lifecycleScope.launch { viewModel.setTransactionMetadataInfoShown() }wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt (3)
90-92: Use lifecycle-aware state collection in ComposePrefer collectAsStateWithLifecycle to avoid collecting when the UI is not started/resumed.
- val uiState by uiStateFlow.collectAsState() + val uiState by uiStateFlow.collectAsStateWithLifecycle()Requires androidx.lifecycle:lifecycle-runtime-compose.
120-121: Avoid per-recomposition allocationsMemoize DecimalFormat.
- val decimalFormat = DecimalFormat("0.000") + val decimalFormat = remember { DecimalFormat("0.000") }
220-227: Null-safe subtitle and visibility guard
- Ensure MenuItem.subtitle accepts null. If not, pass an empty string or omit the subtitle.
- Visibility is correctly gated by SUPPORTS_TXMETADATA and uiState.transactionMetadataVisible.
If non-null required:
- subtitle = uiState.transactionMetadataSubtitle, + subtitle = uiState.transactionMetadataSubtitle ?: "",wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt (1)
194-196: Avoid unnecessary UI updatesSkip update if unchanged to prevent recompositions.
- fun updateTransactionMetadataSubtitle(subtitle: String?) { - _uiState.value = _uiState.value.copy(transactionMetadataSubtitle = subtitle) - } + fun updateTransactionMetadataSubtitle(subtitle: String?) { + if (_uiState.value.transactionMetadataSubtitle != subtitle) { + _uiState.value = _uiState.value.copy(transactionMetadataSubtitle = subtitle) + } + }wallet/src/de/schildbach/wallet/ui/SettingsFragment.kt (1)
239-254: Simplify observer wiring and bind to view lifecycleAvoid nested observe and use viewLifecycleOwner.lifecycleScope for view-tied coroutines.
- lifecycleScope.launch { + viewLifecycleOwner.lifecycleScope.launch { transactionMetadataSettingsViewModel.loadLastWorkId() } - transactionMetadataSettingsViewModel.lastSaveWorkId.filterNotNull().observe(viewLifecycleOwner) { workId -> - transactionMetadataSettingsViewModel.observePublishOperation(workId).observe(viewLifecycleOwner) { - val progress = it.data?.progress?.let { data -> BaseWorker.extractProgress(data) } ?: 0 - setTransactionMetadataText(progress != 100 && progress != -1, progress) - } - } + transactionMetadataSettingsViewModel.lastSaveWorkId + .filterNotNull() + .onEach { workId -> + transactionMetadataSettingsViewModel.observePublishOperation(workId) + .onEach { state -> + val progress = state.data?.progress?.let { d -> BaseWorker.extractProgress(d) } ?: 0 + setTransactionMetadataText(progress != 100 && progress != -1, progress) + } + .launchIn(viewLifecycleOwner.lifecycleScope) + } + .launchIn(viewLifecycleOwner.lifecycleScope)Add:
import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.launchIn
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/src/main/java/org/dash/wallet/common/ui/components/MyTheme.kt(0 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(5 hunks)wallet/src/de/schildbach/wallet/ui/SettingsFragment.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/more/SettingsFragment.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataDialog.kt(1 hunks)
💤 Files with no reviewable changes (2)
- common/src/main/java/org/dash/wallet/common/ui/components/MyTheme.kt
- wallet/src/de/schildbach/wallet/ui/more/SettingsFragment.kt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
🔇 Additional comments (6)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (3)
155-158: LGTM! Testnet timing configuration improved.The addition of
Constants.IS_TESTNET_BUILDto the timing conditions appropriately enables shorter push periods and cutoff intervals for testnet builds, facilitating faster testing cycles without requiring DEBUG mode.
1064-1069: Verify the key index calculation aligns with the specification.The key index calculation changed significantly from:
transactionMetadataChangeCacheDao.count() + transactionMetadataDocumentDao.countAllRequests()to:
1 + transactionMetadataDocumentDao.countAllRequests()This removes the count of cached (unpublished) items from the calculation. This change could have implications:
- Key reuse risk: If cached items exist but aren't counted, subsequent publishes might reuse the same key indices.
- Derivation path consistency: The constant
1as the base suggests a deliberate starting point, but the rationale isn't clear.- Alignment with specification: Given the PR title mentions "align with specification," this might be intentional.
Please confirm:
- Does this match the txmetadata specification's key derivation requirements?
- Are there scenarios where cached items could cause key index collisions?
- Is the base of
1instead of0intentional?Based on learnings (as the PR description mentions finalizing txmetadata and aligning with specification).
808-828: Hash byte order handling is correct and consistent.The change to
wrapReversed()is appropriate. The codebase demonstrates a symmetric and consistent pattern:
- Receiving from platform (line 808):
Sha256Hash.wrapReversed(metadata.txId)converts platform's big-endian format to little-endian for storage- Sending to platform (line 1045):
it.txId.reversedBytesconverts from internal little-endian storage back to big-endian for the platform protocol- Explicit confirmation (lines 810, 1045): Both locations explicitly state "tx hash is stored in LE"
- Consistent usage: All subsequent database operations (lines 809–1017) use the converted
txIdAsHashfor DAO queries and entity creationThis follows Bitcoin protocol conventions: hashes are transmitted in big-endian (wire format) but stored internally in little-endian. The symmetric reversal pattern on send/receive confirms the implementation is correct.
wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt (1)
264-267: Preview looks goodPreview seeds the new TX metadata state; helpful for design review.
wallet/src/de/schildbach/wallet/ui/SettingsFragment.kt (1)
119-137: ****The review comment misunderstands SafeArgs behavior. The nav graph defines arguments as
first_timeanduse_navigation(snake_case), and the code correctly uses these exact names in thebundleOf()call. SafeArgs does not convert snake_case XML to camelCase properties—it preserves the declared names. The suggested fix using camelCase property names would break navigation because those properties don't exist. The code is correct as-is.Likely an incorrect or invalid review comment.
wallet/src/de/schildbach/wallet/ui/more/TransactionMetadataDialog.kt (1)
75-78: The review comment is incorrect—no issues foundThe
show(activity)method is properly defined in the parent classOffsetDialogFragment(lines 115-117) and correctly delegates to the standardshow(FragmentManager, String)API viaactivity.supportFragmentManager. There is no recursion, no compilation risk, and no need to refactor. The code works as intended through inheritance.Likely an incorrect or invalid review comment.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes