Page MenuHomePhabricator

[landing] SIWE component with RainbowKit that connects & signs a message
ClosedPublic

Authored by derek on Aug 31 2022, 10:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jun 9, 7:06 PM
Unknown Object (File)
Mon, Jun 3, 1:53 PM
Unknown Object (File)
Mon, Jun 3, 10:54 AM
Unknown Object (File)
Wed, May 29, 4:18 PM
Unknown Object (File)
Mon, May 27, 11:51 AM
Unknown Object (File)
Mon, May 27, 11:51 AM
Unknown Object (File)
Mon, May 27, 11:51 AM
Unknown Object (File)
Mon, May 27, 11:51 AM

Details

Summary

take two of D4510, which was reverted because of breaking css changes. re-pasting the summary here

The goal: to sign in with ethereum! to do that we need a signed message from the wallet to pass to the identity service (which is currently native code, not in the browser).

This diff adds a top level routing page that includes RainbowKit's Connect Wallet button, as well as a function that will pass the signed message back from the webview to native where the identity service code lives.

More context for why we're making these changes in landing, why there are so many package changes, and what other options have been considered can be found in linear, issue 1212.

Test Plan

run /landing and open /commlanding/siwe on a simulator webview to get a message over the bridge

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul requested changes to this revision.Aug 31 2022, 11:17 PM

Don't have enough context on the ethereum stuff to give this a proper review, but jotted down some things I noticed should probably be addressed before someone does a proper review

keyserver/images/ethereum_icon.svg
1 ↗(On Diff #16132)

Can we add a newline to the end of this file?

1 ↗(On Diff #16132)

Can you provide a source for this SVG asset? Is it from some sort of official Ethereum brand guide?

At a high level anytime there's anything visual, the more screenshots the better


Opened the SVG in sketch and it looks as I'd expect (blue bg added for visibility)

d07cd1.png (436×414 px, 47 KB)

keyserver/src/responders/landing-handler.js
159–161 ↗(On Diff #16132)

Guessing that removing the newline here wasn't intentional?

landing/landing.react.js
49–58 ↗(On Diff #16132)

I think what we typically do in the codebase in this sort of scenario is to declare a variable (undefined by default) and then to set it if whatever condition is true.

eg

let header;
if (!onQR && !onSIWE) {
  header = <Header />;
}

It looks like here we're instead declaring and setting the variable to whatever value, and then unsetting (by setting to null) based on whatever condition.

Just a style thing I noticed, not sure it matters nor do I personally care... just figured I'd flag

landing/siwe.css
38 ↗(On Diff #16132)

I think we can cut the siwe prefix for selector names

If we do decide to keep it, might be good to style it as siweH1

landing/siwe.react.js
69 ↗(On Diff #16132)

Personally I don't mind this, but the team has decided against ternaries in JSX like this: https://www.notion.so/commapp/Use-ternary-conditionals-sparingly-f4ba44a10259403592a1d15440a9847e

72 ↗(On Diff #16132)

Can we use React.useCallback() for this

76 ↗(On Diff #16132)

Can we memoize this

This revision now requires changes to proceed.Aug 31 2022, 11:17 PM

responded to atul's comments

derek added inline comments.
landing/landing.react.js
49–58 ↗(On Diff #16132)

that logic wouldn't work for onQR or onSIWE - i think reversing the logic is more sensible here, seeing as most pages need the header & footer

atul added 1 blocking reviewer(s): varun.

Resigning since my high-level "React" feedback was addressed and I don't have context on the crypto stuff.

varun requested changes to this revision.Sep 13 2022, 11:13 PM
varun added inline comments.
landing/siwe.react.js
1 ↗(On Diff #16214)

i feel like this file could be a separate diff. no need to restructure things now, though

23–26 ↗(On Diff #16214)

inline comments that explain what we're doing would be helpful before all these wagmi function calls

84 ↗(On Diff #16214)

not super familiar with react. can you explain what this function is doing?

This revision now requires changes to proceed.Sep 13 2022, 11:13 PM
derek marked an inline comment as done.

added comment with docs link

derek added inline comments.
landing/siwe.react.js
1 ↗(On Diff #16214)

most of this is boilerplate from their docs

84 ↗(On Diff #16214)

the respective hooks access the wagmi client & rainbowkit functionality via React Context. using context, every component below the wrappers in the tree are exposed to the singletons that you invoke functionality like popping up modals, signing, etc.

thanks for the explanation!

This revision is now accepted and ready to land.Sep 14 2022, 10:30 AM