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

Update RFC process proposal #9

Merged
merged 13 commits into from
Jul 8, 2019
Merged

Update RFC process proposal #9

merged 13 commits into from
Jul 8, 2019

Conversation

lehnberg
Copy link
Contributor

This PR clarifies the RFC process proposal.

Specifically:

  • Adds state diagram
  • Full circles transitions and process flows
  • By design only allows approved PRs to be merged
  • Aligns the process closer to Rust than to BIP/PEP.

It also proposes that assets such as .puml and .svg are kept in a separate directory at root level, named according to the RFC number.

Copy link
Member

@yeastplume yeastplume left a comment

Choose a reason for hiding this comment

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

Generally fine with this. One question, how do changes to this process get approved? Is it the core team's job to approve or disapprove PRs to this RFC?

text/0001-rfc-process.md Outdated Show resolved Hide resolved

Eventually, the relevant community organization will either accept the RFC by changing its status to 'Accepted' or reject it by setting it to 'Rejected'.
#### Active
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to have another status that denotes whether the RFC has been implemented. And RFC could be Active/Unimplemented or Active/Implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that Rust is light touch here and I think it's intentionally: Once an RFC is merged and "active", a tracking issue is opened in the relevant repo, and that's where all the implementation details and subsequent attention is directed. Which makes sense to me. The RFC itself is therefore is not a full spec and something that should be tracked. Instead becomes something of a historical artefact.

They don't even have a state for Active/Inactive in Rust, if RFC1 is replaced by RFC2, they simply put a comment in RFC1 that it's been superseded by RFC2 and link to it.

To me, this makes sense, as you ultimately track the actual issues you have in your sub-team's repo.

Copy link
Member

Choose a reason for hiding this comment

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

Think I can handle that. I'd say in most cases as development occurs there would be technical issues popping up that might require changing, say an API definition in the original RFC. I assume these kinds of definitions would be worth updating in the RFC as implementation progresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is a really good question. Here's my thinking based on my reading of the Rust process and how I think it makes sense for us to start:

Let's say there's an API definition (affecting grin-wallet) in an RFC that's accepted. A tracking issue for this is created in grin-wallet, and then at some point there's work starting to implement this, by opening PRs in grin-wallet. And then at some point throughout the course of implementation, some details of the API change.

Would it be worth while to update the RFC? Maybe. Depends on the change and the developer's inclination to do it.

Should it matter whether the RFC is updated or not? By design, no. The RFC does not act as a "final spec" or ultimate source of truth. That's what documentation in grin-wallet or the code itself is there for. Instead, in the RFC there will be a link to the tracking issue. And in the tracking issue, there will be a link to a PR. And in that PR there will be documentation references (hopefully). And if there's future iterations, one would hope that either the documentation is updated or that future PRs reference that PR.

The point here is that we should not introduce a process where we expect developers or contributors to keep an RFC document up to date and go back to refer to that as some kind of ultimate reference. As we're going to struggle to maintain that.

Makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

Unless the RFC is more of an open specification, where you might have some protocol spec with a link to a maintained reference implementation. But I think it would be fairly evident in that case that the RFC needs to remain updated.

text/0001-rfc-process.md Outdated Show resolved Hide resolved
text/0001-rfc-process.md Outdated Show resolved Hide resolved
text/0001-rfc-process.md Show resolved Hide resolved
@lehnberg
Copy link
Contributor Author

Thanks for the review @yeastplume, I've made some changes based on your feedback, have a look.

One question, how do changes to this process get approved? Is it the core team's job to approve or disapprove PRs to this RFC?

Good question. Since the RFC process is something we'd like to be consistent across all sub-teams and the project as a whole, this to me feels like something that falls under Core's remit. I would imagine they would consult with sub-teams and the people using the process as they begin evaluate proposals.

And if we're to follow the proposed RFC process, a future "substantial" overhaul to the RFC process should then be opened as a new RFC rather than making edits to the existing one, if that makes sense.

@yeastplume
Copy link
Member

Perhaps a paragraph on changes to the RFC process then, more or less what you've written there.

@lehnberg
Copy link
Contributor Author

lehnberg commented Jul 1, 2019

Makes sense, just added a section regarding this.

@lehnberg
Copy link
Contributor Author

lehnberg commented Jul 8, 2019

Not received any more comments, merging this as per discussion in last governance meeting.

@lehnberg lehnberg merged commit 25a083c into mimblewimble:master Jul 8, 2019
@lehnberg lehnberg deleted the rfc branch July 8, 2019 16:08
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.

2 participants