332 lines
15 KiB
Markdown
332 lines
15 KiB
Markdown
# Solana Coding Guidelines
|
|
|
|
The goal of these guidelines is to improve developer productivity by allowing
|
|
developers to jump into any file in the codebase and not need to adapt to
|
|
inconsistencies in how the code is written. The codebase should appear as if it
|
|
had been authored by a single developer. If you don't agree with a convention,
|
|
submit a PR patching this document and let's discuss! Once the PR is accepted,
|
|
*all* code should be updated as soon as possible to reflect the new
|
|
conventions.
|
|
|
|
## Pull Requests
|
|
|
|
Small, frequent PRs are much preferred to large, infrequent ones. A large PR is
|
|
difficult to review, can block others from making progress, and can quickly get
|
|
its author into "rebase hell". A large PR oftentimes arises when one change
|
|
requires another, which requires another, and then another. When you notice
|
|
those dependencies, put the fix into a commit of its own, then checkout a new
|
|
branch, and cherry-pick it.
|
|
|
|
```bash
|
|
$ git commit -am "Fix foo, needed by bar"
|
|
$ git checkout master
|
|
$ git checkout -b fix-foo
|
|
$ git cherry-pick fix-bar
|
|
$ git push --set-upstream origin fix-foo
|
|
```
|
|
|
|
Open a PR to start the review process and then jump back to your original
|
|
branch to keep making progress. Consider rebasing to make your fix the first
|
|
commit:
|
|
|
|
```bash
|
|
$ git checkout fix-bar
|
|
$ git rebase -i master <Move fix-foo to top>
|
|
```
|
|
|
|
Once the commit is merged, rebase the original branch to purge the
|
|
cherry-picked commit:
|
|
|
|
```bash
|
|
$ git pull --rebase upstream master
|
|
```
|
|
|
|
### How big is too big?
|
|
|
|
If there are no functional changes, PRs can be very large and that's no
|
|
problem. If, however, your changes are making meaningful changes or additions,
|
|
then about 1,000 lines of changes is about the most you should ask a Solana
|
|
maintainer to review.
|
|
|
|
### Should I send small PRs as I develop large, new components?
|
|
|
|
Add only code to the codebase that is ready to be deployed. If you are building
|
|
a large library, consider developing it in a separate git repository. When it
|
|
is ready to be integrated, the Solana Labs Maintainers will work with you to decide
|
|
on a path forward. Smaller libraries may be copied in whereas very large ones
|
|
may be pulled in with a package manager.
|
|
|
|
## Getting Pull Requests Merged
|
|
|
|
There is no single person assigned to watching GitHub PR queue and ushering you
|
|
through the process. Typically, you will ask the person that wrote a component
|
|
to review changes to it. You can find the author using `git blame` or asking on
|
|
Discord. When working to get your PR merged, it's most important to understand
|
|
that changing the code is your priority and not necessarily a priority of the
|
|
person you need an approval from. Also, while you may interact the most with
|
|
the component author, you should aim to be inclusive of others. Providing a
|
|
detailed problem description is the most effective means of engaging both the
|
|
component author and other potentially interested parties.
|
|
|
|
Consider opening all PRs as Draft Pull Requests first. Using a draft PR allows
|
|
you to kickstart the CI automation, which typically takes between 10 and 30
|
|
minutes to execute. Use that time to write a detailed problem description. Once
|
|
the description is written and CI succeeds, click the "Ready to Review" button
|
|
and add reviewers. Adding reviewers before CI succeeds is a fast path to losing
|
|
reviewer engagement. Not only will they be notified and see the PR is not yet
|
|
ready for them, they will also be bombarded with additional notifications
|
|
each time you push a commit to get past CI or until they "mute" the PR. Once
|
|
muted, you'll need to reach out over some other medium, such as Discord, to
|
|
request they have another look. When you use draft PRs, no notifications are
|
|
sent when you push commits and edit the PR description. Use draft PRs
|
|
liberally. Don't bug the humans until you have gotten past the bots.
|
|
|
|
### What should be in my PR description?
|
|
|
|
Reviewing code is hard work and generally involves an attempt to guess the
|
|
author's intent at various levels. Please assume reviewer time is scarce and do
|
|
what you can to make your PR as consumable as possible. Inspired by techniques
|
|
for writing good whitepapers, the guidance here aims to maximize reviewer
|
|
engagement.
|
|
|
|
Assume the reviewer will spend no more than a few seconds reading the PR title.
|
|
If it doesn't describe a noteworthy change, don't expect the reviewer to click
|
|
to see more.
|
|
|
|
Next, like the abstract of a whitepaper, the reviewer will spend ~30 seconds
|
|
reading the PR problem description. If what is described there doesn't look
|
|
more important than competing issues, don't expect the reviewer to read on.
|
|
|
|
Next, the reviewer will read the proposed changes. At this point, the reviewer
|
|
needs to be convinced the proposed changes are a *good* solution to the problem
|
|
described above. If the proposed changes, not the code changes, generates
|
|
discussion, consider closing the PR and returning with a design proposal
|
|
instead.
|
|
|
|
Finally, once the reviewer understands the problem and agrees with the approach
|
|
to solving it, the reviewer will view the code changes. At this point, the
|
|
reviewer is simply looking to see if the implementation actually implements
|
|
what was proposed and if that implementation is maintainable. When a concise,
|
|
readable test for each new code path is present, the reviewer can safely ignore
|
|
the details of its implementation. When those tests are missing, expect to
|
|
either lose engagement or get a pile of review comments as the reviewer
|
|
attempts to consider every ambiguity in your implementation.
|
|
|
|
### The PR Title
|
|
|
|
The PR title should contain a brief summary of the change, from the perspective
|
|
of the user. Examples of good titles:
|
|
|
|
* Add rent to accounts
|
|
* Fix out-of-memory error in validator
|
|
* Clean up `process_message()` in runtime
|
|
|
|
The conventions here are all the same as a good git commit title:
|
|
|
|
* First word capitalized and in the imperative mood, not past tense ("add", not
|
|
"added")
|
|
* No trailing period
|
|
* What was done, whom it was done to, and in what context
|
|
|
|
### The PR Problem Statement
|
|
|
|
The git repo implements a product with various features. The problem statement
|
|
should describe how the product is missing a feature, how a feature is
|
|
incomplete, or how the implementation of a feature is somehow undesirable. If
|
|
an issue being fixed already describes the problem, go ahead and copy-paste it.
|
|
As mentioned above, reviewer time is scarce. Given a queue of PRs to review,
|
|
the reviewer may ignore PRs that expect them to click through links to see if
|
|
the PR warrants attention.
|
|
|
|
### The Proposed Changes
|
|
|
|
Typically the content under the "Proposed changes" section will be a bulleted
|
|
list of steps taken to solve the problem. Oftentimes, the list is identical to
|
|
the subject lines of the git commits contained in the PR. It's especially
|
|
generous (and not expected) to rebase or reword commits such that each change
|
|
matches the logical flow in your PR description.
|
|
|
|
### The PR / Issue Labels
|
|
|
|
Labels make it easier to manage and track PRs / issues. Below some common labels
|
|
that we use in Solana. For the complete list of labels, please refer to the
|
|
[label page](https://github.com/solana-labs/solana/issues/labels):
|
|
|
|
* "feature-gate": when you add a new feature gate or modify the behavior of
|
|
an existing feature gate, please add the "feature-gate" label to your PR.
|
|
New feature gates should also always have a corresponding tracking issue
|
|
(go to "New Issue" -> "Feature Gate Tracker [Get Started](https://github.com/solana-labs/solana/issues/new?assignees=&labels=feature-gate&template=1-feature-gate.yml&title=Feature+Gate%3A+)")
|
|
and should be updated each time the feature is activated on a cluster.
|
|
|
|
* "automerge": When a PR is labelled with "automerge", the PR will be
|
|
automatically merged once CI passes. In general, this label should only
|
|
be used for small hot-fix (fewer than 100 lines) or automatic generated
|
|
PRs. If you're uncertain, it's usually the case that the PR is not
|
|
qualified as "automerge".
|
|
|
|
* "good first issue": If you happen to find an issue that is non-urgent and
|
|
self-contained with moderate scope, you might want to consider attaching
|
|
"good first issue" to it as it might be a good practice for newcomers.
|
|
|
|
### When will my PR be reviewed?
|
|
|
|
PRs are typically reviewed and merged in under 7 days. If your PR has been open
|
|
for longer, it's a strong indicator that the reviewers aren't confident the
|
|
change meets the quality standards of the codebase. You might consider closing
|
|
it and coming back with smaller PRs and longer descriptions detailing what
|
|
problem it solves and how it solves it. Old PRs will be marked stale and then
|
|
closed automatically 7 days later.
|
|
|
|
### How to manage review feedback?
|
|
|
|
After a reviewer provides feedback, you can quickly say "acknowledged, will
|
|
fix" using a thumb's up emoji. If you're confident your fix is exactly as
|
|
prescribed, add a reply "Fixed in COMMIT\_HASH" and mark the comment as
|
|
resolved. If you're not sure, reply "Is this what you had in mind?
|
|
COMMIT\_HASH" and if so, the reviewer will reply and mark the conversation as
|
|
resolved. Marking conversations as resolved is an excellent way to engage more
|
|
reviewers. Leaving conversations open may imply the PR is not yet ready for
|
|
additional review.
|
|
|
|
### When will my PR be re-reviewed?
|
|
|
|
Recall that once your PR is opened, a notification is sent every time you push
|
|
a commit. After a reviewer adds feedback, they won't be checking on the status
|
|
of that feedback after every new commit. Instead, directly mention the reviewer
|
|
when you feel your PR is ready for another pass.
|
|
|
|
### Is your PR easy to say "yes" to?
|
|
|
|
PRs that are easier to review are more likely to be reviewed. Strive to make
|
|
your PR easy to say "yes" to.
|
|
|
|
Non-exhaustive list of things that make it *harder* to review:
|
|
|
|
* Additional changes that are orthogonal to the problem statement and proposed
|
|
changes. Instead move those changes to a different PR.
|
|
* Renaming variables/functions/types unnecessarily and/or without explanation.
|
|
* Not following established conventions in the function/module/crate/repo.
|
|
* Changing whitespace: moving code and/or reformatting code. Make such changes
|
|
in a separate PR.
|
|
* Force-pushing the branch unnecessarily; this makes it harder to track any
|
|
previous comments on specific lines of code, and also harder to track changes
|
|
already reviewed from previous commits.
|
|
* When force-pushing is required—for example to handle a merge conflict—and
|
|
no new changes have been made since the previous review, indicating as such
|
|
is beneficial.
|
|
* Not responding to comments from previous rounds of review. Follow the
|
|
guidance in [How to manage review feedback?](#how-to-manage-review-feedback).
|
|
|
|
Non-exhaustive list of things that make it *easier* to review:
|
|
|
|
* Adding tests for all new/changed behavior.
|
|
* Including in the PR's description any non-automated testing that was
|
|
performed.
|
|
* Including relevant results for changes that target performance improvements.
|
|
|
|
Note that these lists are *independent* of how simple/complicated the actual
|
|
*code* changes are.
|
|
|
|
## Draft Pull Requests
|
|
|
|
If you want early feedback on your PR, use GitHub's "Draft Pull Request"
|
|
mechanism. Draft PRs are a convenient way to collaborate with the Solana
|
|
maintainers without triggering notifications as you make changes. When you feel
|
|
your PR is ready for a broader audience, you can transition your draft PR to a
|
|
standard PR with the click of a button.
|
|
|
|
Do not add reviewers to draft PRs. GitHub doesn't automatically clear
|
|
approvals when you click "Ready for Review", so a review that meant "I approve
|
|
of the direction" suddenly has the appearance of "I approve of these changes."
|
|
Instead, add a comment that mentions the usernames that you would like a review
|
|
from. Ask explicitly what you would like feedback on.
|
|
|
|
## Crate Creation
|
|
|
|
If your PR includes a new crate, you must publish its v0.0.1 version
|
|
before the PR can be merged. Here are the steps:
|
|
|
|
* Create a sub-directory for your new crate.
|
|
* Under the newly-created directory, create a Cargo.toml file. Below is an
|
|
example template:
|
|
|
|
```toml
|
|
[package]
|
|
name = "solana-<PACKAGE_NAME>"
|
|
version = "0.0.1"
|
|
description = "<DESCRIPTION>"
|
|
authors = ["Solana Labs Maintainers <maintainers@solanalabs.com>"]
|
|
repository = "https://github.com/solana-labs/solana"
|
|
homepage = "https://solana.com/"
|
|
documentation = "https://docs.rs/solana-<PACKAGE_NAME>"
|
|
license = "Apache-2.0"
|
|
edition = "2021"
|
|
```
|
|
|
|
* Submit the PR for initial review. You should see the crate-check CI
|
|
job fails because the newly created crate is not yet published.
|
|
|
|
* Once all review feedback has been addressed, publish v0.0.1 of the crate
|
|
under your personal crates.io account, and then transfer the crate ownership
|
|
to solana-grimes.
|
|
https://crates.io/policies#package-ownership
|
|
|
|
* After successful publication, update the PR by replacing the v0.0.1 version
|
|
number with the correct version. At this time you should see the crate-check
|
|
CI job passes, and your published crate should be available under
|
|
https://crates.io/crates/.
|
|
|
|
## Rust coding conventions
|
|
|
|
* All Rust code is formatted using the latest version of `rustfmt`. Once
|
|
installed, it will be updated automatically when you update the compiler with
|
|
`rustup`.
|
|
|
|
* All Rust code is linted with Clippy. If you'd prefer to ignore its advice, do
|
|
so explicitly:
|
|
|
|
```rust
|
|
#[allow(clippy::too_many_arguments)]
|
|
```
|
|
|
|
Note: Clippy defaults can be overridden in the top-level file `.clippy.toml`.
|
|
|
|
* For variable names, when in doubt, spell it out. The mapping from type names
|
|
to variable names is to lowercase the type name, putting an underscore before
|
|
each capital letter. Variable names should *not* be abbreviated unless being
|
|
used as closure arguments and the brevity improves readability. When a function
|
|
has multiple instances of the same type, qualify each with a prefix and
|
|
underscore (i.e. alice\_keypair) or a numeric suffix (i.e. tx0).
|
|
|
|
* For function and method names, use `<verb>_<subject>`. For unit tests, that
|
|
verb should always be `test` and for benchmarks the verb should always be
|
|
`bench`. Avoid namespacing function names with some arbitrary word. Avoid
|
|
abbreviating words in function names.
|
|
|
|
* As they say, "When in Rome, do as the Romans do." A good patch should
|
|
acknowledge the coding conventions of the code that surrounds it, even in the
|
|
case where that code has not yet been updated to meet the conventions described
|
|
here.
|
|
|
|
|
|
## Terminology
|
|
|
|
Inventing new terms is allowed, but should only be done when the term is widely
|
|
used and understood. Avoid introducing new 3-letter terms, which can be
|
|
confused with 3-letter acronyms.
|
|
|
|
[Terms currently in use](https://solana.com/docs/terminology)
|
|
|
|
|
|
## Design Proposals
|
|
|
|
This Solana validator client's architecture is described by docs generated from markdown files in the `docs/src/`
|
|
directory and viewable on the official [Solana Labs Validator Client](https://docs.solanalabs.com) documentation website.
|
|
|
|
Current design proposals may be viewed on the docs site:
|
|
|
|
1. [Accepted Proposals](https://docs.solanalabs.com/proposals/accepted-design-proposals)
|
|
2. [Implemented Proposals](https://docs.solanalabs.com/implemented-proposals/implemented-proposals)
|
|
|
|
New design proposals should follow this guide on [how to submit a design proposal](./docs/src/proposals.md#submit-a-design-proposal).
|