feat(@desktop/keycard): authenticate flow added #7246
Conversation
Jenkins BuildsClick to see older builds (72)
|
saledjenic
force-pushed
the
feat/issue-7205
branch
12 times, most recently
from
1dda566
to
0ca6f90
Compare
saledjenic
changed the title
saledjenic
requested review from
a team,
osmaczko,
stefandunca,
glitchminer and
endulab
and removed request for
a team
saledjenic
requested review from
igor-sirotin
and removed request for
a team
saledjenic
force-pushed
the
feat/issue-7205
branch
3 times, most recently
from
d9aafd5
to
25601a1
Compare
There was a problem hiding this comment.
I can see you put a lot of effort into this and appreciate it.
I did my best to help by matching the requirements (design) with the states and presentation logic. However, without a clear understanding of the workflows and corner cases, I can't say I trust myself to cover much. I find it easy to get lost in the back and forth between QML events, NIM states, and keycard library input/output.
I trust you have all covered and have a deep understanding of keycard implementation.
Moving forward, I feel that more testing, a state machine diagram and some refactoring can bring some sanity to the workflows and conditions that drive them.
Otherwise helping with keycard implementation seems like a full-time job.
| @@ -6,7 +6,7 @@ QtObject: | |||
| type UserProfile* = ref object of QObject | |||
| # fields which cannot change | |||
| username: string | |||
| address: string | |||
| @@ -101,6 +105,31 @@ StatusModal { | |||
| return | |||
| } | |||
| } | |||
| if (root.sharedKeycardModule.currentState.flowType === Constants.keycardSharedFlow.authentication) { | |||
There was a problem hiding this comment.
Can't all these state checks be grouped together in a few meaningful function calls and reused?
I tried to check which overlapped with what but I get easily lost. Also matching this with requirements in design is not easy. I trust you know what you did :)
There was a problem hiding this comment.
@stefandunca it could be grouped, but in that case it will be difficult for figuring out even for me who wrote this after some time, that's why I've splitted each flow and handle the states within each flow separately even some states are the same among flows. Tracking root.sharedKeycardModule.currentState.flowType you can be sure what flow we're working with.
@stefandunca yes, I agree, this is true, but I don't see other way to cover all that correctly in Nim. You just need to follow the flow and states and realize what's happening, also need to understand flows on the keycard lib side, cause they do not match the design flows at all.
Would be nice to have, but creating something like that is very hard, we will use figma designs for that. |
Images required for authentication flow added
Prop `address` renamed to `keyUid` to be consistent in code and name the things in the way what they really are.
new procs added to the wallet account service: - `addMigratedKeyPair` - `getAllMigratedKeyPairs` - `getMigratedKeyPairByKeyUID` - `setKeycardName` - `keycardLocked` - `keycardUnlocked` - `deleteKeycard`
shared keycard popup module is extended with: - `uniqueIdentifier` - `settingsService` - `walletAccountService` - `keychainService`
- login states updated so they can resolve enter pin state from each state that flow may be in - not a keycard state added - login plugin state added
- Added `Authenticate` flow - `Setup a new Keycard with an existing account` updated so it includes `Authenticate` flow from the point where is needed (when migrating a profile keypair) - We are missing `Unlock Keycard` flow for this one, will be added once it is developed
saledjenic
force-pushed
the
feat/issue-7205
branch
from
25601a1
to
7cc3f55
Compare
glitchminer
requested review from
anastasiyaig
and removed request for
glitchminer
Corresponding
status-goPR:key_uidcolumn added to theaccountstable status-go#2839keypairstable added and necessary endpoints exposed via accounts api status-go#2846Corresponding
StatusQPR:StatusPasswordInputStatusQ#890Fixes: #7205
@ALL maybe this diagram may help in understanding the purpose of shared keycard module.