test: add ffi cucumber wallet balance test#7189
test: add ffi cucumber wallet balance test#7189SWvheerden merged 6 commits intotari-project:developmentfrom
Conversation
Added cucumber test to verify wallet balance with recovery into FFI wallet.
WalkthroughThe changes introduce new test scenarios and steps for wallet restoration and balance verification using FFI, update FFI function signatures to include an additional Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant TariWorld
participant Wallet
participant FFIWallet
TestRunner->>Wallet: Mine blocks, fund wallet
TestRunner->>Wallet: Remember wallet balance as "balance_key"
TestRunner->>FFIWallet: Recover from seed words
TestRunner->>FFIWallet: Wait for balance sync
TestRunner->>TariWorld: Retrieve remembered balance
FFIWallet->>TariWorld: Get current balance
TestRunner->>TestRunner: Assert FFIWallet balance == remembered balance
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
hansieodendaal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
integration_tests/src/wallet_ffi.rs (1)
236-250: Remove incomplete commented code.The commented-out alternative implementation has several issues (missing imports for
c_int, undefinedseed_words_push_wordfunction) and appears to be incomplete. Consider removing it to keep the codebase clean.-// pub fn create_seed_words(words: Vec<&str>) -> ffi::SeedWords { -// let mut error = 0; -// let error_ptr = &mut error as *mut c_int; -// let seed_words = ffi::SeedWords::create_empty_seed_words(); -// -// for word in words { -// let word = CString::new(word).unwrap(); -// let word_str: *const c_char = CString::into_raw(word) as *const c_char; -// unsafe { -// seed_words_push_word(seed_words.get_ptr(), word_str, ptr::null(), error_ptr), -// } -// } -// -// seed_words -// }integration_tests/tests/steps/wallet_steps.rs (2)
141-141: Remove debug print statements.The debug print statements appear to be temporary debugging code that should be removed from the production test code.
Apply this diff to clean up the debug statements:
- println!("remember_wallet_balance - 0");- println!("remember_wallet_balance - 1");- println!("remember_wallet_balance - 2: {:?}", balance);- println!("remember_wallet_balance - 3");Also applies to: 144-144, 150-150, 153-153
152-152: Remove commented code.The commented line should be removed to keep the codebase clean.
Apply this diff:
- // world.balance.insert(balance_key, GetBalanceResponse::default());integration_tests/tests/features/WalletFFI.feature (2)
36-37: Remove duplicate height check.Lines 36-37 contain duplicate height checks which are redundant and may cause confusion.
- Then all nodes are at height 12 - Then all nodes are at height 12 + Then all nodes are at height 12
40-40: Consider making balance expectations more resilient.The specific balance amount
184620500000 uTis quite precise and may become fragile if mining reward calculations change in the future. Consider using a more flexible approach or documenting why this exact amount is expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
integration_tests/src/ffi/ffi_import.rs(4 hunks)integration_tests/src/ffi/seed_words.rs(3 hunks)integration_tests/src/wallet_ffi.rs(1 hunks)integration_tests/src/world.rs(3 hunks)integration_tests/tests/features/WalletFFI.feature(2 hunks)integration_tests/tests/steps/wallet_ffi_steps.rs(3 hunks)integration_tests/tests/steps/wallet_steps.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (11)
integration_tests/src/world.rs (1)
31-31: LGTM! Well-structured addition of balance tracking.The new
balancefield follows the existing pattern in theTariWorldstruct, with proper import, declaration, and initialization. This addition supports the PR objective of testing wallet balance verification after recovery.Also applies to: 90-90, 133-133
integration_tests/src/ffi/seed_words.rs (3)
23-23: LGTM! Improved type safety and explicit imports.The changes improve type safety by using
*mut TariSeedWordsinstead of*mut c_voidand make imports more explicit. These are good practices for FFI code.Also applies to: 25-25, 28-28, 31-31
42-42: LGTM! Better method naming.The rename from
createtocreate_empty_seed_wordsmakes the method's purpose clearer and more descriptive.
100-102:⚠️ Potential issueFix potential memory leak in CString handling.
The current implementation has a potential issue with CString memory management. You're creating a CString, converting it to a raw pointer, but the original CString
wmight be dropped before the FFI call.Apply this fix to ensure proper memory management:
- let w = CString::new(word).unwrap(); - let w_str: *const c_char = CString::into_raw(w) as *const c_char; + let w_str = CString::new(word).unwrap().into_raw(); result = ffi_import::seed_words_push_word(self.ptr, w_str, ptr::null(), &mut error);This ensures the CString is properly converted to a raw pointer without potential premature deallocation.
Likely an incorrect or invalid review comment.
integration_tests/src/wallet_ffi.rs (1)
229-229: LGTM! Correct method call update.The function call correctly uses the renamed
create_empty_seed_words()method, maintaining consistency with the changes in the FFI layer.integration_tests/tests/steps/wallet_steps.rs (1)
135-154: LGTM! Utility function implementation is correct.The core functionality correctly implements a test utility to capture wallet balances for later verification in cucumber tests. The function properly:
- Retrieves the wallet process from the test world
- Gets a gRPC client and validates transactions
- Fetches the current balance
- Stores it in the world state for later comparison
This aligns well with the PR objective to add cucumber tests for wallet balance verification after recovery.
integration_tests/tests/features/WalletFFI.feature (1)
24-47: Well-structured test scenario for balance verification.The new test scenario comprehensively covers wallet restoration and balance verification, which aligns perfectly with the PR objectives. The setup includes proper network configuration with seed nodes, multiple wallets, and mining operations before testing the core functionality.
integration_tests/tests/steps/wallet_ffi_steps.rs (2)
152-168: Excellent balance verification implementation.The
ffi_has_balancestep is well-implemented with:
- Proper construction of
GetBalanceResponsefrom FFI wallet balance components- Clear assertion with detailed error messages for debugging
- Correct integration with the world's balance map
This enables robust verification that restored wallets maintain the same balance as the original.
555-555: Good fix for explicit type annotation.The explicit type annotation
Vec<&str>resolves potential type inference issues and makes the code more readable.integration_tests/src/ffi/ffi_import.rs (2)
420-420: Parameter naming improvements for clarity.The parameter renames (
log_level→log_verbosityandcallback_base_node_state_updated→callback_base_node_state) improve naming consistency and clarity.Also applies to: 470-470
204-209:Details
✅ Verification successful
Verify all callers use the updated function signature.
The
seed_words_push_wordfunction signature now includes an additionalpassphraseparameter. Ensure that all code calling this function has been updated to pass the new parameter.
🏁 Script executed:
#!/bin/bash # Description: Find all usages of seed_words_push_word to verify they match the new signature # Expected: All calls should include the passphrase parameter echo "Searching for seed_words_push_word function calls..." rg -A 3 "seed_words_push_word" --type rustLength of output: 3054
All
seed_words_push_wordcalls include the newpassphraseparameterA quick ripgrep search confirms that every invocation of
seed_words_push_word—in both the integration tests and thebase_layer/wallet_ffimodule—now passes apassphraseargument (ptr::null()), matching the updated FFI signature. No further updates are needed here.
Test Results (CI) 3 files 126 suites 41m 47s ⏱️ Results for commit e73fc38. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 10 suites +10 2h 45m 52s ⏱️ + 2h 45m 52s For more details on these failures, see this check. Results for commit e73fc38. ± Comparison against base commit 3de58ec. ♻️ This comment has been updated with latest results. |
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
Its looking good, but I think we should test the balance with tx as well as coinbases
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
integration_tests/tests/steps/mod.rs (1)
61-69: Improve error handling and consider performance optimizations.The logging function has several areas for improvement:
- Error handling: Using
unwrap()could cause panics if file operations fail- Performance: Opening/closing the file for each log call is inefficient
- Thread safety: No synchronization for concurrent access from multiple test steps
Consider this improved implementation:
+use std::sync::Mutex; +use std::fs::OpenOptions; +use std::path::PathBuf; +use std::io::Write; + +static LOG_FILE: Mutex<Option<std::fs::File>> = Mutex::new(None); + pub fn cucumber_steps_log<S: AsRef<str>>(log_message: S) { - let log_file = PathBuf::from("./log/steps.log"); - let mut file = OpenOptions::new() - .create(true) - .append(true) - .open(log_file) - .unwrap(); - writeln!(file, "{}", log_message.as_ref()).unwrap(); + let mut file_guard = LOG_FILE.lock().unwrap(); + if file_guard.is_none() { + let log_file = PathBuf::from("./log/steps.log"); + if let Some(parent) = log_file.parent() { + let _ = std::fs::create_dir_all(parent); + } + *file_guard = OpenOptions::new() + .create(true) + .append(true) + .open(log_file) + .ok(); + } + if let Some(ref mut file) = *file_guard { + let _ = writeln!(file, "{}", log_message.as_ref()); + let _ = file.flush(); + } }This approach:
- Uses a static mutex for thread safety
- Keeps the file open across calls
- Creates the log directory if it doesn't exist
- Gracefully handles errors without panicking
integration_tests/tests/steps/wallet_steps.rs (1)
893-998: Consider using structured logging for debug statements.While the addition of debug logging is helpful, consider using the existing logging framework consistently throughout the function.
- cucumber_steps_log(" - here 1"); + log::debug!(target: LOG_TARGET, "Creating payment recipient for interactive transaction"); let payment_recipient = PaymentRecipient { // ... payment details ... }; - cucumber_steps_log(" - here 2"); + log::debug!(target: LOG_TARGET, "Sending transfer request"); let tx_res = sender_wallet_client.transfer(transfer_req).await.unwrap().into_inner(); - cucumber_steps_log(" - here 3"); + log::debug!(target: LOG_TARGET, "Transfer request completed"); let tx_res = tx_res.results; - cucumber_steps_log(format!("Transaction results: {:?}", tx_res)); + log::debug!(target: LOG_TARGET, "Transaction results: {:?}", tx_res);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)applications/minotari_console_wallet/src/lib.rs(1 hunks)integration_tests/Cargo.toml(1 hunks)integration_tests/src/wallet_process.rs(3 hunks)integration_tests/src/world.rs(3 hunks)integration_tests/tests/features/BlockTemplate.feature(1 hunks)integration_tests/tests/features/StressTest.feature(2 hunks)integration_tests/tests/features/TransactionInfo.feature(1 hunks)integration_tests/tests/features/WalletFFI.feature(10 hunks)integration_tests/tests/features/WalletMonitoring.feature(2 hunks)integration_tests/tests/features/WalletQuery.feature(1 hunks)integration_tests/tests/features/WalletRecovery.feature(3 hunks)integration_tests/tests/features/WalletRoutingMechanism.feature(1 hunks)integration_tests/tests/features/WalletTransactions.feature(14 hunks)integration_tests/tests/features/WalletTransfer.feature(2 hunks)integration_tests/tests/steps/mod.rs(2 hunks)integration_tests/tests/steps/wallet_cli_steps.rs(4 hunks)integration_tests/tests/steps/wallet_steps.rs(19 hunks)
✅ Files skipped from review due to trivial changes (9)
- integration_tests/Cargo.toml
- integration_tests/tests/features/TransactionInfo.feature
- integration_tests/tests/features/BlockTemplate.feature
- integration_tests/tests/features/WalletMonitoring.feature
- integration_tests/tests/features/WalletRecovery.feature
- integration_tests/tests/features/WalletTransfer.feature
- integration_tests/tests/features/WalletRoutingMechanism.feature
- applications/minotari_console_wallet/src/lib.rs
- integration_tests/tests/features/StressTest.feature
🚧 Files skipped from review as they are similar to previous changes (2)
- integration_tests/tests/features/WalletFFI.feature
- integration_tests/src/world.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (9)
integration_tests/tests/steps/mod.rs (1)
23-25: LGTM!The imports are appropriate for the file logging functionality being added.
integration_tests/tests/features/WalletQuery.feature (1)
24-24: LGTM!The change to explicitly specify "one-sided" transaction type improves test clarity and aligns with the broader effort to standardize transaction semantics across the test suite.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
833-836: LGTM!The enhanced error logging with transaction ID improves diagnostic capabilities. This will help with debugging when transactions are not found after being sent, providing more context for troubleshooting.
integration_tests/tests/features/WalletTransactions.feature (2)
19-20: LGTM!The systematic updates to explicitly specify transaction types ("one-sided transaction", "interactive transaction", "one-sided stealth transaction") significantly improve test clarity and documentation. These changes align with the broader effort to standardize transaction semantics across the test suite without altering the underlying test logic.
Also applies to: 25-25, 31-31, 49-50, 55-55, 61-61, 77-77, 112-112, 116-116, 139-139, 180-180, 188-188, 202-206
233-233: LGTM!The updates to commented scenarios also correctly reflect the explicit transaction type terminology, maintaining consistency throughout the file.
Also applies to: 251-251, 279-279, 313-313, 331-331, 358-358, 363-363, 404-404
integration_tests/src/wallet_process.rs (1)
52-53: LGTM!The addition of
base_node_nameandpeer_seedsfields toWalletProcessis well-implemented. The fields are properly initialized and used to enhance wallet configuration for test scenarios.Also applies to: 104-113, 197-197, 199-199
integration_tests/tests/steps/wallet_cli_steps.rs (1)
410-480: Well-structured wallet recovery implementations!The export and recovery functions for view/spend keys are properly implemented with good error handling and clear JSON parsing logic. The comment showing the expected JSON format is helpful for maintainability.
integration_tests/tests/steps/wallet_steps.rs (2)
723-791: Consistent updates to transaction types.Good job on consistently updating all transaction functions to explicitly specify the transaction type. The changes clearly distinguish between one-sided (
payment_type: 1) and interactive (payment_type: 0) transactions, which aligns well with the PR objectives.Also applies to: 789-891, 1342-1441, 1497-1592, 1688-1836, 2195-2310, 2731-2856
2976-2990: Well-implemented balance verification step!The
wallet_has_balancefunction properly retrieves the stored balance and compares it with the current wallet balance, providing clear error messages on mismatch.
hansieodendaal
force-pushed
the
ho_verify_balance
branch
from
09eef5e to
5abb9ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
integration_tests/tests/steps/wallet_cli_steps.rs (1)
368-372: Address the previous review comment about error handling.This code segment still lacks proper error handling for file operations as mentioned in a previous review comment. The seed words file operations can fail if the file is missing or unreadable.
Consider applying the previously suggested error handling improvement to make the code more robust.
🧹 Nitpick comments (3)
integration_tests/tests/steps/wallet_cli_steps.rs (3)
373-378: Simplify seed words parsing logic.The current chain of transformations can be simplified and made more readable.
- let saved_seed_words = line - .split_whitespace() - .collect::<Vec<_>>() - .into_iter() - .map(|v| v.to_string()) - .collect::<Vec<_>>(); + let saved_seed_words: Vec<String> = line + .split_whitespace() + .map(|word| word.to_string()) + .collect();
392-396: Refactor SeedWords construction for better readability.The seed words construction can be simplified.
- let mut seed_words = SeedWords::new(vec![]); - for word in &saved_seed_words { - seed_words.push(word.to_string()); - } + let seed_words = SeedWords::new(saved_seed_words);
356-408: Consider extracting common wallet management patterns.All three functions follow a similar pattern of killing wallets, sleeping, setting up CLI, and spawning wallets. This could be refactored into helper functions to reduce code duplication.
Consider creating helper functions like:
kill_wallet_and_wait(world: &mut TariWorld, wallet_name: &str)setup_base_node_cli(world: &TariWorld, node: &str) -> CliArgsspawn_wallet_with_cli(world: &mut TariWorld, wallet_name: String, node: String, cli: CliArgs)Also applies to: 410-443, 445-480
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)applications/minotari_console_wallet/src/lib.rs(1 hunks)integration_tests/Cargo.toml(1 hunks)integration_tests/src/wallet_ffi.rs(1 hunks)integration_tests/src/wallet_process.rs(3 hunks)integration_tests/src/world.rs(3 hunks)integration_tests/tests/features/BlockTemplate.feature(1 hunks)integration_tests/tests/features/StressTest.feature(2 hunks)integration_tests/tests/features/TransactionInfo.feature(1 hunks)integration_tests/tests/features/WalletFFI.feature(10 hunks)integration_tests/tests/features/WalletMonitoring.feature(2 hunks)integration_tests/tests/features/WalletQuery.feature(1 hunks)integration_tests/tests/features/WalletRecovery.feature(3 hunks)integration_tests/tests/features/WalletRoutingMechanism.feature(1 hunks)integration_tests/tests/features/WalletTransactions.feature(14 hunks)integration_tests/tests/features/WalletTransfer.feature(2 hunks)integration_tests/tests/steps/mod.rs(2 hunks)integration_tests/tests/steps/wallet_cli_steps.rs(4 hunks)integration_tests/tests/steps/wallet_steps.rs(19 hunks)
✅ Files skipped from review due to trivial changes (5)
- integration_tests/Cargo.toml
- integration_tests/tests/features/BlockTemplate.feature
- integration_tests/tests/features/WalletMonitoring.feature
- integration_tests/tests/features/WalletRoutingMechanism.feature
- integration_tests/tests/features/WalletTransactions.feature
🚧 Files skipped from review as they are similar to previous changes (13)
- integration_tests/tests/features/WalletQuery.feature
- integration_tests/tests/features/WalletTransfer.feature
- integration_tests/src/wallet_ffi.rs
- integration_tests/tests/features/WalletRecovery.feature
- integration_tests/tests/features/TransactionInfo.feature
- integration_tests/tests/steps/mod.rs
- integration_tests/src/world.rs
- integration_tests/tests/features/StressTest.feature
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
- integration_tests/src/wallet_process.rs
- applications/minotari_console_wallet/src/lib.rs
- integration_tests/tests/features/WalletFFI.feature
- integration_tests/tests/steps/wallet_steps.rs
🧰 Additional context used
🪛 Gitleaks (8.26.0)
integration_tests/tests/steps/wallet_cli_steps.rs
459-459: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: ci
🔇 Additional comments (2)
integration_tests/tests/steps/wallet_cli_steps.rs (2)
23-23: LGTM: Import additions support the new functionality.The new imports are necessary for the added wallet recovery and key management functionality.
Also applies to: 34-34, 47-47
420-422: 🛠️ Refactor suggestionReplace panic with proper error handling.
Using
panic!in test code can make debugging more difficult. Consider using a more graceful error handling approach.- } else { - panic!("Wallet '{}' not found", wallet_name); - }; + } else { + eprintln!("Wallet '{}' not found", wallet_name); + return; + };Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration_tests/tests/steps/mod.rs (1)
64-69: Consider ensuring log directory exists before writing.The logging function is well-implemented but could be more robust by ensuring the log directory exists before attempting to write to it.
pub fn cucumber_steps_log<S: AsRef<str>>(log_message: S) { + let log_dir = PathBuf::from("./log"); + if !log_dir.exists() { + std::fs::create_dir_all(&log_dir).unwrap(); + } let log_file = PathBuf::from("./log/steps.log"); let mut file = OpenOptions::new().create(true).append(true).open(log_file).unwrap(); let timestamp = Local::now().format("%Y-%m-%d %H:%M:%S").to_string(); writeln!(file, "[{}] {}", timestamp, log_message.as_ref()).unwrap(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
integration_tests/tests/features/WalletFFI.feature(10 hunks)integration_tests/tests/steps/mod.rs(2 hunks)integration_tests/tests/steps/wallet_cli_steps.rs(3 hunks)integration_tests/tests/steps/wallet_ffi_steps.rs(13 hunks)integration_tests/tests/steps/wallet_steps.rs(77 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- integration_tests/tests/features/WalletFFI.feature
- integration_tests/tests/steps/wallet_ffi_steps.rs
- integration_tests/tests/steps/wallet_steps.rs
🧰 Additional context used
🧠 Learnings (1)
integration_tests/tests/steps/wallet_cli_steps.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7189
File: integration_tests/tests/steps/wallet_cli_steps.rs:464-467
Timestamp: 2025-06-07T06:49:32.152Z
Learning: In integration test code, using panic! for setup validation and prerequisite checking is acceptable and often preferred, as it provides immediate feedback when test scenarios are incorrectly configured rather than allowing tests to continue in invalid states.
🪛 Gitleaks (8.26.0)
integration_tests/tests/steps/wallet_cli_steps.rs
450-450: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (6)
integration_tests/tests/steps/mod.rs (2)
23-30: LGTM! New imports support enhanced test functionality.The additional imports for file operations, I/O, and chrono are well-organized and directly support the new logging and seed word retrieval functionality.
71-89: LGTM! Appropriate error handling for integration test context.The function correctly uses
panic!for setup validation, which provides immediate feedback when test prerequisites aren't met. The error messages are descriptive and will help with debugging test scenario issues.integration_tests/tests/steps/wallet_cli_steps.rs (4)
34-50: LGTM! New imports support wallet recovery test functionality.The imports for
ExportViewKeyAndSpendKeyArgs,SeedWords, andget_saved_seed_wordsare appropriately added to support the new CLI-based wallet recovery and key management test steps.
358-399: LGTM! Well-implemented wallet recovery test step.The function correctly:
- Handles existing wallet cleanup with conditional killing
- Sets up CLI arguments for recovery mode with base node configuration
- Uses the shared
get_saved_seed_wordsutility for consistent seed word retrieval- Properly spawns the recovered wallet with appropriate parameters
The logic flow is clear and follows the established patterns in the test suite.
401-434: LGTM! Proper implementation of key export functionality.The function correctly:
- Handles wallet process management with conditional killing
- Sets up CLI arguments for key export with appropriate output file path
- Stores the output file path in the world state for later retrieval
- Uses appropriate panic for missing wallet (correct for test context)
The implementation follows established patterns and integrates well with the test infrastructure.
436-476: LGTM! Comprehensive view wallet creation from keys.The function is well-implemented with:
- Proper wallet cleanup and removal from world state
- Clear JSON structure documentation in comments
- Appropriate error handling using panic for test validation
- Correct CLI argument setup for view and spend keys
The static analysis hint about "Generic API Key" on line 450 is a false positive - it's just a comment documenting the expected JSON format.
🧰 Tools
🪛 Gitleaks (8.26.0)
450-450: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
* development: (607 commits) Wallet GRPC port comment fix from 18142 to 18143 (tari-project#7221) feat: integrated address support for Ledger (tari-project#7198) chore: new release v4.1.1-pre.0 (tari-project#7211) fix: migration can now correctly resume after stopping (tari-project#7210) fix: only revalidated rejected transactions on startup (tari-project#7209) fix: add filtering flag back (tari-project#7208) feat: improve wallet balance checks from external clients (tari-project#7207) feat!: update grpc supply query (tari-project#7137) docs: Updated API GRPC and Exchange Guide (tari-project#7205) chore: new release v4.4.0-pre.0 (tari-project#7202) feat: update base node proto to search bytes (tari-project#7201) feat: full PayRef implementation (tari-project#7154) test: add ffi cucumber wallet balance test (tari-project#7189) chore: fix tests (tari-project#7196) fix(network-discovery): add back idle event handling (tari-project#7194) Update SECURITY.md (tari-project#7193) fix: transaction manager service unmined lookup (tari-project#7192) fix: wallet ffi database name mismatch for mobile wallet (tari-project#7191) fix: payment_id deserialize (tari-project#7187) fix: remove code for deleting stale peers (tari-project#7184) ...
2 participants
Description
As a client I want to be able to restore my ffi wallet from seed wordscucumber test.Motivation and Context
Trying to solve various balance query inconsistencies.
How Has This Been Tested?
Cucumber test:
As a client I want to be able to check my balance from restored walletsWhat process can a PR reviewer use to test or verify this change?
Code review
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests