Skip to content

fix(lint): stop reporting lengths that can never truncate - #341

Open
venugopalanvip wants to merge 3 commits into
LedgerHQ:mainfrom
venugopalanvip:fix-v2-lint-hidden-label-length
Open

venugopalanvip wants to merge 3 commits into
LedgerHQ:mainfrom
venugopalanvip:fix-v2-lint-hidden-label-length

Conversation

@venugopalanvip

@venugopalanvip venugopalanvip commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

v2 only. Two false positives in ValidateMaxLengthLinter, both the same shape: it measures a string that never reaches a screen against the limit for strings that do. I found them while working through the registry's audit backlog, where the real length warnings are the ones you want to see.

1. A hidden label cannot be truncated

_collect_long_labels measured every label, including fields the device never draws.

$ erc7730 lint registry/p2p/calldata-P2pSsvProxyFactory.json
🟠 Display label too long: `Referrer Config Basis Points` exceeds 20 characters
   and may be truncated on Ledger devices.

That field is "visible": "never". Descriptors routinely spell out hidden struct leaves in full — it is how a reader tells #._referrerConfig.basisPoints from #._clientConfig.basisPoints.

Two rules mean "never drawn" and are now skipped:

rule drawn? measured?
always yes yes
optional when the wallet can yes
unset yes yes
ifNotIn when the value is not in the list yes
never no no
mustMatch no — checked, not shown no

mustMatch is worth spelling out: it is a constraint the device enforces without displaying the field, which is exactly why convert_erc7730_v2_input_to_calldata already lets such a field omit its label —

# A mustMatch field is never displayed, so the descriptor is allowed to omit its label, but
# the FIELD struct still requires a NAME tag.

A linter that permits no label while warning about a long one contradicts the converter.

ifNotIn and optional are deliberately not skipped: both reach a screen at least some of the time.

2. {path} is template syntax, not displayed text

interpolatedIntent was measured as written, so the braces and the path inside them counted toward the 30-character limit. Neither is displayed, and the length of the value that replaces them is not knowable here — an amount may render as 1 ETH or as eighteen decimal places.

Measured across every descriptor in the clear-signing registry:

count
interpolated intents longer than 30 chars as written 109
of those, whose literal text still exceeds 30 24
warnings produced by the template syntax alone 85

78% of these warnings are about characters that never reach a device. The clearest case:

Swap {execution.desc.amount} for at least {execution.desc.minReturnAmount} to recipient {execution.desc.dstReceiver}

116 characters as written, 33 of literal text. {execution.desc.minReturnAmount} alone is 32 characters of path.

A real one that stops warning — registry/lido/calldata-stETH-L2.json:

Send {sharesAmount} to {recipient}      34 as written, 9 displayed

Only the literal text is certain to reach the screen, so that is what is measured now. Over the limit on that alone means the intent truncates whatever the values render to — conservative, and it keeps all 24 genuine cases, including the two examples above.

Severity and scope

Both remain warnings; nothing changes an exit code. Neither touches intent, id, metadata or enum lengths, which are literal strings and were always measured correctly.

Tests

tests/v2/lint/test_lint_validate_max_length.py is new: all six visibility rules, a ResolvedFieldGroup case proving a hidden child does not hide its visible sibling's label, and the literal-length measure including a 32-character path inside a placeholder and a literal that is genuinely over.

Verified end to end: the two warnings quoted above are gone, a long label on a visible field still reports, and a literal-only intent over 30 still reports.

`_collect_long_labels` measured every label against the 20-character device
limit, including labels on fields the device never draws. Those cannot be
truncated, because they never reach a screen.

It fires today. `erc7730 lint` on
`registry/p2p/calldata-P2pSsvProxyFactory.json` in the clear-signing registry
reports "Display label too long" for `Referrer Config Basis Points`, a field
carrying `"visible": "never"`. Descriptors routinely name hidden struct leaves
in full, so the warning is noise that competes with the real ones.

Two rules are skipped now:

- `never` — never drawn.
- `mustMatch` — a constraint the device enforces without displaying the field,
  which is why `convert_erc7730_v2_input_to_calldata` lets such a field omit
  its label entirely.

`ifNotIn` and `optional` are not skipped: both put the field on a screen at
least some of the time, so their labels can still truncate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@venugopalanvip
venugopalanvip requested a review from a team as a code owner September 15, 2026 10:18
venugopalanvip and others added 2 commits September 16, 2026 13:12
`interpolatedIntent` was measured as written, so `{path}` counted toward the
30-character limit. The braces and the path inside them are template syntax and
never reach a screen, and the length of the value that replaces them is not
known here -- an amount may render as "1 ETH" or as eighteen decimal places.

Across the clear-signing registry, 109 interpolated intents are longer than 30
characters as written. Only 24 have literal text that still exceeds the limit.
The other 85 warnings measure `{execution.desc.minReturnAmount}` and the like:
32 characters of path that is never displayed.

Only the literal text around the placeholders is certain to reach the screen, so
that is what is measured now. Over the limit on that alone means the intent
truncates whatever the values render to, which keeps the check conservative and
keeps the 24 real cases.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@venugopalanvip venugopalanvip changed the title fix(lint): do not warn that a hidden label may be truncated fix(lint): stop reporting lengths that can never truncate Sep 16, 2026
@venugopalanvip

Copy link
Copy Markdown
Contributor Author

Added a second false positive from the same linter in 67d66b2, and retitled accordingly — it is the same shape as the first, so it seemed better as one review than a second PR touching the same method.

interpolatedIntent was measured as written, so {path} counted toward the 30-character limit. Across the registry, 109 interpolated intents exceed 30 characters as written and only 24 have literal text that still does — so 85 of those warnings are about braces and path text that never reach a device. {execution.desc.minReturnAmount} is 32 characters on its own.

Only the literal text is measured now. Over the limit on that alone means the intent truncates whatever the values render to, so the check stays conservative and keeps all 24 genuine cases.

Happy to split this back out if you would rather review them separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant