Page MenuHomePhabricator
Differential D5023

[native] added webview to login with working SIWE (registration only)
ClosedPublic
Actions

Authored by • derek on Sep 1 2022, 11:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 17, 1:12 PM
Unknown Object (File)
Fri, May 17, 7:20 AM
Unknown Object (File)
Thu, May 16, 8:44 AM
Unknown Object (File)
Wed, May 15, 8:04 PM
Unknown Object (File)
Sun, May 12, 3:39 PM
Unknown Object (File)
Sun, May 12, 12:55 AM
Unknown Object (File)
Fri, May 10, 1:45 AM
Unknown Object (File)
Wed, May 8, 12:30 AM
View All Files
Subscribers
• abosh
ashoat
atul
• jacek
tomek
varun

Details

Reviewers
ashoat
• abosh
varun
atul
Commits
rCOMM9f2b86dcbcef: [native] added webview to login with working SIWE (registration only)
Summary

linear issue
THIS IS A PoC
added a third button to login with a SIWE prompt that opens landing, signs a message, and registers with username: address, password: signature
obviously that is not the final form of registration/login

originally, i had added another react navigation screen to the account modals. from a UX perspective, i like the idea of matching the other sign in experiences instead of presenting a full modal screen. it's definitely ugly tho.
i think the endgame is a bottomsheet modal that pulls up to about ~half the screen, that way we can re-use it anytime we need to sign messages.

Test Plan

this only works in the sim as of right now. run /landing and make sure you have a wallet on a physical device. when rainbowkit prompts you to connect, tap walletconnect & then "scan QR" in the top right. depends on D4997

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• derek created this revision.Sep 1 2022, 11:38 AM
Herald added a subscriber: tomek. · View Herald TranscriptSep 1 2022, 11:38 AM
• derek edited the summary of this revision. (Show Details)Sep 1 2022, 11:39 AM
• derek edited the summary of this revision. (Show Details)
• derek edited the summary of this revision. (Show Details)
• derek edited the summary of this revision. (Show Details)
• derek edited the summary of this revision. (Show Details)Sep 1 2022, 11:41 AM
• derek edited the summary of this revision. (Show Details)

please add relevant reviewers/subscribers, first time making native changes i'll add those reviewers/subs from now on :)

Harbormaster completed remote builds in B11811: Diff 16215.Sep 1 2022, 11:48 AM
• derek requested review of this revision.Sep 1 2022, 11:48 AM
• derek edited the summary of this revision. (Show Details)Sep 1 2022, 11:50 AM
• derek edited the summary of this revision. (Show Details)
• derek edited the summary of this revision. (Show Details)
atul requested changes to this revision.Sep 6 2022, 12:54 AM

THIS IS A MOCKUP, WIP

Do you plan on making further changes, or is this ready for review? Feel free to re-request review if it's good to go!

This revision now requires changes to proceed.Sep 6 2022, 12:54 AM
• derek requested review of this revision.Sep 6 2022, 7:07 AM

@atul sorry that was unclear - i meant WIP in the context of SIWE. as a diff, this is ready for review

• derek edited the summary of this revision. (Show Details)Sep 6 2022, 7:08 AM
• derek updated this revision to Diff 16387.Sep 6 2022, 12:04 PM

fixes for physical devices, see https://linear.app/comm/issue/ENG-1747/cant-load-landing-on-physical-device

Harbormaster completed remote builds in B11936: Diff 16387.Sep 6 2022, 12:14 PM
atul resigned from this revision.Sep 7 2022, 9:33 PM
atul added a reviewer: varun.
atul added a subscriber: varun.

Left a quick note about memoizing source prop for WebView component, but don't have enough context on identity/crypto stuff to do a proper review. Adding @varun as blocking.

native/account/siwe-panel.react.js
79 ↗(On Diff #16387)

Can we memoize this w/ React.useMemo

ashoat requested changes to this revision.Sep 8 2022, 9:50 AM
ashoat added inline comments.
landing/webpack.config.cjs
40–41 ↗(On Diff #16387)

Please separate this out into its own diff

web/webpack.config.cjs
33 ↗(On Diff #16387)

Why was this change made?

40–41 ↗(On Diff #16387)

Please separate this out itself (into the same diff that changes landing/webpack.config.cjs

This revision now requires changes to proceed.Sep 8 2022, 9:50 AM
• derek updated this revision to Diff 16662.Sep 14 2022, 7:21 AM

removed webpack config changes

• derek updated this revision to Diff 16663.Sep 14 2022, 7:23 AM
• derek marked an inline comment as done.

memoized webview src

• derek added a comment.Sep 14 2022, 7:23 AM

addressed all feedback with changes

Harbormaster completed remote builds in B12140: Diff 16662.Sep 14 2022, 7:31 AM
Harbormaster completed remote builds in B12141: Diff 16663.
ashoat requested changes to this revision.Sep 15 2022, 8:01 AM
ashoat added inline comments.
native/account/logged-out-modal.react.js
484 ↗(On Diff #16663)

We don't use inline conditionals within JSX like this in the codebase

560 ↗(On Diff #16663)

Nit: add a newline here

native/account/siwe-panel.react.js
25–27 ↗(On Diff #16663)

We shouldn't be hardcoding like this. Instead, we should construct a URL in the same way we do elsewhere in the app. Can you create a task to address this later? Please link the task here before landing

50 ↗(On Diff #16663)

This doesn't need to be async

82 ↗(On Diff #16663)

If we're not using this here, we should take it out until we are using it

This revision now requires changes to proceed.Sep 15 2022, 8:01 AM
• derek updated this revision to Diff 16760.Sep 16 2022, 8:11 AM
• derek marked an inline comment as done.

addressed comments

• derek marked 4 inline comments as done.Sep 16 2022, 8:12 AM
• derek added inline comments.
native/account/siwe-panel.react.js
25–27 ↗(On Diff #16663)

https://linear.app/comm/issue/ENG-1834/derive-siwe-url-instead-of-hardcoding-it

ashoat accepted this revision.Sep 16 2022, 8:13 AM
ashoat added inline comments.
native/account/logged-out-modal.react.js
457 ↗(On Diff #16760)

Let's keep the first character lowercase, so siweButton

Harbormaster completed remote builds in B12219: Diff 16760.Sep 16 2022, 8:21 AM
• derek updated this revision to Diff 16765.Sep 16 2022, 8:33 AM

camelcase babey

This revision was not accepted when it landed; it landed in state Needs Review.Sep 16 2022, 8:35 AM
This revision was landed with ongoing or failed builds.
Closed by commit rCOMM9f2b86dcbcef: [native] added webview to login with working SIWE (registration only) (authored by • derek). · Explain Why
This revision was automatically updated to reflect the committed changes.
• derek added a commit: rCOMM9f2b86dcbcef: [native] added webview to login with working SIWE (registration only).
Harbormaster completed remote builds in B12221: Diff 16765.Sep 16 2022, 8:43 AM
atul added inline comments.Dec 19 2022, 1:37 PM
native/account/siwe-panel.react.js
41–101

Not really understanding why we have a ConnectedRegisterPanel that wraps another functional component?

Think there might've been some confusion on the pattern after looking at another Connected class component? I think it makes sense to just merge these right?

(CC @ashoat, @tomek)

ashoat added inline comments.Dec 19 2022, 1:41 PM
native/account/siwe-panel.react.js
41–101

Ah yeah you should merge

Revision Contents
Changeset List

PathSize
native/
account/
30 lines
103 lines

Diff 16766

View Options

native/account/logged-out-modal.react.js

Loading...
View Options

native/account/siwe-panel.react.js

Loading...