fix(wallet)!: support multiple vault locks, handle partial commit case - #1611
Conversation
WalkthroughComprehensive wallet SDK storage trait restructuring alongside stealth transfer signing flow refactoring, introducing read/write storage trait split with CommitableStore, enhanced lock lifecycle management via vault_locks table, branch tracking in key records, and multiple signing/commitment patterns across transaction handling and context management. Changes
Sequence DiagramsequenceDiagram
participant User
participant StealthTransfer as StealthTransferApi
participant Signer as Signer
participant Lock as LockManager
participant TxSubmit as TransactionSubmit
participant Storage as Storage
rect rgb(240, 248, 255)
note over StealthTransfer: New Signing Flow
User->>StealthTransfer: transfer(amount, recipient)
StealthTransfer->>StealthTransfer: compute fee_signer & main_signer
alt has additional_signer
StealthTransfer->>Signer: sign with additional_signer
end
StealthTransfer->>Signer: sign final with main_signer
end
rect rgb(255, 245, 238)
note over Lock: Lock Lifecycle
StealthTransfer->>Lock: lock_fee_inputs()
StealthTransfer->>TxSubmit: submit_transaction_with_opts(lock_id)
TxSubmit->>Storage: locks_set_transaction_id(lock_id)
end
rect rgb(240, 255, 240)
note over Storage: Finalization with Diff
alt execution succeeds & has diff
TxSubmit->>Storage: locks_unlock_finalized(lock_id, diff)
else execution fails or no diff
TxSubmit->>Storage: locks_release(lock_id)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: 60+ files modified across multiple interconnected subsystems. While ~40% of changes are homogeneous import consolidations (reducing complexity), the remaining changes involve: (1) foundational trait redesign splitting monolithic Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine_types/src/resource_container.rs (1)
619-623: Bug:Stealth::lock_allreturns a Fungible container.This breaks type invariants and will cause
unlockto fail withResourceTypeMismatch. It should returnSelf::stealth(...), notSelf::fungible(...).Apply this fix:
// Sets to zero and returns the amount let newly_locked_amount = mem::take(revealed_amount); *locked_amount += newly_locked_amount; - Ok(Self::fungible(resource_address, newly_locked_amount)) + Ok(Self::stealth(resource_address, newly_locked_amount))crates/wallet/storage_sqlite/src/writer.rs (1)
901-939: Bug: finalizing revealed funds handles only one vault row and subtracts onceSelecting first (vault_id, amount) loses other rows for the same lock_id, then deletes all rows. Result: balances for other vaults aren’t decremented.
Apply this fix to finalize all rows atomically:
- // Fetch the vault locked by this lock_id - let (vault_id, amount) = vault_locks::table - .select((vault_locks::vault_id, vault_locks::amount)) - .filter(vault_locks::lock_id.eq(lock_id)) - .first::<(i32, i64)>(self.connection()) - .optional() - .map_err(|e| WalletStorageError::general(OPERATION, e))? - .ok_or_else(|| WalletStorageError::NotFound { - operation: OPERATION, - entity: "vault lock".to_string(), - key: lock_id.to_string(), - })?; - - // Delete the lock record - diesel::delete(vault_locks::table) - .filter(vault_locks::lock_id.eq(lock_id)) - .execute(self.connection()) - .map_err(|e| WalletStorageError::general(OPERATION, e))?; - - let num_rows = diesel::update(vaults::table) - .set(vaults::revealed_balance.eq(vaults::revealed_balance.sub(amount))) - .filter(vaults::id.eq(vault_id)) - .execute(self.connection()) - .map_err(|e| WalletStorageError::general(OPERATION, e))?; + // Fetch all vault rows locked by this lock_id + let rows = vault_locks::table + .select((vault_locks::vault_id, vault_locks::amount)) + .filter(vault_locks::lock_id.eq(lock_id)) + .load::<(i32, i64)>(self.connection()) + .map_err(|e| WalletStorageError::general(OPERATION, e))?; + if rows.is_empty() { + return Err(WalletStorageError::NotFound { + operation: OPERATION, + entity: "vault lock".to_string(), + key: lock_id.to_string(), + }); + } + + // Apply decrements per vault + for (vault_id, amount) in rows { + let affected = diesel::update(vaults::table) + .set(vaults::revealed_balance.eq(vaults::revealed_balance.sub(amount))) + .filter(vaults::id.eq(vault_id)) + .execute(self.connection()) + .map_err(|e| WalletStorageError::general(OPERATION, e))?; + if affected == 0 { + return Err(WalletStorageError::NotFound { + operation: OPERATION, + entity: "lock on vault".to_string(), + key: format!("{lock_id} (vault_id={vault_id})"), + }); + } + } + + // Delete all lock records + diesel::delete(vault_locks::table.filter(vault_locks::lock_id.eq(lock_id))) + .execute(self.connection()) + .map_err(|e| WalletStorageError::general(OPERATION, e))?;
🧹 Nitpick comments (14)
crates/wallet/sdk_services/src/indexer_rest_api.rs (1)
255-256: TODO comment clearly documents a potential optimization.The comment articulates a concrete performance concern—inefficiency of JSON + hex encoding for large protobuf payloads—and proposes a reasonable mitigation (supporting
application/x-protobufin the indexer REST API). Well-written and actionable for future work.However, consider whether this optimization is part of the core objectives of this PR (vault locks and partial commit handling), or if it should be tracked as a separate GitHub issue to avoid scope creep and keep the PR focused on the intended changes.
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
93-93: Consider refactoring to reduce function complexity.Suppressing
clippy::too_many_linesindicates the function handles multiple concerns. Consider extracting the UTXO filtering/cleanup logic (lines 146-166) into a separate method to improve maintainability.
134-136: Address the TODO and previous bug mentioned.The TODO references a previous bug where "spent [UTXOs] were marked as unspent." While this PR adds logic to remove missing UTXOs from the queue, the underlying issue of checking local UTXO state before querying the indexer remains unaddressed. This optimization would improve performance and prevent redundant processing.
Do you want me to help implement this optimization or open a new issue to track it?
crates/wallet/sdk/src/apis/non_fungible_tokens.rs (1)
15-16: Remove unusedCommitableStoreimport or use it in the bound.
CommitableStoreis imported but not referenced; this may fail under deny(warnings). Either drop it or change the bound towhere TStore: WalletStore + CommitableStore.Apply this if not needed:
- storage::{CommitableStore, WalletStorageError, WalletStore, WalletStoreReader, WalletStoreWriter}, + storage::{WalletStorageError, WalletStore, WalletStoreReader, WalletStoreWriter},applications/tari_indexer/web_ui/src/routes/Substates/Substates.tsx (1)
62-63: Verify type literal matches backend (“Utxo”).Confirm the API expects exactly "Utxo" (case/spelling). If the server uses a canonical enum/string, prefer importing it from a shared package to avoid drift.
crates/wallet/sdk/src/models/key.rs (1)
56-63: Propagate and persistbranchconsistently; verify migration/serialization.
- Field additions look correct and mapped in
From<DerivedWalletKey>.- Please confirm DB migrations (or the documented data-dir reset) initialize
WalletKeyRecord.branchfor existing rows.- Validate any API payloads that include
WalletPublicKey(now withbranch) remain compatible or are versioned.Also applies to: 78-81, 106-111, 123-128, 140-147
crates/wallet/sdk/src/sdk.rs (1)
98-135: Consider removing commented-out context management code.These commented-out helper methods for read/write context management appear to be unused scaffolding, especially since the AI summary indicates that context management is implemented in
crates/wallet/sdk/src/apis/context.rs. Removing dead code helps maintain codebase clarity.If these methods are intended for future use or documentation, consider adding a comment explaining their purpose or moving them to a design document.
crates/wallet/sdk_services/src/transaction_service/handle.rs (1)
14-19: Plumbing lock_id into SubmitTransaction looks correct; consider an options struct to avoid positional None,None.Two Option parameters are easy to swap. A small Options struct (e.g., SubmitOpts { new_account_info, lock_id }) or builder would reduce call-site errors.
crates/wallet/sdk_services/src/transaction_service/service.rs (1)
152-164: Linking lock_id to transaction_id occurs in a separate DB tx; consider making it atomic.If locks_set_transaction_id() fails, the transaction remains inserted but unlinked. Prefer a single store write that inserts the tx and links the lock (or rolls back both) to avoid partial state.
Would you like a follow-up API on TransactionApi like insert_new_transaction_and_link(transaction, new_account_info, lock_id) -> tx_id?
applications/tari_walletd/src/handlers/accounts.rs (1)
286-289: Boolean parameter readability and semantics.The added false is unclear at call sites. Confirm it excludes locked outputs (intended?), and consider replacing the bool with a descriptive enum or a builder for clarity.
What does the new boolean flag control exactly here?
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
385-457: Avoid per-UTXO write transactions inside the scan loopCreating and committing a write tx per output is expensive and increases contention. Batch within a single write tx (or small chunks) for this loop.
Example:
- for (addr, utxo) in outputs { - let mut tx = self.store.create_write_tx()?; + let mut tx = self.store.create_write_tx()?; + for (addr, utxo) in outputs { match tx .stealth_outputs_get_by_commitment(resource_address, &commitment) .optional()? { // ... } - tx.commit()?; } + tx.commit()?;crates/wallet/sdk/src/apis/stealth_transfer.rs (2)
602-631: Change output indexing guarded by filter is safe, but brittleIndex math depends on which outputs are positive after filtering. Current guard is fine, but consider selecting by tag/owner or passing back the created change commitment to avoid positional coupling.
796-798: Naming nit: main_signer vs additional_signer may confuse integratorsHere main_signer holds the fee signer and additional_signer may hold the transfer signer. Consider renaming to fee_signer and transfer_signer.
crates/wallet/sdk/src/apis/context.rs (1)
144-152: Safer Drop: rollback on panic, otherwise commit.Commit during unwinding can persist inconsistent state. Prefer rollback when panicking.
impl<TCtx: CommitableStore> Drop for WithContext<TCtx> { fn drop(&mut self) { if let Some(mut ctx) = self.ctx.take() { - if let Err(err) = ctx.commit() { - log::error!("Failed to commit context on drop: {}", err); - } + if std::thread::panicking() { + if let Err(err) = ctx.rollback() { + log::error!("Failed to rollback context during panic: {}", err); + } + } else if let Err(err) = ctx.commit() { + log::error!("Failed to commit context on drop: {}", err); + } } } }
📜 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 (44)
applications/tari_indexer/src/lib.rs(1 hunks)applications/tari_indexer/src/network_state_sync/worker.rs(1 hunks)applications/tari_indexer/web_ui/src/routes/Substates/Substates.tsx(2 hunks)applications/tari_walletd/src/handlers/accounts.rs(6 hunks)applications/tari_walletd/src/handlers/auth/jwt.rs(1 hunks)applications/tari_walletd/src/services/webauthn.rs(1 hunks)applications/tari_walletd/web_ui/src/routes/StealthUtxoList/StealthUtxoList.tsx(1 hunks)crates/engine_types/src/commit_result.rs(2 hunks)crates/engine_types/src/resource_container.rs(2 hunks)crates/wallet/crypto/src/memo.rs(0 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(1 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(1 hunks)crates/wallet/sdk/src/apis/config.rs(1 hunks)crates/wallet/sdk/src/apis/context.rs(1 hunks)crates/wallet/sdk/src/apis/key_manager.rs(5 hunks)crates/wallet/sdk/src/apis/non_fungible_tokens.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(5 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(13 hunks)crates/wallet/sdk/src/apis/transaction.rs(5 hunks)crates/wallet/sdk/src/models/key.rs(5 hunks)crates/wallet/sdk/src/sdk.rs(1 hunks)crates/wallet/sdk/src/storage.rs(7 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(1 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(5 hunks)crates/wallet/sdk_services/src/account_monitor/scanner.rs(12 hunks)crates/wallet/sdk_services/src/indexer_rest_api.rs(1 hunks)crates/wallet/sdk_services/src/transaction_service/handle.rs(4 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(3 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs(4 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(2 hunks)crates/wallet/storage_sqlite/src/lib.rs(3 hunks)crates/wallet/storage_sqlite/src/models/vault.rs(3 hunks)crates/wallet/storage_sqlite/src/reader.rs(12 hunks)crates/wallet/storage_sqlite/src/schema.rs(3 hunks)crates/wallet/storage_sqlite/src/writer.rs(13 hunks)crates/wallet/storage_sqlite/tests/accounts.rs(1 hunks)crates/wallet/storage_sqlite/tests/config.rs(1 hunks)crates/wallet/storage_sqlite/tests/key_manager_state.rs(1 hunks)crates/wallet/storage_sqlite/tests/substates.rs(1 hunks)crates/wallet/storage_sqlite/tests/transaction.rs(1 hunks)lints.toml(1 hunks)utilities/db_inspector/src/webserver/server.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/wallet/crypto/src/memo.rs
⏰ 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: machete
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: fmt
🔇 Additional comments (68)
crates/wallet/sdk/Cargo.toml (1)
38-38: LGTM! Tokio sync dependency appropriately added.The addition of
tokiowith thesyncfeature anddefault-features = falsefollows best practices: it's workspace-pinned for consistency, minimal features reduce the attack surface and binary size, and the synchronization primitives (e.g.,Mutex,RwLock) align well with the PR's goal of managing concurrent vault locks. The placement in core dependencies (not dev-only) is correct since vault locking is fundamental SDK functionality.lints.toml (1)
25-25: Re-enablingdbg_macrolint aligns with logging improvements.Uncommenting this lint is a good practice to prevent debug macros in production code and aligns with the PR's logging/formatting refinements mentioned in the summary.
Verify that no remaining
dbg!()calls exist in the codebase that would now be flagged as errors.crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
151-157: LGTM! Appropriate log level adjustment.Reducing this intermediate shard completion message from
info!todebug!improves signal-to-noise ratio in logs. The important final scan completion message at line 210 remains at info level, ensuring users still get visibility into overall progress.utilities/db_inspector/src/webserver/server.rs (2)
22-29: LGTM! Correct path parameter syntax in macro.The route parameter syntax has been correctly updated to use Axum's standard brace notation
{db_name}. The double braces{{db_name}}in theformat!macro (line 25) properly escape to produce{db_name}in the resulting route string.
39-65: Route parameter syntax correctly aligned with Axum standards.All route definitions have been systematically updated to use brace-based path parameters
{db_name}, which is the standard syntax for Axum. The changes are consistent across all affected routes.Optionally verify that the route handlers correctly extract the
db_nameparameter using Axum'sPathextractor with matching field names:crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
98-98: LGTM: Consistent logging style update.The emoji prefix changes standardize logging across the account monitor subsystem.
Also applies to: 105-105, 128-128, 192-192, 197-197, 222-222, 227-227, 241-241
crates/wallet/sdk_services/src/account_monitor/scanner.rs (3)
38-38: LGTM: Derive additions enable useful traits.Adding
DebugandClonetoAccountScannerexpands the public API surface in a backward-compatible way and aligns with the broader refactoring to support concurrent operations.
57-57: LGTM: Consistent logging style update.The emoji prefix changes standardize logging across the account scanner, matching the updates in monitor.rs.
Also applies to: 432-432, 447-447, 472-472, 525-525, 533-533, 539-539, 553-553, 599-599, 611-611, 659-659, 674-674
414-414: Verify interior mutability in storage layer.Changing
process_resultfrom&mut selfto&selfrelaxes mutability requirements, enabling concurrent access patterns. This aligns with the PR's goal of supporting multiple vault locks. However, ensure that the storage layer (WalletStoreand related traits) properly handles interior mutability for all mutation operations called within this method (e.g.,update_account,add_vault,save_nft, etc.).Run the following to confirm the storage trait refactoring provides sound interior mutability:
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
15-15: LGTM!The
WalletStorageErrorimport is necessary for the new transaction handling logic.
168-172: Verify empty response handling aligns with error handling strategy.The change from error to
continuewhen no UTXOs are returned is more resilient. Combined with the pre-query cleanup (lines 146-166), this makes sense. However, ensure this doesn't mask legitimate indexer connectivity issues that should surface as errors in the outer error handling (lines 59-77).applications/tari_indexer/src/lib.rs (1)
192-195: LGTM: switch to structureddebug!logging.Consistent with the module’s
LOG_TARGET; no behavior change.applications/tari_walletd/web_ui/src/routes/StealthUtxoList/StealthUtxoList.tsx (1)
67-73: LGTM: column widths align with 6-column layout.Totals = 100%, headers and colSpan remain consistent after removing the extra column.
applications/tari_indexer/web_ui/src/routes/Substates/Substates.tsx (1)
94-95: Nit: dependency array change is fine.Trailing comma is a no-op; readability OK.
crates/engine_types/src/resource_container.rs (1)
333-336: LGTM: clearer error messages.Updated wording (“Bucket or vault…”) improves diagnostics; no behavior change.
Also applies to: 345-348
crates/engine_types/src/commit_result.rs (1)
200-203: LGTM: addreject()accessors.Simple, ergonomic access to rejection reasons; aligns with existing
any_*APIs.Also applies to: 295-301
crates/wallet/sdk/src/apis/accounts.rs (1)
42-42: LGTM: Import addition aligns with storage trait refactoring.The addition of
CommitableStoreto the imports is consistent with the broader PR objective to introduce granular storage traits.applications/tari_walletd/src/services/webauthn.rs (1)
6-12: LGTM: Storage trait import update.The addition of
CommitableStoreto the import list aligns with the PR's storage trait refactoring. The import formatting is clear and consistent.crates/wallet/sdk/src/apis/config.rs (1)
9-9: LGTM: Import addition supports storage trait refactoring.The
CommitableStoreimport aligns with the broader storage interface restructuring in this PR.crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
19-19: LGTM: Storage trait import update.The addition of
CommitableStoreis consistent with the storage interface restructuring across the PR.crates/wallet/sdk/tests/confidential_output_api.rs (1)
10-10: LGTM: Test imports updated for new storage traits.The change from
WalletStoretoReadableWalletStorecorrectly reflects the storage trait refactoring and is appropriate for this test's usage patterns.applications/tari_walletd/src/handlers/auth/jwt.rs (1)
10-16: LGTM: Storage import update for trait refactoring.The addition of
CommitableStoreto the import list is consistent with the PR's storage trait restructuring. The multi-line import formatting enhances readability.crates/wallet/storage_sqlite/tests/accounts.rs (1)
8-8: LGTM: Test imports updated for granular storage traits.The import changes correctly reflect the storage trait split into
ReadableWalletStore,WriteableWalletStore, andCommitableStore, replacing the monolithicWalletStoretrait. This aligns with the PR's architectural improvements.crates/wallet/storage_sqlite/tests/transaction.rs (1)
8-8: Import update aligns with storage trait refactor.The addition of
CommitableStore,ReadableWalletStore, andWriteableWalletStoreto the import surface is consistent with the broader PR refactoring that splits storage capabilities into granular traits.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (3)
104-114: Vaults table correctly updated for multi-lock architecture.The removal of
locked_revealed_balanceand_locked_bycolumns (per the AI summary) and the retention of core vault fields aligns with the shift to thevault_locksjunction table for managing multiple locks per vault.
119-129: Vault locks junction table properly designed.The
vault_lockstable correctly implements a many-to-many relationship between vaults and locks with:
- Appropriate foreign keys with
CASCADEdelete for referential integrity- Unique constraint on
(vault_id, lock_id)preventing duplicatesamountfield to track the locked amount per lockThis enables the concurrent transfer capability mentioned in the PR objectives.
186-186: Partial unique index correctly handles NULL transaction IDs.The filtered unique index on
transaction_idallows multiple locks without assigned transactions (NULL values) while enforcing uniqueness for locks with transaction IDs. This is the correct approach for optional transaction association.crates/wallet/storage_sqlite/tests/key_manager_state.rs (1)
5-11: Import surface updated consistently.The expanded imports align with the storage trait split introduced across the PR.
crates/wallet/storage_sqlite/tests/substates.rs (1)
8-14: Import surface updated consistently.The expanded imports align with the storage trait split introduced across the PR.
crates/wallet/sdk/src/apis/key_manager.rs (2)
37-37: Import updated for new storage trait.The addition of
CommitableStorealigns with the storage API refactoring across the PR.
79-79: Branch field added consistently to key records.The
branchfield has been added to all key-related struct constructions (WalletKeyRecord,WalletPublicKey,DerivedWalletKey), ensuring branch context is preserved throughout the key lifecycle. The changes are consistent across all construction sites.Also applies to: 141-141, 149-149, 174-174, 261-261
crates/wallet/storage_sqlite/tests/config.rs (1)
5-11: Import surface updated consistently.The expanded imports align with the storage trait split introduced across the PR.
crates/wallet/storage_sqlite/src/lib.rs (3)
21-21: Import updated for storage trait split.The updated imports reflect the split of storage capabilities into
ReadableWalletStoreandWriteableWalletStoretraits.
57-67: ReadableWalletStore implementation is correct.The implementation properly provides read-only transaction capabilities with appropriate transaction lifecycle management.
69-79: WriteableWalletStore implementation is correct.The implementation properly provides write transaction capabilities, completing the separation of read and write storage concerns.
crates/wallet/storage_sqlite/src/schema.rs (3)
218-226: Vault locks table schema correctly defined.The Diesel schema declaration matches the migration SQL and properly defines the junction table for vault-lock relationships.
228-242: Vaults table schema updated correctly.The schema reflects the removal of locking-related columns, consistent with the migration to the
vault_lockstable.
270-271: Table relationships properly declared.The
joinable!declarations and inclusion inallow_tables_to_appear_in_same_query!correctly establish the relationships betweenvault_locks,locks, andvaultsfor Diesel ORM queries.Also applies to: 291-291
crates/wallet/sdk_services/src/transaction_service/handle.rs (2)
39-41: LGTM on delegating with defaults.Forwarding None for new_account_info and lock_id is clear and preserves prior behavior.
58-75: submit_transaction_with_opts correctly threads lock_id; please verify all call-sites updated.New param surfaces widely; ensure no stale usages.
Run to find and review all call sites:
crates/wallet/sdk_services/src/transaction_service/service.rs (1)
109-121: Request handling forwards lock_id correctly.Match arm captures lock_id and passes it through as intended.
applications/tari_walletd/src/handlers/accounts.rs (3)
679-689: Submit with extended opts (None lock_id) looks fine.Matches new signature; no lock to associate in this flow.
1013-1030: Dry‑run path properly releases the lock before submission.Looks correct; aligns with TODO to avoid locking on dry runs at the source.
1032-1047: unlock_on_failure usage—confirm signature and intent.You pass the awaited Result into unlock_on_failure(). If the API expects a closure/future to run and auto‑release on error, consider passing a closure to also cover pre‑submission errors. Otherwise, confirm it’s defined to accept a Result and release accordingly.
Please confirm unlock_on_failure(lock_id, result) semantics.
crates/wallet/sdk/src/apis/transaction.rs (5)
92-107: Good: map immediate submission rejection to InvalidTransaction and return Ok(false).Clearer caller contract and preserves error semantics for non‑rejection statuses.
260-279: Finalize/release logic aligns with diff‑driven commit; ensure 1‑tx ↔ 1‑lock invariant.This now unlocks a single linked lock when a diff exists, else releases all. If multiple locks can be tied to a single tx, some may leak. Please confirm the invariant that exactly one WalletLockId is linked per tx.
295-303: locks_set_transaction_id API is a useful addition.Simple, single-purpose linking method—good.
355-357: Equality fix in commit_diff is correct.Comparing by value avoids missed child attachment.
305-316: Release path targets a single lock; verify this matches schema/usage.If the store can hold multiple locks per tx, iterate and release all; otherwise this is correct.
Run to find the linking/getter API and check its cardinality:
crates/wallet/storage_sqlite/src/models/vault.rs (1)
62-67: Do notexpecton BigDecimal conversion; propagate an error instead. Potential runtime panic.BigDecimal::to_u128() returns None on overflow or non-integer scale; this will panic. Storage code must not panic.
Apply a safe conversion and error mapping:
- locked_revealed_balance: Amount::from( - locked_revealed_balance - .to_u128() - // Should be impossible because sqlite is limited to i64 - .expect("locked more than u128::MAX funds"), - ), + locked_revealed_balance: { + use bigdecimal::ToPrimitive; + let v = locked_revealed_balance + .to_i64() + .ok_or_else(|| WalletStorageError::DecodingError { + operation: "try_into_vault", + item: "vault.locked_revealed_balance", + details: format!("non-integer or out of i64 range: {locked_revealed_balance}"), + })?; + if v < 0 { + return Err(WalletStorageError::DecodingError { + operation: "try_into_vault", + item: "vault.locked_revealed_balance", + details: "negative locked balance".to_string(), + }); + } + Amount::from(v) + },Additionally, please confirm upstream that locked_revealed_balance is stored in base units (no fractional scale).
crates/wallet/sdk/src/apis/stealth_outputs.rs (4)
188-190: Bold move to centralize release via tx.locks_releaseThis simplifies lock teardown into a single atomic op. LGTM.
192-196: Finalize method returns the wrong API error type for this moduleThis is StealthOutputsApi, but the method returns ConfidentialOutputsApiError. Align the error type to avoid surprising call sites and unnecessary coupling.
[ suggest_recommended_refactor ]
Apply this diff:- pub fn finalize_lock(&self, lock_id: WalletLockId, diff: &SubstateDiff) -> Result<(), ConfidentialOutputsApiError> { + pub fn finalize_lock(&self, lock_id: WalletLockId, diff: &SubstateDiff) -> Result<(), StealthOutputsApiError> { self.store - .with_write_tx(|tx| tx.locks_unlock_finalized(lock_id, diff))?; + .with_write_tx(|tx| tx.locks_unlock_finalized(lock_id, diff))?; Ok(()) }If other modules depend on the previous signature, add a thin wrapper in their APIs instead of reusing this method with a mismatched error.
261-267: New exclude_locked toggle is a good additionForwarding exclude_locked down to storage keeps the API precise and avoids client-side filtering. LGTM.
293-305: upsert_utxo preserves non-Unspent status — sensible guardrailOnly allowing status changes from Unspent prevents rescans from reviving spent/invalid outputs. LGTM.
To confirm no unintended status flips elsewhere, search for direct updates bypassing this method:
crates/wallet/sdk/src/apis/stealth_transfer.rs (3)
481-489: Good: explicit logging when adding unconfirmed fee change outputImproves traceability during concurrent transfers. LGTM.
677-687: unlock_on_failure made public — good ergonomicsCentralizes lock cleanup on error paths. LGTM.
235-241: Critical: lock_id is re-created and shadowed in PreferConfidential, leaking the caller’s lockThis branch creates a new lock_id instead of using the provided one, so the returned InputsToSpend cannot be finalized/released with the outer lock. Locks will leak and outputs remain stuck.
Apply this fix:
- let lock_id = self.outputs_api.create_lock()?; - let (inputs, blinded_amount_locked) = self.outputs_api.lock_outputs_until_partial_amount( + let (inputs, blinded_amount_locked) = self.outputs_api.lock_outputs_until_partial_amount( owner_account_component_address, &resource_address, spend_amount, - lock_id, + lock_id, )?;Add a test to assert all locked outputs for a transfer share the same lock_id.
crates/wallet/storage_sqlite/src/writer.rs (2)
1188-1215: Allowing re-lock of same-lock_id unconfirmed outputs is correct for spending fee changeThe added OR branch enabling LockedUnconfirmed for the same lock avoids races when spending change in a single flow. LGTM.
1642-1643: Drop now calls rollback_internal — good safety netPrevents dangling write transactions. LGTM.
crates/wallet/sdk/src/storage.rs (4)
79-95: with_write_tx handles commit/rollback internallyClearer ergonomics and fewer footguns for callers. LGTM.
253-258: Reader now supports exclude_locked for stealth outputsGood capability uplift with explicit param. LGTM.
277-281: locks_get_by_transaction_id returns a single idMakes sense for 1:1 lock/tx association. LGTM.
478-485: Lock lifecycle APIs (release/unlock_finalized) are well‑documentedInterfaces align with new diff-driven finalization model. LGTM.
crates/wallet/storage_sqlite/src/reader.rs (4)
534-548: vaults_get includes locked_revealed_balance via vault_locks sumAccurate balance view without overloading the main vault row. LGTM.
568-603: vaults_get_by_resource computes locked_revealed_balance correctlyConsistent with vaults_get; improves accuracy under concurrent locks. LGTM.
928-953: exclude_locked flag plumbed through for stealth outputsOptional lock filtering at the DB is the right place. LGTM.
1076-1092: locks_get_by_transaction_id returns NotFound for missing — appropriateBehavior matches new trait signature. LGTM.
crates/wallet/sdk/src/apis/context.rs (1)
18-21: Verify Clone on SdkReadContext is valid.Deriving Clone requires TStore::ReadTransaction<'ctx>: Clone. If it isn’t, this will not compile. Either remove Clone or add a manual impl with the appropriate bound.
Possible quick fix (remove Clone):
-#[derive(Debug, Clone)] +#[derive(Debug)] pub struct SdkReadContext<'ctx, TStore: ReadableWalletStore> { reader: TStore::ReadTransaction<'ctx>, }Or implement Clone conditionally:
impl<'ctx, TStore> Clone for SdkReadContext<'ctx, TStore> where TStore: ReadableWalletStore, TStore::ReadTransaction<'ctx>: Clone, { fn clone(&self) -> Self { Self { reader: self.reader.clone() } } }
Test Results (CI)338 tests - 113 334 ✅ - 117 1h 8m 50s ⏱️ + 17m 47s For more details on these failures, see this check. Results for commit 14d0d3c. ± Comparison against base commit 2cff16c. This pull request removes 113 tests. |
4271c81 to
858fe67
Compare
858fe67 to
e4cfbb3
Compare
2 participants
Description
fix(wallet)!: support multiple vault locks, handle partial commit case
Motivation and Context
The payment processor implementation highlighted some issues when performing transfers concurrently. This PR addresses those issues.
This PR allows multiple vault locks to be acquired to allow concurrent transfers of vault funds.
There was a TODO in the code to handle the case where the main transaction intent was rejected, but the fee intent accepted. This could result in incorrect fund tracking (output and vault) in this case. Specifically, it was observed that the wallet would attempt to spend outputs that were never created on-chain in the payment processor.
How Has This Been Tested?
Manually (payment processor)
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
UI Updates
Chores