feat: add transfer key to new and existing identities#1470
Merged
HashEngineering merged 15 commits intomasterfrom Mar 10, 2026
Merged
feat: add transfer key to new and existing identities#1470HashEngineering merged 15 commits intomasterfrom
HashEngineering merged 15 commits intomasterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new IdentityRepository (interface + implementation), migrates identity-related logic and DI wiring from PlatformRepo into IdentityRepository across services, workers, ViewModels, and UI, trims PlatformRepo surface, and bumps two build version constants. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as View / ViewModel
participant ID as IdentityRepository
participant DB as AppDatabase
participant PL as PlatformRepo
participant API as Platform API
UI->>ID: request user / balance / search / contact lookup
ID->>DB: read cached identity/profile/contacts
DB-->>ID: cached data
alt cached sufficient
ID-->>UI: return cached result / Flow update
else need remote resolution
ID->>PL: resolve username / profile / documents
PL-->>ID: platform documents / identifiers
ID->>API: call platform API for balance / upgrades
API-->>ID: balances / responses
ID->>DB: persist updates
DB-->>ID: ack
ID-->>UI: return aggregated result / Flow update
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt (4)
382-389:⚠️ Potential issue | 🟡 MinorPotential NPE:
blockchainIdentity!!without null guard.The non-null assertion on
blockchainIdentitycan crash if identity is null. Consider adding an early guard or using a more descriptive exception.🛡️ Proposed fix with explicit null check
private suspend fun addTopUp(txId: Sha256Hash): TopUp { + val identity = identityRepository.blockchainIdentity + ?: throw IllegalStateException("Cannot add top-up without blockchain identity") val topUp = TopUp( txId, - identityRepository.blockchainIdentity!!.uniqueIdString + identity.uniqueIdString ) topUpsDao.insert(topUp) return topUp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt` around lines 382 - 389, The addTopUp function uses identityRepository.blockchainIdentity!! which can throw an NPE; update addTopUp to null-check identityRepository.blockchainIdentity before using it (or retrieve it into a local val and check), and if null either throw a descriptive IllegalStateException (e.g., "blockchain identity not initialized") or return a recoverable error; ensure the TopUp constructor still receives a non-null uniqueIdString only after the guard and keep the topUpsDao.insert(topUp) call after the check.
539-554:⚠️ Potential issue | 🟠 MajorHigher-risk NPE in fallback path.
When
topUpis null, the code falls back toidentityRepository.blockchainIdentity!!.uniqueIdentifier. This fallback path will definitely be executed for new transactions, making the NPE risk more likely.🛡️ Proposed fix with early guard
log.info("checking all topup transactions") + val blockchainIdentity = identityRepository.blockchainIdentity + if (blockchainIdentity == null) { + log.warn("Cannot check topup transactions: no blockchain identity") + checkedPreviousTopUps = true + return + } authExtension.topupFundingTransactions.forEach { assetLockTx -> val topUp = topUpsDao.getByTxId(assetLockTx.txId) if (topUp == null || topUp.notUsed()) { - val identity = topUp?.toUserId ?: identityRepository.blockchainIdentity!!.uniqueIdentifier.toString() + val identity = topUp?.toUserId ?: blockchainIdentity.uniqueIdentifier.toString() if (topUp == null) { topUpsDao.insert(TopUp(assetLockTx.txId, identity)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt` around lines 539 - 554, The null-check logic can still NPE because it force-unwraps identityRepository.blockchainIdentity and platformRepo.getWalletEncryptionKey(); modify the branch handling when topUp == null to safely obtain the identity and wallet key before using them: first retrieve val identityObj = identityRepository.blockchainIdentity and if it's null skip or log and continue, then compute val identity = topUp?.toUserId ?: identityObj.uniqueIdentifier.toString() and only then call topUpsDao.insert(TopUp(...)) and topUpIdentity(...); also avoid force-unwrapping platformRepo.getWalletEncryptionKey() by obtaining the key into a val and skipping/handling the operation if it is null so topUpIdentity is never called with a null key.
419-426:⚠️ Potential issue | 🟡 MinorSame NPE risk pattern with
blockchainIdentity!!.This follows the same pattern as
addTopUp. If the entry points to top-up flows don't guarantee identity existence, this will crash.🛡️ Proposed fix with explicit null check
+ val identity = identityRepository.blockchainIdentity + ?: throw IllegalStateException("Cannot top up without blockchain identity") try { - identityRepository.blockchainIdentity!!.topUp( + identity.topUp( topUpTx, aesKeyParameter, useISLock = true, waitForChainlock = true )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt` around lines 419 - 426, The code uses identityRepository.blockchainIdentity!! when calling topUp (with topUpTx and aesKeyParameter), which can NPE if blockchainIdentity is null; change this to an explicit null check before calling identityRepository.blockchainIdentity.topUp (or use a safe-call with elvis): if identityRepository.blockchainIdentity == null then log an error (including context like topUpTx.txId) and abort/return an error instead of invoking topUp, otherwise call blockchainIdentity.topUp(topUpTx, aesKeyParameter, useISLock = true, waitForChainlock = true); apply the same pattern you used for addTopUp to ensure no NPE.
674-686:⚠️ Potential issue | 🟡 MinorNPE risk with
dashPayProfile!!on line 678.
getLocalUserProfile()may return null, butdashPayProfile!!is used without a prior null check. The existingassetLockTx != nullcheck doesn't guarantee profile existence.🛡️ Proposed fix with null check
- val dashPayProfile = identityRepository.getLocalUserProfile() val assetLockTx = fundingTxes[invitation.txid] - if (assetLockTx != null) { - val appsFlyerLink = createAppsFlyerLink(dashPayProfile!!, assetLockTx, encryptionKey) + val dashPayProfile = identityRepository.getLocalUserProfile() + if (assetLockTx != null && dashPayProfile != null) { + val appsFlyerLink = createAppsFlyerLink(dashPayProfile, assetLockTx, encryptionKey)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt` around lines 674 - 686, The code calls identityRepository.getLocalUserProfile() and then force-unwraps dashPayProfile with dashPayProfile!! before passing it to createAppsFlyerLink, which can NPE; change this to safely handle a null profile by checking dashPayProfile for null (e.g., if (dashPayProfile != null) { ... } or return/skip when null) before calling createAppsFlyerLink and updateInvitation so createAppsFlyerLink and updateInvitation are only invoked with a non-null profile; reference dashPayProfile, getLocalUserProfile(), createAppsFlyerLink(...), updateInvitation(...) and invitation.copy(...) when applying the fix.wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt (1)
555-565:⚠️ Potential issue | 🟠 MajorUse the active/latest account reference when deriving the contact address.
Line 562 resolves the next contact address from
mostRecentContactRequest!!.accountReference, but the code above computesfirstTimestamp = min(...), so this is actually the oldest request. With transfer keys keyed byaccountReference, re-requested contacts can be resolved against a stale key/address here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt` around lines 555 - 565, The code uses firstTimestamp = min(...) and then selects mostRecentContactRequest = map[firstTimestamp], which actually picks the oldest request; change the logic to select the latest contact request (use max of contactRequest.timestamp or compute lastTimestamp) and then pass that request's accountReference into identityRepository.getNextContactAddress(dashPayProfile.userId, ...). Update references to firstTimestamp/mostRecentContactRequest to reflect the latest timestamp so the derived contact address uses the active/latest accountReference from dashPayContactRequests.wallet/src/de/schildbach/wallet/service/platform/PlatformBroadcastService.kt (1)
91-112:⚠️ Potential issue | 🟠 MajorVerify transfer key upgrade path for existing contacts.
Lines 102-108 skip
addPaymentKeyChainFromContact()once a receiving keychain exists.upgradeIdentity()only adds missing keys to the user's own identity, not to contact keychains. IfaddPaymentKeyChainFromContact()is the only place that attaches transfer keys to contacts, existing identities will never receive upgrades when contact request parameters change (e.g., newaccountReference). Confirm whether keychains are mutated in-place during sync, or implement an explicit upgrade mechanism for already-linked contacts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/service/platform/PlatformBroadcastService.kt` around lines 91 - 112, The code skips calling addPaymentKeyChainFromContact(...) when hasReceivingKeyChain(contact) is true, which prevents existing contacts from receiving transfer key upgrades; update the logic so that after creating the contact request (platform.contactRequests.create) you always verify and apply any missing transfer/transfer-account keys for existing contacts (not only on missing receiving chain) by invoking an explicit upgrade path—either call a new helper (e.g., upgradeContactKeyChainIfNeeded) or call addPaymentKeyChainFromContact(...) conditionally to add missing transfer keys for EvolutionContact instances even when walletDataProvider.wallet!!.hasReceivingKeyChain(contact) is true, and ensure you still propagate context (Context.propagate(...)), persist changes via identityRepository.updateDashPayContactRequest(...) and trigger platformSyncService.postUpdateBloomFilters() as appropriate; also consider leveraging upgradeIdentity(...) semantics to detect and merge missing keys rather than relying solely on in-place mutation.wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt (1)
309-312:⚠️ Potential issue | 🟡 MinorAdd a descriptive message to the exception.
Similar to the other case, this
IllegalStateException()lacks context. This pattern appears in bothcreateIdentityandcreateIdentityFromInvitationmethods.Proposed fix
else -> { - throw IllegalStateException() + throw IllegalStateException("Username must be provided when no existing blockchain identity data exists") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt` around lines 309 - 312, The IllegalStateException thrown in the else branches of createIdentity and createIdentityFromInvitation lacks context; update those throw sites to include a descriptive message explaining why this state is illegal (e.g., include the unexpected enum/value or operation name) mirroring the pattern used elsewhere in the file so logs show meaningful diagnostics; locate the throw in methods createIdentity and createIdentityFromInvitation and replace IllegalStateException() with IllegalStateException("descriptive message about the unexpected state or input").wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
699-708:⚠️ Potential issue | 🔴 CriticalGuard
blockchainIdentitybefore resolving transaction contacts.
observeContacts()is backed by the local DB, so it can emit beforeIdentityRepository.init()finishes. In that window, Line 703 dereferencesblockchainIdentity!!and crashes the home-screen refresh.Suggested fix
viewModelWorkerScope.launch { + val blockchainIdentity = identityRepository.blockchainIdentity ?: return@launch val contactsMap = if (this@MainViewModel.contacts.isNotEmpty()) { txs.filterNot { it.isEntirelySelf(walletData.transactionBag) } .mapNotNull { tx -> - identityRepository.blockchainIdentity!!.getContactForTransaction(tx)?.let { contactId -> + blockchainIdentity.getContactForTransaction(tx)?.let { contactId -> contacts[contactId]?.let { contact -> tx.txId to contact } } }.toMap()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt` around lines 699 - 708, The crash occurs because observeContacts()'s coroutine dereferences identityRepository.blockchainIdentity!! before IdentityRepository.init() may have set it; update the mapping inside MainViewModel's viewModelWorkerScope.launch so it first checks identityRepository.blockchainIdentity for null and only resolves contacts when non-null (e.g., skip or return empty map if blockchainIdentity is null), replacing the forced !! with a safe check or safe-call against identityRepository.blockchainIdentity.getContactForTransaction(tx) to avoid NPEs in getContactForTransaction.
🧹 Nitpick comments (3)
wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsLiveData.kt (1)
31-31: Remove unusedplatformRepoparameter from NotificationsLiveData and update NotificationsViewModel.The
platformRepoparameter in NotificationsLiveData (line 31) is never used in the class. After migratingshouldShowAlert()andsearchContacts()toidentityRepository, this parameter can be safely removed. Additionally, update the instantiation in NotificationsViewModel to removeplatformRepofrom the constructor call.Required changes
wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsLiveData.kt:
open class NotificationsLiveData( private val identityRepository: IdentityRepository, - val platformRepo: PlatformRepo, platformSyncService: PlatformSyncService, protected val scope: CoroutineScope, private val userAlertDao: UserAlertDao ): ContactsBasedLiveData<Resource<List<NotificationItem>>>(platformSyncService) {wallet/src/de/schildbach/wallet/ui/dashpay/notification/NotificationsViewModel.kt:
- val notificationsLiveData = NotificationsLiveData(identityRepository, platformRepo, platformSyncService, viewModelScope, userAlertDao) + val notificationsLiveData = NotificationsLiveData(identityRepository, platformSyncService, viewModelScope, userAlertDao)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsLiveData.kt` at line 31, Remove the unused constructor parameter platformRepo from the NotificationsLiveData class and update all call sites (notably the NotificationsViewModel instantiation) to stop passing platformRepo; since shouldShowAlert() and searchContacts() are now on identityRepository, delete the platformRepo parameter declaration in NotificationsLiveData and remove it from the NotificationsViewModel constructor call where NotificationsLiveData(...) is created, ensuring the class compiles and no other references to platformRepo remain in NotificationsLiveData.wallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.kt (1)
63-63: Consider makingidentityRepositoryprivate.The property is exposed as public (
valwithoutprivate), which differs from other injected dependencies in this class (e.g.,analytics,dashSystemService,walletDataare private). Unless there's a specific need for external access, consider making it private for consistency.Proposed fix
- val identityRepository: IdentityRepository, + private val identityRepository: IdentityRepository,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.kt` at line 63, The injected IdentityRepository property in DashPayUserActivityViewModel is currently public (val identityRepository) while other injected dependencies are private; make it private to match consistency by changing the declaration to a private val identityRepository so external access is prevented unless required—update any internal usages within DashPayUserActivityViewModel to reference the now-private identityRepository and ensure no external callers rely on it before making the change.wallet/src/de/schildbach/wallet/ui/dashpay/NotificationCountLiveData.kt (1)
28-28: Remove unusedplatformRepoparameter fromNotificationCountLiveData.This parameter was passed to the constructor but is never used in the class. After the migration to
identityRepository, it can be safely removed from both the class constructor and the instantiation in MainViewModel.kt (line 294).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/ui/dashpay/NotificationCountLiveData.kt` at line 28, Remove the unused constructor parameter platformRepo from the NotificationCountLiveData class definition (symbol: NotificationCountLiveData) and update its constructor call sites to no longer pass platformRepo — specifically remove the argument in the instantiation inside MainViewModel (symbol: MainViewModel, around the previous call at the old line 294). Ensure any imports or references to PlatformRepo in NotificationCountLiveData are deleted if unused, and run a build to confirm no other call sites pass platformRepo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt`:
- Around line 123-145: The singleton must clear its in-memory identity and the
upgrade flag when storage is cleared or identity is absent: change
_blockchainIdentity from a lateinit var to a nullable var (e.g., private var
_blockchainIdentity: BlockchainIdentity? = null), update the blockchainIdentity
getter to return _blockchainIdentity directly and hasBlockchainIdentity to check
_blockchainIdentity != null, then in clearBlockchainIdentityData() set
_blockchainIdentity = null and hasCheckedIdentityForUpgrade = false; apply the
same clearing logic where storage-absence is handled (the similar block around
lines 208-213) so the previous wallet's identity and the
hasCheckedIdentityForUpgrade latch are both cleared.
- Around line 366-408: The upgrade flow latches hasCheckedIdentityForUpgrade too
early and treats a null/getIdentityBalance miss as success, causing retries to
stop even when keys are still missing; fix by: in upgradeIdentity() don't set
hasCheckedIdentityForUpgrade before calling addMissingKeys — only set it when
addMissingKeys returns true; in addMissingKeys(), treat null balance or missing
wallet/identity initialization as a retry-needed case and call
dashPayConfig.set(UPGRADE_IDENTITY_REQUIRED, true) (rather than leaving it
unset), and ensure you check the actual in-memory identity object
(_blockchainIdentity) initialization/state instead of hasIdentity() so you only
proceed when the object is fully initialized; keep existing success path to set
UPGRADE_IDENTITY_REQUIRED=false when keys were added.
- Around line 889-899: getIdentityFromPublicKeyId currently collapses "lookup
unavailable" into a null identity; instead, when
platformRepo.getWalletEncryptionKey() returns null (unencrypted wallet) you must
surface that as a lookup-unavailable condition rather than a definitive miss
(e.g. throw or return a specific error/exception), and do not swallow retryable
DAPI failures — do not catch MaxRetriesReachedException or
NoAvailableAddressesForRetryException and return null; let them propagate (or
wrap and rethrow) so callers (preBlockDownload) can distinguish
transient/unavailable lookups from "identity not found". Update
getIdentityFromPublicKeyId to check platformRepo.getWalletEncryptionKey(), throw
a LookupUnavailableException (or rethrow the caught exceptions) before calling
platformRepo.getBlockchainIdentityKey and
platform.stateRepository.fetchIdentityFromPubKeyHash, and remove the existing
catch blocks that return null for those exceptions.
In
`@wallet/src/de/schildbach/wallet/service/platform/PlatformBroadcastService.kt`:
- Line 91: Replace unsafe non-null assertions on
identityRepository.blockchainIdentity in PlatformBroadcastService with explicit
null checks and descriptive failures: in each public method that currently does
val blockchainIdentity = identityRepository.blockchainIdentity!! (the three call
sites), use a safe read like val blockchainIdentity =
identityRepository.blockchainIdentity ?: throw IllegalStateException("blockchain
identity not available; ensure identity is loaded before calling
PlatformBroadcastService.<methodName>") or return a failure result/error
response appropriate to the method API. Update the three occurrences (the
statements creating blockchainIdentity) so they surface a clear, descriptive
error instead of allowing an NPE.
In `@wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt`:
- Around line 182-186: The init() currently launches identityRepository.init()
and initializeStateRepository() asynchronously and returns immediately, causing
a race with preBlockDownload(), initSync(), and updateContactRequests() which
read hasBlockchainIdentity/blockchainIdentity and stateRepository; change
PlatformSyncService.init() so it waits for those initializations to complete
before returning (e.g., start a coroutine for identityRepository.init() and
initializeStateRepository() and call job.join(), or run them inside runBlocking)
so identityRepository.init() and initializeStateRepository() finish before any
calls to preBlockDownload(), initSync(), or updateContactRequests() occur.
- Around line 1726-1737: The initializeStateRepository() seeding misses
identities from sent/accepted contact requests, causing sent contacts without
profile rows to be unmarked; update the method to also iterate
dashPayContactRequestDao.loadFromMe(identityId) (or the DAO method that returns
sent requests) and call
platform.stateRepository.addValidIdentity(it.userIdentifier) for each entry, in
addition to the existing dashPayContactRequestDao.loadFromOthers(...) and
dashPayProfileDao.loadAll() calls, before calling
platform.stateRepository.storeIdentity(blockchainIdentity.identity!!).
In `@wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt`:
- Around line 539-541: Replace the bare IllegalStateException() in the else
branch inside CreateIdentityService (the else shown in the diff) with an
exception that includes a descriptive message and relevant state context (e.g.
the unexpected branch name or the variable representing the current state); for
example, throw IllegalStateException("Unexpected identity creation state:
$state") or otherwise interpolate the appropriate local variables so the log
shows why this branch was reached. Ensure you reference the same local
identifier(s) used in the surrounding when/if branches so the message is
informative for debugging.
In `@wallet/src/de/schildbach/wallet/ui/dashpay/work/UpdateProfileWorker.kt`:
- Line 84: The worker currently dereferences blockchainIdentity (from
identityRepository.blockchainIdentity) with !! outside the try, which can throw
if the identity is missing; update the UpdateProfileWorker logic to check for a
null blockchainIdentity before building the profile payload (or wrap the
dereference inside the existing try), and if it's null return
Result.failure(...) (or a specific retry/failure as appropriate) instead of
letting an exception escape; reference the blockchainIdentity variable and
identityRepository.blockchainIdentity and ensure the profile-building path
(where blockchainIdentity was used) only runs when the identity is non-null.
In `@wallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.kt`:
- Around line 194-195: The code uses identityRepository.blockchainIdentity!!
which can throw an NPE; update the DashPayUserActivityViewModel code to
null-check blockchainIdentity (or use a safe-call + let) before calling
getContactTransactions: if blockchainIdentity is null return early or handle the
error path, otherwise call
blockchainIdentity.getContactTransactions(Identifier.from(userId),
accountReference); ensure any downstream code expects the possibly-absent
result.
In `@wallet/src/de/schildbach/wallet/ui/invite/InviteHandler.kt`:
- Around line 48-49: InviteHandler currently declares `@Inject` lateinit vars
platformRepo and identityRepository but is manually instantiated, so Hilt won't
inject them; replace the `@Inject` properties by adding PlatformRepo and
IdentityRepository parameters to InviteHandler's constructor (remove `@Inject` and
lateinit), update all call sites that create InviteHandler to pass these
dependencies, and then use the injected constructor fields when calling
identityRepository.clearBlockchainIdentityData() (and any platformRepo usage);
alternatively, if you prefer Hilt-managed creation, convert InviteHandler to use
an `@Inject` constructor and obtain instances from Hilt-managed components instead
of manual instantiation.
---
Outside diff comments:
In
`@wallet/src/de/schildbach/wallet/service/platform/PlatformBroadcastService.kt`:
- Around line 91-112: The code skips calling addPaymentKeyChainFromContact(...)
when hasReceivingKeyChain(contact) is true, which prevents existing contacts
from receiving transfer key upgrades; update the logic so that after creating
the contact request (platform.contactRequests.create) you always verify and
apply any missing transfer/transfer-account keys for existing contacts (not only
on missing receiving chain) by invoking an explicit upgrade path—either call a
new helper (e.g., upgradeContactKeyChainIfNeeded) or call
addPaymentKeyChainFromContact(...) conditionally to add missing transfer keys
for EvolutionContact instances even when
walletDataProvider.wallet!!.hasReceivingKeyChain(contact) is true, and ensure
you still propagate context (Context.propagate(...)), persist changes via
identityRepository.updateDashPayContactRequest(...) and trigger
platformSyncService.postUpdateBloomFilters() as appropriate; also consider
leveraging upgradeIdentity(...) semantics to detect and merge missing keys
rather than relying solely on in-place mutation.
In `@wallet/src/de/schildbach/wallet/service/platform/TopUpRepository.kt`:
- Around line 382-389: The addTopUp function uses
identityRepository.blockchainIdentity!! which can throw an NPE; update addTopUp
to null-check identityRepository.blockchainIdentity before using it (or retrieve
it into a local val and check), and if null either throw a descriptive
IllegalStateException (e.g., "blockchain identity not initialized") or return a
recoverable error; ensure the TopUp constructor still receives a non-null
uniqueIdString only after the guard and keep the topUpsDao.insert(topUp) call
after the check.
- Around line 539-554: The null-check logic can still NPE because it
force-unwraps identityRepository.blockchainIdentity and
platformRepo.getWalletEncryptionKey(); modify the branch handling when topUp ==
null to safely obtain the identity and wallet key before using them: first
retrieve val identityObj = identityRepository.blockchainIdentity and if it's
null skip or log and continue, then compute val identity = topUp?.toUserId ?:
identityObj.uniqueIdentifier.toString() and only then call
topUpsDao.insert(TopUp(...)) and topUpIdentity(...); also avoid force-unwrapping
platformRepo.getWalletEncryptionKey() by obtaining the key into a val and
skipping/handling the operation if it is null so topUpIdentity is never called
with a null key.
- Around line 419-426: The code uses identityRepository.blockchainIdentity!!
when calling topUp (with topUpTx and aesKeyParameter), which can NPE if
blockchainIdentity is null; change this to an explicit null check before calling
identityRepository.blockchainIdentity.topUp (or use a safe-call with elvis): if
identityRepository.blockchainIdentity == null then log an error (including
context like topUpTx.txId) and abort/return an error instead of invoking topUp,
otherwise call blockchainIdentity.topUp(topUpTx, aesKeyParameter, useISLock =
true, waitForChainlock = true); apply the same pattern you used for addTopUp to
ensure no NPE.
- Around line 674-686: The code calls identityRepository.getLocalUserProfile()
and then force-unwraps dashPayProfile with dashPayProfile!! before passing it to
createAppsFlyerLink, which can NPE; change this to safely handle a null profile
by checking dashPayProfile for null (e.g., if (dashPayProfile != null) { ... }
or return/skip when null) before calling createAppsFlyerLink and
updateInvitation so createAppsFlyerLink and updateInvitation are only invoked
with a non-null profile; reference dashPayProfile, getLocalUserProfile(),
createAppsFlyerLink(...), updateInvitation(...) and invitation.copy(...) when
applying the fix.
In `@wallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.kt`:
- Around line 309-312: The IllegalStateException thrown in the else branches of
createIdentity and createIdentityFromInvitation lacks context; update those
throw sites to include a descriptive message explaining why this state is
illegal (e.g., include the unexpected enum/value or operation name) mirroring
the pattern used elsewhere in the file so logs show meaningful diagnostics;
locate the throw in methods createIdentity and createIdentityFromInvitation and
replace IllegalStateException() with IllegalStateException("descriptive message
about the unexpected state or input").
In `@wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt`:
- Around line 699-708: The crash occurs because observeContacts()'s coroutine
dereferences identityRepository.blockchainIdentity!! before
IdentityRepository.init() may have set it; update the mapping inside
MainViewModel's viewModelWorkerScope.launch so it first checks
identityRepository.blockchainIdentity for null and only resolves contacts when
non-null (e.g., skip or return empty map if blockchainIdentity is null),
replacing the forced !! with a safe check or safe-call against
identityRepository.blockchainIdentity.getContactForTransaction(tx) to avoid NPEs
in getContactForTransaction.
In `@wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt`:
- Around line 555-565: The code uses firstTimestamp = min(...) and then selects
mostRecentContactRequest = map[firstTimestamp], which actually picks the oldest
request; change the logic to select the latest contact request (use max of
contactRequest.timestamp or compute lastTimestamp) and then pass that request's
accountReference into
identityRepository.getNextContactAddress(dashPayProfile.userId, ...). Update
references to firstTimestamp/mostRecentContactRequest to reflect the latest
timestamp so the derived contact address uses the active/latest accountReference
from dashPayContactRequests.
---
Nitpick comments:
In `@wallet/src/de/schildbach/wallet/ui/dashpay/NotificationCountLiveData.kt`:
- Line 28: Remove the unused constructor parameter platformRepo from the
NotificationCountLiveData class definition (symbol: NotificationCountLiveData)
and update its constructor call sites to no longer pass platformRepo —
specifically remove the argument in the instantiation inside MainViewModel
(symbol: MainViewModel, around the previous call at the old line 294). Ensure
any imports or references to PlatformRepo in NotificationCountLiveData are
deleted if unused, and run a build to confirm no other call sites pass
platformRepo.
In `@wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsLiveData.kt`:
- Line 31: Remove the unused constructor parameter platformRepo from the
NotificationsLiveData class and update all call sites (notably the
NotificationsViewModel instantiation) to stop passing platformRepo; since
shouldShowAlert() and searchContacts() are now on identityRepository, delete the
platformRepo parameter declaration in NotificationsLiveData and remove it from
the NotificationsViewModel constructor call where NotificationsLiveData(...) is
created, ensuring the class compiles and no other references to platformRepo
remain in NotificationsLiveData.
In `@wallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.kt`:
- Line 63: The injected IdentityRepository property in
DashPayUserActivityViewModel is currently public (val identityRepository) while
other injected dependencies are private; make it private to match consistency by
changing the declaration to a private val identityRepository so external access
is prevented unless required—update any internal usages within
DashPayUserActivityViewModel to reference the now-private identityRepository and
ensure no external callers rely on it before making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc34c452-374e-4673-96fd-5852efd846cb
📒 Files selected for processing (29)
build.gradlewallet/src/de/schildbach/wallet/WalletApplication.javawallet/src/de/schildbach/wallet/WalletApplicationExt.ktwallet/src/de/schildbach/wallet/di/AppModule.ktwallet/src/de/schildbach/wallet/di/DashPayModule.ktwallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.ktwallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.ktwallet/src/de/schildbach/wallet/service/CoinJoinService.ktwallet/src/de/schildbach/wallet/service/platform/IdentityRepository.ktwallet/src/de/schildbach/wallet/service/platform/PlatformBroadcastService.ktwallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.ktwallet/src/de/schildbach/wallet/service/platform/TopUpRepository.ktwallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.ktwallet/src/de/schildbach/wallet/service/platform/work/TopupIdentityWorker.ktwallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.ktwallet/src/de/schildbach/wallet/ui/TransactionResultViewModel.ktwallet/src/de/schildbach/wallet/ui/dashpay/CreateIdentityService.ktwallet/src/de/schildbach/wallet/ui/dashpay/DashPayViewModel.ktwallet/src/de/schildbach/wallet/ui/dashpay/EditProfileViewModel.ktwallet/src/de/schildbach/wallet/ui/dashpay/NotificationCountLiveData.ktwallet/src/de/schildbach/wallet/ui/dashpay/NotificationsLiveData.ktwallet/src/de/schildbach/wallet/ui/dashpay/PlatformRepo.ktwallet/src/de/schildbach/wallet/ui/dashpay/notification/NotificationsViewModel.ktwallet/src/de/schildbach/wallet/ui/dashpay/utils/DashPayConfig.ktwallet/src/de/schildbach/wallet/ui/dashpay/work/SendInviteWorker.ktwallet/src/de/schildbach/wallet/ui/dashpay/work/UpdateProfileWorker.ktwallet/src/de/schildbach/wallet/ui/invite/InviteHandler.ktwallet/src/de/schildbach/wallet/ui/main/MainViewModel.ktwallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt
wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+889
to
+899
| override fun getIdentityFromPublicKeyId(): Identity? { | ||
| return try { | ||
| platformRepo.getWalletEncryptionKey()?.let { | ||
| val firstIdentityKey = platformRepo.getBlockchainIdentityKey(0, it) ?: return null | ||
| platform.stateRepository.fetchIdentityFromPubKeyHash(firstIdentityKey.pubKeyHash) | ||
| } | ||
| } catch (e: MaxRetriesReachedException) { | ||
| null | ||
| } catch (e: NoAvailableAddressesForRetryException) { | ||
| null | ||
| } |
Contributor
There was a problem hiding this comment.
Don't collapse 'lookup unavailable' into 'identity not found'.
On unencrypted wallets getWalletEncryptionKey() returns null, so the safe-call skips the pubkey lookup entirely. The catch blocks also fold retryable DAPI failures into the same null result. preBlockDownload() treats that as a definitive miss and will skip recovery for identities that actually exist.
Suggested fix
override fun getIdentityFromPublicKeyId(): Identity? {
return try {
- platformRepo.getWalletEncryptionKey()?.let {
- val firstIdentityKey = platformRepo.getBlockchainIdentityKey(0, it) ?: return null
- platform.stateRepository.fetchIdentityFromPubKeyHash(firstIdentityKey.pubKeyHash)
- }
+ val keyParameter = platformRepo.getWalletEncryptionKey()
+ if (walletApplication.wallet!!.isEncrypted && keyParameter == null) {
+ throw IllegalStateException("Wallet encryption key unavailable")
+ }
+ val firstIdentityKey = platformRepo.getBlockchainIdentityKey(0, keyParameter) ?: return null
+ platform.stateRepository.fetchIdentityFromPubKeyHash(firstIdentityKey.pubKeyHash)
} catch (e: MaxRetriesReachedException) {
- null
+ throw e
} catch (e: NoAvailableAddressesForRetryException) {
- null
+ throw e
}
}🧰 Tools
🪛 detekt (1.23.8)
[warning] 895-895: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 897-897: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt`
around lines 889 - 899, getIdentityFromPublicKeyId currently collapses "lookup
unavailable" into a null identity; instead, when
platformRepo.getWalletEncryptionKey() returns null (unencrypted wallet) you must
surface that as a lookup-unavailable condition rather than a definitive miss
(e.g. throw or return a specific error/exception), and do not swallow retryable
DAPI failures — do not catch MaxRetriesReachedException or
NoAvailableAddressesForRetryException and return null; let them propagate (or
wrap and rethrow) so callers (preBlockDownload) can distinguish
transient/unavailable lookups from "identity not found". Update
getIdentityFromPublicKeyId to check platformRepo.getWalletEncryptionKey(), throw
a LookupUnavailableException (or rethrow the caught exceptions) before calling
platformRepo.getBlockchainIdentityKey and
platform.stateRepository.fetchIdentityFromPubKeyHash, and remove the existing
catch blocks that return null for those exceptions.
Collaborator
Author
There was a problem hiding this comment.
we don't have a good way to handle this failure. We would need to stop sync process otherwise a rescan blockchain is required.
Contributor
There was a problem hiding this comment.
@HashEngineering, understood — if the caller can't distinguish "transient failure" from "no identity" without stopping the entire sync, propagating the exceptions becomes a much larger change.
As a minimal improvement, you could at least log the swallowed exceptions at warn/error level before returning null, so operators can see in the logs when a DAPI retry failure is masquerading as a clean "not found." This also satisfies the detekt SwallowedException warnings without changing any control flow:
override fun getIdentityFromPublicKeyId(): Identity? {
return try {
platformRepo.getWalletEncryptionKey()?.let {
val firstIdentityKey = platformRepo.getBlockchainIdentityKey(0, it) ?: return null
platform.stateRepository.fetchIdentityFromPubKeyHash(firstIdentityKey.pubKeyHash)
}
} catch (e: MaxRetriesReachedException) {
log.warn("getIdentityFromPublicKeyId: DAPI max retries reached, returning null (identity lookup may be incomplete)", e)
null
} catch (e: NoAvailableAddressesForRetryException) {
log.warn("getIdentityFromPublicKeyId: No available DAPI addresses, returning null (identity lookup may be incomplete)", e)
null
}
}The unencrypted wallet case (key = null → safe call skips) is arguably acceptable as-is since there's no key to look up with, though a comment noting the behaviour would help future readers. The larger issue of callers being unable to distinguish transient failures from definitive misses could be tracked as a follow-up.
Would you like me to open a GitHub issue to track the sync-stopping / rescan-required logic as a future improvement?
✏️ Learnings added
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1470
File: wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt:889-899
Timestamp: 2026-03-09T03:31:06.650Z
Learning: In `wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt`, `getIdentityFromPublicKeyId()` intentionally swallows `MaxRetriesReachedException` and `NoAvailableAddressesForRetryException` by returning null. The developer (HashEngineering) confirmed there is no good way to handle these failures without stopping the entire sync process, which would require a rescan blockchain. This is an accepted architectural tradeoff; do not flag these swallowed exceptions as critical issues in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
wallet/src/de/schildbach/wallet/service/platform/PlatformBroadcastService.kt
Outdated
Show resolved
Hide resolved
Comment on lines
182
to
+186
| override fun init() { | ||
| syncScope.launch { platformRepo.init() } | ||
| syncScope.launch { | ||
| identityRepository.init() | ||
| initializeStateRepository() | ||
| } |
Contributor
There was a problem hiding this comment.
Await IdentityRepository initialization before starting sync work.
init() now fires identityRepository.init()/initializeStateRepository() on a background coroutine and returns immediately. preBlockDownload(), initSync(), and updateContactRequests() all rely on hasBlockchainIdentity or blockchainIdentity!!, so a cold start can skip the first sync cycle or run with an empty stateRepository if those entrypoints win the race.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt`
around lines 182 - 186, The init() currently launches identityRepository.init()
and initializeStateRepository() asynchronously and returns immediately, causing
a race with preBlockDownload(), initSync(), and updateContactRequests() which
read hasBlockchainIdentity/blockchainIdentity and stateRepository; change
PlatformSyncService.init() so it waits for those initializations to complete
before returning (e.g., start a coroutine for identityRepository.init() and
initializeStateRepository() and call job.join(), or run them inside runBlocking)
so identityRepository.init() and initializeStateRepository() finish before any
calls to preBlockDownload(), initSync(), or updateContactRequests() occur.
Collaborator
Author
There was a problem hiding this comment.
we will handle this later
Contributor
There was a problem hiding this comment.
@HashEngineering, understood — I'll note this for later. Would you like me to open a GitHub issue to track this race condition so it doesn't get lost?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@HashEngineering, I've created the GitHub issue to track this race condition. You can find it in the repository's issue tracker. The issue captures the root cause, the affected code location, and a proposed fix for future reference.
🏁 Script executed:
gh issue create \
--repo dashpay/dash-wallet \
--title "Race condition: PlatformSyncService.init() doesn't await identity repository initialization" \
--assignee HashEngineering \
--body "## Summary
\`PlatformSynchronizationService.init()\` launches \`identityRepository.init()\` and \`initializeStateRepository()\` on a background coroutine and returns immediately. This creates a race condition where \`preBlockDownload()\`, \`initSync()\`, and \`updateContactRequests()\` may be called before initialization completes, reading \`hasBlockchainIdentity\`/\`blockchainIdentity\` or using an empty \`stateRepository\`.
## Affected File
\`wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt\` (around lines 182–186)
## Root Cause
\`\`\`kotlin
override fun init() {
syncScope.launch {
identityRepository.init()
initializeStateRepository()
}
// Returns immediately — callers may race with the above coroutine
}
\`\`\`
## Proposed Fix
Make \`init()\` wait for \`identityRepository.init()\` and \`initializeStateRepository()\` to complete before any call to \`preBlockDownload()\`, \`initSync()\`, or \`updateContactRequests()\` occurs. Options include:
- Storing the launched job and calling \`job.join()\` in \`preBlockDownload()\`/\`initSync()\` before proceeding.
- Converting \`init()\` to a \`suspend fun\` and awaiting the initializations directly.
## References
- PR: https://github.com/dashpay/dash-wallet/pull/1470
- Review comment: https://github.com/dashpay/dash-wallet/pull/1470#discussion_r2902690539
- Requested by: `@HashEngineering`"
Length of output: 50
@HashEngineering, the issue has been created and is ready for tracking:
#1471 — Race condition: PlatformSyncService.init() doesn't await identity repository initialization
It's assigned to you and includes the root cause, affected file location, and proposed fix options for when you're ready to address it.
wallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt`:
- Around line 621-625: The flow currently filters only on hasUsername then
force-unwraps identityData.userId in the flatMapLatest lambda, which can NPE
during onboarding; change the upstream predicate to ensure userId is non-null
(e.g. add a check like identityData.userId != null or replace
filterNotNull()/filter { it.hasUsername } with a single filter { it.hasUsername
&& it.userId != null }) or safely unwrap userId with let before calling
contacts-related logic so the code in the flatMapLatest (where
identityData.userId!! is referenced) never force-unwraps a null userId.
- Around line 890-892: getLocalUserProfile() currently force-unwraps
loadBlockchainIdentityBaseData() and its userId which will throw after
clearBlockchainIdentityData(); update getLocalUserProfile() to safely handle
nulls by calling loadBlockchainIdentityBaseData()?, check if the returned base
data or baseData.userId is null, and return null when either is missing,
otherwise call dashPayProfileDao.loadByUserId(userId) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c16bb954-8f67-4bf3-b3c9-59573d48b072
📒 Files selected for processing (4)
wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.ktwallet/src/de/schildbach/wallet/service/platform/PlatformBroadcastService.ktwallet/src/de/schildbach/wallet/ui/invite/InviteHandler.ktwallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt
wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.kt`:
- Around line 194-197: The current code early-returns emptyList() when
identityRepository.blockchainIdentity is null, which discards already-built
results; instead, change the logic in DashPayUserActivityViewModel (around
identityRepository.blockchainIdentity check) to log the null identity but do NOT
return from the withContext block—fall back to an empty transaction list (e.g.,
set a local transactionResults = emptyList()) and continue merging with the
existing results variable so previously collected contact/invitation items are
preserved before the function returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99aaf35b-be28-4ff3-a8e0-7545f829c3dd
📒 Files selected for processing (2)
wallet/src/de/schildbach/wallet/ui/DashPayUserActivityViewModel.ktwallet/src/de/schildbach/wallet/ui/dashpay/work/UpdateProfileWorker.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- wallet/src/de/schildbach/wallet/ui/dashpay/work/UpdateProfileWorker.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt (1)
456-469: Consider usingfirstOrNullfor cleaner null handling.Using
firstOrNull { }instead of wrappingfirst { }in a try-catch is more idiomatic Kotlin and eliminates the swallowed exception warning.Proposed refactor
val userIds = if (onlyExactUsername) { val result = mutableListOf<Identifier>() - val exactNameDoc = try { - DomainDocument(nameDocuments.first { text == it.data["normalizedLabel"] }) - } catch (e: NoSuchElementException) { - null - } + val exactNameDoc = nameDocuments + .firstOrNull { text == it.data["normalizedLabel"] } + ?.let { DomainDocument(it) } if (exactNameDoc != null) { result.add(platformRepo.getIdentityForName(exactNameDoc)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt` around lines 456 - 469, Replace the try/catch around nameDocuments.first { ... } with idiomatic firstOrNull to avoid swallowing exceptions: in the onlyExactUsername branch, use nameDocuments.firstOrNull { text == it.data["normalizedLabel"] } to get a nullable element, wrap it in DomainDocument only when non-null (e.g., via let or conditional), then call platformRepo.getIdentityForName as before; this keeps the same behavior for onlyExactUsername and the use of identifierDocumentMap.keys else branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt`:
- Around line 802-807: The KDoc for override suspend fun
clearDatabase(includeInvitations: Boolean) in IdentityRepository.kt is stale and
contradicts the implementation; update it by either removing the lines stating
"This should not be a suspended method" or by revising the comment to accurately
describe why clearDatabase is suspendable (e.g., because it performs
coroutine-based IO) so the doc matches the function signature; modify the KDoc
above the clearDatabase method (the comment block starting "This is used by java
code, outside of coroutines") to reflect the current design or remove the
contradictory sentence.
- Around line 232-240: The two log statements in IdentityRepository.kt are
referencing the class field _blockchainIdentity?.uniqueIdString instead of the
newly constructed local blockchainIdentity, which can be null/stale; update both
log calls to use blockchainIdentity.uniqueIdString (or
blockchainIdentity?.uniqueIdString where needed) and ensure the log message
shows the local object's uniqueIdString and userId (e.g.,
blockchainIdentity.userId / blockchainIdentity?.uniqueIdString) so the logs
reflect the identity being loaded rather than the repository field.
---
Nitpick comments:
In `@wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt`:
- Around line 456-469: Replace the try/catch around nameDocuments.first { ... }
with idiomatic firstOrNull to avoid swallowing exceptions: in the
onlyExactUsername branch, use nameDocuments.firstOrNull { text ==
it.data["normalizedLabel"] } to get a nullable element, wrap it in
DomainDocument only when non-null (e.g., via let or conditional), then call
platformRepo.getIdentityForName as before; this keeps the same behavior for
onlyExactUsername and the use of identifierDocumentMap.keys else branch
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fbdb0ea-9a8c-4849-bd0b-991991583374
📒 Files selected for processing (1)
wallet/src/de/schildbach/wallet/service/platform/IdentityRepository.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
Refactor
Chores