Skip to content

feat(engine)!: add TakeFromBucket instruction + wallet multiple transfers - #1614

Merged
sdbondi merged 2 commits into
tari-project:developmentfrom
sdbondi:wallet-send-multiple-recipients
Oct 24, 2025
Merged

feat(engine)!: add TakeFromBucket instruction + wallet multiple transfers#1614
sdbondi merged 2 commits into
tari-project:developmentfrom
sdbondi:wallet-send-multiple-recipients

Conversation

@sdbondi

@sdbondi sdbondi commented Oct 24, 2025

Copy link
Copy Markdown
Member

Description

feat(engine)!: add TakeFromBucket instruction
feat(wallet)!: support for multiple outputs/stealth transfers in one transaction

Motivation and Context

Allows buckets to be split at the transaction-level
Allows spending to multiple recipients in one transaction

How Has This Been Tested?

New unit tests, manually, existing integration tests

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

Summary by CodeRabbit

  • New Features

    • Multi-output stealth transfers: send to multiple destinations in one transaction.
    • New "take from bucket" instruction to move specified amounts between workspace buckets.
  • Refactoring

    • Stealth transfer payload now uses a transfers array with per-output handling and improved account/deposit flow.
    • Improved workspace handling and validation for more accurate finalization checks.
  • Chores

    • Bumped workspace and client package versions.

@coderabbitai

coderabbitai Bot commented Oct 24, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Refactors stealth transfer requests to use a per-transfer transfers collection and introduces a new TakeFromBucket instruction with workspace bucket semantics. Also adds runtime workspace helpers, updates builders to accept optional address allocation, and updates bindings, clients, tests, and version bumps.

Changes

Cohort / File(s) Summary
Stealth transfer SDK (Rust)
crates/wallet/sdk/src/apis/stealth_transfer/... (api.rs, error.rs, mod.rs, params.rs, types.rs)
Introduces StealthTransferParams and TransferOutput with validation, new StealthTransferOutput/types, moves from flat output fields to outputs/transfers, and surfaces new StealthTransferApiError. Reworks per-output account determination and transaction construction.
Stealth outputs API usage
crates/wallet/sdk/src/apis/stealth_outputs.rs
Replaces OutputToCreate with StealthOutputToCreate<'a> in transfer statement generation and adjusts borrowing semantics.
Handler, UI, bindings, client updates
applications/tari_walletd/src/handlers/accounts.rs, applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts, integration_tests/src/wallet_daemon_client.rs, bindings/src/types/wallet-daemon-client/StealthTransfer.ts, bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts, bindings/src/wallet-daemon-client.ts, clients/wallet_daemon_client/src/types.rs
Switches public request shape to use transfers: Array<StealthTransfer> (per-transfer destination/amount/memo) across handler, web UI hook, bindings, client types, and integration tests. Re-exports new StealthTransfer type.
TakeFromBucket instruction (proto → runtime → builder → tests)
crates/p2p/proto/transaction.proto, crates/p2p/src/conversions/transaction.rs, crates/transaction/src/v1/instruction.rs, crates/transaction/src/builder/mod.rs, crates/engine/src/transaction/processor.rs, crates/engine/src/runtime/mod.rs, crates/engine/src/runtime/impl.rs, crates/engine/tests/account.rs
Adds TakeFromBucket { input_bucket, amount, output_bucket } message/variant, conversion (proto ↔ internal), TransactionBuilder::take_from_bucket, processor handling to Get/Take/Put on workspace, RuntimeInterface::put_on_workspace and its impl, and tests exercising the new instruction.
Workspace/state fixes & tooling
crates/engine/src/runtime/working_state.rs, crates/template_test_tooling/src/read_only_state_store.rs, crates/engine/tests/test.rs
validate_finalized now counts only non-zero buckets for dangling errors; with_substates signature updated to pass (SubstateId, &Substate); added get_all_resources helper; small test signature adjustments and removal of large nft_indexes tests.
Builder API changes (address allocation optional)
crates/template_lib/src/component/instance.rs, crates/template_lib/src/resource/builder/{fungible,confidential,non_fungible,stealth}.rs, crates/template_test_tooling/templates/faucet/src/lib.rs
with_address_allocation now consumes self and delegates to new with_address_allocation_opt(Option<...>); faucet constructor mint_with_opts added to accept optional address allocation.
Bindings/types and misc version bumps
bindings/package.json, clients/javascript/wallet_daemon_client/package.json, bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts
Package version bumps (1.18.2→1.19.0, 1.9.3→1.10.0) and trivial TS type form change string[]Array<string>.
Workspace API helper
crates/engine/src/runtime/impl.rs
Adds private helper put_on_workspace(id, value) to validate, invoke modules, and insert IndexedValue into workspace.
Transaction weight adjustments
crates/transaction/src/v1/transaction.rs
Adds weight for TakeFromBucket (1) and adjusts stealth statement weight baseline.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client
    participant Handler
    participant SDK
    participant TxBuilder
    participant Processor
    participant Runtime

    rect rgb(220, 255, 220)
    note over Client,SDK: New stealth transfer flow (per-transfer collection)
    Client->>Handler: POST StealthTransferRequest(transfers: [StealthTransfer...])
    Handler->>SDK: StealthTransferParams(outputs: Vec<TransferOutput>)
    SDK->>SDK: validate outputs, determine per-output accounts
    SDK->>TxBuilder: add per-output instructions (create account? deposit)
    TxBuilder->>Processor: submit transaction
    Processor->>Runtime: execute instructions (including TakeFromBucket if present)
    Runtime->>Processor: put_on_workspace / get / put results
    end
Loading
sequenceDiagram
    autonumber
    participant User
    participant TransactionBuilder
    participant Processor
    participant Workspace

    User->>TransactionBuilder: take_from_bucket(label, amount, output_label)
    TransactionBuilder->>TransactionBuilder: resolve workspace ids, append Instruction::TakeFromBucket
    TransactionBuilder->>Processor: submit transaction
    rect rgb(255, 240, 200)
    note over Processor,Workspace: TakeFromBucket execution
    Processor->>Workspace: Get(input_bucket)
    Workspace-->>Processor: return Bucket
    Processor->>Processor: perform BucketAction::Take(amount)
    Processor->>Workspace: put_on_workspace(output_bucket, result_bucket)
    Workspace-->>Processor: ack
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30–40 minutes

Changes are cross-cutting (bindings, SDK, runtime, proto, builder, tests) and introduce a new instruction plus validation/parameter refactors; review requires verifying type/serialization consistency and runtime behavior but follows consistent patterns.

Possibly related PRs

Poem

🐇 I hopped through code with tiny feet,

Transfers nested, outputs neat,
Buckets split from workspace deep,
Builders sip options, no more leap,
A little rabbit cheers this feat!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 47.62% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "feat(engine)!: add TakeFromBucket instruction + wallet multiple transfers" clearly and specifically identifies the two main features introduced in this changeset. The title accurately reflects the addition of the TakeFromBucket instruction (evident across multiple files including proto definitions, transaction processor, type definitions, and tests) and the refactoring of the stealth transfer API to support multiple outputs via Vec instead of flat fields (evident in the wallet SDK, bindings, and web UI changes). The title uses appropriate conventional commit format with a breaking change indicator (!), is concise at 73 characters, and avoids vague terminology while being specific enough for developers scanning history to understand the primary changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (6)
crates/template_test_tooling/src/read_only_state_store.rs (1)

54-63: Consider defensive error handling for type conversions.

The implementation correctly collects all resources, but relies on unwrap() at line 58 assuming that a SubstateId::Resource always corresponds to an actual Resource substate. While this is likely guaranteed by system invariants, consider using ? with proper error propagation or ok_or_else() for more defensive programming.

Optional refactor example:

     pub fn get_all_resources(&self) -> Result<HashMap<ResourceAddress, Resource>, StateStoreError> {
         let mut resources = HashMap::new();
         self.with_substates(|id, substate| {
             if let SubstateId::Resource(resource_address) = id {
-                let resource = substate.substate_value().as_resource().unwrap();
+                if let Some(resource) = substate.substate_value().as_resource() {
                     resources.insert(*resource_address, resource.clone());
+                }
             }
         })?;
         Ok(resources)
     }
crates/wallet/sdk/src/apis/stealth_transfer/params.rs (1)

93-99: Use checked arithmetic for Amount to prevent overflow.

total_output_amount() and the sums in total_output_amount()/total_revealed_output_amount() use plain addition. Prefer checked/saturating math (or an explicit overflow guard in validate) to avoid undefined behavior if Amount can overflow.

  • If Amount provides checked_add/saturating_add, use it and map overflow to StealthTransferApiError::AmountOverflow.
  • Otherwise, add a validate-phase guard that rejects outputs where revealed_amount + blinded_amount would overflow. Based on learnings.

Also applies to: 116-118

crates/wallet/sdk/src/apis/stealth_transfer/types.rs (3)

14-21: Add Debug and Clone derives to StealthTransferOutput.

This struct serves as the main output type for stealth transfers. Adding Debug and Clone derives will improve debuggability and usability, especially when this type needs to be passed around or inspected during development.

Apply this diff:

+#[derive(Debug, Clone)]
 pub struct StealthTransferOutput {
     pub transaction: UnsignedTransaction,
     pub lock_id: WalletLockId,
     pub fee_inputs: InputsToSpend,
     pub transfer_inputs: InputsToSpend,
     pub additional_signer: Option<WalletPublicKey>,
     pub main_signer: WalletPublicKey,
 }

23-32: Consider adding Clone derive if UnblindedStealthInputWitness supports it.

Adding Clone would provide flexibility when these inputs need to be referenced in multiple contexts during transaction construction.

If the wrapped UnblindedStealthInputWitness implements Clone, apply this diff:

-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct UnblindedInputToSpend {
     pub witness: UnblindedStealthInputWitness,
 }

61-65: Add Debug and Clone derives to AccountDetails.

This struct should have basic derives for better usability and debuggability.

Apply this diff:

+#[derive(Debug, Clone)]
 pub struct AccountDetails {
     pub address: ComponentAddress,
     pub vaults: Vec<VaultId>,
     pub exists: bool,
 }
crates/engine/tests/test.rs (1)

99-108: Unused parameter in closure signature.

The closure signature was updated to include a first parameter that is unused (indicated by _). Consider documenting what this parameter represents, as it may confuse developers updating existing code.

If the first parameter is consistently unused in most cases, consider whether the API could provide both a single-parameter and two-parameter overload, or document the purpose of the first parameter.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1daa17 and 1e66877.

📒 Files selected for processing (32)
  • applications/tari_walletd/src/handlers/accounts.rs (2 hunks)
  • applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1 hunks)
  • bindings/package.json (1 hunks)
  • bindings/src/types/Instruction.ts (1 hunks)
  • bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts (1 hunks)
  • bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1 hunks)
  • bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (1 hunks)
  • bindings/src/wallet-daemon-client.ts (1 hunks)
  • clients/javascript/wallet_daemon_client/package.json (1 hunks)
  • clients/wallet_daemon_client/src/types.rs (1 hunks)
  • crates/engine/src/runtime/impl.rs (3 hunks)
  • crates/engine/src/runtime/mod.rs (1 hunks)
  • crates/engine/src/runtime/working_state.rs (1 hunks)
  • crates/engine/src/transaction/processor.rs (2 hunks)
  • crates/engine/tests/account.rs (1 hunks)
  • crates/engine/tests/test.rs (1 hunks)
  • crates/p2p/proto/transaction.proto (2 hunks)
  • crates/p2p/src/conversions/transaction.rs (2 hunks)
  • crates/template_lib/src/component/instance.rs (1 hunks)
  • crates/template_lib/src/resource/builder/fungible.rs (1 hunks)
  • crates/template_test_tooling/src/read_only_state_store.rs (2 hunks)
  • crates/template_test_tooling/templates/faucet/src/lib.rs (1 hunks)
  • crates/transaction/src/builder/mod.rs (1 hunks)
  • crates/transaction/src/v1/instruction.rs (2 hunks)
  • crates/transaction/src/v1/transaction.rs (2 hunks)
  • crates/wallet/sdk/src/apis/stealth_outputs.rs (3 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/api.rs (13 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/mod.rs (1 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/params.rs (1 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1 hunks)
  • integration_tests/src/wallet_daemon_client.rs (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (23)
crates/engine/tests/account.rs (4)
crates/transaction/src/builder/mod.rs (2)
  • take_from_bucket (259-272)
  • new (53-59)
crates/template_test_tooling/src/read_only_state_store.rs (1)
  • new (21-23)
crates/transaction/src/transaction.rs (2)
  • new (51-53)
  • builder (47-49)
crates/template_test_tooling/src/template_test.rs (1)
  • owner_proof (404-406)
crates/engine/src/runtime/mod.rs (2)
crates/engine/src/runtime/impl.rs (1)
  • put_on_workspace (2697-2705)
bindings/src/types/IndexedValue.ts (1)
  • IndexedValue (4-4)
crates/transaction/src/builder/mod.rs (2)
crates/engine/tests/account.rs (1)
  • take_from_bucket (282-331)
bindings/src/types/Instruction.ts (1)
  • Instruction (15-42)
crates/template_test_tooling/templates/faucet/src/lib.rs (3)
crates/template_lib/src/resource/builder/fungible.rs (1)
  • new (64-75)
crates/template_lib/src/resource/builder/mod.rs (1)
  • public_fungible (54-56)
crates/template_lib/src/component/instance.rs (2)
  • new (24-31)
  • new (98-100)
crates/p2p/src/conversions/transaction.rs (3)
crates/engine/tests/account.rs (1)
  • take_from_bucket (282-331)
crates/transaction/src/builder/mod.rs (1)
  • take_from_bucket (259-272)
bindings/src/types/Instruction.ts (1)
  • Instruction (15-42)
integration_tests/src/wallet_daemon_client.rs (2)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
  • StealthTransfer (6-11)
crates/template_lib/src/component/instance.rs (1)
  • address (116-118)
bindings/src/types/Instruction.ts (2)
bindings/src/types/WorkspaceOffsetId.ts (1)
  • WorkspaceOffsetId (3-3)
bindings/src/types/Amount.ts (1)
  • Amount (12-12)
bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (4)
bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
  • ComponentAddressOrName (4-4)
bindings/src/types/ConfidentialTransferInputSelection.ts (1)
  • ConfidentialTransferInputSelection (3-7)
bindings/src/types/ResourceAddress.ts (1)
  • ResourceAddress (6-6)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
  • StealthTransfer (6-11)
applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
  • params (675-687)
crates/wallet/sdk/src/apis/stealth_transfer/mod.rs (2)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
  • params (675-687)
crates/wallet/sdk/src/apis/mod.rs (1)
  • stealth_transfer (16-16)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (2)
bindings/src/types/OotleAddress.ts (1)
  • OotleAddress (3-3)
bindings/src/types/Amount.ts (1)
  • Amount (12-12)
applications/tari_walletd/src/handlers/accounts.rs (1)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
  • transfer (287-587)
crates/template_lib/src/resource/builder/fungible.rs (4)
crates/template_lib/src/component/instance.rs (3)
  • with_address_allocation (34-36)
  • address (116-118)
  • with_address_allocation_opt (39-45)
crates/template_lib/src/resource/builder/confidential.rs (1)
  • with_address_allocation (79-82)
crates/template_lib/src/resource/builder/non_fungible.rs (1)
  • with_address_allocation (77-80)
crates/template_lib/src/resource/builder/stealth.rs (1)
  • with_address_allocation (80-83)
crates/template_test_tooling/src/read_only_state_store.rs (4)
bindings/src/types/ResourceAddress.ts (1)
  • ResourceAddress (6-6)
bindings/src/types/Resource.ts (1)
  • Resource (9-22)
bindings/src/types/SubstateId.ts (1)
  • SubstateId (6-6)
bindings/src/types/Substate.ts (1)
  • Substate (4-4)
crates/transaction/src/v1/transaction.rs (1)
bindings/src/types/Instruction.ts (1)
  • Instruction (15-42)
clients/wallet_daemon_client/src/types.rs (2)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
  • StealthTransfer (6-11)
bindings/src/types/OotleAddress.ts (1)
  • OotleAddress (3-3)
crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
  • is_not_found_error (780-782)
crates/transaction/src/v1/instruction.rs (2)
bindings/src/types/WorkspaceOffsetId.ts (1)
  • WorkspaceOffsetId (3-3)
bindings/src/types/Amount.ts (1)
  • Amount (12-12)
crates/wallet/sdk/src/apis/stealth_transfer/params.rs (6)
bindings/src/types/OotleAddress.ts (1)
  • OotleAddress (3-3)
bindings/src/types/Network.ts (1)
  • Network (6-6)
bindings/src/types/ResourceAddress.ts (1)
  • ResourceAddress (6-6)
bindings/src/types/Amount.ts (1)
  • Amount (12-12)
bindings/src/types/ConfidentialTransferInputSelection.ts (1)
  • ConfidentialTransferInputSelection (3-7)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
  • outputs (691-691)
crates/wallet/sdk/src/apis/stealth_transfer/types.rs (6)
crates/wallet/crypto/src/unblinded_statement.rs (1)
  • memo (68-70)
bindings/src/types/ComponentAddress.ts (1)
  • ComponentAddress (6-6)
bindings/src/types/VaultId.ts (1)
  • VaultId (6-6)
bindings/src/types/Amount.ts (1)
  • Amount (12-12)
bindings/src/types/UnsignedTransaction.ts (1)
  • UnsignedTransaction (4-4)
crates/p2p/src/conversions/transaction.rs (2)
  • value (830-834)
  • value (891-895)
crates/template_lib/src/component/instance.rs (1)
crates/template_lib/src/resource/builder/fungible.rs (2)
  • with_address_allocation (120-122)
  • with_address_allocation_opt (125-128)
crates/engine/src/transaction/processor.rs (4)
bindings/src/types/Instruction.ts (1)
  • Instruction (15-42)
crates/template_lib/src/models/bucket.rs (1)
  • amount (175-183)
bindings/src/types/IndexedValue.ts (1)
  • IndexedValue (4-4)
bindings/src/types/InstructionResult.ts (1)
  • InstructionResult (5-5)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
  • params (675-687)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: check nightly
  • GitHub Check: test
  • GitHub Check: machete
  • GitHub Check: clippy
  • GitHub Check: check stable
  • GitHub Check: fmt
🔇 Additional comments (40)
bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts (1)

7-7: Array notation is consistent across ts-rs generated files—no action needed.

Verification confirms that Array<string> notation is systematically used across all ts-rs generated type files in the bindings directory (6 instances found across multiple directories), with no instances of string[] notation. This indicates the change is the expected output of the ts-rs code generator, not a manual stylistic edit. The file is correctly generated and consistent with the codebase.

crates/template_test_tooling/src/read_only_state_store.rs (1)

90-94: LGTM: Improved API design.

The signature change from FnMut(Substate) to FnMut(&SubstateId, &Substate) is an improvement that provides more context to callers while avoiding unnecessary cloning during iteration. The implementation correctly forwards both parameters to the closure.

clients/wallet_daemon_client/src/types.rs (2)

1070-1078: LGTM! Well-structured type definition.

The StealthTransfer struct clearly encapsulates per-transfer data with appropriate field types. The skip_serializing_if attribute on output_memo is a good optimization for optional fields.


1064-1064: No changes needed—empty transfers are already validated.

The code properly validates that the transfers vector is non-empty. The StealthTransferParams::validate() function checks if self.outputs.is_empty() and returns an error with the message "At least one output must be specified" if no outputs are provided. This validation is called in the handler before processing the request.

crates/template_test_tooling/templates/faucet/src/lib.rs (2)

38-40: LGTM! Clean delegation pattern.

The refactor maintains backward compatibility while enabling the new optional address allocation functionality.


42-56: LGTM! Well-structured API extension.

The new mint_with_opts function correctly extends the faucet API to support optional address allocation without breaking existing consumers.

crates/template_lib/src/resource/builder/fungible.rs (1)

120-122: LGTM! Appropriate builder pattern update.

Changing to consume self by value and delegating to the optional variant is a clean refactor that maintains the fluent builder API.

crates/template_lib/src/component/instance.rs (2)

34-36: LGTM! Clean builder pattern refactor.

The change to consume self and delegate to the optional variant maintains the fluent builder API while centralizing the allocation logic.


39-45: LGTM! Excellent defensive implementation.

The assertion at Line 41 prevents accidental overwriting of an existing allocation, providing clear feedback if the builder is misconfigured. The None case safely no-ops, making this pattern robust.

crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1)

18-44: LGTM: coherent error surface.

Enum is well-structured and composes upstream APIs cleanly; IsNotFoundError impl matches store semantics.

applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1)

118-130: <!-- [scratchpad_start]
[task verification_1/1 check_request_contract_3/3]
Status: Verification complete. All findings gathered.
[observations]

  • StealthTransferRequest struct (line 1060-1068):
    • owner_account: ComponentAddressOrName ✓
    • transfers: Vec ✓
    • Matches review expectation
  • StealthTransfer struct (1072-1078):
    • destination_address: OotleAddress ✓
    • blinded_output_amount: Amount ✗ (NOT number - this is the issue!)
    • revealed_output_amount: Amount ✗ (NOT number - this is the issue!)
    • output_memo: Option ✓
  • Amount type: exports as string | number in TypeScript per previous search
  • Current implementation (lines 153-167):
    • Uses "owner_account" field name correctly ✓
    • Passes params.amount as number directly to Amount fields
    • No type conversion to Amount - values just cast implicitly
  • TransferParams.amount typed as number (line 120), should be Amount

[analysis]
CORRECT ISSUES IDENTIFIED:

  1. TransferParams.amount is number but should be Amount to prevent precision loss
  2. The implementation in useAccounts.ts passes number values to Amount-typed fields
  3. Amount supports string | number in TS, but proper typing prevents runtime errors
  4. The owner_account field name is ALREADY CORRECT (not a bug in current code)

VERIFIED FACTS:

  • Server expects owner_account (confirmed ✓)
  • Server expects transfers array with these exact fields (confirmed ✓)
  • Amount type should be used for blinded_output_amount and revealed_output_amount (confirmed ✓)
  • Current code already uses owner_account correctly at line 154 (already fixed or was never wrong)

[done]
Ready to generate rewritten review comment. The review comment is mostly correct but needs refinement:

  • The field name concern (owner_account vs account) is ALREADY addressed in the implementation
  • The Amount typing concern is the main actionable issue remaining
    </scratchpad_end] -->

Tighten amount typing from number to Amount; server contract verified.

The code correctly uses owner_account and the transfers array structure matches the server contract. However, TransferParams.amount should use the Amount type instead of number to prevent precision loss for large values:

-  amount: number;
+  amount: Amount; // from @tari-project/typescript-bindings

The blinded_output_amount and revealed_output_amount fields in the transfers array (lines 160–161) are correctly mapped to Amount types server-side; using Amount at the parameter level ensures type safety and consistency.

crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1)

