search

stake-house / wagyu-key-gen

GNU General Public License v3.0
60 stars 42 forks source link

Allowing non-24 word mnemonics #183

Closed valefar-on-discord closed 6 months ago

valefar-on-discord commented 6 months ago

Updating the Existing Mnemonic flow to remove the 24 word requirement and change to a minimum of 12 words.

The CLI seems to support any valid mnemonic above and including 12 words, 9 words fail as an example. We can introduce a similar restriction on our end and then depend on the CLI for anything beyond to do the verification.

Speaking of verification, I noticed the returned error message was pretty technical in response and some users may be confused. I created a search string of "That is not a valid mnemonic" that is always returned with the error message and, if so, will display a more friendly error message encouraging the user to double check their input.

fixes #164

remyroy commented 6 months ago

The new logic enables 13, 43 or 159 words for the mnemonic. I wonder if these even work or if they are common enough for us to support in a way that is not confusing and not error prone. I know 24 words is common, I know some people used 12 words. I wonder if we should restrict importing to only these 2 values. Let me check in staking-deposit-cli and around for a few reference documents.

remyroy commented 6 months ago

It seems like staking-deposit-cli only accepts 12, 25 or 3 word lenghts from https://github.com/ethereum/staking-deposit-cli/blob/fdab65d33a63632e1935e9a9235119a46e37c221/staking_deposit/key_handling/key_derivation/mnemonic.py#L116 where 25 words are actually 24 words + an empty word for the 25th word if you do not use a password.

valefar-on-discord commented 6 months ago

It seems like staking-deposit-cli only accepts 12, 25 or 3 word lengths from https://github.com/ethereum/staking-deposit-cli/blob/fdab65d33a63632e1935e9a9235119a46e37c221/staking_deposit/key_handling/key_derivation/mnemonic.py#L116 where 25 words are actually 24 words + an empty word for the 25th word if you do not use a password.

I was confused by that line as well but range creates a list of options [x, y) in increments of z so in this case: 

y = range(12, 25, 3)
print(list(y))

would output [12, 15, 18, 21, 24]

Given there were a few more options than expected, I thought it best to mostly depend on the CLI for mnemonic validation. If you would like we could put an upper bound of 24 as well to reduce that dependency.

remyroy commented 6 months ago

Oh, you are right about that range function. My bad. Let me keep exploring what the various documents about key derivation says about the mnemonic lenght.

remyroy commented 6 months ago

I think we should only support mnemonic of lengths of either 12, 15, 18, 21 or 24 words and validate that length before calling the staking-deposit-cli internals. If you think this is fine, can you update this PR to validate for these lengths and show a descriptive message if the length is not supported.

valefar-on-discord commented 6 months ago

I think we should only support mnemonic of lengths of either 12, 15, 18, 21 or 24 words and validate that length before calling the staking-deposit-cli internals. If you think this is fine, can you update this PR to validate for these lengths and show a descriptive message if the length is not supported.

Updated to support those specific lengths and made a dynamic messaged based on supported lengths

remyroy commented 6 months ago

I just tested this PR with 12, 15, 18, 21 and 24 words mnemonic and it works fine. I just tested with lengths different than 12, 15, 18, 21 and 24 and the correct error message is shown.