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
F3389592: D5023.id16766.diff
Fri, Nov 29, 7:44 PM
Unknown Object (File)
Wed, Nov 27, 12:24 AM
Unknown Object (File)
Tue, Nov 26, 4:41 PM
Unknown Object (File)
Tue, Nov 26, 4:24 PM
Unknown Object (File)
Sat, Nov 16, 1:30 PM
Unknown Object (File)
Mon, Nov 11, 1:52 AM
Unknown Object (File)
Oct 28 2024, 5:07 AM
Unknown Object (File)
Oct 28 2024, 5:07 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
Branch
derek/siwe-webview-native
Lint
No Lint Coverage
Unit
No Test Coverage

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

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

560

Nit: add a newline here

native/account/siwe-panel.react.js
25–27

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

This doesn't need to be async

82

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

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 ↗(On Diff #16766)

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 ↗(On Diff #16766)

Ah yeah you should merge

Revision Contents
Changeset List

PathSize
native/
account/
25 lines
110 lines

Diff 16663

View Options

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

Loading...
View Options

native/account/siwe-panel.react.js

Loading...