[Refactor] Xpub export: Replace the "coordinator" setting with "xpub_qr_type" by kdmukai · Pull Request #844 · SeedSigner/seedsigner

kdmukai · 2025-12-14T12:19:16Z

Description

The current xpub export flow prompts the user for which software coordinator they're using. But most wallets that support xpub import have now coalesced around the BCUR crypto-address animated QR format.

So we can change the selection from a specific wallet-based prompt to focus instead on the format, with a strongly opinionated default that suits most cases.

Before:

After:

Design Consideration

I intentionally opted to use the more user-friendly term "animated" along with the "(default)" hint to minimize user confusion. No one is familiar at all with the phrase "BCUR crypto-account".

Unfortunately, in the future we will probably end up with new animated xpub QR formats which may eventually force us to be a bit more nerdy to differentiate them (e.g. "Animated (BCUR)" + "Animated (foo)"). But we'll deal with that when we have to.

And I kept the "Specter legacy" format but left it disabled by default. I doubt it's even used by any actual users anymore. In another year or so we can probably remove that format altogether.

How the formats map

format	wallets
BCUR crypto-account	Sparrow Bull Bitcoin Nunchuk Specter Desktop (current)
Static	BlueWallet Keeper
Specter legacy	Specter Desktop (old)

Other Notes

The new xpub_qr_format SettingsEntry was moved into the Advanced section of Settings. In its new form there is much less need for it to be a user-facing configuration (whereas before it would be expected that users would enable/disable which software coordinators they use).
test_missing_settings_get_defaults in test_controller.py was removed as it makes more sense as a Settings-specific test. But it was too fragile with too much hard-coding of expected defaults. It was refactored into the new test_settings_defaults in test_settings.py.
Multiselect options are vulnerable to the bug identified below. PR [Bugfix] Multiselect settings must have a selection #850 was filed separately to resolve this.

This pull request is categorized as a:

Code refactor

Checklist

I’ve run pytest and made sure all unit tests pass before submitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

N/A

I have tested this PR on the following platforms/os:

Raspberry Pi OS Manual Build
SeedSigner OS on a Pi0/Pi0W board

Chaitanya-Keyal · 2025-12-14T17:45:36Z

Nit: Comment here should be updated, replacing "coordinator" with "qr_format" or "xpub_qr_format"

Chaitanya-Keyal · 2025-12-14T17:54:10Z

Nit: coord_tuple should probably be replaced with something like qr_format_tuple across all occurrences in this test.

Chaitanya-Keyal · 2025-12-14T18:39:27Z

I have a general suggestion here:

With the replacement of the coordinator-based setting by the new xpub QR format setting in this PR, it might make sense to merge it with the existing xpub export enable/disable setting. Having two separate settings entries for what is effectively a single feature could be confusing or misleading for users.

This is especially true if a user has xpub export disabled but still sees a settings entry prompting them to choose between multiple Xpub QR formats. One possible simplification would be to drop the separate xpub export toggle entirely. If all options in the QR format setting are unchecked, that could implicitly indicate that xpub export is disabled.

Chaitanya-Keyal · 2025-12-14T19:22:38Z

+        for display_name, setting_option in zip(self.settings.get_multiselect_value_display_names(SettingsConstants.SETTING__XPUB_QR_FORMAT), self.settings.get_value(SettingsConstants.SETTING__XPUB_QR_FORMAT)):
            button_data.append(ButtonOption(display_name, return_data=setting_option))



While writing my previous comment, I started wondering what currently happens if all options are unchecked. While testing this, I ran into an IndexError because the loop here doesn’t append anything to the button_data list.

Of course, this is largely user-induced and users wouldn’t deliberately unselect everything. However, this could become a realistic scenario if the mental model I mentioned earlier (unchecked implies disabled) is something other users naturally adopt as well.

This same index error also surfaces in the script type selection view, where users can unselect all available options. It does not occur in the sig type selection view, since that case is constrained to either one option selected or both available, and button_data there is predefined rather than derived from settings.

I think we should ensure that button_data has at least two options before passing it to the screen. If there is only one option, it can be selected by default as it is today. This logic could potentially live in the base ButtonListScreen component, which would simplify several call sites. An empty list could explicitly error, and a single-option list could immediately return that value.

If the list is empty, we have a few options:

Prompt the user to first select at least one option in settings.

List all available options, effectively treating "none selected" as "all selected" in settings.

Treat the "none selected" state (for script type or xpub format) as implicitly disabling xpub export, potentially also updating the setting automatically, although this might get confusing.

All of this likely belongs in a separate PR, but for now I think it would be good to at least ensure that button_data is never empty and that this potential IndexError does not surface through this view.

Apologies for the long comment!

a single-option list could immediately return that value.

This won't work as we use (maybe mis-use) ButtonListScreen for single "Done" / "I understand" / etc acknowledgment screens.

All of this likely belongs in a separate PR, but for now I think it would be good to at least ensure that button_data is never empty and that this potential IndexError does not surface through this view.

Yeah, I think multiselect SettingsEntry types can just verify that at least one option is selected when exiting their Settings screen. The three settings related to xpub export are the only ones that are multiselect so far. And they should all require at least one value selected.

But rather than add error-handling code here that will then become unnecessary once the Settings restrictions are in place, I'd prefer to make this PR depend upon the upcoming multiselect PR.

kdmukai · 2025-12-16T22:45:06Z

it might make sense to merge it with the existing xpub export enable/disable setting.

