Page MenuHomePhabricator
Differential D6225

[web] Conditionally display `ConnectButton` once `nonce` received from `keyserver`
ClosedPublic
Actions

Authored by atul on Jan 10 2023, 4:58 PM.
Tags
None
Referenced Files
F3492431: D6225.diff
Wed, Dec 18, 11:52 PM
Unknown Object (File)
Thu, Nov 28, 4:25 AM
Unknown Object (File)
Thu, Nov 28, 4:25 AM
Unknown Object (File)
Thu, Nov 28, 4:25 AM
Unknown Object (File)
Thu, Nov 28, 4:24 AM
Unknown Object (File)
Thu, Nov 28, 4:24 AM
Unknown Object (File)
Thu, Nov 28, 4:24 AM
Unknown Object (File)
Thu, Nov 28, 4:24 AM
View All Files
Subscribers
None

Details

Reviewers
ashoat
tomek
Commits
rCOMMf1e39f51bffd: [web] Conditionally display `ConnectButton` once `nonce` received from…
rCOMM2a0e35670441: [web] Conditionally display `ConnectButton` once `nonce` received from…
Summary

We'll request a nonce from the keyserver siwe_nonce endpoint once a wallet has been successfully connected.

We display an ActivityIndicator while we wait for the request to complete. If the request was successful, we'll display the ConnectButton. If not, the ActivityIndicator component will show an ! to indicate error. Error message + handling will be improved later in the stack.

Depends on D6221

Test Plan

