Conversation
| keyType === 'qr' | ||
| ? this.keyIterator.parsedAccount?.hdPath || | ||
| this.keyIterator.parsedAccount?.accounts?.[0]?.hdPath | ||
| : undefined |
There was a problem hiding this comment.
@superKalo I'm not proficient enough here so tagging Kalo for assistance.
I guess I'm wondering why do we have different hd paths for accounts? And is it safe that we're not using an account specific hdPath for qr accounts but defaulting to this.keyIterator.parsedAccount?.hdPath for each one of them?
There was a problem hiding this comment.
Yeah, good catch.
The hd path is critical to know, since we calculate the next accounts ourselves from the xpub of the first account we receive from the QR code.
So in this case I'd advice in the key iterator - we make sure the hdPath of the first account exists and if not - to fallback with the standard hd path in there. So on this level (in the AccountPicker) - we should always have the hdPath and throw if missing.
| keyType === 'qr' && qrWalletConfig | ||
| ? qrWalletConfig.hdPathTemplate || this.hdPathTemplate | ||
| : this.hdPathTemplate | ||
| ) as HD_PATH_TEMPLATE_TYPE |
There was a problem hiding this comment.
I see we prioritize qrWalletConfig.hdPathTemplate here. What if the user has changed this from the UI?
My questions are:
- Is the user allowed to change his hdPathTemplate from the UI
- If yes, would be reflected in
qrWalletConfig.hdPathTemplate(I don't think so as this happens insetHDPathTemplate)
Imo, it would be better to find a way to use the controller this.hdPathTemplate at all times. Maybe set it initially from the qrWalletConfig and allow the UI to dispatch changes after
There was a problem hiding this comment.
- I think the answer is no - hd path should not be changable in our UI. But we should test with different hd path set on the hardware wallet (e. g. on imToken wallet) or in general - what's the flow other wallets do for different than standard hd path.
…e props are qr-based hw specific
| hdPathTemplate: this.hdPathTemplate as HD_PATH_TEMPLATE_TYPE, | ||
| hdPathTemplate: hdPathTemplate, | ||
| relativePathTemplate: relativePathTemplate, | ||
| originHdPath: originPath, |
There was a problem hiding this comment.
I don't like this part. It has a lot of noise.
For all external signers we have hdPathTemplate, which is for example: m/44'/60'/0'/0/<account>
So why we need extra 2 props relativePathTemplate (example: 0/{index}) and originHdPath (example: m/44'/60'/0')? Can't we just calculate them based on the hdPathTemplate?
Also, why mixing <account> (that we already use for all) and {index}. They mean the same thing.
Also, it's strange that these two values are only set for QR-based wallets and would be undefined for all others. If they are QR-based specifics only, why don't we name them somehow indicating that (same applies for (masterFingerprint, naming doesn't suggest it's QR-based wallets only and for all others - would be undefined)
| hdPathTemplate: keyIterator.walletConfig!.hdPathTemplate, | ||
| pageSize: 5, | ||
| shouldAddNextAccountAutomatically: false, | ||
| shouldSearchForLinkedAccounts: false |
There was a problem hiding this comment.
Please add context why searching for linked accounts is disabled. Otherwise, it will get lost in the Slack discussions. Base on memory, it was decided like so for simplicity sake?
|
|
||
| qrWalletType?: QrWalletType | ||
| qrProtocol?: QrProtocolType | ||
| originHdPath?: string // for accounts imported from QR, to store the original path used on the wallet, as some wallets (like Keystone) don't provide the possibility to retrieve the key by xpub, but only by path, so we need to keep track of it in order to be able to retrieve the key later on |
There was a problem hiding this comment.
Can you give an example how this is different than the combo between the existing hdPathTemplate and the index props?
| qrProtocol?: QrProtocolType | ||
| originHdPath?: string // for accounts imported from QR, to store the original path used on the wallet, as some wallets (like Keystone) don't provide the possibility to retrieve the key by xpub, but only by path, so we need to keep track of it in order to be able to retrieve the key later on | ||
| masterFingerprint?: string // for Ledger, optional for others, as it can be used for additional info but is not required to retrieve the key | ||
| relativePathTemplate?: string // QR related |
There was a problem hiding this comment.
Can you give an example how this is different than the combo between the existing hdPathTemplate and the index props?
3 participants
paired with: https://github.com/AmbireTech/ambire-app/pull/6803
Summary