-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(onbaording): implement basic functions for the new onboarding #17003
base: master
Are you sure you want to change the base?
Conversation
Fixes #16832 Implements all the needed basic Nim functions for the new onboarding. They do no do anything just yet. They shall be integrated in another commit.
Jenkins Builds
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
proc createAccountAndLogin*(self: Controller, password: string): string = | ||
return self.accountsService.createAccountAndLogin( | ||
password, | ||
displayName = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to still generate at least some displayName
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I haven't tested yet since I've yet to integrate it. I imagine that if we set it to ""
, it will automatically default to the alias
, which should be what we want.
I think for now the alias is the three word name, but we were already thinking of replacing it for the pubkey, and it's an easy change
mnemonic, | ||
password, | ||
recoverAccount, | ||
displayName = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
CreateProfileWithKeycardExistingSeedphrase, | ||
LoginWithSeedphrase, | ||
LoginWithSyncing, | ||
LoginWithKeycard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to use the C++ enum here as the only source of truth (which I created in the other PR which is not merged yet ofc), and also in QML. Perhaps later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah. I didn't find an enum that was usable, so I created one, but I have nothing against getting rid of this one to share the same you have once I integrate.
Do you know how to access the enum from Nim?
QtProperty[int] keycardRemainingPinAttempts: | ||
read = getKeycardRemainingPinAttempts | ||
notify = keycardRemainingPinAttemptsChanged | ||
proc setKeycardRemainingPinAttempts(self: View, keycardRemainingPinAttempts: int) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably not have a setter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same for some of the other props in this file too)
Fixes #16832
Implements all the needed basic Nim functions for the new onboarding.
They do no do anything just yet. They shall be integrated in another PR.
Everything compiles nonetheless.