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

WIP - feat: Update Keychain for new modified-zip32 #1624

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

Conversation

jurevans
Copy link
Collaborator

@jurevans jurevans commented Feb 11, 2025

Resolves #1611

  • Modified zip32 is handled with a constant BIP44 path of m/44'/877'/0'/0'/2147483647'
  • Update SDK tests for shielded accounts
  • Add field to storage (shielded accounts only) indicating modified zip32 path
    • This should be modifiedZip32Path: "m/44'/877'/0'/0'/2147483647'"
    • This should only apply to shielded accounts derived from a mnemonic! Imported private keys (seed for zip32) is already derived, therefore, we can't make any assumptions as to how it was derived, so leave this field out!)
  • Accounts (mnemonic-based, shielded-only) without this field should display a warning to re-import
  • Add Zip32 form in Advanced (will attach designs soon)
  • Handle "Account Already Imported" better
    • Say you specify custom paths (BIP44 and ZIP32), if the BIP44 account hasn't already been used, but the ZIP32 has, you get an error on import
    • When this happens, the Transparent account exists and is set as the active account, but, you can't see it in the overview :( This needs to be fixed!

Testing

New functionality:

  • Set some accounts up in an extension based on main (running extension locally so its store is preserved when switching branches)
  • Switch to this branch, reload extension in Manage Extensions - When launching the account overview, you should see a warning to update your accounts
  • Re-import one of the accounts, and that account should no longer have the warning!
  • Also, can test custom derivation paths
    • You must specify a custom path for Zip32 if you have already imported the default path (should see an error that the account already exists)

The most critical thing to confirm is:

  • The same mnemonic & path should produce the same keys in:
    • Keychain extension (this branch)
    • CLI wallet (version 1.1.1+)
      • Check spending and viewing keys, as CLI payment addresses are diversified - Keychain currently uses default_address off of the viewing key (will introduce additional payment addresses after this release!)
    • Ledger HW Wallet (version 3.0.0+)
      • Verify viewing key

Notes

New shielded accounts generated from a mnemonic (not private key import) now look like this in storage:

{
    "id": "0459542.....",
    "address": "znam1...",
    "alias": "Tester",
    "parentId": "485057eb-...",
    "path": {
        "account": 0
    },
    "type": "shielded-keys",
    "modifiedZip32Path": "m/44'/877'/0'/0'/2147483647'",
    "owner": "zvknam1....",
    "pseudoExtendedKey": "039cd768...",
    "source": "imported",
    "timestamp": 0
}

This indicates:

  1. That is was derived using modified-zip32
  2. How exactly the seed for the zip32 derivation was derived

So, with this we can check whether or not the user's shielded keys are supported.

@jurevans jurevans added this to the Phase 3 milestone Feb 11, 2025
@jurevans jurevans self-assigned this Feb 11, 2025
@jurevans jurevans force-pushed the feat/update-zipe32-derivation branch 3 times, most recently from 8750cfe to f1b80a4 Compare February 12, 2025 12:53
@jurevans jurevans force-pushed the feat/update-zipe32-derivation branch from f1b80a4 to 1ff8780 Compare February 12, 2025 16:58
parentId,
type,
alias,
info,
source,
timestamp
);
return derivedAccount;
return derivedShieldedAccount;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just a note about this change: So we had a deriveAccount method, which would alternatively call deriveTransparentAccount or deriveShieldedAccount, but it was never used for transparent, so I consolidated it into one function, deriveShieldedAccount

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.

Extension: Update modified Zip32 derivation path to match CLI
1 participant