34-39: LGTM!

The struct is well-designed with appropriate derives and a lifetime parameter for the memo reference.

crates/transaction/src/v1/transaction.rs (2)

191-191: LGTM!

The weight of 1 for TakeFromBucket is consistent with other simple workspace operations like AssertBucketContains and ClaimValidatorFees.


199-202: Baseline weight added for stealth transfers.

The addition of a fixed baseline weight of 100 for stealth transfer statements reflects the cryptographic overhead of these operations. This is a reasonable approach given the computational cost of validating stealth proofs.

bindings/src/wallet-daemon-client.ts (1)

35-35: LGTM!

The export of StealthTransfer is correctly placed and consistent with the existing export structure.

crates/transaction/src/v1/instruction.rs (2)

75-79: LGTM!

The TakeFromBucket instruction variant is well-structured with appropriate fields for specifying the source bucket, amount to take, and destination bucket.


190-200: LGTM!

The Display implementation for TakeFromBucket properly formats all three fields, maintaining consistency with other instruction Display implementations.

crates/engine/src/runtime/working_state.rs (1)

399-406: Improved dangling bucket validation.

The validation now correctly counts only non-zero buckets as dangling, rather than all buckets. This is more accurate since empty buckets don't represent leaked resources and shouldn't cause transaction failures.

crates/engine/src/runtime/mod.rs (1)

192-192: New RuntimeInterface method added.

The put_on_workspace method extends the runtime API to support placing values directly on the workspace by ID. This is a breaking change to the RuntimeInterface trait, but is necessary to support the new TakeFromBucket instruction flow.

crates/engine/tests/account.rs (1)

281-331: LGTM! Comprehensive test for TakeFromBucket.

The test effectively covers:

  • Creating a faucet and taking free coins
  • Splitting a bucket into two parts using TakeFromBucket
  • Testing the edge case where the source bucket is fully drained (line 302 comment)
  • Verifying final account balances

The test validates that empty buckets don't cause dangling bucket errors, which aligns with the working_state.rs validation changes.

bindings/src/types/Instruction.ts (1)

24-33: TakeFromBucket variant wired correctly.

Nice to see the bindings keeping pace with the engine changes so clients can emit the new instruction without hand-written glue. Nothing further from my side.

bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (1)

7-13: Aggregated transfers shape looks good.

Thanks for moving the request over to the transfers array; this matches the backend refactor and keeps the types tidy.

bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)

1-11: Handshake between Rust and TS stays aligned.

Appreciate the dedicated StealthTransfer type; makes the client payload much clearer.

crates/engine/src/transaction/processor.rs (1)

319-336: Runtime hook for TakeFromBucket looks solid.

The workspace fetch + bucket take + reinsert flow mirrors the existing engine patterns, so I don’t see any gaps here.

crates/engine/src/runtime/impl.rs (2)

1950-1955: Nice touch on state.new_bucket_id().

Switching to the helper keeps bucket allocation consistent with the other call sites.


2697-2705: Handy put_on_workspace helper.

Centralising the insert logic with validation makes the new instruction cleaner.

crates/transaction/src/builder/mod.rs (1)

259-272: Builder API addition fits right in.

This will make composing multi-output flows far less awkward; thanks for the ergonomic win.

applications/tari_walletd/src/handlers/accounts.rs (2)

26-33: Wallet-side refactor matches the new SDK params.

Mapping the request transfers straight into TransferOutput keeps the handler clean and future-proof.


971-986: End-to-end payload assembly still lines up.

The new outputs collection is passed straight through to the SDK, so the daemon remains in lockstep with the client types.

crates/wallet/sdk/src/apis/stealth_outputs.rs (1)

44-44: LGTM! Type rename and borrowing optimization.

The refactor from OutputToCreate to StealthOutputToCreate makes the type more specific and self-documenting. The borrowing change on line 680 (&output.owner_address instead of moving) is a good optimization that avoids unnecessary clones while maintaining safety through the lifetime parameter 'a.

Also applies to: 667-667, 680-680

crates/p2p/src/conversions/transaction.rs (2)

236-244: LGTM! Proper error handling and validation.

The proto-to-internal conversion correctly:

  • Validates required input_bucket field with descriptive error message
  • Handles optional amount with sensible default
  • Protects against output_bucket overflow with proper context

Follows the established pattern from other instructions like AssertBucketContains.


388-400: LGTM! Consistent serialization implementation.

The internal-to-proto conversion properly wraps all fields and follows the same structure as other instructions in this file.

crates/p2p/proto/transaction.proto (1)

35-40: Breaking change properly documented.

