Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ERC: EIP-712 Extensions for Account Abstraction #693

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Oct 30, 2024

Moved from ethereum/EIPs#8985

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Oct 30, 2024

File ERCS/erc-7803.md

Requires 1 more reviewers from @axic, @g11tech, @SamWilsn, @xinbenlv

@eip-review-bot eip-review-bot changed the title Add ERC-TBD: EIP-712 Extensions for Account Abstraction Add ERC: EIP-712 Extensions for Account Abstraction Oct 30, 2024
@github-actions github-actions bot added the w-ci label Oct 30, 2024
ERCS/erc-0000.md Outdated Show resolved Hide resolved
ERCS/erc-0000.md Outdated Show resolved Hide resolved
frangio and others added 2 commits October 30, 2024 21:08
@github-actions github-actions bot removed the w-ci label Oct 31, 2024
@frangio frangio changed the title Add ERC: EIP-712 Extensions for Account Abstraction Add ERC-7803: EIP-712 Extensions for Account Abstraction Oct 31, 2024
@eip-review-bot eip-review-bot changed the title Add ERC-7803: EIP-712 Extensions for Account Abstraction Add ERC: EIP-712 Extensions for Account Abstraction Oct 31, 2024
@github-actions github-actions bot added the w-ci label Oct 31, 2024
In the presence of `signingDomains` the account should encode the message to be signed according to the following recursive procedure:

- `encodeForSigningDomains(signingDomainSeparators : [𝔹²⁵⁶], verifyingDomainSeparator : 𝔹²⁵⁶, message : 𝕊) =`
- If `signingDomainSeparators = [first, ...others]`: `"\x19\x02" ‖ first ‖ encodeForSigningDomains(others, verifyingDomainSeparator, message)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is first the initial dapp domain? If so, I think dapp would be more explicit:

Suggested change
- If `signingDomainSeparators = [first, ...others]`: `"\x19\x02" ‖ first ‖ encodeForSigningDomains(others, verifyingDomainSeparator, message)`
- If `signingDomainSeparators = [dapp, ...others]`: `"\x19\x02" ‖ dapp ‖ encodeForSigningDomains(others, verifyingDomainSeparator, message)`

Copy link

@howydev howydev Nov 5, 2024

Choose a reason for hiding this comment

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

IIUC - signingDomainSeparators is a new addition and will only contain account domains. The dapp domain is the domain in L54

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes these are distinct from the dapp domain.


- `id`: An string that identifies the method. It may be one of:
- `ECDSA`: ECDSA signatures by Externally Owned Accounts.
- `ERC-{n}`: A standard type of signature specified by an ERC. `n` must not be padded with zeros.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an authentication method other than ERC-1271? If 1271 is generalized enough I think this can be simplified to just ECDSA or ERC-1271.

In the case we want to make this ERC more broadly compatible, then it may make sense to define the signature algorithms supported by other networks (e.g. ed25519 in cosmos). I feel this is not the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an authentication method other than ERC-1271?

Yes, ERC-6492 is another standard that is mentioned earlier in the Motivation section. It covers a scenario that ERC-1271 doesn't (predeploy contracts). I'd rather keep this generic than listing the standards that exist now.

Each member of the array is an object with the following keys:

- `id`: An string that identifies the method. It may be one of:
- `ECDSA`: ECDSA signatures by Externally Owned Accounts.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we the spec should suggest priority for ERC-1271 if there's code in the address. Just as we do in SignatureChecker since after EIP-7702, the validation mechanism should decide whether to prioritize ECDSA or ERC-1271.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that if there's code ERC-1271 should take priority but not all contracts implement it that way (e.g., prior versions of OpenZeppelin Contracts didn't), so if a dapp needs a signature for one of those contracts it should be possible to express that order of preferences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is explained in the Motivation section too, let me know if it's not clear.

@frangio frangio changed the title Add ERC: EIP-712 Extensions for Account Abstraction Add ERC-7803: EIP-712 Extensions for Account Abstraction Nov 5, 2024
@eip-review-bot eip-review-bot changed the title Add ERC-7803: EIP-712 Extensions for Account Abstraction Add ERC: EIP-712 Extensions for Account Abstraction Nov 5, 2024
@github-actions github-actions bot removed the w-ci label Nov 5, 2024
@github-actions github-actions bot added the w-ci label Nov 5, 2024
Copy link

github-actions bot commented Nov 5, 2024

The commit bd681c7 (as a parent of 7feed84) contains errors.
Please inspect the Run Summary for details.

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

Successfully merging this pull request may close these issues.

5 participants