Skip to content

Add ERC7730 forward compatability#2

Open
yougogirldoteth wants to merge 6 commits into
mainfrom
feature/erc7730-forward-compat
Open

Add ERC7730 forward compatability#2
yougogirldoteth wants to merge 6 commits into
mainfrom
feature/erc7730-forward-compat

Conversation

@yougogirldoteth

Copy link
Copy Markdown
Member

No description provided.

@jwahdatehagh

Copy link
Copy Markdown
Member

Thanks for this — aligning with ERC-7730 is strategically the right call, and there's a lot in here that main genuinely lacks. I've updated the branch against main (the 120-char descriptions and amount types are merged in, and the two new Hello World Computer contracts are migrated to the clear-signing conventions). Some structured feedback:

What I want to keep

  • Multi-chain deployments and factory context — real gaps today; one file per chain/address doesn't scale to protocols.
  • EIP-712 domain binding on messages — without the domain, a consumer can't actually verify which contract a Permit belongs to.
  • Capability-based includesinterface:erc20-permit as a separate include instead of baking Permit into erc20 is exactly the right modeling, and it generalizes (erc721-enumerable, erc4626, …).
  • Dynamic cross-field references (tokenPath) — our static tokenAddress can't express "the token is that other parameter", which is the common case for routers/vaults.
  • intent vs interpolatedIntent — the stable-summary vs value-bearing-sentence distinction is real.

Concerns before this can merge

  1. Two parallel parameter models. params stays as a "compatibility layer" next to fields, so every function in the contract files now carries each label and description twice. We haven't shipped v1 — there's no installed base to stay compatible with. We should pick one model, not maintain both forever.

  2. Named ABI fragments as keys are a footgun. transfer(address to,uint256 value) makes entry identity depend on parameter names, which don't affect the selector. It needs a whitespace/canonicalization spec it doesn't have, and it creates the merge hazard the PR itself documents ("overrides MUST use the same key form"). Bare name / signature / selector keys plus names carried in fields conveys the same information without the fragility.

  3. metadata next to meta. Two near-identical top-level names with unrelated purposes — authors will mix these up constantly. Suggest definitions or similar for the reusable namespace.

  4. Dual vocabularies. The Type enum now contains both token-id and tokenId, blocknumber and blockNumber, and "legacy aliases MAY be accepted" makes the ambiguity permanent. A standard needs exactly one spelling per concept.

  5. The forward-compat claim overpromises. A 7730 consumer can't actually read these files — 7730 has a different document structure (context / metadata / display). What this achieves is vocabulary alignment, which is valuable, but the draft should either define an explicit export transform to real 7730 documents or scope the claim down.

  6. The path model is borrowed without its grammar. # / $ / @ roots are introduced, but array indexing, nesting, and visible conditions are all left to "resolve deterministically". The @.domain.verifyingContract example uses syntax defined nowhere. ERC-7730 specifies this grammar precisely — if we adopt the idea we should adopt the grammar.

  7. EIP-7730-COMPARISON.md is stale — it references displayHint, inputHint, shortDescription, contract.presentation, none of which exist in the repo anymore.

Suggested path forward

There's also a conceptual collision with #1 (free-form action ids + function refs vs named-fragment keys; intent as template vs static summary). My suggestion: land #1 first — the action model is the better foundation — and rebuild this as a series of small PRs on top of it:

  1. deployments + factory
  2. EIP-712 domain binding for messages
  3. interface:erc20-permit (and friends)
  4. Dynamic references (tokenPath etc.)
  5. Then, if we still want it: the fields/format display model attached to actions, with a single vocabulary and a defined 7730 export mapping.

Happy to pair on splitting this up.

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.

2 participants