The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.
The purpose of this document is to describe code development best practices and guidelines. We expect developers to adhere to these guidelines where applicable. Deviations from these guidelines should be explicitly stated.
Unless otherwise stated, the ICON Foundation requires open source code. The code must include an MIT license, Apache 2.0, or another license explicitly approved by the ICON Foundation. Businesses should use the Apache 2.0 license. If you don’t know what to choose, pick the MIT license.
Code repositories should be hosted on Github. Issue tracking must be made public. Rare exceptions may be made for security vulnerabilities or other sensitive matters. Public code repositories should be pushed to regularly.
All of the following must be documented:
- Public classes, constructors, and functions
- Describe inputs/arguments
- Describe outputs/returns
- Brief narrative description
- Architectures, algorithms, and services
- Extensive narrative description of how they work
- Diagrams where applicable
Code documentation lives in the code files, for example above a function. Narrative documentation lives outside the code files, for example a general knowledge base.
Do
- Keep the audience in mind.
- Treat the audience as you would a close friend. Tell them a story.
- Write succinctly and directly.
- Link to other pages for glossary terms and other supplementary information.
- Use analogy, example, and comparison to get points across.
- Think about how pages flow from one to the next.
Don’t
- Talk down to the audience.
- Assume the audience has prior knowledge on the subject.
- Require the audience to navigate through multiple pages to reach the important information.
Do
- Think of code documentation as a short guide on how to use a function.
- Apply rubberducking to documentation when explaining what the function does.
- Add a minimal code example showing how the function would be used.
Don’t
- Assume a vague description and type signature is sufficient.
- Assume that the code should speak for itself.
Examples
Projects should adopt a code format style and adhere to it from project inception to maintain consistency in readability in the codebase. The adopted code format style should be explicitly stated somewhere in the project, such as the CONTRIBUTING.md file.
We recommend using one of the following styles:
- Google (multiple languages)
- Uber's style guide for Golang
- AirBnb’ style guide for React
- PEP-8, the official style guide for Python
Projects should adhere to a well-formed source code branching pattern. Projects should explicitly state their pull request submission, review, and approval process somewhere in the project, such as the CONTRIBUTING.md file.
We recommend using Feature Branching. In this pattern, developers should follow these steps when branching:
- Create a new Git issue when there is a feature or fix to be written that clearly defines what is needed and why. The Git issue is appropriately labeled, assigned, and given a milestone.
- Create a new branch referencing that Git issue to develop the feature or fix.
- Submit a pull request to merge back to the main branch upon completion of the feature or fix.
- If possible, assign a reviewer to review the code of the pull request.
To minimize divergence between main branch and feature branches, features should be atomic and small enough to be able to merge back to main branch quickly. Features should also be small enough that the pull request to be reviewed is not burdensome to the reviewer. Large features can be broken into many smaller features.
For working within a given repository, please follow the branching conventions of that repository.
You may find that another source code branching pattern works better for you. The important thing is that development is tracked publicly via Git issues and pull requests.
We recommend protecting long lived branches such that all changes require at least one reviewer and one approver. It may be the case that this requirement is too restrictive if changes take too long to get reviewed. For example, if a typo is being fixed, it probably doesn't need a review or approval. The ship/show/ask methodology decreases restrictions so that changes to mainline are not always blocked pending review, but increases instability risk because it is up to the author to request for review and approval. For more information on code reviews, see code reviews.
Issue: "Architecture Diagram Guidelines #6"
Branch name: "feature/architecture-diagram-guidelines"
Pull Request header: "feature/architecture-diagram-guidelines" -> "main" => "#6 Create architecture diagram guidelines #7"
Note that the first number in the Pull Request header references the issue number. The second number is associated with the Pull Request itself
We use semantic versioning and expect all releases to be tagged. See version bumps for more information on how we do versioning.
We sometimes use pre-release labels to indicate that a version is unstable and might not satisfy the intended compatibility requirements. The pre-release labels we use include:
| Version state | Notation | Explanation |
|---|---|---|
| Alpha | -alpha | Testing phase. Sandbox usage. Breaking issues expected |
| Beta | -beta | Testing phase. Real-world usage. Breaking issues expected |
| Release candidate | -rc | Pre-release phase. Real-world usage. Breaking issues may be expected but are not identified |
| Release | No extra notation | Release / ready phase. Real-world usage. No further changes expected to this version |
You may find that another versioning convention works better for you. The important thing is that releases are clearly and consistently tagged, and that versioning methodology is explicit. One alternative we recommend to consider is calendar versioning.
We recommend to squash the commit history of a short-lived branch when merging to a long-lived branch.
We adopt our commit message format from conventional commits. Commits MUST follow this format because it makes commit history easy to understand.
<type>(<optional scope>): <subject>
<BLANK LINE>
<body>
<BLANK LINE>
<footer>
Commit messages should use the imperative mood.
- build: Changes that affect the build system or external dependencies
- chore: Changes to the build process or auxiliary tools and libraries such as documentation generation
- ci: Changes to CI configuration files and scripts
- docs: Changes to the documentation
- feat: A new feature
- fix: A bug fix
- perf: A code change that improves performance
- refactor: A code change that neither fixes a bug nor adds a feature
- revert: Commit that reverts a previous commit
- style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)
- test: Adding missing or correcting existing tests
Breaking changes append ! to the type.
A release should only increase either SemVer major, SemVer minor, or SemVer patch. The SemVer number that gets increased by a release should only ever increase by 1.
A release that contains one or more breaking changes increases SemVer major.
A release that contains one or more feat commits increases SemVer minor.
A release that contains one or more fix commits increases SemVer patch.
Scope is a section of the codebase, such as a package, as perceived by the person reading the changelog generated from commit messages. The list of supported scopes should be specified by the project.
The purpose of the body is to provide context and relevant information for reviewers and future readers. The body should include the motivation for the change and contrast this with previous behavior.
The footer should contain any information about Breaking Changes and is also the place to reference GitHub issues that this commit Closes.
Breaking Changes should start with the word BREAKING CHANGE: with a space or two newlines. The rest of the commit message is then used for this.
fix!(relay): fetch out of sync block
Receiver no longer assumes block confirmation occurs on a strict interval.
Closes #322
BREAKING CHANGE: Breaks client.PollingInterval
Reverts state the commit hash to be reverted in the commit message body.
revert(relay): fetch out of sync block
This reverts commit <hash>.
We follow the test pyramid scheme for testing. This includes unit tests, integration tests, and end-to-end tests.
Note: We require that each progress report and deliverable be comprehensively tested, complete with documentation describing how to reproduce test results.
Unit tests ensure that individual units of code (ie method or class) work as intended. You should have a lot more unit tests than integration or end-to-end tests.
Every logical component should be tested. Tests should consider only the expected inputs and outputs, and not the internal implementation details. You do not need to test trivial code.
Unit tests should be written in isolation and run very fast so that they can be automated to run on commit. Any external dependency used by the system under test should be mocked.
A testing status badge at the top of the README.md file should be included that indicates the status of automated testing after automated build. We also recommend to include a code coverage badge.
Integration tests ensure that individual modules work together as intended. Integration tests should be hermetic. To achieve this, we recommend only testing integration testing two individual modules and mocking any external dependencies of the two modules. Every interaction between two modules should be tested for every module.
End-to-end tests automate user stories and should be designed to replicate as closely as possible what the user's way of interacting with the software is. Because end-to-end tests involve many interacting components, they may behave unexpectedly. They also may take significantly longer to run than unit or integration tests. Because they are slow and difficult to maintain, we recommend only having the bare minimum end-to-end tests and primarily relying on unit and integration tests. For frontend development, we prefer Selenium or Puppetteer for webdriver UI tests.
Code reviews involve having another developer review code changes for the purpose of improving code health.
Although CI automates away most style checks, the code review is one of the last quality gates before code gets merged into mainline. Sometimes CI doesn't pick up on everything. The reviewer should check that the code is styled consistently with the rest of the project.
Code changes should integrate well with the rest of the code base. They should logically flow and work as the author intends. The functionality should be properly tested and consider potential vulnerabilities and corner cases.
The reviewer should ensure that the code is documented in accordance with the documentation guidelines and the project-specific guidelines.
Code changes should be as small and as simple as possible. The reviewer should consider whether the changes can quickly be understood, the likelihood that the changes introduce bugs when being called, whether the code is actually needed, and whether the code is over-engineering.
Reviewers should aim to improve code quality, but not so prohibitively that it discourages changes from being submitted. If the author can demonstrate that their changes are equally valid to a suggested alternative, the author's preference should be accepted.
Code reviews turnaround should be within one business day. Slow code reviews decrease the speed of iteration and the entire project's development cycle.
Changes should be small and incremental enough that they can be reviewed quickly.
Feature addition code should be considered done only after completing the following:
- Code is properly documented
- All tests are passed
- Code is reviewed and approved
- Functional acceptance criteria are met
- Non-functional acceptance criteria are met
- Code is deployed to a specified environment
We prefer Docker containers to prevent issues with dependencies and versioning.
dApp code bases are not considered complete until audited by a reputable security auditor to minimize risk to the end user. Note that the operative word here is "minimizes," as it cannot be 100% guaranteed that a dApp will work as intended and not impact the end user negatively.