SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
731 stars 171 forks source link

Create seed with coin flips #619

Open BtcAutoNode opened 3 weeks ago

BtcAutoNode commented 3 weeks ago

Description

Describe the change simply. Provide a reason for the change. Creation of seeds via entropy input from coin flips.

Include screenshots of any new or modified screens (or at least explain why they were omitted) grafik grafik

This pull request is categorized as a:

Checklist

If you modified or added functionality/workflow, did you add new unit tests?

grafik

I have tested this PR on the following platforms/os:

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

kdmukai commented 3 days ago

I think this was my fault (definitely not yours), but now that we're actually exposing the coin flip code to users, generate_mnemonic_from_coin_flips should:

I realize that the KeyboardScreen.return_after_n_chars gives us the same length guarantees, but it's better for the mnemonic_generation helper to be bulletproof.

kdmukai commented 3 days ago

Confirmed that the unit tests match the expected results in iancoleman.io

kdmukai commented 3 days ago

Would ideally have 2-3 FlowTests:

(the current test_seed_flows.py has lots of missing FlowTests for existing functionality, unfortunately)

kdmukai commented 3 days ago

Also, tagging in @easyuxd for thoughts on the "H" and "T" keyboard icons. They're being pulled from the FontAwesome pack, not our UI font.

kdmukai commented 3 days ago

Should also include new screenshots in tests/screenshot_generator/generator.py

easyuxd commented 3 days ago

Since we're trying to get away from using FontAwesome icons, can we swap the "H" and "T" for the regular Open Sans characters?

Adding a coinflip icon to our SeedSigner icon set is a to-do for me, so I'm okay with using this FontAwesome stack-of-coins icon as a placeholder in the menu until I can replace it.

BtcAutoNode commented 3 days ago

@easyuxd : Would this be better? 1) grafik Or this one: 2) grafik

Initially I tried to do it exactly like the dice roll seed creation. Only copying the ...dice... functions/constants etc and changed 'dice' to 'coin'. I had a hard time finding icons and decided to use H and T, but so, using these awesomefont characters the buttons had the same size as in the dice creation screen. This is also a matter of what someone likes more or not (the 2 screens I've added here).

Or without the additional texts: 3) grafik And: 4) grafik

@kdmukai : As mentioned in the chat, the same changes as proposed above will need to be done for the dice roll scenario as well, I guess. I was only able to do this with my 0% python experience if I can copy and paste existing code and check what needs to be changed (like we only have 2 values, different icons etc). So without these changes in the dice creation I have nothing to look at and re-use ,)

BtcAutoNode commented 3 days ago

I've just recognized by chance that in PR534 someone already added coding for the coin flip seed creation, hm. And in the Seedsigner dev branch in the tools_screens.py file there is already a function for the coin flip (same as in this PR I think). So I am a little confused right now. This one here: grafik

easyuxd commented 3 days ago

@BtcAutoNode Much better with the change in font, thanks! I prefer the full-width layout of #2. It's more balanced, and the counter at the bottom is useful and it helps understand the H and T labels.

I can't speak to a previous coin-flip effort, either @kdmukai or @newtonick should know.