Skip to content

Latest commit

 

History

History
417 lines (304 loc) · 22.6 KB

CONTRIBUTING.md

File metadata and controls

417 lines (304 loc) · 22.6 KB

Contributing Guidelines

​:+1::tada: First off, thanks for taking the time to contribute! :tada::+1:

The following is a set of guidelines for contributing to the Filecoin Project. Feel free to propose changes, as this is a living document.

Filecoin, including go-filecoin and all related modules, follows the Filecoin Code of Conduct.

Table Of Contents

How can I contribute?

Here at go-filecoin, there’s always a lot of work to do. There are many ways you can support the project, from progamming, writing, organizing, and more. Consider these as starting points:

  • Submit bugs: Perform a cursory search to see if the problem has already been reported. If it does exist, add a 👍 to the issue to indicate this is also an issue for you, and add a comment if there is extra information you can contribute. If it does not exist, create a new issue (using the Bug report template).

  • Write code: Once you've read this contributing guide, check out Good First Issues for well-prepared starter issues.

  • Improve or write documentation: Docs currently live in the Wiki. You can add feedback to #1689, or submit a PR with proposed changes (process coming soon).

  • New ideas: Open a thread on the Discussion Board.

What should I know before getting started?

Check out the Go-Filecoin code overview for a brief tour of the code.

Design Before Code

  • Write down design intent before writing code, and subject it to constructive feedback.
  • Major changes should have a Design Doc.
  • For minor changes, file an issue for your change if it doesn't yet have one, and outline your implementation plan on the issue.

Pull Requests and Reviews

We review every change before merging to master, using GitHub pull requests. Try to keep PRs small, no more than 400 lines or 8 files. Code reviews are easier, faster, and more effective when the diff is small.

go-filecoin requires 2 approvals for all PRs. We use the following process, which aims to merge code quickly and efficiently while avoiding both accidental and malicious introduction of bugs, unintended consequences, or vulnerabilities.

  1. The first review is done by a committer familiar with that area of the codebase. Once they deem it ready for merge, they will assign a maintainer for the second review.
  2. Once the PR has 2 Approvals and blocking comments have been addressed, the committer rebases and merges the PR.
    • If the author has merge access as a committer or maintainer, they merge their own PRs.

If your PR hasn't been reviewed in 3 days, pinging reviewers via Github or community chat is welcome and encouraged.

We use the following conventions for code reviews:

  • "Approve" means approval. If there are comments and approval, it is expected that you'll address the comments before merging. Ask for clarification if you're not sure.

    • Example: reviewer points out an off by one error in a blocking comment, but Approves the PR. Reviewee must fix the error, but PR can progress to maintainer review. Committer confirms this before merge.

  • "Request Changes" means you don't have approval, and the reviewer wants another look.

    • Example: the whole design of an abstraction is wrong and reviewer wants to see it reworked.

  • By default, code review comments are advisory: the reviewee should consider them but doesn't have to respond or address them. Comments that start with "BLOCKING" must be addressed and responded to. If a reviewer makes a blocking comment but does not block merging (by marking the review "Add Comments" or "Approve") then the reviewee can merge if the issue is addressed.

In rare cases, a maintainer may request approval from all maintainers for a wide-reaching PR.

Reviewer Responsibilities:

Avoid lengthy design discussions in PR reviews. Major design questions should be addressed during the Design Before Code step. If the conversation snowballs, prefer to merge and spin out an issue for discussion, and then follow up on the process failure that led to needing a big design discussion in a PR.

It is considered helpful to add blocking comments to PRs that introduce protocol changes that do not appear in the spec.

Landing Changes

We strongly discourage merge commits on the master branch. When merging to master:

  • squash your PR's commits into one commit with an encompassing message, and
  • rebase against master so that your commit lands as a "fast-forward", without a merge commit.

At the command line, update your branch with git fetch origin +master:master; git rebase master. "Merge" your changes with git checkout master; git merge --ff-only <mybranch>, and push.

On GitHub, use the grey "updated branch" button, which adds a merge commit to your branch, but then land your changes with the "rebase and merge" or "squash and merge" options on the green merge button. Both of these will rebase your branch, squashing out any trailing merge commits in the process.

