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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →

[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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →

[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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →

[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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →

[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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →

[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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →

[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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →

[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

[claude]

Claude finished @fnando's task in 2m 28s —— View job

---

PR Re-Review — Clear-sign classic transactions on Ledger (round 3)

Re-reviewed against the current branch tip (cc73055). The implementation looks ship-ready. I found one stale comment from the polling refactor; everything else from the prior rounds is either fixed or has been explicitly declined by the author. Inline comment posted.

What I checked this round

Area	Status
is_soroban_tx / is_soroban_tx_env visibility	✅ pub(super) and gated on additional-libs (cmd/soroban-cli/src/signer/ledger.rs:12-34).
Stale #[allow(clippy::missing_panics_doc)] removed	✅ Confirmed gone from both sign_transaction and sign_fee_bump_transaction (cmd/crates/stellar-ledger/src/lib.rs:141-181). No unwrap/expect/indexing/unreachable! in sign_tagged_transaction.
Polling replaces hard-coded click counts	✅ advance_to_approve_and_confirm polls for "Approve" text, capped at 50 clicks (cmd/crates/stellar-ledger/src/emulator_test_support/util.rs:209-220). Both approve_tx_signature and approve_tx_hash_signature delegate to it.
Fee-bump APDU unit test	✅ test_sign_fee_bump_tx (cmd/crates/stellar-ledger/src/lib.rs:492-589) builds the expected chunked SIGN_TX bytes from the same framing logic and asserts both chunks plus the response. Has an assert_eq!(chunks.len(), 2, ...) guard so the test fails loudly if the fixture changes.
transaction_env_hash reuse in mod.rs	✅ Local and SecureStore arms both use it (cmd/soroban-cli/src/signer/mod.rs:376-385). TxV0 guard now lives once in Signer::sign_tx_env (line 318).
Soroban detection through fee-bump	✅ is_soroban_tx_env recurses into FeeBumpTransactionInnerTx::Tx(inner).
TxV0 handling	✅ Rejected at Signer::sign_tx_env (top-level guard). Inner TxV0 => Err(UnsupportedTransactionEnvelopeType) in LedgerEntry::sign_tx_env is now defensive/unreachable but harmless.
Signature(signature_bytes.try_into()?)	✅ Returns TryFromSliceError for wrong-length device responses, no panic.
APDU layout for fee-bump	✅ Correct: HD_PATH_ELEMENTS_COUNT + path bytes + serialized TransactionSignaturePayload (with TxFeeBump tagged variant), chunked at CHUNK_SIZE, P1/P2 flags set correctly.
Speculos integration test signature verification	✅ Verifies against the host-computed tx hash (which is SHA256(TransactionSignaturePayload)), which is exactly what the device signs.
New security concerns	✅ None. Same net win as before: more device verification, less reliance on Hash Signing mode.

Findings

• cmd/crates/soroban-test/tests/it/emulator.rs:16-22 — Comment still references "Speculos click counts are calibrated for this exact Payment + memo layout", but the click counts were removed in cc73055 in favor of polling for the "Approve" screen. Inline comment with a one-block suggestion posted.

Items previously raised and resolved by author decision (not re-flagging)

• Fee-bump label inconsistency in the Ledger infoln — author: "we don't care about distinguishing the label; it doesn't bring much to the table". Closed.
• Single-variant let for FeeBumpTransactionInnerTx — author: "let becoming refutable is a loud compile error. Not worth the extra match nesting". Closed.
• Redundant SHA-256 of network passphrase — author: "deduping one SHA-256 over a ~30-char string is not worth a new public helper". Closed.

Verdict

LGTM after addressing the stale comment (optional — cosmetic). The two unit-test + polling improvements in this round materially reduce maintenance risk; test_sign_fee_bump_tx's chunks.len() == 2 assertion in particular gives the fee-bump path a strong tripwire if the framing ever shifts.

Todo

• Diff against origin/main and identify what changed since prior review
• Read the latest cmd/soroban-cli/src/signer/ledger.rs
• Read the latest cmd/soroban-cli/src/signer/mod.rs
• Read the latest cmd/crates/stellar-ledger/src/lib.rs
• Read the latest cmd/crates/stellar-ledger/src/emulator_test_support/util.rs
• Read the latest cmd/crates/soroban-test/tests/it/emulator.rs
• Confirm each prior unresolved item is either addressed or explicitly declined
• Look for new issues introduced by the follow-up commits
• Post the final review
  • branch clear-sign-classic-tx

[claude]

Stale comment after the polling refactor. This block still says the Speculos click counts are "calibrated for this exact Payment + memo layout", but commit cc73055 replaced the per-device-model click counts in approve_tx_signature with advance_to_approve_and_confirm, which polls until the on-screen text reads "Approve" (capped at 50 right-clicks). The fixture shape no longer drives click counts, so the rationale here misleads readers.

Suggested change

	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) and the transaction shape mirror stellar-ledger's
	// `test_sign_tx`, whose Speculos click counts are calibrated for this exact
	// Payment + memo layout.
	// Sign a classic Payment envelope with a Ledger identity end-to-end. After the
	// blind-signing fix the CLI sends the full `TransactionSignaturePayload` to the
	// device via APDU `INS=0x04` (SIGN_TX), so the user approves the parsed
	// operation — not a hex hash. The Speculos approval flow used here
	// (`approve_tx_signature`) polls until the on-screen text reads "Approve",
	// so it tolerates changes in the device review screen flow.

Fix this →