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

Move EIP-712 to Last Call #5271

Merged
merged 21 commits into from
Jul 25, 2022
Merged

Move EIP-712 to Last Call #5271

merged 21 commits into from
Jul 25, 2022

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Jul 15, 2022

There have been no substantial changes to this EIP in over a year.

This PR also includes non-normative changes to adapt the EIP to new formatting requirements. A mention of "EIP-719" was removed because said EIP doesn't exist. The "Implementation" section was removed as it consisted of an implementation log that doesn't belong in the EIP format.

@eth-bot
Copy link
Collaborator

eth-bot commented Jul 15, 2022

All tests passed; auto-merging...

(pass) eip-712.md

classification
updateEIP
  • passed!

@xinbenlv
Copy link
Contributor

So looking forward to it

@MicahZoltu
Copy link
Contributor

Please see the files tab where a new bot has made a bunch of comments (they don't currently trigger any email/notification).

@frangio
Copy link
Contributor Author

frangio commented Jul 17, 2022

@MicahZoltu If I understand correctly these errors are due to EIP format requirements that have changed or become stricter since this document was written.

Should I make the changes required to fit the new format? Will those interfere with the attempt to move to Last Call status?

@SamWilsn
Copy link
Contributor

Should I make the changes required to fit the new format?

Yes please!

Will those interfere with the attempt to move to Last Call status?

Nope. Status changes are usually where we make sure EIPs are up to date with the latest format requirements.

Do note, however, that substantial changes after the EIP enters last call might require resetting the last call deadline.

EIPS/eip-712.md Outdated Show resolved Hide resolved
EIPS/eip-712.md Outdated Show resolved Hide resolved
EIPS/eip-712.md Outdated Show resolved Hide resolved
EIPS/eip-712.md Show resolved Hide resolved
EIPS/eip-712.md Outdated Show resolved Hide resolved
EIPS/eip-712.md Outdated Show resolved Hide resolved
EIPS/eip-712.md Show resolved Hide resolved
EIPS/eip-712.md Show resolved Hide resolved
EIPS/eip-712.md Show resolved Hide resolved
EIPS/eip-712.md Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor Author

frangio commented Jul 19, 2022

This should be ready to merge once one of the authors approves it.

@LogvinovLeon @recmo @dekz

@frangio
Copy link
Contributor Author

frangio commented Jul 22, 2022

@SamWilsn This has got author approval. Just need editor approval now. 🙂

@github-actions
Copy link

The commit 451b062 (as a parent of 213ef96) contains errors. Please inspect the Run Summary for details.

@frangio
Copy link
Contributor Author

frangio commented Jul 23, 2022

Ok I've moved the existing section about replay attacks into a Security Considerations section, I think we should be good now.

cc @SamWilsn

@frangio
Copy link
Contributor Author

frangio commented Jul 24, 2022

I've added a small note concerning the distribution of the signature and in particular frontrunning attacks, as suggested by @xinbenlv, which should be uncontroversial.

But this PR really shouldn't result in any further modifications as it already has author approval. Please merge as soon as possible!

Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Let's try this again.

@Pandapip1
Copy link
Member

@frangio please push an empty commit.

Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

CI should be fixed

@MicahZoltu MicahZoltu closed this Jul 25, 2022
@MicahZoltu MicahZoltu reopened this Jul 25, 2022
@eth-bot eth-bot enabled auto-merge (squash) July 25, 2022 09:51
@eth-bot eth-bot merged commit 11143fe into ethereum:master Jul 25, 2022
@frangio frangio deleted the eip-712-last-call branch August 1, 2022 14:30
@frangio frangio mentioned this pull request Aug 11, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Move EIP-712 to Last Call

* fix sections

* fix eip references

* remove external links

* shorten title

* reformat notes

* remove self references

* add links to referenced eips

* add description

* add tentative last call deadline

* remove standard from description

* add specification header

* convert image to relative

Co-authored-by: Sam Wilson <[email protected]>

* use relative image src

* remove mention of EIP-719

* remove implementation section

* simplify abstract

* add security considerations section

* fix section order

* add note on frontrunning attacks

* empty commit

Co-authored-by: Sam Wilson <[email protected]>
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.

7 participants