openst / developer-guidelines Goto Github PK

View Code? Open in Web Editor NEW

3.0 3.0 8.0 61 KB

Guidelines on code, project setup, etc.

License: MIT License

developer-guidelines's People

Contributors

Stargazers

Watchers

Forkers

schemar jasonklein 0xsarvesh gulshanvasnani d-ost mosaicdao

developer-guidelines's Issues

🤵 Formalize the release process

Think about and draft a good release process based on a discussion with the team. (The proposal does not need to satisfy everyone on the team, but take everyone's input into account and possibly discuss why you did or did not follow some points) (Make sure the proposal is in-line with git flow)

Look at other projects' release processes (e.g. GitLab).

Outcome: PR to developer guidelines.

There is some written form that presents an improved proposal with an argumentation why the proposal was made the way it was.

Below is a proposal to get you started:

time-based releases
on develop we start 0.x.0-alpha.1 version numbers
at fixed time intervals, we move develop to release-0.x branch where the version moves to 0.x.0-beta.1
from the release branch, we integrate in JLP, and/or later OST Platform;
upon success beta-testing, we organize external security assessments
releases that have been reviewed by external experts get 0.x.0-rc.1 tags, following review corrections
on release day (on regular interval or following other scheduled plans), release branch is moved to master and tagged as 0.x.0
later fixes on the release increase the patch number on the release branch, and are merged to master

Deduplicate code

Organization and UtilityToken contracts are used in more than one repositories.
All repositories have its own implementations, so

Its code duplication
It is difficult to maintain code across the repositories.

Following are the items that we are trying to achieve in this epic

1. The code will be moved to a new repository.

openst/organization-contracts already exists.
A new repository will be created for utility token contracts.

2. These repositories should include the contract interacts

These repositories should auto-generate the contract interact code from the ABI JSON files.
Contract interacts creation in type-script has a lot of boilerplate code and its time consuming to maintain it manually. It will be helpful to consume APIs with proper typed parameters.

So for the creation of contract interacts the proposal will be as follows

Do research on ABI to typescript generators
Analyze if we can use any one of the existing generators that best fit our requirements, or we need to manually write the code.
Analyze if the unit tests are needed.

Few example of generators are given below (You may add more)

Typechain (this looks interesting)
Soltsice.
@0xproject/abi-gen. (this looks interesting)
Go-ethereum has a CLI tool called abigen -> Go/Java/Objective-C wrapper. (Just for reference)

3. Publish NPM package
While publishing the NPM package

The contract interacts should be autogenerated before publishing and should be exported.

4. Use git submodule to use the contracts in the dependent repository

OpenST/openst-contracts
OpenST/mosaic-contracts
OpenST/brandedtoken-contracts

5. Update dependent repositories to use published NPM module.

Unified logging

We should agree on a logging library and use that across all repositories.
What about Winston?

Should we use require with a meaningful message in addition to SafeMath?

For example when transferring tokens, we usually use SafeMath to subtract amounts. SafeMath will fail if the user does not have the required amount available to make the transfer.

The question is whether we should have an additional require before SafeMath that asserts that the calculation is possible. It would have the benefit of a clear error message to the consumer.
However, that would leave the question of whether to use SafeMath in the first place ...

Publish organization and utility tokens as npm package

Follow-up to OpenST/mosaic-contracts#728

Organization and utility token contracts should be published and included as NPM packages to prevent multiple definitions of the same contract. See OpenST/mosaic-contracts#728 for details.

A follow-up ticket would be to use the npm packages instead of git-submodules in the consumers.

write release notes on Github for 0.10

openst-contracts https://github.com/OpenST/openst-contracts/releases/tag/0.10.0-rc.3
openst.js https://github.com/OpenST/openst.js/releases/tag/0.10.0-beta.2
brandedtoken-contracts https://github.com/OpenST/brandedtoken-contracts/releases/tag/0.10.0-rc.1
brandedtoken.js https://github.com/OpenST/brandedtoken.js/releases/tag/0.10.0-beta.1
mosaic-contracts https://github.com/OpenST/mosaic-contracts/releases/tag/0.10.0-rc.1
mosaic.js https://github.com/OpenST/mosaic.js/releases/tag/0.10.0-beta.5
mosaic-chains

Use resources from developer guidelines

Link to developer guidelines from the README.
E.g. code of conduct, style guide, etc.

Default ports should be non-conflicting

The default ports used by ganache and integration tests should be non-conflicting. Furthermore, the test ports should not be the same as standard eth node ports.

/cc @benjaminbollen @pgev

Labels should be unified across repositories

We need to:

decide which labels are required,
decide how to set-up the labels (colors and names),
create the labels across all L3 repositories,
and start applying the labels to new tickets.

Create typescript developer guidelines.

Changelog management

We need to find a process to manage the changelogs on an ongoing basis.

Find out if there are tools to auto-generate changelogs and/or PRs to ease the process of maintaining the changelogs.
Also add documentation about the desired processes to this repository.

Update solidity style guide wrt documenting an author for a contract

Currently, solidity style guide in this repo mentions the followings:

Avoid using @author documentation tag.

In the context of this ticket, please update documentation and let developer to add @author ethereum NatSpec tag for contracts and interfaces in the following format:

@author John Doe - <[email protected]>

check all repos use SafeMath as NPM from OpenZeppelin

should just be a quick check; if lacking in any repo, list them here

Reminder: fix lint issues.

This is just a reminder ticket that will appear on the top of the backlog pipeline.
Fix all the linting issues before raising the PR.

NPM Publish must be aligned across all repositories

Make sure that the guidelines clearly state that it should be sufficient to run npm publish without manually running any command before.
The prepack step should take care of everything that needs to be done before publishing (especially on a new clone).

Handling of internal and external dependencies of packages

[Updated 20190403]

We are of the opinion that we should use ^ where possible and sensible to ensure we run with the most recent updated dependencies. The lock file should be committed and updated at regular intervals.

We decided to use the ^ symbol or version ranges for "internal" dependencies. Internal dependencies are projects under the @openst project.

[ Updated 20190411]
For external dependencies we want to use fixed versions.

Until we are at version 1.0.0, we should use a version range like >=0.10.0 <0.11.0 to prevent propagation of breaking changes in newer minor versions.

The scope of this ticket is exclusively an update to the developer guidelines.

Unfinished issues should comment their state

As a developer
I want to see the status of a ticket inside the ticket
Because it wastes time going through all related PRs and possibly tickets.

Given that you worked on a ticket and assigned it to a PR
When the PR does not fully solve the ticket (no Fixes #111)
Then you should add a comment to the ticket about what's done and what is still open.

/cc @benjaminbollen

Deduplicate ERC20 Token

Follow-up to OpenST/mosaic-contracts#728

For testing, all repositories should use ERC20 Token from openzeppelin's npm package instead of re-creating it themselves.

Some repositories require an ERC20 interface in order to call contracts from within tests and other things. For those cases, use a dependency on openzeppelin.

https://github.com/OpenZeppelin/openzeppelin-solidity/tree/master/contracts/token

Solidity order of execution should be documented

Solidity code should follow the "Checks-Effects-Interactions" pattern.

The guideline should also make clear where events should be placed.
When you create the PR, argue why you decided where to place the event calls.

Solidity considers events to be state changes. So please regard that in relation to external calls?

You should update the Solidity guide that is already part of the developer guidelines.

push tag on master and publish NPM 0.10.0

Clarify definition(s) of "value token"

"Value token", while conceptually consistent, has different specific meanings in different code contexts (e.g., BrandedToken vs EIP20Gateway), but the context-specific meanings may not be clear or consistently applied in those different contexts.

The different meanings in the OpenST repos (mosaic-contracts, brandedtoken-contracts, etc.), both documented and implemented, should be clarified and aligned in a terminology document in this repo (add the document if it does not exist; update readme to point to the document if the readme does not already do so). This ticket does not include making alignment/clarity/consistency in the other repos—any such need should be specified in other tickets.

Old contract repositories should be deprecated

I tried deprecating @openstfoundation/{mosaic,brandedtoken,openst}-contracts using npm. Even though there was no error, it seems the packages are not deprecated.

I ran, for example, npm deprecate @openstfoundation/mosaic-contracts "use @openst/mosaic-contracts instead".

Find out how to deprecate packages properly and deprecate the @openstfoundation packages that were moved to @openst.

npm publishing

Branch protection settings should be aligned across repositories

All repositories use Travis CI for automated testing.
The branch protection settings of a repository govern, among other things, the requirements a PR has to meet before it can be merged.

These settings should be unified across all of our repositories.

The settings that must be decided on are:

Should the same rules apply to all branches? (probably not; e.g. master more strict than develop)
For different branches:
- Dismiss stale pull request approvals when new commits are pushed?
- Require review from Code Owners?
- Restrict who can dismiss pull request reviews?
- Require branches to be up to date before merging?
- Require signed commits?
- Include administrators?
- Restrict who can push to matching branches?

These question need to be answered and the answers need to be documented in a file in the developer guidelines repository (does not have to be part of the main README, as it is very specific).