fix: fix username creation bugs and remove some ANR's #1350
Conversation
WalkthroughThis pull request introduces several asynchronous enhancements and balance management improvements across multiple modules. The changes update the build version, add new balance properties and coroutine-based logic for handling transactions and metadata, refactor UI observers to consolidate balance updates, and refine identity funding checks. Additionally, transaction lists are now sorted by time. Overall, these modifications streamline asynchronous operations and improve the consistency of balance-related updates and notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant Observer as WalletData Observer
participant VM as MainViewModel
participant Fragment as WalletFragment
participant UI as User Interface
Observer->>VM: Update balance and mixedBalance LiveData
VM-->>Fragment: Notify observers with new data
Fragment->>UI: Call updateMixedAndTotalBalance()
UI-->>Fragment: Render updated balance display
sequenceDiagram
participant Observer as WalletMostRecentTransactionsObserver
participant Scope as workerScope (Coroutine)
participant Processor as Transaction Processing
participant Closer as awaitClose Block
Observer->>Scope: Launch coroutine for transaction emission
Scope->>Processor: Process transaction asynchronously
Closer->>Scope: Cancel workerJob upon flow close
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wallet/src/de/schildbach/wallet/ui/transactions/TransactionGroupViewModel.kt (1)
105-105: Consider optimizing for frequent updates.While the current implementation is functional, consider these optimizations:
- Move sorting to database level if possible
- Add reverse sort option for UI flexibility
- Consider caching sorted results if the list is accessed frequently
Example optimization moving sort to database level:
// In your data access layer @Query("SELECT * FROM transactions ORDER BY time") fun getTransactionsSortedByTime(): Flow<List<Transaction>>wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt (1)
380-401: Extract magic number as a constant.The balance checking logic has been improved to use identity-specific balance and proper contestability checks. However, the magic number
1000used for balance comparison should be extracted as a constant for better maintainability.Apply this diff to improve maintainability:
+ companion object { + private const val BALANCE_MULTIPLIER = 1000L // Conversion factor for balance comparison + } // ... - if (balanceInfo.balance < balanceRequirement.value * 1000) { + if (balanceInfo.balance < balanceRequirement.value * BALANCE_MULTIPLIER) {wallet/src/de/schildbach/wallet/transactions/WalletMostRecentTransactionsObserver.kt (1)
39-50: Consider usingwithContextinstead oflaunch.Since you're not doing any fire-and-forget operations and need to wait for the transaction emission to complete, consider using
withContext(Dispatchers.IO)instead oflaunch. This would make the asynchronous nature of the code more explicit.- workerScope.launch { + workerScope.launch { + withContext(Dispatchers.IO) { org.bitcoinj.core.Context.propagate(Constants.CONTEXT) val allTxs = wallet.walletTransactions if (allTxs.any()) { var mostRecentTx = allTxs.first() allTxs.forEach { if (it.transaction.updateTime > mostRecentTx.transaction.updateTime) { mostRecentTx = it } } trySend(mostRecentTx.transaction) } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
build.gradle(1 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(7 hunks)wallet/src/de/schildbach/wallet/transactions/WalletMostRecentTransactionsObserver.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/LockScreenActivity.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TransactionGroupViewModel.kt(1 hunks)
🔇 Additional comments (17)
wallet/src/de/schildbach/wallet/ui/transactions/TransactionGroupViewModel.kt (1)
105-105: LGTM! The sort operation aligns with ANR fixes.The addition of
.sortedBy { row -> row.time }ensures consistent transaction ordering and helps prevent UI jank. The existing debounce of 500ms helps manage the performance impact of the sort operation.wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt (2)
12-12: LGTM!The import is correctly added for the new balance checking functionality.
322-327: LGTM! Improved error handling for preorder and domain document issues.The error handling has been enhanced to properly handle cases where:
- Preorder document with salted domain hash is missing
- Domain document is missing
In both cases, the creation state is appropriately reset to PREORDER_REGISTERING to allow for retry.wallet/src/de/schildbach/wallet/transactions/WalletMostRecentTransactionsObserver.kt (2)
22-24: LGTM! Good use of coroutines for asynchronous transaction handling.The addition of
SupervisorJobandCoroutineScopewithDispatchers.IOis a good choice for handling transaction emission asynchronously, which should help prevent ANRs.Also applies to: 35-36
95-95: LGTM! Proper cleanup of coroutine resources.The addition of
workerJob.cancel()in theawaitCloseblock ensures proper cleanup of coroutine resources when the flow is closed.wallet/src/de/schildbach/wallet/ui/main/WalletFragment.kt (2)
175-175: LGTM! Good refactoring of balance update logic.The consolidation of balance update logic into
updateMixedAndTotalBalance()improves code maintainability by reducing duplication. The addition ofmixedBalanceobserver ensures the UI is updated when mixed balance changes.Also applies to: 209-213
221-227: LGTM! Clean implementation of balance update method.The implementation of
updateMixedAndTotalBalance()is clean and uses string resources correctly for localization.wallet/src/de/schildbach/wallet/ui/LockScreenActivity.kt (1)
177-182: LGTM! Good use of coroutines for asynchronous balance check.The change to make balance check asynchronous in
setupBackupSeedReminder()usinglifecycleScope.launchandobserveBalance().first()helps prevent ANRs.wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (3)
209-211: LGTM! Good addition of mixed balance LiveData.The addition of
_mixedBalanceLiveData and its public getter improves the ViewModel's ability to manage and expose mixed balance state.
256-260: LGTM! Clean implementation of balance string properties.The implementation of
walletBalanceStringandmixedBalanceStringcorrectly uses the LiveData values and provides proper formatting.
358-360: LGTM! Proper observation of mixed balance.The observation of
COINJOIN_SPENDABLEbalance type ensures that mixed balance is properly tracked and updated.wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (5)
243-244: LGTM! Properties are correctly initialized.The new balance properties are properly initialized with
Coin.ZEROand have appropriate visibility modifiers.
258-260: Great improvement! Using coroutines for async metadata handling.This change helps prevent ANRs by moving the metadata sync operation off the main thread.
1014-1016: Consider the implications of context mismatch.While logging the warning is good, a context mismatch could indicate deeper issues. Please verify:
- Under what conditions would the contexts not match?
- What are the potential impacts on wallet operations?
- Should we handle this case more robustly?
1131-1139: LGTM! Well-structured balance observation setup.The implementation correctly:
- Observes both regular and CoinJoin balances
- Updates internal state
- Triggers appropriate UI updates through notifications
1164-1164: LGTM! Using cached balance value.The change improves efficiency by using the cached balance property instead of directly accessing the wallet.
build.gradle (1)
7-7:❓ Verification inconclusive
Verify the implications of using a SNAPSHOT version.
Moving from stable version 1.7.2 to snapshot version 1.7.3-SNAPSHOT could introduce instability. Please verify:
- Is this intentional for testing?
- Are there known issues with 1.7.2 that necessitate this change?
- What's the timeline for the stable 1.7.3 release?
🌐 Web query:
What is the latest stable version of DPP (Dash Platform Protocol)?💡 Result:
The latest stable version of Dash Platform Protocol (DPP) is 1.5.1, as indicated by its JavaScript implementation published on NPM 15 days prior to February 15, 2025[6]. This aligns with ongoing development activity in Dash’s ecosystem, including recent updates to Dash Core (v22.0.0)[4], though Core updates focus on network-layer enhancements rather than DPP itself.
For context:
- DPP governs serialization and validation rules for Dash Platform’s core data structures (data contracts, documents, state transitions)[5].
- The protocol underpins decentralized applications (dApps) built on Dash Platform[3][9].
No newer major versions are mentioned in official documentation or release notes as of February 2025[7][8].
Citations:
- 1: https://docs.dash.org/projects/core/en/stable/docs/reference/p2p-network-protocol-versions.html
- 2: https://dashif.org/Ingest/
- 3: https://academy.bit2me.com/en/que-es-dash-criptomoneda/
- 4: https://nownodes.io/blog/update-dash-core-version-22-0-0/
- 5: https://docs.dash.org/projects/platform/en/latest/docs/explanations/platform-protocol.html
- 6: https://www.npmjs.com/package/@dashevo/wasm-dpp
- 7: https://dash-greek.readthedocs.io/el/stable/docs/core/reference/p2p-network-protocol-versions.html
- 8: https://github.com/dashpay/platform/releases
- 9: https://docs.dash.org/projects/platform/en/latest/docs/sdk-js/overview.html
- 10: https://dashif.org/guidelines/earlier-versions/
Action Required: Confirm Intentional Use of a SNAPSHOT DPP Version
The code now uses:
dppVersion = "1.7.3-SNAPSHOT"However, note that the latest stable version of the Dash Platform Protocol is 1.5.1 per recent data. Please clarify the following:
- Is using the SNAPSHOT version intentional for testing, or is it an oversight?
- Were any issues with the previous stable version (presumably 1.7.2) driving this change?
- What is the timeline or plan for releasing a stable version corresponding to 1.7.3?
This clarification is important to ensure that we aren’t inadvertently introducing instability in production deployments.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
1054-1058: 🛠️ Refactor suggestionConsider propagating errors to callers.
The method swallows all exceptions (except
CancellationException), which could mask serious issues. Callers have no way to know if the operation failed.Consider one of these approaches:
- Return a
Resulttype to indicate success/failure:- private suspend fun publishChangeCache(before: Long) { + private suspend fun publishChangeCache(before: Long): Result<Unit> { if (!Constants.SUPPORTS_TXMETADATA) { - return + return Result.success(Unit) } try { // ... existing code ... + return Result.success(Unit) } catch (_: CancellationException) { log.info("publishing updates canceled") + throw _ } catch (e: Exception) { log.error("publishing exception caught", e) + return Result.failure(e) } }
- Or create a custom exception type for platform sync errors:
class PlatformSyncException(message: String, cause: Throwable? = null) : Exception(message, cause)
🧹 Nitpick comments (11)
common/src/main/java/org/dash/wallet/common/WalletDataProvider.kt (1)
77-78: Add documentation for new balance observation methods.Please add KDoc comments to clarify:
- The purpose and behavior of these new methods
- How
observeBalance()differs from the existingobserveBalance(balanceType, coinSelector)- The relationship between these Flow-based methods and the synchronous
getWalletBalance()Example documentation:
/** * Observes the mixed balance of the wallet. * @return A Flow emitting the current mixed balance whenever it changes */ fun observeMixedBalance(): Flow<Coin> /** * Observes the total balance of the wallet. * @return A Flow emitting the current total balance whenever it changes */ fun observeBalance(): Flow<Coin>wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (3)
984-986: Move logging after the guard clause.The log statement appears before the guard clause, which means it will log even when transaction metadata is not supported. This could lead to confusing logs.
Apply this diff to fix the logging sequence:
private suspend fun publishChangeCache(before: Long) { if (!Constants.SUPPORTS_TXMETADATA) { return } + log.info("publishing updates to tx metadata items before $before") val itemsToPublish = hashMapOf<Sha256Hash, TransactionMetadataCacheItem>() - log.info("publishing updates to tx metadata items before $before")Also applies to: 987-988
987-987: Improve logging consistency and structure.The logging pattern could be improved for better observability:
- Some log messages could be more descriptive
- Log levels are not consistently applied
- Missing structured logging for better parsing
Consider these improvements:
- log.info("publishing updates to tx metadata items before $before") + log.debug("Starting metadata publish for items before timestamp: $before") - log.info("preparing to publish ${changedItems.size} tx metadata changes to platform") + log.info("Found {} items to publish to platform", changedItems.size) - log.info("publishing ${itemsToPublish.values.size} tx metadata items to platform") + log.info("Publishing metadata items to platform [count={}, timestamp={}]", + itemsToPublish.values.size, before) - log.info("published ${itemsToPublish.values.size} tx metadata items to platform") + log.info("Successfully published metadata items [count={}]", itemsToPublish.values.size) - log.error("publishing exception caught", e) + log.error("Failed to publish metadata items [count={}, error={}]", + itemsToPublish.values.size, e.message, e)Also applies to: 996-996, 1045-1045, 1049-1049, 1057-1057
117-134: Consider breaking down the service into smaller, focused components.The
PlatformSynchronizationServiceclass has grown quite large and handles multiple responsibilities (contacts, metadata, usernames, etc.). This makes it harder to maintain and test.Consider:
- Breaking down into smaller services:
interface ContactSyncService { suspend fun updateContactRequests() suspend fun updateContactProfiles() } interface MetadataSyncService { suspend fun publishChangeCache() suspend fun updateTransactionMetadata() } interface UsernameSyncService { suspend fun updateUsernameRequests() suspend fun checkVotingStatus() }
- Using a facade pattern to coordinate these services:
class PlatformSynchronizationFacade( private val contactSync: ContactSyncService, private val metadataSync: MetadataSyncService, private val usernameSync: UsernameSyncService ) : PlatformSyncService { // Coordinate the individual services }This would:
- Improve separation of concerns
- Make the code more testable
- Make it easier to maintain and extend
wallet/src/de/schildbach/wallet/Constants.java (2)
137-139: Consider consolidating platform support flags.The devnet configuration shows three closely related platform support flags being set consecutively. Consider grouping these flags into a configuration object or enum to better manage platform feature support.
Example refactor:
- SUPPORTS_PLATFORM = !is32Bit; - SUPPORTS_INVITES = false; - SUPPORTS_TXMETADATA = false; + public static final class PlatformFeatures { + public static final boolean ENABLED = !is32Bit; + public static final boolean INVITES = false; + public static final boolean TX_METADATA = false; + }
63-67: Consider documenting the platform feature flag dependencies.The growing number of platform-related feature flags (
SUPPORTS_PLATFORM,SUPPORTS_INVITES,SUPPORTS_TXMETADATA) suggests a need for:
- Documentation of dependencies between these flags
- A migration plan for enabling these features
- Impact analysis when these flags are enabled/disabled
This will help prevent potential issues during the gradual rollout of platform features.
wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt (3)
56-61: Graceful throttling approach.
By deferring balance updates toonThrottledWalletChanged, the new logic helps avoid spamming the observer. Confirm that the throttling interval is appropriate for high-volume transactions.
74-85: Check performance overhead inemitBalances().
This callswallet.getBalance(...)every time the wallet changes. For very frequent changes, consider caching logic or rate-limiting the calls.
87-93: Consider error handling in the callbackFlow.
A runtime exception inemitBalance()could leave the flow in an undefined state. Gracefully closing the channel on errors may improve resilience.wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (2)
257-259: Decimal formatting for wallet balance string.
Ensuring the format always shows three decimal places can be helpful, but confirm localized behavior if the app supports diverse locales.
261-262: Decimal formatting for mixed balance string.
Same observation as above: consider localizing or adjusting decimal places based on user preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
build.gradle(1 hunks)common/src/main/java/org/dash/wallet/common/WalletDataProvider.kt(1 hunks)wallet/res/layout/wallet_transactions_fragment.xml(1 hunks)wallet/src/de/schildbach/wallet/Constants.java(4 hunks)wallet/src/de/schildbach/wallet/WalletApplication.java(6 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(7 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(1 hunks)wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt(6 hunks)wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build.gradle
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (19)
wallet/src/de/schildbach/wallet/Constants.java (2)
66-67: LGTM! Well-documented constant declaration.The TODO comment clearly indicates the temporary nature of this flag and when it should be removed.
97-97:✅ Verification successful
Verify platform support configuration across build flavors.
The
SUPPORTS_TXMETADATAflag is consistently set tofalseacross all build flavors, which aligns with the TODO comment. However, we should ensure that this configuration is synchronized with the platform's transaction metadata implementation timeline.Run this script to check for any references to transaction metadata in the codebase that might be affected:
Also applies to: 116-116, 139-139
🏁 Script executed:
#!/bin/bash # Description: Search for transaction metadata references that might be affected by the flag # Search for transaction metadata related code rg -i "transaction.*metadata|txmetadata" --type java --type kotlin # Search for platform-related configuration files fd -e json -e xml -e properties -e gradle "platform|metadata" --type f --exec grep -i "transaction.*metadata|txmetadata" {} \;Length of output: 59465
Transaction Metadata Flag Consistency Verified
The
SUPPORTS_TXMETADATAflag is confirmed to be set tofalseacross all build flavors (see file wallet/src/de/schildbach/wallet/Constants.java, lines 97, 116, and 139), matching the existing TODO. Our verification shows that while transaction metadata functionality is referenced broadly (in providers, services, and tests), no build flavor inadvertently enables it. Please ensure that once the platform’s timeline for implementing transaction metadata support becomes definitive, both the flag and its related code paths (e.g., in PlatformSyncService.kt and the TransactionMetadataProvider integrations) are updated accordingly.wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt (4)
27-28: Import statements look good.
They properly introduce Flow-based constructs for asynchronous balance state management.
68-72: Robust cleanup inclose().
Removing the event listener, callbacks, and canceling theemitterJobensures there are no memory leaks. Looks excellent.
125-125: Proper job cancellation.
Canceling theemitterJobinawaitCloseis a sound approach for preventing leaks in the callbackFlow.
40-40:✅ Verification successful
Validate external usage after constructor signature change.
Previously, this observer accepted optional parameters. Now it only accepts a singleWallet, which may break existing usage.Please confirm that all references to the old constructor have been updated properly by searching the codebase:
🏁 Script executed:
#!/bin/bash rg -A 3 "WalletBalanceObserver("Length of output: 129
I'll update the verification command to escape the parenthesis correctly. Let's re-run the search with the following shell script:
Please run this updated script and share the output to confirm that all references to the old constructor have been updated.
🏁 Script executed:
#!/bin/bash rg -A 3 'WalletBalanceObserver\('Length of output: 742
External Usage Verified for WalletBalanceObserver Constructor Change
We've confirmed that all external references now instantiate the observer using the updated constructor signature. For example, in
wallet/src/de/schildbach/wallet/WalletApplication.javathe observer is correctly invoked with a singleWalletargument (new WalletBalanceObserver(wallet)). No residual references to the old constructor with optional parameters were found.wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (1)
233-233: Recheck empty view logic.
SettingwalletTransactionsEmpty.isVisible = viewModel.transactionsLoadedmay hide the empty view if transactions are still loading. Confirm that this is the intended behavior, especially when an empty set is retrieved.wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (4)
111-111: New import for BigDecimal.
This import is aligned with the new decimal formatting usage for balances.
165-166:transactionsLoadedflag introduced.
This boolean tracks whether all transactions have finished loading. Make sure this is reset appropriately if the user refreshes or re-enters the screen.
211-213:_mixedBalanceLiveData.
Exposing a separatemixedBalanceis a clear design choice for coinjoin or privacy mixes. Implementation looks straightforward.
559-559:transactionsLoaded = trueset after refresh completes.
If there are potential error states or partial loads, ensure to handle them before forcing this totrue. Otherwise, the UI may show an empty state incorrectly.wallet/src/de/schildbach/wallet/WalletApplication.java (4)
213-213: LGTM! Good placement of the WalletBalanceObserver initialization.The
WalletBalanceObserveris properly initialized after the wallet is loaded, ensuring that the observer has access to a fully initialized wallet instance.Also applies to: 579-580
1042-1043: LGTM! Proper cleanup of WalletBalanceObserver.The observer is correctly closed and nullified during wallet wipe, preventing memory leaks and ensuring proper cleanup.
1106-1107: LGTM! Consistent use of WalletBalanceObserver for balance retrieval.The method now correctly uses the centralized
walletBalanceObserverinstead of directly accessing the wallet's balance, maintaining consistency with the observer pattern.
1110-1128: LGTM! Well-implemented balance observation methods.The new methods provide a reactive way to observe balance changes using Kotlin Flow, with proper null checks and error handling.
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (3)
243-244: LGTM! Proper initialization of balance fields.The balance fields are correctly initialized to
Coin.ZEROand serve as a cache for the current balance state.
1131-1139: LGTM! Well-structured balance observation setup.The balance observers are properly set up to update the cached balance fields and trigger blockchain state notifications when changes occur.
1014-1016: LGTM! Good addition of context mismatch warning.The warning log helps identify potential context synchronization issues between the wallet and Constants.CONTEXT.
wallet/res/layout/wallet_transactions_fragment.xml (1)
25-26: LGTM! Improved empty view visibility handling.Setting the empty view to initially invisible prevents UI flickering during the initial load of transactions.
| @@ -74,4 +74,6 @@ interface WalletDataProvider { | |||
| fun checkSendingConditions(address: Address?, amount: Coin) | |||
|
|
|||
| fun observeMostRecentTransaction(): Flow<Transaction> | |||
| fun observeMixedBalance(): Flow<Coin> | |||
| fun observeBalance(): Flow<Coin> | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider renaming or deprecating the parameterized observeBalance method.
The new observeBalance() method might cause confusion with the existing observeBalance(balanceType, coinSelector). Consider either:
- Renaming one of them to be more specific (e.g.,
observeTotalBalance()orobserveFilteredBalance()) - Deprecating the parameterized version if it's being replaced
Let's check if the parameterized version is still in use:
🏁 Script executed:
#!/bin/bash
# Search for usages of observeBalance with parameters
rg "observeBalance\s*\([^)]+\)" -A 2Length of output: 368
Rename or Deprecate Parameterized observeBalance Method – Action Required
The parameterized version observeBalance(balanceType, coinSelector) is still in use in wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt. Deprecating it outright without updating its consumers may introduce breaking changes. Please either:
- Rename one of the methods to clearly differentiate their intent (e.g., use
observeTotalBalance()for the parameterless variant andobserveFilteredBalance()for the parameterized one), or - If deprecation is intended, update and refactor all current usages (such as in
SendCoinsViewModel.kt) before proceeding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt (1)
84-99: Consider adding error handling for balance emission.The balance emission could benefit from try-catch blocks to handle potential errors during balance retrieval and persistence.
fun emitBalances() { emitterScope.launch { org.bitcoinj.core.Context.propagate(Constants.CONTEXT) + try { val mixedBalance = wallet.getBalance(BalanceType.COINJOIN_SPENDABLE) walletUIConfig.set(WalletUIConfig.LAST_MIXED_BALANCE, mixedBalance.value) _mixedBalance.emit(mixedBalance) val totalBalance = wallet.getBalance(BalanceType.ESTIMATED) walletUIConfig.set(WalletUIConfig.LAST_TOTAL_BALANCE, totalBalance.value) _totalBalance.emit(totalBalance) + } catch (e: Exception) { + log.error("Error emitting balances", e) + } } }wallet/src/de/schildbach/wallet/WalletApplication.java (1)
1109-1110: Consider adding a comment explaining the balance retrieval change.The switch from direct wallet balance retrieval to using WalletBalanceObserver should be documented.
-//return wallet.getBalance(Wallet.BalanceType.ESTIMATED); +// Using WalletBalanceObserver for consistent balance tracking across the app return walletBalanceObserver.getTotalBalance().getValue();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/java/org/dash/wallet/common/data/WalletUIConfig.kt(1 hunks)wallet/src/de/schildbach/wallet/WalletApplication.java(7 hunks)wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt(3 hunks)
🔇 Additional comments (8)
common/src/main/java/org/dash/wallet/common/data/WalletUIConfig.kt (1)
66-67: LGTM! Well-structured preference keys for balance persistence.The new preference keys are well-named and use the appropriate type for storing balance values.
wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt (3)
50-56: LGTM! Well-structured state management for balances.The use of StateFlow for both total and mixed balances provides a robust reactive pattern for balance updates.
65-69: LGTM! Proper initialization and cleanup.The initialization block properly sets up the wallet change listener and initializes balances.
71-75: LGTM! Clean resource management.The close method properly cleans up resources by removing listeners and canceling jobs.
wallet/src/de/schildbach/wallet/WalletApplication.java (2)
216-216: LGTM! Centralized balance management.The WalletBalanceObserver instance provides centralized balance management.
1045-1046: LGTM! Proper resource cleanup.The finalizeWipe method properly cleans up the WalletBalanceObserver instance.
wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt (2)
380-400: LGTM! Improved balance check logic.The balance check logic has been improved to:
- Use direct identity balance checks
- Handle contestable usernames correctly
- Apply appropriate balance requirements
322-337: Improved error message handling.The error message handling is now more precise and handles specific error cases better.
| val LAST_TOTAL_BALANCE = longPreferencesKey("last_total_balance") | ||
| val LAST_MIXED_BALANCE = longPreferencesKey("last_mixed_balance") |
There was a problem hiding this comment.
Save the last calculated values so that they can be emitted first when the app starts
| .observeBalance() | ||
| .observeTotalBalance() |
There was a problem hiding this comment.
observeBalance -> observeTotalBalance, make it explicity getting the total balance, not necessarily what is spendable.
This balance should be based on the CoinJoin Mode, but that will be handled later.
| @@ -92,6 +94,7 @@ public final class Constants { | |||
| org.dash.wallet.common.util.Constants.INSTANCE.setEXPLORE_GC_FILE_PATH("explore/explore.db"); | |||
| SUPPORTS_PLATFORM = !is32Bit; | |||
| SUPPORTS_INVITES = false; | |||
| SUPPORTS_TXMETADATA = false; | |||
There was a problem hiding this comment.
This will disable publishing tx-metadata to platform, which is an unfinished feature.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/viewmodels/TransferDashViewModel.kt (1)
117-125: 🛠️ Refactor suggestionAdd error handling for address retrieval failure.
The failure case in
getUserAccountAddress()is silently ignored, which could lead to issues when the address is needed later.Add error handling:
private fun getUserAccountAddress() = viewModelScope.launch(Dispatchers.Main) { when (val response = coinBaseRepository.getUserAccountAddress()) { is ResponseResource.Success -> { observeCoinbaseUserAccountAddress.value = response.value ?: "" } is ResponseResource.Failure -> { + onAddressCreationFailedCallback.call() } } }
🧹 Nitpick comments (7)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (2)
1021-1024: Consider enhancing the context check.While the warning log is helpful, consider adding more context information to aid debugging:
- if (application.wallet?.context != Constants.CONTEXT) { - log.warn("wallet context does not equal Constants.CONTEXT") - } + application.wallet?.context?.let { walletContext -> + if (walletContext != Constants.CONTEXT) { + log.warn("Wallet context mismatch - Expected: ${Constants.CONTEXT}, Actual: $walletContext") + } + }
1322-1377: Consider adding deadlock prevention measures.While the mutex usage is generally correct, there's a potential risk of deadlock if cleanup is called recursively. Consider using tryLock with timeout:
- checkMutex.lock() + // Prevent potential deadlocks with timeout + if (!checkMutex.tryLock(5000)) { + log.warn("Failed to acquire cleanup lock after 5 seconds") + return@launch + }integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/CrowdNodeViewModel.kt (2)
134-175: Consider using viewModelScope.launch for initialization.To prevent potential ANRs, consider moving the initialization of Flow collectors into a coroutine:
init { + viewModelScope.launch { walletDataProvider.observeTotalBalance() .distinctUntilChanged() .onEach { _dashBalance.postValue(it) _hasEnoughBalance.postValue(it >= CrowdNodeConstants.MINIMUM_REQUIRED_DASH) } - .launchIn(viewModelScope) + .collect() walletUIConfig.observe(WalletUIConfig.SELECTED_CURRENCY) .filterNotNull() .flatMapLatest(exchangeRatesProvider::observeExchangeRate) .onEach(_exchangeRate::postValue) - .launchIn(viewModelScope) + .collect() crowdNodeApi.balance .onEach { when (it.status) { Status.LOADING -> { _crowdNodeBalance.postValue( _crowdNodeBalance.value?.copy(balance = it.data ?: Coin.ZERO, isUpdating = true) ) } Status.SUCCESS -> { _crowdNodeBalance.postValue( _crowdNodeBalance.value?.copy(balance = it.data ?: Coin.ZERO, isUpdating = false) ) } Status.ERROR -> { _crowdNodeBalance.postValue(_crowdNodeBalance.value?.copy(isUpdating = false)) networkError.call() } else -> _crowdNodeBalance.postValue(_crowdNodeBalance.value?.copy(isUpdating = false)) } } - .launchIn(viewModelScope) + .collect() config.observe(CrowdNodeConfig.FEE_PERCENTAGE) .onEach { crowdNodeFee = it ?: FeeInfo.DEFAULT_FEE } - .launchIn(viewModelScope) + .collect() + } }
148-168: Consider using StateFlow for better state management.The current implementation uses multiple postValue calls which could lead to dropped updates. Consider using StateFlow for more reliable state management:
- private val _crowdNodeBalance: MutableLiveData<BalanceUIState> = MutableLiveData(BalanceUIState()) - val crowdNodeBalance: LiveData<BalanceUIState> + private val _crowdNodeBalance = MutableStateFlow(BalanceUIState()) + val crowdNodeBalance: StateFlow<BalanceUIState> get() = _crowdNodeBalance crowdNodeApi.balance .onEach { when (it.status) { Status.LOADING -> { - _crowdNodeBalance.postValue( + _crowdNodeBalance.value = _crowdNodeBalance.value?.copy(balance = it.data ?: Coin.ZERO, isUpdating = true) - ) } Status.SUCCESS -> { - _crowdNodeBalance.postValue( + _crowdNodeBalance.value = _crowdNodeBalance.value?.copy(balance = it.data ?: Coin.ZERO, isUpdating = false) - ) } Status.ERROR -> { - _crowdNodeBalance.postValue(_crowdNodeBalance.value?.copy(isUpdating = false)) + _crowdNodeBalance.value = _crowdNodeBalance.value?.copy(isUpdating = false) networkError.call() } - else -> _crowdNodeBalance.postValue(_crowdNodeBalance.value?.copy(isUpdating = false)) + else -> _crowdNodeBalance.value = _crowdNodeBalance.value?.copy(isUpdating = false) } }integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/viewmodels/TransferDashViewModel.kt (1)
331-337: Cache trimmed address value.The
dashAddressproperty trims whitespace on every access, which is inefficient if accessed frequently.Consider caching the trimmed value:
-private val dashAddress: Address - get() = Address.fromString( - walletDataProvider.networkParameters, - (observeCoinbaseAddressState.value ?: observeCoinbaseUserAccountAddress.value ?: "").trim { - it <= ' ' - } - ) +private var cachedAddress: Address? = null +private val dashAddress: Address + get() { + if (cachedAddress == null) { + val trimmedAddress = (observeCoinbaseAddressState.value + ?: observeCoinbaseUserAccountAddress.value + ?: "").trim { it <= ' ' } + cachedAddress = Address.fromString(walletDataProvider.networkParameters, trimmedAddress) + } + return cachedAddress!! + } + +// Add this to clear the cache when address changes +init { + observeCoinbaseAddressState.observeForever { cachedAddress = null } + observeCoinbaseUserAccountAddress.observeForever { cachedAddress = null } +}features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashdirect/DashDirectViewModel.kt (1)
108-137: Consider adding timeouts to network calls to prevent ANRs.The network operations in
purchaseGiftCard()andupdateMerchantDetails()could potentially cause ANRs if the network is slow or unresponsive. Consider adding timeouts and more robust error handling.Here's how you could improve the network calls:
suspend fun purchaseGiftCard(): PurchaseGiftCardResponse.Data? { giftCardMerchant.merchantId?.let { val amountValue = giftCardPaymentValue repository.getDashDirectEmail()?.let { email -> + try { + withTimeout(30_000) { // 30 seconds timeout val response = repository.purchaseGiftCard( merchantId = it, amountUSD = amountValue.toBigDecimal().toDouble(), paymentCurrency = Constants.DASH_CURRENCY, userEmail = email ) // ... rest of the code + } + } catch (e: TimeoutCancellationException) { + log.error("purchaseGiftCard timeout", e) + throw DashDirectException("Request timed out") + } } } return null } suspend fun updateMerchantDetails(merchant: Merchant) { if (merchant.minCardPurchase != null && merchant.maxCardPurchase != null) { return } + try { + withTimeout(30_000) { // 30 seconds timeout val response = getMerchantById(merchant.merchantId!!) if (response is ResponseResource.Success) { response.value?.data?.merchant?.let { merchant.savingsPercentage = it.savingsPercentage merchant.minCardPurchase = it.minimumCardPurchase merchant.maxCardPurchase = it.maximumCardPurchase } } + } + } catch (e: TimeoutCancellationException) { + log.error("updateMerchantDetails timeout", e) + } }Don't forget to add the following import:
import kotlinx.coroutines.withTimeout import kotlinx.coroutines.TimeoutCancellationExceptionAlso applies to: 147-161
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
165-166: Consider makingtransactionsLoadedobservable.The property tracks transaction loading state but isn't observable. Consider using LiveData or StateFlow to allow UI to react to loading state changes.
-var transactionsLoaded = false - private set +private val _transactionsLoaded = MutableStateFlow(false) +val transactionsLoaded: StateFlow<Boolean> = _transactionsLoaded.asStateFlow()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
common/src/main/java/org/dash/wallet/common/WalletDataProvider.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashdirect/DashDirectViewModel.kt(1 hunks)integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/viewmodels/TransferDashViewModel.kt(1 hunks)integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/CrowdNodeViewModel.kt(1 hunks)integrations/crowdnode/src/test/java/org/dash/wallet/integrations/crowdnode/CrowdNodeViewModelTest.kt(1 hunks)wallet/src/de/schildbach/wallet/WalletApplication.java(7 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(12 hunks)wallet/src/de/schildbach/wallet/service/CoinJoinService.kt(2 hunks)wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/LockScreenActivity.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt(6 hunks)wallet/test/de/schildbach/wallet/util/viewModels/MainViewModelTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/java/org/dash/wallet/common/WalletDataProvider.kt
👮 Files not reviewed due to content moderation or server errors (4)
- wallet/src/de/schildbach/wallet/ui/LockScreenActivity.kt
- wallet/src/de/schildbach/wallet/service/CoinJoinService.kt
- wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt
- wallet/src/de/schildbach/wallet/WalletApplication.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (44)
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (3)
173-246: LGTM! Well-structured concurrency control and balance tracking.The addition of the mutex for thread safety and the balance properties for tracking different types of balances is well-implemented.
259-263: LGTM! Improved performance with coroutine-based metadata handling.The change to handle transaction metadata asynchronously using coroutines is a good improvement that reduces blocking operations.
1138-1146: LGTM! Well-implemented balance observation system.The balance observers are properly implemented using LiveData, ensuring thread-safe updates and appropriate lifecycle management.
integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/CrowdNodeViewModel.kt (2)
134-140: LGTM! Balance observation change aligns with codebase-wide refactoring.The switch to
observeTotalBalance()is consistent with similar changes across other ViewModels, providing a more comprehensive view of the user's funds.
291-294: Verify balance check in deposit function.The
depositfunction performs a balance check, but ensure that thedashBalance.valueis not null before comparison:suspend fun deposit(value: Coin, checkBalanceConditions: Boolean): Boolean { - val emptyWallet = value >= dashBalance.value + val currentBalance = dashBalance.value ?: return false + val emptyWallet = value >= currentBalance return crowdNodeApi.deposit(value, emptyWallet, checkBalanceConditions) }wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt (1)
62-62: LGTM! Balance observation update aligns with ANR fixes.The change from
observeBalance()toobserveTotalBalance()is part of a consistent refactoring across the codebase to improve balance handling and reduce ANRs.integrations/crowdnode/src/test/java/org/dash/wallet/integrations/crowdnode/CrowdNodeViewModelTest.kt (1)
80-80: LGTM! Test mock updated to match implementation.The mock setup correctly reflects the change from
observeBalance()toobserveTotalBalance(), maintaining test consistency with the implementation changes.wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt (5)
50-56: LGTM! Improved state management with StateFlow.The introduction of StateFlow for balance management provides better reactivity and state handling, which helps reduce ANRs.
65-71: LGTM! Proper initialization of wallet listeners.The initialization block properly sets up all necessary wallet listeners and initializes the state, ensuring consistent balance updates.
73-79: LGTM! Proper resource cleanup implementation.The
close()method ensures proper cleanup of wallet listeners and cancellation of coroutine jobs, preventing memory leaks.
81-86: LGTM! State persistence implementation.The
emitLastBalances()function properly restores the last known balances from persistent storage, ensuring state consistency across app launches.
88-103: LGTM! Efficient balance emission implementation.The
emitBalances()function efficiently updates and persists both total and mixed balances, with proper context propagation.wallet/test/de/schildbach/wallet/util/viewModels/MainViewModelTest.kt (1)
177-177: LGTM! Test mock updated to match implementation.The mock setup correctly reflects the change from
observeBalance()toobserveTotalBalance(), maintaining test consistency with the implementation changes.wallet/src/de/schildbach/wallet/ui/LockScreenActivity.kt (3)
176-182: LGTM! Improved balance check with coroutines.The refactoring of
setupBackupSeedReminderto use coroutines and flow improves the responsiveness of the UI by making the balance check asynchronous. The implementation correctly:
- Uses
lifecycleScope.launchfor coroutine management- Retrieves balance using
observeTotalBalance().first()- Updates configuration based on balance state
176-182: Great improvement in balance check implementation!The change from synchronous to asynchronous balance checking using coroutines and Flow improves the application's responsiveness by preventing potential blocking of the main thread.
176-182: LGTM! Improved balance check using coroutines.The change from synchronous to asynchronous balance check using coroutines and flow is a good improvement that prevents blocking the main thread.
wallet/src/de/schildbach/wallet/service/CoinJoinService.kt (5)
231-232: LGTM! Consistent use of total balance observation.The change from
observeBalancetoobserveTotalBalancealigns with the new balance observation pattern across the codebase.
391-391: LGTM! Consistent use of wallet balance retrieval.The change to use
getWalletBalance()maintains consistency with the new balance retrieval pattern.
231-232: Consistent use of total balance observation.The change to use
observeTotalBalancealigns with the new balance observation pattern across the application.
231-232: LGTM! Consistent use of total balance observation.The change to use
observeTotalBalancealigns with the new balance observation pattern across the codebase.
391-391: LGTM! Consistent use of wallet balance retrieval.The change to use
getWalletBalancemaintains consistency with the new balance retrieval pattern.wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (9)
165-166: LGTM! Added transaction loading state tracking.The
transactionsLoadedproperty with private setter helps track the transaction loading state.
211-213: LGTM! Added mixed balance support.The addition of
_mixedBalanceLiveData enhances balance management by tracking mixed balance separately.
258-262: LGTM! Improved balance string properties.The renamed balance string properties better reflect their purpose and use the new balance LiveData correctly.
356-362: LGTM! Enhanced balance observation.The implementation correctly observes total and mixed balances using the new observation methods.
165-166: Good addition of transaction loading state.The
transactionsLoadedproperty with private setter helps track when transactions are fully loaded, improving state management.
211-213: Proper separation of mixed balance tracking.The addition of
_mixedBalanceLiveData improves the separation of concerns for different balance types.
258-262: More descriptive property names.Renaming balance properties to include their type (e.g.,
walletBalanceString,mixedBalanceString) improves code clarity.
211-213: LGTM! Added mixed balance LiveData.The addition of mixed balance LiveData enhances the view model's ability to track and expose mixed balance state.
356-362: LGTM! Consistent use of total and mixed balance observation.The changes to use
observeTotalBalanceandobserveMixedBalancealign with the new balance observation pattern.wallet/src/de/schildbach/wallet/WalletApplication.java (12)
582-583: LGTM! Centralized balance observation.The addition of
walletBalanceObservercentralizes balance observation through a dedicated observer.
1109-1110: LGTM! Improved balance retrieval.The change to use
walletBalanceObserver.getTotalBalance()provides a consistent way to retrieve the total balance.
1113-1131: LGTM! Added comprehensive balance observation methods.The new methods
observeTotalBalance()andobserveMixedBalance()provide a clean API for observing different balance types.
1045-1046: LGTM! Proper cleanup of balance observer.The
finalizeWipemethod correctly cleans up the balance observer by closing and nullifying it.
216-216: Good centralization of balance observation.The addition of
walletBalanceObservercentralizes balance tracking, improving the architecture.
1109-1110: Improved balance retrieval pattern.Using
walletBalanceObserver.getTotalBalance()instead of direct wallet access provides a more consistent way to access balances.
1113-1131: Well-structured balance observation methods.The new methods
observeTotalBalanceandobserveMixedBalanceprovide a clean API for observing different balance types.
1045-1046: Proper resource cleanup.The
finalizeWipemethod now properly closes and nullifies thewalletBalanceObserver, preventing potential memory leaks.
216-216: LGTM! Added centralized balance observer.The addition of
walletBalanceObservercentralizes balance management, improving the architecture.
1109-1110: LGTM! Updated balance retrieval to use observer.The change to use
walletBalanceObserverfor balance retrieval maintains consistency with the new pattern.
1113-1131: LGTM! Added total and mixed balance observation methods.The addition of
observeTotalBalanceandobserveMixedBalancemethods enhances the wallet's balance observation capabilities.
1045-1046: LGTM! Proper cleanup of balance observer.The changes ensure proper cleanup of the balance observer during wallet wipe.
integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/viewmodels/TransferDashViewModel.kt (1)
99-99:✅ Verification successful
LGTM! Verify balance calculation behavior.
The change from
observeBalance()toobserveTotalBalance()aligns with similar updates across the codebase. This standardization improves consistency in balance observation.Let's verify the implementation of
observeTotalBalance()to ensure it correctly includes all balance components:
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of observeTotalBalance() and its usage # Test: Search for the implementation and all usages of observeTotalBalance # Find the implementation ast-grep --pattern $'interface WalletDataProvider { $$$ fun observeTotalBalance(): Flow<Coin> { $$$ } $$$ }' # Find all usages rg -A 5 'observeTotalBalance\(\)'Length of output: 8530
Approved: Total Balance Observation Standardization
The switch from
observeBalance()toobserveTotalBalance()has been verified across the codebase. The implementation inWalletDataProviderand its consistent use in other modules (e.g., inDashDirectViewModel.ktand related tests) confirm that this change standardizes balance observation as intended. Please ensure that the aggregated balance calculation still meets all business and UX expectations.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashdirect/DashDirectViewModel.kt (1)
101-105:✅ Verification successful
LGTM! Verify balance calculations remain accurate.
The change from
observeBalance()toobserveTotalBalance()aligns with the broader refactoring of balance management across the application. This should help reduce ANRs by streamlining balance updates.Run the following script to verify that all balance-dependent calculations in this class still work correctly with the total balance:
🏁 Script executed:
#!/bin/bash # Description: Check if balance is used correctly in calculations # Look for methods that use the balance property ast-grep --pattern 'fun $method($params) { $$$ balance $$$ }' # Look for direct balance property access rg '\b_balance\b|\bbalance\b' -A 5Length of output: 67295
Balance Refactoring Approved
The update to use
observeTotalBalance()instead ofobserveBalance()aligns with the broader refactoring seen across the codebase. Our automated scan confirms that balance handling is consistent in various modules, and there’s no evidence that the switch affects any balance-dependent calculations adversely. Please continue to monitor any integration scenarios, but this change appears solid.
| SUPPORTS_PLATFORM = !is32Bit; | ||
| SUPPORTS_INVITES = false; | ||
| SUPPORTS_TXMETADATA = false; |
There was a problem hiding this comment.
We haven't made devnet builds in a long time, but let's update the feature flags.
| @@ -580,6 +578,9 @@ private void afterLoadWallet() { | |||
| // make sure there is at least one recent backup | |||
| if (!getFileStreamPath(Constants.Files.WALLET_KEY_BACKUP_PROTOBUF).exists()) | |||
| backupWallet(); | |||
|
|
|||
| // setup WalletBalanceObserver | |||
| walletBalanceObserver = new WalletBalanceObserver(wallet, walletUIConfig); | |||
There was a problem hiding this comment.
this is the "global" wallet observer.
| walletBalanceObserver.close(); | ||
| walletBalanceObserver = null; |
There was a problem hiding this comment.
when the wallet is replaced or reset, we should also close out the balance observer.
| metadataHandler.post { | ||
| transactionMetadataProvider.syncTransactionBlocking( | ||
| tx | ||
| ) | ||
| serviceScope.launch { | ||
| transactionMetadataProvider.syncTransaction(tx) |
There was a problem hiding this comment.
use coroutine instead of a Handler()
|
|
||
| application.observeTotalBalance().observe(this@BlockchainServiceImpl) { | ||
| balance = it | ||
| handleBlockchainStateNotification(blockchainState, mixingStatus, mixingProgress) | ||
| } | ||
|
|
||
| application.observeMixedBalance().observe(this@BlockchainServiceImpl) { | ||
| mixedBalance = it | ||
| handleBlockchainStateNotification(blockchainState, mixingStatus, mixingProgress) | ||
| } |
There was a problem hiding this comment.
balance updates will trigger notification updates
| @@ -1300,6 +1319,7 @@ class BlockchainServiceImpl : LifecycleService(), BlockchainService { | |||
| serviceScope.launch { | |||
| try { | |||
| onCreateCompleted.await() // wait until onCreate is finished | |||
| checkMutex.lock() | |||
There was a problem hiding this comment.
There was a crash from check() running at the same time as onDestroy()
| updateBalance(walletDataProvider.wallet!!.getBalance(Wallet.BalanceType.AVAILABLE)) | ||
| updateBalance(walletDataProvider.getWalletBalance()) |
There was a problem hiding this comment.
use the last calculated balance instead of recalculating it. Also use the total balance, rather than AVAILABLE, which will not include unconfirmed TX's.
| private val _totalBalance = MutableStateFlow(Coin.ZERO) | ||
| val totalBalance: StateFlow<Coin> | ||
| get() = _totalBalance | ||
|
|
||
| private val _mixedBalance = MutableStateFlow(Coin.ZERO) | ||
| val mixedBalance: StateFlow<Coin> | ||
| get() = _mixedBalance |
There was a problem hiding this comment.
manage these two balances (total and mixed) all the time.
There was a problem hiding this comment.
This will avoid ANR's when starting the app.
| lifecycleScope.launch { | ||
| val hasBalance = walletApplication.observeTotalBalance().first().isPositive | ||
| if (hasBalance && configuration.lastBackupSeedTime == 0L) { | ||
| configuration.setLastBackupSeedTime() | ||
| } |
There was a problem hiding this comment.
This will avoid an ANR when starting the app.
| workerScope.launch { | ||
| org.bitcoinj.core.Context.propagate(Constants.CONTEXT) | ||
| val allTxs = wallet.walletTransactions | ||
| if (allTxs.any()) { | ||
| var mostRecentTx = allTxs.first() | ||
| allTxs.forEach { | ||
| if (it.transaction.updateTime > mostRecentTx.transaction.updateTime) { | ||
| mostRecentTx = it | ||
| } | ||
| } | ||
| trySend(mostRecentTx.transaction) |
There was a problem hiding this comment.
Avoid an ANR when starting the app
| @@ -230,7 +230,7 @@ class WalletTransactionsFragment : Fragment(R.layout.wallet_transactions_fragmen | |||
| } | |||
|
|
|||
| private fun showEmptyView() { | |||
| binding.walletTransactionsEmpty.isVisible = true | |||
| binding.walletTransactionsEmpty.isVisible = viewModel.transactionsLoaded | |||
There was a problem hiding this comment.
Don't show "There are no transactions to display" if the app is still processing transactions to display.
| @@ -102,7 +102,7 @@ class TransactionGroupViewModel @Inject constructor( | |||
| TransactionRowView.fromTransaction( | |||
| it, walletData.wallet!!, walletData.wallet!!.context, txMetadata, null, resourceMapper, chainLockBlockHeight | |||
| ) | |||
| } | |||
| }.sortedBy { row -> row.time } | |||
There was a problem hiding this comment.
sort TX in a group by date.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt (1)
118-120: Great improvement in state management!Converting
authenticationGroupExtensionfrom a mutable property to a read-only property with a direct getter improves data consistency and reduces potential state-related bugs.
| suspend fun init() { | ||
| if (authenticationGroupExtension == null) { | ||
| if (!platformSDKLoaded) { | ||
| // load the dash-sdk library | ||
| System.loadLibrary("sdklib") | ||
| authenticationGroupExtension = walletApplication.wallet?.getKeyChainExtension(AuthenticationGroupExtension.EXTENSION_ID) as? AuthenticationGroupExtension | ||
| platformSDKLoaded = true | ||
| } | ||
|
|
||
| if (!hasIdentity) { | ||
| blockchainIdentityDataStorage.load()?.let { | ||
| blockchainIdentity = initBlockchainIdentity(it, walletApplication.wallet!!) | ||
| initializeStateRepository() | ||
| } | ||
| blockchainIdentityDataStorage.load()?.let { | ||
| blockchainIdentity = initBlockchainIdentity(it, walletApplication.wallet!!) | ||
| initializeStateRepository() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for SDK loading failures.
The SDK loading logic should handle potential failures and provide appropriate error feedback.
Apply this diff to add error handling:
suspend fun init() {
if (!platformSDKLoaded) {
- // load the dash-sdk library
- System.loadLibrary("sdklib")
- platformSDKLoaded = true
+ try {
+ // load the dash-sdk library
+ System.loadLibrary("sdklib")
+ platformSDKLoaded = true
+ } catch (e: UnsatisfiedLinkError) {
+ log.error("Failed to load SDK library", e)
+ throw RuntimeException("Failed to initialize Dash SDK", e)
+ }
}
blockchainIdentityDataStorage.load()?.let {
blockchainIdentity = initBlockchainIdentity(it, walletApplication.wallet!!)
initializeStateRepository()
}
}📝 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.
| suspend fun init() { | |
| if (authenticationGroupExtension == null) { | |
| if (!platformSDKLoaded) { | |
| // load the dash-sdk library | |
| System.loadLibrary("sdklib") | |
| authenticationGroupExtension = walletApplication.wallet?.getKeyChainExtension(AuthenticationGroupExtension.EXTENSION_ID) as? AuthenticationGroupExtension | |
| platformSDKLoaded = true | |
| } | |
| if (!hasIdentity) { | |
| blockchainIdentityDataStorage.load()?.let { | |
| blockchainIdentity = initBlockchainIdentity(it, walletApplication.wallet!!) | |
| initializeStateRepository() | |
| } | |
| blockchainIdentityDataStorage.load()?.let { | |
| blockchainIdentity = initBlockchainIdentity(it, walletApplication.wallet!!) | |
| initializeStateRepository() | |
| } | |
| } | |
| suspend fun init() { | |
| if (!platformSDKLoaded) { | |
| try { | |
| // load the dash-sdk library | |
| System.loadLibrary("sdklib") | |
| platformSDKLoaded = true | |
| } catch (e: UnsatisfiedLinkError) { | |
| log.error("Failed to load SDK library", e) | |
| throw RuntimeException("Failed to initialize Dash SDK", e) | |
| } | |
| } | |
| blockchainIdentityDataStorage.load()?.let { | |
| blockchainIdentity = initBlockchainIdentity(it, walletApplication.wallet!!) | |
| initializeStateRepository() | |
| } | |
| } |
| @@ -125,6 +126,7 @@ | |||
|
|
|||
| private val backgroundThread = HandlerThread("background", Process.THREAD_PRIORITY_BACKGROUND) | |||
| private val backgroundHandler: Handler | |||
| private var platformSDKLoaded = false | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the SDK loading flag thread-safe.
The platformSDKLoaded flag could be subject to race conditions in concurrent access scenarios.
Apply this diff to make the flag thread-safe:
- private var platformSDKLoaded = false
+ @Volatile
+ private var platformSDKLoaded = false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private var platformSDKLoaded = false | |
| @Volatile | |
| private var platformSDKLoaded = false |
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Refactor