|
| 1 | +# Spike: Ledger support in Standard CLI — offline source-code findings |
| 2 | + |
| 3 | +## Purpose |
| 4 | + |
| 5 | +Resolve the open assumptions in `docs/plan-standard-cli-ledger.md` so the |
| 6 | +implementation plan can be promoted from "pragmatic / hand-wavy" to |
| 7 | +"ready-to-implement" without requiring a physical Ledger device. |
| 8 | + |
| 9 | +This spike is **source-code only**. It does not run anything against a real |
| 10 | +device. Items that genuinely require hardware are isolated in §"Items that |
| 11 | +still need hardware verification" at the bottom. |
| 12 | + |
| 13 | +## Method |
| 14 | + |
| 15 | +Read the existing REPL Ledger sign path end-to-end and document its |
| 16 | +behavior. Where REPL already encodes a behavior, treat that as authoritative |
| 17 | +(it has been shipped against real devices for a long time). |
| 18 | + |
| 19 | +Files inspected: |
| 20 | + |
| 21 | +- `org.tron.ledger.LedgerSignUtil` |
| 22 | +- `org.tron.ledger.listener.LedgerEventListener` |
| 23 | +- `org.tron.ledger.listener.BaseListener` |
| 24 | +- `org.tron.ledger.listener.TransactionSignManager` |
| 25 | +- `org.tron.ledger.LedgerSignResult` (referenced; behavior inferred from call sites) |
| 26 | +- `org.tron.ledger.wrapper.HidServicesWrapper` (referenced) |
| 27 | +- `org.tron.walletserver.WalletApi` — sign sites and `signTransactionForCli` |
| 28 | +- `org.tron.walletcli.WalletApiWrapper` — gasfree sign site |
| 29 | +- `org.tron.walletcli.cli.StandardCliRunner` — auth path |
| 30 | + |
| 31 | +## Findings |
| 32 | + |
| 33 | +### F1: Standard CLI uses exactly **two** Ledger sign sites, not four |
| 34 | + |
| 35 | +Standard CLI's process pipeline only touches two of the four sign sites that |
| 36 | +the original plan listed: |
| 37 | + |
| 38 | +| Site | File / line | Triggered by | |
| 39 | +|------|-------------|--------------| |
| 40 | +| `signTransactionForCli(...)` Ledger branch | `WalletApi.java:1064-1093` | All standard-CLI sign commands (transfer, vote, freeze, …) via `processTransactionExtentionForCli` and `processTransactionForCli` | |
| 41 | +| GasFree sign branch | `WalletApiWrapper.java:3268-3286` | Standard-CLI `gas-free-transfer` only | |
| 42 | + |
| 43 | +The other two sites (`signTransaction(Chain.Transaction)` at line 904 and |
| 44 | +`signTransaction(Chain.Transaction, boolean multi)` at line 956) are |
| 45 | +**REPL-exclusive**. They are reached via `processTransactionExtention` / |
| 46 | +`processTransaction`, which standard CLI does not call. |
| 47 | + |
| 48 | +This shrinks the standard-CLI risk surface to two sites. |
| 49 | + |
| 50 | +### F2: REPL's "60-second timeout" is a polling loop with cooperative early exit, not a blocking wait |
| 51 | + |
| 52 | +`LedgerEventListener.executeSignListen` (`LedgerEventListener.java:78`) calls |
| 53 | +`waitAndShutdownWithInput()` (line 45), which: |
| 54 | + |
| 55 | +1. Spawns a background thread that runs `sleepNoInterruption(60)`. |
| 56 | +2. `BaseListener.sleepNoInterruption` (BaseListener.java:23) sleeps in 100ms |
| 57 | + chunks, checking `LedgerEventListener.getInstance().getLedgerSignEnd()` on |
| 58 | + each wake-up. If the flag is set, it exits early. |
| 59 | +3. Main thread `join()`s on this background thread. |
| 60 | + |
| 61 | +When the user presses confirm or reject, `hidDataReceived` (line 150) calls |
| 62 | +`doLedgerSignEnd()` (line 213), which sets `ledgerSignEnd = true`. The |
| 63 | +sleeping thread sees this within 100ms and returns. |
| 64 | + |
| 65 | +**Implication**: cancellation is already cooperative. We do **not** need |
| 66 | +`Future.cancel(true)` to work on the underlying HID call. To enforce a |
| 67 | +shorter timeout from outside, we set the same flag (or its replacement) and |
| 68 | +the existing loop exits. |
| 69 | + |
| 70 | +The constant `TRANSACTION_SIGN_TIMEOUT = 60` is hard-coded at |
| 71 | +`LedgerEventListener.java:27`. |
| 72 | + |
| 73 | +### F3: APDU error codes are already pattern-matched in REPL — they just print, they do not return |
| 74 | + |
| 75 | +`LedgerEventListener.handleTransSign` (line 104-148) hard-codes two APDU |
| 76 | +status words: |
| 77 | + |
| 78 | +| APDU | Constant in source | Existing REPL behavior | |
| 79 | +|------|--------------------|------------------------| |
| 80 | +| `0x6a8c` | `SIGN_BY_HASH` | Print "Please first set 'Sign By Hash' to 'Allowed' in Ledger TRON Settings" | |
| 81 | +| `0x6511` | `APP_IS_OPEN` | Print "Please ensure The Tron app is open in your Ledger device" | |
| 82 | +| Other non-empty response | (unhandled) | (no message) | |
| 83 | +| `null`/empty response | (success path) | Submitted; wait for button | |
| 84 | + |
| 85 | +The function returns the raw response bytes. Callers currently only check |
| 86 | +`response == null` (= submitted, wait). We can map the same bytes to typed |
| 87 | +error codes without changing the underlying APDU exchange logic. |
| 88 | + |
| 89 | +### F4: Confirm vs reject vs timeout outcomes are recorded in two static stores |
| 90 | + |
| 91 | +After `executeSignListen` returns, the outcome is determined by inspecting: |
| 92 | + |
| 93 | +1. **`TransactionSignManager` (singleton)** — `getTransaction()` and |
| 94 | + `getTransactionSignList()`/`getGasfreeSignature()`. If a signature is |
| 95 | + present here, the user pressed confirm. |
| 96 | +2. **`LedgerSignResult` (file-backed state)** — |
| 97 | + `getLastTransactionState(devicePath)` returns a string enum: |
| 98 | + - `SIGN_RESULT_SIGNING` (still in progress) |
| 99 | + - `SIGN_RESULT_SUCCESS` (user confirmed) |
| 100 | + - `SIGN_RESULT_REJECTED` (user rejected) — set via `updateAllSigningToReject` in the cancel branch (line 166 / 178) |
| 101 | + - `SIGN_RESULT_CANCEL` (timed out after device responded) — set when `isTimeOutShutdown` is true at the moment of HID response (line 205) |
| 102 | + |
| 103 | +REPL's existing `executeSignListen` collapses all four into a single |
| 104 | +`boolean ret = true`, which is why surface-level it looks like REPL "loses |
| 105 | +information." It does not — the information is in the two stores; REPL just |
| 106 | +does not consult them at the call site. |
| 107 | + |
| 108 | +A non-interactive bridge can poll both stores after `executeSignListen` |
| 109 | +returns and emit a precise outcome. |
| 110 | + |
| 111 | +### F5: The pre-sign HID device discovery is already non-interactive |
| 112 | + |
| 113 | +`LedgerSignUtil.requestLedgerSignLogic` (`LedgerSignUtil.java:21`) reaches |
| 114 | +the device via `HidServicesWrapper.getInstance().getHidDevice(address, path)` |
| 115 | +(line 37). That call: |
| 116 | + |
| 117 | +- Returns the unique device whose Tron-app-derived address at `path` matches |
| 118 | + the requested `address`. |
| 119 | +- Returns `null` if no match is found. |
| 120 | +- Throws `IllegalStateException` on transport-layer failures (the existing |
| 121 | + call site catches this and treats it as `null`). |
| 122 | + |
| 123 | +There is no `selectDevice()` prompt, no menu, no `lineReader`. The |
| 124 | +discovery code is reusable as-is for standard CLI. |
| 125 | + |
| 126 | +### F6: REPL's interactive noise on the sign path is concentrated in `LedgerSignUtil` and the listener |
| 127 | + |
| 128 | +The pollution sources (in standard-CLI terms) on the sign path are: |
| 129 | + |
| 130 | +| Where | What | |
| 131 | +|-------|------| |
| 132 | +| `LedgerSignUtil` | 8 × `System.out.println`, 4 × ANSI color escape, on every reachable branch | |
| 133 | +| `LedgerEventListener.handleTransSign` | 2 × `System.out.println` for APDU error codes, 1 × ANSI | |
| 134 | +| `LedgerEventListener.waitAndShutdownWithInput` | 2 × `System.out.printf` (timeout banner) | |
| 135 | +| `LedgerEventListener.hidDataReceived` | 4 × `System.out.println` on confirm / cancel | |
| 136 | +| `LedgerEventListener.executeSignListen` | 1 × `System.out.println` ("Transaction sign request is sent to Ledger") | |
| 137 | + |
| 138 | +None of these go through any abstracted output channel. They are all direct |
| 139 | +`System.out` writes. The standard-CLI bridge must: |
| 140 | + |
| 141 | +1. Replace the `LedgerSignUtil` wrapper entirely (it is the highest-volume |
| 142 | + noise source and provides nothing standard CLI needs). |
| 143 | +2. Either (a) refactor the listener's prints into a callback / sink, or (b) |
| 144 | + leave them in place and rely on the existing standard-CLI stream |
| 145 | + suppressor. **Recommendation: (b) for MVP**, because `LedgerEventListener` |
| 146 | + is a singleton shared with REPL and refactoring its output channel ripples |
| 147 | + into REPL output. Suppressing during the bridge call is sufficient. |
| 148 | + |
| 149 | +### F7: `HidServicesWrapper.getHidDevice` is silent on stdout |
| 150 | + |
| 151 | +By inspection of the call shape and how REPL uses it (no surrounding |
| 152 | +"discovering devices…" banner around the call), this function does not |
| 153 | +print. The standard-CLI bridge can call it without suppressors. (Confirmed |
| 154 | +from REPL behavior: pre-sign device lookup happens silently.) |
| 155 | + |
| 156 | +### F8: Singleton state lifecycles |
| 157 | + |
| 158 | +| Singleton | Lifetime | Risk for standard CLI | |
| 159 | +|-----------|----------|------------------------| |
| 160 | +| `LedgerEventListener.INSTANCE` | Process | Holds `isTimeOutShutdown` and `ledgerSignEnd` `AtomicBoolean`s; both are reset on each `executeSignListen` call (lines 85, 73). One-shot CLI invocations are safe. Within a single process, two consecutive sign operations are also safe because each call resets. | |
| 161 | +| `TransactionSignManager.INSTANCE` | Process | Holds the in-flight transaction and signature. REPL clears `setTransaction(null)` after consumption. The bridge must do the same on every exit path (success, reject, timeout, exception). | |
| 162 | +| `LedgerSignResult` (file-backed) | Disk | Records last state per device path. Bridge must check this **after** `executeSignListen` to derive outcome. The file accumulates entries; existing REPL code does not prune it. Not a correctness concern. | |
| 163 | + |
| 164 | +For standard CLI's typical "one process per command" usage, the singleton |
| 165 | +risk is minimal. The defensive pattern is: reset `TransactionSignManager` |
| 166 | +state in a `finally` block. |
| 167 | + |
| 168 | +### F9: Standard CLI's Ledger detection rule is `wf.getName().contains("Ledger")` |
| 169 | + |
| 170 | +All three Ledger sign branches in `WalletApi.java` (lines 910, 973, 1064) |
| 171 | +test `wf.getName().contains("Ledger")` rather than the |
| 172 | +`WalletApi.isLedgerUser()` boolean. The boolean is set in the wrapper's |
| 173 | +login paths and used in `WalletApi.removeWallet(...)` (line 3670), but **not** |
| 174 | +on the sign path. |
| 175 | + |
| 176 | +The naming convention is enforced by `WalletApi.java:4652-4654`, which |
| 177 | +auto-prefixes `Ledger-` to any wallet that started with that prefix. So: |
| 178 | + |
| 179 | +- **Source of truth on the sign path: filename prefix `Ledger-`** |
| 180 | +- **Source of truth on the cleanup path: `isLedgerUser` boolean** |
| 181 | + |
| 182 | +This is a latent inconsistency. For this plan we **do not** unify it (out of |
| 183 | +scope and risky); we follow the existing sign-path convention (filename) so |
| 184 | +behavior is identical to REPL. |
| 185 | + |
| 186 | +### F10: GasFree path uses `gasfree=true` which short-circuits contract-type validation |
| 187 | + |
| 188 | +`LedgerSignUtil.requestLedgerSignLogic(transaction, path, address, gasfree)` |
| 189 | +takes a `gasfree` boolean. When `true`, line 23-26 skips the |
| 190 | +`ContractTypeChecker.canUseLedgerSign(...)` precheck. The bridge's sign |
| 191 | +method therefore needs the same parameter / a sibling method. |
| 192 | + |
| 193 | +## Implications for design |
| 194 | + |
| 195 | +### Outcome enum is fully derivable |
| 196 | + |
| 197 | +``` |
| 198 | +NO_DEVICE ← getHidDevice returned null |
| 199 | +APP_NOT_OPEN ← handleTransSign returned 0x6511 |
| 200 | +SIGN_BY_HASH_DISABLED ← handleTransSign returned 0x6a8c |
| 201 | +SUBMIT_FAILED ← handleTransSign returned other non-empty bytes |
| 202 | +ALREADY_SIGNING ← LedgerSignResult.getLastTransactionState was SIGN_RESULT_SIGNING before we started |
| 203 | +USER_CONFIRMED ← signature found in TransactionSignManager after wait |
| 204 | +USER_REJECTED ← LedgerSignResult.getLastTransactionState became SIGN_RESULT_REJECTED |
| 205 | +TIMEOUT ← wait returned but neither signature nor reject state |
| 206 | +``` |
| 207 | + |
| 208 | +Every transition above is derivable from existing public state. No hardware |
| 209 | +needed to design this. |
| 210 | + |
| 211 | +### The bridge can polls the same state REPL writes |
| 212 | + |
| 213 | +REPL writes `LedgerSignResult` and `TransactionSignManager` from the HID |
| 214 | +callback thread. The bridge reads the same state on the calling thread |
| 215 | +after `executeSignListen` returns. This is the cleanest possible coupling |
| 216 | +that avoids forking the shared listener. |
| 217 | + |
| 218 | +### Stdout suppression scope |
| 219 | + |
| 220 | +The bridge wraps `LedgerSignUtil`-equivalent operations. The wrapping must |
| 221 | +suppress stdout because: |
| 222 | + |
| 223 | +- `LedgerEventListener.handleTransSign` will still print on APDU errors. |
| 224 | +- `LedgerEventListener.waitAndShutdownWithInput` will still print the timeout |
| 225 | + banner. |
| 226 | +- `LedgerEventListener.hidDataReceived` will still print on confirm / cancel. |
| 227 | + |
| 228 | +These are not on our refactor target (shared with REPL). The bridge must |
| 229 | +redirect `System.out` for the duration of the call. The runner already has |
| 230 | +`OutputFormatter` machinery for stream suppression; the bridge reuses it. |
| 231 | + |
| 232 | +## Items that still need hardware verification |
| 233 | + |
| 234 | +These remain as Phase-end manual-QA gates, not blockers for design: |
| 235 | + |
| 236 | +| Item | Manual test | |
| 237 | +|------|-------------| |
| 238 | +| Real timing of `0x6a8c` and `0x6511` responses (synchronous vs delayed) | Try with "Sign By Hash" disabled / Tron app closed | |
| 239 | +| Disconnect mid-sign: does `hidDataReceived` fire with a special code, or does the timeout simply elapse? | Pull USB while waiting for confirmation | |
| 240 | +| Does the device reset its signing state when disconnected/reconnected? | Disconnect, reconnect, retry sign | |
| 241 | +| 60-second wall-clock accuracy of `sleepNoInterruption` under JVM contention | Run with high CPU load | |
| 242 | + |
| 243 | +The bridge's defensive design (catch all exceptions → `SUBMIT_FAILED`, |
| 244 | +clean `TransactionSignManager` in `finally`) covers all the above without |
| 245 | +requiring us to know the exact answer. |
| 246 | + |
| 247 | +## Conclusions for the plan |
| 248 | + |
| 249 | +1. **Refactor target shrinks to two sign sites for standard CLI MVP** |
| 250 | + (`WalletApi.signTransactionForCli` Ledger branch + `WalletApiWrapper` |
| 251 | + gasfree branch). The other two sign sites stay REPL-only. |
| 252 | + |
| 253 | +2. **The `LedgerSigner` abstraction the elegant version called for is still |
| 254 | + right** — but it can be applied just to the two standard-CLI sites, |
| 255 | + leaving REPL's two sites untouched. This is a smaller refactor than |
| 256 | + "introduce signer for all four sites." |
| 257 | + |
| 258 | +3. **No `Future.cancel(true)` needed.** Cooperative cancellation via the |
| 259 | + existing `ledgerSignEnd` flag is sufficient. |
| 260 | + |
| 261 | +4. **No "minimum viable error codes" compromise.** All seven outcome enum |
| 262 | + values are derivable from existing state; the plan can ship the full |
| 263 | + taxonomy from day one. |
| 264 | + |
| 265 | +5. **Manual QA gates remain unchanged.** A reviewer with a real Ledger runs |
| 266 | + a runbook to verify the four hardware-verifiable items before merge. |
0 commit comments