fix!: require a specific signer for input spend + key manager backend - #1602
Conversation
WalkthroughIntroduces signer-scoped stealth transfer handling and metadata hashing: adds Executable::signers_iter, threads a required_signer through stealth statements/validations, replaces spend_stealth_utxos with validate_and_spend_stealth_utxos returning ValidatedStealthTransfer, adds Hash64, refactors key management to LocalKeyStore and updates transaction builder/signing flows and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WalletSDK as Wallet SDK
participant KeyStore as LocalKeyStore / KeyMgr
participant Crypto as Wallet Crypto
participant Builder as TransactionBuilder
participant Engine as Engine Runtime
participant State as WorkingState
WalletSDK->>KeyStore: derive signing key -> public bytes (required_signer)
KeyStore-->>WalletSDK: required_signer
WalletSDK->>Crypto: create_transfer_statement(required_signer, outputs)
Crypto-->>WalletSDK: StealthTransferStatement + owner proofs
WalletSDK->>Builder: builder.map(...sign...) -> build_with_signatures(signatures)
Builder->>Engine: submit transaction
Engine->>State: validate_and_spend_stealth_utxos(stmt, view_key)
State->>State: check required_signer in scope
State->>State: validate ownership proofs (required_signer, metadata_hash)
State->>State: validate transfer balance -> ValidatedStealthTransfer
State-->>Engine: validation result / AccessDeniedStealthTransferSigner
Engine-->>WalletSDK: submission result
sequenceDiagram
autonumber
participant TX as Transaction (Executable)
participant Wrapped as WrappedTransaction / TemplateTest
TX->>TX: signers_iter() yields [seal_signer?, signatures...]
TX-->>Builder: main_signer = signers_iter().next()
Wrapped->>TX: WrappedTransaction.signers_iter() delegates to transaction.signers_iter()
Note over TX,Wrapped: signers_iter used to auto-add proofs and build auth params
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (1)
crates/wallet/sdk/src/apis/key_manager.rs (1)
195-205: Bug: view_only_key_id uses the wrong key idYou derive view_only_key but set view_only_key_id from key (owner). Should use view_only_key.as_key_id().
Apply:
- view_only_key_id: key.as_key_id(), - owner_key_id: key.as_key_id(), + view_only_key_id: view_only_key.as_key_id(), + owner_key_id: key.as_key_id(),
🧹 Nitpick comments (17)
crates/template_lib_types/src/crypto/scalar.rs (1)
28-30: Consider extracting the zero-check pattern.The exact same implementation appears in
RistrettoPublicKeyBytes::is_zero()(incrates/template_lib_types/src/crypto/ristretto.rslines 52-54). If this pattern appears in additional fixed-size byte array types, consider extracting it into a helper trait or macro to reduce duplication.Example trait approach:
pub trait FixedByteArrayZeroCheck { fn is_zero(&self) -> bool; } impl<const N: usize> FixedByteArrayZeroCheck for [u8; N] { fn is_zero(&self) -> bool { self.iter().all(|&b| b == 0) } }Then both
Scalar32BytesandRistrettoPublicKeyBytescould leverage this trait by delegating toself.0.is_zero().crates/engine/src/executables/transaction.rs (1)
26-31: Signer ordering logic looks correct; watch for duplicates.Yields seal signer (if authorized) then all signature keys. If consumers assume uniqueness, consider deduping or documenting that duplicates may appear.
crates/transaction/src/builder/mod.rs (1)
418-430: Guard against missingwith_authorized_seal_signer()when signatures are present.Current default only authorizes the seal signer if there are no signatures. With pre-attached signatures, forgetting to call
with_authorized_seal_signer()can undermine the “required signer” guarantee. Add a debug assertion to catch this during development.Suggested patch:
pub fn build(self) -> UnsealedTransactionV1 { - let builder = self.then(|builder| { + let builder = self.then(|builder| { // This is so that we dont have to add this in a lot of places - TODO: this is an assumption that may not // apply to all transactions if builder.signatures.is_empty() { builder.with_authorized_seal_signer() } else { builder } }); + // Dev-safety: if there are signatures, require explicit seal signer authorization + #[cfg(debug_assertions)] + { + let has_sigs = !builder.signatures.is_empty(); + let is_authorized = match &builder.unsigned_transaction { + UnsignedTransaction::V1(tx) => tx.is_seal_signer_authorized, + }; + debug_assert!( + !has_sigs || is_authorized, + "Transactions with pre-attached signatures should call \ + `with_authorized_seal_signer()` before signing to commit the required signer" + ); + } + builder.unsigned_transaction.build_with_signatures(builder.signatures) }Also applies to: 429-429
crates/wallet/crypto/src/stealth.rs (1)
94-116: Avoid duplicating inputs_statement and extra Vec clone.Build
inputs_statementonce and reuse it for the balance proof and final return. This removes a clone and reduces duplication.- let inputs_statement = StealthInputsStatement { - inputs: inputs_to_spend.clone(), - revealed_amount: revealed_input_amount, - required_signer, - }; + let inputs_statement = StealthInputsStatement { + inputs: inputs_to_spend, + revealed_amount: revealed_input_amount, + required_signer, + }; let balance_proof = generate_stealth_balance_proof_signature( &agg_input_mask, &agg_output_mask, &inputs_statement, &outputs_statement, ); - Ok(StealthTransferStatement { - inputs_statement: StealthInputsStatement { - inputs: inputs_to_spend, - revealed_amount: revealed_input_amount, - required_signer, - }, - outputs_statement, - balance_proof, - }) + Ok(StealthTransferStatement { + inputs_statement, + outputs_statement, + balance_proof, + })applications/tari_walletd/src/handlers/accounts.rs (1)
997-1013: Avoid double-releasing the lock on dry-run failureYou release before the dry run and again on error. The second release just logs an error. Simplify by releasing once.
Apply this diff:
if req.dry_run { // Release the lock immediately as dry run does not submit the transaction - // TODO: maybe transfer() should not lock the outputs if it's a dry run if let Err(err) = sdk.stealth_outputs_api().release_lock(transfer.lock_id) { error!( target: LOG_TARGET, "Failed to release locked outputs for dry run : {}", err ); } let result = transaction_service.submit_dry_run_transaction(transaction).await; return match result { Ok(res) => Ok(StealthTransferResponse { transaction_id: res.finalize.transaction_hash.into(), }), Err(e) => { - if let Err(err) = sdk.stealth_outputs_api().release_lock(transfer.lock_id) { - error!( - target: LOG_TARGET, - "Failed to release locked outputs after dry run failure: {}", - err - ); - } - Err(anyhow::anyhow!("Dry run transaction failed: {}", e)) }, }; }crates/wallet/crypto/tests/stealth_transfer_statement.rs (1)
4-8: Use rand::rngs::OsRng instead of aead::OsRngAvoid mixing RNG types; use rand::rngs::OsRng which matches tari_crypto expectations.
-use chacha20poly1305::aead::OsRng; +use rand::rngs::OsRng;crates/engine/tests/stealth.rs (1)
350-357: Guard against OOB when corrupting the range proofIf agg_range_proof length changes, rp[100] could panic. Use a checked index.
- rp[100] ^= 0xFF; // Corrupt the range proof + let idx = 100.min(rp.len().saturating_sub(1)); + rp[idx] ^= 0xFF; // Corrupt the range proofcrates/engine_types/src/stealth/transfer.rs (1)
54-63: Also require zero public nonce for the revealed-only edge caseYou enforce zero signature when there are no stealth inputs/outputs. For a canonical encoding, also require the public nonce to be zero.
- if balance_proof.get_signature().as_bytes() != RistrettoSecretKey::default().as_bytes() { + if balance_proof.get_signature().as_bytes() != RistrettoSecretKey::default().as_bytes() + || !balance_proof.get_public_nonce().is_zero() + { return Err(ResourceError::InvalidBalanceProof { details: "Balance proof signature verification failed for revealed amount. This typically indicates \ that the transfer statement total input amount != total output amount." .to_string(), }); }crates/template_test_tooling/src/template_test.rs (1)
524-539: Auto-extend proofs from tx signers — consider dedupBehavior aligns with the new signer requirement. Minor: extend() may introduce duplicates if callers already provided identity proofs. Not harmful, but you could dedup to keep auth scope tidy.
Example:
- if self.auto_add_proofs_from_signers { - proofs.extend( - transaction - .signers_iter() - .map(|pk| NonFungibleAddress::from_public_key(*pk)), - ); - } + if self.auto_add_proofs_from_signers { + use std::collections::HashSet; + let mut seen = HashSet::new(); + proofs.retain(|p| seen.insert(p.clone())); + proofs.extend( + transaction + .signers_iter() + .map(|pk| NonFungibleAddress::from_public_key(*pk)) + .filter(|p| seen.insert(p.clone())), + ); + }crates/engine/src/runtime/working_state.rs (1)
270-312: Signer-in-scope enforcement and ownership proof binding: solid; minor readability tweakLogic correctly:
- Enforces required signer presence in auth scope
- Binds ownership proofs to required signer + outputs metadata
- Locks/downs each UTXO and validates
Small readability improvement for the scope check:
- let proofs = self.base_call_scope().auth_scope().virtual_proofs(); - if proofs - .iter() - .filter(|p| *p.resource_address() == PUBLIC_IDENTITY_RESOURCE_ADDRESS) - .all(|p| p.id().as_u256().map(|b| b.as_slice()) != Some(required_signer.as_bytes())) - { + let proofs = self.base_call_scope().auth_scope().virtual_proofs(); + if !proofs.iter().any(|p| + *p.resource_address() == PUBLIC_IDENTITY_RESOURCE_ADDRESS && + p.id() + .as_u256() + .map(|b| b.as_slice() == required_signer.as_bytes()) + .unwrap_or(false) + ) { return Err(RuntimeError::AccessDeniedStealthTransferSigner { required_signer: *required_signer, }); }Note: The lock/unlock order (down -> unlock -> validate) is acceptable in this single-threaded transaction context; if you prefer stronger TOCTOU hygiene, keep the lock until after validation. Optional.
Ensure all call sites now use
validate_and_spend_stealth_utxosand no one references the removedspend_stealth_utxos.crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
445-474: Fee/input lock rollback on error is helpfulReleasing fee locks on input-selection error reduces stuck locks. Consider unifying both locks under a single DB transaction in future.
crates/wallet/sdk/src/key_managers/local.rs (1)
54-64: Error variants likely obsolete
PasswordManagerApiErrorandReadOnlyModearen’t used in this backend after refactor to key_store-only. Consider pruning to simplify API.crates/wallet/sdk/src/local_key_store.rs (1)
8-16: Tighten module dependency to avoid circular couplingLocalKeyStore depends on apis::key_manager::WalletKeyManager while key_manager.rs depends on LocalKeyStore. Move the WalletKeyManager type alias to a neutral module (e.g., a crypto/types module) to avoid cross-module coupling.
Also applies to: 19-24
crates/engine_types/src/crypto/messages.rs (1)
74-79: Use a dedicated domain label for metadata hashingstealth_statement_metadata64 uses EngineHashDomainLabel::StealthOwnership. Prefer a distinct label (e.g., StealthStatementMetadata) to prevent ambiguity and accidental cross-domain collisions.
Apply if the label exists (or add it):
-pub fn stealth_statement_metadata64(outputs_statement: &StealthOutputsStatement) -> Hash64 { - engine_hasher64(EngineHashDomainLabel::StealthOwnership) +pub fn stealth_statement_metadata64(outputs_statement: &StealthOutputsStatement) -> Hash64 { + engine_hasher64(EngineHashDomainLabel::StealthStatementMetadata) .chain(outputs_statement) .result() .into() }crates/engine_types/src/hash.rs (2)
15-23: Incorrect docs: says 32 bytes; type is 64 bytesFix misleading docs to 64 bytes and update panic notes.
Apply:
-/// Representation of a 32-byte hash value +/// Representation of a 64-byte hash value @@ - /// Panics if `N` is greater than Self::LENGTH (32) + /// Panics if `N` is greater than Self::LENGTH (64) @@ - /// Panics if `N` is greater than Self::LENGTH (32) + /// Panics if `N` is greater than Self::LENGTH (64)Also applies to: 64-75, 77-88
140-144: Avoid DerefMut for hash typesExposing &mut [u8] lets callers mutate a Hash64 in-place, breaking invariants if used as map keys or cached values. Prefer immutable accessors only.
Apply:
-impl DerefMut for Hash64 { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -}crates/wallet/sdk/src/apis/key_manager.rs (1)
217-233: Align index parameter types for consistencyderive_view_only_keypair/derive_account_key_pair take u64 while most APIs use DerivedKeyIndex. Consider switching to DerivedKeyIndex for consistency and type clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
applications/tari_app_utilities/src/transaction_executor.rs(2 hunks)applications/tari_walletd/src/handlers/accounts.rs(5 hunks)applications/tari_walletd/src/handlers/nfts.rs(1 hunks)applications/tari_walletd/src/handlers/transaction.rs(2 hunks)applications/tari_walletd/src/handlers/validator.rs(1 hunks)crates/engine/src/executables/mod.rs(1 hunks)crates/engine/src/executables/transaction.rs(1 hunks)crates/engine/src/runtime/error.rs(2 hunks)crates/engine/src/runtime/working_state.rs(5 hunks)crates/engine/tests/signature.rs(3 hunks)crates/engine/tests/stealth.rs(27 hunks)crates/engine/tests/test.rs(1 hunks)crates/engine_types/src/crypto/messages.rs(1 hunks)crates/engine_types/src/crypto/mod.rs(0 hunks)crates/engine_types/src/crypto/utxo_spend.rs(0 hunks)crates/engine_types/src/hash.rs(1 hunks)crates/engine_types/src/lib.rs(1 hunks)crates/engine_types/src/stealth/transfer.rs(5 hunks)crates/p2p/proto/transaction.proto(1 hunks)crates/p2p/src/conversions/transaction.rs(2 hunks)crates/template_builtin/templates/faucet/src/lib.rs(1 hunks)crates/template_lib/src/models/non_fungible.rs(1 hunks)crates/template_lib/src/models/stealth.rs(3 hunks)crates/template_lib_types/src/crypto/scalar.rs(1 hunks)crates/template_lib_types/src/encrypted_data.rs(1 hunks)crates/template_lib_types/src/max_bytes.rs(2 hunks)crates/template_test_tooling/src/support/stealth.rs(7 hunks)crates/template_test_tooling/src/template_test.rs(6 hunks)crates/template_test_tooling/src/wrapped_transaction.rs(1 hunks)crates/transaction/src/builder/mod.rs(2 hunks)crates/transaction/src/unsigned_transaction.rs(1 hunks)crates/wallet/crypto/src/balance_proof.rs(3 hunks)crates/wallet/crypto/src/stealth.rs(7 hunks)crates/wallet/crypto/tests/output_statement.rs(0 hunks)crates/wallet/crypto/tests/stealth_transfer_statement.rs(1 hunks)crates/wallet/sdk/src/apis/key_manager.rs(4 hunks)crates/wallet/sdk/src/apis/stealth_crypto.rs(2 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(3 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(12 hunks)crates/wallet/sdk/src/key_managers/backend.rs(2 hunks)crates/wallet/sdk/src/key_managers/local.rs(3 hunks)crates/wallet/sdk/src/local_key_store.rs(2 hunks)crates/wallet/sdk/src/models/key.rs(0 hunks)crates/wallet/sdk/src/sdk.rs(3 hunks)utilities/tariswap_test_bench/src/tariswap.rs(2 hunks)
💤 Files with no reviewable changes (4)
- crates/wallet/sdk/src/models/key.rs
- crates/engine_types/src/crypto/utxo_spend.rs
- crates/wallet/crypto/tests/output_statement.rs
- crates/engine_types/src/crypto/mod.rs
🧰 Additional context used
🧬 Code graph analysis (37)
crates/engine/src/executables/mod.rs (3)
crates/engine/src/executables/transaction.rs (1)
signers_iter(26-31)crates/template_test_tooling/src/wrapped_transaction.rs (1)
signers_iter(43-45)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/template_lib_types/src/max_bytes.rs (1)
crates/template_lib_types/src/encrypted_data.rs (1)
empty(26-28)
crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
utilities/tariswap_test_bench/src/tariswap.rs (3)
crates/transaction/src/transaction.rs (1)
builder(47-49)utilities/transaction_generator/src/transaction_builders/free_coins.rs (1)
builder(12-36)utilities/transaction_generator/src/transaction_builders/manifest.rs (1)
builder(14-31)
crates/template_test_tooling/src/wrapped_transaction.rs (3)
crates/engine/src/executables/mod.rs (1)
signers_iter(21-21)crates/engine/src/executables/transaction.rs (1)
signers_iter(26-31)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/engine_types/src/lib.rs (1)
crates/storage/src/global/models/validator_node.rs (1)
hash(45-47)
applications/tari_walletd/src/handlers/validator.rs (3)
crates/engine_types/src/validator_fee.rs (1)
claim_public_key(156-158)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/models/key.rs (1)
derived(297-299)
crates/template_lib_types/src/encrypted_data.rs (1)
crates/template_lib_types/src/max_bytes.rs (1)
empty(39-41)
applications/tari_walletd/src/handlers/transaction.rs (3)
crates/transaction/src/transaction.rs (2)
builder(47-49)signatures(99-103)crates/transaction/src/builder/mod.rs (1)
signatures(410-412)crates/transaction/src/v1/transaction.rs (1)
signatures(64-66)
crates/engine/tests/test.rs (2)
crates/engine_types/src/resource.rs (1)
owner_key(115-117)crates/template_test_tooling/src/template_test.rs (1)
to_public_key_bytes(416-418)
crates/transaction/src/unsigned_transaction.rs (1)
crates/transaction/src/builder/mod.rs (3)
signatures(410-412)new(53-59)build(418-430)
crates/template_lib/src/models/non_fungible.rs (1)
bindings/src/types/NonFungibleId.ts (1)
NonFungibleId(6-6)
crates/wallet/sdk/src/key_managers/backend.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/local_key_store.rs (1)
derive_secret(47-54)crates/wallet/sdk/src/models/key.rs (1)
key_index(221-223)
crates/wallet/crypto/tests/stealth_transfer_statement.rs (9)
bindings/src/types/PrivateOutput.ts (1)
PrivateOutput(6-11)bindings/src/types/UtxoOutput.ts (1)
UtxoOutput(6-14)crates/wallet/crypto/src/balance_proof.rs (1)
generate_stealth_balance_proof_signature(38-55)crates/wallet/crypto/src/stealth.rs (3)
create_outputs_statement(118-161)create_transfer_statement(29-116)output_statements(122-152)crates/wallet/crypto/src/confidential.rs (1)
create_withdraw_proof(21-72)crates/template_lib/src/models/confidential_proof.rs (1)
is_revealed_only(107-119)crates/engine_types/src/stealth/transfer.rs (2)
validate_transfer_balance(33-134)validate_ownership_proof(136-171)crates/engine_types/src/crypto/messages.rs (1)
stealth_statement_metadata64(74-79)crates/template_lib_types/src/encrypted_data.rs (3)
empty(26-28)try_from(76-86)min_size(30-32)
crates/wallet/sdk/src/key_managers/local.rs (3)
crates/wallet/sdk/src/apis/key_manager.rs (1)
new(51-63)crates/wallet/sdk/src/local_key_store.rs (1)
new(27-37)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/template_test_tooling/src/support/stealth.rs (2)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)crates/engine_types/src/resource.rs (1)
view_key(126-128)
applications/tari_walletd/src/handlers/accounts.rs (1)
crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
transfer(315-659)
crates/engine/src/runtime/working_state.rs (5)
crates/engine_types/src/stealth/outputs.rs (1)
stmt(43-82)crates/engine_types/src/resource.rs (2)
view_key(126-128)new(54-81)crates/engine_types/src/crypto/messages.rs (1)
stealth_statement_metadata64(74-79)crates/template_lib/src/models/stealth.rs (1)
new(58-69)crates/engine_types/src/stealth/transfer.rs (2)
validate_ownership_proof(136-171)validate_transfer_balance(33-134)
crates/template_lib/src/models/stealth.rs (2)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/StealthInputsStatement.ts (1)
StealthInputsStatement(8-17)
crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/template_test_tooling/src/template_test.rs (1)
crates/template_lib/src/models/non_fungible.rs (3)
new(220-223)new(305-307)from_public_key(233-238)
crates/engine/src/executables/transaction.rs (2)
crates/engine/src/executables/mod.rs (1)
signers_iter(21-21)crates/template_test_tooling/src/wrapped_transaction.rs (1)
signers_iter(43-45)
crates/wallet/crypto/src/stealth.rs (2)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)crates/engine_types/src/crypto/messages.rs (1)
stealth_statement_metadata64(74-79)
crates/template_builtin/templates/faucet/src/lib.rs (5)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
transfer(315-659)bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)bindings/src/types/StealthInputsStatement.ts (1)
StealthInputsStatement(8-17)crates/template_lib/src/models/stealth.rs (1)
new_revealed_only(72-74)
crates/engine_types/src/hash.rs (2)
crates/template_lib_types/src/crypto/scalar.rs (1)
fmt(83-88)crates/tari_bor/src/lib.rs (2)
encode_into_std_writer(61-66)to_value(96-98)
crates/transaction/src/builder/mod.rs (4)
crates/template_lib/src/resource/builder/fungible.rs (1)
then(95-97)crates/template_lib/src/resource/builder/confidential.rs (1)
then(61-63)crates/template_lib/src/resource/builder/non_fungible.rs (1)
then(59-61)crates/template_lib/src/resource/builder/stealth.rs (1)
then(62-64)
crates/wallet/sdk/src/sdk.rs (3)
crates/wallet/sdk/src/apis/key_manager.rs (1)
new(51-63)crates/wallet/sdk/src/key_managers/local.rs (1)
new(24-26)crates/wallet/sdk/src/local_key_store.rs (1)
new(27-37)
crates/engine_types/src/stealth/transfer.rs (6)
bindings/src/types/StealthInput.ts (1)
StealthInput(8-18)bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)crates/engine_types/src/crypto/helpers.rs (1)
try_decode_to_signature(91-93)bindings/src/types/UtxoOutput.ts (1)
UtxoOutput(6-14)crates/engine_types/src/byte_types.rs (5)
convert_from_byte_type(30-31)convert_from_byte_type(62-64)convert_from_byte_type(79-81)convert_from_byte_type(101-105)convert_from_byte_type(124-129)crates/engine_types/src/crypto/messages.rs (1)
stealth_ownership64(59-72)
crates/template_lib_types/src/crypto/scalar.rs (1)
crates/template_lib_types/src/crypto/ristretto.rs (1)
is_zero(53-55)
crates/engine_types/src/crypto/messages.rs (1)
crates/engine_types/src/hashing.rs (1)
engine_hasher64(35-37)
crates/engine/tests/stealth.rs (3)
crates/template_lib/src/models/stealth.rs (1)
new(58-69)crates/template_test_tooling/src/template_test.rs (3)
new(94-96)None(95-95)owner_proof(404-406)crates/template_test_tooling/src/support/stealth.rs (2)
generate_mint_statement(41-67)generate_transfer_data(116-135)
crates/wallet/sdk/src/local_key_store.rs (3)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(51-63)crates/wallet/sdk/src/key_managers/backend.rs (1)
derive_secret(22-22)
crates/wallet/crypto/src/balance_proof.rs (4)
crates/engine_types/src/hash.rs (2)
default(147-149)zero(28-30)crates/template_lib_types/src/crypto/scalar.rs (1)
zero(24-26)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)crates/engine_types/src/crypto/messages.rs (1)
stealth_ownership64(59-72)
crates/engine/tests/signature.rs (2)
crates/template_test_tooling/src/support/stealth.rs (1)
generate_transfer_data(116-135)crates/template_test_tooling/src/template_test.rs (1)
owner_proof(404-406)
crates/wallet/sdk/src/apis/key_manager.rs (4)
crates/wallet/crypto/src/encryption.rs (1)
encrypt_with_password(100-134)crates/wallet/sdk/src/key_managers/local.rs (1)
new(24-26)crates/wallet/sdk/src/local_key_store.rs (1)
new(27-37)crates/wallet/sdk/src/models/key.rs (2)
derived(297-299)secret(158-160)
crates/wallet/sdk/src/apis/stealth_transfer.rs (5)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
key_manager_api(361-378)params(683-695)crates/wallet/sdk/src/sdk.rs (1)
key_manager_api(159-167)crates/wallet/sdk/src/models/key.rs (12)
key_id(65-67)key_id(137-139)key_id(162-164)derived(297-299)from(118-123)from(143-148)from(172-177)from(181-186)from(190-195)from(199-204)from(276-278)from(282-284)
crates/engine/src/runtime/error.rs (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check stable
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: clippy
- GitHub Check: machete
🔇 Additional comments (54)
crates/template_lib_types/src/crypto/scalar.rs (1)
28-30: LGTM! Correct and useful addition.The
is_zero()implementation is correct and idiomatic. It usesiter().all()which short-circuits on the first non-zero byte, making it efficient. This is a natural companion to the existingzero()constructor and will be useful in validation paths for the stealth transfer flows introduced in this PR.crates/template_lib/src/models/non_fungible.rs (1)
176-181: Zero-copy accessor looks good; confirm downstream impact.Returning
&[u8; 32]avoids unnecessary copies. It’s a public API change vs the prior owned array; please ensure all call sites (and any FFI/bindings) are updated accordingly.Also applies to: 178-178
crates/transaction/src/unsigned_transaction.rs (2)
132-137: Good separation of concerns withbuild_with_signatures.Centralizing signature injection here clarifies intent and keeps the wrapper simple.
139-141: Conveniencebuild()delegating tobuild_with_signaturesis fine.This preserves the old ergonomics. Please verify any prior
.build(signatures)call sites were migrated.crates/engine/src/executables/transaction.rs (1)
23-24: Derivingmain_signerfromsigners_iter()is cleaner.This removes conditional branching and standardizes ordering.
crates/transaction/src/builder/mod.rs (2)
69-72:then(Self -> Self)aligns with other builders.Consistent chaining API. Ensure external users expecting a generic return updated to use
map.
73-75:map(Self -> T)addition is helpful for Result-based flows.Enables error propagation and non-
Selftransforms cleanly.utilities/tariswap_test_bench/src/tariswap.rs (2)
149-157:map(...)?is the right migration here.This ensures the contextual signature is applied (or errors) before
build(). One check: you pass&primary_account_pkwhile signing with the account key—confirm the context is intended to be the primary account in this flow.
353-354: Addingwith_authorized_seal_signer()before build is correct.Matches the new required-signer model and prevents malleability in these flows.
applications/tari_walletd/src/handlers/transaction.rs (2)
330-341: LGTM! Builder refactoring aligns with new flow.The change from
thentomapindicates that the signing operation now returns a non-Resulttype directly, simplifying the builder chain. Error propagation is still maintained via the?operator after themapcall.
353-353: LGTM! More explicit method name.The rename from
buildtobuild_with_signaturesimproves API clarity by making it explicit that signatures are being provided to the build operation.crates/engine/tests/test.rs (1)
75-75: LGTM! Test utility method rename.The method name change from
get_test_public_key_bytes()toto_public_key_bytes()aligns with updated test utilities and maintains consistency with the broader refactoring.crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
48-75: LGTM! Required signer parameter properly propagated.The addition of the
required_signerparameter is correctly propagated through tostealth::create_transfer_statement, enabling the malleability fix that commits to a specific transaction signer at input-spend time.crates/wallet/crypto/src/balance_proof.rs (2)
57-67: LGTM! Signer-scoped ownership proof signature.The addition of
required_signerandmetadata_hashparameters enables signer-aware stealth ownership proofs, which is essential for the malleability fix. The parameters are correctly incorporated into thestealth_ownership64message construction.
38-55: Confirm public API exposureThe function’s visibility changed from
pub(crate)topub, exposing it beyond the crate. It’s only used internally (instealth.rsand tests). Ensure this wider API is intentional; if not, revert topub(crate).crates/p2p/proto/transaction.proto (1)
235-235: LGTM! Protobuf field addition for signer-scoped transfers.The
required_signerfield addition toStealthInputsStatementenables the malleability fix by carrying signer commitment information through the protocol layer. The field number assignment (3) is correct and follows the existing sequence.applications/tari_walletd/src/handlers/nfts.rs (1)
306-313: LGTM! Builder refactoring consistent with transaction handler.The change from
thentomapaligns with the broader builder flow refactoring, matching the pattern applied in the transaction handler. The signing operation and error handling remain correct.crates/template_lib_types/src/encrypted_data.rs (1)
26-28: LGTM! Convenient empty constructor.The
empty()constructor provides a clean way to create zero-lengthEncryptedDatainstances, properly delegating toMaxBytes::empty(). This is consistent with the pattern established in theMaxBytestype.crates/engine/src/runtime/error.rs (1)
48-48: LGTM! New error variant for signer enforcement.The
AccessDeniedStealthTransferSignererror variant provides clear, specific feedback when a stealth transfer is attempted without the required signer present. This supports the malleability fix by enforcing signer requirements at runtime.Also applies to: 177-178
crates/engine/src/executables/mod.rs (1)
21-21: LGTM! Clean iterator-based API addition.The new
signers_itermethod provides a more flexible way to access all signers compared tomain_signer. The trait method signature follows Rust conventions withimpl Iterator, and the implementation inTransactioncorrectly chains the seal signer (when authorized) with other signatures.crates/engine_types/src/lib.rs (2)
35-35: LGTM! Standard module organization.Adding the private
hashmodule follows Rust conventions for organizing related functionality.
43-43: LGTM! Proper re-export pattern.The public re-export of hash module contents makes Hash64 and related types available to downstream code through the engine_types crate facade.
crates/engine/tests/signature.rs (6)
79-86: LGTM! Test updated for required signer flow.The addition of
test.to_public_key_bytes()as therequired_signerparameter correctly aligns with the PR's objective to commit to a specific transaction signer at input-spend time.
91-91: LGTM! Ownership proof expectation updated correctly.The change from
vec![]tovec![test.owner_proof()]properly reflects the new ownership proof requirements in the signer-scoped transfer flow.
113-114: LGTM! Multi-claim test updated consistently.Both transfer data generation calls now include the required signer parameter, maintaining consistency with the single-claim test pattern.
122-122: LGTM! Ownership proof updated for multi-claim.
132-132: LGTM! Bad signature test updated correctly.The test maintains its original intent (verifying signature validation failure) while adapting to the new required signer parameter.
138-138: LGTM! Ownership proof updated for bad signature test.applications/tari_app_utilities/src/transaction_executor.rs (2)
8-8: LGTM! Import added for new API usage.The
Executabletrait import enables access to the newsigners_itermethod used in the refactored code below.
123-126: LGTM! Cleaner signer iteration approach.The refactored code delegates signer iteration logic to
transaction.signers_iter(), removing duplicate seal signer authorization checks and simplifying the auth scope construction. This improves maintainability by centralizing signer logic in the Transaction type.crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
32-32: LGTM! Import added for new field type.The
RistrettoPublicKeyBytesimport supports the newrequired_signerfield added toTransferStatementParams.
766-766: Breaking change: new required field added to TransferStatementParams.The addition of
required_signer: RistrettoPublicKeyBytesis a breaking change for any code constructingTransferStatementParams. This aligns with the PR's stated breaking changes requiring data directory deletion.
715-715: LGTM! Required signer properly propagated.The
params.required_signeris correctly passed togenerate_transfer_statement, enabling signer-scoped stealth transfers as intended by the PR.crates/template_test_tooling/src/wrapped_transaction.rs (1)
43-45: LGTM! Proper trait implementation via delegation.The
signers_iterimplementation correctly delegates to the underlying transaction's method, maintaining consistency with other delegated methods in this wrapper.crates/template_lib_types/src/max_bytes.rs (2)
39-41: LGTM! New empty constructor added.The
empty()constructor provides a clear way to create an emptyMaxBytesinstance, improving API clarity.
59-59: Verify no reliance on old Default behavior
No directMaxBytes::default()usages found; manually audit any implicitDefault::default()initializations ofMaxBytesto ensure no code assumes a non-empty or zero‐filled buffer.crates/wallet/sdk/src/key_managers/backend.rs (2)
6-6: LGTM! Import updated for new trait method.The
DerivedKeyIndeximport is needed for the newderive_secretmethod signature.
22-22: Breaking change: new required method on WalletKeyStore trait.The addition of
derive_secretrequires all trait implementors to provide an implementation. This aligns with the PR's refactoring of the key manager to use the wallet key store backend.applications/tari_walletd/src/handlers/validator.rs (1)
182-199: Signing flow/error propagation LGTM; confirm context semantics.Result-based chaining and dual-sign path are correct. Please confirm
sign_with_context(..., builder.with_authorized_seal_signer())both:
- binds the claim key as an authorized seal signer, and
- leaves the builder ready for the final account-key sign at Line 201.
Optional: add a debug log when the dual-sign path is taken for observability.
crates/wallet/crypto/src/stealth.rs (1)
59-61: Good: bind ownership proofs to outputs metadata.Hashing outputs once via
stealth_statement_metadata64and passing it into owner-proof signing is correct and reduces malleability.crates/p2p/src/conversions/transaction.rs (1)
813-818: Serialization and proto schema verified
required_signeris defined inStealthInputsStatementincrates/p2p/proto/transaction.proto:235. Ensure network/proto versioning gates handle older nodes missing this field.applications/tari_walletd/src/handlers/accounts.rs (3)
537-545: Signer binding for claim burn is correctYou commit the required_signer (nonce key) into the transfer statement and sign the tx with the same key. This enforces signer presence at execution time without leaking the account key. LGTM.
Also applies to: 564-565
962-967: Good: explicit guard for owner_key_id presenceClear error path for view-only accounts before building the transfer. LGTM.
990-995: Signer carried through from builder to signatureUsing authorized_sealed_signer().build() and then signing with transfer.signing_key_branch/id keeps the tx signer aligned with the statement’s required_signer. LGTM.
crates/engine/tests/stealth.rs (1)
630-671: Great negative test for signer enforcementAsserting AccessDeniedStealthTransferSigner when the tx is signed by the wrong key exercises the new policy well. LGTM.
crates/wallet/sdk/src/sdk.rs (1)
48-48: Key store integration looks correctLocalKeyStore is consistently wired into KeyManagerApi and LocalSignerApi, and passed into StealthTransferApi. This aligns with the backend switch. LGTM.
Also applies to: 158-167, 171-175, 225-233
crates/template_test_tooling/src/template_test.rs (3)
181-182: Sensible default: auto-add proofs from signersDefaulting
auto_add_proofs_from_signersto true in tests matches the new signer requirement and reduces boilerplate. LGTM.
228-236: Clear toggles for signer-proof auto-injectionEnable/disable methods are straightforward and useful for test control. LGTM.
416-418: Rename toto_public_key_byteslooks goodCompact helper to get bytes; consistent with other usages. LGTM.
crates/engine/src/runtime/working_state.rs (1)
1561-1562: New flow adoptionSwitch to
validate_and_spend_stealth_utxosintegrates signer and metadata validation cleanly. LGTM.crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
508-519: Threadingrequired_signerinto both fee and transfer statementsGood: both statements commit to the same
required_signer, aligning with the new engine requirement. After applying the fix above, this remains consistent.Also applies to: 592-609
crates/wallet/sdk/src/key_managers/local.rs (1)
36-51: Key derivation/signing flow looks correctDeriving secrets via
key_storefor both Derived and Imported keys and signing withRistrettoSchnorris sound. LGTM post-OsRng fix.crates/template_test_tooling/src/support/stealth.rs (1)
45-66: Required signer propagation across helpersAdding
required_signerto mint/transfer data and threading it tocreate_transfer_statementaligns tests with the new engine checks. Looks consistent.Also applies to: 116-135, 137-153, 166-173, 217-224
crates/engine_types/src/hash.rs (1)
11-14: serde_with::hex emits raw bytes for non-human-readable serializers
The localserializeusesSerializer::serialize_byteswhenis_human_readable()is false, so any non-human-readable serde format (including your BOR-via-serde implementation) will emit raw bytes as expected.
Test Results (CI)475 tests +3 464 ✅ +4 1h 35m 28s ⏱️ -43s For more details on these failures, see this check. Results for commit 788b18a. ± Comparison against base commit 53bf250. This pull request removes 5 and adds 8 tests. Note that renamed tests count towards both. |
5338d47 to
ddca5a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
237-241: Consider removing redundant console.error.The error is already being captured in the formError state and displayed to the user. The console.error on line 241 is redundant since the error information is surfaced via the UI.
Apply this diff to remove the redundant logging:
setFormError({ type: "general", message: `Failed to estimate fee: ${error}`, }); - console.error("Fee estimation failed:", error); return;crates/engine_types/src/hash.rs (1)
16-86: Docstrings still describe a 32-byte hashAll of these docs call out 32-byte lengths, but
Hash64::LENGTHis 64. This mismatch will trip up anyone relying on the docs. Please update the wording to reflect the actual size (or drop the hard-coded number and just referenceSelf::LENGTH) so the documentation stays truthful.-/// Representation of a 32-byte hash value +/// Representation of a 64-byte hash value @@ - /// Panics if `N` is greater than Self::LENGTH (32) + /// Panics if `N` is greater than `Self::LENGTH`. @@ - /// Panics if `N` is greater than Self::LENGTH (32) + /// Panics if `N` is greater than `Self::LENGTH`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx(6 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx(8 hunks)bindings/package.json(1 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/Hash64.ts(1 hunks)bindings/src/types/StealthInputsStatement.ts(2 hunks)crates/engine_types/src/hash.rs(1 hunks)crates/state_store_rocksdb/src/codecs/small_bytes.rs(2 hunks)crates/state_store_rocksdb/src/key.rs(0 hunks)crates/template_lib/src/models/non_fungible.rs(3 hunks)crates/template_lib/src/models/stealth.rs(2 hunks)crates/template_lib_types/src/crypto/scalar.rs(3 hunks)crates/template_lib_types/src/entity_id.rs(4 hunks)crates/template_lib_types/src/hash.rs(3 hunks)crates/template_lib_types/src/hex.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(11 hunks)
💤 Files with no reviewable changes (1)
- crates/state_store_rocksdb/src/key.rs
✅ Files skipped from review due to trivial changes (1)
- crates/template_lib_types/src/entity_id.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/template_lib/src/models/stealth.rs
🧰 Additional context used
🧬 Code graph analysis (8)
crates/template_lib_types/src/hash.rs (1)
crates/template_lib_types/src/hex.rs (2)
fixed_bytes_from_hex(8-18)write_hex_fmt(39-44)
crates/state_store_rocksdb/src/codecs/small_bytes.rs (1)
crates/template_lib_types/src/hex.rs (1)
write_hex_fmt(39-44)
crates/template_lib_types/src/crypto/scalar.rs (2)
crates/template_lib_types/src/hex.rs (1)
write_hex_fmt(39-44)crates/template_lib_types/src/crypto/ristretto.rs (1)
is_zero(53-55)
bindings/src/types/StealthInputsStatement.ts (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx (1)
FormError(67-70)
crates/template_lib/src/models/non_fungible.rs (1)
crates/template_lib_types/src/hex.rs (1)
write_hex_fmt(39-44)
crates/engine_types/src/hash.rs (4)
crates/template_lib_types/src/hash.rs (17)
fmt(158-160)fmt(171-173)write_hex_fmt(67-69)from_array(50-52)into_array(54-56)as_slice(58-60)from_hex(62-65)try_from_slice(71-73)try_from(125-132)try_from(138-140)leading_bytes(80-86)trailing_bytes(93-99)as_ref(103-105)from(109-111)deref(146-148)deref_mut(152-154)serialize_deserialize(181-187)crates/template_lib_types/src/hex.rs (1)
write_hex_fmt(39-44)bindings/src/types/Hash64.ts (1)
Hash64(6-6)crates/tari_bor/src/lib.rs (2)
encode_into_std_writer(61-66)to_value(96-98)
crates/wallet/sdk/src/apis/stealth_transfer.rs (6)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
key_manager_api(361-378)crates/wallet/sdk/src/sdk.rs (1)
key_manager_api(159-167)crates/wallet/sdk/src/models/account.rs (2)
owner_key_id(33-35)owner_key_id(91-93)crates/wallet/sdk/src/models/key.rs (9)
derived(297-299)from(118-123)from(143-148)from(172-177)from(181-186)from(190-195)from(199-204)from(276-278)from(282-284)
⏰ 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
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (25)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (3)
69-69: LGTM!The formError state initialization is correct and follows React best practices.
124-174: Good UX pattern for error clearing.Clearing formError on user interactions (form field changes and badge toggle) provides a smooth user experience by dismissing error messages when the user attempts to correct the issue.
325-325: LGTM!The formError prop is correctly passed to FormStep, enabling error display in the form UI.
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx (3)
67-70: LGTM!The FormError type is well-structured with a discriminated union on the
typefield, enabling type-safe error handling for different form fields.
162-262: Good error display positioning.The DisplayFormError component is strategically placed before the address input, amount input, and general action buttons, ensuring errors appear near the relevant UI elements for better UX.
276-284: LGTM!The DisplayFormError helper component is simple and effective. It conditionally renders error messages based on type matching, providing clean separation of concerns.
bindings/src/types/StealthInputsStatement.ts (2)
3-3: LGTM: Import correctly added.The import for
RistrettoPublicKeyBytesis properly added to support the newrequired_signerfield.
18-21: All construction sites correctly include the required_signer field.Verification of the Rust codebase confirms that all places constructing
StealthInputsStatementproperly include therequired_signerfield:
crates/wallet/crypto/src/stealth.rs(lines 94–98, 108–112): Direct struct literal construction withrequired_signercrates/template_builtin/templates/faucet/src/lib.rs(line 32): Factory methodnew_revealed_only()which requiresrequired_signeras a parameter- Factory methods
new()andnew_revealed_only()enforcerequired_signerthrough their function signatures- TypeScript bindings correctly reflect the required field
- Protobuf schema and conversions properly handle the field
The security fix is complete and consistently applied.
bindings/package.json (1)
3-3: LGTM!The version bump from 1.18.0 to 1.18.1 appropriately reflects the addition of the new Hash64 type to the public API surface.
crates/template_lib/src/models/non_fungible.rs (2)
176-181: LGTM!Returning a borrowed reference instead of copying the 32-byte array is a good optimization. This avoids an unnecessary allocation and aligns with Rust's borrowing best practices.
289-289: LGTM!The refactoring to use the centralized
write_hex_fmthelper ensures consistent hex formatting across the codebase.crates/state_store_rocksdb/src/codecs/small_bytes.rs (1)
119-119: LGTM!Using the centralized
write_hex_fmthelper eliminates code duplication and ensures consistent hex formatting behavior.bindings/src/index.ts (1)
54-54: LGTM!The Hash64 export is correctly positioned alphabetically and makes the new type available to consumers of the bindings package.
crates/template_lib_types/src/hex.rs (2)
41-41: LGTM!Removing the debug flag
?from the format string is correct. The debug flag was unnecessary and could have produced unexpected output. The format{:02x}correctly produces lowercase hex with zero-padding.
46-56: LGTM!The new test verifies that byte sequences are correctly formatted as hex strings, ensuring the
bytes_to_hexfunction works as expected.crates/template_lib_types/src/hash.rs (3)
31-34: LGTM!The updated imports correctly bring in the centralized
write_hex_fmtutility for hex formatting.
68-68: LGTM!Delegating to the centralized
write_hex_fmthelper eliminates code duplication while maintaining the same formatting behavior.
159-159: LGTM!The Display implementation now uses the centralized hex formatting utility, ensuring consistent behavior across the codebase.
crates/template_lib_types/src/crypto/scalar.rs (2)
28-30: LGTM!The new
is_zero()method is a useful utility that efficiently checks whether the scalar is the zero value. The implementation is consistent with similar methods in the codebase (e.g.,RistrettoPublicKeyBytes::is_zero()).
84-84: LGTM!The Display implementation now uses the centralized
write_hex_fmthelper, ensuring consistent hex formatting across the codebase.crates/wallet/sdk/src/apis/stealth_transfer.rs (5)
491-508: Past critical issue has been resolved.The double-derivation issue flagged in the previous review (calling
next_derived_key_indextwice) no longer exists in the current code. The nonce key is now derived exactly once at line 501, and the resultingsigning_key_idis consistently used for both:
- Computing
required_signer(lines 504-507)- Returning in
TransferOutput(line 638)The design is correct:
required_signeris the transaction-level signer (committed in statements), whilespend_key(always account key) proves UTXO ownership. When no revealed funds are involved, using a nonce key forrequired_signerpreserves privacy while still spending UTXOs with the account key.
510-521: LGTM: Fee statement correctly uses account key for spending and required_signer for transaction authorization.The parameters are correctly set:
spend_key_branch: KeyBranch::Accountandspend_key_id: owner_key_idprove UTXO ownershiprequired_signer: required_signer_pkenforces transaction-level authorization (can be nonce key for privacy)
581-598: LGTM: Transfer statement follows the same correct pattern.Consistent with the fee statement: account key for UTXO spending, required_signer for transaction authorization.
637-639: LGTM: Signing key fields properly returned for subsequent transaction signing.The
signing_key_branchandsigning_key_idare correctly propagated to the caller, enabling them to sign the transaction with the same key committed asrequired_signerin the statements. This completes the signer-scoped stealth transfer flow.Also applies to: 757-759
883-884: LGTM: Error propagation correctly added.The
KeyManagerApierror variant enables proper error propagation from key manager operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx (1)
275-283: LGTM! Consider optional accessibility enhancement.The
DisplayFormErrorhelper component has correct conditional rendering logic and appropriate styling. The early returns prevent unnecessary renders.Optional enhancement: Consider adding ARIA attributes for better accessibility:
function DisplayFormError({ forType, formError }: { forType: FormError["type"]; formError?: FormError | null }) { if (!formError) return null; if (formError.type !== forType) return null; return ( - <Typography color="error" sx={{ mb: 2 }}> + <Typography color="error" sx={{ mb: 2 }} role="alert" aria-live="polite"> {formError.message} </Typography> ); }crates/engine_types/src/hash.rs (1)
95-99: ReplaceSelf::LENGTHwith literal64in trait impl for clarity.Using
Self::LENGTHin the trait implementation header is non-standard. While it may compile in recent Rust versions, the idiomatic approach is to use the literal value for better clarity and compatibility.Apply this diff:
-impl From<[u8; Self::LENGTH]> for Hash64 { - fn from(hash: [u8; Self::LENGTH]) -> Self { +impl From<[u8; 64]> for Hash64 { + fn from(hash: [u8; 64]) -> Self { Self::from_array(hash) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx(7 hunks)crates/engine_types/src/hash.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(4 hunks)crates/wallet/sdk/src/storage.rs(1 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(4 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(2 hunks)crates/wallet/storage_sqlite/src/reader.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
crates/wallet/storage_sqlite/src/reader.rs (2)
crates/wallet/sdk/src/storage.rs (2)
stealth_outputs_count_by_status(235-240)general(131-136)crates/wallet/storage_sqlite/src/writer.rs (14)
stealth_outputs(1065-1081)stealth_outputs(1152-1152)stealth_outputs(1153-1153)stealth_outputs(1182-1182)stealth_outputs(1183-1183)stealth_outputs(1195-1195)stealth_outputs(1196-1196)stealth_outputs(1215-1215)stealth_outputs(1216-1216)accounts(590-594)accounts(632-635)accounts(897-900)accounts(960-963)accounts(1059-1062)
crates/engine_types/src/hash.rs (3)
crates/template_lib_types/src/hash.rs (18)
fmt(158-160)fmt(171-173)write_hex_fmt(67-69)from_array(50-52)into_array(54-56)as_slice(58-60)from_hex(62-65)try_from_slice(71-73)try_from(125-132)try_from(138-140)leading_bytes(80-86)trailing_bytes(93-99)as_ref(103-105)from(109-111)from_str(117-119)deref(146-148)deref_mut(152-154)serialize_deserialize(181-187)crates/template_lib_types/src/hex.rs (1)
write_hex_fmt(39-44)crates/tari_bor/src/lib.rs (2)
encode_into_std_writer(61-66)to_value(96-98)
crates/wallet/sdk/src/storage.rs (4)
crates/wallet/storage_sqlite/src/reader.rs (1)
stealth_outputs_count_by_status(861-886)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/OutputStatus.ts (1)
OutputStatus(3-3)
crates/wallet/sdk/src/apis/stealth_outputs.rs (6)
bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)crates/wallet/sdk/src/apis/accounts.rs (1)
count(167-171)bindings/src/types/OutputStatus.ts (1)
OutputStatus(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: file licenses
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: machete
🔇 Additional comments (17)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
52-52: Approve rename with breaking change notice
Grammar improved with a consistent rename; no references to the old names remain. However, this is a breaking change to the public API—ensure all external callers are updated and bump the crate’s major version.applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx (4)
67-70: LGTM! Clean error type definition.The
FormErrortype is well-structured with a discriminated union for error categories and a message field. This provides good type safety for error handling across the form.
58-58: LGTM! Proper prop integration.The
formErrorprop is correctly added as optional toFormStepPropsand properly destructured in the component. This maintains backward compatibility while enabling error display functionality.Also applies to: 83-83
161-161: Verify handling of "fee" error type.The
DisplayFormErrorcomponent is used for "address", "amount", and "general" error types, but theFormErrortype also includes "fee". Since the fee field is disabled (line 251), fee errors may not be expected in this component.Please confirm whether:
- Fee errors should be displayed somewhere (e.g., near the fee input at lines 239-257)
- The "fee" error type is intended for use in a different context or component
If fee errors should be displayed here, add:
// Around line 238, before the fee TextField <DisplayFormError forType="fee" formError={formError} />Also applies to: 213-213, 261-261
120-120: Past review comment has been addressed.The debugging
console.log(formError)statement mentioned in the previous review has been successfully removed. This line now contains only the return statement.crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
112-128: Good call on guarding the stream with local stateDeriving
unspent_countfirst and only asking the network for spent entries when we actually have local unspent records keeps the sync lean without sacrificing correctness. ✅crates/wallet/storage_sqlite/src/reader.rs (1)
861-886: Count accessor matches existing query patternsThe new count implementation reuses the established subselect style and keeps error handling consistent, so the API can rely on it without surprises.
crates/wallet/sdk/src/storage.rs (1)
235-241: Trait surface cleanly extendedNice to see the reader trait stay in sync with the SQLite implementation; downstream callers get the count helper without extra boilerplate.
crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
303-312: Lightweight count helper fits the read pathLeveraging the store reader directly keeps the API thin while giving callers the unspent count they need for optimisations like the scanner tweak.
721-778: Required signer threading looks solidPropagating
required_signerthroughTransferStatementParamsand intogenerate_transfer_statementlines up with the crypto changes and keeps the balancing checks untouched.crates/engine_types/src/hash.rs (7)
4-14: LGTM!The imports are appropriate and all are utilized in the implementation.
17-17: Consider addingborsh::BorshDeserializeif bidirectional borsh serialization is needed.The struct derives
borsh::BorshSerializebut notborsh::BorshDeserialize. If Hash64 needs to be deserialized from borsh-encoded data, add the missing derive.Apply this diff if bidirectional borsh support is required:
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Hash, Serialize, Deserialize, borsh::BorshSerialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Hash, Serialize, Deserialize, borsh::BorshSerialize, borsh::BorshDeserialize)]
26-43: LGTM!The constructors and accessors are correctly implemented as
constmethods where appropriate.
45-52: LGTM!The hex parsing correctly validates the input length before decoding.
54-60: LGTM!The hex formatting correctly delegates to the shared
write_hex_fmthelper, andtry_from_sliceappropriately delegates to theTryFromimplementation.
101-154: LGTM!The trait implementations are correct and follow Rust best practices. The
Displayimplementation correctly delegates to the shared hex formatting helper.
156-178: LGTM!The error type is well-defined with
thiserror, and the test comprehensively validates both serialization correctness and the expected BOR value representation.
eb10847 to
13d2a28
Compare
2 participants
Description
Motivation and Context
How Has This Been Tested?
New unit tests
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests