This differential implements screen in registration flow to create backup message and its signature. It is consistent with the following figma design:
https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?type=design&node-id=11617-293984&mode=design&t=kZlddBvG5d3ZbRFk-0. The screen is placed as the last step
in registration flow. As of this differential generated siwe message and signature is logged. Next diffs will implement relevant JSI to put in the SQLite.
Details
Details
Execute registration flow. Ensure that we go through backup key generation step. Ensure that message and signature is logged to the console.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 73 | Will be replaced in shortly in the diff stack. | |
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 73 ↗ | (On Diff #39063) | Was this console.log just for debugging? Should we remove it before landing? |
| 82–119 ↗ | (On Diff #39063) | Wouldn't hurt to memoize these |
| 128 ↗ | (On Diff #39063) | Would be good to get @ginsu's sign-off on these colors |
| 135 ↗ | (On Diff #39063) | Would be good to get @ginsu's sign-off on these colors |
Comment Actions
Would also be good to include some screenshots of what it looks like in Test Plan so reviewers can side-by-side with the Figma designs
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | Can we add a "gate" variable here, so that it's possible to launch the new registration flow without the ETH backup step? (This is just in case the new registration flow is launched before ETH backup is ready, but I suspect it won't be necessary) |
Comment Actions
would be good to get a screenshot to see the colors visually as a sanity check for the colors but the color variables follow our conventions
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | Are you referring to isStaffRelease() check or check against FUTURE_CODE_VERSION or something like const commServicesAccessToken = false;? |
| native/account/registration/siwe-backup-message-creation.react.js | ||
| 73 ↗ | (On Diff #39063) | This is just a placeholder. Next two diffs replace it with calls hat persist it in SQLite. I wanted to separate persistence diffs from visual diffs. |
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | Probably more like the second one, but did you mean usingCommServicesAccessToken? |
Comment Actions
Video with the registration flow:
Screen with backup secrets generation screen before clicking Secure with Ethereum Wallet
after clicking Secure with Ethereum Wallet
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | Yes - I thought about usingCommServicesAccessToken. |
Comment Actions
Screen with backup secrets generation screen before clicking Secure with Ethereum Wallet
Noticed a typo here (pointed out inline). This diff introduces new copy but I was not set as a blocking reviewer; to reiterate, I must be a blocking reviewer on ANY diff that introduces new copy, and copy from Figma designs can NEVER be trusted.
Separately, this screen looks really empty. I think it would be good to introduce some sort of icon or graphic.
I searched for "backup" in react-native-vector-icons here, and MaterialIcons's backup icon looks like a good option. Can we render a large version of that icon in white here?
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 97 ↗ | (On Diff #39063) | |
Comment Actions
Add icon to enchance user experience.
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | I think that we can adopt the similar pattern as we have for enableNewRegistrationFlow. |
| native/account/registration/siwe-backup-message-creation.react.js | ||
| 82–119 ↗ | (On Diff #39063) | Handled in D11636 |
Comment Actions
Hmm, you seem to have used a different icon than the one I requested. Can you update it to use the one I mentioned here?
MaterialIcons's backup icon looks like a good option. Can we render a large version of that icon in white here?
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | What's enableNewRegistrationFlow? I can't find it in master or in this diff |
| native/account/registration/siwe-backup-message-creation.react.js | ||
| 98 ↗ | (On Diff #39141) | Typo was not fixed |
| 82–119 ↗ | (On Diff #39063) | Did you mean to reference another diff? D11636 is this diff |
Comment Actions
Can you share an updated screenshot?
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | Sure, so you're thinking of introducing a new gate variable (not enableNewRegistrationMode) that will be set to __DEV__ initially? |
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 98 ↗ | (On Diff #39144) | Realized another minor typo here. Should be "Comm encrypts user backups" |
| native/account/registration/avatar-selection.react.js | ||
|---|---|---|
| 148 ↗ | (On Diff #39063) | Yes - that will enable us to potentially launch new registration flow without SIWE backup if we ever need to. |
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 48 ↗ | (On Diff #39144) | Let's replace this with "Encrypt with Ethereum Wallet" |
Comment Actions
If we go back from registration terms screen then siwe backup message creation screen looks like this:
Tapping violet button directs us to registration terms screen while tapping black button restarts SIWE flow.
| native/account/registration/siwe-backup-message-creation.react.js | ||
|---|---|---|
| 116 ↗ | (On Diff #39208) | Since we're using "signature" in the other state, let's be consistent about that Also let's lowercase it (sorry I accidentally gave you copy with a capitalized "Wallet" earlier – that's inconsistent with our other buttons) |