[chukwudiikeh]

Fixes #372

RestoreKeyCommand::execute built the extended key with (mnemonic.clone(), &self.password).0. The .0 keeps only the mnemonic, so --password was silently dropped and a key generated with key generate --password restored to a different key, with no error.

This PR passes the password the same way GenerateKeyCommand does, and adds test_cli_key_restore_with_password, which checks that restoring with the same password returns the same xprv and fingerprint, and that restoring without it returns a different xprv. The new test fails on master.

Fixed

key restore --password now uses the password, so passphrase-protected mnemonics restore to the correct key.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.00%. Comparing base (3dec0e0) to head (5558555).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #373   +/-   ##
=======================================
  Coverage   60.00%   60.00%           
=======================================
  Files          25       25           
  Lines        4736     4736           
=======================================
  Hits         2842     2842           
  Misses       1894     1894           

Flag	Coverage Δ
rust	60.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[chukwudiikeh]

@tvpeter @vadim-anfv ,I would appreciate a review on this when you have the time.

[emma31-dev]

NACK

I feel there is something wrong with the way key generation or restoration works that whether I use a password or not to generate a new key it can be recovered without it or with an entirely wrong password. try this:

cargo run key generate --password dogface

you get the response:

{
  "xprv": "tprv8Zgx.....tSaG",
  "mnemonic": "...",
  "fingerprint": "..."
}

now try to restore it without a password or one entirely wrong and you can still get the key. I am not sure it is from your change or something wrong with this implementation:

let xkey: ExtendedKey = (mnemonic.clone(), self.password.clone()).into_extended_key()?;

I would like to hear what the maintainers have to say about this. As of now this issue is bigger than just the password not being used.

[chukwudiikeh]

Thanks for testing, @emma31-dev. I re-ran your steps on this branch with key generate --password dogface:

restore with --password dogface → same xprv, fingerprint 0b012590
restore with no password → different xprv, fingerprint 702a3c5e
restore with --password catface → different xprv, fingerprint 3a5f8a09

A wrong or missing passphrase still returns a key. That’s expected under BIP-39, because every passphrase gives a valid but different wallet. All testnet master keys also start with tprv8ZgxMBicQKsP, so the prefix will always look the same. Comparing the full xprv or the fingerprint shows they differ. On master, restoring with dogface gives 702a3c5e, the same as no password, which is the bug this PR fixes. test_cli_key_restore_with_password asserts exactly this. Could you re-check by comparing fingerprints?

@emma31-dev

[emma31-dev]

Everything is working as expected. Thanks for working on this. Rebase to one commit so i'll ACK

[chukwudiikeh]

Thanks @emma31-dev, all three commits squashed into a single commit.

[emma31-dev]

ACK 5558555

[muawiya1]

tACK 5558555

Tested locally on macOS by checking out master and the PR branch (5558555).

On master, I reproduced the issue: key generate --password foo and key restore with the same mnemonic and password returned different fingerprints (b6c5c9d0 and 0a25a2eb).

On the PR branch, I repeated the steps with a newly generated mnemonic. Both commands returned the same xprv and fingerprint (5231e436), confirming the fix.

I also ran cargo test --test cli test_cli_key_restore_with_password, which passed.

cargo fmt --all -- --check was clean. Clippy was also clean with --all-targets -- -D warnings, using both --no-default-features and --all-features.

[muawiya1]

This is just a follow up. I have one small, non-blocking suggestion. Could you update the --password doc comment for both GenerateKeyCommand and RestoreKeyCommand in src/handlers/key.rs? It currently says “Seed password.” I think explaining that restoring the same key requires the same mnemonic and passphrase would make it clearer. Using a different passphrase does not return an error, it produces a different valid key.

[vadim-anfv]

tACK 5558555

Reproduced the bug on master and confirmed it is gone here: with the same mnemonic and password restore returns the same key, with a different password or none a different one, as BIP-39 requires.

I do not want to hold this PR up, so I will open issues for the two follow-ups below rather than ask for changes here.

• @muawiya1's suggestion about the --password doc comment makes sense, and since BIP-39 calls it a passphrase, the field should be named that way too.
• The derivation duplicated in GenerateKeyCommand and RestoreKeyCommand is worth extracting into one function, since that duplication is how this bug appeared.

[chukwudiikeh]

Thanks for testing @emma31-dev, @muawiya1 and @vadim-anfv I will be happy to pick up both follow-ups (the --passphrase rename with the doc comment,and extracting the shared derivation logic) once the issues are open.

[tvpeter]

ACK 5558555

Please always use the provided templates for issues and PRs.

Thank you.