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 EIP-6051: Private Key Encapsulation #6051

Merged
merged 39 commits into from
Dec 27, 2022
Merged

Conversation

weiji-cryptonatty
Copy link
Contributor

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:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@weiji-cryptonatty weiji-cryptonatty changed the title Added EIP for private key encapsulation Added EIP-6051 for private key encapsulation Nov 28, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Nov 28, 2022

A critical exception has occurred:
Message: pr 6051 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Nov 28, 2022
@Pandapip1 Pandapip1 changed the title Added EIP-6051 for private key encapsulation Add EIP-6051: private key encapsulation Nov 28, 2022
@Pandapip1 Pandapip1 changed the title Add EIP-6051: private key encapsulation Add EIP-6051: Private Key Encapsulation Nov 28, 2022
EIPS/eip-6051.md Outdated Show resolved Hide resolved
EIPS/eip-6051.md Outdated Show resolved Hide resolved
EIPS/eip-6051.md Outdated Show resolved Hide resolved
weiji-cryptonatty and others added 2 commits November 30, 2022 10:27
Co-authored-by: xinbenlv <zzn@zzn.im>
@weiji-cryptonatty
Copy link
Contributor Author

Hi @xinbenlv I had pushed some more commits to address all issues except the ERC vs Interface. Could you please review again? Thanks a lot.

weiji-cryptonatty and others added 8 commits December 9, 2022 17:48
Co-authored-by: xinbenlv <zzn@zzn.im>
Co-authored-by: xinbenlv <zzn@zzn.im>
Co-authored-by: xinbenlv <zzn@zzn.im>
Co-authored-by: xinbenlv <zzn@zzn.im>
Co-authored-by: xinbenlv <zzn@zzn.im>
Co-authored-by: xinbenlv <zzn@zzn.im>
@weiji-cryptonatty
Copy link
Contributor Author

These are my main editorial review comments, many of them are due to the grammar issue. I totally understand grammar challenge myself because I speak English as second language. Please consider run via a grammar checker.

Another major issue is to keep it consistent for terminologies that you specifically defined. For example, when addressing "Recipient" or "Sender", please capitalize.

Hi @xinbenlv thank you again for the very careful and very detailed reviews. I can see why EIP editors are so overwhelmed. I really appreciate what you had done here.

I think I have fixed all issues. I also ran another round with grammarly.com and fixed almost all issues it suggests (with very few I did not agree with it). Please review again when you do find time.

@xinbenlv
Copy link
Contributor

xinbenlv commented Dec 9, 2022

From a technical perspective, I personally still think, if the EIP is defining a way to encapsulate and unwrap private key via eth_ interface, that is considered an interface category, but we can address it later.

xinbenlv
xinbenlv previously approved these changes Dec 9, 2022
Copy link
Contributor

@xinbenlv xinbenlv left a comment

Choose a reason for hiding this comment

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

I think this is good to merge as a draft now. Defer to @Pandapip1 @SamWilsn to follow up the merging.

EIPS/eip-6051.md Outdated Show resolved Hide resolved
EIPS/eip-6051.md Outdated Show resolved Hide resolved
EIPS/eip-6051.md Outdated Show resolved Hide resolved
EIPS/eip-6051.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@weiji-cryptonatty weiji-cryptonatty left a comment

Choose a reason for hiding this comment

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

suggestions accepted

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
weiji-cryptonatty and others added 4 commits December 22, 2022 09:44
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
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.

LGTM for a draft

@eth-bot eth-bot enabled auto-merge (squash) December 27, 2022 14:58
@eth-bot eth-bot merged commit e666b5b into ethereum:master Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants