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

ADD: tool to generate last mnemonic word #5722

Merged
merged 14 commits into from Nov 2, 2023

Conversation

3bitcoins
Copy link
Contributor

@3bitcoins 3bitcoins commented Oct 6, 2023

Closes #5680

Screenshot_1697197785

Screenshot_1697197935

@limpbrains
Copy link
Collaborator

Can you please add tests to the PR. Maybe extract checkMnemonic logic to a separate function and write a unit test for it.

Thank you for contributing to BlueWallet!

@3bitcoins
Copy link
Contributor Author

@limpbrains I've moved the core logic to it's own file and added tests. Do you guys plan to ever review #5690 ? If so, I'll adapt it to use this helper instead of having this logic implemented twice.

@limpbrains
Copy link
Collaborator

limpbrains commented Oct 13, 2023

I've tried, works fine.
I think UX can be better.

  • We need to show error if mnemonic is incorrect.
  • show last word before button. Reserve space for it
  • Maybe text explanation of what is going on here whould be nice, user can be confused about why it is random. (I'm not sure)

@3bitcoins
Copy link
Contributor Author

@limpbrains I've swapped the place of the result and the button, and added a message if the input is invalid. I also fixed a couple linter warnings i missed.

Based on how the other tools are, I don't think it's necessary to describe the exact usage of the tool in the screen itself. If you want, maybe some kind of documentation?

@3bitcoins
Copy link
Contributor Author

@Overtorment Do you think you could review this?

@BlueWallet BlueWallet deleted a comment from DrockDaniel Oct 21, 2023
@GladosBlueWallet
Copy link
Collaborator

Wake the fuck up samurai, we have PRs to merge

image

[all PRs for @Overtorment] https://github.com/BlueWallet/BlueWallet/pulls/review-requested/Overtorment

Copy link
Member

@Overtorment Overtorment left a comment

Choose a reason for hiding this comment

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

thanks for the contribution! merging this.
however, theres room for improvement, if you fancy doing a follow-up PR, most importantly, converting it to typescript (we now have a rule - every new file added might be in ts, while old js files are gradually converted)

'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon';
const result = generateChecksumWords(input);
assert.ok(result);
assert.strictEqual(result.length, 8);
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to check actual words generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logically it seems that if the right amount are generated and they're all valid, it's generated the right words

}

const random = await randomBytes(1);
const randomindex = Math.round((random.readUInt8(0) / 255) * (possibleWords.length - 1));
Copy link
Member

Choose a reason for hiding this comment

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

Math.random() might have worked better here, without this awkward buffer-to-int conversion (readUInt8(0) / 255). not a deal breaker tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure to what extent Math.random() was considered cryptographically secure in this project, and opted to use the safer RNG that i see used for other keygen purposes

import * as bip39 from 'bip39';
import createHash from 'create-hash';

// partial (11 or 23 word) seed phrase
Copy link
Member

Choose a reason for hiding this comment

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

link to documentation would be nice

@Overtorment Overtorment merged commit 53271d1 into BlueWallet:master Nov 2, 2023
9 of 10 checks passed
@3bitcoins 3bitcoins deleted the autofill-seed branch November 2, 2023 22:23
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.

New tool: generate a checksum word for a provided mnemonic seed
4 participants