We may enable GitHub branch protections requiring that a CI build has passed on the branch, that the branch is up to date with master, or both. If either protection is not in force, committers should use their best judgement when landing an untested commit.

Developer Do's and Don'ts

  • DO be aware of patterns from closely related systems. It often makes sense to draw patterns from more established, closely related platforms than to derive them from first principles. For example, we draw heavily on patterns in IPFS for things like configuration, commands, and persistence. Similarly, we draw on patterns in Ethereum for message processing.
  • DO NOT create technical debt. Half-completed features give us a false sense of progress.
    • Example: you add a cache for something. To prevent the cache from becoming a DOS vector, it requires the addition of a tricky replacement policy. You are tempted to defer that work because there is a lot else to do. "Do not create technical debt" means you should implement the replacement policy along with the cache, and not defer that work into the future.
  • DO NOT add dependencies on Node or add more implementation to Node: The Node has become a god object and a dependency of convenience. Abstractions should not take a Node as a dependency: they should take the narrower set of things they actually need. Building blocks should not go on Node; they should go into separate, smaller abstractions that depend on the narrow set of things they actually need. More detail in #1223.

Issues and tracking

We use GitHub issues to track all significant work, including design, implementation, documentation and community efforts. We also use ZenHub to record issue priority and track team progress. ZenHub adds some useful project management overlay data to GitHub issues.

Good First Issues

Ready to begin? Here are well-prepared starter issues (E-good-first-issue) for your coding pleasure. They have clear problem statements, pointers to the right areas of the code base, and clear acceptance criteria.

To pick up an issue:

  1. Assign it to yourself.
  2. Ask for any clarifications via the issue, pinging in community chat as needed.
  3. For issues labeled PROTOCOL BREAKING see the spec section for additional instructions.
  4. Create a PR with your changes, following the Pull Request and Code Review guidelines.

For continued adventures, search for issues with the label E-help-wanted. These are slightly thornier problems that are also reasonably well-prepared.

Pipelines (ZenHub)

We use ZenHub pipelines to track the flow of work on open issues. This usage can vary over time, but the pipelines roughly indicate:

  • Inbox: new issues yet to be triaged
  • Candidates: reasonably complete issues, ready for prioritisation by maintainers
  • Icebox: no near-term intention for implementation
  • Backlog: not an immediate priority, but likely to become one in the future
  • Ready: ready for implementation
  • In Progress: actively being worked on
  • Review/QA: in review (usually triggered by linking a PR)
  • Blocked: unable to progress

Committers and maintainers are expected to keep pipeline information up to date, including on behalf of contributors whose work they are shepherding. Other contributors do not need to maintain this state.

ZenHub can also track dependencies between issues, which is encouraged.

Labels (GitHub)

We use GitHub issue labels to aid browsing, search and discovery of issues related to some outcome or theme. We use a relatively small set of labels, which doesn't change very frequently. Labels are used inclusively, to aid discovery. An issue may have multiple labels; not all issues are expected to have labels. A label is never expected to be "done" (stable state of no open issues).

Labels mark dimensions including:

  • Area (name prefixed with A-): an area of code functionality
  • Category (name prefixed with C-): type of issue, e.g. bug, tech debt, ux
  • Engagement (name prefixed with E-): issues suitable for broader community involvement
  • Importance (name prefixed with I-): highlights why issues are important, e.g. related to security, scale, or runtime crashes
  • Priority (P0 thru P3): not consistently used across the repo; may be used within epics or areas
  • ...plus a few other binary tags

Epics (ZenHub)

An epic is a set of issues required to deliver a particular outcome, typically designed in a design doc. Epics support focus and forecasting through exclusion or inclusion of (ir-)relevant issues. We will typically scope an epic to a deliverable targeting a particular release, so most epics should live less than six weeks.

Releases (ZenHub)

Release tags identify issues targeted to or blocking a particular go-filecoin software release. Release tags are forward-looking and support forecasting and focus though inclusion or exclusion of issues. At present (April 2019) we aim for a time-based release roughly every six weeks. ZenHub release tags can span multiple repositories.

Milestones (GitHub)

We don't actively use GitHub milestones (but have in the past). Milestones support short time-based cycles such as sprints. Milestones are repo-specific.

Roles

There are four main roles for people participating in go-filecoin. Each has a specific set of abilities and responsibilities: Contributors, Collaborators, Committers, and Maintainers.

Contributors

Anyone is welcome. If you have created an issue, commented on an issue or discussion forum thread, or submitted a PR, you are a contributor.

Responsibilities:

Abilities:

  • Open issues and PRs
  • Comment on issues and PRs

Collaborators

A Collaborator is someone who demonstrates helpful contributions to the project.

Responsibilities:

  • Make helpful contributions via issues, PRs, and other venues

Abilities:

  • Write to the repo (but cannot merge to master)
  • Manage issues

Committers

A Committer is someone with a broad understanding of the codebase and the judgment and humility to call on others' expertise when needed. They have a consistent track record of quality contributions, regular participation, and enabling others.

Responsibilities:

  • Review PRs and guide work to ready-to-merge
  • With maintainer approval, rebase and merge contributor PRs
  • Issue triage and other project stewardship

Abilities:

  • Manage issues
  • Merge PRs

Maintainers

A Maintainer is someone:

  1. who is invested in and broadly familiar with the project, as demonstrated by a history of significant technical-, process-, and/or project-level contributions;
  2. who deeply understands the system, especially knowing when and who to defer to as a reviewer, and with an eye towards unintended consequences;
  3. who is actively engaged in project progress and stewardship by enabling others through project-wide planning, code reviews, design feedback, etc.; and
  4. who is a model of trustworthiness, technical judgement, civility, and helpfulness.

Currently, go-filecoin maintainers are (alphabetically): @acruikshank, @anorth, @phritz, @whyrusleeping. They can be mentioned in issues and PRs by tagging @filecoin-project/go-filecoin-maintainers.

Responsibilities:

  • Review: Timely, friendly review of PRs and design docs to ensure high-quality code and grow knowledge of committers and frequent contributors
  • Planning and Improvements: Participate meaningfully in technical and process-related improvements at the project level
  • Make significant, direct technical contributions
  • Backstop for hard problems and general project stewardship (TODO: improve wording)

Abilities:

  • Manage issues
  • Merge PRs

Becoming a committer or maintainer: Anyone can nominate someone for committership or maintainership by filing an issue pointing to evidence that the candidate (1) meets the definition and (2) is already performing the responsibilities described in Roles. Existing maintainers must unanimously approve the new candidate. Removing a committer or maintainer requires either self-nomination, or confirmation by at least 66% of existing maintainers.

Additional Developer Notes

Error Handling

The overarching concern is safety of the user: do not give the user an incorrect view of the world.

  • DO NOT INTENTIONALLY CRASH THE NODE. Don't panic() if you can exit cleanly. panic()ing stops the node from doing useful work that might be unrelated to the error and does not give subsystems an opportunity to clean up, thus potentially creating additional problems.
    • If an error is likely a function of an input, discard the input.
    • If an error could be transient, attempt to continue making progress.
    • If an error appears to be permanent or we have inconsistent internal state, error out to the top level and exit cleanly if possible.

We should log an ERROR only in truly unexpected conditions that should not happen and that a dev should go look at.

Testing

  • All new code should be accompanied by unit tests (except commands, which should be daemon tested). Prefer focussed unit tests to integration tests for thorough validation of behaviour. Existing code is not necessarily a good model, here.
  • Prefer to test the output/contracts.
  • Daemon tests (integration tests that run a node and send it commands):
    • Daemon tests should test integration, not comprehensive functionality
    • Daemon tests should validate that their responses conform to a JSON schema
  • Tests should be placed in a separate package, and follow the naming pattern $PACKAGE_test. For example, a test of the chain package should live in a package named chain_test. In limited situations, exceptions may be made for some "white box" tests placed in the same package as the code it tests.

Profiling

We use pprof to capture and visualize performance metrics.

To capture (for example) a CPU profile, launch the daemon and then make an HTTP request of the following form:

curl 'http://localhost:${CMDAPI_PORT}/debug/pprof/profile?seconds=15' > /tmp/profile.dump

Then, use pprof to view the dump:

go tool pprof /tmp/profile.dump

Gotchas

  • Equality
    • Don't use == to compare *types.Block; use Block.Equals()
    • Ditto for *cid.Cid
    • For types.Message use types.MsgCidsEqual (or better submit a PR to add Message.Equals())
    • DO use == for address.Address, it's just an address

Conventions and Style

There are always exceptions, but generally:

  • Comments:
    • Use precise language.
      • NO: "Actor is the builtin actor responsible for individual accounts". What does "responsible for" mean? What is an individual account?
      • YES: "Actor represents a user's account, holding its balance and nonce. A message from a user is sent from their account actor."
    • Comments shouldn't just say what the thing does, they should briefly say why it might be called or how it is used.
      • NO: "flushAndCache flushes and caches the input tipset's state"
      • YES: "flushAndCache flushes and caches the input tipset's state. It is called after successfully running messages to persist state changes."
  • Accessors: Foo(), SetFoo()
  • Predicates: isFoo()
  • Variable names
    • Channels: fooCh
    • Cids: fooCid
  • Test-only methods: use a parallel type
  • Logging
    • Warning: noteworthy but not completely unexpected
    • Error: a truly unexpected condition that should not happen in Real Life and that a dev should go look at
  • Protocol messages are nouns (eg, DealQuery, DealResponse). Handlers are verbs (eg, QueryDeal).
  • Do not put implementation in commands. Commands should call out to functionality elsewhere, in a pattern borrowed from Git Internals: Plumbing and Porcelain.

We use the following import ordering.

import (
        [stdlib packages, alpha-sorted]
        <single, empty line>
        [external packages]
        <single, empty line>
        [go-filecoin packages]
)

Example:

import (
	"context"
	"testing"

	cmds "github.com/ipfs/go-ipfs-cmds"
	"github.com/ipfs/go-cid"
	ipld "github.com/ipfs/go-ipld-format"
	"github.com/stretchr/testify/assert"

	"github.com/filecoin-project/go-filecoin/testhelpers"
	"github.com/filecoin-project/go-filecoin/types"
)

The Spec

The Filecoin Specification must be in sync with the code.

We have some light process in place to keep the go-filecoin implementation from getting out of sync with the spec. Most tasks are performed by the spec shepherd. The following are the tasks all contributors should be aware of.

Filing / triaging go-filecoin issues

A protocol change is a modification to go-filecoin that breaks interoperability with the current version of go-filecoin. When filing issues that involve a protocol change please flag this issue with the PROTOCOL BREAKING label and identify the breaking changes in the Protocol Changes section of the issue template. If you do not know if an issue involves a protocol change you should just leave this section blank. Protocol changes will be added during the triage process.

Picking up issues

When picking up an issue with items in the Protocol Changes section it is your responsibility to dig into the relevant sections of the spec. If the spec does not have the information necessary for a developer to implement this issue it is your responsibility to file an issue, or better yet, a PR in the specs repo. In other words the PROTOCOL BREAKING label implies an additional acceptance criterion, that all protocol changes are documented in the spec. You can go ahead and implement in parallel, but note that your PR will be blocked from merging until the spec reflects this protocol change. If a change requires enough work to warrant multiple PRs and all this work is blocked on a spec PR merging it is encouraged to submit these PRs to a branch other than master. Then once the spec PR is merged this branch can be merged with master. If at any point the change to the spec seems too onerous, for example if you would have to write multiple pages or a completely new section, reach out to the spec shepherd who will clarify your responsibilities and potentially move the spec work to a separate issue. The spec shepherd may choose to overrule blocking on the spec based on relative priorities of the spec and go-filecoin projects.

What is the bar for inclusion in master?

Presently (Q1'19) the minimum bar is:

  • Unit tests.
  • Tests must not be flaky.
  • Must pass CI.
  • Code review (see above).
  • Lint (go run ./build/*.go lint).
  • Must match a subset of the spec.
  • Documentation is up to date.
  • For major or risky changes: the code must be integrated. Integrated means verifying that it works for real in a devnet, not just that it passes tests. This higher bar ensures major changes actually work, and reduces surprises/nightmares during release.