feat!(wallet): persist wallet events - #1631
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughEvent types and event handling were moved from the services events module into the SDK models, new SDK EventsApi and storage writer support were added to persist events (DB schema and writer impl), event structs were enriched (Serialize, Display, added fields), multiple imports updated across handlers/services, and event logging calls were introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Scanner as Account/UTXO Scanner
participant Monitor as Account Monitor
participant SDK as Wallet SDK
participant EventAPI as EventsApi
participant Store as Wallet Store (WriteTx)
Note over Scanner,Monitor: Event constructed (enriched with version/account_address)
Scanner->>Monitor: emit WalletEvent
Monitor->>EventAPI: event_api().log_event(event)
EventAPI->>SDK: begin write transaction
SDK->>Store: with_write_tx()
EventAPI->>Store: append_wallet_event(event)
Store->>Store: resolve account_id, serialize event_data
Store->>Store: INSERT INTO wallet_events
Store-->>SDK: commit/rollback
SDK-->>EventAPI: result
EventAPI-->>Monitor: ok / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (30)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
applications/tari_walletd/src/handlers/accounts.rs(1 hunks)applications/tari_walletd/src/handlers/context.rs(1 hunks)applications/tari_walletd/src/handlers/helpers.rs(1 hunks)applications/tari_walletd/src/handlers/rpc.rs(1 hunks)applications/tari_walletd/src/handlers/transaction.rs(1 hunks)applications/tari_walletd/src/services/mod.rs(2 hunks)applications/tari_walletd/src/services/template_monitor.rs(1 hunks)crates/template_lib_types/src/encrypted_data.rs(1 hunks)crates/template_lib_types/src/max_string.rs(2 hunks)crates/wallet/crypto/src/memo.rs(3 hunks)crates/wallet/sdk/src/apis/events.rs(1 hunks)crates/wallet/sdk/src/apis/mod.rs(1 hunks)crates/wallet/sdk/src/models/event.rs(2 hunks)crates/wallet/sdk/src/models/mod.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(2 hunks)crates/wallet/sdk/src/storage/mod.rs(1 hunks)crates/wallet/sdk/src/storage/writer.rs(2 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(2 hunks)crates/wallet/sdk_services/src/account_monitor/scanner.rs(9 hunks)crates/wallet/sdk_services/src/lib.rs(0 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(2 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(2 hunks)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/worker.rs(2 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(2 hunks)crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/down.sql(0 hunks)crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/up.sql(0 hunks)crates/wallet/storage_sqlite/src/schema.rs(4 hunks)crates/wallet/storage_sqlite/src/writer.rs(2 hunks)
💤 Files with no reviewable changes (3)
- crates/wallet/sdk_services/src/lib.rs
- crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/up.sql
- crates/wallet/storage_sqlite/migrations/2025-10-29-122554_add_timeout_at_to_locks/down.sql
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-19T10:49:02.115Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1543
File: crates/common_types/src/shard_state_versions.rs:59-74
Timestamp: 2025-08-19T10:49:02.115Z
Learning: In crates/common_types/src/shard_state_versions.rs, MAX_SHARDS represents the maximum shard number + 1 (257) used for bounds checking, not the maximum capacity. The current code correctly validates that shard group end numbers don't exceed the maximum possible shard number (256).
Applied to files:
crates/template_lib_types/src/encrypted_data.rs
📚 Learning: 2025-11-04T10:10:24.249Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1629
File: applications/tari_walletd/src/handlers/accounts.rs:1001-1002
Timestamp: 2025-11-04T10:10:24.249Z
Learning: In applications/tari_walletd/src/handlers/accounts.rs, the expect() on Memo::new_pay_ref_and_bytes_truncate at line 1002 is safe and intentional. PayRef is validated to be at most 64 bytes during address decoding (PayRef::MAX_LEN = 64), and the function only returns None if payref exceeds 252 bytes (Memo::MAX_BYTES_LENGTH - 1). Since 64 < 252, None is impossible with a valid PayRef.
Applied to files:
crates/wallet/crypto/src/memo.rs
🧬 Code graph analysis (15)
crates/wallet/sdk/src/apis/mod.rs (1)
crates/engine/src/runtime/working_state.rs (1)
events(1313-1315)
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
notify(89-93)crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
crates/template_lib_types/src/max_string.rs (4)
crates/wallet/crypto/src/memo.rs (1)
fmt(259-285)crates/wallet/sdk/src/models/event.rs (2)
fmt(93-95)fmt(174-188)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (1)
fmt(216-218)crates/engine_types/src/confidential/claim.rs (1)
fmt(28-40)
applications/tari_walletd/src/services/mod.rs (2)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
new(43-49)notify(89-93)crates/wallet/sdk_services/src/notify.rs (2)
new(12-15)notify(21-23)
crates/wallet/sdk_services/src/transaction_service/service.rs (3)
bindings/src/types/TransactionStatus.ts (1)
TransactionStatus(3-11)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
notify(89-93)crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
crates/wallet/sdk/src/storage/writer.rs (1)
crates/wallet/storage_sqlite/src/writer.rs (1)
append_wallet_event(1671-1715)
crates/wallet/sdk/src/apis/events.rs (2)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
is_not_found_error(309-311)crates/wallet/sdk/src/models/event.rs (11)
from(33-35)from(39-41)from(45-47)from(51-53)from(57-59)from(63-65)from(69-71)from(75-77)from(81-83)from(87-89)from(192-205)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (2)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
notify(89-93)crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
applications/tari_walletd/src/handlers/accounts.rs (3)
bindings/src/types/wallet-daemon-client/BranchAndKeyId.ts (1)
BranchAndKeyId(5-5)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/storage_sqlite/src/writer.rs (3)
crates/wallet/sdk/src/storage/writer.rs (1)
append_wallet_event(222-222)crates/wallet/storage_sqlite/src/reader.rs (15)
accounts(380-382)accounts(402-405)accounts(418-420)accounts(435-437)accounts(446-448)accounts(477-479)accounts(498-501)accounts(528-531)accounts(612-615)accounts(759-762)accounts(806-809)accounts(829-832)accounts(972-975)accounts(1009-1012)accounts(1042-1046)crates/wallet/sdk/src/storage/error.rs (1)
general(48-53)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
applications/tari_walletd/src/services/template_monitor.rs (2)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
notify(89-93)crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)
crates/wallet/sdk/src/models/event.rs (8)
bindings/src/types/FinalizeResult.ts (1)
FinalizeResult(9-16)bindings/src/types/UtxoAddress.ts (1)
UtxoAddress(4-4)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/TransactionId.ts (1)
TransactionId(3-3)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/TransactionStatus.ts (1)
TransactionStatus(3-11)bindings/src/types/wallet-daemon-client/AuthLoginRequest.ts (1)
AuthLoginRequest(4-8)bindings/src/types/UtxoSpent.ts (1)
UtxoSpent(4-4)
crates/wallet/crypto/src/memo.rs (3)
applications/tari_wallet_cli/src/command/transaction.rs (10)
s(808-808)s(811-811)s(814-814)s(817-817)s(820-820)s(823-823)s(826-826)s(829-829)s(832-832)s(836-836)applications/tari_walletd/web_ui/src/components/Memo.tsx (1)
Memo(13-49)bindings/src/types/Memo.ts (1)
Memo(3-3)
crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
crates/wallet/sdk/src/sdk.rs (2)
accounts_api(199-206)substate_api(195-197)
⏰ 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). (7)
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: file licenses
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: fmt
- GitHub Check: machete
🔇 Additional comments (44)
crates/template_lib_types/src/encrypted_data.rs (1)
8-8: LGTM! Documentation improves clarity.The added documentation comment accurately describes the MAX_SIZE constant, correctly stating the 335-byte maximum (80 bytes for encrypted data without memo + 255 bytes for memo).
applications/tari_walletd/src/services/template_monitor.rs (3)
13-13: LGTM: Import path updates align with the refactoring.The import changes correctly reflect the movement of
WalletEventfrom the services layer to the models layer, consistent with the broader SDK refactor described in the PR summary.Also applies to: 18-18
80-82: No issues found. Code is compatible.The
WalletEvent::TransactionFinalizedvariant and its structure are fully compatible. Verification confirms:
TransactionFinalizedEventstruct exists withfinalize: FinalizeResultfieldFinalizeResultstruct hasresult: TransactionResultfieldTransactionResult::any_accept()method exists and returnsOption<&SubstateDiff>The pattern matches existing usage throughout the codebase.
13-13: Clarify what specifically changed in this PR; concerns about event persistence are valid but lack specificity.The review comment raises legitimate concerns about breaking changes in event persistence, but the code shown in template_monitor.rs (lines 13, 80-119) does not demonstrate what changed. Line 13 is an import, and lines 80-119 contain standard event-handling logic without modifications visible.
The actual breaking-change risk lies in the event serialization layer:
- WalletEvent structs are marked with
Serializeonly—noDeserializeimplementation exists- Events are persisted as JSON without versioning or schema tags
- Any change to WalletEvent variant structure or payload fields will render previously persisted events unreadable
To address the concerns properly:
- Show the diff: What changed in the WalletEvent definition or storage schema in this PR?
- Migration strategy: If WalletEvent schema changed, confirm whether:
- Backward compatibility is maintained (old JSON still deserializes), OR
- Data directory deletion is the intended recovery path
- Testing: For any breaking schema change, add integration tests verifying serialization/deserialization round-trips.
crates/template_lib_types/src/max_string.rs (1)
4-8: Display forMaxStringis spot on.
Delegating to the inner slice keeps formatting cheap and lets the newMemodisplay compile cleanly.Also applies to: 68-71
crates/wallet/crypto/src/memo.rs (2)
121-133: Ergonomic UTF-8 accessor lands nicely.
This mirrors existing parsing logic and saves every caller from rewriting the split/UTF-8 dance while failing softly on bad data.
258-285: Display coverage across variants looks great.
The explicit invalid-encoding guard plus hex formatting will make diagnostics far clearer without risking panics.applications/tari_walletd/src/handlers/accounts.rs (1)
33-34: Import swap aligns with the new models surface.
Thanks for consolidating the event types undertari_ootle_wallet_sdk::models.crates/wallet/sdk/src/apis/mod.rs (1)
9-9: Nice to seeeventswired into the API module.
Keeps the public surface consistent with the new SDK entry points.crates/wallet/sdk/src/sdk.rs (1)
26-27: Event API exposure looks great.
Borrowing the store matches the other API constructors and gives callers the missing hook to log events.Also applies to: 164-166
applications/tari_walletd/src/handlers/helpers.rs (1)
13-13: LGTM! Import path updated correctly.The import path for event types has been successfully updated to use the models module, aligning with the architectural refactor to consolidate event types.
applications/tari_walletd/src/handlers/context.rs (1)
5-5: LGTM! Import path updated correctly.The WalletEvent import has been correctly updated to reference the models module.
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
15-15: LGTM! Import path updated correctly.The WalletEvent import has been correctly updated to use the models module.
26-26: LGTM! Local imports consolidated.The local imports have been correctly consolidated to use
crate::notifyandcrate::utxo_scannerpaths.crates/wallet/sdk/src/models/mod.rs (2)
8-8: LGTM! Event module added to models.This addition properly exposes the event module as part of the models layer, enabling the architectural consolidation of event types.
24-24: LGTM! Event types exported correctly.The public re-export of event types makes them accessible via the models module, consistent with the refactoring goals.
applications/tari_walletd/src/handlers/rpc.rs (1)
5-5: LGTM! Import path updated correctly.The AuthLoginRequestEvent import has been correctly updated to reference the models module.
crates/wallet/sdk_services/src/transaction_service/service.rs (2)
10-18: LGTM! Model imports expanded correctly.The models import has been properly expanded to include all necessary event types, consolidating them from the previous events module.
35-35: LGTM! Notify import path updated.The Notify import has been correctly updated to use the
crate::notifypath.crates/wallet/sdk_services/src/account_monitor/monitor.rs (2)
19-19: LGTM! Import path updated correctly.The WalletEvent import has been correctly updated to use the models module.
251-254: Event persistence added with appropriate error handling.The addition of event logging via
event_api().log_event()introduces the key new behavior of this PR—persisting wallet events for historical review. The error handling is appropriate:
- Debug logging for event receipt helps with traceability
- Warning on persistence failure is non-blocking, which is correct since event logging should not disrupt the main workflow
- Errors are logged but not propagated, treating persistence as an auxiliary feature
applications/tari_walletd/src/services/mod.rs (2)
18-18: LGTM! Import path updated correctly.The WalletEvent import has been correctly updated to reference the models module.
57-57: LGTM! UtxoRecovery wiring enhanced with event notification.The addition of
.with_notify(notify.clone())properly wires the event notification system into the UtxoRecovery service, enabling it to emit events that will be persisted via the new event logging functionality.crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
6-6: LGTM - Import path updated to reflect new module structure.The import path for
WalletEventhas been correctly updated to use the SDK models layer.crates/wallet/sdk/src/storage/mod.rs (1)
39-42: LGTM - Trait bounds extended to enable event persistence.The addition of
WalletEventStoreWriterto the trait bounds enables write transactions to persist wallet events alongside other storage operations, maintaining transactional consistency.crates/wallet/sdk/src/storage/writer.rs (1)
221-223: LGTM - Clean trait design for event persistence.The
WalletEventStoreWritertrait provides a focused interface for appending wallet events to storage. The implementation (visible in the SQLite writer) properly handles event serialization and account association.crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
105-107: LGTM - Event emission optimized for empty batches.The early return when the batch is empty avoids emitting unnecessary
UtxoRecoveryStartedEventandUtxoRecoveryCompletedEventpairs. This is a sensible optimization, though it does change the observable behavior for consumers monitoring these events.applications/tari_walletd/src/handlers/transaction.rs (1)
14-14: LGTM - Import paths updated to reflect module reorganization.The imports correctly reference the new locations for
WalletEvent(models) andTransactionServiceError(services).crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
240-243: LGTM - Event enriched with account context.The
UtxoSpentEventnow includes theaccount_addressfield, providing better context for event consumers. This aligns with the PR's goal of enriching events with contextual information.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
310-310: Consider foreign key behavior on account deletion.The
ON DELETE NO ACTIONconstraint means wallet events will remain if an account is deleted, which could lead to orphaned records. Verify this is intentional for audit trail purposes, or considerON DELETE CASCADEif events should be removed with accounts.crates/wallet/storage_sqlite/src/writer.rs (1)
1670-1716: LGTM! Event persistence implementation is solid.The implementation correctly:
- Extracts optional account addresses from each event variant
- Serializes event payloads using JSON
- Resolves account IDs from addresses when present
- Handles missing accounts gracefully with
optional()- Inserts event records with appropriate error handling
crates/wallet/sdk/src/apis/events.rs (3)
11-20: LGTM! API structure follows established patterns.The EventsApi follows the same pattern as other APIs in the codebase (SubstatesApi, AccountsApi) with a store reference and simple constructor.
22-26: LGTM! Event logging implementation is correct.The method properly delegates to the storage layer through a write transaction, ensuring events are persisted atomically.
41-51: LGTM! Error handling follows established patterns.The error type correctly wraps WalletStorageError and delegates the IsNotFoundError trait implementation to the wrapped error.
crates/wallet/sdk_services/src/account_monitor/scanner.rs (4)
148-151: LGTM! Version tracking properly added.The AccountChangedEvent now correctly includes the account version from the substate, enabling event consumers to track specific account state versions.
459-519: LGTM! Version tracking integrated correctly.The changes properly:
- Extract version from account substates (line 467)
- Thread version through the processing logic (line 481)
- Store account address and version tuples for later notification (line 517)
550-587: LGTM! Vault-associated account version tracking is correct.The logic properly:
- Checks if the account is known (line 550-557)
- Retrieves the account substate version (lines 560-564)
- Falls back to version 0 if account not found (line 564)
- Includes version in updated_accounts tracking (line 587)
The fallback to version 0 is reasonable for edge cases where the account might not be in the database yet.
615-628: LGTM! Event notification properly includes version.The notification loop correctly:
- Iterates over updated accounts with versions
- Constructs AccountChangedEvent with both address and version
- Ensures downstream consumers receive complete state information
crates/wallet/sdk/src/models/event.rs (6)
19-19: LGTM! AuthLoginRequest variant now consistent with other events.The variant now properly holds an AuthLoginRequestEvent payload, making it consistent with other WalletEvent variants and enabling proper serialization and handling.
26-30: LGTM! Event type discrimination and display are well-implemented.The additions provide:
- Type-level event discrimination via
as_event_type()(lines 26-30)- Clean display formatting by delegating to WalletEventType (lines 92-96)
This is useful for logging, storage, and event filtering.
Also applies to: 92-96
98-157: LGTM! Serialization support enables event persistence.All event structs now derive
serde::Serialize, which is essential for:
- Persisting events to the wallet_events table
- Transmitting events over APIs
- Logging and debugging
The serialization is consistently applied across all event types.
119-123: LGTM! Version field adds important context.The
versionfield in AccountChangedEvent enables consumers to:
- Track specific account state versions
- Correlate events with blockchain state
- Detect and handle stale events
This aligns with the scanner changes that track and propagate version information.
153-157: LGTM! Account address context improves event utility.Adding
account_addressto UtxoSpentEvent provides essential context about which account the spent UTXO belonged to, making the event more actionable for consumers.
159-206: LGTM! WalletEventType provides clean type-level event discrimination.The implementation is well-designed:
- Enum variants correspond 1:1 with WalletEvent variants (lines 159-171)
- Display implementation provides consistent string representations (lines 173-189)
- From implementation is exhaustive and correct (lines 191-206)
This enables filtering, storage, and logging without carrying event payloads.
Test Results (CI)490 tests +27 477 ✅ +14 1h 35m 25s ⏱️ + 44m 38s For more details on these failures, see this check. Results for commit 67083a9. ± Comparison against base commit f040d7c. |
67083a9 to
6dbea72
Compare
2 participants
Description
feat!(wallet): persist wallet events
Motivation and Context
Allows a history of wallet events to be reviewed
How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Improvements