Here's what it looks like:

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D6225_1
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Jan 10 2023, 4:58 PM
atul edited the test plan for this revision. (Show Details)Jan 10 2023, 4:59 PM
atul added inline comments.
web/account/log-in-form.react.js
204–214 ↗(On Diff #20771)

Yes, this isn't very pretty. Going to refactor all of this out into a new component that will be much cleaner.

atul published this revision for review.Jan 10 2023, 5:01 PM
Harbormaster completed remote builds in B15240: Diff 20771.Jan 10 2023, 5:13 PM
ashoat accepted this revision.Jan 10 2023, 7:20 PM
ashoat added inline comments.
web/account/log-in-form.react.js
107 ↗(On Diff #20771)

Why are we waiting on signer before we get the nonce? Why not get the nonce right away?

214 ↗(On Diff #20771)

It would be identical to check isDev && signer here

This revision is now accepted and ready to land.Jan 10 2023, 7:20 PM
atul added inline comments.Jan 12 2023, 12:50 PM
web/account/log-in-form.react.js
107 ↗(On Diff #20771)

For some reason I guess I thought we needed to handle the case where

  1. A user hits connect wallet and connects a wallet
  2. The user hits "disconnect wallet"
  3. User hits connect wallet again and needs a new nonce

This approach lets us handle the "resetting" of the nonce... but on second thought I don't think that's necessary.

I'll modify this so that we make the call to siwe_nonce endpoint as soon as the user hits Connect Wallet

214 ↗(On Diff #20771)

True, I typically prefer to be explicit and avoid simplifying booleans but in this case I think it's still clear what's going on. Will update.

atul updated this revision to Diff 20898.Jan 12 2023, 1:22 PM

rebase before addressing feedback

atul updated this revision to Diff 20900.Jan 12 2023, 1:27 PM

simplify boolean && arc amend

Harbormaster completed remote builds in B15323: Diff 20898.Jan 12 2023, 1:37 PM
Harbormaster completed remote builds in B15325: Diff 20900.Jan 12 2023, 1:41 PM
atul updated this revision to Diff 20901.Jan 12 2023, 1:58 PM

Hit siwe_nonce endpoint as soon as SIWE button is pressed

Harbormaster completed remote builds in B15326: Diff 20901.Jan 12 2023, 2:13 PM
Closed by commit rCOMM2a0e35670441: [web] Conditionally display `ConnectButton` once `nonce` received from… (authored by atul). · Explain WhyJan 12 2023, 2:34 PM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM2a0e35670441: [web] Conditionally display `ConnectButton` once `nonce` received from….
atul added a reverting change: rCOMM334db77d43cc: Revert "[web] Conditionally display `ConnectButton` once `nonce` received from….Jan 12 2023, 3:43 PM
atul added inline comments.Jan 12 2023, 3:52 PM
web/account/log-in-form.react.js
107 ↗(On Diff #20771)

I'll modify this so that we make the call to siwe_nonce endpoint as soon as the user hits Connect Wallet

Ah I remember why I did it like that before.

Because the siwe_nonce endpoint is only hit when the user imperatively hits the "Sign in with Ethereum" button, the nonce will be missing if they refresh the page and their wallet remains connected. The useEffect handled that scenario and would fetch the nonce if necessary.

There may be an alternative approach where we proactively make call to siwe_nonce endpoint when the SIWE button is hit, and then in addition have a useEffect that calls the siwe_nonce endpoint if there's no nonce AND there's no request in progress (which we can check via getSIWENonceLoadingStatusSelector)... but that adds unnecessary complexity IMO.

Why not get the nonce right away?

To clarify when you say "right away" do you mean when LogInForm is first loaded, or when the user has indicated that they're interested in SIWE (by hitting the SIWE button and starting the flow)?

I think we discussed that we did not want to call siwe_nonce until we knew if the user wanted to SIWE, but we may have also been discussing a hypothetical "tab approach" at that point so just want to clarify to make sure we're on the same page.

For now will revert the landed diff and go back to the useEffect approach that handles page reloads correctly.

atul reopened this revision.Jan 12 2023, 3:53 PM
This revision is now accepted and ready to land.Jan 12 2023, 3:53 PM
atul updated this revision to Diff 20911.Jan 12 2023, 4:01 PM

go back to useEffect approach

This revision was landed with ongoing or failed builds.Jan 12 2023, 4:04 PM
Closed by commit rCOMMf1e39f51bffd: [web] Conditionally display `ConnectButton` once `nonce` received from… (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMMf1e39f51bffd: [web] Conditionally display `ConnectButton` once `nonce` received from….
ashoat added a comment.Jan 12 2023, 4:04 PM

Agree we should wait until user has indicated they want to SIWE before we fetch the nonce. That said:

There may be an alternative approach where we proactively make call to siwe_nonce endpoint when the SIWE button is hit, and then in addition have a useEffect that calls the siwe_nonce endpoint if there's no nonce AND there's no request in progress (which we can check via getSIWENonceLoadingStatusSelector)... but that adds unnecessary complexity IMO.

Can't we just fetch the nonce in onSIWEButtonClick? The user still needs to press that button again if they refresh the page, right?

ashoat mentioned this in D6258: [web] Pull `web/SIWE` out from `LogInForm` component.Jan 12 2023, 4:07 PM
atul added a comment.Jan 12 2023, 4:07 PM
In D6225#187552, @ashoat wrote:

Can't we just fetch the nonce in onSIWEButtonClick? The user still needs to press that button again if they refresh the page, right?

Ah the way it's currently setup we go straight to displaying the ConnectButton if the user has already connected a wallet and refreshes the page:

We could force them to hit the SIWE button again, but thought it's a better experience to go straight to showing the connected wallet and "Sign in" button

ashoat added a comment.Jan 12 2023, 4:09 PM

Ah okay, then the best thing would be to have an effect that dispatches the generate nonce request if:

  1. There's no nonce and there's no generateNonce call in progress, AND
  2. EITHER
    • signer is set
    • SIWE button was pressed (React state that is set in onSIWEButtonClick)
atul added a comment.Jan 12 2023, 4:11 PM
In D6225#187560, @ashoat wrote:

Ah okay, then the best thing would be to have an effect that dispatches the generate nonce request if:

  1. There's no nonce and there's no generateNonce call in progress, AND
  2. EITHER
    • signer is set
    • SIWE button was pressed (React state that is set in onSIWEButtonClick)

Gotcha that makes sense, will put up diff with that change

Harbormaster completed remote builds in B15333: Diff 20911.Jan 12 2023, 4:19 PM
ashoat added a comment.Jan 18 2023, 6:42 PM

Gotcha that makes sense, will put up diff with that change

Was this ever done? If not can you link a Linear task please?

atul added a comment.Jan 18 2023, 6:59 PM
In D6225#188791, @ashoat wrote:

Gotcha that makes sense, will put up diff with that change

Was this ever done? If not can you link a Linear task please?

It's part of my local stack, I'll create a Linear task anyways just since it's easier to track stuff there than on old/landed diffs.

atul added a comment.Jan 18 2023, 7:01 PM
In D6225#188803, @atul wrote:

It's part of my local stack, I'll create a Linear task anyways just since it's easier to track stuff there than on old/landed diffs.

https://linear.app/comm/issue/ENG-2741/[web]-proactively-fetch-nonce-once-user-has-indicated-they-intend-to

Revision Contents
Changeset List

PathSize
web/
account/
39 lines

Diff 20911

View Options

web/account/log-in-form.react.js

Loading...