|
1 | | -Contributing to LNP/BP Core Library |
2 | | -=================================== |
3 | | - |
4 | | -The LNP/BP project operates an open contributor model where anyone is welcome to |
5 | | -contribute towards development in the form of peer review, documentation, |
6 | | -testing and patches. |
7 | | - |
8 | | -Anyone is invited to contribute without regard to technical experience, |
9 | | -"expertise", OSS experience, age, or other concern. However, the development of |
10 | | -standards & reference implementations demands a high-level of rigor, adversarial |
11 | | -thinking, thorough testing and risk-minimization. Any bug may cost users real |
12 | | -money. That being said, we deeply welcome people contributing for the first time |
13 | | -to an open source project or pick up Rust while contributing. Don't be shy, |
14 | | -you'll learn. |
15 | | - |
16 | | -Communications Channels |
17 | | ------------------------ |
| 1 | +Contributing guidelines |
| 2 | +======================= |
| 3 | + |
| 4 | +Contributions are very welcome. When contributing code, please follow these |
| 5 | +simple guidelines. |
| 6 | + |
| 7 | +#### Table Of Contents |
| 8 | +- [Contribution workflow](#contribution-workflow) |
| 9 | + * [Proposing changes](#proposing-changes) |
| 10 | + * [Preparing PRs](#preparing-prs) |
| 11 | + * [Peer review](#peer-review) |
| 12 | +- [Coding conventions](#coding-conventions) |
| 13 | +- [Security](#security) |
| 14 | +- [Testing](#testing) |
| 15 | +- [Going further](#going-further) |
| 16 | + |
| 17 | +Overview |
| 18 | +-------- |
18 | 19 |
|
19 | | -Communication about LNP/BP standards & implementations happens primarily |
20 | | -in #lnp-pb IRC chat on Freenode with the logs available at |
21 | | -<http://gnusha.org/lnp-bp/> |
| 20 | +* Before adding any code dependencies, check with the maintainers if this is okay. |
| 21 | +* Write properly formatted comments: they should be English sentences, eg: |
22 | 22 |
|
23 | | -Discussion about code base improvements happens in GitHub issues and on pull |
24 | | -requests. |
| 23 | + // Return the current UNIX time. |
25 | 24 |
|
26 | | -Major projects are tracked [here](https://github.com/orgs/LNP-BP/projects). |
27 | | -Major milestones are tracked [here](https://github.com/LNP-BP/rust-lnpbp/milestones). |
| 25 | +* Read the DCO and make sure all commits are signed off, using `git commit -s`. |
| 26 | +* Follow the guidelines when proposing code changes (see below). |
| 27 | +* Write properly formatted git commits (see below). |
| 28 | +* Run the tests with `cargo test --workspace --all-features`. |
| 29 | +* Make sure you run `rustfmt` on your code (see below details). |
| 30 | +* Please don't file an issue to ask a question. Each repository - or |
| 31 | + GitHub organization has a "Discussions" with Q&A section; please post your |
| 32 | + questions there. You'll get faster results by using this channel. |
28 | 33 |
|
29 | 34 | Contribution Workflow |
30 | 35 | --------------------- |
31 | | - |
32 | 36 | The codebase is maintained using the "contributor workflow" where everyone |
33 | 37 | without exception contributes patch proposals using "pull requests". This |
34 | 38 | facilitates social contribution, easy testing and peer review. |
35 | 39 |
|
36 | 40 | To contribute a patch, the workflow is a as follows: |
37 | 41 |
|
38 | | - 1. Fork Repository |
39 | | - 2. Create topic branch |
40 | | - 3. Commit patches |
| 42 | +1. Fork Repository |
| 43 | +2. Create topic branch |
| 44 | +3. Commit patches |
41 | 45 |
|
42 | | -In general commits should be atomic and diffs should be easy to read. For this |
43 | | -reason do not mix any formatting fixes or code moves with actual code changes. |
44 | | -Further, each commit, individually, should compile and pass tests, in order to |
| 46 | +In general commits should be atomic and diffs should be easy to read. For this |
| 47 | +reason do not mix any formatting fixes or code moves with actual code changes. |
| 48 | +Further, each commit, individually, should compile and pass tests, in order to |
45 | 49 | ensure git bisect and other automated tools function properly. |
46 | 50 |
|
47 | | -When adding a new feature thought must be given to the long term technical debt. |
| 51 | +When adding a new feature thought must be given to the long term technical debt. |
48 | 52 | Every new features should be covered by unit tests. |
49 | 53 |
|
50 | 54 | When refactoring, structure your PR to make it easy to review and don't hesitate |
51 | 55 | to split it into multiple small, focused PRs. |
52 | 56 |
|
53 | | -The Minimal Supported Rust Version is nightly for the period of active |
54 | | -development; it is enforced by our Travis. Later we plan to fix to some specific |
55 | | -Rust version after the initial library release. |
56 | | - |
57 | 57 | Commits should cover both the issue fixed and the solution's rationale. |
58 | | -These [guidelines](https://chris.beams.io/posts/git-commit/) should be kept in |
| 58 | +These [guidelines](https://chris.beams.io/posts/git-commit/) should be kept in |
59 | 59 | mind. |
60 | 60 |
|
61 | | -To facilitate communication with other contributors, the project is making use |
62 | | -of GitHub's "assignee" field. First check that no one is assigned and then |
63 | | -comment suggesting that you're working on it. If someone is already assigned, |
64 | | -don't hesitate to ask if the assigned party or previous commenters are still |
| 61 | +To facilitate communication with other contributors, the project is making use |
| 62 | +of GitHub's "assignee" field. First check that no one is assigned and then |
| 63 | +comment suggesting that you're working on it. If someone is already assigned, |
| 64 | +don't hesitate to ask if the assigned party or previous commenters are still |
65 | 65 | working on it if it has been awhile. |
66 | 66 |
|
67 | | -Branches information |
68 | | --------------------- |
| 67 | +### Proposing changes |
| 68 | + |
| 69 | +When proposing changes via a pull-request or patch: |
| 70 | + |
| 71 | +* Isolate changes in separate commits to make the review process easier. |
| 72 | +* Don't make unrelated changes, unless it happens to be an obvious improvement to |
| 73 | + code you are touching anyway ("boyscout rule"). |
| 74 | +* Rebase on `master` when needed. |
| 75 | +* Keep your changesets small, specific and uncontroversial, so that they can be |
| 76 | + merged more quickly. |
| 77 | +* If the change is substantial or requires re-architecting certain parts of the |
| 78 | + codebase, write a proposal in English first, and get consensus on that before |
| 79 | + proposing the code changes. |
| 80 | + |
| 81 | +### Preparing PRs |
69 | 82 |
|
70 | | -The main library development happens in the `master` branch. This branch must |
71 | | -always compile without errors (using Travis CI). All external contributions are |
| 83 | +The main library development happens in the `master` branch. This branch must |
| 84 | +always compile without errors (using Travis CI). All external contributions are |
72 | 85 | made within PRs into this branch. |
73 | 86 |
|
74 | | -Each commitment within a PR to the `master` must |
75 | | -* compile without errors; |
76 | | -* contain all necessary tests for the introduced functional; |
77 | | -* contain all docs. |
| 87 | +Prerequisites that a PR must satisfy for merging into the `master` branch: |
| 88 | +* the tip of any PR branch must compile and pass unit tests with no errors, with |
| 89 | + every feature combination (including compiling the fuzztests) on MSRV, stable |
| 90 | + and nightly compilers (this is partially automated with CI, so the rule |
| 91 | + is that we will not accept commits which do not pass GitHub CI); |
| 92 | +* contain all necessary tests for the introduced functional (either as a part of |
| 93 | + commits, or, more preferably, as separate commits, so that it's easy to |
| 94 | + reorder them during review and check that the new tests fail without the new |
| 95 | + code); |
| 96 | +* contain all inline docs for newly introduced API and pass doc tests; |
| 97 | +* be based on the recent `master` tip from the original repository at. |
| 98 | + |
| 99 | +NB: reviewers may run more complex test/CI scripts, thus, satisfying all the |
| 100 | +requirements above is just a preliminary, but not necessary sufficient step for |
| 101 | +getting the PR accepted as a valid candidate PR for the `master` branch. |
| 102 | + |
| 103 | +Additionally, to the `master` branch some repositories may have `develop` branch |
| 104 | +for any experimental developments. This branch may not compile and should not be |
| 105 | +used by any projects depending on the library. |
| 106 | + |
| 107 | +### Writing Git commit messages |
| 108 | + |
| 109 | +A properly formed git commit subject line should always be able to complete the |
| 110 | +following sentence: |
| 111 | + |
| 112 | + If applied, this commit will _____ |
| 113 | + |
| 114 | +In addition, it should be capitalized and *must not* include a period. |
| 115 | + |
| 116 | +For example, the following message is well formed: |
| 117 | + |
| 118 | + Add support for .gif files |
78 | 119 |
|
79 | | -Additionally to the `master` branch the repository has `develop` branch for any |
80 | | -experimental developments. This branch may not compile and should not be used by |
81 | | -any projects depending on `lnpbp` library. |
| 120 | +While these ones are **not**: `Adding support for .gif files`, |
| 121 | +`Added support for .gif files`. |
82 | 122 |
|
| 123 | +When it comes to formatting, here's a model git commit message[1]: |
83 | 124 |
|
84 | | -Peer review |
85 | | ------------ |
| 125 | + Capitalized, short (50 chars or less) summary |
| 126 | + |
| 127 | + More detailed explanatory text, if necessary. Wrap it to about 72 |
| 128 | + characters or so. In some contexts, the first line is treated as the |
| 129 | + subject of an email and the rest of the text as the body. The blank |
| 130 | + line separating the summary from the body is critical (unless you omit |
| 131 | + the body entirely); tools like rebase can get confused if you run the |
| 132 | + two together. |
| 133 | + |
| 134 | + Write your commit message in the imperative: "Fix bug" and not "Fixed bug" |
| 135 | + or "Fixes bug." This convention matches up with commit messages generated |
| 136 | + by commands like git merge and git revert. |
| 137 | + |
| 138 | + Further paragraphs come after blank lines. |
| 139 | + |
| 140 | + - Bullet points are okay, too. |
| 141 | + |
| 142 | + - Typically a hyphen or asterisk is used for the bullet, followed by a |
| 143 | + single space, with blank lines in between, but conventions vary here. |
| 144 | + |
| 145 | + - Use a hanging indent. |
| 146 | + |
| 147 | +### Peer review |
86 | 148 |
|
87 | 149 | Anyone may participate in peer review which is expressed by comments in the pull |
88 | 150 | request. Typically reviewers will review the code for obvious errors, as well as |
89 | 151 | test out the patch set and opine on the technical merits of the patch. PR should |
90 | | -be reviewed first on the conceptual level before focusing on code style or |
| 152 | +be reviewed first on the conceptual level before focusing on code style or |
91 | 153 | grammar fixes. |
92 | 154 |
|
93 | 155 | Coding Conventions |
94 | 156 | ------------------ |
95 | | - |
96 | | -Rust-fmt should be used as a coding style recommendations in general, with a |
97 | | -default coding style. By default, Rustfmt uses a style which conforms to the |
98 | | -[Rust style guide][style guide] that has been formalized through the [style RFC |
99 | | -process][fmt rfcs]. It is also required to run `cargo fmt` to make the code |
100 | | -formatted according to `rustfmt` parameters |
101 | | - |
| 157 | +Our CI enforces [clippy's](https://github.com/rust-lang/rust-clippy) |
| 158 | +[default linting](https://rust-lang.github.io/rust-clippy/rust-1.52.0/index.html) |
| 159 | +and [rustfmt](https://github.com/rust-lang/rustfmt) formatting defined by rules |
| 160 | +in [.rustfmt.toml](./.rustfmt.toml). The linter should be run with current |
| 161 | +stable rust compiler, while formatter requires nightly version due to the use of |
| 162 | +unstable formatting parameters. |
| 163 | + |
| 164 | +If you use rustup, to lint locally you may run the following instructions: |
| 165 | + |
| 166 | +```console |
| 167 | +rustup component add clippy |
| 168 | +rustup component add fmt |
| 169 | +cargo +stable clippy --workspace --all-features |
| 170 | +cargo +nightly fmt --all |
| 171 | +``` |
102 | 172 |
|
103 | 173 | Security |
104 | 174 | -------- |
| 175 | +Responsible disclosure of security vulnerabilities helps prevent user loss of |
| 176 | +privacy. If you believe a vulnerability may affect other implementations, please |
| 177 | +inform them. Guidelines for a responsible disclosure can be found in |
| 178 | +[SECURITY.md](./SECURITY.md) file in the project root. |
105 | 179 |
|
106 | | -Security is the primary focus of Rust-LNPBP; disclosure of security |
107 | | -vulnerabilities helps prevent user loss of funds. If you believe a vulnerability |
108 | | -may affect other implementations, please inform them. |
109 | | - |
110 | | -Note that Rust-LNPBP is currently considered "pre-production" during this time, |
111 | | -there is no special handling of security issues. Please simply open an issue on |
112 | | -Github. |
113 | | - |
114 | | -Testing |
115 | | -------- |
116 | | - |
117 | | -Related to the security aspect, Rust-LNPBP developers take testing very |
118 | | -seriously. Due to the modular nature of the project, writing new functional |
119 | | -tests is easy and good test coverage of the codebase is an important goal. |
120 | | -Refactoring the project to enable fine-grained unit testing is also an ongoing |
121 | | -effort. |
122 | | - |
123 | | -Fuzzing is heavily encouraged: feel free to add related material under `fuzz/` |
124 | | - |
125 | | -Mutation testing is planned; any contribution there would be warmly welcomed. |
| 180 | +Note that some of our projects are currently considered "pre-production". |
| 181 | +Such projects can be distinguished by the absence of `SECURITY.md`. In such |
| 182 | +cases there are no special handling of security issues; please simply open |
| 183 | +an issue on GitHub. |
126 | 184 |
|
127 | 185 | Going further |
128 | 186 | ------------- |
129 | | - |
130 | | -You may be interested by Jon Atack guide on [How to review Bitcoin Core PRs](https://github.com/jonatack/bitcoin-development/blob/master/how-to-review-bitcoin-core-prs.md) |
131 | | -and [How to make Bitcoin Core PRs](https://github.com/jonatack/bitcoin-development/blob/master/how-to-make-bitcoin-core-prs.md). |
132 | | -While there are differences between the projects in terms of context and maturity, many |
133 | | -of the suggestions offered apply to this project. |
| 187 | +You may be interested in Jon Atack guide on |
| 188 | +[How to review Bitcoin Core PRs][Review] and [How to make Bitcoin Core PRs][PR]. |
| 189 | +While there are differences between the projects in terms of context and |
| 190 | +maturity, many of the suggestions offered apply to this project. |
134 | 191 |
|
135 | 192 | Overall, have fun :) |
| 193 | + |
| 194 | +[1]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html |
| 195 | +[Review]: https://github.com/jonatack/bitcoin-development/blob/master/how-to-review-bitcoin-core-prs.md |
| 196 | +[PR]: https://github.com/jonatack/bitcoin-development/blob/master/how-to-make-bitcoin-core-prs.md |
0 commit comments