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

ERC-5247: Smart Proposal #5247

Merged
merged 26 commits into from Jul 16, 2022
Merged

ERC-5247: Smart Proposal #5247

merged 26 commits into from Jul 16, 2022

Conversation

xinbenlv
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.

@xinbenlv xinbenlv changed the title Create eip-executable-proposal Executable Proposal Standard Jul 13, 2022
@xinbenlv xinbenlv changed the title Executable Proposal Standard Proposal Standard Jul 13, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Jul 13, 2022

All tests passed; auto-merging...

(pass) eip-5247.md

classification
updateEIP
  • passed!

@xinbenlv xinbenlv changed the title Proposal Standard Executable Proposal Standard Jul 13, 2022
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
EIPS/eip-executable-proposal Outdated Show resolved Hide resolved
xinbenlv and others added 9 commits July 13, 2022 17:17
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@xinbenlv xinbenlv changed the title Executable Proposal Standard ERC-5247: Executable Proposal Standard Jul 14, 2022
Copy link
Contributor Author

@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.

Done update

@xinbenlv
Copy link
Contributor Author

@Pandapip1 thanks for the comment. I update it with your suggestion. I don't know why Auto-Merge Bot still yield

---
## (fail) EIPS/eip-5247.md
| classification |
| ------------- |
| `ambiguous` |
- eip-5247.md has no identifiable authors who can approve the PR (only considering the base version)
Error: Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
---
## (fail) EIPS/eip-5247.md
| classification |
| ------------- |
| `ambiguous` |
- eip-5247.md has no identifiable authors who can approve the PR (only considering the base version)
Error: Process completed with exit code 1.

Is it because I need an editor to give permission?

@xinbenlv
Copy link
Contributor Author

Hi editors, all issues are resolved except for your review

- File with name EIPS/eip-5247.md is new and new files must be reviewed
- This PR requires review from one of [@lightclient, @axic, @samwilsn, @pandapip1]
Error: Process completed with exit code 1.

@xinbenlv xinbenlv changed the title ERC-5247: Executable Proposal Standard ERC-5247: Smart Proposal Jul 14, 2022
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.

Trigger EIPW

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.

Trigger EIPW

@xinbenlv
Copy link
Contributor Author

I don't know why it's failing with

Setting environment variables for Vercel CLI
Error: No commit found for SHA: patch-14

Any suggestions?

@MicahZoltu
Copy link
Contributor

cc @Pandapip1

@xinbenlv
Copy link
Contributor Author

Trigger EIPW

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Jul 15, 2022

Seems everything else passed. Now looking for ERC track review from an Editor among @lightclient, @axic, @SamWilsn @Pandapip1

## (fail) eip-5247.md
| classification |
| ------------- |
| `newEIPFile` |
- File with name EIPS/eip-5247.md is new and new files must be reviewed
- This PR requires review from one of [@lightclient, @axic, @samwilsn, @pandapip1]

EIPS/eip-5247.md Outdated Show resolved Hide resolved
Comment on lines +46 to +65
// TODO: add Proposal Cancel/Edit/Withdraw Event and Functions?

// TODO: decide whether we require generating ProposalId in the method or not?
// TODO: if require generating ProposalId internally, can it be incremental hash-generated?
// TODO: what if proposal need to demonstrate sufficient support? How to input quorum?
function createProposal(
uint256 proposalId,
string calldata proposalUri,
uint8[] calldata optionIds,
address[] calldata targets,
uint256[] calldata values,
bytes[] calldata calldatas,
uint256 startblock,
uint256 endblock,
bytes calldata extraParams
) external returns (uint256 registeredProposalId);

// TODO: what's most proper way to update the voting period?
// TODO: do we want to include cancel or withdraw?
// TODO: what's the best way to include weight scheme?
Copy link
Member

Choose a reason for hiding this comment

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

These TODOs need to be either resolved or removed before this can be merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally allow TODOs in draft. The goal with DRAFT is to get something out, so as long as it generally is formatted correctly and it has all of the relevant sections and it isn't completely empty I will merge it as a draft. These definitely need to be addressed prior to Review though.

type: Standards Track
category: ERC
created: 2022-07-13
requires: 7
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
requires: 7
requires: 7, 1202

requires is a bit of a misnomer; it really should be called references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. but I would humbly reject this revision suggestion
I was hoping to have a separate reference eip or related eip in the preemble. But while it's still requires, I would not add 1202 to avoid confusion. The purpose is to decouple 1202 and 5247

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if it's ok for you @Pandapip1

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit of a contentious point, so I'll let this slide for now. It can always be changed later if/when this goes to Review, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rewrite the text of the EIP so it doesn't reference EIPs that aren't required? Generally speaking, EIPs shouldn't be making comparative arguments, they should focus exclusively on specifying a thing. If you are just referring to other EIPs to compare against then that sort of content probably should just be left out of the EIP entirely.

