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

Handle "Error: unreachable" message when importing short private key #720

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

stanisloe
Copy link

If user attempts to import private key of length less than 64 symbols namada extension continues with the import process but in the end throws Failed while "Encrypting and storing private key.". Error: unreachable to the user. While it is unlikelly someone will be importing private key of value 123 it is possible to trim few characters from your private key when you copy it and receive error which is not very informative. ( Which happened to me and thought there is something wrong with extension )

Changes:
Validation for private key minimum length and error message in case wrong length was provided.
Validation before submitting private key to extension background.

Tests:
Built and imported extension to my browser and tested that it is not possible to proceed with short key and meaningfull validation error message is presented to the user.
Also checked the normal flow with proper key.

Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

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

LGTM! Left a comment about function name, also make sure your branch is up to date with main(I did not check that) :) Other than that works nicely!

@@ -47,27 +47,31 @@ export const SeedPhraseImport: React.FC<Props> = ({ onConfirm }) => {
Array.from(mnemonicsRange)
);

const privateKeyError = (() => {
const validation = validatePrivateKey(filterPrivateKeyPrefix(privateKey));
const validatePkAndFormatErrorMessage = (key: string): string => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine to call it validatePk. We can also return {valid: boolean, msg: string} or something, as it seems a bit more explicit :)

Copy link
Author

Choose a reason for hiding this comment

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

sure. updated function name and return type 👍

@mateuszjasiuk mateuszjasiuk added bug Something isn't working App: Extension labels Apr 9, 2024
@stanisloe
Copy link
Author

Thank for the feedback ! Pushed new commit with updates. Currently the branch is up to date with main.

Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

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

LGTM! I will add another reviewer though :)

Copy link
Collaborator

@jurevans jurevans left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App: Extension bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants