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

Mnemonic Startup GUI #659

Merged
merged 1 commit into from
Feb 17, 2020
Merged

Mnemonic Startup GUI #659

merged 1 commit into from
Feb 17, 2020

Conversation

KolbyML
Copy link
Contributor

@KolbyML KolbyML commented Feb 16, 2020

This PR was for the #451 a bounty Navcoin had to add a Startup GUI which allowed for the ability to restore and create wallets with Mnemonic words. Mnemonic words are like a read readable master key for BIP32 a bitcoin proposal to make key generation derived from one key or a masterkey instead of being randomly created as before hand.

This page is the first page and it allows for the user to select Restore wallet an option to restore a wallet with your mnemonic words or Create a new wallet if you have never created a wallet that supports Mnemonic words before
image

The restore page is where you would enter your mnemonic words if you were restoring it from the mnemonic words you kept safe. If a word is spelt incorrectly or invalid the box will have a red outline, if it is correct it will have a purple outline.
image

The reveal page. This page is shown after the user selects Create New Wallet and shows the user his Mnemonic words that he must write down and keep safe.
image

The sort page. This page is shown after the reveal page and it is used to confirm whether the user has written down his words.
image

The address the bounty should be paid to NdTrSEe6bMMvVwNUimxs2BiWkX5Xn9MW7o

@mxaddict
Copy link
Contributor

mxaddict commented Feb 16, 2020 via email

@chasingkirkjufell
Copy link
Contributor

Same as mxaddict.

@navbuilder
Copy link

A new build of 5ae6e0d has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@navbuilder
Copy link

A new build of 208b4f0 has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@chasingkirkjufell
Copy link
Contributor

Gitian build of 208b4f0 on Ubuntu 19.04 64GB RAM.

Observation 1:
Wallet client shows welcome page even though wallet.dat was previously created. Users do not have to option to skip the welcome page but only have to choose between creating a new wallet vs importing an old one.

Creating an wallet when wallet.dat already exists actually displays the mnemonic seeds of the existing wallet instead of a new wallet.

Expectation 1:
Wallet client should not have a welcome page when wallet.dat already exists.

Observation 2:
When wallet.dat does not exist, after re-entering all the mnemonic seeds, the wallet will fail to boot up and show
key.cpp:148: CPubKey CKey::GetPubKey() const: Assertion `fValid' failed.

Expectation 2:
When wallet.dat does not exist, after re-entering all the mnemonic seeds, the wallet should load normally.

Observation 3 (non-critical):
When re-entering the seeds, the only way to correct a misplaced seed word is to drag and drop another one on top of it. It'd be nice if the users can simply drag and drop the word back down. An extreme case would be if a user finds out the seeds are misplaced after using all the words, the user will have no way to correct it but return to the previous step and come back doing it all over again.

Also tested:
Importing devnet wallet worked without issue.

@KolbyML
Copy link
Contributor Author

KolbyML commented Feb 16, 2020

Observation 1:
Wallet client shows welcome page even though wallet.dat was previously created. Users do not have to option to skip the welcome page but only have to choose between creating a new wallet vs importing an old one.

I have now fixed this issue stated above ^

Observation 2:
When wallet.dat does not exist, after re-entering all the mnemonic seeds, the wallet will fail to boot up and show
key.cpp:148: CPubKey CKey::GetPubKey() const: Assertion `fValid' failed.

I have also just fixed this issue stated above the issue was that I was redefining words in wallet.cpp instead of just setting the value.

Observation 3 (non-critical):
When re-entering the seeds, the only way to correct a misplaced seed word is to drag and drop another one on top of it. It'd be nice if the users can simply drag and drop the word back down. An extreme case would be if a user finds out the seeds are misplaced after using all the words, the user will have no way to correct it but return to the previous step and come back doing it all over again.

For Observation 3 that is just an inherent issue with Qt drag and drop system and nothing can really be done about that. I spent hours looking into that 8 months back.

