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

Authored by • derek on Jul 12 2022, 12:39 PM.

Details

Reviewers

ashoat
varun
• abosh
atul

Commits

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

Summary

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

• derek created this revision.Jul 12 2022, 12:39 PM

• derek mentioned this in D4349: Summary: added boilerplate for SIWE.Jul 12 2022, 12:40 PM

• derek edited the test plan for this revision. (Show Details)Jul 12 2022, 12:43 PM

• derek added a reviewer: varun.

Harbormaster completed remote builds in B10468: Diff 14414.Jul 12 2022, 12:52 PM

• derek requested review of this revision.Jul 12 2022, 12:52 PM

I know the reason, but it would be worth explaining to everybody why we're putting this in landing

Could you please provide a description of these changes? And perhaps also a screenshot of what's changed visually. Also, to Ashoat's point, would like to know why we're making these changes in landing

This revision now requires changes to proceed.Jul 13 2022, 1:54 PM

• derek edited the summary of this revision. (Show Details)Jul 15 2022, 8:28 AM

• derek edited the summary of this revision. (Show Details)

@varun added a description

@derek this diff is still on your queue. Make sure to hit "Re-request review" in the dropdown at the bottom of the screen if you're ready for review and want it to appear on @varun's queue

That said, I really don't think you have provided enough context in the summary. @varun wasn't asking you to provide some proof that these changes were requested by me, he's asking you to explain what the changes are, why we're making them, what other options have been considered, and why we decided to take this approach. These are details you will need to provide on most diffs, certainly diffs that are this complicated. Communication is key :)

@varun i added more context in the description & linked the linear issue
@ashoat welcoming feedback on how i can improve my summaries/test plans

• derek edited the summary of this revision. (Show Details)Aug 11 2022, 11:26 AM

Awesome, new dependencies look good and I love that this diff is small! @atul has the most background on the landing page so would love if he could take a look too

welcoming feedback on how i can improve my summaries/test plans

Don't really have much context on this diff so I'm resigning but I can offer some input on this.

Title your diffs with something in [] at the beginning that mark what section of the codebase this diff is working in. So for this diff, you could do [rainbowkit] or [landing]. I don't think Summary: at the beginning of a diff title is necessary, so the [] can replace that.
The diff should be small, but the summary and test plan should be descriptive. Obviously it's good to be concise, but it never hurts to give as much context as you can to make it easy for your reviewers to jump into reviewing. If there's something your reviewers need to Google independently for a long time, that's probably not good.
Linking Linear issues is an easy way to provide context without having to write a bunch on Phabricator, especially if your reviewers are already subscribed to the relevant Linear issue.
Adding photos and videos to your diffs (if applicable) can also be a great way to help out your reviewers, relevant doc on that
If you have multiple diffs that depend on each other or are related, you can add them to a stack using the depends on feature. Some examples of depends on: D4786, D4772.
I'd recommend you take a look at @atul or @jacek's diffs for inspiration on how to write your own. I don't look at everyone's diffs on Phabricator, but @atul and @jacek consistently put up high quality diffs that make it easy for a reviewer to understand what's going on with minimal overhead.

• derek retitled this revision from Summary: SIWE component with RainbowKit that connects & signs a message to [landing] SIWE component with RainbowKit that connects & signs a message.Aug 11 2022, 12:32 PM

Amazing advice @abosh

The changes to landing/landing.react.js make sense, don't have much context on the rest so resigning to unblock.

varun accepted this revision.Aug 15 2022, 6:44 AM

This revision is now accepted and ready to land.Aug 15 2022, 6:44 AM

Closed by commit rCOMM4dd1bf70caab: [landing] SIWE component with RainbowKit that connects & signs a message (authored by • derek). · Explain WhyAug 26 2022, 8:31 AM

This revision was automatically updated to reflect the committed changes.

• derek added a commit: rCOMM4dd1bf70caab: [landing] SIWE component with RainbowKit that connects & signs a message.

• derek mentioned this in D4960: [landing] fixed fatal bundling issue using window outside function scope.Aug 26 2022, 11:41 AM

• derek mentioned this in rCOMM87421718444f: [landing] fixed fatal bundling issue using window outside function scope.Aug 26 2022, 12:27 PM

• derek added a reverting change: D4971: Revert "[landing] SIWE component with RainbowKit that connects & signs a message".Aug 27 2022, 10:46 AM

• derek added a reverting change: rCOMM2681d4fbb030: Revert "[landing] SIWE component with RainbowKit that connects & signs a….Aug 27 2022, 11:11 AM

• derek mentioned this in D4997: [landing] SIWE component with RainbowKit that connects & signs a message.Aug 31 2022, 10:55 AM