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

feat: wire-up Ledger hdwallet #5336

Merged
merged 62 commits into from
Oct 11, 2023
Merged

feat: wire-up Ledger hdwallet #5336

merged 62 commits into from
Oct 11, 2023

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Sep 21, 2023

Description

Working in this base functionality PR:

  • Receive
  • Sends
  • Txs
  • Swaps - with caveats, only same-chain for now until we cache things properly
  • Re-pair device / race 'TransportRaceCondition: An action was already pending on the Ledger device. Please deny or reconnect.' condition
  • Reconnect on app close disconnect i.e DisconnectedDeviceDuringOperation: Failed to execute 'transferOut' on 'USBDevice': The device was disconnected.

To be implemented in follow-ups:

  • Ledger support for Cosmos and THORChain (RUNE)
  • BCH receive address format - fixed in feat: ledger BCH cashaddr format hdwallet#640
  • Accounts caching and/or UX improvements WRT the need to open xyz app first
  • Verification of addresses at key times so users can ensure they're dealing with the right Ledger/account
  • Programatically open the relevant app before call() to specific apps?
  • Cross-chain swaps - can't be implemented until either/all 3 above are implemented

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)

N/A

Risk

Large regression risk in useReceiveAddress / getReceiveAddress being too strongly memoized i.e stale receive address on wallet change/"update" for all wallets. Think things like next receive address being broken for UTXOs, receive addresses when switching wallets or MM account being the one from the previously connected wallet/account, etc.

Testing

  • Ledger pairing works
  • Ledger chains connection works (currently requires to open the matching app for each chain before clicking "Connect" or doing any operation i.e Tx signing or receive address for a given chain
  • Sends work for all chains (assumes the relevant app is currently open before starting the send flow)
    • Make sure to test with Legacy/SegWit/SegWit Native for LTC and BTC
  • Accounts are loaded for all chains and script types i.e all EVM chains, DOGE, BCH, BTC (all 3 script types) and LTC (all 3 script types)
  • Ensure the accounts are matching the ones from native - import the same seed as native into your Ledger for testing

Engineering

  • All Ledger capability is safely flagged off

Operations

  • ☝🏽

Screenshots (if applicable)

image image image
connect.mov
image image image image image image image image

Note the non-standard receive address for BCH here - to be tackled in a follow-up. It does work (tested with self sends as can be seen above), although does shows up with a warning WRT unverfied inputs on the Ledger currently, which may be related to the address format.

image image

Copy link
Contributor Author

gomesalexandre commented Sep 21, 2023

Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

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

conceptually LGTM, pushing this to wood for ops to test before merging (as i don't have a ledger)

@gomesalexandre
Copy link
Contributor Author

Merging as this has ops signoff

@gomesalexandre gomesalexandre enabled auto-merge (squash) October 11, 2023 18:21
@gomesalexandre gomesalexandre merged commit 1093460 into develop Oct 11, 2023
6 checks passed
@gomesalexandre gomesalexandre deleted the feat_ledger_wire_up branch October 11, 2023 18:22
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