fix(wallet/sdk)!: split transfer and transaction signing - #1597
Conversation
WalkthroughRefactors stealth/confidential cryptography to use Witness types, updates balance-proof message domains and signatures, introduces unsigned stealth transfer construction, adjusts SDK APIs (keys, stealth/confidential, transactions), adds account resource association, modifies DB schema for minimum_value_promise, revises UI fee estimations, and changes transaction pre-checks to require inputs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI/Client
participant SDK as Wallet SDK
participant SO as StealthOutputsApi
participant SC as StealthCryptoApi
participant ST as StealthTransferApi
participant TX as TransactionApi
participant NW as Network
UI->>SDK: request stealth transfer (params)
SDK->>SO: resolve_input_masks(inputs)
SO-->>SDK: InputSpendData[]
SDK->>SO: generate_transfer_statement(TransferStatementParams)
SO-->>SDK: StealthTransferStatement
SDK->>ST: generate_transfer_transaction(statement) [unsigned]
ST-->>SDK: UnsignedTransaction + locks
Note over SDK,ST: Locks managed via release_lock/finalize_lock
SDK->>TX: insert_new_transaction(unsigned_tx)
TX-->>SDK: transaction_id
SDK->>NW: submit_transaction(signed_tx)
alt success
NW-->>SDK: ok
SDK->>SO: finalize_lock(lock_id)
else error
NW-->>SDK: err
SDK->>SO: release_lock(lock_id)
end
sequenceDiagram
autonumber
actor WD as Wallet Daemon
participant AM as AccountMonitorHandle
participant MON as AccountMonitor
participant ACC as Accounts API
participant IDX as Resource Cache
WD->>AM: associate_resource(account, resource)
AM->>MON: AssociateResource(account, resource)
MON->>IDX: fetch_and_cache_resource(resource)
IDX-->>MON: resource (cached)
MON->>ACC: associate_resource(account, resource)
ACC-->>MON: ok
MON-->>AM: ok
AM-->>WD: ok
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
applications/tari_validator_node/src/transaction_validators/is_shard_applicable.rs (1)
11-14: RenameIsShardApplicableto match its input-only check
Update the docstring and struct name to reflect that only inputs are validated:-/// Refuse to process the transaction if it does not apply to any shard (i.e. does not have any inputs or claim burn -/// tombstones). +/// Refuse to process the transaction if it does not have any inputs. #[derive(Debug, Clone, Default)] -pub struct IsShardApplicable; +pub struct HasInputsValidator; impl Validator<Transaction> for HasInputsValidator { // ... }applications/tari_walletd/src/handlers/confidential.rs (2)
149-156: Change output encryption likely uses the wrong owner secretYou encrypt the change using
&change_mask.keyas the owner secret. Change outputs should be decryptable by the wallet’s owner key. This looks inconsistent with the main output (uses&account_key.secret) and may render change outputs unviewable.Apply:
- let encrypted_data = sdk.confidential_crypto_api().encrypt_value_and_mask( + let encrypted_data = sdk.confidential_crypto_api().encrypt_value_and_mask( change_amount_u64, &change_mask.key, &public_nonce, - &change_mask.key, + &account_key.secret, None, )?;Please confirm the intended decryption key for change; if change should be owned by the account, use the account owner secret consistently.
86-89: Prevent locked outputs from remaining stuck on error (add RAII unlock guard)If any step after locking fails, inputs remain locked. Add a scope guard to auto-release on early returns.
Apply imports:
use std::fs; +use scopeguard::{guard, ScopeGuard};Create the guard right after creating the lock:
- let lock_id = sdk.confidential_outputs_api().create_lock()?; + let lock_id = sdk.confidential_outputs_api().create_lock()?; + let unlock_guard = guard(lock_id, |id| { + let _ = sdk.confidential_outputs_api().release_revealed_funds(id); + let _ = sdk.confidential_outputs_api().release_locked_outputs(id); + });Dismiss on success before returning:
- Ok(ProofsGenerateResponse { + // Success: prevent the guard from releasing + let _ = ScopeGuard::into_inner(unlock_guard); + Ok(ProofsGenerateResponse { proof_id: lock_id, proof, })Note: add
scopeguard = "1"to Cargo.toml.Also applies to: 65-66, 198-202
crates/wallet/sdk/src/models/stealth_output.rs (1)
13-33: Include minimum_value_promise in all initializers
storage_sqlite’stry_convertsetsminimum_value_promisecorrectly, but incrates/wallet/sdk/src/apis/stealth_transfer.rs(around line 693) andcrates/wallet/sdk/src/apis/stealth_outputs.rs(around line 579) theStealthOutputModelliterals omitminimum_value_promise. Also verify TS/FFI bindings and DB migrations add the new column.crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
253-261: BUG: New lock_id is created and shadows the function parameter, leaking locks and breaking finalize/release.PreferConfidential branch calls create_lock(), shadowing the passed lock_id. The newly locked outputs are not associated with the caller’s lock_id and won’t be finalized/released correctly.
Apply this diff to use the caller’s lock_id and prevent leaks:
- let lock_id = self.outputs_api.create_lock()?; - let (inputs, blinded_amount_locked) = self.outputs_api.lock_outputs_until_partial_amount( + let (inputs, blinded_amount_locked) = self.outputs_api.lock_outputs_until_partial_amount( owner_account.component_address(), &resource_address, spend_amount, lock_id, )?;No other changes needed; subsequent release_lock calls will now use the correct lock_id (the function parameter).
🧹 Nitpick comments (18)
crates/wallet/sdk/src/models/key.rs (1)
95-97: LGTM! Consider returning by value for consistency.The accessor correctly exposes the
key_idfield for external usage, aligning with the PR's goals. However, sinceKeyIdimplementsCopy(line 220), idiomatic Rust typically returns Copy types by value rather than by reference. Additionally,WalletKeyRecord::key_id()at line 22-24 returnsKeyIdby value, creating a minor inconsistency.Consider this diff for consistency and idiomaticity:
- pub fn key_id(&self) -> &KeyId { - &self.key_id + pub fn key_id(&self) -> KeyId { + self.key_id }applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
205-209: Conditional fee buffer logic looks reasonable, but track the TODO.The conditional fee buffer (applied only for Confidential resource types) is more thoughtful than the blanket buffer removal in other files. The TODO comment indicates awareness that this is a workaround for variable bullet proof sizes.
However, ensure that:
- The 100-unit buffer is adequate for all confidential transaction scenarios
- The TODO is tracked in an issue to implement a more robust solution
Do you want me to open a tracking issue for the TODO to implement proper variable-size fee estimation for confidential transactions?
crates/wallet/sdk/src/apis/key_manager.rs (1)
283-297: Consider renaming to reflect side effects.The method
next_derived_key_indexboth advances the index counter in storage (line 294) and returns the new value, but the name suggests it only retrieves. Consider a name likeallocate_next_key_indexoradvance_and_get_next_indexto better convey that it mutates state.The logic itself is correct: the write transaction ensures atomicity, and
insert_or_ignoreprovides idempotency safeguards.Apply this diff to clarify the method's purpose:
- pub fn next_derived_key_index(&self, branch: KeyBranch) -> Result<DerivedKeyIndex, KeyManagerApiError> { + pub fn allocate_next_key_index(&self, branch: KeyBranch) -> Result<DerivedKeyIndex, KeyManagerApiError> { let mut tx = self.store.create_write_tx()?; let next_index = tx .key_manager_get_last_index(branch.as_str())And update the call site at line 278:
pub fn next_key(&self, branch: KeyBranch) -> Result<DerivedWalletKey, KeyManagerApiError> { - let next_key_id = self.next_derived_key_index(branch)?; + let next_key_id = self.allocate_next_key_index(branch)?; let key = self.derive_key(branch, next_key_id)?;crates/wallet/sdk/src/lib.rs (1)
20-20: Consider listing explicit re-exports instead of wildcard.The wildcard re-export
pub use tari_ootle_address::*;makes all public items from that crate available through this SDK. While convenient, it can introduce unexpected items into the public API and makes it harder to track what's actually being exposed. Consider explicitly listing the types you want to re-export for better API clarity and maintainability.Apply this pattern if you want more explicit control:
-pub use tari_ootle_address::*; +pub use tari_ootle_address::{ + // List specific types to re-export, e.g.: + // OotleAddress, AddressBuilder, etc. +};crates/wallet/storage_sqlite/src/models/stealth_output.rs (1)
38-38: LGTM with database integrity assumption.The addition of
minimum_value_promisefollows the existing pattern for numeric fields. Line 85 castsi64tou64, which assumes the database value is non-negative. While a negative value would indicate database corruption (an unlikely scenario given the insertion code ensures non-negative values), you might consider adding an explicit check if you want defense-in-depth:minimum_value_promise: self.minimum_value_promise.try_into().map_err(|_| { WalletStorageError::DecodingError { operation: "try_into_output", item: "minimum_value_promise", details: format!("Corrupt db: negative minimum_value_promise {}", self.minimum_value_promise), } })?,Also applies to: 85-85
applications/tari_validator_node/src/transaction_validators/signature.rs (1)
20-25: Replacetransaction.to_id()withtransaction.calculate_id().In applications/tari_validator_node/src/transaction_validators/signature.rs (line 23), use
calculate_id()to align with the rest of the codebase.applications/tari_validator_node/src/p2p/services/mempool/service.rs (1)
239-244: Consider error variant naming consistency.The check
has_inputs()now triggersTransactionValidationError::NoInvolvedShards. While the change aligns with the broader refactor, the error name "NoInvolvedShards" may be less precise than "NoInputs" for describing the actual condition being checked.Consider whether using a
NoInputsvariant (which exists at line 34 in error.rs) would be more semantically accurate thanNoInvolvedShardsfor this check, or clarify the relationship between having no inputs and having no involved shards in the error documentation.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (2)
235-256: Avoid editing the initial migration; add a new migration for schema changesChanging 2023-02-08-122514_initial/up.sql breaks upgrade paths for existing nodes. Even with the “delete data dir” note, keeping migration history append-only avoids confusion and accidental mixed states.
- Create a new migration that adds
minimum_value_promise BIGINT NOT NULLtostealth_outputs.- Leave the original initial migration untouched for clean history.
Please confirm whether operators actually need to wipe data. If not strictly required, ship a forward migration instead of modifying the initial file.
258-260: Add a composite index including account_id for common queriesCurrent index is on
(resource_address, status). Wallet lookups typically filter by account + resource + status. Consider:
(owner_account_id, status)or(owner_account_id, resource_address, status)as a covering index.Apply this addition:
CREATE UNIQUE INDEX stealth_outputs_uniq_resource_addr_commitment ON stealth_outputs (resource_address, commitment); CREATE INDEX stealth_outputs_idx_resource_status ON stealth_outputs (resource_address, status); +CREATE INDEX stealth_outputs_idx_account_resource_status + ON stealth_outputs (owner_account_id, resource_address, status);crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
137-156: AssociateResource flow looks good; trigger an immediate UTXO scan for the new resourceAfter successfully associating, kick off a scan so balances update without waiting for the next periodic cycle.
Apply after association:
async fn associate_resource_with_account( &self, account_address: &ComponentAddress, resource_address: ResourceAddress, ) -> Result<(), AccountMonitorError> { let accounts_api = self.wallet_sdk.accounts_api(); self.fetch_and_cache_resource(&resource_address).await?; accounts_api.associate_stealth_resource(account_address, resource_address)?; + // Optional: proactively scan the newly associated resource + self.utxo_scanner_handle.request_scan(*account_address, resource_address); Ok(()) }Confirm
request_scanexpects owned addresses (it appears to), and that this aligns with your UX expectations. Based on learnings (tari_template_lib prelude usage best practices).Also applies to: 158-166
applications/tari_walletd/src/handlers/confidential.rs (1)
256-264: Output proof lacks resource view key; extend request to include resource address
resource_view_key: Noneis a TODO. Without it, verifiers lacking the resource view key cannot validate commitments consistently.
- Add
resource_addresstoConfidentialCreateOutputProofRequest.- Fetch and pass the view key as in transfer proof:
let vk = sdk.substate_api().fetch_resource(resource_address).await?.to_view_key_public_key()?;- Set
resource_view_key: Some(vk).I can submit a patch for the request/handler and TypeScript bindings if desired.
crates/transaction/src/transaction.rs (1)
179-181: Provide a deprecated alias for has_inputs()
Add a temporary deprecated alias foris_shard_applicable()to preserve upstream compatibility (internal search shows no remaining calls):pub fn has_inputs(&self) -> bool { !self.inputs().is_empty() } #[deprecated(note = "Use has_inputs(). This will be removed in a future release.")] pub fn is_shard_applicable(&self) -> bool { self.has_inputs() }applications/tari_walletd/src/handlers/accounts.rs (1)
995-1002: Redundant lock release is safe but could be clearer.The dry-run flow releases the lock immediately at line 996, then attempts to release it again on error at line 1009. The second release will only succeed if the first failed, making it a safe but redundant fallback.
Consider tracking whether the first release succeeded to avoid the redundant attempt, or add a comment explaining the fallback pattern:
// Release the lock immediately as dry run does not submit the transaction // If release fails here, we'll attempt again on error if let Err(err) = sdk.stealth_outputs_api().release_lock(transfer.lock_id) { error!(/* ... */); }Also applies to: 1009-1009
crates/wallet/crypto/src/balance_proof.rs (2)
4-4: Unify RNG import to avoid rand_core trait mismatches.Using OsRng re-exported from other crates can lead to trait conflicts. Prefer rand_core::OsRng across the workspace.
Apply this diff:
- use chacha20poly1305::aead::OsRng; + use rand_core::OsRng;
34-35: Avoid bare unwraps on signature generation.Replace unwrap with expect(...) for debugging context or handle errors if feasible.
Example:
- let sig = EngineSchnorrSignature::sign_raw_uniform(&secret_excess, nonce, &message).unwrap(); + let sig = EngineSchnorrSignature::sign_raw_uniform(&secret_excess, nonce, &message) + .expect("sign_raw_uniform failed for confidential balance proof");Similarly for other unwraps in this file.
Also applies to: 49-50, 59-61
crates/wallet/crypto/src/stealth.rs (1)
81-86: Eliminate unnecessary clones and duplicate statement construction.Reuse inputs_statement instead of cloning inputs_to_spend and reconstructing later.
Apply this diff:
- let agg_output_mask = output_statements - .clone() - .into_iter() - .map(|stmt| &stmt.witness.mask) - .fold(RistrettoSecretKey::default(), |agg, mask| agg + mask); + let agg_output_mask = output_statements + .clone() + .into_iter() + .map(|stmt| &stmt.witness.mask) + .fold(RistrettoSecretKey::default(), |agg, mask| agg + mask); - let inputs_statement = StealthInputsStatement { - inputs: inputs_to_spend.clone(), - revealed_amount: revealed_input_amount, - }; + let inputs_statement = StealthInputsStatement { + inputs: inputs_to_spend.clone(), + revealed_amount: revealed_input_amount, + }; let outputs_statement = create_outputs_statement(output_statements, revealed_output_amount)?; 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, - }, - outputs_statement, - balance_proof, - }) + Ok(StealthTransferStatement { + inputs_statement, + outputs_statement, + balance_proof, + })Also applies to: 87-92, 100-107
crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
4-4: Use rand_core::OsRng to avoid trait version conflicts.Importing OsRng via digest::crypto_common can mismatch trait versions with tari_crypto.
Apply this diff:
- use digest::crypto_common::rand_core::OsRng; + use rand_core::OsRng;
221-231: Deduplicate lock_revealed_funds and lock_funds_in_vault.Both perform the same action. Keep one API to reduce surface area and confusion.
Also applies to: 179-188
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
applications/tari_indexer/src/network_client.rs(1 hunks)applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs(0 hunks)applications/tari_validator_node/src/p2p/services/mempool/service.rs(1 hunks)applications/tari_validator_node/src/transaction_validators/error.rs(1 hunks)applications/tari_validator_node/src/transaction_validators/is_shard_applicable.rs(1 hunks)applications/tari_validator_node/src/transaction_validators/signature.rs(2 hunks)applications/tari_walletd/src/handlers/accounts.rs(7 hunks)applications/tari_walletd/src/handlers/confidential.rs(4 hunks)applications/tari_walletd/src/lib.rs(1 hunks)applications/tari_walletd/src/main.rs(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/SendNft.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx(1 hunks)crates/engine_types/src/crypto/messages.rs(2 hunks)crates/engine_types/src/crypto/utxo_spend.rs(1 hunks)crates/engine_types/src/hashing.rs(2 hunks)crates/engine_types/src/stealth/transfer.rs(1 hunks)crates/template_lib/src/models/stealth.rs(0 hunks)crates/template_test_tooling/src/support/confidential.rs(4 hunks)crates/template_test_tooling/src/support/stealth.rs(6 hunks)crates/transaction/src/transaction.rs(1 hunks)crates/wallet/crypto/src/balance_proof.rs(2 hunks)crates/wallet/crypto/src/bullet_proof.rs(1 hunks)crates/wallet/crypto/src/confidential.rs(3 hunks)crates/wallet/crypto/src/stealth.rs(8 hunks)crates/wallet/crypto/src/unblinded_statement.rs(3 hunks)crates/wallet/crypto/tests/output_statement.rs(5 hunks)crates/wallet/crypto/tests/viewable_balance_proof.rs(1 hunks)crates/wallet/sdk/src/apis/confidential_crypto.rs(3 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(2 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(3 hunks)crates/wallet/sdk/src/apis/key_manager.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_crypto.rs(3 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(7 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(21 hunks)crates/wallet/sdk/src/apis/transaction.rs(2 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/models/key.rs(1 hunks)crates/wallet/sdk/src/models/stealth_output.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(3 hunks)crates/wallet/sdk_services/src/account_monitor/handle.rs(3 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(3 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(1 hunks)crates/wallet/storage_sqlite/src/models/stealth_output.rs(2 hunks)crates/wallet/storage_sqlite/src/schema.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(1 hunks)utilities/tariswap_test_bench/src/runner.rs(1 hunks)
💤 Files with no reviewable changes (2)
- crates/template_lib/src/models/stealth.rs
- applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs
🧰 Additional context used
🧬 Code graph analysis (23)
crates/wallet/sdk/src/models/stealth_output.rs (4)
bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/EncryptedData.ts (1)
EncryptedData(7-7)bindings/src/types/Amount.ts (1)
Amount(12-12)
applications/tari_validator_node/src/transaction_validators/error.rs (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
crates/engine_types/src/crypto/utxo_spend.rs (2)
crates/engine_types/src/crypto/messages.rs (1)
stealth_ownership64(53-61)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/stealth/transfer.rs (1)
crates/engine_types/src/crypto/messages.rs (1)
stealth_balance_proof64(39-51)
crates/wallet/crypto/tests/viewable_balance_proof.rs (1)
crates/wallet/crypto/src/confidential.rs (1)
create_output_statement(74-136)
crates/template_test_tooling/src/support/confidential.rs (1)
crates/template_lib_types/src/encrypted_data.rs (1)
min_size(26-28)
crates/wallet/crypto/src/unblinded_statement.rs (1)
crates/engine_types/src/crypto/helpers.rs (1)
commit_amount_checked(68-71)
applications/tari_walletd/src/handlers/accounts.rs (3)
crates/wallet/sdk/src/models/account.rs (3)
owner_key_id(33-35)owner_key_id(91-93)account(71-73)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
transfer(212-426)crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
transfer(302-611)
applications/tari_validator_node/src/p2p/services/mempool/service.rs (2)
applications/tari_indexer/src/network_client.rs (1)
transaction(55-55)crates/wallet/storage_sqlite/src/writer.rs (2)
transaction(359-362)transaction(363-368)
crates/wallet/crypto/tests/output_statement.rs (1)
crates/wallet/crypto/src/unblinded_statement.rs (1)
mask(64-66)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (4)
bindings/src/types/Resource.ts (1)
Resource(9-22)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)crates/wallet/sdk/src/sdk.rs (1)
accounts_api(180-187)
crates/wallet/sdk/src/models/key.rs (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk/src/sdk.rs (2)
crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
new(61-73)crates/wallet/sdk/src/apis/config.rs (1)
new(18-23)
crates/wallet/sdk/src/lib.rs (1)
bindings/src/types/Network.ts (1)
Network(6-6)
crates/wallet/sdk_services/src/account_monitor/handle.rs (2)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)
crates/wallet/sdk/src/apis/key_manager.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/wallet/crypto/src/stealth.rs (4)
crates/engine_types/src/stealth/outputs.rs (1)
stmt(43-82)crates/wallet/crypto/src/unblinded_statement.rs (1)
mask(64-66)crates/wallet/crypto/src/balance_proof.rs (1)
generate_stealth_balance_proof_signature(38-51)crates/wallet/crypto/src/bullet_proof.rs (1)
generate_extended_bullet_proof(20-73)
crates/wallet/sdk_services/src/transaction_service/service.rs (1)
crates/wallet/sdk/src/sdk.rs (1)
transaction_api(172-174)
crates/wallet/crypto/src/balance_proof.rs (7)
bindings/src/types/StealthInputsStatement.ts (1)
StealthInputsStatement(8-17)bindings/src/types/StealthOutputsStatement.ts (1)
StealthOutputsStatement(9-24)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/SchnorrSignatureBytes.ts (1)
SchnorrSignatureBytes(5-5)crates/engine_types/src/crypto/messages.rs (2)
stealth_balance_proof64(39-51)stealth_ownership64(53-61)
crates/wallet/sdk/src/apis/stealth_transfer.rs (3)
crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
params(676-688)new(67-79)crates/template_test_tooling/src/support/stealth.rs (1)
inputs(195-206)crates/wallet/sdk/src/models/account.rs (3)
owner_key_id(33-35)owner_key_id(91-93)account(71-73)
crates/template_test_tooling/src/support/stealth.rs (2)
crates/wallet/crypto/src/unblinded_statement.rs (1)
mask(64-66)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
outputs(692-692)
crates/engine_types/src/crypto/messages.rs (3)
bindings/src/types/StealthInputsStatement.ts (1)
StealthInputsStatement(8-17)bindings/src/types/StealthOutputsStatement.ts (1)
StealthOutputsStatement(9-24)crates/engine_types/src/hashing.rs (1)
engine_hasher64(35-37)
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
crates/wallet/sdk/src/models/account.rs (4)
owner_key_id(33-35)owner_key_id(91-93)view_only_key_id(29-31)view_only_key_id(87-89)crates/ootle_address/src/ootle_address.rs (2)
network(40-42)network(224-226)crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
generate_transfer_statement(48-73)
⏰ 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: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: clippy
- GitHub Check: machete
🔇 Additional comments (56)
applications/tari_walletd/web_ui/src/routes/AssetVault/NFTs/components/SendNft.tsx (1)
209-210: Verify fee accuracy for NFT transfers and alignment with confidential transfer handling.The fee buffer has been completely removed for NFT transfers in both
estimateFeeWithTargetAccountandestimateFee(lines 209-210 and 234-235). However, inSendMoney.tsx(lines 205-209), a 100-unit buffer is still applied for Confidential resource types.Confirm that:
- Backend fee estimation is now accurate enough for NFTs to avoid insufficient fee errors
- The difference in handling between NFT transfers (no buffer) and Confidential token transfers (buffer retained) is intentional and justified
applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx (1)
83-83: Confirm backend dry-run fee accuracy
- The UI change in applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx L83 removes the 100n buffer from the fee estimate. Verify that the backend’s dry-run endpoint now returns sufficiently accurate fees to avoid transaction failures due to underpayment.
applications/tari_walletd/src/main.rs (1)
147-150: LGTM! Explicit panic provides clear error context.The defensive
expectcall with a descriptive message is appropriate here. Since seed words are initialized immediately before this call (lines 140-146), a None result indicates a logic bug that should halt execution with a clear diagnostic.crates/wallet/sdk/src/apis/key_manager.rs (1)
277-281: LGTM! Clean separation of index management and key derivation.The refactored
next_keymethod now properly delegates index advancement tonext_derived_key_indexbefore deriving the key. This separation of concerns improves maintainability and makes the key generation flow more explicit.applications/tari_walletd/src/lib.rs (1)
76-88: Verify all downstream account key usages handle lazy initialization. The removal ofget_or_create_initial(KeyBranch::Account)can leaveowner_key_idunset, causing calls likeget_account_owner_key(e.g., inhandlers/nfts.rs,handlers/transaction.rs) to panic or error. Ensure each path initializes or derives the account key before use.crates/engine_types/src/hashing.rs (1)
129-129: LGTM!The domain label renaming from
StealthTransfertoStealthBalanceProofis consistent between the enum variant and its string representation. This aligns with the broader balance-proof refactor described in the PR objectives.Also applies to: 156-156
utilities/tariswap_test_bench/src/runner.rs (1)
52-52: LGTM!The removal of
.awaitcorrectly reflects the API change whereinsert_new_transactionwas converted from async to sync. The error propagation with?is appropriate for the new synchronous signature.crates/wallet/storage_sqlite/src/writer.rs (1)
1128-1128: LGTM with existing limitation noted.The cast from
u64toi64follows the existing pattern in this file for persisting large numeric values (see similar casts with TODO comments at lines 736-741). While this could overflow for values exceedingi64::MAX, it's a known limitation already present in the codebase for handling amounts in SQLite.crates/wallet/sdk_services/src/transaction_service/service.rs (1)
154-154: LGTM!The removal of
.awaitand formatting change correctly reflect the API refactor whereinsert_new_transactionbecame synchronous. The error propagation remains correct.crates/wallet/sdk/src/apis/transaction.rs (2)
53-65: LGTM! Async removal aligns with synchronous storage operations.The change from
async fntofnis appropriate since the implementation usesself.store.with_write_tx(), which appears to be a synchronous operation. The added documentation clearly describes the behavior and return semantics.
67-72: Well-documented async method.The documentation clearly describes the submission behavior, status transitions, and return semantics. The method correctly remains async as it performs network operations.
crates/wallet/storage_sqlite/src/schema.rs (1)
164-164: LGTM! Schema change aligns with PR objectives.The addition of
minimum_value_promisecolumn to thestealth_outputstable is consistent with the PR's goal to add minimum value promise tracking. As noted in the PR objectives, this requires the data directory to be deleted, which is expected for a breaking schema change.applications/tari_indexer/src/network_client.rs (1)
41-43: LGTM! Pre-check condition aligned with input validation.The change from
is_shard_applicable()tohas_inputs()provides a more direct validation of the transaction's input presence. The errorNoInputsProvidedaccurately reflects the check being performed.crates/engine_types/src/stealth/transfer.rs (1)
116-121: LGTM! Enhanced security through statement-based message construction.The change to use
stealth_balance_proof64with completeinputs_statementandoutputs_statement(instead of individual revealed amounts) strengthens the balance proof by preventing potential output malleability, as stated in the PR objectives. This is a well-considered cryptographic improvement.crates/engine_types/src/crypto/utxo_spend.rs (2)
25-25: LGTM! Simplified ownership message construction.The message construction now uses
stealth_ownership64(&input.commitment, &utxo.output.public_nonce), which is a cleaner approach consistent with the broader cryptographic refactor in this PR.
32-36: Signature verification method is correct
.verifyinvokes the domain-aware Verifiable trait for ownership proofs (used elsewhere for knowledge proofs), whereasverify_raw_uniformis reserved for balance proofs. The 64-bytemessagearray auto-borrows to&[u8], matchingfn verify(&self, …, message: &[u8]).crates/template_test_tooling/src/support/confidential.rs (5)
11-11: LGTM! Type import updated to use Witness pattern.The import change from
UnblindedOutputStatementtoUnblindedOutputWitnessaligns with the broader refactor to use Witness-based types throughout the codebase.
38-45: LGTM! Witness construction with appropriate test values.The
UnblindedOutputWitnessis correctly constructed with all required fields. The use of default/zero values forsender_public_nonce,minimum_value_promise, and minimalencrypted_datais appropriate for test scaffolding.
48-55: LGTM! Change statement construction mirrors output pattern.The change statement construction follows the same pattern as the output statement, ensuring consistency in the test tooling.
152-159: LGTM! Withdraw proof output witness construction.The output witness construction in the withdraw proof follows the established pattern with appropriate test values.
160-167: LGTM! Withdraw proof change witness construction.The change witness construction in the withdraw proof is consistent with the output witness pattern.
applications/tari_validator_node/src/transaction_validators/error.rs (1)
39-40: LGTM! Clear error variant for missing signer.The new
NoMainSignererror variant provides clear semantics and includes thetransaction_idfor debugging. This supports the enhanced signature validation introduced in this PR.crates/wallet/sdk_services/src/account_monitor/handle.rs (2)
16-20: LGTM!The new
AssociateResourcevariant follows the established request/response pattern consistently with the existingRefreshAccountvariant.
57-72: LGTM!The
associate_resourcemethod correctly implements the request/response pattern, matching the structure and error handling of the existing methods.crates/wallet/crypto/src/bullet_proof.rs (1)
18-22: LGTM!The type rename from
UnblindedOutputStatementtoUnblindedOutputWitnessis a straightforward refactor. The function logic remains unchanged and continues to access the same struct fields (mask,amount,minimum_value_promise).crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
13-13: LGTM!The type rename from
UnblindedOutputStatementtoUnblindedOutputWitnessis consistent with the broader refactoring effort. The method logic and field assignments remain unchanged.Also applies to: 428-468
crates/wallet/crypto/tests/viewable_balance_proof.rs (1)
12-12: LGTM!The test helper has been correctly updated to use
UnblindedOutputWitnessinstead ofUnblindedOutputStatement. The field assignments remain consistent with the previous structure.Also applies to: 19-29
crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
144-150: LGTM!The
release_revealed_fundsmethod correctly follows the established transaction pattern: create write transaction, perform the operation, and commit.
160-166: LGTM!The
finalize_locked_revealed_fundsmethod correctly follows the established transaction pattern, consistent with similar methods in this API.crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
19-22: LGTM!The type renames from
UnblindedOutputStatement,UnblindedStealthInputStatement, andUnblindedStealthOutputStatementto theirWitnesscounterparts are straightforward refactors. The method implementations remain unchanged and correctly delegate to the underlying crypto functions.Also applies to: 48-73, 116-128
crates/wallet/sdk/src/sdk.rs (4)
72-83: LGTM!The network validation logic correctly prevents database/config network mismatches by:
- Reading the stored network (if present)
- Validating it matches the config network
- Initializing the stored network if absent
This ensures consistency but note that existing wallets without a stored network will have one written automatically.
93-97: LGTM!The new
get_store_networkhelper method correctly retrieves the network from storage and returnsNoneif not found, following the.optional()pattern used elsewhere in the codebase.
217-222: Verify StealthTransferApi constructor signature matches.The
StealthTransferApi::newcall was updated to removekey_manager_apiandstealth_crypto_apiparameters. Ensure the constructor signature incrates/wallet/sdk/src/apis/stealth_transfer.rsmatches this usage.Based on the relevant code snippet from
crates/wallet/sdk/src/apis/stealth_transfer.rs(lines 60-72), the constructor signature appears correct and matches the call site.
288-293: LGTM!The change to return
Option<SeedWords>instead of requiring a seed to exist is a good improvement. The implementation correctly usesmapandtransposeto convert fromOption<&CipherSeed>toResult<Option<SeedWords>>.crates/wallet/crypto/tests/output_statement.rs (2)
15-17: LGTM!The test helper
make_input_statementshas been correctly updated to construct and returnVec<UnblindedStealthInputWitness>instead of the previousStatementtype. The field assignments remain consistent.Also applies to: 92-104
106-139: LGTM!The test helper
make_output_statementshas been correctly updated to use the newWitnesstypes. Note the structural change at lines 119-136 whereUnblindedStealthOutputWitnessnow contains a nestedwitnessfield of typeUnblindedOutputWitness, reflecting the new witness-based structure.crates/wallet/crypto/src/confidential.rs (1)
17-17: LGTM! Type rename is consistent.The rename from
UnblindedOutputStatementtoUnblindedOutputWitnessaligns with the broader refactor across the codebase to use Witness-based structures.Also applies to: 24-26, 75-77
crates/wallet/sdk/src/apis/confidential_crypto.rs (1)
14-14: LGTM! Public API updated consistently.The public API signatures are correctly updated to use
UnblindedOutputWitness, maintaining consistency with the internal implementation changes.Also applies to: 42-44, 93-93
crates/wallet/crypto/src/unblinded_statement.rs (2)
11-18: LGTM! Structural changes align with PR objectives.The addition of
minimum_value_promiseandencrypted_datafields toUnblindedOutputWitnesssupports the PR's goal of adding minimum value promise tracking to stealth outputs. The rename from Statement to Witness clarifies the semantic role of these structures.
27-31: LGTM! Field rename improves semantic clarity.The field rename from
statementtowitnessinUnblindedStealthOutputWitnessbetter reflects the nature of this data structure as witness information for outputs.applications/tari_walletd/src/handlers/accounts.rs (4)
949-954: Verify necessity of early owner_key_id requirement.The handler requires
owner_key_idto be present upfront, but the signing logic (lines 977-984) may use a throwaway nonce instead of the account key when there are no revealed amounts. This creates a situation where an owner key is required but might not be used.Consider clarifying the design intent: Is the owner_key_id required for account creation/validation purposes even if stealth transfers with no revealed amounts don't need it for signing?
Based on the relevant code snippet from
crates/wallet/sdk/src/apis/stealth_transfer.rs, the transfer API also validates owner_key_id presence, so this appears to be an intentional API constraint.
977-990: LGTM! Signing logic correctly handles revealed vs. fully confidential transfers.The conditional signing approach is correct:
- For transfers with revealed amounts (fee or transfer inputs), the account owner key is required for authorization.
- For fully confidential stealth transfers with no revealed amounts, a throwaway nonce suffices as no account-level authorization is needed.
1022-1024: LGTM! Transaction ID association improves lock tracking.Associating the lock with the transaction ID before submission enables proper cleanup if the transaction fails or is aborted later.
1070-1073: LGTM! Resource association delegated to account monitor.The change from local cache population to delegating resource association to the account monitor improves separation of concerns and ensures consistent state management.
crates/template_test_tooling/src/support/stealth.rs (1)
16-18: LGTM! Test tooling updated consistently with Witness refactor.All type imports, construction sites, and field access patterns are correctly updated to use the new Witness-based structures and the renamed
witnessfield (previouslystatement).Also applies to: 89-100, 177-191, 199-199, 217-217
crates/engine_types/src/crypto/messages.rs (2)
39-51: LGTM! Balance proof signature improved for anti-malleability.The changes to include full
StealthInputsStatementandStealthOutputsStatementobjects in the balance proof signature (instead of just amounts) align with the PR objective to prevent potential output malleability. This provides cryptographic binding to the complete input/output structure, not just the amounts.The function rename to
stealth_balance_proof64and domain label update toStealthBalanceProofimprove semantic clarity.
53-61: Verify security implications of reduced signature.The
stealth_ownership64signature was reduced from including(public_key, public_nonce, commitment, public_output_nonce)to only(commitment, public_output_nonce). Removing the public key and nonce from the message preimage changes the cryptographic binding.Verify that the ownership proof remains secure with the reduced signature. Specifically:
- Does the proof still adequately bind the owner to the commitment?
- Could removing these fields enable any malleability or replay attacks?
- Was this change intentional as part of the signature refactor?
Given that the PR description mentions security improvements and manual testing was performed, this is likely intentional, but explicit verification would provide confidence.
crates/wallet/crypto/src/balance_proof.rs (3)
47-50: Correct switch to the new domain-separated message (stealth_balance_proof64).Message includes public_excess, public_nonce, and both statements; aligns with engine_types/messages.rs.
58-60: Owner proof message domain updated and signing path looks correct.Using stealth_ownership64(commitment, public_output_nonce) with EngineSchnorrSignature::sign is consistent with engine message format.
44-51: Zero secret_excess behavior — verify intent.If agg_input_mask == agg_output_mask, secret_excess == 0. Do we require a non-zero signature (vs. zero/empty proof) in the stealth case? Confirm engine validation expectations.
crates/wallet/crypto/src/stealth.rs (1)
110-153: Outputs statement from witnesses looks solid.Range proof aggregation and viewable balance proof construction align with witness fields and engine validators.
crates/wallet/sdk/src/apis/stealth_transfer.rs (2)
432-462: Fee change calculation and balancing — looks consistent with statement validation.fee_stealth_change_amt = stealth_inputs - max_fee, and the revealed component is balanced via input_revealed_amount/output_revealed_amount.
596-603: Consider linking locks to the transaction id once available.To aid recovery/observability, call outputs_api.locks_set_transaction_id(lock_id, tx_id) when you have a stable id (signed or unsigned if available).
Do we have an UnsignedTransaction id accessor, or is this only available post-signing? If post-signing, ensure the caller updates the lock with the final TransactionId.
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
195-202: release_lock now also releases vault locks — good.This avoids stranded revealed-funds locks when releasing stealth UTXO locks.
604-665: Output witness creation flow looks right.Key derivation, encryption, tag derivation, and owner public key generation align with the witness model.
667-710: Transfer statement generation performs a strict balance check — good guardrail.Witness resolution and statement creation integrate cleanly with the crypto API.
Test Results (CI)472 tests +27 464 ✅ +19 1h 38m 13s ⏱️ + 47m 2s For more details on these failures, see this check. Results for commit ea592b6. ± Comparison against base commit a343c57. |
2 participants
Description
fix(wallet/sdk)!: split transfer and transaction signing
fix!: include input and output statement in balance proof signature message
fix(wallet)!: added minimum value promise to stealth outputs table
Motivation and Context
Allows SDK users to generate unsigned transfer transactions and sign them using a secure offline cryptographic device.
Added transfer statement inputs and outputs to the balance signature message preimage to prevent potential malleability of outputs.
How Has This Been Tested?
Existing tests, manually, in this commit tari-project/ootle-wallet-cli@7c196d9
Breaking Changes
Summary by CodeRabbit
New Features
Improvements
Bug Fixes