--restore-wallet restores words from file #295

Psycho-Pirate · 2024-10-30T11:29:09Z

Added functionality to store words everytime new wallet is generated or wallet is restored by user.

--restore-wallet reads from the file if it exists, otherwise asks for words from user.

vincenzopalazzo

light review, I should checkout the branch! but in the meanwhile I left some comments

lampod-cli/src/main.rs

Harshit933 · 2024-11-02T15:49:14Z

There is some problem with the wallet design as of now. If there is a wallet that already exists in the .lampo/regtest/wallet.dat and if we accidently run lampod-cli --network <chain> the phrase in the wallet.dat file is replaced with a freshly generated one [This is a major issue], maybe we can check if the wallet.dat file already exists then don't proceed with generating new phrases, could be related to #296. Also, I think we should password protect the file and write only the encrypted seed phrase in the wallet.dat file but it could be a work for another PR.

vincenzopalazzo · 2024-11-02T17:51:18Z

I played with the code, what is wrong with my solution? @Psycho-Pirate

It solved some problems and removed the mnemonic as a Option type because there was some problem with the write and read the file.

However, I am open to discussing if there is any case that I am not keeping into count?

Probably this is also the problem that @Harshit933 point out inside the review

P.S: At this stage of lampo we should refactor the code and make it better, and not try to work with the current codebase, because most of the time there is no optimal code.

The best option to see the diff in action is to use git apply <file>

diff --git a/lampod-cli/src/main.rs b/lampod-cli/src/main.rs
index 5864816..c9a2d8f 100644
--- a/lampod-cli/src/main.rs
+++ b/lampod-cli/src/main.rs
@@ -48,33 +48,29 @@ fn main() -> error::Result<()> {
     Ok(())
 }

