chore: support the new wormhole aggregation pipeline#71
Conversation
|
I now have a thorough understanding of the PR. Here is my comprehensive review. PR #71 Review: Support the new wormhole aggregation pipelineSummary: This PR upgrades the wormhole ZK circuit crates from v1.0.7 to v1.2.1, adopts the new aggregation pipeline from qp-zk-circuits#96, restructures storage proof keys, adds a 1. Dependency Concerns (High Priority)All wormhole crates are patched to git branches, not released versions: [patch.crates-io]
qp-zk-circuits-common = { git = "...", branch = "illuzen/back-to-compact" }
qp-wormhole-circuit = { git = "...", branch = "illuzen/back-to-compact" }
# ...7 more patchesAnd qp-dilithium-crypto = { git = "https://github.com/Quantus-Network/chain.git", branch = "illuzen/split-hashers" }The PR description acknowledges this ("Once that one is approved we can release it and update the wormhole crates here"), but this PR should not be merged until those branches are released to crates.io. Git branch dependencies are fragile -- force-pushes or branch deletions would break the build. 2. Architecture Changes (Good)
Aggregation API migration --
3. DRY Violations (Medium Priority)The following types and constants are defined identically in 3 places:
The integration test file should import from use quantus_cli::{
TransferProofKey, TransferProofData, NATIVE_ASSET_ID,
SCALE_DOWN_FACTOR, VOLUME_FEE_BPS, compute_output_amount,
};Additionally, the storage key computation is duplicated between 4. Storage Proof Key Restructure (Important Change)The This is a cleaner design:
The auto-generated 5. Code Quality Issues
struct FuzzProofParams<'a> {
bins_dir: &'a Path,
read_proof: &'a ReadProof<sp_core::H256>,
state_root_hex: &'a str,
// ...
}Fuzz test is ~840 lines embedded in Custom PRNG (
6.
|
| Area | Unit Tests | Integration Tests | Notes |
|---|---|---|---|
compute_leaf_hash |
NONE | Covered indirectly | Should have a unit test with a known vector |
compute_wormhole_address |
NONE | Covered indirectly | Should have a unit test |
compute_nullifier |
NONE | Covered indirectly | Marked #[allow(dead_code)] |
compute_storage_key |
length check only | Covered | Should verify actual bytes against known value |
quantize_amount |
YES (2 cases) | -- | Good |
compute_output_amount |
YES (2 cases) | -- | Good |
generate_proof |
NONE | Covered (requires chain) | Needs prover bins -- hard to unit test |
CircuitBinsConfig deser |
YES (updated) | -- | Good |
DilithiumPair API |
YES (existing) | -- | Tests updated for .public() / .secret_bytes() |
Layer0Aggregator |
NONE | Covered (requires chain) | -- |
| Fuzz test | Interactive CLI | -- | Not automated; requires running chain |
build.rs |
NONE | -- | Hard to test; verified by successful build |
Key gap: compute_leaf_hash and compute_wormhole_address are critical security functions with no unit tests. These should have deterministic test vectors to catch regressions (e.g., if the upstream Poseidon2 hash changes).
The fuzz test is a CLI command, not an automated test. It requires a running chain node and a funded wallet. Consider adding at least a unit-level fuzz test that validates the generate_fuzz_cases function itself (correct expected-pass/fail flags) and a circuit-level test that doesn't require a chain.
The 22 unit tests in wormhole.rs are preserved (same count) and updated for the new APIs. The 2 integration tests in tests/wormhole_integration.rs are updated. The 3 new tests in wormhole_lib.rs are minimal.
8. Minor Issues
-
Treasury
Permilldisplay:portion as f64 / 10000.0-- Permill is parts-per-million (0..1,000,000), so dividing by 10,000 gives a percentage. This is correct. -
RemoveBuildCircuitsfromDeveloperCommands-- TheBuildCircuitsCLI subcommand was removed sincebuild.rshandles it. But the README still referencesquantus developer build-circuitsworkflow. The README changes describe it differently but the command no longer exists. -
No CI checks reported on the branch -- the PR has 0 CI runs and 0 comments. This should be investigated.
Summary Recommendations
- Block merge until upstream branches (
illuzen/back-to-compact,illuzen/split-hashers) are released and[patch.crates-io]is removed - Fix DRY violations -- integration tests should import from the library instead of redefining types/constants
- Add unit tests for
compute_leaf_hashandcompute_wormhole_addresswith known test vectors - Refactor the fuzz test -- extract to its own file, use a struct for
try_generate_fuzz_proofparams, useChaCha8Rng - Add
cargo:rerun-if-env-changedtobuild.rs - Run CI before merging
Supports the new aggregation pipeline defined here. Once that one is approved we can release it and update the wormhole crates here.