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

fix: bch Ledger address verify #5499

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Oct 19, 2023

Description

Passes the correct scriptType for BitcoinCash in getAddress down the line, instead of having Ledger give us a Legacy address then converting it into a CashAddr. This works in most cases UX-wise, but won't cut it when actually verifying addresses on the device, since Legacy will be used, and users would be confused as to why verification passed despite address being different (or simply refuse continuing since, well those are different).

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #

Risk

None - now in Ledger-land only (bec83f9) since this was causing native regressions

Moderate - specifying a cashaddr scriptType should be a no-op, but receive address for all BCH-supporting wallets should be tested

Testing

  • Verify your BCH address in Ledger

  • Ensure the address you see is the same as the one on the device

  • Ensure BCH addresses as well as address verification is showing no regression in native

Engineering

  • ☝🏽

Operations

  • ☝🏽

Screenshots (if applicable)

  • Native
image
  • Ledger
image image

Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@gomesalexandre
Copy link
Contributor Author

Closing in favor of shapeshift/hdwallet#650

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.

1 participant