-
Notifications
You must be signed in to change notification settings - Fork 469
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: Account Abstraction Validation Scope Rules #105
Conversation
✅ All reviewers have approved. |
@abcoathup - This ERC references EIP-1153, (transient storage), but the linter keeps complaining that ERC-1153 doesn't exist... |
@abcoathup, @SamWilsn - can you explain why this PR can't be merged? it passes all validations in this ERC (all reported errors by "HTML proofer" are in other, unrelated ercs...) |
misc changes
The commit 47df0e0 (as a parent of 31154b1) contains errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! If you tick the "allow edits from maintainers" checkbox on the pull request, I can make these edits directly and save some round trips.
type: Standards Track | ||
category: ERC | ||
created: 2023-09-01 | ||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These proposals seem like they'd be important to read before implementing the rules referencing them (OP-70 / OP-62).
--- | |
requires: 1153, 7212 | |
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both 1153 and 7212 are optional.
We do declare the validation rules in their presence, but the rules are valid even on networks they are missing, so "requires" seems like a strong word. Is there an equivalent "optional" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dror, I know it's a bit confusing but requires
here means "to understand this EIP/ERC, it requires the other EIP/ERC listed here". It doesn't mean the other EIP/ERCs listed in requires
is a Specification-wise dependency.
It seems in the context 1153 and 7212 are a fit for the requires
. I'd also include 4337.
That said, I won't consider having or not having these requires
a blockage for merge as Status: Draft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't include erc-4337, since 4337 actually requires this one, and it would cause a circular dependency.
I strongly suggest that if "requires" means "it is a mere reference for understanding this erc", then it should be renamed accordingly.
(btw: the HTML processor could generate these "references" automatically. an editor should explicitly specify what is really "required".
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
- clarify motivation and rationale - clearly define eip-7212 and erc-1153 as optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good as a Draft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
Head branch was pushed to by a user without write access
c3eefef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Reviewers Have Approved; Performing Automatic Merge...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md
We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met: