Clear-sign classic transactions on Ledger

[fnando]

What

Route every classic TransactionEnvelope::Tx and TransactionEnvelope::TxFeeBump signed by a Ledger identity through the clear-sign APDU SIGN_TX (INS=0x04) instead of SIGN_TX_HASH (INS=0x08). The full TransactionSignaturePayload XDR is sent to the device so the Ledger Stellar app parses and displays each operation — destination, amount, asset, fee, memo, source account, sequence — for the user to verify before approving.

Smart contract transactions (InvokeHostFunction, ExtendFootprintTtl, RestoreFootprint) continue to use SIGN_TX_HASH because the Ledger app cannot pretty-print these ops yet. The CLI now prints the transaction hash alongside the approval prompt so users can compare it against what the device shows. Smart contract authorization-entry preimage signing (sign_payload) is unchanged.

Why

Classic transactions. Today, signing with a Ledger identity in stellar-cli shows only a hex hash on the device. Users have to enable Hash Signing (meant for advanced/experimental flows) and trust the host. After this change, the everyday flow walks them through each operation on the trusted device screen — the experience hardware-wallet users expect.

Smart contract transactions. The Ledger app cannot render these ops yet, so they still need Hash Signing. Surfacing the host-computed hash in the CLI prompt gives users something concrete to compare against the device screen.

The clear-sign primitive LedgerSigner::sign_transaction (INS=0x04) was already implemented and tested against Speculos but had no production callers — this PR wires it up and adds a sibling sign_fee_bump_transaction so fee-bumps go through the same path.

Known limitations

Smart contract transactions still require Hash Signing to be enabled on the device and only show a hex hash on the screen. This is a device-side gap that will close when the Ledger Stellar app gains smart contract pretty-printing. Smart contract authorization-entry preimages signed via sign_payload are inherently 32-byte hashes, so they remain on the blind-sign APDU.

[claude]

Claude finished @fnando's task in 4m 8s —— View job

---

PR Review — Clear-sign classic transactions on Ledger

Nice, focused change. The core flip (route classic envelopes through SIGN_TX 0x04 with the full TransactionSignaturePayload, keep Soroban ops on SIGN_TX_HASH 0x08) is correctly implemented and the rationale comments are excellent. The new sign_fee_bump_transaction symmetrically uses the existing chunking/APDU machinery via the extracted sign_tagged_transaction, and LedgerEntry::sign_tx_env cleanly replaces the old hash-only path. Below are the items I'd consider before merging — all minor.

Findings

Inline comments posted on:

• cmd/soroban-cli/src/signer/ledger.rs:10-19 — is_soroban_tx / is_soroban_tx_env are over-exposed (pub); only used inside this module. Suggest pub(super) or private.
• cmd/soroban-cli/src/signer/ledger.rs:21-30 — Irrefutable single-variant let for FeeBumpTransactionInnerTx is fragile if the XDR enum grows. Suggest exhaustive match to force a deliberate decision on schema evolution.
• cmd/soroban-cli/src/signer/ledger.rs:116 — Fee-bump UX inconsistency: classic Ledger path always prints "Approve the transaction…" even for TxFeeBump, while format_signing_message in mod.rs distinguishes "fee bump transaction" for the Local/SecureStore arms.
• cmd/soroban-cli/src/signer/ledger.rs:106-133 — Redundant Sha256 of network passphrase: transaction_env_hash computes it internally and the classic branch recomputes it as network_id. Cosmetic.
• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Speculos click counts hard-couple to one tx fixture. Comment acknowledges it; flagging the longer-term maintenance risk.
• cmd/crates/stellar-ledger/src/lib.rs:170 — #[allow(clippy::missing_panics_doc)] on sign_fee_bump_transaction without a visible panic site. Likely inherited from the pre-refactor sign_transaction; worth verifying it's still needed.

Things I checked and did NOT find issues with

• Soroban detection through fee bumps — is_soroban_tx_env correctly recurses into FeeBumpTransactionInnerTx::Tx(inner), so a fee-bump wrapping a Soroban inner correctly takes the blind-sign path.
• TxV0 handling — is_soroban_tx_env returns false for TxV0, which then falls into the classic-path match and returns Error::UnsupportedTransactionEnvelopeType. Correct.
• sign_payload (Soroban auth-entry preimage) — Unchanged, still goes through sign_transaction_hash. Matches the PR description's intent.
• Signature-length safety — Signature(signature_bytes.try_into()?) returns TryFromSliceError for wrong-length device responses instead of panicking. Good.
• APDU layout — sign_tagged_transaction correctly prepends HD_PATH_ELEMENTS_COUNT + path bytes before the serialized TransactionSignaturePayload, chunked at CHUNK_SIZE, with P1_SIGN_TX_FIRST/NOT_FIRST and P2_SIGN_TX_MORE/LAST flags. Mirrors the documented Ledger Stellar app protocol.
• HID transport lifecycle — Comment confirms transport is opened lazily per sign call so the device stays free between operations and can't collide with a concurrent transport. Good.
• No security concerns — The change moves less trust onto the host (device now shows operation details) and reduces dependence on Hash Signing mode being enabled. Net security win.

Todo

• Set up review and load context
• Read cmd/crates/stellar-ledger/src/lib.rs
• Read cmd/soroban-cli/src/signer/ledger.rs
• Read cmd/soroban-cli/src/signer/mod.rs
• Read cmd/crates/soroban-test/tests/it/emulator.rs
• Verify Soroban detection covers fee-bump wrapping
• Verify TxV0 error path
• Spot-check APDU layout against Ledger app spec
• Post inline findings + summary

Overall: ship-ready after the visibility / exhaustive-match nits. The fee-bump label, redundant SHA-256, and click-count coupling are polish items that can land in a follow-up if you prefer to keep this PR tight.
• branch clear-sign-classic-tx

[Copilot]

Pull request overview

Routes Ledger signing for classic Stellar transactions through the Ledger app’s clear-sign APDU (SIGN_TX, INS=0x04) so users can review operation details on-device, while keeping Soroban-related transactions on hash-signing (SIGN_TX_HASH, INS=0x08) and surfacing the hash in the CLI prompt.

Changes:

• Refactors Signer to produce a signature via a single path, and routes Ledger signing through sign_tx_env (clear-sign vs hash-sign based on envelope content).
• Adds fee-bump clear-sign support to the stellar-ledger crate (sign_fee_bump_transaction) and wires it into CLI Ledger signing.
• Updates Speculos integration test to exercise classic clear-signing end-to-end (Payment + memo) using the correct approval click-flow.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
cmd/soroban-cli/src/signer/mod.rs	Refactors signature production and adds local helper(s) for envelope hashing + signing messages.
cmd/soroban-cli/src/signer/ledger.rs	Adds Soroban-envelope detection and implements Ledger sign_tx_env (clear-sign for classic; hash-sign for Soroban ops).
cmd/crates/stellar-ledger/src/lib.rs	Adds sign_fee_bump_transaction and factors signing into a shared tagged-transaction helper.
cmd/crates/soroban-test/tests/it/emulator.rs	Updates Speculos integration test to validate clear-sign classic transaction flow with Ledger identity.

[claude]

Claude finished @fnando's task in 3m 16s —— View job

---

PR Review — Clear-sign classic transactions on Ledger (re-review after fixes)

Re-reviewed against the five follow-up commits since the initial review. Most prior items were addressed cleanly; a couple of polish items are still open.

What was addressed

Prior item	Commit	Verdict
is_soroban_tx / is_soroban_tx_env over-exposed (pub)	669f68a	✅ Narrowed to pub(super) and gated on cfg(feature = "additional-libs"). Clean.
Speculos hard-coded click counts coupled to one tx shape	5cd96c0	✅ Replaced with advance_to_approve_and_confirm that polls for the "Approve" event text, capped at 50 right-clicks. Far more robust to fixture changes.
Stale #[allow(clippy::missing_panics_doc)] on ledger signers	87697b1	✅ Verified — sign_tagged_transaction has no panic sites (every fallible call uses ?, no unwrap/expect/indexing/unreachable!); the allow was indeed stale.
No emulator/unit coverage for sign_fee_bump_transaction	0a7eb63	✅ New test_sign_fee_bump_tx in cmd/crates/stellar-ledger/src/lib.rs:492-589 builds the expected APDU chunks by replicating sign_tagged_transaction's framing, mocks both chunks, and verifies the response. Note: this is the unit-level (mock HTTP) version, not a full Speculos run — fine for the APDU correctness check; an end-to-end fee-bump Speculos test could still be a follow-up.
Redundant SHA-256 / duplicated tx_env_hash helper in dispatch	4fbd74c	✅ in mod.rs: Local/SecureStore arms now both use transaction_env_hash + format_signing_message. ⚠️ The same simplification was not applied to signer/ledger.rs::sign_tx_env, where the Soroban branch calls transaction_env_hash (which hashes the passphrase internally) and the classic branch re-derives network_id from a separate Sha256::digest. Functionally fine — only one branch runs per call — but the two paths still express the same input two different ways.

Still open from the prior review

• Fee-bump UX label inconsistency (signer/ledger.rs:120) — format_signing_message was extracted in mod.rs and now distinguishes "fee bump transaction" vs "transaction" for the Local/SecureStore arms, but the Ledger arm still hardcodes "Approve the transaction on your Ledger device…" for both. After the partial fix, the divergence is now sharper than before — a fee-bump signed with a local key prints the right label, but the same envelope on a Ledger does not. Re-flagged inline.
• Irrefutable single-variant let for FeeBumpTransactionInnerTx (signer/ledger.rs:29) — Still compiles today because the XDR enum has one variant; an exhaustive match would force a deliberate decision if a future XDR upgrade adds another variant. Cosmetic/forward-looking; author may have decided to defer.

New things I checked in this round

• advance_to_approve_and_confirm (util.rs:209-220) — Polls until an event with exact text "Approve" is observed, then clicks both buttons. Assumes the Stellar app's final review screen always emits an "Approve" button label on all three device models (nanos/nanox/nanosp). Reasonable given the app's current UI; if the label ever changes (e.g. "Accept", "Confirm"), the test fails loudly with the 50-click panic — much better failure mode than the previous silent hang. Worth keeping in mind if the Ledger Stellar app UI is ever localized or restyled.
• approve_tx_hash_signature vs approve_tx_signature are now identical bodies. Keeping both names documents intent at call sites (auth-entry vs classic flows), so I'd leave them — just noting they could collapse if you prefer.
• test_sign_fee_bump_tx — Hardcodes the index-0 HD path bytes (m/44'/148'/0') when building the expected APDU. Matches the style of the existing test_sign_tx and is appropriate for an exact-bytes mock. Not a concern.
• produce_signature arm order — Ledger, Lab, Local, SecureStore doesn't follow the enum's declaration order (Local, Ledger, Lab, SecureStore). Stylistic — match arms don't need to follow declaration order, but matching it can make exhaustiveness changes easier to spot. Not blocking.
• No new security concerns. Same conclusion as the first round: more trust on the device, less reliance on Hash Signing mode. Net win.

Verdict

Ship-ready. The two remaining items are minor polish — the fee-bump label inconsistency is the only one I'd nudge on since it's a now-visible divergence after the helper was extracted. Everything else can live in a follow-up.

Todo

• Read updated cmd/soroban-cli/src/signer/ledger.rs
• Read updated cmd/soroban-cli/src/signer/mod.rs
• Read updated cmd/crates/stellar-ledger/src/lib.rs
• Read updated cmd/crates/soroban-test/tests/it/emulator.rs
• Read updated cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Verify each prior feedback item was addressed correctly
• Look for new issues introduced by the follow-up commits
• Post review summary
  • branch clear-sign-classic-tx