feat: improve wallet balance checks from external clients#7207
feat: improve wallet balance checks from external clients#7207SWvheerden merged 11 commits intotari-project:developmentfrom
Conversation
Added cucumber test to verify wallet balance with recovery into FFI wallet.
- Fixed grpc balance and state methods not being updated due to event monitor not being started. - Fixed simualtaneous execution of recovery and scanning tasks during recovery.
…lance # Conflicts: # integration_tests/tests/features/WalletFFI.feature # integration_tests/tests/steps/wallet_steps.rs
WalkthroughThis update introduces several improvements and refactors across the wallet, base node, and integration testing components. Key changes include enhanced state management and event monitoring in wallet services, conditional compilation for base node-specific features, asynchronous refactoring for recovery and scanning logic, improved logging, and a more robust port assignment mechanism in integration tests. Test steps and scenarios are updated for consistency and reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletGrpcServer
participant WalletDebouncer
Client->>WalletGrpcServer: start_balance_debouncer_event_monitor()
WalletGrpcServer->>WalletDebouncer: start_event_monitor_if_needed()
alt Event monitor not started
WalletDebouncer->>WalletDebouncer: Spawn event monitor task
WalletDebouncer->>WalletDebouncer: Set event_monitor_started = true
else Event monitor already started
WalletDebouncer-->>WalletGrpcServer: Do nothing
end
WalletGrpcServer-->>Client: Monitor started or already running
sequenceDiagram
participant UtxoScannerService
participant BaseNode
participant Database
loop Service Running
UtxoScannerService->>BaseNode: Wait for connection
alt Connected
BaseNode-->>UtxoScannerService: StateChanged or NewBlockDetected
UtxoScannerService->>Database: check_recovery_mode()
alt Not in recovery
UtxoScannerService->>UtxoScannerService: should_scan(new_block_hash)
alt Should scan
UtxoScannerService->>UtxoScannerService: Update block_tip_to_scan_to
UtxoScannerService->>UtxoScannerService: Trigger scanning task
end
end
else Disconnected
UtxoScannerService->>UtxoScannerService: Suspend scanning
end
end
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 (
|
Test Results (CI) 3 files 126 suites 1h 11m 0s ⏱️ For more details on these failures, see this check. Results for commit f3550cc. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (3)
base_layer/core/src/common/waiting_requests.rs (1)
52-65: 💡 Verification agent🧩 Analysis chain
insert/removevanish whenbase_nodeis disabled – downstream code may breakBoth public async helpers are now hidden behind
#[cfg(feature = "base_node")].
Any crate ‑ for example the wallet-only binaries – that still depend onWaitingRequests::insert()orremove()will no longer compile when built without the feature flag.A minimal fallback (even a
#[cfg(not(feature = "base_node"))] panic!()stub) would keep the API surface consistent and avoid a feature-gate ripple effect.+#cfg(not(feature = "base_node")] +#[allow(dead_code)] +impl<T> WaitingRequests<T> { + pub async fn insert(&self, _key: RequestKey, _reply_tx: OneshotSender<T>) { + unreachable!("WaitingRequests::insert is only available with the `base_node` feature enabled") + } + + pub async fn remove(&self, _key: RequestKey) -> Option<(OneshotSender<T>, Instant)> { + unreachable!("WaitingRequests::remove is only available with the `base_node` feature enabled") + } +}Please double-check that all non-
base_nodetargets are green after this change.
If none of them actually call these methods, consider making the whole type#[cfg(feature = "base_node")]instead.
🏁 Script executed:
#!/bin/bash # Search for WaitingRequests usage in the codebase rg -n 'WaitingRequests' .Length of output: 1389
🏁 Script executed:
#!/bin/bash # Check for calls to WaitingRequests::insert or ::remove outside the base_node feature rg -n 'waiting_requests\.insert' . rg -n 'waiting_requests\.remove' .Length of output: 472
🏁 Script executed:
#!/bin/bash # Inspect the top of service.rs to see if it's behind a cfg(feature = "base_node") guard sed -n '1,50p' base_layer/core/src/base_node/service/service.rs # Check where base_node module is declared in the core crate and if it's feature-gated rg -n 'mod base_node' -n 'cfg.*base_node' base_layer/core/src/lib.rsLength of output: 2563
Add non-
base_nodestubs forinsert/removeto avoid breakageThe calls to
waiting_requests.insert(…)and.remove(…)in
base_layer/core/src/base_node/service/service.rs(lines 529, 601, 676) are unconditionally compiled, but the methods themselves are only present when thebase_nodefeature is enabled. This will cause compilation errors in any build withdefault-features = false.You should provide fallback implementations when
base_nodeis disabled, for example:--- a/base_layer/core/src/common/waiting_requests.rs +++ b/base_layer/core/src/common/waiting_requests.rs @@ #[cfg(feature = "base_node")] pub async fn insert(&self, key: RequestKey, reply_tx: OneshotSender<T>) { self.requests .write() .await .insert(key, Some((reply_tx, Instant::now()))); } + + #[cfg(not(feature = "base_node"))] + #[allow(dead_code)] + pub async fn insert(&self, _key: RequestKey, _reply_tx: OneshotSender<T>) { + unreachable!("`WaitingRequests::insert` requires the `base_node` feature"); + } @@ #[cfg(feature = "base_node")] pub async fn remove(&self, key: RequestKey) -> Option<(OneshotSender<T>, Instant)> { self.requests.write().await.remove(&key).unwrap_or(None) } + + #[cfg(not(feature = "base_node"))] + #[allow(dead_code)] + pub async fn remove(&self, _key: RequestKey) -> Option<(OneshotSender<T>, Instant)> { + unreachable!("`WaitingRequests::remove` requires the `base_node` feature"); + }Alternatively, if nothing outside
base_nodeever usesWaitingRequests, you could gate the entire type and its callers behind#[cfg(feature = "base_node")]instead.base_layer/core/src/transactions/transaction_components/transaction_output.rs (2)
243-302: 🛠️ Refactor suggestion
⚠️ Potential issue
verify_range_proof()now hidden, but test and external callers still use itThe whole verification path is guarded by
#[cfg(feature = "base_node")].
However:
- The integration tests in this same file (
#[tokio::test]) calltx_output.verify_range_proofunconditionally.- Other crates (wallet, validator, etc.) may rely on the method when compiled without the
base_nodefeature.This leads to compilation failures for
cargo test --no-default-featuresor any binary that disables the flag.Action items:
• Either gate the test module and all external uses with the same feature flag,
• or leave the function available for all builds and internallycfgonly the heavy base-node specific branches.
564-607:⚠️ Potential issue
batch_verify_range_proofshidden behind feature flag but re-exported insuper
super::batch_verify_range_proofsis imported in the test module without a matchingcfg. Same compilation issue as above.If the function truly is base-node-only, gate the test module:
-#[cfg(test)] +#[cfg(all(test, feature = "base_node"))] mod test { ... }or expose a stub for non-base_node builds that simply returns
Ok(()).
♻️ Duplicate comments (1)
integration_tests/src/base_node_process.rs (1)
116-118: Sameunwrap()panic risk as in wallet spawningReplicate the defensive‐coding improvement suggested for
wallet_process.rs
to avoid random CI panics when no port is free.
🧹 Nitpick comments (16)
base_layer/core/src/transactions/transaction_components/transaction_output.rs (1)
46-57: Duplicate / inconsistent imports – minor cleanup
extended_range_proof::ExtendedRangeProofServiceis imported unconditionally, whileextended_range_proof::Statementis gated behindbase_node.
IfExtendedRangeProofServiceis also only used inside the newly-gated code, the unconditional import will become “unused” for non-base_nodebuilds and trigger warnings.Either gate both, or keep both unconditional.
- use tari_crypto::{ - commitment::HomomorphicCommitmentFactory, - errors::RangeProofError, - extended_range_proof::ExtendedRangeProofService, - tari_utilities::hex::Hex, - }; +#[cfg(feature = "base_node")] +use tari_crypto::extended_range_proof::ExtendedRangeProofService; + +use tari_crypto::{ + commitment::HomomorphicCommitmentFactory, + errors::RangeProofError, + tari_utilities::hex::Hex, +};integration_tests/src/world.rs (1)
113-154:assigned_portsshould be aHashSet<u64>– keys & values are identical
IndexMap<u64, u64>stores the port number twice, wastes memory and
communicates no ordering requirements. A simpleHashSet<u64>expresses the
intent (“membership”) and reduces copy-paste errors.- pub assigned_ports: IndexMap<u64, u64>, + pub assigned_ports: HashSet<u64>,Adjust
get_portaccordingly:-if !world.assigned_ports.contains_key(&u64::from(port)) { - break port; -} +if !world.assigned_ports.contains(&u64::from(port)) { + break port; +} ... -world.assigned_ports.insert(u64::from(port), u64::from(port)); +world.assigned_ports.insert(u64::from(port));base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs (2)
23-24: Redundantstd::fmt::DebugimportThe trait is only referenced fully-qualified (
std::fmt::Formatter) in the custom impl and by derives, so thisuseis unused and will raise a warning withrustc -D warnings.
Please remove it.
57-64: CustomDebugimpl duplicates information – could derive and wrapIf the only aim is to rename the variants for log readability, consider composing rather than fully re-implementing:
-impl Debug for UtxoScannerMode { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - UtxoScannerMode::Recovery => write!(f, "UtxoRecoveryMode"), - UtxoScannerMode::Scanning => write!(f, "UtxoScanningMode"), - } - } -} +impl Debug for UtxoScannerMode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let renamed = match self { + UtxoScannerMode::Recovery => "UtxoRecoveryMode", + UtxoScannerMode::Scanning => "UtxoScanningMode", + }; + f.write_str(renamed) + } +}Slightly shorter and avoids repeating
write!boilerplate.applications/minotari_console_wallet/src/wallet_modes.rs (1)
28-34: Several newly added imports are unused
TryFutureExt,Wallet,GetBalanceRequest, andRequestare not referenced anywhere in this file after the change, which will trigger compiler warnings. Remove or gate them behind#[cfg(feature = "grpc")]if they are conditionally used elsewhere.Also applies to: 47-48
integration_tests/src/lib.rs (1)
43-55: Potential infinite loop if every port is occupied
get_portloops forever if no free port exists in the given range. In CI environments with port exhaustion this can hang tests indefinitely. Consider capping attempts and returningNoneafter N tries, letting the caller decide how to proceed.integration_tests/tests/steps/wallet_ffi_steps.rs (1)
88-90: Log message is misleading
The line writes “Adding wallet” while the function is actually stopping / destroying it.- cucumber_steps_log(format!("Adding wallet {}", wallet)); + cucumber_steps_log(format!("Stopping wallet {}", wallet));integration_tests/src/wallet_ffi.rs (1)
64-78: Clean separation of log & data paths – LGTMIntroducing
log_pathand keeping it in the struct simplifies restarts and avoids duplicate path construction.⚠️ Minor: the range18000..18499is upper-exclusive, so 18499 is never used—document this or switch to..=.applications/minotari_console_wallet/src/grpc/wallet_debouncer.rs (1)
87-97: Idempotent monitor start – nice, but could still race
start_event_monitor_if_neededworks because callers hold&mut self, yet settingevent_monitor_startedafter spawning leaves a tiny window for double-spawning if a future refactor allows concurrent callers. Atomically swapping the flag beforetokio::spawn(e.g.,if let Ok(mut flag) = self.event_monitor_started.try_lock() { … }) would close the gap.integration_tests/tests/steps/wallet_steps.rs (2)
851-858: Typo in function name (dest_wallt) will bite compile-time search tools
send_one_sided_transaction_from_source_wallet_to_dest_walltcontains a misspelling (wallt). While the compiler doesn’t care, grep-based tooling and readers do.-async fn send_one_sided_transaction_from_source_wallet_to_dest_wallt( +async fn send_one_sided_transaction_from_source_wallet_to_dest_wallet(Refactor before the typo spreads to step definitions and docs.
308-310: Repeatedunwrap()onget_wallet_addressobscures context on failureEvery call to
world.get_wallet_address(..).await.unwrap()will panic with a cryptic “calledOption::unwrap()on aNonevalue” which doesn’t indicate which wallet was missing.Replace with
expect(or a small helper) to surface the wallet name:-let wallet_address = world.get_wallet_address(&wallet_name).await.unwrap(); +let wallet_address = world + .get_wallet_address(&wallet_name) + .await + .expect(&format!("Address for wallet '{}' not found", wallet_name));Same pattern occurs in the highlighted lines – worth a quick
rg 'unwrap()'sweep.Also applies to: 383-385, 430-432, 470-472, 860-864, 2316-2319, 2850-2853, 3011-3014
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (3)
173-177: Logging placeholders look suspicious
debug!uses three{}-style placeholders but only two unique values (final_heighttwice).
This will compile, yet the message is confusing – it prints the same number in both “Progress(..)” and “Completed(..)”.- "{:?}: Published events 'UtxoScannerEvent::Progress(..{})' and 'UtxoScannerEvent::Completed(..{})'" + "{:?}: Published events 'UtxoScannerEvent::Progress(..{:#})' and 'UtxoScannerEvent::Completed(..{:#})'"Consider either removing the duplicated placeholder or inserting the correct values.
291-299: Add parentheses for clarity around mixed||/&&expressionThe precedence rules make the current logic correct, but the intent is easy to mis-read:
if self.shutdown_signal.is_triggered() || (self.check_recovery_mode().await? && self.mode != UtxoScannerMode::Recovery)Explicit grouping prevents accidental regressions when this line is edited later.
703-721: Async wrappers without awaits introduce needless overhead
set_recovery_mode,check_recovery_mode, andclear_recovery_modeare nowasync, yet they perform only synchronous DB calls and contain no.await.
This:
- Allocates a Future each call for no benefit.
- Hides the fact that the operations are instantaneous, encouraging careless
.await-spamming in call-sites.Unless these functions will become truly asynchronous, revert them to sync variants and drop the
.awaitat call-sites.base_layer/wallet/src/utxo_scanner_service/service.rs (2)
285-307:should_scanlogic is hard to reason aboutThe nested
Optioncomparisons coupled with manual flags make the intent opaque and error-prone.A clearer expression:
matches!( (self.last_block_tip_scanned, self.block_tip_to_scan_to), (Some(last), Some(target)) if last != new_hash && target != new_hash ) || self.last_block_tip_scanned.is_none() && self.block_tip_to_scan_to != Some(new_hash)At minimum, add unit tests documenting the expected truth table.
285-292: Duplicate recovery-mode DB probeBoth this service and
UtxoScannerTaskimplement their owncheck_recovery_mode, duplicating code and increasing drift risk.
Extract a single helper (e.g.resources.is_recovery_active()) and call it from both sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
applications/minotari_console_wallet/src/grpc/wallet_debouncer.rs(5 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(1 hunks)applications/minotari_console_wallet/src/recovery.rs(2 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(3 hunks)base_layer/core/src/common/waiting_requests.rs(2 hunks)base_layer/core/src/transactions/transaction_components/transaction_output.rs(4 hunks)base_layer/wallet/src/utxo_scanner_service/service.rs(8 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(21 hunks)base_layer/wallet/src/utxo_scanner_service/uxto_scanner_service_builder.rs(2 hunks)integration_tests/README.md(1 hunks)integration_tests/src/base_node_process.rs(1 hunks)integration_tests/src/lib.rs(1 hunks)integration_tests/src/merge_mining_proxy.rs(1 hunks)integration_tests/src/wallet_ffi.rs(3 hunks)integration_tests/src/wallet_process.rs(1 hunks)integration_tests/src/world.rs(2 hunks)integration_tests/tests/features/WalletFFI.feature(2 hunks)integration_tests/tests/steps/wallet_ffi_steps.rs(2 hunks)integration_tests/tests/steps/wallet_steps.rs(17 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
integration_tests/src/base_node_process.rs (1)
integration_tests/src/lib.rs (1)
get_port(39-56)
integration_tests/src/merge_mining_proxy.rs (1)
integration_tests/src/lib.rs (1)
get_port(39-56)
integration_tests/tests/steps/wallet_ffi_steps.rs (2)
base_layer/common_types/src/tari_address/mod.rs (1)
from_emoji_string(273-277)integration_tests/src/ffi/wallet_address.rs (1)
emoji_id(85-96)
integration_tests/tests/steps/wallet_steps.rs (3)
integration_tests/src/merge_mining_proxy.rs (1)
create_wallet_client(220-227)integration_tests/tests/steps/mod.rs (1)
cucumber_steps_log(64-69)base_layer/core/src/transactions/transaction_components/encrypted_data.rs (1)
open_from_string(517-522)
🔇 Additional comments (7)
integration_tests/README.md (1)
7-7: LGTM – doc tweak onlyNightly toolchain date updated; no further action required.
integration_tests/src/merge_mining_proxy.rs (1)
55-61:unwrap()will panic on the extremely rareNonepath – prefer graceful error handling
get_portreturns anOption<u64>. Although the current implementation loops until it succeeds, defensive code in integration infrastructure should avoid panicking outright:- port: get_port(world, 18000..18499).unwrap(), + port: get_port(world, 18000..18499) + .expect("Failed to acquire an available port in range 18000-18499"),Using
expectwith a descriptive message keeps the intent clear and prevents an opaque panic.applications/minotari_console_wallet/src/recovery.rs (2)
140-150: Good logging enhancementReplacing bare
println!calls with formattedmsglogged atdebug!level improves observability without altering functionality.
210-213: 👍 Added debug on stream closeThe additional log on
RecvError::Closedwill help diagnose premature channel shutdowns during recovery.integration_tests/tests/features/WalletFFI.feature (1)
46-56: Solid step ordering to avoid double-spend race conditionsExplicitly stopping
OTHER_WALLET(l.47) andMY_WALLET(l.51) before beginning seed-word-based restores makes the scenario deterministic and prevents two live instances from touching the same data directory. Nice catch.integration_tests/tests/steps/wallet_ffi_steps.rs (1)
78-81: Semantic check beats length checkReplacing the fixed-length assertion with
TariAddress::from_emoji_string(&emoji_id)guarantees the string is a valid address rather than merely 132 chars long. Good improvement.applications/minotari_console_wallet/src/grpc/wallet_debouncer.rs (1)
183-186: Expanded event coverageAdding
TransactionImportedandTransactionMinedUnconfirmedto the refresh trigger list ensures balance accuracy for previously overlooked paths—good catch.
| pub async fn start_balance_debouncer_event_monitor(&self) { | ||
| self.debouncer.lock().await.start_event_monitor_if_needed().await | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid holding the tokio::sync::MutexGuard across an .await – risk of starvation & dead-lock
lock().await returns a MutexGuard that is not Send.
By immediately calling an async method (.await) on the guarded value you keep the
lock held for the entire duration of that future, blocking every other task that
needs the same mutex and potentially dead-locking if the future itself needs to
re-acquire the lock internally.
Refactor so the lock is released before the await point and propagate errors:
-pub async fn start_balance_debouncer_event_monitor(&self) {
- self.debouncer.lock().await.start_event_monitor_if_needed().await
-}
+pub async fn start_balance_debouncer_event_monitor(&self) -> Result<(), WalletDebouncerError> {
+ // 1) grab the guard
+ let mut debouncer = self.debouncer.lock().await;
+ // 2) create the future **without** awaiting
+ let fut = debouncer.start_event_monitor_if_needed();
+ drop(debouncer); // 3) release the lock
+ fut.await // 4) await outside the critical section
+}This keeps the critical section minimal and surfaces any failure to the caller.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn start_balance_debouncer_event_monitor(&self) { | |
| self.debouncer.lock().await.start_event_monitor_if_needed().await | |
| } | |
| pub async fn start_balance_debouncer_event_monitor(&self) -> Result<(), WalletDebouncerError> { | |
| // 1) grab the guard | |
| let mut debouncer = self.debouncer.lock().await; | |
| // 2) create the future **without** awaiting | |
| let fut = debouncer.start_event_monitor_if_needed(); | |
| drop(debouncer); // 3) release the lock | |
| fut.await // 4) await outside the critical section | |
| } |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs around
lines 187 to 189, the code holds the tokio::sync::MutexGuard across an await,
which can cause deadlocks and starvation. To fix this, acquire the lock and
extract the necessary data or state without awaiting, then release the lock
before calling any async functions that require awaiting. Also, modify the
function to propagate any errors from start_event_monitor_if_needed to the
caller instead of ignoring them.
| port = get_port(world, 18000..18499).unwrap(); | ||
| grpc_port = get_port(world, 18500..18999).unwrap(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
unwrap() on get_port will panic when the pool is exhausted
get_port returns an Option<u64>, but the call site force-unwraps.
When many wallets / nodes are spawned in parallel the port range can be
exhausted, causing a hard panic and a red CI run.
Consider propagating the error or using expect(...) with a helpful message so
the failure is explicit:
- port = get_port(world, 18000..18499).unwrap();
- grpc_port = get_port(world, 18500..18999).unwrap();
+ port = get_port(world, 18_000..18_499)
+ .expect("No free P2P port available in 18000-18499");
+ grpc_port = get_port(world, 18_500..18_999)
+ .expect("No free gRPC port available in 18500-18999");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| port = get_port(world, 18000..18499).unwrap(); | |
| grpc_port = get_port(world, 18500..18999).unwrap(); | |
| port = get_port(world, 18_000..18_499) | |
| .expect("No free P2P port available in 18000-18499"); | |
| grpc_port = get_port(world, 18_500..18_999) | |
| .expect("No free gRPC port available in 18500-18999"); |
🤖 Prompt for AI Agents
In integration_tests/src/wallet_process.rs around lines 88 to 90, the calls to
get_port use unwrap(), which will cause a panic if no ports are available in the
specified range. Replace unwrap() with expect() and provide a clear error
message indicating that the port pool is exhausted, so failures are explicit and
easier to diagnose during parallel test runs.
| handle.block_on(async { grpc.start_balance_debouncer_event_monitor().await }); | ||
|
|
There was a problem hiding this comment.
Simplify .block_on call and propagate potential errors
start_balance_debouncer_event_monitor() already returns a future; wrapping it in an extra async block is unnecessary, and any error it returns is silently ignored:
- handle.block_on(async { grpc.start_balance_debouncer_event_monitor().await });
+ handle
+ .block_on(grpc.start_balance_debouncer_event_monitor())
+ .map_err(|e| ExitError::new(ExitCode::GrpcError, e.to_string()))?;This keeps the code concise and ensures the wallet exits cleanly if the monitor fails to start.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handle.block_on(async { grpc.start_balance_debouncer_event_monitor().await }); | |
| handle | |
| .block_on(grpc.start_balance_debouncer_event_monitor()) | |
| .map_err(|e| ExitError::new(ExitCode::GrpcError, e.to_string()))?; |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/wallet_modes.rs at lines 486 to 487,
remove the unnecessary async block wrapping the call to
grpc.start_balance_debouncer_event_monitor() inside handle.block_on. Instead,
directly pass the future returned by start_balance_debouncer_event_monitor() to
block_on and handle the Result it returns by propagating any errors to ensure
failures are not ignored and the wallet can exit cleanly if the monitor fails to
start.
| pub fn get_port(world: &mut TariWorld, range: Range<u16>) -> Option<u64> { | ||
| let min = range.clone().min().expect("A minimum possible port number"); | ||
| let max = range.max().expect("A maximum possible port number"); | ||
|
|
||
| loop { | ||
| let port = rand::thread_rng().gen_range(min..max); | ||
| let port = loop { | ||
| let port = rand::thread_rng().gen_range(min..max); | ||
| if !world.assigned_ports.contains_key(&u64::from(port)) { | ||
| break port; | ||
| } | ||
| }; | ||
|
|
||
| if TcpListener::bind(("127.0.0.1", port)).is_ok() { | ||
| world.assigned_ports.insert(u64::from(port), u64::from(port)); | ||
| return Some(u64::from(port)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
min() / max() iterate the entire range each call – use range.start/range.end instead
Range<u16> already exposes its bounds; invoking iterator adapters is unnecessary and marginally wasteful:
- let min = range.clone().min().expect("A minimum possible port number");
- let max = range.max().expect("A maximum possible port number");
+ let min = range.start;
+ let max = range.end;Clearer, faster, and avoids the .expect() noise.
🤖 Prompt for AI Agents
In integration_tests/src/lib.rs around lines 39 to 55, the code uses range.min()
and range.max() which unnecessarily iterate over the entire range and require
expect() calls. Replace these with direct access to range.start and range.end to
get the minimum and maximum bounds of the range. This change will simplify the
code, improve performance, and remove the need for expect() calls.
| { | ||
| let ffi_wallet = world.get_mut_ffi_wallet(&wallet).unwrap(); | ||
| ffi_wallet.restart(ffi_wallet.port); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-binding to the same TCP port can fail on some OSes
restart(ffi_wallet.port) re-uses the previous port immediately after destroy(). If the old listener is still in TIME_WAIT, the bind may fail sporadically.
Consider picking a fresh port:
- let ffi_wallet = world.get_mut_ffi_wallet(&wallet).unwrap();
- ffi_wallet.restart(ffi_wallet.port);
+ let ffi_wallet = world.get_mut_ffi_wallet(&wallet).unwrap();
+ let new_port = get_port(world, 18000..18499).expect("No free port");
+ ffi_wallet.restart(new_port);🤖 Prompt for AI Agents
In integration_tests/tests/steps/wallet_ffi_steps.rs around lines 617 to 620,
the code calls restart with the same TCP port which can fail if the old socket
is in TIME_WAIT state. To fix this, modify the restart call to use a fresh,
dynamically allocated port instead of reusing ffi_wallet.port, ensuring the new
port is free and avoiding bind errors.
| pub fn restart(&mut self, port: u64) { | ||
| self.wallet.lock().unwrap().destroy(); | ||
| let port = get_port(18000..18499).unwrap(); | ||
| let transport_config = | ||
| ffi::TransportConfig::create_tcp(CString::new(format!("/ip4/127.0.0.1/tcp/{}", port)).unwrap().into_raw()); | ||
| let now: DateTime<Utc> = SystemTime::now().into(); | ||
| let base_dir = format!("./log/ffi_wallets/{}", now.format("%Y%m%d-%H%M%S")); | ||
| let comms_config = ffi::CommsConfig::create(port, transport_config, base_dir.clone()); | ||
| let log_path = format!("{}/log/ffi_wallet.log", base_dir); | ||
| self.wallet = ffi::Wallet::create(comms_config, log_path, null()); | ||
| let comms_config = ffi::CommsConfig::create( | ||
| port, | ||
| transport_config, | ||
| self.base_dir.as_os_str().to_str().unwrap().into(), | ||
| ); | ||
| self.wallet = ffi::Wallet::create(comms_config, self.log_path.clone(), null()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Port reuse risk mirrors the test-step concern
restart(&mut self, port) keeps the same port by design. For robustness, grab a fresh unused port (see comment in step file) or at least handle bind errors and retry.
🤖 Prompt for AI Agents
In integration_tests/src/wallet_ffi.rs around lines 180 to 190, the restart
method reuses the same port which risks bind errors if the port is still in use.
Modify the method to either acquire a fresh unused port dynamically before
creating the transport config or add error handling around the wallet creation
to catch bind failures and retry with a different port. This ensures robustness
by avoiding port conflicts during restart.
| #[then(expr = "all wallets validate their transactions")] | ||
| #[when(expr = "all wallets validate their transactions")] | ||
| async fn all_wallets_validate_their_transactions(world: &mut TariWorld) { | ||
| let wallets = world.wallets.keys().cloned().collect::<Vec<_>>(); | ||
| for wallet in &wallets { | ||
| let mut client = create_wallet_client(world, wallet.clone()).await.unwrap(); | ||
| let result = client.validate_all_transactions(ValidateRequest {}).await; | ||
| if let Err(e) = result { | ||
| cucumber_steps_log(format!( | ||
| "Error! Wallet {} failed to validate transactions, error: {:?}", | ||
| wallet, e | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Step handler never fails when validation fails – errors are silently ignored
validate_all_transactions is the whole point of this step. By only logging the Err and continuing, the test suite will happily pass even when every single wallet fails validation.
- let result = client.validate_all_transactions(ValidateRequest {}).await;
- if let Err(e) = result {
- cucumber_steps_log(format!(
- "Error! Wallet {} failed to validate transactions, error: {:?}",
- wallet, e
- ));
- }
+ client
+ .validate_all_transactions(ValidateRequest {})
+ .await
+ .unwrap_or_else(|e| panic!("Wallet {} failed to validate transactions: {:?}", wallet, e));Fail fast so that CI actually reports regressions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[then(expr = "all wallets validate their transactions")] | |
| #[when(expr = "all wallets validate their transactions")] | |
| async fn all_wallets_validate_their_transactions(world: &mut TariWorld) { | |
| let wallets = world.wallets.keys().cloned().collect::<Vec<_>>(); | |
| for wallet in &wallets { | |
| let mut client = create_wallet_client(world, wallet.clone()).await.unwrap(); | |
| let result = client.validate_all_transactions(ValidateRequest {}).await; | |
| if let Err(e) = result { | |
| cucumber_steps_log(format!( | |
| "Error! Wallet {} failed to validate transactions, error: {:?}", | |
| wallet, e | |
| )); | |
| } | |
| } | |
| } | |
| #[then(expr = "all wallets validate their transactions")] | |
| #[when(expr = "all wallets validate their transactions")] | |
| async fn all_wallets_validate_their_transactions(world: &mut TariWorld) { | |
| let wallets = world.wallets.keys().cloned().collect::<Vec<_>>(); | |
| for wallet in &wallets { | |
| let mut client = create_wallet_client(world, wallet.clone()).await.unwrap(); | |
| client | |
| .validate_all_transactions(ValidateRequest {}) | |
| .await | |
| .unwrap_or_else(|e| panic!("Wallet {} failed to validate transactions: {:?}", wallet, e)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In integration_tests/tests/steps/wallet_steps.rs around lines 724 to 738, the
step handler currently logs errors from validate_all_transactions but does not
fail the test on error, causing silent test passes. Modify the code to
immediately return or panic on the first validation error to fail fast and
ensure the test suite reports regressions properly.
| let task = self.create_task(local_shutdown.to_signal()); | ||
| let mode = self.mode.clone(); | ||
| let mut task_join_handle = task::spawn(async move { | ||
| trace!(target: LOG_TARGET, "{:?}: Spawning new UTXO scanning task", mode); | ||
| if let Err(err) = task.run().await { | ||
| error!(target: LOG_TARGET, "Error scanning UTXOs: {}", err); | ||
| error!(target: LOG_TARGET, "{:?}: Error scanning UTXOs: {}", mode, err); | ||
| } | ||
| }) | ||
| .fuse(); | ||
|
|
||
| // These events will change the scanning behaviour: | ||
| // - Base node state changes will trigger a new scanning round if the block tip has changed and not yet | ||
| // scanned. | ||
| // - A successfully completed scanning round will update the last scanned block tip. | ||
| // - A newly established base node connection will reset the last and current scanned block tip states to | ||
| // force a new scanning round when the next base node state change is received. | ||
| // - One-sided payment message changes will update the message used in one-sided payments. | ||
| // - Recovery message changes will update the message used in recovery. | ||
| // - Shutdown signal will stop the task if it is running, and let that thread exit gracefully. | ||
| loop { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential overlap of multiple scanning tasks
local_shutdown.trigger() is fired to stop the running task, then the loop breaks and immediately starts a new task on the next iteration.
Because the previous JoinHandle is discarded without an await or abort, a slow-to-shutdown task can keep running in the background, leading to:
- two scanners touching the DB concurrently,
- duplicated progress events,
- unnecessary load.
Before spawning the next task, wait for the previous one to finish (e.g. store the handle outside the inner loop and await it after triggering shutdown) or call handle.abort() to guarantee termination.
🤖 Prompt for AI Agents
In base_layer/wallet/src/utxo_scanner_service/service.rs around lines 181 to
200, the code spawns a new scanning task each loop iteration without properly
awaiting or aborting the previous task's JoinHandle, causing overlapping tasks
and potential concurrency issues. To fix this, store the JoinHandle outside the
loop and before starting a new task, trigger shutdown on the current task and
either await its completion or call abort() on its handle to ensure the previous
task has fully stopped before spawning the next one.
* 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
and that the balances will correspond.
Motivation and Context
Various reports have been received regarding discrepancies in wallet balances between the console wallet, TU wallet (view-only wallet), and the mobile wallet (FFI wallet).
How Has This Been Tested?
New cucumber test:
Scenario: 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
Run the cucumber test
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests