-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
qml: rework auth #8382
base: master
Are you sure you want to change the base?
qml: rework auth #8382
Conversation
6abce44
to
78f0122
Compare
This looks complicated, and I don't have time to review it today. |
This is actually less impact than the alternative, which would need a refactor of the wallet loading logic in relation to qewalletdb, expand its scope beyond just wallet db stuff (qewalletdb is used to trigger whether a password is needed before opening the wallet, amongst other things). This PR stays much closer to what e.g. the desktop client does, doesn't need refactoring of the qml wallet loading logic, does not need a password at startup for keystore-only passwords (might even be a feature instead of a bug?) Finally, I don't think I can fully implement and test the alternative today, so that would mean a delay until after the weekend |
… wallets This is a hugely hackish -- it uses the kivy approach, which uses this same hack... I am not really content with it but it should be relatively easy to review, and if ok, should hotfix the linked issue. fixes spesmilo#8374 related spesmilo#8382
return False | ||
|
||
try: | ||
self.wallet.keystore.check_password(password) |
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.
I don't think this always behaves correctly, e.g. for a multisig wallet that has multiple keystores. This only checks the first keystore. I would prefer if we could just use wallet.check_password()
directly, which has this logic encapsulated. None of the other GUIs call keystore.check_password() AFAICT.
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.
agreed, but wallet.check_password()
directly calls wallet.keystore.check_password()
so I'm not sure why this would be problematic
now handles keystore-only password and defines a new set of auth methods
Co-authored-by: ghost43 <[email protected]>
now handles keystore-only password and defines a new set of auth methods.