chore!: upgrade to minotari 5.0.0 - #1560
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRepository-wide migration from tari_core/tari_key_manager to new component crates (tari_transaction_components, tari_node_components, tari_transaction_key_manager), switch from PaymentId to MemoField (bytes) for memo/payment_id fields, manifest/workspace updates to development branches, wallet-daemon API/config refactors, indexer/VN logging and epoch handling tweaks, and extensive integration-test rewrites to network-centric flows with relaxed height checks and cucumber logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant World
participant ConsoleWallet
participant BaseNode
participant Indexer
participant WalletDaemon
Tester->>World: boot network (base, console wallet, miner, indexer, walletd, VN)
World->>BaseNode: mine initial blocks
ConsoleWallet->>BaseNode: submit VN registration (payment_id = MemoField bytes)
BaseNode-->>World: tx accepted / epoch updated
Indexer->>World: scan to at least target height
Tester->>WalletDaemon: authenticate (None) / override keyring password
Note over WalletDaemon: walletd accessible at /json_rpc
sequenceDiagram
autonumber
actor Tester
participant ConsoleWallet
participant WalletDaemon
participant BaseNode
Tester->>ConsoleWallet: burn amount -> returns BURN_PROOF (ExtClaimBurnProof)
Tester->>WalletDaemon: claim_burn(proof, account, max_fee)
WalletDaemon->>BaseNode: submit claim tx (payment_id = MemoField::Burn bytes)
BaseNode-->>WalletDaemon: tx_id
Tester->>WalletDaemon: wait_transaction_result(tx_id, timeout)
WalletDaemon-->>Tester: FinalizeResult (success/fail)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (9)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results (CI)395 tests - 24 395 ✅ ± 0 44m 18s ⏱️ - 9m 25s Results for commit 1fefdf7. ± Comparison against base commit e0b97c2. This pull request removes 24 tests. |
Description --- fix: set validator registration value to 1000uT for localnet and igor Motivation and Context --- The transaction builder checks that you aren't sending less funds than the fee cost. This causes a registration sending 0 to fail. Send 1000 to cover for this. How Has This Been Tested? --- Tested registration in tari-ootle PR (tari-project/tari-ootle#1560) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Raised the minimum validator node registration deposit to 1000 MicroMinotari on localnet and Igor. * Users registering validator nodes on these networks must now provide at least 1000 MicroMinotari as a deposit. * No changes to public APIs or user interfaces; impact is limited to validator registration requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description --- fix: set validator registration value to 1000uT for localnet and igor Motivation and Context --- The transaction builder checks that you aren't sending less funds than the fee cost. This causes a registration sending 0 to fail. Send 1000 to cover for this. How Has This Been Tested? --- Tested registration in tari-ootle PR (tari-project/tari-ootle#1560) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Raised the minimum validator node registration deposit to 1000 MicroMinotari on localnet and Igor. * Users registering validator nodes on these networks must now provide at least 1000 MicroMinotari as a deposit. * No changes to public APIs or user interfaces; impact is limited to validator registration requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description --- fix: set validator registration value to 1000uT for localnet and igor Motivation and Context --- The transaction builder checks that you aren't sending less funds than the fee cost. This causes a registration sending 0 to fail. Send 1000 to cover for this. How Has This Been Tested? --- Tested registration in tari-ootle PR (tari-project/tari-ootle#1560) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Raised the minimum validator node registration deposit to 1000 MicroMinotari on localnet and Igor. * Users registering validator nodes on these networks must now provide at least 1000 MicroMinotari as a deposit. * No changes to public APIs or user interfaces; impact is limited to validator registration requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description --- fix(wallet): use minimum value promise in VN reg output Motivation and Context --- Base node validation for VN reg requires the minimum value promise to be >= deposit amount Ref tari-project/tari-ootle#1560 How Has This Been Tested? --- In PR tari-project/tari-ootle#1560 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an optional "minimum value promise" for pay-to-self transactions and UTXO aggregation. * Propagated across wallet transaction flows and advanced transaction options so it's available where outputs are constructed. * Defaults to zero when not specified, preserving existing behavior and workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
b9b7aa5 to
f27a7bf
Compare
2e05e6c to
7a63f69
Compare
7a63f69 to
716a3c3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
applications/tari_swarm_daemon/src/process_manager/manager.rs (2)
451-484: Bug: inverted existence check and misuse ofenvswhen creating instancesLogic is flipped: you error when the instance does not exist, and you try to read
envs()from a would-be duplicate. This prevents creation and can panic later.- let Some(instance) = self.instance_manager.instances().find(|i| i.name() == name) else { - if reply - .send(Err(anyhow!( - "Instance with name '{name}' already exists. Please choose a different name", - ))) - .is_err() - { - log::warn!("Request cancelled before response could be sent") - } - return Ok(()); - }; - - let envs = instance.envs().to_vec(); + if self + .instance_manager + .instances() + .any(|i| i.name() == name) + { + let _ = reply.send(Err(anyhow!( + "Instance with name '{name}' already exists. Please choose a different name", + ))); + return Ok(()); + } + + // No prior instance with this name; start with no additional env vars + let envs = vec![];
821-824: Compile-time type error in log expression
min_expected_blocks * initial_emission_amountmixesu64with a non-primitive amount type. Use.as_u64()as you did in the comparison above.- min_expected_blocks * initial_emission_amount + min_expected_blocks * initial_emission_amount.as_u64()clients/base_node_client/src/grpc.rs (2)
72-79: Avoid unwrap(); propagate connection errors.This can panic under transient network issues. Use ? to bubble up BaseNodeClientError instead.
- let inner = self.connection().await.unwrap(); + let inner = self.connection().await?;
25-46: Update downstreamValidatorNodeChangeusages and conversions
Multiple consumers still assume the old local type/signature—these must be updated to use the gRPC message or theTryFromimpl:
- crates/epoch_oracles/src/base_layer/mod.rs (L463–L467):
get_validator_node_changesnow returnsVec<grpc::ValidatorNodeChange>; adjust the call site to import the gRPC type and convert each element to your local enum.- crates/epoch_oracles/src/configured/oracle.rs (L134–L136): mapping via
vn.claim_key/vn.public_keyis invalid for the gRPC message; replace withValidatorNodeChange::try_from(vn)?or explicitly match onvn.change.crates/wallet/sdk/src/sdk.rs (2)
93-121: Avoid bricking existing wallets when RecoveryNeeded is missing.
Older stores may not have ConfigKey::RecoveryNeeded set. Default and persist false instead of erroring.- let requires_recovery = self.config_api().get(ConfigKey::RecoveryNeeded).optional()?; - // This should have been set - it is an error if it is not - requires_recovery.ok_or_else(|| WalletSdkError::InvariantError { - details: "Cipher seed already initialized but recovery_needed not set.".to_string(), - }) + let requires_recovery = self.config_api().get(ConfigKey::RecoveryNeeded).optional()?; + let requires_recovery = match requires_recovery { + Some(v) => v, + None => { + warn!(target: LOG_TARGET, "RecoveryNeeded flag missing; defaulting to false for backward compatibility."); + self.config_api().set(ConfigKey::RecoveryNeeded, &false, false)?; + false + }, + }; + Ok(requires_recovery)
373-386: Remove staleKeyManagerErrorvariants in APIs The errorsKeyManagerErrorincrates/wallet/sdk/src/apis/key_manager.rs:261andcrates/wallet/sdk/src/apis/stealth_scanner.rs:247must be replaced with the updated error types or removed.crates/epoch_oracles/src/base_layer/mod.rs (1)
119-121: Critical: last_epoch_hash loaded from the wrong store key.
load_initial_state reads BaseLayerLastScannedBlockHash instead of BaseLayerLastEpochHash; set_last_epoch_block writes BaseLayerLastEpochHash. This breaks epoch hash recovery after restarts.Apply:
- self.last_epoch_hash = self - .store - .get(StoreKey::BaseLayerLastScannedBlockHash.as_key_bytes()) + self.last_epoch_hash = self + .store + .get(StoreKey::BaseLayerLastEpochHash.as_key_bytes()) .map_err(BaseLayerOracleError::StoreError)?;Also applies to: 573-579
integration_tests/tests/steps/wallet.rs (1)
79-110: Fix type mismatch and brittle timeout check in balance wait loop.
iterationsdefaults toi32and is compared to au64. Also use>=to avoid off-by-one surprises.Apply this diff:
- let mut iterations = 0; + let mut iterations: u64 = 0; @@ - if iterations == MAX_WAIT_TIME_SECS.div_ceil(2) { + if iterations >= MAX_WAIT_TIME_SECS.div_ceil(2) {crates/wallet/sdk/src/apis/key_manager.rs (1)
149-160: Bug: inconsistent branch identifier used for persistence can desync indices.Everywhere else you use
branch.as_ref()as the store key, but here you insert usingkey_manager.branch_seed. If these ever differ,last_index()/set_activewill read/update a different row, causing repeated index resets or duplicate rows. Use the same identifier for all store ops.Apply:
- tx.key_manager_insert(&key_manager.branch_seed, key_manager.key_index())?; + tx.key_manager_insert(branch.as_ref(), key_manager.key_index())?;Optional safety net: Add a uniqueness constraint on the key-manager table for
(branch)to fail fast on mismatches.
🧹 Nitpick comments (68)
integration_tests/src/templates/faucet/src/lib.rs (1)
46-48: Prefer explicit conversion for clarity and trait stability
1000.into()relies onFrom<u64> for Amount. If that impl ever changes, this could break inference. Consider being explicit.- self.take_amount_of_free_coins(1000.into()) + self.take_amount_of_free_coins(Amount::from(1000))applications/tari_indexer/src/network_client.rs (2)
52-54: Good guard; add timing/observability (and optional timeout) to avoid blind waitsKeeping this wait is sensible. Add a quick timing log so we can see if/when this is a bottleneck. Optional: wrap with a configurable timeout if hangs are a concern.
Apply:
- // Ensure initial scanning has completed to ensure an accurate epoch - self.epoch_manager.wait_for_initial_scanning_to_complete().await?; + // Ensure initial scanning has completed to ensure an accurate epoch + let t0 = std::time::Instant::now(); + self.epoch_manager.wait_for_initial_scanning_to_complete().await?; + log::debug!(target: LOG_TARGET, "Initial scanning completed in {:?}", t0.elapsed());(Optional timeout sketch outside this hunk)
// use tokio::time::{timeout, Duration}; // timeout(Duration::from_secs(config.initial_scan_timeout_secs), // self.epoch_manager.wait_for_initial_scanning_to_complete() //).await.map_err(|_| NetworkClientError::EpochManagerError(EpochManagerError::Other("initial scan timeout".into())))??;
151-157: Helpful context log; consider debug level and add shard-group countThe new log is useful. In high-QPS paths, consider
debug!to reduce noise, and after buildingall_membersadd the unique shard-group count for correlation.Add after
committee_sizeis computed (outside this hunk):- let committee_size = all_members.len(); + let committee_size = all_members.len(); + log::debug!(target: LOG_TARGET, "Resolved committees for {} unique shard groups at epoch {}", committee_size, epoch);Nit:
committee_sizeactually represents the number of shard groups here, not the number of validators. Consider renaming and adjusting the error message for clarity:- let committee_size = all_members.len(); + let num_shard_groups = all_members.len();- return Err(NetworkClientError::AllValidatorsFailed { - committee_size, + return Err(NetworkClientError::AllValidatorsFailed { + committee_size: num_shard_groups, last_error, });And in the error display (outside this hunk):
- #[error("Rpc call failed for all ({committee_size}) validators: {}", .last_error.as_deref().unwrap_or("unknown"))] + #[error("RPC calls failed for all ({committee_size}) shard groups: {}", .last_error.as_deref().unwrap_or("unknown"))]applications/tari_swarm_daemon/src/process_manager/manager.rs (2)
807-812: Avoid potential panic: handle empty consensus constants gracefully
pop().unwrap()will panic if the list is empty. Return a proper error instead.- let constants = NetworkConsensus::from(convert_network_to_l1_network(&self.network)) - .create_consensus_constants() - .pop() - .unwrap(); + let constants = NetworkConsensus::from(convert_network_to_l1_network(&self.network)) + .create_consensus_constants() + .pop() + .ok_or_else(|| anyhow!("No consensus constants available for selected network"))?;
616-626: Return full path, not just the filenameYou create the file at
out_path.join(file_name)but return onlyfile_name. Callers likely expect the full path to the created file.- Ok(file_name) + Ok(path)Would you like me to scan callers to confirm expectations?
README.md (1)
91-97: Wording nit: it’s a tag, not a branchThe commands fetch/checkout a tag. Adjust the wording for accuracy.
-`cd` into `tari` and change the branch `v5.0.0-pre.8`: +`cd` into `tari` and checkout the tag `v5.0.0-pre.8`:applications/tari_swarm_daemon/src/process_definitions/minotari_wallet.rs (1)
67-68: Verify http_server_url semantics and avoid port clash with base node.You’re setting wallet http_server_url to the base node HTTP port. If this flag is for the wallet’s own server, it will collide; if it configures a client endpoint, the name is misleading.
Option A (wallet hosts its own HTTP server):
- .arg(format!( - "-pwallet.http_server_url=http://{listen_ip}:{base_node_api_port}" - )) + let wallet_http_port = context.get_free_port("wallet_http").await?; + .arg(format!( + "-pwallet.http_server_url=http://{listen_ip}:{wallet_http_port}" + ))Option B (wallet needs BN HTTP client URL): keep using the client flag:
- "-pwallet.http_server_url=http://{listen_ip}:{base_node_api_port}" + "-pwallet.http_client_url=http://{listen_ip}:{base_node_api_port}"Please confirm the intended flag in minotari 5.0.0 CLI.
integration_tests/src/templates/basic_nft/src/lib.rs (2)
51-53: Access rules set to allow_all for tests (LGTM).Appropriate for test templates; ensure no prod templates rely on this.
Consider a comment like “test-only: allow_all” to prevent copy/paste into prod code.
Also applies to: 68-70
72-74: Be explicit about quantity type to aid readability.withdraw(1) is fine if Into is implemented; using Amount::one() (or an explicit Amount) can reduce ambiguity.
- self.vault.withdraw(1) + self.vault.withdraw(Amount::one())integration_tests/tests/steps/miner.rs (2)
33-33: Consider timeout over loop counter.A bounded timeout is clearer than iter_count with fixed sleep; also mitigates flakiness if sleep changes.
- tokio::time::sleep(std::time::Duration::from_millis(500)).await; - if iter_count > 100 { + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + if iter_count >= 100 { panic!("Timed out waiting for tip height to reach {}", start_tip + num_blocks); }Or:
tokio::time::timeout(std::time::Duration::from_secs(50), async { while tip.height_of_longest_chain < start_tip + num_blocks { tip = client.get_tip_info().await.unwrap(); tokio::time::sleep(std::time::Duration::from_millis(500)).await; } }).await.expect("Timed out waiting for tip height");
39-42: Prefer structured logging over eprintln!.Use the repo’s logging (log/tracing) so messages appear in CI logs consistently.
- eprintln!( - "Base node {} reached tip height {}", - bn.name, tip.height_of_longest_chain - ); + log::info!("Base node {} reached tip height {}", bn.name, tip.height_of_longest_chain);applications/tari_watcher/Cargo.toml (1)
15-15: Consider disabling default features for parity.Several crates set default-features = false for tari_transaction_components; align here unless defaults are required.
Apply:
-tari_transaction_components = { workspace = true } # Used for VN registration signature +tari_transaction_components = { workspace = true, default-features = false } # Used for VN registration signaturecrates/epoch_manager/Cargo.toml (1)
13-13: Disable default features on minotari_app_grpc to avoid unintended pulls.Unless you rely on its defaults, keep it lean.
-minotari_app_grpc = { workspace = true, optional = true } +minotari_app_grpc = { workspace = true, optional = true, default-features = false }applications/tari_validator_node/src/lib.rs (1)
115-115: Add context to JoinError from spawn_blocking.Propagate a clearer error if the blocking task panics or the runtime is shutting down.
Apply:
- task::spawn_blocking(preload_crypto_services).await?; + use anyhow::Context; + task::spawn_blocking(preload_crypto_services) + .await + .context("preloading crypto services (spawn_blocking)")?;applications/tari_swarm_daemon/Cargo.toml (1)
12-12: Align default‐features usage here withclients/base_node_client(optional).
Onlyclients/base_node_client/Cargo.tomlcurrently setsdefault-features = falsefortari_transaction_components. If this daemon doesn’t rely on any of the crate’s default features, consider addingdefault-features = falseto shrink the dependency graph:-tari_transaction_components = { workspace = true } # Used for VN registration signature +tari_transaction_components = { workspace = true, default-features = false } # Used for VN registration signatureapplications/tari_walletd/src/config.rs (1)
90-110: Gate the password override for non-production buildsReduce risk surface by feature-gating or env-guarding this field so it’s unusable in prod configs.
Example approach (outline):
- Add a cargo feature e.g.
dangerous_password_override.- Under that feature, include the field; otherwise, reject/ignore it on load.
applications/tari_swarm_daemon/src/process_manager/processes/minotari_wallet.rs (1)
57-59: Use anyhow::Context and avoid extra allocation for the memoSlightly cleaner error propagation and fewer conversions.
- payment_id: MemoField::new_open("Burn funds in swarm".as_bytes().to_vec(), TxType::Burn) - .map_err(|e| anyhow!("Failed to create MemoField: {e}"))? - .to_bytes(), + payment_id: { + let bytes = MemoField::new_open(b"Burn funds in swarm".to_vec(), TxType::Burn) + .context("Failed to create MemoField")? + .to_bytes(); + bytes + },Add the import:
use anyhow::Context;integration_tests/src/wallet_daemon_cli.rs (1)
904-906: Helper now always returns an authed client — consider a fallible variant.Add a try_get_auth_wallet_daemon_client returning Result to avoid panics when the name is wrong, while keeping this infallible helper for tests.
Example addition (outside this hunk):
pub async fn try_get_auth_wallet_daemon_client( world: &TariWorld, name: &str, ) -> anyhow::Result<WalletDaemonClient> { let wd = world .wallet_daemons .get(name) .ok_or_else(|| anyhow::anyhow!("No wallet daemon named {}", name))?; Ok(wd.get_authed_client().await) }integration_tests/src/validator_node.rs (1)
98-115: Resilient RPC errors inwait_for_consensus_to_start
Replaceunwrap()with amatchto log transient RPC errors and continue polling under the same 60 s timeout.- loop { - let resp = client.get_consensus_status().await.unwrap(); - if resp.state == "Running" { - return; - } + loop { + match client.get_consensus_status().await { + Ok(resp) if resp.state == "Running" => return, + Ok(_) => {}, + Err(e) => log::warn!("get_consensus_status failed: {e}"), + } attempts -= 1; if attempts == 0 { panic!( "Validator node did not start consensus in time: status: {}, epoch: {}", - resp.state, resp.epoch + client.get_consensus_status().await.map(|r| r.state).unwrap_or_else(|_| "<err>".into()), + client.get_consensus_status().await.map(|r| r.epoch).unwrap_or_default() ); } tokio::time::sleep(std::time::Duration::from_secs(1)).await; }integration_tests/src/wallet.rs (4)
119-126: Remove stale, commented base-node key/port code.
Dead code adds noise and confuses future readers. Safe to delete.Apply:
- // let base_node_public_key = world - // .base_nodes - // .get(&base_node_name) - // .unwrap() - // .identity - // .public_key() - // .clone(); - // let base_node_port = world.base_nodes.get(&base_node_name).unwrap().port;
179-183: Delete commented base_node_service_peers block.
If custom base node is deprecated in 5.0.0, keep history in git, not comments.- // wallet_config.wallet.base_node_service_peers = Some(format!( - // "{}::/ip4/127.0.0.1/tcp/{}", - // base_node_public_key, base_node_port - // ));
220-233: Dropped connectivity wait can cause flakiness; add a lightweight readiness check or remove block.
Consider polling identify() + one base-node HTTP call, or delete this block entirely.- // let mut status = wallet_client.get_network_status(Empty {}).await.unwrap().into_inner(); - // let mut counter = 0; - // while status.status != ConnectivityStatus::Online as i32 { - // eprintln!( - // "Waiting for wallet to connect to base node {} on port {} (status: {:?})", - // base_node_name, base_node_port, status - // ); - // tokio::time::sleep(Duration::from_secs(1)).await; - // counter += 1; - // if counter > 20 { - // panic!("Wallet failed to connect to base node"); - // } - // status = wallet_client.get_network_status(Empty {}).await.unwrap().into_inner(); - // }Optional readiness (outside this block):
// After identify(): // tokio::time::timeout(Duration::from_secs(20), async { // // ping base node HTTP or check tip height > 0 via wallet RPC if available // }).await.expect("wallet/base-node not ready in time");
71-97: Retry loop off-by-one and no connect timeout.
Currently allows 12 tries and unbounded connect(). Add connect_timeout and fix counter.- let endpoint = Endpoint::from_str(&wallet_addr).unwrap(); - let mut attempts = 0; + let endpoint = Endpoint::from_str(&wallet_addr) + .expect("Invalid wallet GRPC address") + .connect_timeout(Duration::from_secs(5)); + let mut attempts = 0usize; let channel = loop { if self.handle.is_finished() { panic!("Wallet thread has ended"); } - match endpoint.connect().await { + attempts += 1; + match endpoint.connect().await { Ok(channel) => break channel, Err(e) => { eprintln!( "Attempt: {}/10 Could not connect to wallet GRPC address {}: {}", - attempts, wallet_addr, e + attempts, wallet_addr, e ); - if attempts > 10 { + if attempts >= 10 { panic!("Failed to connect to wallet GRPC address {}", wallet_addr); } tokio::time::sleep(Duration::from_secs(1)).await; - attempts += 1; }, } };applications/tari_swarm_daemon/src/layer_one_transactions/submitter.rs (2)
62-68: Use as_bytes() before to_vec() for signatures to avoid trait ambiguity.
Keeps consistency with exit path below and avoids relying on impls that may change.- validator_node_signature: Some(grpc::Signature { - public_nonce: registration.signature.public_nonce().to_vec(), - signature: registration.signature.signature().to_vec(), - }), + validator_node_signature: Some(grpc::Signature { + public_nonce: registration.signature.public_nonce().as_bytes().to_vec(), + signature: registration.signature.signature().as_bytes().to_vec(), + }),
75-79: Also convert optional sidechain key via as_bytes().
Prevents relying on Into<Vec> impls.- sidechain_deployment_key: registration - .sidechain_public_key - .map(|key| key.to_vec()) - .unwrap_or_default(), + sidechain_deployment_key: registration + .sidechain_public_key + .map(|key| key.as_bytes().to_vec()) + .unwrap_or_default(),applications/tari_watcher/src/minotari.rs (2)
134-141: Prefer as_bytes() for keys/signatures to ensure explicit conversion.
Matches other call sites and avoids implicit Vec conversions.- validator_node_public_key: info.public_key.to_vec(), + validator_node_public_key: info.public_key.as_bytes().to_vec(), validator_node_signature: Some(grpc::Signature { - public_nonce: info.signature.public_nonce().to_vec(), - signature: info.signature.signature().to_vec(), + public_nonce: info.signature.public_nonce().as_bytes().to_vec(), + signature: info.signature.signature().as_bytes().to_vec(), }), - validator_node_claim_public_key: info.claim_fees_public_key.to_vec(), + validator_node_claim_public_key: info.claim_fees_public_key.as_bytes().to_vec(),
194-201: Make signature bytes extraction explicit.
Consistency with exit path and other modules.validator_node_signature: Some(grpc::Signature { - public_nonce: registration.signature.public_nonce().to_vec(), - signature: registration.signature.signature().to_vec(), + public_nonce: registration.signature.public_nonce().as_bytes().to_vec(), + signature: registration.signature.signature().as_bytes().to_vec(), }),Cargo.toml (2)
140-147: Consider disabling default features on new component crates at the workspace level.To prevent accidental feature creep and keep crates in control of features, set
default-features = falsefor these workspace-scoped deps (if supported by the crates).-tari_transaction_components = { git = "https://github.com/tari-project/tari.git", branch = "development" } -tari_node_components = { git = "https://github.com/tari-project/tari.git", branch = "development" } -tari_transaction_key_manager = { git = "https://github.com/tari-project/tari.git", branch = "development" } +tari_transaction_components = { git = "https://github.com/tari-project/tari.git", branch = "development", default-features = false } +tari_node_components = { git = "https://github.com/tari-project/tari.git", branch = "development", default-features = false } +tari_transaction_key_manager = { git = "https://github.com/tari-project/tari.git", branch = "development", default-features = false }
205-205: libsqlite3-sys 0.30.1: considerbundledto avoid system-SQLite mismatches in CI.This crate often fails on CI/macOS without a consistent SQLite. Enabling
bundledtrades a small build-time cost for reliability.-libsqlite3-sys = "0.30.1" +libsqlite3-sys = { version = "0.30.1", features = ["bundled"] }If you prefer system SQLite, ensure runners have a compatible lib and headers; otherwise expect intermittent link errors.
integration_tests/tests/features/state_sync.feature (1)
47-48: Nit: prefer “And” after the first Then for consistency.Pure style; earlier you used “Then … And …”. Consider aligning here.
-Then VN has scanned to at least height 50 -Then VN2 has scanned to at least height 50 +Then VN has scanned to at least height 50 +And VN2 has scanned to at least height 50integration_tests/src/util.rs (1)
12-15: Centralize the log target and consider a macro for zero-cost formatting.
- Define a shared constant for the target to avoid "cucumber" string duplication across files.
- Optional: provide a
cucumber!macro so call sites don’t needformat!(avoids eager allocation).Apply this minimal change for the target:
+pub const CUCUMBER_LOG_TARGET: &str = "cucumber"; + pub fn cucumber_log<T: AsRef<str>>(msg: T) { // eprintln!("CUCUMBER: {}", msg.as_ref()); - info!(target: "cucumber", "{}", msg.as_ref()); + info!(target: CUCUMBER_LOG_TARGET, "{}", msg.as_ref()); }Optional macro to add (usage: cucumber!("Base node identity: {}", id)):
#[macro_export] macro_rules! cucumber { ($($arg:tt)*) => { ::log::info!(target: "cucumber", $($arg)*); }; }integration_tests/tests/features/epoch_change.feature (1)
82-82: Commented expectation updated; consider pruning or tagging the scenario.Either remove the commented block or guard it with a tag to avoid drift.
integration_tests/src/base_node.rs (1)
71-71: Route logs via cucumber_log — LGTM.Consistent with the new test logging approach.
If you adopt the
cucumber!macro suggested in util.rs, update these call sites to avoidformat!:- cucumber_log(format!("Base node identity: {}", base_node_identity)); + cucumber!("Base node identity: {}", base_node_identity); - cucumber_log(format!("Using base_node temp_dir: {}", temp_dir.display())); + cucumber!("Using base_node temp_dir: {}", temp_dir.display());Also applies to: 97-97
integration_tests/tests/log4rs/cucumber.yml (1)
138-144: Confirm dual-target logging intent for cucumber.Appending to both cucumber and ootle increases I/O and duplicates records; keep only cucumber if separation is desired.
cucumber: level: debug appenders: - - cucumber - - ootle + - cucumber additive: falsecrates/wallet/sdk/src/apis/accounts.rs (1)
57-59: Avoid duplicate logic—delegate to the free function.- pub fn derive_account_address_from_public_key(&self, public_key: &RistrettoPublicKeyBytes) -> ComponentAddress { - derive_component_address_from_public_key(&ACCOUNT_TEMPLATE_ADDRESS, public_key) - } + pub fn derive_account_address_from_public_key(&self, public_key: &RistrettoPublicKeyBytes) -> ComponentAddress { + super::accounts::derive_account_address_from_public_key(public_key) + }Alternatively, deprecate the free function and keep only the method for a single API surface.
integration_tests/tests/features/wallet_daemon.feature (2)
107-107: Wording nit: “to proof” → “to produce proof” (if step regex allows)If step definitions aren’t strict, consider:
-When I burn 1000T on wallet NETWORK_CONSOLE_WALLET to proof BURN_PROOF for wallet daemon WALLET_D +When I burn 1000T on wallet NETWORK_CONSOLE_WALLET to produce proof BURN_PROOF for wallet daemon WALLET_DIf strict, ignore this to avoid breaking the matcher.
114-114: Minor wording: duplicate “address”If the step matcher permits, simplify:
-When I convert commitment in proof BURN_PROOF into COMM_ADDRESS address +When I convert commitment in proof BURN_PROOF into COMM_ADDRESScrates/wallet/sdk/src/lib.rs (1)
15-15: AliasWalletSecretKeynow points toDerivedKey— clarify semantics to avoid confusion.
If this is a key handle/derivation output (not a raw secret scalar), consider documenting to prevent misuse.Apply doc comment for clarity:
- pub type WalletSecretKey = tari_transaction_components::key_manager::tari_key_manager::DerivedKey; + /// Alias to the wallet key manager's DerivedKey type used for derived wallet keys. + pub type WalletSecretKey = tari_transaction_components::key_manager::tari_key_manager::DerivedKey;integration_tests/tests/steps/indexer.rs (1)
49-76: “At least height” check — good; tweak assertion text and consider a timeout helper.
Logic with>=removes flakiness. Update panic text to match the new condition.- panic!( - "Indexer {} did not scan to block height {}. Current height: {}", - name, block_height, stats.current_block_height - ); + panic!( + "Indexer {} did not scan to at least block height {}. Current height: {}", + name, block_height, stats.current_block_height + );Optional: extract the “poll with reset-on-progress and max wait” into a reusable helper or drive
remainingfrom the configured scanning interval.integration_tests/src/indexer.rs (2)
73-84: Avoidunwrap()inadd_peer; return a Result to surface dial failures.
Panicking here makes test failures opaque. Propagate the client error.- pub async fn add_peer(&self, public_key: RistrettoPublicKeyBytes, port: u16) { + pub async fn add_peer( + &self, + public_key: RistrettoPublicKeyBytes, + port: u16, + ) -> Result<(), tari_indexer_client::IndexerClientError> { let mut jrpc_client = self.get_jrpc_indexer_client(); - jrpc_client + jrpc_client .add_peer(AddPeerRequest { public_key, addresses: vec![multiaddr!(Ip4([127, 0, 0, 1]), Tcp(port))], wait_for_dial: true, }) - .await - .unwrap(); + .await + .map(|_| ()) }Call sites can
?and add context.
151-159: Prefer 127.0.0.1 over localhost for client endpoints.
Minor portability tweak to avoid IPv6/hosts resolution surprises.You can change both constructors to:
let endpoint: Url = Url::parse(&format!("http://127.0.0.1:{}", self.json_rpc_port)).unwrap(); // and similarly for GraphQLapplications/tari_walletd/src/cli.rs (1)
131-138: UpdateCreateAccounthelp text
Change theaboutto reflect account creation and options:- #[clap(about = "Generate a new key and output the public key")] + #[clap(about = "Create a new account; optionally set it active and/or write the public key to a file")] CreateAccount { #[clap(long, alias = "key")] key_index: Option<u64>, #[clap(long)] set_active: bool, #[clap(long, alias = "output", short = 'o')] output_path: Option<PathBuf>, },Dispatch for
Subcommand::CreateAccountis already wired inmain.rs:68–71.integration_tests/src/miner.rs (2)
84-90: Prefer &str over &String for miner_name.Takes a borrowed String where a string slice suffices; avoids needless specificity.
-async fn create_base_node_client(world: &TariWorld, miner_name: &String) -> BaseNodeClient { +async fn create_base_node_client(world: &TariWorld, miner_name: &str) -> BaseNodeClient { let miner = world.miners.get(miner_name).unwrap();
76-81: Replace fixed sleep with a wait-on-condition to reduce flakiness.A hardcoded 100ms may be racy under load/CI. Poll the base node for template availability or mempool drain with a bounded timeout/retry.
integration_tests/tests/features/claim_burn.feature (4)
17-17: Mempool timing: 10s may be tight.Consider a longer bounded wait or a polling step with backoff to reduce flakes on CI.
Also applies to: 38-38
18-19: Coupling “mine N blocks” with “scanned to ≥ height” can still race.Prefer a single eventual condition like “wait for VN scanned to ≥ H” (optionally mining as needed) to avoid timing gaps between mining and scanning.
Also applies to: 39-40
14-14: Trim trailing whitespace.Minor formatting nit in these steps.
- When I burn 10T on wallet NETWORK_CONSOLE_WALLET to proof BURN_PROOF for wallet daemon WALLET_D + When I burn 10T on wallet NETWORK_CONSOLE_WALLET to proof BURN_PROOF for wallet daemon WALLET_DAlso applies to: 35-35
45-47: Assert failure semantics for double-claim.If available, assert a specific error code/message to make the negative case deterministic, not just “it fails”.
clients/base_node_client/src/grpc.rs (2)
126-131: Public API now exposes gRPC types — confirm callers and bindings.Returning grpc::ValidatorNodeChange couples the client API to transport. If intentional, ensure all consumers (Rust and TS) are updated; otherwise consider an internal domain type with TryFrom.
Optional readability tweak:
- ) -> Result<Vec<minotari_app_grpc::tari_rpc::ValidatorNodeChange>, BaseNodeClientError> { + ) -> Result<Vec<grpc::ValidatorNodeChange>, BaseNodeClientError> {And keep:
- let changes = result.changes; - Ok(changes) + Ok(result.changes)Also applies to: 140-142
76-93: Error context for mempool stream.Current Err maps to ConnectionError after warn!. Consider preserving the source error for callers.
- Err(e) => { - warn!(target: LOG_TARGET, "Error getting mempool transaction count: {}", e); - return Err(BaseNodeClientError::ConnectionError); - }, + Err(e) => { + warn!(target: LOG_TARGET, "Error getting mempool transaction count: {}", e); + return Err(e.into()); + },integration_tests/tests/steps/common.rs (1)
10-21: Improve log context and guard against silent overwrites.
Include the proof name in the log and assert if a duplicate key would overwrite an existing substate id.- cucumber_log(format!( - "Converted commitment {} into address: {}", - proof.claim_proof.commitment, address - )); - world.substate_ids.insert(new_name, address.into()); + cucumber_log(format!( + "Converted commitment in proof {} (commitment: {}) into address {}", + proof_name, proof.claim_proof.commitment, address + )); + let prev = world.substate_ids.insert(new_name.clone(), address.into()); + assert!(prev.is_none(), "Substate id '{}' already exists", new_name);integration_tests/tests/steps/network.rs (1)
22-26: Define block-count constants to avoid mismatched logs.
Centralize “10” and “20” to prevent future drift.- const BASE_NODE_NAME: &str = "NETWORK_BASE_NODE"; - const CONSOLE_WALLET_NAME: &str = "NETWORK_CONSOLE_WALLET"; + const BASE_NODE_NAME: &str = "NETWORK_BASE_NODE"; + const CONSOLE_WALLET_NAME: &str = "NETWORK_CONSOLE_WALLET"; const MINER_NAME: &str = "NETWORK_MINER"; - const INDEXER_NAME: &str = "NETWORK_INDEXER"; + const INDEXER_NAME: &str = "NETWORK_INDEXER"; + const BLOCKS_INITIAL: u64 = 10; + const BLOCKS_POST_REG: u64 = 20;And use them below:
- miner::miner_mines_new_blocks(world, MINER_NAME.to_string(), 10).await; - cucumber_log("Mined 10 blocks"); + miner::miner_mines_new_blocks(world, MINER_NAME.to_string(), BLOCKS_INITIAL).await; + cucumber_log(&format!("Mined {} blocks", BLOCKS_INITIAL)); - miner::miner_mines_new_blocks(world, MINER_NAME.to_string(), 20).await; - cucumber_log("Mined 26 blocks"); - indexer::indexer_has_scanned_to_at_least_height(world, INDEXER_NAME.to_string(), 20).await; - cucumber_log("Indexer has scanned up to or past height 26"); + miner::miner_mines_new_blocks(world, MINER_NAME.to_string(), BLOCKS_POST_REG).await; + cucumber_log(&format!("Mined {} blocks", BLOCKS_POST_REG)); + indexer::indexer_has_scanned_to_at_least_height(world, INDEXER_NAME.to_string(), BLOCKS_POST_REG).await; + cucumber_log(&format!( + "Indexer has scanned up to or past height {}", + BLOCKS_POST_REG + ));applications/tari_walletd/src/lib.rs (1)
65-69: Prefer owning SeedWords to enable secure zeroization.
Passing Option<&SeedWords> prevents wiping secrets. Take Option and pass .as_ref() to the SDK.Apply:
-pub async fn run_tari_ootle_walletd( - config: ApplicationConfig, - seed_words: Option<&SeedWords>, +pub async fn run_tari_ootle_walletd( + config: ApplicationConfig, + seed_words: Option<SeedWords>, shutdown_signal: ShutdownSignal, ) -> Result<(), anyhow::Error> { @@ - let needs_seed_recovery = wallet_sdk.initialize_cipher_seed(seed_words)?; + let needs_seed_recovery = wallet_sdk.initialize_cipher_seed(seed_words.as_ref())?;Optionally drop the owned seed after use to limit lifetime.
Also applies to: 76-76
crates/epoch_oracles/src/base_layer/mod.rs (1)
469-477: Don’t fail the whole scan on a single malformed VN change.
Degrade gracefully: log and skip bad entries; emit changes for the valid ones.Apply:
- let node_changes = node_changes - .into_iter() - .map(TryInto::try_into) - .collect::<Result<Vec<_>, _>>() - .map_err(|e| { - BaseLayerOracleError::InvalidBaseNodeResponse(format!( - "Failed to convert validator node change: {}", - e - )) - })?; + let mut node_changes_converted = Vec::with_capacity(node_changes.len()); + for change in node_changes { + match TryInto::try_into(change) { + Ok(c) => node_changes_converted.push(c), + Err(e) => { + warn!(target: LOG_TARGET, "Invalid validator node change from base node: {e}"); + } + } + } + let node_changes = node_changes_converted;integration_tests/tests/steps/validator_node.rs (2)
191-194: Replace unwrap() on MemoField with expect for clearer failures.Apply:
- payment_id: MemoField::new_open_from_string("Register by cucumber", TxType::ValidatorNodeRegistration) - .unwrap() - .to_bytes(), + payment_id: MemoField::new_open_from_string("Register by cucumber", TxType::ValidatorNodeRegistration) + .expect("failed to encode MemoField for VN registration") + .to_bytes(),
391-398: Tighten error messages around GetState.
Use expect(...) instead of unwrap() to include context if the RPC fails; keep optional() handling as-is.Apply:
- .optional() - .unwrap() + .optional() + .expect("get_state RPC failed for substate address")Also applies to: 405-414
applications/tari_walletd/src/main.rs (1)
62-64: Avoid keeping the override password as a plain String in config.Consider
secrecy::SecretString(and zeroize on drop) to reduce exposure in logs/mem dumps.If you want, I can sketch the config/type changes across the SDK/config to propagate
SecretString.integration_tests/tests/steps/wallet.rs (1)
47-53: Minor consistency: use the helper that avoids alloc.
MemoField::open_from_string("Burn", TxType::Burn)keeps things consistent with other steps and avoids a temporary Vec.- payment_id: MemoField::new_open("Burn".as_bytes().to_vec(), TxType::Burn) - .unwrap() - .to_bytes(), + payment_id: MemoField::open_from_string("Burn", TxType::Burn).to_bytes(),integration_tests/tests/steps/wallet_daemon.rs (2)
54-70: Track the TODO on negative-path validation.Current comment notes an upstream limitation. Consider marking with an issue so it doesn’t get lost.
I can open a follow-up issue describing the missing VN-behaviour assertion and propose a guard to detect the specific failure mode.
177-191: Fix string interpolation in cucumber log.The braces won’t interpolate; use
format!(or log the hex if preferred).- let public_key = nonce.public_key; - cucumber_log("Burning funds using claim key {public_key}"); + let public_key = nonce.public_key; + cucumber_log(format!("Burning funds using claim key {:?}", public_key));integration_tests/src/lib.rs (3)
105-137: Init path looks correct; tighten panic messagesPrefer expect() with context over unwrap() for quicker triage in flaky CI.
- ) - .unwrap(); + ) + .expect("failed to construct default_payment_address from wallet_private_key"); ... - key_manager: create_memory_db_key_manager().await.unwrap(), + key_manager: create_memory_db_key_manager() + .await + .expect("failed to create in-memory key manager"),
166-169: Improve panic message for missing scenario nameClearer guidance when misused.
- self.current_scenario_name.as_deref().expect("No current scenario") + self.current_scenario_name + .as_deref() + .expect("current_scenario_name is None; ensure it is set before spawning processes that use it")
328-330: Debug label mismatch: “addresses” → “substate_ids”Minor clarity fix.
- .field("claim_proofs", &self.claim_proofs.keys()) - .field("addresses", &self.substate_ids.keys()) + .field("claim_proofs", &self.claim_proofs.keys()) + .field("substate_ids", &self.substate_ids.keys())integration_tests/src/wallet_daemon.rs (2)
145-159: claim_burn helper — consider fee configurabilityHardcoded max_fee=5000 may cause intermittent failures if fees rise. Allow an optional parameter with a sensible default.
- pub async fn claim_burn( - &self, - account_name: &str, - claim_proof: ExtClaimBurnProof, - ) -> Result<ClaimBurnResponse, WalletDaemonClientError> { + pub async fn claim_burn( + &self, + account_name: &str, + claim_proof: ExtClaimBurnProof, + max_fee: Option<u64>, + ) -> Result<ClaimBurnResponse, WalletDaemonClientError> { let mut client = self.get_authed_client().await; let req = ClaimBurnRequest { account: ComponentAddressOrName::Name(account_name.into()), claim_proof, - max_fee: Some(5000), + max_fee: max_fee.or(Some(5_000)), }; client.claim_burn(req).await }
161-170: wait_for_transaction_result — consider timeout paramTo reduce fixed waits in features, expose timeout_secs as a parameter (default 30).
- pub async fn wait_for_transaction_result(&self, tx_id: TransactionId) -> TransactionWaitResultResponse { + pub async fn wait_for_transaction_result( + &self, + tx_id: TransactionId, + timeout_secs: Option<u64>, + ) -> TransactionWaitResultResponse { let mut client = self.get_authed_client().await; client .wait_transaction_result(TransactionWaitResultRequest { transaction_id: tx_id, - timeout_secs: Some(30), + timeout_secs: timeout_secs.or(Some(30)), }) .await .unwrap() }crates/wallet/sdk/src/apis/key_manager.rs (3)
87-91: Tidy: simplify error mapping noise.You repeatedly do
.map_err(key_manager::error::KeyManagerServiceError::from)?. Prefer.map_err(Into::into)?(or let?work directly if you add aFromimpl toKeyManagerApiError), reducing verbosity and coupling.Example changes:
- .map_err(key_manager::error::KeyManagerServiceError::from)?; + .map_err(Into::into)?;Repeat similarly in the other occurrences.
Also applies to: 103-110, 152-160, 210-214
208-214: Avoid write-side effects during brute-force search.
search_for_key_within_rangecurrently callsget_or_create_key_manager, which can create a DB row as a side effect. For a read-only search, construct a non-mutating manager instead.Apply:
- let km = self.get_or_create_key_manager(branch)?; + let km = self.get_key_manager(branch, 0);
251-253: Minor: avoid cloning CipherSeed if not necessary.If
TariKeyManager::fromsupports borrowing the seed, prefer that to avoid repeated clones on hot paths. Otherwise, consider caching managers per branch if derivations are frequent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
.gitignore(1 hunks)Cargo.toml(4 hunks)README.md(1 hunks)applications/tari_indexer/src/network_client.rs(2 hunks)applications/tari_swarm_daemon/Cargo.toml(1 hunks)applications/tari_swarm_daemon/src/layer_one_transactions/submitter.rs(3 hunks)applications/tari_swarm_daemon/src/process_definitions/minotari_node.rs(0 hunks)applications/tari_swarm_daemon/src/process_definitions/minotari_wallet.rs(1 hunks)applications/tari_swarm_daemon/src/process_manager/manager.rs(1 hunks)applications/tari_swarm_daemon/src/process_manager/processes/minotari_wallet.rs(2 hunks)applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs(1 hunks)applications/tari_validator_node/Cargo.toml(1 hunks)applications/tari_validator_node/src/json_rpc/handlers.rs(1 hunks)applications/tari_validator_node/src/lib.rs(2 hunks)applications/tari_walletd/Cargo.toml(2 hunks)applications/tari_walletd/src/cli.rs(3 hunks)applications/tari_walletd/src/config.rs(2 hunks)applications/tari_walletd/src/lib.rs(3 hunks)applications/tari_walletd/src/main.rs(5 hunks)applications/tari_walletd/src/services/recovery_service.rs(2 hunks)applications/tari_watcher/Cargo.toml(1 hunks)applications/tari_watcher/src/minotari.rs(3 hunks)clients/base_node_client/Cargo.toml(1 hunks)clients/base_node_client/src/grpc.rs(3 hunks)clients/base_node_client/src/traits.rs(1 hunks)clients/base_node_client/src/types.rs(1 hunks)crates/epoch_manager/Cargo.toml(2 hunks)crates/epoch_manager/src/epoch_event_oracle/event.rs(2 hunks)crates/epoch_oracles/Cargo.toml(2 hunks)crates/epoch_oracles/src/base_layer/mod.rs(6 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(1 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(2 hunks)crates/wallet/sdk/src/apis/key_manager.rs(10 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/models/key.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(4 hunks)integration_tests/Cargo.toml(2 hunks)integration_tests/src/base_node.rs(3 hunks)integration_tests/src/indexer.rs(4 hunks)integration_tests/src/lib.rs(7 hunks)integration_tests/src/miner.rs(2 hunks)integration_tests/src/templates/basic_nft/src/lib.rs(3 hunks)integration_tests/src/templates/faucet/src/lib.rs(1 hunks)integration_tests/src/util.rs(1 hunks)integration_tests/src/validator_node.rs(5 hunks)integration_tests/src/wallet.rs(6 hunks)integration_tests/src/wallet_daemon.rs(5 hunks)integration_tests/src/wallet_daemon_cli.rs(2 hunks)integration_tests/tests/cucumber.rs(1 hunks)integration_tests/tests/features/claim_burn.feature(1 hunks)integration_tests/tests/features/claim_fees.feature(4 hunks)integration_tests/tests/features/committee.feature(1 hunks)integration_tests/tests/features/epoch_change.feature(2 hunks)integration_tests/tests/features/eviction.feature(1 hunks)integration_tests/tests/features/state_sync.feature(2 hunks)integration_tests/tests/features/substates.feature(1 hunks)integration_tests/tests/features/transfer.feature(5 hunks)integration_tests/tests/features/wallet_daemon.feature(2 hunks)integration_tests/tests/log4rs/cucumber.yml(2 hunks)integration_tests/tests/steps/common.rs(1 hunks)integration_tests/tests/steps/indexer.rs(2 hunks)integration_tests/tests/steps/miner.rs(1 hunks)integration_tests/tests/steps/network.rs(2 hunks)integration_tests/tests/steps/validator_node.rs(10 hunks)integration_tests/tests/steps/wallet.rs(3 hunks)integration_tests/tests/steps/wallet_daemon.rs(6 hunks)
💤 Files with no reviewable changes (1)
- applications/tari_swarm_daemon/src/process_definitions/minotari_node.rs
🧰 Additional context used
🧬 Code graph analysis (18)
applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs (1)
clients/javascript/wallet_daemon_client/src/index.ts (1)
WalletDaemonClient(102-357)
integration_tests/src/base_node.rs (1)
integration_tests/src/util.rs (1)
cucumber_log(12-15)
integration_tests/src/templates/faucet/src/lib.rs (1)
crates/template_lib/src/auth/access_rules.rs (1)
allow_all(137-142)
crates/epoch_manager/src/epoch_event_oracle/event.rs (1)
crates/common_types/src/substate_address.rs (1)
from_hash_and_version(97-105)
integration_tests/tests/steps/common.rs (2)
integration_tests/src/util.rs (1)
cucumber_log(12-15)crates/template_lib/src/models/layer_one_commitment.rs (1)
from_commitment(34-36)
integration_tests/src/templates/basic_nft/src/lib.rs (1)
crates/template_lib/src/auth/access_rules.rs (1)
allow_all(137-142)
crates/wallet/sdk/src/apis/accounts.rs (4)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)bindings/src/helpers/consts.ts (1)
ACCOUNT_TEMPLATE_ADDRESS(6-6)
integration_tests/tests/steps/indexer.rs (2)
integration_tests/tests/cucumber.rs (3)
world(316-316)world(523-523)world(674-683)integration_tests/tests/steps/validator_node.rs (2)
world(139-148)world(470-474)
integration_tests/tests/steps/network.rs (11)
integration_tests/src/indexer.rs (1)
spawn_indexer(162-235)integration_tests/src/miner.rs (1)
register_miner_process(52-59)integration_tests/src/util.rs (1)
cucumber_log(12-15)integration_tests/src/validator_node.rs (1)
spawn_validator_node(118-218)integration_tests/src/wallet.rs (1)
spawn_wallet(115-236)integration_tests/src/wallet_daemon.rs (1)
spawn_wallet_daemon(69-111)integration_tests/src/base_node.rs (2)
world(78-82)spawn_base_node(61-170)integration_tests/tests/steps/validator_node.rs (4)
world(139-148)world(470-474)send_vn_registration(169-208)assert_vn_is_registered(285-318)integration_tests/tests/steps/miner.rs (1)
miner_mines_new_blocks(15-44)integration_tests/tests/steps/wallet.rs (1)
check_balance(78-117)integration_tests/tests/steps/indexer.rs (1)
indexer_has_scanned_to_at_least_height(50-76)
integration_tests/src/indexer.rs (4)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)applications/tari_validator_node/src/json_rpc/handlers.rs (1)
add_peer(648-696)applications/tari_indexer/src/json_rpc/handlers.rs (1)
add_peer(174-221)clients/tari_indexer_client/src/json_rpc_client.rs (1)
add_peer(83-85)
clients/base_node_client/src/traits.rs (5)
bindings/src/types/validator-node-client/ValidatorNodeChange.ts (1)
ValidatorNodeChange(9-18)bindings/src/types/BlockHeader.ts (1)
BlockHeader(9-73)bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/SubstateAddress.ts (1)
SubstateAddress(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
integration_tests/tests/steps/wallet.rs (8)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-3)bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/Scalar32Bytes.ts (1)
Scalar32Bytes(3-3)bindings/src/types/CommitmentSignatureBytes.ts (1)
CommitmentSignatureBytes(5-5)bindings/src/types/RangeProofBytes.ts (1)
RangeProofBytes(9-9)bindings/src/types/wallet-daemon-client/ClaimBurnProof.ts (1)
ClaimBurnProof(7-12)bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts (1)
ExtClaimBurnProof(4-4)
integration_tests/tests/steps/wallet_daemon.rs (5)
integration_tests/src/util.rs (1)
cucumber_log(12-15)integration_tests/src/wallet_daemon.rs (1)
claim_burn(145-159)integration_tests/src/wallet_daemon_cli.rs (2)
claim_burn(81-109)get_auth_wallet_daemon_client(904-906)clients/wallet_daemon_client/src/lib.rs (1)
claim_burn(342-347)crates/template_lib_types/src/crypto/commitment_signature.rs (1)
from_bytes(39-54)
integration_tests/src/lib.rs (4)
bindings/src/types/SubstateId.ts (1)
SubstateId(15-24)bindings/src/types/SubstateRequirement.ts (1)
SubstateRequirement(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts (1)
ExtClaimBurnProof(4-4)
clients/base_node_client/src/grpc.rs (1)
bindings/src/types/validator-node-client/ValidatorNodeChange.ts (1)
ValidatorNodeChange(9-18)
applications/tari_walletd/src/main.rs (1)
applications/tari_walletd/src/lib.rs (3)
init_wallet_store(166-170)initialize_wallet_sdk(172-189)run_tari_ootle_walletd(65-164)
integration_tests/src/wallet_daemon.rs (5)
applications/tari_walletd/src/lib.rs (1)
run_tari_ootle_walletd(65-164)integration_tests/src/logging.rs (1)
get_base_dir_for_scenario(32-42)clients/javascript/wallet_daemon_client/src/index.ts (1)
WalletDaemonClient(102-357)integration_tests/src/wallet_daemon_cli.rs (1)
claim_burn(81-109)clients/wallet_daemon_client/src/lib.rs (1)
claim_burn(342-347)
integration_tests/tests/steps/validator_node.rs (3)
integration_tests/src/util.rs (1)
cucumber_log(12-15)integration_tests/tests/cucumber.rs (3)
world(316-316)world(523-523)world(674-683)crates/common_types/src/substate_address.rs (1)
from_substate_id(40-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: clippy
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: fmt
| # moon | ||
| .moon/cache | ||
| .moon/docker | ||
| ./cucumber-output-junit.xml No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: pattern won’t match; use a root-anchored ignore
.gitignore doesn’t treat "./" specially. Use a leading slash to anchor to repo root so the file is actually ignored.
-./cucumber-output-junit.xml
+/cucumber-output-junit.xml📝 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.
| ./cucumber-output-junit.xml | |
| /cucumber-output-junit.xml |
🤖 Prompt for AI Agents
In .gitignore around line 65, the pattern "./cucumber-output-junit.xml" won't
match because .gitignore doesn't treat "./" specially; replace it with a
root-anchored pattern "/cucumber-output-junit.xml" so the file at the repo root
is actually ignored (remove the "./" and add the leading slash).
| pub override_keyring_password: Option<SafePassword>, | ||
| } |
There was a problem hiding this comment.
Prevent leaking override_keyring_password via serialization/logging
Storing a password in config is risky; this field currently serializes (struct derives Serialize). Ensure it never gets emitted to logs or “dump config” endpoints.
Apply:
- pub override_keyring_password: Option<SafePassword>,
+ /// DANGER: Dev/test-only override. Do not persist or log this value.
+ /// This field is never serialized to avoid accidental disclosure.
+ #[serde(skip_serializing)]
+ pub override_keyring_password: Option<SafePassword>,📝 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 override_keyring_password: Option<SafePassword>, | |
| } | |
| /// DANGER: Dev/test-only override. Do not persist or log this value. | |
| /// This field is never serialized to avoid accidental disclosure. | |
| #[serde(skip_serializing)] | |
| pub override_keyring_password: Option<SafePassword>, | |
| } |
🤖 Prompt for AI Agents
In applications/tari_walletd/src/config.rs around lines 82-83, the
override_keyring_password field is currently serialized and can leak via logs or
config dumps; mark the field to never be serialized and prevent it showing up in
debug output by adding serde attributes (e.g. #[serde(skip_serializing,
skip_deserializing)] or #[serde(skip)]) to exclude it from any Serde-based dumps
and either implement a custom Debug for the struct that omits this field or
annotate it to be skipped by your debug/redaction utility (e.g. #[debug(skip)]
or use the crate-specific redaction attribute) so it is never emitted in logs or
endpoints.
| pub fn initialize_wallet_sdk( | ||
| cli: &Cli, | ||
| config: &ApplicationConfig, | ||
| store: SqliteWalletStore, | ||
| ) -> anyhow::Result<WalletSdk<SqliteWalletStore, IndexerJsonRpcNetworkInterface>> { | ||
| let sdk_config = WalletSdkConfig { | ||
| network: config.ootle_wallet_daemon.network, | ||
| override_keyring_password: cli.override_keyring_password.clone(), | ||
| override_keyring_password: config.ootle_wallet_daemon.override_keyring_password.clone(), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t source keyring passwords from config; prefer env with redaction.
Reading override_keyring_password from config risks plaintext leakage. At minimum, allow an env override and avoid logging it.
Apply:
let sdk_config = WalletSdkConfig {
network: config.ootle_wallet_daemon.network,
- override_keyring_password: config.ootle_wallet_daemon.override_keyring_password.clone(),
+ override_keyring_password: std::env::var("TARI_WALLETD_KEYRING_PASSWORD")
+ .ok()
+ .or_else(|| config.ootle_wallet_daemon.override_keyring_password.clone()),
};🤖 Prompt for AI Agents
In applications/tari_walletd/src/lib.rs around lines 172 to 179, the code
currently reads override_keyring_password directly from config which risks
plaintext leakage; change it to prefer a secret provided via environment (e.g.
OOTLE_KEYRING_PASSWORD) and only fall back to the config value if the env var is
unset; treat the password as an Option (or a secret wrapper) so it can be
redacted from logs and never printed; update WalletSdkConfig construction to use
that env-first value and ensure any logging or error messages never include the
password content.
| Some(Subcommand::CreateAccount { | ||
| key_index, | ||
| set_active, | ||
| output_path, | ||
| }) => { | ||
| let wallet_store = init_wallet_store(&config)?; | ||
| let mut sdk = initialize_wallet_sdk(&cli, &config, wallet_store)?; | ||
| let mut sdk = initialize_wallet_sdk(&config, wallet_store)?; | ||
| sdk.initialize_cipher_seed(cli.wallet_restore.seed_words.as_ref())?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
JSON serialization likely fails for RistrettoPublicKey; also avoid hardcoding account name.
RistrettoPublicKeyusually doesn’t implementSerialize. Emit bytes (or hex) instead.- Hardcoding
Some("Fee")for a generic “CreateAccount” command is surprising; preferNoneor a CLI-provided label.
Apply these diffs:
- sdk.accounts_api()
- .add_account(Some("Fee"), &account_addr, secret.key_index, false, true)?;
+ sdk.accounts_api()
+ .add_account(None, &account_addr, secret.key_index, false, true)?;- let json = json!({
- "address": account_addr,
- "public_key": public_key,
- "key_index": secret.key_index,
- });
+ let json = json!({
+ "address": account_addr.to_string(),
+ "public_key": public_key.to_byte_type(),
+ "key_index": secret.key_index,
+ });Please run a local cargo check -p tari_walletd to ensure serde impls are satisfied for the address type, or switch that to .to_string() as shown.
Also applies to: 82-89, 95-97
🏁 Script executed:
#!/bin/bash
set -e
echo "1. Searching for RistrettoPublicKey occurrences and potential Serialize impls"
rg -n 'RistrettoPublicKey' -C3 || true
rg -nP 'impl\s+.*serde::Serialize\s+for\s+RistrettoPublicKey' -C3 || true
echo "2. Searching for available byte‐conversion methods on RistrettoPublicKey"
rg -n 'to_bytes' -C3 || true
rg -n 'to_vec' -C3 || true
rg -n 'to_byte_type' -C3 || true
echo "3. Inspecting add_account signature in the SDK"
rg -nP '\.add_account\(' -C3 || trueLength of output: 268243
Ensure JSON fields are serializable and remove hardcoded account name
- Replace
with
sdk.accounts_api() .add_account(Some("Fee"), &account_addr, secret.key_index, false, true)?;
sdk.accounts_api() .add_account(None, &account_addr, secret.key_index, false, true)?;
- Replace
with
let json = json!({ "address": account_addr, "public_key": public_key, "key_index": secret.key_index, });
let json = json!({ "address": account_addr.to_string(), "public_key": public_key.to_byte_type(), "key_index": secret.key_index, });
Also apply the same changes at lines 82–89 and 95–97.
🤖 Prompt for AI Agents
In applications/tari_walletd/src/main.rs around lines 68–75 (and also apply the
same changes at lines 82–89 and 95–97): the review asks to remove the hardcoded
account name and ensure JSON fields are serializable; replace the call that
passes Some("Fee") to add_account with None, and when building the JSON object
convert address and public_key to serializable types by calling to_string() on
the address and using the public key's serialization method (e.g.
to_byte_type()) for public_key, leaving key_index as-is.
| # external minotari/tari dependencies | ||
| minotari_app_grpc = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| minotari_app_utilities = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| minotari_console_wallet = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| minotari_node = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| minotari_node_grpc_client = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| minotari_wallet = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| minotari_wallet_grpc_client = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_common = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_common_types = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_common_sqlite = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_hashing = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_sidechain = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_jellyfish = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| minotari_app_grpc = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| minotari_app_utilities = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| minotari_console_wallet = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| minotari_node = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| minotari_node_grpc_client = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| minotari_wallet = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| minotari_wallet_grpc_client = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_common = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_common_types = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_common_sqlite = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_hashing = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_sidechain = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_jellyfish = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
|
|
||
| # avoid including default features so each crate can choose which ones to import | ||
| tari_core = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1", default-features = false } | ||
| tari_key_manager = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_metrics = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_mmr = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_p2p = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_shutdown = { git = "https://github.com/tari-project/tari.git", tag = "v4.9.0-pre.1" } | ||
| tari_transaction_components = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_node_components = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_transaction_key_manager = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_metrics = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_mmr = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_p2p = { git = "https://github.com/tari-project/tari.git", branch = "development" } | ||
| tari_shutdown = { git = "https://github.com/tari-project/tari.git", branch = "development" } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pin git dependencies to a release tag or commit (avoid tracking development).
Tracking upstream branch = "development" is non-reproducible and risky for a breaking chore to 5.0.0. Please pin to the 5.0.0 tag (or a specific commit) across these deps for deterministic builds.
Example diff for one dep (repeat for all in 125–147):
-minotari_node = { git = "https://github.com/tari-project/tari.git", branch = "development" }
+minotari_node = { git = "https://github.com/tari-project/tari.git", tag = "v5.0.0" }Quick check to list all branch-pinned deps:
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*[\w-]+\s*=\s*\{[^}]*\bgit\s*=.*\bbranch\s*=\s*"(development|main|master)"' Cargo.tomlLength of output: 2123
Pin Tari git dependencies to v5.0.0 tag
Replace branch = "development" with tag = "v5.0.0" for all Tari dependencies in Cargo.toml (lines 126–147) to ensure deterministic, reproducible builds.
🤖 Prompt for AI Agents
In Cargo.toml around lines 125 to 147, the Tari git dependencies currently use
branch = "development" which yields non-deterministic builds; update each Tari
dependency entry to use tag = "v5.0.0" instead of branch = "development" so the
dependencies are pinned to the v5.0.0 tag for reproducible builds (i.e., replace
each occurrence of branch = "development" with tag = "v5.0.0" for all
Tari-related dependencies listed).
| Then VN has scanned to at least height 17 | ||
| And indexer IDX has scanned to at least height 17 | ||
| Then the validator node VN is listed as registered |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good change to “at least height” to reduce test flakiness.
Ensure step definitions support the new phrasing everywhere:
Also applies to: 40-41, 72-74, 86-86
🏁 Script executed:
#!/bin/bash
rg -n 'scanned to height ' integration_tests/tests/features
rg -n 'scanned to at least height ' integration_testsLength of output: 4102
Update remaining “scanned to height” steps
integration_tests/tests/features/eviction.feature lines 35, 56 and 59 still use “scanned to height {int}”; replace each with “scanned to at least height {int}” to match the updated step definitions.
🤖 Prompt for AI Agents
integration_tests/tests/features/eviction.feature around lines 35, 56, and 59:
update the step text occurrences that read "scanned to height {int}" to "scanned
to at least height {int}" so they match the updated step definitions; ensure you
change each exact step line (including spacing/casing) to the new phrase so the
feature uses the updated matcher.
2 participants
Description
chore!: upgrade to minotari 5.0.0
test: fix some cucumbers
Summary by CodeRabbit
New Features
Changes
Documentation
Chores