fix!: large byte buf fix, engine optimise, integration tests - #1589
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughBumps template workspace crates to 0.14.0; introduces a boxed Bytes type and migrates many arg/ABI surfaces to Bytes; adds engine/WASM limits and runtime guards; expands indexer state‑sync, JSON‑RPC and sync persistence; simplifies walletd transfer responses; and refactors tests, bindings and web UI routing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as IndexerJsonRpcClient
participant Server as Indexer JSON-RPC Server
participant Handlers as Handlers.get_network_sync_state
participant EpochMgr as epoch_manager
participant SubstateMgr as substate_manager
Client->>Server: get_network_sync_state()
Server->>Handlers: dispatch
rect rgb(235,245,255)
Handlers->>EpochMgr: get_network_description()
EpochMgr-->>Handlers: NetworkDescription
end
rect rgb(240,255,240)
Handlers->>SubstateMgr: get_sync_progress() (optional)
SubstateMgr-->>Handlers: SyncProgress | error
end
Handlers-->>Server: GetNetworkSyncStateResponse
Server-->>Client: result
sequenceDiagram
autonumber
participant Worker as StateSync Worker
participant Pools as ValidatorCommitteeRpcPool
participant Committee as Committee Member
loop until session established
Worker->>Pools: new_session()
Pools->>Committee: try open session with random member
alt success
Committee-->>Pools: session
Pools-->>Worker: session
else failure
Committee-->>Pools: error (record failed address)
Pools-->>Worker: retry
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (107)
⛔ Files not processed due to max files limit (20)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
75dfd44 to
cb401cc
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
applications/tari_watcher/src/monitoring.rs (1)
17-26: Fields should be public or use getters.The struct fields
idandblockare currently private, but they are accessed directly in lines 122, 222, and 230 (e.g.,tx.id,tx.block). This will cause compilation errors.Apply this diff to make the fields public:
pub struct TransactionRefInBlock { - id: u64, - block: u64, + pub id: u64, + pub block: u64, }Alternatively, if you prefer to keep fields private, add getter methods:
impl TransactionRefInBlock { pub fn new(id: u64, block: u64) -> Self { Self { id, block } } + + pub fn id(&self) -> u64 { + self.id + } + + pub fn block(&self) -> u64 { + self.block + } }And update the access sites to use
tx.id()andtx.block().integration_tests/src/wallet_daemon_client.rs (1)
101-197: Remove commented-out code block.The large commented-out section (lines 134-172) should be deleted rather than left in the codebase. If this code might be needed for reference, move it to version control history or documentation.
Apply this diff to remove the dead code:
.await .unwrap(); - // let signing_key_index = account.key_index; - // - // - // let resource_address = STEALTH_TARI_RESOURCE_ADDRESS; - // - // let create_transfer_proof_req = ProofsGenerateRequest { - // account: Some(source_account_name), - // amount: amount.into(), - // reveal_amount: Amount::zero(), - // resource_address, - // destination_public_key, - // }; - // - // let transfer_proof_resp = client.create_transfer_proof(create_transfer_proof_req).await.unwrap(); - // let withdraw_proof = transfer_proof_resp.proof; - // let proof_id = transfer_proof_resp.proof_id; - // - // let transaction = transaction_builder() - // .fee_transaction_pay_from_component(source_component_address, 5000) - // .call_method(source_component_address, "withdraw_confidential", args![ - // resource_address, - // withdraw_proof - // ]) - // .put_last_instruction_output_on_workspace("bucket") - // .call_method(destination_account, "deposit", args![Workspace("bucket")]) - // .with_min_epoch(min_epoch) - // .with_max_epoch(max_epoch) - // .build_unsigned_transaction(); - // - // let submit_req = TransactionSubmitRequest { - // transaction, - // signing_key_index: Some(signing_key_index), - // proof_ids: vec![proof_id], - // detect_inputs: true, - // detect_inputs_use_unversioned: true, - // }; - // - // let submit_resp = client.submit_transaction(submit_req).await.unwrap(); - // let wait_req = TransactionWaitResultRequest {crates/engine/tests/templates/composability/src/lib.rs (1)
23-23: AddBytesto the template prelude
TheBytestype used incrates/engine/tests/templates/composability/src/lib.rsisn’t currently re-exported bytari_template_lib::prelude. Incrates/template_lib/src/prelude.rs, add a line such as:pub use crate::bytes::Bytes;so template authors can
use tari_template_lib::prelude::*to accessBytes.integration_tests/tests/features/indexer.feature (1)
59-68: Inconsistent naming: IDX vs INDEXER.This scenario uses
IDX(line 64, 67) while other scenarios useINDEXER. Additionally, it uses the old initialization pattern (base node + indexer) instead of the new network setup pattern used elsewhere in this file.Consider updating this scenario to match the pattern used in other scenarios:
Scenario: Indexer GraphQL requests work - # Initialize a base node, wallet, miner and VN - Given a base node BASE - - # Initialize an indexer - Given an indexer IDX connected to base node BASE + Given a network with registered validator VN and wallet daemon WALLET_D # Check GraphQL request - Then IDX indexer GraphQL request works + Then INDEXER indexer GraphQL request works
♻️ Duplicate comments (1)
applications/tari_walletd/src/handlers/accounts.rs (1)
883-886: Same pattern: dry_run result discarded.As with
handle_transfer(lines 815-819), the dry_run execution result is ignored here. This is consistent across handlers but prevents clients from validating transactions before submission. See earlier comment for verification request.
🧹 Nitpick comments (19)
integration_tests/tests/features/substates.feature (1)
26-26: Minor typo in comment.The comment contains "thas" which should be "that":
"the same component version that has already been downed".Apply this diff:
- # We should get an error if we se as inputs the same component version thas has already been downed from previous transactions + # We should get an error if we use as inputs the same component version that has already been downed from previous transactionsNote: Also corrected "se" → "use" for clarity.
integration_tests/tests/features/claim_fees.feature (1)
40-50: Consider extracting common network spec to Background.The network spec (lines 40-50) is identical to the first scenario (lines 9-19). In Gherkin, you can use a
Backgroundsection to avoid duplication and ensure consistency across scenarios.Consider refactoring to:
@claim_fees Feature: Claim Fees Background: Given a network with spec """ validators: - name: VN fee_claim_account: VN_FEES walletds: - name: WALLET_D with_account: VN_FEES indexer: name: IDX """ @serial @fixed Scenario: Claim validator fees # Run some transactions to generate fees ... @serial Scenario: Prevent double claim of validator fees # Run some transactions to generate fees ...applications/tari_wallet_cli/src/command/transaction.rs (1)
448-448: Consider displaying refunded fee amount for consistency.The
handle_sendfunction displays both the total fee and refunded amount (lines 398-402), buthandle_confidential_transferonly shows the total fee. For consistency and better user experience, consider showing the refund information here as well.Apply this diff to add refund information:
println!("Transaction: {}", resp.transaction_id); - println!("Fee: {}", resp.final_fee); + println!( + "Fee: {} ({} refunded)", + resp.final_fee, + result.fee_receipt.total_refunded() + ); println!(); summarize_finalize_result(&result);applications/tari_walletd/src/handlers/validator.rs (1)
135-169: Fee building refactor is correct; consider the optimization noted in TODOs.The two-phase bucket handling (collect via workspace, then deposit each separately) is logically sound and handles the account creation case properly. However, as noted in the TODO comments:
- Lines 161-164 highlight that depositing buckets individually increases gas costs. The suggested improvement—collecting all buckets and depositing once—would be more efficient.
- Lines 140-145 note that making
create_accountidempotent or auto-adding inputs from instructions could simplify the conditional logic.Consider prioritizing the workspace bucket collection improvement to reduce transaction gas costs, especially for users claiming fees from many shards.
integration_tests/tests/features/leader_failure.feature (3)
1-3: Consider updating the copyright year.This is a new file created in 2025, but the copyright year is 2022. Update the year to 2025 to reflect the actual creation date.
Apply this diff:
-# Copyright 2022 The Tari Project +# Copyright 2025 The Tari Project # SPDX-License-Identifier: BSD-3-Clause
21-21: Use "When" instead of "Then" for the stop action.Line 21 performs an action (stopping VN4) rather than asserting an outcome. In Gherkin, "When" is the conventional keyword for actions, while "Then" is reserved for assertions and outcomes.
Apply this diff:
- Then I stop validator node VN4 + When I stop validator node VN4
28-54: Document the reason for ignoring this scenario.The second scenario is marked with
@ignore, which disables it. Consider adding a comment explaining why it's ignored (e.g., resource constraints, known RocksDB "too many open files" issue mentioned in the PR objectives, or a specific ticket number).This will help maintainers understand when the test can be re-enabled. For example:
# Ignored due to RocksDB file descriptor limits in CI (see issue #XXXX) @serial @ignore Scenario: Leader failure with multiple committeesAlso note the inconsistency: this scenario correctly uses "When" for the stop action (line 49), while the first scenario uses "Then" (line 21). Applying the suggested fix to line 21 will resolve this inconsistency.
applications/tari_indexer/src/graphql/model/events.rs (1)
71-74: Consider removing trailing punctuation.The log message has a trailing comma and space after the last parameter. While not incorrect, it's inconsistent with typical log formatting.
Apply this diff to clean up the formatting:
- "Querying events. topic: {}, substate_id: {}, offset: {}, limit: {}, ", topic.display(), substate_id.display(), offset.display(), limit.display(), + "Querying events. topic: {}, substate_id: {}, offset: {}, limit: {}", topic.display(), substate_id.display(), offset.display(), limit.display()crates/common_types/src/shard_state_versions.rs (1)
80-91: Add documentation and consider validation.The logic correctly maps indices to shards (global at 0, then shard_group.start() + offset). However, the method lacks documentation and doesn't validate that the provided
shard_groupmatches the internal structure.Consider these improvements:
- Add documentation explaining the preconditions:
+ /// Converts the internal state versions into a map from shard to state version. + /// + /// # Arguments + /// * `shard_group` - The shard group that was used to construct this ShardStateVersions. + /// Must match the original shard group or the mapping will be incorrect. + /// + /// # Returns + /// An IndexMap where index 0 maps to the global shard, and subsequent indices map to + /// shards starting from shard_group.start(). pub fn convert_to_map(&self, shard_group: ShardGroup) -> IndexMap<Shard, StateVersion> {
- Add validation (optional but recommended):
pub fn convert_to_map(&self, shard_group: ShardGroup) -> IndexMap<Shard, StateVersion> { // Validate that the length matches expected structure let expected_len = shard_group.len() + 1; // +1 for global shard if self.len() != expected_len { panic!( "Length mismatch: ShardStateVersions has {} elements but shard_group expects {}", self.len(), expected_len ); } let mut map = IndexMap::with_capacity(self.len()); // ... rest of implementation }This validation would catch mismatches between the shard group and internal state, preventing silent data corruption.
integration_tests/tests/features/fungible.feature (1)
12-12: Document scenario-specific XTR funding amounts.
Extract the varied funding values into shared constants or add inline comments explaining why simple tests use 1–2 XTR while committee/transfer/NFT scenarios require 2 000 000 XTR.crates/engine/tests/test.rs (1)
152-167: Consider removing or documenting the commented debug code.The commented-out ABI-printing block serves as a development utility but adds clutter to the test file. Consider either:
- Removing it if it's no longer needed
- Moving it to a separate developer utilities module
- Adding a brief comment explaining when/why developers would uncomment it
crates/engine/src/wasm/process.rs (1)
258-261: Verify eprintln usage for production.The improved error handling with
EngineArgDecodeFailedis good, but usingeprintln!for decoding errors may not be appropriate for production. In a WASM/engine context, stderr might not be captured or monitored.Consider using the structured
log::error!macro (already available viause log::*;) for consistency with the rest of the codebase.Apply this diff to use structured logging:
- let decoded = decode_exact(&args).map_err(|e| { - eprintln!("Failed to decode args: {}", e); - WasmExecutionError::EngineArgDecodeFailed(e) - })?; + let decoded = decode_exact(&args).map_err(|e| { + log::error!(target: LOG_TARGET, "Failed to decode engine args: {}", e); + WasmExecutionError::EngineArgDecodeFailed(e) + })?;crates/engine/tests/templates/limits/src/lib.rs (1)
1-23: LGTM! Clean test template for Bytes validation.The template structure correctly uses the new
Bytestype and follows tari_template_lib conventions. The implementation is appropriately minimal for testing large payload handling.Optional: Consider adding a getter method.
If test scenarios need to verify the stored data, you might want to add a getter:
pub fn set_data(&mut self, data: Bytes) { self.data = data; } + + pub fn get_data(&self) -> &Bytes { + &self.data + }crates/engine/tests/limits.rs (1)
37-37: TODO: add tests for other engine limits.The comment notes missing test coverage for other limits (e.g., max memory pages, recursion depth). Consider creating tests for these limits in a follow-up to ensure comprehensive validation of engine safety guardrails.
Would you like me to generate a test scaffold for memory and recursion limits, or open an issue to track this task?
crates/engine_types/src/limits.rs (1)
26-36: Consider documenting the distinction betweenmax_call_sizeandmax_internal_call_size.Both limits are set to 1 MiB. While having separate limits for external and internal calls provides flexibility, a brief comment explaining when each limit applies would improve maintainability.
Consider adding documentation:
pub struct EngineLimits { pub max_substate_outputs: usize, pub max_substate_size: usize, + /// Maximum size for external/public API calls pub max_call_size: usize, + /// Maximum size for internal engine calls pub max_internal_call_size: usize, pub max_logs: usize,integration_tests/tests/features/state_sync.feature (1)
31-32: Remove or restore commented-out wait patterns.The commented wait steps suggest uncertainty about the correct synchronization approach. Either restore them if needed for test stability, or remove them to keep the test clean.
If these waits are no longer needed, apply this diff:
-# When I wait for validator VN has leaf block height of at least 1 at epoch 4 -# When I wait for validator VN2 has leaf block height of at least 1 at epoch 4 -integration_tests/tests/steps/indexer.rs (1)
222-298: Clarify epoch sync target (prev_epoch) and document is_none_or predicate
- prev_epoch = epoch.checked_sub(Epoch(1)) intentionally targets N−1; switch to epoch if you need indexer to reach the current epoch.
.is_none_or(|sv| sv > v)onOption<&StateVersion>correctly pauses when a shard is missing or the validator’s version is ahead; consider adding an inline comment (and ensure the OptionExt trait is in scope) for future readers.crates/template_lib_types/src/bytes.rs (1)
17-21: Optional: Consider removing redundantfrom_vecin favor ofFromtrait.The
from_vecmethod duplicates functionality provided by theFrom<Vec<u8>>trait (lines 37-41). While not harmful, users can simply useBytes::from(vec)orvec.into(). Consider removingfrom_vecto reduce API surface, or keep it if explicit construction is preferred for ergonomics.crates/engine_types/src/serde_with/hex.rs (1)
133-140: Consider adding a test for non-human-readable serialization.The current test only covers the human-readable (JSON/hex) path. Since the PR's main goal is to optimize byte buffer serialization in non-human-readable formats (CBOR), consider adding a test using a binary format like
ciboriumorbincodeto verify the optimization.Add a test for binary serialization:
#[test] fn test_serialize_binary() { let data = TestCase { fixed: [1; 32], vec: vec![5; 100], }; // Using ciborium for CBOR serialization let mut serialized = Vec::new(); ciborium::ser::into_writer(&data, &mut serialized).unwrap(); let deserialized: TestCase = ciborium::de::from_reader(&serialized[..]).unwrap(); assert_eq!(data.fixed, deserialized.fixed); assert_eq!(data.vec, deserialized.vec); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
Cargo.toml(1 hunks)applications/tari_app_utilities/config_presets/d_indexer.toml(1 hunks)applications/tari_indexer/src/bootstrap.rs(1 hunks)applications/tari_indexer/src/config.rs(2 hunks)applications/tari_indexer/src/graphql/model/events.rs(2 hunks)applications/tari_indexer/src/json_rpc/error.rs(1 hunks)applications/tari_indexer/src/json_rpc/handlers.rs(3 hunks)applications/tari_indexer/src/json_rpc/server.rs(2 hunks)applications/tari_indexer/src/lib.rs(1 hunks)applications/tari_indexer/src/network_state_sync/committee_client.rs(1 hunks)applications/tari_indexer/src/network_state_sync/mod.rs(1 hunks)applications/tari_indexer/src/network_state_sync/worker.rs(2 hunks)applications/tari_indexer/src/substate_manager.rs(3 hunks)applications/tari_validator_node/src/genesis_state.rs(1 hunks)applications/tari_validator_node/src/json_rpc/handlers.rs(6 hunks)applications/tari_validator_node/src/json_rpc/jrpc_errors.rs(1 hunks)applications/tari_validator_node_cli/src/command/transaction.rs(2 hunks)applications/tari_wallet_cli/src/command/transaction.rs(2 hunks)applications/tari_walletd/src/handlers/accounts.rs(3 hunks)applications/tari_walletd/src/handlers/helpers.rs(1 hunks)applications/tari_walletd/src/handlers/validator.rs(3 hunks)applications/tari_watcher/src/manager.rs(3 hunks)applications/tari_watcher/src/monitoring.rs(2 hunks)bindings/src/types/validator-node-client/GetConsensusStatusResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsTransferResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/ConfidentialTransferResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/WalletSubstateInfo.ts(1 hunks)clients/tari_indexer_client/src/json_rpc_client.rs(2 hunks)clients/tari_indexer_client/src/types.rs(2 hunks)clients/validator_node_client/Cargo.toml(1 hunks)clients/validator_node_client/src/types.rs(3 hunks)clients/wallet_daemon_client/src/types.rs(0 hunks)crates/common_types/src/shard_state_versions.rs(2 hunks)crates/consensus/src/hotstuff/state_machine/state.rs(4 hunks)crates/engine/src/runtime/engine_args.rs(2 hunks)crates/engine/src/runtime/impl.rs(3 hunks)crates/engine/src/runtime/mod.rs(0 hunks)crates/engine/src/wasm/compile.rs(2 hunks)crates/engine/src/wasm/environment.rs(1 hunks)crates/engine/src/wasm/error.rs(3 hunks)crates/engine/src/wasm/module.rs(4 hunks)crates/engine/src/wasm/process.rs(3 hunks)crates/engine/tests/limits.rs(1 hunks)crates/engine/tests/templates/buggy/Cargo.toml(1 hunks)crates/engine/tests/templates/buggy/src/lib.rs(1 hunks)crates/engine/tests/templates/composability/src/lib.rs(1 hunks)crates/engine/tests/templates/limits/Cargo.toml(1 hunks)crates/engine/tests/templates/limits/src/lib.rs(1 hunks)crates/engine/tests/test.rs(2 hunks)crates/engine_types/src/commit_result.rs(3 hunks)crates/engine_types/src/indexed_value.rs(0 hunks)crates/engine_types/src/limits.rs(1 hunks)crates/engine_types/src/serde_with/hex.rs(3 hunks)crates/p2p/src/conversions/transaction.rs(2 hunks)crates/template_abi/Cargo.toml(1 hunks)crates/template_abi/src/abi/mod.rs(1 hunks)crates/template_abi/src/version.rs(1 hunks)crates/template_lib/Cargo.toml(1 hunks)crates/template_lib/src/args/mod.rs(1 hunks)crates/template_lib/src/args/types.rs(11 hunks)crates/template_lib/src/component/manager.rs(3 hunks)crates/template_lib/src/prelude.rs(1 hunks)crates/template_lib/src/template/manager.rs(3 hunks)crates/template_lib_types/Cargo.toml(1 hunks)crates/template_lib_types/src/bytes.rs(1 hunks)crates/template_lib_types/src/engine_args.rs(2 hunks)crates/template_lib_types/src/lib.rs(1 hunks)crates/template_lib_types/src/serde_helpers.rs(5 hunks)crates/template_macros/Cargo.toml(1 hunks)crates/template_test_tooling/src/builtin_component_state.rs(2 hunks)crates/transaction/src/args.rs(4 hunks)crates/transaction/src/builder/mod.rs(1 hunks)crates/transaction/src/special_json_arg_syntax.rs(1 hunks)crates/transaction/src/transaction.rs(2 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(1 hunks)integration_tests/Cargo.toml(2 hunks)integration_tests/src/base_node.rs(1 hunks)integration_tests/src/indexer.rs(4 hunks)integration_tests/src/lib.rs(8 hunks)integration_tests/src/miner.rs(2 hunks)integration_tests/src/template.rs(1 hunks)integration_tests/src/templates/fees/Cargo.toml(1 hunks)integration_tests/src/templates/fees/src/lib.rs(1 hunks)integration_tests/src/util.rs(1 hunks)integration_tests/src/validator_node.rs(7 hunks)integration_tests/src/validator_node_cli.rs(3 hunks)integration_tests/src/wallet.rs(4 hunks)integration_tests/src/wallet_daemon.rs(2 hunks)integration_tests/src/wallet_daemon_client.rs(18 hunks)integration_tests/tests/cucumber.rs(15 hunks)integration_tests/tests/features/claim_burn.feature(2 hunks)integration_tests/tests/features/claim_fees.feature(2 hunks)integration_tests/tests/features/committee.feature(1 hunks)integration_tests/tests/features/concurrency.feature(1 hunks)integration_tests/tests/features/counter.feature(2 hunks)integration_tests/tests/features/epoch_change.feature(2 hunks)integration_tests/tests/features/eviction.feature(1 hunks)integration_tests/tests/features/fungible.feature(1 hunks)integration_tests/tests/features/indexer.feature(4 hunks)integration_tests/tests/features/leader_failure.feature(1 hunks)integration_tests/tests/features/leader_failure.feature.ignore(0 hunks)integration_tests/tests/features/nft.feature(3 hunks)integration_tests/tests/features/state_sync.feature(1 hunks)integration_tests/tests/features/substates.feature(1 hunks)integration_tests/tests/features/transfer.feature(1 hunks)integration_tests/tests/features/wallet_daemon.feature(2 hunks)integration_tests/tests/steps/indexer.rs(5 hunks)
⛔ Files not processed due to max files limit (12)
- integration_tests/tests/steps/miner.rs
- integration_tests/tests/steps/mod.rs
- integration_tests/tests/steps/network.rs
- integration_tests/tests/steps/network/mod.rs
- integration_tests/tests/steps/network/spec.rs
- integration_tests/tests/steps/network/step.rs
- integration_tests/tests/steps/outputs.rs
- integration_tests/tests/steps/validator_node.rs
- integration_tests/tests/steps/wallet.rs
- integration_tests/tests/steps/wallet_daemon.rs
- networking/core/src/error.rs
- networking/swarm/src/config.rs
💤 Files with no reviewable changes (4)
- integration_tests/tests/features/leader_failure.feature.ignore
- clients/wallet_daemon_client/src/types.rs
- crates/engine_types/src/indexed_value.rs
- crates/engine/src/runtime/mod.rs
🧰 Additional context used
🧬 Code graph analysis (42)
crates/transaction/src/args.rs (2)
bindings/src/types/InstructionArg.ts (1)
InstructionArg(7-7)crates/tari_bor/src/lib.rs (1)
decode_exact(123-132)
applications/tari_validator_node_cli/src/command/transaction.rs (2)
bindings/src/types/InstructionArg.ts (1)
InstructionArg(7-7)crates/transaction/src/args.rs (1)
raw_literal_bytes(85-87)
integration_tests/src/miner.rs (1)
integration_tests/src/util.rs (1)
cucumber_log(12-17)
applications/tari_watcher/src/manager.rs (1)
applications/tari_watcher/src/monitoring.rs (3)
process_status_alert(163-239)process_status_log(97-128)new(23-25)
crates/transaction/src/special_json_arg_syntax.rs (1)
bindings/src/types/InstructionArg.ts (1)
InstructionArg(7-7)
applications/tari_indexer/src/json_rpc/handlers.rs (2)
clients/tari_indexer_client/src/json_rpc_client.rs (1)
get_network_sync_state(170-172)applications/tari_indexer/src/json_rpc/error.rs (1)
internal_error(13-26)
applications/tari_validator_node/src/genesis_state.rs (1)
bindings/src/types/Metadata.ts (1)
Metadata(6-6)
crates/template_test_tooling/src/builtin_component_state.rs (1)
crates/engine_types/src/resource.rs (1)
metadata(203-205)
crates/engine/src/wasm/process.rs (1)
crates/tari_bor/src/lib.rs (1)
decode_exact(123-132)
integration_tests/src/util.rs (1)
utilities/tariswap_test_bench/src/timer.rs (1)
info(32-34)
integration_tests/src/indexer.rs (3)
integration_tests/src/helpers.rs (4)
check_join_handle(104-123)get_address_from_output(125-139)get_os_assigned_ports(21-23)wait_listener_on_local_port(55-102)integration_tests/src/logging.rs (1)
get_base_dir_for_scenario(32-42)integration_tests/src/util.rs (1)
cucumber_log(12-17)
bindings/src/types/wallet-daemon-client/AccountsTransferResponse.ts (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
applications/tari_wallet_cli/src/command/transaction.rs (3)
bindings/src/types/wallet-daemon-client/TransactionWaitResultRequest.ts (1)
TransactionWaitResultRequest(4-4)crates/engine_types/src/fees.rs (1)
total_refunded(34-38)applications/tari_validator_node_cli/src/command/transaction.rs (1)
summarize_finalize_result(428-512)
crates/engine_types/src/commit_result.rs (1)
bindings/src/types/SubstateDiff.ts (1)
SubstateDiff(6-10)
integration_tests/src/validator_node.rs (4)
integration_tests/src/util.rs (1)
cucumber_log(12-17)integration_tests/src/helpers.rs (2)
get_os_assigned_ports(21-23)get_os_assigned_port(16-19)integration_tests/tests/cucumber.rs (3)
world(316-316)world(523-523)world(629-638)integration_tests/tests/steps/validator_node.rs (2)
world(104-113)world(457-461)
applications/tari_walletd/src/handlers/accounts.rs (3)
bindings/src/types/wallet-daemon-client/AccountsTransferResponse.ts (1)
AccountsTransferResponse(4-4)applications/tari_walletd/src/handlers/context.rs (2)
transaction_service(85-87)notifier(55-57)bindings/src/types/wallet-daemon-client/ConfidentialTransferResponse.ts (1)
ConfidentialTransferResponse(4-4)
crates/wallet/sdk/src/apis/confidential_transfer.rs (4)
crates/engine_types/src/resource_container.rs (2)
resource_address(176-183)resource_type(185-192)crates/engine_types/src/vault.rs (2)
resource_address(122-124)resource_type(126-128)crates/template_lib/src/models/vault.rs (3)
resource_address(138-143)resource_address(348-357)resource_type(360-362)crates/engine_types/src/resource.rs (1)
resource_type(107-109)
integration_tests/tests/cucumber.rs (5)
integration_tests/src/wallet.rs (1)
spawn_minotari_wallet(116-225)integration_tests/src/wallet_daemon.rs (1)
spawn_wallet_daemon(70-113)integration_tests/src/validator_node_cli.rs (1)
create_component(20-65)integration_tests/src/wallet_daemon_client.rs (4)
call_component(695-763)concurrent_call_component(765-819)submit_manifest(477-560)submit_manifest_with_signing_keys(391-475)clients/wallet_daemon_client/src/lib.rs (1)
list_account_nfts(413-418)
crates/transaction/src/builder/mod.rs (1)
bindings/src/types/InstructionArg.ts (1)
InstructionArg(7-7)
integration_tests/src/lib.rs (3)
bindings/src/types/SubstateRequirement.ts (1)
SubstateRequirement(3-3)bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)crates/consensus/src/consensus_constants.rs (1)
devnet(53-69)
bindings/src/types/wallet-daemon-client/ConfidentialTransferResponse.ts (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
integration_tests/src/validator_node_cli.rs (3)
bindings/src/helpers/consts.ts (1)
ACCOUNT_TEMPLATE_ADDRESS(6-6)crates/engine_types/src/substate.rs (1)
component(590-595)crates/engine_types/src/resource.rs (1)
token_symbol(207-209)
clients/tari_indexer_client/src/types.rs (5)
bindings/src/types/Shard.ts (1)
Shard(3-3)bindings/src/types/SubstateType.ts (1)
SubstateType(3-12)bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NumPreshards.ts (1)
NumPreshards(3-3)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)
integration_tests/src/template.rs (1)
integration_tests/src/wallet_daemon_client.rs (1)
get_auth_wallet_daemon_client(904-906)
bindings/src/types/validator-node-client/GetConsensusStatusResponse.ts (1)
bindings/src/types/Shard.ts (1)
Shard(3-3)
integration_tests/src/wallet_daemon.rs (1)
integration_tests/src/util.rs (1)
cucumber_log(12-17)
integration_tests/tests/steps/indexer.rs (3)
integration_tests/src/indexer.rs (1)
spawn_indexer(129-205)integration_tests/src/util.rs (1)
cucumber_log(12-17)bindings/src/types/Epoch.ts (1)
Epoch(3-3)
clients/validator_node_client/src/types.rs (1)
bindings/src/types/Shard.ts (1)
Shard(3-3)
clients/tari_indexer_client/src/json_rpc_client.rs (1)
applications/tari_indexer/src/json_rpc/handlers.rs (1)
get_network_sync_state(701-731)
crates/engine/src/wasm/module.rs (3)
bindings/src/types/FunctionDef.ts (1)
FunctionDef(5-5)bindings/src/types/TemplateDef.ts (1)
TemplateDef(4-4)crates/template_macros/src/template/abi.rs (2)
func(50-54)tuple(190-201)
integration_tests/src/base_node.rs (1)
integration_tests/src/util.rs (1)
cucumber_log(12-17)
integration_tests/src/wallet.rs (1)
integration_tests/src/util.rs (1)
cucumber_log(12-17)
applications/tari_indexer/src/substate_manager.rs (2)
applications/tari_indexer/src/bootstrap.rs (1)
tx(345-345)applications/tari_indexer/src/network_state_sync/worker.rs (1)
tx(154-154)
crates/engine/src/wasm/compile.rs (1)
applications/tari_swarm_daemon/src/process_manager/instances/instance.rs (1)
envs(76-78)
applications/tari_validator_node/src/json_rpc/handlers.rs (2)
applications/tari_validator_node/src/json_rpc/jrpc_errors.rs (2)
internal_error(47-60)not_found(62-71)bindings/src/types/validator-node-client/GetConsensusStatusResponse.ts (1)
GetConsensusStatusResponse(7-12)
bindings/src/types/wallet-daemon-client/WalletSubstateInfo.ts (2)
bindings/src/types/SubstateId.ts (1)
SubstateId(6-6)bindings/src/types/Hash.ts (1)
Hash(6-6)
crates/p2p/src/conversions/transaction.rs (1)
crates/transaction/src/args.rs (1)
raw_literal_bytes(85-87)
crates/common_types/src/shard_state_versions.rs (2)
bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)crates/common_types/src/shard.rs (1)
global(29-31)
crates/engine/tests/limits.rs (1)
bindings/src/types/RejectReason.ts (1)
RejectReason(4-12)
crates/engine_types/src/serde_with/hex.rs (1)
crates/template_lib_types/src/crypto/range_proof.rs (1)
serde_helpers(67-67)
crates/template_abi/src/abi/mod.rs (1)
crates/engine/tests/templates/buggy/src/lib.rs (1)
tari_engine(58-58)
integration_tests/src/wallet_daemon_client.rs (8)
bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)integration_tests/src/validator_node_cli.rs (1)
add_outputs_from_diff(67-165)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/wallet-daemon-client/AccountsCreateRequest.ts (1)
AccountsCreateRequest(3-3)bindings/src/types/wallet-daemon-client/AccountsCreateFreeTestCoinsRequest.ts (1)
AccountsCreateFreeTestCoinsRequest(5-9)bindings/src/types/wallet-daemon-client/TransactionWaitResultRequest.ts (1)
TransactionWaitResultRequest(4-4)
⏰ 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). (3)
- GitHub Check: check nightly
- GitHub Check: clippy
- GitHub Check: test
| pub async fn new_session(&mut self) -> Result<ValidatorRpcSession, ValidatorCommitteeClientError> { | ||
| let epoch = self.epoch_manager.get_current_epoch(); | ||
| let member = self | ||
| .epoch_manager | ||
| .get_random_committee_member(epoch, Some(self.shard_group), self.past_failed_nodes.clone()) | ||
| .await?; | ||
| self.session_for_peer(member.address).await | ||
| loop { | ||
| let member = self | ||
| .epoch_manager | ||
| .get_random_committee_member(epoch, Some(self.shard_group), self.past_failed_nodes.clone()) | ||
| .await?; | ||
| let result = self.session_for_peer(member.address).await; | ||
| match result { | ||
| Ok(session) => return Ok(session), | ||
| Err(err) => { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "Failed to create session for validator '{}': {}", member, err | ||
| ); | ||
| self.past_failed_nodes.push(member.address); | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Potential infinite loop risk in new_session retry logic.
The indefinite retry loop (lines 43-59) lacks a termination condition and could block forever if:
- All committee members fail repeatedly
get_random_committee_membercontinues returning members despite all having failed- No timeout is enforced at this layer
Additionally, past_failed_nodes accumulates across calls and is never cleared, which may exhaust the available pool over time.
Consider adopting the pattern from try_with_random_members (lines 91-99), which properly handles exhaustion:
pub async fn new_session(&mut self) -> Result<ValidatorRpcSession, ValidatorCommitteeClientError> {
let epoch = self.epoch_manager.get_current_epoch();
+ let mut attempted = vec![];
loop {
- let member = self
+ let Some(member) = self
.epoch_manager
- .get_random_committee_member(epoch, Some(self.shard_group), self.past_failed_nodes.clone())
- .await?;
+ .get_random_committee_member(epoch, Some(self.shard_group), attempted.clone())
+ .await
+ .optional()?
+ else {
+ return Err(ValidatorCommitteeClientError::AllValidatorsFailed {
+ committee_size: attempted.len(),
+ last_error: Some("No more committee members available".to_string()),
+ });
+ };
+
let result = self.session_for_peer(member.address).await;
match result {
Ok(session) => return Ok(session),
Err(err) => {
warn!(
target: LOG_TARGET,
"Failed to create session for validator '{}': {}", member, err
);
- self.past_failed_nodes.push(member.address);
+ attempted.push(member.address);
},
}
}
}Verify the behavior of get_random_committee_member when the excluded list exhausts all committee members:
🏁 Script executed:
#!/bin/bash
# Description: Check how get_random_committee_member handles exhaustion of available members
# Search for the implementation of get_random_committee_member
ast-grep --pattern $'fn get_random_committee_member($$$) {
$$$
}'
# Also check for Optional-related return handling
rg -nP -A10 'fn get_random_committee_member' --type rustLength of output: 4828
Prevent unbounded retries and reset failure state in new_session
new_session currently loops forever on session_for_peer errors and never clears past_failed_nodes. Add a max retry limit (or timeout/backoff) to avoid infinite loops if all peers fail, and reset past_failed_nodes at the start of each call.
🤖 Prompt for AI Agents
In applications/tari_indexer/src/network_state_sync/committee_client.rs around
lines 41–60, new_session currently resets no state and may loop forever when
session_for_peer keeps failing; reset self.past_failed_nodes at the start of the
function and add a bounded retry policy (e.g. const MAX_ATTEMPTS = N) or a
timeout/backoff loop: clear past_failed_nodes, perform up to MAX_ATTEMPTS
attempts calling get_random_committee_member and session_for_peer, on each
failure push the failed address and await a short backoff (tokio::time::sleep
with exponential backoff) before retrying, and if attempts are exhausted return
a ValidatorCommitteeClientError indicating no available peers rather than
looping forever.
There was a problem hiding this comment.
get_random_committee_member will error if no member is found
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementation of get_random_committee_member to verify error handling
ast-grep --pattern 'async fn get_random_committee_member($$$) -> $$$ {
$$$
}'
Length of output: 4273
🏁 Script executed:
#!/bin/bash
# Search for how get_random_committee_member handles the case when no members are available
rg -nP -B3 -A15 'pub async fn get_random_committee_member' --type rust
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how past_failed_nodes is used elsewhere in the codebase
rg -nP 'past_failed_nodes' --type rust -C3
Length of output: 3757
@sdbondi, thank you for the clarification! You're absolutely right—the ? operator on line 47 will propagate the error from get_random_committee_member when no members are available, which prevents the infinite loop concern CodeRabbit raised.
Looking at the test implementation, when all validators are excluded, it returns:
EpochManagerError::StorageError(StorageError::NotFound {
item: "validator_nodes",
key: format!("in shard group {shard_group}"),
})So the loop will terminate with an error rather than running forever.
One design note: past_failed_nodes accumulates across multiple new_session calls and is never cleared (unlike try_with_random_members which uses a local attempted vec). This appears intentional—to maintain a persistent failure list across the lifetime of the pool and avoid repeatedly trying nodes that have failed before. If this is the intended behavior, the current implementation is fine.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/tari_walletd/web_ui/src/App.tsx (1)
282-289: Fix redirect target for Stealth UTXOs to include defaultresource_address
The GuardedRoute’sredirectcurrently points to/stealth-utxos—no matching route exists for the base path. Update it to:import { XTR } from "@tari-project/typescript-bindings"; <GuardedRoute isAuthenticated={isAuthenticated} redirect={`/stealth-utxos/${XTR}`} component={StealthUtxoListPage} />—or alternatively add a
Route path="stealth-utxos"that redirects to the default segment.
🧹 Nitpick comments (2)
applications/tari_walletd/web_ui/src/components/Breadcrumbs.tsx (1)
45-46: Consider consistent text transformation approach.The last breadcrumb applies
toLowerCase()twice: once in the style transform and once on the label itself (line 46). Consider using only CSStext-transform: 'lowercase'for consistency with other breadcrumb items.Apply this diff to rely solely on CSS transformation:
- <span key={breadcrumbLabel} style={{ textTransform: "lowercase", fontWeight: "bold" }}> - {breadcrumbLabel.toLowerCase()} + <span key={breadcrumbLabel} style={{ textTransform: "lowercase", fontWeight: "bold" }}> + {breadcrumbLabel} </span>applications/tari_walletd/web_ui/src/routes/StealthUtxoList/StealthUtxoList.tsx (1)
43-44: Handle missing balance data gracefully.If
balancesDatais undefined or the resource isn't found in balances,currencySymbolbecomes an empty string. While this prevents crashes, displaying currency values without a symbol (line 106) might confuse users.Consider adding a fallback symbol or loading state:
- const resourceBalance = balancesData?.balances?.find((balance) => balance.resource_address === resourceAddress); - const currencySymbol = resourceBalance ? resourceBalance.token_symbol || "" : ""; + const resourceBalance = balancesData?.balances?.find((balance) => balance.resource_address === resourceAddress); + const currencySymbol = resourceBalance?.token_symbol || (resourceAddress === XTR ? "XTR" : "");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
applications/tari_walletd/web_ui/src/App.tsx(2 hunks)applications/tari_walletd/web_ui/src/components/Breadcrumbs.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/Tokens.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/StealthUtxoList/StealthUtxoList.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
applications/tari_walletd/web_ui/src/routes/StealthUtxoList/StealthUtxoList.tsx (5)
bindings/src/types/Account.ts (1)
Account(4-10)bindings/src/types/OutputStatus.ts (1)
OutputStatus(3-3)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1)
useAccountsGetBalances(263-277)applications/tari_walletd/web_ui/src/utils/helpers.tsx (1)
substateIdToString(100-109)bindings/src/helpers/consts.ts (1)
XTR(10-10)
⏰ 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). (6)
- GitHub Check: test
- GitHub Check: file licenses
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: machete
🔇 Additional comments (5)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/Tokens.tsx (1)
114-120: LGTM! Navigation updated to use dynamic resource routing.The change correctly passes
resource_addressas a route parameter, enabling resource-specific stealth UTXO views. This aligns with the dynamic routing pattern introduced in App.tsx.applications/tari_walletd/web_ui/src/App.tsx (1)
126-129: LGTM! Breadcrumb route configuration updated for dynamic routing.The breadcrumb route correctly reflects the new parameterized path with
dynamic: true, enabling proper breadcrumb rendering for resource-specific stealth UTXO pages.applications/tari_walletd/web_ui/src/routes/StealthUtxoList/StealthUtxoList.tsx (2)
40-41: LGTM! Route parameter handling with sensible default.The component correctly reads
resource_addressfrom route parameters and defaults toXTR, providing a consistent user experience when accessing the route without a parameter.
59-63: LGTM! Clean integration with API hook.The hook invocation correctly passes the route-derived
resourceAddressand status filter, simplifying the component logic compared to the previous implementation.applications/tari_walletd/web_ui/src/components/Breadcrumbs.tsx (1)
41-64: Use actual path for undefined breadcrumb segments
The fallback branch currently links any unmatched breadcrumb to “/”. Either ensure every route segment is covered by yourbreadcrumbRoutessomatch.routeis never undefined, or update the else‐case to use the actual segment path (e.g.to={match.pathname}) instead of hardcoding “/”.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clients/tari_indexer_client/src/types.rs (1)
406-436: Fix duplicate TypeScript rename attribute causing type collisions.Line 413 uses
rename = "IndexerGetEpochManagerStatsResponse", which is the same rename already used byGetEpochManagerStatsResponseat line 308. This causes a TypeScript export name collision where both structs overwrite the same exported type.Additionally,
NetworkDescription(line 422) andSyncProgress(line 431) lack explicitrenameattributes, resulting in inconsistent naming compared to other types in this file that use the "Indexer" prefix convention.Apply this diff to fix the TypeScript rename attributes:
#[derive(Debug, Clone, Serialize, Deserialize)] #[cfg_attr( feature = "ts", derive(ts_rs::TS), ts( export, export_to = "tari-indexer-client/", - rename = "IndexerGetEpochManagerStatsResponse" + rename = "IndexerGetNetworkSyncStateResponse" ) )] pub struct GetNetworkSyncStateResponse { pub network_desc: NetworkDescription, pub sync_progress: Option<SyncProgress>, } #[derive(Debug, Clone, Serialize, Deserialize)] -#[cfg_attr(feature = "ts", derive(ts_rs::TS), ts(export, export_to = "tari-indexer-client/"))] +#[cfg_attr( + feature = "ts", + derive(ts_rs::TS), + ts(export, export_to = "tari-indexer-client/", rename = "IndexerNetworkDescription") +)] pub struct NetworkDescription { pub epoch: Epoch, // (shard group, num members) pub shard_groups: Vec<(ShardGroup, u32)>, pub num_preshards: NumPreshards, } #[derive(Debug, Clone, Serialize, Deserialize)] -#[cfg_attr(feature = "ts", derive(ts_rs::TS), ts(export, export_to = "tari-indexer-client/"))] +#[cfg_attr( + feature = "ts", + derive(ts_rs::TS), + ts(export, export_to = "tari-indexer-client/", rename = "IndexerSyncProgress") +)] pub struct SyncProgress { pub last_epoch: Epoch, pub checkpoint_progress: Vec<(ShardGroup, Epoch)>, pub last_state_versions: Vec<(Shard, (StateVersion, Epoch))>, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
applications/tari_indexer/src/graphql/model/events.rs(2 hunks)applications/tari_indexer/src/json_rpc/error.rs(2 hunks)applications/tari_indexer/src/json_rpc/handlers.rs(4 hunks)applications/tari_validator_node/src/json_rpc/jrpc_errors.rs(2 hunks)bindings/src/tari-indexer-client.ts(2 hunks)bindings/src/types/tari-indexer-client/NetworkDescription.ts(1 hunks)bindings/src/types/tari-indexer-client/SyncProgress.ts(1 hunks)clients/tari_indexer_client/src/types.rs(2 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(1 hunks)integration_tests/src/templates/fees/Cargo.toml(1 hunks)integration_tests/tests/features/claim_fees.feature(2 hunks)integration_tests/tests/features/substates.feature(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- integration_tests/src/templates/fees/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- applications/tari_validator_node/src/json_rpc/jrpc_errors.rs
- crates/wallet/sdk/src/apis/confidential_transfer.rs
- applications/tari_indexer/src/graphql/model/events.rs
- applications/tari_indexer/src/json_rpc/error.rs
🧰 Additional context used
🧬 Code graph analysis (4)
bindings/src/types/tari-indexer-client/SyncProgress.ts (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)bindings/src/types/Shard.ts (1)
Shard(3-3)
bindings/src/types/tari-indexer-client/NetworkDescription.ts (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)bindings/src/types/NumPreshards.ts (1)
NumPreshards(3-3)
applications/tari_indexer/src/json_rpc/handlers.rs (4)
bindings/src/types/tari-indexer-client/NetworkDescription.ts (1)
NetworkDescription(6-10)bindings/src/types/tari-indexer-client/SyncProgress.ts (1)
SyncProgress(7-11)clients/tari_indexer_client/src/json_rpc_client.rs (1)
get_network_sync_state(170-172)applications/tari_indexer/src/json_rpc/error.rs (1)
internal_error(13-29)
clients/tari_indexer_client/src/types.rs (6)
bindings/src/types/Shard.ts (1)
Shard(3-3)bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/NumPreshards.ts (1)
NumPreshards(3-3)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)bindings/src/types/tari-indexer-client/NetworkDescription.ts (1)
NetworkDescription(6-10)bindings/src/types/tari-indexer-client/SyncProgress.ts (1)
SyncProgress(7-11)
⏰ 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: check nightly
- GitHub Check: machete
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: check stable
🔇 Additional comments (17)
integration_tests/tests/features/claim_fees.feature (10)
9-19: LGTM! Declarative network spec improves test maintainability.The migration to a declarative network specification consolidates setup and makes the test more readable. The spec correctly defines the validator VN with fee_claim_account VN_FEES, wallet daemon WALLET_D with access to VN_FEES, and indexer IDX.
22-24: Fee generation setup is clear.The test creates an account with 10 XTR, publishes a template, and runs up 550k in fees. This provides sufficient setup for the fee claim scenarios.
32-33: Past review comment addressed.The comment has been updated from "Claim fees into ACC2" to "Claim fees into VN_FEES" to match the implementation.
30-30: Initial balance check ensures clean state.Verifying that VN_FEES starts at exactly 0 before the claim establishes a known baseline for the subsequent balance assertions.
35-36: Conservative balance threshold reduces test brittleness.Checking for "at least 500000" when 550000 fees were generated accounts for the claim transaction's own fee costs, making the test more robust.
38-38: Verify removal of @fixed tag.The first scenario uses
@serial @fixedwhile this scenario uses only@serial. Ensure the removal of the@fixedtag is intentional.
40-50: Network spec duplication is standard for independent scenarios.Each Gherkin scenario should be independently executable, so duplicating the network spec is expected and correct.
62-62: Negative test ensures claim authorization.Testing that claiming fees with the wrong account (ACC1) fails correctly validates the authorization logic.
64-69: Higher balance threshold appropriate for double-claim test.This scenario checks for "at least 550000" compared to the first scenario's "at least 500000". The stricter threshold is appropriate here because this test is specifically verifying the exact behavior of double claims, and the comment correctly notes the actual fees are "just over 600k".
71-73: Second claim logic correctly validates fee-from-fee behavior.The comment and assertion correctly capture that the second claim succeeds (because the first claim transaction itself generated a fee) but only yields a small additional amount. The upper bound of 700000 appropriately constrains the test to verify no double-counting occurs.
integration_tests/tests/features/substates.feature (1)
12-12: Approve funding amount 10 XTR initial funding is sufficient and consistent with other tests; no action needed.bindings/src/tari-indexer-client.ts (1)
16-16: LGTM!The new exports for
NetworkDescriptionandSyncProgressfollow the existing pattern and correctly expose the types needed for the network sync state API.Also applies to: 41-41
bindings/src/types/tari-indexer-client/SyncProgress.ts (1)
1-11: LGTM!The generated
SyncProgresstype correctly represents the network synchronization progress with proper TypeScript types forlast_epoch,checkpoint_progress, andlast_state_versions.bindings/src/types/tari-indexer-client/NetworkDescription.ts (1)
1-10: LGTM!The generated
NetworkDescriptiontype correctly represents the network configuration with proper TypeScript types forepoch,shard_groups, andnum_preshards.clients/tari_indexer_client/src/types.rs (1)
18-25: LGTM!The updated imports correctly include the types needed for the new network sync state structures (
Shard,Epoch,NumPreshards,ShardGroup,StateVersion).applications/tari_indexer/src/json_rpc/handlers.rs (2)
49-49: LGTM!The new imports for
GetNetworkSyncStateResponse,NetworkDescription, andSyncProgressare correctly added to support the new network sync state endpoint.Also applies to: 74-74, 77-77
701-731: LGTM!The new
get_network_sync_statehandler correctly:
- Fetches network description from the epoch manager
- Optionally retrieves sync progress from the substate manager
- Maps internal structures to API response types with proper error handling
- Constructs and returns the
GetNetworkSyncStateResponse
| if limit > 1000 { | ||
| return Err(JsonRpcResponse::error( | ||
| answer_id.clone(), | ||
| JsonRpcError::new( | ||
| JsonRpcErrorReason::InvalidParams, | ||
| "Limit cannot be greater than 1000".to_string(), | ||
| json::Value::Null, | ||
| ), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Remove unreachable duplicate limit check.
The check at line 851 is unreachable because:
- Line 839 already rejects requests where
req.limit > 1000 - Line 850 uses
unwrap_or(100), solimitcan only exceed 1000 ifreq.limitwasSome(value > 1000) - That case is already handled by the first check
Apply this diff to remove the duplicate check:
let limit = req.limit.unwrap_or(100);
- if limit > 1000 {
- return Err(JsonRpcResponse::error(
- answer_id.clone(),
- JsonRpcError::new(
- JsonRpcErrorReason::InvalidParams,
- "Limit cannot be greater than 1000".to_string(),
- json::Value::Null,
- ),
- ));
- }
let transactions = self🤖 Prompt for AI Agents
In applications/tari_indexer/src/json_rpc/handlers.rs around lines 851-860,
there's an unreachable duplicate check that returns an error when limit > 1000;
remove that redundant if-block (the check and its JsonRpcResponse::error return)
because req.limit > 1000 is already handled earlier (line ~839) and limit is
derived with unwrap_or(100), so this branch can never be hit.
Test Results (CI)71 tests - 370 67 ✅ - 361 8m 32s ⏱️ - 1h 15m 1s For more details on these failures, see this check. Results for commit 905a096. ± Comparison against base commit 3873c11. This pull request removes 370 tests. |
905a096 to
0abbe63
Compare
2 participants
Description
fix(engine)!: template calls with large bytes (> 64k) work
fix(engine): fix possible use after free in template ABI
fix(engine): adds Bytes type to improve serialization of byte buffers for template developers
fix(network): increase gossip size to allow 2Mib payloads to allow for bigger transactions to finalize more quickly
fix(tests): integration tests output management reworked to make it easier to use the correct accounts/resources
fix(tests): fix integration tests (may still break in CI due to rocksdb, too many open files)
minimum template version 0.13.0 -> 0.14.0
Motivation and Context
Due to lack of specialisation in rust, an implementation (e.g. ciborium) cannot implement serialisation for
Vec<T>and have optimal serialisation forVec<u8>unless crates like serde_with or custom implementations (#[serde(with = "...")]) are used.This causes byte arrays to serialize in CBOR (roughly) as
{array}{len}{integer}{value}{integer}{value}....instead of{bytes}{value}. This causes OOMs in templates for large byte vecs and greatly inflates the storage cost of byte vecs.This PR fixes some of the internal de/serialisation to represent bytes in a more optimal way for engine calls and adds the Bytes type that template devs can use to reduce storage costs.
Vec<u8>in a component is still serialised in the inflated way (perhaps the template macro could detect and add the required attributes but that is left for another time).All cucumbers use the new network spec step for initial setup.
How Has This Been Tested?
Cucumber tests, existing tests, new unit tests
What process can a PR reviewer use to test or verify this change?
Run cucumbers
Breaking Changes
BREAKING CHANGE: all templates will need to be recompiled to use ABI 0.14.0
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Chores