Flexible Aggregator

[illuzen]

Summary

Adds dual-output support and random partitioning to the wormhole multiround flow, plus developer tooling improvements.

Key Changes

Dual Output Random Partitioning

• Each proof can send to two exit accounts with separate amounts
• compute_random_output_assignments() randomly distributes outputs across targets
• Enables enhanced privacy through unpredictable output distribution

New Developer Command

quantus developer build-circuits --circuits-path ../qp-zk-circuits --chain-path ../chain

Builds circuit binaries and copies to CLI/chain directories.

Simplified Verification

• Removed single-proof verify command (use verify-aggregated)
• Added --verify flag to parse-proof for local cryptographic verification

Cleanup

• Removed unused multisend command
• Fixed deprecation warnings in keystore
• Updated dilithium/hdwallet dependencies

Testing

• Multiround flow tested: 2 rounds × 2 proofs, 3 rounds x 3 proofs
• Dual outputs verified on-chain (multiple NativeTransferred events)
• Balance verification passes within tolerance

[n13]

Gemini

Here is a review of the changes:

🛑 Blockers

• Cargo.toml Patches: The [patch] section (lines 21-31) points to local directories (../qp-zk-circuits, ../chain). This will break the build for other developers or CI environments that don't have these repositories checked out in the exact same relative paths. These patches should be removed, and the dependencies should point to git repositories or published crates.

⚠️ Concerns

• Hardcoded Paths in BuildCircuits: In src/cli/mod.rs, the BuildCircuits command defaults to ../qp-zk-circuits and ../chain. While these are just defaults, they imply a specific workspace structure. Consider if this is the intended workflow for all users.
• wormhole.rs Size: src/cli/wormhole.rs is quite large (~137KB). Consider breaking this down into smaller modules (e.g., proofs.rs, commands.rs, utils.rs) to improve maintainability.

✨ Improvements

• Timing in main.rs: Adding execution time to the CLI output is a nice UX touch!
• Wallet Updates: The updates to qp-rusty-crystals-hdwallet and keystore.rs seem to clean up the key derivation logic nicely.

Code-Specific Comments

• src/cli/wormhole.rs:
  • The error handling in generate_proof generally looks good, mapping errors to QuantusError.
  • The use of expect("BytesDigest is always 32 bytes") seems safe given the type guarantees.

Action Items:

1. Remove the [patch] section from Cargo.toml.
2. Verify that the updated qp-rusty-crystals-* dependencies are available/published.

Great work on the integration! Just need to fix the dependency paths to make it merge-ready.

[n13]

Opus Pro 4.6

Here's my review of PR #62. This is a substantial PR that touches the wormhole module, wallet layer, and developer tooling. I'll break it down by severity.

---

PR #62 Review

What This PR Does

• Splits wormhole Generate into Transfer + Generate (decoupling the transfer submission from proof generation)
• Adds Multiround command for multi-round wormhole flow testing with random output partitioning
• Adds ParseProof command for debugging proof files
• Adds BuildCircuits developer command to build and distribute circuit binaries
• Switches to pre-built circuit binaries (new_from_files / from_prebuilt_with_paths) instead of runtime circuit construction
• Dual output support (output_amount_1/output_amount_2, exit_account_1/exit_account_2)
• Updates HD wallet API to new derive_key_from_mnemonic / SensitiveBytes32
• Adds command timing to main.rs

---

Critical Issues

1. [patch] sections should NOT be merged

The Cargo.toml adds patch sections pointing to local filesystem paths:

[patch."https://github.com/Quantus-Network/qp-zk-circuits"]
qp-wormhole-circuit = { path = "../qp-zk-circuits/wormhole/circuit" }
...
[patch.crates-io]
qp-dilithium-crypto = { path = "../chain/primitives/dilithium-crypto" }

This will break builds for anyone who doesn't have sibling repos at those exact paths. This looks like development-only config that should be removed before merge.

2. Balance verification math appears incorrect in run_multiround

The amount parameter is documented as "Amount per transfer" but used as the total amount partitioned across proofs:

let partition_amounts = random_partition(amount, num_proofs, min_per_proof);

So amount is the total sent. But the final verification does:

let total_sent = (num_proofs as u128) * amount;  // Bug: should just be `amount`
let final_round_amount = calculate_round_amount(amount, rounds);
let total_received = (num_proofs as u128) * final_round_amount;  // Bug: same issue

This multiplies by num_proofs again, inflating both expected sent and received amounts. The verification will still "pass" (the ratio stays the same) but the logged numbers will be wrong and the tolerance calculation will be off.

---

DRY Violations (High Priority)

3. generate_proof() and generate_proof_with_dual_output() are ~80% identical

Both functions:

• Parse secrets, block hashes, funding accounts the same way
• Generate unspendable accounts identically
• Build storage keys identically
• Fetch storage proofs identically
• Load provers from pre-built bins identically

The only difference is how PublicCircuitInputs is constructed (single output vs dual output). This should be extracted into a shared helper that builds the PrivateCircuitInputs and storage proof, with only the public inputs differing.

4. verify_aggregated_proof() and verify_aggregated_and_get_events() overlap significantly

Both submit unsigned aggregated proof transactions and watch for status. check_proof_verification_events() also duplicates some event-processing logic. Consider having verify_aggregated_proof call verify_aggregated_and_get_events and discard the events.

5. include_bytes! vs runtime file loading inconsistency

• generate_proof and aggregate_proofs load circuit data at runtime via new_from_files()
• verify_aggregated_and_get_events and parse_proof_file embed circuit data at compile time via include_bytes!("../../generated-bins/...")

This is inconsistent and means the binary must be compiled with generated-bins/ present even if you don't use those commands. Pick one approach.

---

Medium Issues

6. Verify (single proof) command was silently removed

The old code had WormholeCommands::Verify for single proof on-chain verification. The new code only has VerifyAggregated. Was this intentional? If single-proof verification is no longer supported, it should be documented.

7. run_multiround is ~500 lines

This function is very long and handles wallet loading, secret derivation, transfers, proof generation, aggregation, on-chain verification, balance tracking, and cleanup. Breaking it into clearly named phase functions would improve readability and testability.

8. CLI arg description mismatch

/// Amount per transfer in planck (default: 10 DEV)
#[arg(short, long, default_value = "10000000000000")]
amount: u128,

The help says "per transfer" but the code uses it as a total that gets partitioned. This will confuse users.

9. parse_exit_account used for non-exit-accounts

At line ~2001 in the new code, parse_exit_account is called to parse a funding account. Consider renaming to something generic like parse_account_id to avoid confusion.

---

Minor / Nits

10. #[allow(dead_code)] on TransferInfo -- the amount field IS used (current_transfers.iter().map(|t| t.amount)), so this allow may not be needed.

11. parse_transfer_events matches events only by to address. If multiple transfers to the same address appear in one block, it will pick the first one, which could be wrong.

12. No tests for the new functions: format_balance, random_partition, compute_random_output_assignments, calculate_round_amount, derive_wormhole_secret. These are pure functions that would be easy to unit test.

13. The WormholeVerifier import was removed from the top of wormhole.rs but is still used via local use statements in several functions. Fine for correctness, but a bit inconsistent.

---

Summary

Severity	Count	Key Items
Critical	2	Local [patch] in Cargo.toml; balance verification math bug
High (DRY)	3	Duplicate proof generation, duplicate verification, include_bytes inconsistency
Medium	4	Removed Verify command, long function, arg description mismatch, function naming
Minor	4	Dead code allow, event matching, missing tests, import style

The main blockers are the [patch] sections in Cargo.toml (will break CI/other devs) and the substantial code duplication in the proof generation / verification paths. The balance verification bug should also be fixed before merge.