airgap-it / airgap-vault

The AirGap Vault is installed on a spare smartphone that has no connection to any network, thus it is air gapped. This app handles the private key.
MIT License
400 stars 111 forks source link

Metamask QR cannot be generated when address is using customized derivation path (My Crypto, MEW) #128

Open sbacelic opened 2 years ago

sbacelic commented 2 years ago

I have one account with custom derivation path using legacy format used by My Crypto/MEW: m/44'/60'/0'/1. When I try to sync the account and choose MetaMask QR I get the message: "Message is not compatible with the selected QR code type".

sbacelic commented 2 years ago

The problem is that address publicKey in not in extended public key format and MetamaskGenerator expects it to be, otherwise it will fail when decoding from base58:

    generateCryptoAccountMessage(data) {
        return __awaiter(this, void 0, void 0, function* () {
            const account = data.payload;
            const extendedPublicKey = bip32.fromBase58(account.publicKey);

Would you be interested in PR that could support different derivation paths (Ledger Live, BIP44, MEW/MyCrypto), similar like Keystone does so migration from legacy Ledger setup could be supported?

AndreasGassmann commented 2 years ago

Is your account with the derivation path m/44'/60'/0'/1 a single account or an HD account?

I don't think it's possible to use this account with MetaMask if it's a single account.

MetMask expects an extended public key, it does not work with a regular public key. It will then derive multiple accounts by using the derivation paths /0/0, /0/1, /0/2, ... So the derivation path you have, m/44'/60'/0'/1, can not be constructed this way.

If your account is an HD account, meaning your first ETH account is m/44'/60'/0'/1/0/0, then you should be able to simply add an ETH HD account in AirGap Vault and set the derivation path to m/44'/60'/0'/1, which will then result in MetaMask displaying the accounts m/44'/60'/0'/1/0/0, m/44'/60'/0'/1/0/1, etc.

sbacelic commented 2 years ago

Is your account with the derivation path m/44'/60'/0'/1 a single account or an HD account? I don't think it's possible to use this account with MetaMask if it's a single account.

No, it is actually a HD account, I use 3 addresses m/44'/60'/0'/0, m/44'/60'/0'/1 and m/44'/60'/0'/2.

MetMask expects an extended public key, it does not work with a regular public key. It will then derive multiple accounts by using the derivation paths /0/0, /0/1, /0/2, ... So the derivation path you have, m/44'/60'/0'/1, can not be constructed this way.

Metamask will ask for HD path, when connecting using ledger:

Screenshot 2022-04-10 at 13 56 15

Based on the selection it will generate ext. pubkey using preselected HD format.

If your account is an HD account, meaning your first ETH account is m/44'/60'/0'/1/0/0, then you should be able to simply add an ETH HD account in AirGap Vault and set the derivation path to m/44'/60'/0'/1, which will then result in MetaMask displaying the accounts m/44'/60'/0'/1/0/0, m/44'/60'/0'/1/0/1, etc.

AirGap Vault assumes that HD path is in BIP44 format, however as my wallet was generated a long time ago my addresses are under an unexpected (legacy/mew) HD path.

I was able to hack support for different HD format in my local dev. env., but I'm not sure if supporting this will be useful for the wide audience. I'm already in the process of migrating to addresses under BIP44 HD path so I won't be needing this in the end.

Maybe its just enough to mention that only BIP44 HD path is supported.

AndreasGassmann commented 2 years ago

Thanks for the information. Just to confirm, when pairing with the Ledger you can select different paths, but with QR based signers you cannot?

If that's the case I'll pass this information along to MetaMask.

sbacelic commented 2 years ago

The actual issue in AirGap Vault, not in MetaMask. MM just reads the QR. It is up to AirGap to derive the right path. Check how Keystone does it.

pvdyck commented 1 year ago

Any solution to this issue ?

anotherBobDot commented 1 year ago

Folks, this is a big issue. There has to be a way to pass an {account_index} variable for HD wallet. For example: https://gist.github.com/miguelmota/034c81886bd23b25c64ec5eda9251641 Notice how Ledger Live accounts are made and Legacy ones. Airgap automatically assumes to append /0/{account_index} at the end. Example: if HD path is: m/44'/60'/0' then the first address is m/44'/60'/0'/0/0 and next one is m/44'/60'/0'/0/1 which is not WHAT is desired. What is needed for ledger legacy support is m/44'/60'/0'/1 = m/44'/60'/0'/{account_index} for ledger live m/44'/60'/1'/0/0 = m/44'/60'/{account_index}'/0/0 Please make it the way as Keystone wallet did in order for people have seamless transition from ledger seed phrases. Thank you.

@AndreasGassmann

anotherBobDot commented 1 year ago

@AndreasGassmann check this line: https://github.com/airgap-it/airgap-vault/blob/master/src/app/pages/address-explorer/address-explorer.page.ts#L96

l1quid8 commented 1 year ago

Can we please get this feature implemented? Requiring users to move assets over to a new address creates too much friction for new users.