EIPS/eip-5247.md Outdated Show resolved Hide resolved
EIPS/eip-5247.md Outdated Show resolved Hide resolved
EIPS/eip-5247.md Outdated Show resolved Hide resolved
EIPS/eip-5247.md Outdated Show resolved Hide resolved
EIPS/eip-5247.md Outdated Show resolved Hide resolved
Comment on lines +46 to +50
// TODO: add Proposal Cancel/Edit/Withdraw Event and Functions?

// TODO: decide whether we require generating ProposalId in the method or not?
// TODO: if require generating ProposalId internally, can it be incremental hash-generated?
// TODO: what if proposal need to demonstrate sufficient support? How to input quorum?
Copy link
Member

Choose a reason for hiding this comment

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

These TODOs need to be either resolved or removed before this can be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I keep them as draft and as highlight these design decision to invoke discussion? I will guarantee to remove/resolve these TODOs before moving to review

Copy link
Member

Choose a reason for hiding this comment

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

@SamWilsn would this be okay? I'm inclined to say yes but I'd like to have another opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally allow TODOs up until Review stage. The original intent of Draft was that the barrier to entry for a draft should be very low, just enough to get you a number and a place people could start to submit PRs against. We have drifted a bit away from that since then and also started requiring that people structure the file properly, but I don't think we should go so far as to require a draft EIP actually be finished.

xinbenlv and others added 6 commits July 15, 2022 22:05
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Thank you for the suggestion!

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Jul 16, 2022 via email

@MicahZoltu
Copy link
Contributor

how about in rationale describing the "why" of design choice?

Is it possible to describe the "why" without linking to another EIP and instead describing the different design choice in the other EIP briefly? That is generally preferable, as it allows you to focus specifically on the thing that is different without requiring the read understand the entire other EIP.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Jul 16, 2022 via email

@kodiakhq kodiakhq bot merged commit d768349 into ethereum:master Jul 16, 2022
@MicahZoltu
Copy link
Contributor

Which particular mentioning of EIPs are you referring to when suggesting removing? In Smart Proposal we might refer to EIPs that easily illustrative the use case of Smart Proposal such as "here is a contract call in bytecode to make a EIP20 transfer" this helps reader to much easier understand what this EIP can be put into context.

For that example, I don't see why you need to refer to EIP-20. You can just say something like, "here is an example call of the function transfer(address, uint256) ..." The fact that it happens to match the signature from EIP-20 is not relevant to this EIP.

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Jul 17, 2022

Which particular mentioning of EIPs are you referring to when suggesting removing? In Smart Proposal we might refer to EIPs that easily illustrative the use case of Smart Proposal such as "here is a contract call in bytecode to make a EIP20 transfer" this helps reader to much easier understand what this EIP can be put into context.

For that example, I don't see why you need to refer to EIP-20. You can just say something like, "here is an example call of the function transfer(address, uint256) ..." The fact that it happens to match the signature from EIP-20 is not relevant to this EIP.

Micah, please allow me to debate with the question about "whether one should remove EIP" via an argument of the the purpose of an EIP.

First of all, let me explain this particular case: Here is a logic chain rationale:

  1. Why one would want to specify a calldata in the proposal? -> because they may want to make arbitrary function call
  2. Why would one want to make arbitrary function call? -> because one may want to make call to transfer
  3. Why would one want to make call to transfer? -> because one may want to do ERC20 calls.

Maybe one would say, the 3rd point is not necessary, because anyone familiar enough with ERC20 and the fact that they are specifying transfer would easily come up with their derived conclusion easily. Considering ERC20 is one of the most widely used applications, as an EIP author I find it much easier to explain to an arbitrary reader (assuming Ethereum community member) why being able to interact with ERC is useful.

Secondly, let's generalize:

If we want to reduce the rationale linking to the context, why don't we just remove rationale section? (It's not that I am proposing to remove rationale section, I just feel that a lot of suggestion in removing references has made the EIP proposal towards the direction of harder to read.

As far as I read, EIP spells out as "Ethereum Improvement Proposal", and ERC spells "Ethereum Request for Comment", both term give me a sense that author need to both demonstrate "what to do" and "why" so as to rally community members for adoption.

Therefore, given the purpose of EIP / ERC, I would like to advocate a principle of testing of an editorial change: from a lay Ethereum community member, do the suggested editing change make the document more readable or less readable.

Pandapip1 added a commit to Pandapip1/EIPs that referenced this pull request Jul 18, 2022
* Create eip-executable-proposal

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update eip-executable-proposal

* Change file name to EIP-num

* Change authors to author

* Update

* With rationale of decoupling voting and proposal

* Update eip-5247.md

* Update eip-5247.md

* Update eip-5247.md

* Update eip-5247.md

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Thank you for the suggestion!

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@xinbenlv xinbenlv deleted the patch-14 branch December 9, 2022 03:40
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Create eip-executable-proposal

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-executable-proposal

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update eip-executable-proposal

* Change file name to EIP-num

* Change authors to author

* Update

* With rationale of decoupling voting and proposal

* Update eip-5247.md

* Update eip-5247.md

* Update eip-5247.md

* Update eip-5247.md

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Thank you for the suggestion!

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

* Update EIPS/eip-5247.md

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
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.

None yet

4 participants