I had forgotten that we had that option until seeing it recently while working on this PR. I'm not sure there's a use case for deactivating such a core feature and I can't recall the original rationale for adding the option.

newtonick · 2025-12-19T20:02:52Z

Did some testing on this PR and this is what I found so far:

Blockstream BTC Wallet (Formally Green Wallet)
- Animated and Static work for Single Sig Segwit (Setup as Jade Watch Only)
- Could not find a way to do Multisig or P2TR
Blue Wallet
- Animated and Static work
  - Single Sig Segwit
  - Single Sig Nested
  - Multi Sig Segwit
- Multi Sig Nested did not appear to be supported
- P2TR did not appear to be supported
Nunchuk:
- Animated works for:
  - Single Sig Segwit
  - Single Sig Nested
  - Multi Sig Segwit
  - Multi Sig Nested
- Not able to created a P2TR wallet
- Not able to get static Xpub QR to work
Bull Wallet
- Animated works for:
  - Single Sig Segwit
  - Nested Sig
- Did not support P2TR
- Did not see a way to do Multisig
Keeper
- Animated did not work
- Static works for:
  - Single Sig Segwit
  - Multi Sig Segwit
  - P2TR
Zeus:
- Animated and Static works for:
  - Single Sig Segwit
  - Nested Sig Segwit
  - P2TR
Cove:
- Anmimated worked for Single Sig Segwit
- Static did not work
- P2TR and Nested Single Sig did not work
Sparrow:
- Animated and Static work for everything

Chaitanya-Keyal · 2025-12-22T20:06:52Z

        """
        settings_name = "Test SettingsQR"
-        settingsqr_data = f"""settings::v1 name={ settings_name.replace(" ", "_") } persistent=D coords=spa,spd denom=thr network=M qr_density=M xpub_export=E sigs=ss,ms scripts=nat,nes,tr xpub_details=E passphrase=E camera=180 compact_seedqr=E bip85=D priv_warn=E dire_warn=E partners=E"""
+        settingsqr_data = f"""settings::v1 name={ settings_name.replace(" ", "_") } persistent=D xpub_qr=urca,sta denom=thr network=M qr_density=M xpub_export=E sigs=ss,ms scripts=nat,nes,tr xpub_qr=urca,sta xpub_details=E passphrase=E camera=180 compact_seedqr=E bip85=D priv_warn=E dire_warn=E partners=E"""


Looks like xpub_qr=urca,sta got duplicated here by mistake

After screwing this up so many times in this PR, I've written #861 to clean up this area.

But rather than create dependencies, each PR can stand on its own. I also didn't want to slow down acceptance of this "coordinator" Settings change.

Chaitanya-Keyal

Tested ACK.

Verified that all instances of Coordinator have been replaced with Xpub QR Format, along with the associated changes.
Verified that the updated tests are valid and all tests pass.
Also tested on my raspi and confirmed that the output formats work as expected (Although, this is trivial since there are no internal logic changes).

newtonick · 2026-01-20T02:32:10Z

tACK and reviewed. This PR is actually less code that I thought it would be to make this change. Most of the code changes were actually in the test suite.

Chaitanya-Keyal suggested changes Dec 14, 2025

View reviewed changes

Chaitanya-Keyal reviewed Dec 14, 2025

View reviewed changes

Replace the "coordinator" setting with "xpub_qr_type"

b90e3d9

kdmukai force-pushed the xpub_export_replace_coordinators branch from b8634d1 to b90e3d9 Compare December 16, 2025 22:32

Remove last "coord" Settings references

c49f644

kdmukai mentioned this pull request Dec 17, 2025

[Bugfix] Multiselect settings must have a selection #850

Merged

5 tasks

newtonick added this to the 0.8.7 milestone Dec 19, 2025

newtonick added this to @SeedSigner Development Board Dec 19, 2025

newtonick moved this to 0.8.7 Needs Code Review in @SeedSigner Development Board Dec 19, 2025

Chaitanya-Keyal reviewed Dec 22, 2025

View reviewed changes

kdmukai mentioned this pull request Dec 28, 2025

[Cleanup] SettingsDefinition minor improvements, better test maintainability #861

Merged

5 tasks

Remove duplicated attr entry in test

3f9ec5c

Chaitanya-Keyal approved these changes Dec 28, 2025

View reviewed changes

newtonick merged commit cf3f941 into SeedSigner:dev Jan 20, 2026
2 checks passed

newtonick moved this from 0.8.7 Needs Code Review to 0.8.7 Merged in @SeedSigner Development Board Jan 20, 2026

newtonick mentioned this pull request Jan 20, 2026

Export xpub eliminate #851

Merged

12 tasks

newtonick mentioned this pull request Feb 19, 2026

Add Keeper to default coordinators list #680

Closed

12 tasks

		for display_name, setting_option in zip(self.settings.get_multiselect_value_display_names(SettingsConstants.SETTING__XPUB_QR_FORMAT), self.settings.get_value(SettingsConstants.SETTING__XPUB_QR_FORMAT)):
		button_data.append(ButtonOption(display_name, return_data=setting_option))

Conversation

kdmukai commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Design Consideration

How the formats map

Other Notes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Chaitanya-Keyal commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmukai commented Dec 16, 2025

Uh oh!

newtonick commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chaitanya-Keyal left a comment

Choose a reason for hiding this comment

Uh oh!

newtonick commented Jan 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kdmukai commented Dec 14, 2025 •

edited

Loading

Chaitanya-Keyal commented Dec 14, 2025 •

edited

Loading

newtonick commented Dec 19, 2025 •

edited

Loading