Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 72560bfaa. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Traced reduceMnemonicSelection and isMnemonicSelectionComplete at the PR head (ConfirmMnemonicScreen.kt:89-121) against the only consumer of the result: Continue is enabled solely by isComplete (line 239), which requires stack.size == original.size and every position's chip word to equal original[position], so the backupVerified write in BackupNavSheetViewModel.onSuccessContinue (line 141) is unreachable from this screen unless all words are in the right slot. A wrong word can only ever be the last entry, because the reducer refuses to append while the last position is incorrect, and only that entry can be dropped (line 96-97), so correct words are never removed by the chip or by the red-word row; onSelectedWordPress feeds the last stack index back through the same reducer, and SelectedWordItem enables the click only when the row shows a wrong word. Duplicate BIP39 words are handled per chip index, not per string, and a second chip with the same word is still selectable since the duplicate check is on indices (line 99). The wrong-last-word case is fixed: the old firstNullIndex == -1 early return is gone and a full stack whose last entry is wrong still allows dropLast. Back (popBackStack), sheet dismiss (resetState + NavHost disposal) and activity recreation all yield a fresh shuffle with an empty stack because both rememberSaveable calls are keyed on originalSeed and the view model resets to the 24-word placeholder on dispose; the placeholder-keyed restore of a 12-index order cannot index out of range because the placeholder has 24 words. Saved state now holds only Int lists (ArrayList/EmptyList/SingletonList are all Serializable), no mnemonic words. Behaviour matches the issue #1275 row for #634 (tap red word or its chip clears; no overwrite). No iOS twin PR found. Pre-existing and untouched by this PR: if keychain.loadString(BIP39_MNEMONIC) returns null, the placeholder 'secret' x24 mnemonic flows into ShowMnemonic and ConfirmMnemonic and could be 'confirmed'; that is a broken-wallet state and not a regression here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-backup-word # Conflicts: # journeys/README.md
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 50f871aba. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up on 6095019 and 55ada41 (new journey): Reviewed the two journey commits (6095019, 55ada41) against ConfirmMnemonicScreen.kt at head 50f871a. Every tag the journey names exists at head: HeaderMenu, DrawerSettings, Tab-security (CustomTabRowWithSpacing), BackupWallet, TapToReveal, ContinueShowMnemonic, backup_confirm_mnemonic_screen, Word-, SelectedWord-, ContinueConfirmMnemonic and HomeScrollView; Settings > Security > Back up wallet opens Sheet.Backup() at ShowMnemonic, so the screen order matches. Each step follows reduceMnemonicSelection (lines 89-102): chip for word 2 is ignored while slot 2 is red, the red row and the re-tapped chip both drop the last index, the green row is not clickable (clickableAlpha adds no modifier when disabled) and the chip for word 1 is ignored because its index is already in the stack; Continue is enabled only by isComplete. With the fix reverted the red-word tap has no click handler, so the journey fails at the 'slot 2 is empty' check. Seed handling is safe: the precondition is a throwaway dev (regtest) wallet, words are referred to by position and must not be copied, BlockScreenshots is a no-op on debug so colour checks can use a screenshot, and the runner stops before Continue so backupVerified is untouched. The identifier row for SelectedWord- and the not-ported cross-platform row answer the earlier ovi-reviewer thread.
|
@jvsena42 e2e ci red, should sync with master to retrigger |
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Audited - no findings.
QA: emu-1 redroid emulator on Android 15 (API 35), dev regtest debug build
-
✅ passed: Confirm Recovery Phrase opened with 12 unselected chips, 12 empty slots, and Continue disabled.
-
✅ passed: All 2a–2d wrong-word selection, blocking, red-word clearing, and chip re-tap behaviors passed.
-
✅ passed: The correct first word stayed locked when tapping either its selected row or chip.
-
✅ passed: A wrong seventh word appeared in the right column and cleared when its row was tapped.
-
✅ passed: All 12 correct words enabled Continue and advanced to the recovery-phrase warning screen.
-
✅ passed: Font-scale recreation produced a fresh empty shuffle, then the completed flow reached the warning screen.
Ticked Manual Tests 1–6, including 2a–2d.
Tip
Test 1 worth a journey:
- Open the menu and tap Settings
- Open Security and tap Back up your wallet
- Tap Tap To Reveal and note the 12 words by position
- Tap Continue
- Verify 12 chips are unselected, all slots are empty, and Continue is disabled
Test 2 worth a journey:
- Tap the chip for word 1, then the chip for word 3
- Verify slot 1 is correct and slot 2 is wrong
- Tap the chip for word 2 and verify slot 2 does not change
- Tap the wrong word in slot 2 and verify it clears
- Tap the chip for word 3 twice and verify the wrong word appears, then clears
Test 3 worth a journey:
- Start with word 1 correct in slot 1
- Tap the correct word in slot 1 and verify it stays
- Tap the chip for word 1 and verify slot 1 stays correct and slot 2 stays empty
Test 4 worth a journey:
- Fill words 1 through 6 in order
- Tap the chip for word 8
- Verify slot 7 in the right column is wrong
- Tap the wrong word in slot 7 and verify it clears
Test 5 worth a journey:
- Open Confirm Recovery Phrase and select all 12 words in order
- Verify every slot is correct and Continue is enabled
- Tap Continue
- Verify the recovery-phrase warning screen appears
Test 6 worth a journey:
- Select the first three recovery words
- Change the device font scale
- Verify a fresh shuffle, 12 empty slots, and disabled Continue
- Select all 12 words in the new order
- Tap Continue and verify the warning screen
- Restore the original device font scale and verify the app remains open
Coverage:
QA: 6 of 6 manual tests passed
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Awaits QA.
Coverage:
QA: journeys and manual tests queued
Reviewed by claude-opus-5-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)







