fix(wallet)!: improved input selection algo - #1637
Conversation
WalkthroughRefactors numeric amount types across the wallet codebase: replaces Amount wrappers with primitive u64/bigint in APIs, storage, crypto and tests; introduces a branch-and-bound input selection module; updates storage schema and batch locking APIs; and removes one ApplicationErrorCode variant. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Wallet CLI
participant Handler as Proof Handler
participant Crypto as Crypto Module
Note over CLI,Handler: Proof generation type change
CLI->>Handler: GenerateArgs { amount: u64 }
Handler->>Crypto: create_confidential_output_proof(ConfidentialCreateOutputProofRequest{ amount: u64 })
Crypto->>Handler: ConfidentialOutputProof
sequenceDiagram
participant API as Stealth Output API
participant Storage as Wallet Storage
participant Selector as Input Selection
Note over API,Selector: New input selection flow
API->>Storage: stealth_outputs_get_unspent_by_account(...)
Storage->>API: Vec<StealthOutputInfo (u64 values)>
API->>Selector: select(inputs, target, BranchAndBound, max_inputs)
Selector->>API: SelectionResult { total_value: Amount, selected_keys }
API->>Storage: stealth_outputs_lock_many(resource_addr, selected_commitments, lock_id)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
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 |
7fbce02 to
86e0a62
Compare
86e0a62 to
4376262
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/wallet_daemon_client/src/types.rs (1)
493-500: Annotateconfidential_amountasbigintfor generated TS bindings.
bindings/src/types/ProofsGenerateRequest.tsnow emitsconfidential_amount: bigint, butts-rsstill defaultsu64tonumber, so downstream TypeScript consumers will see a mismatched type and break at compile/runtime. Please override the generated type to keep the Rust and TS views in sync.#[cfg_attr(feature = "ts", derive(ts_rs::TS), ts(export, export_to = "wallet-daemon-client/"))] pub struct ProofsGenerateRequest { + #[cfg_attr(feature = "ts", ts(type = "bigint"))] pub confidential_amount: u64,
🧹 Nitpick comments (3)
crates/wallet/sdk/src/models/input_selection/mod.rs (1)
1-12: LGTM! New input selection module introduced.The new
InputSelectionAlgorithmenum provides configurable input selection strategies as described in the PR objectives. The enum is well-structured with clear variant names and appropriate derives.For future enhancement, consider adding doc comments explaining when to use each algorithm variant (e.g., "BranchAndBound minimizes waste and fragmentation but may be slower for large UTXO sets").
crates/engine_types/src/crypto/helpers.rs (1)
73-76: LGTM! New u64 commitment helper added.The
commit_u64_amountfunction provides a clean API for committing u64 amounts directly, complementing the existingcommit_amountfunctions. This aligns well with the PR's migration to native u64 types.Consider adding documentation similar to the existing
commit_amountfunction to maintain consistency:+/// Creates a Pedersen commitment to the given u64 amount using the provided mask. pub fn commit_u64_amount(mask: &RistrettoSecretKey, amount: u64) -> PedersenCommitment { get_commitment_factory().commit_value(mask, amount) }crates/wallet/storage_sqlite/src/writer.rs (1)
1259-1294: Consider adding lock validation for consistency.The new
stealth_outputs_lock_manymethod correctly implements batch locking with proper error handling. However, it doesn't validate that thelock_idexists, unlikestealth_outputs_lock_smallest_amount(line 1209) which callsself.ensure_lock_exists(lock_id).For consistency with the existing
stealth_outputs_lock_smallest_amountmethod, consider adding lock validation at the beginning:fn stealth_outputs_lock_many( &mut self, resource_address: &ResourceAddress, utxos: &[&PedersenCommitmentBytes], lock_id: WalletLockId, ) -> Result<(), WalletStorageError> { const OPERATION: &str = "stealth_outputs_lock_many"; use crate::schema::stealth_outputs; + + self.ensure_lock_exists(lock_id)?; let num_rows = diesel::update(stealth_outputs::table)This ensures the lock exists before attempting to lock outputs, providing better error messages if an invalid lock_id is used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
applications/tari_wallet_cli/src/command/proof.rs(2 hunks)applications/tari_walletd/src/handlers/accounts.rs(5 hunks)applications/tari_walletd/src/handlers/confidential.rs(7 hunks)applications/tari_walletd/src/handlers/stealth_utxos.rs(1 hunks)applications/tari_walletd/src/jrpc_server.rs(0 hunks)applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts(1 hunks)bindings/src/types/wallet-daemon-client/ConfidentialCreateOutputProofRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/ProofsGenerateRequest.ts(1 hunks)bindings/src/types/wallet-daemon-client/StealthTransfer.ts(1 hunks)bindings/src/types/wallet-daemon-client/TransferOutput.ts(1 hunks)clients/javascript/wallet_daemon_client/src/index.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(4 hunks)crates/engine/tests/burn.rs(1 hunks)crates/engine/tests/confidential.rs(2 hunks)crates/engine/tests/recall.rs(1 hunks)crates/engine/tests/stealth.rs(12 hunks)crates/engine_types/src/crypto/helpers.rs(1 hunks)crates/template_lib_types/src/amount/amount.rs(5 hunks)crates/template_test_tooling/src/support/confidential.rs(3 hunks)crates/template_test_tooling/src/support/stealth.rs(6 hunks)crates/transaction/Cargo.toml(1 hunks)crates/wallet/crypto/src/bullet_proof.rs(1 hunks)crates/wallet/crypto/src/confidential.rs(6 hunks)crates/wallet/crypto/src/encrypted_data.rs(2 hunks)crates/wallet/crypto/src/stealth.rs(4 hunks)crates/wallet/crypto/src/unblinded_statement.rs(5 hunks)crates/wallet/crypto/src/viewable_balance_proof.rs(1 hunks)crates/wallet/crypto/tests/stealth_transfer_statement.rs(1 hunks)crates/wallet/crypto/tests/viewable_balance_proof.rs(4 hunks)crates/wallet/sdk/Cargo.toml(1 hunks)crates/wallet/sdk/src/apis/confidential_crypto.rs(1 hunks)crates/wallet/sdk/src/apis/confidential_outputs.rs(1 hunks)crates/wallet/sdk/src/apis/confidential_transfer.rs(5 hunks)crates/wallet/sdk/src/apis/stealth_crypto.rs(1 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(10 hunks)crates/wallet/sdk/src/apis/stealth_transfer/api.rs(10 hunks)crates/wallet/sdk/src/apis/stealth_transfer/params.rs(3 hunks)crates/wallet/sdk/src/apis/stealth_transfer/types.rs(2 hunks)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs(1 hunks)crates/wallet/sdk/src/models/input_selection/mod.rs(1 hunks)crates/wallet/sdk/src/models/mod.rs(1 hunks)crates/wallet/sdk/src/models/stealth_output.rs(2 hunks)crates/wallet/sdk/src/storage/reader.rs(2 hunks)crates/wallet/sdk/src/storage/writer.rs(2 hunks)crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql(1 hunks)crates/wallet/storage_sqlite/src/models/stealth_output.rs(4 hunks)crates/wallet/storage_sqlite/src/reader.rs(4 hunks)crates/wallet/storage_sqlite/src/schema.rs(1 hunks)crates/wallet/storage_sqlite/src/writer.rs(3 hunks)integration_tests/src/wallet_daemon_client.rs(1 hunks)integration_tests/tests/steps/wallet_daemon.rs(2 hunks)utilities/traffic-sim/src/sim.rs(2 hunks)
💤 Files with no reviewable changes (1)
- applications/tari_walletd/src/jrpc_server.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1551
File: applications/tari_indexer/src/storage_sqlite/models/utxo.rs:68-0
Timestamp: 2025-08-26T06:41:43.682Z
Learning: In the Tari codebase, u64/u32 values are stored as i64/i32 in SQLite to satisfy database constraints. Since both types have the same bit width and the data comes from trusted local databases, casting from i64/i32 back to u64/u32 is safe and preserves the original unsigned values.
📚 Learning: 2025-11-04T10:10:24.258Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1629
File: applications/tari_walletd/src/handlers/accounts.rs:1001-1002
Timestamp: 2025-11-04T10:10:24.258Z
Learning: In applications/tari_walletd/src/handlers/accounts.rs, the expect() on Memo::new_pay_ref_and_bytes_truncate at line 1002 is safe and intentional. PayRef is validated to be at most 64 bytes during address decoding (PayRef::MAX_LEN = 64), and the function only returns None if payref exceeds 252 bytes (Memo::MAX_BYTES_LENGTH - 1). Since 64 < 252, None is impossible with a valid PayRef.
Applied to files:
applications/tari_walletd/src/handlers/accounts.rsapplications/tari_walletd/src/handlers/confidential.rscrates/wallet/crypto/src/bullet_proof.rscrates/wallet/sdk/src/apis/stealth_outputs.rs
📚 Learning: 2025-08-26T06:41:43.682Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1551
File: applications/tari_indexer/src/storage_sqlite/models/utxo.rs:68-0
Timestamp: 2025-08-26T06:41:43.682Z
Learning: In the Tari codebase, u64/u32 values are stored as i64/i32 in SQLite to satisfy database constraints. Since both types have the same bit width and the data comes from trusted local databases, casting from i64/i32 back to u64/u32 is safe and preserves the original unsigned values.
Applied to files:
crates/wallet/storage_sqlite/src/models/stealth_output.rs
🧬 Code graph analysis (35)
crates/engine/tests/stealth.rs (2)
crates/template_lib_types/src/amount/amount.rs (1)
try_from(433-435)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
outputs(716-716)
applications/tari_wallet_cli/src/command/proof.rs (1)
bindings/src/types/wallet-daemon-client/ConfidentialCreateOutputProofRequest.ts (1)
ConfidentialCreateOutputProofRequest(3-3)
crates/wallet/storage_sqlite/src/schema.rs (4)
crates/wallet/crypto/src/unblinded_statement.rs (1)
value(60-62)crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1)
value(28-30)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (1)
value(21-23)bindings/src/helpers/BigAmount.ts (1)
value(21-23)
applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (2)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
params(700-712)crates/wallet/sdk/src/apis/stealth_transfer/api.rs (1)
params(523-527)
crates/wallet/crypto/tests/viewable_balance_proof.rs (6)
crates/wallet/crypto/src/confidential.rs (1)
create_output_statement(70-127)crates/wallet/crypto/src/unblinded_statement.rs (1)
value(60-62)crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1)
value(28-30)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (1)
value(21-23)crates/p2p/src/conversions/transaction.rs (2)
value(836-840)value(897-901)crates/engine_types/src/resource.rs (1)
view_key(126-128)
crates/engine/tests/recall.rs (3)
crates/template_lib/src/resource/builder/confidential.rs (1)
initial_supply(236-243)crates/template_lib/src/resource/builder/stealth.rs (1)
initial_supply(231-238)crates/template_test_tooling/src/support/confidential.rs (1)
generate_confidential_output_statement(17-22)
integration_tests/tests/steps/wallet_daemon.rs (2)
crates/template_lib/src/models/bucket.rs (1)
amount(206-214)crates/template_lib/src/models/proof.rs (1)
amount(104-112)
applications/tari_walletd/src/handlers/accounts.rs (2)
crates/wallet/sdk/src/models/stealth_output.rs (1)
from(71-79)applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_params(161-172)
crates/engine_types/src/crypto/helpers.rs (1)
crates/wallet/crypto/src/unblinded_statement.rs (1)
mask(64-66)
crates/wallet/crypto/src/stealth.rs (3)
crates/wallet/crypto/src/confidential.rs (1)
create_valid_proof(138-154)bindings/src/types/StealthOutputsStatement.ts (1)
StealthOutputsStatement(9-24)crates/engine_types/src/stealth/outputs.rs (1)
validate_stealth_outputs_statement(32-85)
clients/wallet_daemon_client/src/types.rs (1)
bindings/src/types/Amount.ts (1)
Amount(12-12)
crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (2)
crates/template_lib_types/src/amount/amount.rs (2)
new(52-54)zero(57-59)crates/wallet/sdk/src/apis/stealth_outputs.rs (2)
new(73-85)result(223-232)
crates/wallet/sdk/src/storage/reader.rs (2)
crates/wallet/sdk/src/apis/stealth_transfer/params.rs (1)
resource_address(154-161)crates/wallet/storage_sqlite/src/reader.rs (1)
stealth_outputs_get_unspent_for_spending(960-995)
crates/wallet/sdk/src/models/stealth_output.rs (4)
bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)bindings/src/types/EncryptedData.ts (1)
EncryptedData(7-7)
crates/wallet/sdk/src/apis/stealth_transfer/params.rs (3)
bindings/src/types/wallet-daemon-client/TransferOutput.ts (1)
TransferOutput(6-24)clients/wallet_daemon_client/src/types.rs (2)
total_output_amount(1077-1082)from(918-926)bindings/src/types/Amount.ts (1)
Amount(12-12)
crates/wallet/storage_sqlite/src/models/stealth_output.rs (2)
crates/template_lib/src/models/vault.rs (1)
from_hex(77-80)crates/template_lib_types/src/encrypted_data.rs (1)
len(39-41)
crates/wallet/storage_sqlite/src/writer.rs (3)
crates/wallet/sdk/src/storage/writer.rs (1)
stealth_outputs_lock_many(156-161)applications/tari_walletd/src/handlers/accounts.rs (1)
stealth_outputs(294-298)crates/wallet/storage_sqlite/src/reader.rs (6)
stealth_outputs(870-877)stealth_outputs(902-914)stealth_outputs(969-987)stealth_outputs(1004-1006)stealth_outputs(1040-1043)dsl(872-872)
applications/tari_walletd/src/handlers/confidential.rs (2)
applications/tari_walletd/src/handlers/helpers.rs (1)
invalid_request(191-196)crates/wallet/sdk/src/apis/confidential_transfer.rs (1)
confidential_amount(499-505)
crates/wallet/crypto/src/encrypted_data.rs (3)
crates/wallet/crypto/src/unblinded_statement.rs (2)
value(60-62)mask(64-66)crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1)
value(28-30)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (1)
value(21-23)
crates/wallet/sdk/src/apis/stealth_transfer/types.rs (2)
crates/wallet/crypto/src/unblinded_statement.rs (1)
value(60-62)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (1)
value(21-23)
crates/wallet/sdk/src/storage/writer.rs (2)
bindings/src/types/PedersenCommitmentBytes.ts (1)
PedersenCommitmentBytes(6-6)crates/wallet/storage_sqlite/src/writer.rs (1)
stealth_outputs_lock_many(1259-1294)
crates/engine/tests/confidential.rs (1)
crates/template_test_tooling/src/support/confidential.rs (1)
generate_confidential_output_statement(17-22)
crates/wallet/crypto/src/viewable_balance_proof.rs (3)
crates/engine_types/src/crypto/helpers.rs (1)
get_commitment_factory(50-52)bindings/src/types/ViewableBalanceProof.ts (1)
ViewableBalanceProof(27-62)bindings/src/types/Scalar32Bytes.ts (1)
Scalar32Bytes(3-3)
crates/wallet/crypto/src/bullet_proof.rs (1)
crates/engine_types/src/stealth/outputs.rs (1)
stmt(43-82)
crates/wallet/crypto/src/confidential.rs (3)
crates/engine_types/src/crypto/helpers.rs (1)
commit_u64_amount(74-76)crates/wallet/crypto/src/stealth.rs (1)
create_valid_proof(167-185)crates/engine_types/src/confidential/validation.rs (1)
validate_confidential_statement(25-119)
crates/wallet/sdk/src/apis/stealth_transfer/api.rs (3)
crates/wallet/sdk/src/apis/stealth_transfer/types.rs (2)
total_stealth_input_amount(55-57)value(28-30)crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
params(700-712)crates/wallet/crypto/src/unblinded_statement.rs (1)
value(60-62)
crates/wallet/storage_sqlite/src/reader.rs (2)
crates/wallet/storage_sqlite/src/writer.rs (10)
stealth_outputs(126-126)stealth_outputs(127-127)stealth_outputs(157-164)stealth_outputs(213-213)stealth_outputs(214-214)stealth_outputs(229-229)stealth_outputs(230-230)stealth_outputs(1217-1233)stealth_outputs(1341-1341)stealth_outputs(1342-1342)crates/wallet/sdk/src/storage/reader.rs (1)
stealth_outputs_get_unspent_for_spending(136-141)
crates/wallet/sdk/src/apis/confidential_transfer.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)crates/wallet/sdk/src/models/stealth_output.rs (1)
from(71-79)
crates/wallet/sdk/src/apis/confidential_outputs.rs (3)
crates/wallet/crypto/src/unblinded_statement.rs (1)
value(60-62)crates/wallet/sdk/src/apis/stealth_transfer/types.rs (1)
value(28-30)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (1)
value(21-23)
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
new(32-42)lock_outputs_internal(79-110)crates/wallet/sdk/src/models/stealth_output.rs (1)
from(71-79)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (2)
inputs(72-72)select(65-153)
crates/template_test_tooling/src/support/stealth.rs (3)
bindings/src/types/Amount.ts (1)
Amount(12-12)crates/template_lib/src/models/stealth.rs (1)
revealed_output_amount(116-118)bindings/src/types/StealthOutputsStatement.ts (1)
StealthOutputsStatement(9-24)
crates/wallet/crypto/src/unblinded_statement.rs (1)
crates/engine_types/src/crypto/helpers.rs (1)
commit_u64_amount(74-76)
crates/template_test_tooling/src/support/confidential.rs (3)
bindings/src/types/ConfidentialOutputStatement.ts (1)
ConfidentialOutputStatement(10-32)bindings/src/types/Amount.ts (1)
Amount(12-12)crates/template_lib_types/src/amount/amount.rs (1)
zero(57-59)
crates/engine/tests/burn.rs (2)
crates/template_lib/src/resource/builder/stealth.rs (1)
initial_supply(231-238)crates/template_test_tooling/src/support/confidential.rs (1)
generate_confidential_output_statement(17-22)
crates/wallet/crypto/tests/stealth_transfer_statement.rs (2)
crates/template_lib_types/src/amount/amount.rs (1)
new(52-54)crates/wallet/crypto/src/unblinded_statement.rs (2)
new(40-42)mask(64-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (51)
crates/transaction/Cargo.toml (2)
28-28: Verify completeness of the provided code snippet.The AI summary mentions removing a dev-dependency on
tari_borfrom the [dev-dependencies] section, but the provided code snippet ends at line 28 (thetsfeature definition) and does not show the [dev-dependencies] section. Please confirm that the full file, including any dev-dependencies removal, is captured in the review context.
28-28: TypeScript bindings expansion is correctly scoped.The
tsfeature now properly enables TypeScript bindings for all dependent crates (tari_engine_types,tari_ootle_common_types,tari_template_lib,tari_bor) that are already declared as dependencies. This aligns with the PR's type system refactoring (u64/bigint vs Amount), ensuring that TypeScript bindings are generated for crates affected by these changes.crates/wallet/sdk/Cargo.toml (1)
44-55: No issues found. All referenced crates expose atsfeature.Verification confirms that all 8 crates (
tari_ootle_address,tari_bor,tari_engine_types,tari_template_abi,tari_template_lib,tari_transaction,tari_ootle_wallet_crypto,tari_consensus_types) have thetsfeature available. The change is safe and ready for merge.applications/tari_walletd/src/handlers/accounts.rs (4)
296-298: LGTM: Type conversion aligns with PR objectives.The conversion of
o.valuetoAmountusingAmount::from()is consistent with the PR's goal of using u64 for single UTXO values and Amount for aggregations. The filter change (removing owner_account constraint) is appropriate since the outputs are already scoped to the account viaget_unspent_outputs_by_account()on line 288.
327-328: LGTM: Consistent type handling for aggregations.The
Amount::from(o.value)conversions are consistent with the earlier changes and follow the PR's design of using Amount for aggregations that may exceed u64::MAX.
476-495: LGTM: Proper u64 arithmetic with appropriate edge case handling.The changes correctly handle u64 arithmetic:
- Line 478:
checked_subis appropriate for u64 subtraction with overflow protection- Line 481: Zero-check prevents claiming when fees consume the entire amount
- Line 495: Passing u64
final_amounttoencrypt_value_and_maskaligns with the comment on lines 491-493 about u64::MAX limits in confidential encryption
1157-1157: LGTM: Idiomatic numeric comparison.The change from
blinded_amount.is_positive()toblinded_amount > 0is idiomatic for numeric types and correctly filters out zero-value outputs.applications/tari_walletd/web_ui/src/services/api/hooks/useAccounts.ts (1)
169-169: Review comment is incorrect — the code is safe from BigInt conversion errors.The amount passed to
BigInt(params.amount)is guaranteed to be an integer. In SendMoney.tsx (lines 196 and 272), the amount is calculated as:Math.floor((parseFloat(transferFormState.amount) || 0) * Math.pow(10, balanceEntry.divisibility))The
Math.floor()call ensures only integers are passed toTransferParams, eliminating any decimal values that would cause aRangeErrorinBigInt().The inconsistency between line 169 (
BigInt(params.amount)) and line 170 (params.amountused directly) exists, but both receive the same integer value, so both operations are correct.Likely an incorrect or invalid review comment.
crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
264-264: No issues found — the conversion target type is clearly defined and properly supported.The
.into()call convertsvalue(u64, per PR objective) intoAmountvia the macroimpl_from!(Amount, u64)at line 423 ofcrates/template_lib_types/src/amount/amount.rs. The type is not ambiguous;ConfidentialOutputModel.valueexpectsAmount, and the conversion is explicitly implemented. The code is correct.
256-256: Original review comment is based on incorrect assumptions about data flow.The code is safe. Invalid outputs created with the
0sentinel value (line 256) are stored withOutputStatus::Invalidand are filtered out by the database query before reaching aggregation logic. Theconfidential_outputs_lock_smallest_amount()query explicitly filters:status.eq(OutputStatus::Unspent.as_key_str()), ensuring only valid outputs are aggregated at line 100. The type conversion using.into()(line 264) correctly converts the u64 toAmount, and this pattern mirrors the identical implementation instealth_outputs.rs.Likely an incorrect or invalid review comment.
clients/wallet_daemon_client/src/types.rs (1)
1078-1081: Good call on converting per-output amounts toAmount.Wrapping each blinded amount in
Amountbefore summing avoids the previousu64overflow risk and keeps the aggregate accurate.crates/engine/tests/recall.rs (1)
24-25: Test update matches new helper signatureSwitching to the primitive literal keeps the test in sync with the u64-based helper; everything else still lines up.
crates/wallet/storage_sqlite/migrations/2023-02-08-122514_initial/up.sql (1)
253-254: Column type change looks goodMigrating
stealth_outputs.valuetoBIGINTkeeps schema aligned with the new u64-backed model.bindings/src/types/wallet-daemon-client/TransferOutput.ts (1)
19-19: Bindings update aligns with bigint migration
blinded_amountadoptingbigintkeeps this generated type consistent with the rest of the wallet-daemon bindings changes.crates/engine/tests/burn.rs (1)
13-13: LGTM! Type conversion aligns with function signature.The call site correctly passes a plain
u64literal instead ofAmount::from(1000), matching the updated signature ofgenerate_confidential_output_statement(output_amount: u64, ...).crates/wallet/sdk/src/models/mod.rs (1)
10-10: LGTM! New input selection module exposed.The new
input_selectionmodule provides the branch-and-bound algorithm referenced in the PR objectives. This is the expected public API expansion.utilities/traffic-sim/src/sim.rs (2)
232-232: LGTM! Type conversion removed per API change.The
blinded_output_amountfield now acceptsu64directly, eliminating the need for.into()conversion.
460-460: LGTM! Type conversion removed per API change.The
blinded_amountfield now acceptsu64directly, eliminating the need for.into()conversion.clients/javascript/wallet_daemon_client/src/index.ts (1)
20-20: API surface reduction noted.Per the AI summary, the
accountsGetPayRefAddressmethod and related types (AccountsGetPayRefAddressRequest,AccountsGetPayRefAddressResponse) were removed. While the removal itself isn't visible in the annotated code, this represents a breaking change to the public JavaScript client API.crates/wallet/crypto/tests/viewable_balance_proof.rs (2)
19-29: LGTM! Helper function updated for u64 amounts.The
create_output_statementfunction signature correctly changed fromAmounttou64, and the implementation directly assigns thevalueparameter toamountfield.
47-47: LGTM! Test call sites updated consistently.All invocations of
create_output_statementcorrectly pass plainu64literals instead ofAmountvalues.Also applies to: 66-66, 97-98
bindings/src/types/wallet-daemon-client/StealthTransfer.ts (1)
8-8: LGTM! Breaking change:bigintreplacesAmount.The
blinded_output_amountfield type changed fromAmounttobigint, aligning with the Rust-side shift tou64. This is a breaking change for TypeScript consumers who must now providebigintvalues directly.crates/wallet/crypto/src/encrypted_data.rs (2)
67-67: LGTM! Unnecessary conversion removed.The
MaskAndValueconstruction now directly usesvalueinstead ofvalue.into(), consistent with the field now beingu64.
40-40: Code is correct—to_commitment()returnsPedersenCommitmentdirectly, notOption.The
DecryptedData::to_commitment()method incrates/wallet/crypto/src/unblinded_statement.rs(lines 72–74) has a return type ofPedersenCommitment, notOption<PedersenCommitment>. The assignment at line 40 is safe with no implicit unwrapping or runtime panic risk.Likely an incorrect or invalid review comment.
crates/wallet/storage_sqlite/src/schema.rs (1)
156-156: Review comment is factually incorrect regarding the schema change.The
stealth_outputs.valuecolumn has beenBIGINTsince the initial migration (2023-02-08-122514_initial), not changing fromTexttoBigInt. The schema.rs file is auto-generated by Diesel and simply reflects the existing database structure. No migration is needed because no column type conversion is occurring.Likely an incorrect or invalid review comment.
integration_tests/src/wallet_daemon_client.rs (1)
106-106: LGTM! Type change aligns with the Amount → u64 migration.The function signature update from
Amounttou64is consistent with the PR objective to use u64 for individual UTXO values and simplifies the API by removing the need for type conversions at call sites.integration_tests/tests/steps/wallet_daemon.rs (1)
439-439: LGTM! Call sites correctly updated to match the new signature.The removal of
.into()conversions is appropriate sincetransfer_stealthnow acceptsu64directly instead ofAmount.Also applies to: 464-464
applications/tari_wallet_cli/src/command/proof.rs (2)
38-38: Good change: amounts should be unsigned.Changing from
i64tou64is semantically correct for amounts and improves type safety by rejecting negative values at the CLI boundary. Note that this could be a breaking change if users previously (incorrectly) passed negative values.
69-69: LGTM! Conversion removed to match updated API.The removal of
.into()is correct sinceConfidentialCreateOutputProofRequestnow expectsamount: u64directly.crates/wallet/sdk/src/apis/stealth_transfer/types.rs (3)
28-30: LGTM! Correctly implements the u64-for-individual-values pattern.The return type change from
Amounttou64aligns with the PR objective to use u64 for single UTXO values.
36-36: LGTM! Field type updated consistently.The field type change from
Amounttou64is consistent with the broader migration pattern.
56-56: Excellent overflow safety pattern.The implementation correctly uses
Amount::from(i.value)when summing individual u64 values, which allows the sum to exceed u64::MAX without overflow. This matches the PR's stated design: "use Amount when the upper bound may exceed u64::MAX."crates/engine/tests/stealth.rs (2)
103-103: LGTM! Test data simplified with direct numeric literals.The removal of
.into()conversions simplifies test data construction now thatMaskAndValue.valueisu64directly. The changes correctly adapt to the updated type system.Also applies to: 141-141, 177-177, 219-219, 223-223, 273-273, 305-305, 343-343, 392-392, 461-461, 505-505, 509-509, 580-580, 584-584, 646-646, 650-650
396-396: Good practice: explicit type conversion.Using
u64::try_from(...).unwrap()makes the type conversion explicit and would catch overflow issues if the division result somehow exceeded u64::MAX. This is better than an implicit conversion.bindings/src/types/wallet-daemon-client/ConfidentialCreateOutputProofRequest.ts (1)
3-3: LGTM! Consistent type migration in TypeScript bindings.The type change from
Amounttobigintaligns with the broader migration to use native numeric types in the TypeScript bindings. Note this is a breaking change for consumers of this type.crates/wallet/crypto/src/stealth.rs (3)
62-62: LGTM! Commitment extraction simplified correctly.The direct call to
to_commitment()without error handling aligns with the new u64-based amount system where commitment creation is always valid for any u64 value.Also applies to: 121-121
167-185: LGTM! Test helper updated to use u64.The signature change from
Amounttou64is consistent with the broader migration to native numeric types throughout the codebase.
189-189: LGTM! Test calls updated for u64 parameters.Also applies to: 195-195
crates/engine/tests/confidential.rs (2)
71-71: LGTM! Test updated for u64 parameter.
362-362: LGTM! Tuple values simplified to raw integers.The removal of
.into()conversions aligns with the helper function now accepting raw u64 values.crates/wallet/sdk/src/apis/stealth_transfer/params.rs (2)
44-44: LGTM! Validation logic updated for u64 type.The change from
is_positive()to> 0andis_zero()to== 0correctly adapts the validation logic for the new u64 type.Also applies to: 63-63
111-111: Verification confirmed: TypeScript bindings correctly match the Rust definition.The
blinded_amountfield is properly typed asbigintin the TypeScript bindings, which is the correct representation for Rust'su64type. The migration is consistent across both Rust and TypeScript codebases.crates/wallet/storage_sqlite/src/writer.rs (2)
29-36: LGTM! Import updated for enhanced error messages.The addition of
Displayableenables better formatting of UTXO arrays in error messages (line 1288).
1310-1310: LGTM! Value storage optimized to use i64.The change from
value.to_string()tovalue as i64improves storage efficiency and aligns with the established pattern of storing u64 values as i64 in SQLite. As per learnings, this cast is safe and preserves the original unsigned value.Based on learnings
crates/wallet/sdk/src/storage/writer.rs (1)
13-13: LGTM! New trait method for batch locking added.The
stealth_outputs_lock_manymethod extends theWalletStoreWritertrait to support batch locking of stealth outputs. The signature is well-defined and matches the implementation instorage_sqlite.Also applies to: 156-161
crates/wallet/crypto/src/confidential.rs (4)
5-5: LGTM! Commitment creation updated to use u64 helper.The change from
commit_amount_checkedtocommit_u64_amountsimplifies the code by removing unnecessary error handling for non-negative amounts, since u64 is inherently non-negative.Also applies to: 38-38
79-79: LGTM! Commitment extraction simplified.The removal of
.ok_or(...)indicates thatto_commitment()now returnsPedersenCommitmentdirectly instead ofOption<PedersenCommitment>, which is consistent with u64-based commitments always being valid.Also applies to: 101-101
96-96: LGTM! Unnecessary validation removed.Since the amount is now u64, the
non_negative_checked()call is no longer needed. Usingunwrap_or_default()directly is correct and simpler.
134-154: LGTM! Tests updated for u64 parameters.The test helper signature and calls are correctly updated to use raw u64 values instead of
Amounttypes.Also applies to: 158-169
crates/wallet/storage_sqlite/src/models/stealth_output.rs (2)
30-30: LGTM! Value storage optimized to i64.The change from
Stringtoi64for thevaluefield improves storage efficiency and type safety. The cast fromi64tou64at line 65 is safe and preserves the original unsigned value, as per learnings.Based on learnings
Also applies to: 65-65
101-137: LGTM! Conversion to StealthOutputInfo added.The new
TryFromimplementation provides a clean conversion path from the database model toStealthOutputInfo. Error handling is comprehensive with specific operation names and detailed error messages for each field.
35d8eb1 to
0dc065e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bindings/src/types/wallet-daemon-client/ProofsGenerateRequest.ts (1)
9-9: Breaking change: field renamed (type preserved).The field was renamed from
amounttoconfidential_amount, which is a breaking API change for TypeScript consumers. However, the type remainsAmount(notbigintas claimed in the past review comment). This contradicts the PR description stating "Breaking changes: None."Note: The past review comment mentioned both a field rename and type change to
bigint, but the current code shows only the field name changed while the typeAmountwas preserved.
🧹 Nitpick comments (1)
crates/wallet/sdk/src/apis/stealth_outputs.rs (1)
225-225: Redundant.take(INPUT_LIMIT)operation.The
branch_and_bound::selectfunction already respects themax_inputsparameter (passed asINPUT_LIMITon line 216), so it cannot return more thanINPUT_LIMITselected keys. The.take(INPUT_LIMIT)here is defensive but unnecessary.Apply this diff to remove the redundant operation:
let outputs = result .selected_keys() .iter() - .take(INPUT_LIMIT) .map(|selected| {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
applications/tari_walletd/src/handlers/confidential.rs(7 hunks)bindings/src/types/wallet-daemon-client/ProofsGenerateRequest.ts(1 hunks)clients/wallet_daemon_client/src/types.rs(4 hunks)crates/wallet/sdk/src/apis/stealth_outputs.rs(10 hunks)crates/wallet/storage_sqlite/src/reader.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/wallet_daemon_client/src/types.rs
- applications/tari_walletd/src/handlers/confidential.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T10:10:24.258Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1629
File: applications/tari_walletd/src/handlers/accounts.rs:1001-1002
Timestamp: 2025-11-04T10:10:24.258Z
Learning: In applications/tari_walletd/src/handlers/accounts.rs, the expect() on Memo::new_pay_ref_and_bytes_truncate at line 1002 is safe and intentional. PayRef is validated to be at most 64 bytes during address decoding (PayRef::MAX_LEN = 64), and the function only returns None if payref exceeds 252 bytes (Memo::MAX_BYTES_LENGTH - 1). Since 64 < 252, None is impossible with a valid PayRef.
Applied to files:
crates/wallet/sdk/src/apis/stealth_outputs.rs
🧬 Code graph analysis (3)
bindings/src/types/wallet-daemon-client/ProofsGenerateRequest.ts (1)
bindings/src/types/Amount.ts (1)
Amount(12-12)
crates/wallet/storage_sqlite/src/reader.rs (2)
crates/wallet/storage_sqlite/src/writer.rs (10)
stealth_outputs(126-126)stealth_outputs(127-127)stealth_outputs(157-164)stealth_outputs(213-213)stealth_outputs(214-214)stealth_outputs(229-229)stealth_outputs(230-230)stealth_outputs(1217-1233)stealth_outputs(1341-1341)stealth_outputs(1342-1342)crates/wallet/sdk/src/storage/reader.rs (1)
stealth_outputs_get_unspent_for_spending(136-141)
crates/wallet/sdk/src/apis/stealth_outputs.rs (3)
crates/wallet/sdk/src/apis/confidential_outputs.rs (2)
new(32-42)lock_outputs_internal(79-110)crates/wallet/sdk/src/models/stealth_output.rs (1)
from(71-79)crates/wallet/sdk/src/models/input_selection/branch_and_bound.rs (2)
inputs(72-72)select(65-153)
⏰ 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 stable
- GitHub Check: clippy
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: machete
- GitHub Check: fmt
🔇 Additional comments (7)
crates/wallet/storage_sqlite/src/reader.rs (3)
42-42: LGTM: Import addition.The
StealthOutputInfoimport is correctly used in the updated method signatures below.
920-958: LGTM: Enhanced method signature and return type.The addition of the optional
resource_addressparameter and the return type change toStealthOutputInfoimprove the API flexibility. The method appropriately serves general querying purposes, while spending-specific filters are handled by the dedicatedstealth_outputs_get_unspent_for_spendingmethod below.
960-998: LGTM: Spendability predicates correctly implemented.The method now includes all required spendability filters (lines 987-989), resolving the critical issue flagged in the previous review. The implementation correctly:
- Excludes view-only outputs (
owner_key_id.is_not_null())- Excludes burnt outputs (
is_burnt.eq(false))- Excludes frozen outputs (
is_frozen.eq(false))- Allows outputs created within the transaction (
LockedUnconfirmedwith matchinglock_id)This ensures only truly spendable outputs are returned for branch-and-bound input selection, preventing downstream signing and locking failures.
crates/wallet/sdk/src/apis/stealth_outputs.rs (4)
103-105: Good optimization for zero amount case.The early return when amount is zero avoids unnecessary processing and database transactions.
168-175: Excellent fix for the INPUT_LIMIT bug!The INPUT_LIMIT check is now correctly positioned before the locking call, which prevents locking the (limit+1)th UTXO that wouldn't be returned. This addresses the critical issue raised in the previous review.
640-640: Appropriate type change for individual UTXO value.Changing from
Amounttou64for individual output amounts aligns with the PR's approach of usingu64for single UTXO values and reservingAmountfor sums that may exceedu64::MAX.
713-715: Correct use of Amount for summing values.Converting individual UTXO values to
Amountbefore summing ensures the total can safely exceedu64::MAX, which aligns with the PR's approach of usingAmountfor aggregate values.
2 participants
Description
feat: implement a simplified branch-and-bound algorithm for spend input selection.
fix: use u64 for all single utxo values, and Amount for when the upper bound exceeds u64::MAX (e.g sum of funds)
fix: account for and respect the upper input limit (currently 1000)
Motivation and Context
Branch-and-Bound is potentially very good at minimising waste, change & fragmentation when selecting inputs. This is a simplified version of what is used in Bitcoin Core. The simplifications are due to not accounting for input weight (since all inputs have equal weights) and target fees.
The input limit is not respected.
How Has This Been Tested?
New unit tests, partially manually although not with a wallet that has many thousands of UTXOs to spend
What process can a PR reviewer use to test or verify this change?
Breaking Changes
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes