feat(wallet/sdk)!: pluggable key store backend - #1649
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors the wallet SDK to a spec-driven WalletSdkSpec, consolidates key-management and signing APIs (KeyId/DerivedKeyId, SignerApi, KeyManagerApi), adds genesis resource helpers, updates many handlers/services/tests to the new APIs, adjusts TypeScript bindings, and removes Templates UI and a stale Vite config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant WalletSdk as WalletSdk<TSpec>
participant KeyMgr as KeyManagerApi<TSpec>
participant Signer as SignerApi<TSpec>
participant KeyStore as TSpec::KeyStore
Client->>WalletSdk: initialize_with_local_key_store(...)
WalletSdk->>KeyStore: store cipher seed / use LocalKeyStore
Client->>WalletSdk: key_manager_api()
WalletSdk-->>KeyMgr: provide KeyManagerApi<TSpec>
Client->>KeyMgr: get_key(key_id)
KeyMgr->>KeyStore: derive_secret(branch, index)
KeyStore-->>KeyMgr: secret
KeyMgr-->>Client: WalletSecretKey
Client->>WalletSdk: signer_api()
WalletSdk-->>Signer: provide SignerApi<TSpec>
Client->>Signer: sign(key_id, transaction)
Signer->>KeyMgr: get_key(key_id)
KeyMgr->>KeyStore: derive_secret(...)
KeyStore-->>KeyMgr: secret
KeyMgr-->>Signer: WalletSecretKey
Signer->>Signer: create signature
Signer-->>Client: SignatureOutput
sequenceDiagram
participant Old as Old API
participant New as New API
Note over Old: Branch-based
Old->>Old: local_signer_api().sign(KeyBranch, key_id, tx)
Old->>Old: get_public_key(KeyBranch, key_id)
Old->>Old: seal_signer = BranchAndKeyId::for_account(owner_key_id)
Old->>Old: KeyId::derived(0)
Note over New: Spec-driven & KeyId-only
New->>New: signer_api().sign(key_id, tx)
New->>New: get_public_key(key_id)
New->>New: seal_signer = owner_key_id
New->>New: KeyId::derived(KeyBranch::Account, 0)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Key areas needing focused review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/wallet/storage_sqlite/tests/accounts.rs (1)
22-32: UseKeyBranch::ViewOnlyKeyfor the view‑only key to match other call sitesHere you pass
KeyId::derived(KeyBranch::Account, 0)for both key arguments. In other places (e.g.,crates/wallet/sdk/tests/support/harness.rsandutilities/tariswap_test_bench/src/accounts.rs) the first key for account creation isKeyBranch::ViewOnlyKeyand the second isKeyBranch::Account. For consistency and clearer semantics, this should likely be:- KeyId::derived(KeyBranch::Account, 0), - Some(KeyId::derived(KeyBranch::Account, 0)), + KeyId::derived(KeyBranch::ViewOnlyKey, 0), + Some(KeyId::derived(KeyBranch::Account, 0)),[scratchpad_end] -->
crates/wallet/sdk/src/local_key_store.rs (1)
60-70: Remove unused error variants and imports fromLocalKeyStoreError.Verification confirms that the
PasswordManager,WalletStorage, andCiphererror variants are unused and should be removed. TheLocalKeyStoreimplementation only uses theNoCipherSeedvariant. The corresponding imports on lines 11 and 15 (PasswordManagerApiErrorandWalletStorageError) should also be removed.applications/tari_walletd/src/handlers/nfts.rs (1)
294-312: Fix final transaction signature to use fee payer keyThe critical issue is confirmed. Line 309 in
applications/tari_walletd/src/handlers/nfts.rssigns the transaction withaccount_owner_key_idwhen it should sign withfee_payer_key_id.Current code signs twice with the source account's key. The established pattern in
tariswap.rs(add_liquidityfunction) shows:
sign_with_contextauthorizes the source account's operations- Final
signauthorizes the fee payerSince the transaction pays fees from
fee_payer_account_address, the fee payer must provide the final signature. The suggested fix is correct:- let transaction = sdk.signer_api().sign(account_owner_key_id, transaction)?; + let transaction = sdk.signer_api().sign(fee_payer_key_id, transaction)?;applications/tari_walletd/src/handlers/transaction.rs (1)
298-313: Fix manifest signing flow to align with established signer/context modelThe code currently violates the signer/context pattern established throughout the codebase. In
applications/tari_walletd/src/handlers/transaction.rs(thesignhandler at lines 160, 163), the established pattern is:
- Other signers call
sign_with_contextwith the primary sealing signer's public key as context- The final seal is done with the primary sealing signer's key_id
The manifest handler at lines 329–336 and 350 breaks this pattern when
signing_key_id != account_owner_key_id:
- Line 333 passes
key.to_public_key()(signing_key's public key) as context, but should pass the account owner's public key- Line 350 seals with
key.key_id(signing_key_id), but should seal withaccount_owner_key_idWhen
signing_key_iddiffers fromaccount_owner_key_id, treatsigning_key_idas an additional signer (like "other_signers" in thesignhandler), and preserveaccount_owner_key_idas the primary sealing signer for both the context and final seal.
🧹 Nitpick comments (21)
utilities/tariswap_test_bench/src/accounts.rs (1)
24-71: Signer API integration looks good; consider normalizing view‑only branches and signing paths
- Using
key_manager_api().get_public_key(KeyId::derived(KeyBranch::Account, idx))plusself.sdk.signer_api().sign(owner_key.key_id, transaction)cleanly routes signing through the newSignerApiand the pluggable key store.- For the initial “free coins” account you pass
KeyId::derived(KeyBranch::ViewOnlyKey, 0)as the view‑only key, but increate_accountsyou register each account withowner.key_id(anAccountbranch key) as both view‑only and owner. That’s workable, but you may want to switch the view‑only parameter there toKeyId::derived(KeyBranch::ViewOnlyKey, idx)(or explicitly document the reuse) for consistency.- Longer term,
fund_accountsstill relies onderive_account_key(0)andbuild_and_seal(&key.key). For HSM‑style backends, unifying this onsigner_api().sign(...)would avoid exposing raw secret keys outside the keystore.Also applies to: 84-121
crates/wallet/sdk/src/key_managers/backend.rs (1)
4-12: Based on the verification, the review comment correctly identifies a stylistic inconsistency between the new implementation and the existing pattern. TheSignabletrait guarantees thatMessageOutput: AsRef<[u8]>, so both approaches are functionally correct due to Rust's type coercion. However, the existing helper incrates/wallet/sdk/src/models/key.rsexplicitly calls.as_ref(), which is a clearer pattern.Align
signimplementation with existingSignableusage patternThe new default
signimplementation differs from the established pattern:
Existing (crates/wallet/sdk/src/models/key.rs:179–180):
let message = item.as_signing_message(context); RistrettoSchnorr::sign(&self.secret, message.as_ref(), &mut OsRng)New (crates/wallet/sdk/src/key_managers/backend.rs:33):
RistrettoSchnorr::sign(&secret, message.as_signing_message(context), &mut OsRng)While both are technically sound (the
Signabletrait guaranteesMessageOutput: AsRef<[u8]>), aligning with the explicit.as_ref()call makes the conversion intent clearer and ensures consistency:- let signature = RistrettoSchnorr::sign(&secret, message.as_signing_message(context), &mut OsRng) + let msg = message.as_signing_message(context); + let signature = RistrettoSchnorr::sign(&secret, msg.as_ref(), &mut OsRng)Additionally, document in the trait that
signis the primary extension point for HSM or remote keystore backends that cannot exposeRistrettoSecretKey.integration_tests/tests/steps/wallet_daemon.rs (1)
128-135: Consider more graceful error handling for missing owner_key_id.The
expect()call on line 130 will panic ifowner_key_idisNone, which could make debugging test failures more difficult. Consider using a more descriptive error message or handling the error case explicitly to provide better context in integration tests.let transaction_submit_req = TransactionSubmitRequest { transaction, - seal_signer: account.owner_key_id().expect("no owner key id"), + seal_signer: account.owner_key_id().ok_or_else(|| anyhow::anyhow!( + "Account {} does not have an owner key ID", + account_name + ))?, other_signers: vec![], detect_inputs: true, detect_inputs_use_unversioned: true, lock_ids: vec![], };utilities/tariswap_test_bench/src/tariswap.rs (3)
94-101:add_liquiditysigning flow aligns with new signer API (minor ergonomics nits)The pattern:
- derive
primary_account_keyviakey_manager_api().get_public_key(primary_account.owner_key_id.expect(...))?,- use
sign_with_context(account.owner_key_id.expect(...), &primary_account_pk, builder), and- then
sign(primary_account_key.key_id(), transaction)matches the intended “account authorizes, primary account pays fee” flow.
Two small optional tweaks:
- Consider replacing the
expect("no owner key id")calls with a fallible path (e.g.,ok_or_elsereturning an error), even in benches, to avoid panics on misconfigured data.- If this utility ever moves out of pure benchmarking, wiring those errors through
anyhow::Resultwould make failures easier to debug.Also applies to: 151-161
218-223: Mirrorpublic_key()usage and signer pattern indo_tariswap_swapsThe
do_tariswap_swapsfunction correctly mirrors the two-step signing used inadd_liquidity:
sign_with_context(account.owner_key_id.expect(...), &primary_account_pk, transaction)for account authorization,- followed by
sign(primary_account_key.key_id(), transaction.build())for the fee-paying primary account.For consistency and to avoid relying on field visibility, I’d suggest using the accessor here as you did above:
- let primary_account_pk = primary_account_key.public_key.to_byte_type(); + let primary_account_pk = primary_account_key.public_key().to_byte_type();That keeps the code resilient if the underlying key type ever changes its field visibility.
Also applies to: 275-285
350-354: Final swap loop uses account key directly for both authorization and feesIn the final “faucet → XTR” loop, the account both authorizes and pays the fee, so using:
self.sdk .signer_api() .sign(account.owner_key_id().expect("no owner key id"), transaction)?;without
sign_with_contextmatches the simpler single-account pattern elsewhere (e.g., mint flows).If you want to harden the bench against misconfigured accounts, you could again replace the
expectwith a fallible conversion, but that’s optional for test-bench code.crates/wallet/sdk/src/spec.rs (1)
1-18:WalletSdkSpectrait and error aliases cleanly centralize SDK configurationThe
WalletSdkSpectrait with associatedStore,KeyStore, andNetworkInterfacetypes, plus theKeyStoreError/NetworkInterfaceErroraliases, is a tidy abstraction for pluggable backends and matches the direction implied by the PR description.Optionally, once usage stabilizes, consider adding doc comments that spell out any extra expectations on these associated types (e.g.,
Send + Syncor clonability) so downstream implementors have a clear contract.crates/wallet/sdk/src/storage/mod.rs (1)
75-77: NewClonebound onWalletStoreis a public contract changeRequiring
CloneonWalletStore(and in the blanket impl) makes sense if the newWalletSdkSpec-based APIs need to clone the store handle, but it is also a behavioral change:
- Any existing external implementor of
WalletStorenow must implementClone.- If
WalletStoreis intended as an externally implemented trait, this is effectively a breaking change in the public surface.If the clone requirement is only needed in a few places, an alternative would be to constrain those specific generics (e.g.,
TSpec::Store: WalletStore + Clone) rather than on the core trait; otherwise it’d be good to explicitly document this requirement for downstream implementors.applications/tari_app_utilities/src/genesis_resources.rs (1)
1-54: Genesis resource helpers encapsulate prior inline definitions (worth locking in with tests)Both
get_public_identity_resourceandget_stealth_tari_resourcelook consistent with the intended genesis setup:
- Public identity: non‑fungible, no owner, default access rules,
TOKEN_SYMBOL = "ID", total supply tracking disabled.- Stealth XTR:
ResourceType::Stealth, no owner, access rules explicitly set torule!(deny_all)for mint/burn/recall/freeze/update_access_rules, symboltXTRon testnets vsXTRotherwise, divisibility 6, and total supply tracking disabled per the comment.Given how critical these are to chain bootstrapping, it’d be valuable to add a small test (or snapshot) that asserts these helpers produce the same resources as the previous inline genesis definitions, so any future drift is caught early.
applications/tari_walletd/src/lib.rs (1)
85-96: Genesis resource upsert on startup is straightforward; consider future extensibilityUpserting the stealth TARI and public-identity resources immediately after initializing the SDK ensures the wallet has the required genesis resources without a network fetch, and the use of
upsert_resourcekeeps the operation idempotent across restarts. If you expect alternate or extended genesis sets in future networks or environments, you might later want to funnel this block through a small helper (e.g.,insert_genesis_resources(&mut wallet_sdk, config.ootle_wallet_daemon.network)) so that other applications/tests can share the same logic and so new genesis resources have a single place to be added.applications/tari_walletd/src/services/mod.rs (1)
16-32: spawn_services specialization to OotleWalletDaemonSpec is consistent with the new SDK modelChanging
spawn_servicesto takeWalletSdk<OotleWalletDaemonSpec>aligns this module with the daemon’s concrete spec and keeps all downstream services on the same SDK type. The internal wiring (transaction service, template monitor, UTXO scanner, account monitor) remains unchanged and should behave equivalently.If you want to reduce type-name friction between modules, you could consider importing the
WalletSdkalias fromapplications::tari_walletd::libhere instead of the genericWalletSdkfrom the SDK crate, to avoid having two differentWalletSdkmeanings depending on the module.crates/wallet/sdk_services/src/account_recovery/service.rs (1)
76-99: Clarify and align KeyId/KeyBranch usage for recovered accountsUsing
key.key_index()for logging, resetting the active key, and constructing account names keeps the recovery flow readable and tied directly to derived key indices. Intry_recover_account, for accounts found on-chain you now explicitly derive KeyIds by branch:
KeyId::derived(KeyBranch::ViewOnlyKey, key.key_index())KeyId::derived(KeyBranch::Account, key.key_index())while for the “not found on chain” case you pass
key.as_key_id()for both view and account ids.If this asymmetry is intentional (e.g., to keep pre-existing behavior for accounts that only ever had a single key vs. newer accounts that split view/account branches), it would be helpful to capture that in a short comment or helper to avoid regressions later. Otherwise, consider normalizing both branches to derive view/account KeyIds in the same way so that:
view_only_key_idalways usesKeyBranch::ViewOnlyKey.account_key_idalways usesKeyBranch::Account.That would also align more directly with how
UtxoScannernow callsget_key(account.view_only_key_id())?.Also applies to: 119-135, 172-181, 219-233
integration_tests/src/wallet_daemon_client.rs (1)
441-461: Consistent use ofowner_key_idasseal_signeracross helpersAll four helpers now derive
owner_key_idfrom the account and pass it directly asseal_signeronTransactionSubmitRequest. This is consistent with the new API surface and keeps the test helpers aligned with the daemon behavior.Note that all paths use
expectwhenowner_key_idisNone, which will panic if a view-only or otherwise non-signing account is accidentally used. That’s fine for test-only utilities, but if these helpers are ever reused in broader contexts it could be worth bubbling an error instead of panicking.Also applies to: 529-549, 645-667, 947-957
crates/wallet/sdk_services/src/transaction_service/service.rs (1)
216-232:clear_stale_lockshelper matches LocksApi behaviorThe new
clear_stale_lockshelper correctly useswallet_sdk.locks_api().clear_stale_locks()and logs either an info or debug message depending on whether anything was cleared. This keeps lock cleanup behavior contained and makeson_polleasier to read.Minor nit: the local variable is named
transaction_apibut actually holds the locks API, which might be slightly confusing for future readers; consider renaming tolocks_apiwhen you next touch this code.crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
296-300: Write-transaction type tied to TSpec::Store is correct but verboseThe new
spendsignature,fn spend( tx: &mut <TSpec::Store as WriteableWalletStore>::WriteTransaction<'_>, // ... )accurately reflects that the write transaction comes from
TSpec::StoreimplementingWriteableWalletStore. It’s a bit noisy, but technically correct and future-proof for different store implementations.If desired, you could introduce a local type alias for the write transaction to make the signature more readable, but it’s not strictly necessary.
crates/wallet/sdk/src/apis/accounts.rs (1)
43-51: AccountsApi WalletSdkSpec migration and KeyId handling look coherentThe migration of
AccountsApitoTSpec: WalletSdkSpec(store + substate + key manager) plus the switch toKeyId/KeyBranchis internally consistent:
add_accountnow usingkey_manager_api.get_key(key_id)?.to_public_key().to_byte_type()correctly relies on the newKeyIdabstraction.get_address_for_account’s match onKeyId::Derived { key_branch, index }and use ofkey_manager_api.derive_key(key_branch, index)lines up with the newDerivedKeyIdmodel, whileImportedcontinues to go viaget_imported_key.- The async
resolve_account_by_public_keyimpl reuses the same APIs and generics cleanly.I don’t see correctness issues here; just consider optionally combining the two
impl<'a, TSpec> AccountsApi<'a, TSpec> where TSpec: WalletSdkSpecblocks for brevity.Also applies to: 54-60, 66-81, 133-139, 241-251, 400-455
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
381-390: Nonce key derivation is correct but may over-generate keysThe new signing-key selection:
- Uses
owner_key_idwhen revealed funds or badges require an account auth signature.- Falls back to
next_derived_key_id(KeyBranch::Nonce)?.into()when only confidential funds are involved.This is functionally sound, and keeping
spend_key_idas the account owner key whilerequired_signermay be a nonce key is consistent with howStealthOutputsApi::generate_transfer_statementuses those values.One minor side-effect: in the “all-confidential, no-badge” case, you may derive two distinct nonce keys (one for fees, one for the main transfer) since
next_derived_key_idis called twice and the equality check (fee_signer.key_id() == signing_key_id) will never hit in that path. If key-store churn is a concern, you could reuse the same nonce key across fee and main statements when both choose the nonce branch.Also applies to: 433-447, 589-595
crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
32-47: ConfidentialTransferApi spec migration and account key lookup are reasonableThe migration of
ConfidentialTransferApitoTSpec: WalletSdkSpecand the switch to:
- Resolving the account owner key via
self.key_manager_api.get_key(account_owner_key_id)?, and- Using that same
KeyIdfor bothview_only_key_idandowner_key_idon the locked change output,are consistent with the new
KeyId/KeyBranchmodel and with a design where confidential outputs are both viewed and spent with the same account key.If the intent is to eventually support a distinct view-only key for confidential outputs (similar to the stealth path), you might want to separate those IDs, but for the current semantics this looks correct.
Also applies to: 49-61, 220-227, 275-277, 334-359, 410-412
crates/wallet/sdk/src/models/key.rs (1)
39-51: Verify KeyBranch.as_str values vs external expectations
KeyBranch::as_strreturns:
"transactions"forTransaction(plural), and"elgamal_view_key"forElgamalEncryptionViewKey,while the TypeScript binding for
KeyBranchuses"transaction"and"elgamal_encryption_view_key"as string literals. Ifas_str()is used as the branch identifier for key derivation in the keystore/HSM layer, any change or mismatch here could make existing keys unreachable.Please double‑check these string constants against the previous behaviour and the keystore backend contract; if they must remain stable, consider aligning them with the TS / serde representations (or clearly separating “display” vs “derivation path” names).
Also applies to: 59-63
crates/wallet/sdk/src/apis/signer.rs (1)
46-48: TODO: Clarify imported key signing requirements.This TODO raises a valid architectural question about whether signing from imported keys is necessary, given they are typically view-only keys. Resolving this could simplify the API by removing the KeyManagerApi dependency in favor of just the KeyStore.
Do you want me to open an issue to track the investigation of this architectural decision?
crates/wallet/sdk/src/apis/key_manager.rs (1)
154-154: TODO: Optimization opportunity for imported key public keys.This TODO suggests storing the public key in the database to avoid fetching the secret key when only the public key is needed. This would be a good optimization for frequently accessed view-only keys.
Do you want me to open an issue to track this optimization?
📜 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 (52)
applications/tari_app_utilities/src/genesis_resources.rs(1 hunks)applications/tari_app_utilities/src/lib.rs(1 hunks)applications/tari_validator_node/src/genesis_state.rs(2 hunks)applications/tari_wallet_cli/src/command/transaction.rs(5 hunks)applications/tari_walletd/src/handlers/accounts.rs(8 hunks)applications/tari_walletd/src/handlers/confidential.rs(1 hunks)applications/tari_walletd/src/handlers/helpers.rs(3 hunks)applications/tari_walletd/src/handlers/keys.rs(1 hunks)applications/tari_walletd/src/handlers/nfts.rs(4 hunks)applications/tari_walletd/src/handlers/transaction.rs(10 hunks)applications/tari_walletd/src/handlers/validator.rs(4 hunks)applications/tari_walletd/src/lib.rs(4 hunks)applications/tari_walletd/src/main.rs(1 hunks)applications/tari_walletd/src/services/mod.rs(2 hunks)applications/tari_walletd/src/services/template_monitor.rs(1 hunks)clients/wallet_daemon_client/src/types.rs(2 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/accounts.rs(4 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(3 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(2 hunks)crates/wallet/sdk/src/apis/key_manager.rs(12 hunks)crates/wallet/sdk/src/apis/signer.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(5 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(5 hunks)crates/wallet/sdk/src/key_managers/backend.rs(1 hunks)crates/wallet/sdk/src/key_managers/local.rs(0 hunks)crates/wallet/sdk/src/key_managers/mod.rs(0 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/local_key_store.rs(2 hunks)crates/wallet/sdk/src/models/account.rs(1 hunks)crates/wallet/sdk/src/models/key.rs(8 hunks)crates/wallet/sdk/src/network.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(10 hunks)crates/wallet/sdk/src/spec.rs(1 hunks)crates/wallet/sdk/src/storage/mod.rs(1 hunks)crates/wallet/sdk/tests/confidential_output_api.rs(2 hunks)crates/wallet/sdk/tests/support/harness.rs(8 hunks)crates/wallet/sdk_services/src/account_monitor/monitor.rs(2 hunks)crates/wallet/sdk_services/src/account_monitor/scanner.rs(2 hunks)crates/wallet/sdk_services/src/account_recovery/service.rs(6 hunks)crates/wallet/sdk_services/src/transaction_service/service.rs(5 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs(3 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(6 hunks)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs(3 hunks)crates/wallet/sdk_services/src/utxo_scanner/worker.rs(4 hunks)crates/wallet/storage_sqlite/tests/accounts.rs(2 hunks)integration_tests/src/wallet_daemon_client.rs(5 hunks)integration_tests/tests/steps/wallet_daemon.rs(2 hunks)utilities/tariswap_test_bench/src/accounts.rs(5 hunks)utilities/tariswap_test_bench/src/runner.rs(3 hunks)utilities/tariswap_test_bench/src/tariswap.rs(7 hunks)utilities/traffic-sim/src/sim.rs(3 hunks)
💤 Files with no reviewable changes (2)
- crates/wallet/sdk/src/key_managers/mod.rs
- crates/wallet/sdk/src/key_managers/local.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T10:10:24.258Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1629
File: applications/tari_walletd/src/handlers/accounts.rs:1001-1002
Timestamp: 2025-11-04T10:10:24.258Z
Learning: In applications/tari_walletd/src/handlers/accounts.rs, the expect() on Memo::new_pay_ref_and_bytes_truncate at line 1002 is safe and intentional. PayRef is validated to be at most 64 bytes during address decoding (PayRef::MAX_LEN = 64), and the function only returns None if payref exceeds 252 bytes (Memo::MAX_BYTES_LENGTH - 1). Since 64 < 252, None is impossible with a valid PayRef.
Applied to files:
utilities/traffic-sim/src/sim.rsapplications/tari_walletd/src/handlers/accounts.rs
🧬 Code graph analysis (40)
clients/wallet_daemon_client/src/types.rs (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
utilities/tariswap_test_bench/src/tariswap.rs (2)
bindings/src/types/Account.ts (1)
Account(7-16)crates/wallet/sdk/src/models/account.rs (1)
account(85-87)
applications/tari_wallet_cli/src/command/transaction.rs (1)
crates/wallet/sdk/src/models/account.rs (2)
owner_key_id(35-37)owner_key_id(105-107)
applications/tari_walletd/src/services/template_monitor.rs (9)
crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(52-72)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
new(60-82)crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
new(42-44)crates/wallet/sdk_services/src/transaction_service/service.rs (1)
new(57-75)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(22-24)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(54-74)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(58-62)new(117-125)
applications/tari_walletd/src/handlers/keys.rs (2)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(353-355)
applications/tari_walletd/src/handlers/validator.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (5)
key_id(74-76)key_id(145-147)key_id(170-172)derived(353-355)index(313-315)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
utilities/traffic-sim/src/sim.rs (3)
bindings/src/types/AccountWithAddress.ts (1)
AccountWithAddress(5-5)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/models/account.rs (1)
account(85-87)
applications/tari_walletd/src/lib.rs (2)
applications/tari_app_utilities/src/genesis_resources.rs (2)
get_public_identity_resource(15-28)get_stealth_tari_resource(30-54)crates/wallet/sdk/src/sdk.rs (2)
initialize_with_local_key_store(243-266)store(104-106)
crates/wallet/sdk_services/src/account_recovery/service.rs (7)
applications/tari_walletd/src/services/template_monitor.rs (1)
new(26-32)crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk/src/models/key.rs (4)
new(309-311)key_index(121-123)key_index(235-237)derived(353-355)crates/wallet/sdk_services/src/transaction_service/service.rs (1)
new(57-75)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(54-74)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
new(43-49)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(58-62)new(117-125)
crates/wallet/storage_sqlite/tests/accounts.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)crates/wallet/sdk/src/models/key.rs (1)
derived(353-355)
crates/wallet/sdk_services/src/account_monitor/scanner.rs (6)
applications/tari_walletd/src/services/template_monitor.rs (1)
new(26-32)crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
new(60-82)crates/wallet/sdk_services/src/transaction_service/service.rs (1)
new(57-75)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(22-24)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(58-62)new(117-125)
crates/wallet/sdk/tests/support/harness.rs (4)
crates/wallet/sdk/src/local_key_store.rs (1)
cipher_seed(33-35)crates/wallet/sdk/src/sdk.rs (2)
network(116-118)initialize_with_local_key_store(243-266)crates/wallet/sdk/src/models/key.rs (1)
derived(353-355)crates/wallet/sdk/src/network.rs (4)
get_status(83-83)get_status(89-91)get_error_message(84-84)get_error_message(93-95)
integration_tests/tests/steps/wallet_daemon.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/wallet/sdk_services/src/transaction_service/service.rs (8)
crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(52-72)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
new(81-93)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
new(60-82)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(22-24)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(54-74)crates/wallet/sdk/src/apis/locks.rs (1)
clear_stale_locks(64-67)
crates/wallet/sdk_services/src/utxo_scanner/worker.rs (7)
applications/tari_walletd/src/services/template_monitor.rs (1)
new(26-32)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
new(60-82)crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
new(42-44)crates/wallet/sdk_services/src/account_recovery/service.rs (1)
new(42-54)crates/wallet/sdk_services/src/transaction_service/service.rs (1)
new(57-75)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(22-24)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
new(43-49)notify(89-93)
crates/wallet/sdk/src/key_managers/backend.rs (4)
crates/wallet/sdk/src/local_key_store.rs (2)
derive_secret(45-52)key_birthday(54-57)crates/wallet/sdk/src/models/key.rs (6)
branch(317-319)key_index(121-123)key_index(235-237)sign(178-182)index(313-315)secret(166-168)crates/wallet/sdk/src/apis/signer.rs (1)
sign(73-78)crates/transaction/src/v1/signature.rs (3)
sign(36-46)signature(59-61)signature(118-120)
applications/tari_walletd/src/services/mod.rs (1)
applications/tari_walletd/src/handlers/context.rs (2)
shutdown_signal(81-83)wallet_sdk(63-65)
applications/tari_app_utilities/src/genesis_resources.rs (8)
bindings/src/types/Resource.ts (1)
Resource(10-23)bindings/src/types/Network.ts (1)
Network(6-6)bindings/src/types/OwnerRule.ts (1)
OwnerRule(7-7)bindings/src/types/ResourceAccessRules.ts (1)
ResourceAccessRules(7-16)bindings/src/types/Metadata.ts (1)
Metadata(6-6)bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)crates/template_lib/src/auth/access_rules.rs (1)
deny_all(240-251)
crates/wallet/sdk/src/apis/confidential_outputs.rs (5)
crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(52-72)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
new(81-93)key_manager_api(397-414)crates/wallet/sdk/src/sdk.rs (2)
store(104-106)key_manager_api(135-144)
crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (8)
applications/tari_walletd/src/services/template_monitor.rs (1)
new(26-32)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
new(60-82)crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
new(42-44)crates/wallet/sdk_services/src/transaction_service/service.rs (1)
new(57-75)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(54-74)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (1)
new(43-49)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(58-62)new(117-125)crates/wallet/sdk/tests/support/harness.rs (1)
sdk(169-171)
utilities/tariswap_test_bench/src/accounts.rs (4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (1)
derived(353-355)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/Account.ts (1)
Account(7-16)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (4)
crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(22-24)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(58-62)new(117-125)crates/wallet/sdk/tests/support/harness.rs (1)
sdk(169-171)
applications/tari_walletd/src/handlers/transaction.rs (1)
crates/wallet/sdk/src/models/account.rs (2)
owner_key_id(35-37)owner_key_id(105-107)
crates/wallet/sdk/tests/confidential_output_api.rs (2)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/models/key.rs (1)
derived(353-355)
crates/wallet/sdk_services/src/account_monitor/monitor.rs (9)
applications/tari_walletd/src/services/template_monitor.rs (1)
new(26-32)crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(52-72)crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
new(42-44)crates/wallet/sdk_services/src/transaction_service/service.rs (1)
new(57-75)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(22-24)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (1)
new(54-74)crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (2)
new(43-49)notify(89-93)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(58-62)new(117-125)
crates/wallet/sdk/src/apis/stealth_outputs.rs (6)
crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(52-72)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
new(61-78)crates/wallet/sdk/src/sdk.rs (5)
store(104-106)key_manager_api(135-144)config_api(108-110)config_api(272-273)config_api(315-317)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (5)
crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(52-72)crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
new(81-93)key_manager_api(397-414)crates/wallet/sdk/src/models/account.rs (3)
new(77-79)owner_key_id(35-37)owner_key_id(105-107)crates/wallet/sdk/src/sdk.rs (7)
accounts_api(164-172)locks_api(124-126)substate_api(160-162)key_manager_api(135-144)config_api(108-110)config_api(272-273)config_api(315-317)
crates/wallet/sdk_services/src/utxo_scanner/utxo_recovery.rs (7)
applications/tari_walletd/src/services/template_monitor.rs (1)
new(26-32)crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk_services/src/account_monitor/monitor.rs (1)
new(60-82)crates/wallet/sdk_services/src/account_monitor/scanner.rs (1)
new(42-44)crates/wallet/sdk_services/src/utxo_scanner/scanner.rs (1)
new(22-24)crates/wallet/sdk_services/src/utxo_scanner/worker.rs (2)
new(58-62)new(117-125)
integration_tests/src/wallet_daemon_client.rs (2)
bindings/src/types/Account.ts (1)
Account(7-16)crates/wallet/sdk/src/models/account.rs (3)
owner_key_id(35-37)owner_key_id(105-107)account(85-87)
applications/tari_walletd/src/handlers/helpers.rs (3)
crates/wallet/sdk/tests/support/harness.rs (1)
sdk(169-171)crates/wallet/sdk/src/sdk.rs (1)
accounts_api(164-172)crates/wallet/sdk/src/apis/accounts.rs (1)
get_account_or_default(299-308)
crates/wallet/sdk/src/apis/confidential_transfer.rs (4)
crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
new(81-93)key_manager_api(397-414)crates/wallet/sdk/src/sdk.rs (9)
key_manager_api(135-144)accounts_api(164-172)locks_api(124-126)confidential_outputs_api(182-184)substate_api(160-162)transaction_api(156-158)config_api(108-110)config_api(272-273)config_api(315-317)
applications/tari_walletd/src/handlers/accounts.rs (2)
crates/wallet/sdk/src/apis/confidential_transfer.rs (2)
new(52-72)transfer(210-422)crates/wallet/sdk/src/apis/stealth_transfer/api.rs (2)
new(61-78)transfer(284-597)
crates/wallet/sdk/src/apis/accounts.rs (5)
crates/wallet/sdk/src/models/key.rs (8)
public_key(82-84)public_key(141-143)public_key(239-241)new(309-311)key_id(74-76)key_id(145-147)key_id(170-172)index(313-315)crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk/src/sdk.rs (3)
network(116-118)store(104-106)key_manager_api(135-144)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
crates/wallet/sdk/src/models/key.rs (4)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/apis/signer.rs (1)
sign(73-78)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/key_managers/backend.rs (1)
sign(25-39)
crates/wallet/sdk/src/apis/signer.rs (3)
crates/wallet/sdk/src/apis/key_manager.rs (2)
new(62-76)clone(333-341)crates/wallet/sdk/src/local_key_store.rs (1)
new(24-26)crates/wallet/sdk/src/key_managers/backend.rs (1)
sign(25-39)
applications/tari_validator_node/src/genesis_state.rs (1)
applications/tari_app_utilities/src/genesis_resources.rs (2)
get_public_identity_resource(15-28)get_stealth_tari_resource(30-54)
utilities/tariswap_test_bench/src/runner.rs (2)
crates/wallet/sdk/src/local_key_store.rs (1)
cipher_seed(33-35)crates/wallet/sdk/src/sdk.rs (2)
initialize_with_local_key_store(243-266)store(104-106)
crates/wallet/sdk/src/local_key_store.rs (2)
crates/wallet/sdk/src/apis/key_manager.rs (1)
new(62-76)crates/wallet/sdk/src/key_managers/backend.rs (1)
key_birthday(43-43)
crates/wallet/sdk/src/sdk.rs (3)
crates/wallet/sdk/src/apis/key_manager.rs (3)
key_store(78-80)new(62-76)clone(333-341)crates/wallet/sdk/src/apis/config.rs (1)
new(18-23)crates/wallet/sdk/src/local_key_store.rs (1)
cipher_seed(33-35)
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/models/key.rs (16)
new(309-311)derived(353-355)branch(317-319)index(313-315)secret(166-168)key_id(74-76)key_id(145-147)key_id(170-172)from(151-156)from(186-191)from(195-200)from(204-209)from(213-218)from(290-292)from(296-298)from(384-389)crates/wallet/sdk/src/sdk.rs (3)
network(116-118)store(104-106)clone(365-373)
⏰ 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). (4)
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: check nightly
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx (1)
215-215: Remove redundant non-null assertion operator.The
!operator is unnecessary here since lines 210-212 already ensureowner_key_idexists and throw an error if it doesn't. TypeScript should infer the value is non-null after the guard check.Apply this diff:
- seal_signer: account.account.owner_key_id!, + seal_signer: account.account.owner_key_id,bindings/src/types/wallet-daemon-client/DerivedKeyId.ts (1)
4-4: DerivedKeyId shape matches KeyId.Derived and looks goodDefining
DerivedKeyIdas{ branch: KeyBranch; index: bigint }cleanly mirrors theDerivedvariant ofKeyIdwhile avoiding a circular dependency onKeyIditself. This should make it straightforward to derive or reference keys by branch/index across the SDK.If you ever revisit the Rust side, you might consider aligning the field name (
branchvskey_branchinKeyId.Derived) for ergonomics, but this is purely cosmetic and not required for this PR.bindings/src/types/PublishedTemplate.ts (1)
3-16: Type expansion looks good; consider clarifyingbinaryencodingThe new
PublishedTemplateshape withauthor,binary, andat_epochis clear and matches the richer metadata needs. One minor concern is thatbinary: stringplus the comment “Binary of the template” doesn’t specify whether this is raw bytes, hex, base64, etc., which can trip up clients.If this is, for example, hex- or base64-encoded, consider tightening the doc comment (and possibly the field name) to reflect that encoding so TS consumers know how to parse/produce it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
applications/tari_validator_node/web_ui/src/App.tsx(0 hunks)applications/tari_validator_node/web_ui/src/Components/MenuItems.tsx(0 hunks)applications/tari_validator_node/web_ui/src/main.tsx(0 hunks)applications/tari_validator_node/web_ui/src/routes/Templates/Templates.tsx(0 hunks)applications/tari_validator_node/web_ui/src/routes/VN/Components/Templates.tsx(0 hunks)applications/tari_validator_node/web_ui/src/routes/VN/ValidatorNode.tsx(0 hunks)applications/tari_validator_node/web_ui/src/utils/json_rpc.tsx(0 hunks)applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx(2 hunks)applications/tari_walletd/web_ui/src/routes/FlowEditor/FlowEditor.tsx(1 hunks)bindings/package.json(1 hunks)bindings/src/index.ts(0 hunks)bindings/src/types/Instruction.ts(1 hunks)bindings/src/types/PublishedTemplate.ts(1 hunks)bindings/src/types/WalletTransaction.ts(1 hunks)bindings/src/types/tari-indexer-client/GetTemplateDefinitionResponse.ts(1 hunks)bindings/src/types/tari-indexer-client/IndexerGetEpochManagerStatsResponse.ts(1 hunks)bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts(1 hunks)bindings/src/types/tari-indexer-client/TemplateMetadata.ts(1 hunks)bindings/src/types/validator-node-client/GetTemplateResponse.ts(1 hunks)bindings/src/types/validator-node-client/GetTemplatesRequest.ts(0 hunks)bindings/src/types/validator-node-client/GetTemplatesResponse.ts(0 hunks)bindings/src/types/validator-node-client/VNTemplateMetadata.ts(1 hunks)bindings/src/types/wallet-daemon-client/AccountsCreateStealthTransferStatementResponse.ts(1 hunks)bindings/src/types/wallet-daemon-client/DerivedKeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/KeyId.ts(1 hunks)bindings/src/types/wallet-daemon-client/NewAccountData.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts(1 hunks)bindings/src/validator-node-client.ts(0 hunks)bindings/src/wallet-daemon-client.ts(2 hunks)crates/common_types/src/network.rs(2 hunks)crates/engine_types/src/resource.rs(2 hunks)crates/template_lib/src/auth/access_rules.rs(1 hunks)crates/template_lib_types/src/resource_type.rs(1 hunks)
💤 Files with no reviewable changes (11)
- applications/tari_validator_node/web_ui/src/main.tsx
- applications/tari_validator_node/web_ui/src/routes/Templates/Templates.tsx
- applications/tari_validator_node/web_ui/src/Components/MenuItems.tsx
- bindings/src/types/validator-node-client/GetTemplatesResponse.ts
- applications/tari_validator_node/web_ui/src/App.tsx
- applications/tari_validator_node/web_ui/src/routes/VN/Components/Templates.tsx
- bindings/src/index.ts
- applications/tari_validator_node/web_ui/src/routes/VN/ValidatorNode.tsx
- applications/tari_validator_node/web_ui/src/utils/json_rpc.tsx
- bindings/src/validator-node-client.ts
- bindings/src/types/validator-node-client/GetTemplatesRequest.ts
✅ Files skipped from review due to trivial changes (1)
- bindings/package.json
🧰 Additional context used
🧬 Code graph analysis (11)
crates/template_lib/src/auth/access_rules.rs (2)
crates/engine_types/src/resource.rs (1)
new(52-83)crates/template_lib/src/resource/builder/confidential.rs (1)
new(29-41)
bindings/src/types/tari-indexer-client/GetTemplateDefinitionResponse.ts (1)
bindings/src/types/TemplateDef.ts (1)
TemplateDef(4-4)
bindings/src/types/validator-node-client/GetTemplateResponse.ts (2)
bindings/src/types/validator-node-client/VNTemplateMetadata.ts (1)
VNTemplateMetadata(5-5)bindings/src/types/validator-node-client/TemplateAbi.ts (1)
TemplateAbi(4-4)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
bindings/src/types/wallet-daemon-client/DerivedKeyId.ts (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
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(4-4)
crates/engine_types/src/resource.rs (3)
crates/template_lib_types/src/amount/amount.rs (2)
new(52-54)zero(57-59)crates/template_lib/src/resource/builder/confidential.rs (1)
new(29-41)crates/template_lib/src/resource/builder/fungible.rs (1)
new(64-75)
bindings/src/types/validator-node-client/VNTemplateMetadata.ts (2)
bindings/src/types/Hash.ts (1)
Hash(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (1)
bindings/src/helpers/enum.ts (1)
matchesTypeEnum(4-26)
bindings/src/types/wallet-daemon-client/AccountsCreateStealthTransferStatementResponse.ts (1)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(4-4)
bindings/src/types/tari-indexer-client/TemplateMetadata.ts (3)
bindings/src/types/Hash.ts (1)
Hash(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/Epoch.ts (1)
Epoch(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). (2)
- GitHub Check: check stable
- GitHub Check: clippy
🔇 Additional comments (19)
bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts (1)
7-7: LGTM! Semantically identical type syntax.The change from
Array<string>tostring[]is purely stylistic and semantically identical in TypeScript. Since this is an auto-generated file (by ts-rs), the change reflects updates in the code generator or source Rust definitions.crates/common_types/src/network.rs (1)
50-69: LGTM! Const functions enable compile-time evaluation.Both
as_byteandis_testnetare correctly marked asconst fn. The implementations are trivially const-compatible:
as_byteperforms a simple enum-to-repr castis_testnetuses thematches!macro, which is const-stableThis change is backward-compatible and aligns with the const-friendly API pattern already established by
as_key_str.crates/template_lib_types/src/resource_type.rs (1)
38-55: LGTM! Enabling const evaluation for resource type checks.The const qualifiers on these accessor methods enable compile-time evaluation while maintaining full backward compatibility. The
matches!macro is const-compatible, making these changes straightforward and correct.crates/template_lib/src/auth/access_rules.rs (1)
224-237: LGTM! Enabling const construction for resource access rules.Making
new()a const function allowsResourceAccessRulesto be constructed in const contexts. All initialization values are const-evaluable enum variants.crates/engine_types/src/resource.rs (1)
52-83: LGTM! Const-compatible resource construction with refactored initialization.The conversion to
const fnis well-executed. Thetotal_supplyinitialization was appropriately refactored from.filter()(not const-compatible) to an explicit if/else, preserving the original logic while enabling const evaluation. The use ofAmount::zero()andResourceType::is_non_fungible()both work in const contexts.bindings/src/types/tari-indexer-client/IndexerGetEpochManagerStatsResponse.ts (1)
7-7: Unable to locate consumer code; manual verification required.The search found no TypeScript call sites that access
current_block_hash. The type is exported but not actively used in the indexed codebase. This could indicate:
- The type is newly generated or consumers haven't been committed yet
- Consumers exist in code outside the current search scope (tests, other branches, generated code)
- The change aligns with the similar
GetEpochManagerStatsResponsetype, which also definescurrent_block_hashasstringSince this is a generated file from ts-rs, verify:
- The Rust source type has been updated correctly
- Any frontend/consumer code that uses
IndexerGetEpochManagerStatsResponsehandles the string type- The string format (hex-encoded, base64, bech32) is defined in the Rust serialization/documentation
bindings/src/types/tari-indexer-client/GetTemplateDefinitionResponse.ts (1)
4-4: Newcode_sizefield looks consistent with template-related metadataThe addition of a required
code_size: numberalongsidenameanddefinitionaligns with the other template metadata changes in this PR and keeps the response shape simple and explicit. As long as the indexer API always includes this field for the bound version, this looks good.bindings/src/types/tari-indexer-client/TemplateMetadata.ts (1)
2-12: Template metadata extensions are coherent; confirmbinary_sharepresentation for consumersThe switch of
binary_shatostringplus the newauthor_public_key,code_size, andepochfields, typed viaHash,RistrettoPublicKeyBytes, andEpoch, looks consistent with the rest of the bindings and gives richer metadata without complicating the shape.The only thing to double‑check is that all callers previously expecting
binary_shaasnumber[]have been updated to handle the string form (and that the chosen encoding, e.g. hex vs base64, is documented at the API level).bindings/src/types/validator-node-client/VNTemplateMetadata.ts (2)
3-3: LGTM: Import added for new field type.The import for
RistrettoPublicKeyBytesis correctly added to support the newauthorfield.
5-5: Type-level breaking change, but no code updates required.While
VNTemplateMetadatais a breaking change at the type signature level (removedbinary_sha, addedcode_sizeandauthor), verification shows this change has no runtime impact on the codebase: themetadatafield ofGetTemplateResponseis never accessed in any application code. The validator node UI only consumes theabiproperty from the response.Since this is an auto-generated type from Rust (ts-rs), the Rust backend has already been updated to provide these new fields. No additional TypeScript consumer updates are needed.
applications/tari_walletd/web_ui/src/routes/AssetVault/Components/ClaimFees.tsx (2)
98-98: Backend API compatibility verified. Thekey_branchfield is supported and"account"is a validKeyBranchvalue. Both code changes (lines 98 and 191) are consistent, type-safe, and appropriate for this component's context.
191-191: Approve thekey_branchaddition; RPC structure is type-safe and compatible.The change at line 191 correctly structures the RPC call with
{ KeyId: { Derived: { key_branch: "account", index: BigInt(formState.keyIndex) } } }, which matches theKeyIdtype definition exactly. TheDerivedvariant expectskey_branch: KeyBranchandindex: bigint, both of which are provided correctly. This is consistent with line 98's use of the samekey_branch: "account"pattern. TypeScript type checking via the auto-generated bindings confirms the structure is valid.bindings/src/types/wallet-daemon-client/TransactionSubmitRequest.ts (1)
3-8: KeyId migration for signers looks consistent; ensure server side matchesSwitching
seal_signerandother_signerstoKeyIdaligns with the newKeyIdunion type and removes the extra BranchAndKeyId wrapper, which simplifies the API surface. As long as the wallet-daemon RPC now expects/returnsKeyIdfor these fields on the Rust side in the same commit, this change is sound.Please double‑check that all Rust wallet-daemon handlers and any non‑TS clients constructing
TransactionSubmitRequesthave been updated to theKeyIdJSON shape so there is no wire‑format mismatch at deploy time.bindings/src/types/wallet-daemon-client/AccountsCreateStealthTransferStatementResponse.ts (1)
3-8: Aligning signing_keys with KeyId-based signer modelUpdating
signing_keystoArray<KeyId>is consistent with the new unified signer identifier model and matches theKeyIddefinition used elsewhere in the bindings.Confirm that any code constructing
AccountsCreateStealthTransferStatementResponse(tests, mocks, or non‑TS clients) has been updated to emitKeyIdvalues rather than the previous BranchAndKeyId shape so downstream consumers don’t break unexpectedly.bindings/src/types/wallet-daemon-client/NewAccountData.ts (1)
2-2: Import path adjustment for ComponentAddress seems correctPointing
ComponentAddressto../ComponentAddressmatches the expected layout (shared type at the parenttypeslevel rather than underwallet-daemon-client).Please run
tsc(or your usual TS build) to confirm there are no remaining imports referencing./ComponentAddressand that only a singleComponentAddressdefinition is present in the bindings.bindings/src/types/Instruction.ts (1)
36-36: Encoding confirmed as BASE64 but type documentation remains unclear – verify all producersThe
binaryfield change fromArray<number>tostringuses BASE64 encoding, which is confirmed by thebase64FromArrayBufferimplementation found inapplications/tari_walletd/web_ui/src/utils/helpers.tsx. The web UI properly convertsArrayBufferto a base64-encoded string before sending it inPublishTemplateRequest.However, the encoding format is not documented at the type level in
bindings/src/types/Instruction.ts(line 36), and the field namebinaryalone doesn't convey the format. Verify that:
- All code constructing
PublishTemplateinstructions (wallet daemon backend, any external clients, tests) uses the same BASE64 encoding- The encoding is consistent across all producers and consumers
Consider adding a comment to the type definition clarifying the BASE64 requirement.
bindings/src/types/WalletTransaction.ts (1)
6-6: Import path realignment looks correctUsing
./wallet-daemon-client/NewAccountDataaligns this type with the rest of the wallet-daemon-client bindings and keepsWalletTransaction’s shape unchanged at the API level.bindings/src/wallet-daemon-client.ts (2)
50-50: DerivedKeyId export wiring looks goodRe-exporting
./types/wallet-daemon-client/DerivedKeyIdfrom the barrel keeps the TS client’s public surface in sync with the new key-derivation model.
127-127: NewAccountData barrel export is consistentSurfacing
NewAccountDatahere matches the new import path used byWalletTransactionand keeps consumers on a single entrypoint for wallet-daemon types.
Test Results (CI)515 tests ±0 507 ✅ +2 1h 33m 28s ⏱️ + 4m 2s For more details on these failures, see this check. Results for commit 3313485. ± Comparison against base commit 3b86ab7. |
sdbondi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/wallet/sdk/src/apis/signer.rs (1)
45-55: Clarify or deprecate imported-key signing behavior.The TODO correctly questions whether imported keys should be signable (vs strictly view-only). It would be good to either (a) explicitly commit to supporting signing with imported keys (and add tests/docs), or (b) plan to deprecate this branch and rely solely on the key store for signing, simplifying
SignerApi’s dependency surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
applications/tari_swarm_daemon/webui/vite.config.ts.timestamp-1712311434695-a58d35a3bc145.mjs(0 hunks)crates/wallet/sdk/src/apis/signer.rs(1 hunks)crates/wallet/sdk/src/sdk.rs(10 hunks)
💤 Files with no reviewable changes (1)
- applications/tari_swarm_daemon/webui/vite.config.ts.timestamp-1712311434695-a58d35a3bc145.mjs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/wallet/sdk/src/apis/signer.rs (5)
crates/wallet/sdk/src/models/key.rs (18)
fmt(60-62)fmt(262-268)fmt(393-400)new(309-311)key_id(74-76)key_id(145-147)key_id(170-172)index(313-315)imported(357-359)sign(178-182)from(151-156)from(186-191)from(195-200)from(204-209)from(213-218)from(290-292)from(296-298)from(384-389)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(4-4)crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/key_manager.rs (2)
new(62-76)clone(333-341)crates/wallet/sdk/src/key_managers/backend.rs (1)
sign(25-39)
crates/wallet/sdk/src/sdk.rs (4)
crates/wallet/sdk/src/apis/signer.rs (3)
fmt(104-110)new(22-24)clone(86-90)crates/wallet/sdk/src/apis/key_manager.rs (3)
key_store(78-80)new(62-76)clone(333-341)crates/wallet/sdk/src/apis/config.rs (1)
new(18-23)crates/wallet/sdk/src/local_key_store.rs (1)
cipher_seed(33-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). (4)
- GitHub Check: clippy
- GitHub Check: check stable
- GitHub Check: test
- GitHub Check: check nightly
🔇 Additional comments (8)
crates/wallet/sdk/src/apis/signer.rs (2)
17-24: SignerApi genericization and signing flow look correct.Using
KeyManagerApi<'a, TSpec>as the only dependency and branching onKeyId::{Derived, Imported}withSignable/IntoSignedis clean and matches the new key-store–driven model. The derived-key path delegates tokey_store().sign(...), and the imported-key path constructs a consistentSignatureOutputfrom the fetched key, which is what external callers expect.Also applies to: 26-56, 59-78
81-91: Error surface and Clone impl are consistent with the new spec-based APIs.
SignerApiError<TSpec>cleanly wraps key store, storage, and key-manager errors, and the customDebugimpl provides useful variant context. TheCloneimpl simply delegates toKeyManagerApi::clone, which keeps cloning behavior straightforward.Also applies to: 93-111
crates/wallet/sdk/src/sdk.rs (6)
55-61: Core WalletSdk initialization and network invariant are sound.Using
WalletSdk<TSpec: WalletSdkSpec>withinitialize(...)that callscheck_or_set_store_networkgives a clear construction path and prevents mixing data from different networks, with a precise error message when there is a mismatch.get_store_networkandcheck_or_set_store_networkare straightforward and reuseConfigApicleanly.Also applies to: 63-71, 82-102
104-123: API accessors correctly wrap the spec-based components.The various
*_apiaccessors (config, key manager, signer, locks, events, accounts, confidential/stealth APIs, templates, NFTs, etc.) now consistently use the spec-associated types and pass references/values in line with the constructors from the other modules. The updated docs forkey_manager_apiandsigner_apicorrectly describe the pluggable key-store backend.Also applies to: 132-148, 150-195, 201-227, 229-235
238-265: Local key-store initialization and cipher-seed loading are coherent.
initialize_with_local_key_storecorrectly reuses the network check, loads an encrypted cipher seed viaload_cipher_seed, wraps it inWalletCipherSeed, and constructsLocalKeyStorewith the appropriate initial state. Theload_cipher_seedhelper cleanly combinesConfigApiandPasswordManagerApito decrypt the stored seed, while safely treating a missing cipher seed as an expected “not yet initialized” case.Also applies to: 266-280
282-300: Cipher-seed creation, restore, and readout integrate well with LocalKeyStore.
create_cipher_seedandrestore_cipher_seed_from_seed_wordsboth persist the encrypted cipher seed throughConfigApiand then push the plaintext seed intoLocalKeyStoreviaset_cipher_seed, keeping store and key store in sync.load_seed_wordsnow correctly sources the current seed fromkey_store.cipher_seed(), which is the right abstraction boundary for the local backend.Also applies to: 302-310
356-372: Clone and Debug implementations for WalletSdk are appropriate.The manual
Cloneimpl respects the spec’s associated types and only requiresStore,NetworkInterface, andKeyStoreto beClone, whileDebugfocuses onconfigandepoch_birthday, which are the most relevant fields to log. This keeps cloning and logging behavior explicit and predictable.Also applies to: 374-380
312-353: Fix semantic inconsistency forRecoveryNeededflag on wallet upgrades.The review concern is valid. On upgraded wallets with an existing cipher seed but no
RecoveryNeededflag in the database,initialize_cipher_seed()will error withInvariantErrorat line 335, whileis_recovery_needed()silently defaults tofalseat line 317. Sinceinitialize_cipher_seed()is called on every startup after migrations (confirmed in tari_walletd and test code), this breaks wallet loading for any pre-existing database that lacks the flag.Recommend either:
- (a) Backfill
RecoveryNeededduring database migrations or first-timeinitialize_cipher_seed()call, or- (b) Treat missing flag consistently as
falseat line 333—adjust the error handling to default tofalseinstead of treating it as an invariant violation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/wallet/sdk/src/sdk.rs (1)
104-236: LGTM! API accessors correctly use spec-based types.All API accessor methods have been properly updated to use
TSpecassociated types (TSpec::Store,TSpec::NetworkInterface,TSpec::KeyStore). The documentation correctly describes the key store as "configured" rather than "local", addressing previous review feedback.
🧹 Nitpick comments (2)
integration_tests/src/lib.rs (1)
127-127: Consider propagating the error instead of unwrapping.The
unwrap()will panic ifKeyManager::new_random()fails. While acceptable in test code, propagating the error would provide better diagnostics.Apply this diff to improve error handling:
- async fn init() -> Self { + async fn init() -> Result<Self, Box<dyn std::error::Error>> { let wallet_private_key = PrivateKey::random(&mut OsRng); let default_payment_address = TariAddress::new_single_address( CompressedPublicKey::from_secret_key(&wallet_private_key), L1Network::LocalNet, TariAddressFeatures::create_interactive_and_one_sided(), ) .unwrap(); - Self { + Ok(Self { consensus_constants: ConsensusConstants::devnet(), base_nodes: IndexMap::new(), wallets: IndexMap::new(), validator_nodes: IndexMap::new(), indexers: IndexMap::new(), vn_seeds: IndexMap::new(), miners: IndexMap::new(), templates: IndexMap::new(), outputs: IndexMap::new(), http_server: None, template_mock_server_port: None, current_scenario_name: None, claim_proofs: HashMap::new(), substate_ids: IndexMap::new(), num_databases_saved: 0, - key_manager: KeyManager::new_random().unwrap(), + key_manager: KeyManager::new_random()?, wallet_accounts: IndexMap::new(), wallet_daemons: IndexMap::new(), minotari_wallet_private_key: wallet_private_key, default_payment_address, consensus_manager: ConsensusManager::builder(L1Network::LocalNet).build(), eviction_proofs: HashMap::new(), - } + }) }crates/wallet/sdk/src/local_key_store.rs (1)
55-65: Review error variants for relevance.The
LocalKeyStoreErrorenum includesPasswordManagerandWalletStoragevariants, but the refactoredLocalKeyStoreno longer directly depends onPasswordManagerApior wallet storage. Verify these error variants are still needed or can be removed.#!/bin/bash # Check if PasswordManager or WalletStorage errors are constructed anywhere rg -nC3 'LocalKeyStoreError::(PasswordManager|WalletStorage)' --type=rust # Check conversions from these error types to LocalKeyStoreError rg -nC3 'PasswordManagerApiError.*LocalKeyStoreError|WalletStorageError.*LocalKeyStoreError' --type=rust
📜 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 (9)
crates/wallet/sdk/Cargo.toml(2 hunks)crates/wallet/sdk/src/apis/key_manager.rs(13 hunks)crates/wallet/sdk/src/cipher_seed.rs(1 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/local_key_store.rs(2 hunks)crates/wallet/sdk/src/sdk.rs(10 hunks)integration_tests/src/lib.rs(4 hunks)integration_tests/src/miner.rs(3 hunks)integration_tests/tests/steps/wallet_daemon.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/tests/steps/wallet_daemon.rs
🧰 Additional context used
🧬 Code graph analysis (5)
crates/wallet/sdk/src/cipher_seed.rs (1)
crates/wallet/sdk/src/local_key_store.rs (1)
cipher_seed(31-33)
integration_tests/src/miner.rs (1)
crates/wallet/sdk/src/models/key.rs (3)
key_id(74-76)key_id(145-147)key_id(170-172)
crates/wallet/sdk/src/local_key_store.rs (2)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(31-36)crates/wallet/sdk/src/key_managers/backend.rs (2)
derive_secret(22-22)key_birthday(43-43)
crates/wallet/sdk/src/sdk.rs (6)
crates/wallet/sdk/src/apis/key_manager.rs (3)
key_store(74-76)new(58-72)clone(329-337)crates/wallet/sdk/src/local_key_store.rs (2)
new(22-24)cipher_seed(31-33)crates/wallet/sdk/src/apis/confidential_outputs.rs (1)
new(40-50)crates/wallet/sdk/src/apis/accounts.rs (1)
new(67-81)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
new(52-72)crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
new(81-93)key_manager_api(397-414)
crates/wallet/sdk/src/apis/key_manager.rs (2)
crates/wallet/crypto/src/encryption.rs (2)
decrypt_with_password(30-98)encrypt_with_password(100-134)crates/wallet/sdk/src/models/key.rs (13)
new(309-311)derived(353-355)branch(317-319)index(313-315)secret(166-168)from(151-156)from(186-191)from(195-200)from(204-209)from(213-218)from(290-292)from(296-298)from(384-389)
⏰ 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: check stable
- GitHub Check: machete
- GitHub Check: clippy
- GitHub Check: test
🔇 Additional comments (30)
crates/wallet/sdk/Cargo.toml (2)
23-23: New dependencies appropriately aligned with key store backend changes.The additions of
tari_hashingandrandare reasonable for supporting key derivation and randomization in the pluggable key store backend implementation.Also applies to: 36-36
42-42: The change is safe and correctly implemented.Verification confirms that
tari_transaction_componentshas been properly moved to dev-dependencies with no impact on SDK runtime code. Key findings:
- SDK source files import from
tari_transaction(main dependency), nottari_transaction_components- No imports of
tari_transaction_componentsexist incrates/wallet/sdk/src/tari_transactionremains in main dependencies and supports all runtime needsThe trait-based abstraction refactoring is sound—the SDK's production code has no dependency on
tari_transaction_components.integration_tests/src/lib.rs (2)
49-49: LGTM! Import updated for unified KeyManager.The import change from
MemoryDbKeyManagertoKeyManageraligns with the PR's objective of introducing a pluggable key store backend.
91-91: LGTM! Field type updated consistently.The field type change is consistent with the new KeyManager abstraction.
integration_tests/src/miner.rs (4)
35-35: LGTM! Import aligned with KeyManager refactoring.The import change is consistent with the broader refactoring to use the unified
KeyManagertype.
94-94: LGTM! Call site updated for synchronous method.The removal of
.awaitcorrectly reflects the change toscript_key_idbecoming a synchronous method inlib.rs.
125-125: LGTM! Parameter type updated consistently.The parameter type change from
MemoryDbKeyManagertoKeyManageris consistent with the pluggable key store backend refactoring.
154-167: Verify external dependency:generate_coinbase_with_wallet_outputintari_transaction_componentsis synchronous.The function is imported from an external crate (
tari_transaction_componentsfromhttps://github.com/tari-project/tari.git, branchdevelopment) and is called synchronously at line 154 without.await. While the call site correctly reflects synchronous usage, the actual function signature cannot be verified from within this repository. Manually confirm that the external function definition intari_transaction_componentsis a synchronous function (notasync fn) and that this change aligns with any recent KeyManager API refactoring in the Tari project.crates/wallet/sdk/src/cipher_seed.rs (1)
21-36: LGTM! Type alias improves code clarity.The
SafeCipherSeedtype alias provides better semantics forArc<CipherSeed>and is used consistently throughout the codebase. The refactor maintains backward compatibility while improving readability.crates/wallet/sdk/src/lib.rs (1)
11-18: LGTM! Module visibility changes support the new architecture.The changes appropriately expose the
local_key_storemodule and re-export thespecmodule contents, enabling the pluggable key store backend design while maintaining a clean public API surface.crates/wallet/sdk/src/local_key_store.rs (4)
16-29: LGTM! Simplified LocalKeyStore design.The refactored
LocalKeyStoreremoves generic dependencies and provides a clean interface for cipher seed management. The fluent API pattern inset_cipher_seedis well-implemented.
31-37: LGTM! Clear accessor methods.The distinction between
cipher_seed()(returnsOption) andget_cipher_seed()(returnsResult) provides appropriate flexibility for different usage contexts.
40-53: LGTM! Clean WalletKeyStore implementation.The trait implementation correctly delegates to the
derive_private_keyhelper and provides appropriate birthday access via the cipher seed.
67-93: Verify that the const assertion protects the correct type.The const assertion at lines 87-89 checks
RistrettoSecretKey::WIDE_REDUCTION_LENbut the actual type being constructed isPrivateKey(line 92). AlthoughPrivateKeyis an application-level wrapper aroundRistrettoSecretKey, verify that both types have identical length requirements forfrom_uniform_bytes. IfPrivateKey::from_uniform_byteshas different length expectations thanRistrettoSecretKey::WIDE_REDUCTION_LEN, the compile-time assertion may not provide the safety guarantee intended.crates/wallet/sdk/src/apis/key_manager.rs (10)
49-76: LGTM! Well-structured generic API.The refactored
KeyManagerApiproperly usesWalletSdkSpecassociated types and provides a clean constructor. Thekey_store()accessor enables internal components to access the key store as needed.
78-96: LGTM! Correct KeyId usage.The
get_all_derived_keysmethod correctly uses the updatedKeyId::derived(branch, index)signature and properly derives keys through the key store.
98-112: LGTM! Secure key import with canonical validation.The
get_imported_keymethod properly decrypts the stored key and validates it's in canonical form, which is an important security measure to prevent malleability issues.
114-131: LGTM! Secure key import with encryption.The
import_keymethod correctly encrypts the secret key before storage and provides appropriate error handling for encryption failures.
133-165: LGTM! Flexible key access methods.The
get_keyandget_public_keymethods properly handle both imported and derived keys. TheInto<KeyId>bound onget_public_keyprovides nice ergonomic flexibility.
174-221: LGTM! Consistent DerivedKeyId usage.The key derivation methods correctly use the new
DerivedKeyIdstructure that consolidates branch and index. Thederive_account_addressmethod properly derives both account and view keys.
243-280: LGTM! Proper key index management.The
next_key,next_public_key, andnext_derived_key_idmethods correctly manage key indices using the newDerivedKeyIdstructure. The special handling to ensure the view key branch is created alongside the account branch (line 272) is appropriate.
312-325: LGTM! Updated birthday access.The method correctly uses the renamed
key_birthday()trait method and maintains the same birthday-to-epoch calculation logic.
328-338: LGTM! Appropriate Clone implementation.The
Cloneimplementation correctly clones all fields. Since they are references, this is efficient.
340-363: LGTM! Appropriate error variant addition.The new
CipherErrorvariant properly handles encryption/decryption errors from the new key import/export functionality.crates/wallet/sdk/src/sdk.rs (6)
48-102: LGTM! Robust SDK initialization with network validation.The refactored
WalletSdk<TSpec>properly uses theWalletSdkSpectrait to abstract storage, network, and key store backends. Thecheck_or_set_store_networkmethod provides important validation to ensure configuration consistency between the in-memory config and persisted database state.
238-263: LGTM! Convenient initialization for LocalKeyStore.The
initialize_with_local_key_storemethod provides a convenient initialization path for the common case of usingLocalKeyStore. It properly loads any existing cipher seed or defaults toNone.
265-309: LGTM! Complete cipher seed lifecycle management.The cipher seed lifecycle methods (
load_cipher_seed,create_cipher_seed,restore_cipher_seed_from_seed_words,load_seed_words) provide comprehensive seed management functionality with proper encryption/decryption and key store integration.
311-353: LGTM! Robust cipher seed initialization logic.The
initialize_cipher_seedmethod handles both new wallet creation and seed recovery scenarios appropriately. The warning when seed words are provided for an already-initialized wallet (lines 327-330) is good defensive programming to prevent accidental data loss.
355-380: LGTM! Appropriate Clone and Debug implementations.The
Cloneimplementation has correct trait bounds, and theDebugimplementation properly excludes sensitive data like keys and store contents, only showing configuration and birthday information.
382-398: LGTM! Comprehensive error handling.The
WalletSdkErrorenum covers all necessary error cases. TheInvariantErrorvariant with descriptive details is particularly useful for catching and debugging logic errors.
2 participants
Description
feat(wallet/sdk): pluggable key store backend
fix(wallet/daemon): use hard-coded genesis resources instead of fetching them on startup
Motivation and Context
Allows the SDK to use other key store implementations, allowing for usage with HSMs
Genesis resources are immutable, so instead of fallibly fetching them from the network on startup, we'll use the hard-coded genesis state.
How Has This Been Tested?
Manually
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Refactoring
Bug Fixes & Improvements
UI/Documentation