-
Notifications
You must be signed in to change notification settings - Fork 174
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
[New Feature] Entropy-to-mnemonic CLI utility #404
Conversation
|
||
|
||
|
||
if __name__ == "__main__": |
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.
Besides another sole instance in settings_definitions.py, nowhere else in our codebase are we already using this convention -- of an executable cli/library. Would this entire if __name__ == "__main__":
block be more at home in tools/???
I tried moving this "main" block into tools/mnemonic_generation.py. It makes importing seedsigner a strange hack... but if user installed seedsigner locally, it would just be adding a from seedsigner.helpers.mnemonic_generation import *
and then we wouldn't have cli code that exists in the standard codebase except where users clone the repo.
It's just a thought, I'm still not sure. It's so common while prototyping, maybe less common in production unless users can normally use it (not the case for code in seedsigner-os).
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.
If the concern/goal is to keep the CLI code out of the standard release package, I think there are better ways to package it (e.g. its own dir outside of src
that isn't included in a release or a dir in src
that is explicitly excluded from the build process).
That being said, for now I like the directness of the organization here: this file does X for SeedSigner, run its CLI to play with X interactively yourself. No imports (aside from embit
) or redirects or obfuscation. This code does X. Period.
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 hereby withdraw my comments above.
I too see the value in the way it is all-in-one. Can verify dice rolls with the exact file that seedsigner is using.
babeebe
to
ca660a6
Compare
I've had no problems duplicating seedwords while doing so w/ dice labeled 1-6. However for coins, when running for a 24w seed:
I get:
while I was expecting:
and similarly for a 12w made from coins:
I get:
while I was expecting:
I'm also having a problem with the --zero-indexed-dice option
I get:
While I was expecting to get the same as above... because, I'm not sure, I was thinking that each roll would just be reduced by 1 and then they'd all be hashed at the end. Ian Coleman gives me two different mnemonics depeding on interpretation; when I select "Base 10" I get:
and when I select "Hex" I get:
|
For your first example, to reproduce the same coin flips in iancoleman.io, you have to change "Mnemonic length" from "Use Raw Entropy (3 words per 32 bits)" to "24 Words".
For zero-indexed dice rolls, you can use Base6, Base10, or Hex and, again, specify "Mnemonic Length" as "12 Words". I've updated the documentation with this info. |
Yes, I did test this, and yes, I did ACK this 3 days ago. But I have additional thoughts expressed below... so I take back my ACK. I'll try not to make this a habit. |
30da08a
to
3bca811
Compare
In the meeting this morning, I shared the thought of waiting to merge this until after the next release. It's about "Let's not over-feed the concern-trolls with code changes that include lots of refactoring just before release on a code file named 'mnemonic_generation.py'". Also, after looking at the code, while I'm not concerned, I wonder... "Does Maybe this code deserves many-many ACKs before it's considered for merge? |
@jdlcdl this is a great point about the But for the CLI's stated use case (interactive testing, confirmation, etc) I think some way to generate new test seeds is useful. And note that the usage does include:
|
3bca811
to
fe55ccd
Compare
Now that Release 0.7.0 is out, so that peers will have plenty of time to review this otherwise sensitive code: As of fe55ccd ACK tested. |
@kdmukai can you resolve the conflict in this PR? Thanks! |
This PR is pending some slight refactoring to move the cli utility out of |
fe55ccd
to
fe9c248
Compare
Updated! Moved CLI functionality into tools/ and updated the docs and related impacted files. |
* Makes our existing mnemonic_generation.oy accessible as an interactive CLI utility for converting dice rolls or coin flips into 12- or 24-word mnemonics as well as the final word calculation. * Reduces python dependencies in mnemonic_generation.py * Removes need to always specify `wordlist_language_code`. * Adds constants for number of dice rolls for each mnemonic phrase length (since 99 may become 100 soon). * Generates test dice rolls, coin flips, or word selections via its "rand12" and "rand24" option. * Adds support for coin flips, even though the SeedSigner UI does not support coin flips yet (and might not ever). * Updates dice_verification.md accordingly.
3addd5c
to
1ed703b
Compare
@newtonick the "needs revision" tag can be removed. PR has been updated to what I consider a "final" form (pending further feedback). |
Tested and LGTM |
wordlist_language_code
.calculate_checksum
to accept mnemonic as list or string w/various delimiters.