Fix minor wallet issues (#220)

# Changelog

- added [`tempfile`](https://crates.io/crates/tempfile) to create test
dirs for testing
- updated test functions and util functions accordingly (removed
redundant helper functions and replaced references)
- added `rust-toolchain.toml` with stable rust `1.80.0`
- minor refactors to CI workflows to match the `rust-toolchain.toml`
rust version
- git ignoring IDE directory (`.vscode/`)
- fixed `new_at_index` function to print checksum wallet address
(currently printing bech32 address to stdout)
- updated `ensure_no_wallet_exists` function to handle the following
cases:
  - where wallet path is a directory
  - where wallet path is an empty file
- this also addresses:
#186
- added unit tests for `ensure_no_wallet_exists`

---------

Co-authored-by: Kaya Gokalp <[email protected]>

Author: zees-dev

.github/workflows/ci.yml

@@ -12,7 +12,8 @@ env:
   CARGO_TERM_COLOR: always
   RUSTFLAGS: -D warnings
   REGISTRY: ghcr.io
-  RUST_VERSION_COV: nightly-2024-06-05
+  RUST_VERSION: 1.82.0
+  NIGHTLY_RUST_VERSION: nightly-2024-10-28
 
 jobs:
   cancel-previous-runs:
@@ -29,17 +30,17 @@ jobs:
     permissions: # Write access to push changes to pages
       contents: write
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - name: Install latest Rust
         uses: dtolnay/rust-toolchain@master
         with:
-          toolchain: ${{ env.RUST_VERSION_COV }}
+          toolchain: ${{ env.NIGHTLY_RUST_VERSION }}
 
       - name: Install cargo-llvm-codecov
         uses: taiki-e/install-action@cargo-llvm-cov
 
       - name: Code coverage report
-        run: cargo  +${{ env.RUST_VERSION_COV }} llvm-cov --all-features --lcov --branch --output-path lcov.info
+        run: cargo +${{ env.NIGHTLY_RUST_VERSION }} llvm-cov --all-features --lcov --branch --output-path lcov.info
 
       - name: Setup LCOV
         uses: hrishikesh-kadam/setup-lcov@v1

.github/workflows/markdown-lint.yml

@@ -13,8 +13,8 @@ jobs:
     name: Markdown Lint
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-node@v3
+      - uses: actions/checkout@v4
+      - uses: actions/setup-node@v4
         with:
           node-version: 18
       - run: |

.gitignore

@@ -1 +1,6 @@
+
+# IDE
+.vscode
+
+# Output
 /target

Cargo.lock

@@ -39,3 +39,6 @@ path = "src/lib.rs"
 [[bin]]
 name = "forc-wallet"
 path = "src/main.rs"
+
+[dev-dependencies]
+tempfile = "3.13.0"

Cargo.toml

@@ -0,0 +1,3 @@
+[toolchain]
+profile = "default" # include rustfmt, clippy
+channel = "1.82.0"

rust-toolchain.toml

@@ -453,18 +453,15 @@ pub(crate) fn derive_account(
     Ok(derive_account_unlocked(wallet_path, account_ix, password)?.lock())
 }
 
-fn new_at_index(
-    keystore: &EthKeystore,
-    wallet_path: &Path,
-    account_ix: usize,
-) -> Result<Bech32Address> {
+fn new_at_index(keystore: &EthKeystore, wallet_path: &Path, account_ix: usize) -> Result<String> {
     let prompt = format!("Please enter your wallet password to derive account {account_ix}: ");
     let password = rpassword::prompt_password(prompt)?;
     let account = derive_account(wallet_path, account_ix, &password)?;
     let account_addr = account.address();
     cache_address(&keystore.crypto.ciphertext, account_ix, account_addr)?;
-    println!("Wallet address: {account_addr}");
-    Ok(account_addr.clone())
+    let checksum_addr = checksum_encode(&Address::from(account_addr).to_string())?;
+    println!("Wallet address: {checksum_addr}");
+    Ok(checksum_addr)
 }
 
 pub fn new_at_index_cli(wallet_path: &Path, account_ix: usize) -> Result<()> {

src/account.rs

@@ -142,13 +142,21 @@ pub(crate) fn ensure_no_wallet_exists(
     force: bool,
     mut reader: impl BufRead,
 ) -> Result<()> {
-    if wallet_path.exists() {
+    let remove_wallet = || {
+        if wallet_path.is_dir() {
+            fs::remove_dir_all(wallet_path).unwrap();
+        } else {
+            fs::remove_file(wallet_path).unwrap();
+        }
+    };
+
+    if wallet_path.exists() && fs::metadata(wallet_path)?.len() > 0 {
         if force {
             println_warning(&format!(
                 "Because the `--force` argument was supplied, the wallet at {} will be removed.",
                 wallet_path.display(),
             ));
-            fs::remove_file(wallet_path).unwrap();
+            remove_wallet();
         } else {
             println_warning(&format!(
                 "There is an existing wallet at {}. \
@@ -158,7 +166,7 @@ pub(crate) fn ensure_no_wallet_exists(
             let mut need_replace = String::new();
             reader.read_line(&mut need_replace).unwrap();
             if need_replace.trim() == "y" {
-                fs::remove_file(wallet_path).unwrap();
+                remove_wallet();
             } else {
                 bail!(
                     "Failed to create a new wallet at {} \
@@ -174,32 +182,47 @@ pub(crate) fn ensure_no_wallet_exists(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::utils::test_utils::{with_tmp_dir, TEST_MNEMONIC, TEST_PASSWORD};
+    use crate::utils::test_utils::{TEST_MNEMONIC, TEST_PASSWORD};
     // simulate input
     const INPUT_NOP: &[u8; 1] = b"\n";
     const INPUT_YES: &[u8; 2] = b"y\n";
     const INPUT_NO: &[u8; 2] = b"n\n";
 
-    fn remove_wallet(wallet_path: &Path) {
-        if wallet_path.exists() {
-            fs::remove_file(wallet_path).unwrap();
-        }
+    /// Represents the possible serialized states of a wallet.
+    /// Used primarily for simulating wallet creation and serialization processes.
+    enum WalletSerializedState {
+        Empty,
+        WithData(String),
     }
-    fn create_wallet(wallet_path: &Path) {
+
+    /// Simulates the serialization of a wallet to a file, optionally including dummy data.
+    /// Primarily used to test if checks for wallet file existence are functioning correctly.
+    fn serialize_wallet_to_file(wallet_path: &Path, state: WalletSerializedState) {
+        // Create the wallet file if it does not exist.
         if !wallet_path.exists() {
             fs::File::create(wallet_path).unwrap();
         }
+
+        // Write content to the wallet file based on the specified state.
+        if let WalletSerializedState::WithData(data) = state {
+            fs::write(wallet_path, data).unwrap();
+        }
+    }
+
+    fn remove_wallet(wallet_path: &Path) {
+        if wallet_path.exists() {
+            fs::remove_file(wallet_path).unwrap();
+        }
     }
 
     #[test]
     fn handle_absolute_path_argument() {
-        with_tmp_dir(|tmp_dir| {
-            let tmp_dir_abs = tmp_dir.canonicalize().unwrap();
-            let wallet_path = tmp_dir_abs.join("wallet.json");
-            write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
-                .unwrap();
-            load_wallet(&wallet_path).unwrap();
-        })
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let tmp_dir_abs = tmp_dir.path().canonicalize().unwrap();
+        let wallet_path = tmp_dir_abs.join("wallet.json");
+        write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
+            .unwrap();
+        load_wallet(&wallet_path).unwrap();
     }
 
     #[test]
@@ -223,88 +246,113 @@ mod tests {
     }
     #[test]
     fn encrypt_and_save_phrase() {
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
-                .unwrap();
-            let phrase_recovered = eth_keystore::decrypt_key(wallet_path, TEST_PASSWORD).unwrap();
-            let phrase = String::from_utf8(phrase_recovered).unwrap();
-            assert_eq!(phrase, TEST_MNEMONIC)
-        });
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("wallet.json");
+        write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
+            .unwrap();
+        let phrase_recovered = eth_keystore::decrypt_key(wallet_path, TEST_PASSWORD).unwrap();
+        let phrase = String::from_utf8(phrase_recovered).unwrap();
+        assert_eq!(phrase, TEST_MNEMONIC)
     }
 
     #[test]
     fn write_wallet() {
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
-                .unwrap();
-            load_wallet(&wallet_path).unwrap();
-        })
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("wallet.json");
+        write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
+            .unwrap();
+        load_wallet(&wallet_path).unwrap();
     }
 
     #[test]
     #[should_panic]
     fn write_wallet_to_existing_file_should_fail() {
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
-                .unwrap();
-            write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
-                .unwrap();
-        })
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("wallet.json");
+        write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
+            .unwrap();
+        write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
+            .unwrap();
     }
 
     #[test]
     fn write_wallet_subdir() {
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("path").join("to").join("wallet");
-            write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
-                .unwrap();
-            load_wallet(&wallet_path).unwrap();
-        })
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("path").join("to").join("wallet.json");
+        write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
+            .unwrap();
+        load_wallet(&wallet_path).unwrap();
     }
 
     #[test]
     fn test_ensure_no_wallet_exists_no_wallet() {
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            remove_wallet(&wallet_path);
-            ensure_no_wallet_exists(&wallet_path, false, &INPUT_NOP[..]).unwrap();
-        });
-    }
-
-    #[test]
-    #[should_panic]
-    fn test_ensure_no_wallet_exists_throws_err() {
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            create_wallet(&wallet_path);
-            ensure_no_wallet_exists(&wallet_path, false, &INPUT_NO[..]).unwrap();
-        });
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("wallet.json");
+        remove_wallet(&wallet_path);
+        ensure_no_wallet_exists(&wallet_path, false, &INPUT_NOP[..]).unwrap();
     }
 
     #[test]
     fn test_ensure_no_wallet_exists_exists_wallet() {
         // case: wallet path exist without --force and input[yes]
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            create_wallet(&wallet_path);
-            ensure_no_wallet_exists(&wallet_path, false, &INPUT_YES[..]).unwrap();
-        });
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("wallet.json");
+        serialize_wallet_to_file(&wallet_path, WalletSerializedState::Empty);
+        ensure_no_wallet_exists(&wallet_path, false, &INPUT_YES[..]).unwrap();
+
         // case: wallet path exist with --force
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            create_wallet(&wallet_path);
-            ensure_no_wallet_exists(&wallet_path, true, &INPUT_NOP[..]).unwrap();
-        });
-        // case: wallet path exist without --force and supply a different wallet path
-        with_tmp_dir(|tmp_dir| {
-            let wallet_path = tmp_dir.join("wallet.json");
-            create_wallet(&wallet_path);
-            let diff_wallet_path = tmp_dir.join("custom-wallet.json");
-            ensure_no_wallet_exists(&diff_wallet_path, false, &INPUT_NOP[..]).unwrap();
-        });
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("empty_wallet.json");
+        serialize_wallet_to_file(&wallet_path, WalletSerializedState::Empty);
+
+        // Empty file should not trigger the replacement prompt
+        ensure_no_wallet_exists(&wallet_path, false, &INPUT_YES[..]).unwrap();
+        assert!(wallet_path.exists(), "Empty file should remain untouched");
+    }
+
+    #[test]
+    fn test_ensure_no_wallet_exists_nonempty_file() {
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("nonempty_wallet.json");
+
+        // Create non-empty file
+        serialize_wallet_to_file(
+            &wallet_path,
+            WalletSerializedState::WithData("some wallet content".to_string()),
+        );
+
+        // Test with --force flag
+        ensure_no_wallet_exists(&wallet_path, true, &INPUT_NO[..]).unwrap();
+        assert!(
+            !wallet_path.exists(),
+            "File should be removed with --force flag"
+        );
+
+        // Test with user confirmation (yes)
+        serialize_wallet_to_file(
+            &wallet_path,
+            WalletSerializedState::WithData("some wallet content".to_string()),
+        );
+        ensure_no_wallet_exists(&wallet_path, false, &INPUT_YES[..]).unwrap();
+        assert!(
+            !wallet_path.exists(),
+            "File should be removed after user confirmation"
+        );
+
+        // Test with user rejection (no)
+        serialize_wallet_to_file(
+            &wallet_path,
+            WalletSerializedState::WithData("some wallet content".to_string()),
+        );
+        let result = ensure_no_wallet_exists(&wallet_path, false, &INPUT_NO[..]);
+        assert!(
+            result.is_err(),
+            "Should error when user rejects file removal"
+        );
+        assert!(
+            wallet_path.exists(),
+            "File should remain when user rejects removal"
+        );
     }
 }
 
@@ -316,35 +364,15 @@ pub(crate) mod test_utils {
     pub(crate) const TEST_MNEMONIC: &str = "rapid mechanic escape victory bacon switch soda math embrace frozen novel document wait motor thrive ski addict ripple bid magnet horse merge brisk exile";
     pub(crate) const TEST_PASSWORD: &str = "1234";
 
-    /// Create a tmp folder and execute the given test function `f`
-    pub(crate) fn with_tmp_dir<F>(f: F)
-    where
-        F: FnOnce(&Path) + panic::UnwindSafe,
-    {
-        let tmp_dir_name = format!("forc-wallet-test-{:x}", rand::random::<u64>());
-        let tmp_dir = user_fuel_dir().join(".tmp").join(tmp_dir_name);
-        std::fs::create_dir_all(&tmp_dir).unwrap();
-        let panic = panic::catch_unwind(|| f(&tmp_dir));
-        std::fs::remove_dir_all(&tmp_dir).unwrap();
-        if let Err(e) = panic {
-            panic::resume_unwind(e);
-        }
-    }
-
-    /// Saves a default test mnemonic to the disk
-    pub(crate) fn save_dummy_wallet_file(wallet_path: &Path) {
-        write_wallet_from_mnemonic_and_password(wallet_path, TEST_MNEMONIC, TEST_PASSWORD).unwrap();
-    }
-
-    /// The same as `with_tmp_dir`, but also provides a test wallet.
+    /// Creates temp dir with a temp/test wallet.
     pub(crate) fn with_tmp_dir_and_wallet<F>(f: F)
     where
         F: FnOnce(&Path, &Path) + panic::UnwindSafe,
     {
-        with_tmp_dir(|dir| {
-            let wallet_path = dir.join("wallet.json");
-            save_dummy_wallet_file(&wallet_path);
-            f(dir, &wallet_path);
-        })
+        let tmp_dir = tempfile::TempDir::new().unwrap();
+        let wallet_path = tmp_dir.path().join("wallet.json");
+        write_wallet_from_mnemonic_and_password(&wallet_path, TEST_MNEMONIC, TEST_PASSWORD)
+            .unwrap();
+        f(tmp_dir.path(), &wallet_path);
     }
 }