Skip to content

chore(wallet/sdk): minor cleanup - #1627

Merged
sdbondi merged 1 commit into
tari-project:developmentfrom
sdbondi:sdk-cleanup-storage-traits
Nov 3, 2025
Merged

chore(wallet/sdk): minor cleanup#1627
sdbondi merged 1 commit into
tari-project:developmentfrom
sdbondi:sdk-cleanup-storage-traits

Conversation

@sdbondi

@sdbondi sdbondi commented Nov 3, 2025

Copy link
Copy Markdown
Member

Description

chore: move traits into modules

Motivation and Context

Investigating improvements to api with better atomicity, but unfortunately, was not able to figure it out without large changes. However part of that was moving traits into smaller modules, so why not PR it?

Summary by CodeRabbit

  • New Features

    • Enhanced lock management with new timeout-based creation, release, vault fund locking, and stale lock cleanup capabilities.
  • Bug Fixes

    • Corrected storage type naming inconsistency across wallet components.
  • Refactor

    • Reorganized wallet storage layer with improved transaction handling and modularity.
    • Removed deprecated SDK context management methods; use new storage transaction APIs instead.

@sdbondi
sdbondi merged commit e01b553 into tari-project:development Nov 3, 2025
11 checks passed
@coderabbitai

coderabbitai Bot commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

Walkthrough

This PR consolidates the wallet storage abstraction by restructuring a monolithic storage module into separate error, reader, and writer components, fixes the typo CommitableStoreCommittableStore across the codebase, refactors LocksApi with trait-bounded impl blocks, removes context management methods from WalletSdk, and updates the accounts API to use transactional helpers.

Changes

Cohort / File(s) Summary
Typo Fix: CommitableStore → CommittableStore
applications/tari_walletd/src/handlers/auth/jwt.rs, applications/tari_walletd/src/services/webauthn.rs, crates/wallet/sdk/src/apis/confidential_outputs.rs, crates/wallet/sdk/src/apis/config.rs, crates/wallet/sdk/src/apis/key_manager.rs, crates/wallet/sdk/src/apis/non_fungible_tokens.rs, crates/wallet/sdk/src/apis/stealth_outputs.rs, crates/wallet/sdk/src/models/lock_guard.rs, crates/wallet/storage_sqlite/src/writer.rs, crates/wallet/storage_sqlite/tests/*
Import spelling corrected from CommitableStore to CommittableStore across multiple modules.
Storage Module Restructuring
crates/wallet/sdk/src/storage.rs (removed), crates/wallet/sdk/src/storage/error.rs, crates/wallet/sdk/src/storage/mod.rs, crates/wallet/sdk/src/storage/reader.rs, crates/wallet/sdk/src/storage/writer.rs
Monolithic storage.rs file removed and refactored into modular components: error.rs defines WalletStorageError and CommittableStore trait; mod.rs defines ReadableWalletStore, WriteableWalletStore, WalletStore with blanket impls and with_read_tx/with_write_tx helpers; reader.rs defines comprehensive WalletStoreReader trait; writer.rs defines WalletStoreWriter trait extending CommittableStore.
LocksApi Refactoring
crates/wallet/sdk/src/apis/locks.rs
Split impl blocks by trait bounds: general impl with no bounds, WriteableWalletStore-bound impl containing mutating operations (create_lock_with_timeout, release_lock, finalize_lock, lock_funds_in_vault, clear_stale_locks), and ReadableWalletStore-bound impl containing read operation (get_lock_by_transaction_id).
Accounts API Update
crates/wallet/sdk/src/apis/accounts.rs
Changed count() method to use `store.with_read_tx(
WalletSdk Context Methods Removal
crates/wallet/sdk/src/sdk.rs
Removed four public methods: create_read_context(), with_read_context(), create_write_context(), and with_write_context().

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant SDK as WalletSdk
    participant Store as ReadableWalletStore/<br/>WriteableWalletStore
    participant Tx as Transaction

    rect rgb(200, 230, 255)
    Note over Client,Tx: Read Flow (with_read_tx)
    Client->>SDK: api.with_read_tx(closure)
    SDK->>Store: create_read_tx()
    Store->>Tx: create transaction
    Tx-->>SDK: ReadTransaction
    SDK->>Tx: closure(&mut tx)
    Tx->>Tx: read operations
    Tx-->>SDK: Result<R>
    SDK-->>Client: Result<R>
    end

    rect rgb(230, 200, 255)
    Note over Client,Tx: Write Flow (with_write_tx)
    Client->>SDK: api.with_write_tx(closure)
    SDK->>Store: create_write_tx()
    Store->>Tx: create transaction
    Tx-->>SDK: WriteTransaction
    SDK->>Tx: closure(&mut tx)
    Tx->>Tx: write operations
    alt Success
        Tx->>Tx: commit()
        Tx-->>SDK: Ok(Result<R>)
    else Error in Closure
        Tx->>Tx: rollback()
        Tx-->>SDK: Err(WalletStorageError)
    end
    SDK-->>Client: Result<R>
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Storage module restructuring: Verify that trait definitions, blanket implementations, and with_read_tx/with_write_tx logic are correctly implemented across error.rs, mod.rs, reader.rs, and writer.rs files
  • LocksApi trait-bounded impl blocks: Ensure all methods are correctly placed in the appropriate impl block (WriteableWalletStore vs. ReadableWalletStore vs. unbounded) and that trait bounds are properly enforced
  • Removed public APIs: Confirm that removal of context management methods from WalletSdk doesn't break existing callers and validate the migration path to with_read_tx/with_write_tx patterns
  • Repetitive import updates: While most are straightforward spelling corrections, verify that all CommitableStore → CommittableStore renames are complete and consistent across all files
  • Transaction logic update in accounts.rs: Validate that the closure-based pattern in count() correctly replaces the previous transaction handling

Possibly related PRs

Suggested labels

P-acks_required, P-reviews_required

Suggested reviewers

  • stringhandler

Poem

🐰 A typo fixed, storage neat,
From one big file to modules sweet,
With read and write txns flow,
Through branching bounds, the traits now glow,
Context gone, but closures greet!

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7ab4ae0 and 1b5e0a6.

📒 Files selected for processing (22)
  • applications/tari_walletd/src/handlers/auth/jwt.rs (1 hunks)
  • applications/tari_walletd/src/services/webauthn.rs (1 hunks)
  • crates/wallet/sdk/src/apis/accounts.rs (2 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/key_manager.rs (1 hunks)
  • crates/wallet/sdk/src/apis/locks.rs (2 hunks)
  • crates/wallet/sdk/src/apis/non_fungible_tokens.rs (1 hunks)
  • crates/wallet/sdk/src/apis/stealth_outputs.rs (1 hunks)
  • crates/wallet/sdk/src/models/lock_guard.rs (1 hunks)
  • crates/wallet/sdk/src/sdk.rs (0 hunks)
  • crates/wallet/sdk/src/storage.rs (0 hunks)
  • crates/wallet/sdk/src/storage/error.rs (1 hunks)
  • crates/wallet/sdk/src/storage/mod.rs (1 hunks)
  • crates/wallet/sdk/src/storage/reader.rs (1 hunks)
  • crates/wallet/sdk/src/storage/writer.rs (1 hunks)
  • crates/wallet/storage_sqlite/src/writer.rs (2 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)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants