Skip to content
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

Feat/HD-Wallet SLIP0010 #1190

Merged
merged 17 commits into from
Nov 21, 2023
Merged

Feat/HD-Wallet SLIP0010 #1190

merged 17 commits into from
Nov 21, 2023

Conversation

nillo
Copy link
Contributor

@nillo nillo commented Nov 7, 2023

  • Explain why and how for this pull request
    added functions to match the legacy function but using standards.

Copy link

changeset-bot bot commented Nov 7, 2023

🦋 Changeset detected

Latest commit: 6b8ae3d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@kadena/hd-wallet Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Nov 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
alpha-docs ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2023 3:14pm
docs-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2023 3:14pm
react-ui ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2023 3:14pm
tools ⬜️ Ignored (Inspect) Visit Preview Nov 20, 2023 3:14pm

@nillo nillo changed the base branch from main to feat/hd-wallet November 8, 2023 07:31
*/
export function kadenaGenMnemonic(): string {
const words = bip39.generateMnemonic(wordlist);
if (bip39.validateMnemonic(words, wordlist) === false) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it possible to create invalid words?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid point, only when the wordlist, that comes from @scure39 is diffetent, the generated words could be invalid, this check is redundant when words aren't manually passed

} else {
seed = encryptOrEncodeSeed(seedBuffer);
}
return {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe its better to just return the encrypted version.
then we decrypt it in the keyGeneration function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const seedBuffer = await bip39.mnemonicToSeed(mnemonic);
let seed: string;

if (typeof password === 'string' && password.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we allow empty string as password?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would we? (please elaborate a case where it would be usefull?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a wallet perspective, we're planning on allowing multiple mnemonics. For testnet and devnet I wouldn't care whether it has a password, so it being empty could be fine

* @throws {Error} Throws an error if the seed buffer is not provided, if the indices are invalid, or if encryption fails.
*/
export function kadenaGenKeypair(
seedBuffer: Uint8Array,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should accept the encrypted seed instead.
we can decrypt it easily with the password
What do you say?

* @returns {([string, string] | [string, string][])} - Depending on the input, either a tuple for a single key pair or an array of tuples for a range of key pairs, with the private key encrypted if a password is provided.
* @throws {Error} Throws an error if the seed buffer is not provided, if the indices are invalid, or if encryption fails.
*/
export function kadenaGenKeypair(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its good to write two overloads functions since the return type is also different

Comment on lines 195 to 204
export function kadenaRestoreSeedBufferFromSeed(
seed: string,
password?: string,
): Uint8Array {
if (typeof password !== 'undefined') {
return extractSeedBuffer(seed, password);
} else {
return extractSeedBuffer(seed);
}
}
Copy link
Member

@alber70g alber70g Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the private API.
Password will be mandatory

Comment on lines 227 to 230
export function kadenaSignWithSeed(
seed: Uint8Array,
index: number,
): (tx: IUnsignedCommand) => { sigs: { sig: string }[] } {
Copy link
Member

@alber70g alber70g Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export function kadenaSignWithSeed(
  password: string,
  seed: Uint8Array,
  derivationPath: string, // default value  `m/44'/626'/{$index}'/0'/0'`
  message: string,
): Uint8Array

@javadkh2 javadkh2 force-pushed the feat/hd-wallet-non-legacy branch 2 times, most recently from f81b0a0 to e51a405 Compare November 9, 2023 10:05
export async function kadenaMnemonicToSeed(
password: string,
mnemonic: string,
// wordList: string[] = wordlist,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably turn this on right?

@javadkh2 javadkh2 marked this pull request as ready for review November 14, 2023 10:28
Base automatically changed from feat/hd-wallet to main November 17, 2023 12:59
@javadkh2 javadkh2 changed the title Feat/HD-Wallet non legacy functions Feat/HD-Wallet bip44ish way Nov 17, 2023
@javadkh2 javadkh2 changed the title Feat/HD-Wallet bip44ish way Feat/HD-Wallet SLIP0010 Nov 17, 2023
@javadkh2 javadkh2 merged commit 3045d87 into main Nov 21, 2023
12 checks passed
@javadkh2 javadkh2 deleted the feat/hd-wallet-non-legacy branch November 21, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants