feat: move nostr key to accounts #1964
Conversation
|
I left the |
|
🚀 Thanks for the pull request! Here are the current build files for testing: Download and unzip the file for your browser. Refer to the readme for detailed install instructions. This build is brought to you by: Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name) Don't forget: keep earning sats! |
|
A unit-test is failing. Look like I can't delete the nostr key? On master I can delete and after reload the field is still empty. Doesn't seem to work here. On master I get the "use extension option": Here I do not get this?: Maybe I missed something? I reloaded the extension, had two accounts already added (test LND and albydev) |
|
Should work now! |
| @@ -13,8 +13,20 @@ const defaultProps = { | |||
| }; | |||
|
|
|||
| const mockAccounts: Accounts = { | |||
| "1": { id: "1", connector: "lnd", config: "", name: "LND account" }, | |||
| "2": { id: "2", connector: "galoy", config: "", name: "Galoy account" }, | |||
| "1": { | |||
There was a problem hiding this comment.
In another PR these could be moved to, right?:
https://github.com/getAlby/lightning-browser-extension/tree/task-nostr-account/tests/fixtures
Also these should be moved there, not sure why these neded up in src:
https://github.com/getAlby/lightning-browser-extension/tree/task-nostr-account/src/fixtures
Maybe adding the |
|
Just tested it, nice work! 💪 Here is some feedback on the UI: Connector logoCan we maybe use the connector logo here instead of the default avatar? Alby labI think with this feature we can finally move Nostr out of the Lab status. Wdyt? ExportThe export icon isn't very intuitive. Can we at add an export label maybe? Account nameShouldn't the account name be reflected in the account select? |
|
[NEW!] Updated Nostr section
|
src/app/screens/Account.tsx
Outdated
| } | ||
|
|
||
| function generatePublicKey(priv: string) { | ||
| const nostr = new Nostr(priv); |
There was a problem hiding this comment.
Should this be done via a msg.request... instead of importing Nostr from the background script?
There was a problem hiding this comment.
Nostr is just a utility class that is used to generate the public key from the private key. But you raised a good point. Maybe the Nostr class should be moved outside of the background-script folder. (maybe into src/common/utils?)
There was a problem hiding this comment.
it should, we need some good folder structure. (I dislike "utils" because those are mostly bins for everything).
| const account = accounts[id]; | ||
| account.nostrPrivateKey = privateKey | ||
| ? encryptData(privateKey, password) | ||
| : null; |
There was a problem hiding this comment.
I think we should do a special "delete" action to remove the key and not do this by setting the private key to null. I would think that having a special key would prevent the user a bit better from accidentially deleting it.
|
|
Since the avatars PR #1992 is now also merged, can we replace the default avatar on the detail page as well? |
|
After manually saving a pasted private-key I get a blank screen: nostr-manual-save.movCan you reproduce this?
After reloading the page everything is fine and the key is stored. |
Whoops! Did you use a "nsec" string? It expects a hex string as a private key. Not really sure if we should also accept "nsec" directly. (cc @bumi) But yes, will be adding a toast to specify the requirement! |
|
Should this look like this in dark-mode?: |
Describe the changes you have made in this PR
This removes the nostr key in settings (which is being used globally across all accounts) and adds functionality for accounts to have nostr keys instead
Link this PR to an issue [optional]
Fixes #1934
Type of change
(Remove other not matching type)
feat: New feature (non-breaking change which adds functionality)Screenshots of the changes [optional]
Account Page
Settings
Accounts Page
How has this been tested?
Tested manually
Checklist