feat(wallet)!: view only account SDK support - #1592
Conversation
|
Warning Rate limit exceeded@sdbondi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughReplace numeric key indices with KeyId/DerivedKeyIndex across CLI, daemon, SDK, storage, bindings, and web UI; introduce view-only and owner key separation, CipherSeedRestore/PasswordManager, Key/KeyId models, confidential outputs migration, UTXO scanner/monitor events, new wallet crypto encryption module, and many API/signature updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI
participant Daemon as Wallet Daemon
participant SDK as Wallet SDK
participant KM as KeyManager
participant Builder as Tx Builder
User->>CLI: submit manifest/tx (may include signing_key_id)
CLI->>Daemon: TransactionSubmit { signing_key_id }
Daemon->>SDK: submit request
alt signing_key_id provided
SDK->>KM: get_key_or_active(Some(signing_key_id))
else default account
SDK->>SDK: get_default_account()
SDK->>KM: get_key_or_active(Some(account.owner_key_id))
end
KM-->>SDK: Key { secret, public }
SDK->>Builder: add_signer(pub) / seal with secret
Builder-->>SDK: sealed tx
SDK-->>Daemon: submission result
Daemon-->>CLI: response
sequenceDiagram
autonumber
participant Monitor as AccountMonitor
participant Scanner as UtxoScanner
participant Round as UtxoScannerRound
participant Store as Wallet Store
participant Events as EventBus
Monitor->>Scanner: scan_and_enqueue_utxos(account, resource)
Scanner->>Round: UtxoScannerRound::new(account_addr, view_key, resource, notify)
Round->>Round: scan_for_utxo_updates()
Round-->>Scanner: num_found
alt num_found > 0
Scanner->>Store: enqueue(account_addr, found_utxos)
Scanner->>Events: emit UtxoRecoveryStarted/Recovered/Completed
Events-->>Monitor: notify
else
Scanner-->>Monitor: no findings
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
applications/tari_walletd/web_ui/src/services/api/hooks/useTransactions.tsx (1)
35-42: Bug: queryKey missing transaction id; causes cache collisionsInclude hash in the key to avoid stale data across different transactions.
Apply this diff:
- return useQuery({ - queryKey: ["transaction_details"], + return useQuery({ + queryKey: ["transaction_details", hash], queryFn: () => { return transactionsGet({ transaction_id: hash }); }, });applications/tari_wallet_cli/src/command/transaction.rs (1)
269-281: Dry-run: don’t wait; print finalize result; set signing_key_idDry-run returns a finalize result immediately. Waiting on the network is unnecessary. Also align signing_key_id with manifest path.
- let resp = client - .submit_transaction_dry_run(TransactionSubmitDryRunRequest { - transaction, - signing_key_id: None, - detect_inputs: common.detect_inputs.unwrap_or(true), - detect_inputs_use_unversioned: true, - proof_ids: vec![], - }) - .await?; - wait_transaction_result(resp.transaction_id, client).await?; + let resp = client + .submit_transaction_dry_run(TransactionSubmitDryRunRequest { + transaction, + signing_key_id: fee_account.owner_key_id, + detect_inputs: common.detect_inputs.unwrap_or(true), + detect_inputs_use_unversioned: true, + proof_ids: vec![], + }) + .await?; + summarize_finalize_result(&resp.result.finalize);applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (1)
89-108: Key selection never resolves the owning accountTwo issues combine here:
matchesTypeEnumonly checks object identity for nested payloads, so comparing{ Derived: { index: … } }against a fresh object always returnsfalse.selected_account(and the formatted account label) therefore staysundefined.- The select value path coerces a
bigint→ string → number (+e.target.value) and stores it asnumber, while the menu items expose a string. MUI compares by strict equality, so the current selection displays empty, and indexes ≥ 2^53 lose precision.Keep the value in its original
KeyId(or at minimum asbigint/string) and implement an explicit structural comparison on the variant payload.-const onClaimFeesKeyChange = (e: SelectChangeEvent<number>) => { +const onClaimFeesKeyChange = (e: SelectChangeEvent<string>) => { if (!dataKeysList) { return; } - const keyIndex = +e.target.value; - if (keyIndex === formState.keyIndex) { + const keyId = JSON.parse(e.target.value) as KeyId; + if (matchesTypeEnum(formState.keyId, keyId)) { return; } - const selected_account = dataAccountsList?.accounts.find((account: AccountInfo) => - matchesTypeEnum(account.account.owner_key_id, { Derived: { index: BigInt(keyIndex) } }), - ); + const selected_account = dataAccountsList?.accounts.find((account: AccountInfo) => + keyIdsEqual(account.account.owner_key_id, keyId), + );Add a
keyIdsEqualhelper performing a proper deep comparison on the variant payload, store the selectedKeyIdin state, and adjustSelect’svalue/MenuItembinding to useJSON.stringify(keyId)(or similar stable encoding) rather than lossy number coercions.Also applies to: 210-228
applications/tari_walletd/src/handlers/validator.rs (1)
181-191: Replace account public key with claim public key in add_signer- .add_signer(&account_key.to_public_key().to_byte_type(), &secret.key) + .add_signer(&claim_public_key.to_byte_type(), &secret.key)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
345-356: Fix view-only key metadata on change outputsWe’re tagging change outputs with the owner key ID in both
view_only_key_idandowner_key_id. Downstream lookups will therefore try to decrypt with an account-branch key instead of the actual view key, so the wallet will fail to recover its own change outputs. Please store the real view key ID instead.- view_only_key_id: account_key.key_id, + view_only_key_id: account.view_only_key_id(), owner_key_id: Some(account_key.key_id),crates/wallet/sdk/src/apis/config.rs (1)
49-58: Implement decryption before returning encrypted configs.
get_decryptedcurrently ignores theis_encryptedflag and the supplied key, handing callers whatever bytes were stored. The first time we persist a genuinely encrypted blob (e.g. via the new password manager) this path will feed ciphertext intoserdeand either fail hard or leak raw bytes to code that assumes cleartext. Please decrypt with the provided key (or, at minimum, surface a dedicated error) before returning the value.crates/wallet/sdk_services/src/account_recovery/service.rs (1)
192-223: Handle owner-key mismatch by marking the account view-only.After logging that the on-chain owner key is missing or different, we still register the account with
owner_key_id = KeyId::derived(...). That marks the account as spendable, so later flows (spends, signer selection) will try to use a key we know won’t work and end up failing noisily. Instead, detect the mismatch and passNoneforowner_key_idso the wallet treats the account as view-only. For example:- self.wallet_sdk.accounts_api().add_account( + let owner_key_matches = component.owner_key.is_some_and(|pk| pk == public_key); + self.wallet_sdk.accounts_api().add_account( Some(format!("recovered-account-{}", key.key_index).as_str()), &account_addr, - KeyId::derived(key.key_index), - KeyId::derived(key.key_index), + KeyId::derived(key.key_index), + owner_key_matches.then(|| KeyId::derived(key.key_index)), true,This keeps spends from being attempted with keys we already know are unusable.
crates/wallet/sdk/src/sdk.rs (1)
89-107: Don’t fail initialization whenRecoveryNeededisn’t seeded yet.For wallets created before this PR,
ConfigKey::RecoveryNeededdoesn’t exist. The new logic turns that into anInvariantError, so upgraded wallets will fail to start even though everything else is intact. Please treatNoneas a default (e.g.unwrap_or(false)), optionally backfill the key, and only error if the value is present but malformed.
🧹 Nitpick comments (26)
crates/wallet/storage_sqlite/src/models/stealth_output.rs (1)
32-34: Schema change: new KeyId fields.
Ensure Diesel schema and migrations set:
- view_only_key_id: NOT NULL TEXT containing JSON for KeyId
- owner_key_id: NULLABLE TEXT
Consider an index on (owner_account_id, resource_address, status, owner_key_id, is_burnt, is_frozen) to support writer/reader filters.crates/wallet/sdk/src/models/account.rs (1)
83-93: Delegating getters in AccountWithAddress.
LGTM. One caution: Account::owner_public_key returns stored field, but AccountWithAddress::owner_public_key derives from address (Line 95). Ensure these never diverge.crates/wallet/sdk/src/models/key.rs (3)
166-197: KeyType Display/FromStr.
OK. Consider accepting lowercase variants later if UX requires it.
199-215: KeyIdOrPublicKey: consider serde derives if used in APIs.
If this type appears in request/response models, derive Serialize/Deserialize.Apply:
- pub enum KeyIdOrPublicKey { + #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] + pub enum KeyIdOrPublicKey {
216-254: KeyId helpers: duplicate imported accessors.
imported_view_key_idandimported_owner_key_idreturn the same value. Consider a singleimported_id()to avoid confusion.Apply:
- pub fn imported_view_key_id(&self) -> Option<ImportedKeyId> { - match self { - Self::Imported { local_key_id } => Some(*local_key_id), - Self::Derived { .. } => None, - } - } - - pub fn imported_owner_key_id(&self) -> Option<ImportedKeyId> { - match self { - Self::Imported { local_key_id, .. } => Some(*local_key_id), - Self::Derived { .. } => None, - } - } + pub fn imported_id(&self) -> Option<ImportedKeyId> { + match self { + Self::Imported { local_key_id } => Some(*local_key_id), + Self::Derived { .. } => None, + } + }applications/tari_walletd/web_ui/src/services/api/hooks/useKeys.tsx (1)
51-53: Consider safe bigint-to-number conversion.The
setActivefunction now acceptsindexasbigintand converts it toNumberbefore callingkeysSetActive. While key indices are unlikely to exceedNumber.MAX_SAFE_INTEGER(2^53 - 1), consider adding a safeguard or validation to ensure the conversion doesn't lose precision, especially if the system might support very large key indices in the future.Apply this diff to add a safe conversion check:
export const useKeysSetActive = () => { const setActive = async (index: bigint) => { + if (index > Number.MAX_SAFE_INTEGER) { + throw new Error(`Key index ${index} exceeds maximum safe integer`); + } const result = await keysSetActive({ index: Number(index) }); return result; };applications/tari_walletd/web_ui/src/services/api/hooks/useTransactions.tsx (1)
44-51: Broaden transactions queryKey; status alone is insufficientIf req includes filters (account, pagination), cache will collide. Key the full request.
Apply one of:
- return useQuery({ - queryKey: ["transactions", req.status], + return useQuery({ + queryKey: ["transactions", req], queryFn: () => transactionsGetAll(req),or (stable JSON key):
- return useQuery({ - queryKey: ["transactions", req.status], + return useQuery({ + queryKey: ["transactions", JSON.stringify(req)], queryFn: () => transactionsGetAll(req),bindings/test/enumHelpers.test.ts (3)
7-15: Rename suite to match functionKeep suite name consistent with the function under test.
-describe("matchesEnum", () => { +describe("matchesTypeEnum", () => {
34-38: Test name contradicts expectationIt throws, not returns false. Rename for clarity.
- it("returns false if enum object has no keys", () => { + it("throws an error if enum object has no keys", () => {
40-47: Clarify null handling and cover null-null caseRename and assert the spec: both null → true; one null → false.
- it("matches enum with primitive number value", () => { - const enumObject = null; - const value = { B: 456 }; - expect(matchesTypeEnum(enumObject, value)).toBe(false); - const enumObject2 = { B: 456 }; - const value2 = null; - expect(matchesTypeEnum(enumObject2, value2)).toBe(false); - }); + it("handles null cases correctly", () => { + // both null -> true + expect(matchesTypeEnum(null, null)).toBe(true); + // one null -> false + expect(matchesTypeEnum(null, { B: 456 })).toBe(false); + expect(matchesTypeEnum({ B: 456 } as any, null)).toBe(false); + });Optional: add an object-valued variant test to lock in shallow equality semantics (by reference). I can provide a patch if desired.
applications/tari_wallet_cli/src/command/transaction.rs (1)
283-292: Use owner_key_id for signing in non-dry-run submitThis aligns with KeyId-based signing used in submit_manifest and avoids daemon-side ambiguity.
- let request = TransactionSubmitRequest { - transaction, - signing_key_id: None, + let request = TransactionSubmitRequest { + transaction, + signing_key_id: fee_account.owner_key_id, detect_inputs: common.detect_inputs.unwrap_or(true), detect_inputs_use_unversioned: true, proof_ids: vec![], };applications/tari_walletd/src/lib.rs (1)
78-80: Avoid borrowing SeedWords in CipherSeedRestore; clone before mappingIf FromSeedWords expects owned SeedWords, mapping from &SeedWords will fail or limit lifetimes. Clone to an owned value first.
- let needs_seed_recovery = - wallet_sdk.initialize_cipher_seed(seed_words.map(CipherSeedRestore::FromSeedWords).unwrap_or_default())?; + let needs_seed_recovery = wallet_sdk.initialize_cipher_seed( + seed_words + .cloned() + .map(CipherSeedRestore::FromSeedWords) + .unwrap_or_default(), + )?;applications/tari_wallet_cli/src/command/account.rs (2)
60-60: Consider renaming CLI arg for consistency.The CLI argument is named
key_idbut is passed to request fields namedkey_index(lines 114, 171, 182). This naming mismatch can cause confusion for maintainers.Consider renaming the CLI argument to match:
- pub key_id: Option<u64>, + pub key_index: Option<u64>,And update the alias:
- #[clap(long, short, alias = "key")] + #[clap(long, short, alias = "key", alias = "key-id")]This maintains backward compatibility via the alias while improving clarity.
86-86: Consider renaming CLI arg for consistency.Similar to the
CreateArgs, this argument name (key_id) doesn't match the request field name (key_index). See the comment on line 60 for details.bindings/src/types/wallet-daemon-client/KeysListResponse.ts (1)
2-3: Update consumer tuple types for RistrettoPublicKeyBytes
In your UI components, change the tuple annotation from[KeyId, string, boolean]to[KeyId, RistrettoPublicKeyBytes, boolean]so the public-key element reflects the new alias.crates/template_lib/src/prelude.rs (1)
41-41: ResourceType still re-exported; conflicts with summaryThe code re-exports ResourceType in the prelude, but the PR summary says it was removed/moved. Confirm intent. If you meant to drop it from prelude, remove this re-export and import from types where needed.
- ResourceType,Based on learnings
crates/wallet/crypto/src/hashers.rs (1)
11-11: New 32-byte hasher alias: add a helper or remove until usedProvide a public wallet_hasher32 helper mirroring the 64-bit one, or drop the alias if unused to avoid dead code.
pub type OotleWalletHasher32<M> = DomainSeparatedBorshHasher<M, Blake2b<U32>>; +pub fn wallet_hasher32(network: Network, label: &'static str) -> OotleWalletHasher32<OotleWalletHashDomain> { + OotleWalletHasher32::new_with_label(&format!("{}.n{}", label, network.as_byte())) +}crates/wallet/sdk/src/models/confidential_output.rs (1)
21-23: Clarify semantics of owner_key_id and ensure bindings alignAdd a brief doc comment that None means view-only (cannot spend), mirroring StealthOutputModel. If this struct is exported to TS, ensure ts-rs exports are updated so KeyId maps correctly for web/UI consumers.
applications/tari_wallet_cli/src/command/key.rs (1)
69-76: KeyId display and activation flow mismatch
- print_keys now shows KeyId but “Use { index }” still accepts u64; this won’t select Imported keys.
- Rename loop var from index to key_id for clarity, and ensure Display formatting for KeyId is human-friendly.
Consider adding a UseById path (or parsing “Derived:123” / “Imported:5”) and exposing a set_active_key_by_id API to support both variants.
applications/tari_walletd/src/handlers/nfts.rs (1)
239-244: Use a consistent accessor for owner_key_idYou mix field access (fee_payer_account.owner_key_id) and a method (source_account.account.owner_key_id()). Prefer one pattern across both to avoid confusion and potential compile issues.
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
121-147: Stale comment: no longer grouping by indexThe comment “Group by account key index” is outdated after the KeyId changes. Update to reflect grouping behavior or remove.
crates/wallet/storage_sqlite/src/schema.rs (2)
8-11: Define a canonical on-disk format for KeyId and add supporting indexes
- Text storage for KeyId is fine, but standardize encoding (e.g.,
derived:<index>/imported:<local_id>or JSON) to prevent ambiguity and simplify parsing.- Add indexes for common lookups:
- accounts(view_only_key_id), accounts(owner_key_id), accounts(owner_public_key)
- confidential_outputs(view_only_key_id), confidential_outputs(owner_key_id)
- stealth_outputs(view_only_key_id), stealth_outputs(owner_key_id)
- utxo_process_queue(account_key_id)
Add these in the corresponding migration to avoid full scans on sync/reorg paths.
Please confirm the migration adds these indexes or let me generate the SQL if needed.
Also applies to: 50-52, 158-160, 207-207
73-81: Imported keys table: consider constraints/metadata
- Add UNIQUE(label) if labels are intended to be user-unique.
- Consider adding updated_at and a NOT NULL constraint on key_type with a CHECK over allowed values.
These improve integrity and operability without behavioral changes.
crates/template_lib_types/src/resource_type.rs (1)
18-21: Solid addition; consider adding BorshDeserialize for symmetryIf borsh is used for decode in this crate, also derive BorshDeserialize.
-#[cfg_attr(feature = "borsh", derive(borsh::BorshSerialize))] +#[cfg_attr(feature = "borsh", derive(borsh::BorshSerialize, borsh::BorshDeserialize))]crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
706-708: Technical debt - XTR input addition.The TODO comment indicates that adding the XTR input is a workaround that should be removed in the future. Consider creating a tracking issue to address this technical debt and clarify why this input is currently necessary.
Do you want me to open an issue to track the removal of this workaround?
crates/wallet/sdk/src/cipher_seed.rs (1)
21-41: Add documentation for thread-safety semantics and lifetimes.The
WalletCipherSeedenum wrapsCipherSeedin anArcfor thread-safe sharing, andcipher_seed()returns a reference with a lifetime tied to theArc. While the implementation is correct, adding doc comments would help users understand:
- The thread-safety guarantees provided by
Arc- The lifetime constraints of the returned reference from
cipher_seed()- When to use
NonevsCipherSeedvariantsConsider adding documentation:
+/// Wrapper for optional cipher seed with thread-safe sharing. +/// +/// The `CipherSeed` variant wraps the seed in an `Arc` for safe sharing across threads. +/// The `None` variant represents an uninitialized or unavailable seed state. #[derive(Debug, Clone, Default)] pub enum WalletCipherSeed { #[default] None, CipherSeed(Arc<CipherSeed>), } impl WalletCipherSeed { + /// Returns a reference to the cipher seed if available. + /// + /// The returned reference has a lifetime tied to the `Arc`, ensuring safety. + /// Returns `None` if the seed is not initialized. pub fn cipher_seed(&self) -> Option<&CipherSeed> {
📜 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 (107)
applications/tari_wallet_cli/src/command/account.rs(3 hunks)applications/tari_wallet_cli/src/command/key.rs(2 hunks)applications/tari_wallet_cli/src/command/transaction.rs(4 hunks)applications/tari_wallet_cli/src/command/validator.rs(2 hunks)applications/tari_walletd/src/handlers/accounts.rs(13 hunks)applications/tari_walletd/src/handlers/confidential.rs(4 hunks)applications/tari_walletd/src/handlers/helpers.rs(2 hunks)applications/tari_walletd/src/handlers/keys.rs(2 hunks)applications/tari_walletd/src/handlers/mod.rs(0 hunks)applications/tari_walletd/src/handlers/nfts.rs(5 hunks)applications/tari_walletd/src/handlers/settings.rs(1 hunks)applications/tari_walletd/src/handlers/transaction.rs(11 hunks)applications/tari_walletd/src/handlers/validator.rs(4 hunks)applications/tari_walletd/src/lib.rs(2 hunks)applications/tari_walletd/src/main.rs(5 hunks)applications/tari_walletd/src/services/mod.rs(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx(5 hunks)applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Accounts.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useKeys.tsx(1 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useTransactions.tsx(1 hunks)bindings/src/helpers/enum.ts(1 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/Account.ts(1 hunks)bindings/src/types/ResourceType.ts(0 hunks)bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsCreateOrGetRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsCreateRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeysListResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitDryRunRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitManifestRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts(1 hunks)bindings/src/wallet-daemon-client.ts(2 hunks)bindings/test/enumHelpers.test.ts(1 hunks)clients/wallet_daemon_client/Cargo.toml(1 hunks)clients/wallet_daemon_client/src/types.rs(9 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/shenanigans.rs(1 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/proof.rs(1 hunks)crates/engine_types/src/resource.rs(1 hunks)crates/engine_types/src/resource_container.rs(1 hunks)crates/engine_types/src/vault.rs(1 hunks)crates/ootle_address/src/ootle_address.rs(1 hunks)crates/template_lib/src/args/types.rs(1 hunks)crates/template_lib/src/models/bucket.rs(1 hunks)crates/template_lib/src/models/encrypted_data.rs(2 hunks)crates/template_lib/src/models/proof.rs(1 hunks)crates/template_lib/src/models/vault.rs(1 hunks)crates/template_lib/src/prelude.rs(2 hunks)crates/template_lib/src/resource/builder/confidential.rs(1 hunks)crates/template_lib/src/resource/builder/fungible.rs(1 hunks)crates/template_lib/src/resource/builder/non_fungible.rs(1 hunks)crates/template_lib/src/resource/builder/stealth.rs(1 hunks)crates/template_lib/src/resource/mod.rs(1 hunks)crates/template_lib_types/src/lib.rs(1 hunks)crates/template_lib_types/src/resource_type.rs(1 hunks)crates/wallet/crypto/Cargo.toml(1 hunks)crates/wallet/crypto/src/encryption.rs(1 hunks)crates/wallet/crypto/src/hashers.rs(1 hunks)crates/wallet/crypto/src/lib.rs(1 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(8 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(12 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(13 hunks)crates/wallet/sdk/src/apis/config.rs(4 hunks)crates/wallet/sdk/src/apis/key_manager.rs(5 hunks)crates/wallet/sdk/src/apis/mod.rs(1 hunks)crates/wallet/sdk/src/apis/password_manager.rs(1 hunks)crates/wallet/sdk/src/apis/resources.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(8 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(4 hunks)crates/wallet/sdk/src/apis/transaction.rs(2 hunks)crates/wallet/sdk/src/cipher_seed.rs(1 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/models/account.rs(3 hunks)crates/wallet/sdk/src/models/confidential_output.rs(2 hunks)crates/wallet/sdk/src/models/key.rs(1 hunks)crates/wallet/sdk/src/models/stealth_output.rs(2 hunks)crates/wallet/sdk/src/models/vault.rs(1 hunks)crates/wallet/sdk/src/sdk.rs(9 hunks)crates/wallet/sdk/src/storage.rs(9 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(6 hunks)crates/wallet/sdk/tests/support/harness.rs(3 hunks)crates/wallet/sdk_services/Cargo.toml(0 hunks)crates/wallet/sdk_services/src/account_monitor.rs(2 hunks)crates/wallet/sdk_services/src/account_recovery/service.rs(7 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(5 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(5 hunks)crates/wallet/storage_sqlite/src/models/account.rs(2 hunks)crates/wallet/storage_sqlite/src/models/confidential_output.rs(3 hunks)crates/wallet/storage_sqlite/src/models/mod.rs(2 hunks)crates/wallet/storage_sqlite/src/models/resource.rs(2 hunks)crates/wallet/storage_sqlite/src/models/stealth_output.rs(3 hunks)crates/wallet/storage_sqlite/src/models/transaction.rs(1 hunks)crates/wallet/storage_sqlite/src/models/utxo_process_queue.rs(1 hunks)crates/wallet/storage_sqlite/src/models/vault.rs(1 hunks)crates/wallet/storage_sqlite/src/reader.rs(9 hunks)crates/wallet/storage_sqlite/src/schema.rs(6 hunks)crates/wallet/storage_sqlite/src/serialization.rs(1 hunks)
⛔ Files not processed due to max files limit (7)
- crates/wallet/storage_sqlite/src/writer.rs
- crates/wallet/storage_sqlite/tests/accounts.rs
- integration_tests/src/wallet_daemon_client.rs
- integration_tests/tests/steps/wallet_daemon.rs
- utilities/tariswap_test_bench/src/accounts.rs
- utilities/tariswap_test_bench/src/runner.rs
- utilities/tariswap_test_bench/src/tariswap.rs
💤 Files with no reviewable changes (3)
- applications/tari_walletd/src/handlers/mod.rs
- crates/wallet/sdk_services/Cargo.toml
- bindings/src/types/ResourceType.ts
🧰 Additional context used
🧬 Code graph analysis (68)
applications/tari_walletd/src/handlers/confidential.rs (1)
applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_request(189-194)
applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx (1)
crates/wallet/sdk/src/models/account.rs (1)
account(71-73)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (2)
bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts (2)
bindings/src/types/UnsignedTransaction.ts (1)
UnsignedTransaction(4-4)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/template_lib/src/prelude.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
bindings/src/types/wallet-daemon-client/TransactionSubmitManifestRequest.ts (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_walletd/src/services/mod.rs (1)
applications/tari_walletd/src/handlers/mod.rs (1)
wasm_optimizer(20-20)
crates/template_lib/src/resource/builder/fungible.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/engine_types/src/resource.rs (3)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts (2)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)
applications/tari_wallet_cli/src/command/key.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/template_lib/src/args/types.rs (2)
bindings/src/types/StealthValueProof.ts (1)
StealthValueProof(9-15)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/template_lib/src/resource/builder/non_fungible.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/models/confidential_output.rs (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/engine_types/src/vault.rs (3)
bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
bindings/src/types/Account.ts (3)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/engine_types/src/resource_container.rs (3)
bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
bindings/src/types/wallet-daemon-client/KeysListResponse.ts (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/wallet/sdk/src/apis/accounts.rs (3)
crates/ootle_address/src/ootle_address.rs (5)
network(40-42)network(224-226)account_public_key(48-50)view_only_key(44-46)view_only_key(228-230)crates/wallet/sdk/src/models/key.rs (8)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)key_id(22-24)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(91-103)
crates/template_lib/src/models/bucket.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/apis/resources.rs (2)
bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/models/vault.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (3)
bindings/src/types/wallet-daemon-client/AccountInfo.ts (1)
AccountInfo(5-5)bindings/src/helpers/enum.ts (1)
matchesTypeEnum(4-35)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Accounts.tsx (1)
crates/wallet/sdk/src/models/account.rs (1)
account(71-73)
crates/template_lib/src/resource/builder/confidential.rs (2)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/models/stealth_output.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/OutputStatus.ts (1)
OutputStatus(3-3)
crates/wallet/sdk/src/apis/confidential_transfer.rs (2)
crates/wallet/sdk/src/sdk.rs (2)
confidential_outputs_api(183-185)key_manager_api(144-152)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
key_manager_api(355-371)
bindings/src/types/wallet-daemon-client/TransactionSubmitDryRunRequest.ts (2)
bindings/src/types/UnsignedTransaction.ts (1)
UnsignedTransaction(4-4)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/engine_types/src/bucket.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/lib.rs (1)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)
crates/wallet/sdk/src/models/account.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/template_lib_types/src/resource_type.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/tests/support/harness.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)
crates/wallet/sdk/src/apis/stealth_transfer.rs (2)
crates/wallet/sdk/src/models/account.rs (3)
owner_key_id(33-35)owner_key_id(91-93)account(71-73)crates/template_lib/src/models/stealth.rs (1)
revealed_input_amount(82-84)
crates/wallet/sdk/tests/confidential_output_api.rs (2)
crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)crates/wallet/sdk/tests/support/harness.rs (2)
test_account_address(79-83)test_vault_address(85-89)
applications/tari_walletd/src/handlers/validator.rs (7)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/sdk/src/models/account.rs (1)
account(71-73)applications/tari_walletd/src/handlers/helpers.rs (1)
get_account_or_default(140-157)crates/wallet/sdk/src/apis/accounts.rs (1)
get_account_or_default(291-300)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (2)
key_id(22-24)secret(91-93)
applications/tari_walletd/src/handlers/keys.rs (1)
bindings/src/types/wallet-daemon-client/KeysListResponse.ts (1)
KeysListResponse(5-10)
crates/wallet/sdk/src/apis/stealth_outputs.rs (5)
bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/sdk/src/models/account.rs (4)
owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/models/key.rs (1)
key_id(22-24)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)
crates/template_lib/src/resource/builder/stealth.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/template_lib/src/models/vault.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk_services/src/account_recovery/service.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)
crates/wallet/storage_sqlite/src/models/account.rs (2)
crates/wallet/storage_sqlite/src/reader.rs (15)
accounts(381-383)accounts(403-406)accounts(419-421)accounts(436-438)accounts(447-449)accounts(478-480)accounts(499-502)accounts(529-532)accounts(564-567)accounts(598-601)accounts(729-732)accounts(776-779)accounts(799-802)accounts(903-906)accounts(940-943)bindings/src/types/Account.ts (1)
Account(6-14)
crates/engine_types/src/proof.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
applications/tari_walletd/src/handlers/helpers.rs (3)
crates/wallet/sdk/src/models/account.rs (2)
address(25-27)address(79-81)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)bindings/src/helpers/consts.ts (1)
ACCOUNT_TEMPLATE_ADDRESS(6-6)
crates/wallet/storage_sqlite/src/models/resource.rs (1)
crates/wallet/storage_sqlite/src/models/transaction.rs (1)
deserialize_json(46-46)
applications/tari_wallet_cli/src/command/validator.rs (2)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts (1)
GetValidatorFeesRequest(5-5)
crates/wallet/storage_sqlite/src/models/vault.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
applications/tari_walletd/src/handlers/nfts.rs (3)
crates/wallet/sdk/src/models/account.rs (1)
account(71-73)applications/tari_walletd/src/handlers/helpers.rs (2)
invalid_params(159-170)get_account_with_inputs(96-110)crates/wallet/sdk/src/sdk.rs (1)
key_manager_api(144-152)
crates/template_lib/src/models/proof.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
clients/wallet_daemon_client/src/types.rs (6)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)
crates/wallet/storage_sqlite/src/reader.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/storage.rs (5)
key_manager_get_raw_imported_key(155-155)confidential_outputs_get_unspent_balance(212-212)confidential_outputs_get_locked_by_lock_id(213-216)confidential_outputs_get_by_commitment(217-221)confidential_outputs_get_by_account_and_status(223-227)
crates/wallet/sdk/src/storage.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/storage_sqlite/src/reader.rs (6)
key_manager_get_raw_imported_key(164-193)confidential_outputs_get_unspent_balance(673-696)confidential_outputs_get_locked_by_lock_id(698-748)confidential_outputs_get_by_commitment(750-790)confidential_outputs_get_by_account_and_status(792-830)key_type(184-185)crates/wallet/storage_sqlite/src/writer.rs (6)
key_manager_insert_imported_key(280-302)confidential_outputs_lock_smallest_amount(854-940)confidential_outputs_insert(942-975)confidential_outputs_finalize_by_lock_id(977-1005)confidential_outputs_release_by_lock_id(1007-1030)utxo_process_queue_extend(1468-1490)crates/wallet/sdk/src/models/account.rs (6)
view_only_key_id(29-31)view_only_key_id(87-89)owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)
crates/wallet/sdk/src/models/key.rs (4)
crates/ootle_address/src/ootle_address.rs (2)
fmt(137-139)from_str(145-147)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/transaction/src/v1/signature.rs (2)
public_key(63-65)public_key(122-124)
bindings/test/enumHelpers.test.ts (1)
bindings/src/helpers/enum.ts (1)
matchesTypeEnum(4-35)
crates/wallet/storage_sqlite/src/schema.rs (3)
crates/wallet/sdk/src/models/account.rs (6)
owner_public_key(37-39)owner_public_key(95-97)view_only_key_id(29-31)view_only_key_id(87-89)owner_key_id(33-35)owner_key_id(91-93)crates/engine_types/src/utxo.rs (1)
owner_public_key(42-44)crates/wallet/storage_sqlite/src/reader.rs (2)
key_manager_imported_keys(168-174)key_type(184-185)
crates/engine/tests/shenanigans.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/apis/password_manager.rs (1)
crates/wallet/sdk/src/sdk.rs (1)
sdk_config(127-129)
crates/wallet/sdk/src/apis/key_manager.rs (5)
crates/wallet/crypto/src/encryption.rs (2)
decrypt_with_password(46-114)encrypt_with_password(116-150)crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/models/key.rs (14)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)derived(226-228)secret_key(158-160)imported(230-232)key_id(22-24)key_index(150-152)public_key(30-32)public_key(154-156)
crates/wallet/storage_sqlite/src/models/stealth_output.rs (3)
crates/wallet/storage_sqlite/src/reader.rs (4)
stealth_outputs(839-846)stealth_outputs(869-879)stealth_outputs(892-894)stealth_outputs(928-931)crates/wallet/storage_sqlite/src/writer.rs (9)
stealth_outputs(1049-1065)stealth_outputs(1134-1134)stealth_outputs(1135-1135)stealth_outputs(1164-1164)stealth_outputs(1165-1165)stealth_outputs(1177-1177)stealth_outputs(1178-1178)stealth_outputs(1197-1197)stealth_outputs(1198-1198)crates/wallet/storage_sqlite/src/models/transaction.rs (1)
deserialize_json(46-46)
applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (2)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)applications/tari_walletd/web_ui/src/utils/json_rpc.ts (1)
validatorsGetFees(322-323)
crates/wallet/sdk/src/sdk.rs (3)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/apis/accounts.rs (1)
new(57-69)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(91-103)
applications/tari_walletd/src/main.rs (4)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/models/account.rs (6)
name(41-43)name(83-85)address(25-27)address(79-81)view_only_key_id(29-31)view_only_key_id(87-89)bindings/src/types/Account.ts (1)
Account(6-14)
applications/tari_walletd/src/handlers/transaction.rs (5)
applications/tari_walletd/src/handlers/helpers.rs (6)
get_account(112-123)get_account_or_default(140-157)invalid_params(159-170)invalid_request(189-194)not_found(180-187)transaction_rejected(196-201)crates/wallet/sdk/src/apis/accounts.rs (1)
get_account_or_default(291-300)applications/tari_walletd/src/services/wasm_optimizer.rs (1)
optimize_wasm_template(35-54)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/Account.ts (1)
Account(6-14)
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (5)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/UtxoUnspent.ts (1)
UtxoUnspent(5-5)bindings/src/types/WalletUtxoUpdate.ts (1)
WalletUtxoUpdate(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/crypto/src/kdfs.rs (1)
encrypted_data_dh_kdf_aead(32-44)
applications/tari_walletd/src/handlers/accounts.rs (1)
applications/tari_walletd/src/handlers/helpers.rs (1)
get_account_by_key_index(125-138)
⏰ 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 nightly
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: machete
Test Results (CI)454 tests +33 453 ✅ +32 1h 32m 31s ⏱️ + 44m 32s For more details on these failures, see this check. Results for commit f8d6ef7. ± Comparison against base commit 7458abd. ♻️ This comment has been updated with latest results. |
1d00c21 to
1f036b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (10)
crates/wallet/crypto/src/encryption.rs (1)
25-26: Use Argon2id for password KDF.As flagged in a previous review, the implementation uses Argon2d which is vulnerable to side-channel attacks in password-based scenarios. OWASP recommends Argon2id for password storage.
Apply this diff:
- argon2::Argon2::new(argon2::Algorithm::Argon2d, argon2::Version::V0x13, params) + argon2::Argon2::new(argon2::Algorithm::Argon2id, argon2::Version::V0x13, params)crates/wallet/sdk/src/apis/config.rs (1)
70-88:set_encryptedstill persists plaintextThis method flips
is_encrypted = trueyet writes the raw payload and ignores the provided key, so secrets end up in cleartext while callers think they are encrypted. That’s the same security regression flagged earlier—please encrypt before storing or fail fast until encryption is implemented.pub fn set_encrypted<T: Serialize + ?Sized>( &self, key: ConfigKey, value: &T, _encryption_key: impl AsRef<[u8]>, ) -> Result<(), ConfigApiError> { - // TODO: encrypt - self.set_opts(key, value, true) + // TODO: encrypt + Err(ConfigApiError::EncryptedItem { key }) }applications/tari_walletd/src/handlers/nfts.rs (1)
295-309: Critical: mismatched signer public/secret keys.The
add_signercall pairs the fee payer's public key with the source account's secret key. The public/secret must belong to the same key. Use the source owner's public key when signing with the source owner's secret:Apply this diff to fix the mismatched key pair:
.add_signer( - &fee_owner_key.to_public_key().to_byte_type(), + &source_account_secret_key.to_public_key().to_byte_type(), &source_account_secret_key.secret, ) .build_and_seal(&fee_owner_key.secret);The
build_and_sealcall correctly uses the fee payer's secret key, so that line should remain unchanged.applications/tari_walletd/src/main.rs (1)
110-117: Misleading JSON field name for KeyId type.The JSON field
key_indexcontains aKeyIdobject ({ Derived: { index: bigint } }or{ Imported: { local_key_id: bigint } }), not a simple numeric index. This is a breaking change for consumers expecting a number, and the field name is misleading.Apply this diff to rename the field to accurately reflect the KeyId type:
let json = json!({ "component_address": account_addr, "address": account_address.address.to_byte_type(), "account_public_key": public_key, "view_only_public_key": view_only_public_key, "view_only_private_key": hex::encode(view_only_secret.secret().as_bytes()), - "key_index": account_address.view_only_key_id, + "view_only_key_id": account_address.view_only_key_id, });Alternatively, if backward compatibility is required, emit both fields:
let json = json!({ "component_address": account_addr, "address": account_address.address.to_byte_type(), "account_public_key": public_key, "view_only_public_key": view_only_public_key, "view_only_private_key": hex::encode(view_only_secret.secret().as_bytes()), - "key_index": account_address.view_only_key_id, + "view_only_key_id": account_address.view_only_key_id, + "key_index": account_address.view_only_key_id, // deprecated, use view_only_key_id });crates/wallet/storage_sqlite/src/models/confidential_output.rs (1)
61-75: Still unwrapping hex decode errors
unwrap()will panic on corrupt DB hex, resurrecting the crash previously flagged. Convert the decode failures intoWalletStorageErrorlike the other fields instead of panicking.- sender_public_nonce: self - .sender_public_nonce - .map(|nonce| RistrettoPublicKeyBytes::from_hex(&nonce).unwrap()), + sender_public_nonce: self + .sender_public_nonce + .map(|nonce| { + RistrettoPublicKeyBytes::from_hex(&nonce).map_err(|_| WalletStorageError::DecodingError { + operation: "try_into_output", + item: "sender_public_nonce", + details: "Corrupt db: invalid hex".to_string(), + }) + }) + .transpose()?, @@ - public_asset_tag: self - .public_asset_tag - .map(|tag| RistrettoPublicKeyBytes::from_hex(&tag).unwrap()), + public_asset_tag: self + .public_asset_tag + .map(|tag| { + RistrettoPublicKeyBytes::from_hex(&tag).map_err(|_| WalletStorageError::DecodingError { + operation: "try_into_output", + item: "public_asset_tag", + details: "Corrupt db: invalid hex".to_string(), + }) + }) + .transpose()?,bindings/src/helpers/enum.ts (1)
21-34: Variant match still relies on payload equalityWe still compare the payload objects by reference, so
{ Derived: { index: 1n } }never matches an equivalent value. Just checking for the discriminant key is enough.- if (!(key in value)) { - return false; - } - - // check the value - const enumValue = (enumObject as any)[key]; - const valueValue = (value as any)[key]; - - // Check for primitive types - if (typeof enumValue === "string" || typeof enumValue === "number" || typeof enumValue === "boolean") { - return typeof enumValue === typeof valueValue; - } - - // Check for object types (shallow check) - if (typeof enumValue === "object" && enumValue !== null) { - return enumValue === valueValue; - } - return false; + return Object.prototype.hasOwnProperty.call(value, key);applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (1)
253-256: Crash when rendering imported keys
extractKeyIndex(account[0])returnsnullfor Imported/view-only keys, so the non-null assertion feedsnull.toString()and the dialog blows up as soon as those keys appear (the new view-only flow). Guard the null and avoid using it as the value.- {dataKeysList?.keys.map((account: [KeyId, string, boolean], i) => ( - <MenuItem key={i} value={extractKeyIndex(account[0])!.toString()}> - {formatKey(account)} - </MenuItem> - ))} + {dataKeysList?.keys.map(([keyId, publicKey, isActive], i) => { + const derivedIndex = extractKeyIndex(keyId); + if (derivedIndex === null) { + return ( + <MenuItem key={i} value={displayKeyId(keyId)} disabled> + {formatKey([keyId, publicKey, isActive])} + </MenuItem> + ); + } + return ( + <MenuItem key={i} value={derivedIndex.toString()}> + {formatKey([keyId, publicKey, isActive])} + </MenuItem> + ); + })}Remember to expose
displayKeyIdoutsideformatKey(or lift it) so it’s available here.applications/tari_walletd/src/handlers/transaction.rs (2)
89-89: Handle missing owner key in fee account.
fee_account.owner_key_id()returnsOption<KeyId>, but is passed directly tosigning_key_id. If the fee account is view-only (no owner key), this will fail. Wrap in.ok_or_else(...)to return a clear error.Based on past review comments.
Apply this pattern:
- signing_key_id: fee_account.owner_key_id(), + signing_key_id: fee_account.owner_key_id().ok_or_else(|| { + invalid_params("fee_account", Some("Fee account must have an owner key")) + })?,
541-541: Handle missing owner key in fee account (template publish path).
fee_account.owner_key_id()returnsOption<KeyId>, but is passed directly. Wrap in.ok_or_else(...)to return a clear error if the fee account is view-only.Based on past review comments.
Apply this pattern:
- signing_key_id: fee_account.owner_key_id(), + signing_key_id: fee_account.owner_key_id().ok_or_else(|| { + invalid_params("fee_account", Some("Fee account must have an owner key")) + })?,Also applies to: 562-562
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
181-184: Release stealth locks via stealth table (not confidential)This calls the confidential outputs release; stealth locks remain stuck.
Apply this diff:
- tx.confidential_outputs_release_by_lock_id(lock_id)?; + tx.stealth_outputs_release_by_lock_id(lock_id)?;
🧹 Nitpick comments (13)
crates/wallet/sdk/src/apis/resources.rs (1)
34-38: Consider implementing a dedicated storage-layer exists check.The current implementation fetches the entire resource to determine existence, which is inefficient for large resources. The TODO comment correctly identifies this performance concern.
Recommended approach:
Add a lightweight
resources_exists(address)method at the storage layer that performs an existence check without loading the full resource data. This would be more efficient, especially for large resources.Would you like me to open an issue to track this optimization, or would you prefer to implement the optimized version in this PR?
crates/wallet/sdk_services/src/account_monitor.rs (2)
96-104: Consider adding an enable method for API symmetry.The builder API only provides
disable_periodic_scanning_with_utxos(). Consider addingenable_periodic_scanning_with_utxos()for symmetry, even though the default is enabled. This would make the API more discoverable and allow re-enabling after disabling.Apply this diff to add the enable method:
+ pub fn enable_periodic_scanning_with_utxos(mut self) -> Self { + self.enable_periodic_scanning_with_utxos = true; + self + } + pub fn disable_periodic_scanning_with_utxos(mut self) -> Self { self.enable_periodic_scanning_with_utxos = false; self }
911-937: Consider extracting common logic to reduce duplication.The two methods
refresh_accountandrefresh_account_with_utxosare nearly identical, differing only in thescan_for_utxosflag. Consider extracting common logic into a private helper method.Apply this diff to reduce duplication:
/// Triggers an immediate refresh of the specified account. Returns `true` if the account was updated, otherwise /// `false`. pub async fn refresh_account(&self, account: ComponentAddress) -> Result<bool, AccountMonitorError> { + self.refresh_account_internal(account, false).await + } + + pub async fn refresh_account_with_utxos(&self, account: ComponentAddress) -> Result<bool, AccountMonitorError> { + self.refresh_account_internal(account, true).await + } + + async fn refresh_account_internal( + &self, + account: ComponentAddress, + scan_for_utxos: bool, + ) -> Result<bool, AccountMonitorError> { let (reply_tx, reply_rx) = oneshot::channel(); self.sender .send(AccountMonitorRequest::RefreshAccount { account, - scan_for_utxos: false, + scan_for_utxos, reply: reply_tx, }) .await .map_err(|_| AccountMonitorError::ServiceShutdown)?; reply_rx.await.map_err(|_| AccountMonitorError::ServiceShutdown)? } - - pub async fn refresh_account_with_utxos(&self, account: ComponentAddress) -> Result<bool, AccountMonitorError> { - let (reply_tx, reply_rx) = oneshot::channel(); - self.sender - .send(AccountMonitorRequest::RefreshAccount { - account, - scan_for_utxos: true, - reply: reply_tx, - }) - .await - .map_err(|_| AccountMonitorError::ServiceShutdown)?; - reply_rx.await.map_err(|_| AccountMonitorError::ServiceShutdown)? - }crates/template_lib_types/src/amount/amount.rs (3)
317-350: LGTM with a note on the TODO and a suggested refactor.The decimal formatting logic is correct and handles edge cases well (negative values, zero integer part, zero padding). However, there's a performance concern noted at line 343.
Suggested refactor to eliminate the temporary string allocation:
Currently,
fractional_partis converted to a string at line 343 just to calculate its length for zero-padding, then formatted again at line 349. You can avoid this by:
- Calculating the number of digits in
fractional_partusing logarithm or repeated division, OR- Formatting
fractional_partonce into the output buffer with manual zero-paddingHere's an example of approach #2:
- // TODO: calculate the decimal string without allocating a string first - let fractional_str = fractional_part.to_string(); - let mut padding_needed = decimals as usize - fractional_str.len(); + // Calculate number of digits in fractional_part + let num_digits = if fractional_part.is_zero() { + 1 + } else { + let mut temp = fractional_part; + let mut count = 0; + while !temp.is_zero() { + temp = temp / ten; + count += 1; + } + count + }; + let mut padding_needed = decimals as usize - num_digits; while padding_needed > 0 { write!(f, "0")?; padding_needed -= 1; } write!(f, "{}", fractional_part)This eliminates the temporary
fractional_strallocation and resolves the TODO.
618-641: Good test coverage, consider adding edge case tests.The test suite covers the main scenarios well (positive, negative, zero padding, trailing zeros). Consider adding tests for:
Amount::ZEROwith various decimal precisionsAmount::MAXandAmount::MINwith decimals to ensure no overflow/underflow in formatting- Very large
decimalsvalues (e.g., decimals > 20) to test behavior at precision boundariesExample additional tests:
#[test] fn fmt_decimals_edge_cases() { // Test zero let zero = Amount::ZERO; assert_eq!(zero.to_decimal_string(0), "0"); assert_eq!(zero.to_decimal_string(6), "0.000000"); // Test with very large decimals let a = Amount::from(1); assert_eq!(a.to_decimal_string(20), "0.00000000000000000001"); // Test MAX/MIN don't panic let max = Amount::MAX; let _ = max.to_decimal_string(6); // Should not panic let min = Amount::MIN; let _ = min.to_decimal_string(6); // Should not panic }
6-6: Remove unused fmt::Debug import
Line 6: dropfmt::Debugfrom theuse tari_template_abi::rust::{…}import—it's only referenced by the derive and isn’t used elsewhere.crates/wallet/crypto/src/encryption.rs (1)
221-301: Good test coverage of error paths.The tests validate key failure modes:
- Checksum mismatch (lines 236-251)
- Invalid version (lines 253-267)
- Invalid length (lines 269-283)
- Corrupted payload (lines 285-300)
Consider adding tests for:
- Wrong password (MAC verification failure)
- Empty plaintext edge case
- MAC-specific corruption (vs checksum)
- Nonce uniqueness (different salts → different ciphertexts for same plaintext)
crates/wallet/crypto/Cargo.toml (1)
19-26: Update Argon2 to the latest stable releaseArgon2 0.5.3 is older than the current non-prerelease version on crates.io; consider upgrading. crc32fast 1.5.0 and subtle 2.6.1 are up-to-date with no known vulnerabilities.
applications/tari_walletd/web_ui/src/services/api/hooks/useKeys.tsx (1)
51-52: Guard against unsafe bigint→number conversionCasting an arbitrary
biginttoNumberwill silently truncate once the derived index exceeds 2^53‑1. That undermines the whole point of plumingbigintthrough the UI and can make us activate the wrong key. Add an explicit max-safe check (or switch to a string payload) before callingkeysSetActive.const setActive = async (index: bigint) => { - const result = await keysSetActive({ index: Number(index) }); + const maxSafe = BigInt(Number.MAX_SAFE_INTEGER); + if (index > maxSafe) { + throw new RangeError(`Key index ${index} exceeds supported range`); + } + const result = await keysSetActive({ index: Number(index) }); return result; };bindings/test/enumHelpers.test.ts (1)
40-47: Clarify the null handling test logic.Lines 40-47 test null handling but the logic is inconsistent with the test name "matches enum with primitive number value". The test checks that
matchesTypeEnum(null, { B: 456 })returns false andmatchesTypeEnum({ B: 456 }, null)returns false, which is correct behavior, but the test name suggests it should be testing primitive number matching.Consider renaming the test to better reflect what it's testing:
- it("matches enum with primitive number value", () => { + it("returns false when one argument is null and the other is not", () => { const enumObject = null; const value = { B: 456 }; expect(matchesTypeEnum(enumObject, value)).toBe(false); const enumObject2 = { B: 456 }; const value2 = null; expect(matchesTypeEnum(enumObject2, value2)).toBe(false); });crates/wallet/sdk/src/apis/password_manager.rs (2)
83-97: Simplify error handling:Entry::newwon't returnNoEntry
keyring::Entry::newwon't yieldError::NoEntry; that is returned byget_password/set_password. TheNoEntryarm here is unreachable and obscures intent.Apply this diff:
- let result = keyring::Entry::new(KEYRING_ENTRIES_SERVICE, key); - - match result { - Ok(entry) => Ok(entry), - Err(keyring::Error::NoEntry) => { - // NoEntry maps to various errors in the keyring codebase, including AccessDenied, keyExpired etc. - // Entry::new says that it will only return an error if the service/user are invalid but there may be - // more errors possible e.g. AccessDenied. In any case we provide a better error than NoEntry for this - // case. We dont want IsNotFoundError to be true for this case. - Err(PasswordManagerApiError::FailedToAccessKeyRing) - }, - Err(err) => Err(err.into()), - } + match keyring::Entry::new(KEYRING_ENTRIES_SERVICE, key) { + Ok(entry) => Ok(entry), + // Entry::new fails for invalid service/user or unsupported backend + Err(err) => Err(err.into()), + }
69-72: Stabilize keyring entry key formattingUse the canonical network key string to avoid accidental formatting changes.
- let key = format!("{}-{}-{}", CIPHER_SEED_PASSWORD_KEYRING_ENTRY_NAME, self.network, nonce); + let key = format!( + "{}-{}-{}", + CIPHER_SEED_PASSWORD_KEYRING_ENTRY_NAME, + self.network.as_key_str(), + nonce + );crates/wallet/sdk/src/sdk.rs (1)
85-117: Docstring vs behavior mismatch for return valueDoc says: “Returns true if the cipher seed was recovered from the seed words, otherwise false.” When a seed already exists, this returns the current RecoveryNeeded flag instead. Clarify the doc or change behavior to always return false in the already‑initialized case.
Would you prefer adjusting the doc to “returns whether recovery is needed” for pre‑initialized wallets, or change the code to always return
Ok(false)in that branch?
📜 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 (107)
applications/tari_wallet_cli/src/command/account.rs(3 hunks)applications/tari_wallet_cli/src/command/key.rs(2 hunks)applications/tari_wallet_cli/src/command/transaction.rs(4 hunks)applications/tari_wallet_cli/src/command/validator.rs(2 hunks)applications/tari_walletd/Cargo.toml(1 hunks)applications/tari_walletd/src/handlers/accounts.rs(15 hunks)applications/tari_walletd/src/handlers/confidential.rs(4 hunks)applications/tari_walletd/src/handlers/helpers.rs(2 hunks)applications/tari_walletd/src/handlers/keys.rs(2 hunks)applications/tari_walletd/src/handlers/mod.rs(0 hunks)applications/tari_walletd/src/handlers/nfts.rs(5 hunks)applications/tari_walletd/src/handlers/settings.rs(1 hunks)applications/tari_walletd/src/handlers/transaction.rs(11 hunks)applications/tari_walletd/src/handlers/validator.rs(4 hunks)applications/tari_walletd/src/lib.rs(2 hunks)applications/tari_walletd/src/main.rs(5 hunks)applications/tari_walletd/src/services/mod.rs(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx(5 hunks)applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Accounts.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useKeys.tsx(1 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useTransactions.tsx(1 hunks)bindings/src/helpers/enum.ts(1 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/Account.ts(1 hunks)bindings/src/types/ResourceType.ts(0 hunks)bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsCreateOrGetRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsCreateRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeysListResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitDryRunRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitManifestRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts(1 hunks)bindings/src/wallet-daemon-client.ts(2 hunks)bindings/test/enumHelpers.test.ts(1 hunks)clients/wallet_daemon_client/Cargo.toml(1 hunks)clients/wallet_daemon_client/src/types.rs(9 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/shenanigans.rs(1 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/proof.rs(1 hunks)crates/engine_types/src/resource.rs(1 hunks)crates/engine_types/src/resource_container.rs(1 hunks)crates/engine_types/src/vault.rs(1 hunks)crates/ootle_address/src/ootle_address.rs(1 hunks)crates/template_builtin/build.rs(1 hunks)crates/template_lib/src/args/types.rs(1 hunks)crates/template_lib/src/models/bucket.rs(1 hunks)crates/template_lib/src/models/encrypted_data.rs(2 hunks)crates/template_lib/src/models/proof.rs(1 hunks)crates/template_lib/src/models/vault.rs(1 hunks)crates/template_lib/src/prelude.rs(2 hunks)crates/template_lib/src/resource/builder/confidential.rs(1 hunks)crates/template_lib/src/resource/builder/fungible.rs(1 hunks)crates/template_lib/src/resource/builder/non_fungible.rs(1 hunks)crates/template_lib/src/resource/builder/stealth.rs(1 hunks)crates/template_lib/src/resource/mod.rs(1 hunks)crates/template_lib_types/src/amount/amount.rs(3 hunks)crates/template_lib_types/src/lib.rs(1 hunks)crates/template_lib_types/src/resource_type.rs(1 hunks)crates/wallet/crypto/Cargo.toml(1 hunks)crates/wallet/crypto/src/encryption.rs(1 hunks)crates/wallet/crypto/src/hashers.rs(1 hunks)crates/wallet/crypto/src/lib.rs(1 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(7 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(12 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(13 hunks)crates/wallet/sdk/src/apis/config.rs(4 hunks)crates/wallet/sdk/src/apis/key_manager.rs(5 hunks)crates/wallet/sdk/src/apis/mod.rs(1 hunks)crates/wallet/sdk/src/apis/password_manager.rs(1 hunks)crates/wallet/sdk/src/apis/resources.rs(2 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(8 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(4 hunks)crates/wallet/sdk/src/apis/transaction.rs(2 hunks)crates/wallet/sdk/src/cipher_seed.rs(1 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/models/account.rs(3 hunks)crates/wallet/sdk/src/models/confidential_output.rs(2 hunks)crates/wallet/sdk/src/models/key.rs(1 hunks)crates/wallet/sdk/src/models/stealth_output.rs(2 hunks)crates/wallet/sdk/src/models/vault.rs(1 hunks)crates/wallet/sdk/src/sdk.rs(9 hunks)crates/wallet/sdk/src/storage.rs(9 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(6 hunks)crates/wallet/sdk/tests/support/harness.rs(3 hunks)crates/wallet/sdk_services/Cargo.toml(0 hunks)crates/wallet/sdk_services/src/account_monitor.rs(15 hunks)crates/wallet/sdk_services/src/account_recovery/service.rs(7 hunks)crates/wallet/sdk_services/src/utxo_scanner/error.rs(2 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(6 hunks)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs(8 hunks)crates/wallet/sdk_services/src/utxo_scanner/worker.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(6 hunks)crates/wallet/storage_sqlite/src/models/account.rs(2 hunks)crates/wallet/storage_sqlite/src/models/confidential_output.rs(3 hunks)crates/wallet/storage_sqlite/src/models/mod.rs(2 hunks)crates/wallet/storage_sqlite/src/models/resource.rs(2 hunks)crates/wallet/storage_sqlite/src/models/stealth_output.rs(3 hunks)crates/wallet/storage_sqlite/src/models/transaction.rs(1 hunks)
⛔ Files not processed due to max files limit (12)
- crates/wallet/storage_sqlite/src/models/utxo_process_queue.rs
- crates/wallet/storage_sqlite/src/models/vault.rs
- crates/wallet/storage_sqlite/src/reader.rs
- crates/wallet/storage_sqlite/src/schema.rs
- crates/wallet/storage_sqlite/src/serialization.rs
- crates/wallet/storage_sqlite/src/writer.rs
- crates/wallet/storage_sqlite/tests/accounts.rs
- integration_tests/src/wallet_daemon_client.rs
- integration_tests/tests/steps/wallet_daemon.rs
- utilities/tariswap_test_bench/src/accounts.rs
- utilities/tariswap_test_bench/src/runner.rs
- utilities/tariswap_test_bench/src/tariswap.rs
💤 Files with no reviewable changes (3)
- bindings/src/types/ResourceType.ts
- applications/tari_walletd/src/handlers/mod.rs
- crates/wallet/sdk_services/Cargo.toml
✅ Files skipped from review due to trivial changes (2)
- crates/template_lib/src/resource/builder/fungible.rs
- crates/engine_types/src/bucket.rs
🚧 Files skipped from review as they are similar to previous changes (40)
- applications/tari_walletd/web_ui/src/routes/Wallet/Components/Accounts.tsx
- crates/engine/tests/shenanigans.rs
- crates/wallet/sdk/src/lib.rs
- crates/template_lib/src/models/proof.rs
- applications/tari_walletd/src/handlers/settings.rs
- bindings/src/types/Account.ts
- applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx
- bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts
- crates/template_lib/src/models/encrypted_data.rs
- applications/tari_wallet_cli/src/command/account.rs
- crates/wallet/storage_sqlite/src/models/resource.rs
- crates/template_lib/src/models/bucket.rs
- crates/wallet/sdk/src/apis/transaction.rs
- crates/wallet/sdk/src/models/vault.rs
- crates/wallet/storage_sqlite/src/models/transaction.rs
- crates/engine_types/src/vault.rs
- applications/tari_walletd/web_ui/src/services/api/hooks/useTransactions.tsx
- bindings/src/wallet-daemon-client.ts
- crates/wallet/storage_sqlite/src/models/account.rs
- crates/engine_types/src/proof.rs
- applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx
- crates/template_lib_types/src/resource_type.rs
- crates/engine_types/src/resource_container.rs
- crates/wallet/sdk/src/cipher_seed.rs
- crates/wallet/storage_sqlite/src/models/mod.rs
- crates/template_lib/src/resource/builder/non_fungible.rs
- clients/wallet_daemon_client/src/types.rs
- bindings/src/types/wallet-daemon-client/TransactionSubmitDryRunRequest.ts
- crates/template_lib/src/args/types.rs
- bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts
- crates/wallet/sdk/Cargo.toml
- applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts
- crates/template_lib/src/resource/builder/stealth.rs
- crates/template_lib/src/resource/mod.rs
- clients/wallet_daemon_client/Cargo.toml
- crates/engine/src/transaction/processor.rs
- applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx
- crates/ootle_address/src/ootle_address.rs
- crates/engine_types/src/resource.rs
- crates/wallet/storage_sqlite/src/models/stealth_output.rs
🧰 Additional context used
🧬 Code graph analysis (39)
applications/tari_wallet_cli/src/command/validator.rs (2)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts (1)
GetValidatorFeesRequest(5-5)
applications/tari_walletd/src/handlers/helpers.rs (3)
crates/wallet/sdk/src/models/account.rs (2)
address(25-27)address(79-81)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)bindings/src/helpers/consts.ts (1)
ACCOUNT_TEMPLATE_ADDRESS(6-6)
bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts (2)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)
bindings/src/types/wallet-daemon-client/TransactionSubmitManifestRequest.ts (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_walletd/src/handlers/validator.rs (5)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/Account.ts (1)
Account(6-14)applications/tari_walletd/src/handlers/helpers.rs (1)
get_account_or_default(140-157)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (2)
key_id(22-24)secret(91-93)
applications/tari_walletd/src/handlers/confidential.rs (1)
applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_request(189-194)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (3)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)crates/wallet/sdk/src/models/account.rs (3)
account(71-73)address(25-27)address(79-81)crates/wallet/sdk_services/src/notify.rs (1)
new(12-15)
crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/crypto/src/kdfs.rs (1)
encrypted_data_dh_kdf_aead(32-44)
crates/wallet/sdk/src/apis/stealth_transfer.rs (2)
crates/wallet/sdk/src/models/account.rs (3)
owner_key_id(33-35)owner_key_id(91-93)account(71-73)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/wallet/sdk/src/models/confidential_output.rs (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk/tests/confidential_output_api.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)crates/wallet/sdk/tests/support/harness.rs (2)
test_account_address(79-83)test_vault_address(85-89)
crates/template_lib/src/models/vault.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
applications/tari_walletd/src/main.rs (3)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/models/account.rs (5)
address(25-27)address(79-81)view_only_public_key(99-101)view_only_key_id(29-31)view_only_key_id(87-89)crates/wallet/sdk/src/models/key.rs (3)
public_key(30-32)public_key(154-156)secret(91-93)
applications/tari_walletd/src/handlers/nfts.rs (2)
applications/tari_walletd/src/handlers/helpers.rs (2)
invalid_params(159-170)get_account_with_inputs(96-110)crates/wallet/sdk/src/sdk.rs (1)
key_manager_api(144-152)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (7)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/UtxoSpent.ts (1)
UtxoSpent(4-4)bindings/src/types/UtxoUnspent.ts (1)
UtxoUnspent(5-5)bindings/src/types/WalletUtxoUpdate.ts (1)
WalletUtxoUpdate(6-6)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)crates/engine_types/src/resource.rs (1)
view_key(126-128)
crates/wallet/sdk/src/apis/confidential_transfer.rs (2)
crates/wallet/sdk/src/sdk.rs (2)
confidential_outputs_api(183-185)key_manager_api(144-152)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
key_manager_api(355-372)
bindings/src/types/wallet-daemon-client/KeysListResponse.ts (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/wallet/sdk_services/src/account_recovery/service.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/sdk/src/models/account.rs (4)
owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)
applications/tari_walletd/src/lib.rs (1)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)
applications/tari_wallet_cli/src/command/key.rs (3)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/wallet/sdk/src/apis/key_manager.rs (4)
crates/wallet/crypto/src/encryption.rs (2)
decrypt_with_password(46-114)encrypt_with_password(116-150)crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/apis/accounts.rs (1)
new(57-69)crates/wallet/sdk/src/models/key.rs (12)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)derived(226-228)secret_key(158-160)imported(230-232)key_id(22-24)key_index(150-152)
crates/wallet/sdk/src/apis/accounts.rs (9)
bindings/src/helpers/consts.ts (1)
XTR(10-10)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/ootle_address/src/ootle_address.rs (5)
network(40-42)network(224-226)account_public_key(48-50)view_only_key(44-46)view_only_key(228-230)crates/wallet/sdk/src/sdk.rs (3)
network(131-133)store(119-121)key_manager_api(144-152)crates/wallet/sdk/src/models/key.rs (8)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)key_id(22-24)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(91-103)crates/wallet/storage_sqlite/src/reader.rs (11)
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(563-566)accounts(597-600)accounts(728-731)
crates/template_lib/src/prelude.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/apis/password_manager.rs (1)
bindings/src/types/Network.ts (1)
Network(6-6)
applications/tari_walletd/src/handlers/accounts.rs (3)
crates/wallet/sdk/src/sdk.rs (2)
accounts_api(166-173)network(131-133)applications/tari_walletd/src/handlers/helpers.rs (1)
get_account_by_key_index(125-138)crates/wallet/sdk/src/models/account.rs (2)
account(71-73)view_only_public_key(99-101)
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (3)
bindings/src/types/wallet-daemon-client/AccountInfo.ts (1)
AccountInfo(5-5)bindings/src/helpers/enum.ts (1)
matchesTypeEnum(4-35)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
bindings/test/enumHelpers.test.ts (1)
bindings/src/helpers/enum.ts (1)
matchesTypeEnum(4-35)
crates/wallet/sdk/tests/support/harness.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/sdk.rs (1)
accounts_api(166-173)crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (4)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (1)
watch(120-120)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(51-70)
crates/wallet/sdk/src/models/key.rs (4)
crates/ootle_address/src/ootle_address.rs (2)
fmt(137-139)from_str(145-147)crates/template_lib/src/args/types.rs (9)
fmt(79-86)fmt(108-110)fmt(152-157)fmt(211-216)fmt(334-351)fmt(464-469)fmt(660-665)from(205-207)from_str(92-100)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk/src/models/account.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
applications/tari_walletd/src/handlers/transaction.rs (3)
applications/tari_walletd/src/handlers/helpers.rs (6)
get_account(112-123)get_account_or_default(140-157)invalid_params(159-170)invalid_request(189-194)not_found(180-187)transaction_rejected(196-201)applications/tari_walletd/src/services/wasm_optimizer.rs (1)
optimize_wasm_template(35-54)bindings/src/types/Account.ts (1)
Account(6-14)
crates/wallet/crypto/src/hashers.rs (1)
crates/wallet/sdk/src/sdk.rs (1)
network(131-133)
crates/wallet/sdk_services/src/account_monitor.rs (2)
applications/tari_walletd/src/handlers/context.rs (1)
shutdown_signal(82-84)crates/wallet/sdk/src/sdk.rs (2)
accounts_api(166-173)resources_api(175-177)
crates/template_lib/src/resource/builder/confidential.rs (2)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/models/stealth_output.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/OutputStatus.ts (1)
OutputStatus(3-3)
crates/wallet/sdk/src/storage.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/storage_sqlite/src/reader.rs (6)
key_manager_get_raw_imported_key(163-192)confidential_outputs_get_unspent_balance(672-695)confidential_outputs_get_locked_by_lock_id(697-747)confidential_outputs_get_by_commitment(749-789)confidential_outputs_get_by_account_and_status(791-829)key_type(183-184)crates/wallet/storage_sqlite/src/writer.rs (6)
key_manager_insert_imported_key(280-302)confidential_outputs_lock_smallest_amount(856-942)confidential_outputs_insert(944-977)confidential_outputs_finalize_by_lock_id(979-1007)confidential_outputs_release_by_lock_id(1009-1032)utxo_process_queue_extend(1470-1497)crates/wallet/sdk/src/models/account.rs (6)
view_only_key_id(29-31)view_only_key_id(87-89)owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)
crates/wallet/sdk/src/sdk.rs (3)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/apis/accounts.rs (1)
new(57-69)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(91-103)
1f036b0 to
a61b1f6
Compare
a61b1f6 to
58c445e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/tari_walletd/src/handlers/transaction.rs (1)
87-94: Fail fast when fee account lacks an owner key
fee_account.owner_key_id()returnsNonefor view-only accounts, but we forward it straight into the submit path. The key manager then falls back to the “active” key (or errors later), so view-only wallets hit opaque signing failures instead of a clear “can’t spend” response. Please reject the request up front by turning theOptioninto aResultand only passingSome(signing_key_id)to the submit call sites (both the regular and dry-run flows).let fee_account = get_account(&req.fee_account, &sdk.accounts_api())?; + let signing_key_id = fee_account + .owner_key_id() + .ok_or_else(|| invalid_params("fee_account", Some("fee account must have an owner key")))?; + let transaction = builder .fee_transaction_pay_from_component(*fee_account.component_address(), req.max_fee) .with_min_epoch(req.min_epoch.map(Epoch)) .with_max_epoch(req.max_epoch.map(Epoch)) .build_unsigned_transaction(); let request = TransactionSubmitRequest { transaction, - signing_key_id: fee_account.owner_key_id(), + signing_key_id: Some(signing_key_id), detect_inputs: req.override_inputs.unwrap_or_default(), detect_inputs_use_unversioned: true, proof_ids: vec![], };Apply the same guard in the publish-template dry-run/submit paths.
Also applies to: 540-566
♻️ Duplicate comments (11)
applications/tari_walletd/src/handlers/nfts.rs (1)
305-309: Critical: Mismatched signer public/secret keys (appears reverted).The
add_signercall pairs the fee payer's public key with the source account's secret key, which is cryptographically invalid. Public and secret keys must belong to the same keypair.This exact issue was previously flagged and marked as addressed in commit 1f036b0, but the problematic code is still present. Please verify whether the fix was reverted or not properly merged.
Apply this diff to use matching key pairs:
.add_signer( - &fee_owner_key.to_public_key().to_byte_type(), + &source_account_secret_key.to_public_key().to_byte_type(), &source_account_secret_key.secret, ) .build_and_seal(&fee_owner_key.secret);bindings/src/helpers/enum.ts (1)
4-35: Reference equality breaks variant matching.Line 32's
enumValue === valueValueuses reference equality for object payloads, causing equivalent variants with different object instances to fail comparison. For example,{Derived: {index: 1n}}will not match another{Derived: {index: 1n}}instance.This was flagged in a previous review with a suggested fix to check only the discriminant key presence instead of comparing payload values by reference.
crates/wallet/sdk/src/apis/key_manager.rs (1)
236-250: Critical bug: view_only_key_id uses owner key.Line 248 sets
view_only_key_id: key.as_key_id()instead ofview_only_key.as_key_id(), causing identical owner and view key IDs. This breaks downstream flows that resolve view keys or compare owner vs. view IDs.This was flagged in a previous review with a suggested fix.
crates/wallet/storage_sqlite/src/models/confidential_output.rs (2)
73-75: Avoid unwrap() on hex decode; return proper storage errors.The
unwrap()call can panic on corrupt database data. The hex decode error should be mapped toWalletStorageErrorfor consistent error handling.Apply this diff to handle decode errors gracefully:
public_asset_tag: self .public_asset_tag - .map(|tag| RistrettoPublicKeyBytes::from_hex(&tag).unwrap()), + .map(|tag| { + RistrettoPublicKeyBytes::from_hex(&tag).map_err(|_| WalletStorageError::DecodingError { + operation: "try_into_output", + item: "public_asset_tag", + details: "Corrupt db: invalid hex".to_string(), + }) + }) + .transpose()?,
61-63: Avoid unwrap() on hex decode; return proper storage errors.The
unwrap()call can panic on corrupt database data. The hex decode error should be mapped toWalletStorageErrorlike other fields in this function.Apply this diff to handle decode errors gracefully:
sender_public_nonce: self .sender_public_nonce - .map(|nonce| RistrettoPublicKeyBytes::from_hex(&nonce).unwrap()), + .map(|nonce| { + RistrettoPublicKeyBytes::from_hex(&nonce).map_err(|_| WalletStorageError::DecodingError { + operation: "try_into_output", + item: "sender_public_nonce", + details: "Corrupt db: invalid hex".to_string(), + }) + }) + .transpose()?,applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (1)
253-257: Crash when Imported keys are present (already flagged).
extractKeyIndexreturnsnullfor{ Imported { … } }, yet line 254 immediately asserts non-null with!and calls.toString(). As soon as the list includes an imported/view-only key, this will crash at render-time.The previous review comment provides a detailed fix. Guard the variant before stringifying or use the
KeyIditself as the MenuItem value instead of forcing a numeric index.applications/tari_walletd/src/main.rs (1)
108-117: JSON field name "key_index" is misleading (already flagged).The JSON output field
key_indexon line 116 now contains aKeyIdobject (enum with Derived/Imported variants), not a simple numeric index. This is a breaking change for consumers expecting a number, and the field name is misleading.The previous review comment provides a detailed fix to rename the field to
view_only_key_idor add backward compatibility handling.applications/tari_walletd/src/handlers/validator.rs (1)
181-186: Fix signer key mismatchWe still sign with the claim secret but pair it with the account-owner public key. That produces an invalid signature and the transaction gets rejected. Use
claim_public_key.to_byte_type()(the key that matchessecret) when adding the signer.applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (2)
40-46: Handle KeyId variants safely before renderingAccessing
key.Derived.indexafter a// @ts-ignorewill explode the moment we render an Imported key (it’s undefined at runtime). On top of that React 18 still chokes on rawbigint, and this row has no stablekeyprop. Please branch on"Derived" in keyId, stringify whichever identifier you show, and give the<TableRow>a deterministic key (e.g.derived-${index}/imported-${local_key_id}). Until this is fixed the view-only rows crash the page.
61-66: Make Activate guard Imported keys and convert the index
setActivesilently no-ops for Imported keys (the button still shows) and we pass abigintintomutateSetActive, which expects a JSnumber. Please bail out early (or hide the button) when"Imported" in keyId, and cast withNumber(keyId.Derived.index)before calling the mutation.crates/wallet/sdk/src/models/key.rs (1)
84-98: Address the previously flagged security concern.The
secretfield remains public despite the past review comment requesting it be made private. Making it private would reduce the risk of accidental secret exposure. Thesecret()accessor already exists.Apply this diff to address the concern:
#[derive(Clone)] pub struct Key { - pub secret: RistrettoSecretKey, + secret: RistrettoSecretKey, pub key_id: KeyId, } impl Key { + pub fn new(secret: RistrettoSecretKey, key_id: KeyId) -> Self { + Self { secret, key_id } + } + pub fn secret(&self) -> &RistrettoSecretKey { &self.secret }
🧹 Nitpick comments (9)
crates/wallet/crypto/src/hashers.rs (1)
11-11: LGTM! Consider adding a helper function for consistency.The new 32-bit hasher type alias follows the same pattern as the 64-bit version and is correctly implemented.
Optional: Consider whether a
wallet_hasher32helper function (similar towallet_hasher64on line 13) would be beneficial for consistency, though the encryption module usage pattern may not require it.crates/template_lib_types/src/amount/amount.rs (1)
618-641: Good test coverage; consider adding edge case tests.The test comprehensively covers the happy path scenarios for decimal formatting, including negative values and trailing zeros.
If bounds validation is added to
fmt_decimalsas suggested above, consider adding test cases for:
- Very large
decimalsvalues (e.g., > 57)- Boundary values (e.g., decimals = 57)
- Zero value with various decimal places
bindings/test/enumHelpers.test.ts (1)
34-38: Fix test description to match expectation.Line 34 says “returns false…”, yet the assertion on Line 37 deliberately expects an error to be thrown. Please align the description (or assertion) so the test documents the behavior accurately.
crates/wallet/sdk/src/apis/resources.rs (1)
34-38: Consider implementing a dedicated exists query for better performance.The current implementation fetches the entire resource just to check existence. For large resources or high-frequency checks, this could be inefficient.
As noted in the TODO, consider adding a storage-level
resources_exists()method that performs a lightweight existence check (e.g.,SELECT COUNT(1)orSELECT 1query) without retrieving the full resource data.crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
706-708: TODO comment flags temporary workaround.The TODO comment indicates that adding the XTR input is a temporary measure. Consider creating a tracking issue if this workaround needs to be addressed in a future release.
Do you want me to help create a GitHub issue to track the removal of this workaround?
applications/tari_walletd/src/handlers/keys.rs (1)
47-47: Confirm derived-only behavior:get_all_derived_keyscallstx.key_manager_get_alland only returns derived keys; imported keys are handled viaget_imported_key. Consider adding or updating API docs to clarify this behavior.applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
108-108: Add a React key for each row
data.keys.map(...)returns<TableRow>elements without akey. This spams warnings and hurts reconciliation—use the same identifier you display (e.g.rowKeyfrom the KeyId guard).crates/wallet/sdk/src/models/key.rs (2)
42-57: LGTM with consideration.The
ImportedWalletKeystruct provides appropriate functionality for imported keys. Note that thekeyfield is public, which could increase the risk of accidental secret exposure. Consider whether accessor methods would be more appropriate, similar to the past review comment on theKeystruct.
59-82: LGTM with consideration.The
DerivedWalletKeystruct and itsFromimplementation correctly support the migration from the oldDerivedKeytype. The publickeyfield has the same consideration asImportedWalletKeyregarding potential secret exposure.
📜 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 (107)
applications/tari_wallet_cli/src/command/account.rs(3 hunks)applications/tari_wallet_cli/src/command/key.rs(2 hunks)applications/tari_wallet_cli/src/command/transaction.rs(4 hunks)applications/tari_wallet_cli/src/command/validator.rs(2 hunks)applications/tari_walletd/Cargo.toml(1 hunks)applications/tari_walletd/src/handlers/accounts.rs(15 hunks)applications/tari_walletd/src/handlers/confidential.rs(4 hunks)applications/tari_walletd/src/handlers/helpers.rs(2 hunks)applications/tari_walletd/src/handlers/keys.rs(2 hunks)applications/tari_walletd/src/handlers/mod.rs(0 hunks)applications/tari_walletd/src/handlers/nfts.rs(5 hunks)applications/tari_walletd/src/handlers/settings.rs(1 hunks)applications/tari_walletd/src/handlers/transaction.rs(11 hunks)applications/tari_walletd/src/handlers/validator.rs(4 hunks)applications/tari_walletd/src/lib.rs(2 hunks)applications/tari_walletd/src/main.rs(5 hunks)applications/tari_walletd/src/services/mod.rs(1 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx(5 hunks)applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Accounts.tsx(1 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useKeys.tsx(1 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useTransactions.tsx(1 hunks)bindings/src/helpers/enum.ts(1 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/Account.ts(1 hunks)bindings/src/types/ResourceType.ts(0 hunks)bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsCreateOrGetRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsCreateRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeysListResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitDryRunRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitManifestRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts(1 hunks)bindings/src/wallet-daemon-client.ts(2 hunks)bindings/test/enumHelpers.test.ts(1 hunks)clients/wallet_daemon_client/Cargo.toml(1 hunks)clients/wallet_daemon_client/src/types.rs(9 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/shenanigans.rs(1 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/proof.rs(1 hunks)crates/engine_types/src/resource.rs(1 hunks)crates/engine_types/src/resource_container.rs(1 hunks)crates/engine_types/src/vault.rs(1 hunks)crates/ootle_address/src/ootle_address.rs(1 hunks)crates/template_builtin/build.rs(1 hunks)crates/template_lib/src/args/types.rs(1 hunks)crates/template_lib/src/models/bucket.rs(1 hunks)crates/template_lib/src/models/encrypted_data.rs(2 hunks)crates/template_lib/src/models/proof.rs(1 hunks)crates/template_lib/src/models/vault.rs(1 hunks)crates/template_lib/src/prelude.rs(2 hunks)crates/template_lib/src/resource/builder/confidential.rs(1 hunks)crates/template_lib/src/resource/builder/fungible.rs(1 hunks)crates/template_lib/src/resource/builder/non_fungible.rs(1 hunks)crates/template_lib/src/resource/builder/stealth.rs(1 hunks)crates/template_lib/src/resource/mod.rs(1 hunks)crates/template_lib_types/src/amount/amount.rs(3 hunks)crates/template_lib_types/src/lib.rs(1 hunks)crates/template_lib_types/src/resource_type.rs(1 hunks)crates/wallet/crypto/Cargo.toml(1 hunks)crates/wallet/crypto/src/encryption.rs(1 hunks)crates/wallet/crypto/src/hashers.rs(1 hunks)crates/wallet/crypto/src/lib.rs(1 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(7 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(12 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(13 hunks)crates/wallet/sdk/src/apis/config.rs(4 hunks)crates/wallet/sdk/src/apis/key_manager.rs(5 hunks)crates/wallet/sdk/src/apis/mod.rs(1 hunks)crates/wallet/sdk/src/apis/password_manager.rs(1 hunks)crates/wallet/sdk/src/apis/resources.rs(2 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(8 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(4 hunks)crates/wallet/sdk/src/apis/transaction.rs(2 hunks)crates/wallet/sdk/src/cipher_seed.rs(1 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/models/account.rs(3 hunks)crates/wallet/sdk/src/models/confidential_output.rs(2 hunks)crates/wallet/sdk/src/models/key.rs(1 hunks)crates/wallet/sdk/src/models/stealth_output.rs(2 hunks)crates/wallet/sdk/src/models/vault.rs(1 hunks)crates/wallet/sdk/src/sdk.rs(9 hunks)crates/wallet/sdk/src/storage.rs(9 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(6 hunks)crates/wallet/sdk/tests/support/harness.rs(3 hunks)crates/wallet/sdk_services/Cargo.toml(0 hunks)crates/wallet/sdk_services/src/account_monitor.rs(15 hunks)crates/wallet/sdk_services/src/account_recovery/service.rs(7 hunks)crates/wallet/sdk_services/src/utxo_scanner/error.rs(2 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(6 hunks)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs(8 hunks)crates/wallet/sdk_services/src/utxo_scanner/worker.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(6 hunks)crates/wallet/storage_sqlite/src/models/account.rs(2 hunks)crates/wallet/storage_sqlite/src/models/confidential_output.rs(3 hunks)crates/wallet/storage_sqlite/src/models/mod.rs(2 hunks)crates/wallet/storage_sqlite/src/models/resource.rs(2 hunks)crates/wallet/storage_sqlite/src/models/stealth_output.rs(3 hunks)crates/wallet/storage_sqlite/src/models/transaction.rs(1 hunks)
⛔ Files not processed due to max files limit (12)
- crates/wallet/storage_sqlite/src/models/utxo_process_queue.rs
- crates/wallet/storage_sqlite/src/models/vault.rs
- crates/wallet/storage_sqlite/src/reader.rs
- crates/wallet/storage_sqlite/src/schema.rs
- crates/wallet/storage_sqlite/src/serialization.rs
- crates/wallet/storage_sqlite/src/writer.rs
- crates/wallet/storage_sqlite/tests/accounts.rs
- integration_tests/src/wallet_daemon_client.rs
- integration_tests/tests/steps/wallet_daemon.rs
- utilities/tariswap_test_bench/src/accounts.rs
- utilities/tariswap_test_bench/src/runner.rs
- utilities/tariswap_test_bench/src/tariswap.rs
💤 Files with no reviewable changes (3)
- bindings/src/types/ResourceType.ts
- applications/tari_walletd/src/handlers/mod.rs
- crates/wallet/sdk_services/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- crates/wallet/sdk/src/models/vault.rs
🚧 Files skipped from review as they are similar to previous changes (48)
- crates/engine/tests/shenanigans.rs
- crates/engine_types/src/resource_container.rs
- crates/template_lib/src/models/encrypted_data.rs
- crates/template_lib_types/src/lib.rs
- crates/template_lib/src/resource/builder/stealth.rs
- applications/tari_walletd/src/services/mod.rs
- crates/wallet/sdk/Cargo.toml
- crates/engine/src/transaction/processor.rs
- crates/wallet/storage_sqlite/src/models/stealth_output.rs
- crates/wallet/sdk/src/apis/mod.rs
- crates/template_lib/src/resource/builder/fungible.rs
- crates/template_lib/src/models/vault.rs
- applications/tari_walletd/src/lib.rs
- applications/tari_walletd/src/handlers/settings.rs
- applications/tari_wallet_cli/src/command/validator.rs
- crates/wallet/sdk/src/apis/confidential_transfer.rs
- crates/wallet/storage_sqlite/src/models/mod.rs
- crates/wallet/storage_sqlite/src/models/transaction.rs
- crates/engine_types/src/proof.rs
- applications/tari_walletd/web_ui/src/services/api/hooks/useKeys.tsx
- crates/wallet/crypto/src/lib.rs
- crates/wallet/sdk/src/apis/stealth_outputs.rs
- applications/tari_walletd/web_ui/src/routes/Manifest/Manifest.tsx
- clients/wallet_daemon_client/Cargo.toml
- crates/wallet/sdk/tests/confidential_output_api.rs
- crates/template_lib/src/prelude.rs
- crates/ootle_address/src/ootle_address.rs
- crates/wallet/sdk/src/cipher_seed.rs
- crates/engine_types/src/resource.rs
- crates/wallet/sdk/src/apis/config.rs
- bindings/src/types/wallet-daemon-client/TransactionSubmitDryRunRequest.ts
- bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts
- crates/template_lib/src/args/types.rs
- crates/engine_types/src/bucket.rs
- applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx
- applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts
- crates/wallet/crypto/Cargo.toml
- bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts
- crates/wallet/sdk/src/lib.rs
- crates/template_lib/src/resource/builder/confidential.rs
- crates/template_builtin/build.rs
- bindings/src/types/wallet-daemon-client/KeysListResponse.ts
- applications/tari_walletd/web_ui/src/services/api/hooks/useTransactions.tsx
- crates/wallet/crypto/src/encryption.rs
- applications/tari_wallet_cli/src/command/transaction.rs
- applications/tari_walletd/Cargo.toml
- bindings/src/types/wallet-daemon-client/AccountsCreateOrGetRequest.ts
- applications/tari_wallet_cli/src/command/key.rs
🧰 Additional context used
🧬 Code graph analysis (40)
crates/wallet/storage_sqlite/src/models/resource.rs (2)
crates/engine_types/src/resource.rs (1)
auth_hook(139-141)crates/wallet/storage_sqlite/src/models/transaction.rs (1)
deserialize_json(46-46)
bindings/src/types/wallet-daemon-client/GetValidatorFeesRequest.ts (2)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)
crates/template_lib/src/models/proof.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (4)
crates/wallet/sdk/src/models/account.rs (1)
account(71-73)bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(51-70)
bindings/src/types/wallet-daemon-client/TransactionSubmitManifestRequest.ts (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/storage_sqlite/src/models/account.rs (2)
crates/wallet/storage_sqlite/src/reader.rs (11)
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(563-566)accounts(597-600)accounts(728-731)bindings/src/types/Account.ts (1)
Account(6-14)
applications/tari_walletd/src/handlers/confidential.rs (1)
applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_request(189-194)
crates/wallet/sdk/src/models/stealth_output.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/OutputStatus.ts (1)
OutputStatus(3-3)
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (1)
crates/wallet/sdk/src/models/account.rs (1)
account(71-73)
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_walletd/src/handlers/helpers.rs (5)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)crates/wallet/sdk/src/models/key.rs (1)
key_index(150-152)crates/wallet/sdk/src/models/account.rs (2)
address(25-27)address(79-81)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)bindings/src/helpers/consts.ts (1)
ACCOUNT_TEMPLATE_ADDRESS(6-6)
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (3)
bindings/src/types/wallet-daemon-client/AccountInfo.ts (1)
AccountInfo(5-5)bindings/src/helpers/enum.ts (1)
matchesTypeEnum(4-35)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk_services/src/account_recovery/service.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)
applications/tari_walletd/src/handlers/nfts.rs (2)
applications/tari_walletd/src/handlers/helpers.rs (2)
invalid_params(159-170)get_account_with_inputs(96-110)crates/wallet/sdk/src/sdk.rs (1)
key_manager_api(144-152)
crates/template_lib/src/resource/builder/non_fungible.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (3)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)crates/wallet/sdk/src/models/account.rs (3)
account(71-73)address(25-27)address(79-81)crates/wallet/sdk_services/src/notify.rs (1)
new(12-15)
crates/wallet/sdk/src/models/account.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
applications/tari_walletd/src/main.rs (4)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/models/account.rs (5)
address(25-27)address(79-81)view_only_public_key(99-101)view_only_key_id(29-31)view_only_key_id(87-89)crates/ootle_address/src/ootle_address.rs (1)
to_byte_type(253-259)crates/wallet/sdk/src/models/key.rs (3)
public_key(30-32)public_key(154-156)secret(91-93)
applications/tari_walletd/src/handlers/validator.rs (4)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/Account.ts (1)
Account(6-14)applications/tari_walletd/src/handlers/helpers.rs (1)
get_account_or_default(140-157)crates/wallet/sdk/src/models/key.rs (2)
key_id(22-24)secret(91-93)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (6)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/UtxoSpent.ts (1)
UtxoSpent(4-4)bindings/src/types/UtxoUnspent.ts (1)
UtxoUnspent(5-5)bindings/src/types/WalletUtxoUpdate.ts (1)
WalletUtxoUpdate(6-6)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)
bindings/src/types/Account.ts (3)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
applications/tari_walletd/src/handlers/keys.rs (1)
bindings/src/types/wallet-daemon-client/KeysListResponse.ts (1)
KeysListResponse(5-10)
crates/wallet/sdk/tests/support/harness.rs (1)
crates/wallet/sdk/src/models/key.rs (1)
derived(226-228)
crates/wallet/sdk/src/sdk.rs (3)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/apis/accounts.rs (1)
new(57-69)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(91-103)
crates/engine_types/src/vault.rs (3)
bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/template_lib/src/models/bucket.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/models/confidential_output.rs (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk/src/apis/accounts.rs (6)
bindings/src/types/Network.ts (1)
Network(6-6)bindings/src/helpers/consts.ts (1)
XTR(10-10)bindings/src/types/Account.ts (1)
Account(6-14)crates/ootle_address/src/ootle_address.rs (5)
network(40-42)network(224-226)account_public_key(48-50)view_only_key(44-46)view_only_key(228-230)crates/wallet/sdk/src/sdk.rs (3)
network(131-133)store(119-121)key_manager_api(144-152)crates/wallet/sdk/src/models/key.rs (8)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)key_id(22-24)
crates/template_lib_types/src/resource_type.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/wallet/sdk/src/apis/password_manager.rs (1)
crates/wallet/sdk/src/sdk.rs (1)
sdk_config(127-129)
applications/tari_walletd/src/handlers/accounts.rs (3)
crates/wallet/sdk/src/sdk.rs (2)
accounts_api(166-173)network(131-133)applications/tari_walletd/src/handlers/helpers.rs (2)
get_account_by_key_index(125-138)invalid_params(159-170)crates/wallet/sdk/src/models/account.rs (2)
account(71-73)view_only_public_key(99-101)
crates/wallet/sdk/src/apis/key_manager.rs (3)
crates/wallet/crypto/src/encryption.rs (2)
decrypt_with_password(30-98)encrypt_with_password(100-134)crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/models/key.rs (12)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)derived(226-228)secret_key(158-160)imported(230-232)key_id(22-24)key_index(150-152)
clients/wallet_daemon_client/src/types.rs (6)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)bindings/src/types/ShardGroup.ts (1)
ShardGroup(4-4)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)
crates/wallet/sdk/src/apis/stealth_transfer.rs (3)
crates/wallet/sdk/src/models/account.rs (3)
owner_key_id(33-35)owner_key_id(91-93)account(71-73)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/helpers/consts.ts (1)
XTR(10-10)
crates/wallet/sdk/src/apis/confidential_outputs.rs (3)
bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/crypto/src/kdfs.rs (1)
encrypted_data_dh_kdf_aead(32-44)crates/engine_types/src/resource.rs (1)
view_key(126-128)
crates/wallet/sdk_services/src/account_monitor.rs (3)
applications/tari_walletd/src/handlers/context.rs (1)
shutdown_signal(82-84)crates/wallet/sdk/src/sdk.rs (2)
accounts_api(166-173)resources_api(175-177)bindings/src/helpers/consts.ts (1)
XTR(10-10)
crates/wallet/sdk/src/storage.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/storage_sqlite/src/reader.rs (6)
key_manager_get_raw_imported_key(163-192)confidential_outputs_get_unspent_balance(672-695)confidential_outputs_get_locked_by_lock_id(697-747)confidential_outputs_get_by_commitment(749-789)confidential_outputs_get_by_account_and_status(791-829)key_type(183-184)crates/wallet/storage_sqlite/src/writer.rs (6)
key_manager_insert_imported_key(280-302)confidential_outputs_lock_smallest_amount(856-942)confidential_outputs_insert(944-977)confidential_outputs_finalize_by_lock_id(979-1007)confidential_outputs_release_by_lock_id(1009-1032)utxo_process_queue_extend(1470-1497)
crates/wallet/sdk/src/models/key.rs (4)
crates/ootle_address/src/ootle_address.rs (2)
fmt(137-139)from_str(145-147)crates/template_lib/src/args/types.rs (9)
fmt(79-86)fmt(108-110)fmt(152-157)fmt(211-216)fmt(334-351)fmt(464-469)fmt(660-665)from(205-207)from_str(92-100)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_walletd/src/handlers/transaction.rs (4)
applications/tari_walletd/src/handlers/helpers.rs (6)
get_account(112-123)get_account_or_default(140-157)invalid_params(159-170)invalid_request(189-194)not_found(180-187)transaction_rejected(196-201)crates/wallet/sdk/src/apis/accounts.rs (1)
get_account_or_default(294-303)applications/tari_walletd/src/services/wasm_optimizer.rs (1)
optimize_wasm_template(35-54)bindings/src/types/Account.ts (1)
Account(6-14)
bindings/test/enumHelpers.test.ts (1)
bindings/src/helpers/enum.ts (1)
matchesTypeEnum(4-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). (3)
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: clippy
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/wallet/sdk/src/models/key.rs (1)
85-97: KeepKey.secretprivate.Leaving the secret key field public invites accidental reads/logging across the SDK; we already flagged this earlier. Please make the field private and expose access via the existing accessor plus a constructor so callers cannot obtain the secret by value.
#[derive(Clone)] pub struct Key { - pub secret: RistrettoSecretKey, + secret: RistrettoSecretKey, pub key_id: KeyId, } impl Key { + pub fn new(secret: RistrettoSecretKey, key_id: KeyId) -> Self { + Self { secret, key_id } + } pub fn secret(&self) -> &RistrettoSecretKey { &self.secret }bindings/src/helpers/enum.ts (1)
4-26: Bug: object variant comparison uses reference equalityFor discriminated unions like
KeyId({ Derived: {...} } | { Imported: {...} }), line 25 returns false when comparing equivalent variants with different object instances because===checks reference equality. Matching should be based on the discriminant key, not object identity.As suggested in the previous review, simplify to return true once both objects have the same discriminant key:
export function matchesTypeEnum<T extends Object>(enumObject: T | null, value: T | null): boolean { if (enumObject === null && value === null) { return true; } if (enumObject === null || value === null) { return false; } const keys = Object.keys(enumObject); if (keys.length !== 1) { throw new Error("Enum object must have exactly one key"); } const key = keys[0] as keyof T; - if (!(key in value)) { - return false; - } - - // check the value - const enumValue = (enumObject as any)[key]; - const valueValue = (value as any)[key]; - - return enumValue === valueValue; + return key in (value as any); }This reliably matches variant types for both primitive and object payloads.
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
359-372: Critical: Panic on imported view-only keys still presentDespite being marked as addressed in previous reviews, the
.expect("get_all_derived_keys returns only derived keys")on line 364 will still panic when processingKeyId::Imported, completely breaking the new view-only account feature introduced in this PR.The previous review provided a detailed fix:
- Build a
HashMapmappingview_only_key_id→account_public_keyfrom stored accounts- Branch on
view_key.key_id.derived_index():
- If
Some(idx): derive account key as currently done- If
None(imported): setaccount_key: Noneand look upaccount_public_keyfrom the HashMap- Remove the
expect()callWithout this fix, the first sync attempt after importing a view-only account will panic.
Based on past review comments.
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
64-68: Disable Activate control for Imported keys in the UI.The current implementation silently ignores Imported keys, making the Activate button appear clickable but non-functional. This creates a confusing user experience.
Move the Imported key check to the
Keycomponent to disable or hide the Activate control:function Key([keyId, pk, active]: [KeyId, string, boolean], setActive: (key_id: KeyId) => void) { const rowKey = "Derived" in keyId ? `derived-${keyId.Derived.index.toString()}` : `imported-${keyId.Imported.local_key_id.toString()}`; const displayIndex = "Derived" in keyId ? keyId.Derived.index.toString() : keyId.Imported.local_key_id.toString(); + const canActivate = "Derived" in keyId; return ( <TableRow key={rowKey}> <DataTableCell>{displayIndex}</DataTableCell> <DataTableCell>{pk}</DataTableCell> <DataTableCell> - {active ? <b>Active</b> : <div onClick={() => setActive(keyId)}>Activate</div>} + {active ? <b>Active</b> : canActivate ? <div onClick={() => setActive(keyId)}>Activate</div> : <span style={{color: '#999'}}>N/A</span>} </DataTableCell> </TableRow> ); }Additionally, ensure
mutateSetActivecan handle BigInt or convert explicitly:const setActive = (keyId: KeyId) => { if ("Derived" in keyId) { - mutateSetActive(keyId.Derived.index); + mutateSetActive(Number(keyId.Derived.index)); } };
🧹 Nitpick comments (5)
bindings/src/helpers/enum.ts (1)
4-4: Prefer lowercaseobjectover uppercaseObject.TypeScript convention uses lowercase
objectfor the non-primitive type, while uppercaseObjectrefers to the global Object interface.-export function matchesTypeEnum<T extends Object>(enumObject: T | null, value: T | null): boolean { +export function matchesTypeEnum<T extends object>(enumObject: T | null, value: T | null): boolean {crates/wallet/crypto/src/encryption.rs (3)
64-64: Consider replacingexpectwith explicit error handling.While the
expecthere should never panic (the salt slice fromsplit_at_checkedis guaranteed to be exactlySALT_LENGTHbytes), defensive programming suggests returning aCipherErrorinstead.Apply this diff for more defensive error handling:
- let salt: [u8; SALT_LENGTH] = copy_fixed_checked(salt).expect("Salt length is SALT_LENGTH"); + let salt: [u8; SALT_LENGTH] = copy_fixed_checked(salt) + .ok_or_else(|| CipherError(format!("Invalid salt length {}", salt.len())))?;
107-107: Consider simplifying the double-authentication pattern.The current implementation encrypts a MAC of the plaintext along with the plaintext itself, then applies ChaCha20Poly1305 authenticated encryption on top. This provides two layers of authentication:
- The MAC authenticates the plaintext (lines 107, 114)
- The ChaCha20Poly1305 tag authenticates the ciphertext (verified during decryption at line 83)
Since ChaCha20Poly1305 is a well-vetted AEAD that already provides both confidentiality and authenticity, the additional MAC is cryptographically redundant. While defense-in-depth has merit, this pattern adds computational overhead (Blake2b hashing) without meaningful security benefit.
Option 1 (simpler): Remove the MAC entirely and rely solely on ChaCha20Poly1305's built-in authentication.
Option 2 (if you prefer defense-in-depth): Document the rationale for the double-authentication pattern so future maintainers understand the design choice.
Also applies to: 114-114, 87-87, 90-90
217-297: Good test coverage.The tests cover the critical paths including successful encryption/decryption and various failure modes (invalid checksum, version, length, and corrupted data).
Optional enhancement: Consider adding a test case for wrong password to verify that decryption fails gracefully with an incorrect passphrase:
#[test] fn it_fails_for_wrong_password() { let password = b"correct horse battery staple"; let wrong_password = b"incorrect horse battery staple"; let data = b"The quick brown fox jumps over the lazy dog"; let encrypted = encrypt_with_password(data, password).expect("encryption failed"); let result = decrypt_with_password(&encrypted, wrong_password); assert!(result.is_err()); }crates/wallet/sdk/src/apis/accounts.rs (1)
93-104: Consider fetching the account from storage for consistency.Currently,
create_accountconstructs theAccountstruct inline after callingadd_account. This pattern differs from other methods (get_default,get_account_by_name, etc.), which fetch the account from storage and then construct the address. Constructing inline could lead to inconsistency if the storage layer applies transformations or defaults during insertion.Consider refactoring to fetch the account from storage after insertion:
self.add_account( account_name, &account_component_address, account_address.view_only_key_id, account_address.owner_key_id, false, is_default, )?; - Ok(AccountWithAddress { - account: Account { - name: account_name.map(String::from), - component_address: account_component_address, - view_only_key_id: account_address.view_only_key_id, - owner_key_id: Some(account_address.owner_key_id), - owner_public_key: account_public_key, - is_confirmed_on_chain: false, - is_default, - }, - address: account_address.address.to_byte_type(), - }) + self.get_account_by_address(&account_component_address) }This ensures the returned account matches exactly what was stored, maintaining consistency with the rest of the API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
applications/tari_walletd/web_ui/src/main.tsx(0 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(3 hunks)bindings/src/helpers/enum.ts(1 hunks)clients/javascript/wallet_daemon_client/src/index.ts(1 hunks)crates/template_lib_types/src/amount/amount.rs(3 hunks)crates/wallet/crypto/src/encryption.rs(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(7 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(8 hunks)crates/wallet/sdk/src/models/key.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(6 hunks)crates/wallet/storage_sqlite/src/models/confidential_output.rs(4 hunks)
💤 Files with no reviewable changes (1)
- applications/tari_walletd/web_ui/src/main.tsx
✅ Files skipped from review due to trivial changes (1)
- clients/javascript/wallet_daemon_client/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/template_lib_types/src/amount/amount.rs
- applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts
🧰 Additional context used
🧬 Code graph analysis (5)
crates/wallet/storage_sqlite/src/models/confidential_output.rs (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/wallet/sdk/src/apis/stealth_outputs.rs (5)
crates/wallet/sdk/src/models/account.rs (4)
owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/models/key.rs (1)
key_id(22-24)crates/engine_types/src/utxo.rs (2)
output(34-36)owner_public_key(42-44)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)
crates/wallet/sdk/src/models/key.rs (3)
crates/ootle_address/src/ootle_address.rs (2)
fmt(137-139)from_str(145-147)crates/template_lib/src/args/types.rs (9)
fmt(79-86)fmt(108-110)fmt(152-157)fmt(211-216)fmt(334-351)fmt(464-469)fmt(660-665)from(205-207)from_str(92-100)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk/src/apis/accounts.rs (5)
bindings/src/types/Network.ts (1)
Network(6-6)crates/ootle_address/src/ootle_address.rs (5)
network(40-42)network(224-226)account_public_key(48-50)view_only_key(44-46)view_only_key(228-230)crates/wallet/sdk/src/sdk.rs (2)
network(131-133)store(119-121)crates/wallet/sdk/src/models/key.rs (8)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)key_id(22-24)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(91-103)
🔇 Additional comments (16)
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
181-181: LGTM! Stealth lock release fix applied.The change from
confidential_outputs_release_by_lock_idtostealth_outputs_release_by_lock_idcorrectly releases stealth outputs instead of confidential outputs, fixing the issue where locked stealth outputs would remain unusable.
202-215: LGTM! Proper handling of owner key requirement for spending.The code correctly validates that an account has an owner key before attempting to spend, and provides a clear error message when view-only accounts (which lack owner keys) attempt to spend. The migration to the new key manager API methods is properly implemented.
524-563: LGTM! View-only account handling implemented correctly.The branching logic properly handles both full accounts (with owner keys) and view-only accounts:
- Full accounts validate ownership by deriving the stealth address and comparing with the output
- View-only accounts (without owner keys) can decrypt and track outputs but mark them as viewable without ownership validation
The clear logging at line 548 helps users understand the view-only limitation.
crates/wallet/storage_sqlite/src/models/confidential_output.rs (1)
62-69: Thanks for hardening the nonce decode.Replacing the
unwrap()with proper error propagation prevents panics on corrupt DB rows and keeps storage failures deterministic. Nicely done.applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
111-111: LGTM!The data mapping correctly types the keys as
[KeyId, string, boolean]and passes them to theKeycomponent along with thesetActivecallback.crates/wallet/crypto/src/encryption.rs (6)
1-28: LGTM!The imports and constants are well-defined. The 5-byte salt is appropriately expanded to a 16-byte Argon2 salt via Blake2b hashing (line 157-160), meeting cryptographic standards.
100-134: LGTM!The encryption flow is correct and well-structured. Proper use of
OsRngfor salt generation, and the capacity pre-allocation at lines 108-109 is efficient.
155-181: LGTM! Argon2id issue has been addressed.The key derivation correctly uses
Argon2id(line 175) with OWASP-recommended parameters (46 MiB memory, t-cost=1, p-cost=1). This addresses the critical issue raised in the past review comment about usingArgon2d, which is vulnerable to side-channel attacks in password-based scenarios.Based on past review comment.
137-152: LGTM!The helper functions are well-implemented:
- ChaCha20Poly1305 encryption/decryption with proper error handling
- Blake2b MAC generation with domain separation via labeled hashers
- Clean separation of concerns
Also applies to: 184-202
204-206: LGTM!Clean error type using
thiserrorfor idiomatic error handling.
208-215: LGTM!Safe utility function with proper bounds checking.
crates/wallet/sdk/src/apis/accounts.rs (5)
9-9: LGTM! Network field and imports properly integrated.The addition of the
networkfield toAccountsApiand the corresponding constructor update are well-integrated. All new imports (FromByteType,RistrettoOotleAddress,Network,XTR, and the new model types) are used appropriately throughout the file.Also applies to: 12-12, 16-16, 21-21, 28-40, 46-46, 57-69
134-135: Verify that XTR should be the default stealth resource for all account types.Lines 134-135 initialize every account with
XTRas a default stealth resource. Since this PR adds view-only account support, please confirm whether view-only accounts should also haveXTRadded by default. View-only accounts cannot spend or mutate, so initializing them with a stealth resource might not be appropriate.Please verify:
- Should view-only accounts be initialized with XTR?
- Should there be a distinction in default resources based on account type (owner vs. view-only)?
If view-only accounts should not have XTR by default, consider adding a parameter to control resource initialization:
pub fn add_account<K: Into<KeyIdOrPublicKey>>( &self, account_name: Option<&str>, account_address: &ComponentAddress, view_only_key_id: KeyId, owner_key: K, is_confirmed_on_chain: bool, is_default: bool, + initialize_stealth_resources: bool, ) -> Result<(), AccountsApiError> { ... let mut associated_stealth_resources = HashSet::new(); - associated_stealth_resources.insert(XTR); + if initialize_stealth_resources { + associated_stealth_resources.insert(XTR); + }
162-173: LGTM! Address resolution is consistently implemented.The new
get_address_for_accounthelper provides a centralized mechanism for resolving account addresses, and it's consistently used across all relevant methods (get_many,get_default,get_account_by_name,get_account_by_address,get_account_by_public_key). The implementation correctly handles bothKeyId::DerivedandKeyId::Importedcases, and includes proper error handling when converting the owner public key.This refactor improves maintainability by eliminating code duplication and establishing a single source of truth for address resolution.
Also applies to: 185-192, 194-201, 227-234, 236-259, 281-292
261-269: LGTM! Stealth resource association properly implemented.The new
associate_stealth_resourcemethod correctly delegates to the storage layer and follows the established pattern used by other methods in this API.
81-81: account_key() is correct: it returns theRistrettoPublicKeywhich.to_byte_type()converts into the expected byte type.
| function Key([keyId, pk, active]: [KeyId, string, boolean], setActive: (key_id: KeyId) => void) { | ||
| const rowKey = | ||
| "Derived" in keyId | ||
| ? `derived-${keyId.Derived.index.toString()}` | ||
| : `imported-${keyId.Imported.local_key_id.toString()}`; | ||
| return ( | ||
| <TableRow key={key[0]}> | ||
| <DataTableCell>{key[0]}</DataTableCell> | ||
| <DataTableCell>{key[1]}</DataTableCell> | ||
| <DataTableCell>{key[2] ? <b>Active</b> : <div onClick={() => setActive(key[0])}>Activate</div>}</DataTableCell> | ||
| <TableRow> | ||
| <DataTableCell>{rowKey}</DataTableCell> | ||
| <DataTableCell>{pk}</DataTableCell> | ||
| <DataTableCell>{active ? <b>Active</b> : <div onClick={() => setActive(keyId)}>Activate</div>}</DataTableCell> | ||
| </TableRow> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix missing React key prop and incorrect display value.
Two critical issues:
- Missing key prop: The computed
rowKeyis not used as thekeyprop onTableRow(line 46), causing React warnings and potential rendering issues. - Wrong display: Line 47 displays the
rowKeystring (e.g., "derived-123" or "imported-456") instead of just the index/ID number.
Apply this diff to fix both issues:
function Key([keyId, pk, active]: [KeyId, string, boolean], setActive: (key_id: KeyId) => void) {
const rowKey =
"Derived" in keyId
? `derived-${keyId.Derived.index.toString()}`
: `imported-${keyId.Imported.local_key_id.toString()}`;
+ const displayIndex =
+ "Derived" in keyId ? keyId.Derived.index.toString() : keyId.Imported.local_key_id.toString();
return (
- <TableRow>
- <DataTableCell>{rowKey}</DataTableCell>
+ <TableRow key={rowKey}>
+ <DataTableCell>{displayIndex}</DataTableCell>
<DataTableCell>{pk}</DataTableCell>
<DataTableCell>{active ? <b>Active</b> : <div onClick={() => setActive(keyId)}>Activate</div>}</DataTableCell>
</TableRow>
);
}📝 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.
| function Key([keyId, pk, active]: [KeyId, string, boolean], setActive: (key_id: KeyId) => void) { | |
| const rowKey = | |
| "Derived" in keyId | |
| ? `derived-${keyId.Derived.index.toString()}` | |
| : `imported-${keyId.Imported.local_key_id.toString()}`; | |
| return ( | |
| <TableRow key={key[0]}> | |
| <DataTableCell>{key[0]}</DataTableCell> | |
| <DataTableCell>{key[1]}</DataTableCell> | |
| <DataTableCell>{key[2] ? <b>Active</b> : <div onClick={() => setActive(key[0])}>Activate</div>}</DataTableCell> | |
| <TableRow> | |
| <DataTableCell>{rowKey}</DataTableCell> | |
| <DataTableCell>{pk}</DataTableCell> | |
| <DataTableCell>{active ? <b>Active</b> : <div onClick={() => setActive(keyId)}>Activate</div>}</DataTableCell> | |
| </TableRow> | |
| ); | |
| } | |
| function Key( | |
| [keyId, pk, active]: [KeyId, string, boolean], | |
| setActive: (key_id: KeyId) => void | |
| ) { | |
| const rowKey = | |
| "Derived" in keyId | |
| ? `derived-${keyId.Derived.index.toString()}` | |
| : `imported-${keyId.Imported.local_key_id.toString()}`; | |
| const displayIndex = | |
| "Derived" in keyId | |
| ? keyId.Derived.index.toString() | |
| : keyId.Imported.local_key_id.toString(); | |
| return ( | |
| <TableRow key={rowKey}> | |
| <DataTableCell>{displayIndex}</DataTableCell> | |
| <DataTableCell>{pk}</DataTableCell> | |
| <DataTableCell> | |
| {active ? ( | |
| <b>Active</b> | |
| ) : ( | |
| <div onClick={() => setActive(keyId)}>Activate</div> | |
| )} | |
| </DataTableCell> | |
| </TableRow> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx around
lines 40 to 52, the TableRow is missing its React key prop and the first
DataTableCell incorrectly renders the full rowKey string; set the computed
rowKey as the key prop on TableRow (key={rowKey}) and change the first
DataTableCell to render only the numeric identifier (for Derived render
keyId.Derived.index, for Imported render keyId.Imported.local_key_id), keeping
rowKey for the key prop and leaving the rest of the component logic unchanged.
6a735fc to
f8d6ef7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/wallet/sdk/src/apis/accounts.rs (1)
75-105: Verify consistency between address-derived and key-manager-derived owner public keys.
create_accountderivesaccount_public_keyfrom the provided address (line 81) and uses it to setowner_public_keyin the returnedAccountWithAddress(line 99). However,add_accountretrievesowner_pkfrom the key manager usingowner_key_id(lines 118-122 inadd_account) and stores that key in the database. If these two keys don't match, you'll have:
- The returned
AccountWithAddresscontainsowner_public_key = account_public_key(from the address).- The database stores
owner_public_key = owner_pk(from the key manager).- Later, when the account is retrieved (e.g., via
get_account_by_address),get_address_for_accountreconstructs the address using the storedowner_pk(lines 250-257), which may differ from the original address.This inconsistency can cause address mismatches and break downstream flows that rely on stable addresses.
Consider adding a verification step in
create_accountto ensure the address-derived public key matches the key-manager-derived public key:pub fn create_account( &self, account_name: Option<&str>, is_default: bool, account_address: WalletOotleAddressWithKeyIds, ) -> Result<AccountWithAddress, AccountsApiError> { let account_public_key = account_address.address.account_key().to_byte_type(); let account_component_address = derive_account_address_from_public_key(&account_public_key); + + // Verify that the address-derived key matches the key-manager key + let owner_pk_from_km = self + .key_manager_api + .get_account_owner_key(account_address.owner_key_id)? + .to_public_key() + .to_byte_type(); + if account_public_key != owner_pk_from_km { + return Err(AccountsApiError::StoreError(WalletStorageError::DataInconsistent { + operation: "create_account", + details: format!( + "Address-derived public key {:?} does not match key-manager key {:?}", + account_public_key, owner_pk_from_km + ), + })); + } self.add_account( account_name, &account_component_address, account_address.view_only_key_id, account_address.owner_key_id, false, is_default, )?; Ok(AccountWithAddress { account: Account { name: account_name.map(String::from), component_address: account_component_address, view_only_key_id: account_address.view_only_key_id, owner_key_id: Some(account_address.owner_key_id), - owner_public_key: account_public_key, + owner_public_key: owner_pk_from_km, is_confirmed_on_chain: false, is_default, }, address: account_address.address.to_byte_type(), }) }
♻️ Duplicate comments (7)
bindings/src/helpers/enum.ts (1)
4-26: Reference equality bug persists—apply the previously suggested fix.The critical issue flagged in the past review remains unresolved. Line 25 compares enum payload values using strict equality (
===), which fails for object variants with different references (e.g.,{ Derived: { index: 5 } }vs.{ Derived: { index: 5 } }). For type matching, only the discriminant key matters, not the payload.Apply the diff from the previous review to fix this and change
Objecttoobject.applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (2)
40-52: Fix missing React key prop and incorrect display value.The
TableRowat line 46 is missing thekeyprop, which will cause React warnings. Additionally, line 47 displays the fullrowKeystring (e.g., "derived-123") instead of just the numeric index.Apply this diff to fix both issues:
function Key([keyId, pk, active]: [KeyId, string, boolean], setActive: (key_id: KeyId) => void) { const rowKey = "Derived" in keyId ? `derived-${keyId.Derived.index.toString()}` : `imported-${keyId.Imported.local_key_id.toString()}`; + const displayIndex = + "Derived" in keyId ? keyId.Derived.index.toString() : keyId.Imported.local_key_id.toString(); return ( - <TableRow> - <DataTableCell>{rowKey}</DataTableCell> + <TableRow key={rowKey}> + <DataTableCell>{displayIndex}</DataTableCell> <DataTableCell>{pk}</DataTableCell> <DataTableCell>{active ? <b>Active</b> : <div onClick={() => setActive(keyId)}>Activate</div>}</DataTableCell> </TableRow> ); }
64-68: Disable Activate for Imported keys.The
setActivefunction correctly handles onlyDerivedkeys, but the UI does not disable or hide the "Activate" control forImportedkeys. This allows users to click a control that has no effect.Update the
Keyfunction to conditionally render the Activate control:function Key([keyId, pk, active]: [KeyId, string, boolean], setActive: (key_id: KeyId) => void) { const rowKey = "Derived" in keyId ? `derived-${keyId.Derived.index.toString()}` : `imported-${keyId.Imported.local_key_id.toString()}`; + const displayIndex = + "Derived" in keyId ? keyId.Derived.index.toString() : keyId.Imported.local_key_id.toString(); + const canActivate = "Derived" in keyId; return ( - <TableRow> - <DataTableCell>{rowKey}</DataTableCell> + <TableRow key={rowKey}> + <DataTableCell>{displayIndex}</DataTableCell> <DataTableCell>{pk}</DataTableCell> <DataTableCell> - {active ? <b>Active</b> : <div onClick={() => setActive(keyId)}>Activate</div>} + {active ? <b>Active</b> : canActivate ? <div onClick={() => setActive(keyId)}>Activate</div> : <i>N/A</i>} </DataTableCell> </TableRow> ); }applications/tari_walletd/src/main.rs (1)
118-119: Field name still implies a numeric indexWe still emit the
KeyIdenum under the"key_index"JSON key. This label no longer matches the payload and will keep confusing/breaking callers expecting a plain number. Please rename the field (e.g., to"view_only_key_id") or provide a compatibility alias.crates/wallet/storage_sqlite/src/models/confidential_output.rs (2)
61-69: Excellent error handling improvement for sender_public_nonce.The use of
transpose()andmap_errproperly handles hex decode failures instead of panicking on corrupt DB data. This addresses the critical issue flagged in previous reviews.
79-87: Excellent error handling improvement for public_asset_tag.Consistent with the sender_public_nonce fix, this properly handles hex decode failures instead of panicking.
crates/wallet/sdk/src/models/key.rs (1)
84-98: Public secret field remains unaddressed.The security concern regarding the public
secretfield was previously flagged and remains unresolved. Please implement the suggested fix from the earlier review.
🧹 Nitpick comments (2)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
350-350: Consider refactoring to reduce function complexity.The
verify_and_update_outputsfunction is quite long (~120 lines) and handles multiple responsibilities including key retrieval, output iteration, validation, and storage updates. While the#[allow(clippy::too_many_lines)]is acceptable, extracting helper functions for validation, update logic, or key preparation could improve maintainability and testability.crates/wallet/sdk/src/models/key.rs (1)
166-197: Consider case-insensitive parsing forKeyType::from_str.The current implementation is case-sensitive. If
KeyTypeis parsed from user input, consider adding case-insensitive matching for better user experience.Apply this diff if desired:
impl FromStr for KeyType { type Err = anyhow::Error; fn from_str(s: &str) -> Result<Self, Self::Err> { - match s { - "ViewOnly" => Ok(Self::ViewOnly), - "Owner" => Ok(Self::Owner), - "GeneralPurpose" => Ok(Self::GeneralPurpose), + match s.to_lowercase().as_str() { + "viewonly" => Ok(Self::ViewOnly), + "owner" => Ok(Self::Owner), + "generalpurpose" => Ok(Self::GeneralPurpose), _ => Err(anyhow::anyhow!("Invalid key type: {}", s)), } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
applications/tari_swarm_daemon/src/process_manager/instances/manager.rs(1 hunks)applications/tari_walletd/src/main.rs(5 hunks)applications/tari_walletd/web_ui/src/main.tsx(0 hunks)applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx(3 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(3 hunks)bindings/src/helpers/enum.ts(1 hunks)clients/javascript/wallet_daemon_client/src/index.ts(1 hunks)crates/template_lib_types/src/amount/amount.rs(3 hunks)crates/wallet/crypto/src/encryption.rs(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(7 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(8 hunks)crates/wallet/sdk/src/models/key.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(6 hunks)crates/wallet/storage_sqlite/src/models/confidential_output.rs(4 hunks)
💤 Files with no reviewable changes (1)
- applications/tari_walletd/web_ui/src/main.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- clients/javascript/wallet_daemon_client/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (5)
applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (3)
bindings/src/types/wallet-daemon-client/AccountOrKeyId.ts (1)
AccountOrKeyId(5-5)clients/javascript/wallet_daemon_client/src/index.ts (1)
validatorsGetFees(311-313)applications/tari_walletd/web_ui/src/utils/json_rpc.ts (1)
validatorsGetFees(322-323)
crates/wallet/sdk/src/apis/stealth_outputs.rs (4)
bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/sdk/src/models/account.rs (4)
owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)crates/wallet/sdk/src/models/key.rs (1)
key_id(22-24)crates/engine_types/src/component.rs (1)
derive_component_address_from_public_key(43-53)
crates/wallet/sdk/src/models/key.rs (3)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/transaction/src/v1/signature.rs (2)
public_key(63-65)public_key(122-124)
crates/wallet/sdk/src/apis/accounts.rs (3)
bindings/src/helpers/consts.ts (1)
XTR(10-10)crates/ootle_address/src/ootle_address.rs (5)
network(40-42)network(224-226)account_public_key(48-50)view_only_key(44-46)view_only_key(228-230)crates/wallet/sdk/src/models/key.rs (8)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)key_id(22-24)
applications/tari_walletd/web_ui/src/routes/Wallet/Components/Keys.tsx (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
⏰ 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: fmt
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: check nightly
🔇 Additional comments (28)
applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (2)
81-81: LGTM! Nullish coalescing correctly preserves zero.The use of
??ensures thatkeyIdwith a value of0is correctly passed to the API instead of being converted tonull. This addresses the previous review concern about using||.
315-318: LGTM! Parameter and type rename aligns with KeyId refactor.The function signature and query construction have been updated to use
AccountOrKeyIdinstead ofAccountOrKeyIndex, which aligns with the broader PR changes to introduce KeyId-based typing.applications/tari_swarm_daemon/src/process_manager/instances/manager.rs (1)
177-194: Verify claim_key.json writer uses account_public_key
The loader now expects"account_public_key". Ensure the process that generatesclaim_key.json(e.g. the wallet daemon’s create-account command) writes this field name to avoid runtime extraction errors.crates/template_lib_types/src/amount/amount.rs (4)
6-7: LGTM!The import additions are appropriate for the new decimal formatting functionality.
fmt::Writeprovides the trait bound for generic formatting, andserde::ser::Errorenables custom error messages.
318-327: LGTM!The
to_decimal_stringmethod provides a convenient string-based API that correctly delegates tofmt_decimals. The use ofexpectis appropriate since formatting into aStringcannot fail (only the bounds check infmt_decimalscan fail, which would be caught here).
365-365: LGTM!Delegating to the
Displayimplementation ofinner_value()is more idiomatic and maintainable than manual formatting.
628-660: LGTM!The test coverage is comprehensive and exercises all important paths:
- Various decimal places (0, 2, 5, 6, 8, 57)
- Edge case of 58 decimals (boundary validation)
- Negative values with proper sign handling
- Leading zero padding in fractional parts
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
181-184: LGTM! Stealth outputs now properly released.The change to
stealth_outputs_release_by_lock_idcorrectly releases locked stealth outputs, resolving the critical issue flagged in the previous review.
202-211: LGTM! Proper validation for spending permissions.The code correctly validates that the account has an owner key before allowing spending operations. The error message clearly explains why view-only accounts cannot spend funds.
524-551: LGTM! View-only output handling is correct.The validation logic properly differentiates between spendable and view-only outputs:
- When an owner key exists, it validates the stealth address and marks outputs as Unspent or Invalid accordingly
- When no owner key exists (view-only account), it marks the output as Unspent without ownership validation, with appropriate logging
This correctly implements the view-only account feature described in the PR objectives.
crates/wallet/storage_sqlite/src/models/confidential_output.rs (1)
70-71: KeyId deserialization looks correct.The use of
deserialize_jsonwith proper error propagation via?andtranspose()for the optionalowner_key_idcorrectly handles parsing failures.crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (3)
16-26: New table for imported keys looks well-structured.The
key_manager_imported_keystable has appropriate columns, a unique index onlabel, and proper NOT NULL constraints.
146-161: Confidential outputs schema is consistent with the model changes.The removal of
encryption_secret_key_indexand addition ofview_only_key_id,owner_key_id, andpublic_asset_tagalign with the Rust model. Foreign key constraints onaccount_idandvault_idare properly defined.
276-285: Foreign key constraint properly added.The
utxo_process_queue.account_idnow has theREFERENCES accounts(id) ON DELETE CASCADEconstraint as requested in previous reviews. This ensures referential integrity.crates/wallet/sdk/src/models/key.rs (11)
1-11: LGTM!Imports are appropriate for the key management functionality implemented in this file.
13-33: LGTM!
WalletKeyRecordproperly encapsulates the secret key withpub(crate)visibility and provides controlled access via getter methods.
35-40: LGTM!
WalletOotleAddressWithKeyIdsappropriately uses public fields for non-sensitive data transfer.
75-82: LGTM!The conversion from
tari_transaction_components::key_manager::tari_key_manager::DerivedKeyis implemented correctly.
100-134: Verify conversions after making secret fields private.The
Fromimplementations are correct but will need updates onceImportedWalletKey.keyandDerivedWalletKey.keyare made private (use accessor methods instead of direct field access).
136-141: LGTM!
AccountAndViewKeysappropriately groups account and view keys. OnceKey.secretis made private, this struct will benefit from improved encapsulation.
143-161: Updatesecret_key()accessor after makingDerivedWalletKey.keyprivate.The implementation is correct but will need to call
self.derived_key.secret_key()once the suggested refactor is applied.
163-164: LGTM!Type aliases for
DerivedKeyIndexandImportedKeyIdare clear and appropriate.
199-214: LGTM!
KeyIdOrPublicKeyenum and itsFromimplementations are correct and follow Rust conventions.
216-247: LGTM!
KeyIdenum is well-designed with appropriate constructors and accessors. The past review comment about consolidating duplicate imported key ID accessors appears to have been addressed.
249-258: LGTM!
Displayimplementation forKeyIdprovides clear and consistent formatting.crates/wallet/sdk/src/apis/accounts.rs (3)
107-149: LGTM: add_account refactoring supports both owned and view-only accounts.The refactored signature with
K: Into<KeyIdOrPublicKey>elegantly handles both key-ID-based (owned) and public-key-based (view-only) accounts. The match logic (lines 116-126) correctly retrieves the owner public key from the key manager forKeyIdor uses the provided key directly forPublicKey. The initialization ofassociated_stealth_resourceswithXTR(lines 134-135) provides a sensible default.
236-259: LGTM: get_address_for_account helper centralizes address resolution.The new
get_address_for_accounthelper cleanly resolves theRistrettoOotleAddressby handling bothDerivedandImportedview-only key variants (lines 237-246) and constructing the address fromnetwork,view_only_key, andowner_public_key(lines 247-258). The error handling for the owner public key conversion is clear and descriptive. This refactoring eliminates code duplication across multipleget_*methods and improves maintainability.
162-173: LGTM: Consistent refactoring of get methods to return AccountWithAddress.All the
get_*methods (get_many,get_default,get_account_by_name,get_account_by_address,get_account_by_public_key) now consistently returnAccountWithAddressand use the centralizedget_address_for_accounthelper to resolve addresses. This refactoring ensures uniform behavior and reduces code duplication.Also applies to: 185-192, 194-201, 227-234, 281-292
| let all_used_view_only_keys = self | ||
| .key_manager_api | ||
| .get_all_keys(KeyBranch::ViewOnlyKey)? | ||
| .get_all_derived_keys(KeyBranch::ViewOnlyKey)? | ||
| .into_iter() | ||
| .map(|k| k.key_pair) | ||
| .collect::<Vec<_>>(); | ||
| .map(|view_key| { | ||
| let account_key = self.key_manager_api.derive_account_key( | ||
| view_key | ||
| .key_id | ||
| .derived_index() | ||
| .expect("get_all_derived_keys returns only derived keys"), | ||
| )?; | ||
| Ok::<_, KeyManagerApiError>(AccountAndViewKeys { | ||
| account_public_key: account_key.to_public_key().to_byte_type(), | ||
| account_key: Some(account_key.into()), | ||
| view_only_key: view_key.into(), | ||
| }) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; |
There was a problem hiding this comment.
🧩 Analysis chain
Imported view-only keys are not included in output verification.
The current code only processes derived keys via get_all_derived_keys(KeyBranch::ViewOnlyKey), which excludes imported view-only keys. According to the PR objectives, imported view-only keys should be able to "track (view and sync UTXOs)", but they won't be scanned by this verification flow.
This was previously flagged and marked as addressed in commit a61b1f6, but the current code doesn't include imported keys. This appears to be a regression or incomplete implementation.
Please verify whether imported view-only keys should be included in verify_and_update_outputs. If yes, the code needs to:
- Retrieve imported view-only keys in addition to derived keys
- Handle the
KeyId::Importedvariant without panicking onderived_index().expect(...) - Map imported keys to their corresponding account public keys
Run the following script to check if there's an API to retrieve imported keys and if any test coverage exists:
If imported keys should be included, consider implementing the solution suggested in the previous review or providing a separate method to retrieve all view-only keys (both derived and imported).
🏁 Script executed:
#!/bin/bash
# Check for imported key handling
# Search for methods that might return imported keys
echo "=== Searching for imported key retrieval methods ==="
rg -n "get.*imported.*key" --type rust
# Search for KeyId::Imported usage
echo -e "\n=== Searching for KeyId::Imported usage ==="
rg -n "KeyId::Imported" --type rust
# Check for test coverage of imported keys in stealth outputs
echo -e "\n=== Checking test coverage for imported keys ==="
rg -n "imported.*view.*key" --type rust crates/wallet/sdk/src/apis/stealth_outputs.rs crates/wallet/sdk/tests/Length of output: 1520
Include imported view-only keys in output verification.
Only derived view‐only keys are processed; imported view‐only keys (KeyId::Imported) are omitted and will panic on derived_index().expect(...). Update crates/wallet/sdk/src/apis/stealth_outputs.rs (around lines 355–372) to:
- Retrieve imported view‐only keys via
key_manager_api.get_imported_key(local_key_id) - Handle the
KeyId::Importedvariant without usingexpectonderived_index() - Map imported keys into
AccountAndViewKeysalongside derived keys - Add test coverage to ensure imported view-only keys are picked up in stealth output sync
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/stealth_outputs.rs around lines 355–372, the code
assumes all view-only keys are derived and calls derived_index().expect(...),
which will panic for KeyId::Imported; update the loop to handle both
KeyId::Derived and KeyId::Imported: for derived keys use the existing
derive_account_key flow without expect, and for imported keys call
key_manager_api.get_imported_key(local_key_id) to obtain the corresponding
account/view data and map it into AccountAndViewKeys (setting
account_key/account_public_key appropriately for imported keys), avoid using
expect on derived_index() by matching on the KeyId enum, propagate any
KeyManagerApiError, and add unit tests to cover imported view-only keys being
returned and included in stealth output sync.
| #[derive(Clone)] | ||
| pub struct ImportedWalletKey { | ||
| pub key: RistrettoSecretKey, | ||
| pub import_id: ImportedKeyId, | ||
| pub key_type: KeyType, | ||
| } | ||
|
|
||
| impl ImportedWalletKey { | ||
| pub fn to_public_key(&self) -> RistrettoPublicKey { | ||
| RistrettoPublicKey::from_secret_key(&self.key) | ||
| } | ||
|
|
||
| pub fn as_key_id(&self) -> KeyId { | ||
| KeyId::imported(self.import_id) | ||
| } | ||
| } |
There was a problem hiding this comment.
Make ImportedWalletKey.key private to reduce secret exposure.
The public key field increases the risk of accidental leakage through logging or debugging. Apply the same pattern as WalletKeyRecord.
Apply this diff:
#[derive(Clone)]
pub struct ImportedWalletKey {
- pub key: RistrettoSecretKey,
+ key: RistrettoSecretKey,
pub import_id: ImportedKeyId,
pub key_type: KeyType,
}
impl ImportedWalletKey {
+ pub fn new(key: RistrettoSecretKey, import_id: ImportedKeyId, key_type: KeyType) -> Self {
+ Self { key, import_id, key_type }
+ }
+
+ pub fn secret_key(&self) -> &RistrettoSecretKey {
+ &self.key
+ }
+
pub fn to_public_key(&self) -> RistrettoPublicKey {
RistrettoPublicKey::from_secret_key(&self.key)
}📝 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.
| #[derive(Clone)] | |
| pub struct ImportedWalletKey { | |
| pub key: RistrettoSecretKey, | |
| pub import_id: ImportedKeyId, | |
| pub key_type: KeyType, | |
| } | |
| impl ImportedWalletKey { | |
| pub fn to_public_key(&self) -> RistrettoPublicKey { | |
| RistrettoPublicKey::from_secret_key(&self.key) | |
| } | |
| pub fn as_key_id(&self) -> KeyId { | |
| KeyId::imported(self.import_id) | |
| } | |
| } | |
| #[derive(Clone)] | |
| pub struct ImportedWalletKey { | |
| key: RistrettoSecretKey, | |
| pub import_id: ImportedKeyId, | |
| pub key_type: KeyType, | |
| } | |
| impl ImportedWalletKey { | |
| pub fn new( | |
| key: RistrettoSecretKey, | |
| import_id: ImportedKeyId, | |
| key_type: KeyType, | |
| ) -> Self { | |
| Self { key, import_id, key_type } | |
| } | |
| pub fn secret_key(&self) -> &RistrettoSecretKey { | |
| &self.key | |
| } | |
| pub fn to_public_key(&self) -> RistrettoPublicKey { | |
| RistrettoPublicKey::from_secret_key(&self.key) | |
| } | |
| pub fn as_key_id(&self) -> KeyId { | |
| KeyId::imported(self.import_id) | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/wallet/sdk/src/models/key.rs around lines 42-57, make the secret key
field non-public to avoid accidental leakage: remove the pub modifier on the key
field (keep import_id and key_type as-is), and rely on the existing
to_public_key() and as_key_id() methods for external access; update any call
sites that directly accessed ImportedWalletKey.key to use provided methods or
add a controlled accessor if truly needed.
| #[derive(Clone)] | ||
| pub struct DerivedWalletKey { | ||
| pub key: RistrettoSecretKey, | ||
| pub key_index: DerivedKeyIndex, | ||
| } | ||
|
|
||
| impl DerivedWalletKey { | ||
| pub fn to_public_key(&self) -> RistrettoPublicKey { | ||
| RistrettoPublicKey::from_secret_key(&self.key) | ||
| } | ||
|
|
||
| pub fn as_key_id(&self) -> KeyId { | ||
| KeyId::derived(self.key_index) | ||
| } | ||
| } |
There was a problem hiding this comment.
Make DerivedWalletKey.key private to reduce secret exposure.
The public key field increases the risk of accidental leakage. Apply the same pattern as WalletKeyRecord.
Apply this diff:
#[derive(Clone)]
pub struct DerivedWalletKey {
- pub key: RistrettoSecretKey,
+ key: RistrettoSecretKey,
pub key_index: DerivedKeyIndex,
}
impl DerivedWalletKey {
+ pub fn new(key: RistrettoSecretKey, key_index: DerivedKeyIndex) -> Self {
+ Self { key, key_index }
+ }
+
+ pub fn secret_key(&self) -> &RistrettoSecretKey {
+ &self.key
+ }
+
pub fn to_public_key(&self) -> RistrettoPublicKey {
RistrettoPublicKey::from_secret_key(&self.key)
}🤖 Prompt for AI Agents
In crates/wallet/sdk/src/models/key.rs around lines 59-73, the DerivedWalletKey
struct exposes the secret via its public key field; change "pub key:
RistrettoSecretKey" to a private field "key: RistrettoSecretKey", and add a
restricted accessor such as "pub(crate) fn secret_key(&self) ->
&RistrettoSecretKey" (or another crate-scoped getter matching the
WalletKeyRecord pattern) so external code cannot directly access the secret;
keep the existing to_public_key and as_key_id methods unchanged so public
functionality remains available while reducing secret exposure.
| id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, | ||
| owner_account_id INTEGER NOT NULL, | ||
| resource_address TEXT NOT NULL, | ||
| commitment TEXT NOT NULL, | ||
| value TEXT NOT NULL, | ||
| sender_public_nonce TEXT NOT NULL, | ||
| -- Status can be "Unspent", "Spent", "Locked", "LockedUnconfirmed", "Invalid" | ||
| status TEXT NOT NULL, | ||
| locked_at DATETIME NULL, | ||
| lock_id INTEGER NULL, | ||
| encryption_secret_key_index BIGINT NOT NULL, | ||
| encrypted_data BLOB NOT NULL DEFAULT '', | ||
| tag_byte INTEGER NOT NULL, | ||
| is_burnt BOOLEAN NOT NULL DEFAULT 0, | ||
| is_frozen BOOLEAN NOT NULL DEFAULT 0, | ||
| is_on_chain BOOLEAN NOT NULL, | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| status TEXT NOT NULL, | ||
| locked_at DATETIME NULL, | ||
| lock_id INTEGER NULL, | ||
| view_only_key_id TEXT NOT NULL, | ||
| owner_key_id TEXT NULL, | ||
| encrypted_data BLOB NOT NULL DEFAULT '', | ||
| tag_byte INTEGER NOT NULL, | ||
| is_burnt BOOLEAN NOT NULL DEFAULT 0, | ||
| is_frozen BOOLEAN NOT NULL DEFAULT 0, | ||
| is_on_chain BOOLEAN NOT NULL, | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP |
There was a problem hiding this comment.
Missing foreign key constraint on owner_account_id.
The stealth_outputs.owner_account_id column (line 235) lacks a REFERENCES accounts(id) constraint. Other tables like utxo_process_queue (line 277) and shard_state_versions (line 262) properly define foreign keys with ON DELETE CASCADE. Without this constraint, orphaned records could accumulate if accounts are deleted.
Apply this diff to add the foreign key constraint:
- owner_account_id INTEGER NOT NULL,
+ owner_account_id INTEGER NOT NULL REFERENCES accounts (id) ON DELETE CASCADE,📝 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.
| id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, | |
| owner_account_id INTEGER NOT NULL, | |
| resource_address TEXT NOT NULL, | |
| commitment TEXT NOT NULL, | |
| value TEXT NOT NULL, | |
| sender_public_nonce TEXT NOT NULL, | |
| -- Status can be "Unspent", "Spent", "Locked", "LockedUnconfirmed", "Invalid" | |
| status TEXT NOT NULL, | |
| locked_at DATETIME NULL, | |
| lock_id INTEGER NULL, | |
| encryption_secret_key_index BIGINT NOT NULL, | |
| encrypted_data BLOB NOT NULL DEFAULT '', | |
| tag_byte INTEGER NOT NULL, | |
| is_burnt BOOLEAN NOT NULL DEFAULT 0, | |
| is_frozen BOOLEAN NOT NULL DEFAULT 0, | |
| is_on_chain BOOLEAN NOT NULL, | |
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | |
| status TEXT NOT NULL, | |
| locked_at DATETIME NULL, | |
| lock_id INTEGER NULL, | |
| view_only_key_id TEXT NOT NULL, | |
| owner_key_id TEXT NULL, | |
| encrypted_data BLOB NOT NULL DEFAULT '', | |
| tag_byte INTEGER NOT NULL, | |
| is_burnt BOOLEAN NOT NULL DEFAULT 0, | |
| is_frozen BOOLEAN NOT NULL DEFAULT 0, | |
| is_on_chain BOOLEAN NOT NULL, | |
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | |
| id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT, | |
| owner_account_id INTEGER NOT NULL REFERENCES accounts (id) ON DELETE CASCADE, | |
| resource_address TEXT NOT NULL, | |
| commitment TEXT NOT NULL, | |
| value TEXT NOT NULL, | |
| sender_public_nonce TEXT NOT NULL, | |
| -- Status can be "Unspent", "Spent", "Locked", "LockedUnconfirmed", "Invalid" | |
| status TEXT NOT NULL, | |
| locked_at DATETIME NULL, | |
| lock_id INTEGER NULL, | |
| view_only_key_id TEXT NOT NULL, | |
| owner_key_id TEXT NULL, | |
| encrypted_data BLOB NOT NULL DEFAULT '', | |
| tag_byte INTEGER NOT NULL, | |
| is_burnt BOOLEAN NOT NULL DEFAULT 0, | |
| is_frozen BOOLEAN NOT NULL DEFAULT 0, | |
| is_on_chain BOOLEAN NOT NULL, | |
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP |
🤖 Prompt for AI Agents
In crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql
around lines 234 to 252, the stealth_outputs.owner_account_id column is missing
a foreign key reference to accounts(id); update the table definition to add a
foreign key constraint referencing accounts(id) with ON DELETE CASCADE (either
by changing the owner_account_id column to include "REFERENCES accounts(id) ON
DELETE CASCADE" or by adding a separate "FOREIGN KEY(owner_account_id)
REFERENCES accounts(id) ON DELETE CASCADE" clause) so deleting an account will
cascade and prevent orphaned stealth_outputs.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
applications/tari_walletd/src/handlers/accounts.rs (1)
660-663: Likely inverted condition for new_account flag.NewAccountData should be set when the account is NOT yet on-chain.
Apply this diff:
- account.is_confirmed_on_chain().then(|| NewAccountData { - address: *account.component_address(), - }), + (!account.is_confirmed_on_chain()).then(|| NewAccountData { + address: *account.component_address(), + }),
♻️ Duplicate comments (1)
applications/tari_walletd/src/main.rs (1)
117-117: Field namekey_indexis misleading for KeyId type.As previously noted, the field name
key_indexno longer accurately reflects its content, which is now aKeyIdenum that can be either{ Derived: { index: bigint } }or{ Imported: { local_key_id: bigint } }. This creates a breaking change and misleading API for consumers.The previous review suggested renaming to
view_only_key_idto match the actual type. Please refer to the earlier review comment for the recommended fix.
🧹 Nitpick comments (6)
crates/wallet/sdk/src/apis/substate.rs (1)
66-66: Consider adding documentation.The public method lacks a doc comment. Adding one would improve API discoverability and explain the single-substate retrieval behavior, especially the error case when the network returns an empty response.
Example:
+ /// Retrieves a single substate from the network. + /// + /// This is a convenience method that wraps `get_substates_from_network` for single-substate retrieval. + /// + /// # Errors + /// + /// Returns `SubstateDoesNotExist` if the network does not return the requested substate. pub async fn get_substate_from_network(&self, id: SubstateId) -> Result<Substate, SubstateApiError> {crates/engine/src/wasm/process.rs (1)
34-34: Well-designed refactoring of the validation API.The function signature change from
&LoadedWasmTemplateto&TemplateDefimproves the API by accepting only the required data. Making it public enables reuse across modules while maintaining a clean interface.Consider adding rustdoc to document this public function:
+ /// Validates that the Tari version in the template WASM is compatible with the engine's minimum supported version. + /// + /// # Errors + /// + /// Returns `WasmExecutionError::TemplateVersionMismatch` if the template version is incompatible. pub fn validate_template_tari_version(template_def: &TemplateDef) -> Result<(), WasmExecutionError> {Also applies to: 276-277
crates/engine/tests/templates/buggy/src/lib.rs (1)
46-50: Approve ABI template update; add content validation and auto-generation
- The length-prefix and array size change (59→60 bytes, payload 55→56 bytes) is correct.
- Current tests cover success/error paths but don’t assert the decoded
TemplateDef; add a test to verify the default ABI template’s fields.- To prevent manual drift, generate
_ABI_TEMPLATE_DEFdirectly via the ABI serializer instead of maintaining raw byte arrays.applications/tari_walletd/src/main.rs (1)
76-82: Verify consistency of default cipher seed behavior.The
unwrap_or_default()here contrasts with the explicitunwrap_or(CipherSeedRestore::CreateNewIfRequired)in theSeedWordssubcommand (lines 140-146). While these should be equivalent ifCreateNewIfRequiredis the default implementation, the explicit version is clearer and more maintainable.Consider applying this diff for consistency:
sdk.initialize_cipher_seed( cli.wallet_restore .seed_words .as_ref() .map(CipherSeedRestore::FromSeedWords) - .unwrap_or_default(), + .unwrap_or(CipherSeedRestore::CreateNewIfRequired), )?;crates/wallet/storage_sqlite/src/writer.rs (1)
587-602: Fix typo in comment.The existence check is a good improvement to avoid false NotFound errors when an update doesn't change anything. However, there's a typo in the comment.
Apply this diff to fix the typo:
- // Check if the account exists, because this could have been an update that didnt change anything + // Check if the account exists, because this could have been an update that didn't change anythingapplications/tari_walletd/src/handlers/accounts.rs (1)
994-1008: Avoid redundant network fetch; upsert from the fetched resource instead.fetch_resource already validates and (per comment) caches; the extra get_substate_from_network call is unnecessary I/O.
Apply this diff:
- // Ensure the resource is in the local cache - if !sdk.resources_api().exists(&req.resource_address)? { - let substate = sdk - .substate_api() - .get_substate_from_network(req.resource_address.into()) - .await?; - let resource = substate.into_substate_value().into_resource().ok_or_else(|| { - general_error(format!( - "Indexer returned Substate at address {} is not a resource", - req.resource_address - )) - })?; - sdk.resources_api().upsert_resource(&req.resource_address, &resource)?; - } + // Ensure the resource is in the local cache + if !sdk.resources_api().exists(&req.resource_address)? { + // We already fetched it above; upsert to the local cache without another network call + sdk.resources_api().upsert_resource(&req.resource_address, &resource)?; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
applications/tari_swarm_daemon/src/process_manager/instances/manager.rs(1 hunks)applications/tari_walletd/src/handlers/accounts.rs(15 hunks)applications/tari_walletd/src/main.rs(5 hunks)crates/engine/src/wasm/module.rs(2 hunks)crates/engine/src/wasm/process.rs(3 hunks)crates/engine/tests/templates/buggy/src/lib.rs(1 hunks)crates/engine/tests/test.rs(1 hunks)crates/wallet/sdk/src/apis/substate.rs(1 hunks)crates/wallet/sdk_services/src/account_monitor/handle.rs(1 hunks)crates/wallet/sdk_services/src/account_monitor/mod.rs(1 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(1 hunks)crates/wallet/sdk_services/src/account_monitor/scanner.rs(10 hunks)crates/wallet/sdk_services/src/events.rs(4 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(6 hunks)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs(10 hunks)crates/wallet/sdk_services/src/utxo_scanner/worker.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(16 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/engine/tests/test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/tari_swarm_daemon/src/process_manager/instances/manager.rs
🧰 Additional context used
🧬 Code graph analysis (13)
crates/engine/src/wasm/module.rs (1)
crates/engine/src/wasm/process.rs (1)
validate_template_tari_version(276-290)
applications/tari_walletd/src/main.rs (3)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/models/account.rs (4)
address(25-27)address(79-81)view_only_key_id(29-31)view_only_key_id(87-89)crates/wallet/sdk/src/models/key.rs (1)
secret(91-93)
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (1)
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(25-27)
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (3)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(49-66)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(62-66)new(119-126)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (5)
bindings/src/types/UtxoSpent.ts (1)
UtxoSpent(4-4)bindings/src/types/UtxoUnspent.ts (1)
UtxoUnspent(5-5)bindings/src/types/WalletUtxoUpdate.ts (1)
WalletUtxoUpdate(6-6)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)
applications/tari_walletd/src/handlers/accounts.rs (3)
crates/wallet/sdk/src/sdk.rs (2)
accounts_api(166-173)network(131-133)applications/tari_walletd/src/handlers/helpers.rs (1)
get_account_by_key_index(125-138)crates/wallet/sdk/src/models/account.rs (2)
account(71-73)view_only_public_key(99-101)
crates/wallet/sdk_services/src/events.rs (2)
bindings/src/types/UtxoAddress.ts (1)
UtxoAddress(4-4)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (4)
crates/wallet/sdk_services/src/account_monitor/scanner.rs (2)
new(49-51)refresh_account(53-155)crates/wallet/sdk/src/sdk.rs (1)
accounts_api(166-173)crates/wallet/sdk_services/src/account_monitor/handle.rs (1)
refresh_account(26-37)crates/wallet/sdk_services/src/events.rs (9)
from(23-25)from(29-31)from(35-37)from(41-43)from(47-49)from(53-55)from(59-61)from(65-67)from(71-73)
crates/wallet/sdk_services/src/account_monitor/scanner.rs (3)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (2)
new(63-85)refresh_account(171-192)crates/wallet/sdk_services/src/account_monitor/handle.rs (1)
refresh_account(26-37)crates/wallet/sdk/src/sdk.rs (2)
accounts_api(166-173)resources_api(175-177)
crates/wallet/sdk_services/src/account_monitor/handle.rs (3)
applications/tari_walletd/src/handlers/context.rs (1)
account_monitor(86-88)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
refresh_account(171-192)crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
refresh_account(53-155)
crates/engine/src/wasm/process.rs (3)
bindings/src/types/TemplateDef.ts (1)
TemplateDef(4-4)crates/engine/tests/test.rs (3)
module(117-122)module(126-131)module(216-221)crates/engine/src/wasm/module.rs (1)
template_def(167-169)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (6)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/UtxoAddress.ts (1)
UtxoAddress(4-4)bindings/src/types/UtxoId.ts (1)
UtxoId(3-3)crates/wallet/sdk_services/src/notify.rs (1)
notify(21-23)crates/wallet/sdk/src/models/account.rs (3)
account(71-73)address(25-27)address(79-81)
crates/wallet/storage_sqlite/src/writer.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/storage.rs (6)
key_manager_insert_imported_key(330-335)confidential_outputs_lock_smallest_amount(415-419)confidential_outputs_insert(420-420)confidential_outputs_finalize_by_lock_id(422-422)confidential_outputs_release_by_lock_id(424-424)utxo_process_queue_extend(481-485)crates/wallet/storage_sqlite/src/reader.rs (22)
key_type(183-184)key_manager_imported_keys(167-173)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(563-566)accounts(597-600)accounts(728-731)accounts(775-778)accounts(798-801)accounts(902-905)accounts(939-942)confidential_outputs(687-691)confidential_outputs(704-706)confidential_outputs(756-766)confidential_outputs(804-807)utxo_process_queue(1286-1291)crates/wallet/sdk/src/models/account.rs (8)
view_only_key_id(29-31)view_only_key_id(87-89)owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)address(25-27)address(79-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: fmt
- GitHub Check: clippy
🔇 Additional comments (42)
crates/wallet/sdk/src/apis/substate.rs (1)
66-70: LGTM! Clean convenience wrapper.The implementation correctly wraps the multi-substate method for single-item retrieval. The
clone()on line 67 is necessary sinceidis moved into the error on line 69, and usingremoverather thanget().cloned()is actually more efficient since the map is owned and unused afterward.crates/engine/src/wasm/module.rs (1)
49-49: LGTM! Version validation added to template loading.The addition of
WasmProcess::validate_template_tari_version(&template)appropriately validates template compatibility early in the loading pipeline, before instance and function validation. The import and placement are correct.Also applies to: 92-93
crates/engine/src/wasm/process.rs (1)
81-81: LGTM! Callsite correctly updated.The call to
validate_template_tari_versionproperly passesmodule.template_def()to match the refactored signature.applications/tari_walletd/src/main.rs (2)
93-101: LGTM: Account creation properly handles view-only and owner keys.The separation of
view_only_key_idandowner_key_idcorrectly supports the new view-only account feature. Theis_defaultcalculation (first account becomes default) is a sensible heuristic.
140-146: LGTM: Explicit cipher seed default is clear.The explicit use of
CipherSeedRestore::CreateNewIfRequiredmakes the intent immediately clear compared to relying on the default implementation.crates/wallet/sdk_services/src/transaction_service/service.rs (1)
309-312: LGTM! Appropriate no-op handling for new UTXO recovery events.The transaction service correctly treats the new UTXO recovery lifecycle events as no-ops, as it only needs to respond to transaction-related events.
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (1)
188-195: LGTM! Clean separation between scanning and recovery with conditional notification.The refactored flow correctly:
- Enqueues discovered UTXOs for later recovery
- Returns the count of found UTXOs
- Notifies the recovery worker only when there's actual work to process
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
29-44: LGTM! Function correctly refactored for KeyId-based key management.The changes properly:
- Rename the function to reflect enqueue-based semantics
- Update view key retrieval to use
account.view_only_key_id()instead of key index- Return the count of discovered UTXOs for upstream notification logic
crates/wallet/sdk_services/src/events.rs (3)
17-19: LGTM! Clean addition of UTXO recovery lifecycle events.The three new event variants properly model the recovery workflow: started, individual recovery, and completion.
58-74: LGTM! Proper event conversion implementations.The
Fromimplementations correctly convert each event type into theWalletEventenum.
113-128: LGTM! Well-structured event definitions.The event structs contain appropriate fields:
UtxoRecoveredEvent: identifies the recovered UTXO and its accountUtxoRecoveryStartedEvent/CompletedEvent: track rounds with recovery countscrates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (7)
49-52: LGTM! Clean builder pattern for optional event notification.The
with_notifymethod allows callers to optionally attach event broadcasting, maintaining flexibility for different use cases.
87-91: LGTM! Safe notification helper.The helper correctly handles the optional
notifyfield, silently skipping notification when not configured.
105-115: Clarify: Why publish start/complete events for an empty initial round?When
round_id == 0and the batch is empty, the code publishes bothUtxoRecoveryStartedEventandUtxoRecoveryCompletedEventwithnum_recovered = 0. Is this intentional to signal that the initial scan completed with no UTXOs found, or could this logic be simplified?Consider clarifying the intent with a comment, e.g.:
if self.round_id == 0 { + // Publish events for initial scan completion, even if no UTXOs were found self.notify(UtxoRecoveryStartedEvent {
232-245: LGTM! Past review concern has been properly addressed.The code now correctly handles both owner and view-only accounts:
- Fetches the account record by address (line 232)
- Retrieves the view key via
account.view_only_key_id()(lines 233-236)- Conditionally retrieves the owner key only when
account.owner_key_id()isSome(lines 237-240)- Constructs
AccountAndViewKeyswith optionalaccount_keyThis fixes the previous issue where
get_account_owner_key(found.view_key_id)would fail for view-only accounts.Based on past review comment that flagged incorrect key derivation for view-only accounts.
177-183: LGTM! Correct mapping to account address instead of key index.The change from
view_key_indextoaccount_addrproperly aligns with the ComponentAddress-based account model.
261-271: LGTM! Proper notification for UTXO status updates.The code correctly:
- Updates the UTXO status
- Checks if the UTXO exists via
.optional()?.is_some()- Notifies only when the UTXO was found and updated
285-289: LGTM! Proper notification and logging for recovered UTXOs.The notification correctly includes both the UTXO address and account address, and the log references the composite
keys.account_public_key.crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (4)
33-33: LGTM! Correct type update from DerivedKey to Key.The view key type change aligns with the broader key management refactoring to use the
Keytype that includes both public and secret components.Also applies to: 53-53
68-77: LGTM! Function now returns UTXO count for upstream notification.The change from
Result<(), ...>toResult<usize, ...>allows the caller to know how many UTXOs were discovered, enabling conditional notification logic in the worker.
146-146: LGTM! Correct usage of component address instead of key index.The change from key index to
component_address()properly aligns with the ComponentAddress-based account model.
210-210: LGTM! Correct field access for Key type.The change from
view_key.keytoview_key.secretaligns with the newKeystructure that separates public and secret components.crates/wallet/storage_sqlite/src/writer.rs (7)
31-33: LGTM!The new imports for
ImportedKeyId,KeyId, andKeyTypeare correctly added to support the key management refactoring.
280-302: LGTM!The new
key_manager_insert_imported_keyfunction correctly implements imported key storage with proper error handling and ID retrieval.
522-557: LGTM!The updated
accounts_insertfunction correctly handles the new key management fields with proper serialization. The logic for managing default accounts is preserved.
867-953: LGTM!The renamed and updated
confidential_outputs_lock_smallest_amountfunction correctly filters for spendable outputs by checkingowner_key_id.is_not_null()at line 884. This ensures view-only accounts cannot lock outputs for spending, which is the correct behavior.
955-988: LGTM!The
confidential_outputs_insertfunction correctly serializes the new key fields (view_only_key_idandowner_key_id) to JSON, maintaining consistency with the reader functions.
1045-1134: LGTM!The stealth output functions correctly implement the same key management pattern as confidential outputs:
- Line 1074: Filters for spendable outputs with
owner_key_id.is_not_null()- Lines 1120-1121: Properly serializes the key fields
The implementation is consistent across both output types.
1481-1508: LGTM!The refactored
utxo_process_queue_extendfunction correctly usesComponentAddressinstead of a raw index. The subquery at lines 1492-1497 to resolveaccount_idfrom the address is the correct approach for this refactoring. The use ofon_conflict_do_nothingindicates idempotent design, which is appropriate for a processing queue.Note: The subquery executes for each item in the iterator. For large batches, consider if bulk operations would provide better performance, though the current approach is functionally correct.
applications/tari_walletd/src/handlers/accounts.rs (13)
37-38: Types import relocation looks correct.
164-169: Create-or-get: key_index lookup path is correct.
188-195: Create-or-get: using derived address for creation is correct.
253-255: List now echoes stored account/address; good.
275-276: Balance refresh uses refresh_account_with_utxos; good.
420-423: Claim burn: enforce owner_key_id presence.
480-495: Claim burn: correct use of owner/view-only keys for encryption/tagging.
573-579: Free test coins: enforce owner_key_id presence.
625-647: Free test coins: sign with account owner key; OK.
717-720: Transfer: enforce owner_key_id presence.
805-835: Transfer: sign with account owner key; OK.
1014-1015: Post-association balance refresh: OK.
116-123: create_account signature confirmed
AccountsApi::create_account takesOption<&str>,bool, andWalletOotleAddressWithKeyIds, matching the call withowner_address.
| if let Some(index) = account_address.owner_key_id.derived_index() { | ||
| km.set_active_key(KeyBranch::Account, index)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify handling of imported keys when --set-active is requested.
When owner_key_id is an imported key without a derived index, the active key won't be set even if the user specified --set-active. This silent failure could cause confusion.
Consider one of these approaches:
- Log a warning when
set_activeis true but the key has no derived index - Return an error if imported keys cannot be set as active
- Document this limitation in the command help text
Run this script to check if imported keys are supported in the CreateAccount flow:
🏁 Script executed:
#!/bin/bash
# Description: Check how imported keys are handled in account creation
# Search for imported key handling in account creation
rg -nP -C5 'owner_key_id.*Imported|Imported.*owner_key_id' --type=rust
# Search for derived_index usage and fallback handling
rg -nP -C3 'derived_index.*None|if let Some.*derived_index' --type=rust -g '**/main.rs' -g '**/account*.rs'Length of output: 522
Handle imported keys when --set-active is used
When owner_key_id.derived_index() returns None for an imported key, the active account key isn’t set despite --set-active. Provide user feedback:
- Emit a warning if
--set-activeis true butderived_index()returnsNone. - Or return an error in this case.
- Alternatively, document this limitation in the command help text.
| Ok(event) = self.notify_subscription.recv() => { | ||
| if let Err(e) = self.on_event(event).await { | ||
| error!(target: LOG_TARGET, "Error handling event: {}", e); | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
Handle broadcast receiver errors to keep event stream alive.
Using Ok(event) = self.notify_subscription.recv() silently disables this branch after the first Lagged or Closed error, so the monitor stops processing every future wallet event. We must match on the result, handle lagged messages, and shutdown cleanly on Closed.
- Ok(event) = self.notify_subscription.recv() => {
- if let Err(e) = self.on_event(event).await {
- error!(target: LOG_TARGET, "Error handling event: {}", e);
- }
- },
+ recv_result = self.notify_subscription.recv() => {
+ match recv_result {
+ Ok(event) => {
+ if let Err(e) = self.on_event(event).await {
+ error!(target: LOG_TARGET, "Error handling event: {}", e);
+ }
+ },
+ Err(tokio::sync::broadcast::error::RecvError::Lagged(skipped)) => {
+ warn!(target: LOG_TARGET, "Wallet event stream lagged by {} messages; continuing with latest", skipped);
+ },
+ Err(tokio::sync::broadcast::error::RecvError::Closed) => {
+ info!(target: LOG_TARGET, "Wallet event stream closed; shutting down account monitor");
+ break Ok(());
+ },
+ }
+ },🤖 Prompt for AI Agents
In crates/wallet/sdk_services/src/account_monitor/monitor.rs around lines 118 to
123, the code uses a pattern that ignores errors from
self.notify_subscription.recv(), which causes the branch to stop being executed
after the first Lagged or Closed error; change the match to handle the Result
from recv() explicitly: on Ok(event) call self.on_event(event).await and log
errors as before; on Err(broadcast::RecvError::Lagged(n)) log a warning with n
and continue the loop so the stream stays alive; on
Err(broadcast::RecvError::Closed) perform a clean shutdown of the monitor
(break/return) and log the closure. Ensure you import/qualify
tokio::sync::broadcast::RecvError if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
234-246: AddON DELETE CASCADEtostealth_outputs.owner_account_id.
owner_account_idnow referencesaccounts(id), but withoutON DELETE CASCADEan account deletion will either fail (foreign-key violation) or require brittle manual cleanup, leaving the system unable to reliably drop accounts with associated stealth outputs. Please extend the constraint to cascade deletes so referential integrity is preserved automatically.- owner_account_id INTEGER NOT NULL REFERENCES accounts (id), + owner_account_id INTEGER NOT NULL REFERENCES accounts (id) ON DELETE CASCADE,
🧹 Nitpick comments (2)
crates/wallet/crypto/src/encryption.rs (2)
136-152: Consider simplifying the authentication scheme.The code uses both ChaCha20-Poly1305's AEAD tag (line 140-142, 149-151) and a separate MAC over the plaintext (line 107, 85-97). While defense in depth can be valuable, this dual authentication is non-standard and adds complexity. The AEAD tag alone provides authenticated encryption and is sufficient for most use cases.
If the additional MAC serves a specific purpose (e.g., binding plaintext to version/salt for future compatibility), document the rationale. Otherwise, consider relying solely on the AEAD tag to simplify the implementation.
217-297: Expand test coverage for robustness.The existing tests cover basic success/failure paths. Consider adding:
- A test with an incorrect password to verify authentication failure
- A test confirming that encrypting the same data with the same password produces different ciphertexts (due to random salts)
- Edge cases such as empty data or very large payloads
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/wallet/crypto/src/encryption.rs(1 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(6 hunks)
🔇 Additional comments (5)
crates/wallet/crypto/src/encryption.rs (5)
13-28: LGTM! Constants are well-defined.The constants are appropriately sized and clearly documented. The 16-byte salt and Argon2 parameters align with OWASP recommendations.
30-98: LGTM! Decryption logic is sound.The decryption flow correctly validates version and checksum, derives keys from the salt, decrypts with ChaCha20-Poly1305, and verifies the MAC in constant time. The use of
Zeroizingfor sensitive buffers is good practice.
100-134: Verify that plaintext is not leaked if encryption fails.The encryption flow assembles plaintext and MAC into
encrypted_buf(lines 112-114) before encrypting at line 125. Ifencipherfails, the buffer containing plaintext in cleartext is dropped without explicit zeroization. While Rust's ownership ensures the buffer is dropped, consider wrapping the plaintext portion inZeroizingor handling the failure path more carefully to prevent potential memory leaks of sensitive data.
154-181: LGTM! Key derivation follows OWASP recommendations.The use of Argon2id with recommended parameters (46 MiB memory, 1 iteration) provides strong password-based key derivation. The Argon2 salt is properly derived from the main salt, and keys are stored in a zeroizing
SafeArray.
194-206: LGTM! Helper functions and error type are appropriate.The domain-separated hashers and simple error type are well-designed for this module.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (2)
118-122: Handle broadcast recv errors to keep monitor alive.This is the pattern we already flagged:
Ok(event) = recv()aborts the branch on the firstLaggedorClosederror, so the monitor stops reacting to every later wallet event. Please match on the result, warn on lag, and shut down cleanly onClosed.- Ok(event) = self.notify_subscription.recv() => { - if let Err(e) = self.on_event(event).await { - error!(target: LOG_TARGET, "Error handling event: {}", e); - } - }, + recv_result = self.notify_subscription.recv() => { + match recv_result { + Ok(event) => { + if let Err(e) = self.on_event(event).await { + error!(target: LOG_TARGET, "Error handling event: {}", e); + } + }, + Err(broadcast::error::RecvError::Lagged(skipped)) => { + warn!(target: LOG_TARGET, "Wallet event stream lagged by {} messages; continuing", skipped); + }, + Err(broadcast::error::RecvError::Closed) => { + info!(target: LOG_TARGET, "Wallet event stream closed; shutting down account monitor"); + break Ok(()); + }, + } + },
146-168: Refresh every account, not just the first 100.We still only page once with
get_many(0, 100), so anything past the first page never refreshes or queues stealth scans. Please paginate through the full account list as already discussed.- // TODO: There could be more than 100 accounts - let accounts = accounts_api.get_many(0, 100)?; - for account in accounts { + const PAGE_SIZE: usize = 100; + let mut offset = 0; + loop { + let accounts = accounts_api.get_many(offset, PAGE_SIZE)?; + if accounts.is_empty() { + break; + } + for account in &accounts { + let is_updated = self.scanner.refresh_account(*account.component_address()).await?; + if self.enable_periodic_scanning_with_utxos { + self.refresh_stealth_utxos(*account.component_address()).await?; + } + + if is_updated { + info!( + target: LOG_TARGET, + "👁️🗨️ Account {} has been updated", account + ); + } else { + info!( + target: LOG_TARGET, + "👁️🗨️ Account {} is up to date", account + ); + } + } + offset += accounts.len(); + } - let is_updated = self.scanner.refresh_account(*account.component_address()).await?; - if self.enable_periodic_scanning_with_utxos { - self.refresh_stealth_utxos(*account.component_address()).await?; - } - - if is_updated { - info!( - target: LOG_TARGET, - "👁️🗨️ Account {} has been updated", account - ); - } else { - info!( - target: LOG_TARGET, - "👁️🗨️ Account {} is up to date", account - ); - } - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
applications/tari_walletd/src/services/mod.rs(2 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(1 hunks)crates/wallet/sdk_services/src/events.rs(4 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(1 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs(2 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(9 hunks)crates/wallet/sdk_services/src/utxo_scanner/worker.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/wallet/sdk_services/src/transaction_service/service.rs
🧰 Additional context used
🧬 Code graph analysis (6)
crates/wallet/sdk_services/src/events.rs (2)
bindings/src/types/UtxoAddress.ts (1)
UtxoAddress(4-4)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (3)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
notify(87-91)new(41-47)crates/wallet/sdk_services/src/notify.rs (2)
notify(21-23)new(12-15)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(30-32)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (7)
applications/tari_walletd/src/handlers/context.rs (3)
account_monitor(86-88)wallet_sdk(64-66)shutdown_signal(82-84)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(30-32)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (3)
new(62-66)new(120-128)run(81-102)crates/wallet/sdk_services/src/account_monitor/scanner.rs (2)
new(49-51)refresh_account(53-155)crates/wallet/sdk/src/sdk.rs (1)
accounts_api(166-173)crates/wallet/sdk_services/src/account_monitor/handle.rs (1)
refresh_account(26-37)crates/wallet/sdk_services/src/events.rs (10)
from(24-26)from(30-32)from(36-38)from(42-44)from(48-50)from(54-56)from(60-62)from(66-68)from(72-74)from(78-80)
applications/tari_walletd/src/services/mod.rs (2)
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(30-32)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(62-66)new(120-128)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (10)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/UtxoSpent.ts (1)
UtxoSpent(4-4)bindings/src/types/UtxoUnspent.ts (1)
UtxoUnspent(5-5)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/UtxoAddress.ts (1)
UtxoAddress(4-4)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
notify(87-91)new(41-47)crates/wallet/sdk_services/src/notify.rs (2)
notify(21-23)new(12-15)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(30-32)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(62-66)new(120-128)
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (5)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
notify(87-91)new(41-47)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
new(63-85)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(55-75)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(62-66)new(120-128)crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
new(49-51)
⏰ 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: machete
- GitHub Check: file licenses
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: fmt
🔇 Additional comments (2)
applications/tari_walletd/src/services/mod.rs (2)
4-5: LGTM!The module addition is correctly placed and follows standard Rust conventions.
51-51: LGTM!The updated constructor call correctly passes the
notifyparameter, which is consistent with the new signature ofStealthUtxoScannerWorker::newand follows the same pattern used for other service initializations in this function.
b6601f8 to
b9d1f88
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
118-123: Handle broadcast receiver errors so the monitor keeps running.
Ok(event) = self.notify_subscription.recv()disables this branch permanently after the firstLaggedorClosederror, so the monitor stops reacting to all future wallet events. Handle the fullResultinstead, logging lagged skips and breaking cleanly on closure.- Ok(event) = self.notify_subscription.recv() => { - if let Err(e) = self.on_event(event).await { - error!(target: LOG_TARGET, "Error handling event: {}", e); - } - }, + recv_result = self.notify_subscription.recv() => { + match recv_result { + Ok(event) => { + if let Err(e) = self.on_event(event).await { + error!(target: LOG_TARGET, "Error handling event: {}", e); + } + }, + Err(broadcast::error::RecvError::Lagged(skipped)) => { + warn!( + target: LOG_TARGET, + "Wallet event stream lagged by {} messages; continuing with latest", + skipped + ); + }, + Err(broadcast::error::RecvError::Closed) => { + info!(target: LOG_TARGET, "Wallet event stream closed; shutting down account monitor"); + break Ok(()); + }, + } + },
🧹 Nitpick comments (1)
crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
706-708: Manual XTR input remains required. The transaction builder doesn’t auto-include XTR fee inputs, so.add_input(XTR)is still needed—retain the TODO or file an issue to support automatic fee input inclusion in the builder.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
applications/tari_walletd/src/handlers/accounts.rs(15 hunks)crates/wallet/sdk/src/apis/accounts.rs(7 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(10 hunks)crates/wallet/sdk/src/models/account.rs(2 hunks)crates/wallet/sdk/src/storage.rs(10 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(1 hunks)crates/wallet/storage_sqlite/src/reader.rs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
applications/tari_walletd/src/handlers/accounts.rs (4)
crates/wallet/sdk/src/sdk.rs (2)
accounts_api(166-173)network(131-133)applications/tari_walletd/src/handlers/helpers.rs (1)
get_account_by_key_index(125-138)crates/wallet/storage_sqlite/src/reader.rs (11)
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(563-566)accounts(597-600)accounts(728-731)crates/wallet/sdk/src/models/account.rs (3)
address(79-81)account(71-73)view_only_public_key(99-101)
crates/wallet/sdk/src/storage.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/storage_sqlite/src/reader.rs (7)
key_manager_get_raw_imported_key(163-192)accounts_get_many(399-413)confidential_outputs_get_unspent_balance(672-695)confidential_outputs_get_locked_by_lock_id(697-747)confidential_outputs_get_by_commitment(749-789)confidential_outputs_get_by_account_and_status(791-829)key_type(183-184)crates/wallet/storage_sqlite/src/writer.rs (6)
key_manager_insert_imported_key(280-302)confidential_outputs_lock_smallest_amount(867-953)confidential_outputs_insert(955-988)confidential_outputs_finalize_by_lock_id(990-1018)confidential_outputs_release_by_lock_id(1020-1043)utxo_process_queue_extend(1481-1508)crates/wallet/sdk/src/models/account.rs (6)
view_only_key_id(29-31)view_only_key_id(87-89)owner_key_id(33-35)owner_key_id(91-93)owner_public_key(37-39)owner_public_key(95-97)
crates/wallet/storage_sqlite/src/reader.rs (5)
bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)crates/wallet/sdk/src/storage.rs (6)
key_manager_get_raw_imported_key(155-155)accounts_get_many(182-182)confidential_outputs_get_unspent_balance(212-212)confidential_outputs_get_locked_by_lock_id(213-216)confidential_outputs_get_by_commitment(217-221)confidential_outputs_get_by_account_and_status(223-227)bindings/src/types/VaultId.ts (1)
VaultId(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)crates/wallet/storage_sqlite/src/writer.rs (5)
accounts(589-593)accounts(631-634)accounts(896-899)accounts(958-961)accounts(1056-1059)
crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
crates/wallet/sdk/src/models/account.rs (5)
component_address(25-27)component_address(75-77)owner_key_id(33-35)owner_key_id(91-93)account(71-73)
crates/wallet/sdk/src/apis/accounts.rs (6)
crates/ootle_address/src/ootle_address.rs (5)
network(40-42)network(224-226)account_public_key(48-50)view_only_key(44-46)view_only_key(228-230)crates/wallet/sdk/src/sdk.rs (3)
network(131-133)store(119-121)key_manager_api(144-152)crates/wallet/sdk/src/models/key.rs (8)
from(76-81)from(101-106)from(110-115)from(119-124)from(128-133)from(205-207)from(211-213)key_id(22-24)clients/wallet_daemon_client/src/types.rs (1)
from(919-927)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(91-103)applications/tari_walletd/src/handlers/accounts.rs (1)
accounts(254-263)
crates/wallet/sdk/src/models/account.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/ComponentAddress.ts (1)
ComponentAddress(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (6)
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (3)
new(62-66)new(120-128)run(81-102)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(30-32)crates/wallet/sdk_services/src/account_monitor/scanner.rs (2)
new(49-51)refresh_account(53-155)crates/wallet/sdk/src/sdk.rs (1)
accounts_api(166-173)crates/wallet/sdk_services/src/account_monitor/handle.rs (1)
refresh_account(26-37)crates/wallet/sdk_services/src/events.rs (10)
from(24-26)from(30-32)from(36-38)from(42-44)from(48-50)from(54-56)from(60-62)from(66-68)from(72-74)from(78-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: check nightly
- GitHub Check: clippy
- GitHub Check: fmt
🔇 Additional comments (25)
crates/wallet/sdk/src/models/account.rs (3)
10-10: LGTM: KeyId import added for new fields.The import is correctly added to support the new KeyId-based fields in the Account struct.
17-39: LGTM: View-only and owner key fields properly structured.The new fields
view_only_key_id,owner_key_id, andowner_public_keycorrectly support the view-only account feature. Accessor methods follow appropriate patterns with value copies for KeyId and references for byte arrays.
95-97: Verify the delegation toaddress.account_public_key()instead ofaccount.owner_public_key.Line 96 returns
self.address.account_public_key()rather thanself.account.owner_public_key. While this may be intentional if the address is the authoritative source for the owner key, please confirm this is the desired behavior for consistency with theAccountstruct'sowner_public_key()method at line 37.crates/wallet/storage_sqlite/src/reader.rs (5)
37-37: LGTM: Import additions support key management and resource type features.The additions of
KeyTypeandResourceTypeto the imports are necessary for the new imported key functionality and resource type filtering.Also applies to: 54-54
163-192: LGTM: Imported key retrieval correctly implemented.The
key_manager_get_raw_imported_keymethod properly queries the database, parses the key type, and returns the encrypted key data with appropriate error handling.
399-413: LGTM: Parameter types changed to idiomaticusize.The change from
u64tousizeforoffsetandlimitparameters is more idiomatic for collection indexing in Rust and aligns with the updated trait signature.
672-829: LGTM: Output methods consistently renamed toconfidential_outputs_*.The renaming of the four output-related methods from
outputs_*toconfidential_outputs_*provides clearer semantics and aligns with the updated storage trait interface.
1279-1316: SDK wrapper and utxo_recovery handle ComponentAddress
Both the SDK trait signature and theprocess_utxo_validation_queueusage consumeComponentAddresscorrectly.crates/wallet/sdk/src/storage.rs (4)
38-40: LGTM: Import additions support key management refactor.The additions of
ImportedKeyId,KeyId, andKeyTypeare necessary for the new key management methods and KeyId-based API changes throughout the trait interfaces.
118-121: LGTM: Encryption/decryption error variants added.The new
EncryptionErrorandDecryptionErrorvariants follow the established pattern for error types and will support proper error handling in key encryption operations.
155-155: LGTM: Reader trait interface updated consistently.The reader trait changes align with the implementation:
- New
key_manager_get_raw_imported_keymethod for imported key supportaccounts_get_manyparameter types changed tousize- Output methods renamed to
confidential_outputs_*for clarityutxo_process_queue_fetch_batchreturn type updated to useComponentAddressAlso applies to: 182-182, 211-227, 311-311
330-335: LGTM: Writer trait interface updated for key management and view-only accounts.The writer trait changes properly support the new features:
key_manager_insert_imported_keyenables storing imported keysaccounts_insertsignature updated with KeyId-based parameters and stealth resources- Output methods renamed to
confidential_outputs_*utxo_process_queue_extendnow acceptsComponentAddressinstead of numeric indicesAlso applies to: 376-382, 415-424, 481-485
crates/wallet/sdk/src/apis/accounts.rs (8)
9-9: LGTM: Import additions support view-only account features.The new imports are properly added to support:
- Address type conversions (
FromByteType,RistrettoOotleAddress)- Network-aware address generation (
Network)- Default stealth resources (
XTR)- KeyId-based key management (
KeyId,KeyIdOrPublicKey,WalletOotleAddressWithKeyIds)Also applies to: 12-12, 16-16, 21-21, 31-40
46-46: LGTM: Network field properly integrated into AccountsApi.The
networkfield is correctly added to the struct and properly initialized through the constructor. This is necessary for network-aware address generation inget_address_for_account.Also applies to: 57-68
75-105: LGTM:create_accountproperly refactored with correct owner key.The method has been successfully refactored to:
- Accept
WalletOotleAddressWithKeyIdsfor key ID references- Derive the component address from the account public key
- Delegate to
add_accountfor insertion logic- Correctly populate
owner_public_keywith actual key bytes (line 99), addressing previous review comments
107-149: LGTM:add_accountflexibly handles owner key resolution.The method properly:
- Accepts either
KeyIdorRistrettoPublicKeyBytesvia the generic parameter- Resolves
KeyIdto the public key throughkey_manager_api- Initializes
associated_stealth_resourceswith XTR by default- Calls
accounts_insertwith the updated signature
228-251: LGTM:get_address_for_accountcentralizes address resolution.The helper method correctly:
- Resolves both
DerivedandImportedview-only key IDs- Constructs
RistrettoOotleAddresswith the network context- Converts the owner public key from byte type with proper error handling
- Provides a single point of logic for address resolution used by multiple methods
162-165: Verify the intended return type forget_many.The method now returns
Vec<Account>instead of resolving addresses for each account. The AI summary indicates it should returnVec<AccountWithAddress>, but the code returnsVec<Account>. If addresses are needed, callers must now callget_address_for_accountseparately for each account, which could be less efficient. Please confirm this is the intended behavior.
177-184: LGTM: Consistent address resolution across account methods.The methods
get_default,get_account_by_name,get_account_by_address, andget_account_by_public_keyall consistently use theget_address_for_accounthelper to resolve addresses and returnAccountWithAddress.Also applies to: 186-193, 219-226, 273-284
253-261: LGTM:associate_stealth_resourcemethod added.The new method properly enables associating stealth resources with accounts, delegating to the storage layer with appropriate error handling.
crates/wallet/sdk/src/apis/stealth_transfer.rs (5)
120-120: LGTM! Component address usage is consistent.The migration from
address()tocomponent_address()for vault lookups and error messages is correct and aligns with the broader refactoring to component-address-based account identification.Also applies to: 131-131, 164-164, 218-218, 225-225, 279-279, 309-309
542-564: LGTM! Revealed input handling correctly includes fee inputs.The condition now properly accounts for both main transfer and fee transfer revealed inputs when determining whether to include the owner account as a substate input. This ensures the account and its vaults are available for
withdrawcalls when spending revealed funds for either the transfer or fees.
638-652: LGTM! Owner key validation and signer selection logic is correct.The implementation properly:
- Validates that the account has an owner key (preventing view-only accounts from transferring, which is correct since they cannot spend UTXOs)
- Selects the appropriate signer based on whether revealed inputs are present:
- Revealed inputs require account authorization → sign with account owner key
- Confidential-only inputs don't require account authorization → sign with throwaway nonce
The error message clearly communicates the constraint.
658-669: LGTM! Fee transfer handling correctly branches on revealed amount.The logic appropriately distinguishes between paying fees from revealed account balance versus confidential inputs, matching the transaction signer selection and substate input requirements.
778-779: LGTM! Output model correctly uses separated key identifiers.The change from
encryption_secret_key_indexto separateview_only_key_idandowner_key_idfields correctly implements the view-only account architecture, enabling accounts to track outputs using the view key while requiring the owner key for spending operations.
2 participants
Description
feat(wallet)!: view only account SDK support
fix(wallet): transfer bug when sending non-XTR stealth funds
fix(wallet): update web UI to use KeyId
fix(wallet SDK): import and encrypt keys
Motivation and Context
Added SDK support for importing view-only keys. This allows a wallet to add accounts that it can track (view, sync utxos) but not spend/mutate.
How Has This Been Tested?
Manually, new unit tests, WIP cli wallet
Breaking Changes
Summary by CodeRabbit
New Features
Changes
Tests