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

Validate empty key #4513

Merged
merged 9 commits into from
Jun 14, 2018
Merged

Validate empty key #4513

merged 9 commits into from
Jun 14, 2018

Conversation

danfinlay
Copy link
Contributor

@danfinlay danfinlay commented Jun 5, 2018

Previously importing an empty string would result in a new empty Keyring
object to be constructed, with no notification to the user.

Now we render a clear error explaining the mistake.

This would not result in any bad accounts, just a memory leak.

Previously importing an empty string would result in a new empty Keyring
object to be constructed, with no notification to the user.

Now we render a clear error explaining the mistake.
@metamaskbot
Copy link
Collaborator

Builds ready [3e0747f]: mascara, chrome, firefox, edge, opera

@kumavis
Copy link
Member

kumavis commented Jun 6, 2018

I think we need strong validation, like making sure the private key is actually valid
I think a privateKey 'banana' would still generate an empty keyring

However, they no longer seem to work. I'm unclear why this test is
failing. The private key being provided should be valid.
@danfinlay
Copy link
Contributor Author

Added a commit that includes much more strong private key importing validations, but for some reason they fail. Will have to revisit at some point. Should be easy for someone else to pick up, if they wanted to finish this out.

@kumavis
Copy link
Member

kumavis commented Jun 14, 2018

  1. assert.throws only works for synchronous methods. assert.rejects is added is node v10 for awaiting promises
  2. in async functions, you need to await the method to catch the error in a try-catch
  3. ethereumjs-util takes non-hex-prefixed strings and buffer encodes them as utf8 (!)

@metamaskbot
Copy link
Collaborator

Builds ready [691ac5d]: mascara, chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [e9cb650]: mascara, chrome, firefox, edge, opera

@danfinlay
Copy link
Contributor Author

I would approve this, but I can't b/c I opened the PR. Thanks for clarifying the assert.fails/async issue.

@kumavis
Copy link
Member

kumavis commented Jun 14, 2018

BY THE POWER VESTED IN ME, I DECLARE YOU APPROVED. YOU MAY NOW MERGE WITH THE BASE.

@kumavis kumavis merged commit 1f83a11 into develop Jun 14, 2018
@kumavis kumavis deleted the ValidateEmptyKey branch June 14, 2018 05:01
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