Recap Both issues 1&2 were fixed and then I squashed the commit, also issue 3 is just an issue with how qt works and that is the best solution for drag and drop I found.

@navbuilder
Copy link

A new build of 59a69a2 has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@navbuilder
Copy link

A new build of f1cfe1a has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@aguycalled
Copy link
Member

image

When I click in one of the words it is highlighted. If some words in other columns are also highlighted, those are not reset.

@aguycalled
Copy link
Member

image

This window looks too big for the little content and too dry. Would it be possible to make it smaller and maybe add the NavCoin logo from the splash screen?

@mxaddict
Copy link
Contributor

mxaddict commented Feb 16, 2020

GUI is asking me to restore/create a key when I already have a wallet.dat

Capture

My wallet is in ~/.navcoin4/devnet/wallet.dat

@mxaddict
Copy link
Contributor

I would suggest asking for 8 words randomly instead of all 24 words (This would make it easier for the user)

For example have 16 of the words already pre filled on the drag and drop stage.

@mxaddict
Copy link
Contributor

Also might be better if instead of drag and drop, it would be just click the words on the order that they are required (Drag n Drop is harder then just clicking the correct words in the correct order)

And if they make a mistake, they can just click the word that is wrong (In the top section) to remove it.

@mxaddict
Copy link
Contributor

