[native] use identity wallet login in fullscreen-siwe-panel
ClosedPublic
Actions

Authored by varun on Feb 8 2024, 11:02 PM.

Details

Reviewers

tomek
inka
ashoat

Commits

rCOMMbbea194c3761: [native] use identity wallet login in fullscreen-siwe-panel

Summary

if usingCommServicesAccessToken, SIWE auth will now be done by the identity service

Test Plan

tested the following cases:

fresh eth wallet with enableNewRegistrationMode
- was navigated to the new registration wizard
existing account with enableNewRegistrationMode
- was logged in
fresh eth wallet with !enableNewRegistrationMode
- was registered with identity service
existing account with !enableNewRegistrationMode
- was logged in
invalid siwe signature
- alert displayed

handling reserved users will be tackled in https://linear.app/comm/issue/ENG-4033/update-clients-to-try-new-registration-after-failed-login-due-to

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Feb 8 2024, 11:02 PM

Harbormaster completed remote builds in B26683: Diff 36897.Feb 8 2024, 11:17 PM

varun requested review of this revision.Feb 8 2024, 11:17 PM

tomek added inline comments.Feb 9 2024, 3:18 AM

native/account/fullscreen-siwe-panel.react.js
79	Shouldn't we also dispatch `setDataLoadedActionType` action? Or maybe we're waiting with it for Ashoat's keyserver to also auth?
native/account/siwe-hooks.js
104–110	I guess we can simplify it, but that needs testing

varun added inline comments.Feb 9 2024, 8:17 AM

native/account/fullscreen-siwe-panel.react.js
79	my understanding was that we only want to set dataLoaded once we manage to connect to @ashoat 's keyserver
native/account/siwe-hooks.js
104–110	i think we have to await it on line 109 too otherwise the error gets swallowed and we can't alert from the fullscreen panel

varun added a child revision: D11016: [native] logic for navigating to ExistingEthereumAccount if usingCommServicesAccessToken.Feb 9 2024, 10:39 AM

enableNewRegistrationMode and onAccountDoesNotExist need to be handled here

native/account/fullscreen-siwe-panel.react.js
77–89	If `enableNewRegistrationMode` is set, then this call needs to fail if the account does not exist yet If the call fails because the account does not exist yet, then we need to call `onAccountDoesNotExist`
native/account/siwe-hooks.js
104–110	@tomek's code is implicitly awaiting because `dispatchActionPromise` returns a `Promise`. He's suggesting it needs testing because that `await` might resolve at a slightly different time than the `await siwePromise` line

This revision now requires changes to proceed.Feb 9 2024, 11:14 AM

ashoat added inline comments.Feb 9 2024, 11:26 AM

native/account/siwe-hooks.js
101	This probably will need to handle `doNotRegister`

tomek added inline comments.Feb 12 2024, 12:36 AM

native/account/siwe-hooks.js
104–110	@tomek's code is implicitly awaiting because `dispatchActionPromise` returns a `Promise`. He's suggesting it needs testing because that `await` might resolve at a slightly different time than the `await siwePromise` line Exactly!

varun mentioned this in D11078: [identity] introduce RegisterWalletUser RPC.Feb 14 2024, 10:04 AM

varun mentioned this in D11084: [lib] useIdentityWalletRegister hook.Feb 14 2024, 12:11 PM

varun added a parent revision: D11084: [lib] useIdentityWalletRegister hook.Feb 14 2024, 12:41 PM

varun marked 6 inline comments as done.Feb 15 2024, 2:22 PM

varun added inline comments.

native/account/siwe-hooks.js
104–110	i tried @tomek 's approach initially and the alert in `FullscreenSIWEPanel` was not displayed on an error response from the identity service. on line 90 above we `await siwePromise` the same way, i presume for the same reason

handle enableNewRegistrationMode and onAccountDoesNotExist

varun edited parent revisions, added: D11098: [native] add findUserIDForWalletAddress to CommRustModule; removed: D11084: [lib] useIdentityWalletRegister hook.Feb 15 2024, 2:23 PM

varun edited the test plan for this revision. (Show Details)Feb 15 2024, 2:32 PM

varun edited the test plan for this revision. (Show Details)

varun mentioned this in D11098: [native] add findUserIDForWalletAddress to CommRustModule.Feb 15 2024, 2:38 PM

Harbormaster completed remote builds in B26944: Diff 37286.Feb 15 2024, 2:39 PM

varun mentioned this in rCOMMca0a5351d647: [identity] introduce RegisterWalletUser RPC.Feb 16 2024, 7:20 AM

varun mentioned this in rCOMM02d4b3dd9f77: [lib] useIdentityWalletRegister hook.

Should we "fall back" to the legacy workflow if the new one doesn't work?

In D11005#320730, @ashoat wrote:

Should we "fall back" to the legacy workflow if the new one doesn't work?

we have a new plan instead of a fallback mechanism including automated alerts, daily manual testing, and an opt-in mechanism for clients to inform us when there are issues

ashoat accepted this revision.Feb 22 2024, 7:43 PM

This revision is now accepted and ready to land.Feb 22 2024, 7:43 PM

Closed by commit rCOMMbbea194c3761: [native] use identity wallet login in fullscreen-siwe-panel (authored by varun). · Explain WhyFeb 22 2024, 8:17 PM

This revision was automatically updated to reflect the committed changes.

varun added a commit: rCOMMbbea194c3761: [native] use identity wallet login in fullscreen-siwe-panel.