feat: make key manager stateless#7550
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the WalkthroughLarge refactor converting async, memory-backed key-manager flows to a synchronous Changes
Sequence Diagram(s)sequenceDiagram
participant Old as Old async flow
participant Factory as create_memory_db_key_manager()
participant New as KeyManager::new_random()
participant Builder as TransactionBuilder
rect rgb(220,238,255)
Note over Old,Factory: Previous async pattern
Old->>Factory: create_memory_db_key_manager().await
Factory-->>Old: MemoryDbKeyManager
Old->>Builder: TransactionBuilder::new(...).await
Builder->>Factory: key ops (.await)
end
rect rgb(221,255,221)
Note over Old,New: New synchronous pattern
Old->>New: KeyManager::new_random()
New-->>Old: KeyManager
Old->>Builder: TransactionBuilder::new(...)
Builder->>New: key ops (sync)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Key areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
base_layer/common_types/src/seeds/cipher_seed.rs (1)
135-147: Critical: Incomplete breaking change - remainingCipherSeed::new()calls in dependent test files.The breaking change from
new()torandom()is incomplete. The following test files still call the deprecatedCipherSeed::new()and will fail to compile on non-wasm32 targets:
base_layer/wallet/tests/other/mod.rs:380–let alice_seed = CipherSeed::new();base_layer/wallet/tests/other/mod.rs:417–let recovery_seed = CipherSeed::new();base_layer/wallet/tests/other/mod.rs:434–let recovery_seed = CipherSeed::new();base_layer/wallet/tests/other/mod.rs:700–CipherSeed::new(),All must be updated to use
CipherSeed::random().base_layer/transaction_components/src/transaction_components/one_sided.rs (1)
56-67: Address documentation inconsistency and consider ownership pattern consistency.Two issues identified:
Outdated documentation (line 62): The comment references "Diffie-Hellman shared secret" but the function accepts
&CompressedPublicKey. This should be corrected.Inconsistent ownership:
diffie_hellman_stealth_domain_hasher_dhkeconsumesCommsDHKEby value whilediffie_hellman_stealth_domain_hashertakes&CompressedPublicKeyby reference. Since both only call.as_bytes(), consider takingCommsDHKEby reference for consistency and to avoid unnecessary moves.Apply this diff to fix the documentation:
-/// Stealth address domain separated hasher using Diffie-Hellman shared secret +/// Stealth address domain separated hasher using a compressed public key pub fn diffie_hellman_stealth_domain_hasher(diffie_hellman: &CompressedPublicKey) -> DomainSeparatedHash<Blake2b<U64>> {Consider this diff to make ownership consistent (if
CommsDHKEimplementsAsRef<[u8]>or has.as_bytes()on a reference):-pub fn diffie_hellman_stealth_domain_hasher_dhke(diffie_hellman: CommsDHKE) -> DomainSeparatedHash<Blake2b<U64>> { +pub fn diffie_hellman_stealth_domain_hasher_dhke(diffie_hellman: &CommsDHKE) -> DomainSeparatedHash<Blake2b<U64>> { WalletHasher::new_with_label("stealth_address") .chain(diffie_hellman.as_bytes()) .finalize() }applications/minotari_miner/src/run_miner.rs (1)
371-379: Breaking change: Function signature modified.The function signature has changed:
- Parameter type:
&mut MemoryDbKeyManager→&KeyManager- Mutability: mutable reference → immutable reference
This is a breaking change that affects any code calling this function. The PR description states "Breaking Changes: None", but this should be updated to reflect the actual breaking changes.
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs (1)
229-293: Potential code duplication:ledger_get_public_key_legacyvsledger_get_public_key.Both functions appear to have identical implementations (lines 229-260 and 262-293). If
ledger_get_public_key_legacyis intended for backward compatibility during a transition period, consider:
- Adding a doc comment explaining the distinction and deprecation timeline
- Having one delegate to the other to avoid duplication
- If they're truly identical, removing the
_legacyvariantExample refactoring:
+/// Legacy function maintained for backward compatibility. Use `ledger_get_public_key` instead. +#[deprecated(since = "0.x.0", note = "Use ledger_get_public_key instead")] pub fn ledger_get_public_key_legacy( account: u64, index: u64, branch: LedgerKeyBranch, ) -> Result<RistrettoPublicKey, LedgerDeviceError> { - // ... full implementation + ledger_get_public_key(account, index, branch) }
🧹 Nitpick comments (10)
base_layer/core/src/mempool/priority/prioritized_transaction.rs (1)
152-164: Convert to synchronous tests.Since the tests no longer perform any async operations, the
#[tokio::test]attribute andasync fndeclarations are unnecessary overhead. Consider converting these to regular synchronous tests.Apply this diff to convert the tests to synchronous:
- #[tokio::test] - async fn fee_increases_priority() { + #[test] + fn fee_increases_priority() { let key_manager = KeyManager::new_random().unwrap(); let weighting = TransactionWeight::latest(); let epoch = u64::MAX / 2; let tx = create_tx_with_fee(2 * uT, &key_manager); let p1 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")).unwrap(); let tx = create_tx_with_fee(3 * uT, &key_manager); let p2 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")).unwrap(); assert!(p2 > p1); } - #[tokio::test] - async fn age_increases_priority() { + #[test] + fn age_increases_priority() { let key_manager = KeyManager::new_random().unwrap(); let weighting = TransactionWeight::latest(); let epoch = u64::MAX / 2; let tx = create_tx_with_fee(2 * uT, &key_manager); let p1 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")).unwrap(); let tx = create_tx_with_fee(2 * uT, &key_manager); let p2 = FeePriority::new( &tx, epoch - 1, tx.calculate_weight(&weighting).expect("Failed to get tx"), ) .unwrap(); assert!(p2 > p1); }Also applies to: 166-183
base_layer/transaction_components/src/transaction_components/covenants/fields.rs (1)
401-441: Consider removing async test markers.All test functions in this file are marked with
#[tokio::test]andasync fn, but none of them contain any.awaitcalls or async operations after the migration to synchronousKeyManager::new_random(). The async markers add unnecessary runtime overhead.You can simplify these tests by removing the
asynckeyword and changing#[tokio::test]to#[test]:-#[tokio::test] -async fn it_returns_true_if_eq() { +#[test] +fn it_returns_true_if_eq() {Apply this pattern to all test functions:
it_returns_true_if_eq(line 401)it_returns_false_if_not_eq(line 443)it_returns_true_if_eq_input(line 495)it_constructs_challenge_using_consensus_encoding(line 570)it_retrieves_the_value_as_ref(line 617)Also applies to: 443-488, 495-538, 570-610, 617-641
base_layer/common_types/src/seeds/cipher_seed.rs (1)
149-157: Platform-specific API inconsistency.The wasm32 target retains the
new()method name while non-wasm32 usesrandom(). This creates a platform-specific API inconsistency where cross-platform code would need conditional compilation to call the correct method name.Consider whether this inconsistency is necessary, or if both platforms should use the same method name for consistency.
base_layer/transaction_components/src/consensus/consensus_constants.rs (1)
221-235: Add documentation and tests for the new method.The implementation is logically sound and the assumption is valid—all networks include a consensus version starting at height 0, so the
expect()is safe. However, the method lacks documentation and unit tests, which are important for maintainability.Consider these improvements:
Add a doc comment explaining that it returns the most recent consensus version active at the given height.
Add unit tests covering edge cases (height = 0, height between versions, height after all versions).
Example doc comment:
/// Returns the consensus constants for the given network at the specified block height. /// /// This selects the most recent version of consensus constants that is active at the given /// height by filtering versions where `effective_from_height <= height` and returning /// the one with the maximum `effective_from_height`. pub fn for_network_at_height(network: Network, height: u64) -> Self {base_layer/core/src/chain_storage/blockchain_database.rs (1)
3188-3820: LGTM! Test changes are consistent with synchronous KeyManager.The removal of
.awaitcalls from test helper functions (create_main_chain,create_orphan_chain,create_chained_blocks) is correct and aligns with the migration to synchronousKeyManager. The changes are systematic and consistent across all test functions.Optional refactor: Tests are still marked as
async fnwith#[tokio::test], which is acceptable but could be changed to synchronous tests with#[test]if there are no remaining async operations. However, keeping them async is harmless and may provide flexibility for future changes.base_layer/transaction_components/src/test_helpers/test_helpers_functions.rs (1)
592-592: KeyManager cloning pattern in test helpers.The
key_manager.clone()calls are necessary becauseTransactionBuilder::newtakes ownership of the key manager, while these test helper functions receive a reference to avoid moving. This is acceptable for test code, though it's worth noting that ifKeyManageris expensive to clone, it could impact test performance.Also applies to: 644-644
base_layer/transaction_components/src/transaction_components/test.rs (1)
143-149: Consider keeping structured error assertions.The error handling has changed from pattern matching on
TransactionErrorvariants to string comparison. While acceptable for tests, string comparisons are more brittle and can break with minor error message changes.Consider maintaining structured error assertions when possible:
- assert_eq!( - e.to_string(), - "KeyManager encountered an error: Transaction error: `Error building the transaction: Value provided \ - is outside the range allowed by the range proof`" - ); + assert!(e.to_string().contains("Value provided is outside the range allowed"));This approach is more resilient to minor message formatting changes while still validating the core error condition.
base_layer/core/src/blocks/pre_mine/mod.rs (1)
813-866: Prefer reusing a single key manager for all pre-mine outputsCalling
KeyManager::new_random()inside the loop means every pre-mine output is derived from a freshly seeded wallet type. Besides the extra cost of reinitializing the factories each time, this reseeding makes repeated runs ofcreate_pre_mine_genesis_block_infonon-deterministic compared to the previous implementation that reused one memory key manager and produced stable output/offset derivations. Consider constructing a singleKeyManagerup front (or threading one in from the caller) and reusing it across iterations so the derivations remain reproducible while avoiding the repeated setup cost.applications/minotari_ledger_wallet/comms/src/accessor_methods.rs (1)
466-533: Clarify the distinction between DH shared secret functions.Two DH shared secret functions are present:
ledger_get_dh_shared_secret_dhke(lines 466-499) returnsDiffieHellmanSharedSecret<RistrettoPublicKey>ledger_get_dh_shared_secret(lines 501-533) returnsCompressedPublicKeyWhile the return types differ, consider adding doc comments explaining when to use each variant, as the naming alone may not clearly communicate the semantic difference to consumers.
base_layer/transaction_components/src/key_manager/manager.rs (1)
1-1341: Consider modularizing this large file.The
manager.rsfile is 1,341 lines, which may impact maintainability. While the current organization is functional, consider splitting into smaller modules:
- Core key management operations
- Ledger-specific operations
- Signature operations
- Encryption/decryption utilities
This would improve code navigation and make the module boundaries more explicit.
base_layer/transaction_components/src/test_helpers/test_helpers_functions.rs
Show resolved
Hide resolved
base_layer/transaction_components/src/transaction_components/one_sided.rs
Show resolved
Hide resolved
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
These should make wasm and ledger wallet compile - they fail CI
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs
Outdated
Show resolved
Hide resolved
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/minotari_ledger_wallet/common/Cargo.toml (1)
5-5: Fix typo in comment.The comment contains "msut" which should be "must".
Apply this diff to fix the typo:
-#Note: `rust-version` is not specified here as this msut be kept in sync with the ledger-app-builder +#Note: `rust-version` is not specified here as this must be kept in sync with the ledger-app-builder
♻️ Duplicate comments (1)
base_layer/transaction_components/src/key_manager/key_id.rs (1)
84-94: Fix derived key parsing to match Display output (duplicate issue).This is the same critical round-trip bug flagged in the previous review. The
Derivedbranch requiresparts.len() >= 3, butDisplay::fmt(line 171) outputs"derived.{key}"wherekeymay be a simple string without dots (e.g., "foo"). This meansTariKeyId::Derived { key: SerializedKeyString::from("foo") }will display as"derived.foo"(2 parts), which cannot be parsed back.Note that the test at line 359-362 explicitly expects
"derived.onlytwo"to fail, creating a design inconsistency.Apply this diff to allow single-segment keys:
DERIVED_KEY_BRANCH => { - if parts.len() < 3 { + if parts.len() < 2 { return Err("Wrong derived format".to_string()); };And update the test to reflect the corrected behavior:
- // Derived must have at least 3 parts - assert_eq!( - TariKeyId::from_str("derived.onlytwo").unwrap_err(), - "Wrong derived format" - ); + // Derived must have at least 2 parts (branch + key) + let parsed = TariKeyId::from_str("derived.onlytwo").unwrap(); + assert!(matches!(parsed, TariKeyId::Derived { .. })); + assert_eq!(parsed.to_string(), "derived.onlytwo");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
applications/minotari_ledger_wallet/common/Cargo.toml(1 hunks)applications/minotari_ledger_wallet/common/src/common_types.rs(2 hunks)applications/minotari_ledger_wallet/comms/src/accessor_methods.rs(9 hunks)base_layer/common_types/src/seeds/cipher_seed.rs(6 hunks)base_layer/transaction_components/src/key_manager/key_id.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7432
File: integration_tests/src/ffi/ffi_import.rs:351-356
Timestamp: 2025-08-22T10:22:27.310Z
Learning: The PR "feat!: remove comms from wallet" is a breaking change PR where backward compatibility is intentionally not maintained. The FFI functions `comms_config_create` and `comms_config_destroy` were renamed to `wallet_db_config_create` and `wallet_db_config_destroy` as part of the intentional breaking changes.
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7432
File: applications/minotari_app_grpc/proto/wallet.proto:1933-1948
Timestamp: 2025-08-26T06:28:13.374Z
Learning: In PR #7432 "feat!: remove comms from wallet", the HttpPeer message in the wallet.proto should only include url, last_latency, and is_online fields. The base node public_key and node_id fields mentioned in the original PR summary were removed from the scope and should not be included.
📚 Learning: 2025-07-15T12:23:14.650Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 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.
Applied to files:
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs
📚 Learning: 2025-06-16T14:49:31.831Z
Learnt from: martinserts
Repo: tari-project/tari 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.
Applied to files:
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs
🧬 Code graph analysis (2)
base_layer/transaction_components/src/key_manager/key_id.rs (2)
applications/minotari_ledger_wallet/common/src/common_types.rs (3)
fmt(157-159)from_str(144-153)as_str(130-138)base_layer/transaction_key_manager/src/legacy_key_manager/interface.rs (6)
fmt(220-243)fmt(276-278)from_str(132-215)from(264-266)from(270-272)as_str(258-260)
applications/minotari_ledger_wallet/common/src/common_types.rs (1)
base_layer/transaction_components/src/key_manager/key_id.rs (3)
fmt(169-195)fmt(222-224)from_str(78-164)
🔇 Additional comments (17)
base_layer/transaction_components/src/key_manager/key_id.rs (4)
37-73: LGTM!The constants and
TariKeyIdenum are well-defined with appropriate variants and derives. The integration withLedgerKeyBranchaligns with the broader key manager refactoring.
167-196: Display implementation is correct given the parsing fix.The formatting logic is clean and consistent. Once the
Derivedparsing bug (lines 84-94) is fixed, all variants will round-trip correctly throughDisplayandFromStr.
198-243: LGTM!
SerializedKeyStringis a clean newtype wrapper with appropriate helper methods and conversions.TariKeyAndIdis straightforward. The lack of Serde traits onTariKeyAndIdappears intentional for runtime-only usage.
245-406: Comprehensive test coverage with one update needed.The test suite thoroughly covers display, parsing, round-trips, and error cases for all variants. Once the
Derivedparsing bug is fixed, the test at lines 359-362 should be updated as suggested in the earlier comment.applications/minotari_ledger_wallet/common/Cargo.toml (1)
12-12: LGTM!The serde dependency is correctly configured with
default-features = falseand thederivefeature, which is appropriate for a no_std environment commonly used in ledger applications.applications/minotari_ledger_wallet/common/src/common_types.rs (3)
104-112: LGTM!The
LedgerKeyBranchenum is well-defined with appropriate derives for serialization (Serialize,Deserialize) and comparison operations. The explicit discriminants ensure stable wire format representation.
156-159: LGTM!The
Displayimplementation correctly delegates toas_str(), following the suggestion from the past review comment. This approach is clean and maintainable.
130-154: Roundtrip consistency verified—no issues found.All string and byte conversions are consistent and bidirectional:
- String representations in
as_str()andFromStrmatch exactly for all 5 variants- Byte values in the enum definition match
as_byte()andfrom_byte()patterns- Display impl delegates to
as_str(), maintaining consistency withFromStrPartialEqandEqtraits are derived, enabling proper assertionsThe code exhibits correct roundtrip behavior without defects.
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs (7)
26-26: LGTM!The import of
LedgerKeyBranchfromminotari_ledger_wallet_commonis correct and aligns with the PR's objective to standardize on this branch type.
46-49: LGTM!The
ScriptSignatureKeyenum correctly usesLedgerKeyBranchfor theManagedvariant, maintaining type consistency across the ledger wallet codebase.
228-259: LGTM!The
ledger_get_public_keyfunction signature correctly updated to acceptLedgerKeyBranchinstead of the previousTransactionKeyManagerBranch. The implementation properly serializes the branch byte value for the ledger command.
330-408: LGTM!The
ledger_get_script_offsetfunction correctly migrated to useLedgerKeyBranchfor bothscript_key_indexesandsender_offset_indexesparameters. The serialization logic properly encodes branch bytes for the ledger device.
466-507: LGTM!The
ledger_get_raw_schnorr_signaturefunction correctly updated to useLedgerKeyBranchfor bothprivate_key_branchandnonce_branchparameters, maintaining consistency with the PR's refactoring objectives.
510-548: LGTM!The
ledger_get_script_schnorr_signaturefunction properly migrated to useLedgerKeyBranchfor theprivate_key_branchparameter, aligning with the stateless key manager design.
431-463: Breaking change verified—all callers properly updated.The return type change from
DiffieHellmanSharedSecret<RistrettoPublicKey>toCompressedPublicKeyhas been correctly handled across all callers:
key_manager.rs:379– wrapper function maps the new return typekey_manager.rs:661– consumes wrapper result asCompressedPublicKeyledger_demo/main.rs:224– example code compatible with new typeOptional cleanup: Remove unused import of
DiffieHellmanSharedSecretfromaccessor_methods.rs:34.base_layer/common_types/src/seeds/cipher_seed.rs (2)
415-419: Default implementation correctly updated.The change from
Self::new()toSelf::random()in the Default implementation addresses the WASM build failure mentioned in the past review comment. The implementation correctly delegates to the renamed method for both wasm32 and non-wasm32 targets.
474-480: Test updates are consistent with the API change.All test call sites have been correctly updated from
CipherSeed::new()toCipherSeed::random(), maintaining the same test coverage for encryption, decryption, mnemonic conversion, and passphrase handling.Also applies to: 583-592, 619-642
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
applications/minotari_ledger_wallet/common/Cargo.toml (1)
12-12: Inherit serde version from workspace.A past review comment flagged that serde should use
workspace = trueto inherit the version from the workspace definition, which is the standard practice in this monorepo. This ensures consistent versioning across all packages.Apply this diff:
-serde = { version = "1", default-features = false, features = ["derive"] } +serde = { workspace = true, default-features = false, features = ["derive"] }base_layer/transaction_components/src/key_manager/key_id.rs (3)
23-29: Remove duplicated license disclaimer.Line 23 repeats the BSD disclaimer that is already present on Lines 15-21; please delete the duplicate block.
- // 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. -
85-93: Allow single-segment derived keys.
TariKeyId::Deriveddisplays asderived.foo, but Line 85 still rejects strings where the derived key has only one segment because of theparts.len() < 3guard. Round-tripping valid IDs therefore fails (same issue noted previously). Please relax the guard to accept two parts and adjust the tests accordingly, e.g.:- DERIVED_KEY_BRANCH => { - if parts.len() < 3 { + DERIVED_KEY_BRANCH => { + if parts.len() < 2 { return Err("Wrong derived format".to_string()); };And update
parse_error_casesso"derived.onlytwo"succeeds:- assert_eq!( - TariKeyId::from_str("derived.onlytwo").unwrap_err(), - "Wrong derived format" - ); + let derived = TariKeyId::from_str("derived.onlytwo").unwrap(); + assert_eq!(derived.to_string(), "derived.onlytwo");
107-109: Fix typo in DH encrypted data error.Line 108 still returns
"Wrong encryted data format". Please correct the spelling to “encrypted”.- return Err("Wrong encryted data format".to_string()); + return Err("Wrong encrypted data format".to_string());applications/minotari_console_wallet/src/automation/commands.rs (1)
931-933: Correct the ledger_key parameter for nonce variables.Two of the three
get_random_keycalls incorrectly passledger_key=true:
- Line 931:
script_nonce_keyshould useget_random_key(None, false)(it's a nonce, not a key requiring ledger management)- Line 933:
sender_offset_nonceshould useget_random_key(None, false)(same reason)- Line 932:
sender_offset_keycorrectly usesget_random_key(None, true)The pattern throughout the codebase reserves
ledger_key=truefor actual cryptographic keys and usesfalsefor nonces.Apply this diff:
-let script_nonce_key = key_manager_service.get_random_key(None, true)?; +let script_nonce_key = key_manager_service.get_random_key(None, false)?; let sender_offset_key = key_manager_service.get_random_key(None, true)?; -let sender_offset_nonce = key_manager_service.get_random_key(None, true)?; +let sender_offset_nonce = key_manager_service.get_random_key(None, false)?;
🧹 Nitpick comments (1)
base_layer/transaction_components/src/transaction_builder/builder.rs (1)
1036-1192: Consider simplifying test signatures.The tests continue to use
#[tokio::test]andasync fneven though allTransactionBuilderoperations are now synchronous. While this works correctly, you could simplify by converting test functions to regular synchronous tests unless they need async for other operations.Example:
- #[tokio::test] - async fn change_edge_case() { + #[test] + fn change_edge_case() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
applications/minotari_console_wallet/src/automation/commands.rs(31 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(3 hunks)applications/minotari_ledger_wallet/common/Cargo.toml(1 hunks)applications/minotari_ledger_wallet/comms/src/accessor_methods.rs(9 hunks)base_layer/transaction_components/src/key_manager/key_id.rs(1 hunks)base_layer/transaction_components/src/transaction_builder/builder.rs(58 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7432
File: integration_tests/src/ffi/ffi_import.rs:351-356
Timestamp: 2025-08-22T10:22:27.310Z
Learning: The PR "feat!: remove comms from wallet" is a breaking change PR where backward compatibility is intentionally not maintained. The FFI functions `comms_config_create` and `comms_config_destroy` were renamed to `wallet_db_config_create` and `wallet_db_config_destroy` as part of the intentional breaking changes.
📚 Learning: 2025-08-11T15:37:17.354Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7387
File: base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs:2896-2927
Timestamp: 2025-08-11T15:37:17.354Z
Learning: The `update_accumulated_difficulty` method in `base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs` is migration-specific code that only runs during a once-off migration when existing ChainHeader data is present in LMDB. The consistency guarantees are upheld by the calling function `process_accumulated_data_for_height`, so additional validation within this method is redundant.
Applied to files:
base_layer/transaction_components/src/key_manager/key_id.rs
📚 Learning: 2025-10-03T10:21:49.027Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7514
File: base_layer/transaction_components/src/transaction_builder/builder.rs:704-752
Timestamp: 2025-10-03T10:21:49.027Z
Learning: In base_layer/transaction_components/src/transaction_builder/builder.rs, the change_encrypted_data_if_fee_changed helper should only update fees in MemoField instances that already contain a fee (when get_fee() returns Some). It should not set fees in memos where get_fee() returns None, as not all outputs require fees in their memos.
Applied to files:
base_layer/transaction_components/src/key_manager/key_id.rsapplications/minotari_console_wallet/src/automation/commands.rsbase_layer/transaction_components/src/transaction_builder/builder.rs
📚 Learning: 2025-07-15T12:23:14.650Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 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.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rsapplications/minotari_ledger_wallet/comms/src/accessor_methods.rs
📚 Learning: 2025-06-16T14:49:31.831Z
Learnt from: martinserts
Repo: tari-project/tari 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.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rsapplications/minotari_ledger_wallet/comms/src/accessor_methods.rsbase_layer/transaction_components/src/transaction_builder/builder.rs
📚 Learning: 2025-09-08T08:43:50.910Z
Learnt from: MCozhusheck
Repo: tari-project/tari PR: 7478
File: base_layer/p2p/src/signature_verification/gpg_keys/seed_peers_http.asc:1-15
Timestamp: 2025-09-08T08:43:50.910Z
Learning: In the Tari codebase, temporary test PGP keys for signature verification are acceptable during development phases when they will be replaced with production keys before final release. The maintainers prefer to keep the implementation simple during drafts rather than over-engineering with test-only conditionals for temporary keys.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rs
📚 Learning: 2025-04-23T05:56:30.985Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 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()`.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rs
🧬 Code graph analysis (3)
base_layer/transaction_components/src/key_manager/key_id.rs (2)
applications/minotari_ledger_wallet/common/src/common_types.rs (3)
fmt(157-159)from_str(144-153)as_str(130-138)base_layer/transaction_key_manager/src/legacy_key_manager/interface.rs (6)
fmt(220-243)fmt(276-278)from_str(132-215)from(264-266)from(270-272)as_str(258-260)
applications/minotari_console_wallet/src/automation/commands.rs (5)
base_layer/transaction_components/src/transaction_components/one_sided.rs (1)
public_key_to_output_encryption_key(36-43)base_layer/transaction_components/src/transaction_components/memo_field.rs (1)
new_open_from_string(353-355)base_layer/wallet/src/output_manager_service/service.rs (2)
spend_backup_pre_mine_utxo(1342-1525)new(139-185)base_layer/wallet/src/transaction_service/service.rs (2)
spend_backup_pre_mine_utxo(1838-1887)new(197-267)base_layer/transaction_components/src/transaction_components/unblinded_output.rs (1)
from_wallet_output(174-202)
base_layer/transaction_components/src/transaction_builder/builder.rs (4)
base_layer/wallet/src/output_manager_service/service.rs (2)
new(139-185)zero(2938-2945)base_layer/transaction_components/src/test_helpers/test_helpers_functions.rs (1)
new(106-127)base_layer/transaction_components/src/transaction_components/wallet_output.rs (10)
new(89-140)commitment_mask_key_id(642-644)script(671-673)sender_offset_public_key(712-714)minimum_value_promise(746-748)input_data(693-695)encrypted_data(742-744)features(660-662)covenant(682-684)commitment(776-778)base_layer/transaction_components/src/transaction_components/one_sided.rs (2)
public_key_to_output_encryption_key(36-43)public_key_to_output_spending_key(46-53)
⏰ 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). (10)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with msrv
- GitHub Check: cargo check with stable
- GitHub Check: ledger build tests
- GitHub Check: ci
- GitHub Check: wasm build tests
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (16)
base_layer/transaction_components/src/transaction_builder/builder.rs (5)
75-247: LGTM! Clean async-to-sync conversion in public API.The conversion of
TransactionBuilder::new,add_recipient,add_stealth_recipient,with_input, andwith_outputfrom async to sync is well-executed. All key manager calls now use synchronous interfaces with proper error propagation.
277-563: LGTM! Helper methods correctly converted to synchronous.The private helper methods (
get_pre_build_change_output,add_change_if_required,create_change_memo,build_change) are all properly converted to synchronous operations. The change output generation logic maintains correctness while removing async complexity.
565-676: LGTM! Nonce calculation and fee update logic correctly converted.The
calculate_total_nonce_and_total_public_excessandchange_encrypted_data_if_fee_changedmethods are properly converted to synchronous operations. The signature change forchange_encrypted_data_if_fee_changedto acceptkey_manager: &KMas a parameter aligns well with the stateless design.
680-930: LGTM! Core build() method successfully converted to synchronous.The main
build()method is comprehensively converted from async to sync. All cryptographic operations (partial signatures, kernel offsets, script offsets) now use synchronous key manager calls. The transaction assembly logic remains intact while removing async complexity.
1193-1987: LGTM! All tests properly updated to synchronous KeyManager.All test cases are correctly updated to use the synchronous
KeyManagerAPI. The tests cover:
- Single/multiple recipients
- Stealth addresses
- Fee validation
- View-only wallet recovery
- Historic transaction recovery
All assertions and validations remain intact while successfully removing async complexity.
applications/minotari_console_wallet/src/automation/commands.rs (7)
825-834: LGTM: Consistent memo construction pattern.The updated
MemoField::new_open_from_stringcalls paired with thedetect_tx_metadatahelper provide a clean, consistent approach to memo construction. Error handling follows the established pattern of usingeprintln!withcontinuerather than panicking.Also applies to: 1177-1186, 1710-1722, 1746-1755, 1774-1780, 1940-1946, 1969-1977, 2001-2009, 2032-2040, 2526-2538
934-957: LGTM: Key manager API correctly updated for stateless operation.The synchronous key manager operations correctly use:
- Key ID references for Diffie-Hellman shared secret computation
TariKeyId::LedgerKeywithLedgerKeyBranch::PreMinefor pre-mine outputs- Direct signing operations without async overhead
These changes align well with the PR's goal of simplifying key manager usage.
1812-1812: LGTM: Synchronous UnblindedOutput conversion.The updated
UnblindedOutput::from_wallet_outputcalls correctly pass a reference to the key manager service and operate synchronously, consistent with the stateless key manager refactor.Also applies to: 1883-1883
1420-1509: LGTM: PreMineSigs signature operations correctly refactored.The signature generation logic properly uses:
- Synchronous
sign_with_nonce_and_challengewith key ID referencesimport_keyandverify_maskfor commitment validationget_script_offsetwith key ID slicespublic_key_to_output_encryption_keyfor encryption key derivationAll operations are now synchronous and follow the established error handling pattern.
2169-2172: LGTM: Synchronous view and spend key access.Direct synchronous access to
get_view_key(),get_spend_key(), andget_private_view_key()demonstrates one of the benefits of the stateless key manager refactor—eliminating async overhead for simple key retrieval operations.
2847-2863: LGTM: Well-structured transaction type detection helper.The
detect_tx_metadatahelper centralizes the logic for determining whether a transaction is a self-payment or payment to others. The fallback toTxType::PaymentToOtherwhen wallet addresses are unavailable is a safe default.
2750-2750: LGTM: Synchronous message signing operations.The updated
sign_message_with_spend_keyandsign_script_message_with_spend_keycalls operate synchronously and correctly handle the optional sender offset parameter, consistent with the stateless key manager design.Also applies to: 2776-2777
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs (4)
26-26: LGTM! Consistent type migration to LedgerKeyBranch.The import changes and enum updates align with the PR objective to make the key manager stateless. The addition of the
Derivedvariant toScriptSignatureKeyenables direct private key usage alongside managed keys.Also applies to: 34-34, 47-48
107-107: LGTM! Parameter types consistently updated.All function signatures have been uniformly migrated from
TransactionKeyManagerBranchtoLedgerKeyBranch, which is consistent with the stateless key manager refactor.Also applies to: 231-231, 334-336, 469-471, 513-513
52-636: ****All public functions in this file are actively used. The verification shows every function has confirmed usage:
verify_ledger_application,ledger_get_app_name,ledger_get_version: used in ledger_demo examplesledger_get_public_key,ledger_get_script_signature,ledger_get_script_offset,ledger_get_dh_shared_secret,ledger_get_raw_schnorr_signature,ledger_get_script_schnorr_signature,ledger_get_one_sided_metadata_signature: used in base_layer/transaction_components key_managerledger_get_public_spend_key,ledger_get_view_key: used in console_wallet initializationThe premise that functions are unused is incorrect. Ignore this review comment.
Likely an incorrect or invalid review comment.
431-463: Type change is functionally compatible; no misuse detected.The return type change from
DiffieHellmanSharedSecret<RistrettoPublicKey>toCompressedPublicKey(which isCompressedKey<RistrettoPublicKey>) is valid because both represent compressed curve points. Verification shows:
- key_manager.rs: Function signature already updated to return
CompressedPublicKey(line 376), and the direct return at line 381 is type-compatible.- ledger_demo/main.rs: Correctly uses
.as_bytes().to_vec().to_hex()for serialization (line 225).- No misuse: Neither usage treats the shared point as an authentication key; the point is only serialized or directly returned as expected.
While "CompressedPublicKey" is semantically imprecise for a DH shared point, it's mathematically equivalent and causes no functional issues.
| }) | ||
| } | ||
|
|
||
| fn add_offset_to_key( |
There was a problem hiding this comment.
This name is not 100% clear. I am not sure what a better name is though
There was a problem hiding this comment.
It should also be clear that it saves the key. e.g. add_offset_to_key_and_import
| )) | ||
| } | ||
|
|
||
| fn ledger_get_public_key_wrapper( |
There was a problem hiding this comment.
Perhaps all of these ledger methods should move to a new ledger file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
base_layer/transaction_components/src/key_manager/manager.rs (6)
153-153: Inconsistent domain string construction.This line uses
.as_bytes().to_vec()while line 433 uses the same approach, but Rust'sb"..."literal syntax is more idiomatic and efficient.Apply this diff for consistency:
- let domain = "KEY_MANAGER_private_key".as_bytes().to_vec(); + let domain = b"KEY_MANAGER_private_key".to_vec();Based on past review comments.
164-183: Method name could be clearer about its behavior.The method
add_offset_to_keycreates a new encrypted key ID but doesn't clearly indicate this in its name. Consider renaming to something likederive_offset_keyorcreate_offset_key_idto better reflect that it returns a newTariKeyIdrather than mutating the input.Based on past review comments.
194-429: Consider organizing ledger methods into a separate module.The file contains numerous
ledger_*_wrappermethods (lines 194-429) that are all conditionally compiled. These methods follow a consistent pattern and could be better organized in a separate module (e.g.,manager/ledger.rs) to improve maintainability and readability of the core KeyManager implementation.Based on past review comments.
433-433: Use byte string literal for domain constant.For consistency and efficiency, use Rust's byte string literal syntax.
Apply this diff:
- let domain = "KEY_MANAGER_private_key".as_bytes().to_vec(); + let domain = b"KEY_MANAGER_private_key".to_vec();Based on past review comments.
151-152: Sensitive key material should be zeroized.The
pvt_bytesvariable holds sensitive private key material in memory without zeroization. Consider usingZeroizing<Vec<u8>>or theHiddenwrapper to ensure the bytes are securely erased when dropped.Based on past review comments.
432-432: Sensitive decryption key material should be zeroized.The
private_decryption_keyvariable holds sensitive key bytes without zeroization. UseZeroizing<Vec<u8>>or retain theHiddenwrapper to prevent key material from lingering in memory.Based on past review comments.
applications/minotari_console_wallet/src/automation/commands.rs (2)
825-843: Memo creation forPreMineSpendBackupUtxofollows wallet metadata conventionsUsing
MemoField::new_open_from_stringtogether withdetect_tx_metadata(&wallet, &args.recipient_address).awaitensures the memo’sTxTypereflects payment-to-self vs payment-to-other, and the error path useseprintln!+breakinstead of panicking, matching the established command error‑handling pattern.Based on learnings
931-977: Use non‑ledger keys for nonce values in pre‑mine party step
script_nonce_keyandsender_offset_nonceare ephemeral nonces, but all three calls useget_random_key(None, true)which flags them as ledger keys; onlysender_offset_keyshould be ledger‑managed, while nonces should be created with the non‑ledger flag (as in other call sites across the codebase).I recommend:
- let script_nonce_key = key_manager_service.get_random_key(None, true)?; - let sender_offset_key = key_manager_service.get_random_key(None, true)?; - let sender_offset_nonce = key_manager_service.get_random_key(None, true)?; + let script_nonce_key = key_manager_service.get_random_key(None, false)?; + let sender_offset_key = key_manager_service.get_random_key(None, true)?; + let sender_offset_nonce = key_manager_service.get_random_key(None, false)?;
TariKeyId::LedgerKey { branch: LedgerKeyBranch::PreMine, .. },get_public_key_at_key_id, andsign_script_messageusage looks correct for the actual script key.
🧹 Nitpick comments (3)
base_layer/transaction_components/src/key_manager/manager.rs (2)
473-1275: Consider adding documentation for public trait methods.The
TransactionKeyManagerInterfaceimplementation lacks doc comments on its public methods. Adding documentation would help users understand:
- Expected behavior and guarantees
- Parameter meanings and constraints
- Return value semantics
- Potential error conditions
While the trait itself may have documentation, implementation-specific details (e.g., Ledger vs. software key handling) would benefit from inline comments.
103-471: Consider modularizing the KeyManager implementation.The
KeyManagerimplementation (~1340 lines) combines several concerns:
- Core key management (encryption, derivation)
- Ledger integration (multiple wrapper methods)
- Metadata signature operations
- Recovery and encryption helpers
Consider organizing this into submodules:
manager/core.rs- main struct and core operationsmanager/ledger.rs- ledger-specific wrappersmanager/signatures.rs- signature-related operationsmanager/recovery.rs- key recovery and encryptionThis would improve maintainability as the codebase grows.
base_layer/transaction_components/src/coinbase_builder.rs (1)
93-98: Consider removing the unusedKeyManagerServiceErrorvariant.After the migration to the synchronous
KeyManagerAPI, theKeyManagerServiceErrorvariant on line 95 appears to be unused. TheFrom<KeyManagerError>implementation on lines 106-110 suggests that all key manager errors now map toKeyManagerError.If confirmed unused, apply this diff:
KeyManagerError(String), - #[error("Key manager service error: `{0}`")] - KeyManagerServiceError(String), #[error("Conversion error: {0}")]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
applications/minotari_console_wallet/src/automation/commands.rs(31 hunks)base_layer/core/src/blocks/pre_mine/mod.rs(6 hunks)base_layer/core/src/test_helpers/mod.rs(3 hunks)base_layer/transaction_components/src/coinbase_builder.rs(30 hunks)base_layer/transaction_components/src/key_manager/interface.rs(1 hunks)base_layer/transaction_components/src/key_manager/manager.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- base_layer/core/src/test_helpers/mod.rs
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7432
File: integration_tests/src/ffi/ffi_import.rs:351-356
Timestamp: 2025-08-22T10:22:27.310Z
Learning: The PR "feat!: remove comms from wallet" is a breaking change PR where backward compatibility is intentionally not maintained. The FFI functions `comms_config_create` and `comms_config_destroy` were renamed to `wallet_db_config_create` and `wallet_db_config_destroy` as part of the intentional breaking changes.
📚 Learning: 2025-08-18T14:27:34.527Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7421
File: base_layer/core/src/base_node/sync/header_sync/synchronizer.rs:774-781
Timestamp: 2025-08-18T14:27:34.527Z
Learning: In the header sync synchronizer (base_layer/core/src/base_node/sync/header_sync/synchronizer.rs), non-banable errors from the sync process are handled by the calling function, not within the sync methods themselves. This means that internal state errors like `current_valid_chain_tip_header()` returning `None` will be properly categorized and handled upstream without incorrectly banning peers.
Applied to files:
base_layer/transaction_components/src/coinbase_builder.rs
📚 Learning: 2025-06-16T14:49:31.831Z
Learnt from: martinserts
Repo: tari-project/tari 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.
Applied to files:
base_layer/transaction_components/src/coinbase_builder.rsbase_layer/core/src/blocks/pre_mine/mod.rsapplications/minotari_console_wallet/src/automation/commands.rsbase_layer/transaction_components/src/key_manager/manager.rsbase_layer/transaction_components/src/key_manager/interface.rs
📚 Learning: 2025-10-03T10:21:49.027Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7514
File: base_layer/transaction_components/src/transaction_builder/builder.rs:704-752
Timestamp: 2025-10-03T10:21:49.027Z
Learning: In base_layer/transaction_components/src/transaction_builder/builder.rs, the change_encrypted_data_if_fee_changed helper should only update fees in MemoField instances that already contain a fee (when get_fee() returns Some). It should not set fees in memos where get_fee() returns None, as not all outputs require fees in their memos.
Applied to files:
base_layer/transaction_components/src/coinbase_builder.rsapplications/minotari_console_wallet/src/automation/commands.rs
📚 Learning: 2025-07-15T12:23:14.650Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 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.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rs
📚 Learning: 2025-09-08T08:43:50.910Z
Learnt from: MCozhusheck
Repo: tari-project/tari PR: 7478
File: base_layer/p2p/src/signature_verification/gpg_keys/seed_peers_http.asc:1-15
Timestamp: 2025-09-08T08:43:50.910Z
Learning: In the Tari codebase, temporary test PGP keys for signature verification are acceptable during development phases when they will be replaced with production keys before final release. The maintainers prefer to keep the implementation simple during drafts rather than over-engineering with test-only conditionals for temporary keys.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rs
📚 Learning: 2025-08-11T15:37:17.354Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7387
File: base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs:2896-2927
Timestamp: 2025-08-11T15:37:17.354Z
Learning: The `update_accumulated_difficulty` method in `base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs` is migration-specific code that only runs during a once-off migration when existing ChainHeader data is present in LMDB. The consistency guarantees are upheld by the calling function `process_accumulated_data_for_height`, so additional validation within this method is redundant.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rs
📚 Learning: 2025-04-23T05:56:30.985Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 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()`.
Applied to files:
applications/minotari_console_wallet/src/automation/commands.rs
📚 Learning: 2025-08-22T10:22:27.310Z
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7432
File: integration_tests/src/ffi/ffi_import.rs:351-356
Timestamp: 2025-08-22T10:22:27.310Z
Learning: The PR "feat!: remove comms from wallet" is a breaking change PR where backward compatibility is intentionally not maintained. The FFI functions `comms_config_create` and `comms_config_destroy` were renamed to `wallet_db_config_create` and `wallet_db_config_destroy` as part of the intentional breaking changes.
Applied to files:
base_layer/transaction_components/src/key_manager/interface.rs
🧬 Code graph analysis (5)
base_layer/transaction_components/src/coinbase_builder.rs (6)
base_layer/transaction_key_manager/src/legacy_key_manager/wrapper.rs (2)
key_manager(141-143)new(107-121)base_layer/transaction_key_manager/src/legacy_key_manager/inner.rs (1)
key_manager(105-107)base_layer/transaction_components/src/key_manager/error.rs (4)
from(59-61)from(65-67)from(71-73)from(77-79)base_layer/transaction_components/src/transaction_builder/builder.rs (2)
build(680-930)new(75-106)base_layer/transaction_components/src/test_helpers/test_helpers_functions.rs (2)
new(106-127)default(202-213)base_layer/transaction_components/src/key_manager/manager.rs (1)
new_random(139-144)
base_layer/core/src/blocks/pre_mine/mod.rs (6)
base_layer/transaction_key_manager/src/legacy_key_manager/wrapper.rs (1)
key_manager(141-143)base_layer/transaction_key_manager/src/legacy_key_manager/inner.rs (1)
key_manager(105-107)base_layer/transaction_components/src/key_manager/manager.rs (1)
new_random(139-144)base_layer/transaction_components/src/key_manager/wallet_types.rs (1)
new_random(52-56)base_layer/transaction_key_manager/src/legacy_key_manager/tari_key_manager.rs (1)
random(77-84)base_layer/transaction_components/src/transaction_components/transaction_kernel.rs (1)
build_kernel_signature_challenge(169-183)
applications/minotari_console_wallet/src/automation/commands.rs (3)
infrastructure/tari_script/src/lib.rs (1)
push_pubkey_script(54-56)base_layer/transaction_components/src/transaction_components/one_sided.rs (1)
public_key_to_output_encryption_key(36-43)base_layer/transaction_components/src/transaction_components/memo_field.rs (1)
new_open_from_string(353-355)
base_layer/transaction_components/src/key_manager/manager.rs (4)
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs (7)
ledger_get_dh_shared_secret(431-463)ledger_get_one_sided_metadata_signature(551-636)ledger_get_public_key(228-259)ledger_get_raw_schnorr_signature(466-507)ledger_get_script_offset(330-408)ledger_get_script_schnorr_signature(510-548)ledger_get_script_signature(262-327)base_layer/common_types/src/encryption.rs (2)
decrypt_bytes_integral_nonce(53-75)encrypt_bytes_integral_nonce(78-104)base_layer/transaction_components/src/transaction_components/one_sided.rs (3)
diffie_hellman_stealth_domain_hasher(63-67)public_key_to_output_encryption_key(36-43)public_key_to_output_spending_key(46-53)base_layer/transaction_components/src/key_manager/interface.rs (5)
get_random_key(62-66)get_public_key_at_key_id(69-69)get_script_signature(119-126)get_script_offset(184-188)get_one_sided_metadata_signature(202-212)
base_layer/transaction_components/src/key_manager/interface.rs (3)
base_layer/transaction_key_manager/src/legacy_key_manager/wrapper.rs (33)
key_manager(141-143)get_random_key(169-176)get_public_key_at_key_id(178-180)create_encrypted_key(182-189)get_commitment(191-198)verify_mask(200-208)get_view_key(210-212)get_private_view_key(214-216)get_spend_key(218-220)get_next_commitment_mask_and_script_key(231-234)find_script_key_id_from_commitment_mask_key_id(236-243)get_diffie_hellman_shared_secret(245-252)construct_range_proof(254-262)get_script_signature(264-279)get_partial_script_signature(281-298)get_partial_txo_kernel_signature(300-321)get_txo_kernel_signature_excess_with_offset(323-330)get_txo_private_kernel_offset(332-339)encrypt_data_for_recovery(341-354)try_output_key_recovery(356-364)is_this_output_ours(366-374)get_script_offset(376-383)get_metadata_signature(385-402)get_one_sided_metadata_signature(404-425)sign_message_with_spend_key(222-229)sign_script_message(427-434)sign_script_message_with_spend_key(436-443)sign_with_nonce_and_challenge(445-453)get_receiver_partial_metadata_signature(455-475)get_sender_partial_metadata_signature(480-498)generate_burn_claim_signature(500-511)stealth_address_script_spending_key(513-520)get_private_key(527-529)base_layer/transaction_key_manager/src/legacy_key_manager/inner.rs (33)
key_manager(105-107)get_random_key(207-213)get_public_key_at_key_id(215-217)create_encrypted_key(259-265)get_commitment(286-292)verify_mask(295-302)get_view_key(247-249)get_private_view_key(267-269)get_spend_key(251-253)get_next_commitment_mask_and_script_key(255-257)find_script_key_id_from_commitment_mask_key_id(273-280)get_diffie_hellman_shared_secret(304-311)construct_range_proof(367-375)get_script_signature(327-342)get_partial_script_signature(344-361)get_partial_txo_kernel_signature(521-542)get_txo_kernel_signature_excess_with_offset(544-551)get_txo_private_kernel_offset(512-519)encrypt_data_for_recovery(557-566)try_output_key_recovery(583-591)is_this_output_ours(593-601)get_script_offset(378-385)get_metadata_signature(423-440)get_one_sided_metadata_signature(442-463)sign_message_with_spend_key(395-402)sign_script_message(387-393)sign_script_message_with_spend_key(404-411)sign_with_nonce_and_challenge(413-421)get_receiver_partial_metadata_signature(465-484)get_sender_partial_metadata_signature(489-506)generate_burn_claim_signature(313-321)stealth_address_script_spending_key(603-610)get_private_key(219-221)base_layer/transaction_components/src/key_manager/manager.rs (32)
get_random_key(474-500)get_public_key_at_key_id(502-566)create_encrypted_key(568-579)get_commitment(581-590)verify_mask(592-603)get_view_key(605-609)get_private_view_key(611-613)get_spend_key(615-622)get_next_commitment_mask_and_script_key(624-634)find_script_key_id_from_commitment_mask_key_id(636-653)get_diffie_hellman_shared_secret(655-668)construct_range_proof(670-712)get_script_signature(714-764)get_partial_script_signature(766-800)get_partial_txo_kernel_signature(802-841)get_txo_kernel_signature_excess_with_offset(843-852)get_txo_private_kernel_offset(854-873)encrypt_data_for_recovery(875-898)try_output_key_recovery(900-940)is_this_output_ours(942-962)get_script_offset(964-984)get_metadata_signature(986-1021)get_one_sided_metadata_signature(1023-1060)sign_message_with_spend_key(1062-1091)sign_script_message(1093-1108)sign_script_message_with_spend_key(1110-1125)sign_with_nonce_and_challenge(1127-1161)get_receiver_partial_metadata_signature(1163-1199)get_sender_partial_metadata_signature(1204-1239)generate_burn_claim_signature(1241-1259)stealth_address_script_spending_key(1261-1274)get_private_key(1278-1340)
⏰ 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). (10)
- GitHub Check: wasm build tests
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with msrv
- GitHub Check: ledger build tests
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (28)
base_layer/transaction_components/src/key_manager/manager.rs (1)
1277-1341: LGTM: Private key derivation logic is well-structured.The
get_private_keyimplementation correctly handles allTariKeyIdvariants with appropriate domain separation for derived keys and proper error handling for ledger keys. The match statement is comprehensive and the cryptographic derivations follow the expected patterns.base_layer/transaction_components/src/key_manager/interface.rs (2)
60-275: Trait method signatures are consistent and well-defined.The
TransactionKeyManagerInterfacetrait provides a comprehensive synchronous API for transaction key management operations. The method signatures are clear, consistent in their error handling withKeyManagerError, and properly bounded withClone + Send + Sync + 'staticfor concurrent usage.Note: The async → sync conversion is a breaking change already flagged in past review comments.
277-280: LGTM: Secret key interface is appropriately minimal.The
SecretTransactionKeyManagerInterfacetrait correctly extendsTransactionKeyManagerInterfaceand exposes only the essential private key access method. This design properly segregates secret key operations from public key operations.base_layer/transaction_components/src/coinbase_builder.rs (4)
216-225: LGTM: Clean async-to-sync conversion.The
buildmethod has been successfully converted from async to synchronous, aligning with the PR's objective to make the key manager stateless. The call tobuild_with_rewardon line 224 is now synchronous.
356-470: LGTM: Public coinbase generation functions converted to synchronous.The public functions
generate_coinbaseandgenerate_coinbase_with_wallet_outputhave been successfully converted from async to sync. All key manager operations (lines 420, 439, 454) are now synchronous, and error handling remains correct.
472-1177: LGTM: Test suite successfully updated to use synchronous KeyManager.The test module has been comprehensively updated:
KeyManager::new_random()replaces async memory-backed key manager initialization (line 505)- All key manager operations are now synchronous (lines 584-586, 795, 950-951, 1068-1069, 1137)
- Test functions remain async (for tokio::test) but correctly invoke synchronous APIs
- Test logic and assertions unchanged
233-351: I need to verify the cryptographic design decision for nonce generation in this codebase.The review comment is based on an incorrect assumption and should be dismissed.
Evidence shows that random nonces are required for Schnorr signatures, with each signature requiring a random private key, and reusing the secret nonce value on different messages allows private key recovery. The codebase consistently uses
get_random_key(None, false)for nonce generation across all transaction types (kernel signatures, wallet outputs, coinbase transactions). This is not a behavioral change introduced by the async-to-sync conversion—it is the established, cryptographically correct pattern throughout the implementation. The coinbase builder at line 257 follows the same pattern as every other nonce generation in the codebase, confirming that random nonce generation is intentional and appropriate.Likely an incorrect or invalid review comment.
base_layer/core/src/blocks/pre_mine/mod.rs (5)
27-38: LGTM: Import changes align with stateless KeyManager refactor.The addition of
KeyManagerto the imports alongside the existing trait interfaces is correct for the stateless key manager migration.
795-795: LGTM: Function signature correctly updated to synchronous.The removal of
asyncfrom the function signature aligns with the PR objective to remove all async usage from the key manager.
862-864: LGTM: Synchronous signing and building methods correctly updated.The conversion from async to synchronous calls for
sign_as_sender_and_receiverandtry_buildis correct, with proper reference passing to the key manager.
871-879: LGTM: Kernel signature challenge building correctly updated.The synchronous kernel signature challenge construction with explicit parameters matches the expected
build_kernel_signature_challengemethod signature and correctly usesTransactionKernelVersion::get_current_version().
970-970: LGTM: Test correctly updated to call synchronous function.The removal of
.awaitfrom the function call is correct for the now-synchronouscreate_pre_mine_genesis_block_infofunction.applications/minotari_console_wallet/src/automation/commands.rs (16)
38-106: New key/ledger imports are consistent with downstream usageThe added imports for
LedgerKeyBranch,WalletType,TariKeyId,TransactionKeyManagerInterface,LegacyWalletType,RistrettoSecretKey, andpublic_key_to_output_encryption_keyare all used coherently in the updated pre‑mine, VN registration, and key‑export flows and fit the new stateless key manager model.
1177-1209: Pre‑mine encumber memo and parameter wiring look correctThe memo for each encumbered output now uses
MemoField::new_open_from_stringwithdetect_tx_metadata(&wallet, ¤t_recipient_address).await, and the resultingmemopluscurrent_recipient_addressare threaded through toencumber_aggregate_utxo; error handling is non‑panicking and consistent with other commands.
1420-1516: Key‑manager based signing, decryption, and script/metadata offsets are wired coherentlyUsing
TransactionInputVersion::get_current_version()andTransactionOutputVersion::get_current_version()for challenges,sign_with_nonce_and_challengefor both script and metadata signatures, deriving the encryption key viapublic_key_to_output_encryption_key, and then callingcreate_encrypted_key/verify_maskandstealth_address_script_spending_key/get_script_offsetforms a consistent, key‑manager‑centric pipeline; all fallible steps are guarded with explicit error handling rather than unwraps.
1710-1730: Stealth one‑sided send memo construction is robust
SendOneSidedToStealthAddressnow builds a memo usingMemoField::new_open_from_stringanddetect_tx_metadataand passes the resultingmemointosend_one_sided_to_stealth_address; on failure it logs a specific error and continues, matching the console‑wallet command error pattern.
1746-1767:MakeItRainmemo handling integrates cleanly with the new Tx metadata helperThe command now derives a
TxTypeviadetect_tx_metadata(&wallet, &args.destination).await, constructs a memo accordingly, and threads it through tomake_it_rain, while preserving best‑effort error handling (cleareprintln!and earlycontinueon memo failure).
1774-1786: Coin split memo uses dedicatedTxType::CoinSplit
CoinSplit’s memo is now created withMemoField::new_open_from_string(&args.payment_id, TxType::CoinSplit), which accurately tags this as a coin‑split transaction, and the error path is non‑panicking and descriptive.
1812-1813:UnblindedOutput::from_wallet_outputupdated correctly for key‑manager dependencyBoth
ExportUtxosandExportSpentUtxosnow pass&wallet.key_manager_serviceintoUnblindedOutput::from_wallet_output, which is consistent with the stateless key manager refactor and keeps the CLI commands from owning key‑reconstruction logic themselves.Also applies to: 1882-1884
1969-1987: Finalise SHA atomic swap memo is correctly plumbed
FinaliseShaAtomicSwapnow builds a memo usingTxType::ClaimAtomicSwapand passes it through tofinalise_sha_atomic_swap; memo errors are reported with a clear, command‑specific message before continuing.
2001-2018: HTLC refund memo tagging is appropriate
ClaimShaAtomicSwapRefundusesTxType::HtlcAtomicSwapRefundfor its memo and propagates it intoclaim_htlc_refund; the error path is descriptive and non‑fatal to the CLI process.
2032-2061: Validator node registration: memo and signature construction look soundThe VN registration command now includes a memo (
TxType::ValidatorNodeRegistration) and passes it toregister_validator_node. TheCompressedSignatureconstruction fromvalidator_node_public_nonceandRistrettoSecretKey::from_vec(...)plus optionalsidechain_deployment_keyviafrom_canonical_bytes(...)is consistent with the ristretto key usage elsewhere; all fallible conversions use?rather than panicking.
2170-2173: View/spend key export uses new key manager API correctly
ExportViewKeyAndSpendKeynow callsget_view_key,get_spend_key, andget_private_view_keyonkey_manager_serviceand serializes the public/private view key plus spend key pubkey as expected for this command; no async or panics involved.
2235-2253: Importing paper wallet withLegacyWalletType::DerivedKeysfits the migration storyFor the temporary wallet used to import a paper wallet, using
LegacyWalletType::DerivedKeysand threading it intoinit_walletviaSome(wallet_type)is consistent with the Legacy wallet type refactor and keeps this flow explicitly in the legacy key space.
2501-2501:SignMessagecorrectly delegates to spend‑key signingSwitching to
key_manager_service.sign_message_with_spend_key(&commitment_bytes, sender_offset)centralizes signing in the key manager and avoids ad‑hoc key handling in the CLI; error handling remains viaCommandError.
2776-2778:SignScriptMessagenow uses spend‑key-based script signing
sign_script_message_with_spend_keymirrors the pattern fromSignMessage, pushing script‑message signing into the key manager while preserving the same sender offset semantics.
2526-2551: One‑sided prepare‑for‑signing: memo integration is clean
PrepareOneSidedTransactionForSigningnow derives a memo usingdetect_tx_metadata(&wallet, &args.destination).awaitand passes it intoprepare_one_sided_transaction_for_signingalong with destination, amount, selection criteria, output features, and fee; memo failures are handled with cleareprintln!output andcontinue, not panics.
2847-2863:detect_tx_metadatahelper correctly classifies self vs other paymentsThe new async helper compares the destination against both the wallet’s interactive and one‑sided addresses and returns
TxType::PaymentToSelfonly in those cases, defaulting toPaymentToOtheron mismatch or retrieval error; this centralizes memoTxTypeselection and is used consistently by commands that need per‑destination metadata.
| let memo = match MemoField::new_open_from_string(&args.payment_id, TxType::ClaimAtomicSwap) { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| eprintln!("CoinSplit error! Could not create memo field from payment id string: {e}"); | ||
| continue; | ||
| }, | ||
| }; | ||
| match init_sha_atomic_swap( | ||
| transaction_service.clone(), | ||
| config.fee_per_gram, | ||
| args.amount, | ||
| UtxoSelectionCriteria::default(), | ||
| args.destination, | ||
| MemoField::open_from_string(&args.payment_id, TxType::ClaimAtomicSwap), | ||
| memo, | ||
| ) |
There was a problem hiding this comment.
Fix error label in InitShaAtomicSwap memo construction
Logic‑wise the memo creation and propagation into init_sha_atomic_swap look good, but the error message still says "CoinSplit error!", which is confusing for users invoking the HTLC init command.
Consider:
- eprintln!("CoinSplit error! Could not create memo field from payment id string: {e}");
+ eprintln!("InitShaAtomicSwap error! Could not create memo field from payment id string: {e}");📝 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.
| let memo = match MemoField::new_open_from_string(&args.payment_id, TxType::ClaimAtomicSwap) { | |
| Ok(v) => v, | |
| Err(e) => { | |
| eprintln!("CoinSplit error! Could not create memo field from payment id string: {e}"); | |
| continue; | |
| }, | |
| }; | |
| match init_sha_atomic_swap( | |
| transaction_service.clone(), | |
| config.fee_per_gram, | |
| args.amount, | |
| UtxoSelectionCriteria::default(), | |
| args.destination, | |
| MemoField::open_from_string(&args.payment_id, TxType::ClaimAtomicSwap), | |
| memo, | |
| ) | |
| let memo = match MemoField::new_open_from_string(&args.payment_id, TxType::ClaimAtomicSwap) { | |
| Ok(v) => v, | |
| Err(e) => { | |
| eprintln!("InitShaAtomicSwap error! Could not create memo field from payment id string: {e}"); | |
| continue; | |
| }, | |
| }; | |
| match init_sha_atomic_swap( | |
| transaction_service.clone(), | |
| config.fee_per_gram, | |
| args.amount, | |
| UtxoSelectionCriteria::default(), | |
| args.destination, | |
| memo, | |
| ) |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/automation/commands.rs around lines
1940 to 1954, the error message when MemoField::new_open_from_string fails
incorrectly uses the label "CoinSplit error!" which is misleading for the
InitShaAtomicSwap flow; change the stderr message to reflect the correct
operation (e.g., "InitShaAtomicSwap error! Could not create memo field from
payment id string: {e}") so users see the right context, leaving the existing
continue behavior intact.
| let key_manager = KeyManager::new_random().map_err(|e| e.to_string())?; | ||
| let view_key = public_key_to_output_encryption_key(&CompressedPublicKey::new_from_pk(total_script_key)) | ||
| .map_err(|e| e.to_string())?; | ||
| let view_key_id = key_manager | ||
| .import_key(view_key.clone(), None) | ||
| .await | ||
| .create_encrypted_key(view_key.clone(), None) | ||
| .map_err(|e| e.to_string())?; | ||
| let address_len = u8::try_from(public_keys.len()).map_err(|e| e.to_string())?; | ||
|
|
||
| let (commitment_mask, script_key) = key_manager | ||
| .get_next_commitment_mask_and_script_key() | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| total_private_key = total_private_key + | ||
| &key_manager | ||
| .get_private_key(&commitment_mask.key_id) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| let commitment = key_manager | ||
| .get_commitment(&commitment_mask.key_id, &item.value.into()) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| let mut commitment_bytes = [0u8; 32]; | ||
| commitment_bytes.clone_from_slice(commitment.as_bytes()); | ||
|
|
||
| let sender_offset = key_manager | ||
| .get_next_key(TransactionKeyManagerBranch::SenderOffset.get_branch_key()) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| let sender_offset = key_manager.get_random_key(None, true).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
Critical: Non-deterministic key generation breaks genesis block consensus.
Using KeyManager::new_random() on line 813 creates non-deterministic keys for each output, which makes the genesis block different on every execution. This is a critical consensus issue because:
- The
total_private_key(accumulated on lines 824-827) will differ each time - The kernel signature (lines 869-891) depends on
total_private_key, making it non-deterministic - All commitment masks, script keys, and sender offsets will be random and unreproducible
- Different nodes would generate different genesis blocks, causing a network fork at genesis
For genesis block creation, the key manager must use a deterministic seed or the keys must be passed as parameters to ensure reproducibility across all nodes.
Consider passing a seeded/deterministic KeyManager as a parameter to this function, or derive keys deterministically from the input parameters.
🤖 Prompt for AI Agents
In base_layer/core/src/blocks/pre_mine/mod.rs around lines 813 to 834, the code
calls KeyManager::new_random() which creates non-deterministic keys and thus
makes genesis block construction non-reproducible; replace this by using a
deterministic KeyManager instance (either accept a KeyManager parameter to the
function or construct one from a fixed/derived seed) and remove any calls to
new_random(); ensure all subsequent calls (create_encrypted_key,
get_next_commitment_mask_and_script_key, get_private_key, get_commitment,
get_random_key for sender_offset, etc.) use that deterministic manager so
total_private_key, commitment masks, script keys and sender offsets are
reproducible across nodes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
base_layer/transaction_components/src/key_manager/manager.rs (3)
147-162: Consider extracting the domain string to a constant.The domain string
"KEY_MANAGER_private_key"appears both here (line 153) and indecrypt_encrypted_key(line 433). While a past review comment about this was disputed, extracting it to a named constant (e.g.,const ENCRYPTION_DOMAIN: &[u8] = b"KEY_MANAGER_private_key";) would improve maintainability and prevent typos if the domain ever needs to change.
211-213: Inefficient string conversion pattern.The pattern
TariKeyId::from_str(key_str.to_string().as_str())convertsBox<str>→String→&str→ parse back toTariKeyId. Sincekeyis already aBox<str>, you can useTariKeyId::from_str(&**key)orTariKeyId::from_str(key.as_ref())to avoid the unnecessaryStringallocation. This pattern appears throughout the file at lines 277, 297, 506, 538, 549, 557, 1286, 1318, 1329, and 1335.Apply this pattern throughout:
- let key = TariKeyId::from_str(key_str.to_string().as_str()) + let key = TariKeyId::from_str(key_str.as_ref()) .map_err(|_| KeyManagerError::InvalidKeyId(script_key_id.to_string()))?;
901-941: Consider extracting helper methods for improved readability.The
try_output_key_recoverymethod contains nested match statements and handles two different decryption paths (view key vs. DH shared secret). Consider extracting helper methods liketry_decrypt_with_view_keyandtry_decrypt_with_dh_secretto reduce cognitive complexity and improve testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/transaction_components/src/key_manager/manager.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hansieodendaal
Repo: tari-project/tari PR: 7432
File: integration_tests/src/ffi/ffi_import.rs:351-356
Timestamp: 2025-08-22T10:22:27.310Z
Learning: The PR "feat!: remove comms from wallet" is a breaking change PR where backward compatibility is intentionally not maintained. The FFI functions `comms_config_create` and `comms_config_destroy` were renamed to `wallet_db_config_create` and `wallet_db_config_destroy` as part of the intentional breaking changes.
📚 Learning: 2025-06-16T14:49:31.831Z
Learnt from: martinserts
Repo: tari-project/tari 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.
Applied to files:
base_layer/transaction_components/src/key_manager/manager.rs
🧬 Code graph analysis (1)
base_layer/transaction_components/src/key_manager/manager.rs (4)
applications/minotari_ledger_wallet/comms/src/accessor_methods.rs (7)
ledger_get_dh_shared_secret(431-463)ledger_get_one_sided_metadata_signature(551-636)ledger_get_public_key(228-259)ledger_get_raw_schnorr_signature(466-507)ledger_get_script_offset(330-408)ledger_get_script_schnorr_signature(510-548)ledger_get_script_signature(262-327)base_layer/common_types/src/encryption.rs (2)
decrypt_bytes_integral_nonce(53-75)encrypt_bytes_integral_nonce(78-104)base_layer/transaction_components/src/transaction_components/one_sided.rs (3)
diffie_hellman_stealth_domain_hasher(63-67)public_key_to_output_encryption_key(36-43)public_key_to_output_spending_key(46-53)base_layer/transaction_components/src/transaction_components/encrypted_data.rs (2)
encrypt_data(84-129)decrypt_data(134-194)
🔇 Additional comments (2)
base_layer/transaction_components/src/key_manager/manager.rs (2)
803-842: LGTM! Good handling of coinbase and input/output cases.The method correctly handles the special case where coinbase transactions don't use kernel offsets (lines 818-822), and appropriately negates the signing key for inputs vs. outputs (lines 826-830). The comments clearly explain the non-obvious logic, which is essential for maintaining this code.
1128-1162: Good validation logic for ledger key pairing.The method correctly enforces that both
private_key_idandnoncemust be either ledger keys or non-ledger keys, preventing unsafe mixing of key types. This is important for security.
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
It is looking good.
I would like to know which system-level tests have been done on this branch? Recovery and spending for mainnet ledger pre-mine wallets and normal mainnet wallets come to mind.
base_layer/transaction_key_manager/src/legacy_key_manager/error.rs
Outdated
Show resolved
Hide resolved
3 participants
Description
Changes the key manager to make it stateless.
This allows the key manager to only work from view and spend keys.
Removes all async from the key manager.
Motivation and Context
Simplify the usage of the key manager
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor