feat(backup): backup models and local stores#745
Conversation
| Algo25: 'Algo25', | ||
| HdSeed: 'HdSeed', | ||
| HdKey: 'HdKey', | ||
| LedgerBle: 'LedgerBle', | ||
| NoAuth: 'NoAuth', | ||
| Joint: 'Joint', |
There was a problem hiding this comment.
Should we match these up to the RN versions of these so we don't have to do weird mappings in future? I think we already have a LogicalAccountType or something like that.
There was a problem hiding this comment.
You are right about doing weird mappings, but I prefer paying this price to keep the backup models separate from our internal models. Since backup payload has a potential to be shared between platforms, decoupling the contract models from internal models protects us from making mistakes. If we decide to change something in our internal models, we would have to be careful about backward compatibility of backup feature. We also don't have 1:1 match for hd wallets, since we decided to keep seed separate from keys and not to include it in the hd key items.
There was a problem hiding this comment.
But shouldn't we at least start with the same even if we maintain a separate enum? I just think 'LedgerBLE' is such an odd value, especially since we don't have 'LedgerUSB'. Same with Joint (that's not a term we usse anywhere in the code), etc.
It seems like we should match up the account types to what we have in our db, even if we do maintain a separate list for backup purposes.
|
|
||
| type WithName = { customName?: string | null } | ||
|
|
||
| export type Algo25AddressPayload = WithName & { |
There was a problem hiding this comment.
Should these not just match the WalletAccount types we already have?
| /** 25-word BIP39 mnemonic. */ | ||
| mnemonic: string | ||
| } | ||
| export type HdSeedSecretsPayload = { |
There was a problem hiding this comment.
The Keystore also has a data structure for keys we can probably just reuse? Whatever export returns from the keystore
There was a problem hiding this comment.
Seems like KeyData is what we get from keystore. Using KeyData for backup payload couples the library and backup format, and it breaks the portability of it. Also, it has internal info like parentKeyId or id which backup does not use.
There was a problem hiding this comment.
If the backup doesn't use parentKeyId then how is it going to restore that relationship (which is important)?
| export type BackupGlobalHash = string | ||
|
|
||
| export const BackupItemType = { | ||
| ACCOUNT: 'ACCOUNT', |
There was a problem hiding this comment.
Do we not need KEYS (or SECRETS) separately? Also what about SETTINGS?
There was a problem hiding this comment.
SECRETS live under ACCOUNT item type, we separate them with the key of the payload secrets/ABC... or accounts/ABC.... About SETTINGS, you are right, it will be added here when we add support for settings.
There was a problem hiding this comment.
But an account can have multiple keys, or rather keys can have relationships to each other, so it's not a 1:1 between account and keys? Wouldn't it be cleaner to separate them into two separate payloads?
Also what if we add keys that aren't related to accounts in future? We currently do add secrets that don't relate to accounts (e.g. pin code and biometric pin, duress pin, etc)?
2 participants
Description
This is the parent branch of backup feature. Contains models, local stores, and ui primitives
Related Issues