Skip to content

feat(ethereum): add ABI type awareness with proxy->implementation resolution#347

Open
pepe-anchor wants to merge 8 commits into
mainfrom
eth/abi-type-proxy-resolution
Open

feat(ethereum): add ABI type awareness with proxy->implementation resolution#347
pepe-anchor wants to merge 8 commits into
mainfrom
eth/abi-type-proxy-resolution

Conversation

@pepe-anchor
Copy link
Copy Markdown
Contributor

Why

ABI enrichment for Ethereum assumed the transaction's to address was always the implementation contract. For a proxy that's wrong: the proxy delegates to an implementation, so the calldata selector belongs to the implementation's ABI, not the proxy's. Today a wallet has no way to tell the parser "this address is a proxy, decode against this other implementation ABI", so proxied calls fall back to raw hex.

This adds a way for wallets to declare the ABI type (proxy vs implementation, extensible to multicall etc. later) and, for proxies, link to the implementation ABI used for decoding.

What

  • Proto (Abi message): new AbiType enum (UNSPECIFIED/IMPLEMENTATION/PROXY) and optional abi_type + implementation_address fields. Additive and backward compatible.
  • Decoding: the ABI registry carries an AbiKind + implementation link per address. When a tx targets a registered proxy, calldata is decoded against the linked implementation's ABI, falling back to the proxy's own ABI, then hex. The "To" field gets a Proxy badge and an Implementation address field is shown.
  • Serde: abi_type travels as its string name ("ABI_TYPE_PROXY") over JSON (wallet-facing gateway path), via a serde(with) field attribute + abi_type_serde module in the generated crate. gRPC carries the enum natively.
  • CLI: new --abi-proxy-mappings 0xProxy:0xImpl flag; the existing Name:/path:0xAddr format is untouched.

Security

  • Proxy resolution lives strictly after the known-token short-circuit, so a caller-supplied "proxy" entry can never redirect decoding for a canonical-token address (USDC etc.). Regression-tested.
  • abi_type / implementation_address are not covered by the ABI signature (which hashes abi.value only). This is acceptable: the whole caller-ABI decode path is already untrusted and display-only (only the ABI JSON content is signed, never the address binding), and an attacker who can tamper metadata could already swap the bound ABI. The full ChainMetadata is still committed to by metadata_digest in the enclave output. Documented in the abi_metadata module security notes.

Backwards compatibility

  • Proto change is additive (optional fields). Existing clients that don't send abi_type keep working; the field defaults to absent and is treated as implementation.
  • AbiRegistry::map_address is preserved (defaults to implementation), so compile-time embedded ABI callers are unchanged.
  • Heads-up: adding fields to the borsh-derived Abi shifts metadata_digest for the same logical ABI across parser versions, even when the new fields are unset. Confirm nothing on the Turnkey side pins/compares metadata_digest across versions before rollout. Requires a fresh enclave deployment regardless (it's a binary change).

Rollback

Revert this commit and redeploy the parser binary. The proto change is additive, so no client coordination is needed to roll back; clients that started sending abi_type simply have it ignored by the older binary.

Test evidence

  • visualsign-ethereum: 218 unit tests pass (+ new abi_registry/abi_metadata proxy tests). Note: one pre-existing failure, test_known_token_short_circuit_uses_tx_chain_id_not_metadata, fails on main too (predates this branch; unrelated to this change).
  • lib_test e2e: 9/9, including proxy-decodes-via-implementation and the canonical-token-cannot-be-overridden security test.
  • parser_gateway: serde string round-trip (deserialize / serialize / default / reject-unknown) 11/11.
  • parser_cli: --abi-proxy-mappings flag tests pass. parser_app: 5/5 (digest determinism holds). integration (against built binaries): 17 pass.
  • cargo fmt --check clean; cargo clippy --all-targets -- -D warnings clean; make generated produces no diff.

🤖 Generated with Claude Code

pepe-anchor and others added 2 commits May 29, 2026 11:20
…olution

The ABI enrichment path assumed the destination address in a transaction
was always the implementation contract directly. For proxy contracts this
is wrong: the proxy delegates to an implementation, so the calldata
selector belongs to the implementation's ABI.

Add an AbiType enum (UNSPECIFIED/IMPLEMENTATION/PROXY) and an optional
implementation_address link to the Abi proto message. When a transaction
targets a registered proxy, the parser resolves the link and decodes
calldata against the implementation ABI, then surfaces a "Proxy" badge on
the To field and an Implementation address field so the signer can see
where decoding came from.

Key decisions:
- abi_type and implementation_address are not covered by the ABI
  signature (which hashes abi.value only). This is intentional: proxy
  resolution sits strictly after the known-token short-circuit, so
  canonical tokens (USDC etc.) can never be redirected by a caller
  "proxy" entry. The full ChainMetadata (including these fields) is
  committed to by metadata_digest in the enclave output.
- abi_type serialises as its string name ("ABI_TYPE_PROXY") over JSON,
  wired via a serde(with) field attribute + abi_type_serde module in
  generated/src/lib.rs.
- CLI gains --abi-proxy-mappings 0xProxy:0xImpl, leaving the existing
  Name:/path:0xAddr format untouched.
- metadata_digest shifts for the same logical ABI across parser versions
  (borsh adds a tag byte per new optional field); coordinate with Turnkey
  before rollout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Generated by /finish P3 iteration 0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds ABI-type awareness so proxy contracts can be decoded against a linked implementation ABI rather than the proxy's own (typically empty) ABI. Introduces a new proto AbiType enum plus implementation_address field, threads AbiKind through the in-memory ABI registry, adds a proxy-first lookup in the Ethereum visualizer (with a Proxy badge on the To field and an Implementation address field), and exposes a --abi-proxy-mappings 0xProxy:0xImpl CLI flag. The new enum is serialized as its protobuf string name over JSON via a hand-written serde adapter.

Changes:

  • Proto/generated types: additive AbiType enum and implementation_address on Abi; codegen wires a serde(with = abi_type_serde) adapter so JSON uses string names.
  • Ethereum decoding: AbiRegistry carries AbiKind + implementation link; new visualize_with_abi_registry decodes proxy calldata against the implementation ABI (falling back to the proxy ABI), strictly after the known-token short-circuit so canonical tokens can't be redirected.
  • CLI/metadata: --abi-proxy-mappings stamps ABI_TYPE_PROXY + implementation_address on the proxy entry (synthesizing an empty ABI if none was loaded); extraction in abi_metadata keeps proxies with malformed implementation links as un-linked proxies.

Reviewed changes

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

Show a summary per file
File Description
proto/parser/parser.proto Adds AbiType enum and abi_type/implementation_address fields to Abi.
src/codegen/src/main.rs Wires SERDE_ABI_TYPE field attribute for Abi.abi_type.
src/generated/src/lib.rs New abi_type_serde module for JSON string-name (de)serialization.
src/generated/src/generated/parser.rs Generated AbiType enum and new Abi fields.
src/chain_parsers/visualsign-ethereum/src/abi_registry.rs Adds AbiKind, internal AddressMapping, map_address_with_type, get_abi_kind, get_implementation_abi + tests.
src/chain_parsers/visualsign-ethereum/src/abi_metadata.rs Resolves proto AbiType to AbiKind, links proxies to implementations, security docs, tests.
src/chain_parsers/visualsign-ethereum/src/lib.rs Proxy-aware ABI dispatch, Proxy badge on To, Implementation address field; updated chain-id mismatch test.
src/chain_parsers/visualsign-ethereum/tests/lib_test.rs E2E proxy-decodes-via-implementation and canonical-token-cannot-be-overridden tests.
src/parser/cli/src/ethereum.rs New --abi-proxy-mappings flag, apply_proxy_mappings, shared validate_eth_address, tests.
src/parser/gateway/src/main.rs Serde round-trip tests for abi_type string-name representation.
src/parser/app/src/routes/parse.rs Test fixtures updated for new Abi default fields.
src/integration/tests/signature_metadata_e2e.rs Test fixtures updated for new Abi default fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/parser/cli/src/ethereum.rs Outdated
Generated by /finish P3 iteration 1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/parser/cli/src/ethereum.rs Outdated
Generated by /finish P3 iteration 2.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (1)

src/parser/cli/src/ethereum.rs:503

  • The reply to the previous Copilot review indicated this dead test setup was removed, but the unused proxy_path, impl_path, abi_mappings, and proxy_links locals (lines 434–451) and the let _ = (abi_mappings, proxy_links); suppressor (line 503) are still here. The actual test scenario is exercised only by abi_mappings_uc / proxy_links_lc further down, so the earlier block is dead and the trailing comment "The baseline case still works (lowercase in both flags)" is misleading because nothing in that case is asserted. Consider deleting lines 433–451 and line 503 so the test only contains what it actually exercises.
        // Use a freshly created pair so the uppercase vs lowercase contrast is clear.
        let proxy_uc_path = write_temp_json(
            "proxy_uc.json",
            r#"[{"type":"function","name":"upgradeTo"}]"#,
        );
        let impl_uc_path =
            write_temp_json("impl_uc.json", r#"[{"type":"function","name":"transfer"}]"#);

        // Register the ABI with uppercase address in --abi-json-mappings.
        let abi_mappings_uc = vec![
            format!(
                "Proxy:{}:0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
                proxy_uc_path.display()
            ),
            format!(
                "Impl:{}:0xBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB",
                impl_uc_path.display()
            ),
        ];
        // Supply the same addresses in lowercase via --abi-proxy-mappings.
        let proxy_links_lc = vec![
            "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
                .to_string(),
        ];

        let meta = create_chain_metadata(None, &abi_mappings_uc, &proxy_links_lc)
            .unwrap()
            .expect("should return Some");
        let Metadata::Ethereum(eth) = meta.metadata.unwrap() else {
            panic!("expected Ethereum metadata");
        };

        // Both entries must be present; no duplicate synthetic empty entry.
        assert_eq!(eth.abi_mappings.len(), 2);
        // Proxy must retain its ABI file content (not the synthesized empty ABI).
        let proxy_abi = eth
            .abi_mappings
            .get("0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
            .expect("proxy entry present");
        assert_eq!(proxy_abi.abi_type, Some(AbiType::Proxy as i32));
        assert!(
            proxy_abi.value.contains("upgradeTo"),
            "proxy ABI should contain the file content, not the synthesized empty ABI"
        );
        assert_eq!(
            proxy_abi.implementation_address.as_deref(),
            Some("0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")
        );
    }

    #[test]
    fn test_normalize_eth_address() {
        assert_eq!(
            normalize_eth_address("0xdAC17F958D2ee523a2206206994597C13D831ec7"),
            "0xdac17f958d2ee523a2206206994597c13d831ec7"
        );
        assert_eq!(
            normalize_eth_address("0xABCDEF1234567890abcdef1234567890ABCDEF12"),
            "0xabcdef1234567890abcdef1234567890abcdef12"
        );
        // Already lowercase is a no-op.
        assert_eq!(
            normalize_eth_address("0x1111111111111111111111111111111111111111"),
            "0x1111111111111111111111111111111111111111"
        );
    }
}

@pepe-anchor pepe-anchor marked this pull request as ready for review May 29, 2026 12:19
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall well-shaped. Proxy resolution is single-hop, casing is normalized in both paths, the known-token short-circuit sits correctly ahead of the caller-ABI path, and the metadata_digest shift is acknowledged in the commit body. None of the items below are blockers — flagging for consideration.

1. Proxy fallback path is internally inconsistent

lib.rs:535-541: when the impl ABI is missing or doesn't decode, the code falls through to the proxy's own ABI. In that fallback the Implementation field is not emitted, but the Proxy badge is still emittedlib.rs:591-595 checks only abi_kind. Consider always emitting Implementation whenever the destination is AbiKind::Proxy, with badge text like "Proxy implementation (unresolved)" when the impl ABI didn't drive decoding.

2. Unused Default derive on AbiKind

abi_registry.rs:25-33 derives Default but every call site writes AbiKind::Implementation explicitly. Drop the derive or add a one-line justification.

3. Test coverage gaps

  • Proxy fallback decode: proxy with implementation_address pointing at an unregistered impl (lib.rs:543-548) is unexercised end-to-end.
  • Single-hop semantics: A → B where B is also Proxy, or A → A. Pin the single-hop behavior with a test.
  • Impl ABI present but selector mismatch: verify fallback to proxy's own ABI.

4. CLI apply_proxy_mappings edge case

cli/src/ethereum.rs:150-159: if the proxy ABI file load failed earlier, apply_proxy_mappings silently synthesizes an empty [] ABI. Detect that the proxy address appeared in --abi-json-mappings and emit a louder warning when the synthetic replaces a failed load.

5. Misnamed fixture

cli/src/ethereum.rs:278: "Uniswap:...:0xdAC17F958D2ee523a2206206994597C13D831ec7" uses the USDT mainnet address. Rename UniswapToken.

…solution

# Conflicts:
#	src/chain_parsers/visualsign-ethereum/src/lib.rs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no blockers from my end. my comments above are not blockers. lgtm

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

- Drop unused Default derive on AbiKind; update doc comment
- Emit unresolved_implementation_field when impl ABI present but
  selector mismatches; append raw-hex fallback so calldata is always
  visible to the signer (fixes silent calldata suppression)
- Add three proxy tests: unregistered impl, selector mismatch,
  single-hop A→B→C invariant
- Louder Warning (vs Note) in apply_proxy_mappings when proxy address
  appeared in --abi-json-mappings but its file failed to load
- Rename Uniswap→Token fixture label (address was USDT, not Uniswap)
- Hoist use imports to test module level in gateway/main.rs (CLAUDE.md)
- Change new proxy test abi_mappings from HashMap to BTreeMap
- Document single-hop invariant in get_implementation_abi docstring
- Add diagnostic message to abi_type_rejects_unknown_string assert

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor Author

@pepe-anchor pepe-anchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All five items addressed in b93a6ab:

1. Proxy fallback inconsistencyunresolved_implementation_field is now emitted whenever AbiKind::Proxy is set and the implementation ABI exists but doesn't decode the selector. Also fixed a follow-on: the unresolved marker was making input_fields non-empty, silently gating out the raw-hex fallback — added an explicit FallbackVisualizer append in the else branch so the signer always sees the calldata bytes.

2. Unused Default on AbiKind — derive and #[default] attribute removed; doc comment updated to reference resolve_abi_kind explicitly.

3. Test coverage — three tests added:

  • test_proxy_with_unregistered_impl_produces_no_implementation_field
  • test_proxy_impl_abi_selector_mismatch_emits_unresolved_implementation
  • test_proxy_resolution_is_single_hop (A→B where B is also Proxy)

4. CLI apply_proxy_mappings edge case — pre-compute the set of addresses attempted in --abi-json-mappings; emit Warning: instead of Note: when the synthesized empty ABI replaces a file that was specified but failed to load.

5. Fixture renameUniswapToken.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

The "Ensure working tree is clean" CI step runs cargo fmt and fails on
any diff. Two test modules added in earlier commits weren't formatted:
an unsorted import group in the ethereum lib tests and a long assert!
that rustfmt wraps in the gateway tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three ABI test cases declared abi_mappings as BTreeMap (for deterministic
test setup) but passed it into EthereumMetadata via field shorthand, which
expects HashMap, so the lib-test target failed to compile (E0308). Match the
existing passing tests by collecting into the field's HashMap at the use site.

These compile errors were masked in CI: the "ensure working tree clean" fmt
step failed first, so the linter step that compiles --all-targets never ran.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pepe-anchor lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants