Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] Xpub export can select the wrong coordinator #490

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Sep 15, 2023

Description

How to reproduce the bug:

  • Go to Settings -> Coordinators and enable a few coordinators (end up with at least one enabled) but exclude BlueWallet.
  • Do a final update and enable BlueWallet.
  • Load a seed and begin the xpub export flow.
  • At the coordinator selection screen, choose BlueWallet.
  • Note your debugging output on the next screen; "coordinator" will NOT be "bw".
  • Alternatively, if the resulting xpub QR is animated, it's obviously targeting a different coordinator (BlueWallet xpub QRs should always be static)

What is happening?

Multiselect options just pop or append to a list of available options. In this flow, BlueWallet is now at the end of the list.

But the coordinator selection View is assuming that the options are sorted the way they're defined in the SettingsEntry for that attribute (see: SettingsConstants.ALL_COORDINATORS).

The fix

Use the SettingsEntry to retrieve the selected coordinator via get_selection_option_value_by_display_name which is completely independent of any ordering.

This pull request is categorized as a:

  • Bug fix

Checklist

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

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

  • N/A (??)

Though this bug is sequence-dependent, I didn't think it merited a flow-based test since the ultimate problem was just that the View wasn't using the best technique to retrieve its selected value.

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

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

@newtonick
Copy link
Collaborator

Tested and LGTM

@newtonick newtonick merged commit a446b67 into SeedSigner:dev Apr 12, 2024
1 check passed
@kdmukai kdmukai deleted the bugfix_xpub_coordinator branch September 2, 2024 14:00
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.

2 participants