fix: timeout fund locks after a while - #1621
Conversation
WalkthroughIntroduces a LocksApi and reworks wallet lock lifecycle (WalletLockDropGuard lifetimes, keep_locked/release semantics), replaces AlwaysMissLookupTable with GenerateValueLookup and lookup combinators, narrows fungible checks to is_public_fungible(), updates transfer/finalize flows to return/manage locks, and adds lock timeout support and migration. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Wallet Handler
participant LocksApi as LocksApi
participant Store as WalletStore
participant TxService as TransactionService
Handler->>LocksApi: create_lock_with_timeout(5min)
LocksApi->>Store: locks_create(Some(timeout))
Store-->>LocksApi: WalletLockId
LocksApi-->>Handler: WalletLockDropGuard
Handler->>Handler: lock inputs using lock.id()
Handler->>TxService: submit transaction (with lock.id)
Handler->>LocksApi: lock.keep_locked() -> returns lock_id
Note over Handler,LocksApi: lock retained until finalized
Handler->>LocksApi: finalize_lock(lock_id, diff)
LocksApi->>Store: locks_unlock_finalized(lock_id,diff)
LocksApi-->>Handler: ok
sequenceDiagram
participant Primary as IoReaderValueLookup
participant Fallback as GenerateValueLookup
participant Caller as BruteForceLookup
Caller->>Primary: lookup(value)
alt primary returns Some
Primary-->>Caller: Some(bytes)
else primary returns None
Primary-->>Caller: None
Caller->>Fallback: lookup(value)
Fallback-->>Caller: Some(generated_bytes)
end
Caller-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sdbondi
changed the title
a558926 to
9436d17
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
applications/tari_walletd/web_ui/src/utils/constants.ts (1)
23-28: Use thetoken_symbolparameter instead of hardcodedXTR_CURRENCY.SYMBOLin FormStep.tsx.The TODO comment correctly identifies that "tXTR" is hardcoded, but the codebase already handles network-specific currency symbols dynamically. The backend provides the actual token symbol via the
token_symbolfield in account balances, which is synced to the currency store. However, FormStep.tsx line 249 inconsistently uses the hardcodedXTR_CURRENCY.SYMBOLinstead of thetoken_symbolparameter—unlike line 235, which correctly uses it. Replace line 249 to use the parameter for consistency:- <InputAdornment position="end">µ{XTR_CURRENCY.SYMBOL}</InputAdornment> + <InputAdornment position="end">µ{token_symbol}</InputAdornment>crates/wallet/storage_sqlite/src/writer.rs (1)
1349-1373: Fix race-prone ID retrieval; uselast_insert_rowid()for this connection.Selecting the highest ID after insert can return another session’s row under concurrency. Use SQLite’s
last_insert_rowid()to get the row created by this connection/transaction.Apply:
- // TODO: See if we can upgrade libSQLite 0.35 - let lock_id = locks::table - .select(locks::id) - .order_by(locks::id.desc()) - .first::<i32>(self.connection()) - .map_err(|e| WalletStorageError::general(OPERATION, e))?; + // Safe and atomic for this connection + let lock_id: i32 = diesel::select(dsl::sql::<diesel::sql_types::Integer>("last_insert_rowid()")) + .get_result(self.connection()) + .map_err(|e| WalletStorageError::general(OPERATION, e))?;Also prefer a saturating cast for timeout seconds to avoid silent 68‑year locks:
- let timeout_seconds = i32::try_from(timeout.as_secs()).unwrap_or(i32::MAX); + let timeout_seconds = (timeout.as_secs().min(i32::MAX as u64)) as i32;Optional: avoid constructing SQL with
format!; compute the timestamp in Rust and bind it:// e.g., let timeout_at = OffsetDateTime::now_utc() + time::Duration::seconds(timeout_seconds.into()); // .values(locks::timeout_at.eq(Some(timeout_at.into())))
🧹 Nitpick comments (20)
crates/engine/tests/templates/tariswap/src/lib.rs (1)
228-238: Consider renaming and usingis_public_fungible()for clarity.The function name
check_resource_is_fungibleno longer accurately describes its behavior, as it now acceptsFungible,Confidential, andStealthresource types. This creates a maintainability concern where the function's purpose is unclear from its name.Based on the AI summary noting that other parts of the codebase use
is_public_fungible()for resource type checking, consider refactoring this function to align with that pattern:-fn check_resource_is_fungible(resource: ResourceAddress) { +fn check_resource_is_public_fungible(resource: ResourceAddress) { let resource_type = ResourceManager::get(resource).resource_type(); assert!( - matches!( - resource_type, - ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth - ), - "Resource {} is not fungible", + resource_type.is_public_fungible(), + "Resource {} is not public fungible", resource ); }Note: If
is_public_fungible()is not available onResourceType, the current implementation is acceptable, but the function should still be renamed tocheck_resource_is_public_fungibleto accurately reflect that it accepts multiple resource types beyond justFungible.crates/template_lib/src/models/vault.rs (1)
358-361: Good refactor! Improved code reusability.The new
to_resource_manager()method provides a clean abstraction for obtaining the ResourceManager, which can be reused across the Vault implementation.applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
44-44: Remove unused import.
XTR_CURRENCYis imported but never used in active code. This import should be removed unless the commented-out code at lines 140-145 is intended to be activated.-import { XTR_CURRENCY } from "@utils/constants";crates/wallet/sdk/src/storage.rs (1)
479-480: Document return semantics and add minimal usage guidance.
locks_release_stalereturningusizeis good; add brief rustdoc: “Returns number of released locks.” Reference that stale =timeout_at <= nowand typically called from a background task.crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (1)
37-44: Nice composable fallback API; add docs and a targeted test.The
with_fallbackcomposition is clean and the error mapping is appropriate. Please add:
- rustdoc explaining ordering and error propagation
- a unit test that injects a fallback returning a known value to assert the primary miss → fallback hit path.
crates/wallet/sdk_services/src/transaction_service/service.rs (1)
209-226: Consider throttling and add metrics for observability.Run cleanup less frequently (e.g., every N polls or on a timer) and emit a metric (counter/gauge) for number cleared to monitor drift.
crates/wallet/storage_sqlite/src/writer.rs (1)
1405-1421: Harden stale-release selection and add supporting index.
- Add a defensive guard to only consider locks not yet linked to a transaction:
- .filter(locks::timeout_at.is_not_null()) + .filter(locks::timeout_at.is_not_null()) .filter(locks::timeout_at.le(dsl::now)) + .filter(locks::transaction_id.is_null())
- Operationally, add an index to keep this query fast:
-- migration CREATE INDEX IF NOT EXISTS idx_locks_timeout_at ON locks(timeout_at);This prevents table scans as the locks table grows.
crates/wallet/sdk/tests/confidential_output_api.rs (1)
24-24: Consider creating locks with a timeout for coverage.To exercise timeout behavior, create the lock with an explicit TTL (if supported by the test harness) and add a test that stale locks are released automatically.
I can add a
locks_release_staletest to validate auto-unlock after TTL.crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
109-121: Naming/arg-order consistency for API ergonomics.Locking APIs mix param order: here
(account, resource, lock_id, amount)vs others passinglock_idfirst. Consider standardizing across SDK to reduce call-site mistakes.
122-160: Correct: smallest-first locking loop; handles exhaustion.Loop +
optional()?is fine. Minor: early-return whenamount.is_zero()could skip the write tx entirely; micro-optimization only.- self.store.with_write_tx(|tx| { + if amount.is_zero() { return Ok((Vec::new(), Amount::zero())); } + self.store.with_write_tx(|tx| {crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
118-120: Skip zero-amount revealed locks.Avoid DB writes when
revealed_to_spendis zero.- self.locks_api - .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?; + if revealed_to_spend.is_positive() { + self.locks_api + .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?; + }Apply similarly in other branches.
Also applies to: 144-166, 196-198
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
692-831: Atomicity of the “critical section”.Multiple DB writes occur; a single store TX would be safer. Since TTL mitigates stale locks, treat as recommended.
Plan: wrap lock creation, fee locks, change insertion, and input locks in one write tx API in Locks/Outputs modules.
crates/wallet/sdk/src/models/lock_guard.rs (3)
30-37: Clarifyrelease()semantics; consider renaming.
release(self) -> WalletLockIdtriggers release via Drop; name can mislead. Considerinto_released_id(self)or document behavior clearly.- pub fn release(self) -> WalletLockId { + /// Consumes the guard and releases the lock (via Drop), returning its id. + pub fn into_released_id(self) -> WalletLockId { self.lock_id // Drop will be called here, releasing the lock }
41-68: Drop-based unlock does blocking I/O; acceptable but document thread context.Dropping in async contexts may block. Add a note in docs to avoid dropping on hot paths, and prefer explicit release when on critical threads.
11-25: Lifetime + borrowed store: good for avoiding ownership; minor nit on unused import.
CommitableStoreimport appears unused; consider removing.crates/wallet/sdk/src/apis/locks.rs (2)
29-35: Clamp zero/too-small timeouts to avoid immediate stale release.A zero or near-zero Duration could be released right away by stale cleanup. Clamp to a sane minimum.
Apply within this method:
pub fn create_lock_with_timeout( &self, timeout: Duration, ) -> Result<WalletLockDropGuard<'a, TStore>, LocksApiError> { - let lock_id = self.store.with_write_tx(|tx| tx.locks_create(Some(timeout)))?; + // Prevent accidental immediate expiry + let timeout = timeout.max(Duration::from_secs(1)); + let lock_id = self.store.with_write_tx(|tx| tx.locks_create(Some(timeout)))?; Ok(WalletLockDropGuard::new(lock_id, self.store)) }
66-70: Consider adding more granular error variants.Today all errors surface as StoreError. Adding variants like InvalidTimeout, LockNotFound, AlreadyReleased would improve UX and JSON‑RPC mapping.
applications/tari_walletd/src/handlers/confidential.rs (2)
68-69: Avoid magic number for lock timeout; centralize/configure.Hard-coding 5 minutes obscures policy. Define a module constant or fetch from config.
Example:
+const DEFAULT_LOCK_TIMEOUT: Duration = Duration::from_secs(5 * 60); ... - let lock = sdk.locks_api().create_lock_with_timeout(Duration::from_secs(5 * 60))?; + let lock = sdk.locks_api().create_lock_with_timeout(DEFAULT_LOCK_TIMEOUT)?;
331-343: Use consistent log target for fallback warning.The once‑only fallback warning lacks target. Align with LOG_TARGET for easier filtering.
- warn!("Using value lookup fallback. This will likely result in very slow lookups."); + warn!( + target: LOG_TARGET, + "Using value lookup fallback. This will likely result in very slow lookups." + );applications/tari_walletd/src/handlers/accounts.rs (1)
1085-1087: Deduplicate the 5‑minute timeout.Reuse a constant or config to avoid divergence across handlers.
- let lock = sdk.locks_api().create_lock_with_timeout(Duration::from_secs(5 * 60))?; + let lock = sdk.locks_api().create_lock_with_timeout(DEFAULT_LOCK_TIMEOUT)?;And add near the top of this module:
+const DEFAULT_LOCK_TIMEOUT: Duration = Duration::from_secs(5 * 60);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
applications/tari_walletd/src/handlers/accounts.rs(8 hunks)applications/tari_walletd/src/handlers/confidential.rs(11 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/Settings/Components/DecryptUtxoBalance.tsx(2 hunks)applications/tari_walletd/web_ui/src/utils/constants.ts(1 hunks)applications/tari_walletd/web_ui/src/utils/helpers.tsx(3 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/engine/src/runtime/working_state.rs(1 hunks)crates/engine/tests/access_rules.rs(2 hunks)crates/engine/tests/confidential.rs(3 hunks)crates/engine/tests/stealth.rs(2 hunks)crates/engine/tests/templates/tariswap/src/lib.rs(1 hunks)crates/engine_types/src/crypto/elgamal.rs(3 hunks)crates/engine_types/src/crypto/value_lookup_table.rs(1 hunks)crates/template_lib/src/models/vault.rs(1 hunks)crates/template_lib_types/src/resource_type.rs(2 hunks)crates/template_test_tooling/src/support/mod.rs(1 hunks)crates/wallet/crypto/src/value_lookup/generate_lookup.rs(1 hunks)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs(2 hunks)crates/wallet/crypto/src/value_lookup/mod.rs(1 hunks)crates/wallet/crypto/tests/viewable_balance_proof.rs(2 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(0 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(12 hunks)crates/wallet/sdk/src/apis/locks.rs(1 hunks)crates/wallet/sdk/src/apis/mod.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(15 hunks)crates/wallet/sdk/src/apis/stealth_transfer/error.rs(2 hunks)crates/wallet/sdk/src/apis/stealth_transfer/types.rs(1 hunks)crates/wallet/sdk/src/models/lock_guard.rs(1 hunks)crates/wallet/sdk/src/models/wallet_transaction.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(4 hunks)crates/wallet/sdk/src/storage.rs(3 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(5 hunks)crates/wallet/sdk/tests/support/harness.rs(2 hunks)crates/wallet/sdk_services/src/transaction_service/error.rs(2 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/down.sql(1 hunks)crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/up.sql(1 hunks)crates/wallet/storage_sqlite/src/schema.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(3 hunks)
💤 Files with no reviewable changes (1)
- crates/wallet/sdk/src/apis/confidential_outputs.rs
🧰 Additional context used
🧬 Code graph analysis (22)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx (2)
crates/engine_types/src/resource.rs (1)
token_symbol(207-209)applications/tari_walletd/web_ui/src/utils/constants.ts (1)
XTR_CURRENCY(23-28)
clients/wallet_daemon_client/src/types.rs (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
crates/wallet/sdk/src/apis/mod.rs (2)
crates/wallet/storage_sqlite/src/writer.rs (3)
locks(91-95)locks(1366-1369)locks(1409-1413)crates/wallet/storage_sqlite/src/reader.rs (1)
locks(1078-1081)
crates/wallet/sdk/src/sdk.rs (3)
crates/wallet/storage_sqlite/src/writer.rs (4)
locks(91-95)locks(1366-1369)locks(1409-1413)new(82-86)crates/wallet/storage_sqlite/src/reader.rs (2)
locks(1078-1081)new(75-80)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(55-75)
crates/engine/tests/templates/tariswap/src/lib.rs (2)
crates/template_lib/src/models/vault.rs (1)
resource_type(364-366)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
applications/tari_walletd/web_ui/src/utils/helpers.tsx (1)
applications/tari_walletd/web_ui/src/utils/constants.ts (1)
XTR_CURRENCY(23-28)
crates/wallet/sdk/src/apis/confidential_transfer.rs (4)
crates/wallet/storage_sqlite/src/writer.rs (5)
locks(91-95)locks(1366-1369)locks(1409-1413)transaction(491-494)transaction(495-500)crates/wallet/storage_sqlite/src/reader.rs (1)
locks(1078-1081)crates/wallet/sdk/src/sdk.rs (1)
locks_api(198-200)crates/engine/src/runtime/locking.rs (1)
lock_id(195-197)
crates/wallet/crypto/src/value_lookup/generate_lookup.rs (3)
crates/engine_types/src/crypto/elgamal.rs (3)
lookup(283-287)from(160-162)from(166-171)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (1)
lookup(99-110)
crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (1)
crates/engine_types/src/crypto/value_lookup_table.rs (2)
new(27-29)new(54-56)
crates/wallet/sdk_services/src/transaction_service/service.rs (2)
crates/wallet/sdk/src/apis/locks.rs (1)
clear_stale_locks(60-63)crates/wallet/sdk/src/models/lock_guard.rs (1)
drop(44-67)
crates/wallet/sdk/src/storage.rs (1)
crates/wallet/storage_sqlite/src/writer.rs (2)
locks_create(1349-1373)locks_release_stale(1405-1421)
crates/wallet/sdk/src/models/wallet_transaction.rs (4)
bindings/src/types/FinalizeResult.ts (1)
FinalizeResult(9-16)bindings/src/types/SubstateDiff.ts (1)
SubstateDiff(6-10)bindings/src/types/WalletTransaction.ts (1)
WalletTransaction(8-21)crates/engine_types/src/commit_result.rs (2)
reject(200-202)reject(295-300)
crates/wallet/sdk/src/apis/locks.rs (4)
bindings/src/types/VaultId.ts (1)
VaultId(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)crates/wallet/sdk/src/models/lock_guard.rs (1)
new(19-24)crates/wallet/sdk_services/src/transaction_service/service.rs (2)
new(50-68)clear_stale_locks(209-225)
applications/tari_walletd/web_ui/src/routes/Settings/Components/DecryptUtxoBalance.tsx (1)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
StealthUtxosDecryptValueResponse(4-4)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (3)
crates/wallet/storage_sqlite/src/writer.rs (16)
locks(91-95)locks(1366-1369)locks(1409-1413)stealth_outputs(118-118)stealth_outputs(119-119)stealth_outputs(149-156)stealth_outputs(205-205)stealth_outputs(206-206)stealth_outputs(221-221)stealth_outputs(222-222)stealth_outputs(1207-1223)stealth_outputs(1294-1294)stealth_outputs(1295-1295)None(1397-1397)transaction(491-494)transaction(495-500)crates/wallet/sdk/src/sdk.rs (1)
locks_api(198-200)crates/engine/src/runtime/locking.rs (1)
lock_id(195-197)
crates/wallet/sdk/src/models/lock_guard.rs (3)
crates/wallet/sdk/src/apis/locks.rs (1)
new(20-22)crates/wallet/sdk/tests/support/harness.rs (2)
new(39-78)store(135-137)crates/wallet/sdk/src/sdk.rs (1)
store(178-180)
crates/wallet/sdk/tests/confidential_output_api.rs (3)
bindings/src/types/SubstateDiff.ts (1)
SubstateDiff(6-10)crates/wallet/sdk/src/sdk.rs (1)
locks_api(198-200)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(55-75)
crates/template_lib/src/models/vault.rs (1)
crates/template_lib/src/resource/manager.rs (2)
get(104-106)resource_type(119-127)
applications/tari_walletd/src/handlers/confidential.rs (5)
applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_params(162-173)crates/wallet/sdk/src/models/wallet_transaction.rs (1)
failure_reason_as_string(45-53)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-23)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (2)
lookup(99-110)load(26-35)
applications/tari_walletd/src/handlers/accounts.rs (3)
crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
transfer(212-427)crates/wallet/sdk/src/apis/stealth_transfer/api.rs (2)
transfer(307-601)params(504-508)crates/engine/src/runtime/locking.rs (1)
lock_id(195-197)
crates/engine_types/src/crypto/value_lookup_table.rs (3)
crates/engine_types/src/crypto/elgamal.rs (1)
lookup(283-287)crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-23)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (1)
lookup(99-110)
crates/wallet/storage_sqlite/src/writer.rs (2)
crates/wallet/sdk/src/storage.rs (3)
locks_create(470-470)general(143-148)locks_release_stale(479-479)crates/wallet/storage_sqlite/src/reader.rs (1)
locks(1078-1081)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (64)
applications/tari_walletd/web_ui/src/routes/Settings/Components/DecryptUtxoBalance.tsx (2)
30-30: LGTM! Import path refactoring is clean.The switch from relative imports to path aliases improves maintainability and readability.
41-41: LGTM! Type annotation correctly matches the state.The union type
StealthUtxosDecryptValueResponse | nullproperly reflects the initialnullvalue and aligns with the conditional rendering logic that already checks for null.crates/wallet/sdk/src/models/wallet_transaction.rs (4)
9-9: LGTM!The
SubstateDiffimport is correctly added to support the newfinalized_diff()method.
37-39: LGTM!This convenience method correctly delegates to the existing
status.is_accepted()method.
45-53: No issues found—theFinalizeResult::any_reject()method exists and is correctly used.The verification confirms that
FinalizeResult::any_reject()is defined incrates/engine_types/src/commit_result.rsat line 287 with the signaturepub fn any_reject(&self) -> Option<&RejectReason>. The method returns the expected type and integrates correctly with the chaining infailure_reason_as_string(), wheremap(|reject| reject.to_string())converts the&RejectReasonreference to aString.
41-43: No issues found—method exists and has correct return type.The
TransactionResult::any_accept()method exists incrates/engine_types/src/commit_result.rs(line 256) and correctly returnsOption<&SubstateDiff>. The implementation properly handles all transaction result variants (Accept, AcceptFeeRejectRest, Reject), so thefinalized_diffmethod in the reviewed code is correct.crates/wallet/sdk/Cargo.toml (1)
38-38: The multi-threaded runtime feature is actively used and necessary.The
"rt-multi-thread"feature is correctly added to support timeout-based lock release. The transaction service spawns background tasks viatokio::spawnthat callclear_stale_locks(), and usestokio::time::intervalwith a 5-second polling interval. Both operations require the multi-threaded runtime, making this feature addition essential for proper functionality.crates/engine/tests/access_rules.rs (2)
584-584: LGTM! API migration correctly applied.The update from
is_fungible()tois_public_fungible()correctly reflects the API change in the resource type module.
870-870: LGTM! Consistent API migration.The update correctly applies the renamed method, consistent with the change at line 584.
crates/engine/src/runtime/working_state.rs (1)
728-728: LGTM! API migration correctly applied with clearer semantics.The update from
is_fungible()tois_public_fungible()correctly reflects the API rename and makes the intent more explicit—this check ensures that fungible recalls are only allowed on public fungible or stealth resources.crates/template_lib/src/models/vault.rs (1)
364-366: LGTM! Cleaner implementation using the new helper method.The refactored
resource_type()method now leveragesto_resource_manager(), improving code maintainability.crates/template_lib_types/src/resource_type.rs (1)
39-41: LGTM! API rename improves clarity.The rename from
is_fungible()tois_public_fungible()makes the intent more explicit and helps distinguish between public fungible resources and other fungible types (confidential, stealth). All call sites in the codebase have been correctly updated.Note: This is a breaking change to the public API.
crates/engine_types/src/crypto/elgamal.rs (1)
215-218: ****The concerns raised in this review comment are not grounded in the actual implementations. The
GenerateValueLookupimplementation always returnsOk(Some([u8; 32])), and theTestLookupTablealso always returnsOk(Some(...)). Neither implementation ever returnsNone, which means the early-break code path at lines 215-218 is unreachable with the current implementations.While the
ValueLookupTableinterface theoretically allows returningNone, both current implementations guarantee aSomereturn. The concerns about "gaps in the lookup table" and "missing valid matches later in the range" do not apply in practice. The early-break code is harmless defensive logic that won't affect the function's actual behavior, and test coverage for unreachable code paths is unnecessary.Likely an incorrect or invalid review comment.
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx (2)
242-242: Verify fee value display is correct.The fee value is now displayed directly from
transferFormState.feewithout any formatting or conversion. This differs from the previous implementation. Ensure thattransferFormState.feealready contains the properly formatted value or that this raw display is intentional.
248-250: Fee unit display may be incorrect for non-XTR tokens.The fee endAdornment now hardcodes
µ{XTR_CURRENCY.SYMBOL}(displaying "µtXTR") regardless of which token is being sent. If fees are always paid in XTR, this is correct. However, if fees can be paid in other tokens, this creates a misleading UX.The previous implementation used
token_symbolwhich was more flexible. Verify that fees are always denominated in XTR and cannot be paid in other tokens.applications/tari_walletd/web_ui/src/utils/helpers.tsx (1)
255-307: LGTM! Consistent migration to XTR_CURRENCY.The
formatCurrencyfunction has been consistently updated to useXTR_CURRENCY.DIVISORandXTR_CURRENCY.DECIMALSacross all code paths (bigint, number, string, and fallback). The addition of error logging in the catch blocks (lines 287-288, 303-304) improves debuggability while maintaining safe fallback behavior.crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1)
12-12: LGTM!The addition of
LocksApiErrorfollows the established pattern for error propagation in this module and correctly enables automatic conversion via the#[from]attribute.Also applies to: 54-55
crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1)
12-20: LGTM!The removal of
lock_idfromStealthTransferOutputaligns with the PR's refactoring to manage locks viaLocksApiandWalletLockDropGuard. This improves encapsulation by moving lock lifecycle management out of the return type.crates/wallet/sdk_services/src/transaction_service/error.rs (1)
4-4: LGTM!The addition of
LockApiErrorcorrectly propagatesLocksApiErrorthrough the transaction service error chain, consistent with the broader lock API integration.Also applies to: 14-15
crates/engine_types/src/crypto/value_lookup_table.rs (3)
9-19: LGTM!The blanket implementation for closures provides ergonomic inline lookup table definitions.
21-46: LGTM!
AndThenLookupcorrectly implements a fallback pattern—querying the first lookup and only consulting the fallback when the first returnsNone.
48-70: LGTM!
MapErrLookupcleanly transforms error types, enabling composition of lookup tables with different error types.crates/wallet/sdk/src/apis/mod.rs (1)
10-10: LGTM!The addition of the
locksmodule expands the public API surface to support the new lock management abstraction introduced in this PR.crates/engine/tests/confidential.rs (2)
33-33: LGTM!The import update to
GenerateValueLookupaligns with the value lookup refactoring across the codebase.
532-533: LGTM!The test correctly uses the new
GenerateValueLookupimplementation for brute-forcing confidential balances, replacing the previousAlwaysMissLookupTable.Also applies to: 547-548
crates/wallet/storage_sqlite/src/schema.rs (1)
100-100: LGTM! Schema updated to support lock timeouts.The addition of the nullable
timeout_atcolumn to the locks table correctly supports the PR objective of implementing automatic timeout for fund locks.crates/template_test_tooling/src/support/mod.rs (1)
8-8: LGTM! Value lookup strategy updated.The replacement of
AlwaysMissLookupTablewithGenerateValueLookupaligns with the broader refactoring in this PR. The new implementation supports deterministic value generation for test scenarios.crates/wallet/crypto/tests/viewable_balance_proof.rs (2)
12-12: LGTM! Import updated consistently.The import change from
AlwaysMissLookupTabletoGenerateValueLookupaligns with the project-wide refactoring.
84-84: LGTM! Test updated to use new value lookup.The test now uses
GenerateValueLookupfor brute-forcing balance values, consistent with the refactoring across the codebase.crates/engine/tests/stealth.rs (2)
30-30: LGTM! Import updated for consistency.The import change to
GenerateValueLookupis consistent with the value-lookup refactoring across test modules.
490-490: LGTM! Test updated to use new lookup strategy.The stealth balance brute-forcing now uses
GenerateValueLookup, consistent with the refactoring pattern throughout the PR.crates/wallet/sdk/src/sdk.rs (4)
27-27: LGTM! New API imported.The
LocksApiimport exposes the new locking abstraction to the SDK surface.
198-200: LGTM! LocksApi exposed via SDK.The new
locks_api()method follows the established pattern for exposing wallet APIs and correctly returns aLocksApiinstance with appropriate lifetime.
258-258: LGTM! LocksApi wired into ConfidentialTransferApi.The
LocksApiis correctly passed as a dependency toConfidentialTransferApi, enabling centralized lock management for confidential transfers.
275-275: LGTM! LocksApi wired into StealthTransferApi.The
LocksApiis correctly passed as a dependency toStealthTransferApi, enabling centralized lock management for stealth transfers.crates/wallet/sdk/tests/support/harness.rs (2)
16-16: LGTM! Import updated for drop guard pattern.The addition of
WalletLockDropGuardimport supports the new RAII-based lock management pattern.
118-119: LGTM! Lock creation updated to use LocksApi.The
new_lock()method now uses the centralizedlocks_api()and returns aWalletLockDropGuard, implementing the RAII pattern for automatic lock release on drop.crates/wallet/sdk_services/src/transaction_service/service.rs (1)
200-203: Good: background cleanup wired into poll loop.Hooking stale-lock cleanup after pending checks makes sense and is low risk.
crates/wallet/storage_sqlite/src/writer.rs (1)
1395-1399: Good: clear timeout on transaction-link.Resetting
timeout_atwhen associating a transaction prevents false-positive stale releases.Please confirm there is no other path that sets
transaction_idwithout clearingtimeout_at.crates/wallet/crypto/src/value_lookup/mod.rs (1)
7-11: No migration issues found; removal is complete.The
AlwaysMissLookupTablehas been completely removed with no lingering references in the codebase. All call sites have been successfully migrated toGenerateValueLookup, and the module re-exports are properly structured.crates/wallet/sdk/src/storage.rs (1)
469-471: Review comment is outdated; breaking change is already fully migrated.The review assumes an incomplete migration requiring a compatibility shim, but the codebase shows the opposite: the
locks_createsignature change is already committed and fully consistent:
- Single
WalletStoreWriterimplementation (incrates/wallet/storage_sqlite/src/writer.rs:1349) already uses the new signature- All call sites already migrated:
locks.rs:25passesNone,locks.rs:33passesSome(timeout)- No legacy 0-argument callers exist
- No half-migration requiring gradual migration paths
The signature change has already been adopted everywhere; adding a backward-compatibility shim now would be regressive. If documenting this breaking change in
CHANGELOG.mdwas intended, that's a separate concern unrelated to the code structure itself.Likely an incorrect or invalid review comment.
crates/wallet/sdk/tests/confidential_output_api.rs (2)
42-42: LGTM: release via LocksApi.The explicit
release_lock(lock_id)matches the new lifecycle.
102-104: No action needed; current code is correct.
SubstateDiff::new()is a public method defined at crates/engine_types/src/substate.rs:855–862. The struct also derivesDefault, so both approaches work. Using the explicitnew()constructor is idiomatic and appropriate. The code at lines 102–104 is correct as-is.Likely an incorrect or invalid review comment.
crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
81-107: Good transactional rollback on insufficient funds.Returning
Err(InsufficientFunds)insidewith_write_txavoids persisting partial locks. Solid pattern.Confirm
with_write_txrolls back on error inWriteableWalletStore.
162-167: Add_output path looks fine.No issues; commit boundary is correct.
crates/wallet/sdk/src/apis/confidential_transfer.rs (4)
39-47: LocksApi integration: constructor and struct wiring look good.Field addition and plumbing are consistent with SDK exposure.
Also applies to: 55-75
577-579: Error surface extended with LocksApiError.Appropriate propagation for new lock operations.
419-426: Now I need to examine the actual implementation oflocks_set_transaction_idto determine its idempotency and uniqueness properties:Based on the comprehensive verification performed through code searches and analysis:
Search Context: The ripgrep search confirmed
locks_set_transaction_idis called in multiple locations and follows a consistent pattern: it always receives a unique lock identifier and a transaction ID.Idempotency Analysis: Database UPDATE operations like "UPDATE users SET status = 'active' WHERE id = 1" are idempotent because re-executing them doesn't change the result. The function follows this pattern—it sets a transaction_id for a unique lock identified by lock.id().
Uniqueness: The function receives
lock.id()which uniquely identifies a lock resource, ensuring each lock maps to one transaction_id.Ordering Confirmation: The code places
locks_set_transaction_idbeforekeep_locked(), correctly establishing the transaction ID in persistent storage before releasing the lock guard. This ordering is sound.
All concerns verified—no issues found.
The function exhibits idempotency through its UPDATE-SET semantics on a uniquely identified resource (lock_id), and the operation ordering is correct. Code is ready as-is.
281-287: Add timeout to lock creation at call sites, but establish timeout policy first.The
create_lock_with_timeout()method already exists in the locks API. However, the suggested refactoring lacks an established default timeout policy:
- Call sites needing update: confidential_transfer.rs:281 and stealth_transfer/api.rs:367 currently use
create_lock()without timeout- Missing: No
DEFAULT_LOCK_TTL_SECSconstant or timeout policy defined in the codebase- Infrastructure ready: The storage layer and schema already support timeouts;
clear_stale_locks()handles expirationBefore applying the suggested diff, define the default timeout value (whether as a constant, config parameter, or method parameter) and document the rationale for the chosen duration.
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (5)
64-97: LocksApi field and constructor wiring look consistent.No functional issues spotted.
Also applies to: 80-87
401-412: Statement generation looks correct; signer selection is sound.Inputs/outputs totals and signer derivation are consistent.
Also applies to: 510-525
423-431: Adding unconfirmed outputs under the lock is correct.Lock association via
lock.id()prevents race; good.Also applies to: 538-545
306-312: Validate spend_amount > 0 early.You already validate in
lock_inputs_for_transfer; good reuse.
583-591: The breaking API change has been implemented and integrated.The
stealth_transfer_api().transfer()now returns(WalletLockDropGuard, StealthTransferOutput)as shown in lines 593-600. The primary SDK caller inapplications/tari_walletd/src/handlers/accounts.rshas already been updated:
- Line 1008 unpacks the tuple:
let (lock, transfer) = sdk.stealth_transfer_api().transfer(...)- Line 1034 calls
lock.release()for cleanupThe
confidential_transfer_api().transfer()remains unchanged (returns onlyTransferOutput), so no update is required for that caller at line 930. Integration tests use the daemon client wrapper layer, which abstracts this concern.crates/wallet/sdk/src/models/lock_guard.rs (1)
26-33:keep_locked()correctly disarms Drop.Pattern is sound; returning the id is convenient for mapping.
crates/wallet/sdk/src/apis/locks.rs (1)
14-23: Good, minimal API surface with RAII semantics.Constructor and borrowing store lifetime look correct; methods delegate via write transactions cleanly.
applications/tari_walletd/src/handlers/confidential.rs (3)
77-86: Lock usage is correct (RAII on error, explicit keep on success).Using lock.id() for selection and keep_locked() only after proof creation is the right pattern.
201-205: Right place to keep the lock alive.Deferring keep_locked() until the proof is ready avoids leaking locks on mid‑flow errors.
354-366: Good fallback path when no lookup table configured.Warning + block_in_place + GenerateValueLookup are appropriate here.
applications/tari_walletd/src/handlers/accounts.rs (5)
1008-1023: Lock returned from transfer: good pattern.Returning (lock, transfer) prevents accidental leaks and makes ownership explicit across async boundaries.
1031-1035: Dry‑run releases the lock — correct.Prevents stale locks on dry runs; matches intent noted in comment.
1176-1183: keep_locked for statement flow is correct.Inputs remain locked until the caller submits or cancels; aligns with API expectations.
1045-1052: Lock mapping persistence is implemented—code is correct.The service does persist the lock→transaction mapping as intended. In
crates/wallet/sdk_services/src/transaction_service/service.rs:159-163, after inserting the transaction,locks_set_transaction_id(lock_id, transaction_id)is called when a lock_id is provided. This binding ensures the wallet's finalization/cancellation logic can reliably reference the transaction for a given lock, confirming the flow in your code snippet is sound.
4-4: Migration is backwards compatible — review concern is unfounded.The migration
2025-10-29-122554_add_timeout_at_to_locksadds a nullable column to the locks table, which is schema-compatible and requires no database wipe. The wallet storage already has automatic migration gating viarun_migrations()(called ininit_wallet_store()at startup), which fails fast on errors. The "Breaking changes: None" designation is accurate.
| } else if (name === "fee" && value) { | ||
| // let parsed = parseInt(transferFormState.fee); | ||
| // if (!isNaN(parsed)) { | ||
| // processedValue = parsed.toString(); // (parsed / XTR_CURRENCY.DIVISOR).toString(); | ||
| // } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove or complete the commented-out fee handling code.
Multiple blocks of commented-out code suggest incomplete refactoring. Either complete the fee parsing logic that references XTR_CURRENCY.DIVISOR or remove the dead code to reduce maintenance confusion.
Also applies to: 148-148, 153-153
| pub struct ProofsFinalizeRequest { | ||
| #[cfg_attr(feature = "ts", ts(type = "number"))] | ||
| pub proof_id: WalletLockId, | ||
| pub transaction_id: TransactionId, |
There was a problem hiding this comment.
🧩 Analysis chain
Document the breaking API change.
The addition of the transaction_id field to ProofsFinalizeRequest is a breaking change that will require updates from external wallet daemon clients. Additionally, the removal of the TypeScript type hint ts(type = "number") from proof_id may affect TypeScript bindings.
Ensure that:
- This breaking change is documented in release notes or migration guide
- The TypeScript bindings still generate correctly for
proof_id(it should still be WalletLockId, which should map to the appropriate TypeScript type)
🏁 Script executed:
#!/bin/bash
# Verify TypeScript bindings generation for WalletLockId
rg -A 3 -B 3 "type WalletLockId|interface WalletLockId" bindings/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find ProofsFinalizeRequest struct and check for ts() macro attributes
rg -n "struct ProofsFinalizeRequest" clients/wallet_daemon_client/src/types.rs -A 10Length of output: 478
🏁 Script executed:
#!/bin/bash
# Search for ts() macro usage around the modified lines
rg -n "ts\(" clients/wallet_daemon_client/src/types.rs -B 2 -A 2Length of output: 38944
🏁 Script executed:
#!/bin/bash
# Look for TypeScript binding files
fd -e "\.ts$" -e "\.tsx$" -e "\.d\.ts$" | grep -i wallet | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for CHANGELOG or migration documentation
fd "CHANGELOG|MIGRATION|BREAKING|breaking" -type f | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Find WalletLockId definition
rg -n "type WalletLockId|struct WalletLockId" clients/wallet_daemon_client/src/ -A 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check all WalletLockId field usages and their ts() annotations
rg -n "WalletLockId" clients/wallet_daemon_client/src/types.rs -B 1Length of output: 621
🏁 Script executed:
#!/bin/bash
# Search for any files mentioning ProofsFinalizeRequest for context
rg -r "ProofsFinalizeRequest" --type rust | head -15Length of output: 396
🏁 Script executed:
#!/bin/bash
# View full ProofsFinalizeRequest struct with context
rg -n "struct ProofsGenerateResponse" clients/wallet_daemon_client/src/types.rs -A 8Length of output: 380
🏁 Script executed:
#!/bin/bash
# View full ProofsFinalizeRequest struct with annotations
rg -n "struct ProofsFinalizeRequest" clients/wallet_daemon_client/src/types.rs -B 2 -A 5Length of output: 332
🏁 Script executed:
#!/bin/bash
# Check if WalletLockId is imported
rg -n "use.*WalletLockId|pub.*WalletLockId" clients/wallet_daemon_client/src/types.rs -B 2 -A 2Length of output: 1141
🏁 Script executed:
#!/bin/bash
# Look for WalletLockId in other files
fd "\.rs$" clients/wallet_daemon_client/src | xargs grep -l "WalletLockId" | head -5Length of output: 41
Add missing TypeScript type annotation and document breaking API change.
The transaction_id field addition is a breaking change for external wallet daemon clients. Additionally, proof_id is missing its TypeScript type hint—similar fields (ProofsGenerateResponse.proof_id at line 506 and ProofsCancelRequest.proof_id at line 525) include #[cfg_attr(feature = "ts", ts(type = "number"))], which should also be present on line 514 for consistency in TypeScript bindings.
Required fixes:
- Add
#[cfg_attr(feature = "ts", ts(type = "number"))]annotation toproof_idon line 514 - Document this breaking change in release notes or migration guide
🤖 Prompt for AI Agents
In clients/wallet_daemon_client/src/types.rs around lines 514–515, add the
missing TypeScript annotation to the proof_id field by applying
#[cfg_attr(feature = "ts", ts(type = "number"))] above the pub proof_id:
WalletLockId declaration, and update project release notes/migration guide to
document the breaking API change introduced by adding the new transaction_id
field (include which clients are affected, the new field name/type, and
suggested migration steps).
| pub enum ResourceType { | ||
| /// Fungible tokens do not have individual identity, making them interchangeable. | ||
| /// Examples include monetary units, liquidity pool tokens, or tokenized shares. | ||
| // TODO: rename to PublicFungible |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Address or remove the TODO comment.
TODO comments in production code should be avoided. Either:
- Create an issue to track the enum variant rename and reference it here, or
- Remove the comment if the rename is not planned
Would you like me to help create an issue to track this rename, or should the TODO be removed?
🤖 Prompt for AI Agents
In crates/template_lib_types/src/resource_type.rs at line 24, remove or replace
the lingering TODO comment "TODO: rename to PublicFungible": either remove the
comment entirely if no rename is planned, or replace it with a short reference
to a tracked issue (e.g., "See issue #123: rename to PublicFungible") — if
creating an issue, add the issue number or URL in the comment and ensure the
comment is phrased as a tracked task rather than a TODO.
| pub fn clear_stale_locks(&self) -> Result<usize, LocksApiError> { | ||
| let num = self.store.with_write_tx(|tx| tx.locks_release_stale())?; | ||
| Ok(num) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Schedule stale-lock cleanup; emit metrics.
clear_stale_locks() is useful but passive. Ensure a periodic task invokes it (e.g., every minute) and add a counter/gauge for observability.
🏁 Script executed:
#!/bin/bash
# Where is clear_stale_locks() called on a schedule?
rg -nP -C3 '\bclear_stale_locks\s*\(' -SLength of output: 2231
I need to verify the scheduling and metrics aspects. Let me gather more context about the transaction service:Now let me search for more information about the transaction service implementation to verify the scheduling context:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Examine the broader context of the transaction_service to confirm it's scheduled
cat crates/wallet/sdk_services/src/transaction_service/service.rs | sed -n '180,230p'Length of output: 1863
Now let me search for where on_poll() is scheduled and whether metrics are being used:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find where on_poll() is scheduled or called from
rg -nP -C5 'on_poll|interval|timer' crates/wallet/sdk_services/src/transaction_service/service.rs | head -100Length of output: 1764
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for metrics instrumentation in transaction_service and locks
rg -nP 'counter!|gauge!|metric' crates/wallet/sdk_services/src/transaction_service/service.rs crates/wallet/sdk/src/apis/locks.rsLength of output: 0
Stale-lock cleanup is already scheduled; add counter/gauge metrics for observability.
Scheduling is already implemented—clear_stale_locks() is invoked every 5 seconds by transaction_service.rs's on_poll() method (line 72: time::interval(Duration::from_secs(5))), with wrapper logging when locks are cleared. Add explicit counter and/or gauge metrics to track stale lock cleanup operations instead of relying on logging alone.
| let lock = self.locks_api.create_lock()?; | ||
|
|
||
| // Lock up funds for fees and transfer | ||
| let fee_inputs_to_spend = self.unlock_on_failure( | ||
| lock_id, | ||
| self.lock_fee_inputs(lock_id, &owner_account, params.max_fee, params.fee_input_selection), | ||
| )?; | ||
| let fee_inputs_to_spend = | ||
| self.lock_fee_inputs(lock.id(), &owner_account, params.max_fee, params.fee_input_selection)?; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Create lock with a timeout; centralize default TTL.
Align with timeout goal. Also, consider deriving TTL from config.
+use std::time::Duration;
+const DEFAULT_LOCK_TTL_SECS: u64 = 15 * 60; // or from ConfigApi
...
- let lock = self.locks_api.create_lock()?;
+ let lock = self
+ .locks_api
+ .create_lock_with_timeout(Duration::from_secs(DEFAULT_LOCK_TTL_SECS))?;🏁 Script executed:
#!/bin/bash
rg -n "create_lock_with_timeout|create_lock\(" crates/wallet -g '!**/target/**'Length of output: 518
I need to verify the signature of create_lock_with_timeout() and check for existing TTL constants.
🏁 Script executed:
#!/bin/bash
# Check the signature and implementation of create_lock_with_timeout
echo "=== create_lock_with_timeout implementation ==="
rg -A 10 "pub fn create_lock_with_timeout" crates/wallet/sdk/src/apis/locks.rs
echo -e "\n=== Check for TTL/timeout constants ==="
rg -n "TTL|TIMEOUT|timeout_secs|lock.*duration" crates/wallet/sdk/src -g '!**/target/**' | head -20
echo -e "\n=== Check create_lock implementations ==="
rg -A 5 "pub fn create_lock\(" crates/wallet/sdk/src/apis/locks.rsLength of output: 863
I need to check for configuration patterns and understand the API structure:
🏁 Script executed:
#!/bin/bash
# Search for ConfigApi references and timeout patterns
echo "=== ConfigApi references ==="
rg -n "ConfigApi|config.*timeout|timeout.*config" crates/wallet/sdk/src -g '!**/target/**' | head -15
echo -e "\n=== Search for any lock timeout usage patterns ==="
rg -n "locks_create|lock.*timeout" crates/wallet/sdk/src -g '!**/target/**' | head -20
echo -e "\n=== Check if stealth_transfer has access to config ==="
rg -B 20 "let lock = self.locks_api.create_lock" crates/wallet/sdk/src/apis/stealth_transfer/api.rs | grep -E "struct|impl|config|Config" | head -10Length of output: 2105
🏁 Script executed:
#!/bin/bash
# Check for config patterns and other call site contexts
echo "=== Check confidential_transfer call site context ==="
rg -B 5 -A 5 "let lock = self.locks_api.create_lock" crates/wallet/sdk/src/apis/confidential_transfer.rs | head -20
echo -e "\n=== Check test harness context ==="
rg -B 5 -A 5 "create_lock" crates/wallet/sdk/tests/support/harness.rs | head -20
echo -e "\n=== Look for any config or constants ==="
fd "config" crates/wallet/sdk/src -type f | head -10Length of output: 1076
🏁 Script executed:
#!/bin/bash
# Verify that stealth_transfer API structure and check what other methods/fields are available
echo "=== Check stealth_transfer struct and impl ==="
rg -B 3 "impl.*SteathTransferApi|pub struct SteathTransferApi" crates/wallet/sdk/src/apis/stealth_transfer/api.rs | head -20
echo -e "\n=== Check what fields/APIs are accessible in stealth_transfer ==="
rg "self\." crates/wallet/sdk/src/apis/stealth_transfer/api.rs | grep -E "self\.[a-z_]+_api" | sort -u | head -10Length of output: 876
🏁 Script executed:
#!/bin/bash
# Final check: verify if there are any comments or documentation about why timeouts weren't used
echo "=== Check comments around create_lock usage ==="
rg -B 3 -A 3 "create_lock\(\)" crates/wallet/sdk/src/apis/stealth_transfer/api.rs | head -15
echo -e "\n=== Verify Duration is imported/available ==="
rg "use std::time::Duration|use.*Duration" crates/wallet/sdk/src/apis/stealth_transfer/api.rs
echo -e "\n=== Check if any constants already exist for locks/fees ==="
rg -n "const.*[0-9]|const.*LOCK|const.*FEE" crates/wallet/sdk/src/apis/stealth_transfer/api.rs | head -10Length of output: 418
Use create_lock_with_timeout() with a defined TTL constant to add timeout protection.
The create_lock_with_timeout() method already exists in locks_api and accepts a Duration parameter. This refactoring requires defining a constant for the default lock TTL and updating the call at line 367 (and similar calls in confidential_transfer.rs at line 281) to use the timeout variant. Consider also defining this constant in a centralized location or deriving it from ConfigApi for better configurability.
+use std::time::Duration;
+const DEFAULT_LOCK_TTL_SECS: u64 = 15 * 60;
...
- let lock = self.locks_api.create_lock()?;
+ let lock = self
+ .locks_api
+ .create_lock_with_timeout(Duration::from_secs(DEFAULT_LOCK_TTL_SECS))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let lock = self.locks_api.create_lock()?; | |
| // Lock up funds for fees and transfer | |
| let fee_inputs_to_spend = self.unlock_on_failure( | |
| lock_id, | |
| self.lock_fee_inputs(lock_id, &owner_account, params.max_fee, params.fee_input_selection), | |
| )?; | |
| let fee_inputs_to_spend = | |
| self.lock_fee_inputs(lock.id(), &owner_account, params.max_fee, params.fee_input_selection)?; | |
| let lock = self | |
| .locks_api | |
| .create_lock_with_timeout(Duration::from_secs(DEFAULT_LOCK_TTL_SECS))?; | |
| // Lock up funds for fees and transfer | |
| let fee_inputs_to_spend = | |
| self.lock_fee_inputs(lock.id(), &owner_account, params.max_fee, params.fee_input_selection)?; |
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/stealth_transfer/api.rs around lines 367 to 372,
the code calls locks_api.create_lock() without a TTL; replace this with
locks_api.create_lock_with_timeout(DEFAULT_LOCK_TTL) using a defined Duration
constant (e.g., DEFAULT_LOCK_TTL) instead of the no-timeout variant; add the
DEFAULT_LOCK_TTL constant in a suitable shared location (or fetch it from
ConfigApi if available) and update the analogous call in
crates/wallet/sdk/src/apis/confidential_transfer.rs at line ~281 to use the same
constant to provide timeout protection for locks.
| @@ -0,0 +1 @@ | |||
| -- This file should undo anything in `up.sql` | |||
There was a problem hiding this comment.
Implement the down migration.
The down migration is a placeholder and does not undo the corresponding up migration (which adds the timeout_at column to the locks table). This prevents clean rollback of the migration.
Add the rollback logic:
--- This file should undo anything in `up.sql`
+ALTER TABLE locks DROP COLUMN timeout_at;📝 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.
| -- This file should undo anything in `up.sql` | |
| ALTER TABLE locks DROP COLUMN timeout_at; |
🤖 Prompt for AI Agents
In
crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/down.sql
around line 1, the down migration is currently empty and must undo the up
migration that added the timeout_at column to the locks table; update the file
to drop the timeout_at column (and any index or constraint created for it) so
rollbacks are clean—wrap the operations in a transaction, check/drop any index
by name if the up created one, and ensure the SQL is compatible with SQLite
(e.g., recreate the table without the timeout_at column if SQLite version does
not support DROP COLUMN).
| ALTER TABLE locks | ||
| ADD COLUMN timeout_at DATETIME NULL; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the down migration is complete.
The up migration looks correct. Ensure that the corresponding down migration (presumably in down.sql) properly removes the timeout_at column to allow rollback if needed.
🏁 Script executed:
#!/bin/bash
# Verify down migration exists and contains proper rollback logic
fd -t f "down.sql" crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/ --exec cat {}Length of output: 45
Add rollback logic to down migration.
The down.sql file contains only a comment and lacks the actual SQL to remove the timeout_at column. Add the following to properly enable rollback:
ALTER TABLE locks DROP COLUMN timeout_at;🤖 Prompt for AI Agents
In
crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/up.sql
lines 1-2: the migration adds timeout_at but the down migration file is only a
comment and lacks rollback SQL; open the corresponding down.sql for this
migration and add the SQL statement to remove the column (ALTER TABLE locks DROP
COLUMN timeout_at;) so the migration can be properly rolled back.
Test Results (CI)483 tests +27 469 ✅ +13 1h 36m 38s ⏱️ + 44m 27s For more details on these failures, see this check. Results for commit 9436d17. ± Comparison against base commit 4aff521. |
9436d17 to
a2c281b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
140-145: Remove or complete the commented-out fee handling code.This issue was previously flagged: multiple blocks of commented-out code suggest incomplete refactoring. Either complete the fee parsing logic or remove the dead code to reduce maintenance confusion.
Also applies to: 148-148, 153-153
crates/template_lib_types/src/resource_type.rs (1)
24-24: Duplicate: TODO comment already flagged in previous review.This TODO comment was already flagged in a previous review.
applications/tari_walletd/src/handlers/confidential.rs (1)
225-247: Rebindproof_idto the fetched transaction before finalizing.We still finalize whatever
proof_idthe caller supplies, without checking that it’s the lock associated withreq.transaction_id. An admin (or a compromised client) can therefore finalize an unrelated proof and release incorrect funds. Please fetch the lock that was registered for this transaction (e.g., viatransaction_api().locks_get_by_transaction_id(req.transaction_id)?), ensure it matchesreq.proof_id, and reject the request if it doesn’t before callinglocks_api().finalize_lock(...).crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
20-23: Wrap the lookup result inSome(...).
ValueLookupTable::lookupmust returnResult<Option<[u8; 32]>, _>, but this implementation currently returnsResult<[u8; 32], _>. As written, it won’t compile and—even if coerced—would drop theNonesignal the trait relies on. Please wrap the byte array inSome(...)before returning.- Ok(copy_fixed_checked(pk.as_bytes()).expect("Ristretto public key is always 32 bytes")) + Ok(Some( + copy_fixed_checked(pk.as_bytes()).expect("Ristretto public key is always 32 bytes"), + ))
🧹 Nitpick comments (4)
applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx (1)
44-44: Remove unused import or uncomment the code that uses it.The
XTR_CURRENCYimport is currently unused since all code referencing it (line 143) is commented out. Either remove this import or complete the fee handling refactor.applications/tari_walletd/src/handlers/accounts.rs (1)
1086-1086: Lock timeout implementation looks good; consider extracting the duration constant.The 5-minute timeout is reasonable for the time needed to create and sign transfer statements before they're submitted.
Optionally, extract the timeout duration to a named constant for better maintainability:
const TRANSFER_STATEMENT_LOCK_TIMEOUT: Duration = Duration::from_secs(5 * 60);Then use it:
- let lock = sdk.locks_api().create_lock_with_timeout(Duration::from_secs(5 * 60))?; + let lock = sdk.locks_api().create_lock_with_timeout(TRANSFER_STATEMENT_LOCK_TIMEOUT)?;crates/wallet/storage_sqlite/src/writer.rs (2)
1349-1373: Consider using parameterized queries for the timeout value.The current implementation uses string formatting to inject the timeout duration into the SQL query (Line 1356). While safe in this context since
timeout_secondscomes fromDuration.as_secs(), using SQLite's datetime function with parameters would be more idiomatic:diesel::sql_query("INSERT INTO locks (timeout_at) VALUES (datetime('now', ? || ' seconds'))") .bind::<diesel::sql_types::Text, _>(timeout_seconds.to_string())However, if Diesel's API doesn't easily support this pattern, the current approach is acceptable given the timeout value is not user-controlled.
1405-1421: LGTM: Stale lock cleanup implementation is correct.The method properly identifies locks with expired timeouts and releases them. The approach of fetching all stale lock IDs first, then releasing them individually ensures proper cleanup through the existing
locks_releasemethod, which handles confidential outputs, stealth outputs, and vault locks.For high-volume scenarios with many stale locks, consider batch operations as a future optimization, though the current implementation is acceptable given that stale locks should be rare in normal operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
applications/tari_walletd/src/handlers/accounts.rs(8 hunks)applications/tari_walletd/src/handlers/confidential.rs(11 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/components/SendMoney.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/Settings/Components/DecryptUtxoBalance.tsx(2 hunks)applications/tari_walletd/web_ui/src/utils/constants.ts(1 hunks)applications/tari_walletd/web_ui/src/utils/helpers.tsx(3 hunks)bindings/package.json(0 hunks)bindings/src/types/StealthTransferStatement.ts(2 hunks)bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/engine/src/runtime/working_state.rs(1 hunks)crates/engine/tests/access_rules.rs(2 hunks)crates/engine/tests/confidential.rs(3 hunks)crates/engine/tests/stealth.rs(2 hunks)crates/engine/tests/templates/tariswap/src/lib.rs(1 hunks)crates/engine_types/src/crypto/elgamal.rs(3 hunks)crates/engine_types/src/crypto/value_lookup_table.rs(1 hunks)crates/template_lib/src/models/stealth.rs(0 hunks)crates/template_lib/src/models/vault.rs(1 hunks)crates/template_lib_types/src/resource_type.rs(2 hunks)crates/template_test_tooling/src/support/mod.rs(1 hunks)crates/wallet/crypto/src/value_lookup/generate_lookup.rs(1 hunks)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs(2 hunks)crates/wallet/crypto/src/value_lookup/mod.rs(1 hunks)crates/wallet/crypto/tests/viewable_balance_proof.rs(2 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(0 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(12 hunks)crates/wallet/sdk/src/apis/locks.rs(1 hunks)crates/wallet/sdk/src/apis/mod.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(15 hunks)crates/wallet/sdk/src/apis/stealth_transfer/error.rs(2 hunks)crates/wallet/sdk/src/apis/stealth_transfer/types.rs(1 hunks)crates/wallet/sdk/src/models/lock_guard.rs(1 hunks)crates/wallet/sdk/src/models/wallet_transaction.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(4 hunks)crates/wallet/sdk/src/storage.rs(3 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(5 hunks)crates/wallet/sdk/tests/support/harness.rs(2 hunks)crates/wallet/sdk_services/src/transaction_service/error.rs(2 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/down.sql(1 hunks)crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/up.sql(1 hunks)crates/wallet/storage_sqlite/src/schema.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(3 hunks)
💤 Files with no reviewable changes (3)
- crates/template_lib/src/models/stealth.rs
- bindings/package.json
- crates/wallet/sdk/src/apis/confidential_outputs.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/down.sql
- crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/up.sql
- crates/wallet/sdk/src/apis/mod.rs
- applications/tari_walletd/web_ui/src/routes/AssetVault/Tokens/steps/FormStep.tsx
- crates/engine/src/runtime/working_state.rs
- crates/engine_types/src/crypto/elgamal.rs
- crates/wallet/sdk/src/storage.rs
- applications/tari_walletd/web_ui/src/utils/helpers.tsx
- crates/engine/tests/stealth.rs
- crates/engine/tests/access_rules.rs
- crates/wallet/sdk/src/sdk.rs
- crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs
- crates/wallet/crypto/src/value_lookup/mod.rs
- crates/template_test_tooling/src/support/mod.rs
- crates/template_lib/src/models/vault.rs
- applications/tari_walletd/src/handlers/stealth_utxos.rs
- crates/wallet/sdk/tests/confidential_output_api.rs
- crates/wallet/sdk/src/apis/locks.rs
- crates/wallet/sdk/src/apis/stealth_transfer/types.rs
- crates/wallet/storage_sqlite/src/schema.rs
🧰 Additional context used
🧬 Code graph analysis (15)
bindings/src/types/StealthTransferStatement.ts (1)
bindings/src/types/SchnorrSignatureBytes.ts (1)
SchnorrSignatureBytes(5-5)
clients/wallet_daemon_client/src/types.rs (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
crates/wallet/sdk/src/sdk.rs (1)
locks_api(198-200)
bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
crates/wallet/sdk/src/models/lock_guard.rs (3)
crates/wallet/sdk/src/apis/locks.rs (1)
new(20-22)crates/wallet/sdk/tests/support/harness.rs (2)
new(39-78)store(135-137)crates/wallet/sdk/src/sdk.rs (1)
store(178-180)
crates/engine/tests/templates/tariswap/src/lib.rs (2)
crates/template_lib/src/models/vault.rs (1)
resource_type(364-366)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/storage_sqlite/src/writer.rs (1)
crates/wallet/sdk/src/storage.rs (3)
locks_create(470-470)general(143-148)locks_release_stale(479-479)
crates/engine_types/src/crypto/value_lookup_table.rs (3)
crates/engine_types/src/crypto/elgamal.rs (1)
lookup(283-287)crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-23)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (1)
lookup(99-110)
crates/wallet/sdk/src/models/wallet_transaction.rs (4)
bindings/src/types/FinalizeResult.ts (1)
FinalizeResult(9-16)bindings/src/types/SubstateDiff.ts (1)
SubstateDiff(6-10)bindings/src/types/WalletTransaction.ts (1)
WalletTransaction(8-21)crates/engine_types/src/commit_result.rs (2)
reject(200-202)reject(295-300)
crates/wallet/crypto/src/value_lookup/generate_lookup.rs (3)
crates/engine_types/src/crypto/elgamal.rs (3)
lookup(283-287)from(160-162)from(166-171)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (1)
lookup(99-110)
crates/wallet/sdk_services/src/transaction_service/service.rs (1)
crates/wallet/sdk/src/apis/locks.rs (1)
clear_stale_locks(60-63)
applications/tari_walletd/src/handlers/accounts.rs (3)
crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
transfer(212-427)crates/wallet/sdk/src/apis/stealth_transfer/api.rs (2)
transfer(307-601)params(504-508)applications/tari_walletd/src/handlers/context.rs (1)
transaction_service(89-91)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (3)
crates/wallet/storage_sqlite/src/writer.rs (15)
locks(91-95)locks(1366-1369)locks(1409-1413)stealth_outputs(118-118)stealth_outputs(119-119)stealth_outputs(149-156)stealth_outputs(205-205)stealth_outputs(206-206)stealth_outputs(221-221)stealth_outputs(222-222)stealth_outputs(1207-1223)stealth_outputs(1294-1294)stealth_outputs(1295-1295)transaction(491-494)transaction(495-500)crates/wallet/sdk/src/sdk.rs (1)
locks_api(198-200)crates/engine/src/runtime/locking.rs (1)
lock_id(195-197)
applications/tari_walletd/web_ui/src/routes/Settings/Components/DecryptUtxoBalance.tsx (1)
bindings/src/types/wallet-daemon-client/StealthUtxosDecryptValueResponse.ts (1)
StealthUtxosDecryptValueResponse(4-4)
applications/tari_walletd/src/handlers/confidential.rs (5)
bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts (1)
ProofsFinalizeRequest(4-4)crates/wallet/sdk/src/models/lock_guard.rs (1)
id(26-28)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-23)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (2)
lookup(99-110)load(26-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check nightly
🔇 Additional comments (25)
applications/tari_walletd/web_ui/src/routes/Settings/Components/DecryptUtxoBalance.tsx (2)
30-30: LGTM: Import path modernization.The refactor from relative to alias-based import improves maintainability and reduces fragility when restructuring the codebase.
41-41: LGTM: Type annotation improves accuracy.Explicitly including
| nullin the type aligns the TypeScript type system with the actual usage pattern (initialized withnulland conditionally rendered). This improves type safety without changing functionality.applications/tari_walletd/web_ui/src/utils/constants.ts (1)
23-28: LGTM!The rename from
CURRENCYtoXTR_CURRENCYimproves clarity, and theas constassertion provides better type safety. The TODO note documents testnet-specific configuration appropriately.crates/template_lib_types/src/resource_type.rs (1)
39-41: Rename verification complete—all call sites properly updated.The method rename from
is_fungibletois_public_fungiblehas been successfully applied. Search results confirm no remaining references to the old method name exist, and the new method name is properly used in:
crates/engine/src/runtime/working_state.rs(line 728)crates/engine/tests/access_rules.rs(lines 584, 870)All call sites have been correctly updated.
bindings/src/types/StealthTransferStatement.ts (2)
2-2: LGTM: Import addition for type refinement.The import is necessary to support the type refinement of
balance_prooffrom an inline object to the structuredSchnorrSignatureBytestype.
13-13: The type refinement from an inline{ public_nonce: string; signature: string }to the named typeSchnorrSignatureBytesis safe and not breaking. SinceRistrettoPublicKeyBytesandScalar32Bytesare both type aliases forstring, the types are structurally equivalent. No TypeScript code directly accessesbalance_proofproperties, and all usages inInstruction.tsandAccountsCreateStealthTransferStatementResponse.tsremain compatible. This is a semantically beneficial refinement from the auto-generated bindings.crates/wallet/sdk/src/models/wallet_transaction.rs (2)
9-9: Import addition looks good.The
SubstateDiffimport is necessary for the newfinalized_diff()method and is correctly placed.
37-39: LGTM - clean convenience method.The delegation to
self.status.is_accepted()is straightforward and improves API ergonomics.applications/tari_walletd/src/handlers/accounts.rs (5)
4-4: LGTM: Duration import supports lock timeout feature.This import is necessary for the lock timeout functionality introduced on line 1086.
1031-1034: Dry run lock release looks correct; TODO noted for future optimization.The explicit
lock.release()correctly handles the dry run case where no transaction is submitted. The TODO comment suggests a future optimization to avoid locking outputs during dry runs altogether, which would be more efficient.
1116-1116: LGTM: Correct usage of lock identifier.The
lock.id()is properly passed to track which lock owns the reserved inputs.
1177-1177: LGTM: Lock ID properly captured and returned.The lock ID is correctly obtained from
keep_locked()and returned to the caller (line 1181), enabling the caller to manage the lock lifecycle or rely on the timeout mechanism.
1044-1051: Code is correct as written—ignoring the return value ofkeep_locked()is intentional.The verification shows that
TransactionSubmittedEventhas nolock_idfield (onlytransaction_idandnew_account), so capturing the return value would serve no purpose. In contrast,AccountsCreateStealthTransferStatementResponseat line 1177 requires alock_idfield, which is why that location captures the return value. The lock is properly persisted via theSome(lock.id())parameter passed tosubmit_transaction_with_opts(), andkeep_locked()ensures it remains locked for the wallet to monitor and release upon finalization.crates/wallet/sdk/Cargo.toml (1)
38-38: LGTM: Tokio runtime feature addition is appropriate.Adding the
rt-multi-threadfeature aligns with the async lock management and timeout operations introduced in this PR. This enables concurrent handling of lock operations across the wallet SDK.crates/wallet/sdk/tests/support/harness.rs (2)
16-16: LGTM: Guard pattern import aligns with new lock lifecycle.Adding
WalletLockDropGuardto the test harness imports supports the RAII-style lock management introduced by the LocksApi.
118-120: LGTM: Test harness updated to use LocksApi.The signature change to return
WalletLockDropGuardenables automatic lock release through the Drop trait, which is a safer pattern for tests. Usingsdk.locks_api().create_lock()aligns with the new centralized locking API.crates/wallet/storage_sqlite/src/writer.rs (1)
1395-1398: LGTM: Clearing timeout when linking transaction is correct.Setting
timeout_attoNonewhen associating a lock with a transaction ensures that the lock lifecycle is managed by the transaction rather than by timeout. This prevents premature lock release for in-flight transactions.crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
31-31: LGTM: API surface reduction aligns with LocksApi consolidation.The removal of
VaultIdand vault-based locking methods fromStealthOutputsApiis part of the migration to the centralizedLocksApi. This improves separation of concerns by moving lock lifecycle management to a dedicated API.crates/engine/tests/confidential.rs (2)
33-33: LGTM: Value lookup implementation update.Replacing
AlwaysMissLookupTablewithGenerateValueLookupaligns with the broader value-lookup refactoring across the codebase. The new implementation provides value generation capability for brute-force balance tests.
532-532: LGTM: Test usage updated consistently.Both call sites correctly pass
&mut GenerateValueLookuptotry_brute_force_confidential_balance, maintaining consistency with the updated value lookup approach.Also applies to: 547-547
crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1)
12-12: LGTM: Error propagation support for LocksApi.Adding
LocksApiErroras a variant with the#[from]attribute enables seamless error propagation from LocksApi calls within stealth transfer operations. This follows the established error handling pattern in the codebase.Also applies to: 54-55
crates/wallet/crypto/tests/viewable_balance_proof.rs (2)
12-12: LGTM: Import updated for new value lookup implementation.Consistent with the value-lookup refactoring seen in other test files, replacing
AlwaysMissLookupTablewithGenerateValueLookup.
84-84: LGTM: Test correctly uses new lookup implementation.The brute force balance test properly passes
&mut GenerateValueLookupto the balance proof verification.crates/wallet/sdk_services/src/transaction_service/service.rs (2)
200-202: LGTM: Stale lock cleanup integrated into polling loop.Calling
clear_stale_locksafter transaction checks ensures that expired locks are periodically released. Logging errors without disrupting the polling loop is the correct approach for this background maintenance task.
209-225: LGTM: Clean implementation of stale lock cleanup.The helper function provides clear logging for both success and no-op cases, making it easy to monitor lock cleanup behavior in production. The implementation correctly delegates to the
LocksApifor the actual cleanup logic.
| fn check_resource_is_fungible(resource: ResourceAddress) { | ||
| let resource_type = ResourceManager::get(resource).resource_type(); | ||
| assert!( | ||
| matches!(resource_type, ResourceType::Fungible | ResourceType::Confidential), | ||
| "Resource {} is not fungible nor confidential", | ||
| matches!( | ||
| resource_type, | ||
| ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth | ||
| ), | ||
| "Resource {} is not fungible", | ||
| resource | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update assertion message to reflect all accepted resource types.
The assertion message states "Resource {} is not fungible", but the check accepts three resource types: Fungible, Confidential, and Stealth. The previous message "is not fungible nor confidential" was more accurate, but still incomplete.
Apply this diff to make the error message accurate:
assert!(
matches!(
resource_type,
ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth
),
- "Resource {} is not fungible",
+ "Resource {} must be Fungible, Confidential, or Stealth",
resource
);📝 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 check_resource_is_fungible(resource: ResourceAddress) { | |
| let resource_type = ResourceManager::get(resource).resource_type(); | |
| assert!( | |
| matches!(resource_type, ResourceType::Fungible | ResourceType::Confidential), | |
| "Resource {} is not fungible nor confidential", | |
| matches!( | |
| resource_type, | |
| ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth | |
| ), | |
| "Resource {} is not fungible", | |
| resource | |
| ); | |
| } | |
| fn check_resource_is_fungible(resource: ResourceAddress) { | |
| let resource_type = ResourceManager::get(resource).resource_type(); | |
| assert!( | |
| matches!( | |
| resource_type, | |
| ResourceType::Fungible | ResourceType::Confidential | ResourceType::Stealth | |
| ), | |
| "Resource {} must be Fungible, Confidential, or Stealth", | |
| resource | |
| ); | |
| } |
🤖 Prompt for AI Agents
In crates/engine/tests/templates/tariswap/src/lib.rs around lines 228 to 238,
the assertion message "Resource {} is not fungible" is inaccurate because the
check accepts ResourceType::Fungible, ResourceType::Confidential, and
ResourceType::Stealth; update the assertion message to list all accepted types
(for example: "Resource {} is not fungible, confidential, or stealth") so the
error reflects the actual condition being validated.
| .expect("BUG: an unblinded input amount was negative"); | ||
|
|
||
| if let Some(ref src_vault) = maybe_src_vault { | ||
| self.outputs_api | ||
| .lock_revealed_funds(lock_id, &src_vault.id, revealed_to_spend)?; | ||
| self.locks_api | ||
| .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?; | ||
| } |
There was a problem hiding this comment.
Guard against zero-amount vault locks.
When available_revealed_funds is zero but the vault record still exists, revealed_to_spend becomes zero while maybe_src_vault is Some. We then call lock_funds_in_vault with a zero amount on Line 225, which the Locks API rejects, aborting transfers that rely purely on confidential inputs. Please skip the lock call unless the amount is positive (matching the PreferConfidential branch).
if let Some(ref src_vault) = maybe_src_vault {
- self.locks_api
- .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?;
+ if revealed_to_spend.is_positive() {
+ self.locks_api
+ .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?;
+ }
}📝 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.
| .expect("BUG: an unblinded input amount was negative"); | |
| if let Some(ref src_vault) = maybe_src_vault { | |
| self.outputs_api | |
| .lock_revealed_funds(lock_id, &src_vault.id, revealed_to_spend)?; | |
| self.locks_api | |
| .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?; | |
| } | |
| .expect("BUG: an unblinded input amount was negative"); | |
| if let Some(ref src_vault) = maybe_src_vault { | |
| if revealed_to_spend.is_positive() { | |
| self.locks_api | |
| .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?; | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/stealth_transfer/api.rs around lines 223 to 228,
the code calls self.locks_api.lock_funds_in_vault(lock_id, &src_vault.id,
revealed_to_spend) even when revealed_to_spend is zero, which the Locks API
rejects; change the logic to only call lock_funds_in_vault when
revealed_to_spend > 0 (i.e., guard the call with an if revealed_to_spend > 0
check or equivalent), so zero-amount vault records are skipped just like in the
PreferConfidential branch.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
applications/tari_walletd/src/handlers/confidential.rs (2)
68-68: Consider making the lock timeout configurable.The 5-minute timeout is hardcoded. Consider making this configurable via the handler context config or documenting why this specific duration was chosen.
Example approach:
let timeout = context.config().lock_timeout.unwrap_or(Duration::from_secs(5 * 60)); let lock = sdk.locks_api().create_lock_with_timeout(timeout)?;
209-260: LGTM! Security concern addressed—lock ownership is now validated.The finalization flow now properly validates that the provided
lock_idis associated with the specifiedtransaction_idbefore finalizing (lines 226-235). This prevents the security issue flagged in previous reviews where an Admin caller could finalize arbitrary locks.Minor suggestion: The error message at lines 231-234 could be more helpful by including both the expected and provided lock_id values:
return Err(invalid_params( "lock_id", Some(format!("Lock not associated with this transaction (expected {:?}, got {})", lock_id, req.lock_id)), ));crates/wallet/sdk/src/apis/locks.rs (1)
62-65: Consider adding observability metrics as previously suggested.The method correctly returns the count of cleared locks, and the caller in
transaction_service.rslogs this. As noted in previous reviews, consider adding explicit counter or gauge metrics (e.g.,counter!("wallet.locks.stale_cleared", num)) for better observability instead of relying solely on logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
applications/tari_walletd/src/handlers/confidential.rs(11 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(4 hunks)bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/wallet/crypto/src/value_lookup/generate_lookup.rs(1 hunks)crates/wallet/sdk/src/apis/locks.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/wallet_daemon_client/src/types.rs
- crates/wallet/crypto/src/value_lookup/generate_lookup.rs
🧰 Additional context used
🧬 Code graph analysis (4)
bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts (1)
bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)
applications/tari_walletd/src/handlers/stealth_utxos.rs (3)
crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-25)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (2)
lookup(99-110)load(26-35)
crates/wallet/sdk/src/apis/locks.rs (3)
crates/wallet/sdk/src/models/lock_guard.rs (1)
new(19-24)crates/wallet/sdk_services/src/transaction_service/service.rs (2)
new(50-68)clear_stale_locks(209-225)crates/wallet/sdk/src/storage.rs (1)
is_not_found_error(137-139)
applications/tari_walletd/src/handlers/confidential.rs (7)
bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts (1)
ProofsFinalizeRequest(4-4)crates/wallet/sdk/src/models/lock_guard.rs (1)
id(26-28)applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_params(162-173)crates/wallet/sdk/src/models/wallet_transaction.rs (1)
failure_reason_as_string(45-53)crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-25)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (2)
lookup(99-110)load(26-35)
⏰ 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: check stable
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: fmt
- GitHub Check: machete
- GitHub Check: clippy
🔇 Additional comments (6)
bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts (1)
2-4: LGTM! Breaking change aligns with lock-based finalization.The type change from
{ proof_id: number }to{ lock_id: number; transaction_id: TransactionId }correctly reflects the new finalization flow that validates lock ownership before finalizing. This breaking change is documented in the PR description (wallet DB deletion required).applications/tari_walletd/src/handlers/stealth_utxos.rs (2)
116-147: LGTM! Fallback composition addresses previous review feedback.The implementation correctly loads the lookup table immutably and composes it with a
GenerateValueLookupfallback. The one-time warning mechanism using a captured boolean is safe within the single-threadedspawn_blockingcontext and provides good user feedback when the fallback is actually used.
159-169: LGTM! Warning message improved as suggested in previous review.The warning message now accurately describes the fallback behavior and is more user-friendly than the previous "always misses" phrasing.
applications/tari_walletd/src/handlers/confidential.rs (2)
79-201: LGTM! Proper lock lifecycle management.The code correctly uses
lock.id()for accessing the lock identifier during operations andlock.keep_locked()to prevent auto-release when the guard goes out of scope. This ensures the lock remains active until explicitly finalized or cancelled.
345-376: LGTM! Value lookup fallback implementation consistent with stealth_utxos.rs.The fallback mechanism correctly composes file-based lookup with
GenerateValueLookupand provides user feedback via one-time warnings. The implementation matches the pattern instealth_utxos.rs.crates/wallet/sdk/src/apis/locks.rs (1)
26-37: LGTM! Clean RAII-based lock API design.The lock creation methods correctly return
WalletLockDropGuard, which provides automatic cleanup if the guard is dropped without callingkeep_locked(). The timeout variant enables controlled lock expiration to prevent indefinite holding.
2530b90 to
efdcb88
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
crates/wallet/sdk/src/apis/locks.rs (1)
62-65: Add counter/gauge metrics for observability.The stale-lock cleanup is already scheduled (called every 5 seconds by transaction_service.rs). Add explicit counter and/or gauge metrics to track stale lock cleanup operations instead of relying on logging alone.
Based on learnings
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (2)
367-368: Consider defining lock timeout as a constant.The 5-minute timeout is hardcoded. Consider defining a constant (e.g.,
DEFAULT_LOCK_TTL_SECS) for easier maintenance and potential configuration viaConfigApi.Example:
+const DEFAULT_LOCK_TTL_SECS: u64 = 5 * 60; ... - let lock = self.locks_api.create_lock_with_timeout(Duration::from_secs(5 * 60))?; + let lock = self.locks_api.create_lock_with_timeout(Duration::from_secs(DEFAULT_LOCK_TTL_SECS))?;
225-228: Guard against zero-amount vault locks.When
available_revealed_fundsis zero butmaybe_src_vaultisSome,revealed_to_spendbecomes zero. Line 226-227 then callslock_funds_in_vaultwith a zero amount, which the Locks API should reject. Skip the lock call unless the amount is positive, matching thePreferConfidentialbranch pattern.Apply:
if let Some(ref src_vault) = maybe_src_vault { + if revealed_to_spend.is_positive() { self.locks_api .lock_funds_in_vault(lock_id, &src_vault.id, revealed_to_spend)?; + } }
🧹 Nitpick comments (7)
crates/epoch_oracles/src/configured/real_time_ticker.rs (1)
165-182: Consider adding a termination condition to the infinite loop.While this test is marked
#[ignore], the infinite loop at lines 177-180 could cause issues if someone runs it intentionally. Consider adding a maximum iteration count or timeout to make the test safer for manual execution.Apply this diff to add a safety limit:
+ let mut count = 0; + let max_iterations = 100; loop { let res = poll_fn(|cx| ticker.poll_tick(cx)).await; eprintln!("res: {:?}", res); + count += 1; + if count >= max_iterations { + eprintln!("Reached max iterations"); + break; + } }crates/wallet/sdk/src/apis/locks.rs (4)
31-37: Consider validating timeout bounds.The method accepts any
Durationwithout validation. Consider adding bounds checks to prevent misconfiguration (e.g., timeout too short or excessively long).For example:
pub fn create_lock_with_timeout( &self, timeout: Duration, ) -> Result<WalletLockDropGuard<'a, TStore>, LocksApiError> { + const MIN_TIMEOUT: Duration = Duration::from_secs(60); + const MAX_TIMEOUT: Duration = Duration::from_secs(24 * 60 * 60); + if timeout < MIN_TIMEOUT || timeout > MAX_TIMEOUT { + return Err(LocksApiError::InvalidTimeout { provided: timeout }); + } let lock_id = self.store.with_write_tx(|tx| tx.locks_create(Some(timeout)))?; Ok(WalletLockDropGuard::new(lock_id, self.store)) }
44-48: Clarify method semantics with documentation.The name
finalize_lockmight be misleading—it sounds like it makes the lock permanent, but it actually unlocks outputs after applying the transaction's substate diff. Consider adding a doc comment to clarify that this method unlocks outputs that were successfully spent in a finalized transaction.Example:
+ /// Unlocks outputs that were successfully spent in a finalized transaction by applying + /// the transaction's substate diff. This allows the wallet to update output statuses + /// and free the lock. pub fn finalize_lock(&self, lock_id: WalletLockId, diff: &SubstateDiff) -> Result<(), LocksApiError> {
50-60: Validate that amount is positive before locking.Based on past review comments (stealth_transfer/api.rs lines 223-228), the Locks API rejects zero-amount vault locks. Add a check here to fail fast with a clear error message.
Apply:
pub fn lock_funds_in_vault<A: Into<Amount>>( &self, lock_id: WalletLockId, vault_id: &VaultId, amount_to_lock: A, ) -> Result<(), LocksApiError> { + let amount = amount_to_lock.into(); + if !amount.is_positive() { + return Err(LocksApiError::InvalidAmount { + details: "Cannot lock zero or negative amount in vault".to_string(), + }); + } self.store - .with_write_tx(|tx| tx.vaults_lock_revealed_funds(lock_id, vault_id, amount_to_lock.into()))?; + .with_write_tx(|tx| tx.vaults_lock_revealed_funds(lock_id, vault_id, amount))?; Ok(()) }
75-85: Consider adding domain-specific error variants.The error type only wraps storage errors. Consider adding variants like
InvalidAmount,InvalidTimeout, orLockExpiredto provide clearer error messages to API consumers when validation fails or operations are performed on stale locks.Example:
#[derive(thiserror::Error, Debug)] pub enum LocksApiError { #[error("Store error: {0}")] StoreError(#[from] WalletStorageError), + #[error("Invalid amount: {details}")] + InvalidAmount { details: String }, + #[error("Invalid timeout duration: {details}")] + InvalidTimeout { details: String }, }applications/tari_walletd/src/handlers/confidential.rs (2)
68-68: Consider defining lock timeout as a constant.The 5-minute timeout is hardcoded here and in
stealth_transfer/api.rs. Consider defining a shared constant for consistency and easier maintenance.Example:
+const DEFAULT_LOCK_TTL_SECS: u64 = 5 * 60; ... - let lock = sdk.locks_api().create_lock_with_timeout(Duration::from_secs(5 * 60))?; + let lock = sdk.locks_api().create_lock_with_timeout(Duration::from_secs(DEFAULT_LOCK_TTL_SECS))?;Also applies to: 201-201
226-235: Improve error message for missing lock.When
lock_idisNone(no lock found for the transaction), the error message "Lock not associated with this transaction" is misleading. Consider distinguishing between "no lock found" and "wrong lock provided".Example:
let lock_id = sdk .locks_api() .get_lock_by_transaction_id(req.transaction_id) .optional()?; - if lock_id != Some(req.lock_id) { + match lock_id { + None => { + return Err(invalid_params( + "transaction_id", + Some("No lock found for this transaction"), + )); + }, + Some(id) if id != req.lock_id => { return Err(invalid_params( "lock_id", Some("Lock not associated with this transaction"), )); + }, + _ => {}, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(1 hunks)applications/tari_walletd/src/handlers/confidential.rs(11 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(4 hunks)bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/epoch_oracles/Cargo.toml(1 hunks)crates/epoch_oracles/src/configured/real_time_ticker.rs(3 hunks)crates/wallet/crypto/src/value_lookup/generate_lookup.rs(1 hunks)crates/wallet/sdk/src/apis/locks.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(16 hunks)crates/wallet/sdk/src/models/wallet_transaction.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/wallet/sdk/src/models/wallet_transaction.rs
- bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts
- clients/wallet_daemon_client/src/types.rs
- crates/wallet/crypto/src/value_lookup/generate_lookup.rs
🧰 Additional context used
🧬 Code graph analysis (4)
crates/wallet/sdk/src/apis/locks.rs (4)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
new(80-97)crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
new(32-42)is_not_found_error(296-298)crates/wallet/sdk/src/models/lock_guard.rs (1)
new(19-24)crates/wallet/sdk/src/storage.rs (1)
is_not_found_error(137-139)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (3)
crates/wallet/storage_sqlite/src/writer.rs (15)
locks(91-95)locks(1366-1369)locks(1409-1413)stealth_outputs(118-118)stealth_outputs(119-119)stealth_outputs(149-156)stealth_outputs(205-205)stealth_outputs(206-206)stealth_outputs(221-221)stealth_outputs(222-222)stealth_outputs(1207-1223)stealth_outputs(1294-1294)stealth_outputs(1295-1295)transaction(491-494)transaction(495-500)crates/wallet/sdk/src/sdk.rs (1)
locks_api(198-200)crates/engine/src/runtime/locking.rs (1)
lock_id(195-197)
applications/tari_walletd/src/handlers/confidential.rs (8)
crates/engine_types/src/crypto/helpers.rs (1)
get_commitment_factory(50-52)bindings/src/types/wallet-daemon-client/ProofsFinalizeRequest.ts (1)
ProofsFinalizeRequest(4-4)crates/wallet/sdk/src/models/lock_guard.rs (1)
id(26-28)applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_params(162-173)crates/wallet/sdk/src/models/wallet_transaction.rs (1)
failure_reason_as_string(45-53)crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-25)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (2)
lookup(99-110)load(26-35)
applications/tari_walletd/src/handlers/stealth_utxos.rs (3)
crates/wallet/crypto/src/value_lookup/generate_lookup.rs (1)
lookup(20-25)crates/engine_types/src/crypto/value_lookup_table.rs (4)
lookup(6-6)lookup(16-18)lookup(39-45)lookup(67-69)crates/wallet/crypto/src/value_lookup/io_reader_value_lookup.rs (2)
lookup(99-110)load(26-35)
⏰ 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: check nightly
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: machete
- GitHub Check: file licenses
- GitHub Check: check stable
🔇 Additional comments (18)
crates/epoch_oracles/Cargo.toml (1)
34-34: Approve:serde_jsondev-dependency is correctly used.The change is valid—serde_json is a Rust library for parsing and generating JSON, built upon Serde, and it is actively used in
crates/epoch_oracles/src/configured/real_time_ticker.rsat line 173 within test code (serde_json::from_reader::<_, Config>(&mut file).unwrap();). The code must compile even though the test is marked with#[ignore], making this dev-dependency addition necessary and appropriate. The workspace.dependencies table defines dependencies to be inherited by members of a workspace, and using{ workspace = true }is the correct pattern for this codebase.crates/epoch_oracles/src/configured/real_time_ticker.rs (2)
78-78: LGTM: Off-by-one fix for epoch emission.The change from
>to>=correctly handles the case where the calculated epoch equals the current epoch, ensuring that epoch events are emitted when they should be.Also applies to: 94-94
85-85: LGTM: Improved done_for_now semantics.The dynamic calculation
calculated_epoch == epochaccurately reflects whether the ticker has caught up to real-time, replacing the previous hardcoded boolean values. This provides more precise signaling to consumers about the ticker state.Also applies to: 100-101
crates/wallet/sdk/src/apis/locks.rs (3)
16-24: LGTM! Clean API structure.The
LocksApistruct follows a clean delegation pattern with appropriate visibility for the constructor.
39-42: LGTM!Simple and correct delegation to the storage layer.
67-72: LGTM!Correct use of read transaction for querying lock by transaction ID.
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (6)
4-4: LGTM! Clean LocksApi integration.The imports, struct field, and constructor changes correctly integrate
LocksApiinto theStealthTransferApi. Note that this is a breaking change to the public constructor signature.Also applies to: 45-45, 49-57, 67-67, 83-83, 91-91
146-178: LGTM!The
RevealedOnlybranch correctly validates sufficient funds and locks a positive amount in the vault.
246-286: LGTM! Correct zero-amount guard.The
PreferConfidentialbranch correctly guards thelock_funds_in_vaultcall withrevealed_to_spend.is_positive()at Line 266. This is the pattern that should be applied consistently across all branches (see comment onPreferRevealedbranch).
371-372: LGTM! Consistent lock handle usage.The code correctly uses
lock.id()throughout to extract theWalletLockIdfor locking operations and output tracking.Also applies to: 424-431, 436-442, 539-546
311-311: LGTM! Improved lock lifecycle control.Returning
WalletLockDropGuardalongside the transfer output is a breaking API change that gives callers explicit control over lock lifecycle. This design allows callers to either drop the guard (auto-releasing the lock) or callkeep_locked()to persist it.Also applies to: 594-600
392-393: LGTM! Simplified error handling.Direct calls to
next_derived_key_indexreplace the removedunlock_on_failurehelper. TheWalletLockDropGuardnow handles automatic cleanup on error, simplifying the code.Also applies to: 449-450
applications/tari_walletd/src/handlers/stealth_utxos.rs (3)
10-11: LGTM!Appropriate imports for the new value lookup mechanism.
116-147: LGTM! Correct fallback composition.The file-based lookup is correctly composed with a
GenerateValueLookupfallback, and the one-time warning provides good user feedback when the fallback is triggered.
159-170: LGTM! Correct fallback and updated warning.The
GenerateValueLookupfallback replacesAlwaysMissLookupTable, and the warning message accurately describes the behavior.applications/tari_walletd/src/handlers/confidential.rs (3)
4-4: LGTM!Imports are appropriate for the lock timeout, value lookup, and finalization changes.
Also applies to: 13-18, 30-31
262-271: LGTM!Clean refactor to use
locks_api().release_lock()for cancellation.
345-377: LGTM! Consistent value lookup implementation.The value lookup with fallback follows the same correct pattern as
stealth_utxos.rs, with appropriate warnings for the fallback path.
efdcb88 to
99d2f46
Compare
2 participants
Description
fix: timeout fund locks after a while
fix: improve locking API
Motivation and Context
Automatically release funds locks after a timeout. This prevents unfinalized locks from being held indefinitely, allowing funds to be spent.
Wallet DB needs to be deleted
How Has This Been Tested?
Manaully
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Improvements