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

feat(nostr): support new key format #1927

Merged
merged 3 commits into from
Jan 3, 2023
Merged

Conversation

fczuardi
Copy link
Contributor

Describe the changes you have made in this PR

This is my attempt to address #1920 the field for Nostr private keys on the settings page of the browser extension expects that the user paste a key in "hex" format. However clients like astral.ninja have adopted a new format that uses bech32 with a nsec1 prefix (see NIP-19).

This patch compares the beginning of the provided Nostr private key with the known prefixes in the NIP to check if a conversion is needed, and if so, transforms the input from bech32 to hex.

Link this PR to an issue

Fixes #1920

Type of change

  • feat: New feature (non-breaking change which adds functionality)

How has this been tested?

I havent tested it extensively, I have compared the expected output using console log (refreshing the settings also works). And I have tried my personal nsec key with astral.ninja, it did seem to have signed and published new events.

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

@github-actions
Copy link

github-actions bot commented Jan 2, 2023

🚀 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: Moritz (who recently dropped 1000 sats):

Merry Christmas to the Alby team

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!

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

@fczuardi Thanks for your PR! Just tested it and worked fine. 👌

nit: Should we maybe add a hint that private keys are always stored in HEX format (users could get confused why their private keys do not match when they revisit the settings page)

tACK

@fczuardi
Copy link
Contributor Author

fczuardi commented Jan 3, 2023

It also bothers me a little that the settings page will display a different key when the user returns or refreshes the page. Maybe we should always display the bech32 version to the user, as the purpose of the "nsec" prefix is to prevent accidents like people pasting the private key on places expecting the public one, and in hex format this distinction is visually difficult.

@bumi
Copy link
Collaborator

bumi commented Jan 3, 2023

with more interest in nostr maybe we can move this nostr things out of the settings and labs section and give it's own section (and support keys per account)

@reneaaron
Copy link
Contributor

I agree, however that would give us the same problem with the value changing (but the other way around).

Another idea would be to render the public key (npub...) below? I feel like most users will recognize their public key which would give them some indication that they've imported their key successfully.


const nostr = {
normalizeHex(str: string) {
const NIP19Prefixes = ["npub", "nsec", "note"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

are those always lowercase?

Choose a reason for hiding this comment

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

this so ~

Copy link
Collaborator

Choose a reason for hiding this comment

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

?

@@ -0,0 +1,19 @@
import { bech32Decode } from "../utils/helpers";
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is a nostr/helpers.ts file - does this one also fit there?

* master: (38 commits)
  Update translation files
  Update translation files
  Update translation files
  Update translation files
  fix(nostr): re-encrypt nostr key on pw change getAlby#1925 (getAlby#1930)
  test(e2e): fix alby wallet tests getAlby#1848
  test(e2e): fix alby wallet tests getAlby#1848
  Update translation files
  Translated using Weblate (German)
  fix: copy
  chore: add forgot password link
  fix: copy
  fix: copy
  chore: remove Settings link from navbar
  Translated using Weblate (German)
  Update all development Yarn dependencies (2022-12-31)
  Update react-router-dom to version 6.6.1
  fix: unit test query casing
  chore: update lightning and bitcoin casing
  fix: rename Discover component
  ...
@bumi bumi merged commit 8fad531 into getAlby:master Jan 3, 2023
This pull request was closed.
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