I also see some of the styles are still inline, I can help move these into a stylesheet (Once the GUI PR #557 is merged)

Other than that, nice work on the PR.

@KolbyML
Copy link
Contributor Author

KolbyML commented Feb 16, 2020

GUI is asking me to restore/create a key when I already have a wallet.dat

I Already fixed that can you can confirm which commit you compiled the current commit is: f1cfe1a

@KolbyML
Copy link
Contributor Author

KolbyML commented Feb 16, 2020

I would suggest asking for 8 words randomly instead of all 24 words (This would make it easier for the user)

For example have 16 of the words already pre filled on the drag and drop stage.

This is a onetime setup, so it shouldn't be that much of a hassle do all 24. If the user had to do this more then once in his lifetime, that would be more of a concern, but the goal he knows the placement of all 24 words.

@mxaddict
Copy link
Contributor

GUI is asking me to restore/create a key when I already have a wallet.dat

I Already fixed that can you can confirm which commit you compiled the current commit is: f1cfe1a

I cloned a fresh copy just a an hour ago, so it was latest commit for sure.

Tested on Ubuntu LTS 18.04

@mxaddict
Copy link
Contributor

I would suggest asking for 8 words randomly instead of all 24 words (This would make it easier for the user)
For example have 16 of the words already pre filled on the drag and drop stage.

This is a onetime setup, so it shouldn't be that much of a hassle do all 24. If the user had to do this more then once in his lifetime, that would be more of a concern, but the goal he knows the placement of all 24 words.

This is exactly why I suggested, I do this kind of thing (Importing keys, don't ask why 😅 ) about 1-2 times a month 👍

@mxaddict
Copy link
Contributor

GUI is asking me to restore/create a key when I already have a wallet.dat

I Already fixed that can you can confirm which commit you compiled the current commit is: f1cfe1a

I cloned a fresh copy just a an hour ago, so it was latest commit for sure.

Tested on Ubuntu LTS 18.04

I'm trying the windows version now.

@KolbyML
Copy link
Contributor Author

KolbyML commented Feb 16, 2020

Also might be better if instead of drag and drop, it would be just click the words on the order that they are required (Drag n Drop is harder then just clicking the correct words in the correct order)

^ While that sounds like a good idea, that would probably require a rewrite of what I wrote which wouldn't really be possible at this time

And if they make a mistake, they can just click the word that is wrong (In the top section) to remove it.

^ This is a pretty good idea I can probably add that, the only issue would be which QGraphicsList it got put into which other then that it is possible.

@mxaddict
Copy link
Contributor

Also might be better if instead of drag and drop, it would be just click the words on the order that they are required (Drag n Drop is harder then just clicking the correct words in the correct order)

^ While that sounds like a good idea, that would probably require a rewrite of what I wrote which wouldn't really be possible at this time

And if they make a mistake, they can just click the word that is wrong (In the top section) to remove it.

^ This is a pretty good idea I can probably add that, the only issue would be which QGraphicsList it got put into which other then that it is possible.

Not really deal breakers, I think in the current design we can merge these changes and make modifications to it as needed, important thing is we have a GUI for non CLI savy users to import the keys, good work.

@mxaddict
Copy link
Contributor

Build: f1cfe1a
Ubuntu LTS 18.04

  • Restore works
  • Creating new key works
  • Does not ask to import/create if already have a wallet.dat file

src/mnemonic/generateseed.cpp Outdated Show resolved Hide resolved
@mxaddict
Copy link
Contributor

Build: f795e44
Windows 10 and Ubuntu 18.04

  • Restore works
  • Creating new key works
  • Does not ask to import/create if already have a wallet.dat file on this network (mainnet, devnet, testnet)

@navbuilder
Copy link

A new build of b539b58 has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@navbuilder
Copy link

A new build of f795e44 has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@chasingkirkjufell
Copy link
Contributor

Build: f795e44
Ubuntu 19.04

  • Restore works
  • Creating new key works
  • Does not ask to import/create if already have a wallet.dat file on this network (mainnet, devnet, testnet)

I am happy with this PR now. I will leave the final review for @aguycalled for OSX system.

src/qt/startoptionsmain.cpp Outdated Show resolved Hide resolved
src/qt/startoptionsmain.cpp Outdated Show resolved Hide resolved
src/qt/startoptionsmain.cpp Outdated Show resolved Hide resolved
@aguycalled
Copy link
Member

aguycalled commented Feb 17, 2020

in OSX 10.14 the list of words covers the last row of cells

image

@KolbyML
Copy link
Contributor Author

KolbyML commented Feb 17, 2020

in OSX 10.14 the list of words covers the last row of cells

image

Can you try making the window larger for context of what will happen

Also I don't own a mac so I can't really troubleshot that

@chasingkirkjufell
Copy link
Contributor

chasingkirkjufell commented Feb 17, 2020

I'm seeing it on Ubuntu too now. The last row will show up if the window is enlarged.
Edit: I think it's most likely due to the added words on top.

@mxaddict
Copy link
Contributor

mxaddict commented Feb 17, 2020 via email

@KolbyML
Copy link
Contributor Author

KolbyML commented Feb 17, 2020

I made a change that I believe will fix the issue found of linux and mac, If someone could test it and take a screenshot for me thanks.

@chasingkirkjufell
Copy link
Contributor

4th row now correctly shown on commit a2fd710.
image

@mxaddict
Copy link
Contributor

mxaddict commented Feb 17, 2020 via email

@navbuilder
Copy link

A new build of cc016af has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@navbuilder
Copy link

A new build of a2fd710 has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@navbuilder
Copy link

A new build of 3bd3c98 has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

@aguycalled
Copy link
Member

Tested MacOS, approved after last changes.

@@ -451,7 +475,7 @@ RES_MOVIES = $(wildcard qt/res/movies/spinner-*.png)
NAVCOIN_RC = qt/res/navcoin-qt-res.rc

NAVCOIN_QT_INCLUDES = -I$(builddir)/qt -I$(srcdir)/qt -I$(srcdir)/qt/forms \
-I$(builddir)/qt/forms -DQT_NO_KEYWORDS
Copy link
Member

Choose a reason for hiding this comment

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

same about removing this.
first reference i found makes me wonder maybe it should not be removed unless there's a reason for it.
https://stackoverflow.com/questions/22188432/qt-macro-keywords-cause-name-collisions

@aguycalled aguycalled merged commit 579f9fa into navcoin:master Feb 17, 2020
@navbuilder
Copy link

A new build of 96a460a has completed succesfully!
Binaries available at https://build.nav.community/binaries/mnemonic-startup-gui

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants