-
Notifications
You must be signed in to change notification settings - Fork 41
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
fix keychain deserialize #735
Conversation
24334cd
to
81f2e67
Compare
if (opts.accountsDeleted?.length) | ||
privates.get(this).accountsDeleted = opts.accountsDeleted; |
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.
this is the bug fix, the rest I just replaced the ?.
with a single check on top
Here's the packed extension for this build: |
@@ -140,7 +140,9 @@ export class HdKeychain implements IKeychain { | |||
|
|||
async removeAccount(address: Address): Promise<void> { | |||
const accounts = await this.getAccounts(); | |||
console.log('accounts', accounts); |
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.
👀
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.
+2
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.
😅
Here's the packed extension for this build: |
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.
Good stuff! 💯
@@ -8,17 +9,36 @@ import { KeychainType } from '~/core/types/keychainTypes'; | |||
import { IKeychain, PrivateKey } from '../IKeychain'; | |||
import { autoDiscoverAccounts } from '../utils'; | |||
|
|||
type SupportedHDPath = "m/44'/60'/0'/0"; |
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.
We have this HD path hardcoded in like 3 different files now, we should probably move it to a common place.
(No need to do it right now)
`${privates.get(this).hdPath}/${index}`, | ||
); | ||
const _privates = privates.get(this)!; | ||
if (!_privates.mnemonic) throw new Error('No mnemonic'); |
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.
Would this ever happen?
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.
only If it's called without calling init()
first, I let the types guide me here, if ts said it can I it can, same for the others
pretty sure it should never happen in production, but these throws helped me find a bug I introduced elsewhere faster, so I think it's good for when we are changing code related
const wallet = privates.get(this).getWalletForAddress(address); | ||
const _privates = privates.get(this)!; | ||
const wallet = _privates.getWalletForAddress(address); | ||
if (!wallet?._isSigner) throw new Error('Not a signer'); |
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 here
return wallet; | ||
} | ||
|
||
async serialize(): Promise<SerializedHdKeychain> { | ||
const _privates = privates.get(this)!; | ||
if (!_privates.mnemonic) throw new Error('No mnemonic'); |
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.
and here
Here's the packed extension for this build: |
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.
👍
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.
creating + deleting wallets & viewing secrets seems to work as intended
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Fixes BX-####
Figma link (if any):
What changed (plus any additional context for devs)
Screen recordings / screenshots
What to test
Final checklist
yarn build
).