Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes Soroban auth-entry signing for Ledger-backed identities by replacing eager, long-lived HID transports with a pure-data LedgerEntry that opens the Ledger connection only during signing, and by wiring Ledger support into the Soroban authorization payload signing path.
Changes:
- Introduces
LedgerEntry { hd_path, public_key }and updatesSignerKind::Ledgerto use it, enabling cached-pubkey lookups and lazy HID opens for signing. - Makes Soroban auth signing (
sign_soroban_authorizations/sign_payload) async and removes unnecessary async from argument parsing / signer construction. - Updates SEP-53 message signing helper to use the now-async
Signer::sign_payload.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/signer/mod.rs | Makes Soroban auth signing async; switches Ledger signer to LedgerEntry; implements Ledger payload signing path. |
| cmd/soroban-cli/src/signer/ledger.rs | Adds LedgerEntry that lazily opens HID per sign call; adds sign_payload for Ledger. |
| cmd/soroban-cli/src/config/sign_with.rs | Builds Ledger signers using LedgerEntry (no eager HID open). |
| cmd/soroban-cli/src/config/secret.rs | Makes Secret::signer synchronous; constructs LedgerEntry with cached pubkey/hd_path. |
| cmd/soroban-cli/src/config/mod.rs | Awaits async Soroban auth signing when preparing transactions. |
| cmd/soroban-cli/src/commands/message/sign.rs | Adapts SEP-53 signing helper/tests to async sign_payload. |
| cmd/soroban-cli/src/commands/contract/invoke.rs | Removes now-unneeded awaits for host function parameter building. |
| cmd/soroban-cli/src/commands/contract/deploy/wasm.rs | Removes now-unneeded await for constructor parameter building. |
| cmd/soroban-cli/src/commands/contract/arg_parsing.rs | Makes host function arg parsing synchronous; resolves signers without eager Ledger device access. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Sign Soroban authorization entries when the resolved
Addressargument is a Ledger-backed identity. Previously, invoking a contract function that calledrequire_auth()on an address resolved from a--ledger-backed alias panicked attodo!("ledger device is not implemented")inSigner::get_public_keywhenever the device was connected.Why
Closes #2566. After #2563 added
stellar keys add --ledger, read-only operations and source-account signing worked for Ledger identities, but the Soroban auth path was unfinished:SignerKind::Ledgerheld a live HID transport (Ledger<TransportNativeHID>) eagerly opened during argument parsing, yetget_public_keyandsign_payloadwere unimplemented for it. The transport was also held across parse → simulate → RPC → sign, which conflicted with the second HID transport opened by--source <ledger-alias>(per-processhidapicannot host both at once).This change mirrors the existing
SecureStoreEntrypattern:SignerKind::Ledgernow carries a pure-dataLedgerEntry { hd_path, public_key: Option<PublicKey> }with no live transport. HID is opened lazily inside each sign call (LedgerEntry::sign_tx_hash,LedgerEntry::sign_payload) and dropped immediately, so two transports can never coexist.Signer::get_public_keyfor Ledger now returns the cached strkey synchronously (always populated fromSecret::Ledger), andSigner::sign_payloadis async with a working Ledger arm that calls the samesign_blobprimitive used today for transaction-hash signing.Secret::signer,parse_function_arguments, andbuild_host_function_parametersno longer need to beasync— the only thing that did was the now-removed eager Ledger HID open during arg parsing.Reject
--hd-pathoverrides on Ledger aliasesThe hd-path of a Ledger alias is fixed at
keys add --ledgertime. Accepting a caller-supplied--hd-pathagainst a Ledger alias was either redundant (matching the cached value) or unsafe (mismatching it silently used the wrong cached public key for auth-entry matching and signature-hint derivation while the device signed at a different path).Secret::signerandSecret::public_keynow error withLedgerHdPathFixedfor any non-Nonehd_pathargument when the secret isSecret::Ledger. Discovery on other paths goes through the existing--ledger --hd-path Nescape hatch onkeys public-key/keys address, which queries the live device directly.usize→u32forhd_pathBIP32 derivation paths are 32-bit by spec — that is the type used by the Stellar Ledger app and by
LedgerEntry::hd_path. The codebase had been carryingOption<usize>end-to-end and converting tou32only at the device-call boundary viatry_into().unwrap_or_default(), which silently truncated values aboveu32::MAXto0on 64-bit systems and signed with the wrong account. Switching the chain (Secret::*,SecureStoreEntry,secure_store::*,keyring::*,locator::get_secret_key_with_hd_path,UnresolvedMuxedAccount::resolve_muxed_account,UnresolvedScAddress::resolve,Args::hd_path(), every command's claphd_pathfield) toOption<u32>means clap range-validates at parse time, the silent truncation is gone, and three now-unreachableError::HdPathOutOfRangevariants are removed. The singleas usizecast now lives at sep5'sfrom_path_index(usize, ...)boundary. On-disk TOML format is unaffected — serde serializes either type as a plain number.Known limitations
The Soroban auth-payload path uses
sign_transaction_hash(the existingsign_blobprimitive) — the firmware accepts arbitrary 32-byte payloads when hash signing is enabled. There is no HID emulator wired intosoroban-testyet, so the actual signing path is covered only by manual validation. Unit tests cover the deferred-construction plumbing and the cached-pubkey path.Manual validation against a real Ledger device: