-
Notifications
You must be signed in to change notification settings - Fork 73
feat: replace usize with WordCount enum for mnemonic generation #98
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
base: master
Are you sure you want to change the base?
feat: replace usize with WordCount enum for mnemonic generation #98
Conversation
- Add WordCount enum to restrict word counts to valid BIP39 values (12, 15, 18, 21 & 24) - Update generate(), generate_in(), and generate_in_with() to accept WordCount instead of usize - Update usage in comments and tests accordingly
caarloshenriq
left a comment
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.
tACK
I tested the changes locally:
- Ran the full test suite (
cargo test) and all tests passed. - Performed manual tests by generating mnemonics with different
WordCountvariants (12, 18, and 24 words) usinggenerate,generate_in, andgenerate_in_with. - All generated mnemonics had the correct word count and the generation flow behaved as expected.
Everything worked as intended.
tnull
left a comment
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.
Thanks for looking into this! While it's a nice API improvement, it's probably not super critical so I think we want to hold off on merging this until we have another, more critical change that would require breaking API anyways.
In the meantime, here are a few comments. Please also always disclose in the PR description and commit message if you utilized AI tooling. To this end, rewriting the PR description and commit messages to replace AI slop with concise descriptions would be appreciated. Thanks!
|
|
||
| impl WordCount { | ||
| /// Returns the word count as a usize value. | ||
| pub fn word_count(&self) -> usize { |
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.
Can we make this a impl From<WordCount> for usize?
| R: RngCore + CryptoRng, | ||
| { | ||
| let word_count = word_count.word_count(); | ||
| if is_invalid_word_count(word_count) { |
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.
I think we can remove this check now.
|
|
||
| let dbg = format!("{:?}", m); | ||
|
|
||
| assert!(dbg.starts_with("Mnemonic {") || dbg.starts_with("Mnemonic(") || dbg.contains("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.
These changes here seem unrelated? Please drop them.
This PR:
WordCountenum to restrict word counts to valid BIP39 values (12,15,18,21&24)generate(),generate_in(), andgenerate_in_with()to acceptWordCountinstead ofusizeenum WordCount#97generate()now requires use of theWordCountenum for theword_countparameter.