Skip to content

feat(lint): report which deployment the ABI checks covered - #342

Open
venugopalanvip wants to merge 2 commits into
LedgerHQ:mainfrom
venugopalanvip:report-abi-validation-coverage
Open

venugopalanvip wants to merge 2 commits into
LedgerHQ:mainfrom
venugopalanvip:report-abi-validation-coverage

Conversation

@venugopalanvip

Copy link
Copy Markdown
Contributor

v2 only. Asked for in ethereum/clear-signing-erc7730-registry#2871 — an explicit per-deployment result for ABI-backed field validation, so an unavailable ABI is distinguishable from a verified pass.

What the linter does today

ValidateDisplayFieldsLinter walks contract.deployments, fetches an ABI, and breaks at the first one that yields one. That single ABI backs three checks: field paths, parameter coverage, and selector exhaustiveness.

Nothing said so. On a successful run the linter emitted no output at all, so these two are indistinguishable in the log:

  • a descriptor with one deployment, validated against it;
  • a descriptor with five deployments, validated against the first and never compared against the other four.

registry/p2p/calldata-P2pSsvProxyFactory.json is the second kind. It declares three deployments; one is checked.

What it reports now

🔵 Reference ABI: Display fields validated against the ABI of
   https://etherscan.io//address/0x5ed861aec31ccb496689fd2e0a1a3f8e8d7b8824#code.
   Not compared against 2 further deployment(s): chain id 560048 address
   0x2444fae9394debf503775940af2a3e9364a31e34, chain id 560048 address
   0x91234ffd7d65aa5e4fda60a2e7b9513175df3272.

Three facts, per deployment: the one that supplied the reference, the ones never reached, and the ones whose ABI was unavailable with the reason each failed.

INFO maps to a GitHub notice annotation, so this is visible in CI rather than only on a terminal.

The give-up path

When no deployment yields an ABI, the old code warned once per failed fetch and then returned bare. Now it emits one warning that names every deployment tried and states what was skipped as a result:

🟠 Display fields not validated against an ABI: No deployment yielded a reference ABI,
   so display field paths, parameter coverage and selector exhaustiveness were not
   checked. Tried 2 deployment(s): chain id 1 address 0x…dead: contract source is not
   available on Sourcify, …; chain id 137 address 0x…beef: …

Two deployments produced two warnings before and produce one now, with the same reasons preserved. Nothing is lost, and the message states the consequence rather than leaving a reader to infer it from N separate fetch errors.

That path also had a genuine silent case. The loop treats get_contract_abis(...) is None as "try the next deployment" without warning — only a raised exception warned. With a client that returns no ABI rather than raising, the linter printed:

no issue found ✔️

on a descriptor whose display fields were never checked against anything. That now reports the warning above. get_contract_abis is annotated -> list[ABI], so this may be unreachable through the current client — but the linter tests for it, so it should account for it rather than fall through to a clean pass.

Severity is deliberately unchanged

Still a warning, not an error. Whether an unavailable ABI should block a merge is a registry policy decision — #2871 is explicit that this should "make the existing manual review requirement visible to tooling" without turning basic review into a semantic audit. Making the gap visible is this library's job; making it fail is the registry's call, and it can now act on a titled annotation instead of parsing prose.

Tests

The message building is extracted into _coverage_message and covered in tests/v2/lint/test_lint_validate_display_fields.py: the reference alone, deployments never compared, an unavailable deployment keeping its reason, and both kinds of gap together.

Verified end to end against calldata-P2pSsvProxyFactory.json for the success path, and against a two-deployment descriptor pointing at unverified addresses for the give-up path.

A v2 contract descriptor is validated against one reference ABI: the linter
fetches deployments in order and stops at the first that yields one. Nothing
said so. On a successful run it emitted no output at all, so a descriptor with
three deployments compared against one looked exactly like a descriptor
compared against all three.

`erc7730 lint` now reports, for a contract context:

- the reference it validated against, by explorer URL;
- the deployments it never compared, because an earlier one supplied the ABI;
- the deployments whose ABI was unavailable, each with the reason.

When no deployment yields an ABI at all, the per-deployment fetch warnings are
replaced by one warning naming every deployment tried and stating which checks
were therefore skipped -- field paths, parameter coverage and selector
exhaustiveness. Previously that final give-up was a bare `return`, so a client
returning no ABI without raising produced a clean "no issue found".

Severity is unchanged: this is still a warning, not an error. Whether an
unavailable ABI should block a merge is a registry policy decision, not one
this library should make.

Asked for in ethereum/clear-signing-erc7730-registry#2871.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Redchar1992

Copy link
Copy Markdown

Thanks @venugopalanvip for investigating and implementing this. Following up here from registry #2871 now that the Stage issue is closed.

Explicitly naming the reference deployment, failed ABI fetches with their reasons, and deployments not compared is the distinction I was asking for. I also agree that the coverage fact belongs to the descriptor/deployment pair, not the descriptor as a whole.

Keeping reporting in the library and merge-blocking policy in the registry makes sense. This is a useful coverage-visibility improvement, not validation of every deployment or a replacement for auditor attestation.

@venugopalanvip

Copy link
Copy Markdown
Contributor Author

Thanks @Redchar1992 — and agreed on the framing: this reports coverage, it does not extend it. One reference ABI still backs the checks; the change is that the log now says which one and what it left out.

On the registry issue being closed — @kuzdogan closed the whole "Stage" series while re-scoping priorities, not this point specifically, so the request stands on its own here.

Branch brought up to date with main (it was behind by the merge of #340).

One thing I left for a maintainer to decide rather than assuming. lint_all_and_print_errors returns True when only warnings are present, so nothing here changes an exit code. If Ledger would rather an unfetchable ABI be an error in this library instead of a registry-side policy step, that is a one-line severity change and I am happy to make it — but it would start failing existing descriptors whose contracts are unverified, so it seemed wrong to decide unilaterally.

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.

2 participants