fix(wallet)!: private derived tag and optimised* sync protocol - #1571
Conversation
WalkthroughOverhauls UTXO tagging and storage: introduces a 4‑byte UtxoTag, moves UTXO output storage to binary (bincode), adds indexer JSON‑RPC get_unspent_utxos, refactors SQLite reader/writer/models and migrations, implements a wallet UTXO scanner + recovery with notifications, and removes RevealFunds endpoints and bindings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant JS as JS/TS Bindings
participant C as Indexer Client
participant S as Indexer JSON‑RPC Server
participant H as Handlers
participant R as Storage Reader
Client->>JS: GetUnspentUtxosRequest
JS->>C: get_unspent_utxos(req)
C->>S: JSON‑RPC "get_unspent_utxos"
S->>H: dispatch(method, params)
H->>R: utxos_get_unspent_by_public_nonce_and_tag(resource, [(tag,nonce)...])
R-->>H: Vec<(UtxoId, Utxo)>
H-->>S: GetUnspentUtxosResponse { utxos }
S-->>C: JSON‑RPC result
C-->>JS: response
JS-->>Client: utxos
sequenceDiagram
autonumber
participant W as Walletd Services
participant Scan as StealthUtxoScannerWorker
participant Notif as watch::Sender
participant Recv as UtxoRecovery
participant NI as Network Interface
participant Store as Wallet Store
W->>Scan: spawn scanner worker
Scan->>Notif: subscribe_notifications()
W->>Recv: spawn UtxoRecovery::run
loop Scanner rounds
Scan->>NI: query_stealth_utxo_updates(resource, [(tag,nonce)...])
NI-->>Scan: UtxoUpdateSet
alt has updates
Scan->>Store: tx: mark spent / enqueue unspent / update shard SVs
Scan-->>Notif: notify() (new UTXOs)
end
end
loop Recovery processing
Recv->>Store: fetch queue batch
Recv->>NI: get_unspent_utxos(resource, [(tag,nonce)...])
NI-->>Recv: Vec<(UtxoId, Utxo)>
Recv->>Store: validate + upsert / remove queue items
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sdbondi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
bindings/src/helpers/helpers.ts (1)
21-47: substateIdToString lacks Template and Utxo cases — returns "Unknown"Given the PR expands UTXO handling, not supporting these here will surface "Unknown" in logs/UI.
Apply:
if ("Component" in substateId) { return substateId.Component; } + if ("Template" in substateId) { + return substateId.Template; + } if ("Resource" in substateId) { return substateId.Resource; } if ("Vault" in substateId) { return substateId.Vault; } if ("UnclaimedConfidentialOutput" in substateId) { return substateId.UnclaimedConfidentialOutput; } + if ("Utxo" in substateId) { + const u = substateId.Utxo; + return `utxo_${u.resource_address}_${u.id}`; + }crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
128-146: Add FK on lock_id, enforce status enum, and fix BLOB default.
- lock_id currently lacks referential integrity. Add FK to locks(id) with ON DELETE SET NULL.
- Encode the allowed states with a CHECK to prevent invalid values.
- Use X'' for empty BLOB defaults to avoid storing TEXT ''. Also capitalize BLOB consistently.
Apply:
CREATE TABLE confidential_outputs ( @@ - status TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('Unspent','Spent','Locked','LockedUnconfirmed','Invalid')), @@ - lock_id INTEGER NULL, - encrypted_data blob NOT NULL DEFAULT '', + lock_id INTEGER NULL REFERENCES locks (id) ON DELETE SET NULL, + encrypted_data BLOB NOT NULL DEFAULT X'', @@ ); -CREATE UNIQUE INDEX confidential_outputs_uniq_commitment ON confidential_outputs (commitment); -CREATE INDEX confidential_outputs_idx_account_status ON confidential_outputs (account_id, status); +CREATE UNIQUE INDEX confidential_outputs_uniq_commitment ON confidential_outputs (commitment); +CREATE INDEX confidential_outputs_idx_account_status ON confidential_outputs (account_id, status); +-- Optional, if queries filter by vault + status frequently: +CREATE INDEX confidential_outputs_idx_vault_status ON confidential_outputs (vault_id, status);Optional follow-ups:
- If commitments/nonces are binary elsewhere, consider BLOB (with length CHECK) for commitment/sender_public_nonce/public_asset_tag to avoid hex/text normalization issues.
Also applies to: 148-149
applications/tari_wallet_cli/src/command/account.rs (1)
56-61: Make--default/-dan explicit flag (currently unusable as a positional bool)Without
short/long,is_default: boolbecomes a positional value, which is awkward and easy to misuse. Expose it as a proper flag.Apply:
#[derive(Debug, Args, Clone)] pub struct CreateArgs { #[clap(long, alias = "name")] pub account_name: Option<String>, - pub is_default: bool, + #[clap(long = "default", short = 'd')] + pub is_default: bool, #[clap(long, short, alias = "key")] pub key_id: Option<u64>, }applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
145-154: Make resource_address required on SendMoneyDialog; update call sites and remove XTR fallbacksSendMoneyDialogProps currently declares resource_address as optional but the send code asserts it and fee estimation uses an XTR fallback. I found two usages that omit the prop: applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/Tokens.tsx:135 and applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx:59.
- Change SendMoneyDialogProps.resource_address to required (ResourceAddress) and remove non‑null assertions (
!) inside SendMoneyDialog.- Remove
|| XTRfallbacks and ensure both fee estimation and the actual transfer use the same props.resource_address.- Update all call sites to pass the intended resource_address (at least the two files above).
🧹 Nitpick comments (80)
clients/javascript/wallet_daemon_client/package.json (2)
19-19: Make clean-dist cross‑platform (rm fails on Windows).Use rimraf or shx to avoid shell‑specific rm. Also remove duplication by deleting both targets in one command.
Apply this diff:
- "clean-dist": "rm -rf dist && rm -f tsconfig.tsbuildinfo" + "clean-dist": "rimraf dist tsconfig.tsbuildinfo"Add a workspace devDependency once (preferred at repo root):
"devDependencies": { "rimraf": "^6.0.1" }
24-24: Nit: files entry should not start with a slash.Prefer "dist" over "/dist" for consistency with npm packaging patterns.
- "/dist" + "dist"applications/tari_validator_node/web_ui/package.json (2)
10-10: Make clean-dist cross‑platform.Replace rm with rimraf (or shx) to support Windows contributors.
- "clean-dist": "rm -rf dist && rm -f tsconfig.tsbuildinfo" + "clean-dist": "rimraf dist tsconfig.tsbuildinfo"If added at the workspace root, no per‑package install needed.
6-11: Optional: auto‑clean on build.Hook clean into prebuild to ensure reproducible builds.
"scripts": { "dev": "vite", "build": "tsc && vite build", + "prebuild": "npm run clean-dist", "preview": "vite preview", - "clean-dist": "rm -rf dist && rm -f tsconfig.tsbuildinfo" + "clean-dist": "rimraf dist tsconfig.tsbuildinfo" },applications/tari_indexer/web_ui/package.json (3)
10-10: Make clean-dist cross‑platform.Use rimraf (or shx) instead of rm to avoid OS‑specific failures.
- "clean-dist": "rm -rf dist && rm -f tsconfig.tsbuildinfo" + "clean-dist": "rimraf dist tsconfig.tsbuildinfo"
6-11: Optional: ensure clean builds by default.Run clean before build for consistency across packages.
"scripts": { "dev": "vite", "build": "tsc && vite build", + "prebuild": "npm run clean-dist", "preview": "vite preview", - "clean-dist": "rm -rf dist && rm -f tsconfig.tsbuildinfo" + "clean-dist": "rimraf dist tsconfig.tsbuildinfo" },
7-10: Repo-wide consistency: standardizeclean-dist(and optionalprebuild) scriptsMissing clean-dist:
- applications/tari_swarm_daemon/webui/package.json
- bindings/package.json
- package.json
- utilities/db_inspector/web_ui/package.json
Already define clean-dist (uses
rm -rf dist && rm -f tsconfig.tsbuildinfo):
- applications/tari_indexer/web_ui/package.json
- applications/tari_validator_node/web_ui/package.json
- applications/tari_walletd/web_ui/package.json
- clients/javascript/wallet_daemon_client/package.json
None define a
prebuildscript. Action: add the sameclean-distto the missing package.json files and, if desired, add a repo-wideprebuild(example:"prebuild": "npm run clean-dist") to standardize the build flow.bindings/src/helpers/helpers.ts (3)
147-149: Duplicate branch: ExecutionFailure handled twiceSecond occurrence is redundant.
- if ("ExecutionFailure" in reason) { - return `ExecutionFailure(${reason.ExecutionFailure})`; - }
114-116: Avoid ellipsis for short stringsPrevent awkward outputs when the input is shorter than start+end+3.
export function shortenString(string: string, start: number = 8, end: number = 8) { - return string.substring(0, start) + "..." + string.slice(-end); + if (string.length <= start + end + 3) { + return string; + } + return string.substring(0, start) + "..." + string.slice(-end); }
63-65: Accept legacy "commitment_" alongside new "tombstone_" (bindings/src/helpers/helpers.ts)Repo still emits/parses "commitment_" (hits in engine_types, template_lib, frontend); accept both prefixes for backward compatibility.
Apply:
case "vault": return { Vault: rest }; case "tombstone": return { UnclaimedConfidentialOutput: rest }; + case "commitment": // backward-compat + return { UnclaimedConfidentialOutput: rest }; case "txreceipt":crates/wallet/sdk_services/Cargo.toml (1)
18-19: Gate heavy crypto deps behind a feature to avoid default compile-time bloat.Make
tari_transaction_componentsandtari_cryptooptional and enable them via a scoped feature (e.g.,utxo_scanner). This keeps downstream builds lean when the scanner isn’t used.Apply this diff here, and add a feature section (see separate block below):
-tari_transaction_components = { workspace = true } -tari_crypto = { workspace = true } +tari_transaction_components = { workspace = true, optional = true } +tari_crypto = { workspace = true, optional = true }Add this outside the shown hunk:
[features] default = [] utxo_scanner = ["dep:tari_transaction_components", "dep:tari_crypto", "dep:tokio"]crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
812-818: Per-output tag derivation with nonce_secret and resource scoping—LGTM; add brief docs.Deriving via nonce_secret + resource_address matches the per-output private tag design. Consider a short doc comment to prevent future misuse (e.g., fee change must pass XTR).
@@ - fn create_output_statement( + /// Creates an unblinded stealth output statement and per-output tag. + /// Note: `resource_address` scopes the derived tag and must match the output's resource + /// (e.g., use `XTR` for fee change). + fn create_output_statement(Also applies to: 820-823
applications/tari_wallet_cli/src/command/account.rs (2)
132-139: Don’t force a refresh every time; add an opt-out flagHard-coding
refresh: truecan slow down CLI usage on large wallets. Provide a--no-refreshflag and default to refreshing to preserve current behavior.Apply:
#[derive(Debug, Args, Clone)] pub struct GetBalancesArgs { pub account_name: Option<ComponentAddressOrName>, + /// Skip server refresh for faster, possibly stale balances + #[clap(long = "no-refresh")] + pub no_refresh: bool, } async fn handle_get_balances(args: GetBalancesArgs, client: &mut WalletDaemonClient) -> Result<(), anyhow::Error> { let resp = client .get_account_balances(AccountsGetBalancesRequest { account: args.account_name, - refresh: true, + refresh: !args.no_refresh, }) .await?;
230-236: Clarify help text: accepts name or addressThe prompt says “by its name,” but the type accepts a name or address. Tweak the message.
- println!("Get account component address by its name..."); + println!("Get account component address by name or address...");applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
171-178: Comment is stale relative to behaviorText says “trigger re-estimation” on input change, but auto estimation is now only on submit. Update the comment to avoid confusion.
applications/tari_walletd/src/handlers/confidential.rs (1)
284-287: Potential compatibility risk: new key branch only.Deriving only
ElgamalEncryptionViewKeymay break viewing balances for resources created with the legacyViewKeybranch. Consider a safe fallback.Apply this minimal fallback:
- let view_key = sdk - .key_manager_api() - .derive_key(KeyBranch::ElgamalEncryptionViewKey, req.view_key_id)?; + // Prefer new branch; fallback to legacy branch for backward compatibility + let view_key = sdk + .key_manager_api() + .derive_key(KeyBranch::ElgamalEncryptionViewKey, req.view_key_id) + .or_else(|_| sdk.key_manager_api().derive_key(KeyBranch::ViewKey, req.view_key_id))?;Please confirm all existing environments have rotated to the new branch, or keep this fallback until migration is complete.
applications/tari_walletd/src/services/stealth_utxo_scanner.rs (4)
29-31: Notification plumbing looks good; consider exposing handle subscribe() publicly.The scanner exposes
pub fn subscribe_notifications, but the handle’ssubscribe_notificationsispub(crate). If external crates need to listen, promote it topub.- pub(crate) fn subscribe_notifications(&self) -> watch::Receiver<()> { + pub fn subscribe_notifications(&self) -> watch::Receiver<()> { self.notify_sub.clone() }Also applies to: 43-45, 64-75
91-94: Dead select branch: poll_fn never resolves to Ready.
poll_fut = poll_fn(|cx| self.scanner.poll(cx))always returnsPoll::Pending, so this branch is never taken. The logging comment “All work completed” won’t run here.Two options:
- Keep
poll_futsolely for side effects and remove the branch body.- Or drive polling explicitly on a ticker.
- _ = poll_fut => { - // All work completed - continue - }, + _ = poll_fut => {},
135-138: FuturesMap: good use; minor: log capacity rejections with queue depth.You already warn on
BeyondCapacity. Consider logging current in-progress count to aid ops.- Err(PushError::BeyondCapacity(_)) => { + Err(PushError::BeyondCapacity(_)) => { warn!( target: LOG_TARGET, - "Cannot queue scan for {}: maximum concurrent scans reached", - task + "Cannot queue scan for {}: max concurrent scans ({}) reached; in_progress={}", + task, + MAX_CONCURRENT_SCANS, + self.in_progress_work.len() ); },
155-168: Polling semantics: fine; clarify comment to avoid spin concern.The note about spinning could be misread. Maybe clarify that waking is driven by FuturesMap’s internal waker.
- // NOTE: do not return Ready here. The caller is polling in a loop, and if there is no work to do, the loop will - // spin. + // NOTE: Always return Pending; FuturesMap registers the waker and will wake us on completion/timeouts.crates/wallet/crypto/src/kdfs.rs (1)
85-101: UTXO tag derivation — specify little-endian, document format, and add a cross-language test vectorDerives the 4-byte tag from the 64-byte hash as little-endian — state this exact format in docs so other implementations match. Optional: simplify the code to use
from_le_bytes(...try_into())and add a cross-language test vector (sample inputs → expected UtxoTag) to lock the format.- let mut buf = [0u8; size_of::<u32>()]; - buf.copy_from_slice(&result[..size_of::<u32>()]); - let tag = u32::from_le_bytes(buf); + // Tag = first 4 bytes, little-endian + let tag = u32::from_le_bytes(result.as_ref()[..4].try_into().expect("hash output >= 4 bytes"));Location: crates/wallet/crypto/src/kdfs.rs (fn utxo_tag_stealth_dh). Callers: crates/wallet/sdk/src/apis/stealth_crypto.rs.
applications/tari_indexer/src/storage_sqlite/schema.rs (1)
95-96: Store binary blobs as Binary; indexes already presentcommitment and public_nonce are binary blobs — change them from Text -> Binary in Diesel schema and add a matching migration + model update.
diesel::table! { utxos (id) { id -> Integer, - commitment -> Text, - public_nonce -> Text, + commitment -> Binary, + public_nonce -> Binary, version -> Integer, resource_address -> Text, shard -> Integer, state_version -> BigInt, output -> Nullable<Binary>, utxo_tag -> Integer, is_spent -> Bool, is_burnt -> Bool, is_frozen -> Bool, created_at -> Timestamp, } }Indexes: migrations already define utxos_resource_state_version_shard_idx and utxos_resource_public_nonce_utxo_tag_uniq_partial in applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql (around lines 136–137), so no additional index change required.
crates/wallet/crypto/src/encrypted_data.rs (1)
53-57: Add zeroization guidance for caller-supplied encryption_key; keep signature for now.Call sites derive an AEAD key (kdfs::encrypted_data_dh_kdf_aead) and pass &encryption_key — none pass the mask as the encryption key. Add a short doc comment instructing callers to hold derived keys in a zeroizing wrapper and keep lifetimes tight. Converting the parameter to &Zeroizing is optional but would require updating callers (notably crates/wallet/sdk/src/apis/stealth_crypto.rs:125 and crates/wallet/sdk/src/apis/confidential_crypto.rs:79, plus application handlers under applications/tari_walletd).
Suggested inline doc diff (apply to crates/wallet/crypto/src/encrypted_data.rs):
-pub fn encrypt_value_and_mask( - amount: u64, - mask: &RistrettoSecretKey, - encryption_key: &RistrettoSecretKey, -) -> Result<EncryptedData, WalletCryptoError> { +/// Encrypts amount+mask using an AEAD key derived by the caller (ephemeral; should be zeroized promptly). +pub fn encrypt_value_and_mask( + amount: u64, + mask: &RistrettoSecretKey, + encryption_key: &RistrettoSecretKey, // consider &Zeroizing<RistrettoSecretKey> +) -> Result<EncryptedData, WalletCryptoError> {applications/tari_indexer/src/network_state_sync/block_scanner.rs (1)
371-389: Fix misleading log: fallback is MAX, not UNIX_EPOCH.The message says “Using UNIX_EPOCH” but returns
PrimitiveDateTime::MAX.- warn!( - target: LOG_TARGET, - "Failed to convert block timestamp to OffsetDateTime: {}. Using UNIX_EPOCH", - e - ); + warn!( + target: LOG_TARGET, + "Failed to convert block timestamp to OffsetDateTime: {}. Using PrimitiveDateTime::MAX", + e + );bindings/src/types/UtxoTag.ts (1)
1-8: Consider a small runtime guard for tag range in hand-written code.TS
numberis fine for u32, but a helper reduces misuse.Add a tiny guard:
// bindings/src/utils/isUtxoTag.ts import type { UtxoTag } from "../types/UtxoTag"; export function asUtxoTag(n: number): UtxoTag { if (!Number.isInteger(n) || n < 0 || n > 0xffffffff) { throw new RangeError("UtxoTag must be a 32-bit unsigned integer"); } return n as UtxoTag; }crates/wallet/sdk/src/models/mod.rs (1)
13-13: Public re-export widens SDK API; prefer explicit symbols to avoid accidental surface growthIf the intent is to expose only the new UTXO update types, re-export them explicitly rather than using a glob. This helps keep the public API stable and prevents accidental leaks when adding new items to
utxo_updatelater.- pub use utxo_update::*; + pub use utxo_update::{ + UtxoUpdateSet, + UtxoStateUpdateSet, + WalletUtxoUpdate, + UtxoUnspent, + UtxoSpent, + UtxoBurnt, + };Also applies to: 27-27
crates/template_lib/src/models/unspent_output.rs (1)
5-5: Type migration aligns with bindings; consider TS type hint for clarityThe move to
UtxoTagis correct. To keep generated TS aligned with the dedicated alias (bindings/src/types/UtxoTag.ts), consider annotating the field to emitUtxoTagrather than a raw number in downstream tooling.pub struct StealthUnspentOutput { pub output: UnspentOutput, /// The public key that must prove ownership of this UTXO. This is typically a one time "stealth" public key /// selected by the client. pub owner_public_key: RistrettoPublicKeyBytes, - pub tag: UtxoTag, + #[cfg_attr(feature = "ts", ts(type = "UtxoTag"))] + pub tag: UtxoTag, }Also applies to: 39-39
bindings/src/types/UtxoStateUpdateSet.ts (1)
1-5: Generated type looks goodStructure matches the wallet/update flow. Consider adding doc comments on the Rust source so ts‑rs propagates them here for client ergonomics.
bindings/src/types/tari-indexer-client/GetUnspentUtxosResponse.ts (1)
1-5: Tuple response is fine; consider named fields for client ergonomics (optional).If you want JSON with named fields, change the Rust side from
Vec<(UtxoId, Utxo)>to a struct like{ id, utxo }so ts-rs generates objects instead of tuples.applications/tari_walletd/src/handlers/settings.rs (2)
19-19: Avoid unnecessary clone in handle_get.Use the borrowed SDK consistently as done in handle_set.
- let sdk = context.wallet_sdk().clone(); + let sdk = context.wallet_sdk();
42-46: Persist/runtime update can become inconsistent on partial failure.Current order updates the runtime endpoint first, then writes config. If the config write fails, you have a changed runtime with stale persisted config. Prefer persisting first and roll back on runtime failure.
- sdk.get_network_interface().set_endpoint(&req.indexer_url)?; - sdk.config_api().set(ConfigKey::IndexerUrl, &req.indexer_url, false)?; + let prev = sdk.get_network_interface().get_endpoint().to_string(); + // Persist first + sdk.config_api().set(ConfigKey::IndexerUrl, &req.indexer_url, false)?; + // Then update runtime; revert config on failure + if let Err(e) = sdk.get_network_interface().set_endpoint(&req.indexer_url) { + let _ = sdk.config_api().set(ConfigKey::IndexerUrl, &prev, false); + return Err(anyhow::Error::from(e)); + }crates/wallet/crypto/src/stealth.rs (1)
167-167: UtxoTag adoption in tests and outputs: LGTM.Consider a property test ensuring tags round‑trip intact through statement creation/validation.
Also applies to: 185-186
applications/tari_walletd/src/services/mod.rs (1)
67-71: Wire UtxoRecovery into shutdown; avoid orphaning the task.Pass a ShutdownSignal or select! on it so recovery exits promptly on shutdown.
// Example wrapping (adjust if UtxoRecovery can accept ShutdownSignal directly) let utxo_recovery_join_handle = { let sdk = wallet_sdk.clone(); let notify_sub = utxo_scanner_handle.subscribe_notifications(); let shutdown = shutdown_signal.clone(); tokio::spawn(async move { tokio::select! { res = UtxoRecovery::new(sdk).run(notify_sub) => res, _ = shutdown => Ok(()), } }) };bindings/src/types/UtxoUpdateSet.ts (1)
6-9: Consider Record<> for dictionary-like field.Mapped type with “key in Shard” where Shard=number is equivalent but less idiomatic than Record<Shard, UtxoStateUpdateSet>. If ts-rs allows, prefer Record to make intent clearer.
Confirm JSON round-trip with numeric keys is correct across client/server for shard_updates.
crates/wallet/crypto/tests/output_statement.rs (1)
130-134: Vary tags in tests to better mirror production.Using UtxoTag::new(0) everywhere reduces coverage for tag-related flows. Consider deriving tags per output (even deterministic) to exercise code paths that depend on distinct tags.
Example minimal tweak: derive from index/amount to keep determinism.
bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts (1)
5-5: Request shape sanity-check.Commitment-based fetch aligns with “filter first, then fetch selected UTXOs.” Ensure server expects commitments (not (tag, nonce)) at this stage for parity with Rust types.
Please confirm server handler signature and that bindings/tari-indexer-client.ts uses this type.
crates/template_test_tooling/src/support/stealth.rs (2)
88-91: Don’t hard‑code zero test tags where tag behavior matters.If any tests validate indexer filtering by tag, vary this value per output to avoid false positives.
178-182: Same as above for transfer data helper.Derive a simple deterministic tag per output if these helpers are used in tag-dependent tests.
crates/wallet/sdk_services/src/utxo_scanner/mod.rs (1)
9-12: Prefer explicit re-exports over glob to keep the public API stable.Glob pub use can leak internals on future additions. Re-export specific items instead.
Can you confirm the exact list of types/errors you intend to expose from error and scanner so we can switch to explicit exports?
crates/wallet/sdk/src/apis/confidential_crypto.rs (1)
78-80: Key-derivation move is correct and aligns with decrypt path.Deriving once via kdfs and passing the key to encrypt_value_and_mask is consistent with receiver derivation.
Consider renaming parameter
secrettoprivate_keyfor consistency withderive_encrypted_data_key_for_receiver.crates/engine_types/src/utxo.rs (2)
74-76: Docstring says “tag byte” but type is now UtxoTag (4 bytes).Update the comment for accuracy.
Apply this diff:
- /// Returns the UTXO’s tag byte if the UTXO has not been burnt. + /// Returns the UTXO’s tag (4-byte value) if the UTXO has not been burnt.
150-153: Encode commitment bytes explicitly (avoid relying on Display)File: crates/engine_types/src/utxo.rs Lines: 150-153
- pub fn to_commitment_hex_string(&self) -> String { - // to_string happens to return the hex encoding of the commitment bytes. If that changes, so will this. - self.to_string() - } + pub fn to_commitment_hex_string(&self) -> String { + hex::encode(self.0) + }hex is already a workspace dependency in this crate (features = ["serde"]); no Cargo.toml change required.
crates/p2p/src/conversions/transaction.rs (1)
756-760: Proto field 'tag_byte' is uint32 and matches UtxoTag(u32); optional rename totagrecommendedcrates/p2p/proto/transaction.proto declares
uint32 tag_byteand UtxoTag is a u32 wrapper (crates/template_lib_types/src/crypto/utxo_tag.rs); conversions useUtxoTag::new(val.tag_byte)/val.tag.value()so widths are consistent — consider renaming the proto field totagfor clarity (optional).applications/tari_indexer/src/storage_sqlite/mod.rs (1)
30-31: Prefer explicit re-exports to reduce collision risk
pub use writer::*;andpub use store_factory::*;can unintentionally leak symbols or shadow types. Consider re-exporting only the intended public surface.crates/template_lib_types/src/crypto/utxo_tag.rs (3)
10-12: Guarantee transparent representation across serde/TS/FFIAdd transparent annotations so JSON/TS stay a bare number and ABI layout is guaranteed.
-#[derive(Debug, Clone, Copy, Serialize, Deserialize, Hash, PartialEq, Eq)] -#[cfg_attr(feature = "ts", derive(ts_rs::TS), ts(export))] -pub struct UtxoTag(u32); +#[derive(Debug, Clone, Copy, Serialize, Deserialize, Hash, PartialEq, Eq)] +#[repr(transparent)] +#[serde(transparent)] +#[cfg_attr(feature = "ts", derive(ts_rs::TS), ts(export, type = "number"))] +pub struct UtxoTag(u32);
14-22: Ergonomics: addFromconversionsLightweight helpers make the API nicer without overhead.
impl UtxoTag { pub const fn new(tag: u32) -> Self { Self(tag) } pub const fn value(&self) -> u32 { self.0 } } + +impl From<u32> for UtxoTag { + fn from(v: u32) -> Self { Self(v) } +} + +impl From<UtxoTag> for u32 { + fn from(t: UtxoTag) -> u32 { t.0 } +}
24-27: Log readability: show tag in zero‑padded hexHex is usually friendlier for tags/ids; keeps width fixed (4 bytes => 8 hex chars).
- write!(f, "UtxoTag({})", self.0) + write!(f, "UtxoTag(0x{:08X})", self.0)applications/tari_indexer/src/storage_sqlite/migrations/2023-02-16-145719_initial/up.sql (3)
121-134: Use BLOB for binary fields (commitment,public_nonce)Storing binary as TEXT adds conversion/space overhead. If values are hex-encoded by design, keep TEXT; otherwise prefer BLOB for both to match
output BLOB.- commitment text not NULL, - public_nonce text not NULL, + commitment blob not NULL, + public_nonce blob not NULL,
136-138: Indexing for the new lookup patternsThe partial unique index is great for exact (resource, public_nonce, utxo_tag) lookups. Consider adding a filtered non-unique index to speed scans by resource and tag during phase-1/phase-2 queries.
CREATE INDEX utxos_resource_state_version_shard_idx ON utxos (resource_address, state_version, shard); CREATE UNIQUE INDEX utxos_resource_public_nonce_utxo_tag_uniq_partial ON utxos (resource_address, public_nonce, utxo_tag) WHERE is_spent = false; +CREATE INDEX utxos_resource_utxo_tag_idx ON utxos (resource_address, utxo_tag) WHERE is_spent = false;
119-138: Race-safety of partial unique during spend/unspend transitionsEnsure writes that flip
is_spentand inserts that reuse the same(resource, public_nonce, utxo_tag)are wrapped in a single transaction (IMMEDIATE) to avoid transient uniqueness violations under concurrency.If helpful, I can draft the transaction pattern in the writer.
applications/tari_indexer/src/storage_sqlite/models/substate.rs (1)
47-56: Minor naming nitParameter is aliased to
SubstateRowbut the impl is forSubstateRecord. Consider renamingrow=>recordfor consistency.applications/tari_indexer/src/storage_sqlite/models/utxo_update.rs (1)
15-22: Consider narrowing visibility to pub(crate) if these are not a cross-crate APIIf these types are only consumed within the indexer crate, prefer crate visibility to reduce accidental external coupling.
Apply if appropriate:
-#[derive(Debug, Clone, Serialize, Deserialize)] -pub enum UtxoUpdateRecord { +#[derive(Debug, Clone, Serialize, Deserialize)] +pub(crate) enum UtxoUpdateRecord { @@ -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct UtxoUnspent { +#[derive(Debug, Clone, Serialize, Deserialize)] +pub(crate) struct UtxoUnspent { @@ -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct UtxoSpent { +#[derive(Debug, Clone, Serialize, Deserialize)] +pub(crate) struct UtxoSpent {crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (2)
16-18: Unify generic naming with the trait: use TNetworkInterface consistentlyPurely a readability/consistency nit — align the struct’s generic parameter name with the impl and bound.
-pub struct UtxoScanner<TStore, TWalletInterface> { - sdk: WalletSdk<TStore, TWalletInterface>, +pub struct UtxoScanner<TStore, TNetworkInterface> { + sdk: WalletSdk<TStore, TNetworkInterface>, } -impl<TStore, TNetworkInterface> UtxoScanner<TStore, TNetworkInterface> +impl<TStore, TNetworkInterface> UtxoScanner<TStore, TNetworkInterface> where TStore: WalletStore, TNetworkInterface: WalletNetworkInterface, TNetworkInterface::Error: IsNotFoundError + StatusResponseError,Also applies to: 20-25
30-37: Prefer WalletSdk::network() over ConfigApi::get_network()This avoids a config lookup and uses the new SDK accessor.
- let network = self.sdk.config_api().get_network()?; + let network = self.sdk.network();clients/tari_indexer_client/src/types.rs (1)
373-379: Document max length (1000) for tag_and_nonce_pairs on GetUnspentUtxosRequestServer already rejects >1000 entries; add a short doc comment to GetUnspentUtxosRequest stating the 1000-entry limit and the resulting error.
- Enforcement: applications/tari_indexer/src/json_rpc/handlers.rs (≈ lines 544–553 — returns "cannot query more than 1000 UTXOs").
- Add doc comment: clients/tari_indexer_client/src/types.rs — GetUnspentUtxosRequest (lines 373–379).
applications/tari_indexer/src/substate_manager.rs (2)
116-125: Add documentation for the new public API method.The
get_max_state_versionmethod lacks documentation explaining its purpose and parameters.+ /// Returns the maximum state version for the given resource address and shard. + /// + /// # Arguments + /// * `resource_address` - The resource address to query + /// * `shard` - The shard to query within the resource + /// + /// # Returns + /// The maximum state version found, or an error if the query fails pub fn get_max_state_version( &self, resource_address: &ResourceAddress, shard: Shard, ) -> Result<StateVersion, anyhow::Error> {
127-136: Consider validating the input array size.While the JSON-RPC handler validates that
tag_and_nonce_pairsdoesn't exceed 1000 items, this lower-level method lacks similar validation. Consider adding a reasonable upper bound check here as well to prevent potential DoS scenarios where the database query becomes excessively large.pub fn get_unspent_utxos( &self, resource_address: &ResourceAddress, public_nonce_and_tag: &[(UtxoTag, RistrettoPublicKeyBytes)], ) -> Result<Vec<(UtxoId, Utxo)>, anyhow::Error> { + const MAX_QUERY_SIZE: usize = 10000; + if public_nonce_and_tag.len() > MAX_QUERY_SIZE { + return Err(anyhow::anyhow!( + "Query size exceeds maximum allowed: {} > {}", + public_nonce_and_tag.len(), + MAX_QUERY_SIZE + )); + } let utxos = self .substate_store .with_read_tx(|tx| tx.utxos_get_unspent_by_public_nonce_and_tag(resource_address, public_nonce_and_tag))?; Ok(utxos) }crates/wallet/sdk/src/network.rs (1)
70-74: Consider documenting the expected behavior for empty tag_and_nonce_pairs.The new
get_unspent_utxosmethod should clarify what happens when an empty vector is passed.+ /// Retrieves unspent UTXOs matching the provided tag and public nonce pairs. + /// + /// # Arguments + /// * `resource_address` - The resource address to query + /// * `tag_and_nonce_pairs` - List of (tag, public_nonce) pairs to match. + /// Returns an empty vector if this list is empty. + /// + /// # Returns + /// A vector of (UtxoId, Utxo) pairs for matching unspent UTXOs fn get_unspent_utxos( &self, resource_address: ResourceAddress, tag_and_nonce_pairs: Vec<(UtxoTag, RistrettoPublicKeyBytes)>, ) -> impl Future<Output = Result<Vec<(UtxoId, Utxo)>, Self::Error>> + Send;crates/wallet/sdk/src/apis/stealth_crypto.rs (1)
89-97: Consider adding documentation explaining the sender vs recipient distinction.The new
derive_stealth_output_tag_from_sendermethod would benefit from documentation explaining when to use this versus the recipient version.+ /// Derives a stealth output tag from the sender's perspective. + /// + /// This is used when the sender (wallet owner) needs to derive the tag + /// for outputs they previously received, using their secret key and the + /// sender's public nonce. + /// + /// # Arguments + /// * `network` - The network context + /// * `secret_key` - The receiver's (wallet owner's) secret key + /// * `public_nonce` - The sender's public nonce + /// * `resource_address` - The resource address for tag derivation pub fn derive_stealth_output_tag_from_sender( &self, network: Network, secret_key: &RistrettoSecretKey, public_nonce: &RistrettoPublicKey, resource_address: &ResourceAddress, ) -> UtxoTag {crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
390-408: Consider adding metrics for burnt UTXOs.The code logs when a UTXO is burnt but doesn't update any metrics or counters. Consider tracking the number of burnt UTXOs encountered for monitoring purposes.
let is_frozen = utxo.is_frozen(); let Some(output) = utxo.output() else { debug!( target: LOG_TARGET, "Unknown Utxo output is burnt for commitment: {}. Skipping.", commitment ); + // Consider incrementing a burnt_utxos_count metric here continue; };applications/tari_indexer/src/network_state_sync/worker.rs (1)
457-467: Only enqueue Unspent when output exists; consider accounting for skipped/burnt.Logic is correct to skip when
utxo.outputisNone. Suggest incrementing a counter/metric so we can observe how often we skip burnt/malformed UTXOs.- if let Some(ref output) = utxo.output { + if let Some(ref output) = utxo.output { utxos_buf.push(UtxoUpdateRecord::Unspent(UtxoUnspent { address, version: update.version(), shard, state_version, utxo_output: output.clone(), is_frozen, })); - } + } else { + // Optional: track dropped/burnt UTXOs + // self.stats.increase_burnt_utxos(1); + }crates/wallet/sdk_services/src/indexer_jrpc_impl.rs (1)
193-204: Return type change to UtxoUpdateSet is consistent.Hot path default
per_shard_limit: 100is hard-coded; consider moving to config.- per_shard_limit: 100, + per_shard_limit: self.default_per_shard_limit(), // or from configcrates/wallet/sdk/src/storage.rs (1)
299-300: Public alias improves readability; consider doc comment.Add a brief doc comment to define ordering and intent.
+/// Pair key used to look up UTXOs: (per-output tag, owner public nonce) pub type TagAndPublicNoncePair = (UtxoTag, RistrettoPublicKeyBytes);applications/tari_indexer/src/json_rpc/handlers.rs (1)
494-534: High-watermark computation on hot path; consider cache/coalesce.You query
get_max_state_versionper shard per request. Consider caching tip versions per resource for a short TTL or piggy-backing on prior responses to reduce DB load.crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (3)
89-96: Potential copy/clone of ResourceAddress.
*resource_addrassumesResourceAddressisCopy. Preferresource_addr.clone()to avoid relying on that.- .get_unspent_utxos(*resource_addr, tag_and_nonce_pairs) + .get_unspent_utxos(resource_addr.clone(), tag_and_nonce_pairs)
209-213: Be tolerant to racing removals in the queue.If another process removed the queue item concurrently, treat NotFound as OK to avoid failing the whole recovery step.
- self.sdk.store().with_write_tx(|tx| { - tx.utxo_process_queue_remove_item(resource_address, found.output.tag, found.output.output.public_nonce) - })?; + self.sdk.store().with_write_tx(|tx| { + tx.utxo_process_queue_remove_item(resource_address.clone(), found.output.tag, found.output.output.public_nonce) + .optional() + })?;
38-69: Run loop error policy is sensible; consider jitter on backoff.Add small random jitter to the 5s sleep to avoid thundering-herd if many instances restart simultaneously.
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (2)
30-30: Consider making NUM_PRESHARDS configurableThe TODO comment indicates uncertainty about whether to fetch NUM_PRESHARDS from the network or hardcode it. Having a hardcoded value creates a potential compatibility issue if the network configuration changes.
Consider implementing a fallback mechanism:
- Try to fetch from network on first scan
- Cache the value
- Fall back to hardcoded default if network is unavailable
This would provide better flexibility while maintaining reliability.
176-194: Consider optimizing the atomic transaction scopeThe atomic write transaction includes both spent marking and UTXO queueing operations. While atomicity is important, consider whether all operations need to be in the same transaction for consistency.
If spent marking and UTXO queueing are independent operations, you could potentially split them into separate transactions to reduce lock contention:
-self.sdk.store().with_write_tx(|tx| { - // Mark UTXOs as spent (if they exist) - for spent in self.utxos_to_spend.drain(..) { - if Self::spend(tx, self.resource_address, spent)? { - num_spent += 1; - } - } - - // Queue up tag matching UTXOs for processing - tx.utxo_process_queue_extend(self.resource_address, self.utxos_to_recover.drain(..))?; - - // Update shard state versions - tx.shard_state_version_set_many( - self.account.address(), - self.resource_address, - self.shard_state_versions_to_set.drain(), - ) -})?; +// First transaction: Mark spent UTXOs +self.sdk.store().with_write_tx(|tx| { + for spent in self.utxos_to_spend.drain(..) { + if Self::spend(tx, self.resource_address, spent)? { + num_spent += 1; + } + } + Ok(()) +})?; + +// Second transaction: Queue recoveries and update versions +self.sdk.store().with_write_tx(|tx| { + tx.utxo_process_queue_extend(self.resource_address, self.utxos_to_recover.drain(..))?; + tx.shard_state_version_set_many( + self.account.address(), + self.resource_address, + self.shard_state_versions_to_set.drain(), + ) +})?;However, only make this change if you're certain the operations are truly independent.
applications/tari_indexer/src/storage_sqlite/reader.rs (1)
77-78: Consider using parameterized prefix for substate type filteringThe LIKE query construction using string concatenation could be more robust.
-let address_like = format!("{}_%", substate_type.as_prefix_str()); -query = query.filter(substates::address.like(address_like)); +// Use a bind parameter for the prefix pattern +let address_pattern = format!("{}\\_%", substate_type.as_prefix_str()); +query = query.filter(substates::address.like(address_pattern));Note the escaped underscore to ensure it's treated as a literal character rather than a wildcard.
applications/tari_walletd/src/services/recovery_service.rs (2)
199-199: Fix typo in warning messageThere's a typo in the warning message: "tio" should be "to".
-warn!(target: LOG_TARGET, "⚠️ Account {} has no owner key. This wallet may not be able tio sign for this account", account_addr); +warn!(target: LOG_TARGET, "⚠️ Account {} has no owner key. This wallet may not be able to sign for this account", account_addr);
169-186: Consider batching recovery operationsThe TODO comment on line 160 suggests batching account queries. This is a valid performance concern that should be addressed.
Batching multiple account queries would significantly improve recovery performance, especially for wallets with many accounts. Would you like me to help implement a batched query approach that fetches multiple accounts in a single network request?
applications/tari_indexer/src/storage_sqlite/writer.rs (5)
38-53: Solid write-transaction wrapper; minor panic ergonomics.The wrapper design and consumption semantics look good. Consider replacing unwrap()s with expect() for clearer panic messages during misuse.
Apply this diff:
- fn connection(&mut self) -> &mut SqliteConnection { - self.transaction.as_mut().unwrap().connection() - } + fn connection(&mut self) -> &mut SqliteConnection { + self.transaction + .as_mut() + .expect("write transaction already committed/rolled back") + .connection() + }
81-115: Batch insert may allocate large vectors; consider chunking.If updates can be large, chunk the values to avoid oversized SQL statements and memory spikes.
Example:
- diesel::insert_into(substate_transitions::table) - .values( - updates - .into_iter() - .map(|(epoch, proof)| { - ( /* fields */ ) - }) - .collect::<Vec<_>>(), - ) - .execute(self.connection()) + const CHUNK: usize = 1_000; + let mut buf = Vec::with_capacity(CHUNK); + for (epoch, proof) in updates { + buf.push(( + substate_transitions::shard.eq(shard.as_u32() as i32), + substate_transitions::state_version.eq(state_version.as_u64() as i64), + substate_transitions::epoch.eq(epoch.as_u64() as i64), + substate_transitions::substate_id.eq(proof.substate_id().to_string()), + substate_transitions::substate_type.eq(SubstateType::from(proof.substate_id()).to_string()), + substate_transitions::version.eq(proof.version() as i32), + substate_transitions::is_up.eq(proof.is_create()), + substate_transitions::value_hash.eq(proof.as_create().map(|v| serialize_hex(v.substate.value.to_value_hash(proof.version())))), + )); + if buf.len() == CHUNK { + diesel::insert_into(substate_transitions::table).values(&buf).execute(self.connection()) + .map_err(|e| StorageError::general(OPERATION, e))?; + buf.clear(); + } + } + if !buf.is_empty() { + diesel::insert_into(substate_transitions::table).values(&buf).execute(self.connection()) + .map_err(|e| StorageError::general(OPERATION, e))?; + }
116-171: Row‑by‑row inserts; use batched inserts with chunking for throughput.Single‑row inserts in a loop will be slow. Use multi‑values insert in chunks; avoid nested transactions by not wrapping in diesel::connection.transaction during the batch.
Example:
- for update in updates { + let mut inserts = Vec::new(); + let mut updates_spent = Vec::new(); + const CHUNK: usize = 1_000; + for update in updates { match update { UtxoUpdateRecord::Unspent(unspent) => { - let insert = UtxoRecordInsert { /* ... */ }; - diesel::insert_into(utxos::table) - .values(insert) - .execute(self.connection()) - .map_err(|e| StorageError::general(OPERATION, format!("insert error: {e}")))?; + inserts.push(UtxoRecordInsert { /* ... */ }); + if inserts.len() == CHUNK { + diesel::insert_into(utxos::table).values(&inserts).execute(self.connection()) + .map_err(|e| StorageError::general(OPERATION, format!("insert error: {e}")))?; + inserts.clear(); + } }, UtxoUpdateRecord::Spent(spent) => { - diesel::update(utxos::table) - .filter(utxos::resource_address.eq(resource_address)) - .filter(utxos::commitment.eq(commitment)) - .set(update) - .execute(self.connection()) - .map_err(|e| StorageError::general(OPERATION, format!("update error: {e}")))?; + updates_spent.push((resource_address, commitment, UtxoRecordUpdate { /* ... */ })); + if updates_spent.len() == CHUNK { + for (ra, c, u) in updates_spent.drain(..) { + diesel::update(utxos::table) + .filter(utxos::resource_address.eq(ra)) + .filter(utxos::commitment.eq(c)) + .set(u) + .execute(self.connection()) + .map_err(|e| StorageError::general(OPERATION, format!("update error: {e}")))?; + } + } }, } - } + } + if !inserts.is_empty() { + diesel::insert_into(utxos::table).values(&inserts).execute(self.connection()) + .map_err(|e| StorageError::general(OPERATION, format!("insert error: {e}")))?; + } + for (ra, c, u) in updates_spent { + diesel::update(utxos::table) + .filter(utxos::resource_address.eq(ra)) + .filter(utxos::commitment.eq(c)) + .set(u) + .execute(self.connection()) + .map_err(|e| StorageError::general(OPERATION, format!("update error: {e}")))?; + }
173-214: Race‑free upsert for substates.Replace read‑then‑write with ON CONFLICT DO UPDATE to avoid races and reduce round‑trips.
Example:
- match current_substate { - Some(_) => { - diesel::update(substates::table) - .set(&new_substate) - .filter(substates::address.eq(address)) - .execute(self.connection()) - .map_err(|e| StorageError::QueryError { - reason: format!("Update leaf node: {}", e), - })?; - debug!(target: LOG_TARGET, "Updated substate {} version to {}", address, new_substate.version); - }, - None => { - diesel::insert_into(substates::table) - .values(&new_substate) - .execute(self.connection()) - .map_err(|e| StorageError::QueryError { - reason: format!("Update substate error: {}", e), - })?; - info!(target: LOG_TARGET, "Added new substate {} with version {}", address, new_substate.version); - }, - }; + use crate::storage_sqlite::schema::substates::dsl as s; + diesel::insert_into(substates::table) + .values(&new_substate) + .on_conflict(s::address) + .do_update() + .set(&new_substate) + .execute(self.connection()) + .map_err(|e| StorageError::QueryError { reason: format!("upsert substate: {}", e) })?;
216-242: Event inserts are row‑by‑row; batch for throughput.Collect NewEvent records and insert in chunks to cut round‑trips.
Apply this diff:
- for result in events { - let event = result?; - - diesel::insert_into(events::table) - .values(event) - .execute(self.connection()) - .map_err(|e| StorageError::QueryError { - reason: format!("{OPERATION}: {}", e), - })?; - } + const CHUNK: usize = 1_000; + let mut buf = Vec::new(); + for result in events { + buf.push(result?); + if buf.len() == CHUNK { + diesel::insert_into(events::table).values(&buf).execute(self.connection()) + .map_err(|e| StorageError::QueryError { reason: format!("{OPERATION}: {}", e) })?; + buf.clear(); + } + } + if !buf.is_empty() { + diesel::insert_into(events::table).values(&buf).execute(self.connection()) + .map_err(|e| StorageError::QueryError { reason: format!("{OPERATION}: {}", e) })?; + }applications/tari_indexer/src/storage_sqlite/models/utxo.rs (3)
83-88: Error context strings are inaccurate.The decode target is UtxoOutput and the source is bincode, not a string.
Apply:
- let output = deserialize_bincode::<UtxoOutput, _>(output).map_err(|e| StorageError::DecodingError { - operation: "UtxoRecord::try_convert", - item: "Utxo", - details: format!("Failed to parse Utxo from string: {}", e), - })?; + let output = deserialize_bincode::<UtxoOutput, _>(output).map_err(|e| StorageError::DecodingError { + operation: "UtxoRecord::try_convert_to_update", + item: "UtxoOutput", + details: format!("Failed to decode UtxoOutput (bincode): {}", e), + })?;
103-109: Same inaccurate error strings in try_convert_to_utxo.Apply:
- output: self.output.as_ref().map(deserialize_bincode).transpose().map_err(|e| { - StorageError::DecodingError { - operation: "UtxoRecord::try_convert", - item: "Utxo", - details: format!("Failed to parse Utxo from string: {}", e), - } - })?, + output: self.output.as_ref().map(deserialize_bincode).transpose().map_err(|e| { + StorageError::DecodingError { + operation: "UtxoRecord::try_convert_to_utxo", + item: "UtxoOutput", + details: format!("Failed to decode UtxoOutput (bincode): {}", e), + } + })?,
116-124: Wrong operation/item labels in to_utxo_id error.Apply:
- let commitment = - PedersenCommitmentBytes::from_hex(&self.commitment).map_err(|e| StorageError::DecodingError { - operation: "UtxoRecord::to_address", - item: "UtxoAddress", - details: format!("Failed to parse Commitment from string: {}", e), - })?; + let commitment = PedersenCommitmentBytes::from_hex(&self.commitment).map_err(|e| StorageError::DecodingError { + operation: "UtxoRecord::to_utxo_id", + item: "UtxoId", + details: format!("Failed to parse Commitment from hex: {}", e), + })?;
| pub async fn get_unspent_utxos(&self, value: JsonRpcExtractor) -> JrpcResult { | ||
| let answer_id = value.get_answer_id(); | ||
| let req: GetUnspentUtxosRequest = value.parse_params()?; | ||
| if req.tag_and_nonce_pairs.len() > 1000 { | ||
| return Err(JsonRpcResponse::error( | ||
| answer_id, | ||
| JsonRpcError::new( | ||
| JsonRpcErrorReason::InvalidParams, | ||
| "cannot query more than 1000 UTXOs".to_string(), | ||
| Value::Null, | ||
| ), | ||
| )); | ||
| } | ||
| let utxos = self | ||
| .substate_manager | ||
| .get_unspent_utxos(&req.resource_address, &req.tag_and_nonce_pairs) | ||
| .map_err(|e| { | ||
| Self::internal_error( | ||
| answer_id, | ||
| format!( | ||
| "Error getting UTXOs for resource_address {}, with {} tag/nonce pair(s): {}", | ||
| req.resource_address, | ||
| req.tag_and_nonce_pairs.len(), | ||
| e | ||
| ), | ||
| ) | ||
| })?; | ||
|
|
||
| Ok(JsonRpcResponse::success(answer_id, GetUnspentUtxosResponse { utxos })) | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New get_unspent_utxos handler: validate shape, add dedup, and reconcile bindings.
- Enforces ≤1000 items — good.
- Consider deduplicating
(tag, nonce)pairs to avoid redundant DB hits and reduce response size. - Ensure TS/Rust bindings use
tag_and_nonce_pairsconsistently (some bindings still referencecommitments).
- let req: GetUnspentUtxosRequest = value.parse_params()?;
+ let req: GetUnspentUtxosRequest = value.parse_params()?;
+ // Optional: dedupe to reduce load
+ // let mut pairs = req.tag_and_nonce_pairs.clone();
+ // pairs.sort_unstable();
+ // pairs.dedup();
+ // if pairs.len() != req.tag_and_nonce_pairs.len() { /* maybe log */ }🏁 Script executed:
#!/bin/bash
# Check TS bindings for request/response shape parity
rg -nP --type=ts 'GetUnspentUtxos(Request|Response)' -C3
rg -nP --type=ts 'tag_and_nonce_pairs|commitments' -n -C2 bindings/Length of output: 5054
Reconcile TS bindings with new request shape; optionally dedupe (tag,nonce) pairs
- Optional: deduplicate (tag, nonce) pairs to avoid redundant DB hits — suggested diff below.
- Critical: TypeScript bindings still use "commitments" — update bindings to use "tag_and_nonce_pairs" with the correct element type. See bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts (export in bindings/src/tari-indexer-client.ts).
- let req: GetUnspentUtxosRequest = value.parse_params()?;
+ let req: GetUnspentUtxosRequest = value.parse_params()?;
+ // Optional: dedupe to reduce load
+ // let mut pairs = req.tag_and_nonce_pairs.clone();
+ // pairs.sort_unstable();
+ // pairs.dedup();
+ // if pairs.len() != req.tag_and_nonce_pairs.len() { /* maybe log */ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn get_unspent_utxos(&self, value: JsonRpcExtractor) -> JrpcResult { | |
| let answer_id = value.get_answer_id(); | |
| let req: GetUnspentUtxosRequest = value.parse_params()?; | |
| if req.tag_and_nonce_pairs.len() > 1000 { | |
| return Err(JsonRpcResponse::error( | |
| answer_id, | |
| JsonRpcError::new( | |
| JsonRpcErrorReason::InvalidParams, | |
| "cannot query more than 1000 UTXOs".to_string(), | |
| Value::Null, | |
| ), | |
| )); | |
| } | |
| let utxos = self | |
| .substate_manager | |
| .get_unspent_utxos(&req.resource_address, &req.tag_and_nonce_pairs) | |
| .map_err(|e| { | |
| Self::internal_error( | |
| answer_id, | |
| format!( | |
| "Error getting UTXOs for resource_address {}, with {} tag/nonce pair(s): {}", | |
| req.resource_address, | |
| req.tag_and_nonce_pairs.len(), | |
| e | |
| ), | |
| ) | |
| })?; | |
| Ok(JsonRpcResponse::success(answer_id, GetUnspentUtxosResponse { utxos })) | |
| } | |
| pub async fn get_unspent_utxos(&self, value: JsonRpcExtractor) -> JrpcResult { | |
| let answer_id = value.get_answer_id(); | |
| let req: GetUnspentUtxosRequest = value.parse_params()?; | |
| // Optional: dedupe to reduce load | |
| // let mut pairs = req.tag_and_nonce_pairs.clone(); | |
| // pairs.sort_unstable(); | |
| // pairs.dedup(); | |
| // if pairs.len() != req.tag_and_nonce_pairs.len() { /* maybe log */ } | |
| if req.tag_and_nonce_pairs.len() > 1000 { | |
| return Err(JsonRpcResponse::error( | |
| answer_id, | |
| JsonRpcError::new( | |
| JsonRpcErrorReason::InvalidParams, | |
| "cannot query more than 1000 UTXOs".to_string(), | |
| Value::Null, | |
| ), | |
| )); | |
| } | |
| let utxos = self | |
| .substate_manager | |
| .get_unspent_utxos(&req.resource_address, &req.tag_and_nonce_pairs) | |
| .map_err(|e| { | |
| Self::internal_error( | |
| answer_id, | |
| format!( | |
| "Error getting UTXOs for resource_address {}, with {} tag/nonce pair(s): {}", | |
| req.resource_address, | |
| req.tag_and_nonce_pairs.len(), | |
| e | |
| ), | |
| ) | |
| })?; | |
| Ok(JsonRpcResponse::success(answer_id, GetUnspentUtxosResponse { utxos })) | |
| } |
🤖 Prompt for AI Agents
In applications/tari_indexer/src/json_rpc/handlers.rs around lines 544-574, the
review requests two items: (1) update the TypeScript bindings to replace the old
"commitments" field with "tag_and_nonce_pairs" using the correct element type
(change bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts and
re-export in bindings/src/tari-indexer-client.ts accordingly), and (2)
optionally deduplicate (tag, nonce) pairs before querying the DB to avoid
redundant lookups — implement a small dedupe step on req.tag_and_nonce_pairs
(preserve order if needed) and pass the deduplicated list to
substate_manager.get_unspent_utxos, keeping the existing error handling and
response unchanged.
| "inspect_substate" => handlers.inspect_substate(value).await, | ||
| "get_non_fungibles" => handlers.get_non_fungibles(value).await, | ||
| "get_utxo_updates" => handlers.get_utxo_updates(value).await, | ||
| "get_unspent_utxos" => handlers.get_unspent_utxos(value).await, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add discovery/permission entries for get_unspent_utxos.
Ensure rpc.discover output and any JrpcPermissions map include "get_unspent_utxos" so clients can introspect and authorize it.
🏁 Script executed:
#!/bin/bash
# Verify handler exists and is referenced by rpc.discover/permissions
rg -nP 'get_unspent_utxos' applications/tari_indexer/src/json_rpc/handlers.rs -n -C3
rg -nP 'rpc\\.discover|JrpcPermissions' applications/tari_indexer/src/json_rpc -n -C3Length of output: 563
Add rpc.discover and JrpcPermissions entries for "get_unspent_utxos"
get_unspent_utxos handler exists (applications/tari_indexer/src/json_rpc/handlers.rs:544–560) and is registered in the server mapping (applications/tari_indexer/src/json_rpc/server.rs), but no rpc.discover output or JrpcPermissions entry advertising "get_unspent_utxos" was found under applications/tari_indexer/src/json_rpc — add the method to rpc.discover and the JrpcPermissions map so clients can introspect and authorize it.
| impl<TStore> Clone for KeyManagerApi<'_, TStore> { | ||
| fn clone(&self) -> Self { | ||
| *self | ||
| } | ||
| } | ||
|
|
||
| impl<TStore> Copy for KeyManagerApi<'_, TStore> {} | ||
|
|
There was a problem hiding this comment.
Impls won’t compile with lifetime '_ in impl header; use a named lifetime.
impl<TStore> Clone for KeyManagerApi<'_, TStore> and Copy for KeyManagerApi<'_, TStore> are invalid; the impl must name the lifetime. Also, since the type is Copy, the Clone body returning *self is fine.
Apply this diff:
-impl<TStore> Clone for KeyManagerApi<'_, TStore> {
+impl<'a, TStore> Clone for KeyManagerApi<'a, TStore> {
fn clone(&self) -> Self {
*self
}
}
-impl<TStore> Copy for KeyManagerApi<'_, TStore> {}
+impl<'a, TStore> Copy for KeyManagerApi<'a, TStore> {}Alternative: add #[derive(Clone, Copy)] on the struct and delete these impls.
📝 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.
| impl<TStore> Clone for KeyManagerApi<'_, TStore> { | |
| fn clone(&self) -> Self { | |
| *self | |
| } | |
| } | |
| impl<TStore> Copy for KeyManagerApi<'_, TStore> {} | |
| impl<'a, TStore> Clone for KeyManagerApi<'a, TStore> { | |
| fn clone(&self) -> Self { | |
| *self | |
| } | |
| } | |
| impl<'a, TStore> Copy for KeyManagerApi<'a, TStore> {} |
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/key_manager.rs around lines 260–267 the impl
headers use the anonymous lifetime `'_`, which is invalid; change them to name
the lifetime (e.g. add a lifetime parameter and write `impl<'a, TStore> Clone
for KeyManagerApi<'a, TStore>` and `impl<'a, TStore> Copy for KeyManagerApi<'a,
TStore>`), keeping the `Clone` implementation returning `*self`; alternatively
remove these manual impls and add `#[derive(Clone, Copy)]` to the KeyManagerApi
struct definition to achieve the same effect.
| .then(|| { | ||
| self.create_output_statement( | ||
| &owner_account.to_ristretto_public_key(), | ||
| fee_change, | ||
| ¶ms.resource_address, | ||
| None, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Fee change uses params.resource_address instead of XTR (wrong resource/tag scope).
Fee change outputs are XTR-denominated. Passing params.resource_address here derives the wrong tag and records the output under the wrong resource, making the fee change unspendable/unscannable. Use XTR for both tag derivation and persistence.
Apply this diff:
@@
- .then(|| {
- self.create_output_statement(
- &owner_account.to_ristretto_public_key(),
- fee_change,
- ¶ms.resource_address,
- None,
- )
- })
+ .then(|| {
+ self.create_output_statement(
+ &owner_account.to_ristretto_public_key(),
+ fee_change,
+ &XTR,
+ None,
+ )
+ })
@@
if let Some(ref fee_change) = fee_change_output_statement {
self.add_unconfirmed_output_from_statement(
lock_id,
¶ms.owner_account,
- params.resource_address,
+ XTR,
fee_change,
)?;
}Also applies to: 487-494
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/stealth_transfer.rs around lines 456-463 (and
similarly 487-494), the fee-change output is being created with
params.resource_address which derives the wrong tag/resource; change the code to
use the XTR resource for both tag derivation and persistence when creating
fee-change outputs. Replace the params.resource_address argument with the XTR
resource/address (the project-wide XTR constant or the params.xtr_resource
field) wherever fee_change outputs are made so the tag and stored resource are
XTR.
| types::{crypto::UtxoTag, Amount}, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Field still named tag_byte but now UtxoTag (4 bytes) — rename for clarity and future-proofing.
Keeps code compiling but is misleading. Suggest renaming to tag and keeping a serde alias for back/forward compat.
- pub tag_byte: UtxoTag,
+ #[serde(alias = "tag_byte")]
+ pub tag: UtxoTag,Also applies to: 22-22
🏁 Script executed:
#!/bin/bash
# Update call sites
rg -nP '\btag_byte\b' --type=rustLength of output: 830
Rename field tag_byte → tag in SDK model and update all callers (keep serde alias for compatibility)
Renaming only the SDK struct field will break multiple call-sites and DB mappings; change the field and update these locations.
- crates/wallet/sdk/src/models/stealth_output.rs:22 — change field (see diff).
- crates/wallet/sdk/src/apis/stealth_outputs.rs:542; crates/wallet/sdk/src/apis/stealth_transfer.rs:757 — struct literals using
tag_byte. - crates/wallet/storage_sqlite/src/models/stealth_output.rs:29,73 — storage model and conversion.
- crates/wallet/storage_sqlite/src/writer.rs:1061; crates/wallet/storage_sqlite/src/schema.rs:147 — writer and DB schema.
- crates/p2p/src/conversions/transaction.rs:759,769 — conversion sites using
tag_byte.
Suggested change (apply and update callers accordingly):
- pub tag_byte: UtxoTag,
+ #[serde(alias = "tag_byte")]
+ pub tag: UtxoTag,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/models/stealth_output.rs around lines 8-9 and line 22,
rename the struct field tag_byte → tag and add serde compatibility (e.g.
#[serde(alias = "tag_byte")] on the new tag field) so external JSON still
accepts the old name; then update all call sites to use .tag instead of
.tag_byte: crates/wallet/sdk/src/apis/stealth_outputs.rs (around 542) and
crates/wallet/sdk/src/apis/stealth_transfer.rs (around 757) where struct
literals use tag_byte, crates/wallet/storage_sqlite/src/models/stealth_output.rs
(lines ~29 and ~73) to reflect the storage model and conversion changes, update
writer and schema references in crates/wallet/storage_sqlite/src/writer.rs
(~1061) and crates/wallet/storage_sqlite/src/schema.rs (~147), and update
conversion sites in crates/p2p/src/conversions/transaction.rs (around 759 and
769); ensure conversions, DB column usage, and any struct construction use the
new field name and keep the serde alias for backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
128-146: Add status CHECK and FK for lock_id; consider binary types for cryptographic fields
- Enforce allowed statuses and maintain referential integrity to locks.
- Optional: store commitment/sender_public_nonce/public_asset_tag as BLOBs with length CHECKs or normalize hex with CHECK(lower(...)=...).
CREATE TABLE confidential_outputs ( @@ - status TEXT NOT NULL, + status TEXT NOT NULL CHECK ( + status IN ('Unspent','Spent','Locked','LockedUnconfirmed','Invalid') + ), @@ - lock_id INTEGER NULL, + lock_id INTEGER NULL REFERENCES locks (id) ON DELETE SET NULL, @@ - encrypted_data blob NOT NULL DEFAULT '', + encrypted_data BLOB NOT NULL DEFAULT '' );Optional (if you keep hex TEXT): add normalizers.
- commitment TEXT NOT NULL, + commitment TEXT NOT NULL CHECK (commitment = lower(commitment)), - sender_public_nonce TEXT NULL, + sender_public_nonce TEXT NULL CHECK (sender_public_nonce IS NULL OR sender_public_nonce = lower(sender_public_nonce)), - public_asset_tag TEXT NULL, + public_asset_tag TEXT NULL CHECK (public_asset_tag IS NULL OR public_asset_tag = lower(public_asset_tag)),
♻️ Duplicate comments (4)
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (2)
267-268: Uniqueness now includes account dimension — resolvedThe UNIQUE index includes account_key_index; this fixes cross‑account de‑dupe collisions noted earlier.
256-266: Harden utxo_tag domain and add dequeue helper index
- Constrain 4‑byte tag range explicitly.
- Add helper index for account/resource ordered dequeues.
CREATE TABLE utxo_process_queue ( @@ - utxo_tag INT NOT NULL, + utxo_tag INTEGER NOT NULL CHECK (utxo_tag BETWEEN 0 AND 4294967295), public_nonce TEXT NOT NULL, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ); + +-- Speeds up scans like: WHERE account_key_index=? AND resource_address=? ORDER BY created_at +CREATE INDEX utxo_process_queue_idx_account_resource_created_at + ON utxo_process_queue (account_key_index, resource_address, created_at);Optional: store public_nonce as BLOB with a length CHECK, or enforce hex normalization via CHECK(public_nonce = lower(public_nonce)).
crates/wallet/sdk/src/apis/key_manager.rs (2)
47-47: Fix branch-name mismatch: breaks serde/TS/DB interop.
serde(rename_all = "snake_case")will serializeElgamalEncryptionViewKeyaselgamal_encryption_view_key, and TS bindings use the same. Returningelgamal_view_keyhere will cause wrong branch lookups and persisted rows under a third, incompatible name. Update the mapping.Apply this diff:
- Self::ElgamalEncryptionViewKey => "elgamal_view_key", + Self::ElgamalEncryptionViewKey => "elgamal_encryption_view_key",Run to verify consistency across Rust/TS and spot any lingering pluralization drift for Transaction:
#!/bin/bash # Elgamal view key strings across repo rg -nP 'elgamal_(encryption_)?view_key' -C2 # "transaction" vs "transactions" across Rust/TS (ensure intended) rg -nP '\btransactions?\b' bindings src -C2 --type-add 'ts:*.ts' --type-add 'rs:*.rs'
257-264: impls won’t compile with'_lifetime in impl header.Use a named lifetime or derive on the struct.
Apply this minimal fix:
-impl<TStore> Clone for KeyManagerApi<'_, TStore> { +impl<'a, TStore> Clone for KeyManagerApi<'a, TStore> { fn clone(&self) -> Self { *self } } -impl<TStore> Copy for KeyManagerApi<'_, TStore> {} +impl<'a, TStore> Copy for KeyManagerApi<'a, TStore> {}Optional: alternatively add
#[derive(Clone, Copy)]toKeyManagerApiand remove these manual impls.
🧹 Nitpick comments (3)
bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts (2)
2-4: Clarify “nonce” typing to avoid semantic confusion.You’re using
RistrettoPublicKeyBytesfor the “public_nonce”. That’s likely correct byte-wise but semantically ambiguous. Consider introducing an aliasPublicNonceBytes = RistrettoPublicKeyBytes(outside generated files) and using it in consumer-facing APIs/docs for clarity.
6-9: Constrain UtxoTag to u32 and document encoding expectations.
- TS
numberis IEEE‑754; add a guard where requests are built to ensure0 <= tag <= 0xFFFF_FFFFand integer.- Confirm tuple order
[tag, nonce]matches the server expectation.- Since
nonceis hex, actual JSON payloads exceed the “36 bytes/UTXO” claim—fine for now per PR notes, but document that the 36 bytes refers to a future binary path.Here’s a small helper you can add outside generated files:
export function makeGetUnspentUtxosRequest( resource_address: ResourceAddress, pairs: Array<[UtxoTag, RistrettoPublicKeyBytes]> ): GetUnspentUtxosRequest { const isU32 = (n: number) => Number.isInteger(n) && n >= 0 && n <= 0xFFFF_FFFF; for (const [tag, nonce] of pairs) { if (!isU32(tag)) throw new Error("UtxoTag must be a u32 (0..=2^32-1)"); if (!/^[0-9a-fA-F]{64}$/.test(nonce)) throw new Error("Nonce must be 32-byte hex"); } return { resource_address, tag_and_nonce_pairs: pairs }; }bindings/src/types/UtxoUnspent.ts (1)
2-3: Canonicalize and validate public_nonce encoding.RistrettoPublicKeyBytes is a string; confirm canonical format (e.g., 32-byte hex, lowercase) and add a lightweight validator at ingestion boundaries.
Example helper (no deps):
export function validateUtxoUnspent(u: any): asserts u is import("../types/UtxoUnspent").UtxoUnspent { if (!u || typeof u !== "object") throw new Error("invalid object"); const tag = (u as any).tag; if (!Number.isInteger(tag) || tag < 0 || tag > 0xFFFFFFFF) throw new Error("tag out of u32 range"); const pn = (u as any).public_nonce; if (typeof pn !== "string" || !/^[0-9a-f]{64}$/i.test(pn)) throw new Error("public_nonce must be 32-byte hex"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/src/types/UtxoUnspent.ts(1 hunks)bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeyBranch.ts(1 hunks)crates/wallet/sdk/src/apis/key_manager.rs(3 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/src/types/wallet-daemon-client/KeyBranch.ts
🧰 Additional context used
🧬 Code graph analysis (2)
bindings/src/types/UtxoUnspent.ts (2)
bindings/src/types/UtxoTag.ts (1)
UtxoTag(8-8)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts (3)
bindings/src/types/UtxoTag.ts (1)
UtxoTag(8-8)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: machete
- GitHub Check: fmt
- GitHub Check: clippy
🔇 Additional comments (6)
bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts (1)
1-1: Generated file notice acknowledged — no manual edits.Header is correct and prevents accidental modifications.
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (2)
148-149: Indexing on (account_id, status) looks goodGood selectivity for common wallet queries; no changes requested.
254-255: Shard state version composite index is appropriateThe added covering index supports ordered/range queries per (account, resource, shard); keep as-is.
crates/wallet/sdk/src/apis/key_manager.rs (1)
31-33: ElgamalEncryptionViewKey variant: LGTM.Docs/readability are clear; placement in the enum makes sense.
bindings/src/types/UtxoUnspent.ts (2)
5-5: Treat UtxoUnspent.tag as a full u32; avoid truthiness checks.tag can be 0x00000000 (0). Any if (x.tag), x.tag && ..., !!x.tag, or x.tag ? ... checks will drop valid UTXOs — use explicit checks (x.tag !== undefined && x.tag !== null) or explicit numeric/range/enum comparisons instead.
The provided search returned no matches; run this broader search and fix any hits:
rg -nP --type=ts -C2 \ -e '\bif\s*\(\s*[^)]*\.tag\s*\)' \ -e '\b[A-Za-z_]\w*\.tag\s*&&' \ -e '\b[A-Za-z_]\w*\.tag\s*\?\s*' \ -e '!!\s*[A-Za-z_]\w*\.tag\b' || true
5-5: No TS callsites found — resolve.bindings/src/types/UtxoUnspent.ts now exports only { tag, public_nonce }; repo-wide search shows UtxoUnspent referenced only in generated bindings and Rust code—no TypeScript/JS consumers access utxo, address, version, shard, or state_version.
Test Results (CI)419 tests +24 413 ✅ +18 1h 18m 5s ⏱️ + 32m 23s For more details on these failures, see this check. Results for commit 1aa6f63. ± Comparison against base commit 2c938d1. ♻️ This comment has been updated with latest results. |
baead0e to
1aa6f63
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
256-267: utxo_tag width is too small for 4-byte tags; add range check and normalize nonceUtxoTag is up to 4,294,967,295. Storing as INT (mapped to 32-bit in Diesel) risks overflow in code and incorrect matching. Use BIGINT with a CHECK. Also normalize public_nonce or store as BLOB.
CREATE TABLE utxo_process_queue ( id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, account_key_index BIGINT NOT NULL, resource_address TEXT NOT NULL, - utxo_tag INT NOT NULL, - public_nonce TEXT NOT NULL, + utxo_tag BIGINT NOT NULL CHECK (utxo_tag BETWEEN 0 AND 4294967295), + -- Prefer BLOB with a length CHECK if fixed size; otherwise normalize case + public_nonce TEXT NOT NULL CHECK (public_nonce = lower(public_nonce)), created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ); CREATE UNIQUE INDEX utxo_process_queue_account_resource_tag_nonce_uniq ON utxo_process_queue (account_key_index, resource_address, utxo_tag, public_nonce); +-- Optional helper for account/resource dequeue scans +-- CREATE INDEX utxo_process_queue_idx_account_resource_created_at +-- ON utxo_process_queue (account_key_index, resource_address, created_at);
🧹 Nitpick comments (9)
bindings/src/types/UtxoUnspent.ts (2)
5-5: Strengthen typing for 4‑byte tag and 32‑byte nonce; add branding/validation.The generated types are
numberandstring, which are easy to misuse. Don’t edit this file; add a thin, hand‑written wrapper type and runtime guards at the boundary.Example (new file bindings/src/public-types.ts):
// brands.ts export type Uint32 = number & { readonly __uint32: unique symbol }; export type Hex32Bytes = string & { readonly __hex32: unique symbol }; // 32 bytes => 64 hex chars // public-types.ts import type { UtxoUnspent as GenUtxoUnspent } from "./types/UtxoUnspent"; import { z } from "zod"; export type UtxoUnspent = { tag: Uint32; public_nonce: Hex32Bytes }; export const UtxoUnspentSchema = z.object({ tag: z.number().int().min(0).max(0xFFFF_FFFF), public_nonce: z.string().regex(/^[0-9a-fA-F]{64}$/, "expected 32-byte hex"), }); export function parseUtxoUnspent(v: unknown): UtxoUnspent { const p = UtxoUnspentSchema.parse(v); return p as UtxoUnspent; }Then import
UtxoUnspentfrompublic-typesin app code, keeping this generated file untouched.
5-5: Clarify encoding forpublic_nonce.
RistrettoPublicKeyBytesisstring; specify hex vs base64 to avoid cross‑service mismatch. Prefer documenting via Rust struct docs or a serde/ts‑rs alias (e.g., rename toRistrettoPublicKeyHex), or enforce via the runtime schema above.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (2)
128-149: Add supporting index for hot-path lookup and fix BLOB default
- Writer uses vault_id + status + value ordering to pick smallest UTXO. Add a covering index to avoid full scans.
- For encrypted_data, use X'' (empty BLOB) instead of '' (empty TEXT) as the default.
CREATE TABLE confidential_outputs ( @@ - encrypted_data blob NOT NULL DEFAULT '', + encrypted_data BLOB NOT NULL DEFAULT X'', created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ); CREATE UNIQUE INDEX confidential_outputs_uniq_commitment ON confidential_outputs (commitment); CREATE INDEX confidential_outputs_idx_account_status ON confidential_outputs (account_id, status); +-- Supports outputs_lock_smallest_amount (vault_id + status + value asc) +CREATE INDEX confidential_outputs_idx_vault_status_value + ON confidential_outputs (vault_id, status, value ASC);
128-146: Consider cascading FKs to prevent orphansconfidential_outputs.account_id and .vault_id currently have no ON DELETE policy. Consider ON DELETE CASCADE if the parent account/vault is removed to keep referential integrity.
crates/wallet/storage_sqlite/src/writer.rs (5)
934-957: Fix copy-paste in error context and update timestamps on mutation
- The error labels still say outputs_finalize_by_proof_id.
- Consider setting updated_at on status changes.
- .map_err(|e| WalletStorageError::general("outputs_finalize_by_proof_id", e))?; + .map_err(|e| WalletStorageError::general("outputs_finalize_by_lock_id", e))?; @@ - .map_err(|e| WalletStorageError::general("outputs_finalize_by_proof_id", e))?; + .map_err(|e| WalletStorageError::general("outputs_finalize_by_lock_id", e))?;Optional (set updated_at in both updates):
- .set(( + .set(( confidential_outputs::status.eq(OutputStatus::Unspent.as_key_str()), confidential_outputs::lock_id.eq::<Option<i32>>(None), confidential_outputs::locked_at.eq::<Option<PrimitiveDateTime>>(None), + confidential_outputs::updated_at.eq(dsl::now), ))
965-982: Fix copy-paste in error context; consider updated_atSame as above: replace outputs_unlock_by_proof_id with outputs_release_by_lock_id; optionally set updated_at.
- .map_err(|e| WalletStorageError::general("outputs_unlock_by_proof_id", e))?; + .map_err(|e| WalletStorageError::general("outputs_release_by_lock_id", e))?; @@ - .map_err(|e| WalletStorageError::general("outputs_unlock_by_proof_id", e))?; + .map_err(|e| WalletStorageError::general("outputs_release_by_lock_id", e))?;
845-851: Also set updated_at when locking an outputKeeps timestamps consistent on state changes.
let changeset = ( confidential_outputs::status.eq(OutputStatus::LockedForSpend.as_key_str()), confidential_outputs::lock_id.eq(lock_id), confidential_outputs::locked_at.eq(diesel::dsl::now), + confidential_outputs::updated_at.eq(dsl::now), );
1122-1122: Comment typoSays “confidential_outputs” but mutates stealth_outputs. Fix the comment.
615-621: Minor message mismatchSecond expect message says “revealed balance is too large” when updating confidential_balance.
- vaults::confidential_balance.eq(confidential_balance - .to_u64_checked() - .expect("revealed balance is too large") as i64), + vaults::confidential_balance.eq(confidential_balance + .to_u64_checked() + .expect("confidential balance is too large") as i64),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bindings/src/types/UtxoUnspent.ts(1 hunks)bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeyBranch.ts(1 hunks)crates/wallet/sdk/src/apis/key_manager.rs(3 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(3 hunks)crates/wallet/storage_sqlite/src/writer.rs(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bindings/src/types/wallet-daemon-client/KeyBranch.ts
- bindings/src/types/tari-indexer-client/GetUnspentUtxosRequest.ts
- crates/wallet/sdk/src/apis/key_manager.rs
🧰 Additional context used
🧬 Code graph analysis (2)
bindings/src/types/UtxoUnspent.ts (2)
bindings/src/types/UtxoTag.ts (1)
UtxoTag(8-8)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/wallet/storage_sqlite/src/writer.rs (7)
crates/wallet/storage_sqlite/src/reader.rs (5)
stealth_outputs(807-814)stealth_outputs(837-847)stealth_outputs(860-862)stealth_outputs(896-899)utxo_process_queue(1207-1210)bindings/src/types/UtxoId.ts (1)
UtxoId(3-3)bindings/src/types/UtxoUnspent.ts (1)
UtxoUnspent(5-5)bindings/src/types/UtxoTag.ts (1)
UtxoTag(8-8)crates/engine_types/src/utxo.rs (4)
id(95-97)output(45-47)resource_address(91-93)tag(75-77)crates/wallet/sdk/src/storage.rs (3)
stealth_outputs_mark_as_spent(410-414)utxo_process_queue_extend(457-461)utxo_process_queue_remove_item(462-467)applications/tari_indexer/src/storage_sqlite/serialization.rs (1)
serialize_hex(44-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check stable
- GitHub Check: fmt
- GitHub Check: check nightly
🔇 Additional comments (5)
bindings/src/types/UtxoUnspent.ts (2)
2-3: LGTM: imports align with new wire types.Imports point to the correct generated types and reflect the new protocol shape.
5-5: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo
echo "=== 0) Show declared type file if present ==="
if [ -f bindings/src/types/UtxoUnspent.ts ]; then
echo "bindings/src/types/UtxoUnspent.ts:"
sed -n '1,240p' bindings/src/types/UtxoUnspent.ts || true
else
echo "bindings/src/types/UtxoUnspent.ts not found"
fiecho
echo "=== 1) Locate UtxoUnspent usages ==="
rg -nP -C2 '\bUtxoUnspent\b' || trueecho
echo "=== 2) Flag potential legacy field accesses (property access) ==="
rg -nP -C2 '.(address|version|shard|state_version|utxo)\b' || trueecho
echo "=== 3) Flag legacy destructures (const/let/var/params) ==="
rg -nP -C2 '(?:const|let|var|(|function)\s+[^\n{]{\s[^}]*\b(address|version|shard|state_version|utxo)\b' || trueecho
echo "=== 4) Flag object literals that include legacy keys (e.g., { address: ... }) ==="
rg -nP -C2 '\b(address|version|shard|state_version|utxo)\s*:' || true</blockquote></details> <details> <summary>crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)</summary><blockquote> `254-255`: **LGTM: composite shard index matches reader patterns** The added (account_id, resource_id, shard, state_version) index is appropriate for ordered version scans. </blockquote></details> <details> <summary>crates/wallet/storage_sqlite/src/writer.rs (2)</summary><blockquote> `1061-1061`: **LGTM: tag_byte cast is safe** UtxoTagByte fits in i32; insert looks correct. --- `1419-1471`: **Make utxo_process_queue.utxo_tag BigInt (i64) and update model + call sites** schema currently defines utxo_tag -> Integer, so Diesel will keep using i32. Change the column to BigInt and update all dependent types/casts: - crates/wallet/storage_sqlite/src/schema.rs — change utxo_process_queue.utxo_tag -> BigInt. - crates/wallet/storage_sqlite/src/models/utxo_process_queue.rs — change pub utxo_tag: i32 → i64. - crates/wallet/storage_sqlite/src/writer.rs — replace casts unspent.tag.value() as i32 and tag.value() as i32 → as i64 (≈ lines 1431, 1454). - crates/wallet/storage_sqlite/src/reader.rs — read row.utxo_tag as i64 and update conversion to UtxoTag (currently row.utxo_tag as u32; ≈ line 1221). - crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql — change utxo_tag INT → BIGINT and adjust any index DDL. - Update other consumers (applications/tari_indexer and any other modules referencing utxo_tag) to use i64. <details> <summary>⛔ Skipped due to learnings</summary>Learnt from: sdbondi
PR: #1551
File: applications/tari_indexer/src/storage_sqlite/models/utxo.rs:68-0
Timestamp: 2025-08-26T06:41:43.682Z
Learning: In the Tari codebase, u64/u32 values are stored as i64/i32 in SQLite to satisfy database constraints. Since both types have the same bit width and the data comes from trusted local databases, casting from i64/i32 back to u64/u32 is safe and preserves the original unsigned values.</details> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| fn utxo_process_queue_extend<I: IntoIterator<Item = (u64, UtxoUnspent)>>( | ||
| &mut self, | ||
| resource_address: &ResourceAddress, | ||
| items: I, | ||
| ) -> Result<(), WalletStorageError> { | ||
| const OPERATION: &str = "utxo_process_queue_extend"; | ||
| use crate::schema::utxo_process_queue; | ||
|
|
||
| for (account_key_index, unspent) in items { | ||
| diesel::insert_into(utxo_process_queue::table) | ||
| .values(( | ||
| utxo_process_queue::account_key_index.eq(account_key_index as i64), | ||
| utxo_process_queue::utxo_tag.eq(unspent.tag.value() as i32), | ||
| utxo_process_queue::public_nonce.eq(serialize_hex(unspent.public_nonce)), | ||
| utxo_process_queue::resource_address.eq(resource_address.to_string()), | ||
| )) | ||
| .on_conflict_do_nothing() | ||
| .execute(self.connection()) | ||
| .map_err(|e| WalletStorageError::general(OPERATION, e))?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Store 4‑byte UtxoTag without overflow; batch inserts to reduce round‑trips
- Casting tag to i32 will overflow for tags > 2,147,483,647. Use i64 and align with BIGINT column.
- Optional: insert in a single multi-values statement instead of a loop.
fn utxo_process_queue_extend<I: IntoIterator<Item = (u64, UtxoUnspent)>>(
@@
- for (account_key_index, unspent) in items {
- diesel::insert_into(utxo_process_queue::table)
- .values((
- utxo_process_queue::account_key_index.eq(account_key_index as i64),
- utxo_process_queue::utxo_tag.eq(unspent.tag.value() as i32),
- utxo_process_queue::public_nonce.eq(serialize_hex(unspent.public_nonce)),
- utxo_process_queue::resource_address.eq(resource_address.to_string()),
- ))
- .on_conflict_do_nothing()
- .execute(self.connection())
- .map_err(|e| WalletStorageError::general(OPERATION, e))?;
- }
+ let rows: Vec<_> = items
+ .into_iter()
+ .map(|(account_key_index, unspent)| {
+ (
+ utxo_process_queue::account_key_index.eq(account_key_index as i64),
+ utxo_process_queue::utxo_tag.eq(unspent.tag.value() as i64),
+ utxo_process_queue::public_nonce.eq(serialize_hex(unspent.public_nonce)),
+ utxo_process_queue::resource_address.eq(resource_address.to_string()),
+ )
+ })
+ .collect();
+ if !rows.is_empty() {
+ diesel::insert_into(utxo_process_queue::table)
+ .values(&rows)
+ .on_conflict_do_nothing()
+ .execute(self.connection())
+ .map_err(|e| WalletStorageError::general(OPERATION, e))?;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn utxo_process_queue_extend<I: IntoIterator<Item = (u64, UtxoUnspent)>>( | |
| &mut self, | |
| resource_address: &ResourceAddress, | |
| items: I, | |
| ) -> Result<(), WalletStorageError> { | |
| const OPERATION: &str = "utxo_process_queue_extend"; | |
| use crate::schema::utxo_process_queue; | |
| for (account_key_index, unspent) in items { | |
| diesel::insert_into(utxo_process_queue::table) | |
| .values(( | |
| utxo_process_queue::account_key_index.eq(account_key_index as i64), | |
| utxo_process_queue::utxo_tag.eq(unspent.tag.value() as i32), | |
| utxo_process_queue::public_nonce.eq(serialize_hex(unspent.public_nonce)), | |
| utxo_process_queue::resource_address.eq(resource_address.to_string()), | |
| )) | |
| .on_conflict_do_nothing() | |
| .execute(self.connection()) | |
| .map_err(|e| WalletStorageError::general(OPERATION, e))?; | |
| } | |
| Ok(()) | |
| } | |
| fn utxo_process_queue_extend<I: IntoIterator<Item = (u64, UtxoUnspent)>>( | |
| &mut self, | |
| resource_address: &ResourceAddress, | |
| items: I, | |
| ) -> Result<(), WalletStorageError> { | |
| const OPERATION: &str = "utxo_process_queue_extend"; | |
| use crate::schema::utxo_process_queue; | |
| let rows: Vec<_> = items | |
| .into_iter() | |
| .map(|(account_key_index, unspent)| { | |
| ( | |
| utxo_process_queue::account_key_index.eq(account_key_index as i64), | |
| utxo_process_queue::utxo_tag.eq(unspent.tag.value() as i64), | |
| utxo_process_queue::public_nonce.eq(serialize_hex(unspent.public_nonce)), | |
| utxo_process_queue::resource_address.eq(resource_address.to_string()), | |
| ) | |
| }) | |
| .collect(); | |
| if !rows.is_empty() { | |
| diesel::insert_into(utxo_process_queue::table) | |
| .values(&rows) | |
| .on_conflict_do_nothing() | |
| .execute(self.connection()) | |
| .map_err(|e| WalletStorageError::general(OPERATION, e))?; | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In crates/wallet/storage_sqlite/src/writer.rs around lines 1419 to 1441, the
code casts UtxoTag to i32 which will overflow for tags > 2,147,483,647 and
mismatches the BIGINT column; change the utxo_tag parameter to use i64 (cast tag
to i64) and update the diesel column expression to expect a 64-bit integer. Also
reduce DB round-trips by collecting the (account_key_index, utxo_tag,
public_nonce, resource_address) tuples into a Vec and perform a single
diesel::insert_into(...).values(&vec).on_conflict_do_nothing().execute(self.connection())
call, mapping any diesel error to WalletStorageError::general(OPERATION, e).
| fn utxo_process_queue_remove_item( | ||
| &mut self, | ||
| resource_address: ResourceAddress, | ||
| tag: UtxoTag, | ||
| public_nonce: RistrettoPublicKeyBytes, | ||
| ) -> Result<(), WalletStorageError> { | ||
| const OPERATION: &str = "utxo_process_queue_remove_item"; | ||
| use crate::schema::utxo_process_queue; | ||
|
|
||
| let num_affected = diesel::delete(utxo_process_queue::table) | ||
| .filter(utxo_process_queue::resource_address.eq(resource_address.to_string())) | ||
| .filter(utxo_process_queue::utxo_tag.eq(tag.value() as i32)) | ||
| .filter(utxo_process_queue::public_nonce.eq(serialize_hex(public_nonce))) | ||
| .execute(self.connection()) | ||
| .map_err(|e| WalletStorageError::general(OPERATION, e))?; | ||
|
|
||
| if num_affected == 0 { | ||
| return Err(WalletStorageError::NotFound { | ||
| operation: OPERATION, | ||
| entity: "utxo_process_queue item".to_string(), | ||
| key: format!( | ||
| "resource_address={}, tag={}, public_nonce={}", | ||
| resource_address, tag, public_nonce | ||
| ), | ||
| }); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deletion is account-agnostic; will remove entries for other accounts with identical tuples
The UNIQUE index now includes account_key_index. Removing by (resource_address, tag, public_nonce) will delete matching items for all accounts, dropping work for others. Include the account dimension. Also fix tag cast to i64.
-fn utxo_process_queue_remove_item(
- &mut self,
- resource_address: ResourceAddress,
- tag: UtxoTag,
- public_nonce: RistrettoPublicKeyBytes,
-) -> Result<(), WalletStorageError> {
+fn utxo_process_queue_remove_item(
+ &mut self,
+ account_key_index: u64,
+ resource_address: ResourceAddress,
+ tag: UtxoTag,
+ public_nonce: RistrettoPublicKeyBytes,
+) -> Result<(), WalletStorageError> {
@@
- let num_affected = diesel::delete(utxo_process_queue::table)
- .filter(utxo_process_queue::resource_address.eq(resource_address.to_string()))
- .filter(utxo_process_queue::utxo_tag.eq(tag.value() as i32))
- .filter(utxo_process_queue::public_nonce.eq(serialize_hex(public_nonce)))
+ let num_affected = diesel::delete(utxo_process_queue::table)
+ .filter(utxo_process_queue::account_key_index.eq(account_key_index as i64))
+ .filter(utxo_process_queue::resource_address.eq(resource_address.to_string()))
+ .filter(utxo_process_queue::utxo_tag.eq(tag.value() as i64))
+ .filter(utxo_process_queue::public_nonce.eq(serialize_hex(public_nonce)))
.execute(self.connection())
.map_err(|e| WalletStorageError::general(OPERATION, e))?;
@@
- return Err(WalletStorageError::NotFound {
+ return Err(WalletStorageError::NotFound {
operation: OPERATION,
entity: "utxo_process_queue item".to_string(),
- key: format!(
- "resource_address={}, tag={}, public_nonce={}",
- resource_address, tag, public_nonce
- ),
+ key: format!(
+ "account_key_index={}, resource_address={}, tag={}, public_nonce={}",
+ account_key_index, resource_address, tag, public_nonce
+ ),
});Also update the trait and call sites:
- fn utxo_process_queue_remove_item(
- &mut self,
- resource_address: ResourceAddress,
- tag: UtxoTag,
- public_nonce: RistrettoPublicKeyBytes,
- ) -> Result<(), WalletStorageError>;
+ fn utxo_process_queue_remove_item(
+ &mut self,
+ account_key_index: u64,
+ resource_address: ResourceAddress,
+ tag: UtxoTag,
+ public_nonce: RistrettoPublicKeyBytes,
+ ) -> Result<(), WalletStorageError>;📝 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.
| fn utxo_process_queue_remove_item( | |
| &mut self, | |
| resource_address: ResourceAddress, | |
| tag: UtxoTag, | |
| public_nonce: RistrettoPublicKeyBytes, | |
| ) -> Result<(), WalletStorageError> { | |
| const OPERATION: &str = "utxo_process_queue_remove_item"; | |
| use crate::schema::utxo_process_queue; | |
| let num_affected = diesel::delete(utxo_process_queue::table) | |
| .filter(utxo_process_queue::resource_address.eq(resource_address.to_string())) | |
| .filter(utxo_process_queue::utxo_tag.eq(tag.value() as i32)) | |
| .filter(utxo_process_queue::public_nonce.eq(serialize_hex(public_nonce))) | |
| .execute(self.connection()) | |
| .map_err(|e| WalletStorageError::general(OPERATION, e))?; | |
| if num_affected == 0 { | |
| return Err(WalletStorageError::NotFound { | |
| operation: OPERATION, | |
| entity: "utxo_process_queue item".to_string(), | |
| key: format!( | |
| "resource_address={}, tag={}, public_nonce={}", | |
| resource_address, tag, public_nonce | |
| ), | |
| }); | |
| } | |
| Ok(()) | |
| } | |
| fn utxo_process_queue_remove_item( | |
| &mut self, | |
| account_key_index: u64, | |
| resource_address: ResourceAddress, | |
| tag: UtxoTag, | |
| public_nonce: RistrettoPublicKeyBytes, | |
| ) -> Result<(), WalletStorageError> { | |
| const OPERATION: &str = "utxo_process_queue_remove_item"; | |
| use crate::schema::utxo_process_queue; | |
| let num_affected = diesel::delete(utxo_process_queue::table) | |
| .filter(utxo_process_queue::account_key_index.eq(account_key_index as i64)) | |
| .filter(utxo_process_queue::resource_address.eq(resource_address.to_string())) | |
| .filter(utxo_process_queue::utxo_tag.eq(tag.value() as i64)) | |
| .filter(utxo_process_queue::public_nonce.eq(serialize_hex(public_nonce))) | |
| .execute(self.connection()) | |
| .map_err(|e| WalletStorageError::general(OPERATION, e))?; | |
| if num_affected == 0 { | |
| return Err(WalletStorageError::NotFound { | |
| operation: OPERATION, | |
| entity: "utxo_process_queue item".to_string(), | |
| key: format!( | |
| "account_key_index={}, resource_address={}, tag={}, public_nonce={}", | |
| account_key_index, resource_address, tag, public_nonce | |
| ), | |
| }); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In crates/wallet/storage_sqlite/src/writer.rs around lines 1443-1471, the delete
is missing the account_key_index and casts tag to i32 which can delete rows
across accounts; update the delete filter chain to also filter by
account_key_index (use the account key parameter added to the trait) and change
the utxo_tag comparison to cast tag.value() to i64, propagate the new account
parameter in the method signature and update the trait and all call sites to
pass the correct account_key_index so deletions are scoped to the intended
account.
* development: fix(wallet)!: private derived tag and optimised* sync protocol (tari-project#1571)
* development: feat(template_lib): adds engine schnorr signature verification (tari-project#1574) feat(wallet)!: add bech32 address with view-only key (tari-project#1573) feat(walletui): wallet ux improvements (tari-project#1572) fix(wallet)!: private derived tag and optimised* sync protocol (tari-project#1571) feat(walletui): send flow ux improvements (tari-project#1570) doc: update openrpc.json get_connections method (tari-project#1567) chore(deps): bump actions/setup-node from 4 to 5 (tari-project#1565)
2 participants
Description
fix(wallet)!: private derived tag
fix(wallet)!: limit bytes sent on hot path of UTXO sync protocol (36 bytes per utxo)
feat(wallet): utxo download worker processes batches of matching UTXOs as they are found.
Motivation and Context
(tag, public_nonce)tuples for every UTXO per resource and filters these results to those that match the tag derivationD(account_secret, public_nonce, resource_addr) ?= utxo_tag(note: secret required to derive the tag)How Has This Been Tested?
Manually, existing integration tests
What process can a PR reviewer use to test or verify this change?
Send UTXOs to a new wallet from another wallet.
Breaking Changes
Summary by CodeRabbit
New Features
Refactor
Chores