Skip to content
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

Create seed with coin flips #619

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

BtcAutoNode
Copy link

@BtcAutoNode BtcAutoNode commented Nov 3, 2024

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:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

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

  • No, I’m a fool
  • Yes
  • N/A

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.

- add menu entry and views
- add icons (coins ion for the seed menu and T/H for head and tails)
- add coin flip screen
- add number of flips constants
- change layout to 3, 3 similar to the dice screen
- change order of H and T
- add missing space in menu entry
- add pytest tests similar to the tests for dice rolls
- fix copy/paste error
@BtcAutoNode BtcAutoNode changed the title Create seed with coin Create seed with coin flips Nov 3, 2024
class ToolsCoinEntropyMnemonicLengthView(View):
def run(self):
TWELVE = f"12 words ({mnemonic_generation.COIN__NUM_FLIPS__12WORD} flips)"
TWENTY_FOUR = f"24 words ({mnemonic_generation.COIN__NUM_FLIPS__24WORD} flips)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving these constants to be class-level attrs will allow us to specify 12 vs 24 in various FlowTest scenarios.

@kdmukai
Copy link
Contributor

kdmukai commented Nov 23, 2024

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:

  • Verify the length of the coin flip input string.
  • Verify that the contents are only "0" or "1".

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
Copy link
Contributor

kdmukai commented Nov 23, 2024

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

@kdmukai
Copy link
Contributor

kdmukai commented Nov 23, 2024

Would ideally have 2-3 FlowTests:

  • Create 12-word
  • Create 24-word
  • Start creating, but exit early

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

@kdmukai
Copy link
Contributor

kdmukai commented Nov 23, 2024

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
Copy link
Contributor

kdmukai commented Nov 23, 2024

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

@easyuxd
Copy link
Contributor

easyuxd commented Nov 23, 2024

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
Copy link
Author

BtcAutoNode commented Nov 24, 2024

@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
Copy link
Author

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
Copy link
Contributor

easyuxd commented Nov 24, 2024

@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.

- delete heads/tails icon (and stick to normal letters instead of icons (as easyuxd proposed)
- changed coin flip screen) to reflect changes proposed by easyuxd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants