fix: enable P/X chain support for Keystone QR code connections#709
fix: enable P/X chain support for Keystone QR code connections#709MoonBoi9001 wants to merge 7 commits intoava-labs:mainfrom
Conversation
The QR code scanner was only extracting the first key, ignoring the Avalanche X/P key (44'/9000'/0') even when present in Keystone 3 Pro QR data. This caused P/X chain operations to fail with "network not supported" errors for users connecting via QR code. Changes: - Extract all keys from QR codes, not just the first one - Parse both EVM (44'/60') and Avalanche (44'/9000') derivation paths - Set QR code as the default connection method (recommended for all devices) - Update UI text to clarify device support and add i18n Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@gergelylovas @meeh0w wonder if you can take a look at this pr? |
|
@MoonBoi9001 I'll review & test next week 🙏 |
|
Amazing! TYVM 🙌 |
meeh0w
left a comment
There was a problem hiding this comment.
I only left two comments so far.
I did not manage to test it since my Keystone 3 Pro is now on a beta firmware which...doesn't work very well when trying to onboard using QR codes :)
I've contacted the Keystone team and will recheck this PR once they provide me with an updated firmware version.
| for (const index of startingIndexes) { | ||
| const avmKey = await getAddressPublicKeyFromXPub( | ||
| extendedPublicKeyHex, | ||
| index, | ||
| ); | ||
| keys.push({ | ||
| index, | ||
| vm: 'AVM', | ||
| key: buildAddressPublicKey(avmKey, index, 'AVM'), | ||
| }); | ||
| } |
There was a problem hiding this comment.
In case of EVM this would be correct - you'd be creating multiple accounts from a single XPUB (m/44'/60'/0').
In case of X/P chains, though, we want to import minNumberOfKeys extended public keys (one for each account). What you're doing here is creating multiple (minNumberOfKeys) addresses for a single X/P account (i.e. extended public key (`m/44'/9000'/0')).
In Core, the account model for X/P chains looks as follows:
- Account 1: gets the extended public key for
m/44'/9000'/0'and from that we derive a single receive address to display in the UI (m/44'/9000'/0'/0/0). - Account 2: gets the extended public key for
m/44'/9000'/1'and from that we derive a single receive address to display in the UI (m/44'/9000'/1'/0/0). - etc..
However, if the user has funds on other addresses for a given account (e.g. has funds spread across m/44'/9000'/0'/0/0, m/44'/9000'/0'/0/1, ``m/44'/9000'/0'/0/2`) - those funds would still be usable (given an XPUB for the account, we try to discover all addresses with activity/funds on them and make them spendable).
So given an account N, you want to import the following to make X/P chains work:
m/44'/9000'/N'(to be able to find all funds under this xpub)m/44'/9000'/N'/0/0(to be able to receive funds)
| buildExtendedPublicKey(xpub, EVM_BASE_DERIVATION_PATH), | ||
| ); | ||
| evmAddressPublicKeys = await getAddressPublicKeys(xpub); | ||
| } else if (path?.includes("44'/9000'")) { |
There was a problem hiding this comment.
Please:
- use constants for the derivation path prefixes
- create a util function to recognize keys based on their derivation paths
such that when I read the if statement, I immediately know which key I'm processing without having to memorize the derivation paths for given chains. Example:
if (isEvmXpub(key)) {
// ...
} else if (isAvalancheXpub(key)) {
// ...
}
// etc.Each Avalanche X/P xpub represents a distinct account, so derive one address (index 0) per xpub instead of multiple addresses from a single xpub. Use existing @core/common utilities for path parsing instead of hardcoded strings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Improves readability by using isEvmXpub() and isAvalancheXpub() helper functions instead of inline startsWith checks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Moves isEvmDerivationPath and isAvalancheDerivationPath to shared utilities to eliminate duplication across Keystone handlers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changes isAvalancheXpub to getAvalanchePath, returning the path string instead of boolean. This eliminates the need for non-null assertions when extracting the account index from the path. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tests isEvmDerivationPath and isAvalancheDerivationPath functions following AAA pattern and FIRST principles. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Thanks for the review @meeh0w! The X/P derivation fix makes much more sense now. Also took the opportunity to clean up the code a bit. |
Keystone QR codes return paths without the 'm/' prefix (e.g., "44'/60'/0'" instead of "m/44'/60'/0'"). Updated predicates to accept both formats. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Onboarding looks good, but it still doesn't allow for sending these funds using QR codes only.
So far we relied on the presence of Avalanche public keys to determine what kind of device was onboarded:
Later on we relied on this info (SecretType) to determine if the user should use USB or QR codes to sign transactions.
With your changes we can no longer do that, since QR code will still extract the Avalanche keys, and so the UI will still prompt you to connect the device.
This is a good start (huge thank you! ❤️), but requires more changes.
Personally I would pause for a little bit, since Keystone team is about to release a new version of their firmware as they updated @keystonehq/hw-app-avalanche. With their changes, users will be able to Account 2 (and higher), as those were not usable for now (see #739); and also sign transactions involving multiple addresses from single account.
We also need to rethink how to determine which connection type to use -- should we ask the user each time when they sign a transaction? Or maybe add a setting somewhere in the UI?
Summary
Closes #708
The QR code scanner was only extracting the first key from Keystone devices, ignoring the Avalanche X/P key (
44'/9000'/0') even when present in Keystone 3 Pro QR data. This caused P/X chain operations to fail with "network not supported" errors.44'/60') and Avalanche (44'/9000') derivation pathsGracefully falls back to EVM-only support if the QR code doesn't include an Avalanche key.
🤖 Generated with Claude Code