Clear signing: readable parser rework — reinterpret instead of skip - #68
Clear signing: readable parser rework — reinterpret instead of skip#68PrisionMike wants to merge 6 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PrisionMike
left a comment
PrisionMike
left a comment
There was a problem hiding this comment.
Self-review: inline notes on the parts worth the most reviewer attention. Suggested reading order: module docstring -> section 5 (_build_one_format) -> section 4b (the retype) -> the per-formatter helpers.
| # raises on-device, which degrades safely to blind signing). | ||
| # --------------------------------------------------------------------- | ||
|
|
||
|
|
There was a problem hiding this comment.
This block is the crux of the PR — why a uint256 leaf may be retyped to address.
The short version: the descriptor author says "render this uint as an address" (addressName), the firmware formatter raises on int, and the only lever the parser has is the declared ABI type. Since uintN and address are both one static 32-byte word, the retype cannot move or corrupt any other decoded value — worst case the word isn't a clean address at runtime and the firmware's parse_address padding check aborts clear-signing into the ordinary blind-signing flow.
Real registry cases this recovers: 1inch uniswapV3Swap(..., uint256[] pools) ("Last pool" field), 1inch NativeOrderFactory makerOrder.receiver.
| represent, so the caller skips the whole descriptor file rather than emit a | ||
| display format with a missing field. | ||
| A path may also point at a whole array (trailing `.[]` iteration, no | ||
| index appended): the firmware formats each element, so the element type |
There was a problem hiding this comment.
_abi_leaf_at is the mechanical half (pure tree walk, no mutation); _retype_uint_leaf_as_address below is the policy half. The docstring's worked example is the fastest way to convince yourself the index semantics match path_to_dict: names/tuple fields consume an index into fields, array indices step into the shared element type, and a trailing .[] iteration leaves the path on the array so the final unwrap loop lands on the element.
| `pools.[-1]` retypes every `pools` element — and any other field reading | ||
| the same leaf sees an address from then on. That matches the author's | ||
| intent (the whole array holds packed addresses) and is logged by the | ||
| caller as an adjustment. |
There was a problem hiding this comment.
Note the deliberate side effect called out here: arrays have ONE shared element type, so retyping pools.[-1] retypes every pools element, and any other field reading the same leaf (e.g. a raw view of pools.[0]) now sees an address. The early-return for ABI_ADDRESS is what makes two addressName fields on the same leaf idempotent.
|
|
||
| Returns `(candidate, issues, adjustments)`: | ||
| * `candidate` — the buildable display format, or None; | ||
| * `issues` — every unsupported feature hit, as (feature, detail). |
There was a problem hiding this comment.
The contract that replaced nonlocal had_issue: _build_one_format returns (candidate, issues, adjustments) and issues non-empty means the format is dropped. Two properties worth checking:
- The field loop keeps scanning after the first issue, so the log lists everything wrong with a signature, not just the first find.
adjustmentsare returned only whenissuesis empty — a dropped format's retypes die with itsparameter_definitions, so the log never reports an adjustment for something that wasn't emitted.
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| class _FormatContext: |
There was a problem hiding this comment.
_FormatContext is scoped to ONE display format (one signature): parameter_definitions is rebuilt per signature, so an in-place retype can never leak across signatures or files. This replaces the six-parameter threading through the old build_field_dict.
|
Second commit added: trailing byte-slice support, reconciling with the Highlights: Note for reviewers: this makes the PR require a trezorlib built from the |
059b9c1 to
e138757
Compare
|
Amended the firmware-reconcile commit (force-pushed): it now also covers the |
…tations Rework the descriptor parser for reviewability and fold in the reinterpret- instead-of-skip semantics: Structure (behavior-preserving): * Module docstring states the pipeline and the two outcomes every field has: DROP (UnsupportedFeature, format dropped whole) or ADJUST (kept, bent, logged). Sections 1-5 follow the pipeline order. * `_FormatContext` replaces six threaded parameters; `ctx.adjust()` records accepted-but-modified fields. * `_build_one_format` returns (candidate, issues, adjustments) — the `nonlocal had_issue` closure flag is gone; "issues non-empty means drop" is now the visible contract. * The 265-line build_field_dict is split: `_build_path_field` orchestrates, with `_check_kind_or_reinterpret`, `_apply_token_amount_params`, `_apply_threshold`, `_apply_unit_params`, `_apply_date_params` and `_build_non_path_field`/`_const_value_field` each owning one concern. * Deployment expansion extracted to `_expand_deployments`. Semantics (each logged as an adjustment or a distinct drop reason): * addressName on a uint leaf retypes the ABI leaf to ABI_ADDRESS in place (`_abi_leaf_at` + `_retype_uint_leaf_as_address`; uintN and address share the one-word static layout, dirty values degrade to blind signing). Same for tokenAmount's tokenPath at a packed uint. addressName on bytes passes through (firmware renders hex). * Constant fields extend build_const_field (which this replaces): non-string values stringify, $.metadata.constants.* works as a field path, and each failure mode has its own drop reason instead of a silent None. * path_to_dict raises a distinct UnsupportedFeature per drop reason (per-element-field-path, array-slice-path, unknown-path-segment, iteration-over-non-array, unparseable-path, descriptor-path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reconcile with the prisionmike/clear-signing/bytesN-types and sliced-paths firmware branches. bytesN-types (ABI_BYTES20 / ABI_INT160 in the proto enum): * _ABI_TYPE_MAP emits bytes20 and int160. bytes20 classifies as a bytes-like scalar; int160 as numeric (decodes to int on-device). The uint->address retype deliberately does NOT apply to the signed int160. * Recovers the tBTC/threshold descriptors (all 7 remaining unrepresentable-params drops were bytes20). sliced-paths (slice_start/slice_end sint32 on EthereumERC7730Path; the walker views a sliced word as 32 big-endian bytes, DateFormatter accepts sliced bytes as a big-endian timestamp): * path_to_dict accepts one trailing slice on a scalar leaf. A statically 20-byte slice (`token.[-20:]`, the 1inch packed-address pattern) classifies KIND_ADDRESS — addressName and tokenPath work with no retype and no adjustment; other slices classify KIND_BYTES. * `date` over a sliced word is allowed (exact firmware semantics). * Still dropped, each with a distinct reason: slices of arrays/tuples (selects elements, not bytes), non-trailing slices, slices after `.[]` (the firmware would slice the array rather than each element), and out-of-sint32 bounds. * common.py carries the slice bounds on the data-path variant and the fail-fast proto guard now checks for slice_start. 1inch AggregationRouterV6 alone recovers 247 records (364 sliced fields); its only remaining drops are `enum` formatters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The calldata-formatter firmware branch renders a `calldata` field as the nested call it embeds: it resolves the callee address from `callee_path`, fetches that contract's display format, and expands the field into the nested call's rows. `selector` (4 bytes) marks args-only embedded calldata. * `calleePath` is required (the firmware raises without it) and resolves like tokenPath — address kind, or a packed uint retyped to address (logged as callee-address-in-numeric). * Registry extras with no proto representation (amountPath, spenderPath) are dropped from the field and logged as calldata-params-ignored; the nested call still renders in full via the callee's display format. * Distinct drop reasons: calldata-missing-callee, unresolvable-callee-path, invalid-calldata-selector. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The enum-support firmware branch adds FORMATTER_ENUM and an on-device EnumFormatter: the decoded calldata value is a key into a descriptor- supplied mapping (EthereumERC7730EnumEntry, uint32 key -> display string); a key missing from the mapping fails clear signing into blind signing rather than risk showing a wrong value. * `params.$ref` resolves against `$.metadata.enums.<name>`; keys must fit uint32. The JSON-boolean variant (True/False keys over a bool value, flyingtulip) maps to 1/0 — on-device the decoded bool compares equal — logged as enum-bool-keys. * Every accepted enum field is logged (enum-field adjustment) so use of the feature is visible while firmware support is fresh. * Distinct drop reasons: enum-missing-ref, unresolvable-enum-ref, invalid-enum-entry. Recovers the aave interest-rate-mode, okx swap-direction, celo governance and flyingtulip enum fields (19 unsupported-formatter drops). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e138757 to
f119fe8
Compare
|
Rebuilt the branch history (force-push): rebased onto |
The registry's `visible` vocabulary is always / never / optional, plus the spec's rule objects (ifNotIn / mustMatch). `optional` means "wallets MAY display this field if possible or sensible" (ERC-7730) — hiding it is spec-compliant, so it is skipped like `never` and the choice is logged (optional-field-hidden adjustment). The rule objects are true conditionals the firmware cannot honor either way — mustMatch even carries a validation duty — so they drop the display format with their own reason (conditional-visibility) instead of being displayed or hidden unconditionally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The firmware shows the descriptor's provider next to the rendered call (EthereumDisplayFormatInfo.provider_name, externalise-provider-name). `metadata.owner` is authoritative; ownerless descriptors (1inch, safe, …) fall back to "<registry subdirectory>: <metadata.contractName>", or just the subdirectory. Unset when nothing is known. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f119fe8 to
723a9e3
Compare
|
Amended the visibility commit (force-pushed): |
1 participant
Based on
main(#66 landed). The registry-ingestion half is stacked on top as #69.Reviewer's guide
The parser file is rewritten top-to-bottom, so review it as a fresh read, not a diff. The module docstring is the map: it draws the pipeline and defines the two outcomes every descriptor field has — DROP (raise
UnsupportedFeaturewith a distinct feature tag; the whole display format for that signature is dropped, we never emit a format with a field silently missing) and ADJUST (the field is kept but deliberately bent into something the firmware can render, and every bend is logged). Every function serves one of those two verdicts, so it is always clear when a feature was seen and when a field was rejected and why.One commit per concern, in review order:
_FormatContextreplaces parameter threading;_build_one_formatreturns(candidate, issues, adjustments)(issues non-empty ⇒ dropped), replacing the oldnonlocal had_issueflag. Includes the uint→address ABI retype foraddressName/tokenPathover packed uints (uintNandaddressshare the one-word static layout; dirty values degrade to blind signing on-device) and const-value handling supersedingbuild_const_field.bytesN-typesandsliced-pathsfirmware branches. A statically 20-byte slice (token.[-20:]) classifies as address, so 1inch packed-address fields need no retype and no adjustment. Recovers tBTC (bytes20) and all 1inch V5/V6 slice fields.calldata-formatterfirmware branch:callee_path(required; resolves like tokenPath, incl. packed-uint retype) and 4-byteselectorfor args-only embedded calldata. Registry extras (amountPath,spenderPath) have no proto representation and are logged ascalldata-params-ignored.enum-supportfirmware branch:params.$ref → $.metadata.enums.*resolved intoenum_values(uint32 key → display string).True/Falsekeys map to 1/0 (enum-bool-keysadjustment); every accepted enum field is logged (enum-field).visible: optionalmeans "wallets MAY display this field if possible or sensible", so it is hidden likeneverand logged (optional-field-hidden); the spec's rule objects (ifNotIn/mustMatch) are true conditionals the firmware cannot honor and drop the format (conditional-visibility).metadata.owner, falling back to<registry subdir>: <contractName>or the subdir, emitted on every record.Requires
A trezorlib carrying the union of the
calldata-formatterandenum-supportfirmware branches (FORMATTER_CALLDATA/callee_path/selector+FORMATTER_ENUM/enum_values, plusslice_start/slice_endandprovider_name). The fail-fast guard incommon.pynames exactly what's missing.Numbers (with #69's ingestion, registry
5a67786)3590 records emitted, all serializing — 32 calldata fields, 162 enum fields, 709 slice-bearing field defs, provider_name on every record, 28 optional fields hidden (logged). Remaining drops:
nftNameand misc formatters (8), nested field groups, deep ABI shapes.🤖 Generated with Claude Code