[CBR-437] Kernel.Wallets.updatePassword should record if the user decided to remove it #3621
Conversation
There was a problem hiding this comment.
LGTM. @Anviking I have added a couple of remarks I would like to see fixed/explained, but I have approved nevertheless as I don't think they are necessarily deal breakers :)
| @@ -311,8 +313,9 @@ spec = describe "Wallets" $ do | |||
| describe "Wallet update password (kernel)" $ do | |||
| prop "correctly replaces the ESK in the keystore" $ withMaxSuccess 50 $ | |||
| monadicIO $ do | |||
| newPwd <- pick arbitrary | |||
| newPwd <- pick $ arbitrary `suchThat` (/= emptyPassphrase) | |||
There was a problem hiding this comment.
I would suggest not to touch the logic of this test, as this properly tests only one thing, which is the keystore replacement. What am I missing? What's the rationale behind the change?
There was a problem hiding this comment.
I basically duplicated this test, checked for HasSpendingPassword here, and NoSpendingPassword in the new one.
I considered re-calculating the HasSpendingPassword from the arbitrary password in the test below making sure it was the same as the one in the hdRoot, but that felt to implicit, and dependent on arbitrary picking both empty and non-empty passwords.
I could make another copy? (Also, should I strip away the checks already tested here?)
There was a problem hiding this comment.
My (relatively small) concern is that the first property is called "correctly replaces the ESK in the keystore" and, as such, it should work regardless of which spending password we pick to begin with. Right now it feels like this first test is a bit misleading, this is why I have proposed to leave it be and simply add another one (like you did) to check for this specific concern only.
|
|
||
| let hasSpendingPassword = if (newPassword == emptyPassphrase) | ||
| then HD.NoSpendingPassword | ||
| else HD.HasSpendingPassword lastUpdateNow |
There was a problem hiding this comment.
This is the way to go. However, also let's add a comment on why we do this, as it might not be totally obvious to the reader that actually the frontend does allow the removal of the spending password after the wallet has been created (I, for one, got to know it just today)
Anviking
force-pushed
the
anviking/CBR-437/update-password
branch
5 times, most recently
from
77398d5 to
2960f39
Compare
|
Addressed now, I believe. |
Anviking
force-pushed
the
anviking/CBR-437/update-password
branch
from
2960f39 to
5d45665
Compare
|
Looks great, thanks @Anviking ! |
When the password is updated we should check if it is the `emptyPassphrase` and, if so, update `HdRoot` to have no spending password. This commit also adds a test property that ensures the hdRoot has no spending password afterwards, and modifies the existing property to only test updates with non-empty passwords, where we expect the new hdRoot to *have* a spending password.
Anviking
force-pushed
the
anviking/CBR-437/update-password
branch
from
5d45665 to
b7f1e5d
Compare
…-password [CBR-437] Kernel.Wallets.updatePassword should record if the user decided to remove it
…hk/anviking/CBR-437/update-password [CBR-437] Kernel.Wallets.updatePassword should record if the user decided to remove it
Description
When the password is updated we should check if it is the
emptyPassphraseand, if so, updateHdRootto have no spendingpassword.
This
commitpr also adds a test property that ensures the hdRoot has nospending password afterwards, and modifies the existing property to only
test updates with non-empty passwords, where we expect the new hdRoot to
have a spending password.
Linked issue
CBR-437
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)