feat(wasm-utxo): expose v6 (Ironwood) PSBT flow via wasm + TS - #343
Conversation
| return ZcashTransaction.fromWasm(this.wasm.extract_zcash_transaction(maxFeeRate)); | ||
| } | ||
|
|
||
| // --- Zcash v6 (Ironwood / NU6.3) shielding --- |
There was a problem hiding this comment.
maybe we want to create a separate ZcashIronwoodBitGoPsbt.ts instead?
There was a problem hiding this comment.
+1 — I'd do this in two steps:
Now (this PR): TS-level split. New ZcashIronwoodBitGoPsbt.ts extends ZcashBitGoPsbt, move the six v6 methods (addIronwoodOutput / ironwoodTxid / ironwoodTransparentSighash / addIronwoodSignature / combineIronwoodProof) onto it, createIronwood returns it, and fromBytes detects the ZecV6Params marker and returns the subclass. This gives the type-safety win with no wasm change — the underlying wasm object is the same flat BitGoPsbt. The one thing to get right is the fromBytes polymorphism (deserialize needs to return the subclass when the v6 marker is present).
Follow-up: wasm-level split. A genuinely separate WasmZcashIronwoodPsbt struct. Cheaper than I first thought — the generic PSBT ops come free via impl_wasm_psbt_ops!(WasmZcashIronwoodPsbt, psbt) (src/wasm/psbt.rs:782); only the wallet-construction surface (add_wallet_input/add_wallet_output ± _at_index, serialize) is hand-written on BitGoPsbt today and would need delegation — or a second macro arm to generate it. That step removes the zcash()/zcash_mut() runtime guards in favor of compile-time separation, so it's worth doing, just not blocking this PR.
Doing the TS split first keeps the public API shape the follow-up will want (a distinct Ironwood type) without holding up the stack.
Generated by Claude Code
OttoAllmendinger
left a comment
OttoAllmendinger
left a comment
There was a problem hiding this comment.
Review
Overview
Surfaces the v6 flow through wasm-bindgen and the ZcashBitGoPsbt TS wrapper (createIronwood, addIronwoodOutput, ironwoodTxid, ironwoodTransparentSighash, addIronwoodSignature, combineIronwoodProof), plus v6-aware serialize/deserialize that auto-detects a v6 PSBT by the ZecV6Params proprietary key. Convention compliance is good: Uint8Array throughout (recipient/anchor/memo/ovk/sig/txid/proof), bigint for amount, number for non-monetary blockHeight/index/lockTime/expiryHeight, camelCase names, and length-validation at the wasm boundary.
Type separation (ZcashIronwoodBitGoPsbt)
Discussed in the ZcashBitGoPsbt.ts thread — agreed plan is a TS-level split in this PR (subclass returned by createIronwood, with fromBytes returning it when the ZecV6Params marker is present), and a wasm-level split as a follow-up (separate WasmZcashIronwoodPsbt, generic ops via impl_wasm_psbt_ops!, dropping the zcash()/zcash_mut() runtime guards). The TS split is the priority here because it gives compile-time separation — right now the six v6 methods live on ZcashBitGoPsbt and only work when the PSBT is actually v6, failing at runtime otherwise.
Issues
1. (API consistency) ironwoodTxid() returns bytes in display (reverse) order.
It returns a reversed Uint8Array — neither the internal-order txid nor a display hex string, an easy-to-misuse middle ground; the rest of the codebase exposes ids as getId(): string. Consider returning a hex string (display) to match getId, or internal-order bytes and letting the caller reverse+encode. Whatever you pick, document the order — a bare Uint8Array gives no hint it's reversed.
2. (Minor) combine_ironwood_proof clones the whole PSBT and leaves the JS object usable afterward.
The Rust combine_ironwood_proof consumes self, so the wasm binding takes &self and self.psbt.clone()s (PCZT + proof). Functionally fine, but the JS object stays "live" after combine, so a second combineIronwoodProof silently re-runs on stale state. Consider documenting it as terminal, or invalidating on the JS side.
Test coverage
Good smoke coverage (vgid, 32-byte txid/sighash + serialize round-trip, bad-sig reject, bad-recipient-length reject). One gap worth a line: combineIronwoodProof and the happy path of addIronwoodSignature are never exercised from TS — that wasm-bindgen glue is only covered by the native #342 test. A minimal TS combine (even against a placeholder proof) would exercise the binding end-to-end.
Note on the #338 finding (unshield digest)
This top layer is shield-only — addIronwoodOutput adds the shielded output and the transparent side is always the funding inputs; there's no shielded-input → transparent-output path exposed anywhere in the stack. So compute_v6_sig_digest's inputs.is_empty() fallback (my #338 finding #1) is unreachable in the current product surface — every real v6 tx here has ≥1 transparent input. I'd downgrade that finding to "latent, not currently reachable" — still worth fixing for API honesty, but not a shipping blocker.
Praise
Clean length-validation at the wasm boundary (recipient 43 / anchor 32 / memo 512 / ovk 32), the well-documented getrandom shim scoped to the mocha/Node-ESM harness (with a clear note production builds don't need it), and the plain-PSBT auto-detection via the marker key that keeps serialize/fromBytes transparent to callers.
Generated by Claude Code
f151c56 to
178d76a
Compare
| let recipient: [u8; 43] = recipient | ||
| .try_into() | ||
| .map_err(|_| WasmUtxoError::new("recipient must be 43 bytes"))?; | ||
| let anchor: [u8; 32] = anchor | ||
| .try_into() | ||
| .map_err(|_| WasmUtxoError::new("anchor must be 32 bytes"))?; | ||
| let memo: [u8; 512] = memo | ||
| .try_into() | ||
| .map_err(|_| WasmUtxoError::new("memo must be 512 bytes"))?; | ||
| let ovk: Option<[u8; 32]> = match ovk { |
There was a problem hiding this comment.
nit: should we store these lens in a descriptive variables?
There was a problem hiding this comment.
these are not runtime variables, but arguably we could extract a few type aliases
OttoAllmendinger
left a comment
OttoAllmendinger
left a comment
There was a problem hiding this comment.
following up with more notes, to be continued
| * @param walletKeys - The wallet's root keys (sets global xpubs in the PSBT) | ||
| * @param options - Options including blockHeight (at/after NU6.3 activation) | ||
| */ | ||
| static createIronwood( |
There was a problem hiding this comment.
shouldn't this be ZcashIronwoodBitGoPsbt.create?
There was a problem hiding this comment.
Agree — create. Worth applying the same de-prefixing across the whole surface (see the txid / addOutput threads): once the class name carries "Ironwood", every member repeating it is redundant.
Generated by Claude Code
| * transparent inputs/outputs and the Ironwood output are in place; unchanged by signing or | ||
| * proving. | ||
| */ | ||
| ironwoodTxid(): string { |
There was a problem hiding this comment.
same - shoudn't this just the standard txid()?
There was a problem hiding this comment.
One correction on the target name: the codebase convention is getId(), not txid(). ITransaction and transactionBase both expose the display-order id as getId(): string, and the sibling ZcashV6Transaction already returns this exact value as getId() (src/wasm/zcash.rs:126, #[wasm_bindgen(js_name = getId)]). So the consistent rename is ironwoodTxid() → getId(). No collision — the base PSBT classes define no getId.
Generated by Claude Code
There was a problem hiding this comment.
Changed it to getId()
| * @param options.memo - optional 512-byte memo (defaults to the ZIP-302 "no memo" encoding) | ||
| * @param options.ovk - optional 32-byte outgoing viewing key (omit for a keyless build) | ||
| */ | ||
| addIronwoodOutput( |
There was a problem hiding this comment.
we don't need to repeat Ironwood in the method name if we already have it in the classname
addShieldedOutput maybe if we want to keep the option for transparent outputs?
There was a problem hiding this comment.
+1 on addShieldedOutput — it also reads well against the base's addWalletOutput (transparent), making the shielded/transparent distinction explicit. Same de-prefixing for the rest of the surface: transparentSighash, addTransparentSignature, combineProof.
Generated by Claude Code
| let recipient: [u8; 43] = recipient | ||
| .try_into() | ||
| .map_err(|_| WasmUtxoError::new("recipient must be 43 bytes"))?; | ||
| let anchor: [u8; 32] = anchor | ||
| .try_into() | ||
| .map_err(|_| WasmUtxoError::new("anchor must be 32 bytes"))?; | ||
| let memo: [u8; 512] = memo | ||
| .try_into() | ||
| .map_err(|_| WasmUtxoError::new("memo must be 512 bytes"))?; | ||
| let ovk: Option<[u8; 32]> = match ovk { |
There was a problem hiding this comment.
these are not runtime variables, but arguably we could extract a few type aliases
| * Mocha setup: make CSPRNG bytes available to the bundler-target wasm under Node ESM. | ||
| * | ||
| * The bundler-target build sources randomness through getrandom, which — on detecting Node via | ||
| * `process` — calls `module.require("crypto")`. `module` is undefined under Node ESM (this test | ||
| * harness), so provide a working `require`. Production consumers (real bundlers, or the | ||
| * nodejs-target build the microservice uses) don't need this. | ||
| * | ||
| * This lives in a `--require`d setup file rather than at the top of a spec because ESM hoists | ||
| * `import` declarations above every statement in a module body: a shim written inline in a spec runs | ||
| * only *after* the wasm module it is meant to support has been imported and evaluated. It happens to | ||
| * work there because getrandom resolves its backend lazily on first use, but that is a fragile thing | ||
| * to depend on. Loading it here runs it before any spec module is evaluated. | ||
| * | ||
| * Defined as a **self-removing** accessor rather than a plain global. A global `module` is how | ||
| * libraries detect CommonJS (`typeof module !== "undefined"`), and leaving one in place for the whole | ||
| * run would tell that lie to every dependency in the suite. getrandom reads `module.require` exactly | ||
| * once and caches the resulting `crypto` object, so deleting the property on first read narrows the | ||
| * window to that single access. If something else consumes it first, getrandom then fails loudly | ||
| * rather than any test silently changing behavior. | ||
| * | ||
| * The proper fix is a custom getrandom backend in Rust that goes straight to | ||
| * `globalThis.crypto.getRandomValues`, removing the Node-detection path — then this file can go. |
There was a problem hiding this comment.
wait what? there must be a better way. how does it work for our BTC randomness needs?
There was a problem hiding this comment.
BTC musig2 already answers this. generate_musig2_nonces with no session_id auto-generates one via getrandom::getrandom() (mod.rs:1253) — the same getrandom 0.2.16 backend the orchard builder's OsRng uses (the only other copy, 0.3.4, is native-only via tempfile). And test/fixedScript/musig2Nonces.ts exercises exactly that path on master — assert.doesNotThrow(() => generateMusig2Nonces(userKey)) — with no require shim in .mocharc. So wasm CSPRNG already works in this tsx/ESM harness.
That makes the shim's premise suspect: if musig2's auto-gen path sources randomness fine here, the Ironwood path (OsRng → same getrandom) should too. Before landing the shim, worth confirming the Ironwood test actually fails without it — and if it's the same getrandom path, dropping the shim is the better outcome. If there is a real difference (e.g. rand's OsRng reaching the glue differently than a bare getrandom() call), a one-line note would justify keeping it.
Generated by Claude Code
There was a problem hiding this comment.
Removed the shim but the Ironwood test does fail without the shim on Node 18 and so does musig2Nonces.ts
There was a problem hiding this comment.
We don't use node18 anywhere, I initially thought we used it in bitgojs ci tests, but we only test on node20 and above, so removing the shim should not be a probelm.
178d76a to
8d95cd4
Compare
8d95cd4 to
89d9d27
Compare
4 participants
Surface the dedicated ZcashBitGoPsbt v6 methods through the wasm bindings and the TypeScript ZcashBitGoPsbt wrapper.
wasm (src/wasm/fixed_script_wallet): add create_empty_zcash_v6_at_height, add_ironwood_output, ironwood_v6_txid, ironwood_v6_transparent_sighash, add_ironwood_v6_signature, and combine_ironwood_proof, plus private zcash()/zcash_mut() helpers to reach the Zcash arm. Byte-length validation for recipient (43) / anchor (32) / memo (512) / ovk (32) happens at the boundary; randomness uses OsRng (getrandom js).
zcash_psbt: make the internal serialize()/deserialize() v6-aware so the standard from_bytes/serialize path round-trips a v6 PSBT (plain PSBT carrying the transparent skeleton + PCZT + ZecV6Params) without the v4 tx-replacement dance.
TS (ZcashBitGoPsbt.ts): add createIronwood, addIronwoodOutput, ironwoodTxid, ironwoodTransparentSighash, addIronwoodSignature, and combineIronwoodProof.
Test (test/fixedScript/zcashIronwoodPsbt.ts): drive the bindings from TypeScript — build a shield PSBT, assert the Ironwood version group id, compute a 32-byte v6 txid + per-input sighash that survive a serialize round-trip, and check the signature/recipient validation error paths. (Full sign→combine runtime behavior is covered by the native Rust test in PR4.) A small getrandom shim lets the bundler-target wasm source randomness under the Node/mocha harness; production bundler/browser and nodejs-target builds don't need it.