The field number renumbering for existing instruction variants (lines 35-39) and the new TakeFromBucket message definition (lines 65-69) constitute a breaking protocol change. This is acknowledged in the PR description with "Requires deletion of the data directory."

The new message structure correctly aligns with the conversion implementation in crates/p2p/src/conversions/transaction.rs.

Also applies to: 65-69

integration_tests/src/wallet_daemon_client.rs (1)

126-131: LGTM! Test correctly updated for multi-transfer API.

The refactoring from flat transfer fields to a transfers collection properly supports the new capability of sending to multiple recipients in a single transaction. The test correctly creates a single-element vector to match the new API signature.

crates/wallet/sdk/src/apis/stealth_transfer/api.rs (6)

319-328: LGTM! Pre-determining account creation needs is efficient.

This loop gathers which destination accounts need to be created before transaction construction. This approach allows the transaction builder to make informed decisions about whether to create accounts or deposit to existing ones.

The use of HashSet for accounts_to_create provides O(1) lookup during transaction building.


589-676: Well-structured account existence check with proper network fallback.

The function correctly handles multiple scenarios:

  1. ✅ Early exit for stealth-only transfers (no revealed funds)
  2. ✅ Local account check with on-chain confirmation validation
  3. ✅ Network lookup as fallback
  4. ✅ Proper vault substate inclusion for existing accounts

The logic properly adds both the account and vault (if exists) to substate_inputs, ensuring the transaction has required inputs for deposit operations.


507-516: Output ordering is critical for change detection logic.

The chaining order (explicit outputs → change output → filter) is important because line 522 relies on .last() to access the change output. This works correctly because:

  1. change_output is chained after all explicit outputs
  2. The filter preserves order
  3. If change amount is positive, it will be the last element

However, this implicit ordering dependency is subtle and could be fragile.

Consider adding an assertion or comment documenting this ordering requirement:

// NOTE: change_output must be chained last because we access it via .last() below (line 522)
outputs: params
    .outputs
    .iter()
    .map(Into::into)
    .chain(change_output)
    .filter(|o| o.amount.is_positive()),

742-764: Transaction builder correctly handles conditional account creation.

The logic properly:

  1. ✅ Iterates over outputs with index for unique bucket naming
  2. ✅ Skips outputs with no revealed amount
  3. ✅ Uses take_from_bucket to split the output bucket by exact revealed amount
  4. ✅ Creates account with bucket for new accounts (using accounts_to_create set)
  5. ✅ Deposits to existing accounts otherwise

The use of sub_bucket_name with index ensures unique workspace keys.


359-364: LGTM! Consistent with API refactoring.

The change to StealthOutputToCreate with owner_address.clone() is necessary because the type now uses borrowing semantics (as seen in stealth_outputs.rs). The clone here is unavoidable since we need to hold the value across iterations.

Also applies to: 491-495


692-699: Good addition of network parameter for transaction building.

The network parameter (line 692) is properly used on line 704 to set the transaction network via .for_network(network.as_byte()). The accounts_to_create parameter enables the conditional account creation logic.

Comment thread bindings/package.json
Comment thread clients/javascript/wallet_daemon_client/package.json
Comment thread crates/template_lib/src/resource/builder/fungible.rs
Comment thread crates/wallet/sdk/src/apis/stealth_transfer/params.rs Outdated
Comment thread crates/wallet/sdk/src/apis/stealth_transfer/params.rs Outdated
Comment thread crates/wallet/sdk/src/apis/stealth_transfer/types.rs
@sdbondi
sdbondi force-pushed the wallet-send-multiple-recipients branch from 1e66877 to c369a45 Compare October 24, 2025 07:28
@sdbondi
sdbondi force-pushed the wallet-send-multiple-recipients branch from c369a45 to 935bb22 Compare October 24, 2025 07:30
@sdbondi
sdbondi merged commit d80aae4 into tari-project:development Oct 24, 2025
11 checks passed
@sdbondi
sdbondi deleted the wallet-send-multiple-recipients branch October 24, 2025 07:35

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
crates/template_lib/src/resource/builder/fungible.rs (1)

120-128: Consider adding defensive check for duplicate allocation.

The with_address_allocation_opt method directly assigns the address without checking if one already exists, as previously noted. The component builder includes an assertion to prevent accidental overwrites at crates/template_lib/src/component/instance.rs lines 38-44.

Apply this diff to add the defensive check:

 pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self {
-    self.address_allocation = address;
+    if let Some(address) = address {
+        assert!(self.address_allocation.is_none(), "Address allocation already set");
+        self.address_allocation = Some(address);
+    }
     self
 }
bindings/package.json (1)

3-3: [DUPLICATE] Version bump must be 2.0.0 (major), not 1.19.0 (minor)—breaking API change still unresolved.

This is a continuation of the critical issue flagged in the prior review. Backward incompatible API changes increment the major version. The StealthTransferRequest structure change (removal of flat fields: destination_address, blinded_output_amount, revealed_output_amount, output_memo and replacement with transfers: Array<StealthTransfer>) is a breaking change that requires a major version bump per semver specification.

Update the version to "2.0.0" and ensure release notes document the breaking changes for consumers.

🧹 Nitpick comments (5)
crates/template_lib/src/resource/builder/confidential.rs (1)

79-87: Consider adding defensive check for duplicate allocation.

The with_address_allocation_opt method directly assigns the address without checking if one already exists. The component builder (at crates/template_lib/src/component/instance.rs lines 38-44) includes an assertion to prevent accidental overwrites. Adding the same check here would catch builder misuse earlier.

Apply this diff to add the defensive check:

 pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self {
-    self.address_allocation = address;
+    if let Some(address) = address {
+        assert!(self.address_allocation.is_none(), "Address allocation already set");
+        self.address_allocation = Some(address);
+    }
     self
 }
crates/template_lib/src/resource/builder/stealth.rs (1)

80-88: Consider adding defensive check for duplicate allocation.

The with_address_allocation_opt method directly assigns the address without checking if one already exists. The component builder (at crates/template_lib/src/component/instance.rs lines 38-44) includes an assertion to prevent accidental overwrites. Adding the same check here would improve consistency and catch builder misuse earlier.

Apply this diff to add the defensive check:

 pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self {
-    self.address_allocation = address;
+    if let Some(address) = address {
+        assert!(self.address_allocation.is_none(), "Address allocation already set");
+        self.address_allocation = Some(address);
+    }
     self
 }
crates/template_lib/src/resource/builder/non_fungible.rs (1)

77-85: Consider adding defensive check for duplicate allocation.

The with_address_allocation_opt method directly assigns the address without checking if one already exists. The component builder (at crates/template_lib/src/component/instance.rs lines 38-44) includes an assertion to prevent accidental overwrites. Adding the same check here would improve consistency and catch builder misuse earlier.

Apply this diff to add the defensive check:

 pub fn with_address_allocation_opt(mut self, address: Option<ResourceAddressAllocation>) -> Self {
-    self.address_allocation = address;
+    if let Some(address) = address {
+        assert!(self.address_allocation.is_none(), "Address allocation already set");
+        self.address_allocation = Some(address);
+    }
     self
 }
crates/transaction/src/v1/transaction.rs (1)

199-202: Clarify the rationale for the 100 baseline in stealth statement weight.

A baseline of 100 has been added to the stealth statement weight calculation. While this likely accounts for fixed overhead (e.g., balance proof verification), the rationale should be documented.

Consider adding a constant and documentation:

+// Fixed cost for stealth transfer statement verification (balance proof, etc.)
+const STEALTH_STATEMENT_BASE_WEIGHT: u64 = 100;
+
 fn calc_stealth_statement_weight(statement: &StealthTransferStatement) -> u64 {
     // TODO: weight inputs and outputs accordingly - currently outputs cost 2x inputs
-    100 + statement.inputs_statement.inputs.len() as u64 + (statement.outputs_statement.outputs.len() as u64 * 2)
+    STEALTH_STATEMENT_BASE_WEIGHT + 
+        statement.inputs_statement.inputs.len() as u64 + 
+        (statement.outputs_statement.outputs.len() as u64 * 2)
 }
crates/transaction/src/builder/mod.rs (1)

259-272: Consider validating the amount parameter.

The implementation correctly integrates with the workspace and instruction systems. However, there's no validation on the amount parameter.

Consider adding validation to catch errors early:

 pub fn take_from_bucket<T: Into<BuilderWorkspaceKey>, A: Into<Amount>>(
     mut self,
     label: T,
     amount: A,
     output_label: T,
 ) -> Self {
     let key = self.get_workspace_offset_id_from_named_arg(label.into());
     let output_key = self.workspace_ids.insert(output_label.into());
+    let amount = amount.into();
+    // Validate amount is positive (negative amounts should use different operations)
+    if amount.is_negative() {
+        panic!("take_from_bucket amount must be non-negative");
+    }
     self.add_instruction(Instruction::TakeFromBucket {
         input_bucket: key,
-        amount: amount.into(),
+        amount,
         output_bucket: output_key,
     })
 }

Note: This validation may already exist at the runtime level, so this is optional.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e66877 and f9cc889.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (36)
  • Cargo.toml (1 hunks)
  • applications/tari_walletd/src/handlers/accounts.rs (2 hunks)
  • applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1 hunks)
  • bindings/package.json (1 hunks)
  • bindings/src/types/Instruction.ts (1 hunks)
  • bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts (1 hunks)
  • bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1 hunks)
  • bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (1 hunks)
  • bindings/src/wallet-daemon-client.ts (1 hunks)
  • clients/javascript/wallet_daemon_client/package.json (1 hunks)
  • clients/wallet_daemon_client/src/types.rs (1 hunks)
  • crates/engine/src/runtime/impl.rs (3 hunks)
  • crates/engine/src/runtime/mod.rs (1 hunks)
  • crates/engine/src/runtime/working_state.rs (1 hunks)
  • crates/engine/src/transaction/processor.rs (2 hunks)
  • crates/engine/tests/account.rs (1 hunks)
  • crates/engine/tests/test.rs (1 hunks)
  • crates/p2p/proto/transaction.proto (2 hunks)
  • crates/p2p/src/conversions/transaction.rs (2 hunks)
  • crates/template_lib/src/component/instance.rs (1 hunks)
  • crates/template_lib/src/resource/builder/confidential.rs (1 hunks)
  • crates/template_lib/src/resource/builder/fungible.rs (1 hunks)
  • crates/template_lib/src/resource/builder/non_fungible.rs (1 hunks)
  • crates/template_lib/src/resource/builder/stealth.rs (1 hunks)
  • crates/template_test_tooling/src/read_only_state_store.rs (2 hunks)
  • crates/template_test_tooling/templates/faucet/src/lib.rs (1 hunks)
  • crates/transaction/src/builder/mod.rs (1 hunks)
  • crates/transaction/src/v1/instruction.rs (2 hunks)
  • crates/transaction/src/v1/transaction.rs (2 hunks)
  • crates/wallet/sdk/src/apis/stealth_outputs.rs (3 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/api.rs (13 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/error.rs (1 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/mod.rs (1 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/params.rs (1 hunks)
  • crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1 hunks)
  • integration_tests/src/wallet_daemon_client.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
  • crates/transaction/src/v1/instruction.rs
  • crates/template_test_tooling/src/read_only_state_store.rs
  • bindings/src/types/wallet-daemon-client/StealthTransfer.ts
  • bindings/src/types/tari-indexer-client/IndexerGetIdentityResponse.ts
  • clients/wallet_daemon_client/src/types.rs
  • bindings/src/types/Instruction.ts
  • crates/wallet/sdk/src/apis/stealth_transfer/types.rs
  • applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts
  • integration_tests/src/wallet_daemon_client.rs
  • crates/p2p/src/conversions/transaction.rs
  • crates/engine/src/runtime/working_state.rs
  • crates/wallet/sdk/src/apis/stealth_outputs.rs
  • crates/wallet/sdk/src/apis/stealth_transfer/error.rs
  • crates/template_lib/src/component/instance.rs
  • crates/wallet/sdk/src/apis/stealth_transfer/mod.rs
  • clients/javascript/wallet_daemon_client/package.json
  • crates/engine/tests/account.rs
  • crates/template_test_tooling/templates/faucet/src/lib.rs
  • crates/engine/src/runtime/impl.rs
🧰 Additional context used
🧬 Code graph analysis (12)
crates/template_lib/src/resource/builder/fungible.rs (4)
crates/template_lib/src/component/instance.rs (3)
  • with_address_allocation (34-36)
  • address (116-118)
  • with_address_allocation_opt (39-45)
crates/template_lib/src/resource/builder/confidential.rs (2)
  • with_address_allocation (79-81)
  • with_address_allocation_opt (84-87)
crates/template_lib/src/resource/builder/non_fungible.rs (2)
  • with_address_allocation (77-79)
  • with_address_allocation_opt (82-85)
crates/template_lib/src/resource/builder/stealth.rs (2)
  • with_address_allocation (80-82)
  • with_address_allocation_opt (85-88)
crates/engine/src/runtime/mod.rs (2)
crates/engine/src/runtime/impl.rs (1)
  • put_on_workspace (2697-2705)
bindings/src/types/IndexedValue.ts (1)
  • IndexedValue (4-4)
crates/transaction/src/v1/transaction.rs (1)
bindings/src/types/Instruction.ts (1)
  • Instruction (15-42)
crates/template_lib/src/resource/builder/confidential.rs (4)
crates/template_lib/src/component/instance.rs (3)
  • with_address_allocation (34-36)
  • address (116-118)
  • with_address_allocation_opt (39-45)
crates/template_lib/src/resource/builder/fungible.rs (2)
  • with_address_allocation (120-122)
  • with_address_allocation_opt (125-128)
crates/template_lib/src/resource/builder/non_fungible.rs (2)
  • with_address_allocation (77-79)
  • with_address_allocation_opt (82-85)
crates/template_lib/src/resource/builder/stealth.rs (2)
  • with_address_allocation (80-82)
  • with_address_allocation_opt (85-88)
applications/tari_walletd/src/handlers/accounts.rs (1)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
  • transfer (287-591)
crates/wallet/sdk/src/apis/stealth_transfer/params.rs (6)
bindings/src/types/OotleAddress.ts (1)
  • OotleAddress (3-3)
bindings/src/types/Network.ts (1)
  • Network (6-6)
bindings/src/types/ResourceAddress.ts (1)
  • ResourceAddress (6-6)
bindings/src/types/Amount.ts (1)
  • Amount (12-12)
integration_tests/src/wallet_daemon_client.rs (1)
  • confidential_transfer (867-910)
bindings/src/types/ConfidentialTransferInputSelection.ts (1)
  • ConfidentialTransferInputSelection (3-7)
crates/template_lib/src/resource/builder/non_fungible.rs (4)
crates/template_lib/src/component/instance.rs (3)
  • with_address_allocation (34-36)
  • address (116-118)
  • with_address_allocation_opt (39-45)
crates/template_lib/src/resource/builder/confidential.rs (2)
  • with_address_allocation (79-81)
  • with_address_allocation_opt (84-87)
crates/template_lib/src/resource/builder/fungible.rs (2)
  • with_address_allocation (120-122)
  • with_address_allocation_opt (125-128)
crates/template_lib/src/resource/builder/stealth.rs (2)
  • with_address_allocation (80-82)
  • with_address_allocation_opt (85-88)
crates/engine/src/transaction/processor.rs (5)
bindings/src/types/Instruction.ts (1)
  • Instruction (15-42)
crates/engine_types/src/resource_container.rs (1)
  • amount (134-141)
crates/template_lib/src/models/bucket.rs (1)
  • amount (175-183)
bindings/src/types/IndexedValue.ts (1)
  • IndexedValue (4-4)
bindings/src/types/InstructionResult.ts (1)
  • InstructionResult (5-5)
crates/template_lib/src/resource/builder/stealth.rs (4)
crates/template_lib/src/component/instance.rs (3)
  • with_address_allocation (34-36)
  • address (116-118)
  • with_address_allocation_opt (39-45)
crates/template_lib/src/resource/builder/confidential.rs (2)
  • with_address_allocation (79-81)
  • with_address_allocation_opt (84-87)
crates/template_lib/src/resource/builder/fungible.rs (2)
  • with_address_allocation (120-122)
  • with_address_allocation_opt (125-128)
crates/template_lib/src/resource/builder/non_fungible.rs (2)
  • with_address_allocation (77-79)
  • with_address_allocation_opt (82-85)
bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (4)
bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
  • ComponentAddressOrName (4-4)
bindings/src/types/ConfidentialTransferInputSelection.ts (1)
  • ConfidentialTransferInputSelection (3-7)
bindings/src/types/ResourceAddress.ts (1)
  • ResourceAddress (6-6)
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
  • StealthTransfer (6-11)
crates/transaction/src/builder/mod.rs (3)
crates/engine/tests/account.rs (1)
  • take_from_bucket (282-331)
bindings/src/types/Amount.ts (1)
  • Amount (12-12)
bindings/src/types/Instruction.ts (1)
  • Instruction (15-42)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
  • params (675-687)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: test
  • GitHub Check: machete
  • GitHub Check: check nightly
  • GitHub Check: check stable
  • GitHub Check: clippy
  • GitHub Check: fmt
🔇 Additional comments (18)
crates/engine/tests/test.rs (2)

99-99: LGTM! Closure signature updated correctly.

The closure signature has been updated to match the new with_substates API. The first parameter is correctly marked as unused with the underscore prefix.


99-110: Based on my verification:

The removal of the nft_indexes module is intentional and complete.

The search found no references to nft_indexes, nft_index, setup_nft_index, or test_new_nft_index anywhere in the codebase.

Test coverage for NFT functionality is actively maintained through:

  • mod basic_nft (lines 506-891): Tests core NFT functionality including resource creation, minting, burning, and mutable data updates
  • mod emoji_id (lines 893-1025): Tests NFT creation with specific ID formats (emoji-based)
  • mod tickets (lines 1026+): Tests NFT use case with state mutations and access control
  • Integration tests: Extensive NFT operations (mint, list, get, transfer) in integration_tests/tests/cucumber.rs and wallet daemon handlers

The NFT indexing functionality itself continues to be supported in the runtime, storage, and wallet SDK layers. The removal only affects the dedicated test module, and alternative test coverage is comprehensive.

bindings/src/wallet-daemon-client.ts (1)

35-35: LGTM! StealthTransfer export added to public API.

The export properly exposes the new StealthTransfer type, enabling consumers to use it with the updated StealthTransferRequest that now accepts an array of transfers.

crates/engine/src/runtime/mod.rs (1)

192-192: LGTM! Workspace write capability added to RuntimeInterface.

The new put_on_workspace method properly extends the runtime interface to support writing IndexedValue to a workspace. This is used by the new TakeFromBucket instruction to place the output bucket onto the workspace.

bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (1)

5-14: LGTM! API refactored to support multiple transfers.

The refactoring successfully enables multiple stealth transfers in a single transaction by:

  • Replacing single destination_address with transfers: Array<StealthTransfer>
  • Moving per-transfer fields (blinded_output_amount, revealed_output_amount, output_memo) into the StealthTransfer type

This is a clean implementation of the PR's core feature and aligns with the breaking changes notice.

crates/transaction/src/v1/transaction.rs (1)

191-191: LGTM! Weight calculation added for TakeFromBucket.

The weight of 1 is appropriate for the TakeFromBucket instruction, as it performs a simple workspace bucket operation.

crates/p2p/proto/transaction.proto (1)

65-69: LGTM! TakeFromBucket message structure is correct.

The message fields are appropriately typed:

  • input_bucket as WorkspaceOffsetId for source bucket reference
  • amount as Amount for the quantity to take
  • output_bucket as uint32 matching the WorkspaceId type
crates/engine/src/transaction/processor.rs (1)

319-336: All edge cases are properly handled—no changes needed.

The implementation correctly handles all identified scenarios:

  • Insufficient balance: ResourceError::InsufficientBalance is returned with descriptive message showing required vs available amounts
  • Invalid bucket reference: RuntimeError::InvalidArgument is returned if bucket_id is missing
  • Negative/zero amounts: Template-level assert!(amount.is_positive()) validates amounts

All errors propagate correctly through the ? operator chain in the processor.

applications/tari_walletd/src/handlers/accounts.rs (1)

975-984: LGTM! Clean transformation from request to API parameters.

The mapping from transfers array to TransferOutput structs is straightforward and correctly transforms the per-transfer fields. Validation occurs downstream via params.validate(network) at line 987, which ensures all constraints are checked before processing.

crates/wallet/sdk/src/apis/stealth_transfer/params.rs (4)

30-46: LGTM! Validation correctly enforces constraints.

The empty outputs check and blinded output count validation properly address the MAX_LAZY_BP_AGG_FACTORS constraint. Using is_positive() to count blinded outputs is correct since negative amounts are rejected separately and zero-amount outputs don't contribute to bulletproof aggregation.


48-88: LGTM! Comprehensive per-output validation.

The validation logic correctly enforces:

  • Non-negative amounts for both blinded and revealed components
  • At least one positive amount per output (preventing zero-value outputs)
  • Network consistency between destination address and wallet
  • Address format validity

93-119: LGTM! Helper methods are clear and correctly implemented.

The aggregation methods properly sum amounts across outputs, and the TransferOutput struct provides a clean abstraction for per-transfer parameters.


121-136: LGTM! Fallible conversion properly handles errors.

The TryFrom implementation correctly addresses the previous review comment by making the conversion fallible and properly propagating address conversion errors. Using only blinded_amount (line 132) is correct, as StealthOutputToCreate represents the confidential portion, with revealed amounts handled separately in the transaction flow.

crates/wallet/sdk/src/apis/stealth_transfer/api.rs (4)

319-328: LGTM! Per-output destination handling is correctly implemented.

The loop properly determines which destination accounts need creation and populates the required substate inputs. Using a HashSet for accounts_to_create correctly deduplicates when multiple outputs target the same new account. The async calls in the loop are acceptable given the validated output count limit.


593-680: LGTM! Comprehensive destination account determination logic.

The helper correctly handles all scenarios:

  • Early exit for pure blinded transfers (no account needed)
  • Local account optimization to avoid network calls
  • Proper differentiation between confirmed and unconfirmed local accounts
  • Network queries with appropriate error handling
  • Vault detection and input population

The logic properly populates substate_inputs and returns accurate account creation flags.


497-501: LGTM! Fallible output construction properly propagates errors.

The conversion from TransferOutput to StealthOutputToCreate correctly uses the fallible TryFrom implementation, ensuring address conversion errors are caught and propagated before transaction generation.


513-520: LGTM! Transfer statement generation correctly combines outputs and change.

The chaining of outputs_to_create with the optional change_output and filtering for positive amounts ensures only valid outputs are included in the statement.

Cargo.toml (1)

1-3: Verify that template version files were updated per the NOTE requirement.

Line 1 explicitly documents that template version files must be updated when editing the workspace version. Confirm that the version in template_built_in/templates/account and template_built_in/templates/account_nft has also been bumped to 0.14.0 to maintain consistency across the workspace.

Run the following to verify template versions were updated:

Comment on lines +35 to 40
TakeFromBucket take_from_bucket = 10;
PublishTemplate publish_template = 11;
AllocateAddress allocate_address = 12;
StealthTransfer stealth_transfer = 13;
PayFee pay_fee = 14;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Protobuf field renumbering is a breaking change.

The Instruction oneof fields have been renumbered to accommodate the new TakeFromBucket instruction. This is a breaking protocol change that will cause deserialization failures with older versions.

Impact:

  • Existing serialized transactions will fail to deserialize
  • Network protocol incompatibility between old and new nodes
  • Aligns with PR's breaking change notice: "Requires deletion of the data directory"

If backward compatibility is required, consider:

  1. Adding TakeFromBucket at a new field number (e.g., 15) without renumbering existing fields
  2. Implementing a migration path for existing data

If breaking change is acceptable (as indicated by PR):

  • Ensure all nodes are upgraded simultaneously
  • Verify data directory deletion is properly communicated to users
🤖 Prompt for AI Agents
In crates/p2p/proto/transaction.proto around lines 35–40 the oneof Instruction
fields were renumbered which is a breaking protobuf change; to fix, revert the
existing field numbers for PublishTemplate, AllocateAddress, StealthTransfer,
and PayFee to their original values and add TakeFromBucket using a new unused
field number (e.g., 15) so existing wire-format remains compatible, then
regenerate protobuf bindings and update any references to the new field number;
if a breaking change is intentionally required instead, document the requirement
to upgrade all nodes and to delete or migrate on-disk data and ensure release
notes and upgrade scripts communicate that clearly.

Comment on lines +746 to +768
params.outputs.iter().enumerate().fold(builder, |builder, (i, output)| {
if !output.revealed_amount.is_positive() {
return builder;
}

let dest_account = derive_account_address_from_public_key(output.address.account_public_key());
let need_to_create_account = accounts_to_create.contains(&dest_account);
let sub_bucket_name = format!("output-sub-bucket-{i}");
if need_to_create_account {
builder
.take_from_bucket("output_bucket", output.revealed_amount, &sub_bucket_name)
.create_account_with_bucket(
*output.address.account_public_key(),
sub_bucket_name
)
} else {
builder
.take_from_bucket("output_bucket", output.revealed_amount, &sub_bucket_name)
.call_method(dest_account, "deposit", args![Workspace(
sub_bucket_name
)])
}
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical issue: Multiple outputs to the same new account will attempt duplicate account creation.

When multiple outputs target the same non-existent destination account with revealed amounts, the current logic will attempt to create the account multiple times within the same transaction (once per output). Line 752 checks a pre-computed accounts_to_create set, which doesn't get updated as accounts are created during transaction building.

Scenario:

  • Output 0 → new account A with revealed amount
  • Output 1 → same new account A with revealed amount
  • Both outputs see need_to_create_account = true
  • Both execute create_account_with_bucket, causing an engine error

Fix: Track accounts created within the transaction generation loop:

 .then(|builder| {
+    let mut created_accounts = HashSet::new();
     params.outputs.iter().enumerate().fold(builder, |builder, (i, output)| {
         if !output.revealed_amount.is_positive() {
             return builder;
         }

         let dest_account = derive_account_address_from_public_key(output.address.account_public_key());
-        let need_to_create_account = accounts_to_create.contains(&dest_account);
+        let need_to_create_account = accounts_to_create.contains(&dest_account) 
+            && !created_accounts.contains(&dest_account);
         let sub_bucket_name = format!("output-sub-bucket-{i}");
         if need_to_create_account {
+            created_accounts.insert(dest_account);
             builder
                 .take_from_bucket("output_bucket", output.revealed_amount, &sub_bucket_name)
                 .create_account_with_bucket(
                     *output.address.account_public_key(),
                     sub_bucket_name
                 )
         } else {

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In crates/wallet/sdk/src/apis/stealth_transfer/api.rs around lines 746 to 768,
the builder loop uses a precomputed accounts_to_create set so multiple outputs
to the same new account will each try to create the account; change the logic to
track accounts created during transaction construction by introducing a mutable
HashSet (e.g. created_accounts) outside the fold/loop, check
created_accounts.contains(&dest_account) before deciding to create, and when you
perform create_account_with_bucket insert dest_account into created_accounts so
subsequent outputs target that account will call deposit instead of attempting
duplicate creation; ensure the mutable set is accessible inside the closure
(move it or use &mut) and update any types to keep borrow rules satisfied.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants