nostr
/
nostrweb
3
1
Fork 1
Code Issues 24 Pull Requests 3 Projects 1 Releases 27 Activity

Labels Milestones

settings: pasting private key should update pubkey #24

Merged
offbyn merged 1 commits from settings-calc-pubkey into master 2 years ago
Conversation 1 Commits 1 Files Changed 2
offbyn commented 2 years ago
Owner

If the user pastes a private-key and the pubkey input is empty,
it should be ok to generate and autofill the pubkey field without
confusing the user. There might be circumstances where it is
preferred to see an error if the pubkey does not correspond with
the private key.

part of #14

If the user pastes a private-key and the pubkey input is empty, it should be ok to generate and autofill the pubkey field without confusing the user. There might be circumstances where it is preferred to see an error if the pubkey does not correspond with the private key. part of https://git.qcode.ch/nostr/nostrweb/issues/14#issuecomment-178
offbyn added 1 commit 2 years ago
ci/woodpecker/push/woodpecker Pipeline was successful Details
ci/woodpecker/pr/woodpecker Pipeline was successful Details
123b3655e2
settings: pasting private key should update pubkey 
If the user pastes a private-key and the pubkey input is empty,
it should be ok to generate and autofill the pubkey field without
confusing the user. There might be circumstances where it is
preferred to see an error if the pubkey does not correspond with
the private key.
offbyn requested review from x1ddos 2 years ago
x1ddos approved these changes 2 years ago
x1ddos left a comment
Owner

lgtm

lgtm
src/main.js Outdated
@ -607,1 +607,4 @@
privateKeyInput.addEventListener('paste', (event) => {
if (!pubKeyInput.value && event.clipboardData) {
pubKeyInput.value = getPublicKey(event.clipboardData.getData('text'));
x1ddos commented 2 years ago
Owner

this throws an error "Expected 32 bytes of private key" in js console if i paste something other than 32 hex-encoded string. maybe wrap it in try/catch?

this throws an error "Expected 32 bytes of private key" in js console if i paste something other than 32 hex-encoded string. maybe wrap it in try/catch?
👍 1
offbyn commented 2 years ago
Poster
Owner

Good catch, wrapped into try {} catch(err) {} and ignore the error, as on input it triggers a validation and shows a proper error already.

Also moved the function next to the on input event listener.

Good catch, wrapped into `try {} catch(err) {}` and ignore the error, as on input it triggers a validation and shows a proper error already. Also moved the function next to the on input event listener.
offbyn commented 2 years ago
Poster
Owner

There was another issue when pasting a private key.

Added another check that the clipboard data should only be taken if the private key field is either empty or the whole field is selected and overwritten by the pasted data.

There was another issue when pasting a private key. Added another check that the clipboard data should only be taken if the private key field is either empty or the whole field is selected and overwritten by the pasted data.
offbyn force-pushed settings-calc-pubkey from 123b3655e2 to 8ab09fda40 2 years ago
offbyn force-pushed settings-calc-pubkey from 8ab09fda40 to 6b251c9d12 2 years ago
x1ddos reviewed 2 years ago
src/main.js Outdated
@ -582,0 +585,4 @@
}
if (privateKeyInput.value === '' || (
privateKeyInput.selectionStart === 0
&& privateKeyInput.selectionEnd === privateKeyInput.value.length
x1ddos commented 2 years ago
Owner

but does it really matter here? if there's a paste event and it's somewhere in the middle, so the resulting input.value is invalid, more than 32 hex, you'll catch it in try/catch anyway.

but does it really matter here? if there's a paste event and it's somewhere in the middle, so the resulting input.value is invalid, more than 32 hex, you'll catch it in try/catch anyway.
x1ddos commented 2 years ago
Owner

what i mean is:

try {
  pubKeyInput.value = getPublicKey('random-invalid-string');
} catch(err) {}

will not assign to pubKeyInput.value anyway - it'll go into the catch branch due to getPublicKey throwing an error.

so, the complicated if with selection start/end seems superfluous.

what i mean is: ```js try { pubKeyInput.value = getPublicKey('random-invalid-string'); } catch(err) {} ``` will not assign to `pubKeyInput.value` anyway - it'll go into the `catch` branch due to `getPublicKey` throwing an error. so, the complicated `if` with selection start/end seems superfluous.
offbyn commented 2 years ago
Poster
Owner

the issue is on paste event fires before the privateKeyInput field is updated.

if it checks that either privateKeyInput.value is empty or the whole private key is selected then we can assume that only the clipboard content is relevant and set the pubkey.

the issue is on paste event fires before the privateKeyInput field is updated. if it checks that either privateKeyInput.value is empty or the whole private key is selected then we can assume that only the clipboard content is relevant and set the pubkey.
offbyn force-pushed settings-calc-pubkey from 6b251c9d12 to 71945b105f 2 years ago
offbyn commented 2 years ago
Poster
Owner

rebased

rebased
offbyn merged commit 71945b105f into master 2 years ago
offbyn deleted branch settings-calc-pubkey 2 years ago
x1ddos reviewed 2 years ago
src/main.js
@ -607,0 +608,4 @@
if (pubKeyInput.value || !event.clipboardData) {
return;
}
if (privateKeyInput.value === '' || (
x1ddos commented 2 years ago
Owner

maybe add a comment here, to clarify? something like:

make sure paste event overwrites the whole privateKeyInput.value.
otherwise, the computed pubkey may be computed from only a partial
privateKeyInput.value.

i was staring at this again for a minute, trying to remember why this is needed.

maybe add a comment here, to clarify? something like: > make sure paste event overwrites the whole privateKeyInput.value. > otherwise, the computed pubkey may be computed from only a partial > privateKeyInput.value. i was staring at this again for a minute, trying to remember why this is needed.
offbyn commented 2 years ago
Poster
Owner

done 0047c0bdc0

done https://git.qcode.ch/nostr/nostrweb/commit/0047c0bdc0d33f8101aaf2138c7b41d655bb8343

Reviewers

x1ddos approved these changes 2 years ago
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
ci/woodpecker/push/woodpecker Pipeline was successful
Details
The pull request has been merged as 71945b105f.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b settings-calc-pubkey master
git pull origin settings-calc-pubkey

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff settings-calc-pubkey
git push origin master
Sign in to join this conversation.
Reviewers
Request review
No reviewers
x1ddos
Labels
Apply labels
Clear labels
  
bug
   
duplicate
   
feature
   
halp!
   
invalid
   
wontfix
No Label
bug
duplicate
feature
halp!
invalid
wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Set Project
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: nostr/nostrweb#24
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'settings-calc-pubkey'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes