Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Developer's guide and best practices #1075

Merged
merged 2 commits into from
Nov 11, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions CODING_GUIDELINES.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# Coding Guidelines

This document is an extension to [CONTRIBUTING](./CONTRIBUTING.md) and provides more details about the coding guidelines and requirements.

## API & Design

* Code must be well structured:
* packages must have a limited responsibility (different concerns can go to different packages),
* types must be easy to compose,
* think about maintainbility and testability.
* "Depend upon abstractions, [not] concretions".
* Try to limit the number of methods you are exposing. It's easier to expose something later than to hide it.
* Take advantage of `internal` package concept.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal package was not working well with wasmd as a library. Let's not bring it back

* Follow agreed-upon design patterns and naming conventions.
* publicly-exposed functions are named logically, have forward-thinking arguments and return types.
* Avoid global variables and global configurators.
* Favor composable and extensible designs.
* Minimize code duplication.
* Limit third-party dependencies.

Performance:

* Avoid unnecessary operations or memory allocations.

Security:

* Pay proper attention to exploits involving:
* gas usage
* transaction verification and signatures
* malleability
* code must be always deterministic
* Thread safety. If some functionality is not thread-safe, or uses something that is not thread-safe, then clearly indicate the risk on each level.

## Best practices

* Use [goimports](https://pkg.go.dev/golang.org/x/tools/cmd/goimports) as your code formatter.

* Always wrap returned errors.
* Doing `if err != nil { return err }` does not include each callers' context. Pushing errors up the stack without context makes it harder to test and debug. Additionally, a short context description makes it easier for the reader to understand the code. Example:

```go
if !coins.IsZero() {
if err := k.bank.TransferCoins(ctx, caller, contractAddress, coins); err != nil {
return nil, err
}
}
```

* It would be an improvement to return `return nil, sdkerror.Wrap(err, "lock contract coins")`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

* Please notice that fmt.Errorf is not used, because the error handling predates fmt.Errorf and errors.Is

* Limit the use of aliases, when not used during the refactoring process.
116 changes: 116 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
# Contributing

Thank you for considering making contributions to Wasmd!

Contributing to this repo can mean many things, such as participating in
discussion or proposing code changes. To ensure a smooth workflow for all
contributors, the general procedure for contributing has been established:

1. Start by browsing [new issues](https://github.com/CosmWasm/wasmd/issues).
* Looking for a good place to start contributing? How about checking out some [good first issues](https://github.com/CosmWasm/wasmd/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) or [bugs](https://github.com/CosmWasm/wasmd/issues?q=is%3Aopen+is%3Aissue+label%3Abug)?
2. Determine whether a GitHub issue or discussion is more appropriate for your needs:
1. If the issue you want addressed is a specific proposal or a bug, then open a [new issue](https://github.com/CosmWasm/wasmd/issues/new).
2. Review existing [issues](https://github.com/CosmWasm/wasmd/issues) to find an issue you'd like to help with.
3. Participate in thoughtful discussion on that issue.
4. If you would like to contribute:
1. Ensure that the proposal has been accepted.
2. Ensure that nobody else has already begun working on this issue. If they have,
make sure to contact them to collaborate.
3. If nobody has been assigned for the issue and you would like to work on it,
make a comment on the issue to inform the community of your intentions
to begin work.
5. To submit your work as a contribution to the repository follow standard GitHub best practices. See [pull request guideline](#pull-requests) below.

**Note:** For very small or blatantly obvious problems such as typos, you are
not required to an open issue to submit a PR, but be aware that for more complex
problems/features, if a PR is opened before an adequate design discussion has
taken place in a GitHub issue, that PR runs a high likelihood of being rejected.

## Development Procedure

* The latest state of development is on `main`.
* `main` must never fail `make lint test test-race`.
* No `--force` onto `main` (except when reverting a broken commit, which should seldom happen).
* Create a branch to start work:
* Fork the repo (core developers must create a branch directly in the Wasmd repo),
branch from the HEAD of `main`, make some commits, and submit a PR to `main`.
* For core developers working within the `wasmd` repo, follow branch name conventions to ensure a clear
ownership of branches: `{moniker}/{issue#}-branch-name`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to do this branch pattern? I am doing {issue}_name only

* See [Branching Model](#branching-model-and-release) for more details.
* Be sure to run `make format` before every commit. The easiest way
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is good

to do this is have your editor run it for you upon saving a file (most of the editors
will do it anyway using a pre-configured setup of the programming language mode).
* Follow the [CODING GUIDELINES](CODING_GUIDELINES.md), which defines criteria for designing and coding a software.

Code is merged into main through pull request procedure.

### Testing

Tests can be executed by running `make test` at the top level of the Cosmos SDK repository.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Tests can be executed by running `make test` at the top level of the Cosmos SDK repository.
Tests can be executed by running `make test` at the top level of the wasmd repository.


### Pull Requests

Before submitting a pull request:

* merge the latest main `git merge origin/main`,
* run `make lint test` to ensure that all checks and tests pass.

Then:

1. If you have something to show, **start with a `Draft` PR**. It's good to have early validation of your work and we highly recommend this practice. A Draft PR also indicates to the community that the work is in progress.
Draft PRs also helps the core team provide early feedback and ensure the work is in the right direction.
2. When the code is complete, change your PR from `Draft` to `Ready for Review`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

3. Go through the actions for each checkbox present in the PR template description. The PR actions are automatically provided for each new PR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have checkboxes :-)

4. Be sure to include a relevant changelog entry in the `Unreleased` section of `CHANGELOG.md` (see file for log format). The entry should be on top of all others changes in the section.

PRs should have a category prefix that is based on the type of changes being made (for example, `fix`, `feat`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR names should start uppercase . Let's not have prefixes. This can look odd in the history

`refactor`, `docs`, and so on). The *type* must be included in the PR title as a prefix (for example,
`fix: <description>`). This convention ensures that all changes that are committed to the base branch follow the
[Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can add this later when needed. A lot to read for a new contributor

Additionally, each PR should only address a single issue.

Pull requests are merged automatically using [`A:automerge` action](https://mergify.io/features/auto-merge).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used, yet


NOTE: when merging, GitHub will squash commits and rebase on top of the main.

## Protobuf

We use [Protocol Buffers](https://developers.google.com/protocol-buffers) along with [gogoproto](https://github.com/cosmos/gogoproto) to generate code for use in Wasmd.

For deterministic behavior around Protobuf tooling, everything is containerized using Docker. Make sure to have Docker installed on your machine, or head to [Docker's website](https://docs.docker.com/get-docker/) to install it.

For formatting code in `.proto` files, you can run `make proto-format` command.

For linting we use [buf](https://buf.build/). You can use the commands `make proto-lint` to lint your proto files.

To generate the protobuf stubs, you can run `make proto-gen`.

We also added the `make proto-all` command to run all the above commands sequentially.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 my favourite


In order for imports to properly compile in your IDE, you may need to manually set your protobuf path in your IDE's workspace settings/config.

For example, in vscode your `.vscode/settings.json` should look like:

```json
{
"protoc": {
"options": [
"--proto_path=${workspaceRoot}/proto",
]
}
}
```

## Branching Model and Release

User-facing repos should adhere to the trunk based development branching model: https://trunkbaseddevelopment.com. User branches should start with a user name, example: `{moniker}/{issue#}-branch-name`.

Wasmd utilizes [semantic versioning](https://semver.org/).

### PR Targeting

Ensure that you base and target your PR on the `main` branch.

All feature additions and all bug fixes must be targeted against `main`. Exception is for bug fixes which are only related to a released version. In that case, the related bug fix PRs must target against the release branch.

If needed, we backport a commit from `main` to a release branch (excluding consensus breaking feature, API breaking and similar).