feat(prs-556): integrate MetadataTrustPolicy into Ethereum converter

[pepe-anchor]

Why am I making this PR?

The Ethereum converter needs to honour the deploy-time MetadataTrustPolicy: drop unsigned caller ABIs under require-signed, accept them under accept-unsigned while still verifying integrity of present signatures.

What am I changing?

Thread the policy through try_extract_from_chain_metadata. Under AcceptUnsigned, entries with no signature are accepted and present signatures are verified for integrity (signer identity not checked). Under RequireAllowlistedSigner, unsigned entries are dropped and signatures must match an allowed key. Remove the AbiExtraction wrapper struct; the function now returns Option<AbiRegistry>. Update CLI plugin to construct the require-signed posture.

This commit also introduces abi_metadata::signer_allowlist_from_hex, which the parser_app commit on top of this one consumes. That dependency is why this PR sits below parser_app in the stack rather than above it, which is how the original split had it.

Two follow-up commits from review:

docs(prs-556) removes a false claim. Four doc comments asserted, in the present tense, that parser_app and the gRPC server already build their posture from a cmdline flag carried in the signed TVC manifest's pivotArgs. They don't. parser/app/src/registry.rs calls EthereumVisualSignConverter::new() (AcceptUnsigned) and nothing under src/parser/ references MetadataTrustPolicy at all. Out-of-band verifiability is the whole point of this type, so shipping a doc that promises an audit mechanism which isn't wired yet is worse than shipping no doc. The wording now says the cmdline wiring is a planned follow-up. The same commit also spells out in new()'s doc that dropping the signer identity check is a real behaviour change: an entry signed by an unlisted key is now accepted where it used to be rejected.

fix(prs-556) restores the provenance counter. The loop had been simplified to count only abi.signature.is_none(), which dropped the case that actually matters: under AcceptUnsigned an attacker can sign with their own key, integrity verifies, identity is never checked, and the entry stopped counting toward the "provenance unverified" warning. A request whose entire decode came from attacker-self-signed ABIs reported zero unverified mappings. The predicate is back to policy.signer_allowlist().is_none() || abi.signature.is_none(), unsigned_count is renamed unverified_count, and the warning text changed to match.

What is the Linear ticket?

PRS-556

What are the rollback steps?

Revert the commits.

Is this change backwards compatible?

The public API changes (AbiExtraction removed, try_extract_from_chain_metadata return type narrowed). Callers are internal to the workspace.

Behaviour change worth calling out for anyone using the library default: EthereumVisualSignConverter::new() is AcceptUnsigned, and under that posture a present signature is still verified for integrity but its signer is no longer checked against an allowlist. An entry signed by an unlisted key is accepted where it previously was rejected. Deployments that want an auditable posture must construct via with_policy with RequireAllowlistedSigner.

Does this require cross-team/service coordination?

No.

How do I know it works as designed? Which tests exercise this code?

Existing converter tests updated for the new signature. Test helpers (require_signed_converter) constructed from cfg(test) dev keys.

A third commit, test(prs-556), pins that predicate. The counter only feeds a log::warn and no test asserted on it, which is how the regression got in unnoticed. Rather than pull in a log-capture dev-dependency for one assertion, the inline expression is lifted into a named identity_unverified function and tested directly, four tests covering the full truth table. Checked that the regression test actually bites: narrowing the predicate back to abi.signature.is_none() fails identity_unverified_counts_self_signed_entries_under_accept_unsigned and leaves the other three green.

Verified at ded916b: cargo fmt --all --check clean, cargo clippy --all-targets -- -D warnings clean, make -C src test green (39 test binaries, 0 failures).

[Copilot]

Pull request overview

Integrates deploy-time ABI metadata trust policies into Ethereum conversion.

Changes:

• Applies permissive or allowlisted-signer ABI validation policies.
• Simplifies ABI extraction to return Option<AbiRegistry>.
• Adds deploy-time signer-key parsing and tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/visualsign/src/signing.rs	Makes the trust policy non-exhaustive.
src/chain_parsers/visualsign-ethereum/src/lib.rs	Threads policy-aware ABI extraction into conversion.
src/chain_parsers/visualsign-ethereum/src/cli_plugin.rs	Configures strict ABI signing for the CLI.
src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs	Implements policy validation, signer parsing, and tests.

Suppressed comments (1)

src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs:405

• This repeats that parser_app and gRPC already parse these flags, but the only production construction site still uses EthereumVisualSignConverter::new() (src/parser/app/src/registry.rs:20). The new helper is currently unused outside this module, so document it as intended for the follow-up parser_app change.

/// **This is not how a deployment picks its trust posture.** `parser_app` and the
/// gRPC server take the posture from their cmdline
/// (`--accept-unsigned-abis` / `--accept-signatures-from-pubkey`, parsed into a
/// [`MetadataTrustPolicy`] via [`signer_allowlist_from_hex`]) so the choice is
/// auditable in the signed deployment manifest and cannot be influenced per
/// request. This function backs `parser_cli`, which signs the ABI files it loads

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/chain_parsers/visualsign-ethereum/src/cli_plugin.rs:127

• This describes allowlist checking as current production behavior, but parser_app still constructs EthereumVisualSignConverter::new() (src/parser/app/src/registry.rs:20), which uses AcceptUnsigned and deliberately skips signer identity checks. Since the deploy-time wiring is only planned, describe strict production validation as a future requirement rather than an existing guarantee.

            // that already trusts its input files; production trust comes from the
            // service running the parser, which validates the signature and checks the
            // signer's public key against an allowlist during extraction (see
            // `abi_metadata::try_extract_from_chain_metadata`), not from the gRPC
            // caller.

[shahan-khatchadourian-anchorage]

Verified by mutation-testing the branch (cargo test -p visualsign-ethereum --lib, baseline 281 green).

Good: test_register_installs_require_signed_posture goes through the plugin, so a revert to new() fails it. The identity-check relaxation in new() is stated, not buried. signer_allowlist_from_hex canonicalizes key encodings and errors on a typo instead of shrinking fail-closed. if let->match and the hoisted hint are behaviour-preserving.

abi_metadata.rs:1203 - tampering test is vacuous. The swapped body [{"type":"function","name":"approve"}] doesn't deserialize (missing field inputs; alloy-json-abi gives inputs/outputs no serde default), so the entry dies at register_embedded_abi, not at the signature check. Deleting all signature validation from the extraction loop leaves this test green - only test_try_extract_unauthorized_signer_still_rejected fails. With a well-formed swapped body it does fail, so restoring something like the removed OTHER_VALID_ABI is the whole fix. As it stands nothing end-to-end asserts integrity under AcceptUnsigned, the posture parser_app runs.

abi_metadata.rs:167 - the guard misses the site that regressed. The four new tests pin the extracted helper; 128046b7 fixed an inline expression at the call site. Narrowing line 167 back to abi.signature.is_none() leaves all 281 tests green, and so does deleting the unverified_count increment outright. The two deleted end-to-end tests were what watched the counter through the loop.

abi_metadata.rs:44 - the abi argument doesn't affect anything observable. When signer_allowlist() is Some, an unsigned entry has already continued, so the predicate reduces to signer_allowlist().is_none(). Dropping the || abi.signature.is_none() disjunct fails exactly one test - identity_unverified_is_true_for_unsigned_under_require_allowlisted_signer, which documents its own case as unreachable. Also identity_unverified_is_false_for_allowlisted_signers still passes when the fixture is signed with FOREIGN_SIGNER_SEED, so it doesn't pin its name.

abi_metadata.rs:44 / lib.rs:485 - was dropping the counts deliberate? With AbiExtraction gone, log::warn! is the only channel, and neither parser/app nor parser/grpc-server declares a log or tracing dep, so those warnings are no-ops in the enclave - the argument the deleted doc comments made for returning the counts. No behaviour changes today (lib.rs:275 on main discards them), and #454-#456 don't restore them, so what's lost is the PRS-555 hook plus its two tests. Worth stating either way in the body; if they're unwanted, unverified_count should go too.

abi_metadata.rs:473 - signer_allowlist_from_hex has no caller anywhere in the workspace and documents flags that land in #455. Fine for stack order, but the doc drifts if they're renamed.

[shahan-khatchadourian-anchorage]

Scoping the earlier review: two of those findings are blocking, the rest are notes.

1. abi_metadata.rs:1203 - restore a well-formed swapped-in body (the removed OTHER_VALID_ABI, or an inline equivalent with inputs/outputs). As written the fixture doesn't deserialize, so the entry is dropped by register_embedded_abi and the signature check is never what rejects it: with all signature validation deleted from the extraction loop this test still passes.

2. abi_metadata.rs:167 - the counter regression needs a guard at the call site, not only on the helper. Narrowing line 167 back to abi.signature.is_none(), or deleting the unverified_count increment outright, leaves all 281 lib tests green. Either restore an end-to-end assertion on the count or expose it so a test can observe it through the loop.

Neither is a live defect - the extractor behaves correctly today and validate_abi_signature's own unit tests still cover the crypto. It's the regression guards that are hollow, and both fixes are small. Happy to re-review promptly given this parks #454-#456 as well.

The other three (the dropped AbiExtraction counts, the unobservable abi argument, signer_allowlist_from_hex having no caller yet) are notes, not conditions.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[prasanna-anchorage]

Reviewed as the second pair of eyes @shahan-khatchadourian-anchorage asked for. Both of his blocking items are genuinely closed — I re-ran his mutations rather than taking the commit messages' word for it, and added two more. Note his CHANGES_REQUESTED is still what gates this PR; my approval doesn't clear it.

Baseline on head 658b26cb: 283 lib tests green.

Mutation	Before (per his review)	Now
Delete every signature check from the extraction loop	test stayed green	test_accept_unsigned_still_rejects_tampered_signature FAILS (plus test_try_extract_unauthorized_signer_still_rejected)
Narrow identity_unverified to abi.signature.is_none()	all 281 green	test_accept_unsigned_counts_self_signed_entry_as_unverified FAILS (plus the predicate unit test)
Delete the unverified_count increment	all 281 green	test_accept_unsigned_counts_self_signed_entry_as_unverified FAILS

1ff4a408's OTHER_VALID_ABI is the right fix — the fixture now parses on its own, so register_embedded_abi can't be what rejects it and the signature check is the only thing left that can. The doc comment explaining why the fixture has to be well-formed is what stops this regressing again, and is the part I'd have asked for.

e14cf3f5's extract_with_provenance split is the right shape too: the count becomes observable to a test without widening the public API, and the rationale for keeping it private is sound.

I also confirmed his third note empirically. Replacing identity_unverified with just policy.signer_allowlist().is_none() fails exactly one test — identity_unverified_is_true_for_unsigned_under_require_allowlisted_signer, which documents its own case as unreachable through the loop. So the abi argument really is unobservable at the call site; the disjunct is live only for the standalone predicate.

cargo clippy -p visualsign-ethereum --all-targets -- -D warnings clean, cargo test -p visualsign-ethereum --all-targets green (283 lib + 10 integration).

Worth reconsidering before merge (not blocking)

Shahan raised the dropped counts as a note and concluded "no behaviour changes today". I agree there's no live defect, but I'd weigh it a bit heavier, because I think the removal lands worse than it looks.

I verified the logging claim: neither parser/app nor parser/grpc-server declares a log or tracing dependency, and neither initialises a subscriber. parser_cli is the only binary that does (parser/cli/src/logger.rs). So log::warn!("Accepted {unverified_count} ABI mapping(s) with no verified signer") — now the only consumer of the counter — reaches nothing in the shipping enclave binary.

That leaves this PR in an odd position. Under AcceptUnsigned, the posture parser_app runs, a caller-supplied ABI that nobody vouched for can drive the entire decode, and after this change the fact that it happened is recorded nowhere the enclave can observe. The AbiExtraction struct being deleted was the only structured channel, and its own doc comment — removed in this same diff — made precisely this argument:

They are carried this far so that follow-up has something to read instead of a log::warn! the enclave binary compiles away.

So e14cf3f5 adds a genuinely good regression guard for a counter whose sole production consumer is inert, in a PR that removes the plumbing PRS-555 would need to give it one. Both halves are individually defensible; together they're worth a second thought.

Not blocking, and I'm not asking you to rebuild AbiExtraction here — the public signature is cleaner without it and nothing consumes the counts today. But if the intent is still to surface provenance per entry, deleting the carrier now means re-adding it later, and the deleted comment reads like it was left as a deliberate marker. A sentence in the PR description recording that the counts were dropped on purpose and that PRS-555 will need to reintroduce a carrier would be enough to keep the next person from concluding it was an accident.

Minor

let identity_unverified = identity_unverified(policy, abi); shadows the function with a bool of the same name. It reads fine in place, but it means the call site and the predicate can't both be referred to by name in the same scope. A name like provenance_unverified for the local would cost nothing.

[shahan-khatchadourian-anchorage]

Approving stands. Trimming this comment — Prasanna's review already covers the mutation-tested confirmation of both blocking fixes and the dropped-counter/provenance-carrier concern in more depth (he re-ran the mutations rather than reading the diff, and traced the log::warn! dead-end into parser/app/parser/grpc-server having no logging dependency at all), so no need to restate that here.

Two items his review doesn't touch:

1. cli_plugin.rs:123 still claims production "validates the signature and checks the signer's public key against an allowlist during extraction." parser_app (the only production caller) still constructs EthereumVisualSignConverter::new(), i.e. AcceptUnsigned, so no identity check actually runs today. This is the same overclaim class Copilot flagged three times earlier in the PR; the fix commit (4972bc8f) corrected abi_metadata.rs and lib.rs but missed this instance. Worth fixing before merge so the doc doesn't misstate the current trust boundary.

2. Minor, non-blocking: signer_allowlist_from_hex duplicates the parse-and-insert loop in authorized_abi_signers, differing only in error-vs-warn handling — could share a helper parameterized by the on-invalid behavior. Separately, unsigned_rejection_hint is built unconditionally on every call even though it's only read on the rejection path under RequireAllowlistedSigner — building it lazily inside that branch would avoid the allocation under AcceptUnsigned, the posture actually running in production.