-
Notifications
You must be signed in to change notification settings - Fork 172
fix(dashspend): metadata & barcode #1368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* fix: proguard for firebase * fix: last sync timestamp update * chore: remove developer mode * chore: ktlint
* fix: mxing limit & resource mapper * chore: cleanup * chore: coderabbit comments
* fix: prevent NPE if user goes to network monitor before blockchain service has started * fix: many fixes for username creation and restoration * fix: when creating username, properly mark requested name link when saving * fix: remove NPE's with wallet balance observers
* fix: upgrade to kotlin 2.0
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 pull request updates several dependency and plugin versions across Gradle files, modernizes Kotlin code by replacing deprecated methods and adding suspend modifiers, and refactors UI components. It removes obsolete developer mode and sensor functionality while streamlining error handling and state management in multiple modules. Additionally, it introduces new ProGuard rules, enhances coroutine support, and reorganizes layout files and resources for improved readability and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as GiftCardDetailsDialog
participant B as BottomSheetCallback
participant S as setMaxBrightness
U->>D: Opens Gift Card Details Dialog
D->>D: subscribeToBottomSheetCallback()
B->>D: onSlide(event) triggered
D->>S: Invoke setMaxBrightness(enable)
S-->>D: Brightness updated
D->>U: Reflects updated screen brightness
sequenceDiagram
participant U as User
participant R as RequestUsernameFragment
participant K as KeyboardUtil
participant VM as CreateIdentityViewModel
U->>R: Enters username & clicks submit
R->>R: onContinue() invoked
R->>K: Adjust keyboard visibility
R->>VM: Validate and process username
VM-->>R: Return updated UI state
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (20)
wallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt (2)
21-21
: Import statement looks redundantThe
android.content.Intent
import appears to be redundant since there's already an import forIntent.ACTION_VIEW
on line 22, and there's no explicit usage of the fullIntent
class name in the code.-import android.content.Intent
148-151
: Consider implementing unified transition animationsThe overridden
finish()
method applies a custom transition animation. Consider extracting these animation constants to a centralized location to ensure consistent transitions across the application. This would make maintenance easier if animation styles need to be changed in the future.integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/dialogs/StakingDialog.kt (1)
44-57
: Improved UI responsiveness by moving operations to background thread.Moved potentially blocking operations (like API calls or data processing) into a coroutine to prevent blocking the main thread during UI initialization, which enhances user experience by keeping the UI responsive.
Consider adding error handling within the coroutine:
lifecycleScope.launch { + try { binding.stakingMessageTwo.text = getString( R.string.crowdnode_staking_info_message_second, String.format(Locale.getDefault(), "%.1f", viewModel.getMasternodeAPY()) ) binding.stakingFirstMinDepositMessage.text = getString( R.string.crowdnode_staking_info_message, CrowdNodeConstants.DASH_FORMAT.format(CrowdNodeConstants.MINIMUM_DASH_DEPOSIT) ) binding.stakingApyTitle.text = getString( R.string.crowdnode_staking_apy_title, String.format(Locale.getDefault(), "%.1f", viewModel.getCrowdNodeAPY()) ) + } catch (e: Exception) { + // Handle error - maybe set default values or show an error message + Log.e("StakingDialog", "Failed to load staking information", e) + } }wallet/src/de/schildbach/wallet/ui/more/MoreFragment.kt (1)
299-309
: Good centralization of retry logicThe new
retry
method centralizes the logic for determining whether a new username is needed based on the error message, which improves maintainability.Consider extracting the error message patterns into constants or a companion object for better maintainability. This would make it easier to update or expand the list of error patterns in the future.
private fun retry(errorMessage: String) { - val needsNewName = errorMessage.contains("Document transitions with duplicate unique properties") || - errorMessage.contains("Document Contest for vote_poll ContestedDocumentResourceVotePoll") || - errorMessage.contains(Regex("does not have .* as a contender")) || - errorMessage.contains("missing domain document for ") + val needsNewName = errorMessage.contains(ERROR_DUPLICATE_PROPERTIES) || + errorMessage.contains(ERROR_DOCUMENT_CONTEST) || + errorMessage.contains(Regex(ERROR_CONTENDER_REGEX)) || + errorMessage.contains(ERROR_MISSING_DOMAIN) if (!needsNewName) { createIdentityViewModel.retryCreateIdentity() } else { startActivity(Intent(requireContext(), CreateUsernameActivity::class.java)) } }And add these constants to the companion object:
companion object { // ... existing constants private const val ERROR_DUPLICATE_PROPERTIES = "Document transitions with duplicate unique properties" private const val ERROR_DOCUMENT_CONTEST = "Document Contest for vote_poll ContestedDocumentResourceVotePoll" private const val ERROR_CONTENDER_REGEX = "does not have .* as a contender" private const val ERROR_MISSING_DOMAIN = "missing domain document for " }wallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.kt (1)
296-320
: Improved error handling by adding conditional processing for username existence.The new conditional block ensures that multiple identity state updates are only performed when a valid username exists. This change prevents potential NullPointerExceptions and improper state transitions when no username is available.
However, the sequence of state transitions (REQUESTED_NAME_CHECKED → REQUESTED_NAME_CHECKING → REQUESTED_NAME_CHECKED again → VOTING) seems potentially redundant. Consider documenting these transitions for better maintainability.
if (blockchainIdentity.currentUsername != null) { + // State transitions for identity with username: + // 1. Set initial checked state platformRepo.updateIdentityCreationState( blockchainIdentityData, BlockchainIdentityData.CreationState.REQUESTED_NAME_CHECKED ) platformRepo.updateBlockchainIdentityData(blockchainIdentityData, blockchainIdentity) + // 2. Set checking state to verify name platformRepo.updateIdentityCreationState( blockchainIdentityData, BlockchainIdentityData.CreationState.REQUESTED_NAME_CHECKING ) + // 3. Set checked state after verification link recovery // recover the verification link platformRepo.updateIdentityCreationState( blockchainIdentityData, BlockchainIdentityData.CreationState.REQUESTED_NAME_CHECKED ) platformRepo.updateBlockchainIdentityData(blockchainIdentityData, blockchainIdentity) + // 4. Set final voting state // set voting state platformRepo.updateIdentityCreationState( blockchainIdentityData, BlockchainIdentityData.CreationState.VOTING ) platformRepo.updateBlockchainIdentityData(blockchainIdentityData, blockchainIdentity) }wallet/res/layout/fragment_request_username.xml (5)
7-8
: Ensure consistent parent-child styling.You have set
android:background="@color/background_secondary"
on both the parentConstraintLayout
and thisTextView
. If the intention is to inherit the background from the parent, consider removing the redundant background on theTextView
to avoid possible overdraw or color mismatch.
18-18
: Confirm naming consistency.
top_stack
is a bit generic. Since it holds introductory text and instructions, you might rename it for clarity (e.g.,intro_stack
orheader_container
).
84-84
: Encapsulate dimensions in resources.Consider referencing a dimension resource (e.g.,
@dimen/min_height_input
) forandroid:minHeight="72dp"
to maintain consistency across the app and simplify future global adjustments.
206-210
: Set importantForAccessibility on ProgressBar if needed.If this
ProgressBar
indicates loading for visually impaired users, consider addingandroid:importantForAccessibility="yes"
and a content description or usingandroid:contentDescription="@string/loading"
to ensure screen readers announce it.
213-245
: Consider content descriptions for accessibility.The
ImageView
with@+id/check_voting_period
likely conveys a warning or status. Includeandroid:contentDescription
for screen readers. Also, ensure the text elements inside are fully descriptive if they represent time-sensitive or status-based content.Also applies to: 246-246
wallet/src/de/schildbach/wallet/ui/transactions/TxResourceMapper.kt (3)
37-41
: Use or remove thedateTimeFormat
property.Currently,
dateTimeFormat
is a public property with a default override but not used within this file. You may want to either provide usage or remove it if it’s unnecessary.
121-125
: Check for null or unknown errors.This block delegates to
getErrorName(error)
. Make sure that ifTxError.fromTransaction(tx)
returnsnull
, the code is safe. Consider a fallback or explicit handling if there's a possibility of a null result from that method.
147-177
: Enhance user feedback for unconfirmed states.You handle multiple conditions (ChainLocked, InstantSendLocked, etc.). If a transaction hovers between statuses, consider whether more granular user feedback would improve clarity, especially for partial confirmations or long wait times.
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (5)
68-68
: Avoid duplicate imports forStateFlow
.Line 67 already imports
StateFlow
. This secondimport
might be a merge artifact. Consider removing to keep imports clean.
200-201
: Track usage of stale-rate fields.New properties
currentStaleRateState
andrateStaleDismissed
appear to shape UI feedback on outdated rates. Ensure your UI binds torateStaleDismissed
if you intend to hide repeated stale-rate alerts.
556-561
: Update UI only on state changes.You’re setting
_transactions.value
and callinggetContactsAndMetadataForTransactions
after each refresh. Ensure that if there’s no actual data change, you don’t trigger repeated UI updates or expensive lookups.
667-669
: Maintain consistent transaction manipulation.Similar to lines 556-561, be mindful of extra updates. Batch operations can help minimize repeated calls to
_transactions
. Verify minimal duplication in the final logic.
742-743
: Watch floating-point precision.Calculating staking APY with
(100.0 - crowdNodeApi.getFee()) / 100
could cause rounding or precision issues at scale. Consider aBigDecimal
approach for improved accuracy.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt (2)
84-92
: Consider simplifying the bottom sheet callback implementation.The
onStateChanged
method is currently empty and flagged by static analysis. Consider using a lambda-style implementation for just theonSlide
method. Also, you might want to handle the case when the user slides the sheet back up after having slid it down past the -0.5 threshold.-private val bottomSheetCallback = object : BottomSheetBehavior.BottomSheetCallback() { - override fun onStateChanged(bottomSheet: View, newState: Int) { } - - override fun onSlide(bottomSheet: View, slideOffset: Float) { - if (slideOffset < -0.5) { - setMaxBrightness(false) - } - } -} +private val bottomSheetCallback = BottomSheetBehavior.BottomSheetCallback { _, newState -> + // No operation needed here +}.apply { + addOnSlideListener { _, slideOffset -> + if (slideOffset < -0.5) { + setMaxBrightness(false) + } else if (slideOffset >= -0.5 && originalBrightness >= 0) { + setMaxBrightness(true) + } + } +}🧰 Tools
🪛 detekt (1.23.7)
[warning] 85-85: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
256-270
: Ensure thread safety for window operations.The
setMaxBrightness
method accessesdialog?.window
which could potentially be null if the dialog is being dismissed. Consider adding a synchronized block or ensuring this method is only called from the main thread.private fun setMaxBrightness(enable: Boolean) { - val window = dialog?.window ?: return - val params = window.attributes + val dialog = dialog ?: return + requireActivity().runOnUiThread { + val window = dialog.window ?: return@runOnUiThread + val params = window.attributes - if (enable) { - if (originalBrightness < 0) { - originalBrightness = params.screenBrightness - } - params.screenBrightness = 1.0f - } else { - params.screenBrightness = originalBrightness - } + if (enable) { + if (originalBrightness < 0) { + originalBrightness = params.screenBrightness + } + params.screenBrightness = 1.0f + } else { + params.screenBrightness = originalBrightness + } - window.attributes = params + window.attributes = params + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
build.gradle
(2 hunks)common/build.gradle
(2 hunks)common/src/main/java/org/dash/wallet/common/Configuration.java
(0 hunks)common/src/main/java/org/dash/wallet/common/data/ExchangeRatesConfig.kt
(1 hunks)common/src/main/java/org/dash/wallet/common/services/BlockchainStateProvider.kt
(1 hunks)common/src/main/java/org/dash/wallet/common/transactions/TransactionUtils.kt
(0 hunks)common/src/main/java/org/dash/wallet/common/ui/avatar/UserAvatarPlaceholderDrawable.kt
(2 hunks)common/src/main/java/org/dash/wallet/common/util/TextViewExt.kt
(1 hunks)common/src/main/res/values/styles.xml
(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ExploreDatabase.kt
(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ExploreSyncWorker.kt
(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt
(8 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt
(8 hunks)features/exploredash/src/main/res/layout/dialog_gift_card_details.xml
(3 hunks)integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/CrowdNodeViewModel.kt
(2 hunks)integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/dialogs/StakingDialog.kt
(2 hunks)integrations/crowdnode/src/test/java/org/dash/wallet/integrations/crowdnode/CrowdNodeViewModelTest.kt
(1 hunks)wallet/AndroidManifest.xml
(0 hunks)wallet/build.gradle
(2 hunks)wallet/proguard.cfg
(1 hunks)wallet/res/layout/fragment_more.xml
(1 hunks)wallet/res/layout/fragment_request_username.xml
(5 hunks)wallet/res/values/dimens.xml
(1 hunks)wallet/res/values/strings.xml
(1 hunks)wallet/src/de/schildbach/wallet/WalletApplication.java
(4 hunks)wallet/src/de/schildbach/wallet/di/AppModule.kt
(2 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt
(1 hunks)wallet/src/de/schildbach/wallet/service/BlockchainStateDataProvider.kt
(3 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt
(2 hunks)wallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.kt
(2 hunks)wallet/src/de/schildbach/wallet/transactions/coinjoin/CoinJoinTxResourceMapper.kt
(2 hunks)wallet/src/de/schildbach/wallet/transactions/coinjoin/CoinJoinTxWrapperFactory.kt
(0 hunks)wallet/src/de/schildbach/wallet/ui/CreateUsernameActivity.kt
(1 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt
(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/GravatarProfilePictureDialog.kt
(1 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt
(7 hunks)wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt
(3 hunks)wallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt
(2 hunks)wallet/src/de/schildbach/wallet/ui/more/MoreFragment.kt
(3 hunks)wallet/src/de/schildbach/wallet/ui/notifications/NotificationManagerWrapper.kt
(2 hunks)wallet/src/de/schildbach/wallet/ui/transactions/CrowdNodeTxResourceMapper.kt
(1 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TransactionRowView.kt
(0 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TxError.kt
(2 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TxResourceMapper.java
(0 hunks)wallet/src/de/schildbach/wallet/ui/transactions/TxResourceMapper.kt
(1 hunks)wallet/src/de/schildbach/wallet/ui/username/voting/RequestUserNameViewModel.kt
(4 hunks)wallet/src/de/schildbach/wallet/ui/username/voting/RequestUsernameFragment.kt
(6 hunks)
💤 Files with no reviewable changes (6)
- common/src/main/java/org/dash/wallet/common/transactions/TransactionUtils.kt
- wallet/src/de/schildbach/wallet/transactions/coinjoin/CoinJoinTxWrapperFactory.kt
- wallet/src/de/schildbach/wallet/ui/transactions/TransactionRowView.kt
- wallet/AndroidManifest.xml
- common/src/main/java/org/dash/wallet/common/Configuration.java
- wallet/src/de/schildbach/wallet/ui/transactions/TxResourceMapper.java
🧰 Additional context used
🧬 Code Definitions (4)
wallet/src/de/schildbach/wallet/ui/username/voting/RequestUsernameFragment.kt (2)
common/src/main/java/org/dash/wallet/common/util/NavigationExtensions.kt (1)
safeNavigate
(30-40)wallet/src/de/schildbach/wallet/ui/username/voting/VerifyIdentityFragment.kt (1)
checkViewConfirmDialog
(109-120)
integrations/crowdnode/src/test/java/org/dash/wallet/integrations/crowdnode/CrowdNodeViewModelTest.kt (1)
integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/CrowdNodeViewModel.kt (1)
getMasternodeAPY
(412-419)
wallet/src/de/schildbach/wallet/service/BlockchainStateDataProvider.kt (2)
common/src/main/java/org/dash/wallet/common/services/BlockchainStateProvider.kt (1)
getMasternodeAPY
(39-39)integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/CrowdNodeViewModel.kt (1)
getMasternodeAPY
(412-419)
wallet/src/de/schildbach/wallet/WalletApplication.java (4)
common/src/main/java/org/dash/wallet/common/WalletDataProvider.kt (1)
wallet
(33-81)wallet/src/de/schildbach/wallet/transactions/WalletObserver.kt (1)
wallet
(40-179)wallet/src/de/schildbach/wallet/transactions/WalletMostRecentTransactionsObserver.kt (1)
wallet
(34-98)wallet/src/de/schildbach/wallet/transactions/WalletBalanceObserver.kt (1)
wallet
(45-189)
🪛 detekt (1.23.7)
wallet/src/de/schildbach/wallet/service/BlockchainStateDataProvider.kt
[warning] 209-209: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt
[warning] 85-85: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (86)
wallet/src/de/schildbach/wallet/ui/more/AboutActivity.kt (5)
43-44
: Class signature simplified with removal of SensorEventListener interfaceThe removal of the
SensorEventListener
interface from the class signature aligns with the broader changes indicated in the PR summary - the application no longer implements shake detection functionality for developer mode toggling. This simplification is a positive change as it removes unused functionality.
53-95
: onCreate method looks well-structured after removal of sensor setupThe
onCreate
method has been streamlined after removing the sensor setup code. It now focuses entirely on initializing the UI, setting up click listeners, and displaying application information. All functionality appears intact with appropriate error handling around the optional sync operation.
153-160
: Implementation of support contact is clear and well-structuredThe
handleReportIssue()
method appropriately checks if the activity is finishing before showing the dialog, which prevents potential window leaks. The dialog instantiation is also cleanly implemented.
97-107
: Firebase ID handling methods are well-encapsulatedThe
showFirebaseIds()
method properly follows MVVM architecture patterns by observing LiveData from the ViewModel and updating UI accordingly. The click listeners for copying IDs delegate to the ViewModel, maintaining a good separation of concerns.
109-146
: Explore Dash sync status display is thorough and covers edge casesThe
showExploreDashSyncStatus()
method handles all potential states of sync including: in-progress, failed attempts, preloaded data, and successful sync. This comprehensive approach ensures the user always has clear visibility into the sync state.wallet/src/de/schildbach/wallet/ui/transactions/TxError.kt (2)
20-21
: Added necessary imports for context propagation.These imports support the context propagation functionality that was added to the transaction error handling.
40-40
: Thread safety improvement with context propagation.Adding
Context.propagate(Constants.CONTEXT)
at the beginning of the method ensures that the BitcoinJ library operates with the correct context in the current thread. This is important for thread safety when working with BitcoinJ components and helps prevent potential runtime errors.wallet/src/de/schildbach/wallet/ui/dashpay/GravatarProfilePictureDialog.kt (1)
53-53
: Updated to modern Kotlin API.Replaced the deprecated Java-style
toLowerCase()
with the Kotlin-idiomaticlowercase()
function. This modernization maintains the same functionality while following current Kotlin best practices.wallet/proguard.cfg (1)
254-259
:✅ Verification successful
Added Firebase-related ProGuard rules.
These rules prevent obfuscation of Firebase classes, which is essential for proper functionality of Firebase services in the application. This is particularly important for Firebase Authentication, as indicated by the specific rules for auth-related packages.
These rules should help prevent runtime errors related to obfuscated Firebase components. Consider verifying if all required Firebase components are covered by these rules.
🏁 Script executed:
#!/bin/bash # Check if Firebase dependencies are explicitly declared in the project grep -r "com.google.firebase" --include="*.gradle" .Length of output: 1518
Firebase ProGuard rules verified & appear comprehensive
The updated rules in
wallet/proguard.cfg
(lines 254-259) correctly prevent obfuscation of all Firebase components declared across the project. In particular:
- The general rule
-keep class com.google.firebase.** { *; }
ensures that all Firebase classes (including modules such as auth, database, analytics, in-app messaging, messaging, dynamic links, etc.) are preserved.- The inclusion of
-keep class com.google.android.gms.** { *; }
covers related Google services dependencies.- The specific rules for Firebase Authentication (
-keepnames class com.google.firebase.auth.**
and-keep interface com.google.firebase.auth.** { *; }
) address potential issues related to auth component obfuscation.The shell script output confirms that the declared Firebase dependencies (from modules like
wallet
,common
, andfeatures/exploredash
) are adequately covered by these rules.Approved – no further modifications required.
wallet/res/values/strings.xml (1)
181-181
: Added string resource for CoinJoin Combine Dust transactions.This new string resource enhances user understanding by providing a specific label for CoinJoin transactions that combine small amounts (dust) into more practical amounts.
The addition fits properly within the existing series of CoinJoin-related status strings in the file.
wallet/res/values/dimens.xml (1)
21-22
: LGTM: Added new dimension resourcesThe new dimension resources provide useful values that can be referenced across the application layouts: a negative value for shifting UI elements and a zero constant for explicit zero-dimension declarations.
integrations/crowdnode/src/test/java/org/dash/wallet/integrations/crowdnode/CrowdNodeViewModelTest.kt (1)
91-91
: Properly updated test mock for suspend functionThe change from
on
toonBlocking
correctly updates the test to handle thegetMasternodeAPY()
method that has been modified to be a suspend function in the actual implementation.common/src/main/res/values/styles.xml (1)
168-175
: LGTM: Added new style without line constraintsThis new style appropriately inherits from
Overline.Tertiary
and provides the same styling asMenuRowSubTitle
but without the line constraint, allowing for multi-line text display when needed.wallet/src/de/schildbach/wallet/di/AppModule.kt (1)
126-130
: LGTM: Added application-wide coroutine scopeGood implementation of a singleton application scope using
SupervisorJob
andDispatchers.Default
. This provides a structured way to manage coroutines across the application's lifecycle.The
SupervisorJob
ensures that failures in child coroutines won't affect the entire scope, which is appropriate for an application-level scope.wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
1435-1435
: Improved null safety with safe call operator.Replacing the non-null assertion operator (
!!
) with a safe call operator (?.
) makes this code more defensive against potentialNullPointerException
. This change allows the method to gracefully handle cases whereblockChain
is null, returning an empty list instead of crashing.wallet/src/de/schildbach/wallet/ui/CreateUsernameActivity.kt (1)
43-43
: Removed unused view model dependency.The removal of
confirmTransactionSharedViewModel
cleans up unnecessary dependencies. This is a good change that eliminates unused code, potentially reducing memory usage and preventing initialization overhead.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ExploreSyncWorker.kt (1)
156-156
: Improved data consistency when updating database preferences.Refreshing the
databasePrefs
before saving the updated values ensures you're working with the most current data. This prevents potential race conditions and avoids overwriting any changes that might have occurred during sync operations.wallet/res/layout/fragment_more.xml (1)
233-238
: Updated text style and improved design-time preview.Switching to
MenuRowSubTitleWithoutLines
style improves the visual presentation, while adding thetools:text
attribute enhances the design-time preview for developers by showing what error messages might look like.wallet/build.gradle (2)
10-10
: Kotlin Compose plugin added to support Compose features.The addition of this plugin is necessary to support Jetpack Compose for UI development, aligning with modern Android development practices.
98-98
:❓ Verification inconclusive
Compose BOM version updated to 2024.03.00.
The Compose BOM version has been updated from 2023.03.00 to 2024.03.00, which provides access to the latest Compose features and bug fixes.
🏁 Script executed:
#!/bin/bash # Check if the updated Compose BOM version is compatible with the Kotlin version cat gradle.properties | grep kotlin_versionLength of output: 45
Action Required: Confirm Kotlin Compatibility
- The Compose BOM version update to 2024.03.00 appears to be correct and should enable access to the latest Compose features and bug fixes.
- However, the initial script to check for a
kotlin_version
ingradle.properties
returned no output. This suggests that the Kotlin version might be defined elsewhere or not at all in that file.- Please manually verify whether the project explicitly sets the Kotlin version (for example, in any root or submodule
build.gradle
files orsettings.gradle
) and confirm that it is compatible with Compose BOM version 2024.03.00.common/src/main/java/org/dash/wallet/common/ui/avatar/UserAvatarPlaceholderDrawable.kt (2)
34-34
: Updated to use modern Kotlin API for character case conversion.Replaced deprecated
toUpperCase()
with the neweruppercaseChar()
method, which is the recommended approach in Kotlin.
48-48
: Updated string uppercase method to use modern Kotlin API.Replaced deprecated
toUpperCase()
with the neweruppercase()
method, which is the recommended approach in Kotlin.wallet/src/de/schildbach/wallet/ui/notifications/NotificationManagerWrapper.kt (2)
25-25
: Added Bitmap import to support explicit type casting.The imported Bitmap class is necessary for the explicit type casting below.
99-99
: Added explicit type casting for bigLargeIcon parameter.The null parameter now has an explicit
as Bitmap?
cast to improve type safety and compatibility with the newer Kotlin compiler.integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/dialogs/StakingDialog.kt (1)
24-26
: Added coroutine-related imports to support asynchronous operations.These imports enable the use of lifecycleScope and launch for moving potentially blocking operations off the main thread.
common/src/main/java/org/dash/wallet/common/util/TextViewExt.kt (1)
47-48
: Updated to use non-deprecated lowercase method.The code now correctly uses the modern
lowercase()
method instead of the deprecatedtoLowerCase()
. This change aligns with Kotlin's current best practices for string manipulation.common/src/main/java/org/dash/wallet/common/data/ExchangeRatesConfig.kt (1)
8-10
: LGTM: Added singleton annotation.Adding the
@Singleton
annotation ensures that only one instance ofExchangeRatesConfig
will be created and shared across the application. This is appropriate for configuration classes that should maintain consistent state.wallet/src/de/schildbach/wallet/ui/transactions/CrowdNodeTxResourceMapper.kt (1)
29-31
: Refactored method to property for better Kotlin idioms.The change from
override fun getDateTimeFormat(): Int
tooverride val dateTimeFormat: Int
with a getter follows Kotlin's idiomatic approach to properties. This improves code readability while maintaining the same functionality.features/exploredash/src/main/res/layout/dialog_gift_card_details.xml (3)
144-144
: Made barcode placeholder visible by default.Changed the barcode placeholder visibility from
gone
tovisible
, ensuring it's always shown until the actual barcode is loaded. This provides better visual feedback to users during loading states.
181-181
: Updated error visibility in design-time preview.Changed the design-time visibility of the barcode loading error to
gone
, making it consistent with its runtime behavior and improving the accuracy of the layout preview in Android Studio.
325-325
: Hide card error by default.Added
android:visibility="gone"
to ensure the card error is hidden by default, which aligns with the PR objective of tightening the UI around metadata handling. This change supports the improved error state management described in the PR summary.common/src/main/java/org/dash/wallet/common/services/BlockchainStateProvider.kt (1)
39-40
:❓ Verification inconclusive
API change: Methods are now suspend functions
The
getMasternodeAPY()
andgetLastMasternodeAPY()
methods have been converted to suspend functions. This is a significant API change that will require any calling code to invoke these methods from within a coroutine scope.This change aligns with the PR objective of "tightening threading around the updating of metadata" and ensures these potentially long-running operations don't block the main thread.
🏁 Script executed:
#!/bin/bash # Find all direct usages of these methods to ensure they're called from coroutine contexts echo "Searching for usages of getMasternodeAPY() and getLastMasternodeAPY()" rg "getMasternodeAPY\(\)" --type kotlin rg "getLastMasternodeAPY\(\)" --type kotlinLength of output: 2132
Attention: Suspension API update requires verifying coroutine usage
The API methods have now been converted to suspend functions, so all invocations must occur from within a coroutine context. Our search confirmed that the updated methods are used in various modules (e.g., in MainViewModel.kt, BlockchainStateDataProvider.kt, and CrowdNodeViewModel.kt). However, note that in the CrowdNode integration—specifically in
integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/dialogs/StakingDialog.kt
—the call togetMasternodeAPY()
is used inline viaString.format(…)
. Please ensure that this (and any similar usage) is wrapped in a proper coroutine context to avoid runtime issues.
- Verify that all direct calls (particularly within UI/dialog code) occur in coroutine scopes.
- Ensure tests mocking these suspend functions handle suspending behavior appropriately.
wallet/src/de/schildbach/wallet/ui/username/voting/RequestUserNameViewModel.kt (3)
69-69
: Improved UI state tracking for username verificationAdded a new state flag to track when a username check is in progress. This is a good addition that will help prevent multiple concurrent checks and enable the UI to show loading indicators.
250-252
: Added state reset functionalityThe new
reset()
method provides a clean way to reset the UI state to its initial values. This is useful for form resets or when navigating away from and back to the username request screen.
298-298
: Properly managing loading state during username checksThe
checkingUsername
flag is now properly set at the beginning and end of the username checking process, ensuring the UI can accurately reflect the loading state.Also applies to: 329-329
common/build.gradle (1)
6-6
:✅ Verification successful
Updated Compose dependencies and configuration
Added the Kotlin Compose plugin and updated the Compose BOM to version 2024.03.00. This ensures the project uses the latest Jetpack Compose features and optimizations.
Also applies to: 81-81
🏁 Script executed:
#!/bin/bash # Check if the compose plugin is also applied in other modules echo "Checking for Kotlin Compose plugin in other Gradle files" rg "org\.jetbrains\.kotlin\.plugin\.compose" --type gradle echo "Checking for other Compose BOM versions that may need updating" rg "androidx\.compose:compose-bom:" --type gradleLength of output: 788
Compose Plugin and BOM Update Verified
The verification confirms that the Kotlin Compose plugin and the Compose BOM are updated consistently across the project. Both
common/build.gradle
andwallet/build.gradle
now apply the Kotlin Compose plugin and use the Compose BOM version 2024.03.00, ensuring that the latest features and optimizations are available.
- common/build.gradle: Confirmed
apply plugin: 'org.jetbrains.kotlin.plugin.compose'
at line 6.- wallet/build.gradle: Also applies the Compose plugin and sets the BOM to 2024.03.00.
No further changes are required.
wallet/src/de/schildbach/wallet/transactions/coinjoin/CoinJoinTxResourceMapper.kt (2)
31-32
: Refactored method to property overrideReplaced the
getDateTimeFormat()
method with a property override, which is a more idiomatic Kotlin approach. This improves code readability while maintaining the same functionality.
49-49
:✅ Verification successful
Added support for CombineDust transaction type
Added handling for a new CoinJoin transaction type:
CombineDust
, which will now display the appropriate label in the UI.
🏁 Script executed:
#!/bin/bash # Verify that the string resource exists for the new transaction type echo "Checking for the CoinJoin Combine Dust string resource" rg "transaction_row_status_coinjoin_combine_dust" --type xmlLength of output: 304
Verified: CombineDust Transaction Type Support is Correct
The new
CombineDust
case inCoinJoinTxResourceMapper.kt
correctly maps to the string resource, and the corresponding resource exists inwallet/res/values/strings.xml
. No further changes are required.build.gradle (2)
3-10
: Library version updates look goodThe updates to Kotlin (2.1.0), Dagger Hilt (2.53), and Hilt Work (1.2.0) bring the project up to more recent versions.
Since the dependency on dpp has been updated to a SNAPSHOT version (
1.7.4-SNAPSHOT
), verify that this version is stable enough for production use or that there's a plan to update to a stable release before deployment.
66-67
: Good plugin version alignment with KotlinThe KSP plugin version has been updated to match the new Kotlin version, and the Kotlin Compose plugin has been added, which aligns with modern Android development practices.
wallet/src/de/schildbach/wallet/ui/more/MoreFragment.kt (2)
188-193
: Improved error handling flowThe changes improve the error handling by storing the error message in a local variable and using a more precise condition check before calling the retry method.
288-289
: Better error handling with fallbackThe retrieval of the error message with a fallback to an empty string prevents potential null pointer exceptions and ensures consistent behavior.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ExploreDatabase.kt (1)
129-159
: Improved code structure and error handlingThe refactored
onOpen
callback has:
- Reduced nesting levels
- Improved error handling with a cleaner try-catch block
- Maintained the same functionality with a more readable structure
These changes make the code more maintainable while preserving the original behavior.
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (2)
219-219
: Enhanced logging with more contextAdding the current
lastPreBlockStage
to the log message provides more context about the state of the process when this message is generated, which will be helpful for debugging.
234-235
: Improved null safety with defensive programmingThis change enhances robustness by safely handling potential null values for
votingPeriodStart
with a default value of0L
, preventing potentialNullPointerException
errors.wallet/src/de/schildbach/wallet/WalletApplication.java (4)
1126-1127
: Enhanced null-checking to prevent potential NullPointerExceptions.The update adds a check for
walletBalanceObserver == null
alongside the existing wallet null check, improving robustness by ensuring both components are available before proceeding.
1136-1137
: Added defensive null check for wallet balance observer.Similar to other balance observer methods, this change prevents potential crashes by checking if the balance observer is properly initialized before attempting to use it.
1149-1150
: Improved error handling in observeBalance method.This change enhances the robustness of the balance observation functionality by ensuring both the wallet and its observer are available before attempting to observe balances.
1159-1160
: Added null check for walletBalanceObserver in observeSpendableBalance.Consistent with the pattern applied to other balance observation methods, this change prevents potential NullPointerExceptions when the wallet balance observer hasn't been initialized yet.
integrations/crowdnode/src/main/java/org/dash/wallet/integrations/crowdnode/ui/CrowdNodeViewModel.kt (2)
421-424
: Made getCrowdNodeAPY a suspend function for coroutine compatibility.This update ensures consistency with the
getMasternodeAPY()
method it depends on, allowing both methods to be called from coroutine contexts for asynchronous execution.
412-419
:✅ Verification successful
Updated method signature to support coroutine-based execution.
The
getMasternodeAPY()
method has been converted to a suspend function, which aligns with modern Kotlin asynchronous programming patterns and allows it to be called from coroutine contexts.This change likely coordinates with updates in the
BlockchainStateProvider
interface where the methods being called (getMasternodeAPY()
andgetLastMasternodeAPY()
) have also been updated to be suspend functions.
🏁 Script executed:
#!/bin/bash # Check if the BlockchainStateProvider interface has been updated to use suspend functions rg -A 5 "interface BlockchainStateProvider" | grep -A 10 "getMasternodeAPY" || echo "Not found"Length of output: 406
Update Approved: Coroutine-based API changes verified
The updated
getMasternodeAPY()
method inCrowdNodeViewModel.kt
now correctly supports coroutine-based execution. Verification confirms that bothgetMasternodeAPY()
andgetLastMasternodeAPY()
in theBlockchainStateProvider
interface are suspend functions, aligning well with this update.
- The method conversion to a suspend function is appropriate.
- The interface functions being invoked are correctly updated to suspend functions.
wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt (2)
737-737
: Improved state tracking for username verification link handling.Changed the state transition from
REQUESTED_NAME_CHECKED
toREQUESTED_NAME_LINK_SAVING
to provide clearer tracking of when the system is saving a verification link.This state transition provides better granularity in monitoring the identity creation workflow, making debugging and error tracking more effective.
753-753
: Added intermediate state to track verification link saving completion.The state transition from
REQUESTED_NAME_LINK_SAVING
toREQUESTED_NAME_LINK_SAVED
provides a clearer indication that the verification link has been successfully saved.This change improves the state management flow by making it more explicit and traceable, which enhances maintainability and debugging capabilities.
wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (2)
57-57
: Added important utility import for view observation.The addition of this import enables the use of the
observe
extension function, which is now being used in the transaction list update logic.
159-179
: Great improvement to fragment lifecycle handling!This change adds a crucial lifecycle safety check when updating the transaction list. By capturing the current lifecycle state and verifying both that the fragment is still attached (
isAdded
) and in at least the STARTED state before showing the transaction list, you've prevented potential crashes that could occur if the fragment was detached during the async list update.This is a common source of bugs in Android applications, and this defensive programming approach properly handles the race condition between async data updates and fragment lifecycle changes.
wallet/src/de/schildbach/wallet/ui/username/voting/RequestUsernameFragment.kt (7)
7-8
: Added necessary imports for new UI functionality.These imports support the keyboard handling and layout adjustments implemented in this update.
41-41
: Added keyboard utility for improved user experience.This new property will be used to manage keyboard visibility and automatically adjust the UI layout accordingly.
72-78
: Enhanced input handling with keyboard action support.This improvement allows users to submit the form using the keyboard's action button when the continue button is enabled, providing a more intuitive experience for keyboard users.
86-86
: Code cleanup through method extraction.Replacing the inline logic with a call to the extracted
onContinue()
method improves code organization and maintainability.
107-108
: Enhanced UI feedback during username checking.These visibility changes provide better visual feedback to users during the username checking process:
- The progress indicator is shown while checking
- The voting period container is hidden during checking and shown when appropriate
This creates a more responsive and informative user experience.
185-195
: Improved keyboard interaction with automatic layout adjustments.This implementation adjusts the top margin of the UI when the keyboard appears, creating a better visual experience by ensuring content remains properly visible even with the keyboard open.
271-305
: Good refactoring: Extracted complex logic into a dedicated method.Moving this logic from the button click handler to a separate method improves code organization and enables reuse from multiple entry points (button click and keyboard action). The method handles the different flows for contestable usernames versus regular usernames in a clear, structured way.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt (6)
51-57
: Excellent architectural improvement: Consolidated UI state into a data class.This data class creates a single source of truth for the UI state, which will improve state management and testability. Using a data class with immutable properties ensures that state changes are explicit and traceable.
61-61
: Good separation of concerns: Using applicationScope for long-running operations.Injecting the application-scoped coroutine allows long-running operations to continue even if the ViewModel is cleared, which is appropriate for operations like network requests that should complete regardless of UI state.
79-80
: Architectural improvement: Implemented StateFlow for UI state management.Converting from individual LiveData properties to a StateFlow with a single state object:
- Creates a more predictable state update pattern
- Avoids potential race conditions between multiple state properties
- Makes state transitions more explicit and easier to debug
92-101
: Improved state management using immutable updates.Using
_uiState.update { ... }
withcurrentState.copy()
ensures state updates are atomic and preserve all other state properties. This pattern prevents state inconsistencies that could occur with multiple separate property updates.
108-120
: Good optimization: Reusing barcode when value hasn't changed.The conditional update for barcode that only creates a new Barcode object when the value has changed helps optimize performance by avoiding unnecessary object creation and potential rendering.
234-234
: Appropriate usage of applicationScope for network operations.Moving the barcode fetching to the application scope ensures the operation can complete even if the view is destroyed, which is appropriate for this kind of background task that should complete regardless of UI state.
wallet/src/de/schildbach/wallet/service/BlockchainStateDataProvider.kt (1)
157-166
: Good performance improvement: Added suspend modifier for coroutine support.Converting
getLastMasternodeAPY()
to a suspend function withwithContext
improves application performance by moving the potentially heavy calculation to a background thread. This aligns with the interface changes observed in related files and follows modern Kotlin coroutine practices.wallet/res/layout/fragment_request_username.xml (2)
28-28
: Double-check duplication.You already set
@color/background_secondary
on the parent container at line 7. Confirm overlapping backgrounds are intentional or remove the redundant background here.
200-201
: FrameLayout usage appears valid.You’ve introduced a
FrameLayout
to group and layer the voting progress indicator and the voting container. This is a clear pattern for overlaying views. No issues spotted.wallet/src/de/schildbach/wallet/ui/transactions/TxResourceMapper.kt (3)
47-115
: Validate coinjoin logic and transaction states.This function branches heavily based on transaction types and coinjoin checks. Ensure all new
Transaction.Type
cases are thoroughly tested. Also confirm that any newly introduced masternode or identity-related transaction types are handled properly to avoid silent failures.
127-141
: Ensure full coverage on error enumerations.If new
TxError
enums are added later, remember to update this function. Otherwise, unknown errors might be silently mapped toTxError.Unknown
.
188-195
: Cross-check “Sending” condition logic.This condition might cause false positives if broadcast peers are temporarily zero or if transaction confidence updates are delayed. Consider verifying states in real usage to prevent incorrectly labeling a transaction as "sending."
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (4)
156-156
: Examine limited concurrency.You used
Dispatchers.IO.limitedParallelism(1)
. This ensures only one concurrent IO operation in yourviewModelWorkerScope
. Confirm that artificially limiting concurrency won’t cause performance issues (e.g., when fetching multiple resources in parallel).
163-164
: Migration toStateFlow
is consistent.Your shift from
LiveData
toStateFlow
is correct for reactive stream handling. Please confirm UI observers are updated to handle flows properly if you removed anyasLiveData()
usage in other parts of the app.
671-671
: Confirm correct scoping for additional calls.This function calls
viewModelWorkerScope.launch { ... }
. If external calls or canceled jobs rely on main-thread interactions, ensure you handle them on the correct dispatcher or coordinate to avoid race conditions.
676-677
: Avoid blocking main flow.Using
viewModelWorkerScope.launch
for contactingplatformRepo.blockchainIdentity.getContactForTransaction()
is good. Confirm that all UI states remain consistent if the user navigates away and the coroutine is still running.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt (8)
82-83
: Good addition for screen brightness management.Adding the
originalBrightness
variable to store the initial screen brightness allows you to restore it properly when the barcode is no longer displayed, preventing the screen from staying at maximum brightness.
113-166
: Excellent refactoring to use a unified UI state.The transition from observing multiple individual properties to a single
uiState
object is a significant improvement. This consolidated approach simplifies the code, makes state management more predictable, and ensures that UI updates are synchronized.
176-176
: Good addition of bottom sheet callback.Adding the bottom sheet callback subscription helps control screen brightness when the user interacts with the bottom sheet.
194-194
: Good use of extension function.Using the
toUri()
extension function on the URL string is cleaner than creating a URI directly.
200-236
: Excellent barcode generation implementation.The barcode generation logic has been well-implemented with:
- Proper background threading using coroutines
- Appropriate error handling with logging
- Good UI feedback for success/failure states
- Responsive sizing based on the device's screen dimensions
This directly addresses one of the PR objectives to ensure barcodes are always rendered from barcode data.
238-254
: Good implementation of barcode display with brightness control.The implementation properly:
- Displays the barcode using Coil's image loading library
- Sets maximum brightness on successful loading
- Provides visual feedback in case of loading errors
This addresses the PR objective of ensuring 100% brightness when displaying barcodes.
272-279
: Good implementation of callback subscription.The method properly finds the bottom sheet view and sets up the callback. This is essential for tracking bottom sheet interactions to manage screen brightness.
281-284
: Essential cleanup implementation.The overridden
dismiss
andonDestroyView
methods now properly:
- Reset screen brightness to its original value
- Remove the bottom sheet callback to prevent memory leaks
- Call their respective super methods
This ensures that resources are properly cleaned up when the dialog is dismissed or destroyed.
Also applies to: 286-293
override suspend fun getMasternodeAPY(): Double { | ||
return withContext(Dispatchers.Default) { | ||
val masternodeListManager = dashSystemService.system.masternodeListManager | ||
val blockChain = dashSystemService.system.blockChain | ||
if (masternodeListManager != null && blockChain != null) { | ||
val mnlist = masternodeListManager.listAtChainTip | ||
if (mnlist.height != 0L) { | ||
var prevBlock = try { | ||
blockChain.blockStore.get(mnlist.height.toInt() - 1) | ||
} catch (e: BlockStoreException) { | ||
null | ||
} | ||
// if we cannot retrieve the previous block, use the chain tip | ||
if (prevBlock == null) { | ||
prevBlock = blockChain.chainHead | ||
} | ||
|
||
val validMNsCount = if (mnlist.size() != 0) { | ||
var virtualMNCount = 0 | ||
mnlist.forEachMN(true) { entry -> | ||
virtualMNCount += if (entry.isHPMN) 4 else 1 | ||
val validMNsCount = if (mnlist.size() != 0) { | ||
var virtualMNCount = 0 | ||
mnlist.forEachMN(true) { entry -> | ||
virtualMNCount += if (entry.isHPMN) 4 else 1 | ||
} | ||
virtualMNCount | ||
} else { | ||
MASTERNODE_COUNT | ||
} | ||
virtualMNCount | ||
} else { | ||
MASTERNODE_COUNT | ||
} | ||
|
||
if (prevBlock != null) { | ||
val apy = getMasternodeAPY( | ||
walletDataProvider.wallet!!.params, | ||
mnlist.height.toInt(), | ||
prevBlock.header.difficultyTarget, | ||
validMNsCount | ||
) | ||
configuration.prefsKeyCrowdNodeStakingApy = apy.toFloat() | ||
return apy | ||
if (prevBlock != null) { | ||
val apy = getMasternodeAPY( | ||
walletDataProvider.wallet!!.params, | ||
mnlist.height.toInt(), | ||
prevBlock.header.difficultyTarget, | ||
validMNsCount | ||
) | ||
configuration.prefsKeyCrowdNodeStakingApy = apy.toFloat() | ||
return@withContext apy | ||
} | ||
} | ||
} | ||
return@withContext configuration.prefsKeyCrowdNodeStakingApy.toDouble() | ||
} | ||
return configuration.prefsKeyCrowdNodeStakingApy.toDouble() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance improvement with potential exception handling issue.
Converting getMasternodeAPY()
to a suspend function and moving the execution to Dispatchers.Default is a good performance optimization. However, the caught BlockStoreException on line 209 is silently swallowed without logging or proper handling.
Improve the exception handling by logging the exception before falling back to the default path:
try {
blockChain.blockStore.get(mnlist.height.toInt() - 1)
} catch (e: BlockStoreException) {
+ log.error("Failed to get previous block from blockstore", e)
null
}
📝 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.
override suspend fun getMasternodeAPY(): Double { | |
return withContext(Dispatchers.Default) { | |
val masternodeListManager = dashSystemService.system.masternodeListManager | |
val blockChain = dashSystemService.system.blockChain | |
if (masternodeListManager != null && blockChain != null) { | |
val mnlist = masternodeListManager.listAtChainTip | |
if (mnlist.height != 0L) { | |
var prevBlock = try { | |
blockChain.blockStore.get(mnlist.height.toInt() - 1) | |
} catch (e: BlockStoreException) { | |
null | |
} | |
// if we cannot retrieve the previous block, use the chain tip | |
if (prevBlock == null) { | |
prevBlock = blockChain.chainHead | |
} | |
val validMNsCount = if (mnlist.size() != 0) { | |
var virtualMNCount = 0 | |
mnlist.forEachMN(true) { entry -> | |
virtualMNCount += if (entry.isHPMN) 4 else 1 | |
val validMNsCount = if (mnlist.size() != 0) { | |
var virtualMNCount = 0 | |
mnlist.forEachMN(true) { entry -> | |
virtualMNCount += if (entry.isHPMN) 4 else 1 | |
} | |
virtualMNCount | |
} else { | |
MASTERNODE_COUNT | |
} | |
virtualMNCount | |
} else { | |
MASTERNODE_COUNT | |
} | |
if (prevBlock != null) { | |
val apy = getMasternodeAPY( | |
walletDataProvider.wallet!!.params, | |
mnlist.height.toInt(), | |
prevBlock.header.difficultyTarget, | |
validMNsCount | |
) | |
configuration.prefsKeyCrowdNodeStakingApy = apy.toFloat() | |
return apy | |
if (prevBlock != null) { | |
val apy = getMasternodeAPY( | |
walletDataProvider.wallet!!.params, | |
mnlist.height.toInt(), | |
prevBlock.header.difficultyTarget, | |
validMNsCount | |
) | |
configuration.prefsKeyCrowdNodeStakingApy = apy.toFloat() | |
return@withContext apy | |
} | |
} | |
} | |
return@withContext configuration.prefsKeyCrowdNodeStakingApy.toDouble() | |
} | |
return configuration.prefsKeyCrowdNodeStakingApy.toDouble() | |
} | |
override suspend fun getMasternodeAPY(): Double { | |
return withContext(Dispatchers.Default) { | |
val masternodeListManager = dashSystemService.system.masternodeListManager | |
val blockChain = dashSystemService.system.blockChain | |
if (masternodeListManager != null && blockChain != null) { | |
val mnlist = masternodeListManager.listAtChainTip | |
if (mnlist.height != 0L) { | |
var prevBlock = try { | |
blockChain.blockStore.get(mnlist.height.toInt() - 1) | |
} catch (e: BlockStoreException) { | |
log.error("Failed to get previous block from blockstore", e) | |
null | |
} | |
// if we cannot retrieve the previous block, use the chain tip | |
if (prevBlock == null) { | |
prevBlock = blockChain.chainHead | |
} | |
val validMNsCount = if (mnlist.size() != 0) { | |
var virtualMNCount = 0 | |
mnlist.forEachMN(true) { entry -> | |
virtualMNCount += if (entry.isHPMN) 4 else 1 | |
} | |
virtualMNCount | |
} else { | |
MASTERNODE_COUNT | |
} | |
if (prevBlock != null) { | |
val apy = getMasternodeAPY( | |
walletDataProvider.wallet!!.params, | |
mnlist.height.toInt(), | |
prevBlock.header.difficultyTarget, | |
validMNsCount | |
) | |
configuration.prefsKeyCrowdNodeStakingApy = apy.toFloat() | |
return@withContext apy | |
} | |
} | |
} | |
return@withContext configuration.prefsKeyCrowdNodeStakingApy.toDouble() | |
} | |
} |
🧰 Tools
🪛 detekt (1.23.7)
[warning] 209-209: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
} | ||
|
||
private fun updateSyncStatus(state: BlockchainState) { | ||
private suspend fun updateSyncStatus(state: BlockchainState) { | ||
if (_isBlockchainSynced.value != state.isSynced()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Change in function signature.
private suspend fun updateSyncStatus(state: BlockchainState)
might influence call sites. Make sure all invocation points can handle suspension if this was previously a non-suspending method.
fun getMasternodeAPY(): Double | ||
fun getLastMasternodeAPY(): Double | ||
suspend fun getMasternodeAPY(): Double | ||
suspend fun getLastMasternodeAPY(): Double |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withContext
call moved lower the stack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. also tested this to ensure that it works on my devices.
Gift card metadata is missing after relaunch. Some improvements are needed in the Gift Card details screen.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Style