Skip to content

feat: Expose wormhole helpers as SDK + add e2e/read-only examples#104

Merged
czareko merged 3 commits into
mainfrom
feat/stress_test_wormhole_params
Apr 30, 2026
Merged

feat: Expose wormhole helpers as SDK + add e2e/read-only examples#104
czareko merged 3 commits into
mainfrom
feat/stress_test_wormhole_params

Conversation

@czareko
Copy link
Copy Markdown
Contributor

@czareko czareko commented Apr 30, 2026

No description provided.

@czareko czareko requested review from ethan-crypto, illuzen and n13 April 30, 2026 01:02
@n13
Copy link
Copy Markdown
Contributor

n13 commented Apr 30, 2026

I now have everything I need to deliver the review. Cargo check, clippy, and tests all build cleanly on the PR branch.

PR #104 Review — feat: Expose wormhole helpers as SDK + add e2e/read-only examples

Quantus-Network/quantus-cli#104+888 / -26 across 6 files. Already approved by @illuzen; awaiting @ethan-crypto and @n13.

What it does

  1. SDK surface — promotes a set of wormhole helpers in src/cli/wormhole.rs from private to pub: at_best_block, aggregate_proofs, IncludedAt, submit_unsigned_verify_aggregated_proof, verify_aggregated_and_get_events, parse_transfer_events, decode_full_leaf_data, TransferInfo. They're re-exported alongside NativeTransferred from src/lib.rs.
  2. Two new examplesexamples/wormhole_sdk_usage.rs (offline + read-only online demo) and examples/wormhole_sdk_e2e.rs (full deposit→proof→mint loop).
  3. Security bumprustls-webpki 0.103.120.103.13 (RUSTSEC-2026-0104).

Verification I ran locally

  • cargo check --examples — green
  • cargo clippy --examples — no warnings
  • cargo test --lib --no-run — green
  • Diff scanned end-to-end against main

Strengths

  • Error model cleanup. at_best_block was using anyhow::Result in a file that otherwise threads crate::error::Result. This PR fixes that and wraps the inner subxt error in QuantusError::NetworkError(...). Nice composition win for SDK consumers.
  • Logging style fix in verify_aggregated_and_get_events — replaces a stray println!("[quantus-cli] Circuit binaries: ...") with log_verbose!, matching the rest of the file.
  • API ergonomicsIncludedAt now derives PartialEq, Eq and gets a Display impl; small but right.
  • Docs are excellent — both examples carry rich rustdoc that walks through the deposit → proof → aggregate → verify flow and call out exactly which pieces of the SDK get exercised.
  • Graceful degradation in wormhole_sdk_usage.rs — if no node is reachable, it still runs the offline section and exits clean (good for CI).
  • The security bump is minimal and well-scoped. Comment correctly cites the new RUSTSEC ID.

Issues

MAJOR

1. DRY violation — compute_merkle_positions is duplicated.

The helper now exposed as a public SDK function lives in src/cli/wormhole.rs lines 196–240, but a byte-for-byte identical copy still lives in src/collect_rewards_lib.rs lines 33–77.

pub fn compute_merkle_positions(
	unsorted_siblings: &[[Hash256; 3]],
	leaf_hash: Hash256,
) -> (Vec<[Hash256; 3]>, Vec<u8>) {
	use qp_zk_circuits_common::zk_merkle::hash_node_presorted;
	// ...
}
fn compute_merkle_positions(
	unsorted_siblings: &[[Hash256; 3]],
	leaf_hash: Hash256,
) -> (Vec<[Hash256; 3]>, Vec<u8>) {
	use qp_zk_circuits_common::zk_merkle::hash_node_presorted;
	// ... identical body ...
}

This duplication is pre-existing, but since this PR is consciously promoting the wormhole copy to be the SDK helper, this is the right moment to delete the collect_rewards_lib.rs copy and have it call the now-public crate::cli::wormhole::compute_merkle_positions. Otherwise we ship a public SDK that contradicts a private internal copy.

2. DRY in the new examples — both examples reinvent the same scanner.

wait_for_native_transferred in wormhole_sdk_e2e.rs (lines 410–462) and find_recent_native_transferred in wormhole_sdk_usage.rs (lines 232–263) are ~80% the same code (block-range walk + chain_getBlockHash + events().at(...) + find::<NativeTransferred>). The "scan recent blocks for an event matching predicate X" is also repeated several times in the codebase (src/cli/block.rs:861, src/cli/storage.rs:173, src/chain/client.rs, src/cli/events.rs:27, src/collect_rewards_lib.rs:407,880).

Since the whole PR is about exposing an SDK, this is the natural moment to factor a single helper — e.g. pub async fn find_native_transferred_matching(client, predicate, max_lookback, timeout) in cli::wormhole — that both examples consume. That also makes a small but real piece of SDK functionality public: "find my deposit's inclusion event."

MINOR

3. Silent password fallback — clashes with the project's "fail early" rule.

	let password = password
		.or_else(|| env::var("QUANTUS_WALLET_PASSWORD").ok())
		.unwrap_or_default();

If neither --password nor $QUANTUS_WALLET_PASSWORD is set, this silently treats the password as the empty string. Fine for crystal_alice on --dev, but a one-line eprintln!("warning: no --password / $QUANTUS_WALLET_PASSWORD; trying empty password") would match the project's stated "no silent failures" rule.

4. Filename / doc-header mismatch.

examples/wormhole_sdk_usage.rs opens with:

//! End-to-end wormhole SDK example.

…but it's the read-only / no-submission demo. The actual end-to-end one is wormhole_sdk_e2e.rs. Consider re-titling this header to e.g. "Read-only wormhole SDK example (offline + on-chain reads, no submission)" to avoid future confusion.

5. Re-scan-by-one in wait_for_native_transferred.

		let lower = best_number.saturating_sub(INCLUSION_SCAN_BLOCKS);
		let scan_from = last_seen_best.max(lower);

		for n in scan_from..=best_number {
			// ...
		}
		last_seen_best = best_number;

Each loop iteration re-scans best_number because scan_from = last_seen_best and the inclusive range starts at the same block we ended on. Not a correctness bug, but last_seen_best = best_number + 1 would skip the redundant rescan.

6. assert_eq! for an expected runtime condition.

	assert_eq!(asset_id, NATIVE_ASSET_ID, "this example only handles native asset");

A non-native asset on a real chain would panic the example rather than return a useful error. return Err(QuantusError::Generic(...)) is more in line with the rest of the file.

7. Architectural nit: SDK re-exports point at cli::wormhole.

pub use cli::wormhole::{
	aggregate_proofs, at_best_block, compute_merkle_positions, decode_full_leaf_data,
	get_zk_merkle_proof, parse_transfer_events, read_proof_file,
	submit_unsigned_verify_aggregated_proof, verify_aggregated_and_get_events, write_proof_file,
	IncludedAt, TransferInfo,
};

This works, but as the SDK grows, it gets weird that core wormhole helpers live under cli::*. A future PR could move the on-chain helpers to src/wormhole_lib.rs (next to compute_wormhole_address, quantize_amount, etc.), with cli::wormhole reduced to argument parsing + presentation. Out of scope for this PR — flagging only.

Verdict

Overall the PR is in good shape: well-documented, builds clean, passes clippy, makes a coherent SDK surface, and ships a useful security bump. The blocking concern (per your DRY rule) is the duplicate compute_merkle_positions — that should be unified before merge. The example-side DRY (#2) and the silent-password warning (#3) are also worth a quick pass; the rest are nits.

If the duplicate-helper consolidation is in scope for this PR, the cleanest landing is:

  1. Delete the private compute_merkle_positions in src/collect_rewards_lib.rs and replace its call sites with crate::cli::wormhole::compute_merkle_positions.
  2. Extract a find_native_transferred_matching(...) helper into src/cli/wormhole.rs (or a new src/cli/wormhole/sdk.rs) and have both new examples call it.

If they belong in a follow-up PR, leave a TODO linking the issue and merge this. The CI/security bump alone is worth shipping.

Want me to push a follow-up commit (or open a stacked PR) with the DRY consolidation?

@n13
Copy link
Copy Markdown
Contributor

n13 commented Apr 30, 2026

The main DRY should be fixed, to keep the codebase sane

The examples don't matter IMO

The rest also doesn't matter. So just one DRY thing I would clean up.

@czareko
Copy link
Copy Markdown
Contributor Author

czareko commented Apr 30, 2026

@n13, duplicate fixed.

@czareko czareko merged commit 8136e13 into main Apr 30, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants