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] Handle all-caps address QR data; refactor/cleanup; more tests #667

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Jan 20, 2025

Description

fixes #665

  • BUGFIX: parse address QR formats that may be in all caps.
  • Refactor the address QR parsing.
  • Add tests, make existing test more DRY.

This pull request is categorized as a:

  • Bug fix
  • Code refactor

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?

  • Yes

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

# Native Segwit (single sig or multisig), mainnet
self.address_type = (SettingsConstants.TAPROOT, SettingsConstants.MAINNET)
elif addr_prefix == "bc1p":
# Native Segwit (single sig or multisig), mainnet
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and following 2 "elif" comments could be Taproot instead of Native Segwit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed those code comments entirely as they don't add any clarifying information.

@jdlcdl
Copy link

jdlcdl commented Jan 21, 2025

As of a90e7a6,

ACK tested

fairly thoroughly with all supported address types.

  • specter, nunchuk, and sparrow are all fairly standard, only the address is encoded in qrcode.
  • bluewallet has had "bitcoin:" prefix as if a url, and recently started expressing address in all caps,
    No problems from above, except for taproot verification which is resolved by [New Feature] Enable basic p2tr address verification #669

@newtonick
Copy link
Collaborator

ACK and tested

@newtonick newtonick merged commit 8679461 into SeedSigner:dev Jan 23, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged Not Yet Released
Development

Successfully merging this pull request may close these issues.

BlueWallet addresses may be bech32 in all uppercase.
3 participants