Reuse password option prompts again on a wrong password

[pawanjay176]

Issue Addressed

Resolves #4364

Proposed Changes

Prompt the user to enter the password if the password is incorrect on using the reuse-password flag.
This is useful when importing multiple keystores for which there are more than 1 passwords.

cc @remyroy

[michaelsproul]

Code LGTM, has anyone tested in manually?

[remyroy]

I manually tested it and it worked for me.

[pawanjay176]

Just changed a match statement to if in 95518f6

[pawanjay176]

@chong-he Found a bunch of edge cases not properly handled. Need to fix those

[remyroy]

Is there any update on this PR? It would be nice to fix #4364

[chong-he]

@remyroy

After some testing, it was found that the PR requires a fix. Consider a scenario we have 2 keystore files with passwordA, and 3 keystore files with passwordB

When we import the keys, it is all good for the first 2 keystore files. Then it will detect that the third keystore file has a different password, which it will prompt the user to input a new password. The problem is: the UX doesn't check if the password is correct or not, and will import the validator regardless (including the case when a user enters a wrong password) and show:
Successfully imported keystore
Successfully updated validator_definitions.yml.

It also wouldn’t show whether the password is correct or not.

It will then proceed to the fourth keystore, which it will now be able to detect the incorrect password and prompt again for the correct password.

I attach a screenshot for reference.

[chong-he]

The current issue is that upon a reused password, after prompting the user to enter the password (line 177), it does not check whether the password is correct or not and proceed to the next keystore regardless. So I added a call to check_password_on_keystore to check the password.

Suggested change

	previous_password = Some(password.clone());
	break Some(password);
	previous_password = Some(password.clone());
	if check_password_on_keystore(&keystore, &password)? {
	break Some(password);
	}

The issue now is, because it becomes a nested if loop, the UX when the password is incorrect becomes:
Invalid password
at a first try, and then jumps to the beginning of the loop and display:

Invalid password
Reuse previous password.
Invalid password
Reused password incorrect. Retry!

and alternating that way until a correct password is entered. Do we have a better way for the UX? I try using a while loop but that's weird outcome too

[pawanjay176]

Fixed in bd035f9

Thanks for catching it and testing the fix CK :)

[pawanjay176]

@remyroy Sorry for the delay, this one went under the radar.

Fixed the issues now.

[chong-he]

I have tested the feature and can confirm that it is working

[jimmygchen]

@mergify queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 22ccdb6