-fn write_words_to_file<P: AsRef<Path>>(path: P, words: Option<String>) -> io::Result<()> {
+fn write_words_to_file<P: AsRef<Path>>(path: P, words: String) -> error::Result<()> {
     let mut file = OpenOptions::new()
         .write(true)
         .create(true)
         .truncate(true)
         .open(path)?;

-    match words {
-        Some(ref value) => {
-            file.write_all(value.as_bytes())?;
-        }
-        None => {}
-    }
-
+    // FIXME: we should give the possibility to encrypt this file.
+    file.write_all(words.as_bytes())?;
     Ok(())
 }

-fn load_words_from_file<P: AsRef<Path>>(path: P) -> io::Result<Option<String>> {
-    let mut file = File::open(path)?;
+fn load_words_from_file<P: AsRef<Path>>(path: P) -> error::Result<String> {
+    let mut file = File::open(path.as_ref())?;
     let mut content = String::new();

     file.read_to_string(&mut content)?;

     if content.is_empty() {
-        Ok(None)
+        let path = path.as_ref().to_string_lossy().to_string();
+        error::bail!("The content of the wallet located at `{path}`. You lost the secret? Please report a bug this should never happens")
     } else {
-        Ok(Some(content))
+        Ok(content)
     }
 }

@@ -85,24 +81,6 @@ fn run(args: LampoCliArgs) -> error::Result<()> {
     // After this point the configuration is ready!
     let mut lampo_conf: LampoConf = args.try_into()?;

-    let words_path = format!("{}/", lampo_conf.path());
-    let mnemonic = if restore_wallet {
-        if Path::new(&format!("{}/wallet.dat", words_path)).exists() {
-            // Load the mnemonic from the file
-            load_words_from_file(format!("{}/wallet.dat", words_path))?
-        } else {
-            // If file doesn't exist, ask for user input
-            let inputs: String = term::input(
-                "BIP 39 Mnemonic",
-                None,
-                Some("To restore the wallet, lampo needs the BIP39 mnemonic with words separated by spaces."),
-            )?;
-            Some(inputs)
-        }
-    } else {
-        None
-    };
-
     log::debug!(target: "lampod-cli", "init wallet ..");
     // init the logger here
     logger::init(
@@ -141,47 +119,90 @@ fn run(args: LampoCliArgs) -> error::Result<()> {
         _ => error::bail!("client {:?} not supported", client),
     };

-    let wallet = if let Some(ref _private_key) = lampo_conf.private_key {
-        unimplemented!()
-    } else if mnemonic.is_none() {
-        let (wallet, mnemonic) = match client.kind() {
-            lampo_common::backend::BackendKind::Core => {
-                CoreWalletManager::new(Arc::new(lampo_conf.clone()))?
-            }
-            lampo_common::backend::BackendKind::Nakamoto => {
-                error::bail!("wallet is not implemented for nakamoto")
-            }
-        };
-
-        radicle_term::success!("Wallet Generated, please store these words in a safe way");
-        radicle_term::println(
-            radicle_term::format::badge_primary("wallet-keys"),
-            format!("{}", radicle_term::format::highlight(&mnemonic)),
-        );
-        write_words_to_file(format!("{}/wallet.dat", words_path), Some(mnemonic))?;
-        wallet
+    if let Some(ref _private_key) = lampo_conf.private_key {
+        error::bail!("Option to force a private key not available at the moment")
+    }
+
+    let words_path = format!("{}/", lampo_conf.path());
+    // There are several case in this if-else, that are:
+    // 1. lampo is running on a fresh os:
+    //   1.1: the user has a wallet, so need to specify `--restore-wallet`
+    //   1.2: the user does not have a wallet so lampo should generate a new seed for it and store it in a file.
+    // 2. lampo is running on os where there is a wallet, lampo will load the seeds from wallet:
+    //   2.1: The user keep specify --restore-wallet, so lampo should return an error an tell the user that there is already a wallet
+    //   2.2: The user do not specify the --restore-wallet, so lampo load from disk the file, if there is no file it return an error
+    // FIXME: there is a problem of code duplication here, we should move this code in utils functions.
+    let wallet = if restore_wallet {
+        if Path::new(&format!("{}/wallet.dat", words_path)).exists() {
+            // Load the mnemonic from the file
+            let mnemonic = load_words_from_file(format!("{}/wallet.dat", words_path))?;
+            let wallet = match client.kind() {
+                lampo_common::backend::BackendKind::Core => {
+                    // SAFETY: It is safe to unwrap the mnemonic because we check it
+                    // before.
+                    CoreWalletManager::restore(Arc::new(lampo_conf.clone()), &mnemonic)?
+                }
+                lampo_common::backend::BackendKind::Nakamoto => {
+                    error::bail!("wallet is not implemented for nakamoto")
+                }
+            };
+            wallet
+        } else {
+            // If file doesn't exist, ask for user input
+            let mnemonic: String = term::input(
+                "BIP 39 Mnemonic",
+                None,
+                Some("To restore the wallet, lampo needs the BIP39 mnemonic with words separated by spaces."),
+            )?;
+            // FIXME: make some sanity check about the mnemonic string
+            let wallet = match client.kind() {
+                lampo_common::backend::BackendKind::Core => {
+                    // SAFETY: It is safe to unwrap the mnemonic because we check it
+                    // before.
+                    CoreWalletManager::restore(Arc::new(lampo_conf.clone()), &mnemonic)?
+                }
+                lampo_common::backend::BackendKind::Nakamoto => {
+                    error::bail!("wallet is not implemented for nakamoto")
+                }
+            };
+
+            write_words_to_file(format!("{}/wallet.dat", words_path), mnemonic)?;
+            wallet
+        }
     } else {
-        match client.kind() {
-            lampo_common::backend::BackendKind::Core => {
-                // SAFETY: It is safe to unwrap the mnemonic because we check it
-                // before.
-                CoreWalletManager::restore(
-                    Arc::new(lampo_conf.clone()),
-                    &mnemonic.clone().unwrap(),
-                )?
-            }
-            lampo_common::backend::BackendKind::Nakamoto => {
-                error::bail!("wallet is not implemented for nakamoto")
-            }
+        // There is a file, so we load it, and we tell the user through logs that we found it
+        if Path::new(&format!("{}/wallet.dat", words_path)).exists() {
+            // Load the mnemonic from the file
+            let mnemonic = load_words_from_file(format!("{}/wallet.dat", words_path))?;
+            let wallet = match client.kind() {
+                lampo_common::backend::BackendKind::Core => {
+                    // SAFETY: It is safe to unwrap the mnemonic because we check it
+                    // before.
+                    CoreWalletManager::restore(Arc::new(lampo_conf.clone()), &mnemonic)?
+                }
+                lampo_common::backend::BackendKind::Nakamoto => {
+                    error::bail!("wallet is not implemented for nakamoto")
+                }
+            };
+            wallet
+        } else {
+            let (wallet, mnemonic) = match client.kind() {
+                lampo_common::backend::BackendKind::Core => {
+                    CoreWalletManager::new(Arc::new(lampo_conf.clone()))?
+                }
+                lampo_common::backend::BackendKind::Nakamoto => {
+                    error::bail!("wallet is not implemented for nakamoto")
+                }
+            };
+
+            write_words_to_file(format!("{}/wallet.dat", words_path), mnemonic)?;
+            wallet
         }
     };
+
     log::debug!(target: "lampod-cli", "wallet created with success");
     let mut lampod = LampoDaemon::new(lampo_conf.clone(), Arc::new(wallet));

-    if !Path::new(&format!("{}/wallet.dat", words_path)).exists() {
-        write_words_to_file(format!("{}/wallet.dat", words_path), mnemonic)?;
-    }
-
     // Init the lampod
     lampod.init(client)?;

Harshit933 · 2024-11-03T07:08:38Z

@vincenzopalazzo Looks good to me.

Psycho-Pirate · 2024-11-04T17:14:26Z

I played with the code, what is wrong with my solution? @Psycho-Pirate

It solved some problems and removed the mnemonic as a Option type because there was some problem with the write and read the file.

However, I am open to discussing if there is any case that I am not keeping into count?

Probably this is also the problem that @Harshit933 point out inside the review

P.S: At this stage of lampo we should refactor the code and make it better, and not try to work with the current codebase, because most of the time there is no optimal code.

This looks good and covers everything. We can go forward with this approach. But I have some suggestions:

When restore_wallet is false, we are checking for the file and loading the wallet if file exists. I think here we should exit with error saying something like "Wallet already exists. Please use --restore-wallet".
In case we are generating a new wallet we should also display the words generated as we were doing in the existing code.

vincenzopalazzo · 2024-11-05T09:01:25Z

When restore_wallet is false, we are checking for the file and loading the wallet if file exists. I think here we should exit with error saying something like "Wallet already exists. Please use --restore-wallet".

This sounds good to me, thanks for the suggestion.

In case we are generating a new wallet we should also display the words generated as we were doing in the existing code.

This is a privacy leak and we should avoid it. Then we should add a command in lampod-cli show-bip39 that return the words back, what do you think about this?

Psycho-Pirate · 2024-11-05T09:19:30Z

This is a privacy leak and we should avoid it. Then we should add a command in lampod-cli show-bip39 that return the words back, what do you think about this?

I agree! So I will go ahead with this approach and I will add the command lampod-cli show-bip39 in a followup PR

vincenzopalazzo

Ok two comment and we are ready to go, sorry about the confusion here anyway!

lampod-cli/src/main.rs

vincenzopalazzo · 2024-11-05T12:24:42Z

lampod-cli/src/main.rs

+                    // before.
+                    CoreWalletManager::restore(Arc::new(lampo_conf.clone()), &mnemonic)?
+                }
+                lampo_common::backend::BackendKind::Nakamoto => {


same consideration here for a folloup PR

vincenzopalazzo · 2024-11-05T12:26:52Z

lampod-cli/src/main.rs

-            }
+        // If there is a file, we tell the user to use --restore-wallet
+        if Path::new(&format!("{}/wallet.dat", words_path)).exists() {
+            error::bail!("Wallet already exists. Please use --restore-wallet")


nit: I reconsider my self here. This make scripting or using systemd in linux much harder because it is not that trivial run two flavor of command.

Probably you can print a warning message here? and say that lampo is restarting an existing wallet?

vincenzopalazzo

Another iteration and probably we are good

lampod-cli/src/main.rs

vincenzopalazzo · 2024-11-05T13:58:31Z

lampod-cli/src/main.rs

        if Path::new(&format!("{}/wallet.dat", words_path)).exists() {
-            error::bail!("Wallet already exists. Please use --restore-wallet")
+            // Load the mnemonic from the file
+            radicle_term::warning("Loading from existing wallet");


No, use log::warn("Loading from existing wallet");

vincenzopalazzo · 2024-11-05T14:01:15Z

@Psycho-Pirate when you are ready please reassign my review but I think I self review that you do yourself on the code would be good too.

In addition, we need to work on how you present the git history (6 comment for a git apply <patch> that I give are too many https://github.com/vincenzopalazzo/lampo.rs/pull/295/commits)

I think this is a good reading https://cbea.ms/git-commit/ and this is a bad example of commit history that cause a close of the PR #284 (comment)

Psycho-Pirate · 2024-11-05T14:32:24Z

Should I also squash the commits into one?

vincenzopalazzo · 2024-11-05T14:38:41Z

Should I also squash the commits into one?

Up to you, the history should make sense and be easy to cherry-pick, probably this is a good reading https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html#separate-your-changes

… called

Psycho-Pirate · 2024-11-05T15:02:12Z

Squashed it to two commits for cleaner commit history

vincenzopalazzo

ACK aca1633

Assuming that you tested out 😸

Psycho-Pirate requested a review from vincenzopalazzo as a code owner October 30, 2024 11:29

vincenzopalazzo reviewed Oct 30, 2024

View reviewed changes

lampod-cli/src/main.rs Outdated Show resolved Hide resolved

lampod-cli/src/main.rs Outdated Show resolved Hide resolved

Psycho-Pirate commented Nov 1, 2024

View reviewed changes

lampod-cli/src/main.rs Outdated Show resolved Hide resolved

vincenzopalazzo mentioned this pull request Nov 2, 2024

Return an error when lampo-wallet already exists #296

Closed

vincenzopalazzo requested changes Nov 5, 2024

View reviewed changes

Psycho-Pirate added 2 commits November 5, 2024 20:26

--restore-wallet restores words from file

c42ed2e

Load wallet with a warning if file exists and --restore-wallet is not…

aca1633

… called

Psycho-Pirate force-pushed the save_phrase branch from f220ab0 to aca1633 Compare November 5, 2024 15:00

vincenzopalazzo approved these changes Nov 5, 2024

View reviewed changes

vincenzopalazzo merged commit 0d3a954 into vincenzopalazzo:main Nov 5, 2024
3 of 5 checks passed

Psycho-Pirate mentioned this pull request Jan 22, 2025

Support for Nakamoto removed #343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--restore-wallet restores words from file #295

--restore-wallet restores words from file #295

Psycho-Pirate commented Oct 30, 2024

vincenzopalazzo left a comment

Harshit933 commented Nov 2, 2024

vincenzopalazzo commented Nov 2, 2024

Harshit933 commented Nov 3, 2024

Psycho-Pirate commented Nov 4, 2024

vincenzopalazzo commented Nov 5, 2024

Psycho-Pirate commented Nov 5, 2024

vincenzopalazzo left a comment

vincenzopalazzo Nov 5, 2024

vincenzopalazzo Nov 5, 2024

Psycho-Pirate Nov 5, 2024

vincenzopalazzo left a comment

vincenzopalazzo Nov 5, 2024

vincenzopalazzo commented Nov 5, 2024

Psycho-Pirate commented Nov 5, 2024

vincenzopalazzo commented Nov 5, 2024

Psycho-Pirate commented Nov 5, 2024

vincenzopalazzo left a comment

--restore-wallet restores words from file #295

--restore-wallet restores words from file #295

Conversation

Psycho-Pirate commented Oct 30, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Harshit933 commented Nov 2, 2024

vincenzopalazzo commented Nov 2, 2024

Harshit933 commented Nov 3, 2024

Psycho-Pirate commented Nov 4, 2024

vincenzopalazzo commented Nov 5, 2024

Psycho-Pirate commented Nov 5, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo Nov 5, 2024

Choose a reason for hiding this comment

vincenzopalazzo Nov 5, 2024

Choose a reason for hiding this comment

Psycho-Pirate Nov 5, 2024

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo Nov 5, 2024

Choose a reason for hiding this comment

vincenzopalazzo commented Nov 5, 2024

Psycho-Pirate commented Nov 5, 2024

vincenzopalazzo commented Nov 5, 2024

Psycho-Pirate commented Nov 5, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment