Feat/update create invite #1343
Conversation
* chore: remove obsolete Invite*Activity classes
…h-wallet into feat/update-create-invite
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes span updates across build configurations, UI, navigation, database schema, and business logic. Dependency versions in gradle files have been updated and the wallet version incremented. Several invitation-related activities, fragments, and layouts have been removed or refactored, while new invitation flows, dialogs, and navigation graphs have been introduced. Database migrations have been added (upgrading from version 15 to 16) with modifications to the invitation table and associated DAO queries. Service layers were adjusted by replacing the old PlatformRepo with a new TopUpRepository, and various UI components and view models were modernized using view binding, navArgs, and coroutine-based state management. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant VM as InvitationFragmentViewModel
participant Repo as TopUpRepository
participant Worker as SendInviteWorker
User->>VM: Initiate invitation
VM->>Repo: createInviteFundingTransaction()
Repo-->>VM: Return funding transaction details
VM->>Worker: Trigger SendInviteWorker (fundingAddress & value)
Worker->>Repo: Validate & send transaction
Repo-->>Worker: Confirm transaction sent
Worker-->>VM: Return invite sent status
VM->>User: Display invite success feedback
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 (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (25)
wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt (1)
99-100: Potential unused methodThe
canAffordIdentityCreation()method appears to be unused in this class. Consider removing it or documenting its purpose if it's being called from outside this class.wallet/src/de/schildbach/wallet/ui/dashpay/work/SendInviteOperation.kt (1)
35-41: Parameter name mismatch in uniqueWorkName function.There's a semantic disconnect between the function implementation and its usage. The function defines the parameter as
toUserId, but it's now being called with a funding address.Consider updating the parameter name to better reflect its current usage:
- fun uniqueWorkName(toUserId: String) = WORK_NAME + toUserId + fun uniqueWorkName(fundingAddress: String) = WORK_NAME + fundingAddresswallet/res/layout/invitation_preview_view.xml (1)
2-8: Layout structure change appears appropriate, but consider simplificationThe change from LinearLayout to FrameLayout with a nested LinearLayout adds an extra layer of view hierarchy. While this may be necessary for other design requirements not visible in this snippet, consider whether the nested structure is required or if a single LinearLayout would suffice.
-<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android" - android:layout_width="match_parent" - android:layout_height="wrap_content"> - <LinearLayout - android:layout_width="match_parent" - android:layout_height="wrap_content" - android:orientation="vertical"> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" + android:layout_width="match_parent" + android:layout_height="wrap_content" + android:orientation="vertical">And remember to adjust the closing tag at the end of the file:
- </LinearLayout> -</FrameLayout> +</LinearLayout>wallet/res/navigation/nav_invite_history.xml (1)
1-2: Update copyright yearThe copyright year is set to 2025, which is in the future. Update it to the current year.
-<?xml version="1.0" encoding="utf-8"?><!-- - ~ Copyright 2025 Dash Core Group. +<?xml version="1.0" encoding="utf-8"?><!-- + ~ Copyright 2024 Dash Core Group.wallet/src/de/schildbach/wallet/ui/dashpay/ContactsFragment.kt (1)
333-334: Good migration to Navigation Component.Replacing the intent-based activity navigation with Navigation Component's action is a positive architectural change. Consider removing the commented-out code once the migration is fully tested.
- safeNavigate(ContactsFragmentDirections.contactsToSearchUser()) - //startActivity(SearchUserActivity.createIntent(requireContext(), query)) + safeNavigate(ContactsFragmentDirections.contactsToSearchUser())wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt (3)
32-36: Clean up unused imports.There are several unused imports that should be removed from the file:
SendCoinsViewModel(line 32)DialogConfirmUsernameRequestBinding(line 35)This will improve code cleanliness and may slightly reduce build time.
-import de.schildbach.wallet.ui.send.SendCoinsViewModel import de.schildbach.wallet_test.R import de.schildbach.wallet_test.databinding.DialogConfirmTopupBinding -import de.schildbach.wallet_test.databinding.DialogConfirmUsernameRequestBinding import kotlinx.coroutines.launch
56-56: Uncomment or remove analytics tracking.There's a commented-out analytics tracking event that should either be implemented or removed completely.
If analytics tracking is required, consider implementing it:
- // requestUserNameViewModel.logEvent(AnalyticsConstants.UsersContacts.TOPUP_CONFIRM) + invitationFragmentViewModel.logEvent(AnalyticsConstants.UsersContacts.TOPUP_CONFIRM)
72-82: Consider removing commented code blocks.There are multiple lines of commented-out methods. These should either be implemented if needed or removed completely to keep the codebase clean.
If these methods are not needed, remove them entirely. If they will be needed in the future, add a TODO comment explaining why they're being kept.
wallet/src/de/schildbach/wallet/ui/invite/InvitesHistoryFragment.kt (1)
72-75: Use a consistent approach to Fragment transaction naming.The dialog fragment is being shown using the support fragment manager but doesn't follow a consistent naming pattern.
Consider using a more descriptive tag name or a constant:
InviteFilterSelectionDialog .createDialog(this) - .show(requireActivity().supportFragmentManager, "inviteFilterDialog") + .show(requireActivity().supportFragmentManager, FILTER_DIALOG_TAG)And add the constant to the companion object:
companion object { private val log = LoggerFactory.getLogger(InvitesHistoryFragment::class.java) private const val FILTER_DIALOG_TAG = "invite_filter_dialog" }wallet/res/navigation/nav_create_invite.xml (2)
1-80: Well-structured navigation graph with a copyright year issue.The navigation graph is well-organized with appropriate fragments, dialogs, arguments, and navigation actions.
There's an issue with the copyright year at line 2:
- ~ Copyright 2025 Dash Core Group. + ~ Copyright 2024 Dash Core Group.The year is set to 2025, which is in the future. It should be updated to the current year (2024).
52-66: Inconsistent package for ConfirmInviteDialogFragment.The
ConfirmInviteDialogFragmentis referenced with the packagede.schildbach.wallet.ui.morewhile it's being used in an invite-related navigation graph. Consider moving this dialog to thede.schildbach.wallet.ui.invitepackage for better organization or ensure there's a valid reason for this placement.wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (2)
358-366: Consider error handling in concurrent calls.
All three async calls (topUpRepository.updateInvitations(),checkTopUps(), andcheckInvitations()) run in parallel, but if one fails, it may short-circuit the entireawaitAll(). You might want to use a supervisor scope or handle exceptions individually so that a single failure doesn’t halt the other tasks.
1382-1387: Add exception logging or handling.
checkInvitations()callstopUpRepository.checkInvites(it), but errors are not caught. Consider logging or handling possible exceptions (e.g., network failures) to avoid silent failures.wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
921-921: Minor spelling issue.
Consider renamingtickRecieverRegisteredtotickReceiverRegistered(typo: “Reciever” → “Receiver”) for consistency.Apply this diff to fix the spelling:
- private var tickRecieverRegistered = false + private var tickReceiverRegistered = falsewallet/src/de/schildbach/wallet/ui/dashpay/work/SendInviteWorker.kt (2)
39-46: Consider refactoring large constructor with multiple dependencies.
This constructor accepts many injected dependencies (six in total). While this is functional, you could enhance maintainability by extracting certain logic or grouping related dependencies into dedicated helper classes or modules.
74-74: Address pending TODO regarding retry logic.
The comment suggests alignment with another worker for enhanced retry or partial-completion handling. Consider implementing a retry mechanism that skips already-executed steps and retries only the failed ones.Would you like me to create a draft solution or open a new issue to track this retry logic?
Also applies to: 76-76
wallet/src/de/schildbach/wallet/ui/SearchUserFragment.kt (3)
77-82: Remove or revisit large blocks of commented-out code.
Prolonged commented sections (e.g., toolbar setup, options menu handling) can lead to confusion. Either remove them or annotate clearly if they’re placeholders for future development.Also applies to: 288-296
168-171: Optional: Add clarity when checking invite count.
The current logic branches depending on whethergetInviteCount()is zero. Consider logging or clarifying this check to differentiate between no invites vs. other states.
282-285: Consider migrating away from deprecated startActivityForResult.
startActivityForResultis deprecated in newer Android APIs. Migrate to the Activity Result API to improve reliability and lifecycle awareness.wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (1)
593-647: Consider logging or preserving the initial exception in line 621.
A fallback attempt is made in thecatchblock, but the initial failure cause is lost. If the fallback also fails, the original error context is unavailable.} catch (e: Exception) { // Attempt second approach try { InstantSendLock( ... ) } catch (e2: Exception) { + log.warn("First exception: ${e.message}", e) log.warn("Second exception: ${e2.message}", e2) throw e2 } }🧰 Tools
🪛 detekt (1.23.7)
[warning] 621-621: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
wallet/res/values/strings-dashpay.xml (1)
468-476: New invitation fee strings are proper and well-labeled; fix minor double-space.
Consider removing the extra space in line 473:"can request" → "can request".-<string name="invitation_fee_noncontested_message">The person that you send this invitation to can request... +<string name="invitation_fee_noncontested_message">The person that you send this invitation to can request...wallet/src/de/schildbach/wallet/ui/invite/InviteCreatedFragment.kt (2)
74-77: Handle other result codes for a more user-friendly experience.
Currently, onlyRESULT_OKtriggers a flow. Consider logging or informing the user for different outcomes.
91-113: UI state transitions for sendInviteStatusLiveData are well-managed.
Consider reintroducing or finalizing the error dialog logic instead of relying solely on the new error text for better user feedback.wallet/res/navigation/nav_home.xml (2)
120-129: Make sure the new "caller" argument is handled
Thecallerargument is introduced multiple times across your new navigation actions. Verify that these arguments are indeed accessed in the destination fragments or view models, and consider validating them if necessary.Also applies to: 270-279, 421-430, 449-457
437-463: Update the fragment label to avoid confusion
Currently, thesearchUserFragmentis labeled as"ShowSeed". This might be a leftover label from a different screen and can be confusing for users. Consider changing the label to something more descriptive like"SearchUser".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
build.gradle(1 hunks)wallet/AndroidManifest.xml(0 hunks)wallet/CHANGES(1 hunks)wallet/androidTest/de/schildbach/wallet/database/DatabaseMigrationTest.kt(1 hunks)wallet/build.gradle(1 hunks)wallet/res/layout/activity_invite_friends.xml(0 hunks)wallet/res/layout/dialog_invitation_fee.xml(1 hunks)wallet/res/layout/fragment_invite_created.xml(2 hunks)wallet/res/layout/fragment_invite_friend.xml(0 hunks)wallet/res/layout/invitation_preview_view.xml(1 hunks)wallet/res/navigation/nav_create_invite.xml(1 hunks)wallet/res/navigation/nav_home.xml(5 hunks)wallet/res/navigation/nav_invite_friends.xml(0 hunks)wallet/res/navigation/nav_invite_history.xml(1 hunks)wallet/res/values/strings-dashpay.xml(2 hunks)wallet/res/values/strings-extra.xml(1 hunks)wallet/schemas/de.schildbach.wallet.database.AppDatabase/16.json(1 hunks)wallet/src/de/schildbach/wallet/Constants.java(1 hunks)wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt(1 hunks)wallet/src/de/schildbach/wallet/data/UsernameSearchResult.kt(2 hunks)wallet/src/de/schildbach/wallet/database/AppDatabase.kt(1 hunks)wallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt(1 hunks)wallet/src/de/schildbach/wallet/database/dao/InvitationsDao.kt(2 hunks)wallet/src/de/schildbach/wallet/database/entity/Invitation.kt(4 hunks)wallet/src/de/schildbach/wallet/di/DatabaseModule.kt(1 hunks)wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt(9 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(2 hunks)wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt(5 hunks)wallet/src/de/schildbach/wallet/service/platform/work/TopupIdentityWorker.kt(1 hunks)wallet/src/de/schildbach/wallet/service/work/BaseWorker.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/AbstractBindServiceActivity.java(1 hunks)wallet/src/de/schildbach/wallet/ui/InviteHandlerViewModel.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/SearchUserFragment.kt(8 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/ContactsFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/DashPayViewModel.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/work/SendInviteOperation.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/work/SendInviteWorker.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/invite/InvitationFeeDialogFragment.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/invite/InvitationFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/invite/InvitationFragmentViewModel.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/invite/InviteCreatedFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/invite/InviteDetailsFragment.kt(9 hunks)wallet/src/de/schildbach/wallet/ui/invite/InviteFriendActivity.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/invite/InviteFriendFragment.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/invite/InvitesHistoryActivity.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/invite/InvitesHistoryFragment.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/more/MixDashFirstDialogFragment.kt(3 hunks)wallet/src/de/schildbach/wallet/ui/more/MoreFragment.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysFragment.kt(0 hunks)wallet/src/de/schildbach/wallet/ui/username/voting/RequestUserNameViewModel.kt(1 hunks)
💤 Files with no reviewable changes (8)
- wallet/src/de/schildbach/wallet/ui/more/masternode_keys/MasternodeKeysFragment.kt
- wallet/src/de/schildbach/wallet/ui/invite/InvitesHistoryActivity.kt
- wallet/res/navigation/nav_invite_friends.xml
- wallet/res/layout/fragment_invite_friend.xml
- wallet/res/layout/activity_invite_friends.xml
- wallet/AndroidManifest.xml
- wallet/src/de/schildbach/wallet/ui/invite/InviteFriendActivity.kt
- wallet/src/de/schildbach/wallet/ui/invite/InviteFriendFragment.kt
✅ Files skipped from review due to trivial changes (5)
- wallet/res/values/strings-extra.xml
- wallet/src/de/schildbach/wallet/database/AppDatabase.kt
- wallet/src/de/schildbach/wallet/service/platform/work/TopupIdentityWorker.kt
- wallet/res/layout/dialog_invitation_fee.xml
- wallet/schemas/de.schildbach.wallet.database.AppDatabase/16.json
🧰 Additional context used
🪛 detekt (1.23.7)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt
[warning] 573-573: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 575-575: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 621-621: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (87)
wallet/src/de/schildbach/wallet/ui/invite/CreateInviteViewModel.kt (1)
71-72: LGTM: Simplified invite creation condition logicThe refactored code simplifies the condition for determining if an invite can be created by directly checking if the blockchain identity creation is complete, rather than checking for absence or in-progress states. This makes the code more readable and straightforward.
wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt (2)
12-12: Added import for CreditBalanceInfo.The import statement is correctly added to support the changes in the balance check logic.
378-399: Improved balance retrieval logic with null-safety.The new implementation adds robustness by explicitly handling the case where the identity might not be initialized yet. Using the Kotlin safe call operator (
?.let) with a fallback to a default balance (via the elvis operator?:) prevents potential null pointer exceptions that could occur in the previous implementation.The code flow is also improved by flattening the structure and making the conditional logic more straightforward.
wallet/src/de/schildbach/wallet/ui/dashpay/work/SendInviteOperation.kt (6)
27-27: Appropriate import added for new parameter type.The addition of the Coin import is necessary to support the updated function signature.
145-145: Method signature change significantly alters the invitation creation process.The function signature has been updated to accept a funding address and a coin value instead of just an ID. This change better represents the actual data needed for creating an invitation.
148-154: Work data expanded with new parameters.The worker now receives additional information (funding address and coin value) necessary for the invitation process. This aligns with the updated method signature.
155-156: Tagging mechanism updated to use funding address.The tag pattern has been updated to maintain consistency with the new parameter approach.
159-163: Work uniqueness now based on funding address.The unique work name is now based on the funding address instead of an ID, maintaining proper work identification.
90-112:❓ Verification inconclusive
Verify compatibility of allOperationsStatus with new tagging system.
This method extracts user IDs from tags with format "invite:XXX", but with the new changes, these tags will contain funding addresses instead of user IDs. Make sure all code that processes these tags expects funding addresses rather than user IDs.
🏁 Script executed:
#!/bin/bash # Search for other usages of the invite: tag pattern that might expect user IDs rg "invite:" --type kotlin --type java --glob "!**/SendInviteOperation.kt"Length of output: 3444
Action Required: Update Tag Parsing Logic in
allOperationsStatusMethodThe current implementation in
SendInviteOperation.kt(lines 90–112) still extracts and uses the substring following"invite:"as if it were a user ID. With the new tagging system, these tags now contain funding addresses instead. Please verify that downstream components consuming the map emitted by this method correctly interpret these strings as funding addresses. Consider renaming variables (e.g., fromtoUserIdto something likefundingAddress) and updating any related logic if necessary.
- Confirm that all components processing the result of
allOperationsStatusexpect funding addresses.- Update the variable naming and conversion logic to reflect the new semantics of invitation tags.
build.gradle (1)
6-7:✅ Verification successful
Version updates for DashJ and DPP dependencies.
The PR updates the versions of two core dependencies:
- dashjVersion from 21.1.5 to 21.1.6
- dppVersion from 1.7.1 to 1.7.2
These updates align with the PR description to update invitation functionality.
🏁 Script executed:
#!/bin/bash # Description: Verify if these updated dependencies are referenced elsewhere in the codebase rg -l "dashjVersion|dppVersion" --type gradleLength of output: 278
Dependency Version Updates Verified
The updated dependency versions in the root
build.gradlehave been correctly updated (dashjVersion from 21.1.5 to 21.1.6 and dppVersion from 1.7.1 to 1.7.2). The grep scan confirms that these properties are referenced across multiple Gradle files (e.g., in directories likecommon/,features/exploredash/,integrations/*, etc.), which indicates consistency in how these version variables are used throughout the repository.No further modifications are needed on this change.
wallet/src/de/schildbach/wallet/di/DatabaseModule.kt (1)
44-45:❓ Verification inconclusive
Added new database migration for invitation table.
The PR adds migration15to16 to the list of migrations in the database provider. This is consistent with the invitation-related changes mentioned in the PR title.
🏁 Script executed:
#!/bin/bash # Description: Check for the implementation of the migration and related changes to ensure consistency echo "Looking for migration implementation:" rg -A 5 "migration15to16" --type kotlin echo "Looking for DB version update:" rg -A 3 "const val VERSION = 16" --type kotlinLength of output: 2117
Review Update: Confirmed Migration Addition – Please Verify DB Version Update
- The implementation of
migration15to16is present and correctly defined inwallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt.- Its registration in the migration list within
wallet/src/de/schildbach/wallet/di/DatabaseModule.ktis confirmed by the references in our grep output.- However, our lookup for a DB version update (e.g., a constant like
const val VERSION = 16) did not return any results. Please verify manually that the database version is updated appropriately for the new migration.wallet/src/de/schildbach/wallet/ui/more/SecurityFragment.kt (1)
201-201:✅ Verification successful
Added service unbinding during wallet reset.
This change ensures that the service is properly unbound before wiping the wallet during reset. This is an important cleanup step that prevents potential memory leaks or service-related issues.
🏁 Script executed:
#!/bin/bash # Description: Verify that the doUnbindService method is now public in AbstractBindServiceActivity echo "Checking if doUnbindService was changed from protected to public:" rg -B 1 "doUnbindService" --type javaLength of output: 767
Service Unbinding Verified: The
doUnbindService()method is now public
- The shell script output confirms that in
wallet/src/de/schildbach/wallet/ui/AbstractBindServiceActivity.java, thedoUnbindService()method is indeed defined as public.- The call in
SecurityFragment.ktat line 201 correctly invokes this public method, ensuring proper unbinding before resetting the wallet.wallet/androidTest/de/schildbach/wallet/database/DatabaseMigrationTest.kt (1)
65-66:✅ Verification successful
Added new migration to test suite.
The PR correctly adds the new migration15to16 to the test migrations array, ensuring it's included in database migration tests. This is essential for verifying the migration works correctly.
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of migration15to16 and related entity changes echo "Checking migration implementation:" rg -A 20 "migration15to16" --type kotlin echo "Looking for invitation table entity changes:" rg -A 30 "invitation_table" --type kotlinLength of output: 20235
Migration Inclusion and Invitation Table Update Verified
The new migration15to16 is correctly added to the test migrations array and is fully integrated. The migration implementation in
AppDatabaseMigrations.ktproperly drops the oldinvitation_tableand creates it with the updated primary key constraint (based onfundingAddress). The test inwallet/androidTest/de/schildbach/wallet/database/DatabaseMigrationTest.ktconfirms that both migration14to15 and migration15to16 are executed as expected.
- Verified migration implementation in
wallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt.- Confirmed updated
invitation_tablecreation with the new primary key.- Ensured that the test suite includes the new migration and properly applies it.
wallet/CHANGES (1)
2-4: Properly documented version and changes.The changelog entry for v11.1.1 clearly documents both the username fee logic fix and the dependency updates with their specific versions. The mention of bug fixes related to CoinJoin and username creation provides good context for the update.
wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt (1)
47-47: Good addition of null safety.Adding the safe call operator (
?.) improves the code's robustness by preventing potential null pointer exceptions wheninstantSendlockis null. This is a good Kotlin practice.wallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.kt (1)
128-150: Properly implemented database migration with clear documentation.The migration from version 15 to 16 correctly handles the schema change by:
- Documenting that previous versions have no data in the invitation table
- Dropping the old table and recreating it with the new schema
- Changing the primary key from
userIdtofundingAddressThis approach ensures a clean transition to the new schema.
wallet/src/de/schildbach/wallet/ui/AbstractBindServiceActivity.java (1)
81-81:❓ Verification inconclusive
Access modifier change expands API surface.
Changing
doUnbindService()from protected to public expands the class API, allowing external components to directly unbind the service. Ensure this is intentional and that all callers handle the unbinding appropriately.
🏁 Script executed:
#!/bin/bash # Find all new references to doUnbindService() that weren't possible before rg "\.doUnbindService\(\)" --type kotlin --type java | grep -v "unbindServiceServiceConnection" | grep -v "AbstractBindServiceActivity"Length of output: 133
Action Required: Confirm Public Exposure of doUnbindService()
Based on our automated search, there are no clear external references to
doUnbindService()outside of its declaring class. However, the change fromprotectedtopublicstill broadens the API surface. Please manually verify that making this method public is intentional and that no external component will inadvertently invoke it without proper handling.
- Ensure that any external callers that might eventually be added will manage service unbinding correctly.
- Reassess if the method’s visibility should be restricted, especially given the absence of external references in the current codebase.
wallet/src/de/schildbach/wallet/service/work/BaseWorker.kt (4)
3-3: Added SuppressLint annotation import.This import is used for the annotation on the doWork method to suppress RestrictedApi warnings.
12-15: Improved code formatting for better readability.The constructor parameters have been reformatted to place each parameter on a separate line, which improves readability and follows standard Kotlin formatting conventions.
32-38: Added useful errorResult utility method.The new
errorResultmethod provides a convenient way to create standardized failure results with error messages, which will make error handling more consistent across worker implementations.
45-45: Added SuppressLint annotation for RestrictedApi.The
@SuppressLint("RestrictedApi")annotation has been added to suppress warnings about using restricted APIs in the WorkManager library.Make sure this annotation is necessary. RestrictedApi warnings typically indicate using non-public APIs that might change in future releases. Consider investigating if there's a public API alternative instead of suppressing the warning.
wallet/build.gradle (1)
212-213:❓ Verification inconclusive
Updated app version to 11.1.1.
The version has been incremented from 11.0.5 to 11.1.1, which aligns with the new features being added in this PR. The version code has also been updated accordingly.
🏁 Script executed:
#!/bin/bash # Check if there's a corresponding version entry in the CHANGES file if [ -f "../wallet/CHANGES" ]; then grep -A 5 "v11.1.1" ../wallet/CHANGES fiLength of output: 30
Please Verify the Changelog Update
- The
wallet/build.gradlefile correctly updates the app version to 11.1.1 and the version code accordingly.- However, our check for a corresponding entry in the
CHANGESfile (searched for "v11.1.1") produced no output.- Action: Please verify whether the
CHANGESfile should include a new entry for version 11.1.1. If it should, kindly update the file accordingly; if not, confirm that the changelog update is handled differently.wallet/res/layout/fragment_invite_created.xml (2)
68-75: Added status text for invitation creation feedback.A new TextView has been added to display the invitation creation status to the user, improving the user experience by providing clear feedback about the process.
77-84: Added progress indicator for invitation creation.A progress spinner has been added (initially hidden) to visually indicate when invitation creation is in progress, providing better user feedback during potentially lengthy operations.
wallet/src/de/schildbach/wallet/Constants.java (1)
112-112: Enabled invitation feature.The
SUPPORTS_INVITESflag has been changed fromfalsetotrue, enabling the invitation feature that this PR is implementing. This change is consistent with the PR title "Feat/update create invite" and coordinates with the other invitation-related changes in this PR.Ensure that all necessary components for the invitation feature are ready for production use, as enabling this flag will expose the feature to users on TestNet. The flag is located in the TestNet3 configuration block, which suggests this change will only affect TestNet builds for now.
wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsFragment.kt (2)
47-47: Good addition of safeNavigate utilityThe addition of the
safeNavigateimport will help prevent navigation crashes that can occur when navigating during state changes or when navigating to the same destination multiple times.
241-245: Navigation implementation improvedThe change from starting activities to using the Navigation Component with
safeNavigateis a good modernization of the code. This follows Android's recommended navigation patterns and provides a more consistent user experience with proper animations and backstack management.wallet/res/navigation/nav_invite_history.xml (1)
17-60: Navigation graph structure looks goodThe navigation graph properly defines the relationships between fragments with appropriate animations and arguments. The inclusion of the create invite navigation graph through the include tag promotes modularity and reuse.
Consider adding a description for each argument to clarify their purpose:
<argument android:name="caller" + android:description="The screen that initiated navigation to this fragment" app:argType="string" /><argument android:name="invite_index" + android:description="Index of the invite in the list" app:argType="integer" /> <argument android:name="identity_id" + android:description="Identifier of the DashPay identity" app:argType="string" />wallet/src/de/schildbach/wallet/data/UsernameSearchResult.kt (1)
72-88: Sorting extension function is well-implementedThe extension function provides a clean way to sort username search results by different criteria. The lowercase comparisons ensure case-insensitive sorting, and the fallback for empty display names is a good edge case handling.
Two minor suggestions:
- The TODO comment on line 86 should be more specific or tracked in your issue management system.
- Consider adding documentation to explain the purpose of the function and each sorting option.
+/** + * Orders the list of UsernameSearchResult objects according to the specified criteria. + * @param orderBy The sorting criteria to apply. + */ fun ArrayList<UsernameSearchResult>.orderBy(orderBy: UsernameSortOrderBy) {wallet/src/de/schildbach/wallet/database/dao/InvitationsDao.kt (2)
38-39: Well-designed reactive data access method!The new
observeByUserIdmethod provides a clean reactive approach using Flow to observe invitation changes for a specific user, which is good for updating UI components reactively.
53-54: LGTM. This supports the database schema change.This new method aligns well with the database migration where
fundingAddressbecame the primary key for the invitation table. The suspend function allows for efficient coroutine-based data access.wallet/src/de/schildbach/wallet/ui/more/MixDashFirstDialogFragment.kt (3)
41-41: Good callback pattern implementation.Adding this callback property enables flexible dialog behavior, allowing the caller to define custom actions.
47-47: Improved navigation approach.Replacing the hardcoded activity navigation with a callback invocation enhances flexibility and follows modern Android architecture patterns.
69-72: Excellent API extension.This new method creates a clean, intuitive API for showing the dialog with a custom callback. It's well-structured and maintains backward compatibility.
wallet/src/de/schildbach/wallet/ui/dashpay/ContactsFragment.kt (2)
165-166: Good conditional navigation logic.The logic now directly checks the invite count and navigates to the fee screen when appropriate, simplifying the flow and improving code readability.
169-169: Modernized navigation implementation.Using Navigation Component with safe navigation is a great improvement over the previous activity-based approach.
wallet/src/de/schildbach/wallet/ui/InviteHandlerViewModel.kt (2)
40-40: Clean repository dependency update.The change from
PlatformRepotoTopUpRepositorylooks good and aligns with the broader refactoring of invitation-related functionality.
72-72: Consistent method call update.The method call has been properly updated to use the new repository while maintaining the same functionality.
wallet/src/de/schildbach/wallet/ui/more/ConfirmInviteDialogFragment.kt (2)
44-50: LGTM: Well-structured dialog fragment using modern Android practices.The fragment is well-implemented using view binding, navigation args, and proper dependency injection with Hilt.
31-31:✅ Verification successful
Verify import path for ConfirmTopupDialogViewModel.
The import path
de.schildbach.wallet.ui.more.tools.ConfirmTopupDialogViewModelsuggests the ViewModel class is in a different package than what might be expected based on its usage.Check if the ViewModel is in the correct package or if the import should be updated:
🏁 Script executed:
#!/bin/bash # Find all files that define the ConfirmTopupDialogViewModel class find wallet/src -type f -name "*.kt" -exec grep -l "class ConfirmTopupDialogViewModel" {} \;Length of output: 174
Import path verified – no update required.
The ConfirmTopupDialogViewModel class is properly located in thede.schildbach.wallet.ui.more.toolspackage as confirmed by its file location atwallet/src/de/schildbach/wallet/ui/more/tools/ConfirmTopupDialogViewModel.kt.wallet/src/de/schildbach/wallet/ui/dashpay/DashPayViewModel.kt (1)
311-312: LGTM: Good use of coroutine dispatchers for IO operations.The changes properly utilize
withContext(Dispatchers.IO)for database operations, ensuring they don't block the main thread. This follows Kotlin coroutines best practices.wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (1)
306-307: LGTM: Improved navigation with the Navigation Component.Good refactoring from direct activity launch to Navigation Component. This approach is more maintainable and allows for better animation control.
Consider removing the commented-out line since the code has been successfully migrated to use the Navigation Component.
wallet/src/de/schildbach/wallet/ui/invite/InvitesHistoryFragment.kt (4)
37-46: LGTM: Improved structure using navArgs and viewBinding.Good refactoring to use the Navigation Component's argument system and view binding for more type-safe and concise code.
57-60: LGTM: Improved navigation handling with popBackStack.Good simplification of the back navigation logic using the Navigation Component's
popBackStack().
104-112: LGTM: Improved navigation with safeNavigate.Good refactoring to use the Navigation Component with
safeNavigatefor type-safe navigation. The analytics event logging is also now more clearly structured.
114-121: LGTM: Improved navigation for invite details.Good refactoring to use the Navigation Component with
safeNavigatefor navigating to invite details.wallet/src/de/schildbach/wallet/ui/more/MoreFragment.kt (1)
150-162: Good refactoring to Navigation Component.The code has been successfully transitioned from activity-based navigation to using the Navigation Component. The conditional logic for showing MixDashFirstDialogFragment before navigating is well-implemented.
wallet/src/de/schildbach/wallet/ui/invite/InvitationFeeDialogFragment.kt (1)
33-76: The implementation is clean, but there's an unresolved TODO.The dialog fragment implementation for fee selection is well-structured with proper handling of wallet balance constraints and user input validation.
However, there's an unresolved TODO comment at lines 48-49 about
safeNavigatenot working. This should be addressed:- // TODO: why doesn't safeNavigate work - // safeNavigate(InvitationFeeDialogFragmentDirections.toConfirmInviteDialog(selectedFee.value))Is there a specific reason why
safeNavigateisn't working in this context? It might be worth investigating and documenting the issue or removing the TODO if the current implementation is sufficient.wallet/src/de/schildbach/wallet/ui/invite/InvitationFragment.kt (2)
45-50: Good modernization with AndroidEntryPoint and viewModels delegate.The refactoring to use the
@AndroidEntryPointannotation andviewModelsdelegate is a good practice for dependency injection and ViewModel initialization.
68-69: Good upgrade to the Activity Result API.Replacing the deprecated
startActivityForResultpattern with the modern Activity Result API usingsendInviteLauncheris a good improvement for handling activity results.wallet/src/de/schildbach/wallet/database/entity/Invitation.kt (3)
24-25: Migration tokotlinx.parcelizelooks good.
No issues found with the updated imports; this is the recommended approach over the deprecatedkotlinx.android.parcel.
42-52: Ensure no collisions occur with the new primary key.
You’ve switched the primary key tofundingAddressand madetxidnullable. Verify thatfundingAddressis a truly unique value in all cases and that any pre-existing references to a non-nulltxidare updated accordingly.
86-89: Utility function looks fine.
ThehasTransaction()method is concise and readable.wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (6)
52-52: New import forTopUpRepositoryis fine.
No issues with adding the import statement.
195-195: InjectingTopUpRepository.
This promotes a clean separation of concerns for top-up and invitation flows.
359-359: Verify exception handling forhandleSentAssetLockTransaction.
Ensure that errors fromtopUpRepository.handleSentAssetLockTransaction(...)are properly caught or logged, to avoid leaving the system in an inconsistent state if something fails.
641-641: No concerns with the new field.
StoringconnectivityReceiverRegisteredhelps avoid double unregistrations.
685-690: Good approach callingonCreateCompleted.await().
Awaiting onCreate logic ensures the service initialization is complete before proceeding. This prevents race conditions incheckService().
1292-1296: Conditional unregistration is a solid fix.
Great job guarding against multiple unregistration calls with flags likeconnectivityReceiverRegisteredandtickReceiverRegistered.wallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.kt (2)
928-928: Clearing references to the wallet.
NullifyingauthenticationGroupExtensionafter clearing the database helps prevent stale references.
989-989: Confidence-based event listener usage is valid.
Switching tocftx.getConfidence(walletApplication.wallet!!.context)ensures the correctTransactionConfidenceis retrieved. This approach is standard in newer releases of bitcoinj.wallet/src/de/schildbach/wallet/ui/dashpay/work/SendInviteWorker.kt (3)
81-82: Validate non-zero funding value.
The code correctly checks for a zero or missingKEY_VALUEand callserror("missing KEY_VALUE parameter…"). Ensure any caller code gracefully handles this scenario, as a zero value will trigger an abrupt failure here.Also applies to: 98-99
115-126: Verify usage ofsentAtvscreatedAt.
Inside theif (invitation?.sentAt == 0L)block, you setcreatedAtinstead ofsentAt. If it’s intentional to usecreatedAtfor this logic, clarify the naming; otherwise, this could be a bug if you intended to track the actual "sent" time.
148-150: Good exception handling approach.
Catching exceptions, logging them, and returning a structured error result ensures that failures are properly reported. This streamlined error path looks well-implemented.wallet/src/de/schildbach/wallet/ui/SearchUserFragment.kt (1)
61-61: Successfully migrated to a fragment-based approach.
Replacing the activity layout withFragment(R.layout.activity_search_dashpay_profile_root)and usingnavArgsplus a dedicated view binding improves code organization and type safety.Also applies to: 63-64, 70-71
wallet/src/de/schildbach/wallet/ui/invite/InviteDetailsFragment.kt (3)
53-54: Use ofviewBindingandnavArgsis clean and modern.
Adopting these utilities significantly reduces boilerplate and potential null pointer issues.
118-120: Neat argument handling.
AssigningidentityIdandinviteIndexfromargsensures the fragment receives clear, type-safe parameters.
201-201: Check for possible null invitation before using!!.
ThoughfilterNotNull()is applied, ensure there’s no edge case whereinvitation.valuecould be null at runtime, especially if the data flow changes or the invitation is not ready.Also applies to: 205-205
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (10)
19-23: All import statements and class-level documentation look fine.
These changes appear to be strictly additive and do not introduce any negative side effects.Also applies to: 26-26, 28-29, 31-32, 34-34, 39-39, 50-50, 58-58, 60-65, 66-69, 71-75, 77-81
113-133: New invitation-related methods in the interface look appropriate.
All additions (suspend functions and regular functions) are properly named and grouped, and the usage of suspend for database/network-facing operations is consistent.
140-143: Dependency injection for new DAOs and CoinJoinConfig is appropriate.
Ensuring that these dependencies are tested stands out as a good practice.
310-311: Verify reliance on numBroadcastPeers for transaction broadcast logic.
CheckingnumBroadcastPeers() > 0does not necessarily confirm successful propagation. Consider additional checks such as chainlocks or instantsend locks for robust broadcast confirmation.
406-408: DAO update looks good.
This upsert-style insertion is straightforward and minimal.
410-412: Retrieval method is concise and properly delegated to the DAO.
No issues noted.
414-452: Dynamic link creation logic is well-structured.
No correctness or performance issues; usage ofFirebaseDynamicLinksis standard.
454-468: Short dynamic link builder is correctly wrapped in a coroutine suspend.
Exception handling and success callbacks are properly mapped.
471-522: Invite cross-check logic is acceptable.
No concurrency or error-handling concerns beyond standard practice.
524-542: Invitation update logic is properly placed.
Looping over all invitations to detect acceptance is straightforward, and the usage ofplatformRepo.updateDashPayProfileseems consistent.wallet/res/values/strings-dashpay.xml (1)
21-21: Renamed string maintains clarity and consistency.
No functional change is introduced, and "Invitation" remains meaningful.wallet/src/de/schildbach/wallet/ui/invite/InviteCreatedFragment.kt (4)
19-19: New imports and annotations for modern features look fine.
Nothing stands out as problematic.Also applies to: 22-22, 24-28, 32-36, 38-38
49-51: Hiding profilePictureEnvelope and previewButton by default is sensible.
These will be toggled visible once the invite is created successfully.
69-69: Replacing custom methods with direct navigation calls is consistent.
Pop-back-stack usage is straightforward and safer.
82-84: Observing invitation via filterNotNull is good.
Ensures the UI only responds when we have a valid invitation.wallet/res/navigation/nav_home.xml (1)
44-49: Add navigation to SearchUserFragment
The newly introduced action and animations for navigating tosearchUserFragmentappear consistent with the existing pattern. Nicely done!wallet/src/de/schildbach/wallet/ui/invite/InvitationFragmentViewModel.kt (3)
36-46: New coroutine imports and annotations
These additions for coroutine-based flows and experimental APIs look appropriate and align with modern Android best practices.Also applies to: 48-48, 50-50, 59-59
136-140: Memo update logic looks good
Your approach of copying the existing record and updating thememofield is straightforward and readable.
148-151: Guard against null usage of identityId
The code relies onidentityId.value!!in several places, which can lead to runtime crashes if the flow hasn’t emitted a valid identity yet. Confirm thatidentityIdis always set before these calls or add null handling logic as needed.Also applies to: 155-155, 175-175
| identityBalance > 0L && contestable -> (Coin.valueOf(identityBalance / 1000) + walletBalance) > Coin.valueOf(CONTEST_DOCUMENT_FEE / 1000) | ||
| identityBalance > 0L && !contestable -> (Coin.valueOf(identityBalance / 1000) + walletBalance) > Coin.valueOf(NON_CONTEST_DOCUMENT_FEE / 1000) | ||
| identityBalance == 0L && contestable -> walletBalance >= Constants.DASH_PAY_FEE_CONTESTED | ||
| identityBalance == 0L && !contestable -> walletBalance >= Constants.DASH_PAY_FEE | ||
| else -> false // how can we get here? | ||
| } |
There was a problem hiding this comment.
Fixed balance comparison logic for username creation
The changes in the checkUsernameValid function improve how the app determines if users have enough funds to create usernames:
- Added parentheses around the expressions in lines 397-398 to ensure proper order of operations when comparing balances
- Changed the comparison operator from
>to>=in lines 399-400, allowing transactions to proceed when the wallet balance exactly equals the required fee (instead of requiring it to be strictly greater)
These changes fix a potential issue where users with exactly enough balance would be incorrectly told they couldn't afford a username.
| override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | ||
| super.onViewCreated(view, savedInstanceState) | ||
| viewModel.amount = Coin.valueOf(args.amount) | ||
| binding.confirmBtn.setOnClickListener { | ||
| lifecycleScope.launch { | ||
| // requestUserNameViewModel.logEvent(AnalyticsConstants.UsersContacts.TOPUP_CONFIRM) | ||
| val identityId = invitationFragmentViewModel.sendInviteTransaction(Coin.valueOf(args.amount)) | ||
| findNavController().navigate( | ||
| ConfirmInviteDialogFragmentDirections.toInviteCreatedFragment(identityId) | ||
| ) | ||
| } | ||
| } | ||
| binding.dismissBtn.setOnClickListener { dismiss() } | ||
| binding.confirmMessage.isVisible = false | ||
| viewModel.uiState.observe(viewLifecycleOwner) { | ||
| binding.dashAmountView.text = it.amountStr | ||
| binding.fiatSymbolView.text = it.fiatSymbol | ||
| binding.fiatAmountView.text = it.fiatAmountStr | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement error handling for the invite transaction.
The sendInviteTransaction call doesn't have error handling. If the transaction fails, the user won't receive any feedback.
Consider adding try-catch and display an error message to the user:
binding.confirmBtn.setOnClickListener {
lifecycleScope.launch {
- // requestUserNameViewModel.logEvent(AnalyticsConstants.UsersContacts.TOPUP_CONFIRM)
- val identityId = invitationFragmentViewModel.sendInviteTransaction(Coin.valueOf(args.amount))
- findNavController().navigate(
- ConfirmInviteDialogFragmentDirections.toInviteCreatedFragment(identityId)
- )
+ try {
+ // invitationFragmentViewModel.logEvent(AnalyticsConstants.UsersContacts.TOPUP_CONFIRM)
+ val identityId = invitationFragmentViewModel.sendInviteTransaction(Coin.valueOf(args.amount))
+ findNavController().navigate(
+ ConfirmInviteDialogFragmentDirections.toInviteCreatedFragment(identityId)
+ )
+ } catch (e: Exception) {
+ // Log the error
+ // Display an error message
+ binding.confirmMessage.text = getString(R.string.error_sending_invite)
+ binding.confirmMessage.isVisible = true
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | |
| super.onViewCreated(view, savedInstanceState) | |
| viewModel.amount = Coin.valueOf(args.amount) | |
| binding.confirmBtn.setOnClickListener { | |
| lifecycleScope.launch { | |
| // requestUserNameViewModel.logEvent(AnalyticsConstants.UsersContacts.TOPUP_CONFIRM) | |
| val identityId = invitationFragmentViewModel.sendInviteTransaction(Coin.valueOf(args.amount)) | |
| findNavController().navigate( | |
| ConfirmInviteDialogFragmentDirections.toInviteCreatedFragment(identityId) | |
| ) | |
| } | |
| } | |
| binding.dismissBtn.setOnClickListener { dismiss() } | |
| binding.confirmMessage.isVisible = false | |
| viewModel.uiState.observe(viewLifecycleOwner) { | |
| binding.dashAmountView.text = it.amountStr | |
| binding.fiatSymbolView.text = it.fiatSymbol | |
| binding.fiatAmountView.text = it.fiatAmountStr | |
| } | |
| } | |
| override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | |
| super.onViewCreated(view, savedInstanceState) | |
| viewModel.amount = Coin.valueOf(args.amount) | |
| binding.confirmBtn.setOnClickListener { | |
| lifecycleScope.launch { | |
| try { | |
| // invitationFragmentViewModel.logEvent(AnalyticsConstants.UsersContacts.TOPUP_CONFIRM) | |
| val identityId = invitationFragmentViewModel.sendInviteTransaction(Coin.valueOf(args.amount)) | |
| findNavController().navigate( | |
| ConfirmInviteDialogFragmentDirections.toInviteCreatedFragment(identityId) | |
| ) | |
| } catch (e: Exception) { | |
| // Log the error | |
| // Display an error message | |
| binding.confirmMessage.text = getString(R.string.error_sending_invite) | |
| binding.confirmMessage.isVisible = true | |
| } | |
| } | |
| } | |
| binding.dismissBtn.setOnClickListener { dismiss() } | |
| binding.confirmMessage.isVisible = false | |
| viewModel.uiState.observe(viewLifecycleOwner) { | |
| binding.dashAmountView.text = it.amountStr | |
| binding.fiatSymbolView.text = it.fiatSymbol | |
| binding.fiatAmountView.text = it.fiatAmountStr | |
| } | |
| } |
| @@ -65,6 +67,7 @@ data class Invitation(@PrimaryKey val userId: String, | |||
|
|
|||
| other as Invitation | |||
|
|
|||
| if (fundingAddress != other.fundingAddress) return false | |||
There was a problem hiding this comment.
Inconsistent equals/hashCode implementation.
Your equals method includes fundingAddress, userId, txid, etc., but hashCode() currently depends only on txid. This can lead to violations of the contract that equal objects must have the same hash code.
A possible fix:
override fun hashCode(): Int {
- return txid.hashCode()
+ var result = fundingAddress.hashCode()
+ result = 31 * result + userId.hashCode()
+ result = 31 * result + (txid?.hashCode() ?: 0)
+ result = 31 * result + createdAt.hashCode()
+ result = 31 * result + acceptedAt.hashCode()
+ return result
}📝 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.
| if (fundingAddress != other.fundingAddress) return false | |
| override fun hashCode(): Int { | |
| var result = fundingAddress.hashCode() | |
| result = 31 * result + userId.hashCode() | |
| result = 31 * result + (txid?.hashCode() ?: 0) | |
| result = 31 * result + createdAt.hashCode() | |
| result = 31 * result + acceptedAt.hashCode() | |
| return result | |
| } |
| private val workerJob = Job() | ||
| private var workerScope = CoroutineScope(workerJob + Dispatchers.IO) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the custom job/scope is canceled properly to avoid leaks.
The workerJob and workerScope might stay active beyond their intended lifetime unless explicitly canceled on shutdown or when this repository is no longer needed.
+ fun close() {
+ workerJob.cancel()
+ }📝 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 val workerJob = Job() | |
| private var workerScope = CoroutineScope(workerJob + Dispatchers.IO) | |
| private val workerJob = Job() | |
| private var workerScope = CoroutineScope(workerJob + Dispatchers.IO) | |
| fun close() { | |
| workerJob.cancel() | |
| } |
There was a problem hiding this comment.
When should close() be called?
There was a problem hiding this comment.
@HashEngineering The close() method should be called when the lifecycle of the TopUpRepository is ending to ensure proper cleanup of coroutine resources. Since this is a dependency-injected repository, the exact point depends on your application's architecture:
-
If it's a singleton with application scope: Call it during application shutdown in your
Application.onTerminate()or a custom shutdown hook. -
If it's bound to a specific service: Call it in that service's
onDestroy()method. -
If you're using Hilt/Dagger with scoped bindings: Add a custom cleanup handler for your module/component.
For maximum safety, you could implement it as part of an application shutdown process, such as in your WalletApplication class's termination sequence, alongside other resource cleanup operations.
Alternatively, you could refactor to use a viewModelScope or other lifecycle-aware scope instead of creating your own Job/CoroutineScope, which would eliminate the need for manual cleanup.
| val extension = authExtension | ||
|
|
||
| if (platformRepo.hasIdentity) { | ||
| workerScope.launch(Dispatchers.IO) { | ||
| // Context.getOrCreate(platform.params) | ||
| val inviteKey = extension.invitationFundingKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) | ||
| val isInvite = inviteKey != null | ||
| val isTopup = extension.identityTopupKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) != null | ||
| val isIdentity = extension.identityFundingKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) != null | ||
| val identityId = cftx.identityId.toStringBase58() | ||
| if (isInvite && !isTopup && !isIdentity && invitationsDao.loadByUserId(identityId) == null) { | ||
| // this is not in our database | ||
| var invite = Invitation( | ||
| Address.fromKey(Constants.NETWORK_PARAMETERS, inviteKey).toBase58(), | ||
| identityId, | ||
| cftx.txId, | ||
| blockTimestamp, | ||
| "", | ||
| blockTimestamp, | ||
| 0 | ||
| ) | ||
|
|
||
| // profile information here | ||
| try { | ||
| if (platformRepo.updateDashPayProfile(identityId)) { | ||
| val profile = dashPayProfileDao.loadByUserId(identityId) | ||
| invite = invite.copy(acceptedAt = profile?.createdAt ?: -1) // it was accepted in the past, use profile creation as the default | ||
| } | ||
| } catch (e: NullPointerException) { | ||
| // swallow, the identity was not found for this invite | ||
| } catch (e: MaxRetriesReachedException) { | ||
| // swallow, the profile could not be retrieved | ||
| // the invite status update function should be able to try again | ||
| } | ||
| invitationsDao.insert(invite) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid swallowing exceptions in NullPointerException & MaxRetriesReachedException.
Currently, lines 573 and 575 catch and swallow exceptions with no further action or logging, losing contextual error information.
-} catch (e: NullPointerException) {
- // swallow
-} catch (e: MaxRetriesReachedException) {
- // swallow
+} catch (e: NullPointerException) {
+ log.error("NullPointerException encountered while updating DashPayProfile", e)
+} catch (e: MaxRetriesReachedException) {
+ log.error("MaxRetriesReachedException encountered while updating DashPayProfile", e)📝 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 fun handleSentAssetLockTransaction(cftx: AssetLockTransaction, blockTimestamp: Long) { | |
| val extension = authExtension | |
| if (platformRepo.hasIdentity) { | |
| workerScope.launch(Dispatchers.IO) { | |
| // Context.getOrCreate(platform.params) | |
| val inviteKey = extension.invitationFundingKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) | |
| val isInvite = inviteKey != null | |
| val isTopup = extension.identityTopupKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) != null | |
| val isIdentity = extension.identityFundingKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) != null | |
| val identityId = cftx.identityId.toStringBase58() | |
| if (isInvite && !isTopup && !isIdentity && invitationsDao.loadByUserId(identityId) == null) { | |
| // this is not in our database | |
| var invite = Invitation( | |
| Address.fromKey(Constants.NETWORK_PARAMETERS, inviteKey).toBase58(), | |
| identityId, | |
| cftx.txId, | |
| blockTimestamp, | |
| "", | |
| blockTimestamp, | |
| 0 | |
| ) | |
| // profile information here | |
| try { | |
| if (platformRepo.updateDashPayProfile(identityId)) { | |
| val profile = dashPayProfileDao.loadByUserId(identityId) | |
| invite = invite.copy(acceptedAt = profile?.createdAt ?: -1) // it was accepted in the past, use profile creation as the default | |
| } | |
| } catch (e: NullPointerException) { | |
| // swallow, the identity was not found for this invite | |
| } catch (e: MaxRetriesReachedException) { | |
| // swallow, the profile could not be retrieved | |
| // the invite status update function should be able to try again | |
| } | |
| invitationsDao.insert(invite) | |
| } | |
| } | |
| } | |
| } | |
| override fun handleSentAssetLockTransaction(cftx: AssetLockTransaction, blockTimestamp: Long) { | |
| val extension = authExtension | |
| if (platformRepo.hasIdentity) { | |
| workerScope.launch(Dispatchers.IO) { | |
| // Context.getOrCreate(platform.params) | |
| val inviteKey = extension.invitationFundingKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) | |
| val isInvite = inviteKey != null | |
| val isTopup = extension.identityTopupKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) != null | |
| val isIdentity = extension.identityFundingKeyChain.findKeyFromPubHash(cftx.assetLockPublicKeyId.bytes) != null | |
| val identityId = cftx.identityId.toStringBase58() | |
| if (isInvite && !isTopup && !isIdentity && invitationsDao.loadByUserId(identityId) == null) { | |
| // this is not in our database | |
| var invite = Invitation( | |
| Address.fromKey(Constants.NETWORK_PARAMETERS, inviteKey).toBase58(), | |
| identityId, | |
| cftx.txId, | |
| blockTimestamp, | |
| "", | |
| blockTimestamp, | |
| 0 | |
| ) | |
| // profile information here | |
| try { | |
| if (platformRepo.updateDashPayProfile(identityId)) { | |
| val profile = dashPayProfileDao.loadByUserId(identityId) | |
| invite = invite.copy(acceptedAt = profile?.createdAt ?: -1) // it was accepted in the past, use profile creation as the default | |
| } | |
| } catch (e: NullPointerException) { | |
| log.error("NullPointerException encountered while updating DashPayProfile", e) | |
| } catch (e: MaxRetriesReachedException) { | |
| log.error("MaxRetriesReachedException encountered while updating DashPayProfile", e) | |
| } | |
| invitationsDao.insert(invite) | |
| } | |
| } | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.7)
[warning] 573-573: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 575-575: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
| override suspend fun createInviteFundingTransaction( | ||
| blockchainIdentity: BlockchainIdentity, | ||
| keyParameter: KeyParameter?, | ||
| topupAmount: Coin | ||
| ): AssetLockTransaction { | ||
| // dashj Context does not work with coroutines well, so we need to call Context.propogate | ||
| // in each suspend method that uses the dashj Context | ||
| Context.propagate(walletApplication.wallet!!.context) | ||
| log.info("createInviteFundingTransactionAsync prop context") | ||
| val balance = walletApplication.wallet!!.getBalance(Wallet.BalanceType.ESTIMATED_SPENDABLE) | ||
| val emptyWallet = balance == topupAmount && balance <= (topupAmount + Transaction.MIN_NONDUST_OUTPUT) | ||
| val fundingAddress = Address.fromKey( | ||
| Constants.NETWORK_PARAMETERS, | ||
| authExtension.currentKey(AuthenticationKeyChain.KeyChainType.INVITATION_FUNDING) | ||
| ) | ||
| val cftx = blockchainIdentity.createInviteFundingTransaction( | ||
| topupAmount, | ||
| keyParameter, | ||
| useCoinJoin = coinJoinConfig.getMode() != CoinJoinMode.NONE, | ||
| returnChange = true, | ||
| emptyWallet = emptyWallet | ||
| ) | ||
| val invitation = Invitation( | ||
| fundingAddress.toBase58(), | ||
| cftx.identityId.toStringBase58(), cftx.txId, | ||
| System.currentTimeMillis() | ||
| ) | ||
| // update database | ||
| updateInvitation(invitation) | ||
|
|
||
| sendTransaction(cftx) | ||
| // update database | ||
| updateInvitation(invitation.copy(sentAt = System.currentTimeMillis())) | ||
| return cftx | ||
| } |
There was a problem hiding this comment.
Handle exceptions from sendTransaction to avoid marking an invitation as sent prematurely.
If sendTransaction(cftx) fails, the code still updates sentAt, which can lead to inconsistent invitation states in the DB.
sendTransaction(cftx)
- updateInvitation(invitation.copy(sentAt = System.currentTimeMillis()))
+ try {
+ sendTransaction(cftx)
+ updateInvitation(invitation.copy(sentAt = System.currentTimeMillis()))
+ } catch (e: Exception) {
+ log.error("Error sending invitation transaction: ${e.message}", e)
+ // decide if we need to revert or handle partial state
+ }Committable suggestion skipped: line range outside the PR's diff.
| suspend fun sendInviteTransaction(value: Coin): String { | ||
| // ensure that the fundingAddress hasn't been used | ||
| withContext(Dispatchers.IO) { | ||
| val invitation = invitationDao.loadByFundingAddress(fundingAddress) | ||
| while (invitation?.txid != null) { | ||
| authExtension.freshKey(AuthenticationKeyChain.KeyChainType.INVITATION_FUNDING) | ||
| } | ||
| } | ||
| val fundingAddress = this.fundingAddress // save the address locally |
There was a problem hiding this comment.
Re-check after generating a fresh funding address
Within the while (invitation?.txid != null) loop, you’re calling authExtension.freshKey() but never reload the updated fundingAddress from the DAO. This could lead to incorrect assumptions or an infinite loop. Consider re-checking the invitation for each newly generated address:
withContext(Dispatchers.IO) {
- val invitation = invitationDao.loadByFundingAddress(fundingAddress)
- while (invitation?.txid != null) {
- authExtension.freshKey(AuthenticationKeyChain.KeyChainType.INVITATION_FUNDING)
- }
+ var currentInvitation: Invitation?
+ do {
+ currentInvitation = invitationDao.loadByFundingAddress(fundingAddress)
+ if (currentInvitation?.txid != null) {
+ authExtension.freshKey(AuthenticationKeyChain.KeyChainType.INVITATION_FUNDING)
+ }
+ } while (currentInvitation?.txid != 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.
| suspend fun sendInviteTransaction(value: Coin): String { | |
| // ensure that the fundingAddress hasn't been used | |
| withContext(Dispatchers.IO) { | |
| val invitation = invitationDao.loadByFundingAddress(fundingAddress) | |
| while (invitation?.txid != null) { | |
| authExtension.freshKey(AuthenticationKeyChain.KeyChainType.INVITATION_FUNDING) | |
| } | |
| } | |
| val fundingAddress = this.fundingAddress // save the address locally | |
| suspend fun sendInviteTransaction(value: Coin): String { | |
| // ensure that the fundingAddress hasn't been used | |
| withContext(Dispatchers.IO) { | |
| var currentInvitation: Invitation? | |
| do { | |
| currentInvitation = invitationDao.loadByFundingAddress(fundingAddress) | |
| if (currentInvitation?.txid != null) { | |
| authExtension.freshKey(AuthenticationKeyChain.KeyChainType.INVITATION_FUNDING) | |
| } | |
| } while (currentInvitation?.txid != null) | |
| } | |
| val fundingAddress = this.fundingAddress // save the address locally | |
| ... | |
| } |
| private val workerJob = Job() | ||
| private val workerScope = CoroutineScope(workerJob + Dispatchers.IO) | ||
| private val authExtension = platformRepo.authenticationGroupExtension!! |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the custom Job is canceled to avoid leaks
Since you're creating a standalone workerJob and workerScope, override onCleared() in your view model to call workerJob.cancel(). This will properly terminate any running coroutines and free resources:
override fun onCleared() {
super.onCleared()
+ workerJob.cancel()
}📝 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 val workerJob = Job() | |
| private val workerScope = CoroutineScope(workerJob + Dispatchers.IO) | |
| private val authExtension = platformRepo.authenticationGroupExtension!! | |
| private val workerJob = Job() | |
| private val workerScope = CoroutineScope(workerJob + Dispatchers.IO) | |
| private val authExtension = platformRepo.authenticationGroupExtension!! | |
| override fun onCleared() { | |
| super.onCleared() | |
| workerJob.cancel() | |
| } |
| InvitationFeeDialogFragmentDirections.toConfirmInviteDialog(selectedFee.value) | ||
| ) | ||
| // TODO: why doesn't safeNavigate work | ||
| // safeNavigate(InvitationFeeDialogFragmentDirections.toConfirmInviteDialog(selectedFee.value)) |
There was a problem hiding this comment.
If I recall correctly, we need to use dialogSafeNavigate from a OffsetDialogFragment because it has navigator of a different type
…feat/update-create-invite
HashEngineering
changed the base branch from
prepare-release-11.1
to
feature-invites
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements