Raiden Wizard Development Guide

August 7, 2019 · View on GitHub

Welcome! These are the guidelines for anyone interested in contributing to the Raiden Wizard codebase.

Creating an Issue

For Feature Requests
For Bugs

For Feature Requests

If you want to request a feature for the Raiden Wizard you can do so by opening an issue that contains:

A description of the feature you would like to see implemented.
A explanation of why you believe the feature would make a good addition to the Raiden Wizard.

For Bugs

If you experience a problem while using the Raiden Wizard you can address the problem by opening an issue that contains:

A short description of the problem.
A detailed description of your system.
A description of what exact unexpected thing that occurred.
What you were expecting to happen instead.

Coding Style

This section will outline the coding rules for contributing to the Raiden Wizard repository. All code you write should strive to comply with these rules.

Python

All Python code follows the official Python style guide PEP8.

Formatting

For formatting we use the automatic formatter Black. The configurations for black are set in pyproject.toml and to format your code simply run:
```
make black
```
This will ensure that you comply with our formatting rules.

We highly recommend you also use the other linting tools for automatically determining any style violations. All customizable options for pylint are set in .pylint.rc.

Any pull request needs to pass:
```
make lint
```

Line Length

We have a hard limit on a maximum line length of 99 characters. Flake8 will warn you if you exceed this limit.

We also have a soft limit of 80 characters which is not enforced but just there to encourage shorter lines.

Sometimes Black will reformat lines so they go above the hard limit of 99 characters. In such cases try to break up expressions, e.g.

Bellow example will fail the Flake8 test on make lint.

def testalongline(a):
    mysum = int(
        sum(
            content.value.amount
            for content in a.internal_field.fields_internal_long_named_dictionary_variables.values()
        )
    )
    return mysum

Change it to:

def testalongline(a):
    mysum = sum(
        content.value.amount
        for content in a.internal_field.fields_internal_long_named_dictionary_variables.values()
    )
    mysum = int(mysum)
    return mysum

Shadowing Builtins

Pylint will warn you if you try to shadow built-in names. If you want to use a built-in name add a trailing underscore. The name type would for example become type_.

Don't use leading underscores like _type since these are reserved for private attributes.
Docstrings

For docstrings we follow PEP 0257.

A single line docstring should look like this:
```
def a(b: B, c: C) -> D:
    """Here be docs"""
    pass
```
A multi-line docstring should look like this:
```
def a(b: B, c: C) -> D:
    """ Function Title

    body comes
    here
    """
    pass
```
Note that the multi-line docstring has a title and a body.
Naming Convention

Use descriptive variable names and avoid short abbrevations.

Good practice:
```
manager = Manager()
balance_holder = AccountBalanceHolder()
service = RaidenService()
```
Bad practice:
```
mgr = Manager()
a = AccountBalanceHolder()
s = RaidenService()
```
We have introduced some rules for keeping a consistent naming convention throughout the codebase and make it easy for any reader of the code to understand it.

Addresses
- Use <name>_address_hex for hex encoded addresses.
- Use <name>_address for binary encoded addresses.
Lists of Objects
- Use <name>s. For a list Channel object instances it would be channels.
- Use list() instead of [] to initialize an empty list.
  
  This is only for style consistency and may change in the future because it might provide a tiny change in performance.
Mappings/Dicts
- Use <name>_to_<name> for simple one to one mappings, e.g. tokenaddress_to_taskmanager.
- Use <name>_to_<name>s (an added s) if the mapped object is a list, e.g. <tokenaddress_to_taskmanagers>.
- Use dict() instead of {} to initialize an empty dict.
  
  This is only for style consistency and may change in the future because it might provide a tiny change in performance.
Class Attributes and Functions
- Class members should be private by default and start with a leading underscore _.
- Anything that is part of the interface of the class should not have a leading underscore.
- Other parts of the code should only use the class through the public interface.
- Our tests should be testing the public interface of the class.
  
  A minimal example:
```
class Diary:
    def __init__(self, entries: List[str]) -> None:
        self._entries = entries

    def entry(index: int) -> str:
        return _entries[index]
```
  Note the typing of __init__(...) -> None:
NewTypes and Type Comparisons
- For often used types define new types using the typing.NewType function. New type names should be capitalized.
```
Address = NewType('Address', bytes)
```
  You need to define an associate alias which starts with T_ in order to use these type definitions for type comparisons.
```
T_Address = bytes
```
typing.Optional Convention
- If the argument has a default value of None we follow the convention to omit the use of typing.Optional[].
  
  Good Practice:
```
def foo(a: int = None) -> ReturnType:
```
  Bad Practice:
```
def foo(a: typing.Optional[int] = None)
```
Imports

Classes must be imported in the global namespace unless there are name collisions and module imports can be used.
```
import a
from b import Normal


class Conflict:
    pass


def f() -> Tuple[a.Conflict, Normal]:
    return a.Conflict(), Normal()
```

Writing a Test

When developing a feature or working on a bug fix you should always start by writing a test or modifying existing tests.

Write a test and see it fail.
Implement the feature/bug fix.
Confirm that all your new tests pass.

Your addition to the test suit should test your feature/bug fix at the innermost level possible. Avoid integration tests in favor of unit tests whenever possible.

Committing Rules

You can read this guide which has really good advice but some rules you should always follow are:

Commit titles should not exceed 50 characters.
Leave a blank line after the title (this is optional if there is no description)
Leave a description of the commit (this is optional if the commit is really small)

Why are these rules important?

All tools that show you information about git repos treat the first 80 characters as a title. Even GitHub itself does so and the git history will look nice and neat if these simple rules are followed.

Opening a Pull Request

If you would like to contribute to the actual codebase you can open a pull request against the repository.

All pull requests should be:

Self-contained.
As short as possible, addressing a single issue or even a part of an issue.

Consider breaking long pull requests into smaller ones.

To get your pull request merged into the main repository it needs to:

Have one approved review from one of the core developers.
All Continuous Integration tests needs to pass and the CI build should be green.

You also need to sign the Raiden project CLA (Contributor License Agreement), our CLA bot will help you with that after you created your pull request. If you or your employer do not hold the whole copyright of the authorship submitted we can not accept your contribution.

For Frequent Contributors with Write Access

We have a set of labels to put on pull requests for signaling to colleagues what the current state of the pull request is.

These are:

Dev: Please Review

Pull requests that are ready for a reviewer to have a look at.
Dev: Work in Progress

Pull requests that are either not ready for review or are getting review suggestions applied by the author.

Pull Request Reviews

It is the authors responsibility to ask for at least one person to review their pull request. This person should know the area of the code being changed. If the chosen reviewer does not feel fully confident in doing the review they can ask someone else to take an additional look at the code.

All developers in the team should perform pull request reviews. Make it a habit to check this link often to help colleagues who have pull requests pending for review.

We have tools that are automatically run by the CI and which check the quality of the code (flake8, mypy, pylint). Fixes related to linting are therefore not part of pull request reviews.

Reviewers are encouraged not to be nitpicky about the suggested changes they are asking the author for. If something is indeed nitpicky the reviewer is encouraged to state it beforehand so that the author can choose whether to implement the nitpicks or ignore them.

Good practice:

nitpick: I don't really think XYZ makes sense here. If possible it would be nice to have it changed to KLM.

Authors should strive to make pull request reviews easier.

Make the pull requests as small as possible.
Even if some code is touched it doesn't mean it needs to be refactored, e.g. don't mix style/typing change with a big pull request.

Whenever a reviewer starts to review a pull request he or she should write a comment in the pull request stating they are doing so, e.g. "Reviewing this now". This is for keeping track of who is reviewing a pull request and when a review is going on.

When performing a pull request review of non trivial pull requests it is recommended to clone the branch locally, explore the changes with your editor, run tests and experiment with the changes to get a better understanding of the code changes and good constructive feedback can be given to the author.

Integrating Pull Requests

There are two options for integrating a successful pull request into the codebase.

Create a Merge Commit
Rebase and Merge

"Create a Merge Commit" is the GitHub default option which unfortunately is not our preferred option since we can not be sure that the result of the merge will have all test passing. This is because there may be other patches merged since the pull request opened.

There are many pull requests which we definitely know won't have any conflicts and for which enforcing rebase would make no sense therefore we provide the option to use both at our own discretion.

The general guidelines are:

Use Rebase and Merge if patches have been merged to master since the pull request was opened on top of which our pull request may have different behaviour.
Use Create a Merge Commit if patches have been merged to master since the pull request was opened which are related to documentation, infrastructure or completely unrelated parts of the code.