Skip to content

Label Coldcard MK4 and MK5 support - #2500

Open
BullishNode wants to merge 3 commits into
developfrom
fix/coldcard-mk5-labels
Open

Label Coldcard MK4 and MK5 support#2500
BullishNode wants to merge 3 commits into
developfrom
fix/coldcard-mk5-labels

Conversation

@BullishNode

Copy link
Copy Markdown
Contributor

Summary

  • Label the existing Coldcard MK4 integration as supporting both MK4 and MK5.
  • Update the import-wallet list, Coldcard screens, and localized instruction copy to say Coldcard MK4/MK5.
  • Add a short notice to the MK4/MK5 wallet-import and PSBT-signing instructions:

    NFC works better with Coldcard MK5 and can be unreliable with Coldcard MK4.

Internal enum, route, and database identifiers remain unchanged to avoid an unnecessary persistence migration.

Testing

  • TZ=UTC make checks
    • analyzer
    • import-boundary, formatting, and Dart-fix checks
    • 572 application tests
    • 17 bull_ui tests
    • bull_ui_catalogue test
  • ARB files validated and generated localization output checked.

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown

Claude finished @BullishNode's task in 3m 53s —— View job


Review: Label Coldcard MK4 and MK5 support

  • Gather context (diff vs origin/develop, changed files)
  • Review Dart changes (entity, pages, instructions)
  • Review localization (.arb) changes for consistency/regressions
  • Post review summary

Note: I can't submit formal inline review comments from this workflow, so findings are listed below with file/line references.

Summary

This is a low-risk, well-executed relabeling PR. No security issues and no correctness bugs found. The claim in the description holds: internal identifiers (SignerDeviceEntity.coldcardMk4, routes, DB values) are untouched, and displayName is used exclusively for UI rendering (import_coldcard_page.dart:71, show_psbt_screen.dart:53, wallet_details_screen.dart:107-113, watch_only_details_widget.dart:99), so no persistence migration is needed. The subtitle parameter passed to InstructionsBottomSheet.show already exists on the shared widget (lib/core/widgets/bottom_sheet/instructions_bottom_sheet.dart:24), so both new call sites are safe.

Finding (minor, i18n)

coldcardMk4Mk5NfcReliabilityNotice is only defined in app_en.arb (localization/app_en.arb:14836). The other 27 locale files were not given this key, while every other key in this repo — including the recently added coinsErrorUnexpected — has full parity across all 28 ARB files. Because l10n.yaml configures untranslated-messages-file, gen_l10n falls back to the English template, so this compiles and won't crash — but non-English users will see the NFC reliability notice in English on both the wallet-import and PSBT-signing instruction sheets. If translations are handled by a separate pipeline this may be intentional, but it breaks the existing parity convention. Fix this →

Nitpick (optional)

The @...description metadata entries in app_en.arb still say "Coldcard Mk4" (e.g. lines 9252, 9348, 10863). These are translator-facing only and never shown to users, so updating them is purely cosmetic.

Verified

  • All 27 non-English ARB files received exactly the same 11 value edits; existing translations were preserved with only the device-name substring changed (spot-checked fr, zh; verified counts across all locales).
  • No user-facing string anywhere still contains a bare "Coldcard Mk4" — the only remaining "MK4" outside "MK4/MK5" is intentionally in the new notice text.
  • Generated l10n output (lib/generated/l10n) is not committed, so no stale codegen concern.
  • No tests reference the old display name.
  • The notice was correctly added only to the MK4/MK5 sheets, not the Coldcard Q flows.

CI ("Analyze and Test") was still in progress at review time.

@i5hi
i5hi self-requested a review July 23, 2026 15:42
@i5hi

i5hi commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Fixing the locale files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants