Refactoring erc7730.py for logging and readability. - #76
Open
PrisionMike wants to merge 1 commit into
Open
Conversation
…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>
WalkthroughERC-7730 parsing was reorganized into staged ABI, path, field, and deployment processing. Unsupported features now raise categorized errors, allowing individual signatures to be dropped while diagnostics are deduplicated. Field construction adds formatter compatibility checks, constants, Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Descriptor
participant Parser as ERC-7730 parser
participant FieldBuilder as Field builders
participant Deployment as Deployment expansion
Descriptor->>Parser: Load display formats
Parser->>FieldBuilder: Build each signature and resolve fields
FieldBuilder-->>Parser: Candidate, adjustments, or categorized error
Parser->>Deployment: Expand valid candidates
Deployment-->>Descriptor: Display formats and deployment records
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eth_definitions/erc7730.py (1)
348-351: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ArrayElementon a non-array leaf is silently accepted.
.[]iteration over a non-array now raisesiteration-over-non-array, but an explicit index (x.[0]onuint256 x) still appends the index and returns a scalar kind, so a path that walks past a scalar leaf is emitted. Consider the same guard for consistency.🛠️ Proposed guard
elif isinstance(element, ArrayElement): + if leaf_array_depth <= 0: + raise UnsupportedFeature("iteration-over-non-array", path_str) indices.append(element.index) - if leaf_array_depth > 0: - leaf_array_depth -= 1 # indexing peels one array dimension + leaf_array_depth -= 1 # indexing peels one array dimension🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth_definitions/erc7730.py` around lines 348 - 351, Update the ArrayElement handling in the path traversal logic to reject explicit indexing when leaf_array_depth is zero, raising the same iteration-over-non-array error used for non-array iteration. Only append the index and decrement leaf_array_depth when an array dimension is available, preventing scalar leaves from producing invalid paths.
🧹 Nitpick comments (1)
eth_definitions/test_erc7730.py (1)
407-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name doesn't match the case. The descriptor is a whole
uint256[]array, not a tuple; rename to something liketest_addressname_on_whole_array_still_skips_file(or add a tuple case).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth_definitions/test_erc7730.py` around lines 407 - 417, Rename test_addressname_on_tuple_still_skips_file to reflect that it covers a whole uint256[] array, such as test_addressname_on_whole_array_still_skips_file; leave the descriptor and assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eth_definitions/erc7730.py`:
- Around line 1105-1110: Add an early type guard in _build_one_format to
validate that display_format is a dictionary before calling mapping methods such
as get. For non-dict values, append the appropriate malformed-format issue for
the signature and return without building fields, so only that signature is
dropped while valid formats continue processing.
- Around line 943-971: Update _apply_threshold so every normalized threshold hex
string has an even number of digits before assigning out["threshold"], including
resolved/string values and nonnegative integer values. Zero-pad odd-length
normalized values while preserving existing invalid-character and negative-value
rejection.
---
Outside diff comments:
In `@eth_definitions/erc7730.py`:
- Around line 348-351: Update the ArrayElement handling in the path traversal
logic to reject explicit indexing when leaf_array_depth is zero, raising the
same iteration-over-non-array error used for non-array iteration. Only append
the index and decrement leaf_array_depth when an array dimension is available,
preventing scalar leaves from producing invalid paths.
---
Nitpick comments:
In `@eth_definitions/test_erc7730.py`:
- Around line 407-417: Rename test_addressname_on_tuple_still_skips_file to
reflect that it covers a whole uint256[] array, such as
test_addressname_on_whole_array_still_skips_file; leave the descriptor and
assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f349d88-2f44-4042-938b-6230517217bc
📒 Files selected for processing (2)
eth_definitions/erc7730.pyeth_definitions/test_erc7730.py
Comment on lines
+943
to
+971
| def _apply_threshold( | ||
| out: ERC7730Field, params: dict[str, Any], label: str, constants: dict[str, Any] | ||
| ) -> None: | ||
| """Normalize a `threshold` param to hex bytes; reject the unserializable.""" | ||
| threshold = params.get("threshold") | ||
| if isinstance(threshold, str) and threshold.startswith("$"): | ||
| resolved = _resolve_constant(threshold, constants) | ||
| if resolved is None: | ||
| raise UnsupportedFeature( | ||
| "unresolvable-threshold", f"{threshold} (field {label!r})" | ||
| ) | ||
| threshold = resolved | ||
| if isinstance(threshold, str): | ||
| normalized = _normalize_hex(threshold) | ||
| # `_normalize_hex` doesn't validate: a non-hex value would slip through | ||
| # and crash `bytes.fromhex` at serialization time. Reject it here. | ||
| if set(normalized) - _HEX_DIGITS: | ||
| raise UnsupportedFeature( | ||
| "invalid-threshold", f"{threshold!r} (field {label!r})" | ||
| ) | ||
| out["threshold"] = normalized | ||
| elif isinstance(threshold, int): | ||
| # A negative threshold has no byte encoding (`hex(-n)` yields a | ||
| # `-0x…` string that also breaks `bytes.fromhex`). | ||
| if threshold < 0: | ||
| raise UnsupportedFeature( | ||
| "invalid-threshold", f"{threshold} (field {label!r})" | ||
| ) | ||
| out["threshold"] = _normalize_hex(hex(threshold)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Odd-length hex thresholds still break bytes.fromhex at serialization.
The validation covers non-hex chars and negatives, but not parity. hex(4095) → "fff", and a descriptor string "0xfff" normalizes to "fff"; both pass the digit check and then raise ValueError: non-hexadecimal number found when serialized. Zero-pad to an even length.
🛠️ Proposed fix
if isinstance(threshold, str):
normalized = _normalize_hex(threshold)
# `_normalize_hex` doesn't validate: a non-hex value would slip through
# and crash `bytes.fromhex` at serialization time. Reject it here.
if set(normalized) - _HEX_DIGITS:
raise UnsupportedFeature(
"invalid-threshold", f"{threshold!r} (field {label!r})"
)
- out["threshold"] = normalized
+ # `bytes.fromhex` needs full bytes.
+ out["threshold"] = normalized.zfill(len(normalized) + len(normalized) % 2)
elif isinstance(threshold, int):
# A negative threshold has no byte encoding (`hex(-n)` yields a
# `-0x…` string that also breaks `bytes.fromhex`).
if threshold < 0:
raise UnsupportedFeature(
"invalid-threshold", f"{threshold} (field {label!r})"
)
- out["threshold"] = _normalize_hex(hex(threshold))
+ out["threshold"] = f"{threshold:0{2 * ((threshold.bit_length() + 7) // 8) or 2}x}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _apply_threshold( | |
| out: ERC7730Field, params: dict[str, Any], label: str, constants: dict[str, Any] | |
| ) -> None: | |
| """Normalize a `threshold` param to hex bytes; reject the unserializable.""" | |
| threshold = params.get("threshold") | |
| if isinstance(threshold, str) and threshold.startswith("$"): | |
| resolved = _resolve_constant(threshold, constants) | |
| if resolved is None: | |
| raise UnsupportedFeature( | |
| "unresolvable-threshold", f"{threshold} (field {label!r})" | |
| ) | |
| threshold = resolved | |
| if isinstance(threshold, str): | |
| normalized = _normalize_hex(threshold) | |
| # `_normalize_hex` doesn't validate: a non-hex value would slip through | |
| # and crash `bytes.fromhex` at serialization time. Reject it here. | |
| if set(normalized) - _HEX_DIGITS: | |
| raise UnsupportedFeature( | |
| "invalid-threshold", f"{threshold!r} (field {label!r})" | |
| ) | |
| out["threshold"] = normalized | |
| elif isinstance(threshold, int): | |
| # A negative threshold has no byte encoding (`hex(-n)` yields a | |
| # `-0x…` string that also breaks `bytes.fromhex`). | |
| if threshold < 0: | |
| raise UnsupportedFeature( | |
| "invalid-threshold", f"{threshold} (field {label!r})" | |
| ) | |
| out["threshold"] = _normalize_hex(hex(threshold)) | |
| def _apply_threshold( | |
| out: ERC7730Field, params: dict[str, Any], label: str, constants: dict[str, Any] | |
| ) -> None: | |
| """Normalize a `threshold` param to hex bytes; reject the unserializable.""" | |
| threshold = params.get("threshold") | |
| if isinstance(threshold, str) and threshold.startswith("$"): | |
| resolved = _resolve_constant(threshold, constants) | |
| if resolved is None: | |
| raise UnsupportedFeature( | |
| "unresolvable-threshold", f"{threshold} (field {label!r})" | |
| ) | |
| threshold = resolved | |
| if isinstance(threshold, str): | |
| normalized = _normalize_hex(threshold) | |
| # `_normalize_hex` doesn't validate: a non-hex value would slip through | |
| # and crash `bytes.fromhex` at serialization time. Reject it here. | |
| if set(normalized) - _HEX_DIGITS: | |
| raise UnsupportedFeature( | |
| "invalid-threshold", f"{threshold!r} (field {label!r})" | |
| ) | |
| # `bytes.fromhex` needs full bytes. | |
| out["threshold"] = normalized.zfill(len(normalized) + len(normalized) % 2) | |
| elif isinstance(threshold, int): | |
| # A negative threshold has no byte encoding (`hex(-n)` yields a | |
| # `-0x…` string that also breaks `bytes.fromhex`). | |
| if threshold < 0: | |
| raise UnsupportedFeature( | |
| "invalid-threshold", f"{threshold} (field {label!r})" | |
| ) | |
| out["threshold"] = f"{threshold:0{2 * ((threshold.bit_length() + 7) // 8) or 2}x}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@eth_definitions/erc7730.py` around lines 943 - 971, Update _apply_threshold
so every normalized threshold hex string has an even number of digits before
assigning out["threshold"], including resolved/string values and nonnegative
integer values. Zero-pad odd-length normalized values while preserving existing
invalid-character and negative-value rejection.
Comment on lines
+1105
to
+1110
| ctx = _FormatContext(inputs, constants, parameter_definitions) | ||
| field_defs: list[ERC7730Field] = [] | ||
| for f in display_format.get("fields", []): | ||
| if not isinstance(f, dict): | ||
| issues.append(("malformed-field-entry", f"{sig_key}: {f!r}")) | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Non-dict display_format crashes the whole file instead of dropping one signature.
Field entries are validated (malformed-field-entry), but the format value itself isn't: a registry entry like "f(uint256 x)": null or a list raises AttributeError out of build_display_formats, killing the run rather than dropping that signature.
🛠️ Proposed guard (place near the top of `_build_one_format`)
issues: list[tuple[str, str]] = []
+ if not isinstance(display_format, dict):
+ issues.append(("malformed-format-entry", f"{sig_key}: {display_format!r}"))
+ return None, issues, []
if sig_key.startswith("0x"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@eth_definitions/erc7730.py` around lines 1105 - 1110, Add an early type guard
in _build_one_format to validate that display_format is a dictionary before
calling mapping methods such as get. For non-dict values, append the appropriate
malformed-format issue for the signature and return without building fields, so
only that signature is dropped while valid formats continue processing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rework the descriptor parser for reviewability and fold in the reinterpret- instead-of-skip semantics:
Structure (behavior-preserving):
_FormatContextreplaces six threaded parameters;ctx.adjust()records accepted-but-modified fields._build_one_formatreturns (candidate, issues, adjustments) — thenonlocal had_issueclosure flag is gone; "issues non-empty means drop" is now the visible contract._build_path_fieldorchestrates, with_check_kind_or_reinterpret,_apply_token_amount_params,_apply_threshold,_apply_unit_params,_apply_date_paramsand_build_non_path_field/_const_value_fieldeach owning one concern._expand_deployments.Semantics (each logged as an adjustment or a distinct drop reason):
_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).