fix(wallet)!: adds wallet signer API - #1601
Conversation
WalkthroughThis PR refactors key management and signing across the wallet stack: moves KeyBranch into models, replaces build_and_seal with unsigned build + local signer flow, introduces Signable/IntoSigned traits and a Signer API, adds a local key manager/backend, transitions SDK APIs to WalletPublicKey/WalletSecretKey, and versions signature messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Walletd as Walletd Handler
participant SDK as Wallet SDK
participant Signer as SignerApi(Local)
participant KM as KeyManagerBackend
participant Tx as Transaction (Unsigned/Signed)
Client->>Walletd: Request (e.g., transfer)
Walletd->>SDK: Build unsigned transaction
SDK-->>Walletd: Unsigned Tx
Walletd->>Signer: sign(KeyBranch::{Account|Nonce}, KeyId, Unsigned Tx)
activate Signer
Signer->>KM: try_sign(branch.as_str(), key_id, item.as_signing_message(ctx))
KM-->>Signer: {public_key, signature}
Signer-->>Walletd: item.into_signed(public_key, signature)
deactivate Signer
Walletd->>SDK: Submit signed transaction
SDK-->>Client: Result
sequenceDiagram
autonumber
participant TxB as TransactionBuilder
participant Traits as Signable/IntoSigned
participant Signer as SignerApi
participant KM as KeyManagerBackend
TxB->>TxB: build() -> Unsigned
Signer->>TxB: get signing message via Signable
Signer->>KM: try_sign(..., message[64])
KM-->>Signer: signature + pubkey
Signer->>TxB: IntoSigned -> append TransactionSignature
TxB-->>Signer: Signed Transaction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/storage/src/consensus_models/leader_fee.rs (1)
49-56: Overflow risk in branch condition can break fee conservation; compare using division instead of mulThe check
(leader_fee + 1) * num_involved_shards.get() <= transaction_feecan overflow u64, leading to a wrong branch decision and invalid burn/payout totals. Replace with a division-based comparison that avoids overflow.Apply:
pub fn calculate_leader_fee(transaction_fee: u64, num_involved_shards: NonZeroU64, exhaust_divisor: u64) -> LeaderFee { let target_burn = transaction_fee.checked_div(exhaust_divisor).unwrap_or(0); let block_fee_after_burn = transaction_fee - target_burn; - let mut leader_fee = block_fee_after_burn / num_involved_shards; - // The extra amount that is burnt from dividing the number of shards involved - let excess_remainder_burn = block_fee_after_burn % num_involved_shards; + let shards = num_involved_shards.get(); + let mut leader_fee = block_fee_after_burn / shards; + // The extra amount that is burnt from dividing the number of shards involved + let excess_remainder_burn = block_fee_after_burn % shards; // Adjust the leader fee to account for the remainder // If the remainder accounts for an extra burn of greater than half the number of involved shards, we // give each validator an extra 1 in fees if enough fees are available, burning less than the exhaust target. // Otherwise, we burn a little more than/equal to the exhaust target. - let actual_burn = if excess_remainder_burn > 0 && - // If the div floor burn accounts for 1 less fee for more than half of number of shards, and ... - excess_remainder_burn >= num_involved_shards.get() / 2 && - // ... if there are enough fees to pay out an additional 1 to all shards - (leader_fee + 1) * num_involved_shards.get() <= transaction_fee + let actual_burn = if excess_remainder_burn > 0 && + // If the div floor burn accounts for 1 less fee for more than half of number of shards, and ... + excess_remainder_burn >= shards / 2 && + // ... if there are enough fees to pay out an additional 1 to all shards (overflow-safe) + leader_fee < transaction_fee / shards { // Pay each leader 1 more leader_fee += 1; - // We burn a little less (< num_involved_shards) due to the remainder - target_burn.saturating_sub(num_involved_shards.get() - excess_remainder_burn) + // We burn a little less (< num_involved_shards) due to the remainder + target_burn.saturating_sub(shards - excess_remainder_burn) } else { - // We burn a little more (< num_involved_shards) due to the remainder - target_burn + excess_remainder_burn + // We burn a little more (< num_involved_shards) due to the remainder + target_burn + excess_remainder_burn };This preserves behavior while eliminating overflow. Optionally, add
debug_assert!(target_burn >= shards - excess_remainder_burn);in the true branch to document the invariant.Also applies to: 58-63
utilities/tariswap_test_bench/src/tariswap.rs (1)
32-58: Migrate from derive_account_key/build_and_seal to local_signer_apiThis still uses derive_account_key + build_and_seal, which undermines the PR goal to remove secret-key handling and standardize signing. Please switch to the local signer API with KeyBranch::Account and the in_account’s owner_key_id.
Example refactor:
- let key = self.sdk.key_manager_api().derive_account_key(0)?; + // Use the paying account's owner key id for signing fees + let fee_key_id = in_account.owner_key_id.expect("no owner key id"); ... - .build_and_seal(&key.key); + .build(); + + let transaction = self + .sdk + .local_signer_api() + .sign(KeyBranch::Account, fee_key_id, transaction)?;utilities/tariswap_test_bench/src/accounts.rs (1)
156-188: Migrate fund_accounts to local_signer_api; remove secret usageThis still uses derive_account_key + build_and_seal. Please switch to the local signer and the fee_account’s owner_key_id to align with the new security posture.
- let key = self.sdk.key_manager_api().derive_account_key(0)?; + let fee_key_id = fee_account + .owner_key_id + .expect("fee_account has no owner_key_id"); ... - .build_and_seal(&key.key); + .build(); + + let transaction = self + .sdk + .local_signer_api() + .sign(KeyBranch::Account, fee_key_id, transaction)?;applications/tari_walletd/src/handlers/keys.rs (1)
63-68: Branch parameter ignored in set_active/get_active.set_active_key/get_active_key are hardcoded to KeyBranch::Account. If KeysSetActiveRequest carries a branch, use it to avoid surprising behavior.
Suggested change:
- km.set_active_key(KeyBranch::Account, req.index)?; - let key = km.get_active_key(KeyBranch::Account)?; + km.set_active_key(req.branch, req.index)?; + let key = km.get_active_key(req.branch)?;
🧹 Nitpick comments (15)
utilities/tariswap_test_bench/src/tariswap.rs (2)
94-100: Unify key accessors for forward-compatibilityThis uses public_key() while other places (Line 226 and in accounts.rs) use the public field. Pick one style project‑wide; prefer methods to avoid relying on public fields.
222-227: Consistency: use the same accessor styleUse the same accessor style as earlier for the public key.
- let primary_account_pk = primary_account_key.public_key.to_byte_type(); + let primary_account_pk = primary_account_key.public_key().to_byte_type();utilities/tariswap_test_bench/src/accounts.rs (1)
25-30: Optional: prefer getter methods over public fieldsFor consistency with tariswap.rs, consider using methods (e.g., public_key(), key_id()) to avoid relying on public fields.
Also applies to: 89-93, 107-109
applications/tari_walletd/src/handlers/transaction.rs (1)
330-358: Consider adding a clarifying comment for the multi-step signing flow.The signing logic here handles multiple scenarios with conditional signing, signature extraction, and a final signing step. While the implementation appears correct, a brief comment explaining the signing flow would improve maintainability:
- Lines 332-341: Sign with non-owner key if different from account owner
- Lines 342-343: Extract signatures and build unsigned transaction
- Lines 350-353: Build with collected signatures
- Lines 355-357: Add final signature with account owner key
Example comment:
+ // Multi-signature flow: + // 1. Sign with non-owner key if specified (via sign_with_context) + // 2. Build unsigned transaction and collect signatures + // 3. Rebuild with signatures and authorize the sealed signer + // 4. Add final signature with account owner key let transaction = context .transaction_builder()crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (2)
38-39: Naming/readability: variable suggests pubkey, but holds a secret.view_key is now a WalletSecretKey. Consider renaming to view_secret_key (and param) to avoid confusion and accidental misuse.
Also applies to: 60-61
266-271: Avoid direct field access of secret; use an accessor.Accessing WalletSecretKey.secret directly ties callers to struct layout and weakens encapsulation. Prefer a method (e.g., secret() or as_secret()) and keep the field private.
crates/common_types/src/signable.rs (2)
6-10: Trait shape looks good; add minimal docs for contract clarity.Brief doc comments explaining message domain separation expectations and typical Ctx usage will help prevent ambiguous implementations.
12-16: IntoSigned API is straightforward.No issues spotted. Consider a short doc note that self should reflect the final pre-sign state to avoid post-sign mutation hazards.
crates/wallet/sdk/src/key_managers/backend.rs (1)
13-17: Use typed KeyBranch and constrain message type to avoid stringly-typed bugsPrefer a strongly-typed branch and explicit message bound. This reduces runtime mistakes and clarifies expectations for backends.
-use crate::models::KeyId; +use crate::models::{KeyBranch, KeyId}; -pub trait KeyManagerBackend<M> { +pub trait KeyManagerBackend<M: AsRef<[u8]>> { type Error; - fn try_sign(&mut self, branch: &str, key_id: KeyId, message: M) -> Result<SignatureOutput, Self::Error>; + fn try_sign( + &mut self, + branch: KeyBranch, + key_id: KeyId, + message: M, + ) -> Result<SignatureOutput, Self::Error>; }crates/transaction/src/v1/signature.rs (3)
91-105: Remove magic literal; define a V1 schema constantAvoid hardcoding 1. Define a module const and use it in both sign and verify to reduce drift.
+const V1_SCHEMA_VERSION: u16 = 1; @@ - let message = Self::create_message(1, seal_signer, transaction); + let message = Self::create_message(V1_SCHEMA_VERSION, seal_signer, transaction);
107-116: Mirror the constant in verificationKeep sign/verify consistent by using the same constant.
- pub fn verify_v1(&self, seal_signer: &RistrettoPublicKeyBytes, transaction: &UnsignedTransactionV1) -> bool { - let message = Self::create_message(1, seal_signer, transaction); + pub fn verify_v1(&self, seal_signer: &RistrettoPublicKeyBytes, transaction: &UnsignedTransactionV1) -> bool { + let message = Self::create_message(V1_SCHEMA_VERSION, seal_signer, transaction);
126-137: Optional: assert schema-version cohesion in debug buildsAdd a debug assertion to catch mismatched schema versions when future-proofing.
pub fn create_message( schema_version: u16, seal_signer: &RistrettoPublicKeyBytes, transaction: &UnsignedTransactionV1, ) -> [u8; 64] { + debug_assert_eq!(schema_version, V1_SCHEMA_VERSION, "Unexpected schema_version for v1"); let signature_fields = TransactionSignatureFields::from(transaction); engine_hasher64(EngineHashDomainLabel::TransactionSignature) .chain(&schema_version) .chain(seal_signer) .chain(&signature_fields) .result() }crates/wallet/sdk/src/local_key_store.rs (1)
29-47: Secret handling looks correct; add a brief doc comment warning against loggingget_imported_secret returns sensitive material. Add a short doc comment reminding consumers not to log or persist it, and to keep scope minimal. RistrettoSecretKey typically zeroizes on drop, but caller hygiene still matters.
crates/wallet/sdk/src/apis/signer.rs (1)
21-35: Align with a typed branch in backend; avoid passing &strIf KeyManagerBackend takes KeyBranch (recommended), drop as_str() and pass the enum directly.
- let message = item.as_signing_message(context); - let signature = self.backend.try_sign(branch.as_str(), key_id, message)?; + let message = item.as_signing_message(context); + let signature = self.backend.try_sign(branch, key_id, message)?;crates/wallet/sdk/src/key_managers/local.rs (1)
34-37: Clarify the warning comment.The warning about not using
next_keyis helpful, but could be more explicit about why this always returns the same key (because the key manager is initialized with index 0 and never incremented).Consider rewording for clarity:
- /// WARNING: dont use next_key on the key manager because this will always return the same key + /// WARNING: Don't use next_key on this key manager. Since it's initialized with index 0 and never + /// incremented, next_key would always return the same key. Use derive_key with explicit indices instead. fn get_key_manager(&mut self, branch: &str) -> WalletKeyManager {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs(1 hunks)applications/tari_wallet_cli/src/command/key.rs(1 hunks)applications/tari_walletd/src/handlers/accounts.rs(6 hunks)applications/tari_walletd/src/handlers/confidential.rs(2 hunks)applications/tari_walletd/src/handlers/keys.rs(2 hunks)applications/tari_walletd/src/handlers/nfts.rs(3 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(1 hunks)applications/tari_walletd/src/handlers/transaction.rs(5 hunks)applications/tari_walletd/src/handlers/validator.rs(4 hunks)applications/tari_walletd/src/main.rs(1 hunks)clients/wallet_daemon_client/src/lib.rs(1 hunks)clients/wallet_daemon_client/src/types.rs(1 hunks)crates/common_types/src/lib.rs(2 hunks)crates/common_types/src/signable.rs(1 hunks)crates/engine/src/runtime/working_state.rs(1 hunks)crates/storage/src/consensus_models/leader_fee.rs(1 hunks)crates/transaction/src/builder/mod.rs(3 hunks)crates/transaction/src/transaction.rs(1 hunks)crates/transaction/src/unsigned_transaction.rs(3 hunks)crates/transaction/src/v1/signature.rs(3 hunks)crates/transaction/src/v1/transaction.rs(1 hunks)crates/transaction/src/v1/unsealed.rs(4 hunks)crates/transaction/src/v1/unsigned.rs(2 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(2 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(1 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/signer.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(7 hunks)crates/wallet/sdk/src/apis/stealth_transfer.rs(3 hunks)crates/wallet/sdk/src/key_managers/backend.rs(1 hunks)crates/wallet/sdk/src/key_managers/local.rs(1 hunks)crates/wallet/sdk/src/key_managers/mod.rs(1 hunks)crates/wallet/sdk/src/lib.rs(1 hunks)crates/wallet/sdk/src/local_key_store.rs(1 hunks)crates/wallet/sdk/src/models/key.rs(7 hunks)crates/wallet/sdk/src/sdk.rs(3 hunks)crates/wallet/sdk_services/src/account_recovery/service.rs(1 hunks)crates/wallet/sdk_services/src/indexer_rest_api.rs(2 hunks)crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs(3 hunks)integration_tests/tests/steps/wallet.rs(1 hunks)integration_tests/tests/steps/wallet_daemon.rs(1 hunks)utilities/tariswap_test_bench/src/accounts.rs(5 hunks)utilities/tariswap_test_bench/src/tariswap.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (37)
crates/transaction/src/transaction.rs (3)
crates/transaction/src/unsigned_transaction.rs (1)
schema_version(21-25)crates/transaction/src/v1/transaction.rs (1)
schema_version(44-46)crates/transaction/src/v1/unsealed.rs (1)
schema_version(42-44)
crates/wallet/sdk/src/key_managers/backend.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/key_managers/local.rs (1)
try_sign(47-65)crates/wallet/sdk/src/local_key_store.rs (1)
get_imported_secret(32-46)
crates/wallet/sdk/src/apis/confidential_transfer.rs (2)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/OutputStatus.ts (1)
OutputStatus(3-3)
crates/transaction/src/v1/unsigned.rs (4)
crates/common_types/src/signable.rs (1)
as_signing_message(9-9)crates/transaction/src/builder/mod.rs (1)
as_signing_message(488-490)crates/transaction/src/unsigned_transaction.rs (1)
as_signing_message(161-165)crates/transaction/src/v1/signature.rs (2)
create_message(71-76)create_message(126-137)
applications/tari_walletd/src/handlers/nfts.rs (2)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/Account.ts (1)
Account(6-14)
utilities/tariswap_test_bench/src/accounts.rs (6)
bindings/src/types/Account.ts (1)
Account(6-14)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(298-300)crates/wallet/sdk/src/models/account.rs (2)
owner_public_key(37-39)owner_public_key(95-97)utilities/transaction_generator/src/transaction_builders/free_coins.rs (1)
builder(12-36)
applications/tari_wallet_cli/src/command/key.rs (2)
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/apis/signer.rs (2)
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_services/src/indexer_rest_api.rs (1)
networking/rpc_framework/src/status.rs (1)
details(114-116)
crates/transaction/src/unsigned_transaction.rs (7)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)crates/transaction/src/transaction.rs (2)
schema_version(245-249)network(81-85)crates/transaction/src/v1/transaction.rs (2)
schema_version(44-46)network(52-54)crates/transaction/src/v1/unsealed.rs (2)
schema_version(42-44)as_signing_message(130-135)crates/transaction/src/v1/unsigned.rs (3)
set_network(58-61)set_dry_run(63-66)as_signing_message(151-153)crates/common_types/src/signable.rs (1)
as_signing_message(9-9)crates/transaction/src/builder/mod.rs (1)
as_signing_message(488-490)
applications/tari_walletd/src/main.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/wallet/sdk/src/local_key_store.rs (2)
crates/wallet/crypto/src/encryption.rs (1)
decrypt_with_password(30-98)crates/wallet/sdk/src/key_managers/backend.rs (1)
get_imported_secret(22-22)
applications/tari_walletd/src/handlers/keys.rs (4)
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(298-300)bindings/src/types/wallet-daemon-client/KeysCreateResponse.ts (1)
KeysCreateResponse(4-4)
crates/transaction/src/builder/mod.rs (5)
crates/common_types/src/signable.rs (2)
as_signing_message(9-9)into_signed(15-15)crates/transaction/src/unsigned_transaction.rs (1)
as_signing_message(161-165)crates/transaction/src/v1/unsealed.rs (2)
as_signing_message(130-135)into_signed(141-146)crates/transaction/src/v1/unsigned.rs (1)
as_signing_message(151-153)crates/transaction/src/v1/signature.rs (6)
public_key(63-65)public_key(122-124)signature(59-61)signature(118-120)new(32-34)new(87-89)
crates/common_types/src/signable.rs (5)
crates/transaction/src/builder/mod.rs (2)
as_signing_message(488-490)into_signed(496-501)crates/transaction/src/unsigned_transaction.rs (1)
as_signing_message(161-165)crates/transaction/src/v1/unsealed.rs (2)
as_signing_message(130-135)into_signed(141-146)crates/transaction/src/v1/unsigned.rs (1)
as_signing_message(151-153)crates/transaction/src/v1/signature.rs (4)
public_key(63-65)public_key(122-124)signature(59-61)signature(118-120)
clients/wallet_daemon_client/src/types.rs (2)
bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/transaction/src/v1/signature.rs (5)
bindings/src/types/UnsignedTransactionV1.ts (1)
UnsignedTransactionV1(6-18)crates/transaction/src/transaction.rs (1)
schema_version(245-249)crates/transaction/src/unsigned_transaction.rs (1)
schema_version(21-25)crates/transaction/src/v1/unsealed.rs (1)
schema_version(42-44)crates/engine_types/src/hashing.rs (1)
engine_hasher64(35-37)
crates/wallet/sdk_services/src/utxo_scanner/scanner_round.rs (5)
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)crates/engine_types/src/resource.rs (1)
view_key(126-128)
crates/transaction/src/v1/transaction.rs (3)
crates/transaction/src/transaction.rs (1)
schema_version(245-249)crates/transaction/src/unsigned_transaction.rs (1)
schema_version(21-25)crates/transaction/src/v1/unsealed.rs (1)
schema_version(42-44)
crates/wallet/sdk/src/key_managers/local.rs (5)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/apis/key_manager.rs (1)
get_key_manager(332-341)crates/wallet/sdk/src/models/key.rs (12)
from(119-124)from(144-149)from(173-178)from(182-187)from(191-196)from(200-205)from(277-279)from(283-285)key_id(65-67)key_id(138-140)key_id(163-165)secret(159-161)crates/wallet/sdk/src/key_managers/backend.rs (1)
try_sign(16-16)
applications/tari_walletd/src/handlers/confidential.rs (2)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/engine_types/src/resource.rs (1)
view_key(126-128)
integration_tests/tests/steps/wallet_daemon.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/wallet/sdk/src/sdk.rs (2)
crates/wallet/sdk/src/cipher_seed.rs (1)
cipher_seed(29-34)crates/wallet/sdk/src/apis/key_manager.rs (1)
new(50-62)
applications/tari_walletd/src/handlers/transaction.rs (4)
crates/transaction/src/transaction.rs (3)
builder(47-49)signatures(99-103)inputs(123-127)crates/transaction/src/v1/unsigned.rs (2)
builder(33-35)inputs(76-78)crates/transaction/src/builder/mod.rs (1)
signatures(406-408)crates/transaction/src/v1/unsealed.rs (2)
signatures(77-79)inputs(96-98)
crates/transaction/src/v1/unsealed.rs (6)
crates/engine_types/src/hashing.rs (1)
engine_hasher64(35-37)crates/transaction/src/transaction.rs (2)
schema_version(245-249)new(51-53)crates/transaction/src/unsigned_transaction.rs (2)
schema_version(21-25)as_signing_message(161-165)crates/common_types/src/signable.rs (2)
as_signing_message(9-9)into_signed(15-15)crates/transaction/src/builder/mod.rs (3)
as_signing_message(488-490)into_signed(496-501)new(53-59)crates/transaction/src/v1/unsigned.rs (2)
as_signing_message(151-153)new(37-56)
crates/wallet/sdk/src/apis/key_manager.rs (4)
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 (3)
key_id(65-67)key_id(138-140)key_id(163-165)bindings/src/types/Account.ts (1)
Account(6-14)
crates/wallet/sdk/src/apis/stealth_transfer.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
clients/wallet_daemon_client/src/lib.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
bindings/src/types/EncryptedData.ts (1)
EncryptedData(7-7)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)crates/wallet/sdk/src/apis/stealth_crypto.rs (2)
decrypt_value_and_mask(130-146)encrypt_value_and_mask(103-114)
crates/wallet/sdk/src/models/key.rs (3)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/engine/src/runtime/working_state.rs (2)
crates/engine_types/src/fees.rs (1)
total_fees_paid(46-48)bindings/src/types/TransactionReceipt.ts (1)
TransactionReceipt(7-12)
applications/tari_walletd/src/handlers/validator.rs (5)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)bindings/src/types/Account.ts (1)
Account(6-14)crates/wallet/sdk/src/models/key.rs (4)
key_id(65-67)key_id(138-140)key_id(163-165)derived(298-300)crates/common_types/src/fee_pool.rs (1)
derive_fee_pool_address(9-20)
utilities/tariswap_test_bench/src/tariswap.rs (2)
bindings/src/types/Account.ts (1)
Account(6-14)bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
integration_tests/tests/steps/wallet.rs (1)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)
crates/wallet/sdk_services/src/account_recovery/service.rs (2)
bindings/src/types/wallet-daemon-client/KeyBranch.ts (1)
KeyBranch(3-10)bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)
applications/tari_walletd/src/handlers/accounts.rs (3)
bindings/src/types/wallet-daemon-client/KeyId.ts (1)
KeyId(3-3)crates/wallet/sdk/src/models/key.rs (4)
key_id(65-67)key_id(138-140)key_id(163-165)derived(298-300)crates/wallet/sdk/src/models/account.rs (2)
owner_key_id(33-35)owner_key_id(91-93)
⏰ 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: file licenses
- GitHub Check: check stable
- GitHub Check: fmt
- GitHub Check: clippy
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: machete
🔇 Additional comments (61)
crates/engine/src/runtime/working_state.rs (1)
1051-1062: LGTM! Clean refactor that improves readability.Extracting the
total_fees_paidcomputation into a local variable eliminates potential duplicate computation and makes the code clearer.crates/wallet/sdk_services/src/indexer_rest_api.rs (2)
29-35: LGTM! Import addition supports new error handling.The
Displayableimport is correctly added to support formatting error details in the newErrorResponsehandling branch below.
312-322: LGTM! Error handling correctly maps ErrorResponse to appropriate statuses.The logic appropriately distinguishes HTTP 400 errors (invalid requests →
TransactionRejected) from other errors (→InternalError). The use ofdetails.display()from the newly importedDisplayabletrait provides formatted error details for user-facing messages. The cast ofINVALID_REQUEST_CODEtou16is safe and necessary for comparison withsource.status().as_u16().utilities/tariswap_test_bench/src/tariswap.rs (4)
9-9: LGTM: import path updateUsing KeyBranch from models aligns with the new SDK structure.
149-158: Confirm sign_with_context’s context valueYou pass the primary account public key as context while authorizing the per‑account component. Please confirm this is the intended coupling (fee payer as context). If the context should be the account’s owner key instead, adjust accordingly.
160-165: LGTM: final fee-payer signatureSecond signature with primary account key to cover fees looks correct.
277-290: LGTM: two-step signer flowAuthorized seal + contextual sign + fee-payer sign matches the new API.
utilities/tariswap_test_bench/src/accounts.rs (4)
25-31: LGTM: switched to public key retrievalUsing get_public_key + local signer aligns with the new API.
48-51: LGTM: adopting local_signer_api for account creationSigns with KeyBranch::Account and the owner key id; looks correct.
102-116: LGTM: folded builder for multiple accountsThe fold over owners to create accounts and fee prepayment is clean and efficient.
140-143: LGTM: account registration with owner key_idPersisting owner.key_id for both owner and view-only ids matches current usage.
crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
18-18: LGTM: Import updated to reflect new key types.The import change from
KeytoWalletSecretKeyaligns with the broader refactor moving key representations toWalletSecretKeyandWalletPublicKeytypes throughout the wallet SDK.
268-268: LGTM: Parameter type updated consistently.The parameter type change to
WalletSecretKeyis consistent with the import changes and the usage at lines 292 and 315 wherekey.secretandkey.key_idare accessed.applications/tari_walletd/src/main.rs (1)
32-32: LGTM: Import path updated to reflect public API reorganization.The import path change moves
KeyBranchfromapis::key_managerto the publicmodelsmodule, aligning with the broader refactor to expose key-related types via the models module.crates/wallet/sdk/src/apis/mod.rs (1)
13-13: LGTM: New signer module added to public API.This addition expands the public API surface to expose the new signing functionality introduced in this PR, consistent with the PR objectives of adding a local signer API to the wallet SDK.
applications/tari_swarm_daemon/src/process_manager/processes/wallet_daemon.rs (1)
5-5: LGTM: Import path updated consistently.The import path change moves
KeyBranchto the publicmodelsmodule, consistent with the broader refactor across the codebase.applications/tari_walletd/src/handlers/stealth_utxos.rs (1)
82-82: LGTM: Simplified key retrieval using specialized getter.The change from
derive_key(KeyBranch::ElgamalEncryptionViewKey, id)toget_elgamal_encrypted_view_key(id)simplifies the API by using a specialized getter instead of a generic derivation method with branch parameter. The usage at lines 110 and 119 shows the return type is compatible.integration_tests/tests/steps/wallet_daemon.rs (1)
15-15: LGTM: Import path updated in integration tests.The import path change aligns the integration tests with the new public API where
KeyBranchis exported from themodelsmodule.integration_tests/tests/steps/wallet.rs (1)
14-14: LGTM: Import path updated in integration tests.The import path change maintains consistency with the new public API structure where
KeyBranchis now exported from themodelsmodule.clients/wallet_daemon_client/src/lib.rs (1)
40-40: LGTM: Client updated to use new public API path.The wallet daemon client import is updated to reference
KeyBranchfrom the publicmodelsmodule, ensuring the client uses the officially exported API surface. The public interface of the client remains unchanged.crates/transaction/src/v1/unsigned.rs (1)
148-154: LGTM! Signable implementation is correct.The
Signabletrait implementation forUnsignedTransactionV1correctly delegates toTransactionSignature::create_messagewith the appropriate schema version (1) for V1 transactions. This enables the signing flow described in the PR objectives.crates/common_types/src/lib.rs (1)
30-30: LGTM! Standard module addition.The
signablemodule is correctly added and publicly re-exported, making theSignableandIntoSignedtraits available as part of the public API for the signing flow.Also applies to: 54-54
crates/transaction/src/transaction.rs (1)
245-245: Schema version type narrowed to u16.The return type change from
u64tou16is a breaking change that aligns with the PR's stated objective to bind transaction signatures to schema version (v1). This change is consistent across all transaction types.applications/tari_wallet_cli/src/command/key.rs (1)
24-24: LGTM! Import path updated to reflect module restructuring.The consolidated import path for
KeyBranchandKeyIdfromtari_ootle_wallet_sdk::modelsaligns with the PR-wide refactoring that moves these types to the public models module.crates/wallet/sdk/src/key_managers/mod.rs (1)
1-7: LGTM! Well-organized module structure.The module correctly exposes the key management backend interfaces publicly while organizing the implementation details appropriately. The
backendmodule is re-exported for public access to signing traits, and thelocalsubmodule provides the local key manager implementation.crates/wallet/sdk_services/src/account_recovery/service.rs (1)
15-16: LGTM! Import paths updated for module restructuring.The reorganized imports reflect the migration of
KeyBranchandKeyIdto the public models module, consistent with the broader refactoring across the PR.crates/transaction/src/v1/transaction.rs (1)
44-44: Schema version type narrowed to u16.The return type change from
u64tou16for theschema_versionmethod is consistent with the schema versioning changes across the transaction module and aligns with the PR's breaking change to bind signatures to schema version.clients/wallet_daemon_client/src/types.rs (1)
42-48: LGTM! Import paths updated for consistency.The reorganized imports correctly reference
KeyBranchfrom the models module, maintaining consistency with the PR-wide refactoring that centralizes key management types in the public models API.crates/wallet/sdk/src/sdk.rs (1)
169-185: LGTM! Clear panic documentation.The
local_signer_api()method correctly panics with a clear error message if the cipher seed hasn't been initialized, and the documentation explicitly states this precondition. The panic is appropriate for this initialization requirement.applications/tari_walletd/src/handlers/confidential.rs (1)
290-290: LGTM! Improved API specificity.The change from
derive_key(KeyBranch::ElgamalEncryptionViewKey, ...)toget_elgamal_encrypted_view_key(...)is more specific and aligns with the principle of not exposing raw key material directly from the key manager API.crates/transaction/src/unsigned_transaction.rs (2)
21-25: LGTM! Schema versioning support added.The
schema_version()method correctly returns version 1 for V1 transactions, enabling version-aware transaction signing as described in the PR objectives. This is a key component of the breaking change that binds signatures to the schema version.
158-166: LGTM! Clean Signable trait implementation.The trait implementation correctly delegates signing message generation to the inner V1 transaction, maintaining the version-specific signing logic.
crates/transaction/src/builder/mod.rs (2)
69-71: LGTM! Builder pattern enhancement.Making
then()public enables external callers to compose builder operations with closures, improving the builder's flexibility and usability.
485-502: LGTM! Clean trait implementations for signing support.The
SignableandIntoSignedtrait implementations cleanly enable the TransactionBuilder to participate in the generic signing API. The implementations correctly:
- Delegate signing message generation to the underlying unsigned transaction
- Convert signing results (public key + signature) into a TransactionSignature and add it to the builder
applications/tari_walletd/src/handlers/keys.rs (1)
31-33: KeyId-driven public key retrieval is correct.The get_public_key/next_public_key flow aligns with the new KeyId API.
applications/tari_walletd/src/handlers/nfts.rs (3)
122-127: Switch to local_signer_api().sign is aligned with the new flow.Good migration from build_and_seal to build + local signer.
296-314: Verify intent: signer key vs. authorized seal signer context.You authorize the fee payer as the seal signer (with_authorized_seal_signer), then sign_with_context using the source account owner key and the fee payer’s public key as context. Confirm this is the intended authorization model (owner attests fee payer as seal signer) and that no further signatures are needed from the fee payer before submission.
If needed, I can scan call sites to ensure submit/seal paths don’t expect a separate fee-payer signature.
316-319: Final sign after build looks correct.The second sign binds the sealed transaction to the v1 schema as per new semantics.
Ensure no additional signatures are appended after this step, since the seal signature will commit to the current signature set.
crates/transaction/src/v1/unsealed.rs (4)
42-44: Schema version to u16 looks good.Matches the v1 versioning approach used elsewhere.
86-88: Switched to verify_v1 as expected.Consistent with message versioning changes.
138-147: IntoSigned wraps the provided signature without recomputation.Looks correct given the signer computes the signature over as_signing_message.
Double-check that the signing API for UnsealedTransactionV1 uses this exact message (schema_version + self).
127-136: Seal signature and signing message constructors are aligned. TransactionSealSignature::create_message and UnsealedTransactionV1::as_signing_message use the same engine_hasher64→schema_version→transaction sequence.crates/wallet/sdk/src/key_managers/local.rs (2)
47-65: LGTM!The signing implementation correctly handles both derived and imported keys, with appropriate error handling and clear documentation of infallibility assumptions.
68-84: LGTM!The error type design is sound, properly wrapping underlying errors and implementing
IsNotFoundErrorfor error propagation semantics.crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
235-281: LGTM!The refactoring to use
KeyBranch-based key retrieval is clean and improves the API surface. The change from public to private visibility forresolve_output_masks_for_spendingis appropriate since the functionality is now exposed through the new public methods below.
719-752: LGTM!The new public encryption/decryption methods provide a clean abstraction over the lower-level crypto API, properly integrating with the key manager workflow.
756-765: LGTM!The addition of
spend_key_branchtoTransferStatementParamsproperly supports the new key derivation workflow.applications/tari_walletd/src/handlers/accounts.rs (5)
31-31: LGTM!The import update reflects the move of
KeyBranchandKeyIdto the public models module, aligning with the broader API refactoring.
557-564: LGTM!The migration to the two-step build-then-sign pattern is consistent with the new signer API design. Using a throwaway nonce for signing is appropriate here since the claim burn instruction is authorized by the proofs rather than by the signer.
665-669: LGTM!Proper use of account-based signing for the create free test coins operation, which requires account authorization.
856-860: LGTM!The transfer handler correctly uses account-based signing as account authorization is required for withdrawals.
990-1005: LGTM!The conditional key selection logic is well-reasoned: account key signing is required when revealed amounts are involved, otherwise a throwaway nonce suffices. This aligns with the authorization model.
crates/wallet/sdk/src/models/key.rs (4)
14-54: LGTM!The
KeyBranchenum is well-designed with clear documentation for each variant. Theas_strmethod andAsRef<str>implementation provide convenient string conversions. The variants cover all necessary key derivation use cases.
127-150: LGTM!The
WalletPublicKeyabstraction cleanly pairs a public key with its identifier, providing a safer API surface than raw key types. The accessor methods andFrom<DerivedWalletKey>conversion are appropriately implemented.
152-206: LGTM!The
WalletSecretKeytype properly encapsulates a secret key with its identifier, and the multipleFromimplementations provide convenient conversions from various key types. Theto_public_keymethod is a useful addition.
208-213: LGTM!Updating
AccountAndViewKeysto useWalletSecretKeyinstead of the previousKeytype maintains consistency with the new type system.crates/wallet/sdk/src/apis/key_manager.rs (5)
126-145: LGTM!The return type changes from
KeytoWalletSecretKeymaintain API consistency with the new type system. Thepub(crate)visibility forget_keyappropriately restricts access to internal SDK usage.
147-165: LGTM!The new
get_public_keymethod is a useful addition that allows retrieving public keys without exposing secret keys. The TODO comment about optimizing imported key lookups is a good note for future improvements.
167-172: LGTM!The new
get_elgamal_encrypted_view_keymethod provides a clear, purpose-specific API for retrieving ElGamal encryption view keys, improving code readability at call sites.
260-270: LGTM!The
next_public_keymethod complementsnext_keyby returning only the public key representation, useful when secret keys aren't needed.
318-330: LGTM!Updating
get_key_or_activeto returnWalletPublicKeyinstead ofKeymaintains consistency with the new type system while preserving the method's convenience of falling back to the active key.
| id: key.key_id.derived_index().expect("Key is derived"), | ||
| public_key: key.public_key.to_byte_type(), | ||
| }) |
There was a problem hiding this comment.
Avoid panic on non-derived key; map to a user error.
expect("Key is derived") will crash the handler if invariants change. Convert this to a proper error and return 400/500 instead.
Apply this diff to handle the error explicitly:
- Ok(KeysCreateResponse {
- id: key.key_id.derived_index().expect("Key is derived"),
- public_key: key.public_key.to_byte_type(),
- })
+ let id = key
+ .key_id
+ .derived_index()
+ .ok_or_else(|| anyhow::anyhow!("Expected a derived key from key_manager_api()"))?;
+ Ok(KeysCreateResponse {
+ id,
+ public_key: key.public_key.to_byte_type(),
+ })🤖 Prompt for AI Agents
In applications/tari_walletd/src/handlers/keys.rs around lines 34 to 36, the
code calls key.key_id.derived_index().expect("Key is derived") which can panic;
replace the expect with explicit error handling that maps a non-derived key into
a user-facing error and returns an appropriate HTTP response (e.g., 400 Bad
Request) or Result::Err for internal handlers. Change the handler to propagate
the Result from derived_index() (or match its Option) and construct a clear
error variant/message when the key is not derived, then return that error (or
convert it to a response with status 400/500) instead of panicking.
| let key = self | ||
| .sdk | ||
| .key_manager_api() | ||
| .get_public_key(KeyBranch::Account, KeyId::derived(0))?; | ||
| let key_index_start = *account_key_indexes.start(); |
There was a problem hiding this comment.
Fee signer should match pay_fee_account.owner_key_id, not derived(0)
Signing the batch account creation with a hardcoded derived(0) will fail if the paying account uses another key id. Use pay_fee_account.owner_key_id.
- let key = self
- .sdk
- .key_manager_api()
- .get_public_key(KeyBranch::Account, KeyId::derived(0))?;
+ let pay_fee_key_id = pay_fee_account
+ .owner_key_id
+ .expect("pay_fee_account has no owner_key_id");
...
- let transaction = self
- .sdk
- .local_signer_api()
- .sign(KeyBranch::Account, key.key_id, transaction)?;
+ let transaction = self
+ .sdk
+ .local_signer_api()
+ .sign(KeyBranch::Account, pay_fee_key_id, transaction)?;Also applies to: 117-121
🤖 Prompt for AI Agents
In utilities/tariswap_test_bench/src/accounts.rs around lines 81-85 (and
similarly at 117-121), the fee signer key is being fetched using a hardcoded
KeyId::derived(0); replace that with the paying account's owner key id by using
pay_fee_account.owner_key_id when calling get_public_key / key retrieval so the
signer matches pay_fee_account.owner_key_id instead of always derived(0); ensure
both occurrences are updated and any variable names or types adjusted
accordingly.
Test Results (CI)472 tests ±0 462 ✅ +1 1h 31m 56s ⏱️ - 2m 16s For more details on these failures, see this check. Results for commit 402bde1. ± Comparison against base commit edd96aa. |
2 participants
Description
Adds a local signer API to the wallet SDK
Removed the usage of several key manager calls that return secrets
Removed a couple of key manager calls that return secrets.
fix!: bind the schema version to unsigned transaction signatures
Motivation and Context
General-purpose API that signs items that implement the
Signabletrait.This trait is implemented for unsigned transaction components, allowing the API to sign as follows:
The local signer API utilises the locally stored and encrypted Cipher key; however, the underlying signer is backend-agnostic, meaning that a cold wallet backend can be implemented. The backend trait is defined as follows:
How Has This Been Tested?
Manually and existing tests
What process can a PR reviewer use to test or verify this change?
Breaking Changes
BREAKING CHANGE: transactions signatures bind to the schema version (v1) - previous transaction signatures will be invalid after merge
Summary by CodeRabbit
New Features
Refactor
Bug Fixes