Fixes #634
Related to #1275
This PR lets the user clear a wrong word on the Confirm Recovery Phrase screen, including a wrong last word.
Description
Out of Scope
ConfirmMnemonicScreenTest.ktchipOrderAndSelection_surviveRecreationhosts the screen outside the sheet with a fixed phrase, so it covers restoration only when the phrase is unchanged, not the in-sheet path above.BackupConfirmMnemonic.swiftremoves the last word by tapping its chip but has no red-word tap. Needs a follow-up in bitkit-ios, together with the journey below.Design
Preview
Captured on the dev emulator with the public BIP39 test phrase in a temporary local build (not committed). No before screenshots: the master build was not installed on the shared emulator during this run.
Recording of selecting a wrong word, clearing it by tapping the red word and its chip, and filling to 11 words:
634-clear-wrong-word.mp4
QA Notes
Manual Tests
Use a throwaway wallet; the route reveals the recovery phrase.
Steps 1-6 were run on the dev emulator. Eleven correct words plus a wrong 12th cannot be reached on device, since the only chip left is the correct one; that path is covered by a unit test.
Journey covering the above, committed at
journeys/backup/confirm-mnemonic-clear-wrong-word.xmlwith its row in
journeys/README.md(suites and cross-platform). Applied tomasterit fails atthe red-word tap, since
SelectedWord-Nis not clickable there.journeys/backup/confirm-mnemonic-clear-wrong-word.xml
Automated Checks
ConfirmMnemonicSelectionTest.ktcovers the selection logic: words added in order and completion, clearing a wrong word by its chip or by the red word, other chips ignored after a wrong word, a wrong last word cleared when every slot is filled (regression), correct words locked, a full correct selection ignoring taps, repeated words, and out-of-range taps.ConfirmMnemonicScreenTest.ktcovers red-word clearing, locked correct words, ignored and re-tapped chips, and state restoration with an unchanged phrase. Compiled locally but not run, because connected tests uninstall the dev app on the shared emulator.just compile,just test,just lint.