Repository navigation
Conversation
#238) A 145-bin liquidity withdrawal carries 158 post-conditions (155 NFT MaySend + 2 FT + 1 STX) and was rejected at parse. `PostConditions` stored one 8-byte slice per condition inline in `ParsedObj`, so `NUM_SUPPORTED_POST_CONDITIONS` had to stay small enough to keep the struct inside the fixed C parser buffer. #227 raised it 16 -> 128 for the 84-condition case in #224; a wider position goes straight past that too, and raising the cap again would only move the cliff. Stop storing per-condition slices. `PostConditions` now keeps the serialized post-condition block as a single slice and re-walks it on demand, alongside a small table of the offsets that *open* a display unit (the first occurrence of each aggregatable NFT group). Aggregation and paging stay O(conditions x distinct groups) rather than O(conditions^2). Consequences: - size_of::<ParsedObj>() drops 1124 -> 492 bytes on the device, so PARSER_BUFFER_SIZE goes 2048 -> 1024 and no longer needs to be sized per pointer width; the compile-time size assert now covers the host build too. - The condition cap no longer costs RAM. It only bounds parse/display time, so it is raised 128 -> 512. Display behaviour is unchanged: the 158-condition transaction renders the same 26 items, with the 155 MaySend NFTs collapsed to "Count: 155". Tests: the real blob from the issue is added as the `wide-position` zemu fixture and signs on all five device models; a randomized Rust test cross-checks the block walk against a reference implementation of the aggregation rules.
…implicit Three gaps a coverage review of #240 turned up, all at the edges rather than in the reported case: - NUM_SUPPORTED_POST_CONDITIONS was tested from the rejecting side only (513). test_at_cap_is_accepted pins that exactly 512 parse, and that the item count saturates rather than wraps -- it is Transaction::num_items that refuses such a transaction, not the post-condition parser. - MAX_NFT_GROUPS (96) was argued by inspection. test_nft_group_table_bound pins 96 distinct MaySend groups parsing and 97 failing cleanly with ValueOutOfRange at parse, never a silent truncation of the opener table. - The issue #238 transaction was pinned only by zemu screenshots. test_issue_238_transaction_renders_26_items loads the wide-position fixture -- whose post-condition block and argument list are byte-identical to the wallet request attached to the issue -- and asserts the number: 26 items, the six base items, arg0..arg4 with the 145-bin list as a type placeholder, and the 155 MaySend NFTs collapsed to one block ending in Count = 155. 89 rust tests, clippy clean.
…ype name
The gate keyed on the Clarity type of each contract-call argument, so it fired
on arguments that hid nothing: an empty list, `(some u493568)`, an 8-byte
buffer. Ordinary swaps -- app.alexlab.co, and a testnet call whose only argument
was `(list)` -- demanded blind signing while the device had everything it needed
to render them.
Gate on what can actually be rendered instead. `write_value_text` builds a
value's display text into a fixed 256-byte buffer -- the render budget -- and
`arg_is_fully_displayable` is now just "that succeeded". The predicate and the
renderer cannot drift apart, because they are the same function; the type-keyed
match they used to mirror is gone, and with it
`test_displayable_predicate_matches_renderer`'s job of pinning two lists
together.
What that buys, against the transactions reported from the field:
- an empty list or tuple renders `List: []` / `Tuple: {}`. It hides nothing.
- `Some`/`Ok`/`Err` render what they wrap -- `Some(493568)`, not `Option: Some`.
The wrapper stays in the text: approving `(some u493568)` is not the same as
approving `u493568`. Bounded at MAX_WRAPPER_DEPTH levels, so a crafted
argument cannot walk the device stack down.
- buffers up to 64 bytes render as hex. SIP-10 memos are `(optional (buff 34))`,
so every sBTC transfer carrying a real memo comes back out of blind signing.
- strings are paged instead of cut at 60 characters. `pageCount` is a u8, so
truncation was never necessary -- and it dropped four more characters than
advertised, slicing `payload[4..60]` off a payload whose first four bytes are
the length prefix.
- a UTF-8 string renders when its bytes are ASCII, which the device fonts cover.
Still gated, and correctly: populated tuples and lists, which need one display
item per leaf; buffers past the cap; non-ASCII UTF-8; anything past the render
budget; and contract deploys, whose Clarity source is never shown.
Also fixes an empty `string-ascii` argument failing the whole parse. It handed
`page_string` an empty slice, which returns NoData, so the transaction could not
be signed at all -- blind signing enabled or not.
The memo path now shares the same renderer. One rendering change falls out of
that: a bool memo reads `Bool: true` rather than `true`. SIP-10 memos are
buffers, so no real transfer takes that arm.
Verified: 93 rust tests (8 new), 27 host tests, clippy clean. The device target
builds (thumbv6m-none-eabi, no_std, -Z build-std=core); librslib.a grows 1.9 KB.
The testcases.json expectations that move are exactly the buffer and
long-string lines.
Four snapshot sets across all five device models, and nothing else:
contract_call_long_args arg7 renders 0x6162636465666768, was "is Buffer"
swap_with_post_conditions same buffer argument
call_with_string_args the 230-character string is paged in full rather
than cut at 56, so the Nano models gain screens
(16 -> 18) and Stax gains one (7 -> 8)
mainmenu version 0.26.19 -> 0.26.20
None of the eight blind-signing toggles added in d90d91d come out. All three
affected contract calls still carry a populated tuple or a non-ASCII UTF-8
string, so they are still gated -- correctly.
Full suite: 225 tests, 5 models, green.
A tuple or a list argument had no rendering at all -- the device printed the word "Tuple" and the transaction went behind the blind-signing opt-in. That is what makes an ordinary swap on app.bitflow.finance unsignable without accepting "you could lose all your assets": its arguments are two tuples holding three contract principals between them. Flatten them instead. A contract call's arguments are walked once, and every leaf becomes its own display item keyed by the path that reaches it: arg3.a SP1WNM..token-stx-v-1-2 arg3.b SP3K8B..sbtc-token Lists are keyed by index (`arg0[2].amount`), and a `Some`/`Ok`/`Err` wrapping a container is descended into (`arg0.some.n`) rather than left opaque. An *empty* container stays a single item: `List: []` is the whole value, and descending would drop the argument out of the review entirely, so the user would never see it was passed at all. The gate is now a budget rather than a type test. A transaction is blind-signed only when the arguments genuinely cannot be laid out: a leaf the device cannot render, a key path past MAX_KEY_PATH, nesting past TX_DEPTH_LIMIT, more than MAX_ARG_DISPLAY_ITEMS leaves, or -- the transaction-level check -- items that do not fit the u8 display index alongside the post-conditions. Post-conditions and arguments share that index: a wide DLMM withdrawal already spends 26 items on post-conditions, which is why `Transaction::flatten_args` and not the payload makes the call. When it does not fit, arguments fall back to one item each -- the placeholders this replaces. That path is never what someone silently signs: reaching it means the transaction is gated, so they have opted in to seeing it. Two things worth flagging for review: - The leaf count is computed once, at parse time, and memoized on the wrapper. `num_items` runs on every screen the device draws, and re-walking the argument bytes per page turn would cost O(payload) each time on a Nano. - MAX_KEY_PATH is 32, wider than a Nano's 17-character key line, because real Clarity field names (`min-amount-out`) run past 17 and gating on them would reintroduce the problem this change exists to fix. Where the path does not fit the device the *key* is cut to length; the value is always shown in full. Sizing it from the device would have made the gate device-dependent, which it has never been. Verified: 101 rust tests (9 new), 28 host tests, clippy clean, device target builds. Contract_call_long_args -- a nested tuple, a list, a buffer and an optional -- goes from one "is Tuple" screen to 31 rendered items, and out of the gate entirely. Contract_call_string_args stays gated on its non-ASCII string-utf8, which has no faithful rendering on the device fonts.
…emulator Two additions, both about being able to answer "what still gets blind-signed?" with evidence rather than reasoning. Rust: test_every_walk_failure_gates runs all seven ways the walk can end badly -- non-ASCII string-utf8, oversized buffer, string past the render budget, key path past the budget, nesting past the depth limit, more leaves than the budget, and an opaque leaf buried inside a tuple -- and asserts each one leaves `flat_items` empty and reports as gated. That last case matters: flattening is all or nothing, so one bad leaf gates the whole call rather than showing a partial view. test_flattening_implies_a_sound_item_count pins the converse, so no path can report a short count and quietly drop the rest. Zemu: the transactions from the field reports, as fixtures. The bitflow swap-helper-a (tuple arguments) and the alexlab swap-helper (an optional argument) are built from the contract, function, arguments and post-conditions of the actual wallet requests; only the origin differs, since it has to be the emulator's key. The empty-list case is rebuilt from its reported shape -- we do not carry that testnet blob. None of them toggles blind signing: that is the assertion. If the gate comes back for any of them, the review screens never appear and the test fails. Checked while writing these, and unchanged: the three DLMM aggregated fixtures still require blind signing, and should. Their arg0 is a list of 61 tuples of five fields -- 305 display items, past both the argument budget and the 255-item index the device addresses them with. That is the case everyone agrees is genuinely unshowable.
256 files: 165 changed, 91 new.
contract_call_long_args 1 "is Tuple" screen -> 31 rendered items, and no
blind-signing opt-in; the toggle comes out
swap_with_post_conditions same argument list, same story
bitflow_swap_tuple_args new -- arg3.a / arg3.b / arg4.a, the three
alex_swap_optional_arg new -- Some(493568)
empty_list_arg new -- List: []
mainmenu version 0.26.20 -> 0.26.21
Screen counts roughly double on the Nano models, which is the point: those are
values that were not on screen before.
Still carrying their opt-in, all correctly: the four contract deploys, the
non-ASCII string-utf8 call, and the three DLMM aggregated fixtures, whose arg0
is 305 display items.
Full suite: 240 tests, 5 models, green.
…rs in full Flattening introduced a backward-compatibility regression: a contract call with more than MAX_ARG_DISPLAY_ITEMS scalar arguments overran the leaf budget, so `flat_items` came back empty and the gate fired -- yet the one-item-per- argument layout shows every one of those arguments in full, and did so on the previous version without any opt-in. A transaction that reviewed normally on 0.26.19 would have demanded blind signing on this one. Record at parse time whether the fallback layout renders everything (`single_items_render`: the pre-flattening predicate, `Arguments::all` over `arg_is_fully_displayable`), and gate only when NEITHER layout shows the arguments. New behaviour is now a strict superset of old for "reviews normally": anything the previous version showed in full, this one does too. Tests, at three levels: - contract_call.rs: the >200-scalars case is pinned as NOT gated, and the budget test uses a list (a container the fallback cannot show) instead. test_every_walk_failure_gates now also asserts the fallback does not claim to render any of the seven failure shapes. - parsed_obj.rs `gate_paths`: the three outcomes driven end to end through ParsedObj, the object the C layer uses -- review (flattened, and the fallback), blind sign (unrenderable leaf, render budget, leaf budget), and rejected (undecodable argument: unknown type tag, truncated buffer, non-ASCII string-ascii; display-index overflow, which must stay a rejection rather than become a blind-signing prompt). Nesting depth is walked through all three in order, pinning the boundaries: TX_DEPTH_LIMIT nested lists is the first to gate, two past it the first to be rejected. The band between is valid Clarity the network accepts and the device can only blind-sign. - ui_tests.cpp: an undecodable argument is rejected by parser_parse whether blind signing is enabled or not. There is no blind-signing path for data the parser cannot decode -- it cannot find where the argument ends -- and the setting must not be offered as a fix for something it does not fix. Verified: 113 rust tests, 29 host tests, clippy clean, device target builds. No zemu fixture is affected: every one that flattened still flattens, and every one that was gated has a populated container or non-ASCII text the fallback cannot show either.
…ly absent Both predate this branch -- they are on main -- and both contradict the invariant the blind-signing gate exists to hold: nothing the device signs is hidden from the user without their opting in. A merge simulation against main found the first by execution and the second by trace; each is fixed here with the test that would have caught it. 1. Argument overflow was swallowed. `TransactionPayload::num_items` fell back to CONTRACT_CALL_BASE_ITEMS when the contract call's own count overflowed the u8 display index. From 253 arguments up, the payload reported three items, the transaction six, and the review showed origin, nonce, fee and the three base items -- with every argument absent and no warning, since every argument was individually displayable. 250 to 252 arguments were already rejected; 253 and beyond slipped past the check that rejects them. Propagate the error instead: the transaction is refused, as it is for 250. 249 scalar arguments are now pinned as the most a call can carry (255 items exactly), and 250, 252, 253, 254 and 300 as rejected. The previous commit had said the one-item-per-argument fallback "shows every one of those arguments in full"; past 252 that was not true, and now it does not need to be. 2. SIP-10 details were hidden for the wrong calls. `should_hide_sip10_details` keyed on the contract being a listed token; `render_sip10_transfer_args` keys on that AND the function being `transfer`. A call to any other function on a listed token, with a first post-condition matching the token's expected code, took the hide path in the count but not in the renderer: base items and post-conditions dropped from the count, three indices still spent on the base items, so the last three arguments were never reachable and the post-condition never shown. No gate fired, because every argument was displayable. Key the decision on `is_sip10_transfer` -- the same predicate the renderer uses -- so the two cannot disagree. The new SIP10_contract_non_transfer_ not_hidden fixture is the hidden-post-condition fixture with `transfer` renamed to `burn`: it now renders all 13 items, the four arguments and the SentEq post-condition included, where it rendered seven. 114 rust tests, 31 host tests, clippy clean. The existing SIP-10 fixtures, which all call `transfer`, are unchanged.
…ng review A backward-compatibility review of the gate branch found one thing the new rendering showed *less* of than the old: a string-ascii past the render budget used to appear on the blind-signing review as its first 56 characters (cut silently, and four short of the advertised 60); it had become the placeholder "is StringAscii". Same opt-in either way, but less on screen. Show the first MAX_VALUE_TEXT characters with an explicit "..." instead -- what the previous version showed, plus the fact that it was cut. The same courtesy applies to a SIP-10 memo that is only too long. Non-ASCII text still gets the placeholder; there is nothing the device fonts could show of it. Two smaller things from the same review: - The key-line comments were wrong. Every shipped target passes the parser a 64-byte key buffer (the 18-byte line is the retired Nano S), and a Nano X/S+ elides a long key visually. The truncation in render_flat_arg therefore never engages on a device -- only for a smaller caller, such as the host validator's 30-byte buffers. Documented as such; behaviour unchanged. - Device stack headroom for the walk cannot be measured on the host. The new zemu file argument_limits.test.ts signs, on all five models and with no snapshots, a list of exactly MAX_ARG_DISPLAY_ITEMS leaves and a value nested to the deepest level the walk flattens, alternating tuple and list. Zemu runs the real firmware with the real stack canary: if either walk overran, the app would halt and no signature would come back. 117 rust tests, clippy clean.
…UI can address The device refused a contract call whose argument was a list of 128 or more leaves, answering display_idx_out_of_range for a transaction it had just counted and could render. A bisection on the emulator put the boundary between 120 and 124 leaves -- 126 and 130 display items -- which is the 128 line, not anything in the walk. Two causes, both older than this branch: 1. rslib.h declared `_getItem(.., int8_t displayIdx, ..)` while the Rust side takes a `u8`. Index 128 and up crossed the FFI sign-extended; the Rust `display_idx >= num_items` guard then compared a value the caller never sent, and refused the item. Declared `uint8_t`, which is what the Rust signature has always been. 2. zxlib hands the review screens their index through the same signed type -- `viewfunc_getItem_t(int8_t displayIdx, ..)`, deps/ledger-zxlib/app/ui/view.h -- and that one is not ours to change. So item 128 is unreachable in the UI regardless, and `parser_getItem` turns a negative index into `parser_no_data`. The app's real display ceiling is 128 items, not the 255 a u8 suggests. Add MAX_DISPLAY_ITEMS = 128 and enforce it in `num_items` and in the `flatten_args` fit check, so a transaction past it is refused deliberately instead of counted, accepted, and then failed halfway through the review. This is not a behaviour change: such a transaction was already rejected, by parser_validate hitting the same wall. Lower MAX_ARG_DISPLAY_ITEMS 200 -> 64 to sit well inside that ceiling. `render_flat_arg` restarts the walk per screen, so a review costs O(items x leaves): 64 leaves is a few thousand value reads where 200 was forty thousand, on a 30 MHz chip, paid twice -- once by parser_validate and again as the user pages. The reported swaps need one to six leaves; the largest fixture here expands to 31. Verified on the emulator at 8/16/32/48/64/96/100/104/108/112/116/120 leaves accepted and 124/127/128/160/200 refused, before the fix. Tests updated to the real ceiling: 122 scalar arguments fill the index exactly, 123 is refused, and a 64-leaf list reviews or is gated depending on how many items the post-conditions leave it. 117 rust tests, 31 host tests, clippy clean, device target builds.
The CI linter checks app/rust/include/rslib.h and rejected my hand-wrapped declaration (lines 26-27). Reflowed with clang-format -style=file; the only change is where the parameter list breaks. Also corrects the comment above it, which still said a transaction can carry up to 255 display items -- zxlib's viewfunc_getItem_t is int8_t, so MAX_DISPLAY_ITEMS caps it at 128, which is the point of the fix. No behaviour change; host tests still 31/31.
The token registry lives in token_info.c, which host builds do not link, so get_token_info() returned None unconditionally outside the device. Everything reachable only through a registry hit -- the compact SIP-10 transfer card and the predicate that decides which items the review hides -- was therefore dead code to both the unit tests and the fuzzer. Recognising a single contract name gives those paths a way in without pulling the real table into host builds.
A contract call is rendered through the compact "Amount / From / To / Memo" card only when the called function is `transfer` and the contract is in the token registry. The predicate that shrinks the item count, however, matched on the contract address and name alone. For any other function on a registry contract the two disagreed: the item count dropped the three base items and every post-condition, while the renderer still emitted the base items. The review then ran out of slots before reaching the function name, the trailing arguments and the post-conditions -- all of which the signature still covered, and none of which asked the user for blind signing first. A call to a listed token with one argument showed the contract address and nothing else. Suppressing items now requires the call to be one the renderer really shows as a SIP-10 transfer, so the two agree by construction.
The Rust declaration of hash_sha256 typed its length as u16 while the C prototype takes a uint32_t, and crypto_sha256_one_shot narrowed it again on the way to the hasher. Any input longer than 65535 bytes was silently truncated to its remainder modulo 64 KiB. Every target but Nano S buffers 85 KiB, so a SIP-018 structured message can reach that size, and its hash is what gets signed. The device would return a signature that is valid over a prefix of the message it was handed, while the review showed a digest that matched only that prefix. Widening the length to uint32_t end to end makes the signature cover what was actually sent.
The review showed at most 270 characters of a signed message followed by an ellipsis, while the signature covered the whole text. A wallet could therefore put anything it liked past that cut-off and still get it approved. Real sign-in prompts already run past the limit -- the Gamma one is 284 characters -- so this was the normal case, not an edge case. Messages are now paged over directly, rewriting each page as it is copied out so the control-character substitution needs no buffer the size of the message. Past a bound where the page counter would no longer fit in a u8 the excerpt remains, and those messages report that they need blind signing, which routes them through the existing opt-in instead of quietly hiding their tail.
Post-conditions only guarantee anything under Deny, where a movement no condition covers aborts the transaction. Under Allow they are advice, and under Originator (SIP-040) they constrain the origin account alone. The mode byte was parsed and validated but never shown, so all three produced the same protective-looking list of conditions and the user had no way to tell them apart. The mode now sits with the origin, nonce and fee at the head of every transaction review. The C++ UI fixtures move with it.
The crate is on edition 2018, where `panic!` and the assert macros hand a lone string literal through unformatted. Two of the new assertion messages relied on inline captures, so a failure would have printed the placeholder rather than the value that explains it.
Only \b through \r were rewritten, but a message is admitted on `is_ascii()` alone, so the rest of the C0 range and DEL reached the screen untouched. The value buffer is handed to the UI as a C string, so a NUL ends the page it sits on: a page beginning with one renders blank while every byte on it stays covered by the signature. Paging a message in full rather than excerpting it at 270 bytes widened that window to 4 KiB, so it is worth closing in the same breath.
Several of the comments added alongside these fixes asserted things the code does not support, which is worse than saying nothing: a reader takes them as evidence the case was checked. - The token registry stub claimed host builds do not link token_info.c and that the fuzzer was blind to it. CMake links it into both the C++ tests and the fuzzer, and builds the crate with cpp_test, not the fuzzing feature. Only cargo test was ever blind. - The SIP-10 hiding comment listed a fixed set of dropped items. What is lost depends on the argument count, and the set it named holds for no count. - The blind-signing flag claimed JWTs and structured messages "have their own review flows", which reads as covered. Both show a hash instead of the content; they are ungated, and the comment now says so. - The header-item comment said the review opens with the origin address. On a sponsored transaction those values are the sponsor's. - MAX_ASCII_LEN's rationale rotted when paging replaced truncation. - The hash extern's note was inaccurate for cpp_test builds.
The registry stub was added so the item-hiding predicate could be exercised, but the only test asserted the branch where hiding does not happen. Nothing held the positive case, so tightening the predicate further could disable the compact card without a test noticing. The fixture is the existing fungible-post-condition transaction with its payload respliced as a four-argument transfer, which is what the compact card requires.
crypto_helper.c is linked into the host tests and hashes through picohash there, so the length truncation is directly observable without a device -- it just had no test, and nothing in the repo called crypto_sha256_one_shot at all. The vectors cover 70000 bytes, whose digest differs from the truncated input's, and exactly 65536, where the old uint16_t reduced the length to zero and hashed nothing.
The Rust side unit-tests `ByteString::requires_blindsign`, but nothing covered the path from there through `_tx_requires_blindsign` to a refused APDU -- and that path is the control. Every fixture in testcases.json is a transaction, so a message never reached parser_parse in any test: dropping the message arm of the FFI left all 91 Rust and 27 C++ tests green while a 100 KB message signed after showing 270 bytes. Messages are built inline rather than stored as fixtures because the lengths that matter are kilobytes of filler. Removing the FFI arm now fails two of these.
…behaviour apdu_unwrap and apdu_expect compiled to unreachable_unchecked, so an invariant the parser was supposed to have established but had not would not stop the device -- it would let the optimiser assume the impossible and continue into whatever fell out of that, at opt-level "z", in the middle of rendering a transaction for approval. Nothing reaches these today; the point is what happens the first time a parser change makes one reachable. A panic under panic = "immediate-abort" ends the command deterministically instead.
The buffer arm took the declared 32-bit length and added four to it with no check that the bytes were there. On the device's 32-bit target, and with no overflow checks in the release profile, a length near u32::MAX wrapped: a buffer claiming about 4 GiB measured as a handful of bytes, and every field parsed after it came from the wrong offset. Bounding the length against the remaining input, the way the string arms already do, removes both the wrap and the unchecked read.
An input long enough to need more than 255 pages of a given output buffer reported the count modulo 256. That both hid the tail and misplaced the pages that were reachable, because the final page's length was derived from the wrapped count rather than from the input. The count now saturates, and each page's extent is taken from the input length directly.
u64::MAX with six decimals is "18446744073709.551615", 21 characters into a 20-byte buffer. The formatter bounds-checks, so nothing overflowed, but the amount never appeared: on device it was replaced by "ERR", and on host builds the formatter failed outright and left the unwrap at the call site to abort the render.
On a sponsored transaction the device signs as the sponsor, and the three header items are read from the sponsor's spending condition. They were still titled "Origin", attributing them to the party that built the transaction, so the review told the user their own address and nonce belonged to someone else.
Domain fields were validated one at a time and then counted, so a tuple could
satisfy the count while repeating a key and omitting another: {name, name,
version} passed as three valid fields with no chain id. The review renders one
row per field name, so the missing one simply showed nothing while the domain
still hashed into the signature.
The sighash chain skipped a field it failed to fetch and carried on hashing, and an unrecognised field id fell out of the switch into a memcpy of whatever the previous iteration had left in the buffer. Either way the user would have been asked to approve a digest that no longer described the transaction. The parser validates every field before the loop runs, so neither path is reachable today. That is exactly why they should fail closed: the cost of a future parser change reaching one is a signature over the wrong hash.
Nothing dispatches on 0x05 and the APDU spec does not list it; structured messages are signed through INS_SIGN_SECP256K1, which picks the flow from the parsed payload type. Leaving the name in the header implied an entry point that does not exist.
Saturating the count stops the wrap but still hands the user a fraction of the input under a page total that reads as complete -- 45 KiB through a 160-byte buffer showed 4452 bytes as "28 of 28". For a device whose job is to show what it signs, that is the wrong failure. The count now bounds-checks and returns ValueOutOfRange, so an unpageable value fails parser_validate and the transaction is rejected instead of being half-shown. Nothing legitimate is caught by this: contract-call strings are capped at 60 bytes before paging, signed messages at 4096, and names and amounts are far smaller. The one uncapped caller is a SIP-018 domain field, which reads its length from the wire and is bounded only by the transaction buffer.
The note sat between INS_SIGN_JWT_SECP256K1 and INS_GET_MASTER_FINGERPRINT, which ends clang-format's alignment run for the instruction defines: it wanted to re-align the five above the comment to a narrower column, and the lint job failed on the difference. Moving the note above the block leaves the alignment intact. Its second sentence was also wrong. Dispatch does not look at a parsed payload type -- ParsedObj::read sniffs the "SIP018" prefix on the data before anything is parsed -- and "payload type" already names the chunk INIT/ADD/LAST byte one function away in apdu_handler.c, so the phrase pointed at the wrong thing.
The panic rewrite dropped it, leaving the only .rs file in the crate without one.
Clippy's manual_is_multiple_of fires on the modulo form, and it was the only new warning in the crate.
get_stx_items unwrapped amount_stx_str while the identical Staking arm below it returned a parser error, so the one path that could not format an amount aborted the device instead of refusing the item. Widening the buffer put that out of reach, but the asymmetry is worth removing rather than relying on. The comment above amount_stx_str also understated the bound. The device formatter needs two bytes more than the digits it is handed, so 20 bytes was short by two rather than one, and it reported success while writing "ERR" -- which is why the amount silently disappeared instead of erroring. Covered by a test that renders u64::MAX through get_items, where the buffer actually bites, rather than through the formatter underneath it.
The test asserted only on Value::from_bytes, whose outer take rejects an oversized declared length on its own. On the 64-bit host the addition it was meant to guard cannot wrap, so it passed against the unfixed parser and guarded nothing on CI, which runs no 32-bit target. value_len is where the bad measurement surfaces at any pointer width, so assert there as well, and pin the well-formed case to its exact extent.
parse_bad_domain_name and parse_bad_domain_name_type fed inputs with no
"SIP018" prefix, so verify rejected them on the first byte and the domain was
never looked at. Both assertions held for any input at all, which read as
coverage of domain rejection while providing none. They now carry the prefix.
With that fixed, the duplicate-key rejection can be pinned: {name, name,
version} and {name, version, version} are rejected, and a well-formed domain
still parses so the check cannot later be tightened into rejecting valid ones.
The comment above the check also described the wrong symptom. get_item walks
the tuple by position and labels each row with the key it finds, so a repeated
key showed "name" twice rather than leaving a field blank.
The sponsor label was chosen behind cfg(not(any(test, cpp_test))), with the test builds pinned to "Origin", so no unit test, C++ UI test or fuzz build could observe it. The Zemu suite compiles the real branch but never reaches it: check_signer tests the origin hash first, and every sponsored fixture carries the device's own hash there. The label shipped unverified. It needs no fork of its own. Only parser_validate calls check_signer_pk_hash and the tests enter through parser_parse, so self.signer is Invalid in a host build and the match yields "Origin" regardless. The origin binding still needs one, because rendering an unnominated signer is an error on device but merely means "no signer chosen" in a test -- so that arm falls back to the origin, which keeps the existing fixtures working and lets a test nominate the sponsor. A test now nominates each in turn and checks the label, address, nonce and fee all follow. The header-items note at the top of the file said the address is titled "Origin" even when sponsoring, which stopped being true.
compute_sig_hash_chain was made to fail closed inside its loop, but the count that bounds the loop still failed open: _num_multisig_fields collapsed a null state, a non-transaction tag and a malformed count into 0, and the C signature had no error channel to say otherwise. A count of 0 is also what a legitimate first signer reports, so the chain would hash nothing, return zxerr_ok, and hand back a digest covering none of the preceding signers. The count now travels in an out-parameter with a parser_error_t return, the way _hash_mode already does, and the caller refuses to sign when it cannot be read. Nothing reaches this today -- the field count is a be_u32 over a slice that is always at least four bytes -- which is the reason to close it now rather than after a parser change makes it reachable.
…d arguments Both halves of this branch spend the same u8 display index: the post-condition mode adds a header item, and the flattened argument layout budgets against MAX_DISPLAY_ITEMS. Every count and offset that named the old six header items moves by one. The budget boundaries are unchanged in outcome -- 116 items still fit under the 128 ceiling and 131 still do not -- so only the constants move, not which transactions review. The C++ fixtures are regenerated from the harness; each now carries exactly one mode row, and the SIP-10 hidden and shown cases still differ, so the hiding path is still what it was.
Every transaction review gains the post-condition mode item, arguments render flattened one item per leaf, sponsored flows title the header items for the sponsor, and signed messages page in full instead of stopping at 270 characters. Regenerated against apps built from this branch for all five models. 155 of 205 directories move. The ten that do not are the flows with none of those: the main menu, address display, JWT signing and the structured data cases.
A transaction review used to run without confirmation, so the UI had no point at which to offer anything other than the next item. Ask for the progressive review instead, which lets zxlib place its shortcut to the approval in the flow. The shortcut only appears for a blind-signed review past REVIEW_SKIP_MIN_ITEMS. A user who has turned blind signing on and trusts the contract they are calling can then approve without paging through items the device has already said it cannot fully explain, while an ordinary review still has to be read to the end -- a shortcut there would be blind signing without the warning or the opt-in. Address review stays plain. It is three items and there is nothing worth skipping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
Drives the shortcut on the emulated Nano with a captured HODLMM "remove liquidity" transaction: 84 post-conditions and a list argument past the leaf budget, so it is gated and long enough to be offered a way out. The assertion that matters is where the skip lands. It must reach the risk acknowledgement, not the plain approval -- a user who opted in still says so before signing. That path also used to hang: the skip asked the flow engine to start at a step the blind-signing flow does not contain, so pressing both buttons did nothing at all. Nano only. The touch devices reach the same approval through NBGL's own Skip control, whose position the harness has no way to address; their snapshots record that the control is drawn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
Only the DLMM withdrawals move: they are the one transaction in the suite that is both blind signed and long enough for the shortcut to be offered. Everything else reviews exactly as it did, which is the point of keying the offer on blind signing rather than on length alone. On a Nano the review gains one screen, the offer itself, at a fixed place before the first item. On the touch devices no screen is added, but the Skip control in the header takes vertical space and the same content spreads over one more page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
The Nano cases assert that the shortcut reaches the risk acknowledgement rather than the plain approval. The touch devices reach the same place by a different route -- NBGL's own Skip control, then its "Skip review?" confirmation -- and nothing was checking that route at all; the snapshots only showed that the control gets drawn. Extends the existing cases to Stax, Flex and Apex with the same transaction and the same assertion. The Skip control's position is the one thing these tests carry themselves. Zemu has no ButtonKind for it, so every other target comes from getTouchElement and the header position is written down per model. An SDK bump that moves the header therefore breaks these tests, which is most of the reason to have them: it is otherwise a silent loss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
Six screens per model, recording the whole route: the blind-signing warning, the review, the Skip control in the header, the SDK's confirmation prompt, "Accept risk and sign transaction ?", and the signed transaction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
Picks up the two commits this branch depends on: the skip offered once rather than after every item, keyed on a blind-signed review and working when one is being signed, and the same shortcut on the touch devices through NBGL's Skip control. Without them the progressive review this branch turns on is worse than what it replaces -- the old skip menu, which interrupts every item and does nothing at all when blind signing. Points at Zondax/ledger-zxlib#234, which is not merged. Re-point at the merge commit before this leaves draft. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
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.
Value
A user who has deliberately turned blind signing on, and who signs the same call
repeatedly, currently has no way to reach the approval except by paging through
every screen. A wide DeFi withdrawal is 27 screens on a Nano; the device has
already told them it cannot fully explain one of the arguments, so most of that
paging buys nothing.
This adds a shortcut to the approval for those reviews, and only those.
It is offered only when the review is blind signed. On an ordinary review a
shortcut would be a way to approve without reading, which is blind signing in all
but name, reached without the warning that names it and without the setting the
user had to turn on first. It is also only offered past a length threshold, since
on a short review it costs more attention than it saves.
Taking the shortcut lands on "ACCEPT RISK AND APPROVE", not the plain
approval. The opt-in is still acknowledged at the point of signing.
Technical
The offer is one screen at a fixed position in the review, straight after the
review title and before the first item. It is a position in the ring rather than
something injected between items: walking back onto it works, reading past it is
uninterrupted, and nothing is added or removed while the review runs. Skipping
also leaves the paging one index past the last item, so stepping back from the
approval shows the end of the transaction rather than restarting it.
On the touch devices the same transactions ask NBGL for its own
Skipcontrol,so behaviour matches across the fleet.
The path being fixed here used to hang. The flow builder and the skip handler each
chose the approval step independently and disagreed under blind signing, so the
flow engine was asked to start at a step the flow did not contain and nothing
happened. Both now go through one helper, which makes the mismatch impossible to
reintroduce.
Verification
"remove liquidity" transaction and asserts it reaches the risk acknowledgement.
is uninterrupted, back-navigation from the approval lands on the last item, and
a long transaction that is not blind signed offers nothing.
Skip→ "Skip review?" → "Accept risk andsign transaction?".
Snapshot churn is 17 directories: the three DLMM blobs across five models, plus the
two new sets. Nothing else moves, which is the point of keying the offer on blind
signing rather than on length alone.
Notes for review
REVIEW_SKIP_MIN_ITEMS = 12is a judgement call and the obvious thing to arguewith. It is a one-line change and apps can override it.
ICON_NANOSandSKIP_NANOSdefaults to 1, so that target cannot be built here at all.
APP_DIR=/apppassed to the container. That is apre-existing mismatch between the makefiles and the pinned builder image, not
something this branch introduces.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN