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.

Sign up for GitHub

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

Revise CONTRIBUTING #4382

Merged
merged 2 commits into from
Feb 6, 2023
Merged

Revise CONTRIBUTING #4382

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6808086
Revise CONTRIBUTING
thejohnfreeman Jan 6, 2023
b6ec612
Incorporate review comments
thejohnfreeman Jan 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 139 additions & 34 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,117 @@
The XRP Ledger has many and diverse stakeholders, and everyone deserves
a chance to contribute meaningful changes to the code that runs the XRPL.

# Contributing
The XRP Ledger has many and diverse stakeholders, and everyone deserves a chance to contribute meaningful changes to the code that runs the XRPL.
To contribute, please:
1. Fork the repository under your own user.
2. Create a new branch on which to write your changes. Please note that changes which alter transaction processing must be composed via and guarded using [Amendments](https://xrpl.org/amendments.html). Changes which are _read only_ i.e. RPC, or changes which are only refactors and maintain the existing behaviour do not need to be made through an Amendment.
3. Write and test your code.
4. Ensure that your code compiles with the provided build engine and update the provided build engine as part of your PR where needed and where appropriate.
5. Write test cases for your code and include those in `src/test` such that they are runnable from the command line using `./rippled -u`. (Some changes will not be able to be tested this way.)
6. Ensure your code passes automated checks (e.g. clang-format and levelization.)
7. Squash your commits (i.e. rebase) into as few commits as is reasonable to describe your changes at a high level (typically a single commit for a small change.)
8. Open a PR to the main repository onto the _develop_ branch, and follow the provided template.

# Major Changes
If your code change is a major feature, a breaking change or in some other way makes a significant alteration to the way the XRPL will operate, then you must first write an XLS document (XRP Ledger Standard) describing your change.
To do this:
1. Go to [XLS Standards](https://github.com/XRPLF/XRPL-Standards/discussions).
2. Choose the next available standard number.
3. Open a discussion with the appropriate title to propose your draft standard.
4. Link your XLS in your PR.

We assume you are familiar with the general practice of [making contributions
on GitHub][1].
This file includes only special instructions specific to this project.


## Before you start

All of your contributions must be developed in your personal
[fork](https://github.com/XRPLF/rippled/fork).
No personal branches may ever be pushed to the [main project][rippled].
These are the only branches that may ever exist in the main project:

- `develop`: The latest set of unreleased features, and the most common
starting point for contributions.
- `release`: The latest release candidate.
- `master`: The latest stable release.
- `gh-pages`: The documentation for this project, built by Doxygen.

All of your commits must be signed,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe leave a note that "sign-off" is not "signing", since there could be confusion around this (some projects require sign-offs for their CLAs for example).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might also be worth having this checked by a bot in addition to stating it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think GitHub's built-in protections can enforce this, and I'm not aware of a third-party bot that does it, but it is somewhat enforced by the fact that PRs cannot be squashed and merged unless all commits are signed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

GitHub's protections can enforce that the tip of any branch is signed. I don't think it's necessary that all commits are signed as long as the tip is. My understanding is that signing a commit is implicitly verifying all the commits leading up to it (sort of how validating a ledger in rippled implicitly validates all the previous ledgers). It may be worth rephrasing this to something like "All of your commits should be signed. The tip of pushed branches must be signed."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GitHub can enforce that the tip of our branch is signed, not that the tip or any commit of a PR branch is signed. This part of the document is talking about the commits made by contributors, the commits that appear in their PR.

When it comes time to merge a PR using squash-and-merge, GitHub will make the commit and sign it with GitHub's key, but only if all of the commits in the PR are signed by the author. That is why we must require that they sign all of their commits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add this explanation to the document.

and GitHub must know your verifying key.
Please walk through the excellent documentation from GitHub to set
up [signature verification][signing].

[rippled]: https://github.com/XRPLF/rippled
[signing]: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification


## Major contributions

If your contribution is a major feature or breaking change,
then you must first write an XRP Ledger Standard (XLS) describing it.
Go to [XRPL-Standards](https://github.com/XRPLF/XRPL-Standards/discussions),
choose the next available standard number, and
open a discussion with an appropriate title to propose your draft standard.

When you submit a PR for this change, please link the corresponding XLS in the
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this file is touched anyway, I believe it is relevant to be more explicit in the purpose and use of XLS:

When you submit a PR for this change, please link the corresponding XLS draft in the description. As XLS drafts are considered work in progress and open for discussion, PRs should not be submitted without due time for discussions, questions, suggestions and additions to the XLS draft. It is the responsibility of the XLS author to update the draft to match the final implementation when the PR is merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this after some editing.

description.


## Before making a pull request

Changes that alter transaction processing must be guarded by an
[Amendment](https://xrpl.org/amendments.html).
All other changes that maintain the existing behavior do not need an
Amendment.

Ensure that your code compiles according to the build instructions in
[`BUILD.md`](./BUILD.md).
If you create new source files, they must go under `src/ripple`.
You will need to add them to one of the
[source lists](./Builds/CMake/RippledCore.cmake) in CMake.

Please write tests for your code.
If you create new test source files, they must go under `src/test`.
You will need to add them to one of the
[source lists](./Builds/CMake/RippledCore.cmake) in CMake.
If your test can be run offline, in under 60 seconds, then it can be an
automatic test run by `rippled --unittest`.
Otherwise, it must be a manual test.

The source must be formatted according to the style guide below.

Header includes must be [levelized](./Builds/levelization).


## Pull requests

Pull requests must target the `develop` branch.[^1]

[^1]: There are exceptions to this policy for hotfixes, but no one consulting
this document will be in that situation.

Changes to pull requests must be added as new commits.
You may **never force push a branch in a pull request** (e.g. after a rebase).
This preserves the ability for reviewers to filter changes since their last
review.

A pull request must obtain **approvals from at least two reviewers** before it
Copy link
Collaborator

Choose a reason for hiding this comment

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

"reviewer" in that context means "maintainer" - not "random person on Github clicking on reviewed", right?

Copy link
Collaborator Author

@thejohnfreeman thejohnfreeman Jan 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, because we have reviewers who are not maintainers, e.g. myself. The next sentence is there to state that it must be two qualified reviewers (maybe I'll add that adjective), where "qualified" is in the opinion of the maintainer who merges the PR.

can be considered for merge by a Maintainer.
Maintainers retain discretion to require more approvals if they feel the
credibility of the existing approvals is insufficient.

Pull requests must be merged by [squash-and-merge][2]
to preserve a linear history for the `develop` branch.


# Style guide
This is a non-exhaustive list of recommended style guidelines. These are not always strictly enforced and serve as a way to keep the codebase coherent rather than a set of _thou shalt not_ commandments.

This is a non-exhaustive list of recommended style guidelines.
These are not always strictly enforced and serve as a way to keep the codebase coherent rather than a set of _thou shalt not_ commandments.


## Formatting
All code must conform to `clang-format` version 10, unless the result would be unreasonably difficult to read or maintain.
To change your code to conform use `clang-format -i <your changed files>`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be worth keeping in, it should be easier to do the right thing than documenting only how to ignore the formatter. ;-)

Copy link
Collaborator Author

@thejohnfreeman thejohnfreeman Jan 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that documenting how to use clang-format is outside the scope of this document, but I will add this example back in.


All code must conform to `clang-format` version 10,
according to the settings in [`.clang-format`](./.clang-format),
unless the result would be unreasonably difficult to read or maintain.
To demarcate lines that should be left as-is, surround them with comments like
this:

```
// clang-format off
...
// clang-format on
```


## Avoid

1. Proliferation of nearly identical code.
2. Proliferation of new files and classes.
3. Complex inheritance and complex OOP patterns.
Expand All @@ -35,33 +121,52 @@ To change your code to conform use `clang-format -i <your changed files>`.
7. CPU or architecture-specific code unless there is a good reason to include it, and where it is used guard it with macros and provide explanatory comments.
8. Importing new libraries unless there is a very good reason to do so.


## Seek to

9. Extend functionality of existing code rather than creating new code.
10. Prefer readability over terseness where important logic is concerned.
11. Inline functions that are not used or are not likely to be used elsewhere in the codebase.
12. Use clear and self-explanatory names for functions, variables, structs and classes.
13. Use TitleCase for classes, structs and filenames, camelCase for function and variable names, lower case for namespaces and folders.
14. Provide as many comments as you feel that a competent programmer would need to understand what your code does.


# Maintainers
Maintainers are ecosystem participants with elevated access to the repository. They are able to push new code, make decisions on when a release should be made, etc.

## Code Review
New contributors' PRs must be reviewed by at least two of the maintainers. Well established prior contributors can be reviewed by a single maintainer.
Maintainers are ecosystem participants with elevated access to the repository.
They are able to push new code, make decisions on when a release should be
made, etc.


## Adding and Removing
New maintainers can be proposed by two existing maintainers, subject to a vote by a quorum of the existing maintainers. A minimum of 50% support and a 50% participation is required. In the event of a tie vote, the addition of the new maintainer will be rejected.
## Adding and removing

Existing maintainers can resign, or be subject to a vote for removal at the behest of two existing maintainers. A minimum of 60% agreement and 50% participation are required. The XRP Ledger Foundation will have the ability, for cause, to remove an existing maintainer without a vote.
New maintainers can be proposed by two existing maintainers, subject to a vote
by a quorum of the existing maintainers.
A minimum of 50% support and a 50% participation is required.
In the event of a tie vote, the addition of the new maintainer will be
rejected.

Existing maintainers can resign, or be subject to a vote for removal at the
behest of two existing maintainers.
A minimum of 60% agreement and 50% participation are required.
The XRP Ledger Foundation will have the ability, for cause, to remove an
existing maintainer without a vote.


## Current Maintainers

## Existing Maintainers
* [JoelKatz](https://github.com/JoelKatz) (Ripple)
* [Manojsdoshi](https://github.com/manojsdoshi) (Ripple)
* [N3tc4t](https://github.com/n3tc4t) (XRPL Labs)
* [Nikolaos D Bougalis](https://github.com/nbougalis)
* [Nixer89](https://github.com/nixer89) (XRP Ledger Foundation)
* [manojsdoshi](https://github.com/manojsdoshi) (Ripple)
* [n3tc4t](https://github.com/n3tc4t) (XRPL Labs)
* [Nik Bougalis](https://github.com/nbougalis)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nbougalis - confirm the way you prefer your name? (if any preference)

* [nixer89](https://github.com/nixer89) (XRP Ledger Foundation)
* [RichardAH](https://github.com/RichardAH) (XRPL Labs + XRP Ledger Foundation)
* [Seelabs](https://github.com/seelabs) (Ripple)
* [seelabs](https://github.com/seelabs) (Ripple)
* [Silkjaer](https://github.com/Silkjaer) (XRP Ledger Foundation)
* [WietseWind](https://github.com/WietseWind) (XRPL Labs + XRP Ledger Foundation)
* [Ximinez](https://github.com/ximinez) (Ripple)
* [Ed Hennis](https://github.com/ximinez) (Ripple)


[1]: https://docs.github.com/en/get-started/quickstart/contributing-to-projects
[2]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits