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

ZIP 0: ZIP Process #206

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@acityinohio
Copy link
Collaborator

acityinohio commented Feb 16, 2019

Submitting a PR for an updated ZIP process guideline.

As part of this proposal, I'd also support the zips repo being transferred to the Zcash Foundation GitHub org, with all the links being appropriately updated.

@leto

This comment has been minimized.

Copy link

leto commented Feb 17, 2019

👎 to breaking all existing links to current ZIPs

Honestly, it's hard to see the motivation for this change in process, when actually live ZIPs sit languishing, waiting for review: #199

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Feb 17, 2019

@leto A repo move will cause all links to be redirected.

@acityinohio

This comment has been minimized.

Copy link
Collaborator Author

acityinohio commented Feb 19, 2019

I'd be in favor of keeping it in the Zcash Org if a) org membership is public and b) the Foundation has an admin within the org. Barring that I'd prefer the ZIPs to live in a repo outside Zcash Company control, even if it means losing elegant namespacing.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Feb 20, 2019

Renaming and changing the file in the same commit makes it very hard to review the changes from the previous version, particularly as the new file has no changelog, and there aren't any meaningful commit messages. Is this supposed to be a from-scratch rewrite? If so, it should probably have been proposed as its own draft, rather than deleting the previous one.

@acityinohio

This comment has been minimized.

Copy link
Collaborator Author

acityinohio commented Feb 20, 2019

@str4d thanks for the feedback, I'll update it while retaining the old draft (and I think I'm going to switch it back to .rst since all the other ZIPs are included as .rst files and markup is not a bikeshed I'm willing to die on 😄 )

@defuse

This comment has been minimized.

Copy link
Contributor

defuse commented Feb 21, 2019

Looks good! Although I would want to see a "Security & Privacy Considerations" item added to the list of parts a ZIP should have. This section would document known implementation pitfalls, or at least some text demonstrating that the champions have considered the security and privacy consequences of their proposal. RFC 3552 - Guidelines for Writing RFC Text on Security Considerations might be useful; I haven't read it so I don't know if it's any good.

@daira daira force-pushed the acityinohio:master branch from aced2c7 to 96746f8 Feb 24, 2019

@daira

daira approved these changes Feb 24, 2019

Copy link
Contributor

daira left a comment

ACK. I made some minor formatting changes and simplifications.

@acityinohio

This comment has been minimized.

Copy link
Collaborator Author

acityinohio commented Feb 26, 2019

ACK @daira, thanks! Also added a short Security and Privacy Considerations section per @defuse's advice.

@mineZcash

This comment has been minimized.

Copy link

mineZcash commented Feb 28, 2019

I would suggest that some sort of "plain English" explanation of what the ZIP process is and how the average person could contribute to the process be posted somewhere. My gut tells me that many of the people are not commenting here or on the forum because they don't understand the process.

Ditto for the thread that mms posted; https://forum.zcashcommunity.com/t/call-for-nu3-zips-and-network-upgrade-pipeline-process-changes/32749

@kobigurk

This comment has been minimized.

Copy link

kobigurk commented Feb 28, 2019

Small comment - the ZIP Status Field section doesn't contain an Implemented status, and there are references to this status in the workflow.

@rex4539

This comment has been minimized.

Copy link

rex4539 commented Feb 28, 2019

Nit: I'm not keen on using the word "Champion". It's kind of cringeworthy :)

@acityinohio

This comment has been minimized.

Copy link
Collaborator Author

acityinohio commented Mar 1, 2019

Good idea @mineZcash, @sonyamann is working on a blog post/forum post to help get more comments and describe the process in simpler terms.

@kobigurk nice catch! Added an Implementation description in latest commit, thanks.

@rex4539 I hear you, I wanted to confer something that had more weight/ownership than "author"...would "Owner" be a better label? Very open to suggestions here.

@rex4539

This comment has been minimized.

Copy link

rex4539 commented Mar 1, 2019

I'm definitely leaning more towards "Owner" than "Champion".

Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated
@acityinohio

This comment has been minimized.

Copy link
Collaborator Author

acityinohio commented Mar 16, 2019

Changed "Champion" back to "Owner" per @rex4539's suggestion and accepted all of @str4d's changes (and added some clarity re: auxiliary files too @str4d).

How does everyone feel about merging, and if we're good to go, can I be added as a contributor to the ZIPs repo? (rather than moving the repo to the Foundation, per the namespace/confusion issue) Thanks for all the feedback everyone! 🙏

Show resolved Hide resolved zip-0000.rst Outdated
Show resolved Hide resolved zip-0000.rst Outdated

daira added a commit that referenced this pull request Mar 20, 2019

Remove obsolete draft version of ZIP 0 (superseded by #206).
Signed-off-by: Daira Hopwood <daira@jacaranda.org>

@daira daira force-pushed the acityinohio:master branch from 68c2bba to def20e5 Mar 20, 2019

acityinohio and others added some commits Feb 16, 2019

Add ZIP 0 draft.
Author: Daira Hopwood <daira@jacaranda.org>
Author: Josh Cincinnati <acityinohio@users.noreply.github.com>
Mostly trivial wording changes and cosmetics; some simplifications.
Remove OPL licensing; BSD 2-clause is sufficient.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>

@daira daira force-pushed the acityinohio:master branch from def20e5 to fdafb78 Mar 20, 2019

@daira daira requested a review from str4d Mar 20, 2019

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Mar 20, 2019

I updated the PR to account for the draft version of the ZIP having been deleted, by request of Luke Dashjr. However in doing so I made a rookie error (not pulling before force-pushing). I've corrected this now but the commit attribution might be messed up slightly; sorry about that.

daira and others added some commits Mar 20, 2019

adds security and privacy considerations section
Co-authored-by: Josh Cincinnati <acityinohio@users.noreply.github.com>
adds implementation description
Co-authored-by: Josh Cincinnati <acityinohio@users.noreply.github.com>

@daira daira force-pushed the acityinohio:master branch from fdafb78 to 79a773f Mar 20, 2019

@daira daira changed the title Updated ZIP Process Proposal ZIP 0: ZIP Process Mar 20, 2019

daira and others added some commits Mar 20, 2019

Zcash Company -> Electric Coin Company.
Co-authored-by: str4d <thestr4d@gmail.com>
Co-authored-by: Josh Cincinnati <acityinohio@users.noreply.github.com>
Fix references.
Co-authored-by: str4d <thestr4d@gmail.com>
Co-authored-by: Josh Cincinnati <acityinohio@users.noreply.github.com>
Minor updates.
Co-authored-by: str4d <thestr4d@gmail.com>
Co-authored-by: Josh Cincinnati <acityinohio@users.noreply.github.com>
moves motivation section and adds auxilary file clarity
Co-authored-by: Josh Cincinnati <acityinohio@users.noreply.github.com>
s/Champion/Owner/g
Co-authored-by: Josh Cincinnati <acityinohio@users.noreply.github.com>

@daira daira force-pushed the acityinohio:master branch from 79a773f to f61e720 Mar 20, 2019

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Mar 20, 2019

Fixed the commit attributions.

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Mar 20, 2019

@acityinohio I can't add you as a contributor because I'm not an admin for this repo, but I have no objection to you being added.

All @str4d's suggested changes have been made.

@ebfull

ebfull approved these changes Mar 22, 2019

Copy link
Contributor

ebfull left a comment

ACK, but minor nit should be addressed.


* Assign a ZIP number in the pull request.
* Merge the pull request when it is ready.
* List the ZIP in `README.rst <README.rst>`__

This comment has been minimized.

@ebfull

ebfull Mar 22, 2019

Contributor

Currently, this repository only has a README.md.

This comment has been minimized.

@daira

daira Mar 22, 2019

Contributor

I'm inclined to just remove this step. @acityinohio, does that sound reasonable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.