From 925168c23e66cc62272eefb6176f63d2389b4311 Mon Sep 17 00:00:00 2001 From: venugopalanvip <102638505+venugopalanvip@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:29:08 +0530 Subject: [PATCH] fix(lint): stop reporting lengths that can never truncate 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. This repository is the library clear-signing-erc7730-registry now installs for its `validate descriptors` job, so the noise lands on every contributor there. 1. A hidden label cannot be truncated. `_collect_long_labels` measured every label, including fields the device never draws. Descriptors routinely spell out hidden struct leaves in full -- it is how a reader tells `#._referrerConfig.basisPoints` from `#._clientConfig.basisPoints`. `never` and `mustMatch` are skipped. `ifNotIn` and `optional` are not: both reach a screen at least some of the time. `mustMatch` is worth spelling out because convert_erc7730_v2_input_to_calldata already lets such a field omit its label entirely, so warning about a long one contradicted the converter. 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 -- `{execution.desc.minReturnAmount}` is 32 on its own. Only the literal text is certain to reach the screen, so that is what is measured now. Over the limit on that alone means it truncates whatever the values render to. Measured across every descriptor in the registry at 0fbe25b: long labels on hidden fields 83 warnings, none of which can truncate long labels on visible fields 24 warnings, all kept interpolatedIntent over 30 111, of which 87 are placeholder text only and 24 genuinely too long, all kept Co-Authored-By: Claude Opus 5 --- .../lint/v2/lint_validate_max_length.py | 43 ++++++++- .../v2/lint/test_lint_validate_max_length.py | 89 +++++++++++++++++++ 2 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 tests/v2/lint/test_lint_validate_max_length.py diff --git a/src/erc7730/lint/v2/lint_validate_max_length.py b/src/erc7730/lint/v2/lint_validate_max_length.py index 8a826899..40cbb9fc 100644 --- a/src/erc7730/lint/v2/lint_validate_max_length.py +++ b/src/erc7730/lint/v2/lint_validate_max_length.py @@ -4,6 +4,7 @@ Adapted from v1 ValidateMaxLengthLinter to use v2 model types. """ +import re from typing import final, override from erc7730.common.ledger import ( @@ -18,8 +19,15 @@ from erc7730.common.output import OutputAdder from erc7730.lint.v2 import ERC7730Linter from erc7730.model.input.v2.descriptor import InputERC7730Descriptor +from erc7730.model.input.v2.format import VisibilityRule from erc7730.model.resolved.v2.descriptor import ResolvedERC7730Descriptor -from erc7730.model.resolved.v2.display import ResolvedField, ResolvedFieldDescription, ResolvedFieldGroup +from erc7730.model.resolved.v2.display import ( + ResolvedField, + ResolvedFieldDescription, + ResolvedFieldGroup, + ResolvedVisibilityConditions, + ResolvedVisibilityRules, +) @final @@ -79,8 +87,8 @@ def _validate_display_lengths(cls, descriptor: ResolvedERC7730Descriptor, out: O for fmt in descriptor.display.formats.values(): if fmt.intent is not None and isinstance(fmt.intent, str) and len(fmt.intent) > OPERATION_TYPE_MAX_LENGTH: too_long_intents.add(fmt.intent) - if fmt.interpolatedIntent is not None and len(fmt.interpolatedIntent) > OPERATION_TYPE_MAX_LENGTH: - too_long_intents.add(fmt.interpolatedIntent) + if (ii := fmt.interpolatedIntent) is not None and cls._literal_length(ii) > OPERATION_TYPE_MAX_LENGTH: + too_long_intents.add(ii) if fmt.id is not None and len(fmt.id) > OPERATION_TYPE_MAX_LENGTH: too_long_ids.add(fmt.id) @@ -106,10 +114,39 @@ def _validate_display_lengths(cls, descriptor: ResolvedERC7730Descriptor, out: O f"{FIELD_NAME_MAX_LENGTH} characters and may be truncated on Ledger devices.", ) + @staticmethod + def _is_never_displayed(visible: ResolvedVisibilityRules | None) -> bool: + """Whether the field is only ever checked, never put on a screen. + + A ``mustMatch`` field is a constraint the device enforces without displaying it, which + is why the descriptor may omit its label altogether. ``ifNotIn`` and ``optional`` both + do reach a screen, so their labels still count. + """ + if visible == VisibilityRule.NEVER: + return True + return isinstance(visible, ResolvedVisibilityConditions) and visible.mustMatch is not None + + _PLACEHOLDER = re.compile(r"\{[^}]*\}") + + @classmethod + def _literal_length(cls, interpolated_intent: str) -> int: + """Length of the text an interpolated intent is guaranteed to display. + + `{path}` is template syntax. Neither the braces nor the path inside them reaches 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. Only the literal text around the + placeholders is certain to be displayed, so it is the only part that can be measured. + Over the limit on that alone means the intent truncates whatever the values are. + """ + return len(cls._PLACEHOLDER.sub("", interpolated_intent)) + @classmethod def _collect_long_labels(cls, field: ResolvedField, too_long: set[str]) -> None: match field: case ResolvedFieldDescription(): + # a label that never reaches a screen cannot be truncated on one + if cls._is_never_displayed(field.visible): + return if field.label is not None and len(field.label) > FIELD_NAME_MAX_LENGTH: too_long.add(field.label) case ResolvedFieldGroup(): diff --git a/tests/v2/lint/test_lint_validate_max_length.py b/tests/v2/lint/test_lint_validate_max_length.py new file mode 100644 index 00000000..75025fad --- /dev/null +++ b/tests/v2/lint/test_lint_validate_max_length.py @@ -0,0 +1,89 @@ +import pytest + +from erc7730.lint.v2.lint_validate_max_length import ValidateMaxLengthLinter +from erc7730.model.input.v2.format import VisibilityRule +from erc7730.model.paths.path_parser import to_path +from erc7730.model.resolved.v2.display import ( + ResolvedFieldDescription, + ResolvedFieldGroup, + ResolvedVisibilityConditions, + ResolvedVisibilityRules, +) + +# 28 characters, against the 20-character field name limit +LONG = "Referrer Config Basis Points" + + +def field(visible: ResolvedVisibilityRules | None) -> ResolvedFieldDescription: + return ResolvedFieldDescription(label=LONG, path=to_path("#.referrerConfig.basisPoints"), visible=visible) + + +def collected(visible: ResolvedVisibilityRules | None) -> set[str]: + out: set[str] = set() + ValidateMaxLengthLinter._collect_long_labels(field(visible), out) + return out + + +@pytest.mark.parametrize( + "visible", + [ + pytest.param(VisibilityRule.ALWAYS, id="always"), + pytest.param(VisibilityRule.OPTIONAL, id="optional"), + pytest.param(None, id="unset"), + pytest.param(ResolvedVisibilityConditions(ifNotIn=["0"]), id="ifNotIn"), + ], +) +def test_label_that_can_reach_a_screen_is_reported(visible: ResolvedVisibilityRules | None) -> None: + """always, optional, unset and ifNotIn all display the field at least sometimes.""" + assert collected(visible) == {LONG} + + +@pytest.mark.parametrize( + "visible", + [ + pytest.param(VisibilityRule.NEVER, id="never"), + pytest.param(ResolvedVisibilityConditions(mustMatch=["0"]), id="mustMatch"), + ], +) +def test_label_that_never_reaches_a_screen_is_not_reported(visible: ResolvedVisibilityRules) -> None: + """A hidden label cannot truncate, and a mustMatch field may legitimately carry no label.""" + assert collected(visible) == set() + + +def test_a_group_does_not_hide_the_labels_inside_it() -> None: + """ResolvedFieldGroup carries no visibility of its own, so each child decides for itself.""" + group = ResolvedFieldGroup(fields=[field(VisibilityRule.NEVER), field(VisibilityRule.ALWAYS)]) + + out: set[str] = set() + ValidateMaxLengthLinter._collect_long_labels(group, out) + + assert out == {LONG} + + +@pytest.mark.parametrize( + "intent,expected", + [ + pytest.param("Withdraw {_amounts.[]}", 9, id="one_placeholder"), + pytest.param("Send {sharesAmount} to {recipient}", 9, id="two_placeholders"), + pytest.param("Stake ETH with SSV", 18, id="no_placeholder"), + pytest.param("{_approved} unstETH NFTs", 13, id="leading_placeholder"), + ], +) +def test_literal_length_counts_only_what_is_certain_to_be_displayed(intent: str, expected: int) -> None: + """A `{path}` is template syntax, and the value replacing it has no length known here.""" + assert ValidateMaxLengthLinter._literal_length(intent) == expected + + +def test_a_long_path_inside_a_placeholder_does_not_make_the_intent_long() -> None: + """`{execution.desc.minReturnAmount}` is 32 characters of path that never reaches a screen.""" + intent = "Swap for at least {execution.desc.minReturnAmount}" + + assert len(intent) > 30 + assert ValidateMaxLengthLinter._literal_length(intent) == 18 + + +def test_literal_text_over_the_limit_is_still_measured() -> None: + """Literal text alone over the limit truncates whatever the values render to.""" + intent = "Authorize decryption on behalf of {delegator} for {contracts} for {days} days" + + assert ValidateMaxLengthLinter._literal_length(intent) > 30