Thanks for considering contributing to the NEAR MPC network. Whether you're an external developer looking to make a contribution or a member of the NEAR One MPC team working in this repo every day, this guide should help you understand our workflow and our expectations on contributions.
There are several ways to contribute to the MPC network. The following are the most common:
- Run an MPC node.
- Spot a problem or potential improvement and raise an issue.
- Open PRs to resolve issues.
- Review PRs to verify they are correct and follow our standards.
If you're interested in running an MPC node, please reach out to us directly at mpc@nearone.org. The rest of this guide is focused on our flow for creating issues and PRs.
We use issues to define and track changes to our system. A good issue should contain sufficient context so that any developer can pick it up and implement an acceptable solution to it.
We use the following template to ensure all issues are well-defined and provide relevant context:
- Background: Explains relevant aspects of the current state of things and highlights what's currently lacking/problematic.
- Acceptance Criteria: A condition that must be met for the issue to be regarded as completed.
- User Story: (optional) Follows the form: "As a , I want to , so that I can ". This helps clarify the relevance of the issue.
Issues should contain relevant links to other issues and PRs for more context,
and be tagged with relevant labels (e.g. contract, launcher, security, tech-debt, bug).
This helps keep the issues discoverable and easy to track.
Congratulations, you've found an issue you want to work on. Now, please do the following things:
- Assign yourself to the issue to ensure it's visible you're working on it.
- Create a branch and link it to the issue. We recommend using the GitHub UI to create the branch as it will automatically link it and use a canonical format for the branch name.
- Check out the branch and get to work.
Once you've made the relevant changes to fulfill the acceptance criteria and the code lives up to our engineering standards, it's time to open a PR.
When opening the PR, make sure to link the issue in the PR description. Add any other notes that will be helpful for reviewers in the description. However, in many cases it is sufficient to only reference the issue in the PR description.
To ensure consistent high quality code, every PR must conform to the following principles.
- Don't panic
- Maintain local reasonability
- Use safe arithmetic methods
- Separate business logic from I/O
- Add tests
- Measure performance
Beyond our engineering standards, The Rust library team maintains a set of API guidelines. We should also try to follow these to the greatest extent possible where applicable.
While there are a few exceptions to this rule, most of our code should be panic-free.
Therefore we should avoid calling .expect(), .unwrap() in production code, as
well as any other methods that may implicitly or explicitly panic.
The three exceptions to this rule are:
- The top level main function may panic if it is not able to parse arguments, configuration or anything else needed to start the node in the first place.
- In the smart contract, as panicking ensures no side-effects happen in the transaction.
- Code paths that are guaranteed to be dead by runtime invariants.
In the first case there's no harm in panicking since the node hasn't started. The second one is the convention of NEAR smart contracts.
However, the third case is more subtle and debatable. We should minimize dead code paths, but occasionally it's possible to hit scenarios when you can guarantee that a panic won't occur.
Take this code for example:
/// Increments the first number of the slice
fn increment_first_number(numbers: &mut [u8]) {
if numbers.is_empty() {
return;
}
// May not panic since we check above that the array is non-empty
*numbers.get_mut(0).unwrap() += 1
}This function contains an .unwrap(), but we can still guarantee that
this function will never ever panic. Therefore it's not violating
the "Don't panic" principle.
It's often tempting to write code that has implicit sequential dependencies. In these scenarios, the correctness of one expression depends on the presence of other expressions.
Considering the code example from the Don't panic section,
the correctness of the .unwrap() can only be reasoned about by understanding
the numbers.is_empty() check. This entangles these two expressions, making
it harder to reason about and review the code.
This also allows the risk of a future PR accidentally removing the .is_empty()
check potentially causing the panic to occur in production.
If we instead wrote the function the following way:
/// Increments the first number of the slice
fn increment_first_number(numbers: &mut [u8]) {
if let Some(first_number) = numbers.get_mut(0) {
*first_number += 1;
}
}we'd have no sequential dependencies. This code cannot become incorrect by some externally maintained invariant being broken because it doesn't make any assumptions about any surrounding code.
Therefore we recognize the variant as the better alternative.
It's convenient to use the arithmetic operators (+, -, *)
for mathematical expressions. However, these may panic or overflow/underflow
depending on the compiler. This can lead to unintended behavior and
it's best to completely avoid them for primitive types in favor of
explicit methods (checked_add, wrapping_add etc.).
Note that for custom types these operators may still be fine, if they are implemented so that they don't overflow.
For example, typical implementations of cryptographic scalars in some field use modular arithmetic and therefore have well defined behavior when used with these operators.
// Don't
let z = x + y;
// Do
if let Some(z) = x.checked_add(y) {
// Do stuff with z
} else {
// Handle error or explain why this will never happen and panic
}Imagine you're asked to implement a function that posts incrementing fibonacci numbers after waiting the same amount of hours as the current number to some endpoint.
It would be very tempting to write the function the following way.
async fn post_fibonacci_sequence(n: usize) {
for i in 0..n {
tokio::time::sleep(Duration::from_hours(i)).await;
reqwest::Client::new()
.post("https://fibonacci.org/sequence")
.body(i.to_string())
.send()
.await;
}
}Functionally speaking there's nothing wrong with this implementation. It fulfills the requirements and it would work correctly in production.
This is not a problem for a dummy example, but for any non-trivial system it is wise to add tests for important functionality to ensure it works as expected. This is where the naive implementation becomes problematic, because how do you test this system?
The naive way of testing this would be to add a configuration value for the endpoint and auto-advance tokio time in the test. The tests could spin up their own web server which receives the request and asserts that it matches the expectation.
Doing all this work just for a simple test case is both very complex, and completely fails to test half of the functionality in this function. You can assert that the values received to the endpoint are correct but how do you measure that the sleep time is right?
The core problem is that this function mixes I/O and business logic. If we instead broke out the I/O from the function by introducing appropriate traits, we'd end up with the following:
async fn post_fibonacci_sequence(client: impl HttpClient, time: impl Time, n: usize) {
for i in 0..n {
time.sleep(Duration::from_hours(i)).await;
client.post("https://fibonacci.org/sequence")
.body(i.to_string())
.send()
.await;
}Now we can trivially test this function without spinning up any web server or manipulating the tokio runtime. As a bonus, unit testing this kind of functionality is typically lightning fast.
There are other approaches to separate I/O and business logic, and we're currently agnostic to the approach taken but as a general rule we'll reject new changes if they mix I/O and business logic.
Any change to our system should come with tests such that if the change was reverted, these tests would fail.
Currently, we have three types of tests:
- Unit tests: Illustrate and assert behavior of individual pieces of functionality.
- Crate integration tests: Test the public interface of a crate to ensure different parts of it work nicely together.
- System tests: Test the whole system as a single unit.
All of these are important as they serve different purposes. They complement each other and should not be used as a substitute for one-another. E.g. we should not rely on system tests to cover all interesting code paths of all pieces of the system, and we should not run other functionality than the system under test in unit tests.
It's encouraged to follow this structure when writing new tests:
#[test]
fn <system_under_test>__should_<test_assertion>(){
// Given
<setup system under test>
// When
<interact with system under test>
// Then
<assert expected outcome>
}the system under test (SUT) can be many things, but typically this would be a function, method or a struct.
It's easy to get stuck in arguments about what's faster or more expensive when comparing different approaches. In these scenarios, we should strive to measure the performance of the system of interest. Anything that isn't benchmarked properly will regress over time.
Therefore, any proposed performance improvements should come with benchmarks or some other objective measure of improvement and regression tests that prevent the optimization from regressing in future iterations.
This section mainly targets current maintainers of this code authorized to approve PR requests.
A code review is the act of answering the following questions:
- Is this code fulfilling the acceptance criteria of the issue?
- Is the code correct?
- Is the code secure?
- Is the code covered by tests?
- Does the code follow our engineering standards?
- In the absence of explicit standards, does the code respect established patterns and conventions?
If the answer to all of these questions is yes, the PR should be approved. If not, the PR reviewer should request changes. When requesting changes, please be as precise as possible. Concrete code suggestions or even stacked PRs are encouraged.
It can sometimes feel harsh to press the red button to request changes. Therefore, it's common in many projects for people to add their comments and withhold their approval instead of explicitly requesting changes.
In our project, this is discouraged. This is because it can often be frustrating as a PR author to end up in this limbo state where you don't get approval to merge and it's not clear exactly what changes are needed to get there.
Instead, we prefer PR reviewers to explicitly request changes on anything that prevents the PR from being merged. This can be small things, as long as they are objective. A good PR reviewer will make it very easy to understand which requested changes are blocking and need to be addressed for an approval and which requested changes/comments are optional.
If there are no explicit objective blocking changes requested for a PR, it should be approved. Even if you as a reviewer don't like the code. It is not acceptable to withhold an approval for a PR, because you prefer another way of doing something if the way implemented in the PR is correct.
A PR review is binary, in the sense that it should either lead to an approval or requested changes.
However, there are instances when it's okay to do neither.
- Partial reviews. If you have started reviewing a PR but not completed it, it's perfectly fine to drop any comments you've found so far without approving. What's important here is to highlight that you've only done a partial review, and set the expectations for when you aim to complete it.
- Chiming in. PRs often turn into a venue for interesting conversations. Naturally it's okay to ask your peers for opinions or chime in on existing PRs. It is also okay to skim a PR and write some notes/observations on it. Just make sure it's clear to everyone involved that you're not taking any responsibility as a reviewer.
At times it happens that we have different views on some matter regarding the code, and disagree about how things should be done. This may be a disagreement about how to apply/interpret our engineering standards, or how to approach some fundamental tradeoff. It is tempting as a reviewer to withhold your approval to get it your way, and as a PR author it's tempting to refuse to listen to suggestions.
Naturally, for any conflict it's good to explore and understand what's behind each conflicting view. Can it be reduced to some objective principle? Is there a way forward that circumvents this issue? Can we get some perspective from another person?
While a full conflict-resolution guide is out of scope for this guide, there are some general principles we can adopt to ensure we have a good climate to sort these out without blocking critical work:
- The majority isn't necessarily right. It's okay and sometimes good to ask another person for perspective, perhaps they can help uncover the underlying conflict or see the problem from a different angle to help build alignment. However, don't try to make it a competition to outnumber anyone with conflicting views. We don't recognize majority voting as an acceptable method to resolve disagreements in PR reviews.
- Seniority doesn't give you any extra weight. We can often weight opinions differently based on who they come from. It is common to end up in cultures where a certain senior person always gets the last word, and no one dares disagree with them. This is quite backwards. While it's natural that a senior contributor with plenty of experience may often be the one to find the best path forward in complex scenarios, they still need to explain and motivate their proposals on the same terms as any other developer would.
- The PR author has spent the most time with this code and has the final say. The person with the most insight in the current code is the PR author. As already stated, we should never block a PR unless there are objective reasons to do so. If there are any doubts, we should err on the liberal side and accept the PR. A single PR is not the end of the world, and we'll be able to fix any sub-optimal design decisions in the future.
- Security concerns trump everything else. The exception to the above is if there's any concern about the security of the system. Letting through a critical vulnerability may very well be the end of this system. We should never merge a PR if there's any doubt about the security of it. These doubts need to be cleared out first, and even a suspicion of a security issue is legitimate grounds to block a PR.
I hope this guide helps maintain a good climate for contributors in this repo, and helps us guide ourselves towards more performant, robust and maintainable code so that we can continue to deliver all exciting features we want to support.
This guide only scratches the surface of all our collective thoughts and opinions on what constitutes good practices. We'll maintain this as a living document and update it as we uncover more insights on which conventions and standards we want to hold ourselves to.
May the force be with you!