-
Notifications
You must be signed in to change notification settings - Fork 0
feat(onboarding): finishing word recovery passphrase screen [PERA-3266] #46
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
Conversation
|
|
||
| expect(mockPush).toHaveBeenCalledWith('ImportInfo') | ||
| expect(mockPush).toHaveBeenCalledWith('ImportInfo', { | ||
| accountType: 'universal', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's maybe stick with hdwallet in the code. I know the user facing description is Universal Wallet, but all the code references xHDWallet and the account type is hdwallet, etc.
| type WalletAccount, | ||
| } from './models' | ||
| import { KeyType } from '@perawallet/wallet-core-kms' | ||
| import * as bip39 from 'bip39' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR maybe but I think the latest version of xHDWallets I imported actually switched to a different bip39 implementation so I guess we should follow suit and use the same one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've double checked that and it seems like it's using the same implementation to generate the PK for creating a new account. What seems to be different is how derivation works, which seems to use BIP32 instead. But we can definitely address that in the next PR, which will address derivations.
packages/accounts/src/utils.ts
Outdated
| import { KeyType } from '@perawallet/wallet-core-kms' | ||
| import * as bip39 from 'bip39' | ||
|
|
||
| export type UniversalKeyPair = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again maybe call it HDWalletKeyPair
packages/accounts/src/utils.ts
Outdated
| } | ||
| } | ||
|
|
||
| export const createUniversalWalletFromMnemonic = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same Universal -> HDWallet rename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I guess the name here should be specific to keys rather than wallets? E.g. createHDWalletKeyDataFromMnemonic or something?
| createdAt: new Date(), | ||
| type: KeyType.Algo25Key, | ||
| } | ||
| await saveKey(rootKeyPair, seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For HDWallet I actually didn't really investigate seed vs entropy. The API just sometimes needed one and sometimes the other so I stored both for HDWallets. Maybe we can get away with just storing Seed OR maybe we should make Algo25 conform to the same structure just so we don't have to have special logic on the consumer side to do constant switching between structures? (somewhere in the signing flow there is code that does JSON.parse(rootKeypair) and then uses the seed and that would now have to only do that for hdwallets and something else for algo25)
Pull Request Template
Description
Related Issues
Checklist
Additional Notes