Skip to content

✨ (signer-zcash) [LIVE-25878]: Implement transparent signTransaction with Ledger Wallet legacy args#1498

Open
may01 wants to merge 7 commits into
developfrom
feat/live-25878-zcash-signer-implementation
Open

✨ (signer-zcash) [LIVE-25878]: Implement transparent signTransaction with Ledger Wallet legacy args#1498
may01 wants to merge 7 commits into
developfrom
feat/live-25878-zcash-signer-implementation

Conversation

@may01
Copy link
Copy Markdown
Contributor

@may01 may01 commented May 18, 2026

📝 Description

Implements transparent Zcash payment signing in @ledgerhq/device-signer-kit-zcash by replacing the placeholder signTransaction flow with a full Ledger Wallet–compatible pipeline.
Problem: signTransaction previously accepted a raw derivationPath + Uint8Array and did not perform the multi-step Zcash signing protocol (trusted inputs, untrusted hash, Sapling commit, signature assembly).
Solution:

  • Introduce LegacyCreateTransactionArg / LegacyTransaction (aligned with Ledger Wallet createPaymentTransaction and splitTransaction).
  • Orchestrate signing in SignTransactionTask: trusted-input collection per UTXO, change-path handling, untrusted hash input, per-input signing, optional Sapling output commit, and serialized signed transaction output.
  • Add APDU commands: StartUntrustedHashTransactionInputCommand, HashOutputFullCommand, ProvideOutputFullChangePathCommand, ZcashSaplingOutputCommitCommand, plus updates to SignTransactionCommand.
  • Add legacyTransactionUtils for v5 transaction building, script parsing, and wire serialization.
  • Extend GetTrustedInputTask for previous-tx overrides when Sapling/Orchard payloads are not represented in transparent outputs.
  • Add Vitest coverage (command/unit tests + SignTransactionTask integration tests driven by Ledger Wallet APDU log fixtures from 2026-05-11 / 2026-05-12).
  • Document the API in the package README and root README; extend the sample app Zcash signer UI to exercise the new flow.
    API change (minor): signTransaction now takes (args: LegacyCreateTransactionArg, options?) instead of (derivationPath, transaction).

❓ Context

  • JIRA or GitHub link: LIVE-25878
  • Feature: Transparent Zcash transaction signing for Device SDK TS, parity with Ledger Wallet legacy payment APIs.

✅ Checklist

Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.

  • Covered by automatic tests
  • Changeset is provided
  • Documentation is up-to-date
  • Impact of the changes:
  • Published package: @ledgerhq/device-signer-kit-zcash — new signTransaction behavior and exported legacy transaction types.
  • Sample app: Zcash signer page / SignerZcashView — updated to build and submit LegacyCreateTransactionArg.
  • QA focus: Transparent payments with/without change, multi-input flows, additionals: ["zcash"] vs ["zcash", "sapling"], previous txs with Sapling via serializedPreviousTransactionOverride, device approval UX, and comparison against Ledger Wallet on the same fixtures.
  • Out of scope: Shielded-only / Orchard-only flows not covered by transparent outputs without override bytes.

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
device-sdk-ts-sample Ready Ready Preview, Comment May 25, 2026 1:35pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
doc-device-management-kit Ignored Ignored May 25, 2026 1:35pm

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

Danger Check Results

Messages

Danger: All checks passed successfully! 🎉

Generated by 🚫 dangerJS against b7c267c

@may01 may01 marked this pull request as ready for review May 19, 2026 13:23
@may01 may01 requested a review from a team as a code owner May 19, 2026 13:23
Copilot AI review requested due to automatic review settings May 19, 2026 13:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements a full Ledger Wallet–compatible transparent Zcash signTransaction flow in @ledgerhq/device-signer-kit-zcash, replacing the previous placeholder API with a multi-step APDU pipeline (trusted inputs → hash inputs/outputs → optional Sapling commit → per-input signatures → signed tx assembly) and updating the public API accordingly.

Changes:

  • Replace signTransaction(derivationPath, rawTx) with signTransaction(args: LegacyCreateTransactionArg, options?) and thread the new shape through signer/use-cases/app-binder.
  • Add/extend Zcash APDU commands + SignTransactionTask orchestration, plus transaction parsing/serialization utilities and trusted-input chunking updates.
  • Add Vitest coverage (unit + integration-style fixtures), update docs, and expand the sample app UI to craft legacy-compatible signing args.

Reviewed changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
README.md Adds Zcash signer package to the root package list.
packages/signer/signer-zcash/src/internal/use-cases/transaction/SignTransactionUseCase.ts Updates use-case signature to accept LegacyCreateTransactionArg.
packages/signer/signer-zcash/src/internal/use-cases/transaction/SignTransactionUseCase.test.ts Adds test coverage for updated use-case wiring.
packages/signer/signer-zcash/src/internal/DefaultSignerZcash.ts Updates public signer method signature and delegation to use-case.
packages/signer/signer-zcash/src/internal/DefaultSignerZcash.test.ts Adds a basic expectation for the new signTransaction invocation path.
packages/signer/signer-zcash/src/internal/app-binder/ZcashAppBinder.ts Updates binder API to accept transactionArg.
packages/signer/signer-zcash/src/internal/app-binder/ZcashAppBinder.test.ts Adds binder tests for signTransaction and task wiring.
packages/signer/signer-zcash/src/internal/app-binder/task/utils/legacyTransactionUtils.ts Adds legacy tx parsing/serialization helpers used by signing.
packages/signer/signer-zcash/src/internal/app-binder/task/utils/legacyTransactionUtils.test.ts Adds unit tests for transaction util helpers.
packages/signer/signer-zcash/src/internal/app-binder/task/SignTransactionTask.ts Implements the transparent Zcash signing orchestration task.
packages/signer/signer-zcash/src/internal/app-binder/task/SignTransactionTask.test.ts Adds extensive fixture-driven tests for signing task behavior.
packages/signer/signer-zcash/src/internal/app-binder/task/GetTrustedInputTask.ts Adjusts trusted-input chunk splitting (notably scriptSig chunking).
packages/signer/signer-zcash/src/internal/app-binder/task/GetTrustedInputTask.test.ts Updates expected APDUs to match the new chunking.
packages/signer/signer-zcash/src/internal/app-binder/task/fixtures/signTransactionFromLedgerWalletLogs2026-05-12.ts Adds Ledger Wallet log fixture and expectations for a 2-input Sapling case.
packages/signer/signer-zcash/src/internal/app-binder/task/fixtures/signTransactionFromLedgerWalletLogs2026-05-11.ts Adds Ledger Wallet log fixture and expectations for a 1-input Sapling case.
packages/signer/signer-zcash/src/internal/app-binder/command/ZcashSaplingOutputCommitCommand.ts Adds Sapling output-commit command (short SIGN).
packages/signer/signer-zcash/src/internal/app-binder/command/ZcashSaplingOutputCommitCommand.test.ts Adds APDU-shape test for Sapling output commit.
packages/signer/signer-zcash/src/internal/app-binder/command/utils/apduHeaderUtils.ts Adds Zcash INS/P1/P2 constants needed for new commands.
packages/signer/signer-zcash/src/internal/app-binder/command/StartUntrustedHashTransactionInputCommand.ts Adds command for START_UNTRUSTED_HASH_TRANSACTION_INPUT.
packages/signer/signer-zcash/src/internal/app-binder/command/StartUntrustedHashTransactionInputCommand.test.ts Adds unit tests for the new hash-input command.
packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.ts Implements SIGN_TRANSACTION APDU creation and response parsing.
packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.test.ts Adds APDU-shape tests and signature normalization tests.
packages/signer/signer-zcash/src/internal/app-binder/command/ProvideOutputFullChangePathCommand.ts Adds finalize-input command for change-path provisioning.
packages/signer/signer-zcash/src/internal/app-binder/command/HashOutputFullCommand.ts Adds finalize-input command for hashing outputs in chunks.
packages/signer/signer-zcash/src/internal/app-binder/command/FinalizeInputCommands.test.ts Adds unit tests for finalize-input commands.
packages/signer/signer-zcash/src/api/SignerZcash.ts Updates the public interface method signature for signTransaction.
packages/signer/signer-zcash/src/api/model/TransactionOptions.ts Cleans up transaction options type (removes placeholder comment).
packages/signer/signer-zcash/src/api/model/CreateTransactionArg.ts Introduces legacy-compatible transaction arg types.
packages/signer/signer-zcash/src/api/index.ts Re-exports new legacy transaction types and return type.
packages/signer/signer-zcash/src/api/app-binder/SignTransactionDeviceActionTypes.ts Changes signTransaction output type to HexaString.
packages/signer/signer-zcash/README.md Documents the new signing API and provides detailed usage guidance.
apps/sample/src/components/SignerZcashView/index.tsx Updates sample app UI to build LegacyCreateTransactionArg (up to 3 inputs).
apps/sample/src/components/SessionIdWrapper.tsx Loosens component type to React.ComponentType for dynamic import use.
apps/sample/src/components/ResizableTextArea.tsx Adds disabled state styling/behavior to the resizable text area.
apps/sample/src/app/signers/zcash/page.tsx Disables SSR for Zcash signer view via dynamic import.
.changeset/warm-rivers-build.md Adds a minor changeset for the Zcash signer API + feature work.
Comments suppressed due to low confidence (4)

packages/signer/signer-zcash/src/internal/app-binder/task/GetTrustedInputTask.ts:350

  • splitTransactionToTrustedInputChunks no longer verifies that parsing consumed the full input transaction (the previous offset !== transaction.length guard was removed). Without that check, extra trailing bytes in transaction can be silently ignored and still sent to the device as a “valid” trusted input. Consider reinstating a strict consumption check (or explicitly documenting why trailing bytes are acceptable).
  if (isTxV4) {
    chunks.push(transaction.slice(offset));
    offset = transaction.length;
  } else {
    chunks.push(splitV5ExtraData(locktime, expiry));
  }

  return splitForApduData(chunks);

packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.ts:72

  • SignTransactionCommand.getApdu() hard-codes ins: 0x48 instead of using the shared INS.SIGN_TRANSACTION constant from command/utils/apduHeaderUtils. Other commands consistently use INS.* (e.g., ProvideOutputFullChangePathCommand.ts:55 uses INS.FINALIZE_INPUT), so this should follow the same pattern for consistency and easier future refactors.
    const apduArgs: ApduBuilderArgs = {
      cla: ZCASH_CLA,
      ins: 0x48,
      p2: P2.DEFAULT,
      p1: 0x00,
    };

packages/signer/signer-zcash/src/internal/app-binder/task/SignTransactionTask.ts:114

  • outputScriptHex is parsed via Buffer.from(outputScriptHex, "hex"), which does not reliably throw on malformed/odd-length hex and can produce truncated or unexpected bytes. Since this is user-provided API input, consider validating that outputScriptHex is a non-empty, even-length hex string (and erroring early) before sending APDUs.
      timestamp: Buffer.alloc(0),
    };
    const outputScript = Buffer.from(outputScriptHex, "hex");

packages/signer/signer-zcash/src/internal/app-binder/command/SignTransactionCommand.ts:104

  • parseResponse() force-sets signature[0] = 0x30 for any non-empty response. If the goal is to normalize the known Ledger Wallet log quirk where signatures start with 0x31, it would be safer to only rewrite when the first byte is actually 0x31 (so unexpected/non-DER payloads aren’t silently mutated).
    ).orDefaultLazy(() => {
      const signature = new Uint8Array(apduResponse.data);
      if (signature.length > 0) {
        signature[0] = 0x30;
      }
      return CommandResultFactory({

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/signer/signer-zcash/src/internal/app-binder/ZcashAppBinder.test.ts Outdated
Comment thread packages/signer/signer-zcash/src/api/model/CreateTransactionArg.ts Outdated
Comment thread packages/signer/signer-zcash/src/internal/app-binder/ZcashAppBinder.test.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment isn't doing anything anymore.

@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
20.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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