Skip to content

feat(nbgl): size the review chunk from the item's own text, not the worst glyph - #238

Merged
chcmedeiros merged 1 commit into
mainfrom
feat/review-value-fill-pages
Sep 10, 2026
Merged

chcmedeiros merged 1 commit into
mainfrom
feat/review-value-fill-pages

Conversation

@chcmedeiros

@chcmedeiros chcmedeiros commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #237, which closed the truncation hole but paid for it in density.

The cost #237 left behind

v52.1.0 sizes the chunk so that no arrangement of characters can overflow a review page, by assuming every character is as wide as the widest glyph the font can draw (31px on Stax). That is always safe and almost always wrong. Hex and bech32 run about 19 characters to a line, so a page carried 110 characters where it had room for 159, and filled six of its ten lines.

Measure the item instead of assuming the worst

Start from what the value buffer holds and shrink until no page of that item overflows its lines, asking the same question NBGL asks when it breaks its own details pages — how much of this text fits in this many lines at this width (nbgl_getTextMaxLenInNbLines). Cached per item next to the page count, so the search runs once per review.

Measured on Stax, 200-character values:

content chars per page lines used complete
hex / bech32 110 → 159 6 → 9 200/200
digits 110 → 159 7 → 10 200/200
wide glyphs 110 (unchanged) 10 200/200

Nothing is truncated in any of them. The search is constrained by the same fit check that #237 introduced, and #237's budget remains the floor whenever the search cannot settle. The per-page guard stays as the backstop.

For the apps

ledger-cosmos comes back byte for byte. The wasm_boundary_test snapshots return to exactly what they were before v52.1.0 split them across an extra page — verified with cmp, 6 pages, 0 differing frames. So for this app the security fix becomes invisible to normal content, and the snapshot churn #237 caused is undone rather than blessed.

Full suite, all five devices: 179 of 183, the four being the three expected wasm_boundary_test page-count reverts plus one Timeout waiting to connect that passes on re-run.

…orst glyph

v52.1.0 sized the chunk so that no arrangement of characters could overflow a
review page, by assuming every character is as wide as the widest glyph the font
can draw. That is always safe and almost always wrong: hex and bech32 run about
19 characters to a line, so a page carried 110 characters where it had room for
159 and filled six of its ten lines.

Measure the item instead of assuming the worst. Start from what the value buffer
holds and shrink until no page of that item overflows its lines, using the same
question NBGL asks when it breaks its own details pages -- how much of this text
fits in this many lines at this width. The answer is cached per item alongside
the page count, so the search runs once per review.

The chunk length still has to be uniform across an item's pages, because the app
derives each page's offset from it. Breaking every page exactly where its text
stops, as the SDK does, needs a value the app does not hand over one chunk at a
time; that is a wider change than this.

  hex / bech32   110 -> 159 chars per page, 6 -> 9 lines
  digits         110 -> 159 chars per page, 7 -> 10 lines
  wide glyphs    110 chars per page, unchanged, still 10 lines

Nothing is truncated in any of those: the safety the previous release bought is
what the search is constrained by, and its budget remains the floor whenever the
search cannot settle. The per-device guard from v52.1.0 stays as the backstop.

For ledger-cosmos this restores the review byte for byte -- the wasm boundary
snapshots come back to what they were before v52.1.0 split them across an extra
page.
@chcmedeiros
chcmedeiros merged commit 74c1be0 into main Sep 10, 2026
11 checks passed
@chcmedeiros
chcmedeiros deleted the feat/review-value-fill-pages branch September 10, 2026 14:27
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