[WIP] chore: refactor core components #7332
Conversation
WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application/Wallet
participant TC as tari_transaction_components
participant KM as KeyManager (Wrapper/Inner)
participant Consensus as ConsensusManager
participant POW as ProofOfWork
App->>TC: Construct Transaction
TC->>KM: Derive Keys, Signatures, Commitments
TC->>Consensus: Get Consensus Constants for Height
TC->>POW: Validate/Compute Proof of Work
Consensus->>App: Return Network Parameters, Emission, Maturity
POW->>App: Return Difficulty/Validation Result
KM->>App: Return Keys/Signatures/Proofs
Estimated code review effort4 (~90 minutes) Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (3)
base_layer/transaction_components/src/emission.rs (1)
353-356: Unit-test imports still reference the removedtransactionsmoduleThe test module still pulls
MicroMinotariandTfrom the old path which will not compile after the refactor.-use crate::{ - consensus::emission::{Emission, EmissionSchedule}, - transactions::tari_amount::{MicroMinotari, T}, -}; +use crate::{ + consensus::emission::{Emission, EmissionSchedule}, + tari_amount::{MicroMinotari, T}, +};base_layer/common_types/src/lib.rs (1)
41-43: Gate the new WASM module behind a target-specific cfg
mod wasm;pulls inwasm-bindgen, which won’t compile for native targets. Wrap the declaration with a target check to avoid build breaks on non-wasm platforms.pub mod seeds; -mod wasm; + +// Only build the WASM bindings when compiling for `wasm32`. +#[cfg(target_arch = "wasm32")] +mod wasm;base_layer/core/src/consensus/consensus_encoding.rs (1)
23-26: Spelling error in module name will break imports
domain_seperated_consensus_hasher→domain_separated_consensus_hasherThe misspelling propagates to both the
moddeclaration and the public re-export and will fail to compile if the file uses the correct English spelling internally.-mod domain_seperated_consensus_hasher; -pub use domain_seperated_consensus_hasher::DomainSeparatedConsensusHasher; +mod domain_separated_consensus_hasher; +pub use domain_separated_consensus_hasher::DomainSeparatedConsensusHasher;Please rename the file accordingly (
domain_separated_consensus_hasher.rs).
♻️ Duplicate comments (1)
base_layer/transaction_components/src/transaction_components/payment_id.rs (1)
750-758: Stale import in test module will break the buildSame issue as above – update the path or the tests will not compile.
🧹 Nitpick comments (22)
hashing/src/layer2.rs (1)
13-13: Drop the superfluous blank linesThe extra blank lines don’t add clarity and break the prevailing formatting style in
hashing.
Remove them to stay consistent with the crate’s code style.Also applies to: 56-56
base_layer/core/Cargo.toml (1)
31-31: Gate theledgerfeature behind an explicit crate feature
transaction_components = { workspace = true, features = ["ledger"] }pulls in the (heavy) ledger code for every build oftari_core, even when theledgerfeature ontari_coreitself is disabled.
Consider:- tari_transaction_components = { workspace = true, features = ["ledger"] } +tari_transaction_components = { workspace = true, default-features = false, optional = true, features = ["ledger"] } [features] # ... ledger = ["minotari_ledger_wallet_comms", "tari_transaction_components/ledger"]This keeps compile times down for non-ledger builds.
base_layer/transaction_components/README.md (1)
5-6: Tautology / duplicated wording“definitions for common classes and traits, such as Transactions and Blocks, such as Transactions, Blocks etc.”
The phrase repeats itself. Consider:
-This crate contains definitions for common classes and traits, such as Transactions and Blocks, such as Transactions, -Blocks etc. +This crate defines the common data structures and traits used in Tari +transactions and blocks.base_layer/transaction_components/src/transaction_components/transaction_kernel.rs (1)
41-47: Flatten the nestedusegroup for clarityNested brace groups are valid Rust but are uncommon and hurt readability. A flat list is clearer and avoids future merge-conflict pain.
-use crate::{ - consensus::DomainSeparatedConsensusHasher, - { - tari_amount::MicroMinotari, - transaction_components::{KernelFeatures, TransactionError}, - transaction_protocol::TransactionMetadata, - }, -}; +use crate::{ + consensus::DomainSeparatedConsensusHasher, + tari_amount::MicroMinotari, + transaction_components::{KernelFeatures, TransactionError}, + transaction_protocol::TransactionMetadata, +};Cargo.toml (1)
78-78: Consider pinning a version for the new dependency
[workspace.dependencies] tari_transaction_components = { path = "base_layer/transaction_components" }
Pinning aversion = "0.x"(and maintaining it in the crate’sCargo.toml) allows downstream crates outside the workspace to depend on a published version unambiguously.base_layer/transaction_components/src/aggregated_body.rs (1)
41-44: Good move, but import block can be simplifiedLooks correct functionally; consider splitting the long
cfgimport or grouping it with the subsequentcrate::{ … }block for readability.base_layer/transaction_components/src/consensus/consensus_encoding.rs (1)
23-26: Duplicate hasher implementation across crates
DomainSeparatedConsensusHashernow exists in bothcoreandtransaction_components.
Unless the intent is forked implementations, prefer defining it once (e.g., intransaction_components) and re-exporting fromcoreto avoid code drift.base_layer/common_types/src/seeds/mod.rs (1)
23-32: Add minimal API docs for new public modules and constantsAll the sub-modules and string constants are now part of the public API, but there are no
//!or///docs describing their purpose.
Adding one-line doc comments will:
- make
cargo docoutput usable,- help downstream crates understand the domain separation labels,
- avoid the “missing_docs” lint once we turn it on.
No code change required – just short comments.
base_layer/transaction_components/src/key_manager/schema.rs (1)
3-22: Consider indexing & nullable considerations for Diesel schemaThe schema is fine for a first cut, but a couple of tweaks will help in production:
public_keywill likely be queried frequently – add a DB index or UNIQUE constraint now to avoid a costly migration later.timestamp -> Timestampmaps toTIMESTAMP WITHOUT TIME ZONEin PostgreSQL; make sure you always store UTC to prevent ambiguity.- SQLite uses
BLOBforBinary; confirm thatprimary_key_indexdoes not exceed the 1 GiB BLOB limit.No immediate code breakage, just something to address before main-net.
base_layer/transaction_components/src/consensus/mod.rs (1)
24-33: Export order is fine – add module-level docs laterThe re-export hub looks good and compiles cleanly.
Once the WIP flag drops, add a brief//!explaining how the consensus layer is organised – it helps new contributors navigate the crate.base_layer/key_manager/src/lib.rs (1)
5-5: Remove the commented out code.Since the key manager functionality has been moved to the transaction_components crate, this commented line should be removed entirely rather than left as a comment.
-// pub mod key_manager;base_layer/transaction_components/src/consensus/network.rs (1)
1-1: Update copyright year to 2025.This is a new file, so the copyright year should reflect the current year (2025) rather than 2019.
-// Copyright 2019. The Tari Project +// Copyright 2025. The Tari Projectbase_layer/common_types/src/seeds/seed_words.rs (3)
9-14: Remove unnecessary empty lines in imports.use crate::seeds::{ error::MnemonicError, }; - -
43-49: Use the Option API for bounds checking.The manual bounds checking can be replaced with the more idiomatic
get()method.pub fn get_word(&self, index: usize) -> Result<&String, MnemonicError> { - if index > self.len() - 1 { - return Err(MnemonicError::IndexOutOfBounds); - } - - Ok(self.words[index].reveal()) + self.words + .get(index) + .map(|word| word.reveal()) + .ok_or(MnemonicError::IndexOutOfBounds) }
119-119: Fix typo in comment.- // should not derefence, in practice. We do it here, for testing purposes + // should not dereference, in practice. We do it here, for testing purposesbase_layer/transaction_components/src/key_manager/error.rs (2)
49-49: Remove commented out importThis commented out import should be removed to keep the code clean.
-// use crate::error::WalletStorageError;
43-47: Error conversions lose type informationThe manual
Fromimplementations convert errors to strings, which loses the original error type. While this is consistent with the codebase pattern (as noted in the learnings), consider if preserving the error type would be beneficial for error handling upstream.For future consideration: If error type information becomes important for handling specific error cases, you might want to store the original error types instead of converting to strings.
Also applies to: 95-105, 146-156
base_layer/common_types/src/seeds/cipher_seed.rs (1)
72-73: Fix inconsistent spacing in hidden_type! macro callsThe spacing in the macro calls is inconsistent.
-hidden_type!(CipherSeedEncryptionKey, SafeArray<u8, CIPHER_SEED_ENCRYPTION_KEY_BYTES>); -hidden_type!(CipherSeedMacKey, SafeArray< u8, CIPHER_SEED_MAC_KEY_BYTES>); +hidden_type!(CipherSeedEncryptionKey, SafeArray<u8, CIPHER_SEED_ENCRYPTION_KEY_BYTES>); +hidden_type!(CipherSeedMacKey, SafeArray<u8, CIPHER_SEED_MAC_KEY_BYTES>);base_layer/transaction_components/src/consensus/consensus_manager.rs (2)
151-162: Consider using typed errors instead of string errorsThe error handling uses string formatting which makes it harder to handle specific error cases programmatically.
Consider defining specific error variants in
ConsensusManagerErrorfor calculation overflows:pub enum ConsensusManagerError { #[error("Difficulty adjustment encountered an error: `{0}`")] DifficultyAdjustmentError(#[from] DifficultyAdjustmentError), #[error("There is no blockchain to query")] EmptyBlockchain, #[error("RwLock access broken: `{0}`")] PoisonedAccess(String), #[error("No Difficulty adjustment manager present")] MissingDifficultyAdjustmentManager, + #[error("Coinbase calculation overflow: total {total} + fee {fee}")] + CoinbaseOverflow { total: MicroMinotari, fee: MicroMinotari }, }
233-269: Add detailed documentation for complex maturity calculationThis function implements complex logic for calculating spendable rewards based on maturity tranches. Consider adding more detailed documentation explaining the edge cases.
Add comprehensive documentation:
/// Get the total spendable block rewards circulation at the specified height (excluding pre-mine) + /// + /// This function calculates spendable rewards by considering the maturity schedule changes over time. + /// When maturity requirements decrease (e.g., from 720 to 540 blocks), we must account for the + /// transition period where older blocks are still maturing under the previous, longer maturity period. + /// + /// # Example + /// If maturity changes from 720 to 540 blocks at height 5040: + /// - Blocks 0-5039: mature after 720 blocks + /// - Blocks 5040+: mature after 540 blocks + /// - At height 5580 (540 blocks after 5040), blocks from 5040 are spendable, + /// but blocks 4860-5039 are still maturing (need 720 blocks) pub fn block_rewards_spendable_at_height(&self, height: u64) -> Result<MicroMinotari, String> {base_layer/transaction_components/src/key_manager/inner.rs (2)
470-470: Use more specific error messages to distinguish wallet types.The error messages are identical but occur in different wallet type contexts, which could make debugging harder.
Apply this diff to make the error messages more specific:
- KeyManagerServiceError::UnknownError("Invalid private key for Spend".to_string()) + KeyManagerServiceError::UnknownError("Invalid private key for DerivedKeys wallet Spend operation".to_string())And at line 486:
- KeyManagerServiceError::UnknownError("Invalid private key for Spend".to_string()) + KeyManagerServiceError::UnknownError("Invalid private key for ProvidedKeys wallet Spend operation".to_string())Also applies to: 486-486
1579-1580: Fix typo in comment.-// We need to check if its input or output for which we are singing. Signing with an input, we need to sign +// We need to check if its input or output for which we are signing. Signing with an input, we need to sign
📜 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 (57)
Cargo.toml(2 hunks)base_layer/common_types/Cargo.toml(1 hunks)base_layer/common_types/src/lib.rs(1 hunks)base_layer/common_types/src/seeds/cipher_seed.rs(24 hunks)base_layer/common_types/src/seeds/error.rs(2 hunks)base_layer/common_types/src/seeds/mnemonic.rs(2 hunks)base_layer/common_types/src/seeds/mod.rs(1 hunks)base_layer/common_types/src/seeds/seed_words.rs(1 hunks)base_layer/common_types/src/types/mod.rs(1 hunks)base_layer/common_types/src/wasm.rs(1 hunks)base_layer/core/Cargo.toml(1 hunks)base_layer/core/src/common/mod.rs(1 hunks)base_layer/core/src/consensus/consensus_encoding.rs(1 hunks)base_layer/core/src/consensus/mod.rs(0 hunks)base_layer/core/src/transactions/mod.rs(0 hunks)base_layer/core/src/transactions/transaction_protocol/mod.rs(0 hunks)base_layer/key_manager/src/key_manager_service/interface.rs(0 hunks)base_layer/key_manager/src/lib.rs(1 hunks)base_layer/transaction_components/Cargo.toml(1 hunks)base_layer/transaction_components/README.md(1 hunks)base_layer/transaction_components/src/aggregated_body.rs(1 hunks)base_layer/transaction_components/src/coinbase_builder.rs(1 hunks)base_layer/transaction_components/src/consensus/consensus_constants.rs(1 hunks)base_layer/transaction_components/src/consensus/consensus_encoding.rs(1 hunks)base_layer/transaction_components/src/consensus/consensus_manager.rs(1 hunks)base_layer/transaction_components/src/consensus/mod.rs(1 hunks)base_layer/transaction_components/src/consensus/network.rs(1 hunks)base_layer/transaction_components/src/emission.rs(1 hunks)base_layer/transaction_components/src/fee.rs(1 hunks)base_layer/transaction_components/src/key_manager/error.rs(1 hunks)base_layer/transaction_components/src/key_manager/initializer.rs(1 hunks)base_layer/transaction_components/src/key_manager/inner.rs(1 hunks)base_layer/transaction_components/src/key_manager/interface.rs(1 hunks)base_layer/transaction_components/src/key_manager/key_manager.rs(1 hunks)base_layer/transaction_components/src/key_manager/mod.rs(1 hunks)base_layer/transaction_components/src/key_manager/schema.rs(1 hunks)base_layer/transaction_components/src/key_manager/wrapper.rs(1 hunks)base_layer/transaction_components/src/lib.rs(1 hunks)base_layer/transaction_components/src/transaction_components/encrypted_data.rs(1 hunks)base_layer/transaction_components/src/transaction_components/error.rs(1 hunks)base_layer/transaction_components/src/transaction_components/kernel_builder.rs(1 hunks)base_layer/transaction_components/src/transaction_components/kernel_sum.rs(1 hunks)base_layer/transaction_components/src/transaction_components/mod.rs(1 hunks)base_layer/transaction_components/src/transaction_components/one_sided.rs(0 hunks)base_layer/transaction_components/src/transaction_components/output_features.rs(1 hunks)base_layer/transaction_components/src/transaction_components/payment_id.rs(1 hunks)base_layer/transaction_components/src/transaction_components/side_chain/sidechain_feature.rs(1 hunks)base_layer/transaction_components/src/transaction_components/side_chain/validator_node_exit.rs(1 hunks)base_layer/transaction_components/src/transaction_components/side_chain/validator_node_registration.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_builder.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_input.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_kernel.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_metadata.rs(1 hunks)base_layer/wallet_ffi/wallet.h(2 hunks)hashing/src/domains.rs(1 hunks)hashing/src/layer2.rs(2 hunks)
💤 Files with no reviewable changes (5)
- base_layer/core/src/consensus/mod.rs
- base_layer/key_manager/src/key_manager_service/interface.rs
- base_layer/transaction_components/src/transaction_components/one_sided.rs
- base_layer/core/src/transactions/transaction_protocol/mod.rs
- base_layer/core/src/transactions/mod.rs
🧰 Additional context used
🧠 Learnings (25)
base_layer/transaction_components/src/fee.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in `output_to_self` method in the Tari wallet, use an accurate fee estimate instead of `fee_per_gram`. The `input_selection.as_final_fee()` method provides a good initial estimate, and the final fee can be obtained later from `stp.get_fee_amount()`.
base_layer/transaction_components/src/transaction_components/side_chain/sidechain_feature.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/transaction_components/src/coinbase_builder.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
hashing/src/layer2.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
base_layer/common_types/src/lib.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/transaction_components/src/aggregated_body.rs (5)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/block.rs:27-36
Timestamp: 2025-06-14T04:54:52.123Z
Learning: In Rust struct-to-struct conversions, accessing different fields of a struct (like `block.body` and `block.header`) within the same constructor expression typically compiles successfully, even when one field is moved. The partial-move error mainly occurs when trying to use the struct as a whole after partial moves, not when accessing individual fields in the same scope.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/transaction_components/src/transaction_components/payment_id.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/transaction_components/src/transaction_components/error.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/core/src/consensus/consensus_encoding.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
base_layer/core/src/common/mod.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.
base_layer/common_types/Cargo.toml (1)
Learnt from: SWvheerden
PR: tari-project/tari#7111
File: integration_tests/build.rs:7-12
Timestamp: 2025-06-02T07:28:49.228Z
Learning: In build scripts (build.rs files), using `.unwrap()` on environment variables is acceptable and preferred when you want the build to fail if the required environment variables are not set, rather than using fallbacks.
base_layer/transaction_components/src/transaction_components/mod.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/transaction_components/src/transaction_components/transaction_input.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
base_layer/transaction_components/src/transaction_components/transaction.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/transaction_components/src/transaction_components/transaction_builder.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/block.rs:27-36
Timestamp: 2025-06-14T04:54:52.123Z
Learning: In Rust struct-to-struct conversions, accessing different fields of a struct (like `block.body` and `block.header`) within the same constructor expression typically compiles successfully, even when one field is moved. The partial-move error mainly occurs when trying to use the struct as a whole after partial moves, not when accessing individual fields in the same scope.
base_layer/common_types/src/seeds/error.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/wallet_ffi/wallet.h (2)
undefined
<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>
base_layer/transaction_components/src/key_manager/schema.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/storage/database.rs:88-123
Timestamp: 2025-05-03T04:49:27.646Z
Learning: SQLite does not support the DEFAULT keyword in INSERT statements in the same way PostgreSQL does, which makes Diesel's batch insert API problematic when used with SQLite databases.
base_layer/common_types/src/seeds/mnemonic.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/transaction_components/src/key_manager/interface.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/common_types/src/seeds/seed_words.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#7294
File: comms/dht/src/network_discovery/seed_strap.rs:721-735
Timestamp: 2025-07-09T08:13:37.206Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the 10-second STREAM_ITEM_TIMEOUT and retry logic are intentionally designed to handle service conflicts where other services kill seed peer connections during seedstrap operations. The underlying discovery_peer/dial_peer API timeouts are too lenient for seedstrap use cases, so the more aggressive timeout with retry logic is appropriate and necessary.
base_layer/transaction_components/src/key_manager/error.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/transaction_components/src/key_manager/wrapper.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/common_types/src/seeds/cipher_seed.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: tari-project/tari#7294
File: comms/dht/src/network_discovery/seed_strap.rs:721-735
Timestamp: 2025-07-09T08:13:37.206Z
Learning: In comms/dht/src/network_discovery/seed_strap.rs, the 10-second STREAM_ITEM_TIMEOUT and retry logic are intentionally designed to handle service conflicts where other services kill seed peer connections during seedstrap operations. The underlying discovery_peer/dial_peer API timeouts are too lenient for seedstrap use cases, so the more aggressive timeout with retry logic is appropriate and necessary.
base_layer/transaction_components/src/key_manager/inner.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🧬 Code Graph Analysis (4)
base_layer/transaction_components/src/coinbase_builder.rs (2)
base_layer/transaction_components/src/transaction_components/one_sided.rs (2)
shared_secret_to_output_encryption_key(40-47)shared_secret_to_output_spending_key(70-77)base_layer/transaction_components/src/tari_amount.rs (1)
uT(313-315)
base_layer/transaction_components/src/aggregated_body.rs (1)
base_layer/core/src/lib.rs (1)
block_output_mr_hash_from_pruned_mmr(114-116)
base_layer/common_types/src/wasm.rs (1)
integration_tests/src/ffi/wallet_address.rs (1)
emoji_id(85-96)
base_layer/transaction_components/src/consensus/mod.rs (1)
base_layer/core/tests/helpers/test_blockchain.rs (1)
consensus_manager(84-86)
⏰ 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: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
🔇 Additional comments (34)
base_layer/transaction_components/src/transaction_components/kernel_sum.rs (1)
28-28: Import path update looks correct
MicroMinotariis now resolved fromcrate::tari_amount, matching the new crate layout. No further action required.base_layer/transaction_components/src/emission.rs (1)
23-23: Import path updated – OKThe move from
crate::transactions::tari_amounttocrate::tari_amountis consistent with the refactor.base_layer/transaction_components/src/transaction_components/side_chain/sidechain_feature.rs (1)
30-34: Import path realigned – no issuesThe new path (
crate::transaction_components::…) matches the updated crate structure.
No further changes required.base_layer/transaction_components/src/transaction_components/side_chain/validator_node_registration.rs (1)
35-35: CorrectedValidatorNodeSignaturepath – good catchThis fixes the missing import that would have caused a compile error. Nothing else to address.
base_layer/transaction_components/src/transaction_components/output_features.rs (1)
39-47: Import path realignment looks goodImports now reference
crate::transaction_components::*, matching the new crate layout. No functional or compilation issues spotted.base_layer/transaction_components/src/transaction_components/side_chain/validator_node_exit.rs (1)
31-31: Corrected path fixes unresolved-import errorMoving to
crate::transaction_components::ValidatorNodeSignatureremoves the staletransactions::prefix and will compile successfully.base_layer/transaction_components/src/transaction_components/transaction_input.rs (1)
51-62: Imports correctly updated to new crate pathsThe switch to
transaction_components::*and explicittari_amount::MicroMinotarimatches the refactor; no issues detected.base_layer/transaction_components/src/transaction_components/error.rs (2)
37-40: LGTM! Well-structured error handling with clear module organization.The import structure reflects the new crate organization with
transaction_componentsandkey_managermodules properly separated. The error handling patterns are consistent and comprehensive.
96-130: Good error conversion patterns following Rust best practices.The
Fromimplementations properly convert different error types intoTransactionErrorvariants using appropriate error messages. The pattern of converting to string representations is consistent across all implementations.base_layer/transaction_components/src/transaction_components/kernel_builder.rs (1)
28-31: LGTM! Import path simplification aligns with crate restructuring.The consolidation of imports from
crate::transactions::{...}tocrate::{...}reflects the new crate organization and improves maintainability. The change is consistent with the broader refactoring effort.base_layer/transaction_components/src/transaction_components/transaction.rs (1)
35-39: LGTM! Consistent import path updates preserve functionality.The import path changes from
crate::transactions::{...}tocrate::{...}are consistent with the broader crate restructuring while maintaining all existing functionality.base_layer/common_types/src/types/mod.rs (2)
87-89: LGTM! Well-named type alias for Diffie-Hellman key exchange.The
CommsDHKEtype alias provides a clean abstraction for Diffie-Hellman shared secrets usingRistrettoPublicKey, which is consistent with the existing cryptographic type patterns in this module.
97-97: LGTM! Proper import for the new type alias.The import of
DiffieHellmanSharedSecretfromtari_crypto::dhkeis correctly placed and supports the newCommsDHKEtype alias.base_layer/transaction_components/src/transaction_components/transaction_builder.rs (1)
28-32: LGTM! Completes the consistent import path refactoring.The import path changes from
crate::transactions::{...}tocrate::{...}complete the consistent pattern of module organization improvements across all transaction component files while preserving all functionality.Cargo.toml (1)
12-13: ✔ New workspace member compiles cleanly
base_layer/transaction_components/Cargo.tomlcorrectly declares
name = "tari_transaction_components".- No
tari_coreortari_key_managerdependencies in
transaction_components, so there’s no cycle with
base_layer/coreorbase_layer/key_manager.base_layer/core/src/common/mod.rs (1)
41-41: No action requiredThe removed alias appears to be fully migrated to
tari_transaction_components; nothing else changes here.base_layer/transaction_components/src/transaction_components/mod.rs (2)
78-79: Good module organization for expanded transaction functionality.The addition of public
covenantsandone_sidedmodules properly exposes the new transaction functionality as part of the architectural refactor.
83-83: Transaction metadata module appropriately encapsulated.The private
transaction_metadatamodule correctly encapsulates transaction metadata handling within the transaction components crate, replacing the previous location in the core module.base_layer/common_types/Cargo.toml (2)
35-44: Dependencies appropriately support new cryptographic and WASM functionality.The added dependencies are well-chosen for the new features:
wasm-bindgenfor WASM interoperabilityzeroizefor secure memory handlingchacha20andargon2for cryptographic operationscrc32fastfor checksumssubtlefor constant-time operations- Target-specific WASM dependencies for random number generation
49-51: Correct crate type configuration for dual library support.The
[lib]section correctly specifies bothcdylib(for WASM/FFI) andrlib(for Rust) crate types, enabling the library to be used in both contexts.base_layer/transaction_components/src/coinbase_builder.rs (1)
35-64: Import reorganization aligns with new crate structure.The import consolidation and reorganization properly reflects the new
transaction_componentscrate structure without any functional changes to the code.base_layer/common_types/src/seeds/error.rs (2)
29-29: Enum rename improves specificity and clarity.Renaming
KeyManagerErrortoCipherErrorbetter reflects the specific use case for cipher seed operations rather than general key management.
52-62: From trait implementations correctly updated for new enum name.The
Fromtrait implementations properly use the newCipherErrorenum name while maintaining the same error conversion logic.hashing/src/domains.rs (1)
55-69: No duplicate domain declarations; imports have been updated
Workspace-wide searches confirm that eachhash_domain!macro forKeyManagerDomain,WalletOutputRewindKeysDomain, andWalletOutputSpendingKeysDomainappears only inhashing/src/domains.rs. There are no lingering imports of these types from the oldtransaction_componentscrate—it’s now consistently pulled from the dedicated hashing/key_manager crates.base_layer/transaction_components/Cargo.toml (1)
29-35: No missingtari_target_network_*cfgs in transaction_components crate
I verified that all three cfgs (mainnet,nextnet,testnet) are used inbase_layer/transaction_components/src/transaction_components/test.rs, so theunexpected_cfgslint won’t fire. No changes needed here.base_layer/common_types/src/seeds/mnemonic.rs (1)
33-38: LGTM! Error type migration is consistent.The migration from
KeyManagerErrortoCipherErrorand the import path updates are consistent with the overall refactoring of the seeds module.Also applies to: 268-279
base_layer/transaction_components/src/lib.rs (1)
1-33: LGTM!The module structure is well-organized with appropriate visibility modifiers and re-exports. The conditional compilation for
test_helpersis correctly implemented.base_layer/transaction_components/src/consensus/consensus_constants.rs (1)
1-1144: Excellent implementation of consensus constants!The file provides a comprehensive and well-structured definition of consensus parameters for all networks. The builder pattern allows for flexible configuration, and the test coverage validates the emission schedules and range proof types thoroughly.
base_layer/transaction_components/src/key_manager/interface.rs (1)
234-491: Well-designed comprehensive interface!The
TransactionKeyManagerInterfacetrait provides a complete set of async methods for transaction key management, covering key derivation, cryptographic operations, and recovery mechanisms. The interface is well-structured and follows async best practices.base_layer/transaction_components/src/key_manager/key_manager.rs (1)
91-106: Excellent use of domain-separated hashing for key derivationThe implementation correctly uses domain-separated hashing to derive keys, which prevents collisions and ensures proper key isolation. The chaining of entropy, branch seed, and key index is cryptographically sound.
base_layer/transaction_components/src/key_manager/wrapper.rs (1)
60-96: Well-designed thread-safe wrapper implementationThe use of
Arc<RwLock<TransactionKeyManagerInner<TBackend>>>provides proper thread-safe access to the key manager. The async methods and proper lock usage ensure safe concurrent access.base_layer/transaction_components/src/consensus/consensus_manager.rs (1)
384-475: Excellent test coverage for supply calculationsThe unit tests comprehensively verify the supply calculation logic at various heights, including edge cases around maturity changes and pre-mine unlocking. The test data appears to be carefully chosen to cover important scenarios.
base_layer/transaction_components/src/key_manager/inner.rs (2)
1677-1680: Duplicate key imports are already guarded against
Theinsert_imported_keyimplementation in the SQLite storage modules (e.g.
base_layer/key_manager/src/key_manager_service/storage/sqlite_db/mod.rs
and
base_layer/core/src/transactions/transaction_key_manager/storage/sqlite_db/mod.rs)
checks for an existing key viaget_imported_keyand returns early if found. No duplicate database inserts occur, so no additional pre-check is needed inimport_key.
1025-1027: Validate range value before bit shift to prevent potential overflow.If
self.crypto_factories.range_proof.range()returns a value >= 64, the bit shift operation will panic or produce undefined behavior.Add validation before the bit shift:
-if self.crypto_factories.range_proof.range() < 64 && - value >= 1u64.shl(&self.crypto_factories.range_proof.range()) +let range = self.crypto_factories.range_proof.range(); +if range < 64 && value >= 1u64.shl(range)Or even better, add explicit validation:
+let range = self.crypto_factories.range_proof.range(); +if range >= 64 { + return Err(TransactionError::BuilderError( + "Range proof range must be less than 64 bits".into(), + )); +} +if value >= 1u64.shl(range)⛔ Skipped due to learnings
Learnt from: ksrichard PR: tari-project/tari#7129 File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29 Timestamp: 2025-05-29T20:59:32.020Z Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.Learnt from: martinserts PR: tari-project/tari#7122 File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0 Timestamp: 2025-06-16T14:49:31.831Z Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
| use super::EncryptedDataKey; | ||
| use crate::transactions::{tari_amount::MicroMinotari, transaction_components::payment_id::PaymentId}; | ||
| use crate::{tari_amount::MicroMinotari, transaction_components::payment_id::PaymentId}; |
There was a problem hiding this comment.
Update leftover test imports to new module layout
The main crate was migrated from crate::transactions → crate::transaction_components, but the test-only imports (e.g. lines 277 & 81) still point to the old path.
cargo check will fail.
Suggested fixes:
- use crate::transactions::transaction_components::payment_id::PaymentId;
+use crate::transaction_components::payment_id::PaymentId;
- use crate::transactions::transaction_components::{SpentOutput, TransactionInput};
+use crate::transaction_components::{SpentOutput, TransactionInput};Search for any other crate::transactions:: occurrences in this crate and realign them.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
base_layer/transaction_components/src/transaction_components/encrypted_data.rs
around lines 55 to 56, update all test-only imports that still use the old
module path `crate::transactions` to the new path
`crate::transaction_components`. Search the entire file for any occurrences of
`crate::transactions::` and replace them with `crate::transaction_components::`
to ensure the imports align with the current module layout and prevent cargo
check errors.
| @@ -21,7 +21,7 @@ | |||
| // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |||
|
|
|||
| use super::{tari_amount::MicroMinotari, weight::TransactionWeight}; | |||
| use crate::transactions::aggregated_body::AggregateBody; | |||
| use crate::aggregated_body::AggregateBody; | |||
There was a problem hiding this comment.
Import path updated here but stale paths remain elsewhere
AggregateBody was moved, but the test module below still references
crate::transactions::transaction_components::{SpentOutput, TransactionInput}.
To avoid build breaks, update those paths to the new
crate::transaction_components::… equivalents and run cargo test.
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/fee.rs at line 24, the import path for
AggregateBody was updated, but other imports in the test module still use the
old path crate::transactions::transaction_components::{SpentOutput,
TransactionInput}. Update these import paths to the new
crate::transaction_components::{SpentOutput, TransactionInput} to match the
current module structure. After updating, run cargo test to verify the changes
do not break the build.
| use crate::{ | ||
| tari_amount::MicroMinotari, | ||
| transaction_components::encrypted_data::{SIZE_U256, SIZE_VALUE}, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Path corrections are incomplete
The top-level use block was fixed, but the nested test-only import below still targets the deleted transactions namespace.
Search & replace all occurrences of crate::transactions:: in this file.
-use crate::transactions::{
- tari_amount::MicroMinotari,
- transaction_components::{
- payment_id::{PaymentId, TxType},
- EncryptedData,
- },
-};
+use crate::{
+ tari_amount::MicroMinotari,
+ transaction_components::{
+ payment_id::{PaymentId, TxType},
+ EncryptedData,
+ },
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/transaction_components/payment_id.rs
around lines 37 to 40, the import paths still incorrectly reference the deleted
`transactions` namespace. Search the entire file for any occurrences of
`crate::transactions::` and update them to the correct current path by removing
or replacing the `transactions` segment to match the new module structure.
| let emoji_string = "🐢🐋🏦💤🐣👣📱🚜🍍🍉🎺🥊📖🔦😷👾🐺🐬👗🔱🌻💍🎢🎪🛵🐋🐊👞🥝🐍🌸📷🔧🎭🐮⏰🍇💯🐛🌴💨🔌🍪📟🎲🐝🤢🎉🔑🌵🚒🐙😍🐝🍑🐜👂🧩⏰🎀🚀🍵👑💐🎮🎮🎣🎒🍬🍳🍸🍷🍶🍯🍵🥄🍭🥐💣"; | ||
|
|
||
| let tari_address = TariAddress::from_emoji_string(emoji_string).unwrap(); |
There was a problem hiding this comment.
Remove hardcoded wallet address and handle parsing errors properly.
Several concerns with this implementation:
- Security Risk: The hardcoded emoji string appears to be a real wallet address, which could expose sensitive information.
- Error Handling: Using
.unwrap()in WASM context will cause a panic if the emoji string is invalid, leading to poor user experience. - Function Purpose: The function name suggests it "makes" an address, but it only parses a hardcoded one.
Apply this diff to handle errors properly:
#[wasm_bindgen]
pub fn make_tari_address() -> Result<String, JsValue> {
- let emoji_string = "🐢🐋🏦💤🐣👣📱🚜🍍🍉🎺🥊📖🔦😷👾🐺🐬👗🔱🌻💍🎢🎪🛵🐋🐊👞🥝🐍🌸📷🔧🎭🐮⏰🍇💯🐛🌴💨🔌🍪📟🎲🐝🤢🎉🔑🌵🚒🐙😍🐝🍑🐜👂🧩⏰🎀🚀🍵👑💐🎮🎮🎣🎒🍬🍳🍸🍷🍶🍯🍵🥄🍭🥐💣";
+ let emoji_string = "🐢🐋🏦💤🐣👣📱🚜🍍🍉🎺🥊📖🔦😷👾🐺🐬👗🔱🌻💍🎢🎪🛵🐋🐊👞🥝🐍🌸📷🔧🎭🐮⏰🍇💯🐛🌴💨🔌🍪📟🎲🐝🤢🎉🔑🌵🚒🐙😍🐝🍑🐜👂🧩⏰🎀🚀🍵👑💐🎮🎮🎣🎒🍬🍳🍸🍷🍶🍯🍵🥄🍭🥐💣"; // Consider removing this hardcoded address
- let tari_address = TariAddress::from_emoji_string(emoji_string).unwrap();
+ let tari_address = TariAddress::from_emoji_string(emoji_string)
+ .map_err(|e| JsValue::from_str(&format!("Failed to parse emoji string: {}", e)))?;
let emoji_id = tari_address.to_string();
Ok(emoji_id)
}Consider whether this function should accept an emoji string parameter instead of using a hardcoded one.
🤖 Prompt for AI Agents
In base_layer/common_types/src/wasm.rs around lines 8 to 10, remove the
hardcoded emoji string to avoid exposing sensitive wallet information and
prevent security risks. Modify the function to accept an emoji string as a
parameter instead of using a fixed value. Replace the `.unwrap()` call with
proper error handling to gracefully manage parsing failures without panicking,
such as returning a Result or an Option to the caller. This will improve
robustness and align the function's behavior with its intended purpose of
creating an address from a given input.
| tari_utilities = { workspace = true } | ||
| tokio = { version = "1.44", features = ["sync"] } | ||
| log = "0.4" | ||
| rand = "0.8" | ||
| strum = "0.22" | ||
| tari_crypto = { workspace = true, features = ["borsh"] } | ||
| blake2 = "0.10" | ||
| zeroize = "1" |
There was a problem hiding this comment.
tokio = "1.44" does not exist – build will fail
Latest released Tokio is 1.37 (July 2025). Cargo will resolve 1.44 to no version, causing a resolver error.
-tokio = { version = "1.44", features = ["sync"] }
+tokio = { version = "1.37", features = ["sync"] } # or the latest available versionRun cargo update -p tokio --precise 1.37.0 (or newer) to pin a valid version.
🤖 Prompt for AI Agents
In base_layer/transaction_components/Cargo.toml between lines 20 and 27, the
tokio dependency version is set to "1.44", which does not exist and will cause a
build failure. Change the tokio version to a valid released version such as
"1.37" or newer by updating the version string accordingly. Then run `cargo
update -p tokio --precise 1.37.0` to pin the dependency to the valid version.
| // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, | ||
| // INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE | ||
| // DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, | ||
| // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR | ||
| // SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, | ||
| // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE | ||
| // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
|
|
There was a problem hiding this comment.
Remove duplicate copyright text.
These lines duplicate the copyright header that already appears at the beginning of the file (lines 1-22).
Apply this diff to remove the duplicate:
-
-// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
-// INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
-// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
-// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
-// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
-// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-📝 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.
| // THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, | |
| // INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE | |
| // DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, | |
| // SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR | |
| // SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, | |
| // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE | |
| // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. |
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/key_manager/initializer.rs around lines
23 to 30, remove the duplicate copyright header text that repeats the same
content already present in lines 1 to 22. Delete these lines entirely to avoid
redundancy and keep only the initial copyright notice at the top of the file.
base_layer/transaction_components/src/key_manager/initializer.rs
Outdated
Show resolved
Hide resolved
| #[derive(Clone, Derivative, PartialEq, Serialize, Deserialize, Zeroize)] | ||
| #[derivative(Debug)] | ||
| pub struct TariKeyManager<D: Digest + LengthExtensionAttackResistant> { | ||
| #[derivative(Debug = "ignore")] | ||
| seed: CipherSeed, | ||
| #[derivative(Debug = "ignore")] | ||
| pub branch_seed: String, | ||
| primary_key_index: u64, | ||
| digest_type: PhantomData<D>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making branch_seed field private
The branch_seed field is currently public, which exposes internal state of the key manager. This could lead to unintended modifications.
Consider making this field private and providing a getter method if needed:
pub struct TariKeyManager<D: Digest + LengthExtensionAttackResistant> {
#[derivative(Debug = "ignore")]
seed: CipherSeed,
#[derivative(Debug = "ignore")]
- pub branch_seed: String,
+ branch_seed: String,
primary_key_index: u64,
digest_type: PhantomData<D>,
}Add a getter method if external access is required:
pub fn branch_seed(&self) -> &str {
&self.branch_seed
}🤖 Prompt for AI Agents
In base_layer/transaction_components/src/key_manager/key_manager.rs around lines
55 to 64, the branch_seed field is currently public, exposing internal state.
Change the branch_seed field's visibility from public to private by removing the
pub keyword. If external access to branch_seed is needed, add a public getter
method that returns a reference to the branch_seed string slice.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
base_layer/transaction_components/src/proof_of_work/difficulty.rs (1)
1-234: Refactor duplicated Difficulty implementationTwo nearly identical implementations of
Difficultyexist in the repo:
- base_layer/core/src/proof_of_work/difficulty.rs
- base_layer/transaction_components/src/proof_of_work/difficulty.rs
Meanwhile, in
base_layer/core/src/proof_of_work/error.rsyou already import:use tari_transaction_components::proof_of_work::difficulty::Difficulty;To eliminate maintenance overhead and ensure a single source of truth:
• Remove
base_layer/core/src/proof_of_work/difficulty.rs(and update anymod difficulty;declarations inproof_of_work/mod.rs).
• Verify all references inbase_layer/corenow usetari_transaction_components::proof_of_work::difficulty::Difficulty.
• Optionally, extractDifficultyinto a shared crate (e.g.,tari_sharedortari_utilities) and have bothcoreandtransaction_componentsdepend on it.
♻️ Duplicate comments (1)
base_layer/transaction_components/Cargo.toml (1)
35-35: Invalid tokio version will cause build failure.The tokio version "1.44" does not exist and will cause build failures. The latest released version is 1.37 or newer.
-tokio = { version = "1.44", features = ["sync"] } +tokio = { version = "1.37", features = ["sync"] }
🧹 Nitpick comments (2)
base_layer/transaction_components/src/consensus/consensus_manager.rs (1)
232-269: Complex maturity calculation could benefit from additional documentationThe logic for handling maturity tranche transitions is correct but complex. Consider adding more detailed comments or examples to explain the edge case where
last_effective_tranche.maturity < previous_effective_tranch.maturity.// We have to adjust the matured rewards at height to account for the effective from height of the last // effective tranche + // Example: If maturity decreases from 720 to 540 blocks at height 5040, and we're calculating + // spendable rewards at height 5400, we need to use the previous maturity (720) for blocks + // mined before the transition that haven't matured under the new rules yet. let emission_schedule = self.emission_schedule();base_layer/transaction_components/src/key_manager/inner.rs (1)
652-667: Optimize key search algorithmThe current search pattern alternates between forward and backward searching, which requires checking
i <= current_indexin each iteration. This could be optimized by searching in expanding ranges.Consider using a more efficient search pattern:
-for i in 0u64..TRANSACTION_KEY_MANAGER_MAX_SEARCH_DEPTH { - let index = current_index + i; - let public_key = CompressedPublicKey::from_secret_key(&km.derive_key(index)?.key); - if public_key == *key { - trace!(target: LOG_TARGET, "Key found in {} Key Chain at index {}", branch, i); - return Ok(index); - } - if i <= current_index && i != 0u64 { - let index = current_index - i; - let public_key = CompressedPublicKey::from_secret_key(&km.derive_key(index)?.key); - if public_key == *key { - trace!(target: LOG_TARGET, "Key found in {} Key Chain at index {}", branch, index); - return Ok(index); - } - } -} +// Search in expanding ranges from current index +let max_backward = current_index.min(TRANSACTION_KEY_MANAGER_MAX_SEARCH_DEPTH); +let max_forward = TRANSACTION_KEY_MANAGER_MAX_SEARCH_DEPTH - max_backward; + +for i in 0..=max_forward.max(max_backward) { + // Check forward + if i <= max_forward { + let index = current_index + i; + let public_key = CompressedPublicKey::from_secret_key(&km.derive_key(index)?.key); + if public_key == *key { + trace!(target: LOG_TARGET, "Key found in {} Key Chain at index {}", branch, index); + return Ok(index); + } + } + // Check backward (skip i=0 to avoid duplicate check at current_index) + if i > 0 && i <= max_backward { + let index = current_index - i; + let public_key = CompressedPublicKey::from_secret_key(&km.derive_key(index)?.key); + if public_key == *key { + trace!(target: LOG_TARGET, "Key found in {} Key Chain at index {}", branch, index); + return Ok(index); + } + } +}
📜 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 (61)
base_layer/common_types/src/lib.rs(1 hunks)base_layer/common_types/src/seeds/cipher_seed.rs(24 hunks)base_layer/common_types/src/seeds/mod.rs(1 hunks)base_layer/common_types/src/seeds/seed_words.rs(1 hunks)base_layer/common_types/src/types/mod.rs(1 hunks)base_layer/core/src/common/mod.rs(0 hunks)base_layer/core/src/proof_of_work/difficulty.rs(2 hunks)base_layer/core/src/proof_of_work/error.rs(2 hunks)base_layer/key_manager/src/lib.rs(1 hunks)base_layer/transaction_components/Cargo.toml(1 hunks)base_layer/transaction_components/src/coinbase_builder.rs(1 hunks)base_layer/transaction_components/src/consensus/consensus_constants.rs(1 hunks)base_layer/transaction_components/src/consensus/consensus_manager.rs(1 hunks)base_layer/transaction_components/src/consensus/emission.rs(1 hunks)base_layer/transaction_components/src/consensus/mod.rs(1 hunks)base_layer/transaction_components/src/helpers/mod.rs(1 hunks)base_layer/transaction_components/src/key_manager/error.rs(1 hunks)base_layer/transaction_components/src/key_manager/initializer.rs(1 hunks)base_layer/transaction_components/src/key_manager/inner.rs(1 hunks)base_layer/transaction_components/src/key_manager/interface.rs(1 hunks)base_layer/transaction_components/src/key_manager/key_manager.rs(1 hunks)base_layer/transaction_components/src/key_manager/mod.rs(1 hunks)base_layer/transaction_components/src/key_manager/wrapper.rs(1 hunks)base_layer/transaction_components/src/lib.rs(1 hunks)base_layer/transaction_components/src/proof_of_work/difficulty.rs(1 hunks)base_layer/transaction_components/src/proof_of_work/error.rs(1 hunks)base_layer/transaction_components/src/proof_of_work/mod.rs(1 hunks)base_layer/transaction_components/src/proof_of_work/proof_of_work.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/arguments.rs(2 hunks)base_layer/transaction_components/src/transaction_components/covenants/context.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/covenant.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/decoder.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/encoder.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/fields.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/absolute_height.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/and.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/field_eq.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/fields_hashed_eq.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/fields_preserved.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/filter.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/identity.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/not.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/or.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/output_hash_eq.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/filters/xor.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/macros.rs(8 hunks)base_layer/transaction_components/src/transaction_components/covenants/output_set.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/serde.rs(1 hunks)base_layer/transaction_components/src/transaction_components/covenants/token.rs(2 hunks)base_layer/transaction_components/src/transaction_components/error.rs(1 hunks)base_layer/transaction_components/src/transaction_components/mod.rs(3 hunks)base_layer/transaction_components/src/transaction_components/one_sided.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_input.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_kernel.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_output.rs(1 hunks)base_layer/transaction_components/src/transaction_components/unblinded_output.rs(1 hunks)base_layer/transaction_components/src/transaction_components/wallet_output.rs(1 hunks)base_layer/transaction_components/src/transaction_components/wallet_output_builder.rs(1 hunks)base_layer/transaction_components/src/weight.rs(1 hunks)base_layer/wallet_ffi/wallet.h(2 hunks)hashing/src/domains.rs(1 hunks)
💤 Files with no reviewable changes (1)
- base_layer/core/src/common/mod.rs
✅ Files skipped from review due to trivial changes (33)
- base_layer/transaction_components/src/transaction_components/covenants/serde.rs
- base_layer/transaction_components/src/transaction_components/covenants/token.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/absolute_height.rs
- base_layer/transaction_components/src/transaction_components/covenants/context.rs
- base_layer/transaction_components/src/weight.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/not.rs
- base_layer/transaction_components/src/transaction_components/covenants/arguments.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/filter.rs
- base_layer/transaction_components/src/consensus/emission.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/fields_hashed_eq.rs
- base_layer/transaction_components/src/transaction_components/transaction_output.rs
- base_layer/transaction_components/src/transaction_components/covenants/fields.rs
- base_layer/transaction_components/src/transaction_components/covenants/decoder.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/xor.rs
- base_layer/transaction_components/src/transaction_components/unblinded_output.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/identity.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/and.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/fields_preserved.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/output_hash_eq.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/field_eq.rs
- base_layer/transaction_components/src/transaction_components/wallet_output_builder.rs
- base_layer/transaction_components/src/transaction_components/wallet_output.rs
- base_layer/transaction_components/src/transaction_components/covenants/output_set.rs
- base_layer/transaction_components/src/transaction_components/covenants/covenant.rs
- base_layer/transaction_components/src/proof_of_work/mod.rs
- base_layer/transaction_components/src/transaction_components/covenants/macros.rs
- base_layer/transaction_components/src/transaction_components/covenants/encoder.rs
- base_layer/transaction_components/src/proof_of_work/proof_of_work.rs
- base_layer/transaction_components/src/transaction_components/covenants/filters/or.rs
- base_layer/common_types/src/seeds/seed_words.rs
- base_layer/transaction_components/src/helpers/mod.rs
- base_layer/transaction_components/src/transaction_components/transaction_input.rs
- base_layer/wallet_ffi/wallet.h
🚧 Files skipped from review as they are similar to previous changes (17)
- base_layer/transaction_components/src/transaction_components/transaction_kernel.rs
- base_layer/transaction_components/src/transaction_components/error.rs
- base_layer/transaction_components/src/transaction_components/one_sided.rs
- base_layer/transaction_components/src/coinbase_builder.rs
- base_layer/transaction_components/src/transaction_components/mod.rs
- base_layer/common_types/src/types/mod.rs
- base_layer/common_types/src/lib.rs
- hashing/src/domains.rs
- base_layer/common_types/src/seeds/mod.rs
- base_layer/transaction_components/src/key_manager/initializer.rs
- base_layer/key_manager/src/lib.rs
- base_layer/transaction_components/src/consensus/mod.rs
- base_layer/transaction_components/src/key_manager/key_manager.rs
- base_layer/transaction_components/src/key_manager/interface.rs
- base_layer/transaction_components/src/key_manager/mod.rs
- base_layer/common_types/src/seeds/cipher_seed.rs
- base_layer/transaction_components/src/key_manager/wrapper.rs
🧰 Additional context used
🧠 Learnings (6)
base_layer/core/src/proof_of_work/difficulty.rs (2)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/core/src/proof_of_work/error.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
base_layer/transaction_components/src/lib.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/transaction_components/Cargo.toml (4)
Learnt from: leet4tari
PR: tari-project/tari#7206
File: buildtools/docker_rig/tor.Dockerfile:0-0
Timestamp: 2025-06-18T08:09:27.943Z
Learning: In the Tari project's Docker builds (buildtools/docker_rig/tor.Dockerfile), APK package version constraints like `tor>${TOR_VERSION}` should remain unquoted despite general Alpine Linux documentation suggesting quoting. Adding quotes around the version constraint breaks the Docker build process in their specific build environment.
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
base_layer/transaction_components/src/key_manager/error.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.606Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/transaction_components/src/key_manager/inner.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🧬 Code Graph Analysis (1)
base_layer/transaction_components/src/proof_of_work/difficulty.rs (1)
base_layer/core/src/proof_of_work/difficulty.rs (12)
fmt(122-125)from_u64(45-50)as_u64(53-55)min(58-60)max(63-65)big_endian_difficulty(68-71)u256_scalar_to_difficulty(79-86)little_endian_difficulty(74-77)checked_div_u64(88-99)from(116-118)add(133-137)get_difficulty(140-140)
⏰ 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). (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: file licenses
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (23)
base_layer/core/src/proof_of_work/difficulty.rs (1)
143-143: Minor formatting improvement.The blank line addition before the test module improves code readability.
base_layer/core/src/proof_of_work/error.rs (2)
27-28: Import paths correctly updated for new crate structure.The imports have been properly updated to reference
tari_transaction_componentsinstead of local crate paths, which aligns with the refactoring to the new transaction components crate.
68-68: Minor formatting improvement.The blank line addition improves code readability.
base_layer/transaction_components/Cargo.toml (1)
1-12: Well-structured package configuration.The package metadata and feature configuration are properly set up for the new transaction components crate.
base_layer/transaction_components/src/proof_of_work/error.rs (2)
30-48: Well-structured error definitions.The error enums are properly defined with clear error messages and appropriate use of thiserror. The conditional compilation for merge mining errors is handled correctly.
68-88: Comprehensive difficulty error handling.The
DifficultyAdjustmentErrorandDifficultyErrorenums provide comprehensive coverage of difficulty-related error cases with clear error messages.base_layer/transaction_components/src/lib.rs (2)
4-28: Well-organized module structure.The module declarations and re-exports are properly organized, providing a clean public API for the transaction components crate. The separation of concerns between different modules (consensus, key_manager, proof_of_work, etc.) is appropriate.
30-55: Clean implementation of ban-related types.The
BanReasonstruct andBanPeriodenum are well-defined with appropriate accessor methods. The migration of these types from the core crate to the transaction components crate is handled correctly.base_layer/transaction_components/src/proof_of_work/difficulty.rs (3)
33-100: Difficulty implementation looks solidThe implementation correctly:
- Enforces minimum difficulty constraint in the const constructor
- Handles division by zero in scalar conversions
- Provides safe checked division that maintains the minimum difficulty invariant
- Uses U256 for intermediate calculations to prevent overflow
102-127: Good defensive programming practicesExcellent documentation explaining why arithmetic traits should not be implemented - this prevents accidental misuse that could violate difficulty invariants. The locale-aware Display formatting is a nice touch for readability.
143-233: Comprehensive test coverageThe tests thoroughly cover:
- Formatting with thousands separators
- Boundary conditions around MIN_DIFFICULTY
- Both big and little endian conversions
- Overflow prevention
- Division by zero handling
Well done!
base_layer/transaction_components/src/consensus/consensus_constants.rs (4)
49-49: Verify the leap year assumption in ANNUAL_BLOCKS calculationThe calculation uses 366 days per year (leap year). Most blockchain implementations use 365 days for consistency. Is this intentional?
386-809: Network configurations are well-structuredThe network-specific consensus constants are properly organized with:
- Clear progression of parameters across networks
- Appropriate difficulty settings for test vs production networks
- Proper versioning with effective heights for consensus changes
811-846: Well-designed output type and range proof configurationsGood use of static slices for efficiency. The separation between current and all permitted types provides flexibility for network upgrades.
943-1141: Excellent test coverage for emission schedulesThe tests thoroughly verify:
- Emission schedules for multiple networks
- Supply calculations at critical block heights
- Range proof type coverage for all output types
The use of specific test values helps ensure emission math is correct.
base_layer/transaction_components/src/key_manager/error.rs (1)
42-46: Appropriate error conversion implementationsThe manual From implementations correctly convert errors to String representations, following established patterns in the codebase.
Also applies to: 94-104, 145-155
base_layer/transaction_components/src/consensus/consensus_manager.rs (4)
111-121: Efficient consensus constant lookupThe linear search through consensus constants is appropriate given the small number of versions. The logic correctly finds the last constants that are effective at the given height.
145-166: Robust fee calculation with overflow protectionExcellent use of checked arithmetic to prevent overflow when summing fees. The error message is descriptive and helpful.
333-375: Well-designed builder with appropriate validationsThe builder correctly:
- Validates that custom genesis blocks are only allowed on LocalNet
- Provides sensible defaults for chain strength comparison
- Initializes emission schedule from the first consensus constants
384-475: Comprehensive supply calculation testsExcellent test coverage verifying:
- Mined vs spendable rewards distinction
- Pre-mine unlock schedule
- Total supply calculations
- Time-locked amounts
The use of specific expected values helps catch regression in emission calculations.
base_layer/transaction_components/src/key_manager/inner.rs (3)
22-95: Well-structured imports and dependenciesThe imports are properly organized with feature-gated ledger support. The use of domain-separated hashing and proper cryptographic primitives demonstrates security-conscious design.
Also applies to: 98-124
126-157: Solid architectural design for the key managerThe
TransactionKeyManagerInnerstruct is well-designed with proper encapsulation and concurrent access control usingRwLock. The initialization properly sets up all required branches.
1705-1761: Secure encryption implementationThe encryption methods properly use XChaCha20Poly1305 with domain-separated key derivation and include proper zeroization of sensitive data. This demonstrates good security practices.
| use thiserror::Error; | ||
| use crate::proof_of_work::{error::DifficultyError}; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify DifficultyAdjustmentError import is available.
The DifficultyAdjustment trait on line 137 references DifficultyAdjustmentError, but this type is not imported in the current file. According to the summary, this error type was moved to the new transaction_components crate.
🏁 Script executed:
#!/bin/bash
# Description: Check if DifficultyAdjustmentError is imported or available in scope
# Expected: Find the import or confirm it's available through a re-export
# Search for DifficultyAdjustmentError usage in this file
rg -A 3 -B 3 "DifficultyAdjustmentError" base_layer/core/src/proof_of_work/difficulty.rs
# Search for the import in related files
rg -A 2 -B 2 "DifficultyAdjustmentError" base_layer/core/src/proof_of_work/
# Check if it's available from the transaction_components crate
rg -A 2 -B 2 "DifficultyAdjustmentError" base_layer/transaction_components/src/proof_of_work/Length of output: 3795
Add missing DifficultyAdjustmentError import
The DifficultyAdjustment trait in difficulty.rs references DifficultyAdjustmentError but it isn’t imported in that file. Update the imports at the top of base_layer/core/src/proof_of_work/difficulty.rs:
use thiserror::Error;
-use crate::proof_of_work::{error::DifficultyError};
+use crate::proof_of_work::error::{DifficultyError, DifficultyAdjustmentError};• File: base_layer/core/src/proof_of_work/difficulty.rs
• Location: around lines 30–31
📝 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.
| use thiserror::Error; | |
| use crate::proof_of_work::{error::DifficultyError}; | |
| use thiserror::Error; | |
| use crate::proof_of_work::error::{DifficultyError, DifficultyAdjustmentError}; |
🤖 Prompt for AI Agents
In base_layer/core/src/proof_of_work/difficulty.rs around lines 30 to 31, the
file is missing the import for DifficultyAdjustmentError which is referenced by
the DifficultyAdjustment trait. Add an import statement for
DifficultyAdjustmentError alongside the existing imports to ensure the trait can
use the error type without compilation issues.
| pub fn with_emission_amounts( | ||
| mut self, | ||
| intial_amount: MicroMinotari, | ||
| decay: &'static [u64], | ||
| inflation_bips: u64, | ||
| epoch_length: u64, | ||
| ) -> Self { | ||
| self.consensus.emission_initial = intial_amount; | ||
| self.consensus.emission_decay = decay; | ||
| self.consensus.inflation_bips = inflation_bips; | ||
| self.consensus.tail_epoch_length = epoch_length; | ||
| self | ||
| } |
There was a problem hiding this comment.
Fix typo in parameter name
There's a typo in the parameter name intial_amount - should be initial_amount.
pub fn with_emission_amounts(
mut self,
- intial_amount: MicroMinotari,
+ initial_amount: MicroMinotari,
decay: &'static [u64],
inflation_bips: u64,
epoch_length: u64,
) -> Self {
- self.consensus.emission_initial = intial_amount;
+ self.consensus.emission_initial = initial_amount;📝 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 fn with_emission_amounts( | |
| mut self, | |
| intial_amount: MicroMinotari, | |
| decay: &'static [u64], | |
| inflation_bips: u64, | |
| epoch_length: u64, | |
| ) -> Self { | |
| self.consensus.emission_initial = intial_amount; | |
| self.consensus.emission_decay = decay; | |
| self.consensus.inflation_bips = inflation_bips; | |
| self.consensus.tail_epoch_length = epoch_length; | |
| self | |
| } | |
| pub fn with_emission_amounts( | |
| mut self, | |
| initial_amount: MicroMinotari, | |
| decay: &'static [u64], | |
| inflation_bips: u64, | |
| epoch_length: u64, | |
| ) -> Self { | |
| self.consensus.emission_initial = initial_amount; | |
| self.consensus.emission_decay = decay; | |
| self.consensus.inflation_bips = inflation_bips; | |
| self.consensus.tail_epoch_length = epoch_length; | |
| self | |
| } |
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/consensus/consensus_constants.rs around
lines 906 to 918, rename the parameter `intial_amount` to `initial_amount` in
the function signature and update all references to this parameter inside the
function accordingly to fix the typo.
| // use diesel::result::Error as DieselError; | ||
| // use tari_common_sqlite::error::{SqliteStorageError, StorageError}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or properly conditionally compile commented code
There are commented out imports and error variants related to diesel. Either:
- Remove them if they're no longer needed
- Add proper feature gates if they're conditionally required
- Complete the implementation if it's work in progress
Leaving commented code reduces maintainability.
Also applies to: 123-126
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/key_manager/error.rs around lines 23-24
and 123-126, there are commented out imports and error variants related to
diesel. You should either remove these commented lines if they are no longer
needed, or add appropriate feature flags to conditionally compile them if they
are required for certain builds. Alternatively, complete the implementation if
it is still in progress. This will improve code maintainability by avoiding
leftover commented code.
| #[error("Invalid range proof: `{0}`")] | ||
| RangeProofError(String), | ||
| #[error("Tari Key Manager error: `{0}`")] | ||
| TariKeyManagerError(#[from] KMError), |
There was a problem hiding this comment.
Missing imports for error types
The file references KMError and StorageError types but doesn't import them. Add the necessary imports:
+use crate::key_manager::KMError; // or wherever KMError is defined
+use tari_common_sqlite::error::StorageError;Also applies to: 83-83, 140-140, 142-142
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/key_manager/error.rs at lines 73, 83,
140, and 142, the error types KMError and StorageError are used but not
imported. Add the appropriate use statements at the top of the file to import
KMError and StorageError from their respective modules to resolve the missing
import errors.
| commitment_mask_key_id: &TariKeyId, | ||
| amount: &PrivateKey, | ||
| claim_public_key: &CompressedPublicKey, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Semantic confusion: value parameter typed as PrivateKey
The value parameter in several methods is typed as &PrivateKey but semantically represents a transaction amount, not a cryptographic key. This is confusing and could lead to errors.
Consider creating a type alias or wrapper type for clarity:
type ValueAsPrivateKey = PrivateKey;Or better yet, refactor to use the actual amount type and convert internally when needed for cryptographic operations.
Also applies to: 878-880, 1348-1348, 1456-1456
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/key_manager/inner.rs around lines
842-844 and also at 878-880, 1348, and 1456, the parameter named `value` is
typed as `&PrivateKey` but actually represents a transaction amount, causing
semantic confusion. To fix this, refactor these method signatures to use the
appropriate amount type instead of `PrivateKey` and perform any necessary
conversions to `PrivateKey` internally within the method. Alternatively, define
a clear type alias or wrapper for the amount to clarify its purpose and avoid
misuse.
| self.db.increment_key_index(branch)?; | ||
| km.increment_key_index(1) | ||
| }, |
There was a problem hiding this comment.
Potential database/memory inconsistency in key index update
The database increment happens before the in-memory increment. If km.increment_key_index(1) fails, the database will be out of sync with the in-memory state.
Consider updating both atomically:
-self.db.increment_key_index(branch)?;
-km.increment_key_index(1)
+let new_index = km.increment_key_index(1);
+self.db.increment_key_index(branch)?;
+new_index📝 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.
| self.db.increment_key_index(branch)?; | |
| km.increment_key_index(1) | |
| }, | |
| let new_index = km.increment_key_index(1); | |
| self.db.increment_key_index(branch)?; | |
| new_index |
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/key_manager/inner.rs around lines 209
to 211, the code increments the key index in the database before updating the
in-memory key manager, risking inconsistency if the in-memory increment fails.
To fix this, refactor the code to perform both increments atomically, ensuring
that either both succeed or neither does, possibly by combining the operations
in a transaction or by updating the in-memory state first and then the database
with proper error handling to maintain consistency.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
base_layer/transaction_components/src/proof_of_work/error.rs (1)
29-42: MissingMergeMineErrorimport and variant.The
PowErrorenum would need aMergeMineErrorvariant for thebase_nodefeature, but it's not defined here. This appears to be a migration issue where the variant was removed but the corresponding import wasn't added back.base_layer/transaction_components/src/key_manager/key_manager.rs (1)
60-69: Makebranch_seedfield private.The
branch_seedfield exposes internal state and should be private with a getter method if needed.base_layer/transaction_components/src/key_manager/interface.rs (1)
543-544: Fix incorrect import path.The import path references an old module structure and should be updated.
base_layer/transaction_components/src/consensus/consensus_constants.rs (1)
906-918: Fix typo in parameter name.The parameter name
intial_amountshould beinitial_amount.base_layer/transaction_components/src/key_manager/inner.rs (2)
207-209: Database/memory inconsistency risk remainsThe same issue identified in previous reviews still exists - database increment occurs before in-memory increment, risking inconsistency if the in-memory operation fails.
840-842: Semantic confusion with value parameter persistsThe issue identified in previous reviews still exists - the
valueparameter is typed as&PrivateKeybut semantically represents a transaction amount, causing confusion.
🧹 Nitpick comments (5)
base_layer/transaction_components/src/consensus/consensus_manager.rs (3)
45-56: Remove or implement commented out error variant.The
DifficultyAdjustmentErrorvariant is commented out. Either implement it if needed or remove the commented code for cleaner maintenance.#[derive(Debug, Error)] #[allow(clippy::large_enum_variant)] pub enum ConsensusManagerError { - // #[error("Difficulty adjustment encountered an error: `{0}`")] - // DifficultyAdjustmentError(#[from] DifficultyAdjustmentError), #[error("There is no blockchain to query")] EmptyBlockchain,
86-96: Use iterator methods for more idiomatic code.The current implementation works but can be simplified using iterator methods.
pub fn consensus_constants(&self, height: u64) -> &ConsensusConstants { - let mut constants = &self.inner.consensus_constants[0]; - for c in &self.inner.consensus_constants { - if c.effective_from_height() > height { - break; - } - constants = c - } - constants + self.inner.consensus_constants + .iter() + .rev() + .find(|c| c.effective_from_height() <= height) + .unwrap_or(&self.inner.consensus_constants[0]) }
152-189: Fix typo in variable name.There's a typo in the variable name
previous_effective_tranch- should beprevious_effective_tranche.- let previous_effective_tranch = maturity_tranches[last_effective_index.saturating_sub(1)].clone(); + let previous_effective_tranche = maturity_tranches[last_effective_index.saturating_sub(1)].clone(); // We have to adjust the matured rewards at height to account for the effective from height of the last // effective tranche let emission_schedule = self.emission_schedule(); - let matured_rewards_at_height = if last_effective_tranche.maturity < previous_effective_tranch.maturity && - height < last_effective_tranche.effective_from_height + previous_effective_tranch.maturity + let matured_rewards_at_height = if last_effective_tranche.maturity < previous_effective_tranche.maturity && + height < last_effective_tranche.effective_from_height + previous_effective_tranche.maturity { emission_schedule - .supply_at_block(height.saturating_sub(previous_effective_tranch.maturity)) + .supply_at_block(height.saturating_sub(previous_effective_tranche.maturity)) .saturating_sub(self.consensus_constants(height).pre_mine_value()) } else {base_layer/transaction_components/src/key_manager/inner.rs (2)
392-495: Consider refactoring large method for better maintainabilityThe
get_private_keymethod is quite long and handles multiple concerns. Consider breaking it down into smaller, more focused methods for each wallet type or key type to improve readability and maintainability.For example:
async fn get_private_key(&self, key_id: &TariKeyId) -> Result<PrivateKey, KeyManagerServiceError> { match key_id { TariKeyId::Zero => Ok(PrivateKey::default()), TariKeyId::Imported { key } => self.get_imported_private_key(key).await, TariKeyId::Managed { branch, index } => self.get_managed_private_key(branch, *index).await, TariKeyId::Derived { key } => self.get_derived_private_key(key).await, } }
640-700: Consider reducing code duplication in key search methodsThe
find_key_indexandfind_private_key_indexmethods have nearly identical search logic. Consider extracting the common search pattern into a generic helper method.For example:
async fn find_key_with_predicate<F>( &self, branch: &str, predicate: F, ) -> Result<u64, KeyManagerServiceError> where F: Fn(&PrivateKey, u64) -> bool, { // Common search logic here }
📜 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 (35)
base_layer/core/Cargo.toml(1 hunks)base_layer/core/src/blocks/accumulated_data.rs(1 hunks)base_layer/core/src/blocks/block.rs(1 hunks)base_layer/core/src/blocks/block_header.rs(1 hunks)base_layer/core/src/blocks/genesis_block.rs(3 hunks)base_layer/core/src/common/mod.rs(0 hunks)base_layer/core/src/consensus/consensus_encoding.rs(1 hunks)base_layer/core/src/consensus/mod.rs(1 hunks)base_layer/core/src/lib.rs(0 hunks)base_layer/core/src/proof_of_work/difficulty.rs(1 hunks)base_layer/core/src/proof_of_work/error.rs(1 hunks)base_layer/core/src/proof_of_work/mod.rs(0 hunks)base_layer/core/src/transactions/mod.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/storage/database/backend.rs(0 hunks)base_layer/key_manager/src/key_manager_service/storage/database/mod.rs(1 hunks)base_layer/transaction_components/Cargo.toml(1 hunks)base_layer/transaction_components/src/aggregated_body.rs(1 hunks)base_layer/transaction_components/src/coinbase_builder.rs(3 hunks)base_layer/transaction_components/src/consensus/consensus_constants.rs(1 hunks)base_layer/transaction_components/src/consensus/consensus_manager.rs(1 hunks)base_layer/transaction_components/src/key_manager/error.rs(1 hunks)base_layer/transaction_components/src/key_manager/initializer.rs(1 hunks)base_layer/transaction_components/src/key_manager/inner.rs(1 hunks)base_layer/transaction_components/src/key_manager/interface.rs(1 hunks)base_layer/transaction_components/src/key_manager/key_manager.rs(1 hunks)base_layer/transaction_components/src/key_manager/mod.rs(1 hunks)base_layer/transaction_components/src/key_manager/wrapper.rs(1 hunks)base_layer/transaction_components/src/lib.rs(1 hunks)base_layer/transaction_components/src/proof_of_work/error.rs(1 hunks)base_layer/transaction_components/src/transaction_components/kernel_features.rs(1 hunks)base_layer/transaction_components/src/transaction_components/mod.rs(3 hunks)base_layer/transaction_components/src/transaction_components/one_sided.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_metadata.rs(1 hunks)base_layer/transaction_components/src/transaction_components/transaction_output.rs(3 hunks)supply-chain/config.toml(1 hunks)
💤 Files with no reviewable changes (4)
- base_layer/core/src/lib.rs
- base_layer/core/src/transactions/transaction_key_manager/storage/database/backend.rs
- base_layer/core/src/proof_of_work/mod.rs
- base_layer/core/src/common/mod.rs
✅ Files skipped from review due to trivial changes (4)
- base_layer/transaction_components/src/transaction_components/kernel_features.rs
- base_layer/core/src/blocks/accumulated_data.rs
- supply-chain/config.toml
- base_layer/core/src/blocks/block.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- base_layer/core/src/proof_of_work/error.rs
- base_layer/core/src/consensus/consensus_encoding.rs
- base_layer/transaction_components/src/transaction_components/mod.rs
- base_layer/core/src/consensus/mod.rs
- base_layer/transaction_components/src/transaction_components/transaction_metadata.rs
- base_layer/transaction_components/src/transaction_components/one_sided.rs
- base_layer/transaction_components/src/transaction_components/transaction_output.rs
- base_layer/core/Cargo.toml
- base_layer/core/src/proof_of_work/difficulty.rs
- base_layer/transaction_components/Cargo.toml
- base_layer/transaction_components/src/key_manager/initializer.rs
- base_layer/transaction_components/src/key_manager/mod.rs
- base_layer/transaction_components/src/coinbase_builder.rs
- base_layer/transaction_components/src/key_manager/error.rs
- base_layer/transaction_components/src/lib.rs
- base_layer/transaction_components/src/key_manager/wrapper.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
base_layer/core/src/blocks/block_header.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/block.rs:27-36
Timestamp: 2025-06-14T04:54:52.123Z
Learning: In Rust struct-to-struct conversions, accessing different fields of a struct (like `block.body` and `block.header`) within the same constructor expression typically compiles successfully, even when one field is moved. The partial-move error mainly occurs when trying to use the struct as a whole after partial moves, not when accessing individual fields in the same scope.
Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
base_layer/transaction_components/src/aggregated_body.rs (4)
Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/block.rs:27-36
Timestamp: 2025-06-14T04:54:52.123Z
Learning: In Rust struct-to-struct conversions, accessing different fields of a struct (like `block.body` and `block.header`) within the same constructor expression typically compiles successfully, even when one field is moved. The partial-move error mainly occurs when trying to use the struct as a whole after partial moves, not when accessing individual fields in the same scope.
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
base_layer/core/src/blocks/genesis_block.rs (3)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/block.rs:27-36
Timestamp: 2025-06-14T04:54:52.123Z
Learning: In Rust struct-to-struct conversions, accessing different fields of a struct (like `block.body` and `block.header`) within the same constructor expression typically compiles successfully, even when one field is moved. The partial-move error mainly occurs when trying to use the struct as a whole after partial moves, not when accessing individual fields in the same scope.
base_layer/transaction_components/src/proof_of_work/error.rs (1)
Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
base_layer/transaction_components/src/key_manager/interface.rs (2)
Learnt from: hansieodendaal
PR: tari-project/tari#7284
File: applications/minotari_console_wallet/src/automation/commands.rs:0-0
Timestamp: 2025-07-15T12:23:14.650Z
Learning: In applications/minotari_console_wallet/src/automation/commands.rs, the consistent error handling pattern for command execution is to use match statements that: 1) On success: log with debug!, print user feedback, and push tx_id to tx_ids vector for monitoring, 2) On error: print error message with eprintln! using the format "{CommandName} error! {}", rather than using .unwrap() which would panic.
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
base_layer/transaction_components/src/key_manager/inner.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🧬 Code Graph Analysis (2)
base_layer/transaction_components/src/proof_of_work/error.rs (1)
base_layer/core/src/proof_of_work/error.rs (1)
get_ban_reason(50-64)
base_layer/transaction_components/src/consensus/consensus_manager.rs (3)
base_layer/transaction_components/src/consensus/consensus_constants.rs (1)
new(857-864)base_layer/transaction_components/src/transaction_components/transaction_metadata.rs (1)
new(20-27)base_layer/core/src/consensus/consensus_manager.rs (1)
time_locked_pre_mine(223-225)
⏰ 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). (8)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: ledger build tests
🔇 Additional comments (22)
base_layer/core/src/blocks/genesis_block.rs (1)
29-37: Confirm proof-of-work type exports in tari_transaction_componentsI didn’t find
AccumulatedDifficultyorDifficultyin the newtari_transaction_componentscrate—please verify whether these types are publicly exported there. If they are, update the imports in:
base_layer/core/src/blocks/genesis_block.rs(lines 29–37):- use crate::proof_of_work::{AccumulatedDifficulty, Difficulty}; + use tari_transaction_components::proof_of_work::{AccumulatedDifficulty, Difficulty};base_layer/core/src/blocks/block_header.rs(same change for proof-of-work imports)If those types aren’t yet available in
tari_transaction_components, you can keep the local imports for now.base_layer/transaction_components/src/aggregated_body.rs (1)
35-35: Merkle‐root helpers correctly scoped togenesis_block.rsA project-wide search shows that
calculate_header_block_output_mrandcalculate_header_normal_output_mrare only invoked withinbase_layer/core/src/blocks/genesis_block.rs. They no longer live on the generalAggregateBodyAPI, but that’s intentional—these helpers are specific to genesis‐block construction. No other code depends on them, so this refactor both compiles and improves module cohesion.base_layer/transaction_components/src/proof_of_work/error.rs (2)
60-67: LGTM!The
DifficultyAdjustmentErrorenum is well-structured with appropriate trait derivations and clear error messages.
69-80: LGTM!The
DifficultyErrorenum provides comprehensive coverage of difficulty conversion errors with clear messages.base_layer/transaction_components/src/consensus/consensus_manager.rs (2)
103-124: LGTM! Good overflow handling.The method properly uses checked arithmetic to prevent overflow and provides a descriptive error message.
255-346: LGTM! Comprehensive test coverage.The unit tests provide thorough validation of supply calculations with good coverage of edge cases and various block heights.
base_layer/transaction_components/src/key_manager/key_manager.rs (3)
42-58: LGTM! Good security practices.The key structures properly implement security best practices:
- Hide sensitive data from debug output
- Skip deserializing keys to prevent exposure
- Implement Zeroize for secure memory cleanup
96-111: LGTM! Cryptographically sound key derivation.The key derivation properly uses:
- Domain separated hashing to prevent collisions
- Deterministic generation from seed, branch, and index
- Little-endian encoding for consistency
170-232: LGTM! Good test coverage.The unit tests properly verify:
- Unique key generation for different seeds
- Deterministic key derivation
- Branch seed functionality
base_layer/transaction_components/src/key_manager/interface.rs (4)
66-166: LGTM! Well-designed key identifier system.The
TariKeyIdenum provides a flexible key identification system with:
- Multiple key types (Managed, Derived, Imported, Zero)
- Proper serialization/deserialization support
- Clear error messages
- Convenient accessor methods
241-498: LGTM! Comprehensive and well-designed interface.The
TransactionKeyManagerInterfacetrait provides a complete abstraction for key management with:
- Clear async method signatures
- Comprehensive coverage of key operations
- Proper use of generic parameters
- Good separation of concerns
506-525: LGTM! Clean storage abstraction.The
TransactionKeyManagerBackendtrait provides a clean abstraction for key manager storage with clear methods for state management.
552-579: LGTM! Good test coverage for key ID conversions.The tests properly verify string conversions for all TariKeyId variants.
base_layer/transaction_components/src/consensus/consensus_constants.rs (3)
52-129: LGTM! Well-structured consensus constants.The
ConsensusConstantsstruct is well-designed with:
- Comprehensive parameters for blockchain consensus
- Clear documentation for each field
- Appropriate data types
- Proper encapsulation
386-809: LGTM! Comprehensive network configurations.The network-specific consensus constants are well-configured for their respective use cases:
- Test networks have appropriate low difficulties
- Production networks have proper security parameters
- Emission schedules are carefully designed
943-1141: LGTM! Excellent test coverage.The unit tests provide comprehensive validation:
- Emission schedule calculations
- Network constant validity
- Range proof type coverage
- Supply calculations at various heights
base_layer/transaction_components/src/key_manager/inner.rs (6)
124-154: LGTM: Well-structured key manager implementationThe struct design is solid with appropriate use of
RwLockfor thread safety and generic backend abstraction. The constructor properly initializes standard branches.
727-867: LGTM: Cryptographic operations implemented correctlyThe commitment generation, mask verification, and burn proof creation methods are implemented correctly with proper nonce generation and error handling.
223-247: LGTM: Proper conditional compilation for Ledger supportThe conditional compilation pattern for Ledger functionality is well-implemented, providing clear error messages when the feature is disabled and proper functionality when enabled.
1703-1760: LGTM: Secure key encryption implementationThe encryption methods are well-implemented with proper domain separation, secure cipher choice (XChaCha20Poly1305), and appropriate zeroization of sensitive data.
1537-1612: LGTM: Transaction kernel signature methods are correctly implementedThe kernel signature generation methods properly handle the mathematical requirements for inputs vs outputs, with appropriate special handling for coinbase transactions.
370-390: LGTM: Well-structured error handlingThe error handling helper methods provide consistent, informative error messages with proper context including caller, branch, and wallet type information.
| pub branch_seed: String, | ||
| pub primary_key_index: u64, | ||
| } | ||
|
|
There was a problem hiding this comment.
Add missing import for KeyManagerState.
The KeyManagerState struct definition was removed but is still referenced in method signatures (lines 64, 69). Based on the AI summary, this struct is now defined in the new tari_transaction_components crate.
Add the missing import:
+use tari_transaction_components::key_manager::interface::KeyManagerState;📝 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.
| // Add at the top of base_layer/key_manager/src/key_manager_service/storage/database/mod.rs | |
| use tari_transaction_components::key_manager::interface::KeyManagerState; |
🤖 Prompt for AI Agents
In base_layer/key_manager/src/key_manager_service/storage/database/mod.rs at
line 32, add an import statement for the KeyManagerState struct from the
tari_transaction_components crate. This is necessary because KeyManagerState is
still referenced in method signatures around lines 64 and 69 but its definition
was moved to the new crate. Importing it will resolve the missing reference
errors.
| use tari_transaction_components::proof_of_work::{proof_of_work::ProofOfWork, proof_of_work_algorithm::PowAlgorithm}; | ||
| use tari_utilities::{epoch_time::EpochTime, hex::Hex}; | ||
| use thiserror::Error; | ||
|
|
||
| #[cfg(feature = "base_node")] | ||
| use crate::blocks::{BlockBuilder, NewBlockHeaderTemplate}; | ||
| use crate::{ | ||
| blocks::BlocksHashDomain, | ||
| consensus::DomainSeparatedConsensusHasher, | ||
| proof_of_work::{PowAlgorithm, PowError, ProofOfWork}, | ||
| }; | ||
| use crate::{blocks::BlocksHashDomain, consensus::DomainSeparatedConsensusHasher, proof_of_work::PowError}; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify import consistency for proof-of-work types.
The imports show ProofOfWork and PowAlgorithm being imported from the new tari_transaction_components crate, but PowError is still imported from the local proof_of_work module. Ensure all proof-of-work related types are consistently imported from the same source.
🏁 Script executed:
#!/bin/bash
# Check if local proof_of_work module still exists and contains PowError
fd -t f "proof_of_work" base_layer/core/src --exec grep -l "PowError" {}Length of output: 72
I’ll check for the presence of a local proof_of_work module file and any PowError definitions or references:
🏁 Script executed:
#!/bin/bash
# Locate any proof_of_work module files
fd -t f proof_of_work.* base_layer/core/src
# Search for the definition or any references to PowError
rg -n "PowError" base_layer/core/srcLength of output: 2567
Consistently import PowError from the external crate
The local proof_of_work::PowError is still being used in block_header.rs, but the other POW types are now coming from tari_transaction_components. Update the import so that all POW types—including PowError—come from the same source.
- File:
base_layer/core/src/blocks/block_header.rs- Replace the local
PowErrorimport on line 58.
- Replace the local
Diff:
- use crate::{blocks::BlocksHashDomain, consensus::DomainSeparatedConsensusHasher, proof_of_work::PowError};
+ use crate::{blocks::BlocksHashDomain, consensus::DomainSeparatedConsensusHasher};
+ use tari_transaction_components::proof_of_work::error::PowError;Ensure that tari_transaction_components::proof_of_work::error::PowError is publicly exported; adjust the path if the external crate re-exports it differently.
📝 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.
| use tari_transaction_components::proof_of_work::{proof_of_work::ProofOfWork, proof_of_work_algorithm::PowAlgorithm}; | |
| use tari_utilities::{epoch_time::EpochTime, hex::Hex}; | |
| use thiserror::Error; | |
| #[cfg(feature = "base_node")] | |
| use crate::blocks::{BlockBuilder, NewBlockHeaderTemplate}; | |
| use crate::{ | |
| blocks::BlocksHashDomain, | |
| consensus::DomainSeparatedConsensusHasher, | |
| proof_of_work::{PowAlgorithm, PowError, ProofOfWork}, | |
| }; | |
| use crate::{blocks::BlocksHashDomain, consensus::DomainSeparatedConsensusHasher, proof_of_work::PowError}; | |
| use tari_transaction_components::proof_of_work::{proof_of_work::ProofOfWork, proof_of_work_algorithm::PowAlgorithm}; | |
| use tari_utilities::{epoch_time::EpochTime, hex::Hex}; | |
| use thiserror::Error; | |
| #[cfg(feature = "base_node")] | |
| use crate::blocks::{BlockBuilder, NewBlockHeaderTemplate}; | |
| use crate::{blocks::BlocksHashDomain, consensus::DomainSeparatedConsensusHasher}; | |
| use tari_transaction_components::proof_of_work::error::PowError; |
🤖 Prompt for AI Agents
In base_layer/core/src/blocks/block_header.rs around lines 52 to 58, the
PowError import is from the local proof_of_work module while other POW types are
imported from tari_transaction_components. Update the import statement to import
PowError from tari_transaction_components::proof_of_work::error instead of the
local crate. Verify that PowError is publicly exported from the external crate
and adjust the import path accordingly to maintain consistency across all POW
type imports.
| pub fn calculate_header_block_output_mr( | ||
| normal_output_mr: FixedHash, | ||
| coinbases: &Vec<TransactionOutput>, | ||
| ) -> Result<FixedHash, MrHashError> { | ||
| let mut block_output_mmr = PrunedOutputMmr::new(PrunedHashSet::default()); | ||
| for o in coinbases { | ||
| block_output_mmr.push(o.hash().to_vec())?; | ||
| } | ||
| block_output_mmr.push(normal_output_mr.to_vec())?; | ||
| block_output_mr_hash_from_pruned_mmr(&block_output_mmr) | ||
| } | ||
|
|
||
| pub fn calculate_header_normal_output_mr(body: &AggregateBody) -> Result<FixedHash, MrHashError> { | ||
| let mut normal_output_mmr = PrunedOutputMmr::new(PrunedHashSet::default()); | ||
| for o in body.outputs() { | ||
| if !o.features.is_coinbase() { | ||
| normal_output_mmr.push(o.hash().to_vec())?; | ||
| } | ||
| } | ||
| Ok(FixedHash::try_from(normal_output_mmr.get_merkle_root()?)?) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider placement of public functions in test module.
These functions appear to replace functionality removed from aggregated_body.rs, but they're placed in a test module as pub functions. This suggests they might be used outside of tests, which violates the separation between test and production code.
Consider moving these functions to a more appropriate location:
- If they're only used in tests, remove the
pubvisibility - If they're used by other parts of the system, move them to a proper module in the main codebase
- If they're utility functions for genesis block creation, consider creating a dedicated utilities module
- pub fn calculate_header_block_output_mr(
+ fn calculate_header_block_output_mr(- pub fn calculate_header_normal_output_mr(body: &AggregateBody) -> Result<FixedHash, MrHashError> {
+ fn calculate_header_normal_output_mr(body: &AggregateBody) -> Result<FixedHash, MrHashError> {🤖 Prompt for AI Agents
In base_layer/core/src/blocks/genesis_block.rs around lines 810 to 830, the two
public functions are currently placed inside a test module but have pub
visibility, indicating they might be used outside tests which breaks separation
of test and production code. To fix this, determine if these functions are used
only in tests; if so, remove the pub keyword to restrict visibility. If they are
used elsewhere in the system, move them out of the test module into an
appropriate main codebase module, such as a dedicated utilities module for
genesis block-related functions, to maintain proper code organization and
visibility.
| // #[macro_use] | ||
| // #[cfg(feature = "base_node")] | ||
| // pub mod test_helpers; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code or clarify its status.
The test_helpers module is commented out instead of being removed. This creates uncertainty about whether the module is still needed. If it's no longer required due to the refactoring, remove it entirely. If it's still needed, uncomment it with proper feature flags.
Either remove the commented code:
-// #[macro_use]
-// #[cfg(feature = "base_node")]
-// pub mod test_helpers;Or clarify why it's commented out and when it will be restored.
📝 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.
| // #[macro_use] | |
| // #[cfg(feature = "base_node")] | |
| // pub mod test_helpers; |
🤖 Prompt for AI Agents
In base_layer/core/src/transactions/mod.rs around lines 9 to 11, the
test_helpers module is commented out, causing confusion about its necessity.
Either remove these commented lines entirely if the module is no longer needed,
or uncomment them and ensure the feature flags are correctly applied if the
module should remain part of the codebase.
| impl PowError { | ||
| pub fn get_ban_reason(&self) -> Option<BanReason> { | ||
| match self { | ||
| err @ PowError::InvalidProofOfWork | | ||
| err @ PowError::AchievedDifficultyBelowMin | | ||
| err @ PowError::Sha3HeaderNonEmptyPowBytes | | ||
| err @ PowError::RandomxTPowDataTooLong | | ||
| err @ PowError::AchievedDifficultyTooLow { .. } | | ||
| err @ PowError::InvalidTargetDifficulty { .. } => Some(BanReason { | ||
| reason: err.to_string(), | ||
| ban_duration: BanPeriod::Long, | ||
| }), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add missing match arm for MergeMineError variant.
The get_ban_reason method is missing a match arm for the MergeMineError variant when the base_node feature is enabled. This will cause a compilation error.
Add the missing match arm:
impl PowError {
pub fn get_ban_reason(&self) -> Option<BanReason> {
match self {
err @ PowError::InvalidProofOfWork |
err @ PowError::AchievedDifficultyBelowMin |
err @ PowError::Sha3HeaderNonEmptyPowBytes |
err @ PowError::RandomxTPowDataTooLong |
err @ PowError::AchievedDifficultyTooLow { .. } |
err @ PowError::InvalidTargetDifficulty { .. } => Some(BanReason {
reason: err.to_string(),
ban_duration: BanPeriod::Long,
}),
+ #[cfg(feature = "base_node")]
+ PowError::MergeMineError(e) => e.get_ban_reason(),
}
}
}🤖 Prompt for AI Agents
In base_layer/transaction_components/src/proof_of_work/error.rs between lines 44
and 58, the get_ban_reason method lacks a match arm for the MergeMineError
variant when the base_node feature is enabled, causing a compilation error. Add
a match arm for PowError::MergeMineError that returns Some(BanReason) with an
appropriate reason string and ban duration, ensuring it is conditionally
compiled only when the base_node feature is active.
| /// Builds a consensus manager | ||
| pub fn build(mut self) -> Result<ConsensusManager, ConsensusBuilderError> { | ||
| // should not be allowed to set the gen block and have the network type anything else than LocalNet | ||
| // If feature != base_node, gen_block is not available | ||
| if self.consensus_constants.is_empty() { | ||
| self.consensus_constants = self.network.create_consensus_constants(); | ||
| } | ||
|
|
||
| let emission = EmissionSchedule::new( | ||
| self.consensus_constants[0].emission_initial, | ||
| self.consensus_constants[0].emission_decay, | ||
| self.consensus_constants[0].inflation_bips, | ||
| self.consensus_constants[0].tail_epoch_length, | ||
| self.consensus_constants[0].pre_mine_value(), | ||
| ); | ||
|
|
||
| let inner = ConsensusManagerInner { | ||
| consensus_constants: self.consensus_constants, | ||
| network: self.network, | ||
| emission, | ||
| }; | ||
| Ok(ConsensusManager { inner: Arc::new(inner) }) | ||
| } |
There was a problem hiding this comment.
Add validation for empty consensus constants.
The build method accesses consensus_constants[0] without checking if the vector is empty, which could cause a panic.
pub fn build(mut self) -> Result<ConsensusManager, ConsensusBuilderError> {
if self.consensus_constants.is_empty() {
self.consensus_constants = self.network.create_consensus_constants();
}
+
+ if self.consensus_constants.is_empty() {
+ return Err(ConsensusBuilderError::EmptyConsensusConstants);
+ }
let emission = EmissionSchedule::new(
self.consensus_constants[0].emission_initial,Also add the error variant:
#[derive(Debug, thiserror::Error)]
pub enum ConsensusBuilderError {
#[error("Cannot set a genesis block with a network other than LocalNet")]
CannotSetGenesisBlock,
+ #[error("Consensus constants cannot be empty")]
+ EmptyConsensusConstants,
}📝 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.
| /// Builds a consensus manager | |
| pub fn build(mut self) -> Result<ConsensusManager, ConsensusBuilderError> { | |
| // should not be allowed to set the gen block and have the network type anything else than LocalNet | |
| // If feature != base_node, gen_block is not available | |
| if self.consensus_constants.is_empty() { | |
| self.consensus_constants = self.network.create_consensus_constants(); | |
| } | |
| let emission = EmissionSchedule::new( | |
| self.consensus_constants[0].emission_initial, | |
| self.consensus_constants[0].emission_decay, | |
| self.consensus_constants[0].inflation_bips, | |
| self.consensus_constants[0].tail_epoch_length, | |
| self.consensus_constants[0].pre_mine_value(), | |
| ); | |
| let inner = ConsensusManagerInner { | |
| consensus_constants: self.consensus_constants, | |
| network: self.network, | |
| emission, | |
| }; | |
| Ok(ConsensusManager { inner: Arc::new(inner) }) | |
| } | |
| /// Builds a consensus manager | |
| pub fn build(mut self) -> Result<ConsensusManager, ConsensusBuilderError> { | |
| // should not be allowed to set the gen block and have the network type anything else than LocalNet | |
| // If feature != base_node, gen_block is not available | |
| if self.consensus_constants.is_empty() { | |
| self.consensus_constants = self.network.create_consensus_constants(); | |
| } | |
| if self.consensus_constants.is_empty() { | |
| return Err(ConsensusBuilderError::EmptyConsensusConstants); | |
| } | |
| let emission = EmissionSchedule::new( | |
| self.consensus_constants[0].emission_initial, | |
| self.consensus_constants[0].emission_decay, | |
| self.consensus_constants[0].inflation_bips, | |
| self.consensus_constants[0].tail_epoch_length, | |
| self.consensus_constants[0].pre_mine_value(), | |
| ); | |
| let inner = ConsensusManagerInner { | |
| consensus_constants: self.consensus_constants, | |
| network: self.network, | |
| emission, | |
| }; | |
| Ok(ConsensusManager { inner: Arc::new(inner) }) | |
| } |
| /// Builds a consensus manager | |
| pub fn build(mut self) -> Result<ConsensusManager, ConsensusBuilderError> { | |
| // should not be allowed to set the gen block and have the network type anything else than LocalNet | |
| // If feature != base_node, gen_block is not available | |
| if self.consensus_constants.is_empty() { | |
| self.consensus_constants = self.network.create_consensus_constants(); | |
| } | |
| let emission = EmissionSchedule::new( | |
| self.consensus_constants[0].emission_initial, | |
| self.consensus_constants[0].emission_decay, | |
| self.consensus_constants[0].inflation_bips, | |
| self.consensus_constants[0].tail_epoch_length, | |
| self.consensus_constants[0].pre_mine_value(), | |
| ); | |
| let inner = ConsensusManagerInner { | |
| consensus_constants: self.consensus_constants, | |
| network: self.network, | |
| emission, | |
| }; | |
| Ok(ConsensusManager { inner: Arc::new(inner) }) | |
| } | |
| #[derive(Debug, thiserror::Error)] | |
| pub enum ConsensusBuilderError { | |
| #[error("Cannot set a genesis block with a network other than LocalNet")] | |
| CannotSetGenesisBlock, | |
| #[error("Consensus constants cannot be empty")] | |
| EmptyConsensusConstants, | |
| } |
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/consensus/consensus_manager.rs around
lines 224 to 246, the build method accesses consensus_constants[0] without
verifying if consensus_constants is empty, risking a panic. Add a validation
step before accessing consensus_constants[0] to check if the vector is empty; if
it is, return an appropriate error variant indicating empty consensus constants.
Also, define and use a new error variant in ConsensusBuilderError to represent
this validation failure.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
base_layer/transaction_components/src/wasm.rs (2)
3-3: Update or remove the outdated build command comment.The comment references "examples/js-hello-world" which doesn't match the current crate structure (
base_layer/transaction_components).Update the comment to reflect the correct build command:
-//wasm-pack build examples/js-hello-world --target web +//wasm-pack build base_layer/transaction_components --target webOr remove the comment if it's not needed.
8-8: Simplify the return statement.The return statement can be more idiomatic:
- return Ok("".to_string()) + Ok(String::new())The explicit
returnkeyword is unnecessary for the final expression, andString::new()is more idiomatic than"".to_string().
📜 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 (2)
base_layer/transaction_components/Cargo.toml(1 hunks)base_layer/transaction_components/src/wasm.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- base_layer/transaction_components/Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hansieodendaal
PR: tari-project/tari#7280
File: base_layer/core/src/chain_storage/blockchain_database.rs:361-438
Timestamp: 2025-07-04T10:56:46.079Z
Learning: The combination of `tokio::task::spawn(async move {` with `tokio::task::spawn_blocking().await` in the payref rebuild background task works well and shuts down properly with the tokio environment, as confirmed by testing in the Tari codebase.
🧬 Code Graph Analysis (1)
base_layer/transaction_components/src/wasm.rs (1)
base_layer/transaction_components/src/weight.rs (1)
latest(61-63)
⏰ 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). (8)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: ledger build tests
| @@ -0,0 +1,9 @@ | |||
| use wasm_bindgen::prelude::*; | |||
There was a problem hiding this comment.
Add missing imports for Fee and TransactionWeight.
The function uses Fee and TransactionWeight types but these imports are missing.
Add the required imports:
use wasm_bindgen::prelude::*;
+use crate::fee::Fee;
+use crate::weight::TransactionWeight;📝 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.
| use wasm_bindgen::prelude::*; | |
| use wasm_bindgen::prelude::*; | |
| use crate::fee::Fee; | |
| use crate::weight::TransactionWeight; | |
| // ... rest of the file ... |
🤖 Prompt for AI Agents
In base_layer/transaction_components/src/wasm.rs at line 1, the file is missing
imports for the Fee and TransactionWeight types used in the code. Add the
appropriate use statements to import Fee and TransactionWeight from their
respective modules to ensure the code compiles and functions correctly.
| #[wasm_bindgen] | ||
| pub fn make_tari_address() -> Result<String, JsValue> { | ||
| let f0 = Fee::new(TransactionWeight::latest()); | ||
| return Ok("".to_string()) | ||
| } |
There was a problem hiding this comment.
Implement the function or rename to reflect actual behavior.
The function has several issues:
- Function name suggests creating a Tari address but returns an empty string
- Creates a
Feeinstance that is never used (dead code) - Missing actual implementation
Either implement the function properly or rename it to reflect its current behavior:
#[wasm_bindgen]
-pub fn make_tari_address() -> Result<String, JsValue> {
- let f0 = Fee::new(TransactionWeight::latest());
- return Ok("".to_string())
+pub fn make_tari_address() -> Result<String, JsValue> {
+ // TODO: Implement actual address creation logic
+ Ok("".to_string())
}Or rename the function if it's meant to be a placeholder:
#[wasm_bindgen]
-pub fn make_tari_address() -> Result<String, JsValue> {
+pub fn placeholder_function() -> Result<String, JsValue> {
let f0 = Fee::new(TransactionWeight::latest());
- return Ok("".to_string())
+ Ok("".to_string())
}🤖 Prompt for AI Agents
In base_layer/transaction_components/src/wasm.rs around lines 5 to 9, the
function make_tari_address is misleading as it returns an empty string and
creates an unused Fee instance. To fix this, either implement the function to
actually create and return a valid Tari address or rename the function to
indicate it is a placeholder or does not perform address creation. Remove the
unused Fee instance if not needed.
Description
Motivation and Context
How Has This Been Tested?
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores