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

Add import seed phrase. Closes #1727 #1742

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

juliangruber
Copy link
Member

Closes #1727

Screenshot 2024-07-31 at 14 07 29

Copy link
Member

@bajtos bajtos 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 did not test the new functionality, though.

I think a UI with a password input that does not show the secret phrase would provide a better UX than asking the user to copy the seed phrase to the clipboard. It's a small improvement that may not be worth the effort required to implement it.

const button = showDialogSync({
title: 'Import Seed Phrase',
// eslint-disable-next-line max-len
message: 'The seed phrase is used in order to back up your wallet, or move it to a different machine. Please copy it to your clipboard before proceeding. Please be cautious, as this will overwrite the seed phrase currently used, which will be permanently lost.',
Copy link
Member

Choose a reason for hiding this comment

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

I'd like @patrickwoodhead to review this message.

Do we need to explain what is the seed phrase? I would expect the user to understand that if they decide to import the seed phrase,

I think it's also best to put the call to action as the last sentence in the message.

Should we also tell the user that they may want to export the current seed phrase?

Suggested change
message: 'The seed phrase is used in order to back up your wallet, or move it to a different machine. Please copy it to your clipboard before proceeding. Please be cautious, as this will overwrite the seed phrase currently used, which will be permanently lost.',
message: 'Please be cautious. This action will overwrite the seed phrase currently used, which will be permanently lost. Please copy the seed phrase to import to your clipboard before proceeding.',

Copy link
Member Author

Choose a reason for hiding this comment

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

For reference, we're also not explaining what a seed phrase is in the export section.

Copy link
Member Author

Choose a reason for hiding this comment

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

In your suggestion it says Please copy the seed phrase to import, but I think it needs to be Please copye the seed phrase to your clipboard, to import

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 to moving the CTA to the end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I agree that we need to ask the user to be cautious as the first thing. Also, I'm a bit unclear what the CTA is asking the user to do. Is it asking them to copy their current seed phrase out, or copy their new one into the clipboard so that they can paste it into the input box?

Copy link
Member Author

Choose a reason for hiding this comment

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

The user needs to have their current seed phrase copied in their clipboard, from which we will then import it

@juliangruber
Copy link
Member Author

LGTM. I did not test the new functionality, though.

I think a UI with a password input that does not show the secret phrase would provide a better UX than asking the user to copy the seed phrase to the clipboard. It's a small improvement that may not be worth the effort required to implement it.

An input field will require styling, which we don't yet have for this element in this location. It will also require a form, a button, with states, etc, which we don't have here.

I agree that that would be nicer though.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Hang on, this isn't going to work. When we change the wallet private key, we will derive a different passphrase for Station Core and it won't be able to decrypt the file storing the Station ID.

See #1726

@juliangruber juliangruber marked this pull request as draft July 31, 2024 14:29
@juliangruber
Copy link
Member Author

juliangruber commented Jul 31, 2024

  • validate seed phrase
  • re-encode station id

@bajtos
Copy link
Member

bajtos commented Aug 1, 2024

re-encode station id

See #1726 (comment)

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

Successfully merging this pull request may close these issues.

Add UI to restore the wallet from a seed phrase
3 participants