fix(dashpay): improve restore username #1361
Conversation
WalkthroughThis pull request introduces several changes across the project. The dependency version in the Gradle build file was updated to a snapshot release, and a new UI style was added. Adjustments to XML resources include updated styles and string attributes. The Android manifest has a service declaration removed. In addition, various Java and Kotlin files received enhanced null safety, refined logging, and updated control flow—especially in identity creation, restoration, and transaction display—ensuring that the application handles error scenarios and lifecycle conditions more robustly. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MF as MoreFragment
participant VM as CreateIdentityViewModel
participant CA as CreateUsernameActivity
U->>MF: Tap on Request/Retrial
MF->>MF: Execute retry(errorMessage)
alt New username required?
MF->>CA: Start CreateUsernameActivity
else
MF->>VM: retryCreateIdentity()
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
| <!-- WorkManager --> | ||
| <service | ||
| android:name="androidx.work.impl.foreground.SystemForegroundService" | ||
| android:foregroundServiceType="dataSync" | ||
| android:exported="false" /> | ||
|
|
There was a problem hiding this comment.
This was duplicated
| @@ -1432,7 +1432,7 @@ class BlockchainServiceImpl : LifecycleService(), BlockchainService { | |||
| override fun getRecentBlocks(maxBlocks: Int): List<StoredBlock> { | |||
| val blocks: MutableList<StoredBlock> = ArrayList(maxBlocks) | |||
| try { | |||
| var block = blockChain!!.chainHead | |||
| var block = blockChain?.chainHead | |||
There was a problem hiding this comment.
This should prevent a crash on the Network Monitor | Blocks.
| if (blockchainIdentity.currentUsername != null) { | ||
|
|
||
| platformRepo.updateIdentityCreationState( | ||
| blockchainIdentityData, | ||
| BlockchainIdentityData.CreationState.REQUESTED_NAME_CHECKED | ||
| ) | ||
| platformRepo.updateBlockchainIdentityData(blockchainIdentityData, blockchainIdentity) | ||
| platformRepo.updateIdentityCreationState( | ||
| blockchainIdentityData, | ||
| BlockchainIdentityData.CreationState.REQUESTED_NAME_CHECKING | ||
| ) | ||
|
|
||
| // recover the verification link | ||
| platformRepo.updateIdentityCreationState( | ||
| blockchainIdentityData, | ||
| BlockchainIdentityData.CreationState.REQUESTED_NAME_CHECKED | ||
| ) | ||
| platformRepo.updateBlockchainIdentityData(blockchainIdentityData, blockchainIdentity) | ||
| // set voting state | ||
| platformRepo.updateIdentityCreationState( | ||
| blockchainIdentityData, | ||
| BlockchainIdentityData.CreationState.VOTING | ||
| ) | ||
| platformRepo.updateBlockchainIdentityData(blockchainIdentityData, blockchainIdentity) | ||
| } |
There was a problem hiding this comment.
This will prevent a crash or other errors when restoring an identity that doesn't have a username.
| private fun handleRestoreIdentityAction(identity: ByteArray) { | ||
| workInProgress = true | ||
| serviceScope.launch(createIdentityExceptionHandler) { | ||
| restoreIdentity(identity) | ||
| workInProgress = false | ||
| stopSelf() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is not used.
| adapter.submitList(groupedByDate) { | ||
| // Check again if the view is still in a valid state before any post-update operations | ||
| if (isAdded && currentLifecycle.currentState.isAtLeast(Lifecycle.State.STARTED)) { | ||
| showTransactionList() | ||
| } | ||
| } |
There was a problem hiding this comment.
This is supposed to prevent a crash, if the view is invalid.
| @@ -294,6 +296,18 @@ class MoreFragment : Fragment(R.layout.fragment_more) { | |||
| } | |||
| } | |||
|
|
|||
| private fun retry(errorMessage: String) { | |||
There was a problem hiding this comment.
The more screen will support retry that can allow the user to choose a new name under some circumstances.
![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 (2)
wallet/res/values/strings-dashpay.xml (1)
392-393: Clarify the Intent Behind Disabling Formatting for the Username Disclaimer.
The string resource now includesformatted="false":<string name="welcome_request_username_min_balance_disclaimer_all" formatted="false">You have %s Dash. Some usernames cost up to %s Dash.</string>This change ensures that the placeholders (
%s) are treated as literal text rather than triggering runtime formatting. Please verify that this is the intended behavior—if the UI is meant to show the literal%s(perhaps as part of a documentation or demo message) then this is correct; however, if the code expects dynamic substitution for the dash balance and cost, removing formatting might cause the substitution not to occur.wallet/res/values/strings.xml (1)
46-46: Ensure Consistent Handling of the CoinJoin Timeskew Message.
The updated string now explicitly disables formatting:<string name="wallet_coinjoin_timeskew_dialog_msg" formatted="false">Your device time is %s by %d seconds. You cannot use CoinJoin due to this difference. The time settings on your device needs to be changed to “Set time automatically” to use CoinJoin.</string>By adding
formatted="false", the placeholders (%sand%d) will be rendered literally. As with the username disclaimer, please verify that this is intentional. If these placeholders are meant for dynamic substitution at runtime, removing formatting support might lead to unexpected display issues. Otherwise, if you want to show the literal markers to the user (or if substitution is handled differently), this change is consistent with similar modifications elsewhere in the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
build.gradle(1 hunks)common/src/main/res/values/styles.xml(1 hunks)wallet/AndroidManifest.xml(0 hunks)wallet/res/layout/fragment_more.xml(1 hunks)wallet/res/values/strings-dashpay.xml(1 hunks)wallet/res/values/strings.xml(1 hunks)wallet/src/de/schildbach/wallet/WalletApplication.java(4 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(1 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/ui/dashpay/CreateIdentityService.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/more/MoreFragment.kt(3 hunks)
💤 Files with no reviewable changes (1)
- wallet/AndroidManifest.xml
🧰 Additional context used
🧬 Code Definitions (1)
wallet/src/de/schildbach/wallet/WalletApplication.java (5)
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)wallet/src/de/schildbach/wallet/transactions/coinjoin/CoinJoinMixingTxSet.kt (1)
wallet(10-54)
🔇 Additional comments (20)
build.gradle (1)
7-7: Verify compatibility with snapshot dependency version.The change updates the dppVersion from a stable release (1.7.3) to a development snapshot (1.7.4-SNAPSHOT). While this may be intentional to incorporate needed features for username restoration, snapshot versions can introduce instability since they're development builds.
Please ensure that this snapshot version is:
- Compatible with the rest of the dependencies
- Stable enough for your deployment target
- Not causing unexpected behavior in the username restoration process
When this PR is merged, consider setting a reminder to update to a stable release when 1.7.4 is officially released.
common/src/main/res/values/styles.xml (1)
168-175: Appropriate style addition for multi-line text.The new style removes the
android:lines="1"constraint from the parent style while keeping other styling consistent, allowing text to wrap across multiple lines instead of being truncated.This style addition makes sense for displaying potentially longer error messages in the username restoration UI, which is consistent with the PR objective of improving this feature.
wallet/res/layout/fragment_more.xml (1)
233-238: Good UI improvement for error message display.Applying the new
MenuRowSubTitleWithoutLinesstyle to the username restoration status text will provide better user experience by allowing error messages to display across multiple lines (up to 3 as set inmaxLines) rather than truncating longer messages.The addition of a descriptive
tools:textattribute also improves the design-time preview for developers, making it easier to visualize how error messages will appear without affecting the runtime behavior.wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (2)
159-159: Good practice: Capturing lifecycle state.Storing the current lifecycle state when the observer is triggered ensures consistent behavior even if the lifecycle changes during asynchronous operations.
This is a defensive programming technique that makes the code more robust against timing issues.
174-179: Improved lifecycle-aware handling for async callback.This change ensures that
showTransactionList()is only called if the fragment is still added to its activity and its lifecycle is at least in the STARTED state when the adapter's async operation completes.This prevents potential crashes that could occur if the fragment's view is destroyed or detached by the time the adapter finishes processing the list data. The solution correctly addresses the issue noted in the previous review comment.
wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
1435-1435: Good safety improvementChanged from using non-null assertion operator (
!!) to safe call operator (?.), which prevents potentialNullPointerExceptionifblockChainis null.wallet/src/de/schildbach/wallet/WalletApplication.java (4)
1126-1127: Improved null safety checkAdded null check for
walletBalanceObserverto prevent potentialNullPointerExceptionwhen accessing the total balance flow.
1136-1137: Improved null safety checkAdded null check for
walletBalanceObserverto prevent potentialNullPointerExceptionwhen accessing the mixed balance flow.
1149-1150: Improved null safety checkAdded null check for
walletBalanceObserverto prevent potentialNullPointerExceptionwhen accessing the general balance flow.
1159-1160: Improved null safety checkAdded null check for
walletBalanceObserverto prevent potentialNullPointerExceptionwhen accessing the spendable balance flow.wallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.kt (2)
296-320: Improved error handling for identities without usernamesAdded a conditional check for
blockchainIdentity.currentUsernamebefore executing updates to the identity creation state and blockchain identity data. This prevents potential null pointer exceptions when restoring an identity that doesn't have a username.
329-329: Added missing data update for identities without usernamesAdded a line to update the blockchain identity data when identity is not null but username is null, ensuring data consistency in this edge case.
wallet/src/de/schildbach/wallet/ui/more/MoreFragment.kt (3)
188-194: Improved error handling for username creationUpdated the click handler to use the new centralized error handling logic in the
retrymethod, which intelligently determines whether a new username is needed based on the specific error.
287-290: Enhanced retry button logicUpdated the retry button click handler to use the centralized
retrymethod that provides smarter recovery from username creation errors.
299-309: Added smart retry functionality for username errorsCreated a new
retrymethod that analyzes error messages to determine whether the user needs to select a new username or can simply retry with the existing one. This significantly improves the user experience by guiding users to the appropriate action based on the specific error.wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (2)
219-219: Enhanced logging with contextual information.The log statement has been improved by including the
lastPreBlockStagevariable, which provides valuable context about the current state when contact requests are already running.
234-235: Improved null safety handling for votingPeriodStart.The code now safely handles potential null values for
votingPeriodStartby using the Elvis operator to provide a default value of0L. This prevents NullPointerExceptions when checking voting period completion.wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt (3)
736-738: Fixed state transition for verification link saving.The state transition has been corrected to move from
REQUESTED_NAME_CHECKEDtoREQUESTED_NAME_LINK_SAVINGinstead of staying in the same state. This properly reflects the actual flow of the verification link saving process.
752-754: Fixed state transition after saving verification link.The state transition has been corrected to move from
REQUESTED_NAME_LINK_SAVINGtoREQUESTED_NAME_LINK_SAVEDinstead of reverting toREQUESTED_NAME_CHECKING. This ensures proper linear progression through the identity creation states.
794-1030: Removed unused method handleRestoreIdentityAction.The method for handling identity restoration from a byte array has been removed. This is appropriate since the functionality is now handled directly by the
restoreIdentitymethod.
Issue being fixed or feature implemented
kotlin platform:
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores