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/746 - Updates for Ledger HW (shielded keys) #1471

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jurevans
Copy link
Collaborator

@jurevans jurevans commented Jan 6, 2025

Resolves #746

Also this implements a stepped process, so users can know what's happening on the ledger (now that there's 3 approvals instead of 1):

  1. Transparent address & public key
  2. Zip32 Viewing Key derivation
  3. Zip32 Proof-Gen Key derivation

See #1458 from @mateuszjasiuk - We'll also need these changes.

  • Update to latest @zondax/ledger-namada package (currently at 2.0.0)
  • Remove HID transport (see: https://developers.ledger.com/docs/device-interaction/ledgerjs/integration/web-application/web-hid-usb - we should use either HID or USB, but not include support for both!). WebUSB seems to have the most support (supports older versions than WebHID), so we'll use that.
  • Updates to Ledger class in SDK
    • Methods for getViewingKey and getProofGenerationKey (split up getShieldedKeys so that we may turn these into steps, as they are separate approvals)
    • Update type returned for ViewingKey (now returns xfvk)
    • Before storing xfvk, convert to proper bech32m encoding (zvknam1)
  • Store xfvk, ak and nsk
  • It currently takes about 10 seconds to derive viewing key and proof-gen key (each!): We should notify the user what is happening, as there is not indication on Ledger when it is deriving these keys, they will only see each prompt after waiting about 10 seconds

TESTING

  • Run yarn on this branch to get updated packages
  • Install v2.0.2 of Ledger app (via https://hub.zondax.ch/namada for now) NOTE This will require that you are on the latest firmware, so update if needed!
  • Import Ledger account

DESIGNS

Approval steps
image

SCREENSHOTS

Version detection (NOTE: 2.0.0 is the correct min-version, I just did this as a test since I am already upgraded to the latest Ledger app):
Screen Shot 2025-01-09 at 6 44 34 AM

@jurevans jurevans added this to the Ledger MASP readiness milestone Jan 6, 2025
@jurevans jurevans self-assigned this Jan 6, 2025
@jurevans jurevans force-pushed the feat/746-ledger-shielded-keys branch from 6430110 to 94667e1 Compare January 6, 2025 12:07
@jurevans jurevans changed the title WIP: feat/746 - Updates for Ledger WIP: feat/746 - Updates for Ledger HW (shielded keys) Jan 6, 2025
@jurevans jurevans force-pushed the feat/746-ledger-shielded-keys branch 5 times, most recently from 6f8d9b0 to 05608ba Compare January 8, 2025 12:11
@jurevans jurevans requested a review from mateuszjasiuk January 8, 2025 12:48
@@ -20,7 +20,7 @@ type Props = {
status?: CompletionStatus;
statusInfo: string;
parentAccountStore?: AccountStore;
shieldedAccount?: DerivedAccount;
paymentAddress?: string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was just simplifying here - we do not need to track the whole shielded account, only the payment address to display in confirmation

@jurevans jurevans force-pushed the feat/746-ledger-shielded-keys branch 2 times, most recently from 9f74c59 to 14cd8e6 Compare January 10, 2025 13:28
@jurevans jurevans marked this pull request as ready for review January 10, 2025 13:29
@jurevans jurevans changed the title WIP: feat/746 - Updates for Ledger HW (shielded keys) feat/746 - Updates for Ledger HW (shielded keys) Jan 10, 2025
@jurevans jurevans force-pushed the feat/746-ledger-shielded-keys branch from 14cd8e6 to 3ea76fe Compare January 10, 2025 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ledger: Add shielded address when importing account from Ledger HW Wallet
1 participant