Keystone USB integration #119
Conversation
Signed-off-by: Cypher Pepe <[email protected]>
Co-authored-by: Michał Leszczyk <[email protected]>
| if (!hasAVMPublicKey) { | ||
| const publicKeyAVM = AddressPublicKey.fromExtendedPublicKeys( | ||
| secrets.extendedPublicKeys, | ||
| 'secp256k1', | ||
| derivationPathAVM, | ||
| ).toJSON(); | ||
| newPublicKeys.push(publicKeyAVM); | ||
| } |
There was a problem hiding this comment.
I believe this will break onboarding and adding accounts for previous Keystone iterations, where the extended public key for Avalanche is not provided.
It should be iffed and only run for Keystone3 (iThink™)
| if ( | ||
| secretType === SecretType.Keystone || | ||
| secretType === SecretType.Keystone3Pro | ||
| ) { | ||
| const accountIndexToUse = | ||
| accountIndex === undefined ? secrets.account.index : accountIndex; | ||
|
|
||
| const derivationPathEVM = getAddressDerivationPath( | ||
| accountIndexToUse, | ||
| secrets.derivationPathSpec, | ||
| 'EVM', | ||
| ); | ||
| const derivationPathAVM = getAddressDerivationPath( | ||
| accountIndexToUse, | ||
| secrets.derivationPathSpec, | ||
| 'AVM', | ||
| ); | ||
| const evmExtendedPubKey = getExtendedPublicKeyFor( | ||
| secrets.extendedPublicKeys, | ||
| derivationPathEVM, | ||
| 'secp256k1', | ||
| ); | ||
| const avmExtendedPubKey = getExtendedPublicKeyFor( | ||
| secrets.extendedPublicKeys, | ||
| derivationPathAVM, | ||
| 'secp256k1', | ||
| ); | ||
|
|
||
| assertPresent(evmExtendedPubKey, SecretsError.PublicKeyNotFound); | ||
|
|
||
| return new KeystoneWallet( | ||
| secrets.masterFingerprint, | ||
| accountIndexToUse, | ||
| this.keystoneService, | ||
| network.chainId, | ||
| tabId, | ||
| evmExtendedPubKey.key, | ||
| avmExtendedPubKey ? avmExtendedPubKey.key : undefined, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Since this whole clause will only run for X- and P- chains, I think we should error out for SecretType.Keystone and assert the AVM key being present for SecretType.Keystone3Pro.
| await getAddressFromXpubKey(xpubValue, accountIndex + 1, newAddresses); | ||
| } | ||
| if (accountIndex >= 2) { | ||
| capture('OnboardingKeystoneConnected'); |
There was a problem hiding this comment.
Let's make analytics events specific to Keystone 3, this way we know how many users use each of the device versions.
| capture('OnboardingKeystoneConnected'); | |
| capture('OnboardingKeystone3Connected'); |
Same for other capture(...) calls.
There was a problem hiding this comment.
Copy paste mistake, I guess. This whole file talks about Ledger, not Keystone.
| const app = new KeystoneUSBEthSDK( | ||
| (await createKeystoneTransport()) as any, | ||
| ); |
There was a problem hiding this comment.
My Keystone3 is acting up (security upgrade is required and it doesn't seem to be able to go above the required 40% battery charge...).
With that in mind, I can't really test it at the moment, but I'm struggling to understand how this part of the code is supposed to work. I took a look at the createKeystoneTransport function in your SDKs and I'm pretty sure it simply cannot work in the extension realm. It uses the navigator.usb.requestDevice internally, which is not available in the extension's service worker.
This is exactly why with Ledger, we're implementing our own LedgerTransport which pushes out data into the extension's popup, from which we actually can access the USB transport.
Please take a look at these diagrams:
Have you tested the X- and P-Chain signing on this branch with Keystone3 connected?
Description
Changes
Testing
Screenshots:
Checklist for the author
Tick each of them when done or if not applicable.