Details
This whole stack was tested with the following steps:
- On native, attempting to log in with an ETH account that hasn’t been registered yet
- On native, attemping to register with an ETH account that has already been registered
- On native, log in with an ETH account that has already been registered
- On native, register with an ETH account that hasn’t already been registered
- On native, register with a password account
- On native, log in with a password account
- On web, attempting to log in with an ETH account that hasn’t been registered yet
- On web, log in with an ETH account that has already been registered
- On web, log in with a password account
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 288 | Since setCurrentStep(inactiveStep) is not called here, does that mean that it is not possible to retry identity registration? | |
| 317 | At this point, we are registered with identity. Can we name this step differently? 'identity_registration' maybe? The waiting_for really had me thinking for a second that we are not registered yet at this point. | |
| 342–343 | What happens if this runs? We are logged in with identity, but not the keyserver. Will a recovery login be triggered? Will the user be logged out of identity? | |
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 288 | No. Line 212 guarantees that if we're entering this code block, currentStep is already inactiveStep. There's no need to change it | |
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 317 | When we set currentStep.step to waiting_for_identity_registration on line 281, the request to the identity service has already concluded, but we are still waiting for the data to be updated in Redux as a result of the corresponding Redux action. A more accurate name would be waiting_for_identity_registration_data_in_redux or something, but that's a bit of a mouthful. I don't think I like identity_registration. That implies that the step we are currently on is identity registration, but actually during that step we are performing keyserver auth.
I am not sure what you are suggesting here. We already checking both the step and isRegisteredOnIdentity. It appears you are suggesting only checking the step, but I don't see what benefit that would have, or how it would address your concerns about naming. | |
| 342–343 | Handling this correctly is tracked in ENG-7775. In that task, this code will be updated to call deleteAccount to clear the data on the identity service and locally.
Even without ENG-7775, I don't think this would happen. Generally a recovery login has to be triggered by a session invalidation coming from the keyserver, either via a REST request or via the socket.
Not currently, but in ENG-7775 the deleteAccount call will cause the user to be logged out of identity. | |
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 317 | An alternative would be to have inactive, then authoritative_keyserver_registration, and then setting_avatar. I think it's a bit weird that we don't have identity_registration in the list, though. We could rename inactive to identity_registration, but then it's weird that we're in an identity_registration step for most of the lifetime of the app (both before and after the button is clicked), even though we're not actually doing identity registration. I'd like to avoid adding fake steps here... the naming should not affect the logic. Open to other suggestions on the naming, but also don't know if it matters all that much. I can also use the long name waiting_for_identity_registration_data_in_redux if you'd like. | |
| native/account/registration/registration-server-call.js | ||
|---|---|---|
| 288 | Oh, because we only set the step to waiting_for_identity_registration_in_redux after everything has succeeded. I see, thank you | |
| 317 | waiting_for_identity_registration_in_redux is still incorrect at this point here, because if isRegisteredOnIdentity is true, then the redux must have been updated.
I was suggesting adding another step - once redux data is updated, we could change the step from waiting_for_identity_registration to identity_registration_concluded. But if we don't want more steps then that doesn't help. We set the step names after we dispatch identity register and keyserver register respectively, so I think we could use inactive, identity_registration_called, keyserver_registration_called (or dispatched). But if you don't like that either, then I think we can leave it as is. It's not very important, it just seems confusing, but not worth spending more time on I think. | |
waiting_for_identity_registration_in_redux -> identity_registration_dispatched
waiting_for_authoritative_keyserver_registration_in_redux -> authoritative_keyserver_registration_dispatched