SeedSigner / seedsigner

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

Coin flip UI v4 (branch from Keiths PR404 branch) #442

Closed thedon702 closed 8 months ago

thedon702 commented 1 year ago

Added Coin logic, icon and coin GUI to Seedsigner.

jdlcdl commented 1 year ago

After a quick review of the code... Nice and tight, clean commits, made it look simple.

As for the choice to build off an unmerged PR... I see that you're using functionality that didnt previously exist. I wonder if you couldn't implement enough logic within a helper function within your view, maybe in a parallel PR, so that this can work with or without PR-#404 (in similar fashion as is done in the BruteForceAddressVerificationThread methods: derive_singlesig/derive_multisig. Those are sort of helper funcs inside a class inside a view.... with a TODO note to use the helper functions from derive_mnemonic as soon as they're merged.)

Well done on your first PR.

And now I'll reiterate once again, It would be much appreciated to have your eyes on other PRs and Issues in the forum too.

newtonick commented 1 year ago

This PR is pending revision, review and merge of PR #404. Once PR#404 is merged, conflicts can be resolved and review and testing can take place.

kdmukai commented 8 months ago

@thedon702 apologies for the long hiatus. PR #404 will hopefully be moving along soon. In anticipation of that, can you update this PR?

I guess the steps would be something like:

# Get local copy of latest in #404
git fetch pull/404/head:pr_404

# rebase your branch to latest in #404 
git checkout coin_flip_UI_v4
git rebase pr_404

# interactively resolve any conflicts...

# force push the resulting changes
git push --force
thedon702 commented 8 months ago

@kdmukai It should be up to date now. I noticed that you removed DICE__NUM_ROLLS__12WORD variables, so that took me a little bit to figure out.

Can you guys test the code since I don't have my ss setup for debugging atm. I might fire up my emulator as well to do a quick test. The original code was tested last summer and it seems to be basically the same.

Gracias.

kdmukai commented 8 months ago

@thedon702 Did you mean to close this PR? Also not sure how this PR is now showing 0 changes but your branch in your repo still has your changes.

The DICE_NUM_ROLLS__12WORD is still in my PR: https://github.com/SeedSigner/seedsigner/blob/c4aafd0bf32bf245d636f18efdebaeafd1fce5ce/src/seedsigner/helpers/mnemonic_generation.py#L17

thedon702 commented 8 months ago

@kdmukai Oh my bad, I was working off the main dev branch not your branch. Can I submit a new PR after your branch is merged, that would make my life easier. So we can close this PR and I create a new one after the merge.

thedon702 commented 8 months ago

@kdmukai

Btw, when I try to do git fetch pull/404/head:pr_404

I get this error: fatal: couldn't find remote ref pull/404/head

It's weird because when I created this PR last summer it was working fine from your branch with the exact command. But yeah it's probably easiest to wait until you merge so I can redo this PR on the main dev channel.

kdmukai commented 8 months ago

@thedon702 my bad, should be more like git fetch origin pull/404/head:pr_404

Or maybe git fetch upstream...

thedon702 commented 8 months ago

@kdmukai yep, getting the same error with git fetch origin pull/404/head:pr_404. Maybe it's an issue with codespaces, but I don't know why because it was working last summer.

thedon702 commented 8 months ago

@kdmukai worst case you can just put the code in yourself, since it's just a few edits. codespaces just ins't cooporating with your branch for some reason.

thedon702 commented 8 months ago

@kdmukai I managed to figure out what I was doing wrong, I needed to use

git fetch upstream pull/404/head:pr_404 git rebase pr_404

I created a new draft PR, you can check it out and see if it looks ok?