Skip to content

[GUI][SETUP] Added a password step to setup wizard#700

Merged
aguycalled merged 3 commits intonavcoin:masterfrom
mxaddict:add-password-option-to-wizard
Jun 7, 2020
Merged

[GUI][SETUP] Added a password step to setup wizard#700
aguycalled merged 3 commits intonavcoin:masterfrom
mxaddict:add-password-option-to-wizard

Conversation

@mxaddict
Copy link
Copy Markdown
Contributor

@mxaddict mxaddict commented Jun 5, 2020

Capture2

@mxaddict
Copy link
Copy Markdown
Contributor Author

mxaddict commented Jun 5, 2020

related to #697

@mxaddict
Copy link
Copy Markdown
Contributor Author

mxaddict commented Jun 5, 2020

NOTES: Password step should come always at the last step before loading wallet

@navbuilder
Copy link
Copy Markdown

A new build of 9e665a8 has completed succesfully!
Binaries available at https://build.nav.community/binaries/add-password-option-to-wizard

@chasingkirkjufell
Copy link
Copy Markdown
Contributor

The button is "skip/next" when creating a new wallet but only "next" when importing with mnemonics.
image

@Xa5r
Copy link
Copy Markdown

Xa5r commented Jun 5, 2020

Capture
I would change it to "Yes" instead of "Ok"

@Xa5r
Copy link
Copy Markdown

Xa5r commented Jun 5, 2020

The button is "skip/next" when creating a new wallet but only "next" when importing with mnemonics.
image

I think on the step on the picture, there dont need to stay skip/next, because without password is also just going to the next step

@Xa5r
Copy link
Copy Markdown

Xa5r commented Jun 5, 2020

Not sure if this is a bug here, but if you dont set a password and encrypt the wallet later, the mnemonic changes

@mxaddict
Copy link
Copy Markdown
Contributor Author

mxaddict commented Jun 5, 2020

Not sure if this is a bug here, but if you dont set a password and encrypt the wallet later, the mnemonic changes

This won't happen once the fix for that bug is also merged into master

@mxaddict
Copy link
Copy Markdown
Contributor Author

mxaddict commented Jun 5, 2020

The button is "skip/next" when creating a new wallet but only "next" when importing with mnemonics.
image

I think on the step on the picture, there dont need to stay skip/next, because without password is also just going to the next step

Ok, so just "next"?

@Xa5r
Copy link
Copy Markdown

Xa5r commented Jun 5, 2020

Yes
If you don't want set a password, for you it's just the next step.
If you already wrote a password, for you it's not a skip (even you doesn't want a password).
Meaning the state will be used anyway with or without password

@mxaddict
Copy link
Copy Markdown
Contributor Author

mxaddict commented Jun 5, 2020

Yes
If you don't want set a password, for you it's just the next step.
If you already wrote a password, for you it's not a skip in (even he doesn't want a password).
Meaning the state will be used anyway with or without password

Makes sense 👍

@mxaddict
Copy link
Copy Markdown
Contributor Author

mxaddict commented Jun 5, 2020

I've pushed the suggested changes 👍

@navbuilder
Copy link
Copy Markdown

A new build of 1cd0efd has completed succesfully!
Binaries available at https://build.nav.community/binaries/add-password-option-to-wizard

Copy link
Copy Markdown
Contributor

@chasingkirkjufell chasingkirkjufell left a comment

Choose a reason for hiding this comment

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

Tested gitian build on Windows 10.

Copy link
Copy Markdown
Member

@aguycalled aguycalled left a comment

Choose a reason for hiding this comment

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

tested osx, reviewed code. everything ok except of the strings when no password is entered

@aguycalled aguycalled merged commit b1444f2 into navcoin:master Jun 7, 2020
Comment on lines +4076 to +4084
// Check if we gave a password
if (!password.empty()) {
// Is this safe? or do I need to do something more
// to the string after I've converted it to SecureString
SecureString strWalletPass;
strWalletPass.reserve(100);
strWalletPass = password.c_str();
walletInstance->EncryptWallet(strWalletPass);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aguycalled About this part, is this safe? will it not lead the password in memory?

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.

5 participants