Conversation
📝 WalkthroughWalkthroughThis PR introduces v11.6.1, adding support for instant usernames, non-contested username voting, invitation fees, blockchain rescan with optional wallet creation date selection, and PIN upgrade flows. It includes extensive localization updates across 30+ languages, minor test signature updates for suspend functions, build dependency updates (dashj-scrypt-android 1.0.0→1.0.1, dashjVersion and dppVersion snapshot removal), and targeted logic adjustments for network availability tracking and username voting recovery. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt (1)
270-272:⚠️ Potential issue | 🟡 MinorPre-existing bug: error message is incorrect.
The guard checks
!isAssetLock(i.e.,isAssetLockisfalse), but the message says"isAssetLock must be true, but is true". It should say"but is false".Proposed fix
- error("isAssetLock must be true, but is true") + error("isAssetLock must be true, but is false")wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
934-955:⚠️ Potential issue | 🟠 Major
onCapabilitiesChangedis inconsistent withavailableNetworkstracking, causing incorrect impediment state.When capabilities change, this handler adds/removes the NETWORK impediment based solely on the single network's capabilities, ignoring whether other validated networks exist in
availableNetworks. Scenario:
- WiFi (A) and Cellular (B) both available → both in
availableNetworks, no impediment.- WiFi (A) loses validation →
onCapabilitiesChangedaddsImpediment.NETWORK.- Cellular (B) is still valid, but the impediment is now set, blocking the peer group.
Additionally,
availableNetworksis never updated here, so a network that loses validation remains in the set, making theonLostemptiness check unreliable later.Proposed fix
override fun onCapabilitiesChanged(network: Network, capabilities: NetworkCapabilities) { serviceScope.launch { val hasInternet = capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) val hasValidated = capabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED) if (log.isInfoEnabled) { val s = StringBuilder("network capabilities changed: ") .append(network) .append(", hasInternet: ").append(hasInternet) .append(", validated: ").append(hasValidated) log.info(s.toString()) } if (hasInternet && hasValidated) { + availableNetworks.add(network) impediments.remove(Impediment.NETWORK) } else { - impediments.add(Impediment.NETWORK) + availableNetworks.remove(network) + if (availableNetworks.isEmpty()) { + impediments.add(Impediment.NETWORK) + } } updateBlockchainStateImpediments() check() } }wallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.kt (1)
270-273:⚠️ Potential issue | 🔴 CriticalBug:
minOflambda always references the same outerdocumentWithVotesinstead of each contender's document.The lambda iterates over
voteContenders.map.valuesbut uses the captureddocumentWithVotesfrom the enclosingforEach, so every iteration deserializes the same document. The lambda parameter (it) — which is the actual per-contender value — is shadowed by the innerlet. This yields an incorrectearliestCreatedAt.The same bug exists in the duplicated block at lines 387–392.
🐛 Proposed fix
- val earliestCreatedAt = voteContenders.map.values.minOf { - val document = documentWithVotes.serializedDocument?.let { platformRepo.platform.names.deserialize(it) } - document?.createdAt ?: 0 - } + val earliestCreatedAt = voteContenders.map.values.minOf { contenderDoc -> + val document = contenderDoc.serializedDocument?.let { platformRepo.platform.names.deserialize(it) } + document?.createdAt ?: 0 + }Apply the same fix to the second occurrence (lines 387–392).
🤖 Fix all issues with AI agents
In
`@wallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.kt`:
- Around line 293-411: The review points out duplicated contest-processing logic
between the getCurrentlyContestedNames and getAllContestedNames paths; extract
the shared logic into a private method (suggested name processContestedNames)
that accepts parameters (names: List<String>, blockchainIdentity:
BlockchainIdentity, blockchainIdentityData: BlockchainIdentityData,
maybeDualUsernames: Boolean, instantUsername: String?) and returns a Boolean
indicating foundContestedNameInVotingPeriod; move the loop body that iterates
voteContenders, sets
blockchainIdentity.currentUsername/secondaryUsername/primaryUsername, computes
usernameRequestStatus, inserts UsernameRequest, builds/updates UsernameInfo (fix
the minOf computation to deserialize each contender’s serializedDocument rather
than reusing documentWithVotes), set votingStartedAt/requestStatus, call
platformRepo.updateBlockchainIdentityData(blockchainIdentityData,
blockchainIdentity), and enqueue GetUsernameVotingResultOperation when
appropriate; then replace both original code blocks to call
foundContestedNameInVotingPeriod =
processContestedNames(currentlyContestedNames, ...) and, if false,
foundContestedNameInVotingPeriod = processContestedNames(contestedNames, ...),
removing the duplicated code and the inconsistent null-safe checks.
🟡 Minor comments (21)
wallet/res/values-th/strings-extra.xml-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorInconsistent use of English "wallet" vs. Thai "กระเป๋าสตางค์".
Most other strings in this file translate "wallet" as "กระเป๋าสตางค์" (e.g.,
onboarding_load_wallet_erroron Line 5,set_pin_create_new_walleton Line 7,recover_wallet_titleon Line 70). This string uses the English word "wallet" instead, which creates an inconsistency in the Thai localization.wallet/res/values-ro/strings.xml-237-237 (1)
237-237:⚠️ Potential issue | 🟡 MinorTypo: duplicated word "din" → "Scanează din din nou" should be "Scanează din nou".
The immediately preceding string on line 236 (
preferences_initiate_reset_dialog_positive) has the correct form.Proposed fix
- <string name="rescan_blockchain_confirm_button">Scanează din din nou</string> + <string name="rescan_blockchain_confirm_button">Scanează din nou</string>wallet/res/values-el/strings-extra.xml-338-338 (1)
338-338:⚠️ Potential issue | 🟡 MinorLikely grammatical error: "χωρίς μέσω" doesn't make sense in Greek.
"χωρίς" means "without" and "μέσω" means "through/via" — having both together is contradictory. The intended meaning is probably either:
- "Στείλτε και λάβετε Dash μέσω ονομάτων χρήσης" (Send and receive Dash through usernames), or
- "Στείλτε και λάβετε Dash χωρίς διευθύνσεις, μέσω ονομάτων χρήσης" (without addresses, through usernames)
Please verify with a native speaker or the source string.
wallet/res/values-zh-rTW/strings-extra.xml-335-337 (1)
335-337:⚠️ Potential issue | 🟡 MinorExtraneous spaces around "達世幣" in Chinese text.
Lines 336 and 337 have spaces before/around "達世幣" (e.g.,
花費 達世幣,存儲 達世幣 最安全的方式). In Chinese typography, spaces between Chinese characters and Chinese-context words are not standard and will look odd in the UI. Consider removing them:Proposed fix
<string name="welcome_screen_title_1">通過用戶名發送和接收達世幣</string> - <string name="welcome_screen_title_2">在數千家商店花費 達世幣</string> - <string name="welcome_screen_title_3">存儲 達世幣 最安全的方式</string> + <string name="welcome_screen_title_2">在數千家商店花費達世幣</string> + <string name="welcome_screen_title_3">存儲達世幣最安全的方式</string>wallet/res/values-es/strings-extra.xml-268-268 (1)
268-268:⚠️ Potential issue | 🟡 MinorFix accent typo in the invite menu label.
“Invitaciónes” is misspelled in Spanish; it should be “Invitaciones”.
✏️ Suggested fix
- <string name="menu_invite_title">Invitaciónes</string> + <string name="menu_invite_title">Invitaciones</string>wallet/res/values-es/strings.xml-206-212 (1)
206-212:⚠️ Potential issue | 🟡 MinorFix typo/placeholder in the selected date label.
The current value includes an extra “1” and omits the expected “%s” format alignment for the selected date.
✏️ Suggested fix
- <string name="restore_wallet_selected_date">Selecciono: 1%s</string> + <string name="restore_wallet_selected_date">Seleccionado: %s</string>wallet/res/values-pl/strings-dashpay.xml-531-531 (1)
531-531:⚠️ Potential issue | 🟡 MinorFix typo in the invitation fee CTA.
Line 531 has “Potwiedź”, which should be “Potwierdź”.
✏️ Suggested fix
- <string name="invitation_fee_confirm_and_pay_button">Potwiedź i zapłać</string> + <string name="invitation_fee_confirm_and_pay_button">Potwierdź i zapłać</string>wallet/res/values-id/strings-extra.xml-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorClarify onboarding recovery wording (wallet vs private key).
Line 5 now says “Pulihkan kunci pribadi”, but the key is
onboarding_recover_wallet. This could mislead users about the recovery flow. Consider aligning the text with wallet recovery (or explicitly mention the recovery phrase if that’s the intended UI).✏️ Suggested wording update
- <string name="onboarding_recover_wallet">Pulihkan kunci pribadi</string> + <string name="onboarding_recover_wallet">Pulihkan dompet</string>wallet/res/values-de/strings.xml-207-213 (1)
207-213:⚠️ Potential issue | 🟡 MinorKeep pronoun usage consistent in restore-date copy.
Line 208 uses the formal “Ihrer,” while surrounding strings use the informal “deiner/du.” Please align to one form for UX consistency.
Suggested tweak
- <string name="restore_wallet_creation_date_info">Durch die Auswahl des Erstellungsdatums Ihrer Wallet wird die Synchronisierung beschleunigt, da nur Daten ab diesem Datum gescannt werden.</string> + <string name="restore_wallet_creation_date_info">Durch die Auswahl des Erstellungsdatums deiner Wallet wird die Synchronisierung beschleunigt, da nur Daten ab diesem Datum gescannt werden.</string>wallet/res/values-es/strings-dashpay.xml-20-20 (1)
20-20:⚠️ Potential issue | 🟡 MinorFix the placeholder typo (“1%s”).
There’s an extra “1” before the %s placeholder.
✏️ Suggested fix
- <string name="new_account_secondary_confirm_message">Elegiste 1%s como nombre de usuario instantáneo. Ten en cuenta que el nombre de usuario no se puede cambiar una vez registrado.</string> + <string name="new_account_secondary_confirm_message">Elegiste %s como nombre de usuario instantáneo. Ten en cuenta que el nombre de usuario no se puede cambiar una vez registrado.</string>wallet/res/values-es/strings-dashpay.xml-219-220 (1)
219-220:⚠️ Potential issue | 🟡 MinorUse imperative tone for the wait instruction.
“Esperé” is past tense; UI copy should use imperative/neutral.
✏️ Suggested fix
- <string name="join_dashpay_wait">Esperé a que la sincronización termine para crear un nombre de usuario</string> + <string name="join_dashpay_wait">Espera a que la sincronización termine para crear un nombre de usuario</string>features/exploredash/src/main/res/values-el/strings-explore-dash.xml-194-194 (1)
194-194:⚠️ Potential issue | 🟡 MinorMake this label reflect “check current balance.”
The key suggests a balance check, but the value reads “view card details.” Consider aligning to avoid confusing UX.
✏️ Suggested fix
- <string name="purchase_check_current_balance">Δείτε τις λεπτομέρειες της κάρτας</string> + <string name="purchase_check_current_balance">Ελέγξτε το τρέχον υπόλοιπο της κάρτας</string>wallet/res/values-it/strings-dashpay.xml-524-529 (1)
524-529:⚠️ Potential issue | 🟡 MinorAlign the non‑contested length rule with the 20+ wording used elsewhere.
This string says “più di 20 caratteri,” but earlier copy in this file states “20 o più caratteri,” which implies ≥20. Consider aligning for consistency and to avoid off‑by‑one confusion.
✏️ Suggested fix
- <string name="invitation_fee_noncontested_message">La persona a cui invii questo invito può richiedere uno username che contenga un numero compreso tra 2 e 9 e che sia lungo più di 20 caratteri.</string> + <string name="invitation_fee_noncontested_message">La persona a cui invii questo invito può richiedere uno username che contenga un numero compreso tra 2 e 9 e che sia lungo 20 o più caratteri.</string>wallet/res/values-zh-rTW/strings-dashpay.xml-525-543 (1)
525-543:⚠️ Potential issue | 🟡 MinorFix the Dash network wording and extra punctuation.
- “儀表板網絡” reads as “dashboard network,” which is incorrect here.
- The “。.” sequence introduces an extra period.
✏️ Suggested fix
- <string name="invitation_fee_message">每個邀請將需要得到資助，以便接收者可以在儀表板網絡上快速創建其用戶名稱。</string> + <string name="invitation_fee_message">每個邀請將需要得到資助，以便接收者可以在達世幣網路上快速創建其用戶名稱。</string> @@ - <string name="create_instant_username_description">您請求的用戶名將在 2 週內進行投票。為了立即聯繫和付款，您可以創建類似的即時用戶名。.\n\n如果您的原始請求獲得批准，它將取代即時用戶名，並保留聯繫人和歷史記錄。如果沒有獲批，請繼續使用即時用戶名。</string> + <string name="create_instant_username_description">您請求的用戶名將在 2 週內進行投票。為了立即聯繫和付款，您可以創建類似的即時用戶名。\n\n如果您的原始請求獲得批准，它將取代即時用戶名，並保留聯繫人和歷史記錄。如果沒有獲批，請繼續使用即時用戶名。</string>wallet/res/values-nl/strings-dashpay.xml-454-454 (1)
454-454:⚠️ Potential issue | 🟡 MinorTrim unintended leading spaces in Dutch strings.
The current values render with a visible leading space.
🛠️ Suggested fix
- <string name="request_username_length_requirement_noncontested"> Tussen 20 en 23 tekens</string> + <string name="request_username_length_requirement_noncontested">Tussen 20 en 23 tekens</string> @@ - <string name="invitation_fee_noncontested"> Niet betwist</string> + <string name="invitation_fee_noncontested">Niet betwist</string>Also applies to: 527-527
wallet/res/values-fa/strings-dashpay.xml-523-537 (1)
523-537:⚠️ Potential issue | 🟡 MinorCorrect Persian wording for contested/non‑contested labels and instant‑username text.
The current labels read as approved/unapproved, the “two types” sentence is negated, and there’s a typo in “در عیر”. Please verify the wording with your translator.
🛠️ Suggested fix (please confirm with translator)
- <string name="invitation_fee_noncontested">مورد موافقت قرار نگرفت</string> - <string name="invitation_fee_contested">مورد موافقت قرار گرفت</string> + <string name="invitation_fee_noncontested">غیررقابتی</string> + <string name="invitation_fee_contested">رقابتی</string> @@ - <string name="username_types_message">دو نوع نام کاربری نداریم: توافقشده و توافقنشده.\nنامهای کاربری توافقنشده شامل حداقل یک عدد (۲ تا ۹) هستید و یا بیش از ۲۰ کاراکتر دارند و به صورت خودکار مورد تائید قرار میگیرند.\nاگر میخواهید نام کابری توافقشده ایجاد کنید که کوتاهتر و بدون عدد است، باید بدون دعوتنامه در دشپی ثبتنام کنید، کارمزد مربوطه را بپردازید و منتظر تائید پس از تکمیل دوره رأیگیری بمانید.</string> + <string name="username_types_message">دو نوع نام کاربری داریم: توافقشده و توافقنشده.\nنامهای کاربری توافقنشده شامل حداقل یک عدد (۲ تا ۹) هستید و یا بیش از ۲۰ کاراکتر دارند و به صورت خودکار مورد تائید قرار میگیرند.\nاگر میخواهید نام کابری توافقشده ایجاد کنید که کوتاهتر و بدون عدد است، باید بدون دعوتنامه در دشپی ثبتنام کنید، کارمزد مربوطه را بپردازید و منتظر تائید پس از تکمیل دوره رأیگیری بمانید.</string> @@ - <string name="create_instant_username_description">نام کاربری درخواستی شما طی دو هفته آینده به رای گذاشته میشود. برای ارتباطگیری و پرداختهای فوری، میتوانید یک نام کاربری مشابه فوری بسازید.\nاگر درخواست اصلیتان مورد پذیرش قرار گرفت، آن نام کاربری جایگزین این نام کاربری فوری میشود و همه مخاطبان و سابقه آن نیز حفظ خواهد شد. در عیر این صورت، میتوانید همچنان به استفاده از این نام کاربری فوری ادامه دهید. </string> + <string name="create_instant_username_description">نام کاربری درخواستی شما طی دو هفته آینده به رای گذاشته میشود. برای ارتباطگیری و پرداختهای فوری، میتوانید یک نام کاربری مشابه فوری بسازید.\nاگر درخواست اصلیتان مورد پذیرش قرار گرفت، آن نام کاربری جایگزین این نام کاربری فوری میشود و همه مخاطبان و سابقه آن نیز حفظ خواهد شد. در غیر این صورت، میتوانید همچنان به استفاده از این نام کاربری فوری ادامه دهید. </string>Also applies to: 542-542
wallet/res/values-fa/strings-dashpay.xml-19-19 (1)
19-19:⚠️ Potential issue | 🟡 MinorFix missing space around the
%splaceholder.Without the space, the username will render glued to the following word.
🛠️ Suggested fix
- <string name="new_account_secondary_confirm_message">میتوانید %sرا به عنوان نام کاربری فوری انتخاب کنید. دقت داشته باشید که پس از ثبت نام کاربری، دیگر امکان تغییر آن وجود ندارد. </string> + <string name="new_account_secondary_confirm_message">میتوانید %s را به عنوان نام کاربری فوری انتخاب کنید. دقت داشته باشید که پس از ثبت نام کاربری، دیگر امکان تغییر آن وجود ندارد. </string>wallet/res/values-de/strings-dashpay.xml-450-450 (1)
450-450:⚠️ Potential issue | 🟡 MinorGerman grammar: "eine" should be "einem" (dative with "entsprechen").
The verb "entsprechen" requires the dative case: "einem dieser Kriterien" instead of "eine dieser Kriterien".
Proposed fix
- <string name="request_username_requirements_message_invite_noncontested">Der Nutzername muss eine dieser Kriterien entsprechen</string> + <string name="request_username_requirements_message_invite_noncontested">Der Nutzername muss einem dieser Kriterien entsprechen</string>wallet/res/values-fr/strings-dashpay.xml-533-533 (1)
533-533:⚠️ Potential issue | 🟡 MinorFrench grammatical error: "créé" should be "créée" (feminine past participle).
"L'invitation" is feminine in French, so the past participle should agree: "créée" instead of "créé".
Proposed fix
- <string name="request_username_invitation_not_mixed_message">L\'invitation a été créé avec des fonds non mélangés</string> + <string name="request_username_invitation_not_mixed_message">L\'invitation a été créée avec des fonds non mélangés</string>wallet/res/values-zh/strings-dashpay.xml-529-529 (1)
529-529:⚠️ Potential issue | 🟡 MinorMistranslation: "且" (AND) should be "或" (OR) — user must meet either condition, not both.
The noncontested username requirement is that the name contains a digit 2–9 or is 20+ characters long. This string uses "且" (and), which would incorrectly tell users they need both. This is inconsistent with
username_types_messageon Line 537 of this same file, which correctly uses "或" (or).Proposed fix
- <string name="invitation_fee_noncontested_message">收到您发送邀请的人可以申请包含数字2-9且长度超过20个字符的用户名.</string> + <string name="invitation_fee_noncontested_message">收到您发送邀请的人可以申请包含数字2-9或长度超过20个字符的用户名.</string>wallet/res/values-de/strings-dashpay.xml-529-529 (1)
529-529:⚠️ Potential issue | 🟡 MinorMistranslation: "und" (AND) should be "oder" (OR) — same issue as the Chinese locale.
The noncontested requirement is that the username contains a digit 2–9 or is 20+ characters. This string uses "und" (and), telling users both conditions must be met. Compare with
username_types_message(Line 537), which correctly describes them as separate, independently sufficient conditions.Proposed fix
- <string name="invitation_fee_noncontested_message">Der Person, die du diese Einladung sendest, kann einen Nutzernamen beanspruchen der die Nummer 2-9 beinhaltet und mehr als 20 Zeichen hat.</string> + <string name="invitation_fee_noncontested_message">Der Person, der du diese Einladung sendest, kann einen Nutzernamen beanspruchen, der die Nummer 2-9 beinhaltet oder mehr als 20 Zeichen hat.</string>
🧹 Nitpick comments (10)
wallet/res/values-nl/strings-extra.xml (1)
93-93: Pre-existing typo: "jpuw" should be "jouw".Not introduced by this PR, but since you're already touching this file: line 93 contains
jpuw portemonneewhich should bejouw portemonnee.Suggested fix
- <string name="forgot_pin_instruction_3">Log in met je nieuwe PIN om toegang te krijgen tot jpuw portemonnee</string> + <string name="forgot_pin_instruction_3">Log in met je nieuwe PIN om toegang te krijgen tot jouw portemonnee</string>wallet/androidTest/de/schildbach/wallet/security/SecurityGuardMultiThreadingTest.kt (1)
910-1043: Prefer@Ignoreover commenting out the entire test.The
@Ignoreannotation is already imported (line 39) and is the JUnit-idiomatic way to disable tests. It keeps the test visible in reports as "skipped" and avoids ~130 lines of commented-out code that won't be caught by refactoring tools or compilation checks.Suggested approach
-// `@Test` -// fun testIvBackupAndRecovery() { -// ... -// } + `@Ignore`("IV backup/recovery test temporarily disabled") + `@Test` + fun testIvBackupAndRecovery() { + // ... (uncomment the body) + }wallet/res/values-tr/strings-dashpay.xml (1)
391-393: Note:invitation_fee_titlehas the same Turkish value asinvitation_confirm_title(line 19) anddashpay_invite_fee(line 85).All three resolve to "Davet Ücreti". If these are intentionally separate keys for different UI contexts, this is fine. Just flagging in case any of them are redundant or should have distinct wording.
wallet/androidTest/de/schildbach/wallet/transactions/TransactionExportTest.kt (1)
71-79:System.currentTimeMillis()fallback could make the test non-deterministic.Line 73: if
tx.updateTimewere evernull, the fallback toSystem.currentTimeMillis()would produce a wall-clock timestamp, causing the assertion on line 92 (expecting"1970-01-01T00:00:00Z") to fail unpredictably. Consider using a fixed fallback (e.g.,0L) to keep the test deterministic.Suggested fix
- tx.updateTime?.time ?: System.currentTimeMillis(), + tx.updateTime?.time ?: 0L,wallet/src/de/schildbach/wallet/ui/main/WalletTransactionsFragment.kt (1)
420-431: Error-message matching via raw strings is fragile but the new logic is correct.The short-circuit evaluation of
||protects against NPE on line 431 (whenerrorMessageis null, line 430 already yieldstrue). The boolean semantics are sound: "certificate expired" errors will now also route to theCreateUsernameActivityreuse flow, which appears intentional.However, all the error-message checks across lines 420–431 rely on substring matching against server-produced text. If the upstream error wording changes even slightly, these conditions silently break. Consider extracting these strings into named constants (or matching on error codes/types if available) to make them easier to maintain and grep for.
wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt (1)
235-235: Redundant secondContext.propagateat Line 251.The context is already propagated at Line 235 at the top of the same
withContext(Dispatchers.IO)block. Between these two calls, there are no suspension points (only regular function calls), so the thread won't change and the context remains propagated. The second call is harmless but unnecessary.Also applies to: 251-252
Gemfile (1)
11-11: Consider pinning thenkfgem version for reproducible builds.The other gems in this file specify version constraints (
~> 2.227.1,~> 0.1.2), butnkfis unpinned. This is likely added to resolve a Ruby 3.1+ stdlib extraction warning, which is fine, but pinning the version would be consistent with the rest of the file.wallet/src/de/schildbach/wallet/service/BlockchainServiceImpl.kt (1)
1937-1942: TOCTOU onpeerGroup!!.lock.isLocked— listeners may be leaked.The lock state can change between the check and the
removeXxxEventListenercalls. If the lock is held at check time, listeners are silently skipped and never removed. Since this is inonDestroyandforceStopfollows shortly after, the practical risk is low, but a leaked listener could fire during shutdown.Consider wrapping in a
tryLockwith a short timeout instead, or documenting thatforceStopwill clean up remaining listeners.wallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.kt (2)
461-467: Redundant sub-condition can be simplified.
creationState != VOTING || (creationState == VOTING && lostVote && showSecondaryUsername)is equivalent tocreationState != VOTING || (lostVote && showSecondaryUsername)since the second branch already impliescreationState == VOTING.
39-39: Remove unused importsAssetLockTransactionandIdentity.Lines 39 and 48 import
AssetLockTransactionandIdentityrespectively, but neither is referenced in the code. The file usesBlockchainIdentityinstead. These unused imports can be removed.
| // check all contests | ||
| if (!foundContestedNameInVotingPeriod) { | ||
|
|
||
| // check if the network has this name in the queue for voting | ||
| val watch2 = Stopwatch.createStarted() | ||
| val contestedNames = platformRepo.platform.names.getAllContestedNames() | ||
| log.info("getAllContestedNames returns {} names and took {}", contestedNames.size, watch2) | ||
|
|
||
| // now much of this can be put in BlockchainIdentity | ||
| contestedNames.forEach { name -> | ||
| if (maybeDualUsernames && instantUsername?.contains(name) == false) { | ||
| // skip this name if it doesn't appear to be the contested name for the found username | ||
| log.info("getVoteContenders: skipping {} because its not related to {}", name, instantUsername) | ||
| return@forEach | ||
| } | ||
| val voteContenders = platformRepo.getVoteContenders(name) | ||
| val winner = voteContenders.winner | ||
| voteContenders.map.forEach { (identifier, documentWithVotes) -> | ||
| if (blockchainIdentity.uniqueIdentifier == identifier) { | ||
| foundContestedNameInVotingPeriod = true | ||
| if (blockchainIdentity.currentUsername != null) { | ||
| blockchainIdentity.secondaryUsername = blockchainIdentity.currentUsername | ||
| } | ||
| blockchainIdentity.currentUsername = name | ||
| blockchainIdentity.primaryUsername = name | ||
| // load the serialized doc to get voting period and status... | ||
| val usernameRequestStatus = if (winner.isEmpty) { | ||
| UsernameRequestStatus.VOTING | ||
| } else { | ||
| val winnerInfo = winner.get().first | ||
| when { | ||
| winnerInfo.isLocked -> UsernameRequestStatus.LOCKED | ||
| winnerInfo.isWinner(blockchainIdentity.uniqueIdentifier) -> UsernameRequestStatus.APPROVED | ||
| else -> UsernameRequestStatus.LOST_VOTE | ||
| } | ||
| } | ||
|
|
||
| blockchainIdentity.usernameStatuses.apply { | ||
| // clear() | ||
| val usernameInfo = UsernameInfo( | ||
| null, | ||
| UsernameStatus.CONFIRMED, | ||
| blockchainIdentity.currentUsername!!, | ||
| usernameRequestStatus, | ||
| 0 | ||
| ) | ||
| put(blockchainIdentity.currentUsername!!, usernameInfo) | ||
| } | ||
| var votingStartedAt = -1L | ||
| var label = name | ||
| if (winner.isEmpty) { | ||
| val contestedDocument = DomainDocument( | ||
| platformRepo.platform.names.deserialize(documentWithVotes.serializedDocument!!) | ||
| ) | ||
| blockchainIdentity.currentUsername = contestedDocument.label | ||
| votingStartedAt = contestedDocument.createdAt!! | ||
| label = contestedDocument.label | ||
| } | ||
| val verifyDocument = IdentityVerify(platformRepo.platform.platform).get( | ||
| blockchainIdentity.uniqueIdentifier, | ||
| name | ||
| ) | ||
|
|
||
| usernameRequestDao.insert( | ||
| UsernameRequest( | ||
| UsernameRequest.getRequestId( | ||
| blockchainIdentity.uniqueIdString, | ||
| blockchainIdentity.currentUsername!! | ||
| ), | ||
| label, | ||
| name, | ||
| votingStartedAt, | ||
| blockchainIdentity.uniqueIdString, | ||
| verifyDocument?.url, // get it from the document | ||
| documentWithVotes.votes, | ||
| voteContenders.lockVoteTally, | ||
| false | ||
| ) | ||
| ) | ||
| // what if usernameInfo would have been null, we should create it. | ||
| var usernameInfo = | ||
| blockchainIdentity.usernameStatuses[blockchainIdentity.currentUsername!!] | ||
| if (usernameInfo == null) { | ||
| usernameInfo = UsernameInfo( | ||
| null, | ||
| UsernameStatus.CONFIRMED, | ||
| blockchainIdentity.currentUsername!!, | ||
| UsernameRequestStatus.VOTING | ||
| ) | ||
| blockchainIdentity.usernameStatuses[blockchainIdentity.currentUsername!!] = | ||
| usernameInfo | ||
| } | ||
|
|
||
| // determine when voting started by finding the minimum timestamp | ||
| val earliestCreatedAt = voteContenders.map.values.minOf { | ||
| val document = documentWithVotes.serializedDocument?.let { | ||
| platformRepo.platform.names.deserialize(it) | ||
| } | ||
| document?.createdAt ?: 0 | ||
| } | ||
|
|
||
| usernameInfo?.votingStartedAt = earliestCreatedAt | ||
| usernameInfo?.requestStatus = usernameRequestStatus | ||
| platformRepo.updateBlockchainIdentityData(blockchainIdentityData, blockchainIdentity) | ||
|
|
||
| // schedule work to check the status after voting has ended | ||
| if (usernameInfo?.votingStartedAt != 0L) { | ||
| GetUsernameVotingResultOperation(walletApplication) | ||
| .create( | ||
| usernameInfo?.username!!, | ||
| blockchainIdentity.uniqueIdentifier.toString(), | ||
| earliestCreatedAt | ||
| ) | ||
| .enqueue() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
~110 lines of duplicated contest-processing logic.
The block at lines 302–410 (getAllContestedNames path) is nearly identical to lines 187–291 (getCurrentlyContestedNames path). This makes both paths drift-prone — the minOf bug already demonstrates this, and the second copy introduces unnecessary null-safe calls (usernameInfo?.) that differ from the first copy.
Consider extracting a shared method, e.g.:
private fun processContestedNames(
names: List<String>,
blockchainIdentity: BlockchainIdentity,
blockchainIdentityData: BlockchainIdentityData,
maybeDualUsernames: Boolean,
instantUsername: String?
): Boolean { /* shared logic */ }Then the two call sites reduce to:
foundContestedNameInVotingPeriod = processContestedNames(currentlyContestedNames, ...)
if (!foundContestedNameInVotingPeriod) {
val contestedNames = platformRepo.platform.names.getAllContestedNames()
foundContestedNameInVotingPeriod = processContestedNames(contestedNames, ...)
}🤖 Prompt for AI Agents
In
`@wallet/src/de/schildbach/wallet/service/platform/work/RestoreIdentityWorker.kt`
around lines 293 - 411, The review points out duplicated contest-processing
logic between the getCurrentlyContestedNames and getAllContestedNames paths;
extract the shared logic into a private method (suggested name
processContestedNames) that accepts parameters (names: List<String>,
blockchainIdentity: BlockchainIdentity, blockchainIdentityData:
BlockchainIdentityData, maybeDualUsernames: Boolean, instantUsername: String?)
and returns a Boolean indicating foundContestedNameInVotingPeriod; move the loop
body that iterates voteContenders, sets
blockchainIdentity.currentUsername/secondaryUsername/primaryUsername, computes
usernameRequestStatus, inserts UsernameRequest, builds/updates UsernameInfo (fix
the minOf computation to deserialize each contender’s serializedDocument rather
than reusing documentWithVotes), set votingStartedAt/requestStatus, call
platformRepo.updateBlockchainIdentityData(blockchainIdentityData,
blockchainIdentity), and enqueue GetUsernameVotingResultOperation when
appropriate; then replace both original code blocks to call
foundContestedNameInVotingPeriod =
processContestedNames(currentlyContestedNames, ...) and, if false,
foundContestedNameInVotingPeriod = processContestedNames(contestedNames, ...),
removing the duplicated code and the inconsistent null-safe checks.
There was a problem hiding this comment.
we will do this another time
There was a problem hiding this comment.
@HashEngineering, understood! I've noted this for future consideration. Feel free to revisit the refactoring when it fits better into your development schedule.
🧠 Learnings used
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1423
File: wallet/src/de/schildbach/wallet/ui/EditProfileActivity.kt:418-446
Timestamp: 2025-08-25T14:48:39.247Z
Learning: HashEngineering prefers to refactor and reuse topup code for balance validation logic improvements in DashPay activities like EditProfileActivity, rather than implementing individual fixes in the current PR.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: common/src/main/java/org/dash/wallet/common/util/Constants.kt:68-69
Timestamp: 2025-08-25T15:00:20.777Z
Learning: HashEngineering prefers to keep HTTP logging enabled in release mode (using log.info instead of gating with BuildConfig.DEBUG) to debug production errors, even though this may leak URLs in production logs. This is a deliberate trade-off for debugging purposes in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1450
File: docs/security/FALLBACK_TESTING_GUIDE.md:408-428
Timestamp: 2026-01-12T20:56:41.213Z
Learning: HashEngineering prefers to place testing utilities like FallbackTestingUtils in debug-only source sets rather than using ProGuard rules to strip them from release builds. This ensures test code is never compiled into production builds by the build system itself.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: wallet/src/de/schildbach/wallet/ui/more/SettingsScreen.kt:122-151
Timestamp: 2025-09-05T06:47:44.508Z
Learning: HashEngineering prefers to defer UI edge case fixes like the CoinJoin finishing state display issue (showing "Finishing ...%" when mode is OFF but status is FINISHING) to later PRs when the main focus is on redesigning the settings screen.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1422
File: wallet/src/de/schildbach/wallet/data/PaymentIntent.java:476-481
Timestamp: 2025-08-25T15:04:18.461Z
Learning: HashEngineering prefers to keep payeeUsername and payeeUserId in PaymentIntent.toString() logs unredacted for debugging purposes, prioritizing troubleshooting capability over PII protection concerns in the Dash wallet project.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1425
File: common/src/main/java/org/dash/wallet/common/ui/components/DashButton.kt:94-101
Timestamp: 2025-09-05T06:00:25.088Z
Learning: HashEngineering prefers to fix the DashButton alpha dimming issue (using explicit disabled colors instead of overall alpha) in a follow-up PR rather than including it in the current settings screen redesign PR.
Learnt from: HashEngineering
Repo: dashpay/dash-wallet PR: 1417
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/MerchantDao.kt:0-0
Timestamp: 2025-08-08T16:48:49.964Z
Learning: In PR dashpay/dash-wallet#1417, HashEngineering chose to defer adding Room indexes for gift_card_providers (provider, denominationsType, merchantId) to a follow-up PR; do not block the current PR on this optimization. Files: features/exploredash/.../data/explore/MerchantDao.kt and features/exploredash/.../data/dashspend/GiftCardProvider.kt.
1 participant
Issue being fixed or feature implemented
Related PR's and Dependencies
dashpay/dashj#291
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores