feat!: migrate XTR to stealth resource - #1556
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces stealth/XTR semantics across runtime, SDK, bindings, templates, storage and UIs; adds PayFee/stealth fee pipeline with refundable vs non‑refundable handling and total_fee_overcharge; migrates wallet locks to a centralized Locks/WalletLockId model; adds InvalidTransaction error mapped to JSON‑RPC 400; wires nonce keys for claim‑burn via ExtClaimBurnProof. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant RPC as Indexer JSON-RPC
participant TM as Tx Manager
rect rgb(255,250,240)
note over Client,RPC: Client submits transaction via JSON-RPC
Client->>RPC: submit_transaction(tx)
RPC->>TM: submit_transaction(tx)
alt invalid signatures
TM-->>RPC: Err(InvalidTransaction { transaction_id, details })
RPC-->>Client: ApplicationError(400) (json::Value::Null)
else other error
TM-->>RPC: Err(...)
RPC-->>Client: Internal error mapping
else success
TM-->>RPC: Ok(transaction_id)
RPC-->>Client: { id: transaction_id }
end
end
sequenceDiagram
autonumber
participant CLI as CLI
participant WD as Wallet Daemon
participant SDK as Wallet SDK
participant NET as Network
rect rgb(235,245,255)
note over CLI,WD: Claim‑burn using nonce-based ExtClaimBurnProof
CLI->>WD: ClaimBurn { claim_proof: ExtClaimBurnProof }
WD->>SDK: create_nonce_key() -> (claim_public_key, nonce_index)
WD->>SDK: accountsClaimBurn({ account: ComponentAddress, claim_proof })
SDK->>NET: submit_transaction(...)
NET-->>SDK: result
SDK-->>WD: transaction id / status (wait for finality)
WD-->>CLI: final result (via transactionsWaitResult)
end
sequenceDiagram
autonumber
participant TP as Tx Processor
participant RT as Runtime Interface
participant WS as WorkingState
participant FS as FeeState
rect rgb(255,245,235)
note over TP,RT: Pay fee via stealth instruction
TP->>RT: pay_fee(statement, opt_revealed_bucket)
RT->>WS: execute_stealth_transfer(resource=XTR, statement, opt_revealed_bucket)
WS->>FS: add_fee_payment_checked(resource_container, vault_id_opt)
alt no revealed funds available
WS-->>RT: Err(NoFeesPaid)
RT-->>TP: Err(NoFeesPaid)
else payments recorded
WS-->>RT: Ok
RT-->>TP: Ok
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
applications/tari_walletd/web_ui/src/routes/AccountDetails/AccountDetails.tsx (1)
170-176: Public key column renders account address instead of the public keyThe “Public key” cell currently copies the account address. Use the public_key value.
Apply:
- <DataTableCell> - {accountsData?.public_key && <CopyAddress address={accountsData?.account.address!} />} - </DataTableCell> + <DataTableCell> + {accountsData?.public_key && <CopyAddress address={accountsData!.public_key} />} + </DataTableCell>crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
262-288: Potential negative fee-change path can errorGuard uses is_zero(), so a negative confidential_change still enters branch and create_confidential_proof_statement will fail (expects positive). Mirror the main-change logic and gate on is_positive().
Apply:
- let maybe_fee_change_statement = if confidential_change.is_zero() { - // No change necessary - None - } else { + let maybe_fee_change_statement = if confidential_change.is_positive() { let statement = self.create_confidential_proof_statement(&account_public_key, confidential_change, None)?; @@ - Some(statement) - }; + Some(statement) + } else { + None + };applications/tari_wallet_cli/src/command/transaction.rs (1)
401-411: Update CLI help to reference stealth resourceThe
ConfidentialTransferInputSelectionenum only provides the variants
- ConfidentialOnly
- RevealedOnly
- PreferRevealed
- PreferConfidential
There is no dedicated “stealth-preferring” variant, and the
PreferConfidentialbranch in the stealth transfer path handles locking/unlocking stealth inputs correctly. Therefore no change toPreferConfidentialis needed.Please update the CLI documentation comment for the
resource_addressargument inapplications/tari_wallet_cli/src/command/transaction.rsas follows:- /// The address of the resource to send. If not provided, use the default Tari confidential resource + /// The address of the resource to send. If not provided, uses the default Tari stealth resourcecrates/wallet/sdk/src/storage.rs (1)
155-160: Action Required: Update RPC Types and Clients for Newsigned_by_public_keyFilterThe signature of
transactions_fetch_allnow includes a third parameter,signed_by_public_key, introducing a breaking change in the RPC layer. While the core Rust implementation and SDK calls have been updated, the following areas still need adjustments to propagate the new filter correctly:• Rust RPC client (wallet_daemon_client):
– Inclients/wallet_daemon_client/src/types.rs, add
rust pub signed_by_public_key: Option<RistrettoPublicKeyBytes>,
to theTransactionGetAllRequeststruct.
– Ensureget_transactions_allforwards this field in the request.• TypeScript bindings for the daemon client:
– Inbindings/src/types/wallet-daemon-client/TransactionGetAllRequest.ts, extend:
ts export type TransactionGetAllRequest = { status: TransactionStatus | null; component: ComponentAddress | null; signed_by_public_key: Uint8Array | null; };
– Regenerate the TS bindings so that all generated clients include the new field.• JavaScript/TypeScript client code:
– Inclients/javascript/wallet_daemon_client/src/index.ts, ensuretransactionsList(params)callers pass throughsigned_by_public_key.
– Update any helper functions or convenience wrappers (e.g.,transactionsList) to accept the new property.• Web UI JSON-RPC layer:
– Inapplications/tari_walletd/web_ui/src/utils/json_rpc.ts, updatetransactionsGetAllto accept and forwardsigned_by_public_key.
– Inapplications/tari_walletd/web_ui/src/api/hooks/useTransactions.tsx, extend theTransactionGetAllRequestimport to include the new field in hook parameters.• Server-side handler:
– Inapplications/tari_walletd/src/handlers/transaction.rs, the handler signature still uses the oldTransactionGetAllRequest. Update the generated request type (or its mapping) to includesigned_by_public_key, and apply the filter inhandle_get_all_transactions.Optional refactor: consider consolidating the three optional filters into a single
TransactionFilterstruct to simplify future extensions:- fn transactions_fetch_all( - &mut self, - status: Option<TransactionStatus>, - component: Option<ComponentAddress>, - signed_by_public_key: Option<RistrettoPublicKeyBytes>, - ) -> … + struct TransactionFilter { + status: Option<TransactionStatus>, + component: Option<ComponentAddress>, + signed_by_public_key: Option<RistrettoPublicKeyBytes>, + } + fn transactions_fetch_all(&mut self, filter: TransactionFilter) -> …applications/tari_walletd/web_ui/src/api/hooks/useTransactions.tsx (1)
49-66: Fix cache key to include filter params; current key causes collisions across different reqs.Using a static ["transactions"] key will mix results for different filters (status/component/signer_public_key).
Apply:
-export const useGetAllTransactions = (req: TransactionGetAllRequest) => { +export const useGetAllTransactions = (req: TransactionGetAllRequest) => { return useQuery({ - queryKey: ["transactions"], - queryFn: () => transactionsGetAll(req), + // Include filters to prevent cache collisions across different queries + queryKey: ["transactions", req?.status ?? null, req?.component ?? null, req?.signer_public_key ?? null], + queryFn: () => transactionsGetAll(req),This remains compatible with invalidateQueries(["transactions"]) due to partial-key matching.
crates/engine_types/src/resource_container.rs (2)
612-623: Bug: lock_all(…): Stealth branch returns Fungible container.Returning Self::Fungible for a Stealth container will cause type mismatch downstream (unlock/deposit). Should return Stealth.
Apply:
- Ok(Self::fungible(resource_address, newly_locked_amount)) + Ok(Self::stealth(resource_address, newly_locked_amount))
356-370: Ensure allwithdraw_allcall sites handle the newResultreturn type
TheResourceContainer::withdraw_all(andValidatorFee::withdraw_all) APIs now returnResult<_, ResourceError>. Callers no longer receive a bare value and must propagate or handle errors. Please update the following locations:• crates/engine_types/src/vault.rs:82
–Vault::withdraw_allcurrently delegates toresource_container.withdraw_all()but returns a concreteBucket. Change its signature to
pub fn withdraw_all(&mut self) -> Result<Bucket, ResourceError>
and propagate the error (?ormap_err) from the underlying call.• crates/template_lib/src/models/vault.rs:298
– Adjustpub fn withdraw_all(&mut self) -> Bucketto return
Result<Bucket, ResourceError>
and bubble up the error from the engine’sVault::withdraw_all.• Template tests (crates/engine/tests/templates)
– shenanigans/src/lib.rs (lines 69, 153, 159, 165)
– nft/basic_nft/src/lib.rs (lines 109, 132)
– These calls tovault.withdraw_all()andstolen.withdraw_all()must now.unwrap(), use?, or otherwise handleResult.• crates/engine_types/src/validator_fee.rs:163
–pub fn withdraw_all(&mut self) -> Result<ResourceContainer, ResourceError>already returnsResult; verify any call sites (if present) handle errors appropriately.• crates/engine/src/runtime/working_state.rs
– Calls topool_mut.withdraw_all()?andresx.withdraw_all()?are correctly using?. No change needed here.• crates/engine/tests/test.rs:565
–sparkle_nft.withdraw_all()now returnsResult; add error handling.After making these changes, run
cargo check/cargo testto ensure all call sites compile and errors are handled gracefully.crates/wallet/sdk/src/apis/transaction.rs (1)
66-105: submit_transaction now returnsbool─update all call sitesThe signature of
submit_transactionhas changed from returning()toResult<bool, TransactionApiError>. You must update every caller to handle thebool(wheretruemeans pending andfalsemeans invalid) instead of ignoring it. Otherwise callers will receive aboolwhere they expect(), leading to compilation errors or dropped error cases.Key locations needing changes:
- utilities/transaction_submitter/src/main.rs:105
- utilities/tariswap_test_bench/src/runner.rs:55
- integration_tests/src/wallet_daemon_cli.rs:224, 477, 562, 677, 945
- applications/tari_walletd/src/services/transaction_service/service.rs:157–158, 222–223
- applications/tari_wallet_cli/src/command/transaction.rs:283–284, 345–346
- applications/tari_walletd/src/handlers/transaction.rs:169–171, 355–356
- applications/tari_walletd/src/handlers/nfts.rs:126–127, 316–317
- applications/tari_validator_node_cli/src/command/transaction.rs:268–269
- applications/tari_swarm_daemon/src/layer_one_transactions/service.rs:64–66
- applications/tari_indexer/src/transaction_manager/mod.rs:75–76
At each site, change patterns like:
client.submit_transaction(...).await?;to something that checks the result, for example:
if client.submit_transaction(...).await? { // handle pending } else { // handle invalid transaction }Or explicitly ignore the bool when appropriate:
let _ = client.submit_transaction(...).await?;crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
218-237: Missing FK and helpful index onstealth_outputs.
- Add
FOREIGN KEY (owner_account_id) REFERENCES accounts(id) ON DELETE CASCADEto ensure integrity.- Add index on
(owner_account_id, status)for common wallet queries.CREATE TABLE stealth_outputs ( id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, - owner_account_id INTEGER NOT NULL, + owner_account_id INTEGER NOT NULL REFERENCES accounts (id) ON DELETE CASCADE, @@ ); 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_status ON stealth_outputs (owner_account_id, status);crates/wallet/storage_sqlite/src/models/transaction.rs (2)
23-42: Diesel field names don’t match columns; will break at runtime.DB columns are
referenced_componentsandsigners, but struct fields are_referenced_componentsand_signers. Either rename fields or annotate with#[diesel(column_name = ...)].pub struct TransactionRecord { @@ - pub _referenced_components: String, - pub _signers: String, + #[diesel(column_name = referenced_components)] + pub _referenced_components: String, + #[diesel(column_name = signers)] + pub _signers: String,Alternatively, drop underscores and match column names exactly.
58-59: Unsafe cast from i64 → u64 forfinal_fee.Negative values will become huge u64s. Use
try_fromand return a decoding error (or clamp to 0 with a warning).- final_fee: self.final_fee.map(|f| f as u64), + final_fee: self + .final_fee + .map(u64::try_from) + .transpose() + .map_err(|e| WalletStorageError::DecodingError { + operation: "transaction_get", + item: "final_fee", + details: e.to_string(), + })?,crates/engine/src/runtime/impl.rs (1)
1560-1580: Potential address mismatch: STEALTH_TARI_RESOURCE_ADDRESS vs XTR.Here you execute_stealth_transfer with STEALTH_TARI_RESOURCE_ADDRESS and then deposit into a container initialized with XTR. If XTR ≠ STEALTH_TARI_RESOURCE_ADDRESS, deposit will fail at runtime; if they’re aliases, this is confusing. Use one constant consistently (recommend XTR to match the fee checks above and other call sites).
Apply this diff:
- if let Some(statement) = arg.statement { - if let Some(revealed) = - state_mut.execute_stealth_transfer(STEALTH_TARI_RESOURCE_ADDRESS.into(), statement, None)? - { + if let Some(statement) = arg.statement { + if let Some(revealed) = + state_mut.execute_stealth_transfer(XTR.into(), statement, None)? + { container.deposit(revealed)?; } }
🧹 Nitpick comments (62)
crates/engine_types/src/hashing.rs (1)
40-43: Clarify TODO: serializer switch is consensus‑critical; require versioned domains and a migration plan.Right now, TariHasher32/64 hash via tari_bor (CBOR). A blanket move to Borsh will change every digest and anything derived from it (addresses, IDs, receipts, sigs), risking state/consensus break. Please document the constraints and outline a versioned rollout (e.g., introduce V2 domain labels, wire versioned hashers, gating/migrations), rather than a bare TODO.
Apply this small diff to make the intent explicit and harder to misuse:
-// TODO: switch all hashes to borsh +// NOTE: Consensus-critical. Current hashing encodes via tari_bor (CBOR). +// Switching to BORSH will change all digests and derived identifiers (addresses, IDs, receipts, signatures). +// If pursued, introduce versioned domain labels (e.g., TemplateV2, SubstateValueV2), add network gating/migrations, +// and update TariHasher32/64 docs/tests to lock the chosen encoding per domain.crates/engine/tests/test.rs (4)
1076-1082: Prefer expect over unwrap for clearer failures when locating the ticket vaultThis provides a more actionable panic message if the vault map doesn’t contain the resource.
- let vault = vaults.get(&ticket_resource).unwrap(); + let vault = vaults + .get(&ticket_resource) + .expect("account has no vault for the ticket resource");
1088-1091: Remove stale TODO commentThe code now passes a NonFungibleId; the comment about passing a SubstateId is outdated and confusing. Please delete it.
1093-1104: Avoid unnecessary clone of varsNo subsequent uses require cloning here.
- vars.clone(), + vars,
1106-1110: Drop duplicate Ticket struct in function scopeYou already define Ticket at module scope (Lines 1016–1019). The inner definition shadows it and is redundant. Remove the inner struct and keep decoding to Ticket.
crates/template_lib/src/models/resource.rs (1)
63-66:is_tarinow implies stealth address — clarify in docMinor nit: rename or document that “Tari” here means the native stealth resource address to avoid ambiguity with the former confidential address.
- pub fn is_tari(&self) -> bool { + /// Returns true if this equals the native Tari stealth resource address. + pub fn is_tari(&self) -> bool { *self == STEALTH_TARI_RESOURCE_ADDRESS }crates/transaction/src/transaction.rs (2)
16-17: Prefer consistent import path forTemplateAddressElsewhere in this file (tests)
types::TemplateAddressis used. For consistency:-use tari_template_lib::{models::ComponentAddress, prelude::TemplateAddress}; +use tari_template_lib::{models::ComponentAddress, types::TemplateAddress};
198-209: Iterator over referenced template addresses looks goodCollects
CallFunctiontemplate addresses across both main and fee instructions. Consider a follow-up test that asserts the iterator yields the expected template(s) fromcreate_transaction()and ignoresCallMethod/PublishTemplate.applications/tari_indexer/src/transaction_manager/error.rs (1)
11-28: Future-proof enum with non_exhaustive (optional)If external crates exhaustively match TransactionManagerError, future additions become breaking. Consider marking the enum non_exhaustive.
Apply this diff:
#[derive(Debug, thiserror::Error)] -pub enum TransactionManagerError { +#[non_exhaustive] +pub enum TransactionManagerError {crates/wallet/sdk/src/apis/key_manager.rs (1)
115-122: New derive_keypair API is fine; reduce duplication with account variantImplementation is correct. Consider delegating derive_account_key_pair to this generic helper.
Apply:
@@ pub fn derive_account_key_pair(&self, index: u64) -> Result<KeyPair, KeyManagerApiError> { - let key = self.derive_account_key(index)?; - let public_key = RistrettoPublicKey::from_secret_key(&key.key); - Ok(KeyPair { - public_key, - secret_key: key, - }) + self.derive_keypair(KeyBranch::Account, index) }crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
363-393: Main change-output guard is correct; simplify redundant checkYou already guard on is_positive(); change_value == statement.amount is therefore positive. The inner is_positive() check is redundant.
Apply:
- let change_value = statement.amount; - - if change_value.is_positive() { - self.outputs_api.add_output(ConfidentialOutputModel { + let change_value = statement.amount; + self.outputs_api.add_output(ConfidentialOutputModel { account_address: *account.address(), vault_id: src_vault.id, commitment: statement .to_commitment() .expect("BUG: to_commitment negative amount") .to_byte_type(), value: change_value, sender_public_nonce: Some(statement.sender_public_nonce.to_byte_type()), encryption_secret_key_index: account_secret.key_index, encrypted_data: statement.encrypted_data.clone(), public_asset_tag: None, status: OutputStatus::LockedUnconfirmed, lock_id: Some(inputs_to_spend.lock_id), - })?; - } + })?;crates/wallet/sdk/src/models/account.rs (1)
20-40: Added accessors — OK; consider ergonomic name() signatureGetters are fine. Consider returning Option<&str> for name() to avoid callers needing as_deref().
Apply:
- pub fn name(&self) -> Option<&String> { - self.name.as_ref() - } + pub fn name(&self) -> Option<&str> { + self.name.as_deref() + }crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
22-22: Consider using a more descriptive alias name to avoid confusionThe alias
BuiltinAccountforAccountfromtari_template_libmight be confusing since the file also importsAccountfromcrate::models. Consider using a more descriptive name likeTemplateAccountorEngineAccountto make the distinction clearer between the template library's account type and the wallet SDK's account type.-use tari_template_lib::{ - models::{Account as BuiltinAccount, ComponentAddress, ResourceAddress, VaultId}, +use tari_template_lib::{ + models::{Account as TemplateAccount, ComponentAddress, ResourceAddress, VaultId},Also applies to: 38-38
applications/tari_walletd/src/handlers/accounts.rs (1)
637-639: Consider more user-friendly error messagesThe error messages could be more helpful by including the actual amounts. Consider improving the messages to help users understand the issue better.
- if final_amount.is_zero() { - return Err(invalid_params("max_fee", Some("fee equals or exceeds claimed amount"))); - } + if final_amount.is_zero() { + return Err(invalid_params("max_fee", Some(format!("fee ({} XTR) equals or exceeds claimed amount ({} XTR)", + max_fee, unmasked_output.value)))); + }bindings/src/index.ts (1)
117-117: Breaking rename to TariStealthClaim—consider a temporary alias to reduce consumer churn.Provide a deprecation bridge by re-exporting the old name as an alias.
export * from "./types/TariStealthClaim"; +export { TariStealthClaim as ConfidentialClaim } from "./types/TariStealthClaim";crates/consensus_tests/src/support/transaction.rs (1)
112-118: Propagate new total_fee_overcharge field in test fixtures — good; add safeguardBoth FeeReceipt literals now set total_fee_overcharge = 0. Looks correct and consistent with engine_types change.
Double-check other consensus tests with the script from the previous comment to avoid brittle equality failures on FeeReceipt.
Optionally add a small helper (e.g., fee_receipt_with_fee(fee)) to keep these literals DRY in tests.
Also applies to: 136-142
applications/tari_walletd/src/lib.rs (1)
48-48: Eagerly prefetch XTR into substate cache — OK; consider bounded retry for cold-start robustnessImport and fire-and-forget prefetch are fine. A short retry loop will smooth transient indexer/network hiccups without blocking startup.
Apply this bounded retry:
tokio::spawn({ let wallet_sdk = wallet_sdk.clone(); async move { - // Ensures that the XTR resource is available in the substate cache - if let Err(err) = wallet_sdk.substate_api().fetch_resource(XTR).await { - error!(target: LOG_TARGET, "Failed to fetch XTR resource: {}", err); - } + // Ensure XTR is warmed in the substate cache + let mut attempts: u8 = 0; + loop { + match wallet_sdk.substate_api().fetch_resource(XTR).await { + Ok(_) => break, + Err(err) if attempts < 5 => { + attempts += 1; + warn!(target: LOG_TARGET, "XTR fetch attempt {attempts} failed: {err}; retrying in 1s"); + tokio::time::sleep(std::time::Duration::from_secs(1)).await; + } + Err(err) => { + error!(target: LOG_TARGET, "Failed to fetch XTR resource after {attempts} attempts: {err}"); + break; + } + } + } } });If other services assume XTR availability immediately at startup, confirm ordering; otherwise this background warm-up is sufficient.
Also applies to: 80-88
applications/tari_walletd/web_ui/src/routes/Wallet/Wallet.tsx (1)
51-51: Avoid non-null assertion; render conditionallyPrevents passing undefined at runtime and removes the non-null assertion.
- <Transactions account={account!} ownerPublicKey={publicKey} /> + {account && <Transactions account={account} ownerPublicKey={publicKey} />}Follow-up: Ensure Transactions triggers refetch when ownerPublicKey changes (add ownerPublicKey to its effect deps).
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
53-53: Disable submission while creating a key to prevent duplicatesOptional UX hardening: use the mutation’s loading state to disable the Add Key button during in-flight requests.
Example (outside this diff):
const { mutate: mutateCreateKey, isLoading: isCreating } = useKeysCreate("account"); // ... <Button variant="contained" type="submit" disabled={isCreating}>Add Key</Button>Also consider scoping invalidation to the branch in useKeys.tsx:
queryClient.invalidateQueries({ queryKey: ["keys_list", branch] });crates/engine/src/runtime/error.rs (1)
201-202: Map NoFeesPaid to a specific RejectReason for better client UXCurrently NoFeesPaid falls through to ExecutionFailure. Map it to RejectReason::InsufficientFeesPaid so wallets can surface a consistent “fees not paid” message.
pub fn to_reject_reason(&self) -> RejectReason { match self { Self::SubstateNotFound { id } => RejectReason::OneOrMoreInputsNotFound(format!("Substate {id} not found",)), @@ Self::InsufficientFeesPaid { fees_paid, required_fee, } => RejectReason::InsufficientFeesPaid(format!( "Insufficient fees paid: {fees_paid}, required fees: {required_fee}" )), + Self::NoFeesPaid { details } => { + RejectReason::InsufficientFeesPaid(format!("No fees paid from stealth transfer: {details}")) + } err => RejectReason::ExecutionFailure(err.to_string()), } }Also applies to: 294-311
applications/tari_walletd/web_ui/src/routes/Transactions/TransactionsLayout.tsx (1)
30-39: Avoid non-null assertions; gate render until store is hydrated and refetch on owner key changes
- Using account! and publicKey! risks runtime crashes before the store is ready. Guard rendering.
- Also ensure Transactions.tsx refetches when ownerPublicKey changes.
function TransactionsLayout() { - const { account, publicKey } = useAccountStore(); + const { account, publicKey } = useAccountStore(); + if (!account || !publicKey) { + return null; // or a Skeleton/Loading component + } return ( @@ - <Transactions account={account!} ownerPublicKey={publicKey!} /> + <Transactions account={account} ownerPublicKey={publicKey} />And in applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx, update effect deps so data refetches when the owner key changes:
-useEffect(() => { - refetch(); -}, [account]); +useEffect(() => { + refetch(); +}, [account, ownerPublicKey]);applications/tari_indexer/src/json_rpc/handlers.rs (1)
573-583: Return structured error data and log invalid tx for observabilityInclude transaction_id and details in the JSON-RPC error data, and add a warn! so ops can trace client issues.
- TransactionManagerError::InvalidTransaction { - transaction_id, - details, - } => JsonRpcResponse::error( - answer_id, - JsonRpcError::new( - JsonRpcErrorReason::ApplicationError(400), - format!("Transaction {} is invalid: {}", transaction_id, details), - json::Value::Null, - ), - ), + TransactionManagerError::InvalidTransaction { transaction_id, details } => { + warn!(target: LOG_TARGET, "Rejecting invalid transaction {}: {}", transaction_id, details); + JsonRpcResponse::error( + answer_id, + JsonRpcError::new( + JsonRpcErrorReason::ApplicationError(400), + format!("Transaction {} is invalid: {}", transaction_id, details), + json!({ "transaction_id": transaction_id, "details": details }), + ), + ) + },applications/tari_indexer/src/transaction_manager/mod.rs (1)
65-72: Avoid double hash calc and add a warning for visibilityMinor optimization + observability; no behavior change.
- if !transaction.verify_all_signatures() { - // DEV note: If signatures are invalid here (but they should be valid), this probably indicates an issue - // with the JSON decoding (crates/engine_types/src/argument_parser.rs) - return Err(TransactionManagerError::InvalidTransaction { - transaction_id: transaction.calculate_id(), - details: "Transaction has one or more invalid signature(s)".to_string(), - }); - } + if !transaction.verify_all_signatures() { + // DEV note: If signatures are invalid here (but they should be valid), this probably indicates an issue + // with the JSON decoding (crates/engine_types/src/argument_parser.rs) + let tx_id = transaction.calculate_id(); + log::warn!(target: "tari::indexer::transaction_manager", "Rejecting invalid transaction {} due to invalid signatures", tx_id); + return Err(TransactionManagerError::InvalidTransaction { + transaction_id: tx_id, + details: "Transaction has one or more invalid signature(s)".to_string(), + }); + }crates/template_lib/src/models/stealth.rs (2)
49-54: Tighten assertion messages (zero is allowed)“Must be positive” is inaccurate since zero is permitted when inputs exist. Update messages for clarity.
- assert!(!revealed_amount.is_negative(), "Revealed amount must be positive"); + assert!(!revealed_amount.is_negative(), "Revealed amount must be non-negative"); assert!( - !inputs.is_empty() || !revealed_amount.is_zero(), - "At least one input or a revealed amount must be provided" + !inputs.is_empty() || !revealed_amount.is_zero(), + "At least one input or a non-zero revealed amount must be provided" );
49-63: Prefer fallible constructor over panicking asserts for user inputsIf invalid data can come from users, return Result to avoid panics in host apps.
Example (add alongside new):
pub fn new_checked(inputs: Vec<StealthInput>, revealed_amount: Amount) -> Result<Self, &'static str> { if revealed_amount.is_negative() { return Err("Revealed amount must be non-negative"); } if inputs.is_empty() && revealed_amount.is_zero() { return Err("At least one input or a non-zero revealed amount must be provided"); } Ok(Self { inputs, revealed_amount }) }Want me to wire this through call sites and TS bindings?
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/MyAssets.tsx (1)
130-130: Ensure refetch when ownerPublicKey changesThe
Transactionscomponent’suseEffectcurrently only depends onaccount, so it won’t automatically refetch when the signer key updates. AddingownerPublicKeyto the dependency array will trigger a reload of the transaction list whenever the key changes.• In
applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsxuseEffect(() => { refetch(); - }, [account]); + }, [account, ownerPublicKey]);• Verified that all wallet-UI call sites already pass
ownerPublicKey:
TransactionsLayout.tsx(line 38)Wallet.tsx(line 51)AssetVault/Components/MyAssets.tsx(line 130)
• TheBlockDetails.tsxin the validator-node UI imports a differentTransactions(which takes atransactionsprop), so no change is needed there.applications/tari_indexer/web_ui/src/routes/Transaction/components/FeeInformation.tsx (1)
95-95: Format overcharge consistently with other amounts.Use formatXTM for total_fee_overcharge to keep units/formatting consistent and avoid raw bigint rendering.
Apply:
- <DataTableCell>{formatXTM(total_fees_paid)}{total_fee_overcharge > 0 ? ` Overcharge: ${total_fee_overcharge}` : ""}</DataTableCell> + <DataTableCell> + {formatXTM(total_fees_paid)} + {total_fee_overcharge > 0 && <> Overcharge: {formatXTM(total_fee_overcharge)}</>} + </DataTableCell>applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx (1)
146-161: Harden BigInt rendering and avoid optional-chaining edge cases.Guard toString calls and stringify overcharge to prevent rare undefined method access and ensure consistent output.
Apply:
- {feeReceipt?.total_fees_paid.toString() || 0} + {feeReceipt ? feeReceipt.total_fees_paid.toString() : "0"} {feeReceipt?.total_fee_overcharge ? ( <> {" "} - ({feeReceipt.total_fee_overcharge} overcharge{" "} + ({feeReceipt.total_fee_overcharge.toString()} overcharge{" "} <BsQuestionCircle style={{ display: "inline" }} title="An overcharge occurs when paying more fees than required using stealth transfers. To preserve privacy, there is no vault to refund excess fees, therefore the fees are given to validators in their entirety." /> ) </> ) : ( "" )}bindings/src/wallet-daemon-client.ts (1)
26-26: Clarify coexistence of ClaimBurnProof and ExtClaimBurnProof to avoid consumer confusion.Both exports are needed (Ext wraps ClaimBurnProof), but consider marking ClaimBurnProof as deprecated at the barrel to steer new consumers.
Apply this minimal annotation near the export to signal intent:
+// DEPRECATED: Prefer ExtClaimBurnProof; kept for backward compatibility. export * from "./types/wallet-daemon-client/ClaimBurnProof";Also applies to: 107-107
crates/wallet/sdk/tests/confidential_output_api.rs (1)
167-169: Vault configured for stealth resource/typeUsing STEALTH_TARI_RESOURCE_ADDRESS with ResourceType::Stealth matches the new XTR semantics. Consider renaming the test file and API naming in future cleanup to avoid “confidential” vs “stealth” confusion.
applications/tari_walletd/web_ui/src/api/hooks/useKeys.tsx (1)
41-49: Invalidate the exact branch to avoid broad cache churnInvalidating ["keys_list"] works due to partial matching, but targeting the specific branch avoids unnecessary refetches in other tabs.
Apply:
-export const useKeysCreate = (branch: KeyBranch) => { - return useMutation(() => keysCreate({ branch, specific_index: null }), { +export const useKeysCreate = (branch: KeyBranch) => { + return useMutation(() => keysCreate({ branch, specific_index: null }), { onError: (error: ApiError) => { error; }, onSuccess: () => { - queryClient.invalidateQueries(["keys_list"]); + queryClient.invalidateQueries({ queryKey: ["keys_list", branch] }); }, }); };crates/template_lib/src/constants.rs (1)
23-24: Optional: add a deprecated alias for smoother migrationI’ve confirmed that
CONFIDENTIAL_TARI_RESOURCE_ADDRESSdoes not exist elsewhere in the codebase, so introducing it will not conflict with any existing references.Please consider adding the following in
crates/template_lib/src/constants.rsimmediately after line 24:pub const XTR: ResourceAddress = STEALTH_TARI_RESOURCE_ADDRESS; + +#[deprecated(note = "Use STEALTH_TARI_RESOURCE_ADDRESS")] +pub const CONFIDENTIAL_TARI_RESOURCE_ADDRESS: ResourceAddress = STEALTH_TARI_RESOURCE_ADDRESS;• Location: crates/template_lib/src/constants.rs (after the
XTRconstant)
• Purpose: provide a temporary alias to help downstreams migrate without breaking changes; can be removed in the next breaking release.crates/transaction/src/v1/transaction.rs (1)
200-204: Good: centralized stealth weight logic.Helper improves consistency and reuse across instructions. Consider documenting the 2x output cost rationale next to the TODO.
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimBurn.tsx (2)
108-112: Validate fee as a finite number before RPC call.Avoid sending NaN/invalid values; provide immediate UX feedback.
Apply this diff:
- const resp = await accountsClaimBurn({ - account: { ComponentAddress: claimBurnFormState.account }, - claim_proof: JSON.parse(claimBurnFormState.claimProof), - max_fee: +claimBurnFormState.fee, - }); + const maxFee = Number.parseFloat(claimBurnFormState.fee); + if (!Number.isFinite(maxFee)) throw new Error("Invalid fee"); + const resp = await accountsClaimBurn({ + account: { ComponentAddress: claimBurnFormState.account }, + claim_proof: JSON.parse(claimBurnFormState.claimProof), + max_fee: maxFee, + });
113-116: Prevent default form submit to avoid page reload/double-submit.Current handler doesn’t stop
submission. Accept the event and call preventDefault.Outside selected lines, adjust the handler signature and onSubmit:
// 1) Change signature const onClaimBurn = async (e?: React.FormEvent) => { e?.preventDefault(); try { // ... } catch (e) { /* ... */ } }; // 2) Ensure Form uses it <Form onSubmit={onClaimBurn} /* ... */>clients/wallet_daemon_client/src/lib.rs (1)
323-334: New API: accounts_create_diffie_hellman_key is well-shaped.Good use of Into and consistent send_request wiring to accounts.create_dh_key_for_account.
Add a brief rustdoc comment describing expected server behavior and returned nonce_key_index semantics.
applications/tari_swarm_daemon/src/process_manager/manager.rs (1)
599-609: Nit: avoid shadowingwallet(daemon vs console wallet).Shadowing hampers readability. Consider renaming to
wallet_daemonandconsole_wallet.Example:
let wallet_daemon = self.instance_manager.get_wallet_daemon_mut(wallet_instance_id).ok_or_else(...)?; // ... let console_wallet = self.instance_manager.minotari_wallets().next().ok_or_else(...)?;applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs (1)
50-60: LGTM; consider optional WebAuthn pass-through for future parity.If some environments require WebAuthn, you may want an overload or parameter to pass
WebauthnFinishAuthRequestthroughconnect_client(...).applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (1)
48-52: Consider including request params in the React Query key to avoid cache collisions.
useGetAllTransactionsuses a static["transactions"]key. Include{status, component, signer_public_key}to isolate caches across accounts/signers.Proposed update in
useTransactions.tsx:export const useGetAllTransactions = (req: TransactionGetAllRequest) => { return useQuery({ queryKey: ["transactions", req.status, req.component, req.signer_public_key], queryFn: () => transactionsGetAll(req), // ... }); };crates/engine_types/src/instruction.rs (1)
43-45: TODO note in CallMethod args — consider linking an issue or removing before merge.crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
161-175: Delete lock vs. releasing/finalizing outputs: reverse the order to avoid FK/invariant issues.Safer to release/finalize outputs before deleting the lock to prevent transient “dangling lock_id” constraints inside the same TX.
Apply:
pub fn release_locked_outputs(&self, lock_id: OutputLockId) -> Result<(), StealthOutputsApiError> { self.store.with_write_tx(|tx| { - tx.output_locks_delete(lock_id)?; - tx.outputs_release_by_lock_id(lock_id)?; + tx.outputs_release_by_lock_id(lock_id)?; + tx.output_locks_delete(lock_id)?; Ok(()) }) } pub fn finalize_outputs(&self, lock_id: OutputLockId) -> Result<(), StealthOutputsApiError> { self.store.with_write_tx(|tx| { - tx.output_locks_delete(lock_id)?; - tx.stealth_outputs_finalize_by_lock_id(lock_id)?; + tx.stealth_outputs_finalize_by_lock_id(lock_id)?; + tx.output_locks_delete(lock_id)?; Ok(()) }) }
177-222: Crypto path consistency + key hygiene.You derive the shared decrypt key via kdfs::encrypted_data_dh_kdf_aead and then use crypto_api.extract_value_and_mask. Consider centralizing the DH+KDF step behind StealthCryptoApi for a single source of truth and to ease future KDF changes. Also ensure shared_decrypt_key is zeroized after use.
Example:
- let shared_decrypt_key = kdfs::encrypted_data_dh_kdf_aead(&decryption_key_part.key, &nonce); - let (_, mask) = self.crypto_api.extract_value_and_mask(&shared_decrypt_key, &output.commitment, &output.encrypted_data)?; + let shared_decrypt_key = self.crypto_api.derive_encrypted_data_key(&decryption_key_part.key, &nonce)?; + let (_, mask) = self.crypto_api.extract_value_and_mask(&shared_decrypt_key, &output.commitment, &output.encrypted_data)?; + // ensure zeroize(drop) or limited scope for shared_decrypt_keycrates/wallet/storage_sqlite/tests/transaction.rs (1)
14-21: Use an explicit constructor for test key to avoid implicit conversions.Relying on RistrettoSecretKey::from(123) may be brittle. Prefer a helper like from_u64/new_deterministic or random with a seeded RNG for clarity.
Example:
- let key = RistrettoSecretKey::from(123); + let key = RistrettoSecretKey::from(123u64); # or provide a small helper if available, e.g., RistrettoSecretKey::from_u64(123)crates/p2p/proto/transaction.proto (1)
99-101: Model PAY_FEE payload as a oneof to enforce invariants.pay_fee_stealth_transfer_statement and pay_fee_revealed_input_bucket are free-standing fields; nothing prevents them from coexisting with other instruction payloads. A oneof per-instruction payload enforces exclusivity at the schema level.
Sketch:
oneof pay_fee_payload { StealthTransferStatement pay_fee_stealth_transfer_statement = 29; // Optionally include revealed bucket ref inside the same message to keep them coupled WorkspaceOffsetId pay_fee_revealed_input_bucket = 30; }crates/template_builtin/templates/faucet/src/lib.rs (1)
23-46: Rename logs/flags from “confidential” to “stealth” and return semantics.The method is now a stealth transfer; align log/event keys to avoid confusion in tooling. Also document that Option returns Some when revealed_output_amount > 0.
Apply:
- debug!("Withdrawing {} coins from faucet into confidential output", amount); + debug!("Withdrawing {} coins from faucet into stealth output", amount); emit_event("take", [ ("amount", amount.to_string()), - ("confidential", "true".to_string()), + ("stealth", "true".to_string()), ("signer", signer.to_string()), ]);applications/tari_walletd/src/services/transaction_service/service.rs (1)
203-235: fetch_all signature updates and per-tx notifications: LGTM; consider DRYing event emissionBoth paths correctly notify Submitted vs Invalid. You could centralize the “submit and notify” into a small helper to avoid duplication.
Also applies to: 244-255
integration_tests/src/wallet_daemon_cli.rs (1)
92-93: Avoid cloning account_name if the API accepts a borrow.
If accounts_create_diffie_hellman_key accepts &str/AsRef, prefer borrowing to avoid needless allocs.- let resp = client.accounts_create_diffie_hellman_key(account_name.clone()).await?; + let resp = client.accounts_create_diffie_hellman_key(account_name.as_str()).await?;crates/wallet/sdk/src/apis/transaction.rs (1)
161-170: Added signer_public_key filter plumbs through to storage.
Good addition. Consider adding unit tests covering combinations of (status, component, signer).I can draft tests exercising each filter combination if useful.
crates/wallet/storage_sqlite/src/reader.rs (1)
249-265: LIKE on comma/JSON-encoded lists risks false positives; make matches boundary-aware.
If referenced_components/signers are stored as JSON arrays, match quoted values to avoid substring hits; otherwise consider delimiters or normalization.- query = query.filter( - transactions::referenced_components - .like(format!("%{}%", component)) - .or(transactions::signers.like(format!("%{}%", serialize_hex(public_key)))), - ); + query = query.filter( + transactions::referenced_components + .like(format!("%\"{}\"%", component)) + .or(transactions::signers.like(format!("%\"{}\"%", serialize_hex(public_key)))), + ); ... - query = query.filter(transactions::referenced_components.like(format!("%{}%", component))); + query = query.filter(transactions::referenced_components.like(format!("%\"{}\"%", component))); ... - query = query.filter(transactions::signers.like(format!("%{}%", serialize_hex(public_key)))); + query = query.filter(transactions::signers.like(format!("%\"{}\"%", serialize_hex(public_key))));Longer term, consider normalizing to join tables or using SQLite JSON1 (json_each) for indexed membership queries.
crates/wallet/storage_sqlite/src/schema.rs (1)
173-190: Schema change to JSON-backed transactions; ensure supporting indexes exist.
Add a UNIQUE index on transaction_id and secondary indexes on (status, created_at). Given new filters, consider indexes on signers and referenced_components (JSON1/FTS/trigram as appropriate).Please confirm the corresponding migration adds:
- UNIQUE INDEX idx_transactions_txid ON transactions(transaction_id)
- INDEX idx_transactions_status_created_at ON transactions(status, created_at)
- INDEX idx_transactions_signers ON transactions(signers)
- INDEX idx_transactions_ref_components ON transactions(referenced_components)
clients/wallet_daemon_client/src/types.rs (3)
207-210: Parameter naming nit: align with SDK’s fetch_all(signed_by_public_key).
Consider renaming signer_public_key → signed_by_public_key for cross-layer consistency.- pub signer_public_key: Option<RistrettoPublicKeyBytes>, + pub signed_by_public_key: Option<RistrettoPublicKeyBytes>,
456-469: New AccountsCreateDiffieHellmanKey types look good; minor naming consistency.*
To mirror ExtClaimBurnProof.owner_dh_nonce_key_index, consider dh_nonce_key_index here too.- pub nonce_key_index: u64, + pub dh_nonce_key_index: u64,Changing this is a wire break; proceed only if server/clients haven’t shipped yet.
599-606: ExtClaimBurnProof shape is correct; consider BigInt for large indices in TS.
u64 encoded as TS number can lose precision > 2^53-1. If indices may grow large, prefer bigint in bindings.- #[cfg_attr(feature = "ts", ts(type = "number"))] + #[cfg_attr(feature = "ts", ts(type = "bigint"))] pub owner_dh_nonce_key_index: u64,crates/template_builtin/templates/account/src/lib.rs (2)
82-85: Method name/docs imply confidential-only; clarify or guard for non-confidential.If
Vault::commitment_count()panics or is meaningless for Stealth/Fungible/NFT, either:
- rename to
confidential_commitment_counteverywhere and enforce resource type, or- guard and return 0/err for non-confidential resources.
Add an explicit type check to avoid surprising panics.
pub fn confidential_commitment_count(&self, resource: ResourceAddress) -> u32 { - self.get_vault(resource).commitment_count() + let v = self.get_vault(resource); + assert!(v.is_confidential(), "commitment_count only valid for confidential vaults"); + v.commitment_count() }
105-115: Event payload size could explode for large NFT batches.Joining potentially large
nf_idsinto a single event field can bloat logs. Consider truncating, batching, or emitting a count + first N IDs.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
150-159: Consider renamingoutput_locks.transaction_hashtotransaction_id.This table still uses
transaction_hash, which diverges from the new canonicaltransaction_id. Align naming to avoid confusion and query bugs.- transaction_hash TEXT NULL, + transaction_id TEXT NULL,crates/engine/src/runtime/fee_state.rs (1)
41-55: Improve InvalidAmount messages and consider rejecting zero amounts.Fix typos and consider treating zero as invalid input to fail fast rather than carrying zero-value payments.
- return Err(RuntimeError::InvalidAmount { + return Err(RuntimeError::InvalidAmount { amount: resource_container.amount(), - reason: "Payed an invalid amount. Amount must be positive and not overflow".to_string(), + reason: "Paid an invalid amount. Amount must be positive and not overflow".to_string(), }); @@ - return Err(RuntimeError::InvalidAmount { + return Err(RuntimeError::InvalidAmount { amount: resource_container.amount(), - reason: "Payed an invalid amount. Amount overflowed".to_string(), + reason: "Paid an invalid amount. Amount overflowed".to_string(), });If zero should be invalid:
- match self.running_total.checked_add(amount) { + if amount == 0 { + return Err(RuntimeError::InvalidAmount { amount: resource_container.amount(), reason: "Paid zero fees".to_string() }); + } + match self.running_total.checked_add(amount) {crates/transaction/src/builder/mod.rs (1)
291-299: Fee-builder TODO: track as an issue.A dedicated fee-builder type will prevent misuse of non-fee methods in fee context. Suggest opening a follow-up ticket and linking here.
crates/p2p/src/conversions/transaction.rs (1)
817-831: Fix nit: inconsistent error label (“output_statement” vs “outputs_statement”).This improves debuggability.
- .ok_or_else(|| anyhow!("output_statement not provided"))? + .ok_or_else(|| anyhow!("outputs_statement not provided"))?crates/engine/src/runtime/working_state.rs (1)
1426-1531: execute_stealth_transfer: solid validations + correct mint/spend semantics.
- Limits checked.
- Resource resolved and type-gated.
- Optional revealed_funds_bucket validated (address/amount).
- Inputs are downed; outputs are created; revealed amount returned as a stealth container.
Minor optional: ensure resource_lock is always unlocked via guard if future edits add early returns past lock.
crates/engine/src/transaction/processor.rs (1)
376-388: Tiny DRY opportunity: factor bucket resolution.Both pay_fee and stealth_transfer do identical workspace BucketId decoding. Consider a small private helper to reduce duplication.
+fn resolve_bucket_from_workspace(runtime: &Runtime, id: WorkspaceOffsetId) -> Result<tari_template_lib::models::BucketId, RuntimeError> { + runtime.resolve_workspace_id(&id).and_then(|r| { + r.decode().map_err(|e| RuntimeError::InvalidArgument { + argument: "revealed_funds_bucket", + reason: format!("Expected workspace id {id} to be a BucketId: {e}"), + }) + }) +}crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
183-212: Preimage order verified – matches engine implementation; optional rename suggestedI’ve confirmed that the SDK’s
validate_burn_claim_ownership_proofuses the same preimage ordering as the engine’s claim logic:
- Engine (crates/engine/src/runtime/impl.rs lines 2311–2314):
ownership_proof_hasher64(...).chain(public_nonce).chain(commitment).chain(transaction_signer_public_key)- SDK (crates/wallet/sdk/src/apis/stealth_crypto.rs lines 185–188):
ownership_proof_hasher64(...).chain(public_nonce).chain(commitment).chain(account_owner_pk)Since they align, no doc updates are required. As a minor nit for naming consistency, feel free to rename:
- pub fn validate_burn_claim_ownership_proof(...) + pub fn validate_claim_burn_ownership_proof(...)
| Instruction::StealthTransfer { statement, .. } => calc_stealth_statement_weight(statement), | ||
| Instruction::PayFee { statement, .. } => calc_stealth_statement_weight(statement), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Account for revealed_input_bucket in instruction weight.
StealthTransfer/PayFee currently ignore the optional revealed_input_bucket, which under-costs transactions that read from workspace. Add +1 when present to mirror arg-from-workspace costing.
Apply this diff:
- Instruction::StealthTransfer { statement, .. } => calc_stealth_statement_weight(statement),
- Instruction::PayFee { statement, .. } => calc_stealth_statement_weight(statement),
+ Instruction::StealthTransfer { statement, revealed_input_bucket, .. } => {
+ calc_stealth_statement_weight(statement) + revealed_input_bucket.as_ref().map(|_| 1).unwrap_or(0)
+ },
+ Instruction::PayFee { statement, revealed_input_bucket, .. } => {
+ calc_stealth_statement_weight(statement) + revealed_input_bucket.as_ref().map(|_| 1).unwrap_or(0)
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Instruction::StealthTransfer { statement, .. } => calc_stealth_statement_weight(statement), | |
| Instruction::PayFee { statement, .. } => calc_stealth_statement_weight(statement), | |
| } | |
| Instruction::StealthTransfer { statement, revealed_input_bucket, .. } => { | |
| calc_stealth_statement_weight(statement) | |
| + revealed_input_bucket.as_ref().map(|_| 1).unwrap_or(0) | |
| }, | |
| Instruction::PayFee { statement, revealed_input_bucket, .. } => { | |
| calc_stealth_statement_weight(statement) | |
| + revealed_input_bucket.as_ref().map(|_| 1).unwrap_or(0) | |
| }, |
🤖 Prompt for AI Agents
In crates/transaction/src/v1/transaction.rs around lines 195 to 197, the
Instruction::StealthTransfer and Instruction::PayFee match arms call
calc_stealth_statement_weight(statement) but fail to account for the optional
revealed_input_bucket, under-costing transactions that read from workspace;
update both arms to add +1 to the returned weight when
statement.revealed_input_bucket.is_some() (i.e., compute
calc_stealth_statement_weight(statement) + if
statement.revealed_input_bucket.is_some() { 1 } else { 0 }) so the instruction
weight mirrors the arg-from-workspace costing.
014115b to
4559e74
Compare
4559e74 to
cdfb50b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/wallet/sdk/src/storage.rs (1)
155-161: Renamesigned_by_public_keytosigner_public_keyand document union-filter semanticsTo align naming across the SDK/RPC surface and clarify how combined filters behave, please apply the following mandatory refactors:
• crates/wallet/sdk/src/storage.rs
– Rename the third parameter intransactions_fetch_alltosigner_public_key
– Add rustdoc that (1) describes each filter, (2) calls out that when bothcomponentandsigner_public_keyare provided they are combined with an OR (union), and (3) suggests pagination in a follow-up if needed.• crates/wallet/storage_sqlite/src/reader.rs
– Update the impl signature to usesigner_public_key
– Retain the existingor(...)logic (union semantics), and consider adding an explicit comment in the code to signal that this is intentional.• crates/wallet/sdk/src/apis/transaction.rs (and any other call sites)
– Rename all occurrences ofsigned_by_public_keytosigner_public_keyto match the updated API.Example diff for the trait and its impl:
--- a/crates/wallet/sdk/src/storage.rs +++ b/crates/wallet/sdk/src/storage.rs @@ - /// Fetch all transactions matching optional filters. - fn transactions_fetch_all( + /// Fetch all transactions matching optional filters. + /// + /// Each filter is applied if provided: + /// - `status`: match only this status. + /// - `component`: match transactions referencing this component. + /// - `signer_public_key`: match transactions signed by this public key. + /// + /// If both `component` and `signer_public_key` are set, the query uses a **union** (OR) + /// between the two filters. Pagination (limit/offset) is not yet supported; + /// consider adding it in a future iteration. + fn transactions_fetch_all( &mut self, status: Option<TransactionStatus>, component: Option<ComponentAddress>, - signed_by_public_key: Option<RistrettoPublicKeyBytes>, + signer_public_key: Option<RistrettoPublicKeyBytes>, ) -> Result<Vec<WalletTransaction>, WalletStorageError>; --- a/crates/wallet/storage_sqlite/src/reader.rs @@ fn transactions_fetch_all( - &mut self, - status: Option<TransactionStatus>, - component: Option<ComponentAddress>, - signed_by_public_key: Option<RistrettoPublicKeyBytes>, + &mut self, + status: Option<TransactionStatus>, + component: Option<ComponentAddress>, + signer_public_key: Option<RistrettoPublicKeyBytes>, ) -> Result<Vec<WalletTransaction>, WalletStorageError> { use crate::schema::transactions; let mut query = transactions::table .into_boxed() .filter(transactions::dry_run.eq(false)); @@ if let Some(component) = component { - if let Some(public_key) = signed_by_public_key { + if let Some(public_key) = signer_public_key { // union semantics: match either component OR signer query = query.filter( transactions::referenced_components .like(format!("%{}%", component)) .or(transactions::signers.like(format!("%{}%", serialize_hex(public_key)))), ); } else { query = query.filter(transactions::referenced_components.like(format!("%{}%", component))); } } else if let Some(public_key) = signer_public_key { query = query.filter(transactions::signers.like(format!("%{}%", serialize_hex(public_key)))); }• Review all call sites in
crates/wallet/sdk/src/apis/transaction.rs(lines ~165–168) and updatesigned_by_public_key→signer_public_key.These changes will ensure consistent naming, document the union-filter behavior, and set us up for a future pagination enhancement.
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
218-237: Add FK for stealth_outputs.owner_account_id.owner_account_id should reference accounts(id) like outputs.account_id does, otherwise deletes/rollbacks can orphan stealth outputs.
Apply this diff:
CREATE TABLE stealth_outputs ( id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, - owner_account_id INTEGER NOT NULL, + owner_account_id INTEGER NOT NULL REFERENCES accounts (id) ON DELETE CASCADE, resource_address TEXT NOT NULL, commitment TEXT NOT NULL, value TEXT NOT NULL, sender_public_nonce TEXT NOT NULL,crates/wallet/storage_sqlite/src/models/transaction.rs (2)
58-59: Avoid lossy cast from i64 to u64 for final_fee.
as u64can turn negative DB values into huge fees. Use TryFrom and surface a decode error instead of silently corrupting.Apply this diff:
- final_fee: self.final_fee.map(|f| f as u64), + final_fee: self + .final_fee + .map(|f| u64::try_from(f)) + .transpose() + .map_err(|e| WalletStorageError::DecodingError { + operation: "transaction_get", + item: "final_fee", + details: e.to_string(), + })?,
1-21: Immediate Action Required: Replace all lingeringtransaction_hashreferences withtransaction_idThe migration to
transaction_idis incomplete—these leftovers will break at runtime. Please update the following locations:
- integration_tests/src/wallet_daemon_cli.rs
• Line 304: change
transaction_id: resp.result.transaction_hash.into_array().into(),
to use the newtransaction_idfield.- crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql
• Line 156: rename thetransaction_hashcolumn totransaction_id.- crates/wallet/storage_sqlite/src/models/proof.rs
• Line 15: rename struct field
pub transaction_hash: Option<String>,
→pub transaction_id: Option<String>,- crates/wallet/storage_sqlite/src/schema.rs
• Line 81: update
transaction_hash -> Nullable<Text>,
→transaction_id -> Nullable<Text>,- crates/wallet/storage_sqlite/src/reader.rs
• Line 930: replace
.filter(output_locks::transaction_hash.eq(transaction_id.to_string()))
with
.filter(output_locks::transaction_id.eq(transaction_id.to_string()))- crates/wallet/storage_sqlite/src/writer.rs
• Lines 1264 & 1283: rename the field inUpdateOutputLockfromtransaction_hashtotransaction_idand map accordingly.- crates/wallet/sdk/src/apis/substate.rs
• Line 130: adapt any use oftx_receipt.transaction_hashif the SDK’s transaction model has been renamed.- applications/tari_validator_node_cli/src/command/transaction.rs
• Line 79: change the CLI struct field from
transaction_hash: FromHex<TransactionId>,
→transaction_id: FromHex<TransactionId>,
• Line 164: update anyprintln!or help text referencingtransaction_hash.crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
631-637: Signature added with mismatched public key and secret key.You sign with src_account_secret but declare fee_account.owner_public_key as the sealed signer. This will fail signature validation.
- if inputs_to_spend.revealed.is_positive() && params.owner_account.owner_public_key != *fee_account.owner_public_key() { - builder.add_signature(fee_account.owner_public_key(), &src_account_secret.key) + if inputs_to_spend.revealed.is_positive() + && params.owner_account.owner_public_key != *fee_account.owner_public_key() + { + builder.add_signature(¶ms.owner_account.owner_public_key, &src_account_secret.key) } else {crates/engine/src/runtime/working_state.rs (1)
982-1059: Missing insufficient-fees guard in finalize_fees()If remaining_fees > 0 after collecting payments, the transaction should fail. Currently it proceeds and reports partial payment.
Apply this patch just before constructing TransactionReceipt:
@@ - // Refund the remaining refundable payments if any + // Refund the remaining refundable payments if any for (mut resx, refund_vault) in self.fee_state.drain_refundable_fee_payments() { if resx.amount().is_zero() { debug_assert!(!resx.amount().is_negative()); continue; } @@ vault_mut.resource_container_mut().deposit(resx.withdraw_all()?)?; } + // Fail if any fees remain unpaid + if remaining_fees > 0 { + return Err(RuntimeError::InsufficientFeesPaid { + required_fee: total_fees, + fees_paid: total_fees.saturating_sub(remaining_fees), + }); + } + Ok(TransactionReceipt {
♻️ Duplicate comments (15)
applications/tari_walletd/src/services/transaction_service/service.rs (1)
157-171: Event-driven invalid submissions acknowledged (Ok(id) + TransactionInvalidEvent).This preserves the semantic change called out earlier; callers must observe events or check status instead of relying on Err. Looks good.
bindings/src/types/wallet-daemon-client/TransactionGetAllRequest.ts (1)
6-10: Type extended with signer_public_key — looks good; ensure callers pass null explicitly.The addition aligns with Rust Option mapping. Ensure all TransactionGetAllRequest literals include signer_public_key to avoid TS errors.
Run to spot inline literals missing signer_public_key:
#!/bin/bash rg -nP 'transactionsGetAll\\(\\s*{|' -C2 applications rg -nP '\\.transactionsList\\(\\s*{|' -C2 applications rg -nP 'useGetAllTransactions\\(\\s*{|' -C2 applicationsapplications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (2)
53-55: Include ownerPublicKey in refetch deps (prevents stale results).Without it, changing signer won’t refresh.
Apply:
useEffect(() => { refetch(); -}, [account]); +}, [account, ownerPublicKey]);Additionally, prefer fixing cache-keying in the hook so manual refetch isn’t needed (see note below).
93-93: Possible runtime error: toString on undefined.fee_receipt?.total_fees_paid may be undefined; calling .toString() will throw.
Apply:
- <DataTableCell>{fee_receipt?.total_fees_paid.toString() || "--"}</DataTableCell> + <DataTableCell>{fee_receipt?.total_fees_paid?.toString() ?? "--"}</DataTableCell>crates/template_builtin/templates/account/src/lib.rs (1)
188-196: Avoid panic in stealth fee path; lazily create/open the vault (and consider addingresourceto the event).Same panic risk as above; fix by open-or-create before calling
pay_fee_stealth. Optionally include the resource address in the event for consistency with other events.- emit_event("pay_fee", [ - ("stealth", "true".to_string()), - ("num_inputs", transfer.inputs_statement.inputs.len().to_string()), - ]); - self.get_vault_mut(STEALTH_TARI_RESOURCE_ADDRESS) - .pay_fee_stealth(transfer); + emit_event("pay_fee", [ + ("stealth", "true".to_string()), + ("num_inputs", transfer.inputs_statement.inputs.len().to_string()), + ("resource", STEALTH_TARI_RESOURCE_ADDRESS.to_string()), + ]); + let vault_mut = self + .vaults + .entry(STEALTH_TARI_RESOURCE_ADDRESS) + .or_insert_with(|| Vault::new_empty(STEALTH_TARI_RESOURCE_ADDRESS)); + vault_mut.pay_fee_stealth(transfer);utilities/tariswap_test_bench/src/accounts.rs (2)
59-61: LGTM: registering the XTR vault as Stealth is correct.This aligns with the PR migration and ensures the wallet tracks the XTR vault under the Stealth resource type.
59-61: Ensure downstream vault/resource-type checks accept Stealth (repeat).Bench/template code that only allows Fungible|Confidential will panic when encountering Stealth. Please extend those checks. Example fix in utilities/tariswap_test_bench/templates/tariswap/src/lib.rs:
- assert!( - matches!(resource_type, ResourceType::Fungible | ResourceType::Confidential), - "Resource {} is not fungible nor confidential", - resource - ); + assert!( + matches!(resource_type, ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth), + "Resource {} is not fungible, confidential, or stealth", + resource + );Quick scan to verify remaining callsites:
#!/bin/bash set -euo pipefail # Find assertions/branches that still exclude Stealth rg -nP -C2 'matches!\s*\([^)]*ResourceType::Fungible\s*\|\s*ResourceType::Confidential' utilities/ templates/ || true rg -n 'not fungible.*confidential|Confidential[^|]\s*$' utilities/ templates/ -S || true # Audit remaining explicit Confidential-only branches rg -n 'ResourceType::Confidential' -g '!**/target/**'crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
151-159: Legacy transaction_hash lingers; rename to transaction_id and add FK.output_locks still has transaction_hash, which will break joins/lookups now that transactions are keyed by transaction_id. Tie it to the unique transactions(transaction_id) to keep referential integrity.
Apply this diff:
CREATE TABLE output_locks ( id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, resource_address TEXT NOT NULL, vault_id INTEGER NULL REFERENCES vaults (id), - transaction_hash TEXT NULL, + transaction_id TEXT NULL REFERENCES transactions (transaction_id), locked_revealed_amount BIGINT NOT NULL DEFAULT 0, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ); + +CREATE INDEX output_locks_idx_tx_id ON output_locks (transaction_id);crates/engine/tests/fees.rs (1)
133-142: Test bug: comparing account_fee2’s vault against account_fee’s orig_balance (repeat)Same issue previously flagged: baseline was captured for
account_fee, but assertion checksaccount_fee2’s vault. Captureaccount_fee2’s original balance and subtract only the portion it paid.Apply:
- let orig_balance: Amount = test.call_method( - account_fee, + let orig_balance_fee2: Amount = test.call_method( + account_fee2, "balance", call_args![STEALTH_TARI_RESOURCE_ADDRESS], vec![], ); @@ - assert_eq!( - vault.balance(), - orig_balance + Amount::from(200) - Amount::from(payment.total_fees_charged()) - ); + assert_eq!( + vault.balance(), + orig_balance_fee2 - Amount::from(payment.total_fees_charged() - 200) + );crates/engine/src/runtime/fee_state.rs (3)
31-39: Wrong argument label and message wording; fix user-facing error text.Use "resource_address" (not "vault_ref") and tighten the message.
- if *resource_container.resource_address() != XTR { - return Err(RuntimeError::InvalidArgument { - argument: "vault_ref", - reason: format!( - "Fees can only be paid using XTR, however the vault contained resource {}", - resource_container.resource_address() - ), - }); - } + if *resource_container.resource_address() != XTR { + return Err(RuntimeError::InvalidArgument { + argument: "resource_address", + reason: format!( + "Fees can only be paid using XTR, but got {}", + resource_container.resource_address() + ), + }); + }
41-55: Typos: “Payed” → “Paid” in error reasons.Update both occurrences.
- reason: "Payed an invalid amount. Amount must be positive and not overflow".to_string(), + reason: "Paid an invalid amount. Amount must be positive and not overflow".to_string(), ... - reason: "Payed an invalid amount. Amount overflowed".to_string(), + reason: "Paid an invalid amount. Amount overflowed".to_string(),
78-80: add_charge overwrites instead of accumulates; use entry API with saturating_add.Current insert drops prior charges for the same source.
- pub fn add_charge(&mut self, source: FeeSource, amount: u64) { - self.fee_charges.insert(source, amount) - } + pub fn add_charge(&mut self, source: FeeSource, amount: u64) { + self.fee_charges + .entry(source) + .and_modify(|v| *v = v.saturating_add(amount)) + .or_insert(amount); + }applications/tari_walletd/src/handlers/accounts.rs (2)
585-593: Missing validation for range_proof in claim-burn flow.Validate the range proof before proceeding to avoid processing malformed proofs (runtime also checks, but early rejection is preferable).
let reciprocal_claim_public_key_expanded = RistrettoPublicKey::try_from_byte_type(&reciprocal_claim_public_key) .map_err(|e| invalid_params("claim_proof.reciprocal_claim_public_key", Some(e)))?; +// Validate the range proof against the commitment +if !sdk + .stealth_crypto_api() + .validate_range_proof(network, &range_proof, &commitment) +{ + return Err(invalid_params( + "claim_proof.range_proof", + Some("range proof validation failed"), + )); +}
766-766: Outdated call: derive_account_keypair → derive_account_key.Aligns with earlier changes and avoids unused returned key.
- let (account_secret_key, account_public_key) = sdk.key_manager_api().derive_account_keypair(account.key_index())?; + let account_secret_key = sdk.key_manager_api().derive_account_key(account.key_index())?; + let account_public_key = RistrettoPublicKey::from_secret_key(&account_secret_key.key);crates/transaction/src/builder/mod.rs (1)
103-116: Wrong parameter type for stealth fee payment helper; should accept StealthTransferStatement.Passing ConfidentialWithdrawProof to pay_fee_stealth is incorrect.
- pub fn fee_transaction_pay_fees_stealth_from_component<A: Into<ComponentCall>>( + pub fn fee_transaction_pay_fees_stealth_from_component<A: Into<ComponentCall>>( self, call: A, - proof: ConfidentialWithdrawProof, + statement: StealthTransferStatement, ) -> Self { self.add_fee_instruction(Instruction::CallMethod { call: call.into(), method: "pay_fee_stealth".to_string(), - args: call_args![proof], + args: call_args![statement], }) }Also remove the now-unused
ConfidentialWithdrawProofimport from the models list at Line 26.- models::{ConfidentialWithdrawProof, ResourceAddress, StealthTransferStatement}, + models::{ResourceAddress, StealthTransferStatement},
🧹 Nitpick comments (27)
applications/tari_swarm_daemon/Cargo.toml (2)
26-26: Consider gating this dependency behind a Cargo feature to keep builds lean.If the swarm daemon doesn’t always require wallet-SDK at runtime, make it optional and expose a feature (e.g.,
wallet-sdk) so CI and minimal builds can skip it.Apply this diff (adjust feature name if you prefer a different one):
[dependencies] ... -tari_ootle_wallet_sdk = { workspace = true } +tari_ootle_wallet_sdk = { workspace = true, optional = true } +[features] +# Enable wallet SDK-related integration points +wallet-sdk = ["dep:tari_ootle_wallet_sdk"] +# Keep default minimal unless you want it on by default: +default = []
26-26: Feature Gate Wallet SDK Dependency and Confirm No CyclesVerified that:
- The
tari_ootle_wallet_sdkcrate is actively used inapplications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs(e.g. theuse tari_ootle_wallet_sdk::apis::key_manager::KeyBranch;import)- No other workspace crate depends on
tari_swarm_daemon, so there’s no dependency cycleTo avoid pulling in the wallet SDK unnecessarily (or accidentally leaving it unused) and to keep compile times lean, it’s recommended to make the SDK dependency optional and gate its usage behind a feature flag.
Possible changes in applications/tari_swarm_daemon/Cargo.toml:
[dependencies] - tari_ootle_wallet_sdk = { workspace = true } + tari_ootle_wallet_sdk = { workspace = true, optional = true } +[features] + default = [] + wallet-sdk = ["tari_ootle_wallet_sdk"]Then, in your Rust code (e.g. in
wallet_daemon.rs), wrap imports and implementations with the feature gate:#[cfg(feature = "wallet-sdk")] use tari_ootle_wallet_sdk::apis::key_manager::KeyBranch; #[cfg(feature = "wallet-sdk")] pub fn spawn_wallet_daemon() { /* ... */ }Let me know if you’d like assistance wiring up these feature flags and annotating the relevant code paths!
crates/wallet/sdk/src/models/key.rs (2)
35-37: Redundant accessor vs public field — consider consolidating API.
You now expose public_key both as a pub field and via a getter. Prefer one style for consistency. If the intent is encapsulation, consider making KeyPair fields non-pub and keep the accessors.
39-41: Expose secret key carefully; add docs or restrict visibility.
Returning a reference to the raw secret key is convenient but sensitive. At minimum, document intended use (signing only) and caution against logging/serialization. If external crates don’t need this yet, consider pub(crate) first.Apply this minimal doc addition:
- pub fn secret_key(&self) -> &RistrettoSecretKey { + /// Returns a reference to the private key for signing. Do not log or serialize. + pub fn secret_key(&self) -> &RistrettoSecretKey { &self.secret_key.key }applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (6)
49-54: Deduplicate branch literal and make it explicit.Avoid repeating the "account" literal; use a local constant so list/create always target the same branch.
function Keys() { const [showKeyDialog, setShowAddKeyDialog] = useState(false); - const { data, isLoading, isError, error } = useKeysList("account"); + const BRANCH = "account" as const; + const { data, isLoading, isError, error } = useKeysList(BRANCH); const { mutate: mutateSetActive } = useKeysSetActive(); - const { mutate: mutateCreateKey } = useKeysCreate("account"); + const { mutate: mutateCreateKey } = useKeysCreate(BRANCH);
23-23: Prevent form submission side-effects.onSubmitAddKey doesn’t call preventDefault. With react-router’s Form this can attempt a route action. Guard it explicitly.
-import { useState } from "react"; +import { useState, type FormEvent } from "react"; @@ -const onSubmitAddKey = () => { - mutateCreateKey(); - setShowAddKeyDialog(false); -}; +const onSubmitAddKey = (e: FormEvent) => { + e.preventDefault(); + mutateCreateKey(); + setShowAddKeyDialog(false); +};If this Form intentionally targets a route action, switch to a plain
or a div with an onClick handler instead.Also applies to: 63-66
35-35: Fix MUI import path for Button.Use the public entry to avoid brittle deep imports.
-import Button from "@mui/material/Button/Button"; +import Button from "@mui/material/Button";
39-47: Rename helper and tighten types to avoid confusion with React’s key prop.The helper named Key shadows the JSX key concept and uses any. Rename and type it.
-function Key(key: [number, string, boolean], setActive: any) { +function KeyRow(k: [number, string, boolean], setActive: (index: number) => void) { return ( - <TableRow key={key[0]}> - <DataTableCell>{key[0]}</DataTableCell> - <DataTableCell>{key[1]}</DataTableCell> - <DataTableCell>{key[2] ? <b>Active</b> : <div onClick={() => setActive(key[0])}>Activate</div>}</DataTableCell> + <TableRow key={k[0]}> + <DataTableCell>{k[0]}</DataTableCell> + <DataTableCell>{k[1]}</DataTableCell> + <DataTableCell>{k[2] ? <b>Active</b> : <div onClick={() => setActive(k[0])}>Activate</div>}</DataTableCell> </TableRow> ); } @@ - <TableBody>{data && data.keys.map((key: [number, string, boolean]) => Key(key, setActive))}</TableBody> + <TableBody>{data && data.keys.map((key: [number, string, boolean]) => KeyRow(key, setActive))}</TableBody>Also applies to: 104-104
44-44: Use a Button for “Activate” for a11y and consistency.Clicking a div isn’t accessible. Use MUI Button.
- <DataTableCell>{k[2] ? <b>Active</b> : <div onClick={() => setActive(k[0])}>Activate</div>}</DataTableCell> + <DataTableCell> + {k[2] ? ( + <b>Active</b> + ) : ( + <Button size="small" variant="text" onClick={() => setActive(k[0])}> + Activate + </Button> + )} + </DataTableCell>
51-53: Cache invalidation check.The hooks register queries with ["keys_list", branch], but onSuccess invalidates ["keys_list"] (see applications/tari_walletd/web_ui/src/api/hooks/useKeys.tsx). That relies on partial-key matching. If you’re on TanStack Query v4, prefer the object form to be explicit and future-proof:
// in useKeys.tsx (for reference) queryClient.invalidateQueries({ queryKey: ["keys_list"] }); // or, to be exact per-branch: queryClient.invalidateQueries({ queryKey: ["keys_list", branch] });applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (2)
45-45: Prop type can reflect optionality.ownerPublicKey is treated as optional; make the type optional to match usage.
-export default function Transactions({ account, ownerPublicKey }: { account: Account; ownerPublicKey: string }) { +export default function Transactions({ account, ownerPublicKey }: { account: Account; ownerPublicKey?: string | null }) {
48-52: Static React Query key risks cross-filter cache collisions.The hook uses queryKey: ["transactions"]. Include component and signer_public_key to segregate caches; then you can drop manual refetch.
Outside this file (in applications/tari_walletd/web_ui/src/api/hooks/useTransactions.tsx), change:
- return useQuery({ - queryKey: ["transactions"], + return useQuery({ + queryKey: ["transactions", req.component, req.signer_public_key, req.status],applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx (1)
49-49: Import is fine; consider Tooltip for better UX.Optional: wrap BsQuestionCircle with MUI Tooltip for consistency and accessibility.
crates/template_builtin/templates/account/src/lib.rs (1)
82-85: Clarify scope; avoid panic for read-only query.Doc says “Only applies to confidential resources” while stealth also uses commitments; either clarify the doc or make it generic. Also, using
get_vault(...)here will panic if the vault is missing. Returning 0 when absent is safer for an introspection API.- /// Only applies to confidential resources. Returns the number of commitments in the vault. - pub fn confidential_commitment_count(&self, resource: ResourceAddress) -> u32 { - self.get_vault(resource).commitment_count() - } + /// Returns the number of commitments in the vault. For a missing vault, returns 0. + /// Note: Applies to confidential/stealth resources; other types may return 0. + pub fn confidential_commitment_count(&self, resource: ResourceAddress) -> u32 { + self.vaults + .get(&resource) + .map(|v| v.commitment_count()) + .unwrap_or(0) + }bindings/src/types/TariStealthClaim.ts (3)
9-12: Prefer burn_public_nonce over burn_public_key for accuracyField name says “key” but the doc references the Schnorr public nonce. Rename for precision and to reduce integrator confusion. Upstream change required in the Rust struct so ts-rs regenerates this.
Apply after updating the Rust source (ts-rs will regenerate):
- /** - * This is typically the public nonce that the UTXO was burnt with - */ - burn_public_key: RistrettoPublicKeyBytes; + /** + * Single-use public claim nonce the UTXO was burned with. + */ + burn_public_nonce: RistrettoPublicKeyBytes;
8-16: Align address type naming with “stealth” terminologyoutput_address still references UnclaimedConfidentialOutputAddress. For consistency with the migration, consider introducing UnclaimedStealthOutputAddress and using it here. If you want a soft transition, keep a type alias for the old name.
Change in this file (after adding the new type file below):
-import type { UnclaimedConfidentialOutputAddress } from "./UnclaimedConfidentialOutputAddress"; +import type { UnclaimedStealthOutputAddress } from "./UnclaimedStealthOutputAddress"; @@ - output_address: UnclaimedConfidentialOutputAddress; + output_address: UnclaimedStealthOutputAddress;Add new file bindings/src/types/UnclaimedStealthOutputAddress.ts:
export type UnclaimedStealthOutputAddress = string;Optional backward-compat alias (in UnclaimedConfidentialOutputAddress.ts):
-export type UnclaimedConfidentialOutputAddress = string; +export type UnclaimedConfidentialOutputAddress = string; // Deprecated: use UnclaimedStealthOutputAddress
5-5: Type-safety nit: strengthen StealthTransferStatement.balance_proof bytesbalance_proof.public_nonce/signature are plain strings in StealthTransferStatement; other byte-like fields use aliases. Prefer RistrettoPublicKeyBytes (or a dedicated Nonce alias) for public_nonce to avoid accidental misuse.
In bindings/src/types/StealthTransferStatement.ts:
+import type { RistrettoPublicKeyBytes } from "./RistrettoPublicKeyBytes"; @@ - balance_proof: { public_nonce: string; signature: string }; + balance_proof: { public_nonce: RistrettoPublicKeyBytes; signature: string };If a signature alias exists (e.g., SchnorrSignatureBytes), use it instead of string.
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (3)
185-186: Index name says uniq but it isn’t unique.Either make it unique (if intended) or rename the index. Suggest making it unique to prevent duplicate NFT IDs across vaults unless that’s explicitly allowed.
Apply this diff if uniqueness is desired:
-CREATE INDEX nfts_uniq_address ON non_fungible_tokens (nft_id); +CREATE UNIQUE INDEX nfts_uniq_address ON non_fungible_tokens (nft_id);If duplicates across vaults are valid, rename to avoid confusion:
-CREATE INDEX nfts_uniq_address ON non_fungible_tokens (nft_id); +CREATE INDEX nfts_idx_nft_id ON non_fungible_tokens (nft_id);
142-142: Normalize BLOB casing for consistency.outputs.encrypted_data uses
blobwhile other tables useBLOB. Not functional, but consistency helps readability and tooling.Apply this diff:
- encrypted_data blob NOT NULL DEFAULT '', + encrypted_data BLOB NOT NULL DEFAULT '',
178-178: Normalize TEXT type casing.Minor style nit:
resource_id text→TEXT.Apply this diff:
- resource_id text NOT NULL, + resource_id TEXT NOT NULL,crates/wallet/storage_sqlite/src/models/transaction.rs (4)
25-31: Underscored fields can confuse Diesel mapping; consider explicit naming or annotation._referenced_components and _signers are public but intentionally unused. Either:
- Rename to referenced_components and signers and use them where needed, or
- Keep names but document with a comment and add Diesel column_name attributes if you move away from positional selects later.
Example (if adopting name-based mapping in future):
- pub _referenced_components: String, - pub _signers: String, + #[diesel(column_name = referenced_components)] + pub referenced_components: String, + #[diesel(column_name = signers)] + pub signers: String,
45-52: Prefer DB dry_run field or assert consistency with JSON-derived is_dry_run.You compute is_dry_run from the JSON and ignore self.dry_run. This can hide mismatches between stored flags and payload. Either use the DB field (faster, queryable) or assert/log if they diverge.
Apply one of the following diffs:
Option A — trust DB column:
- let is_dry_run = transaction.is_dry_run(); + let is_dry_run = self.dry_run;Option B — assert consistency:
let transaction = deserialize_json::<Transaction>(&self.transaction_json)?; let is_dry_run = transaction.is_dry_run(); + if is_dry_run != self.dry_run { + warn!(target: LOG_TARGET, "dry_run mismatch: json={}, db={}", is_dry_run, self.dry_run); + }
63-66: Treat negative executed_time_ms as None, not 0ms.Current logic coerces invalid negatives to 0ms, masking bad data. Prefer dropping the value when invalid.
Apply this diff:
- execution_time: self - .executed_time_ms - .map(|t| u64::try_from(t).map(Duration::from_millis).unwrap_or_default()), + execution_time: self + .executed_time_ms + .and_then(|t| u64::try_from(t).ok()) + .map(Duration::from_millis),
6-6: Nit: narrow log import.
use log::*;can be narrowed touse log::warn;since that’s the only symbol used here.-use log::*; +use log::warn;crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
599-605: Prefer accessor over field access for consistency and privacy.Use the AccountWithPublicKey accessor instead of reaching into .account.address directly.
- .call_method(params.owner_account.account.address, "withdraw", args![params.resource_address, inputs_to_spend.revealed]) + .call_method(*params.owner_account.address(), "withdraw", args![params.resource_address, inputs_to_spend.revealed])crates/engine/src/runtime/working_state.rs (2)
1426-1531: Explicitly burn revealed_funds_bucket to declare intentThe taken revealed_funds_bucket is dropped implicitly. Burning it explicitly improves clarity and consistency with burn semantics (and future-proofing for events/metrics).
Apply after validating the transfer, before creating outputs:
@@ - let valid_transfer = stealth::validate_transfer(&statement, view_key.as_ref())?; + let valid_transfer = stealth::validate_transfer(&statement, view_key.as_ref())?; + + // Consume the revealed input by burning the provided bucket explicitly + if let Some(bucket) = revealed_funds_bucket { + self.burn_bucket(bucket)?; + }
1446-1454: Auth action choice: Withdraw is acceptable; consider a dedicated StealthTransfer action laterUsing ResourceAuthAction::Withdraw is reasonable now. If policy needs to distinguish stealth transfers from regular withdrawals in future, a dedicated action would help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (99)
applications/tari_indexer/src/json_rpc/handlers.rs(1 hunks)applications/tari_indexer/src/transaction_manager/error.rs(1 hunks)applications/tari_indexer/src/transaction_manager/mod.rs(1 hunks)applications/tari_indexer/web_ui/src/routes/Transaction/components/FeeInformation.tsx(3 hunks)applications/tari_indexer/web_ui/src/routes/Transaction/components/Result.tsx(5 hunks)applications/tari_swarm_daemon/Cargo.toml(1 hunks)applications/tari_swarm_daemon/src/process_manager/manager.rs(3 hunks)applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs(2 hunks)applications/tari_validator_node/src/state_bootstrap.rs(4 hunks)applications/tari_wallet_cli/src/command/account.rs(1 hunks)applications/tari_wallet_cli/src/command/transaction.rs(2 hunks)applications/tari_walletd/src/handlers/accounts.rs(11 hunks)applications/tari_walletd/src/handlers/transaction.rs(2 hunks)applications/tari_walletd/src/lib.rs(2 hunks)applications/tari_walletd/src/services/account_monitor.rs(2 hunks)applications/tari_walletd/src/services/transaction_service/service.rs(5 hunks)applications/tari_walletd/web_ui/src/api/hooks/useAccounts.ts(0 hunks)applications/tari_walletd/web_ui/src/api/hooks/useKeys.tsx(1 hunks)applications/tari_walletd/web_ui/src/api/hooks/useTransactions.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/AccountDetails/AccountDetails.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimBurn.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/MyAssets.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx(4 hunks)applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx(3 hunks)applications/tari_walletd/web_ui/src/routes/Transactions/TransactionsLayout.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Wallet.tsx(2 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/FeeReceipt.ts(1 hunks)bindings/src/types/Instruction.ts(3 hunks)bindings/src/types/TariStealthClaim.ts(1 hunks)bindings/src/types/wallet-daemon-client/ClaimBurnRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeyBranch.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionGetAllRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionGetResponse.ts(1 hunks)bindings/src/wallet-daemon-client.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(3 hunks)crates/consensus_tests/src/support/transaction.rs(2 hunks)crates/engine/src/runtime/error.rs(1 hunks)crates/engine/src/runtime/fee_state.rs(1 hunks)crates/engine/src/runtime/impl.rs(13 hunks)crates/engine/src/runtime/mod.rs(3 hunks)crates/engine/src/runtime/state_store.rs(2 hunks)crates/engine/src/runtime/tracker.rs(2 hunks)crates/engine/src/runtime/working_state.rs(8 hunks)crates/engine/src/state_store/bootstrap.rs(2 hunks)crates/engine/src/transaction/processor.rs(6 hunks)crates/engine/tests/account.rs(1 hunks)crates/engine/tests/fees.rs(22 hunks)crates/engine/tests/templates/shenanigans/src/lib.rs(2 hunks)crates/engine/tests/test.rs(6 hunks)crates/engine_types/src/confidential/claim.rs(1 hunks)crates/engine_types/src/fees.rs(1 hunks)crates/engine_types/src/hashing.rs(1 hunks)crates/engine_types/src/instruction.rs(7 hunks)crates/engine_types/src/resource_container.rs(3 hunks)crates/engine_types/src/validator_fee.rs(2 hunks)crates/engine_types/src/vault.rs(1 hunks)crates/p2p/proto/transaction.proto(3 hunks)crates/p2p/src/conversions/transaction.rs(6 hunks)crates/state_store_tests/src/transactions.rs(2 hunks)crates/template_builtin/templates/account/src/lib.rs(3 hunks)crates/template_builtin/templates/faucet/src/lib.rs(2 hunks)crates/template_builtin/tests/nft_faucet.rs(2 hunks)crates/template_lib/src/args/types.rs(1 hunks)crates/template_lib/src/constants.rs(1 hunks)crates/template_lib/src/models/resource.rs(2 hunks)crates/template_lib/src/models/stealth.rs(1 hunks)crates/template_lib/src/models/vault.rs(2 hunks)crates/template_lib/src/prelude.rs(2 hunks)crates/template_test_tooling/src/builtin_component_state.rs(2 hunks)crates/template_test_tooling/src/template_test.rs(6 hunks)crates/template_test_tooling/templates/faucet/src/lib.rs(0 hunks)crates/transaction/src/builder/mod.rs(6 hunks)crates/transaction/src/transaction.rs(2 hunks)crates/transaction/src/v1/signature.rs(2 hunks)crates/transaction/src/v1/transaction.rs(2 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(6 hunks)crates/wallet/sdk/src/apis/key_manager.rs(3 hunks)crates/wallet/sdk/src/apis/stealth_crypto.rs(7 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(4 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(8 hunks)crates/wallet/sdk/src/apis/transaction.rs(4 hunks)crates/wallet/sdk/src/models/account.rs(1 hunks)crates/wallet/sdk/src/models/key.rs(2 hunks)crates/wallet/sdk/src/storage.rs(1 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(2 hunks)crates/wallet/storage_sqlite/Cargo.toml(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(1 hunks)crates/wallet/storage_sqlite/src/models/mod.rs(1 hunks)crates/wallet/storage_sqlite/src/models/transaction.rs(4 hunks)crates/wallet/storage_sqlite/src/reader.rs(2 hunks)crates/wallet/storage_sqlite/src/schema.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(3 hunks)crates/wallet/storage_sqlite/tests/transaction.rs(2 hunks)integration_tests/src/wallet_daemon_cli.rs(5 hunks)utilities/tariswap_test_bench/src/accounts.rs(1 hunks)
💤 Files with no reviewable changes (2)
- crates/template_test_tooling/templates/faucet/src/lib.rs
- applications/tari_walletd/web_ui/src/api/hooks/useAccounts.ts
✅ Files skipped from review due to trivial changes (1)
- crates/engine_types/src/hashing.rs
🚧 Files skipped from review as they are similar to previous changes (72)
- bindings/src/types/wallet-daemon-client/TransactionGetResponse.ts
- crates/transaction/src/v1/signature.rs
- crates/wallet/storage_sqlite/Cargo.toml
- crates/engine/tests/account.rs
- bindings/src/wallet-daemon-client.ts
- crates/engine/src/runtime/error.rs
- crates/template_lib/src/models/stealth.rs
- applications/tari_walletd/web_ui/src/api/hooks/useTransactions.tsx
- crates/engine_types/src/fees.rs
- crates/template_lib/src/prelude.rs
- crates/wallet/storage_sqlite/src/models/mod.rs
- applications/tari_indexer/src/transaction_manager/error.rs
- crates/template_test_tooling/src/template_test.rs
- bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts
- applications/tari_walletd/web_ui/src/routes/Wallet/Wallet.tsx
- applications/tari_wallet_cli/src/command/transaction.rs
- crates/wallet/sdk/src/models/account.rs
- applications/tari_walletd/web_ui/src/routes/AccountDetails/AccountDetails.tsx
- crates/consensus_tests/src/support/transaction.rs
- applications/tari_walletd/web_ui/src/api/hooks/useKeys.tsx
- crates/engine/src/runtime/tracker.rs
- applications/tari_walletd/web_ui/src/routes/Transactions/TransactionsLayout.tsx
- bindings/src/types/wallet-daemon-client/KeyBranch.ts
- bindings/src/types/Instruction.ts
- crates/wallet/storage_sqlite/tests/transaction.rs
- crates/wallet/storage_sqlite/src/writer.rs
- crates/engine_types/src/vault.rs
- crates/wallet/sdk/src/apis/transaction.rs
- crates/wallet/sdk/tests/confidential_output_api.rs
- applications/tari_walletd/src/lib.rs
- applications/tari_wallet_cli/src/command/account.rs
- crates/engine/src/runtime/mod.rs
- applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs
- applications/tari_indexer/web_ui/src/routes/Transaction/components/Result.tsx
- crates/p2p/proto/transaction.proto
- crates/template_test_tooling/src/builtin_component_state.rs
- applications/tari_walletd/web_ui/src/routes/AssetVault/Components/MyAssets.tsx
- crates/template_lib/src/models/resource.rs
- crates/state_store_tests/src/transactions.rs
- crates/template_lib/src/constants.rs
- crates/engine/tests/templates/shenanigans/src/lib.rs
- applications/tari_indexer/src/json_rpc/handlers.rs
- crates/wallet/sdk/src/apis/stealth_outputs.rs
- applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimBurn.tsx
- crates/wallet/sdk/src/apis/key_manager.rs
- applications/tari_walletd/src/handlers/transaction.rs
- crates/transaction/src/transaction.rs
- crates/engine_types/src/instruction.rs
- bindings/src/types/wallet-daemon-client/ClaimBurnRequest.ts
- crates/engine_types/src/resource_container.rs
- bindings/src/index.ts
- crates/transaction/src/v1/transaction.rs
- crates/engine_types/src/validator_fee.rs
- crates/template_builtin/templates/faucet/src/lib.rs
- crates/template_lib/src/models/vault.rs
- applications/tari_walletd/src/services/account_monitor.rs
- crates/engine/src/transaction/processor.rs
- crates/template_builtin/tests/nft_faucet.rs
- crates/engine/src/state_store/bootstrap.rs
- clients/wallet_daemon_client/src/types.rs
- applications/tari_swarm_daemon/src/process_manager/manager.rs
- applications/tari_indexer/web_ui/src/routes/Transaction/components/FeeInformation.tsx
- bindings/src/types/FeeReceipt.ts
- crates/engine/src/runtime/impl.rs
- crates/engine/tests/test.rs
- crates/wallet/sdk/src/apis/stealth_crypto.rs
- crates/wallet/storage_sqlite/src/reader.rs
- crates/wallet/storage_sqlite/src/schema.rs
- applications/tari_validator_node/src/state_bootstrap.rs
- crates/wallet/sdk/src/apis/confidential_transfer.rs
- applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx
- applications/tari_indexer/src/transaction_manager/mod.rs
🧰 Additional context used
🧬 Code graph analysis (20)
crates/wallet/sdk/src/storage.rs (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (2)
bindings/src/types/Account.ts (1)
Account(4-10)applications/tari_walletd/web_ui/src/api/hooks/useTransactions.tsx (1)
useGetAllTransactions(49-66)
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
applications/tari_walletd/web_ui/src/api/hooks/useKeys.tsx (3)
useKeysList(29-39)useKeysSetActive(52-66)useKeysCreate(41-50)
crates/template_lib/src/args/types.rs (1)
bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)
bindings/src/types/wallet-daemon-client/TransactionGetAllRequest.ts (3)
bindings/src/types/TransactionStatus.ts (1)
TransactionStatus(3-11)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/engine/tests/fees.rs (2)
crates/template_lib/src/models/vault.rs (1)
balance(308-316)crates/template_test_tooling/src/template_test.rs (1)
test_faucet_component(60-62)
crates/wallet/storage_sqlite/src/models/transaction.rs (6)
crates/wallet/storage_sqlite/src/writer.rs (2)
transaction(306-309)transaction(310-315)bindings/src/types/WalletTransaction.ts (1)
WalletTransaction(8-21)crates/transaction/src/transaction.rs (1)
is_dry_run(57-61)crates/transaction/src/v1/transaction.rs (1)
is_dry_run(50-52)crates/transaction/src/v1/unsealed.rs (1)
is_dry_run(51-53)applications/tari_indexer/src/storage_sqlite/serialization.rs (1)
deserialize_hex_try_from(37-48)
crates/engine_types/src/confidential/claim.rs (6)
bindings/src/types/UnclaimedConfidentialOutputAddress.ts (1)
UnclaimedConfidentialOutputAddress(8-8)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)bindings/src/types/CommitmentSignatureBytes.ts (1)
CommitmentSignatureBytes(5-5)bindings/src/types/RangeProofBytes.ts (1)
RangeProofBytes(9-9)bindings/src/types/TariStealthClaim.ts (1)
TariStealthClaim(8-17)
crates/p2p/src/conversions/transaction.rs (2)
bindings/src/types/TariStealthClaim.ts (1)
TariStealthClaim(8-17)bindings/src/types/Instruction.ts (1)
Instruction(14-40)
utilities/tariswap_test_bench/src/accounts.rs (2)
bindings/src/helpers/consts.ts (1)
XTR(10-10)bindings/src/types/ResourceType.ts (1)
ResourceType(20-20)
integration_tests/src/wallet_daemon_cli.rs (7)
bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-3)bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts (1)
ExtClaimBurnProof(4-4)bindings/src/types/wallet-daemon-client/ClaimBurnRequest.ts (1)
ClaimBurnRequest(5-9)bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)bindings/src/types/wallet-daemon-client/ClaimBurnProof.ts (1)
ClaimBurnProof(7-12)crates/engine_types/src/resource_container.rs (1)
resource_address(175-182)
applications/tari_walletd/src/handlers/accounts.rs (5)
bindings/src/types/TariStealthClaim.ts (1)
TariStealthClaim(8-17)bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts (1)
ExtClaimBurnProof(4-4)bindings/src/types/wallet-daemon-client/ClaimBurnProof.ts (1)
ClaimBurnProof(7-12)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-3)applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_params(157-168)
crates/engine/src/runtime/fee_state.rs (5)
bindings/src/types/FeeBreakdown.ts (1)
FeeBreakdown(4-4)bindings/src/types/FeeSource.ts (1)
FeeSource(3-3)bindings/src/types/ResourceContainer.ts (1)
ResourceContainer(11-29)bindings/src/types/VaultId.ts (1)
VaultId(6-6)bindings/src/helpers/consts.ts (1)
XTR(10-10)
crates/wallet/sdk/src/apis/stealth_transfer.rs (2)
crates/transaction/src/v1/transaction.rs (1)
inputs(87-89)crates/wallet/sdk/src/models/account.rs (1)
account(59-61)
bindings/src/types/TariStealthClaim.ts (5)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/UnclaimedConfidentialOutputAddress.ts (1)
UnclaimedConfidentialOutputAddress(8-8)bindings/src/types/RangeProofBytes.ts (1)
RangeProofBytes(9-9)bindings/src/types/CommitmentSignatureBytes.ts (1)
CommitmentSignatureBytes(5-5)bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)
crates/engine/src/runtime/state_store.rs (3)
bindings/src/types/SubstateId.ts (1)
SubstateId(15-24)bindings/src/types/Substate.ts (1)
Substate(4-4)crates/storage/src/consensus_models/substate_change.rs (1)
substate(58-63)
crates/transaction/src/builder/mod.rs (6)
bindings/src/types/TariStealthClaim.ts (1)
TariStealthClaim(8-17)bindings/src/types/ConfidentialWithdrawProof.ts (1)
ConfidentialWithdrawProof(19-30)bindings/src/types/Instruction.ts (1)
Instruction(14-40)crates/template_lib/src/models/vault.rs (1)
pay_fee_stealth(398-407)bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)crates/engine/src/runtime/impl.rs (1)
claim_burn(2298-2376)
applications/tari_walletd/src/services/transaction_service/service.rs (2)
crates/wallet/sdk/src/sdk.rs (1)
transaction_api(157-159)bindings/src/types/TransactionStatus.ts (1)
TransactionStatus(3-11)
crates/template_builtin/templates/account/src/lib.rs (5)
crates/engine/src/runtime/impl.rs (1)
pay_fee(2666-2686)crates/engine/src/runtime/working_state.rs (1)
pay_fee(861-863)crates/template_lib/src/models/vault.rs (2)
pay_fee(383-392)pay_fee_stealth(398-407)crates/engine_types/src/resource_container.rs (1)
amount(134-141)bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)
crates/engine/src/runtime/working_state.rs (6)
crates/engine_types/src/resource_container.rs (3)
stealth(91-101)amount(134-141)resource_address(175-182)crates/engine/src/runtime/validation.rs (1)
check_stealth_transfer_limits(9-21)crates/engine/src/runtime/impl.rs (1)
pay_fee(2666-2686)crates/engine_types/src/utxo.rs (3)
resource_address(87-89)id(91-93)output(45-47)crates/engine_types/src/resource.rs (2)
view_key(126-128)new(54-81)crates/engine_types/src/stealth/transfer.rs (1)
validate_transfer(28-130)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine_types/src/fees.rs (1)
34-38: total_refunded() computes theoretical overpay, not actual refundsThis returns total_fee_payment - total_fees_charged(), which equals refunds + non-refundable overcharge. It should report actual refunds only: total_fee_payment - total_fees_paid.
Fix:
- pub fn total_refunded(&self) -> u64 { - self.total_fee_payment - .checked_sub(self.total_fees_charged()) - .unwrap_or_default() - } + pub fn total_refunded(&self) -> u64 { + self.total_fee_payment + .checked_sub(self.total_fees_paid) + .unwrap_or_default() + }applications/tari_walletd/src/handlers/accounts.rs (1)
1076-1086: Fix duplicated fee rejection check in accounts handlerThe second check is incorrectly calling
fee_reject()again instead ofany_reject(), causing any non-fee rejection to be ignored. Other handlers correctly use the two-step pattern:
- applications/tari_walletd/src/handlers/accounts.rs: lines 689–694
- applications/tari_walletd/src/handlers/accounts.rs: lines 812–818
- applications/tari_walletd/src/handlers/accounts.rs: lines 994–1001
Please update the snippet at lines 1076–1086 accordingly:
let finalized = wait_for_result(&mut events, tx_id).await?; if let Some(reject) = finalized.finalize.result.fee_reject() { return Err(anyhow::anyhow!("Fee transaction rejected: {}", reject)); } - if let Some(reason) = finalized.finalize.fee_reject() { + if let Some(reason) = finalized.finalize.any_reject() { return Err(anyhow::anyhow!( "Fee transaction succeeded (fees charged) however the transaction failed: {}", reason )); }This ensures we first handle explicit fee rejections, then catch any other failure reasons.
♻️ Duplicate comments (6)
crates/wallet/storage_sqlite/src/writer.rs (2)
310-315: Persist signers as hex strings; dedup and keep stable order.Storing raw key objects complicates filtering and may not match UI/API expectations. Persist hex strings; sort+dedup for deterministic JSON. Also update the insert to use signers_hex.
- let signers = transaction - .signatures() - .iter() - .map(|s| s.public_key()) - .chain(iter::once(transaction.seal_signature().public_key())) - .collect::<Vec<_>>(); + let mut signers_hex = transaction + .signatures() + .iter() + .map(|s| serialize_hex(s.public_key())) + .chain(iter::once(serialize_hex(transaction.seal_signature().public_key()))) + .collect::<Vec<_>>(); + signers_hex.sort(); + signers_hex.dedup();- transactions::signers.eq(serialize_json(&signers)?), + transactions::signers.eq(serialize_json(&signers_hex)?),To verify consumers expect hex strings, search for signers filtering/usage:
#!/bin/bash rg -n -C2 --type=rust 'transactions::signers|signer(s)?_public_key|signers' crates | sed -n '1,120p'Also applies to: 321-321
348-348: Fix update filter: to_string() mismatches inserted hex; updates can never match.Insert path writes hex via serialize_hex(...); update path filters with .to_string(), resulting in NotFound on updates.
- .filter(transactions::transaction_id.eq(update.transaction_id.to_string())) + .filter(transactions::transaction_id.eq(serialize_hex(update.transaction_id)))Optional quick check:
#!/bin/bash rg -n -C2 --type=rust 'transactions::transaction_id\.eq\(' crates | sed -n '1,120p'crates/engine_types/src/fees.rs (1)
14-16: Add serde default for backward compatibility; tighten doc wordingWithout #[serde(default)], older serialized FeeReceipt payloads (missing this new field) will fail to deserialize. Also, avoid first-person phrasing in docs.
Apply:
- /// The amount of non-refundable fees which the user overpaid. Fees cannot be refunded when paying purely with a - /// stealth reveal (since we do not know the account/vault to refund). - pub total_fee_overcharge: u64, + /// The amount of non-refundable fees the user overpaid. Fees cannot be refunded when paying purely with a + /// stealth reveal (since the account/vault to refund cannot be determined). + #[serde(default)] + pub total_fee_overcharge: u64,crates/transaction/src/builder/mod.rs (1)
109-116: Fixed: pass StealthTransferStatement to pay_fee_stealth (resolves earlier mismatch).Signature now accepts
StealthTransferStatementand passes it as the sole arg topay_fee_stealth. This addresses the prior concern about usingConfidentialWithdrawProof.Also applies to: 119-121
applications/tari_walletd/src/handlers/accounts.rs (2)
601-602: Good: correct derivation path for stealth masks.Using
KeyBranch::StealthMasksresolves prior review concern.
592-599: Insert range-proof validation before unblind_output
A malformedrange_proofmust be rejected prior to any unblinding or signing. There is novalidate_range_proofonStealthCryptoApi; instead, call the standalonevalidate_bullet_prooffromtari_engine_types.– File:
applications/tari_walletd/src/handlers/accounts.rs
– Location: immediately after line 592 (after expandingreciprocal_claim_public_key_expanded)Suggested diff:
use tari_engine_types::crypto::range_proof::validate_bullet_proof; let reciprocal_claim_public_key_expanded = RistrettoPublicKey::try_from_byte_type(&reciprocal_claim_public_key) .map_err(|e| invalid_params("claim_proof.reciprocal_claim_public_key", Some(e)))?; + // Validate that the claimed commitment is in-range + if let Err(e) = validate_bullet_proof(&range_proof, std::iter::once(&output.commitment)) { + return Err(invalid_params( + "claim_proof.range_proof", + Some(&e.to_string()), + )); + } let unmasked_output = sdk.stealth_crypto_api().unblind_output( &output.commitment, &output.encrypted_data, claim_nonce_keypair.secret_key(), &reciprocal_claim_public_key_expanded, )?;
🧹 Nitpick comments (11)
crates/wallet/storage_sqlite/src/writer.rs (1)
306-309: Ensure deterministic referenced_components (sort + dedup).Stable ordering and dedup make JSON deterministic and help equality checks.
- let ref_components = transaction - .as_referenced_components() - .map(|c| c.to_string()) - .collect::<Vec<_>>(); + let mut ref_components = transaction + .as_referenced_components() + .map(|c| c.to_string()) + .collect::<Vec<_>>(); + ref_components.sort(); + ref_components.dedup();crates/engine_types/src/fees.rs (1)
81-91: Method rename is an external API break; add deprecated alias insert()If any external crate calls FeeBreakdown::insert, this rename will break builds. Provide a deprecated wrapper to smooth upgrades.
Add inside impl FeeBreakdown:
#[deprecated(note = "use FeeBreakdown::add")] pub fn insert(&mut self, source: FeeSource, amount: u64) { self.add(source, amount); }crates/transaction/src/builder/mod.rs (3)
220-242: Heads-up: workspace usage in fee helpers can panic if called via with_fee_instructions_builder.
pay_fee_stealth_with_input_bucketandpay_fee_stealth_with_opt_input_bucketresolve workspace labels; when these are invoked insidewith_fee_instructions_builder, the closure receives a fresh builder with an empty workspace map, so lookups will fail. Either avoid workspace labels in that context or pass through the parent’s workspace map (see suggestion on Lines 296-305).
220-242: Add minimal unit tests for PayFee encoding.Recommend tests that build a tx using
pay_fee_stealth(with and without an input bucket) and assert the produced instruction matchesInstruction::PayFee { statement, revealed_input_bucket }.
296-305: with_fee_instructions_builder loses workspace context; clone parent map to avoid lookups failing.Currently the closure gets a new builder with an empty
workspace_ids, so any workspace label resolution will panic. Clone the parent’s map into the fee-builder at minimum.Apply this diff:
pub fn with_fee_instructions_builder<F: FnOnce(TransactionBuilder) -> TransactionBuilder>(mut self, f: F) -> Self { - // TODO: pass in a fee builder type (probably TransactionBuilder<FeeBuilder> which has applicable methods) - let builder = f(TransactionBuilder::new()); + // TODO: pass in a fee builder type (probably TransactionBuilder<FeeBuilder> which has applicable methods) + // Copy the parent's workspace map so label resolution in fees doesn't panic. + let mut fee_builder = TransactionBuilder::new(); + fee_builder.workspace_ids = self.workspace_ids.clone(); + let builder = f(fee_builder); self.unsigned_transaction .fee_instructions_mut() .extend(builder.unsigned_transaction.into_instructions()); // Reset the signatures as they are no longer valid self.clear_signatures(); self }Longer term, a dedicated FeeBuilder that forbids mutating workspace state would be safer.
crates/wallet/sdk/src/apis/confidential_transfer.rs (3)
245-246: Validate fee parameter and clarify unitsConsider validating
params.max_fee(e.g., nonzero, sane upper bound) and documenting its unit (XTR attounits?) to avoid surprising behavior when0or excessively large values are passed.
301-332: Remove redundant positivity check when creating change output
change_confidential_amount.is_positive()already guaranteesstatement.amount > 0. The secondis_positive()check is redundant. Simplify to always add the change output inside the positive branch.- let maybe_change_statement = if change_confidential_amount.is_positive() { + let maybe_change_statement = if change_confidential_amount.is_positive() { let statement = self.create_confidential_proof_statement( &account_public_key, change_confidential_amount, resource_view_key, )?; - let change_value = statement.amount; - - if change_value.is_positive() { - self.outputs_api.add_output(ConfidentialOutputModel { - account_address: *account.address(), - vault_id: src_vault.id, - commitment: statement - .to_commitment() - .expect("BUG: to_commitment negative amount") - .to_byte_type(), - value: change_value, - sender_public_nonce: Some(statement.sender_public_nonce.to_byte_type()), - encryption_secret_key_index: account_secret.key_index, - encrypted_data: statement.encrypted_data.clone(), - public_asset_tag: None, - status: OutputStatus::LockedUnconfirmed, - lock_id: Some(inputs_to_spend.lock_id), - })?; - } + self.outputs_api.add_output(ConfidentialOutputModel { + account_address: *account.address(), + vault_id: src_vault.id, + commitment: statement + .to_commitment() + .expect("BUG: to_commitment negative amount") + .to_byte_type(), + value: statement.amount, + sender_public_nonce: Some(statement.sender_public_nonce.to_byte_type()), + encryption_secret_key_index: account_secret.key_index, + encrypted_data: statement.encrypted_data.clone(), + public_asset_tag: None, + status: OutputStatus::LockedUnconfirmed, + lock_id: Some(inputs_to_spend.lock_id), + })?; Some(statement) } else { None };
259-262: Outdated log message implies an unlock that no longer happensThis log mentions unlocking “fee fund locks” but there’s no unlock performed here anymore. Update the message to reflect the actual behavior (or perform the unlock if intended).
- warn!(target: LOG_TARGET, "Unlocking fee fund locks after error: {}", e); + warn!(target: LOG_TARGET, "Input selection failed: {}", e);applications/tari_walletd/src/handlers/accounts.rs (3)
561-566: Nit: log references account key, but signing uses the claim nonce key.Improve accuracy of operator logs.
- info!( - target: LOG_TARGET, - "ℹ️ Signing claim burn with key {}. NOTE: This must be the same as the claiming key used in the burn transaction for this to succeed.", - account.owner_public_key - ); + info!( + target: LOG_TARGET, + "ℹ️ Signing claim burn with claim key {}. NOTE: This must match the key used in the burn transaction.", + claim_nonce_keypair.public_key + );
427-449: Edge case: avoid creating a zero-value stealth change output.If
remaining_confidential_amountis zero, generating an output can be invalid/pointless. Branch to produce no change output instead.- let output_statement = UnblindedStealthOutputStatement { /* ... amount: remaining_confidential_amount ... */ }; + let maybe_change = (!remaining_confidential_amount.is_zero()).then_some(UnblindedStealthOutputStatement { /* ... */ }); ... - let transfer = sdk.stealth_crypto_api().generate_transfer_statement( - &inputs, Amount::zero(), array::from_ref(&output_statement), amount_to_reveal, - )?; + let transfer = if let Some(change) = maybe_change { + sdk.stealth_crypto_api().generate_transfer_statement(&inputs, Amount::zero(), array::from_ref(&change), amount_to_reveal)? + } else { + sdk.stealth_crypto_api().generate_transfer_statement(&inputs, Amount::zero(), &[], amount_to_reveal)? + };
773-790: Consistency: prefer derive_account_key + derive public key locally.Keeps key-derivation usage uniform across handlers.
- let (account_secret_key, account_public_key) = sdk.key_manager_api().derive_account_keypair(account.key_index())?; + let account_secret_key = sdk.key_manager_api().derive_account_key(account.key_index())?; + let account_public_key = RistrettoPublicKey::from_secret_key(&account_secret_key.key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
applications/tari_walletd/src/handlers/accounts.rs(11 hunks)crates/engine/src/runtime/fee_state.rs(1 hunks)crates/engine_types/src/fees.rs(2 hunks)crates/transaction/src/builder/mod.rs(8 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(5 hunks)crates/wallet/storage_sqlite/src/writer.rs(3 hunks)utilities/tariswap_test_bench/templates/tariswap/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/runtime/fee_state.rs
🧰 Additional context used
🧬 Code graph analysis (5)
utilities/tariswap_test_bench/templates/tariswap/src/lib.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(20-20)
crates/engine_types/src/fees.rs (1)
bindings/src/types/FeeSource.ts (1)
FeeSource(3-3)
applications/tari_walletd/src/handlers/accounts.rs (6)
bindings/src/types/TariStealthClaim.ts (1)
TariStealthClaim(8-17)bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts (1)
ExtClaimBurnProof(4-4)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-3)crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
transfer(313-509)bindings/src/types/wallet-daemon-client/ClaimBurnProof.ts (1)
ClaimBurnProof(7-12)applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_params(157-168)
crates/transaction/src/builder/mod.rs (6)
bindings/src/types/TariStealthClaim.ts (1)
TariStealthClaim(8-17)bindings/src/types/StealthTransferStatement.ts (1)
StealthTransferStatement(5-13)bindings/src/types/Instruction.ts (1)
Instruction(14-40)crates/template_lib/src/models/vault.rs (1)
pay_fee_stealth(398-407)crates/engine/src/runtime/mod.rs (1)
claim_burn(166-166)crates/engine/src/runtime/impl.rs (1)
claim_burn(2298-2376)
crates/wallet/storage_sqlite/src/writer.rs (2)
crates/wallet/storage_sqlite/src/reader.rs (1)
transactions(226-228)applications/tari_indexer/src/storage_sqlite/serialization.rs (2)
serialize_hex(25-27)serialize_json(9-15)
⏰ 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: check nightly
- GitHub Check: machete
- GitHub Check: clippy
- GitHub Check: test
🔇 Additional comments (15)
crates/wallet/storage_sqlite/src/writer.rs (2)
6-6: LGTM: std::iter importImport for iter::once is appropriate and scoped.
318-318: Good: transaction_id now hex-serialized (aligned with reader/indexer).This matches reader.rs usage of serialize_hex(transaction_id).
utilities/tariswap_test_bench/templates/tariswap/src/lib.rs (1)
229-234: Confirm intended handling ofConfidentialvs.StealthA project-wide search shows that
ResourceType::Confidentialis still referenced in over 50 locations (core crates, tests, the wallet SDK, CLI, etc.). Removing it here would have widespread impact. Please verify which of the following aligns with the PR’s objective:• Keep
Confidentialfor backward compatibility, but improve clarity and consistency in the error message.
• Fully migrate offConfidential(deprecate it) and update usages across the repo in one coordinated effort.If retaining both, I recommend:
- matches!( - resource_type, - ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth - ), - "Resource {} is not fungible (fungible, stealth, confidential)", + matches!( + resource_type, + ResourceType::Fungible | ResourceType::Stealth | ResourceType::Confidential + ), + "Resource {} must be fungible-like (Fungible, Stealth, or Confidential)",If the goal is to disallow
Confidentialhere, update to:- ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth + ResourceType::Fungible | ResourceType::Stealth—and plan to remove or refactor the remaining
Confidentialreferences accordingly. Let me know which path you’d like to take so we can ensure consistency across the codebase.crates/transaction/src/builder/mod.rs (6)
14-14: Stealth claim import aligned with runtime. LGTM.Switching to
TariStealthClaimmatches the engine/runtime expectations.
26-26: Correct import for stealth transfers.Importing
StealthTransferStatementhere is appropriate and consistent with usages below.
87-91: Good high-level API for fee payment via stealth.
fee_transaction_pay_fees_stealthcleanly forwards topay_fee_stealth.
272-272: Verify enum payload shape for ClaimBurn.You’re boxing the claim:
Instruction::ClaimBurn { claim: Box::new(claim) }. Ensure the Rust enum variant expects aBox<TariStealthClaim>; if it takes the value directly, remove the box.Possible fix if unboxed:
- self.add_instruction(Instruction::ClaimBurn { claim: Box::new(claim) }) + self.add_instruction(Instruction::ClaimBurn { claim })
405-417: Auto-assigning an authorized seal signer is a behavioral change—confirm this is desired.Defaulting to
authorized_sealed_signerwhensignaturesis empty may surprise callers who intend to supply signatures later or use a different signer. Consider an opt-in flag or a feature gate, or document prominently.
419-421: Simplified build_and_seal flow.Delegating to
build().seal(...)is clearer and keeps behavior unchanged.crates/wallet/sdk/src/apis/confidential_transfer.rs (2)
431-434: No downstream references tofee_transaction_proof_idfound
Ranrg -n 'fee_transaction_proof_id' -g '!**/target/**'and confirmed there are no remaining callers expecting the removed field.
346-348: Conditionalize fee payment and enforce explicit XTR checkBased on our review, the
fee_transaction_pay_from_componentcall in
crates/wallet/sdk/src/apis/confidential_transfer.rs(lines 346–348) is currently emitted unconditionally—even whenmax_fee == 0—and relies on the assumption thatfrom_accountholds XTR. We should:• Replace the unconditional fee instruction with a conditional wrapper that only pays fees when
max_fee > 0, for example:- // TODO: we assume that from_account has XTR - .fee_transaction_pay_from_component(*from_account.address(), max_fee) - .then(|builder| { + .then(|builder| { + // Only emit PayFee when max_fee > 0 to avoid no-op fee instructions. + let builder = if max_fee > 0 { + builder.fee_transaction_pay_from_component(*from_account.address(), max_fee) + } else { + builder + }; if dest_account_exists { builder // …rest of closure… } else { builder // …alternative branch… } })• Replace the “assume XTR” TODO with an explicit preflight balance check—either:
– Query the account’s confidential balance for XTR before building, returning a clear error if it’s insufficient; or
– Insert a manifest instruction to fund the account with XTR fees (if that aligns with UX requirements).• Manually verify the builder’s semantics to ensure skipping the fee instruction on zero does not alter refundability or conflict with downstream logic (e.g., in stealth transfers or engine tests).
• Consider applying the same conditional fee pattern to
stealth_transfer.rs(which also has a TODO for fee payment) to maintain consistent behavior.• Add or update unit/integration tests around confidential transfers with
max_fee == 0and with insufficient XTR to confirm failure modes and avoid regressions.applications/tari_walletd/src/handlers/accounts.rs (4)
293-319: LGTM: stealth balances aggregation avoids double-counting and stabilizes UI order.Using IndexMap and excluding vaulted resources is correct.
395-404: I’ve added scripts to locate both the lock creation and where the transaction hash is set inaccounts.rs. Once those run, we can confirm whether there are early returns without a corresponding call torelease_locked_outputs, and then update the suggestion accordingly.
419-426: Confirm stealth output tag derivation specThe
derive_stealth_output_tagfunction (crates/wallet/crypto/src/kdfs.rs:77–80) currently takes onlynetworkandowner_public_keyand all call sites (e.g. applications/tari_walletd/src/handlers/accounts.rs:419–426 and 633–641) pass only those two parameters. If the stealth‐output‐tag derivation per the protocol spec must also incorporate the sender’s nonce/ephemeral value, we need to:
- Update the KDF signature in crates/wallet/crypto/src/kdfs.rs to accept the nonce.
- Propagate that change through the SDK API in crates/wallet/sdk/src/apis/stealth_crypto.rs (lines 73–80).
- Adjust all call sites (e.g., applications/tari_walletd/src/handlers/accounts.rs:419–426, 633–641).
Please verify the cryptographic specification for stealth‐output‐tag derivation and ensure the implementation aligns with it.
410-417: encrypt_value_and_mask parameter order is correctAccording to the API signature in
crates/wallet/crypto/src/encrypted_data.rs, the parameters are defined aspub fn encrypt_value_and_mask( amount: u64, mask: &RistrettoSecretKey, public_nonce: &RistrettoPublicKey, secret: &RistrettoSecretKey, ) -> EncryptedData { … }and the
StealthCryptoApiwrapper calls it identically:let data = encrypt_value_and_mask(amount, mask, public_nonce, secret)?;(see
crates/wallet/sdk/src/apis/stealth_crypto.rsat thelet data = encrypt_value_and_mask(…)call).In
accounts.rs(lines 412–417), the code passes&output_mask.key, // mask &public_nonce, // ephemeral public nonce &account_key.key, // recipient’s secret keywhich matches the API’s
(mask, public_nonce, secret)ordering. Swapping in&account_public_keyand&nonce(the suggested diff) would treat the static account public key as the ephemeral nonce and the ephemeral secret as the static secret—breaking the ECDH decryption path.No change is required here; please disregard the original swap suggestion.
Likely an incorrect or invalid review comment.
Test Results (CI)418 tests ±0 394 ✅ ±0 50m 47s ⏱️ - 24m 59s For more details on these failures, see this check. Results for commit 3addf54. ± Comparison against base commit e3d2fa5. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
190-190: Signature change to&Account: verify all call sites and ensureAccount::key_index()is exposed.Confirm downstream code passes
&Accountand thatAccountprovideskey_index().Run:
#!/bin/bash # Call sites of the updated function rg -n -C2 -g '*.rs' '\bresolve_output_masks_for_spending\s*\(' # Any lingering references to AccountWithPublicKey rg -n -C2 -g '*.rs' '\bAccountWithPublicKey\b' # Confirm Account::key_index usage/definition rg -n -C2 -g '*.rs' '\bimpl\b[^{]*\bAccount\b' | sed -n '1,200p' rg -n -C2 -g '*.rs' '\bkey_index\s*\(\s*\)'applications/tari_walletd/src/handlers/accounts.rs (3)
601-601: Correct key branch for stealth masks: LGTMUsing KeyBranch::StealthMasks is correct for stealth UTXO derivation.
490-491: Record lock on stealth API: LGTMlocks_set_transaction_id on stealth_outputs_api fixes the earlier mix-up with confidential locks.
592-600: Missing validation for range_proof in ClaimBurn flowRange proof from ExtClaimBurnProof is never validated. This is consensus/security-critical.
Apply after parsing reciprocal_claim_public_key:
let reciprocal_claim_public_key_expanded = RistrettoPublicKey::try_from_byte_type(&reciprocal_claim_public_key) .map_err(|e| invalid_params("claim_proof.reciprocal_claim_public_key", Some(e)))?; +// Validate the range proof before using the output +if !sdk + .stealth_crypto_api() + .validate_range_proof(network, &range_proof, &commitment) +{ + return Err(invalid_params( + "claim_proof.range_proof", + Some("range proof validation failed"), + )); +}
🧹 Nitpick comments (4)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
32-32: Remove cross-API error type import (will be unused after unifying return type).Keep this module’s API errors consistent; drop
ConfidentialOutputsApiErrorhere.Apply:
- confidential_outputs::ConfidentialOutputsApiError,applications/tari_walletd/src/handlers/accounts.rs (3)
561-565: Log message leaks/claims wrong key; log the claim nonce public key or redactMessage says you’re signing with the claiming key but logs account.owner_public_key. Log the claim nonce public key (or remove) to avoid confusion and reduce sensitive logging.
- info!( - target: LOG_TARGET, - "ℹ️ Signing claim burn with key {}. NOTE: This must be the same as the claiming key used in the burn transaction for this to succeed.", - account.owner_public_key - ); + info!( + target: LOG_TARGET, + "ℹ️ Signing claim burn with claim-nonce key {}", + claim_nonce_keypair.public_key.to_byte_type() + );
603-611: Dead code: zero check is unreachable after checked_sub_positivechecked_sub_positive guarantees final_amount > 0 or errors. The subsequent is_zero() branch can never execute.
- if final_amount.is_zero() { - return Err(invalid_params("max_fee", Some("fee equals or exceeds claimed amount"))); - }
395-404: Consider releasing the lock on early errorsIf an error occurs after creating the lock but before setting the transaction id, the lock may remain. Add a scope guard to cancel on error paths.
- let lock_id = sdk + let lock_id = sdk .stealth_outputs_api() .create_lock_for_resource(&STEALTH_TARI_RESOURCE_ADDRESS)?; + // Optional scope guard to release lock if we exit early + struct LockGuard<'a> { api: &'a _, id: _ } + impl<'a> Drop for LockGuard<'a> { + fn drop(&mut self) { let _ = self.api.locks_cancel(self.id); } + } + let _lock_guard = LockGuard { api: sdk.stealth_outputs_api(), id: lock_id };Drop _lock_guard just after locks_set_transaction_id to keep the lock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
applications/tari_walletd/src/handlers/accounts.rs(11 hunks)crates/consensus/src/hotstuff/block_change_set.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/consensus/src/hotstuff/block_change_set.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
bindings/src/types/Account.ts (1)
Account(4-10)
applications/tari_walletd/src/handlers/accounts.rs (4)
bindings/src/types/TariStealthClaim.ts (1)
TariStealthClaim(8-17)bindings/src/types/wallet-daemon-client/ExtClaimBurnProof.ts (1)
ExtClaimBurnProof(4-4)crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
transfer(313-509)bindings/src/types/wallet-daemon-client/ClaimBurnProof.ts (1)
ClaimBurnProof(7-12)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check stable
- GitHub Check: check nightly
🔇 Additional comments (5)
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
37-37: Import switch toAccountlooks good.Aligns with the PR-wide
AccountWithPublicKey → Accountmigration.
219-221: Good: owner stealth secret derived viaStealthCryptoApi.Centralizes derivation and respects
network.
474-479: Good: derive-and-verify stealth address during unblinding.Correctly guards spendability by matching derived stealth pubkey to the output’s owner key.
applications/tari_walletd/src/handlers/accounts.rs (2)
293-303: Deterministic aggregation of stealth balances: LGTMUsing IndexMap to stabilize UI ordering and folding by resource is a good call.
412-417: Remove order-check suggestion
Allencrypt_value_and_maskcalls align with the canonical(amount, mask, public_nonce, secret)signature incrypto/src/encrypted_data.rs, so no change needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (21)
Cargo.toml (1)
61-66: Remove duplicate workspace member "utilities/transaction_submitter".It appears twice (Lines 62 and 63). Keep a single entry to avoid confusion in tooling.
- "utilities/transaction_submitter",integration_tests/src/http_server.rs (1)
19-30: Normalize URL paths and avoid unnecessary String copies.Ensure consistent leading slash for both matcher and returned URL; accept &str to prevent allocations.
- pub async fn publish_file(&self, url_path: String, file_path: String) -> Mock<'_> { + pub async fn publish_file(&self, url_path: &str, file_path: &str) -> Mock<'_> { let mock = self .server .mock_async(|when, then| { - when.path(format!("/{}", url_path)); - then.status(200).body_from_file(file_path); + let path = format!("/{}", url_path.trim_start_matches('/')); + when.path(path.clone()); + then.status(200).body_from_file(file_path); }) .await; - let url = self.server.url(url_path); + let url = self.server.url(&format!("/{}", url_path.trim_start_matches('/'))); Mock { mock, url } }applications/tari_indexer/src/substate_file_cache.rs (1)
28-56: Impl signature should match trait; return futures fromfninstead ofasync fn.Given the trait returns a future, define
fnmethods that return animpl Future + Send + '_. This avoids relying onasync fn in traitsugar and ensuresSendis preserved.impl SubstateCache for SubstateFileCache { - async fn read(&self, address: String) -> Result<Option<SubstateCacheEntry>, SubstateCacheError> { - let res = cacache::read(&self.cache_dir_path, address).await; - match res { - Ok(value) => { - // cache hit - let entry = decode::<SubstateCacheEntry>(&value).map_err(|e| SubstateCacheError(e.to_string()))?; - Ok(Some(entry)) - }, - Err(e) => { - // cache miss - if let cacache::Error::EntryNotFound(_, _) = e { - Ok(None) - // cache error - } else { - Err(SubstateCacheError(format!("{}", e))) - } - }, - } - } + fn read(&self, address: String) + -> impl std::future::Future<Output = Result<Option<SubstateCacheEntry>, SubstateCacheError>> + Send + '_ + { + async move { + let res = cacache::read(&self.cache_dir_path, address).await; + match res { + Ok(value) => { + let entry = + decode::<SubstateCacheEntry>(&value).map_err(|e| SubstateCacheError(e.to_string()))?; + Ok(Some(entry)) + }, + Err(e) => { + if let cacache::Error::EntryNotFound(..) = e { + Ok(None) + } else { + Err(SubstateCacheError(format!("{}", e))) + } + }, + } + } + } - async fn write(&self, address: String, entry: &SubstateCacheEntry) -> Result<(), SubstateCacheError> { - let encoded_entry = encode(&entry).map_err(|e| SubstateCacheError(e.to_string()))?; - cacache::write(&self.cache_dir_path, address, encoded_entry) - .await - .map_err(|e| SubstateCacheError(format!("{}", e)))?; - Ok(()) - } + fn write(&self, address: String, entry: &SubstateCacheEntry) + -> impl std::future::Future<Output = Result<(), SubstateCacheError>> + Send + '_ + { + async move { + let encoded_entry = encode(&entry).map_err(|e| SubstateCacheError(e.to_string()))?; + cacache::write(&self.cache_dir_path, address, encoded_entry) + .await + .map_err(|e| SubstateCacheError(format!("{}", e)))?; + Ok(()) + } + } }integration_tests/tests/steps/wallet_daemon.rs (1)
121-126: Don’t unwrap fee_reject() inside the failure branch of any_accept(); it can panic and hide the real error.Use a safe fallback that reports fee_reject if present, otherwise a clear generic failure.
- resp.result.result.any_accept().unwrap_or_else(|| { - panic!( - "Expected fee claim to succeeded but failed with {}", - resp.result.result.fee_reject().unwrap() - ) - }); + match resp.result.result.any_accept() { + Some(_) => (), + None => { + if let Some(reason) = resp.result.result.fee_reject() { + panic!("Expected fee claim to succeed but fee_rejected: {}", reason); + } else { + panic!("Expected fee claim to succeed but not accepted"); + } + } + }crates/consensus/src/hotstuff/on_propose.rs (1)
883-887: Usefinalize.result.any_accept()for consistency
Replaceexecution.result().finalize.any_accept()withexecution.result().finalize.result.any_accept()incrates/consensus/src/hotstuff/on_propose.rsat line 882 to match the other call site (line 677) and avoid a compilation error.crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (2)
615-646: Inconsistent handling of missing finalize diff; align with AllAccept and apply committee filteringLocalOnly COMMIT path silently skips when any_accept() returns None, while AllAccept treats it as invariant. This can cause latent state root mismatches and inconsistent behavior. Also, we should consistently filter the diff for the local committee before putting it into the substate store.
Apply:
- if pool_tx.current_decision().is_commit() { - if let Some(diff) = execution.result().finalize.any_accept() { - substate_store.put_diff(diff)?; - } + if pool_tx.current_decision().is_commit() { + let diff = execution + .result() + .finalize + .any_accept() + .ok_or_else(|| HotStuffError::InvariantError(format!( + "evaluate_local_only_command: Transaction {} has COMMIT decision but no finalize diff", + pool_tx.transaction_id() + )))?; + substate_store.put_diff(&filter_diff_for_committee(local_committee_info, diff))?;
1203-1214: DecisionDisagreement fields reversed (local vs remote)Local is set to the leader’s decision (atom.decision) instead of the local node’s decision (tx_rec.current_decision()). This misreports disagreements.
Apply:
- return Ok(Some(NoVoteReason::DecisionDisagreement { - local: atom.decision, - remote: Decision::Commit, - })); + return Ok(Some(NoVoteReason::DecisionDisagreement { + local: tx_rec.current_decision(), + remote: atom.decision, + }));applications/tari_validator_node/src/consensus/block_transaction_executor.rs (1)
104-121: Remove legacy.accept()usages outside the newany_accept()API
- In crates/rpc_state_sync/src/manager_old.rs (around lines 429 and 444), replace
cmd.accept()with the newany_accept()pattern or remove these loops if they’re no longer needed.- In integration_tests/src/wallet_daemon_cli.rs (remove or update the commented-out
.accept()calls to useany_accept()on finalized results).utilities/tariswap_test_bench/src/runner.rs (1)
78-79: Avoid unwrap() on optional timings to prevent sporadic panics.These Option fields can be absent; guard them.
Apply:
- self.stats.add_execution_time(tx.execution_time.unwrap()); - self.stats.add_time_to_finalize(tx.finalized_time.unwrap()); + if let Some(t) = tx.execution_time { + self.stats.add_execution_time(t); + } + if let Some(t) = tx.finalized_time { + self.stats.add_time_to_finalize(t); + }crates/wallet/storage_sqlite/src/models/vault.rs (1)
35-71: Addlocked_bymapping toVaultModel.
- In
crates/wallet/sdk/src/models/vault.rs, extend theVaultModelstruct:pub struct VaultModel { pub account_address: ComponentAddress, pub id: VaultId, pub resource_address: ResourceAddress, pub resource_type: ResourceType, pub token_symbol: String, pub revealed_balance: Amount, pub locked_revealed_balance: Amount, pub confidential_balance: Amount, pub divisibility: u8,
pub locked_by: Option<i32>,
- In
crates/wallet/storage_sqlite/src/models/vault.rs, updatetry_into_vaultto propagate the lock reference:impl Vault { pub(crate) fn try_into_vault(self, account_address: ComponentAddress) -> Result<..., WalletStorageError> { Ok(tari_ootle_wallet_sdk::models::VaultModel { account_address, id: …, resource_address: …, resource_type: …, token_symbol: self.token_symbol, revealed_balance: Amount::from(self.revealed_balance), locked_revealed_balance: Amount::from(self.locked_revealed_balance), confidential_balance: Amount::from(self.confidential_balance), divisibility: u8::try_from(self.divisibility as u32)?,
locked_by: self.locked_by, }) }Ensure the local
Vaultstruct includes alocked_by: Option<i32>field matching the schema.utilities/tariswap_test_bench/src/faucet.rs (1)
53-62: Fix resource filtering and error messages (type-safe compare, accurate text).Filter after mapping to ResourceAddress, and correct the vault error string.
- let resource_address = diff - .up_iter() - .filter(|(addr, _)| *addr != XTR) - .find_map(|(addr, _)| addr.as_resource_address()) - .ok_or_else(|| anyhow::anyhow!("Faucet Resource address not found"))?; + let resource_address = diff + .up_iter() + .find_map(|(addr, _)| addr.as_resource_address()) + .filter(|addr| *addr != XTR) + .ok_or_else(|| anyhow::anyhow!("Faucet resource address not found"))?; @@ - .ok_or_else(|| anyhow::anyhow!("Faucet Resource address not found"))?; + .ok_or_else(|| anyhow::anyhow!("Faucet vault id not found"))?;applications/tari_walletd/src/handlers/confidential.rs (1)
122-139: Critical: change output amount is over-credited (change = inputs − (amount − reveal)).Change must subtract both the sent amount and the revealed amount. As written, change is inflated by 2×reveal. Fix by computing change against amount_to_transfer.
- let spend_amount = req.amount.checked_sub_positive(req.reveal_amount).ok_or_else(|| { - invalid_request(format!( - "Amount to send must be greater than or equal to the amount to reveal. Amount = {}, Revealed = {}", - req.amount, req.reveal_amount - )) - })?; - let change_amount = total_input_value.checked_sub_positive(spend_amount).ok_or_else(|| { - invalid_request(format!( - "Insufficient funds to send {}. Total input value = {}", - req.amount, total_input_value - )) - })?; + let change_amount = total_input_value + .checked_sub_positive(amount_to_transfer) + .ok_or_else(|| { + invalid_request(format!( + "Insufficient funds to send {} (including {} revealed). Total input value = {}", + req.amount, req.reveal_amount, total_input_value + )) + })?;crates/wallet/storage_sqlite/src/models/stealth_output.rs (1)
94-101: Missing update path for is_on_chain
StealthOutputUpdatedoesn’t exposeis_on_chain, so you can’t flip it via regular updates.Add a field:
pub(crate) struct StealthOutputUpdate<'a> { pub status: Option<&'a str>, pub is_burnt: Option<bool>, pub is_frozen: Option<bool>, pub is_on_chain: Option<bool>, // new pub updated_at: Option<PrimitiveDateTime>, }utilities/transaction_submitter/src/main.rs (1)
193-201: Guard full commit semantics when using any_accept()
any_accept()returns a diff for both full commits and fee-only accepts (AcceptFeeRejectRest); only markis_committed = truewhen the transaction is truly fully accepted (e.g. viaFinalizeResult::is_full_accept()orexecution.decision().is_commit()).crates/wallet/sdk/src/apis/confidential_transfer.rs (3)
125-139: Use available revealed balance when preferring revealed funds.This path uses
src_vault.revealed_balance, ignoring existing locks; can over-commit revealed funds.- let revealed_to_spend = cmp::min(src_vault.revealed_balance, spend_amount); + let revealed_to_spend = cmp::min(available_revealed_funds, spend_amount);
182-184: Use available revealed balance in the insufficiency check.Match locking semantics used elsewhere in this method.
- if src_vault.revealed_balance < revealed_to_spend { + if available_revealed_funds < revealed_to_spend { return Err(ConfidentialTransferApiError::InsufficientFunds); }
252-267: The leak issue is confirmed: on error the newly created lock is never cleaned up, andoutputs_apidoes not expose a directlocks_delete—you must call the underlying delete via the raw API. Replace the early-error return with a best-effort unlock:let lock_id = self.outputs_api.create_lock()?; let inputs_to_spend = match self.resolved_inputs_for_transfer( lock_id, params.from_account, params.resource_address, params.amount, params.input_selection, ) { Ok(inputs) => inputs, Err(e) => { - warn!(target: LOG_TARGET, "Unlocking fee fund locks after error: {}", e); - return Err(e); + warn!(target: LOG_TARGET, "Unlocking locks after error: {}", e); + // best-effort cleanup; ignore any deletion error + let _ = self.outputs_api.store.with_write_tx(|tx| tx.locks_delete(lock_id)); + return Err(e); }, };Use
self.outputs_api.store.with_write_tx(|tx| tx.locks_delete(lock_id))for cleanup sinceoutputs_apidoesn’t exposelocks_deletedirectly.crates/wallet/storage_sqlite/src/reader.rs (1)
865-886: Return empty set when no stealth outputs are locked (do not 404).Aligns with
outputs_get_locked_by_lock_idbehavior and reduces false positives upstream.- // account_id should be the same in all rows - let first_row = rows.first().ok_or_else(|| WalletStorageError::NotFound { - operation: OPERATION, - entity: "stealth_output".to_string(), - key: lock_id.to_string(), - })?; + if rows.is_empty() { + return Ok(vec![]); + } + // account_id should be the same in all rows + let first_row = rows.first().unwrap();crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
267-275: Bug: creates a new lock_id shadowing the transaction lock.PreferConfidential branch calls create_lock() and shadows the passed lock_id. This breaks cleanup/linking (only the outer lock_id is released/linked). Use the provided lock_id.
- let lock_id = self.outputs_api.create_lock()?; - let (blinded_inputs, blinded_amount_locked) = self.outputs_api.lock_outputs_until_partial_amount( + let (blinded_inputs, blinded_amount_locked) = self.outputs_api.lock_outputs_until_partial_amount( owner_account.address(), &resource_address, spend_amount, lock_id, )?;crates/engine/src/runtime/impl.rs (1)
1560-1579: Same XTR consistency in VaultAction::PayFee (stealth reveal).Keep the resource address consistent here too.
- if let Some(statement) = arg.statement { - if let Some(revealed) = - state_mut.execute_stealth_transfer(STEALTH_TARI_RESOURCE_ADDRESS.into(), statement, None)? + if let Some(statement) = arg.statement { + if let Some(revealed) = + state_mut.execute_stealth_transfer(XTR.into(), statement, None)? { container.deposit(revealed)?; } }crates/wallet/storage_sqlite/src/writer.rs (1)
642-693: Vault locking doesn’t set locked_by and doesn’t enforce available balance; finalize/unlock will no-op.
- locked_by is never set, yet finalize/unlock filter by locked_by = lock_id → NotFound at runtime.
- No check that revealed_balance - locked_revealed_balance ≥ amount_to_lock → can over-lock.
Proposed fix:
- let (db_id, existing_lock_id) = vaults::table - .select((vaults::id, vaults::locked_by)) + let (db_id, existing_lock_id, revealed_balance, locked_revealed) = vaults::table + .select(( + vaults::id, + vaults::locked_by, + vaults::revealed_balance, + vaults::locked_revealed_balance, + )) .filter(vaults::address.eq(&vault_str)) - .first::<(i32, Option<i32>)>(self.connection()) + .first::<(i32, Option<i32>, i64, i64)>(self.connection()) .map_err(|e| WalletStorageError::general(OPERATION, e))?; @@ // Only one lock per vault (for simplicity, but unlikely to be a real limitation) if existing_lock_id.is_some_and(|l| l != lock_id) { return Err(WalletStorageError::BadQuery { operation: OPERATION, details: format!("Vault {} is already locked by another lock", vault_id), }); } + + // Ensure sufficient revealed funds are available to lock + if revealed_balance.saturating_sub(locked_revealed) < amount_to_lock { + return Err(WalletStorageError::BadQuery { + operation: OPERATION, + details: "insufficient revealed balance to lock the requested amount".to_string(), + }); + } @@ - let changeset = vaults::locked_revealed_balance.eq(vaults::locked_revealed_balance.add(amount_to_lock)); + let changeset = ( + vaults::locked_revealed_balance.eq(vaults::locked_revealed_balance.add(amount_to_lock)), + vaults::locked_by.eq(lock_id), + );
♻️ Duplicate comments (9)
applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx (1)
146-161: Fix crash: toString on possibly undefined; guard fee display.
feeReceipt?.total_fees_paid.toString()will throw iftotal_fees_paidisundefined. Use a nullish-coalescing fallback andString(...). Also keep the overcharge UI behind the existing guard.Apply this diff:
- <DataTableCell> - {feeReceipt?.total_fees_paid.toString() || 0} + <DataTableCell> + {String(feeReceipt?.total_fees_paid ?? 0)} {feeReceipt?.total_fee_overcharge ? ( <> {" "} - ({feeReceipt.total_fee_overcharge} overcharge{" "} + ({feeReceipt.total_fee_overcharge} overcharge{" "} <BsQuestionCircle style={{ display: "inline" }} title="An overcharge occurs when paying more fees than required using stealth transfers. To preserve privacy, there is no vault to refund excess fees, therefore the fees are given to validators in their entirety." /> ) </> ) : ( "" )} </DataTableCell>applications/tari_walletd/src/services/transaction_service/service.rs (1)
157-171: Intentional semantic: return Ok(id) and emit TransactionInvalidEvent on failure—document inlineThis matches the new contract. To prevent regressions for callers expecting Err, add an inline comment clarifying the behavior.
Apply this diff:
- if transaction_api.submit_transaction(transaction_id).await? { + // NOTE: submit_transaction may return false for an invalid TX. In that case we emit + // TransactionInvalidEvent and still return Ok(transaction_id). Callers must subscribe + // to events or poll status instead of relying on Err. + if transaction_api.submit_transaction(transaction_id).await? {Run to spot any lingering call-sites that still rely on Err:
#!/bin/bash rg -nP -C2 '\bsubmit_transaction\(|handle_submit_transaction\(' applications/ | sed -n '1,200p'applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (1)
99-99: Guard optional toString to prevent runtime error.total_fees_paid can be null/undefined; current call may throw. This mirrors a prior suggestion.
- <DataTableCell>{fee_receipt?.total_fees_paid.toString() || "--"}</DataTableCell> + <DataTableCell>{fee_receipt?.total_fees_paid?.toString() ?? "--"}</DataTableCell>utilities/tariswap_test_bench/src/accounts.rs (1)
58-61: Stealth vault registration: OK; ensure downstream templates accept Stealth.The prior review flagged bench-template assertions that only accept Fungible/Confidential. Please verify they now include Stealth.
#!/bin/bash # Find places still gating on Confidential-only rg -nP 'ResourceType::Confidential\b(?!\s*\|)' utilities/ tests/ crates/ -C2 # Look for match arms that exclude Stealth rg -nP 'matches!\s*\(\s*resource_type,\s*\|\s*ResourceType::Fungible\s*\|\s*ResourceType::Confidential' -C2applications/tari_walletd/src/handlers/accounts.rs (1)
591-599: Validate range_proof before constructing the transaction.
range_proofis never verified; malformed proofs could slip through.Apply immediately after expanding
reciprocal_claim_public_key_expanded:let reciprocal_claim_public_key_expanded = RistrettoPublicKey::try_from_byte_type(&reciprocal_claim_public_key) .map_err(|e| invalid_params("claim_proof.reciprocal_claim_public_key", Some(e)))?; +if !sdk + .stealth_crypto_api() + .validate_range_proof(network, &range_proof, &commitment) +{ + return Err(invalid_params( + "claim_proof.range_proof", + Some("range proof validation failed"), + )); +}crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
33-49: Add CHECKs for non-negative numeric columns.Prevents accidental negative values from creeping in via model bugs.
- final_fee BIGINT NULL, + final_fee BIGINT NULL CHECK (final_fee >= 0), @@ - executed_time_ms BIGINT NULL, + executed_time_ms BIGINT NULL CHECK (executed_time_ms >= 0),crates/engine/src/runtime/impl.rs (1)
2666-2686: Use XTR for fee stealth-transfer resource to avoid address drift.RuntimeInterface::pay_fee currently calls execute_stealth_transfer(STEALTH_TARI_RESOURCE_ADDRESS,...). Use the canonical XTR constant as elsewhere.
- let Some(container) = state_mut.execute_stealth_transfer( - STEALTH_TARI_RESOURCE_ADDRESS.into(), + let Some(container) = state_mut.execute_stealth_transfer( + XTR.into(), statement, revealed_funds_bucket, )?crates/wallet/storage_sqlite/src/writer.rs (2)
327-339: Persist signers as hex strings; dedup + stable order (matches reader/UI expectations).Serializing raw key objects into JSON complicates filtering and can diverge from hex formats used elsewhere. Convert to hex, sort, and dedup before persisting.
Apply:
- let signers = transaction - .signatures() - .iter() - .map(|s| s.public_key()) - .chain(iter::once(transaction.seal_signature().public_key())) - .collect::<Vec<_>>(); + let mut signers_hex = transaction + .signatures() + .iter() + .map(|s| serialize_hex(s.public_key())) + .chain(iter::once(serialize_hex(transaction.seal_signature().public_key()))) + .collect::<Vec<_>>(); + signers_hex.sort(); + signers_hex.dedup();- transactions::signers.eq(serialize_json(&signers)?), + transactions::signers.eq(serialize_json(&signers_hex)?),
365-365: Use hex in update filter to match insert/reader.to_string() risks format mismatches; use serialize_hex(update.transaction_id).
- .filter(transactions::transaction_id.eq(update.transaction_id.to_string())) + .filter(transactions::transaction_id.eq(serialize_hex(update.transaction_id)))
🧹 Nitpick comments (56)
applications/tari_validator_node/src/p2p/services/mempool/error.rs (2)
15-26: Unify error message capitalization for consistency.Minor polish: align capitalization across variants.
Apply:
- #[error("Epoch Manager Error: {0}")] + #[error("Epoch manager error: {0}")] @@ - #[error("Storage Error: {0}")] + #[error("Storage error: {0}")]
29-33: Add a retryability helper to centralize caller logic.Provide a small classifier so callers can quickly decide to retry/transient-handle.
impl MempoolError { pub fn is_transaction_validator_error(&self) -> bool { matches!(self, MempoolError::TransactionValidationError(_)) } + + /// Returns true if the error is transient and a retry may succeed. + pub fn is_transient(&self) -> bool { + matches!(self, Self::RequestCancelled | Self::NetworkingError(_)) + } }applications/tari_walletd/web_ui/src/routes/Templates/Templates.tsx (1)
149-149: Prefix looks correct; add a guard and centralize the prefix constant.Avoid double-prefixing if upstream starts returning prefixed values and prevent string-literal drift across files.
Apply within this range:
- <CopyAddress address={`template_${template.address}`} /> + <CopyAddress address={withTemplatePrefix(template.address)} />Add once (outside this range, near imports or above the component):
const TEMPLATE_PREFIX = "template_"; const withTemplatePrefix = (addr: string) => addr?.startsWith(TEMPLATE_PREFIX) ? addr : `${TEMPLATE_PREFIX}${addr}`;applications/tari_validator_node/web_ui/src/routes/VN/Components/Templates.tsx (1)
146-149: Use a shared prefix helper to avoid double-prefixing and magic strings.Keeps UI consistent with future backend changes.
Apply within this range:
- template_{shortenString(address)} + {TEMPLATE_PREFIX}{shortenString(address)}- <CopyToClipboard copy={`template_${address}`} /> + <CopyToClipboard copy={withTemplatePrefix(address)} />Add once (outside this range, near imports):
const TEMPLATE_PREFIX = "template_"; const withTemplatePrefix = (addr: string) => addr?.startsWith(TEMPLATE_PREFIX) ? addr : `${TEMPLATE_PREFIX}${addr}`;applications/tari_walletd/web_ui/src/routes/Transactions/TransactionDetails.tsx (1)
171-173: Minor: simplify with nullish coalescing.More idiomatic and avoids treating empty string as falsy if that ever occurs.
Apply this diff:
- <DataTableCell> - {data.invalid_reason ? data.invalid_reason : renderResult(data?.result)} - </DataTableCell> + <DataTableCell> + {data.invalid_reason ?? renderResult(data?.result)} + </DataTableCell>applications/tari_validator_node_cli/src/command/account.rs (1)
41-41: Accept legacy--dry-runflag as a hidden no-op
Add a hidden, unused boolean field toCreateArgssotari_validator_node_cli account create --dry-runcontinues to work without breaking existing scripts:#[derive(Debug, Args, Clone)] -pub struct CreateArgs {} +pub struct CreateArgs { + #[clap(long = "dry-run", alias = "is-dry-run", hide = true)] + #[doc(hidden)] + _deprecated_dry_run: bool, +}No other literal
--dry-runinvocations were found in this CLI’s docs or scripts.applications/tari_walletd/web_ui/src/routes/Transactions/Transactions.tsx (1)
45-45: Undeclared/unused prop in signature — either use or underscore it.You pass ownerPublicKey through the tree but don’t use it here; underscore to silence unused-prop lint until wired.
-export default function Transactions({ account }: { account: Account; ownerPublicKey: string }) { +export default function Transactions({ + account, + ownerPublicKey: _ownerPublicKey, +}: { + account: Account; + ownerPublicKey: string; +}) {integration_tests/Cargo.toml (1)
52-52: Remove unused “remote” feature and relocate httpmock to dev-dependencies
OnlyMockServer::start_async().awaitis used (noconnect*calls), so theremotefeature isn’t needed; httpmock only appears in theintegration_testscrate, so declare it under[dev-dependencies].integration_tests/src/http_server.rs (1)
33-36: Optional: enrich Debug output with server address for easier test diagnostics.crates/indexer_lib/src/substate_cache.rs (1)
39-47: Avoid forcing allocations for keys; acceptimpl AsRef<str>(optional).
address: Stringforces callers to allocate. Considerimpl AsRef<str>to allow&strwithout allocation. This is a breaking change; weigh it against call sites.- fn read(&self, address: String) -> impl Future<Output = Result<Option<SubstateCacheEntry>, SubstateCacheError>> + Send + '_; + fn read(&self, address: impl AsRef<str>) -> impl Future<Output = Result<Option<SubstateCacheEntry>, SubstateCacheError>> + Send + '_; - fn write(&self, address: String, entry: &SubstateCacheEntry) -> impl Future<Output = Result<(), SubstateCacheError>> + Send + '_; + fn write(&self, address: impl AsRef<str>, entry: &SubstateCacheEntry) -> impl Future<Output = Result<(), SubstateCacheError>> + Send + '_;applications/tari_indexer/src/substate_file_cache.rs (1)
14-25: Prefer storingPathBufoverStringfor filesystem paths.Avoid lossy UTF-8 conversions and unnecessary copies. Store
PathBufand pass&Pathtocacache.#[derive(Debug, Clone)] pub struct SubstateFileCache { - cache_dir_path: String, + cache_dir_path: std::path::PathBuf, } impl SubstateFileCache { - pub fn new(path_buf: PathBuf) -> Result<Self, SubstateCacheError> { - let cache_dir_path = path_buf - .into_os_string() - .into_string() - .map_err(|_| SubstateCacheError("Invalid substate cache path".to_string()))?; - - fs::create_dir_all(&cache_dir_path) + pub fn new(path_buf: PathBuf) -> Result<Self, SubstateCacheError> { + fs::create_dir_all(&path_buf) .map_err(|e| SubstateCacheError(format!("Error creating the cache directory: {}", e)))?; - Ok(Self { cache_dir_path }) + Ok(Self { cache_dir_path: path_buf }) } }And in the impl above:
- let res = cacache::read(&self.cache_dir_path, address).await; + let res = cacache::read(&self.cache_dir_path, address).await;(no call-site changes required).
crates/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs (1)
1313-1319: Fix copy-paste in error contextError message mentions evaluate_local_accept_command in evaluate_all_accept_command.
- let diff = execution.result().finalize.any_accept().ok_or_else(|| { - HotStuffError::InvariantError(format!( - "evaluate_local_accept_command: Transaction {} has COMMIT decision but execution failed when proposing", - tx_rec.transaction_id(), - )) - })?; + let diff = execution.result().finalize.any_accept().ok_or_else(|| { + HotStuffError::InvariantError(format!( + "evaluate_all_accept_command: Transaction {} has COMMIT decision but execution failed when proposing", + tx_rec.transaction_id(), + )) + })?;applications/tari_app_utilities/src/transaction_executor.rs (1)
48-67: Avoid O(n*m) scans by precomputing DOWN setFor many inputs, repeatedly scanning diff.down_iter() is quadratic. Build a set once.
- if let Some(diff) = self.result.finalize.any_accept() { - inputs - .into_iter() - .map(|(substate_req, substate)| { - let requested_specific_version = substate_req.version().is_some(); - let lock_flag = if diff.down_iter().any(|(id, _)| id == substate_req.substate_id()) { + if let Some(diff) = self.result.finalize.any_accept() { + use std::collections::HashSet; + let downed: HashSet<_> = diff.down_iter().map(|(id, _)| id.clone()).collect(); + inputs + .into_iter() + .map(|(substate_req, substate)| { + let requested_specific_version = substate_req.version().is_some(); + let lock_flag = if downed.contains(substate_req.substate_id()) { // Update all inputs that were DOWNed to be write locked SubstateLockType::Write } else { // Any input not downed, gets a read lock SubstateLockType::Read };Add once at top if needed:
use std::collections::HashSet;crates/wallet/sdk/src/models/confidential_output.rs (1)
25-26: Breaking change: lock_id type narrowed to i32; consider backward-compatible deserializationPublic struct now serializes as i32. If older clients persisted/emit u64, deserialization may fail. Consider a custom deserializer to accept both i32 and u64.
Example helper (optional, in this file):
mod compat { use serde::{Deserialize, Deserializer}; pub fn opt_wallet_lock_id<'de, D>(d: D) -> Result<Option<i32>, D::Error> where D: Deserializer<'de> { #[derive(Deserialize)] #[serde(untagged)] enum Num { I(i32), U(u64), N(Option<serde_json::Value>) } Ok(match Num::deserialize(d)? { Num::I(i) => Some(i), Num::U(u) => i32::try_from(u).ok(), Num::N(_) => None, }) } }Then:
#[serde(default, deserialize_with = "compat::opt_wallet_lock_id")] pub lock_id: Option<WalletLockId>,crates/engine_types/src/commit_result.rs (1)
233-239: Provide a deprecation shim for accept() to ease migration.If external crates still call accept(), consider a temporary alias to reduce breakage.
Apply this minimal shim in both impls:
impl FinalizeResult { + #[deprecated(note = "use any_accept() which returns fee-only acceptance as well")] + pub fn accept(&self) -> Option<&SubstateDiff> { + self.any_accept() + } } impl TransactionResult { + #[deprecated(note = "use any_accept() which returns fee-only acceptance as well")] + pub fn accept(&self) -> Option<&SubstateDiff> { + self.any_accept() + } }crates/wallet/crypto/src/stealth.rs (3)
54-61: Minor: size_hint upper bound may be None; fallback is fine, but consider small default.Current preallocation is OK; if inputs are often small/unknown, a small fixed cap (e.g., 4 or 8) can reduce realloc churn.
86-91: Double-iteration over outputs requires Clone on Outputs. Consider a single-pass to drop Clone bound.Optional simplification: iterate once, accumulate agg_output_mask, build outputs, and collect references for proof generation.
Example without Clone bound (illustrative):
-pub fn create_outputs_statement<'a, Outputs: IntoIterator<Item = &'a UnblindedStealthOutputStatement> + Clone>( - output_statements: Outputs, +pub fn create_outputs_statement<'a, Outputs: IntoIterator<Item = &'a UnblindedStealthOutputStatement>>( + output_statements: Outputs, revealed_output_amount: Amount, ) -> Result<StealthOutputsStatement, ConfidentialProofError> { - let outputs = output_statements - .clone() - .into_iter() + let mut outputs = Vec::new(); + let mut unblinded_refs: Vec<&'a UnblindedOutputStatement> = Vec::new(); + let mut agg_output_mask = RistrettoSecretKey::default(); + for output_stmt in output_statements.into_iter() { + let unblinded_stmt = &output_stmt.statement; + unblinded_refs.push(unblinded_stmt); + agg_output_mask = agg_output_mask + &unblinded_stmt.mask; + let commitment = unblinded_stmt.to_commitment().ok_or(ConfidentialProofError::NegativeAmount)?; + outputs.push(StealthUnspentOutput { + output: UnspentOutput { + commitment: commitment.to_byte_type(), + sender_public_nonce: unblinded_stmt.sender_public_nonce.to_byte_type(), + encrypted_data: unblinded_stmt.encrypted_data.clone(), + minimum_value_promise: unblinded_stmt.minimum_value_promise, + viewable_balance_proof: unblinded_stmt + .resource_view_key + .as_ref() + .map(|vk| create_viewable_balance_proof(&unblinded_stmt.mask, unblinded_stmt.amount, &commitment, vk)) + .transpose()?, + }, + owner_public_key: output_stmt.output_owner_public_key.to_byte_type(), + tag: output_stmt.tag, + }); + } - let output_range_proof = generate_extended_bullet_proof(output_statements.into_iter().map(|o| &o.statement))?; + let output_range_proof = generate_extended_bullet_proof(unblinded_refs)?;
92-99: Zeroize aggregated secrets after use.agg_input_mask and agg_output_mask contain sensitive material; zeroize after balance_proof generation.
Example:
let balance_proof = generate_stealth_balance_proof_signature( &agg_input_mask, &agg_output_mask, &revealed_input_amount, &revealed_output_amount, ); + use zeroize::Zeroize; + let mut tmp_in = agg_input_mask; + let mut tmp_out = agg_output_mask; + tmp_in.zeroize(); + tmp_out.zeroize();clients/wallet_daemon_client/src/types.rs (1)
209-210: signer_public_key filter addition is useful; verify index support.If the backend queries by signer key, ensure indexes exist to avoid scans.
utilities/tariswap_test_bench/src/runner.rs (2)
118-121: Hard-coded wallet password in source.Even for LocalNet, prefer env or CLI to avoid committing secrets.
Example:
- let sdk_config = WalletSdkConfig { - network: Network::LocalNet, - override_keyring_password: Some(SafePassword::from_str("N3Va g0nn4 gu355").unwrap()), - }; + let override_pw = std::env::var("WALLET_PASSWORD").ok().map(SafePassword::from); + let sdk_config = WalletSdkConfig { + network: Network::LocalNet, + override_keyring_password: override_pw, + };
110-111: Avoid hard-coding network id.Consider deriving from CLI or sdk_config to keep runner consistent with selected network.
utilities/tariswap_test_bench/src/faucet.rs (1)
7-12: Missing import for Amount.Amount is used but not imported in this module. Add:
use tari_template_lib::types::Amount;crates/engine/tests/access_rules.rs (5)
501-503: Prefer expect(...) over unwrap() for clearer test failures.- .any_accept() - .unwrap() + .any_accept() + .expect("expected accept diff in finalize")
581-583: Prefer expect(...) over unwrap() for clearer test failures.- .any_accept() - .unwrap() + .any_accept() + .expect("expected accept diff in finalize")
653-655: Prefer expect(...) over unwrap() for clearer test failures.- .any_accept() - .unwrap() + .any_accept() + .expect("expected accept diff in finalize")
739-741: Prefer expect(...) over unwrap() for clearer test failures.- .any_accept() - .unwrap() + .any_accept() + .expect("expected accept diff in finalize")
878-879: Prefer expect(...) over unwrap() for clearer test failures.- .any_accept() - .unwrap() + .any_accept() + .expect("expected accept diff in finalize")utilities/tariswap_test_bench/src/tariswap.rs (1)
169-169: Use expect(...) to improve diagnostics.- let diff = result.result.any_accept().unwrap(); + let diff = result.result.any_accept().expect("No accept diff in result (tx likely rejected)");utilities/tariswap_test_bench/src/accounts.rs (3)
46-46: Missing import for Amount.This file uses Amount but doesn’t import it. Add:
use tari_template_lib::types::Amount;
176-181: Avoid repeated any_accept() calls; bind once and improve errors.- let accounts_and_state = result - .result - .any_accept() - .unwrap() + let diff = result.result.any_accept().ok_or_else(|| anyhow::anyhow!("No accept diff in finalization"))?; + let accounts_and_state = diff .up_iter()
194-199: Reuse bound diff instead of calling any_accept() again.- let vault = result - .result - .any_accept() - .unwrap() - .up_iter() + let vault = diff + .up_iter()applications/tari_walletd/src/handlers/confidential.rs (2)
62-62: Add failure-safety: ensure outputs unlock on error.Create a small guard to release locks if any later step fails.
Example pattern:
struct LockGuard<'a> { sdk: &'a WalletSdk, id: u32, armed: bool } impl<'a> Drop for LockGuard<'a> { fn drop(&mut self) { if self.armed { let _ = self.sdk.confidential_outputs_api().release_revealed_funds(self.id); let _ = self.sdk.confidential_outputs_api().release_locked_outputs(self.id); } } }Arm=false after finalize; early errors auto-unlock.
Also applies to: 70-74
191-194: Field naming mismatch: proof_id now semantically a lock_id.Either rename the API field to lock_id (breaking) or include both fields temporarily to deprecate proof_id.
Do you want me to prep a follow-up PR to add lock_id to:
- Rust response type and JSON serde aliases
- TS bindings (ProofsGenerateResponse)
- API docs?
crates/engine/tests/stealth.rs (1)
436-436: Usefinalize.any_accept()instead of accessing.result
Replaceresult.finalize.result.any_accept()withresult.finalize.any_accept()to leverage the built-in helper and keep usage consistent.applications/tari_validator_node_cli/src/command/transaction.rs (2)
251-252: Remove redundant collect.
inputsis already aVec<SubstateRequirement>; re-collecting it is a no-op.- // Convert to shard id - let inputs = inputs.into_iter().collect::<Vec<_>>(); + // Inputs are already collected
289-297: Avoid overloading dry_run_result with real finalize data.Reusing
dry_run_resultfor non-dry-run flows is confusing and leaks internal hacks to callers. Prefer extendingSubmitTransactionResponsewith an optional finalize payload whenwait_for_resultis used; keepdry_run_resultexclusively for dry-runs.Would you like a follow-up PR to add a
finalize_result: Option<...>to the client/server types and migrate call sites?crates/wallet/sdk/tests/confidential_output_api.rs (1)
82-136: Rename proof_id to lock_id for clarity.Residual naming (
proof_id) is misleading now that the type isWalletLockId.- let proof_id = test.new_lock(); + let lock_id = test.new_lock(); - let (inputs, total_value) = outputs_api - .lock_outputs_by_amount(proof_id, &Test::test_vault_address(), 50) + let (inputs, total_value) = outputs_api + .lock_outputs_by_amount(lock_id, &Test::test_vault_address(), 50) .unwrap(); @@ - .with_read_tx(|tx| tx.outputs_get_locked_by_lock_id(proof_id)) + .with_read_tx(|tx| tx.outputs_get_locked_by_lock_id(lock_id)) .unwrap(); @@ - lock_id: Some(proof_id), + lock_id: Some(lock_id), }) .unwrap(); @@ - outputs_api.finalize_outputs_for_lock(proof_id).unwrap(); + outputs_api.finalize_outputs_for_lock(lock_id).unwrap(); @@ - let locked = tx.outputs_get_locked_by_lock_id(proof_id).unwrap(); + let locked = tx.outputs_get_locked_by_lock_id(lock_id).unwrap();integration_tests/src/wallet_daemon_cli.rs (3)
872-873: Handle rejects explicitly before unwrap.
any_accept().unwrap()will panic on full reject. Prefer early error with reason to aid debugging.- add_substate_ids(world, outputs_name, resp.result.result.any_accept().unwrap()); + if let Some(reason) = resp.result.result.any_reject() { + panic!("Transfer rejected: {}", reason); + } + add_substate_ids(world, outputs_name, resp.result.result.any_accept().unwrap());
900-902: Same reject handling as transfer().- add_substate_ids(world, outputs_name, resp.result.result.any_accept().unwrap()); + if let Some(reason) = resp.result.result.any_reject() { + panic!("Confidential transfer rejected: {}", reason); + } + add_substate_ids(world, outputs_name, resp.result.result.any_accept().unwrap());
955-957: Also bail on any_reject().This helper only guards
fee_reject(). Addany_reject()to fail fast on partial rejections.- if let Some(reason) = resp.result.as_ref().and_then(|finalize| finalize.fee_reject()) { + if let Some(reason) = resp.result.as_ref().and_then(|finalize| finalize.fee_reject()) { bail!("Calling component result rejected: {}", reason); } + if let Some(reason) = resp.result.as_ref().and_then(|finalize| finalize.any_reject()) { + bail!("Calling component result rejected: {}", reason); + }applications/tari_validator_node/src/json_rpc/handlers.rs (1)
191-196: Dry-run submission guard LGTM; consider error code alignment.Blocking dry-run submissions here is correct. Optionally, consider mapping to
InvalidRequestfor stricter JSON-RPC semantics, unlessApplicationError(400)is intentional API.crates/wallet/storage_sqlite/src/schema.rs (2)
62-69: Locks table: add indexes.Add indexes to support common lookups and maintainability.
- CREATE INDEX idx_locks_created_at ON locks(created_at);
- CREATE INDEX idx_locks_tx_id ON locks(transaction_id);
172-187: Transactions: add unique/indexes.Given frequent lookups, add:
- UNIQUE INDEX uniq_transactions_txid ON transactions(transaction_id);
- INDEX idx_transactions_created_at ON transactions(created_at);
applications/tari_walletd/src/handlers/accounts.rs (1)
560-564: Log the actual signing key (claim nonce), not the account key.Current log is misleading; the transaction is sealed with the claim nonce key.
- info!( - target: LOG_TARGET, - "ℹ️ Signing claim burn with key {}. NOTE: This must be the same as the claiming key used in the burn transaction for this to succeed.", - account.owner_public_key - ); + info!( + target: LOG_TARGET, + "ℹ️ Signing claim burn with claim-nonce key {} (index {}). Must match the key used in the burn.", + claim_nonce_keypair.public_key, + owner_nonce_key_index + );crates/wallet/sdk/src/apis/transaction.rs (1)
161-169: Filter semantics: consider AND when both component and signer are provided.
ORwidens results and can surprise UIs expecting intersection.- let transactions = tx.transactions_fetch_all(status, component, signed_by_public_key)?; + let transactions = tx.transactions_fetch_all(status, component, signed_by_public_key)?; + // Consider an alternate API or flag to request AND semantics when both filters are present.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (3)
151-157: Index locks by transaction_id.Hot path for releases/finalization; avoids table scans.
CREATE TABLE locks ( id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, transaction_id TEXT NULL, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ); +CREATE INDEX locks_txid_idx ON locks (transaction_id);
128-146: Add FK for outputs.lock_id → locks(id).Keeps referential integrity on lock lifecycle.
- lock_id INTEGER NULL, + lock_id INTEGER NULL REFERENCES locks (id) ON DELETE SET NULL,
215-236: Add FK for stealth_outputs.lock_id → locks(id).Mirror of the outputs table FK.
- lock_id INTEGER NULL, + lock_id INTEGER NULL REFERENCES locks (id) ON DELETE SET NULL,crates/engine/src/transaction/processor.rs (1)
332-349: Align parameter naming with Instruction field.The argument is named revealed_input_bucket in Instruction, but this helper uses revealed_funds_bucket. Rename for consistency and easier grep.
- fn pay_fee( + fn pay_fee( runtime: &Runtime, statement: StealthTransferStatement, - revealed_funds_bucket: Option<WorkspaceOffsetId>, + revealed_input_bucket: Option<WorkspaceOffsetId>, ) -> Result<InstructionResult, TransactionError> { - let revealed_funds_bucket = revealed_funds_bucket + let revealed_funds_bucket = revealed_input_bucket .map(|id| { runtime.resolve_workspace_id(&id).and_then(|r| { r.decode().map_err(|e| RuntimeError::InvalidArgument {crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
71-79: Rename parameter for consistency.locked_id → lock_id to match the rest of the API.
- pub fn lock_outputs_until_partial_amount( + pub fn lock_outputs_until_partial_amount( &self, - locked_id: WalletLockId, + lock_id: WalletLockId, vault_id: &VaultId, amount: Amount, ) -> Result<(Vec<ConfidentialOutputModel>, Amount), ConfidentialOutputsApiError> { - self.store - .with_write_tx(|tx| self.lock_outputs_internal(tx, locked_id, vault_id, amount)) + self.store + .with_write_tx(|tx| self.lock_outputs_internal(tx, lock_id, vault_id, amount)) }
135-149: Be careful deleting the lock in both release and finalize paths.Both release_locked_outputs and finalize_outputs_for_lock call locks_delete(lock_id). If both are triggered (e.g., retries), this can surface spurious NotFound. Consider making locks_delete idempotent or ignore NotFound.
crates/engine/src/runtime/impl.rs (1)
2298-2376: Claim burn flow looks robust. Minor: avoid magic tag byte.Consider a named constant for the 0 tag for readability and future policy changes.
crates/wallet/storage_sqlite/src/writer.rs (1)
808-818: Symmetry: assert lock exists before locking confidential outputs.stealth_outputs_lock_smallest_amount calls ensure_lock_exists; do the same here for consistency and clearer errors.
) -> Result<ConfidentialOutputModel, WalletStorageError> { use crate::schema::{accounts, outputs, vaults}; + self.ensure_lock_exists(lock_id)?;crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
93-101: Align naming with storage: considerlocks_link_transactionor add alias.Public API is
locks_set_transaction_idbut storage useslocks_link_transaction. Prefer consistent terminology or expose both (mark one #[deprecated]).
105-114: Renamelocked_by_id→lock_idfor internal consistency.- pub fn lock_outputs_until_partial_amount( + pub fn lock_outputs_until_partial_amount( &self, account_address: &ComponentAddress, resource_address: &ResourceAddress, amount: Amount, - locked_by_id: WalletLockId, + lock_id: WalletLockId, ) -> Result<(Vec<StealthOutputModel>, Amount), StealthOutputsApiError> { - self.store - .with_write_tx(|tx| self.lock_outputs_internal(tx, account_address, resource_address, amount, locked_by_id)) + self.store + .with_write_tx(|tx| self.lock_outputs_internal(tx, account_address, resource_address, amount, lock_id)) }
163-172: Duplicate vault-lock APIs; deprecate one and forward to the other.Both lock the same funds via the same writer call. Keep a single canonical name to avoid ambiguity.
- pub fn lock_funds_in_vault<A: Into<Amount>>( + #[deprecated(note = "Use lock_revealed_funds instead")] + pub fn lock_funds_in_vault<A: Into<Amount>>( &self, lock_id: WalletLockId, vault_id: &VaultId, amount_to_lock: A, ) -> Result<(), StealthOutputsApiError> { - self.store - .with_write_tx(|tx| tx.vaults_lock_revealed_funds(lock_id, vault_id, amount_to_lock.into()))?; - Ok(()) + self.lock_revealed_funds(lock_id, vault_id, amount_to_lock) }Also applies to: 245-255
3addf54 to
38edd8d
Compare
2 participants
Description
feat!: migrate XTR to stealth resource
fix!: wallet bug which could lead to transactions with invalid signatures
feat!: support for stealth fee payments
fix!: update claim burn
Motivation and Context
XTR can now be claimed directly to a stealth UTXO without revealing who (any public key, vault or account) claimed it.
A caller may not generate a single use claim keypair, and use the public claim nonce in the burn. When claiming, the key index of the nonce claim key must be passed to the claim request. Note that this is optional, and another wallet implementation could simply use the account public key and key index when claiming.
How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Use swarm to burn into an account, mine 10 blocks (localnet), and submit the claim burn transaction
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores