Skip to content

fix(view): offer the review shortcut once, only when blind signing, and make it work - #234

Merged
0xPxt merged 3 commits into
mainfrom
feat/review-fast-path
Sep 16, 2026
Merged

0xPxt merged 3 commits into
mainfrom
feat/review-fast-path

Conversation

@0xPxt

@0xPxt 0xPxt commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Value

A user who has turned blind signing on, and who signs the same call repeatedly,
had no way to reach the approval except by tapping or clicking through every
screen. A wide DeFi withdrawal is 27 display items; the device has already told
them it cannot fully explain one of the arguments, so most of that paging buys
nothing.

The skip menu was meant to be that way out. It did not work.

  • It was offered after every item. On a 27-item review that is 25 copies of a
    screen whose whole purpose is to shorten the review.
  • Taking it did nothing when blind signing — the one case where a shortcut is
    wanted most. Pressing both buttons left the screen exactly where it was.
  • It was offered on ordinary reviews, which is the dangerous one: a shortcut
    there is a way to approve without reading, reached without the blind-signing
    warning and without the setting the user had to turn on.

Technical

h_review_is_skippable() is the single predicate all three UIs share: blind
signed, and past REVIEW_SKIP_MIN_ITEMS. Counting the transaction's own display
items rather than the screens wrapped around them keeps the answer the same on
every device.

On bagl the skip becomes an ordinary UX_STEP_CB at a fixed slot before the first
item, so it is a position in the ring rather than something injected and withdrawn
mid-review. The custom button callback the interstitial needed is gone — left and
right are the flow engine's job again, which is most of the diff in view_x.c.

The blind-signing failure was a disagreement: run_ux_review_flow put
ux_review_flow_3_step_blindsign in the flow while the skip handler always asked
ux_flow_init to start at ux_review_flow_3_step, which that flow does not
contain. Both now call approval_step(), so the mismatch cannot come back.

Skipping leaves the paging one index past the last item, as reading to the end
does, so stepping back from the approval shows the end of the transaction.

MAX_REVIEW_UX_SCREENS goes 10 → 12: the added step makes the worst-case flow
eleven entries in a global array.

On NBGL the same reviews now set SKIPPABLE_OPERATION. That is the platform's own
control, and deliberately not a copy of the Nano screen — reproducing that would
mean rebuilding the review from nbgl_useCaseGenericReview and hand-writing the
blind-signing intro and the "Accept risk and sign" page, which are the SDK's to
own.

Verification

Exercised through Zondax/ledger-stacks#246, which turns the progressive review on:

  • Full zemu suite green, 255 tests across Nano X, Nano S+, Stax, Flex and Apex.
  • A new test drives the shortcut on all five and asserts it reaches "Accept risk
    and sign"
    , never the plain approval.
  • Verified by hand on a physical Nano S+ and a physical Flex: offered once, reading
    past it uninterrupted, back-navigation lands on the last item, and a long
    transaction that is not blind signed offers nothing.

Notes for review

  • Rebased onto main, and zxversion.h now goes to 52.3.0 so apps have a
    version to move to. Minor rather than major: nothing in include/ changed
    shape, since h_review_is_skippable() and REVIEW_SKIP_MIN_ITEMS both live in
    view_internal.h, so a consumer picks this up without touching its own code.
  • view_s.c (Nano S) gets the same gate but is not compile-checked — the app I
    verified against cannot build that target at all.
  • REVIEW_SKIP_MIN_ITEMS = 12 is a judgement call; apps can override it.
  • Apps without APP_BLINDSIGN_MODE_ENABLED now get no shortcut at all, since there
    is no opted-in state to key it on. Deliberate, but it is a behaviour change for
    any such app using the progressive review.

https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN

0xPxt added a commit to Zondax/ledger-stacks that referenced this pull request Sep 4, 2026
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
@chcmedeiros
chcmedeiros marked this pull request as ready for review September 16, 2026 06:19
@chcmedeiros
chcmedeiros self-requested a review September 16, 2026 06:19
0xPxt and others added 3 commits September 16, 2026 08:54
The skip menu had three problems, and they compound.

It was offered after every item. On a 27-item review that is 25 copies of
a screen whose purpose is to shorten the review, so taking the offer at
the first opportunity was the only way it ever saved anything. It is now
an ordinary step at a fixed slot in the flow, straight after the review
title and before the first item: offered once, up front, with reading
uninterrupted afterwards. Walking back onto it works, because it is a
position in the ring rather than something injected and withdrawn while
the review runs. That also retires the custom button callback the
interstitial needed -- left and right are the flow engine's job again.

Taking it did nothing at all when blind signing. The flow builder and the
skip handler each chose the approval step, and disagreed: the builder
uses the blind-signing variant, the handler always asked ux_flow_init to
start at the plain one, which is absent from that flow, so nothing moved.
Both now go through approval_step(), which makes the mismatch impossible
rather than merely fixed.

It was offered on any review. That is the important one. A shortcut on an
ordinary review is a way to approve without reading -- blind signing in
all but name, reached without the warning that names it and without the
setting the user had to turn on first. h_review_is_skippable() now
requires app_mode_blindsign_required(), and a review long enough to be
worth skipping at all; short ones spend a screen to save two clicks.

Skipping also leaves the paging one index past the last item, as reading
to the end does, so stepping back from the approval shows the end of the
transaction rather than restarting it.

MAX_REVIEW_UX_SCREENS goes 10 -> 12. The added step makes the worst-case
flow eleven entries, and that array is a global: overrunning it would
have corrupted whatever follows rather than failing where you could see
it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
NBGL draws its "Skip" control only for an operation that asks for it, and
nothing here ever did, so a touch review had no way to the approval short
of tapping through every page. The same reviews the Nano now offers a
skip on ask for it: blind signed, and long enough to be worth skipping.

Deliberately the platform's control rather than the Nano's screen. On
touch the header button costs no page and no extra tap, and the SDK
already asks "Skip review?" before honouring it, so the shortcut cannot
be taken by accident. Reproducing the Nano's single offer page would mean
rebuilding the review out of nbgl_useCaseGenericReview and hand-writing
the blind-signing intro and the "Accept risk and sign" page, which are
the SDK's to own.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PzJ2P5pR4becLUiDTm17HN
The review shortcut changes behaviour for every consumer that draws a
review: it is now offered once instead of after every item, only on a
blind-signed review past REVIEW_SKIP_MIN_ITEMS, and on touch devices it
appears at all for the first time. An app pinning zxlib needs a version
to move to in order to pick that up.

Minor rather than major because nothing in include/ changed shape -- the
new predicate and REVIEW_SKIP_MIN_ITEMS live in view_internal.h, so no
consumer has to touch its own code to keep building.
@0xPxt
0xPxt force-pushed the feat/review-fast-path branch from 3f986b2 to dae3f4a Compare September 16, 2026 06:54
@0xPxt
0xPxt merged commit 5680661 into main Sep 16, 2026
7 checks passed
@0xPxt
0xPxt deleted the feat/review-fast-path branch September 16, 2026 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants