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

Display warning when generating brain wallets: #2121

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
3 participants
@nbougalis
Member

nbougalis commented May 18, 2017

A brain wallet is a standard wallet that is generated not from a
random seed but by hashing a user-supplied passphrase. Typically,
human-selected passphrases can contain insufficient entropy.

When generating a wallet from a passphrase, we include a warning
to this effect. The warning would be incorrectly displayed even
if the wallet was being generated from a seed.

Display warning when generating brain wallets:
A brain wallet is a standard wallet that is generated not from a
random seed but by hashing a user-supplied passphrase. Typically,
human-selected passphrases can contain insufficient entropy.

When generating a wallet from a passphrase, we include a warning
to this effect. The warning would be incorrectly displayed even
if the wallet was being generated from a seed.

@nbougalis nbougalis requested review from mellery451 and ximinez May 18, 2017

@mellery451

LGTM

@ximinez

ximinez requested changes Jun 5, 2017 edited

WalletPropose_test exercises this function, but nothing checks the result of jss::warning. It would be nice if we could do that, including a case with a high-entropy passphrase.

Edit: I don't know if that's enough to hold up this relatively simple change for, though.

@ximinez

This comment has been minimized.

Show comment
Hide comment
@ximinez

ximinez Jun 5, 2017

Contributor

Add check for the warning to tests: ximinez@2e2fa26
(CI pending)

Contributor

ximinez commented Jun 5, 2017

Add check for the warning to tests: ximinez@2e2fa26
(CI pending)

@nbougalis

This comment has been minimized.

Show comment
Hide comment
@nbougalis

nbougalis Jun 6, 2017

Member

Will accept @ximinez's suggestion.

Member

nbougalis commented Jun 6, 2017

Will accept @ximinez's suggestion.

@nbougalis

This comment has been minimized.

Show comment
Hide comment
@nbougalis

nbougalis Jun 9, 2017

Member

Merged (with Ed's recommended fixed) as fa72795.

Member

nbougalis commented Jun 9, 2017

Merged (with Ed's recommended fixed) as fa72795.

@nbougalis nbougalis closed this Jun 9, 2017

@nbougalis nbougalis deleted the nbougalis:wpw branch Jun 9, 2017

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