Page MenuHomePhabricator

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

Authored by atul on Jan 10 2023, 4:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 5:57 AM
Unknown Object (File)
Thu, Dec 19, 3:17 AM
Unknown Object (File)
Thu, Dec 19, 3:14 AM
Unknown Object (File)
Thu, Dec 19, 2:49 AM
Unknown Object (File)
Thu, Dec 19, 2:45 AM
Unknown Object (File)
Thu, Dec 19, 2:43 AM
Unknown Object (File)
Thu, Dec 19, 2:18 AM
Unknown Object (File)
Wed, Dec 18, 11:52 PM
Subscribers
None

Details

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
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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
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
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.

rebase before addressing feedback

simplify boolean && arc amend

Hit siwe_nonce endpoint as soon as SIWE button is pressed

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.

This revision is now accepted and ready to land.Jan 12 2023, 3:53 PM

go back to useEffect approach

This revision was landed with ongoing or failed builds.Jan 12 2023, 4:04 PM
This revision was automatically updated to reflect the committed changes.

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?

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

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)

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

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

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

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.

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