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 export seed phrase #1218

Merged
merged 7 commits into from
Apr 2, 2024
Merged

Add export seed phrase #1218

merged 7 commits into from
Apr 2, 2024

Conversation

juliangruber
Copy link
Member

@juliangruber juliangruber commented Jan 2, 2024

This feature is required as on some Windows 10+ installations users aren't able to export the secret as outlined in https://docs.filstation.app/your-station-wallet#114f79892571427994c571c39505f7f7 (Windows makes this unavailable)

Screenshot 2024-01-02 at 13 17 45

When looking at the diff, best focus on the 2nd commit. The 1st is a cleanup that I noticed while working on this feature.

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 vaguely remember that we may have different menus on different OSes. Consider testing this change on a Windows machine before merging.

@juliangruber
Copy link
Member Author

I vaguely remember that we may have different menus on different OSes. Consider testing this change on a Windows machine before merging.

You're right, we don't have an application menu on Windows 😱 I'm concerned moving this to the tray, as that's a frequently used place, and the new functionality is too powerful. I think if we move it to the tray, we need to adda dialog "Are you sure...?"

Wdyt?

@bajtos
Copy link
Member

bajtos commented Jan 8, 2024

I vaguely remember that we may have different menus on different OSes. Consider testing this change on a Windows machine before merging.

You're right, we don't have an application menu on Windows 😱 I'm concerned moving this to the tray, as that's a frequently used place, and the new functionality is too powerful. I think if we move it to the tray, we need to adda dialog "Are you sure...?"

Wdyt?

I agree this is a powerful feature.

When I saw this PR, my first reaction was: should we show a dialogue clearly explaining the consequences/ramifications to non-technical users? It looks like we have a similar intuition.

Have you considered adding this action as a new button to the in-app Wallet screen? To me, that's the most logical place where to look for this action. Unfortunately, it also requires a bit more thinking about the UX & UI design. I understand if you don't have an appetite for that, so feel free to continue in the current direction of adding a new menu item.

@juliangruber
Copy link
Member Author

Moved to tray:

Screenshot 2024-04-02 at 10 24 23 Screenshot 2024-04-02 at 10 24 31

@juliangruber juliangruber merged commit 923a47e into main Apr 2, 2024
6 of 9 checks passed
@juliangruber juliangruber deleted the add/export-seed-phrase branch April 2, 2024 12:09
@Destore2023
Copy link

How about show seed phrase instead of copy it in the clipboard for the security season? Some Trojan can read your clipboard.

I do remember @patrickwoodhead said Station will bring a lot of people join web3. So security will be the top priority. Not your key, not your money.

@bajtos
Copy link
Member

bajtos commented Apr 3, 2024

Some Trojan can read your clipboard.

That's a valid concern 👍

The app shows a warning before it puts the seed phrase on the clipboard. Perhaps we can reword the warning so that people think twice before pressing "Copy to Clipboard"?

How about show seed phrase instead of copy it in the clipboard for the security season?

Interesting. How do you envision the next steps in such workflow? After the app shows the seed phrase on the screen, then the user does ... what?

Do you have any concerns about showing the seed phrase on a screen? Once visible on the screen, the seed phrase can be read by people standing nearby, recorded by CCTV cameras that are in many buildings nowadays, and so on.

Personally, I would be more concerned about somebody looking at my screen than malware reading my clipboard.

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.

4 participants