Feature/nar 1595 vault root key generation derivation #268
Conversation
NAR-1595 Vault Root Key Generation & Derivation
Each Vault can have any number of Mnemonics, Root Keys or Private Keys within it. If a Root Key is generated within the vault, it should contain a backup package. Add Backup Encryption Key to Vault TenantA tenant can (optionally) have a
Key GenerationEndpoint: POST Body params
Response body
Requirements
Future work
Key DerivationEndpoint: POST Body params
Response body
Requirements
Import SeedEndpoint: POST body params
response body
Requirements
We DO return a backup of this seed simply so it follows the same rules & they can recover a full vault from one place. Note: the Vault can have multiple generated OR imported root seeds, as well as imported PKs (non-root). Signing calls will reference individual wallet pks not root keys. In the future, we may extend to allow signing to take root keyId + derivation path so you do not have to derive first. Data Modelskey-value data schemas (update Prisma model Backup {
id String id default(uuid())
clientId String map("client_id")
backupPublicKeyHash String map("backup_public_key_hash") // sha256 of the publicKey that was used to encrypt this backup
keyId String map("key_id") // Root Key Id that is backed up
data String // Encrypted backup data, as a jwe string
createdAt DateTime default(now()) map("created_at")
map("backup")
} |
| } | ||
| if ( | ||
| rsaPublicKeySchema.safeParse(backupPublicKey).success === false && | ||
| rsaPrivateKeySchema.safeParse(backupPublicKey).success === false |
There was a problem hiding this comment.
Do we want to support a Secp256k1 key as a backup key ? This code is because we currently only support RSA encryption in signature lib:
see slack message
There was a problem hiding this comment.
No; we'll add more later I'm sure, but today we can just do RSA.
secp256k1 isn't commonly used for encryption, it's good for signing.
apps/vault/src/vault/vault.module.ts
Outdated
| VaultService, | ||
| WalletRepository, | ||
| PrismaService, |
There was a problem hiding this comment.
I had to do this because I'm using PrismaService directly in BackupRepository to write on the newly created table 'Backup'.
Although, it feels it's not great to do that, because we have a 'PrismaModule' and it seems to be breaking boundaries.
I'm unsure on what should be the correct path here
There was a problem hiding this comment.
TLDR: you're correct to use PrismaService directly in your repositories! Just import PersistenceModule.
Pretty sure you can/should actually add PersistenceModule into the imports section above. That's the only change.
You're supposed to use PrismaService directly within your repository; the purpose of the PersistenceModule is to encapsulate all the setup & stuff, not to have a single module where we put all our data access logic.
So when you want to use it, you tell this module that it needs to import stuff from the Prisma Module. You don't import the individual services from that module direclty, just the entire thing.
That also has the TestPrismaService set up which helps in testing.
The client.module.ts in Armory might be a good reference -- it's doing all that, writing to individual tables (not the KV store).
There was a problem hiding this comment.
As @mattschoch said, import the persistence module, and it'll make everything marked as "exported" in that module available here.
You don't have to import the exported providers in your module.
| import { ApiPropertyOptional } from '@nestjs/swagger' | ||
| import { IsNumber, IsOptional, IsString } from 'class-validator' | ||
| import { IsBoolean, IsNumber, IsOptional, IsString } from 'class-validator' | ||
|
|
||
| export class CreateClientDto { |
...rc/shared/module/persistence/schema/migrations/20240522131756_add_backup_table/migration.sql
Outdated
Show resolved
Hide resolved
| return path || `m/44'/60'/${accountIndex}'/${changeIndex}/${addressIndex}` | ||
| } | ||
|
|
||
| export const hdKeyToKid = (key: HDKey): string => { |
There was a problem hiding this comment.
To whoever is wondering what HD means: hierarchical deterministic
| import { ApiProperty } from '@nestjs/swagger' | ||
| import { IsOptional, IsString } from 'class-validator' | ||
|
|
||
| export class GenerateKeyDto { |
There was a problem hiding this comment.
See my other comment about using Zod.
We deprecated class-validator and new DTOs must always use a Zod schema.
apps/vault/src/vault/http/rest/dto/generate-key-response-dto.ts
Outdated
Show resolved
Hide resolved
| await this.prismaService.backup.create({ | ||
| data: { | ||
| clientId, | ||
| backupPublicKeyHash, | ||
| keyId, | ||
| data | ||
| } | ||
| }) |
There was a problem hiding this comment.
Leaving apart my other comment about using key-value.
Let's test your design skills: how can you improve this implementation?
| export class MnemonicRepository { | ||
| private KEY_PREFIX = 'mnemonic' | ||
|
|
||
| constructor(private keyValueService: EncryptKeyValueService) {} |
There was a problem hiding this comment.
Now I'm confused, why have you used the kv service here but not for the backup? 🤔
There was a problem hiding this comment.
Good job, especially adding E2E tests. As more you do it, the better you get at it :)
I left a blocker comment about using a specific table instead of the kv. However, after finishing the review and reading another repository using the kv, I'm unsure if that was intentional and why.
Ptroger
force-pushed
the
feature/nar-1595-vault-root-key-generation-derivation
branch
from
200ed29 to
811293e
Compare
| .set('authorization', `GNAP ${accessToken}`) | ||
| .send({ | ||
| keyId, | ||
| derivationPaths: ['next', 'next'] |
There was a problem hiding this comment.
Is that a common pattern or did you come up with? I have to admit it smells.
There was a problem hiding this comment.
What do you find bothering there ?
It wasn't my finding, but part of the requirement. The way I understand this:
One requirement is to be able to derive 'next' wallet.
Another requirement is to be able to derive multiple wallets on the same API call
Consequence is that you need a value that tells vault to use 'next' path for derivation.
There was a problem hiding this comment.
[IMPORTANT]
If the requirement is to derive the next wallet, why can't we just do that without asking the user to pass next? If you need N wallets in a single call, why can't we use a number?
Example, we use derive as the number of wallets to derive as part of the body:
- If
deriveis undefined, generate the next available index - If
deriveis a number bigger than 0, generate N wallets from the last known index - If
deriveis a number smaller or equal than 0, throw an error.
Isn't that what you want?
There was a problem hiding this comment.
Requirement is you can derive 'next' wallet or ${customPath} wallet.
So having just a number won't work.
Say you want to derive wallet at path m/44'/60'/0'/0/2 AND next wallet. You will pass:
['m/44'/60'/0'/0/2', 'next'];
Following you logic of having a number of wallets, what you could have, is {
derive: number
derivationPaths?: string[]
}
But that doesn't removed the need for the array of strings. I think having only the array is more verbose, and you also don't need derive because its === derivationPaths.length value
As a note, if you are wondering what is the purpose of passing custom derivation paths, it could allow things like:
- organize your HD wallet as you please
- retrieve the PK of a wallet where you still have the rootKey and derivation path
There was a problem hiding this comment.
Btw, currently, deriving a custom path will never update the 'nextAddrIndex' value. I'm not sure about that. Reason why I did not implemented it is because I don't think the current solution should last (nextAddrIndex should not be stored but computed.) I also think the interface for the post request should look like this:
paths: {
accountIndex: number
changeIndex: number
addressIndex: number
} | DerivationPath[]
But in order to do this I need to really understand how to leverage querying capabilities with KV storage. Thus my message to you @wcalderipe this morning
There was a problem hiding this comment.
Fair enough. Maybe I don't understand the problem well enough nor our existent needs, and that's why I think it's overcomplicated.
I still think asking the user to input next is weird when one just wants a single wallet 🤷♂️
Btw, currently, deriving a custom path will never update the 'nextAddrIndex' value. I'm not sure about that. Reason why I did not implemented it is because I don't think the current solution should last (nextAddrIndex should not be stored but computed.) I also think the interface for the post request should look like this:
Could you please explain what you have in mind for "not stored but computed"?
| useValue: {} | ||
| }, | ||
| { | ||
| provide: KeyGenerationService, | ||
| useValue: {} |
There was a problem hiding this comment.
lol... put something in the dependency injection container, otherwise that'll blow with shady errors.
| useValue: {} | |
| }, | |
| { | |
| provide: KeyGenerationService, | |
| useValue: {} | |
| useValue: mock<ImportRepository>() | |
| }, | |
| { | |
| provide: KeyGenerationService, | |
| useValue: mock<KeyGenerationService>() |
There was a problem hiding this comment.
Yeah so I did that to 'fix' the test module as it requires a KeyGenService to be passed. But I did not unit test it, nor do I expect any values from it. What would you consider a valid 'not being used in the test yet needs to be here in order for the test module to run' state ? I thought empty made sense...
There was a problem hiding this comment.
What would you consider a valid 'not being used in the test yet needs to be here in order for the test module to run' state ?
Both an empty object and a mock are valid answers. Personally, I'd prefer an empty mock, even for a test that's not yet using it, because it lays the foundation for future changes. With an empty mock, anyone coming in later just needs to add the desired behavior. With an empty object, they'd have to write the whole thing from scratch.
| }, | ||
| { | ||
| provide: ClientService, | ||
| useValue: { |
There was a problem hiding this comment.
You can use a mock here to mock the whole implementation plus a custom behavior for the findById. Moreover, it'll enforce types on the mocked values which reduces the chance of adding a liability in the test due to a wrong interpretation of how the method works.
| useValue: { | |
| useValue: mock<ClientService>().findById.mockResolvedValue({}) |
apps/vault/src/vault/core/service/__test__/unit/key-generation.service.spec.ts
Outdated
Show resolved
Hide resolved
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [ | ||
| KeyGenerationService, | ||
| { | ||
| provide: WalletRepository, | ||
| useValue: { | ||
| // mock the methods of WalletRepository that are used in keyGenerationService | ||
| // for example: | ||
| save: jest.fn().mockResolvedValue({ | ||
| id: 'walletId', | ||
| address: '0x2c4895215973CbBd778C32c456C074b99daF8Bf1', | ||
| privateKey: PRIVATE_KEY | ||
| }) | ||
| } | ||
| }, | ||
| { | ||
| provide: ImportRepository, | ||
| useValue: {} | ||
| }, | ||
| { | ||
| provide: ClientService, | ||
| useValue: { | ||
| findById: jest.fn().mockResolvedValue({ | ||
| backupPublicKey: rsaBackupKey | ||
| }) | ||
| } | ||
| }, | ||
| { | ||
| provide: MnemonicRepository, | ||
| useValue: { | ||
| save: jest.fn().mockResolvedValue({ | ||
| mnemonic, | ||
| keyId: 'keyId' | ||
| }) | ||
| } | ||
| }, | ||
| { | ||
| provide: BackupRepository, | ||
| useValue: { | ||
| save: jest.fn().mockResolvedValue({}) | ||
| } | ||
| } | ||
| ] | ||
| }).compile() |
There was a problem hiding this comment.
Why are you generating the whole application again? Tell me the problem you were having and I'll try to help you.
There was a problem hiding this comment.
I want to modify the return values from mock services and their methods
There was a problem hiding this comment.
There's a better and more idiomatic way to solve that. Move the mocks to let variables that you define before compiling the module, then, when you need custom behavior, change the mock in the describe or it block.
This way you don't have to compile the whole application again.
apps/vault/src/vault/core/service/__test__/unit/key-generation.service.spec.ts
Outdated
Show resolved
Hide resolved
| @@ -46,4 +48,13 @@ export class ImportController { | |||
|
|
|||
| return response | |||
| } | |||
|
|
|||
| @Post('/seed') | |||
There was a problem hiding this comment.
| @Post('/seed') | |
| @Post('/seeds') |
There was a problem hiding this comment.
Why ? You want to import one seed
There was a problem hiding this comment.
It doesn't matter the number of things you want to do. It's the arbitrary standard we have been trying to follow. It's annoying, but it creates consistency on our APIs.
- Use nouns for endpoint
- Always in the plural
Further reading: https://github.com/stickfigure/blog/wiki/How-to-%28and-how-not-to%29-design-REST-APIs#rule-1-do-use-plural-nouns-for-collections
There was a problem hiding this comment.
Got it. Was unaware of this standard since the other endpoints were singular when I went there. Modifying them aswell
| } | ||
|
|
||
| async save(clientId: string, key: Backup): Promise<Backup> { | ||
| await this.keyValueService.set(this.getKey(clientId, key.keyId.toLowerCase()), coerce.encode(Backup, key)) |
There was a problem hiding this comment.
Hint: instead of calling toLowerCase everywhere, add it to the getKey:
`${this.KEY_PREFIX}:${clientId}:${id}`.toLowerCase()There was a problem hiding this comment.
Yeah I copy-pasted from another repository and did not changed logic, just the objects and names. I'm gonna change it everywhere in another PR then
| DerivationPathStart.refine((val): val is DerivationPathStart => val.startsWith(`m/44'/60'/`), { | ||
| message: "Derivation path must start with m/44'/60'/" | ||
| }) |
| const DerivationPathStart = z.string().startsWith(`m/44'/60'/`) | ||
| type DerivationPathStart = `m/44'/60'/${string}` |
There was a problem hiding this comment.
I think you can use custom here like we do for the hex.
const DERIVATION_PATH_PREFIX = "m/44'/60'/"
export const DerivationPath = z.custom<`${typeof DERIVATION_PATH_PREFIX}${string}`>(
(value) => {
const result = z.string().startsWith(DERIVATION_PATH_PREFIX).safeParse(value)
if (result.success) {
return value
}
return false
},
{
message: `Derivation path must start with ${DERIVATION_PATH_PREFIX}`
}
)
export type DerivationPath = z.infer<typeof DerivationPath>
| }) | ||
| export type Wallet = z.infer<typeof Wallet> | ||
|
|
||
| export const UserFacingWallet = z.object({ |
There was a problem hiding this comment.
The term user facing is weird.
What do you think of private/public or internal/external?
| IMPORTED: 'imported', | ||
| GENERATED: 'generated' |
There was a problem hiding this comment.
A quick suggestion, make constant values upper case so they are very clear to the reader.
| IMPORTED: 'imported', | |
| GENERATED: 'generated' | |
| IMPORTED: 'IMPORTED', | |
| GENERATED: 'GENERATED' |
| export const RootKey = z.object({ | ||
| keyId: z.string().min(1), | ||
| mnemonic: z.string().min(1), | ||
| origin: z.union([z.literal('imported'), z.literal('generated')]), |
There was a problem hiding this comment.
| origin: z.union([z.literal('imported'), z.literal('generated')]), | |
| origin: z.union([z.literal(SeedOrigin.IMPORTED), z.literal(SeedOrigin.GENERATED)]), |
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [ | ||
| KeyGenerationService, | ||
| { | ||
| provide: WalletRepository, | ||
| useValue: { | ||
| // mock the methods of WalletRepository that are used in keyGenerationService | ||
| // for example: | ||
| save: jest.fn().mockResolvedValue({ | ||
| id: 'walletId', | ||
| address: '0x2c4895215973CbBd778C32c456C074b99daF8Bf1', | ||
| privateKey: PRIVATE_KEY | ||
| }) | ||
| } | ||
| }, | ||
| { | ||
| provide: ImportRepository, | ||
| useValue: {} | ||
| }, | ||
| { | ||
| provide: ClientService, | ||
| useValue: { | ||
| findById: jest.fn().mockResolvedValue({ | ||
| backupPublicKey: rsaBackupKey | ||
| }) | ||
| } | ||
| }, | ||
| { | ||
| provide: MnemonicRepository, | ||
| useValue: { | ||
| save: jest.fn().mockResolvedValue({ | ||
| mnemonic, | ||
| keyId: 'keyId' | ||
| }) | ||
| } | ||
| }, | ||
| { | ||
| provide: BackupRepository, | ||
| useValue: { | ||
| save: jest.fn().mockResolvedValue({}) | ||
| } | ||
| } | ||
| ] | ||
| }).compile() |
There was a problem hiding this comment.
There's a better and more idiomatic way to solve that. Move the mocks to let variables that you define before compiling the module, then, when you need custom behavior, change the mock in the describe or it block.
This way you don't have to compile the whole application again.
| }> { | ||
| const { keyId, encryptedSeed } = body | ||
|
|
||
| // const mnemonic = await this.decryptSeed(clientId, keyId, encryptedSeed) |
There was a problem hiding this comment.
| // const mnemonic = await this.decryptSeed(clientId, keyId, encryptedSeed) |
| const client = await this.clientService.findById(clientId) | ||
| const backup = await this.#maybeEncryptAndSaveBackup(clientId, kid, mnemonic, client?.backupPublicKey) |
There was a problem hiding this comment.
[IMPORTANT]
The synergy between maybeEncryptAndSaveBackup and saveMnemonic is difficult to wrap my head around.
- What happens if
maybeEncryptAndSaveBackupreturns undefined? - What happens if a user passes a backup key, but it's invalid (see lines 36 and 37)?
There was a problem hiding this comment.
What happens if maybeEncryptAndSaveBackup returns undefined?
nothing happens. Backup is optional. If the user didn't already provided a backup public key you can encrypt with, then you... don't backup the mnemonic.
What happens if a user passes a backup key, but it's invalid (see lines 36 and 37)?
Nothing happens aswell. That means that the backup key provided was not an RSA key, and we currently only support RSA encryption/decryption. I was unsure about this: #268 (comment)
So it still imports the mnemonic, and warn logs us that a user have a wrongly setuped backup key.
There was a problem hiding this comment.
Ok, let me see if I understood correctly.
- The user adds a backup because they want to feel safe and have the ability to recover their data.
- Unfortunately, they entered an invalid key.
- The application indicated that everything was ok.
- The user moves on with their life, thinking, "I'm safe," and stores the backup key.
- Months pass by, and some issue occurs. The user comes back to recover the data.
- The user can't recover the data because they never had a valid backup key in the first place.
Is that correct?
There was a problem hiding this comment.
The user adds a backup because they want to feel safe and have the ability to recover their data.
This is not implemented.
So most likely when it does,
The user adds a backup because they want to feel safe and have the ability to recover their data.
Unfortunately, they entered an invalid key.
The application indicated that everything was ok.
This won't be true. And endpoint will not return that everything was ok. As stated in the thread I gave you, I thought this would need to be 'provide any public key' and not just 'provide an rsa key' for backup.
So problem there is with the model I stored that is publicKeySchema, and not rsaPublicKeySchema. Thus this defensive code that shouldn't be here. Got it.
There was a problem hiding this comment.
I don't follow your response. What do you pretend to do?
To be transparent, I'm concerned about the behavior here. If the user provides the application with the wrong key, it MUST fail the operation with a clear error message, so the user can fix it and try again.
There was a problem hiding this comment.
Ok. So here, the user does not provides a backupPublicKey. If the key was already provided before, vault uses it to produce a backup by encrypting the mnemonic with it.
Where you are correct is that this bit of code shouldn't be responsible to validade the key, as it was provided before. So it shouldn't validate the key is an RSA public key. The key should have been defined as a specific RSA Public Key, not any Public Key. So that there is no need to validate that the key after it was already accepted and inserted in DB.
Commit that fixes it:
bcb66e7
There was a problem hiding this comment.
I hope it clarifies the issue, lmk
| @@ -46,4 +48,13 @@ export class ImportController { | |||
|
|
|||
| return response | |||
| } | |||
|
|
|||
| @Post('/seed') | |||
There was a problem hiding this comment.
It doesn't matter the number of things you want to do. It's the arbitrary standard we have been trying to follow. It's annoying, but it creates consistency on our APIs.
- Use nouns for endpoint
- Always in the plural
Further reading: https://github.com/stickfigure/blog/wiki/How-to-%28and-how-not-to%29-design-REST-APIs#rule-1-do-use-plural-nouns-for-collections
| } | ||
|
|
||
| async save(clientId: string, key: Backup): Promise<Backup> { | ||
| await this.keyValueService.set(this.getKey(clientId, key.keyId.toLowerCase()), coerce.encode(Backup, key)) |
| if ( | ||
| rsaPublicKeySchema.safeParse(backupPublicKey).success === false && | ||
| rsaPrivateKeySchema.safeParse(backupPublicKey).success === false | ||
| ) { | ||
| this.logger.warn('Invalid backup public key provided. Need an RSA key', { clientId }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Should this not throw? Or at least log an error?
It appears this scenario is
- A backup key exists
- The backup key is corrupt
- Therefore, we will think there is a backup of these keys, but in reality nothing will be backed up, so in a disaster scenario you lose all your keys while thinking you had backups....
Note: throwing likely should not completely end the execution flow though, because if you've already generated & saved the key then you don't want to crash and say unsuccessful while there is a new key already.
There was a problem hiding this comment.
Commit that fixes it:
bcb66e7
| import { ApiPropertyOptional } from '@nestjs/swagger' | ||
| import { IsNumber, IsOptional, IsString } from 'class-validator' | ||
| import { IsBoolean, IsNumber, IsOptional, IsString } from 'class-validator' | ||
|
|
||
| export class CreateClientDto { |
There was a problem hiding this comment.
@Ptroger You didn't change this one to a zod schema
| export const DerivationPath = z.union([ | ||
| z.custom<`${typeof DERIVATION_PATH_PREFIX}${string}`>( | ||
| (value) => { | ||
| const result = z.string().startsWith(DERIVATION_PATH_PREFIX).safeParse(value) | ||
|
|
||
| if (result.success) { | ||
| return value | ||
| } | ||
|
|
||
| return false | ||
| }, | ||
| { | ||
| message: `Derivation path must start with ${DERIVATION_PATH_PREFIX}` | ||
| } | ||
| ), | ||
| z.literal('next') | ||
| ]) | ||
| export type DerivationPath = z.infer<typeof DerivationPath> |
There was a problem hiding this comment.
Why require a specific prefix?
It's completely valid to derive a key from a different path.
There was a problem hiding this comment.
I see viem uses the typing with the prefix. I question enforcing their constraint but okay with it to start -- but why do we need this whole custom zod parser?
There was a problem hiding this comment.
How do you enforce that a wallet was not already derived without a specific standard for paths ?
Ofc you could generate a random string and have 99% chance that it was not already derived. Still doesn't feel very clean.
Viem partially followed bip44, I followed viem.
Meaning, part of the prefix should instead be programmatic (60 is ethereum slip44 in the prefix).
I think if we wish to integrate people that may already have derived wallet and want to import a seed that was already used, it would be better if we were following bip44 when deriving a new wallet from this seed.
| const header = decodeProtectedHeader(encryptedSeed) | ||
| const kid = header.kid | ||
|
|
||
| if (!kid) { | ||
| throw new ApplicationException({ | ||
| message: 'Missing kid in JWE header', | ||
| suggestedHttpStatusCode: HttpStatus.BAD_REQUEST | ||
| }) | ||
| } | ||
|
|
||
| const encryptionPrivateKey = await this.importRepository.findById(clientId, kid) | ||
|
|
||
| if (!encryptionPrivateKey) { | ||
| throw new ApplicationException({ | ||
| message: 'Encryption Key Not Found', | ||
| suggestedHttpStatusCode: HttpStatus.NOT_FOUND | ||
| }) | ||
| } | ||
|
|
||
| const mnemonic = await rsaDecrypt(encryptedSeed, encryptionPrivateKey.jwk) |
There was a problem hiding this comment.
This is all identical to the importEncryptedPrivateKey method, might be nice just use the same exact code, so both just do
const mnemnoic = await this.#decrypt(clientId, encryptedSeed)
or
const pk = await this.#decrypt(clientId, encryptedPrivateKey)
| return wallets.length === 1 ? wallets[0] : wallets | ||
| } | ||
|
|
||
| async storeRootKeyAndFirstWallet( |
There was a problem hiding this comment.
Let's remove this function. The word And is a signal that it should be 2 separate functions instead.
Have 1 function that saves the mnemonic.
Have another function that derives a wallet.
If you want to save the root key and then derive the first wallet, you call both functions.
| constructor(wallets: PrivateWallet[] | PrivateWallet) { | ||
| super() | ||
| this.wallets = Array.isArray(wallets) ? wallets.map(privateToPublicWallet) : privateToPublicWallet(wallets) | ||
| } |
There was a problem hiding this comment.
Ah so that's how you add a constructor to the zod dto!
FYI if you don't actually need to do anything "different" then you don't need to add a constructor, you can just use DeriveWalletResponseDto.create() which will effectively parse the values into the zod type.
There was a problem hiding this comment.
didn't knew that, I think I can remove one or two useless constructors thens
| @Injectable() | ||
| export class BackupRepository { | ||
| constructor(private keyValueService: KeyValueService) {} | ||
| private KEY_PREFIX = 'mnemonic' |
There was a problem hiding this comment.
This doesn't seem like the right prefix for the backup
There was a problem hiding this comment.
copy pasting is dangerous indeed
| if (path === 'next') { | ||
| wallet = await deriveWallet(mnemonicToRootKey(mnemonic), { rootKeyId: opts.keyId, addressIndex: curr }) | ||
| curr++ | ||
| } else { | ||
| wallet = await deriveWallet(mnemonicToRootKey(mnemonic), { rootKeyId: opts.keyId, path }) | ||
| } |
There was a problem hiding this comment.
Can you remove the value next? so instead if a derivationPath is passed, use it, otherwise calculate the next one?
| keyId: z.string().min(1), | ||
| mnemonic: z.string().min(1), | ||
| origin: z.union([z.literal(SeedOrigin.GENERATED), z.literal(SeedOrigin.IMPORTED)]), | ||
| nextAddrIndex: z.number().min(0).default(0) |
There was a problem hiding this comment.
Can we get rid of this? Storing the next-derivation index feels like an antipattern.
Instead, at runtime for key derivation, query for the wallets & look at the last one.
I suggest using a "max" approach. You always derive the next address after the latest one, so if a specific account is derived at a specific path & skips some, then that's the new "current" index. This will avoid the most weird scenarios IMO.
There was a problem hiding this comment.
I didn't planned for this to stay, was a hack before I could query wallets and find the last one.
Btw, this capability is exactly why we need a deterministic derivation path with a prefix and indexes
| export const RootKey = z.object({ | ||
| keyId: z.string().min(1), | ||
| mnemonic: z.string().min(1), | ||
| origin: z.union([z.literal(SeedOrigin.GENERATED), z.literal(SeedOrigin.IMPORTED)]), |
There was a problem hiding this comment.
origin is a good idea, but I worry it's going to be misleading.
My suggestion is that you need to add this field into the wallet schema as well.
Here's the scenario
- Import Seed A, which is stored as
imported - Generate Seed B, which is stored as
generated - Derive Private Key R from Seed A
- Derive Private Key X from Seed B
- Import Private Key Q
Which wallets are imported?
- Key R can be called
importedby looking up seed A and seeing that it was imported - Key X can be called
generatedby looking up seed B and seeing that it was generated - Key Q maybe could be inferred that a null seed means it must have been imported?
wdyt?
There was a problem hiding this comment.
Not sure I understand your comment.
You are saying this:
Key R can be called imported by looking up seed A and seeing that it was imported
Key X can be called generated by looking up seed B and seeing that it was generated
Key Q maybe could be inferred that a null seed means it must have been imported?
Is not enough, and origin should be explicitly added to wallet schema ?
I think it makes sense to add it there. I wrote this because i know that origin exists on rootkey. But for someone else that comes in the codebase, it can be handy to explicitly see the origin in the wallet
* mnemonic generation and key derivation from mnemonic * backup stored mnemonics * added e2e test * logs * repared failing test after Wallet type modification * fixed linting * revised unit test * Unit tests on util, switched table storage for unencrypted KV storage * switch dto declaration to zod * schema and naming changes * switched url to plural * import persistence module and not prisma service * backup has to be rsa key on schema * create-client-dto use zod, use PublicWallet.parse instead of custom func * removed function * removed unnecessary constructor for DTO * remove code duplication in importService * added origin to wallet schema
* mnemonic generation and key derivation from mnemonic * backup stored mnemonics * added e2e test * logs * repared failing test after Wallet type modification * fixed linting * revised unit test * Unit tests on util, switched table storage for unencrypted KV storage * switch dto declaration to zod * schema and naming changes * switched url to plural * import persistence module and not prisma service * backup has to be rsa key on schema * create-client-dto use zod, use PublicWallet.parse instead of custom func * removed function * removed unnecessary constructor for DTO * remove code duplication in importService * added origin to wallet schema
wcalderipe
deleted the
feature/nar-1595-vault-root-key-generation-derivation
branch
No description provided.