Implement the keycard flows for the new onbaording #17127
Conversation
jrainville
requested review from
a team,
micieslak,
caybro and
alexjba
as code owners
jrainville
requested review from
Cuteivist,
igor-sirotin and
glitchminer
and removed request for
a team
jrainville
changed the title
Jenkins BuildsClick to see older builds (68)
|
| try { | ||
| const seedwords = root.getSeedWords() | ||
| d.seedWords = JSON.parse(seedwords) | ||
| root.seedphraseSubmitted(d.seedWords) |
There was a problem hiding this comment.
This should be emitted only after the user has confirmed and moved to the next page
There was a problem hiding this comment.
Earlier we were assuming, that seed words are generated locally, with no interaction with the keycard. So we assumed it's fast, sync operation which never fails. So it was convenient to pass it just as a property.
But if we obtain the seed words from the keycard, the flow is different, with different assumptions.
In the proposed implementation the call remains synchronous. But it may fail (the easiest option is to disconnect keycard in the meantime). Then the UI is left in invalid state, the situation is not handled at all. It makes the flow incomplete.
As it's synchronous, the question is really should be like that. There is communication with the device, probably we should be prepared for some delay without blocking the UI. Now in case of delay we have just freeze.
Next thing is emitting seedphraseSubmitted signal:
const seedwords = root.getSeedWords()
d.seedWords = JSON.parse(seedwords)
root.seedphraseSubmitted(d.seedWords)
This probably doesn't make much sense. The backend knows that seed words were fetched because the function getSeedWords was called.
The problem starts probably in the Figma design, where the case of fetching seed words in not covered at all because wrong assumptions we initially had.
The order of pin setting and seed words fetching is also not updated, making the flow harder to analyze.
@jrainville @caybro wdyt?
There was a problem hiding this comment.
-
GenerateMnemonic(which is called bygetSeedWordsis a pretty fast procedure. But I agree that even in this case it should be done asyncronously.
I think it's fine to merge as is and improve after, wdyt? -
Not sure what you mean is wrong here? 🤔
I don't get why we need toJSON.parse, I'll double check and remove it.
cc @micieslak
| @@ -147,7 +166,9 @@ Page { | |||
| syncState: root.onboardingStore.syncState | |||
| addKeyPairState: root.onboardingStore.addKeyPairState | |||
|
|
|||
| seedWords: root.onboardingStore.getMnemonic().split(" ") | |||
| getSeedWords: function () { | |||
There was a problem hiding this comment.
We can only get the mnemonic when the keycard is authorized or initialized, so I pass the function to be called later when we have the right to call it.
| onboardingModuleInst.obtainingPasswordSuccess.connect(root.obtainingPasswordSuccess) | ||
| onboardingModuleInst.obtainingPasswordError.connect(root.obtainingPasswordError) | ||
| d.onboardingModuleInst.appLoaded.connect(root.appLoaded) | ||
| // TODO implement the following signals |
There was a problem hiding this comment.
Even if not implemented yet, this can stay here uncommented, right?
There was a problem hiding this comment.
Since the view functions don't exist, any connect put below those don't activate because the function throws an error.
It felt safer to comment those for now as it confused me for a while why my signals didn't work.
| signal keycardPinCreated(string pin) | ||
| signal setPinFailed(string error) |
There was a problem hiding this comment.
The general rule regarding signals/functions is that signals are used to notify change/event generated internally within the component. So in vast majority of changes they are called (emitted) from the component, not from outside.
I we want to interact with the component from outside, function is proper choice.
More details can be found there: https://github.com/Furkanzmc/QML-Coding-Guide?tab=readme-ov-file#sh-2-when-to-use-functions-and-signals
| try { | ||
| const seedwords = root.getSeedWords() | ||
| d.seedWords = JSON.parse(seedwords) | ||
| root.seedphraseSubmitted(d.seedWords) |
There was a problem hiding this comment.
Earlier we were assuming, that seed words are generated locally, with no interaction with the keycard. So we assumed it's fast, sync operation which never fails. So it was convenient to pass it just as a property.
But if we obtain the seed words from the keycard, the flow is different, with different assumptions.
In the proposed implementation the call remains synchronous. But it may fail (the easiest option is to disconnect keycard in the meantime). Then the UI is left in invalid state, the situation is not handled at all. It makes the flow incomplete.
As it's synchronous, the question is really should be like that. There is communication with the device, probably we should be prepared for some delay without blocking the UI. Now in case of delay we have just freeze.
Next thing is emitting seedphraseSubmitted signal:
const seedwords = root.getSeedWords()
d.seedWords = JSON.parse(seedwords)
root.seedphraseSubmitted(d.seedWords)
This probably doesn't make much sense. The backend knows that seed words were fetched because the function getSeedWords was called.
The problem starts probably in the Figma design, where the case of fetching seed words in not covered at all because wrong assumptions we initially had.
The order of pin setting and seed words fetching is also not updated, making the flow harder to analyze.
@jrainville @caybro wdyt?
| if (root.keycardState === Onboarding.KeycardState.NotEmpty) { | ||
| if (d.withNewSeedphrase) { | ||
| // Need to authorize before getting a seedphrase | ||
| root.authorizationRequested() |
There was a problem hiding this comment.
This doesn't look correct. If the authorization doesn't require any input from from the user from this page and is not caused by any action triggered there, that request shouldn't be generated be the UI. In other words, it should not be the responsibility of the UI, which should be unaware of this process.
| signal keycardPinCreated(string pin) | ||
| signal setPinFailed(string error) |
There was a problem hiding this comment.
For async operations like adding key pair we have dedicated enum defined (AddKeyPairState in enums.h) which is used for handling that ansyc operation. Similarly for syncing. Here the approach is different, UI emits keycardPinCreatedand expects that at some point in the futuresetPinFailed` will be called or some other thing will happen which means success (btw, how we may know that pin setting succeeded)?
|
@jrainville needs a rebase, and more importantly, updating the QML tests. For that, I'd suggest first updating the individual QML pages we have for the various flows as well |
jrainville
force-pushed
the
feat/status-keycard-go-api-v2
branch
from
d711fb4 to
6b598ce
Compare
jrainville
force-pushed
the
feat/status-keycard-go-api-v2
branch
from
6b598ce to
51baa19
Compare
| @@ -147,7 +166,9 @@ Page { | |||
| syncState: root.onboardingStore.syncState | |||
| addKeyPairState: root.onboardingStore.addKeyPairState | |||
|
|
|||
| seedWords: root.onboardingStore.getMnemonic().split(" ") | |||
| getSeedWords: function () { | |||
There was a problem hiding this comment.
We can only get the mnemonic when the keycard is authorized or initialized, so I pass the function to be called later when we have the right to call it.
| onboardingModuleInst.obtainingPasswordSuccess.connect(root.obtainingPasswordSuccess) | ||
| onboardingModuleInst.obtainingPasswordError.connect(root.obtainingPasswordError) | ||
| d.onboardingModuleInst.appLoaded.connect(root.appLoaded) | ||
| // TODO implement the following signals |
There was a problem hiding this comment.
Since the view functions don't exist, any connect put below those don't activate because the function throws an error.
It felt safer to comment those for now as it confused me for a while why my signals didn't work.
| password = "", # For keycard it will be substituted with`encryption.publicKey` in status-go | ||
| seedPhrase, | ||
| recoverAccount = false, | ||
| keycardInstanceUID = keycardEvent.keycardInfo.instanceUID, |
There was a problem hiding this comment.
We're using this last received keycardEvent, which I think can be a bit wrong.
I think we should kinda hold/remember the active InstanceUID, and in case a keycard was replaced with a different one, we should start any operations from the beginning.
This would basically ensure that any further operations are executed at the expected keycard, not just the one that's currently inserted. The time window for such mistakes is short and it's difficult to repro, but anyway should be important to do.
jrainville
force-pushed
the
feat/status-keycard-go-api-v2
branch
3 times, most recently
from
78e4617 to
ed4446e
Compare
igor-sirotin
force-pushed
the
feat/status-keycard-go-api-v2
branch
from
ed4446e to
0061e2b
Compare
|
I've tested all keycard flows, seems to be working fine. A few minor things I've noted, not sure if this is expected.
|
I fixed the rebase issues yesterday, just forgot to push 🤦 |
Still failing :/ |
@caybro, I will take care of the builds. Would be cool if you can review meanwhile 🙂 |
What does the PR do
Fixes #17079
Hooks all the keycard flows for the new onboarding.
Based on top of #17058 to have the fix for required properties
status-keycard-go PR here done by @igor-sirotin : keycard-tech/status-keycard-go#13
Affected areas
The new onboarding (using the feature flag).
This is mostly Nim code, but I had to modify the QML a lot because I needed to make a lot of the keycard calls async, because they can be slow and would freeze the app (most notable case is the setPin).
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
Create account from scratch:
new-account-basic.webm
Create account with mnemonic:
new-account-mnemonic.webm
"Login" with old keycard:
old-account-keycard.webm
Impact on end user
Nothing without the feature flag.
When it is active, the new onboarding shows and all the flows for the keycard should work when there is no previous account (you need to delete or rename the data dir).
How to test
export FLAG_ONBOARDING_V2_ENABLED=1 && make runRisk
Tick one:
Low because the feature flag needs to be enabled.
When enabled, worst case is some flows don't work properly.