Thanks for considering contributing to Subspace Network!
The best way to start is to join our Forum or Discord, official website has a bunch of learning materials too.
GitHub is primarily used by developers to work on new features, improvements and fix bugs. If you're not sure whether you have an actual bug that needs to be fixed or you just want to ask a question, forum is the best place for that.
This document describes expectations and recommendations that should make contributions such as issues and pull requests on GitHub productive for everyone involved.
Subspace Network is maintained by a small group of passionate individuals with limited time. Number of potential outside contributors is much larger than number of maintainers and wider community of users is even bigger than that.
As such, the process of contributing to Subspace Network needs to be optimized in such a way that it doesn't overwhelm maintainers with unnecessary notifications, questions that can be answered by wider community and follow practices defined in this document below, such that contributions can be processed in the most productive way saving time and making it a better experience overall.
These should be submitted in such a way that they are actionable by developers. Again, if you're unsure or just have a question - use forum, GitHub issue can be created later if it is necessary.
Always search for similar topics already created on the forum or here on GitHub before creating a new one, but don't hijack threads that looks somewhat similar, but are not necessarily the same or have the same root cause.
For bug report to be actionable it should ideally follow the following template:
- On Ubuntu 22.04
- Run
subspace-farmer-ubuntu-x86_64-gemini-1b-2022-jun-18 farmfrom official release - Observe an error
Farmer starts successfully
This error
error: The following required arguments were not provided:
--reward-address <REWARD_ADDRESS>
--plot-size <PLOT_SIZE>
USAGE:
subspace-farmer-ubuntu-x86_64-gemini-1b-2022-jun-18 [OPTIONS] --reward-address <REWARD_ADDRESS> --plot-size <PLOT_SIZE>
For more information try --help
That is an invalid bug report, of course, but you get the idea of what it should look like.
Try to provide as much useful information as possible, but not too much and without rambling, so that developers can quickly diagnose and address it.
When there is something that can be improved or a feature is lacking, it is probably a good idea to start with the forum and search for similar ideas and if there isn't a topic already, create one to see if this would be a welcome change.
Again, try to discuss with wider audience whether it is a good idea before reaching out to maintainers directly.
When it is necessary to create an issue here, try to describe in a concise way what would the improvement or new feature look like, potentially using a similar template as for reporting bugs describing expected user flow there.
Code contributions are more involved, but the simplest rule here is to observe what maintainers do, how existing code looks like and mimic that, but we provide more specific requirements below.
We follow traditional Rust code style with some clarifications and set in rustfmt.toml. We also follow all Clippy
lints (with rare suppressions). As such every commit should successfully pass cargo fmt checks and produce zero
warnings when running cargo clippy. This rule includes not just library and application code, but also examples and
tests.
Every file should end with one and exactly one new line, configure your IDE to insert it automatically. Lines should
not end with whitespaces, this includes code, comments, documentation (*.md files), configs and everything else.
Pay attention to existing code and mimic its structure, for instance we keep dependencies sorted order in Cargo.toml
and if you submit changes where new dependency is added out-of-order you're introducing unnecessary entropy and irritate
maintainers.
If there is an option between being smart with a tricky piece of code or obvious, prefer obvious. Write idiomatic Rust
as good as you can, this often allows to avoid needing .unwrap().
Struct definition and its implementation should be in the same file, ideally with nothing else in between.
In blockchain code use of .unwrap() is strictly forbidden and must never be used (except test code), use .expect()
instead. .expect() must convince reviewer that it will never panic or why panicking might be the preferred outcome
(rarely, but sometimes happens). .expect() correctness should be derivable from local context if it is a standalone
function or invariants of a data structure for internal method. If it can't be guaranteed then Result<T, E> must be
used instead. There are rare exceptions to this such as block numbers that never exceed u32 in Subspace.
Indexing into slices and similar data structures with [] should be avoided and replaced with .get() or similar
methods with explicit handling of cases where element doesn't exist.
Same goes about mathematical operations, especially in runtime logic, in most cases use explicit checked, wrapping or
saturating math, but don't use saturating math just to make things compile if business logic doesn't expect overflow to
ever happen, use checked math with .expect() in such cases or return an error.
Unsafe code is to be avoided except in special cases. If you can't make code to compile and decide to use unsafe to
bypass compiler, chances are you're doing it wrong and should ask for help to avoid unsafe. When you do use unsafe
for valid reasons, just like with .expect(), you need to provide a proof convincing reviewer that it is safe to do so
and can't be exploited with public API from safe Rust.
If code is incomplete or has known issues, make sure to leave a TODO explaining what needs to be done or what isn't handled properly. For big things consider creating an issue.
Prefer longer names of variables, variables with 1-3 characters a typically a bad choice. There are notable exceptions
like using id in entity data structure or i for a simple iteration over things.
Otherwise, look at existing code and try to do your best.
Commits should tell a logical step by step story of the changes and ideally be individually meaningful. Use squashing and rebasing during development, reorder commits if it makes more sense that way, there is great tooling out there for these kinds of modifications.
Reviewer should be able to go through commits one by one and understand what is being changed, with non-trivial changes it is often very hard to review the final diff, so going through commits helps a lot. For this commits should be well-structured and there should be a reasonable number of them. 70 commits for 100 lines or changes is most likely bad, same with one commit that changes many thousands of lines of code in various places at once (lock files are a frequent and notable exception).
If you work on a branch and after a few commits notice that API introduced earlier doesn't work well or need to revert some changes, amend original commit instead of creating a new one much later, since reviewer going through individual commits will likely have a comment about something that was already fixed and will simply have to spend more time reviewing changes back and forth.
Different kinds of changes should ideally be in different commits if not pull requests entirely. For instance if you want to move something and change, move in one commit so reviewer can have a quick look without thinking about it too much and apply actual changes in a separate commit. Same with file renaming, if you rename file and change it significantly in the same commit it'll make reviewer's life very difficult and likely convince Git that one file was deleted and another file added, which breaks tooling for tracking file changes across renames.
It is sometimes the case that some refactoring needs to be done during feature development, try to extract those refactoring commits you have just done into a separate branch, submit it for review and rebase your working branch on refactoring branch so that the changes are clearly separated and easier to review as an example.
Please push your commits regularly, but not necessarily every commit since it may occupy CI time unnecessarily.
And finally, write commit messages in present tense (as opposed to past) and explain changes that should be done, for instance here is a good example:
finalize header at archiving depth and prune any forks at that number
Avoid garbage commit names like "fix", "wip", "🤦 x 2", "......", "AHHHHHHHHH" (those are real examples). In most cases those commits probably shouldn't exist separately and better squashed with some other commit.
Here is a good article from GitHub on this topic: Write Better Commits, Build Better Projects.
It is important to remember that there are many people subscribed to the repository and any changes to pull request will trigger notifications to tens, hundreds, potentially thousands of people. As such try to minimize them or else many people, including maintainers, will unsubscribe and response time for issues and pull request changes will increase dramatically.
Before even submitting pull request make sure to really do self-review. This will allow you to spot minor issues like typos or code you didn't intend to commit in the first place. At this stage use force-pushes to edit original commits to make sure you like the changes yourself.
As explained in commits section above, history should be clean and readable, ready for reviewer to go through and make sense of changes proposed.
Make sure to test your own code locally and/or in CI before submitting a pull request to avoid numerous updates afterwards to fix CI (remember, those changes will trigger extra notifications that could be avoided by following this simple rule). Make sure to add new test cases if applicable.
Pull request should be accompanied by reasonable description. If the change is a trivial typo, description can be avoided completely, but if changes are non-trivial, make sure to explain what changes do, why, what were the alternatives and so on, giving reviewer proper context into changes proposed. If there is a relevant issue or another pull request then link it, if there is an issue that is resolved by proposed changes or PR that becomes obsolete, use corresponding keywords as described in documentation.
Pull request should be ready for review, avoid drafts that are frequently updated, in most cases you can share WIP changes with someone just by sharing a branch diff. Drafts have a narrow and rare use case, use them as last resort, otherwise they become a huge source of distraction due to number of notifications they generated. In most cases you either want changes to be reviewed and merged, in which case regular pull request should be created, or you don't want pull request at all.
Once PR is created, a rule of thumb is to avoid force pushes since it is not always easy to understand what has changed
in this case and forces maintainers to re-review pull request from scratch. Prefer meaningful commits on top instead.
One exception to this is fixing typos or trivial renaming, in that case GitHub will show that force push had trivial
changes, make sure to not rebase on some branch at the same time, it will result in huge diff! It will also potentially
make old commits non-compilable or will cause tests fail, which hurts potential future debugging. Better merge main
later with a button on GitHub. Another exception is when major refactoring is requested that changes the shape of the PR
completely, especially if it reduces its size, in such case it is also easier to re-review rather than analyzing diff on
top of undesirable changes that will pollute the history (opening another PR removes previous context, so generally is
not recommended, but remains an option).
In case you need to test fixes unknown CI issue for branch that already has PR created for it, create a different test branch and push/force-push commits there until you fix it instead of updating PR all the time. If you notice something is missing in the PR, convert it to draft temporarily, so it is clear for others that it is incomplete.
If there were changes requested and addressed, make sure to resolve those that are trivially 100% addressed by recently pushed changes and leave others to resolve by the person that requested changes. Once you have addressed review comments with responses or code changes, make sure to re-request review from that person such that they are aware that PR is ready for review again. If there are several unrelated changes requested, it is better to create a few commits and push them all at once as opposed to pushing multiple commits one by one or pushing one big commit addressing all feedback. For a few simple changes one commit is fine though.
In case there are a few branches that depend on each other, you can submit a PR targeting branch other than main and
once that branch is merged into main, target branch in the pull request will be updated automatically, that way you
can submit a few distinct PRs from a bigger set of changes in a way that is easier for review, though this is only
applicable to those having access to creating branches in the repository.
When leaving more than one comment, make sure to post them as a review from "File changed" tab, you can do that even if you authored pull request. Once done with comments, submit a review all at once, this also minimized amount of distraction for maintainers.
Merging of pull requests should be done without squashing in most cases. This makes it possible to track when changes happen, leave moving and refactoring of files separated when done in the same PR, helps with bisection of problematic changes. Only trivial changes should be squashed or those that are not reviewable for other reasons, though above documentation explains how to avoid that.
In general, try to maximize reviewer performance because code is written once by you, but will be read many times by maintainers, external auditors and anyone else who might be interested later on.