introduce function that uses useIdentityRegister hook to register user with identity service
Details
Details
- Reviewers
ashoat - Commits
- rCOMM8179b46b2201: [native] identityRegisterUsernameAccount function
successfully registered new user with identity service. also confirmed that all the error cases are handled by displaying the correct alert.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 121–142 ↗ | (On Diff #35415) | i'll dedup this after i resolve my flow issue |
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 119 ↗ | (On Diff #35415) | flow complains when i try to return the result from callIdentityRegister here, but i think identityRegisterPromise should be a Promise<UserLoginResponse>, right? |
Comment Actions
publishing now for @ashoat to take a look but will add other reviewers after flow issue is resolved
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 78 ↗ | (On Diff #35415) | Issue 1 is that you're not invoking this function Issue 2 is that the way you've set it up, in order to invoke the function you need all of the parameters Instead of having useIdentityRegister be a function that takes the parameters and returns a new function that takes no parameters You should have useIdentityRegister be a function that takes no parameters and returns a new function that takes the parameters Working patch here |
Comment Actions
I think the extraction of the alert messages should've been a separate diff.
Requesting change because I'm concerned about the reconstruction of keyPayload. I don't think we should be doing this in two separate places... it feels really brittle.
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 86–96 ↗ | (On Diff #35583) | These messages seem super inconsistent and a bit more like they were intended to be human-readable than matched as exact strings. I guess this is meant to be addressed in ENG-3841 |
| 195–202 ↗ | (On Diff #35583) | I assume you're going to address ETH users in a separate diff? |
| native/identity-service/identity-service-context-provider.react.js | ||
| 95–99 ↗ | (On Diff #35583) | This is a really weird API... the signature is on the keyPayload generated below, right? I'm worried that the C++ code that generates the keyPayload may diverge from the JS code below. Why not just return the keyPayload directly, to guarantee that the signature matches up? Seems a lot less brittle |
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 86–96 ↗ | (On Diff #35583) | yeah these should be made consistent as part of that work |
| 195–202 ↗ | (On Diff #35583) | yes |
| native/identity-service/identity-service-context-provider.react.js | ||
| 95–99 ↗ | (On Diff #35583) | i was constructing keyPayload manually because the order of the identity keys mattered for olm session creation. however, @kamil has mitigated this by enforcing the ordering after the keys are pulled from the identity service (see line 92 in D10381, for example). i can just use blobPayload directly now |