|
| 1 | +# Contributing to `Krux` firmware |
| 2 | + |
| 3 | +The development of `Krux` firmware is FOSS and community effort based and |
| 4 | +welcomes contributions from anyone. We are excited you are interested in |
| 5 | +helping us bringing sovereign and private self-custody to everyone. |
| 6 | + |
| 7 | +We welcome contributions in many forms, including bug reports, feature |
| 8 | +requests, code contributions, and documentation improvements. From any |
| 9 | +contributors with any level of experience or expertise. |
| 10 | + |
| 11 | +Besides that,we ask that you respect others and follow the process |
| 12 | +outlined in this document. |
| 13 | + |
| 14 | +## Non `Krux` firmware contributions |
| 15 | + |
| 16 | +This document do not covers any contributions to fields besides the |
| 17 | +firmware and we developers of Selfcustody team aren't responsible for any |
| 18 | +event on that field. |
| 19 | + |
| 20 | +## Communications Channels |
| 21 | + |
| 22 | +The primary communication channel is the [GitHub repository](https://github.com/selfcustody/krux). |
| 23 | + |
| 24 | +## Contribution Workflow |
| 25 | + |
| 26 | +The contribution workflow is designed to facilitate cooperation and ensure a |
| 27 | +high level of quality in the project and not to enforce any type of dogmatic |
| 28 | +procedure on creative level. The process is as follows: |
| 29 | + |
| 30 | +To contribute a patch, the workflow is as follows: |
| 31 | + |
| 32 | + 1. Fork Repository |
| 33 | + 2. Create topic branch |
| 34 | + 3. Commit patches |
| 35 | + |
| 36 | +### Fork Repository |
| 37 | + |
| 38 | +To fork a repository you need a basic understanding of how git and GitHub works. |
| 39 | + |
| 40 | +### Create a topic branch |
| 41 | + |
| 42 | +You always start with a `master` branch initialized. While the community of |
| 43 | +developers standardized this as the **stable** development branch, the `Krux` |
| 44 | +firmware team stated this as the **stable** branch and the `develop` branch |
| 45 | +as the branch that all developers should start a new branch properly named |
| 46 | +to identify when we release: |
| 47 | + |
| 48 | +```bash |
| 49 | +main -> develop -> chore/task-stuff |
| 50 | + -> ci/job-stuff |
| 51 | + -> docs/info-new |
| 52 | + ... |
| 53 | +``` |
| 54 | + |
| 55 | +So, in command line you could do this, keeping in mind what you will do in |
| 56 | +the PR: |
| 57 | + |
| 58 | +```bash |
| 59 | +git checkout -b <type/name> |
| 60 | +``` |
| 61 | + |
| 62 | +Check the item below to proper branch `type/name`. |
| 63 | + |
| 64 | +``` |
| 65 | +``` |
| 66 | + |
| 67 | +### Commit patches |
| 68 | + |
| 69 | +A patch is a set of changes (patches or `diffs`) that you purpose to `Krux` |
| 70 | +firmware developers to include in `develop` branch as a (set) of commits. |
| 71 | + |
| 72 | +In general commits should be atomic and `diffs` should be easy to read. |
| 73 | +For this reason do not mix any formatting fixes or code moves with actual code |
| 74 | +changes. Further, each commit, individually, should as possible compile and |
| 75 | +pass tests, in order to ensure that the `git bisect` and other automated tools |
| 76 | +function properly. |
| 77 | + |
| 78 | +When adding a new feature ensure that it is covered by functional tests where |
| 79 | +possible. Avoiding this will create uncovered codes. |
| 80 | + |
| 81 | +When refactoring, structure your PR to make it easy to review and don't |
| 82 | +hesitate to split it into multiple small, focused PRs. |
| 83 | + |
| 84 | +The Minimum Supported Python Version is **3.12.0** (enforced by our CI). |
| 85 | +Commits should cover both the issue fixed and the solution's rationale. |
| 86 | + |
| 87 | +These [guidelines](https://chris.beams.io/posts/git-commit/) should be kept in |
| 88 | +mind. Commit messages follow the |
| 89 | +["Conventional Commits 1.0.0"](https://www.conventionalcommits.org/en/v1.0.0/) |
| 90 | +to make commit histories easier to read by humans and automated tools. |
| 91 | +The types of commits we use are: |
| 92 | + |
| 93 | +- chore: maintenance tasks; |
| 94 | +- ci: continuous integration; |
| 95 | +- docs: documentation changes; |
| 96 | +- feat: new feature; |
| 97 | +- fix: bug fix; |
| 98 | +- refactor: code change that neither fixes a bug nor adds a feature; |
| 99 | +- style: formatting, missing semi colons, etc; no code change; |
| 100 | +- test: adding missing tests or correcting existing tests. |
| 101 | + |
| 102 | +It is strongly encouraged that you |
| 103 | +[GPG sign](https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits) |
| 104 | +your commits. |
| 105 | + |
| 106 | +## Peer review |
| 107 | + |
| 108 | +To make sure our code has the highest quality and is maintainable for |
| 109 | +posterity, we have a thorough peer review process, where pull requests need |
| 110 | +to be reviewed by at least one maintainer, and must not have any outstanding |
| 111 | +comment from regular contributors. |
| 112 | + |
| 113 | +### Conceptual Review |
| 114 | + |
| 115 | +A review can be a conceptual review, where the reviewer leaves a comment: |
| 116 | + |
| 117 | +- Concept (n)ACK: "I do (not) agree with the general goal of this pull request"; |
| 118 | +- Approach (n)ACK: Concept (N)ACK, but "I do (not) agree with the approach of |
| 119 | + this change"; |
| 120 | +- Untested (u)ACK: "I didn't test, but ACK"; |
| 121 | +- Tested (t)ACK: "I tested and ACKed". |
| 122 | + |
| 123 | +A NACK needs to include a rationale why the change is not worthwhile. |
| 124 | +NACKs without accompanying reasoning may be disregarded: |
| 125 | + |
| 126 | +### Code Review |
| 127 | + |
| 128 | +After conceptual agreement on the change, code review can be provided. |
| 129 | +A review begins with ACK BRANCH_COMMIT, where BRANCH_COMMIT is the top of |
| 130 | +the PR branch, followed by a description of how the reviewer did the review. |
| 131 | +The following language is used within pull request comments: |
| 132 | + |
| 133 | +"I have tested the code", involving change-specific manual testing in |
| 134 | +addition to running the unit, functional, or fuzz tests, and in case it is not |
| 135 | +obvious how the manual testing was done, it should be described; |
| 136 | +"I have not tested the code, but I have reviewed it and it looks OK, I agree |
| 137 | +it can be merged"; |
| 138 | +A "nit" refers to a trivial, often non-blocking issue. |
| 139 | +Project maintainers reserve the right to weigh the opinions of peer reviewers |
| 140 | +using common sense judgement and may also weigh based on merit. Reviewers that |
| 141 | +have demonstrated a deeper commitment and understanding of the project over |
| 142 | +time or who have clear domain expertise may naturally have more weight, as one |
| 143 | +would expect in all walks of life. |
| 144 | + |
| 145 | + |
| 146 | +```markdown |
| 147 | +tACK c00febabe |
| 148 | + |
| 149 | +I liked the approach! Just some minor nit: |
| 150 | + |
| 151 | +- the line 3 has a typo on comment: `Helllo` should be `Hello`. So |
| 152 | +I suggest this change: |
| 153 | + |
| 154 | +<diff> |
| 155 | +- # The resulting Helllo |
| 156 | ++ # The resulting Hello |
| 157 | +result = "Hello" |
| 158 | +</diff> |
| 159 | + |
| 160 | +Besides that, is a neat work @user! |
| 161 | +``` |
| 162 | +``` |
| 163 | +``` |
| 164 | + |
| 165 | + |
| 166 | +## Coding Conventions |
| 167 | + |
| 168 | +There's a few rules to make sure the code is readable and maintainable. |
| 169 | +Most of them are checked by `poetry run poe lint` and `poetry run poe format`, |
| 170 | +and are enforced by CI, both for python and markdown codes. |
| 171 | + |
| 172 | + |
| 173 | +### Python |
| 174 | + |
| 175 | +```python |
| 176 | +# The MIT License (MIT) |
| 177 | + |
| 178 | +# Copyright (c) 2021-2025 Krux contributors |
| 179 | + |
| 180 | +# ... |
| 181 | +class Foo: |
| 182 | + """Some awesome comment""" |
| 183 | + |
| 184 | + def func(self): |
| 185 | + """Some awesome comment""" |
| 186 | + pass |
| 187 | +``` |
| 188 | + |
| 189 | +All files, in theory, should have the MIT license. |
| 190 | +All new features require testing. Tests should be unique and self-describing. |
| 191 | +When it comes error handling, we prefer exact and meaningful error handling |
| 192 | +to deliver consumers(developers and users) an accurate error that describes |
| 193 | +exactly what happened wrong. Instead of: |
| 194 | + |
| 195 | +```python |
| 196 | +# A function that concatenate a int and a float into a str |
| 197 | +def foo( |
| 198 | + bar: int, |
| 199 | + baz: float, |
| 200 | +) -> str: |
| 201 | + str(bar) + str(baz) |
| 202 | +``` |
| 203 | + |
| 204 | +prefer: |
| 205 | + |
| 206 | +```python |
| 207 | +# A function that concatenate a int and a float into a str and why do that |
| 208 | +# for a good and reasonable reason that you should discuss with other developers |
| 209 | +def foo( |
| 210 | + bar: int, |
| 211 | + baz: float, |
| 212 | +) -> str: |
| 213 | + """I do that because i liked"""" |
| 214 | + bar_str = str(bar) |
| 215 | + baz_str = str(baz) |
| 216 | + |
| 217 | + # We need to concatenate to be compliant |
| 218 | + return bar_str + baz_str |
| 219 | +``` |
| 220 | + |
| 221 | +### Markdown |
| 222 | + |
| 223 | +Markdown files are linted too. This keep the `*.md` files standardized and |
| 224 | +proper formated to any code editor. Some of the current applied rules enforced |
| 225 | +by CI: |
| 226 | + |
| 227 | +```markdown |
| 228 | + |
| 229 | +- \# heading should be toplevel heading; |
| 230 | +- lines should be compact: 80 chars per line; |
| 231 | +- raw links aren't allowed, use the [text](link) format instead; |
| 232 | +- items should follow correct identation. |
| 233 | +``` |
| 234 | +``` |
| 235 | +``` |
| 236 | + |
| 237 | +## Testing |
| 238 | + |
| 239 | +We expect to have 100% test coverage for critical parts, |
| 240 | +and a decent level of coverage for everything. We have a |
| 241 | +few types of tests that you can run using `poetry run poe test`. |
| 242 | + |
| 243 | +## Release |
| 244 | + |
| 245 | +Once a maintainer and the contributors decide we have a stable enough `develop` |
| 246 | +with sufficient features, we will merge the `develop` branch with the `main` |
| 247 | +at that point. From this point, all new changes will go in the next release. |
| 248 | +After sufficient testing and making sure we don't have bugs left, this branch |
| 249 | +will be released by one of the maintainers. |
| 250 | + |
| 251 | +The release will have pre-built binaries available on github's asset page. |
| 252 | +They **must** be GPG signed, and have a list of hashes for each asset. |
| 253 | + |
| 254 | +If we find bugs on a release, the fix may be backported and a new minor release |
| 255 | +may be released. This is done by merging fixes on top of the release branch. |
| 256 | +And then performing another release on that branch. |
| 257 | + |
| 258 | +If you have any questions, related to this process or the codebase in general. |
| 259 | +Don't hesitate to reach us out, we are happy to help newcomers in their amazing |
| 260 | +journey. Overall, have fun :) |
0 commit comments