Implement Sign & Verify #154

jamaljsr · 2019-01-21T15:36:34Z

Closes #146

Description

Implement Sign and Verify prompts.

Note: I'm opening this PR for early feedback. The UI can be improved and the code cleaned up a bit more.

Steps to Test

Sign

Open the browser devtools console
Enter webln.enable() and click the Confirm button on the popup
Enter webln.signMessage('hello').then(res => console.log(res))
In the Joule popup, verify the message hello is displayed
Click the Confirm button
Verify that the generated signature is displayed in the console log

Verify

Open the browser devtools console
Enter webln.enable() and click the Confirm button on the popup
Enter webln.verifyMessage('<signature>', 'hello').then(res => console.log(res)) replacing <signature> with the output from the signMessage call above
In the Joule popup, verify the message and signature are displayed correctly
Click the Verify button
Confirm that you receive a success message

Screenshots

jamaljsr · 2019-01-27T05:29:24Z

I mocked up an alternative UI for these prompts. Here are the old and new, side by side.

Personally, I like the new ones better but I'm not married to them. I'm pretty open to feedback/criticism. Just throwing this out there as potential improvement.

wbobeirne

This works fantastic, just how I imagined it. Thanks for bringing it to life man. Your revised designs definitely get closer to what I had in mind, but let me just drop some feedback here that I had written up but hadn't posted before you made those.

Reusing the site icon <-> joule layout for signature was a good move, since it is the site asking for your signature and not a user. However, for verify, you're going to be verifying a node's signature, not necessarily a site. For instance, if there were a lightning-powered Twitter that users signed all of their tweets, you'd want it to present the signature as the user's node's signature, not the site's. So I think for verify, we should show the user's node instead.

When signing, the most important thing is to emphasize what the user is going to sign. Likewise, there will likely be a lot of content in there, potentially JSON. This should probably be much larger, the main part of the popup. I really like the reassuring text though, so keeping that in a small way would be great.

As noted in the comments for verification, this isn't really a step the site needs to be involved in beyond opening the prompt. In that case, we should immediately verify the message, and make it loud and clear whether it's valid or not.

Your new designs address 90% of what I was getting at, awesome work. If you want to go ahead and push the latest designs (I assume you did them in code) I'd be happy to riff on them to give feedback, but if you want to feel it out yourself, by all means! Let me know if any of my feedback wasn't clear.

wbobeirne · 2019-01-26T18:01:57Z

src/app/lib/lnd-http/index.ts

+  };
+
+  verifyMessage = (params: T.VerifyMessageParams) => {
+    return this.request<T.VerifyMessageResponse, T.VerifyMessageParams>(


Verify doesn't actually need a response. The purpose of it just for the user's assurance, the website should probably be doing independent verification.

This code is actually just adding the type for the LND REST API response. The WebLN verifyMessage function in provider.ts still returns void.

wbobeirne · 2019-01-26T18:15:56Z

src/app/prompts/verify.tsx

+              <div className="VerifyPrompt-graphic-icon">
+                <img src={Logo} />
+              </div>
+            </div>


Given that signature verifications are more often than not going to be about user's content signed by their nodes, I think there should be less emphasis on the site that is doing the verification. We should present the verified signature node's icon, alias, pubkey etc. instead.

wbobeirne · 2019-01-26T18:16:24Z

src/app/prompts/verify.tsx

+            </div>
+            <h2 className="VerifyPrompt-title">
+              <strong>{this.origin.name}</strong>
+              requests your verification


This is purely for the user's benefit, so I don't think we should pose this as the site asking for verification.

Updated the message in the UI to just say "Signature Verification"

wbobeirne · 2019-01-26T18:16:51Z

src/app/prompts/verify.tsx

+
+  private handleConfirm = () => {
+    this.setState({ isVerifying: true }, () => {
+      this.props.verifyMessage(this.signature, this.msg);


Verification should happen immediately without the user needing to consent, and we should display the results right off the bat.

wbobeirne · 2019-01-26T18:20:24Z

src/app/prompts/sign.less

+      font-size: 1.4rem;
+      opacity: 0.4;
+    }
+  }


Might be worth making a common component for this, since it's been getting used a lot (Perhaps as a suite of PromptTemplate common components?)

I refactored this into a separate component. I also reused the node icon/pubkey/alias from the payment prompt, so I refactored that into a shared component as well.

jamaljsr · 2019-01-28T13:57:00Z

@wbobeirne This is great feedback. I really appreciate you taking the time to review. All of your requests are clear to me. I will use the new UI as a base and make the changes you suggested. I don't think it'll take too long, so I should have it done in the next day or two.

On a side note, regarding the signing data structure. I agree that we should only accept JSON as the message argument to verifyMessage. Should I include this functionality in this PR or should we keep it separate for now. My concern is releasing this as is then having to worry about backward compatibility when we move to JSON.

jamaljsr · 2019-01-29T07:13:40Z

I thought about the feedback you gave and took the verification screen a bit further, to make the results stand out a lot more. Check the screenshots below and let me know if I went too far. 😄

wbobeirne · 2019-01-30T02:07:41Z

Hmm, I actually think in the case of a verified signature, it's important to show the message without any additional clicks needed. If someone were trying to be deceptive, they might change out the message contents, and unobservant users might just see "Verified" and assume it's what they saw on the site. This is definitely a major edge-case, but I think verification from Joule should be reasonably trustable at a glance, and not require a deeper dive.

I think you could still fit in the two tabs and the textareas if you just made verified / invalid message one line with the icon to the left.

jamaljsr · 2019-01-30T12:45:03Z

Ah, you’re right. Good catch.

I’ll make the change later on today.

jamaljsr · 2019-01-30T14:36:24Z

I updated the Verify UI based on your feedback. Let me know if there are still any issues.

wbobeirne · 2019-01-30T17:26:58Z

That looks absolutely perfect 👍 I'll take another scan at the changes and do a testrun later today, but this looks like it's ready to go.

jamaljsr and others added 10 commits January 16, 2019 19:39

Display prompt when webln.signmessage is called

19e439e

Merge branch 'develop' into feat/sign-verify

9d2e5fb

Add ability to sign raw messages

fba626c

Add ability to verify message & signature

92916f2

Merge branch 'develop' into feat/sign-verify

4a772a3

Imrpove the Verify prompt UI

9cd2b60

Update verify prompt UI in error case

fa88729

Add loader to Verify button

c1cba7b

Rename signedMessage to signature in SignMessageResponse

0c6a9e6

Merge branch 'develop' into feat/sign-verify

7afb4b3

wbobeirne requested changes Jan 28, 2019

View reviewed changes

jamaljsr added 4 commits January 28, 2019 15:14

Update UI for Sign Message prompt

b426889

Update UI for Verify Message prompt

785dee5

Refactor origin swap graphic into a component

8a3c161

Refactor node icon/pubkey/alias into a shared component

8e2f385

jamaljsr added 2 commits January 30, 2019 09:29

Move msg/sig back to main screen

40e1b7b

Fix typo in success message

659565a

wbobeirne approved these changes Feb 1, 2019

View reviewed changes

wbobeirne merged commit 0ff7eba into joule-labs:develop Feb 1, 2019

jamaljsr deleted the feat/sign-verify branch February 1, 2019 14:52

wbobeirne mentioned this pull request Feb 23, 2019

Sign/Verify joule-labs/webln#5

Closed

Implement Sign & Verify #154

Implement Sign & Verify #154

Uh oh!

Conversation

jamaljsr commented Jan 21, 2019

Description

Steps to Test

Screenshots

Uh oh!

jamaljsr commented Jan 27, 2019

Uh oh!

wbobeirne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamaljsr commented Jan 28, 2019

Uh oh!

jamaljsr commented Jan 29, 2019

Uh oh!

wbobeirne commented Jan 30, 2019

Uh oh!

jamaljsr commented Jan 30, 2019

Uh oh!

jamaljsr commented Jan 30, 2019

Uh oh!

wbobeirne commented Jan 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants