From e0b0d8c993d484abf151230403dcd20648b57495 Mon Sep 17 00:00:00 2001 From: zees-dev Date: Wed, 30 Oct 2024 13:14:58 +1300 Subject: [PATCH] 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: https://github.com/FuelLabs/forc-wallet/issues/186 - added unit tests for `ensure_no_wallet_exists` --------- Co-authored-by: Kaya Gokalp --- .github/workflows/ci.yml | 9 +- .github/workflows/markdown-lint.yml | 4 +- .gitignore | 5 + Cargo.lock | 1 + Cargo.toml | 3 + rust-toolchain.toml | 3 + src/account.rs | 11 +- src/utils.rs | 230 ++++++++++++++++------------ 8 files changed, 152 insertions(+), 114 deletions(-) create mode 100644 rust-toolchain.toml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e6f58d2..3e3ba2e 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/.github/workflows/markdown-lint.yml b/.github/workflows/markdown-lint.yml index b4aa28e..1276bac 100644 --- a/.github/workflows/markdown-lint.yml +++ b/.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: | diff --git a/.gitignore b/.gitignore index ea8c4bf..c764265 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,6 @@ + +# IDE +.vscode + +# Output /target diff --git a/Cargo.lock b/Cargo.lock index 6521f42..1ec95b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -985,6 +985,7 @@ dependencies = [ "rand", "rpassword", "serde_json", + "tempfile", "termion", "tiny-bip39", "tokio", diff --git a/Cargo.toml b/Cargo.toml index 8c5988d..f0d6f83 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,3 +39,6 @@ path = "src/lib.rs" [[bin]] name = "forc-wallet" path = "src/main.rs" + +[dev-dependencies] +tempfile = "3.13.0" diff --git a/rust-toolchain.toml b/rust-toolchain.toml new file mode 100644 index 0000000..c69cff7 --- /dev/null +++ b/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +profile = "default" # include rustfmt, clippy +channel = "1.82.0" diff --git a/src/account.rs b/src/account.rs index 6a3d6a7..46b3f49 100644 --- a/src/account.rs +++ b/src/account.rs @@ -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 { +fn new_at_index(keystore: &EthKeystore, wallet_path: &Path, account_ix: usize) -> Result { 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<()> { diff --git a/src/utils.rs b/src/utils.rs index 562d825..f0f6e96 100644 --- a/src/utils.rs +++ b/src/utils.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) - where - F: FnOnce(&Path) + panic::UnwindSafe, - { - let tmp_dir_name = format!("forc-wallet-test-{:x}", rand::random::()); - 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) 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); } }