[Olorunshogo]

PR description

Description

key generate --entropy only matched 12; every other value was silently coerced to a 24-word mnemonic, so 15, 0 or 999 all produced 24 words with no error.

--entropy now parses into a MnemonicWordCount ValueEnum limited to 12 and 24, so any other value is rejected at parse time and --help lists the options. The flag name, -e, WORD_COUNT env var and default of 12 are unchanged.

Fixes #362

--entropy	Before	After
12 / default	12 words	12 words
24	24 words	24 words
15 / 18 / 21	24 words	error listing valid values
0 / 13 / 999	24 words	error listing valid values

Notes to reviewers

• WordCount derives no traits (no Clone), so clap can't parse into it directly. MnemonicWordCount mirrors it with a From impl, following the CompileScriptType pattern used by compile --type.
• Behavior change: values other than 12 and 24 that previously produced 24 words now error. Noted in the CHANGELOG.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[emma31-dev]

Looks good to me. I'll test it later.

[emma31-dev]

Aside that minor issue everything looks good. I have tested everything and its adapted well.

[Olorunshogo]

Please take a look at this @tvpeter @vadim-anfv

[emma31-dev]

@Olorunshogo it has been 2 days . Relax as older PRs will get reviewed before this

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.12%. Comparing base (d2317d2) to head (f6f6fe8).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   59.08%   59.12%   +0.03%     
==========================================
  Files          25       25              
  Lines        4646     4648       +2     
==========================================
+ Hits         2745     2748       +3     
+ Misses       1901     1900       -1     

Flag	Coverage Δ
rust	59.12% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Jennycj]

Thanks for working on this @Olorunshogo
tACK a24de07
-e 12/15/18/21/24 gives exactly that many words with the default still being 12. When arguments not in the possible values list are passed, it errors with exit code 2 and a list of the possible values.

I left an optional suggestion inline.

[vadim-anfv]

tACK a24de07

Tested locally: all five valid word counts produce a mnemonic of the expected length, and invalid values are rejected at parse time.

[Olorunshogo]

Thanks for testing! I applied the default suggestion, and that's the only change since a24de07 . Would appreciate a quick re-ACK on 5efd4ec when you get a chance.

[vadim-anfv]

ACK 5efd4ec

The only change since I tested is the default, now default_value_t = MnemonicWordCount::Words12, which resolves to the same "12".

[Jennycj]

ACK 5efd4ec

All things work as they did earlier with the default still being 12.
Thanks for applying the suggestion!

[tvpeter]

Concept NACK.

So I have been wondering the genuine utility for this PR. Is it to enable all supported BIP39 word counts because the library support it or it is to validate and establish that only 12 and 24 word counts were supported?

For compatibility, the supported hardware wallet devices - coldcard, Bitbox02, Jade, Ledger and Specter do not support 15 or 21 word counts generation. Most of them support restoring 18 word count but do not offer the option to generate and use them. Also, over 90% of popular BIP39 wallets primarily create and are optimized around 12 and/or 24 words only. Bear in mind that, Core wallet do not use BIP39.

If we are validating that only 12 and 24 BIP39 word count are supported, then I think it is reasonable. Aside that, I don't think there is any real value for enabling them.

I am open to hearing other opinions on this.
Thank you.

[emma31-dev]

@tvpeter I could agree there is not much utility in supporting all the BIP39 word counts but that is not the major part of my issue. Currently, the flag --entropy takes any number that is not 12 and generates a 24 word phrase. I thought fixing just that alone was too small of a change so expanding support is logical. Maybe its best to show a warning that the 15, 18, 21 word count has low support but still allow it. If the gains are not worth it then maybe --entropy should be restricted to just 12 and 24.

[vadim-anfv]

Is it to enable all supported BIP39 word counts because the library support it or it is to validate and establish that only 12 and 24 word counts were supported?

@tvpeter let's make it the second one. The PR is about the bug in #362, not the word counts: --entropy silently returns 24 words for anything other than 12, like 15, 0 or 999. @emma31-dev makes the same point above, and that needs fixing either way.

Then the PR only fixes the bug and tightens validation: 12 and 24 as before, anything else rejected instead of silently coerced.

@Olorunshogo so drop Words15, Words18, Words21 and their test cases, rest unchanged. And nothing to warn about, if we don't offer those lengths.

[vadim-anfv]

Correction: #362 does ask for all five lengths, so 12 and 24 is a deliberate narrowing, not the original scope. I ACKed the five-count version twice and still think it is defensible, just not worth holding the PR over.

@Olorunshogo "rest unchanged" was too loose, four more places go with the enum:

• CHANGELOG.md, still says 15, 18 and 21 are now supported
• the PR title
• in tests/integration/init.rs, the [12, 15, 18, 21, 24] loop, and the rejection loop, which should now cover 15, 18, 21 too

[tvpeter]

@tvpeter let's make it the second one. The PR is about the bug in #362, not the word counts: --entropy silently returns 24 words for anything other than 12, like 15, 0 or 999. @emma31-dev makes the same point above, and that needs fixing either way.

Then the PR only fixes the bug and tightens validation: 12 and 24 as before, anything else rejected instead of silently coerced.

@Olorunshogo so drop Words15, Words18, Words21 and their test cases, rest unchanged. And nothing to warn about, if we don't offer those lengths.

I agree.

[Olorunshogo]

Would appreciate a re-ACK on f6f6fe8 when you get a chance.

Thanks for the reviews everyone! I have narrowed the scope to 12 and 24 as @vadim-anfv suggested, and covered all four places you listed:

• MnemonicWordCount now only has Words12 and Words24
• The valid test loop is now [12, 24], and the rejection loop also covers 15, 18 and 21
• Updated the CHANGELOG line
• Updated the PR title

I would appreciate a re-ACK on f6f6fe8