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

Update README and fix scarb package #688

Merged
merged 17 commits into from
Aug 18, 2023
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ name: Lint and test
on:
pull_request:
branches:
- main
- cairo-2
push:
branches:
- main
- cairo-2

jobs:
Expand Down
87 changes: 11 additions & 76 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,69 +6,9 @@ We really appreciate and value contributions to OpenZeppelin Contracts for Cairo

Before starting development, please [create an issue](https://github.com/OpenZeppelin/cairo-contracts/issues/new/choose) to open the discussion, validate that the PR is wanted, and coordinate overall implementation details.

Also, consider that snake case is used for Cairo development in general due to its strong Python bias.
This project follows our [Extensibility pattern](https://docs.openzeppelin.com/contracts-cairo/extensibility), camelCasing all exposed function names and their parameters:

```cairo
@external
func exposedFunc(paramOne, paramTwo){
}
```

All internal and otherwise unexposed functions should resort to snake_case:

```cairo
func internal_func(param_one, param_two){
}
```

Compare our preset contracts with the libraries from which they're derived such as the [ERC20 preset](src/token/erc20/presets/ERC20.cairo) and [ERC20 library](src/token/erc20/presets/ERC20.cairo) for full examples.
See [Function names and coding style](https://docs.openzeppelin.com/contracts-cairo/0.4.0/extensibility#function_names_and_coding_style) for more information.

And make sure to always include tests and documentation for the new developments. Please consider the following conventions:

- Naming
- Libraries should be named `library.cairo`, e.g. `erc20/library.cairo`
- Contracts should be PascalCased i.e. `MyContract.cairo`
- Interfaces should be prefixed with an `I`, as in `IAccount.cairo`
- Test modules should begin with `test_` followed by the contract name i.e. `test_MyContract.py`

- Structure
- Libraries should cede their names to their parent directory and are named `library.cairo` instead
- Interfaces should be alongside the library that the interface defines
- Preset contracts should be within a `presets` directory of the library to which they are a preset
- Here are example paths:
- `openzeppelin.token.erc20.library`
- `openzeppelin.token.erc20.IERC20`
- `openzeppelin.token.erc20.presets.ERC20Mintable`
- And a visual guide:

```python
openzeppelin
└──token
└── erc20
├── library.cairo
├── IERC20.cairo
└── presets
└── ERC20Mintable.cairo
```

- Preset contract testing
- Though, inheritance is not possible in Cairo, this repo utilizes inheritance for testing. This proves useful for testing multiple contracts that stem from the same base library. For example, the preset contracts [ERC20Mintable](src/token/erc20/presets/ERC20Mintable.cairo) and [ERC20Burnable](src/token/erc20/presets/ERC20Burnable.cairo) both share the base ERC20 functionality. To reduce code repetition, we follow these guidelines:
- `BaseSuites`
- module names are not prefixed with `test_`
- set base tests inside a class
- class name should not be prefixed with `Test`; otherwise, these tests run twice

- test modules
- define the base fixture (`contract_factory`) and any other fixtures not used in the base suite i.e. `erc721_minted`
- define the test class and inherit the base class i.e. `class TestERC20(OwnableBase)`
- add tests specific to the preset flavor within the test class

- fixtures
- are not defined in the base suite but are passed, unpacked, and used
- are defined in the tests where they are used
- for modularity, the basic contract factory fixture is always called `contract_factory`
### Coding style

After a few radical changes in the Cairo language (mainly the transition to Cairo 1), our coding style guidelines got automatically deprecated. That's why [we're working on setting new ones](https://github.com/OpenZeppelin/cairo-contracts/issues/696). Feel free to read, contribute, discuss, and ask questions in the issue.
martriay marked this conversation as resolved.
Show resolved Hide resolved

## Creating Pull Requests (PRs)

Expand Down Expand Up @@ -98,25 +38,22 @@ As a contributor, you are expected to fork this repository, work on your own for
3. Make your changes, add your files, update documentation ([see Documentation section](#documentation)), commit, and push to your fork.

```sh
git add SomeFile.js
git add src/file.cairo
git commit "Fix some bug short description #123"
git push origin fix/some-bug-short-description-#123
```

4. Run tests, linter, etc. This can be done by running local continuous integration and make sure it passes. We recommend to use a [python virtual environment](https://docs.python.org/3/tutorial/venv.html).
4. Run tests and linter. This can be done by running local continuous integration and make sure it passes.

```bash
# install tox from testing dependencies
pip install .[testing] # '.[testing]' in zsh

# run tests
tox
scarb test

# stop the build if there are Markdown documentation errors
tox -e lint
# run linter
scarb fmt --check
```

5. Go to [github.com/OpenZeppelin/cairo-contracts](https://github.com/OpenZeppelin/cairo-contracts) in your web browser and issue a new pull request.
5. Go to [OpenZeppelin/cairo-contracts](https://github.com/OpenZeppelin/cairo-contracts) in your web browser and issue a new pull request.
Begin the body of the PR with "Fixes #123" or "Resolves #123" to link the PR to the issue that it is resolving.
*IMPORTANT* Read the PR template very carefully and make sure to follow all the instructions. These instructions
refer to some very important conditions that your PR must meet in order to be accepted, such as making sure that all PR checks pass.
Expand Down Expand Up @@ -145,15 +82,13 @@ If you want to run the documentation UI locally:

## Integration tests

Currently, [starknet's test suite](https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/starknet/testing/starknet.py) has important differences with public networks. Like [not checking signature hints toward the end of the tx flow](https://github.com/OpenZeppelin/cairo-contracts/issues/386).

That's why we strongly suggest testing new features against a testnet before submitting the PR, to make sure that everything works as expected in a real environment.
Currently, starknet's test suite has important differences with public networks. We strongly suggest testing new features against a testnet before submitting the PR, to make sure that everything works as expected in a real environment.
martriay marked this conversation as resolved.
Show resolved Hide resolved

We are looking into defining a better process for these integration tests, but for now the PR author/contributor must suggest an approach to test the feature when applicable, which has to be agreed and reproduced by the reviewer.

## All set

If you have any questions, feel free to post them to github.com/OpenZeppelin/cairo-contracts/issues.
If you have any questions, feel free to post them as an [issue](https://github.com/OpenZeppelin/cairo-contracts/issues).

Finally, if you're looking to collaborate and want to find easy tasks to start, look at the issues we marked as ["Good first issue"](https://github.com/OpenZeppelin/cairo-contracts/labels/good%20first%20issue).

Expand Down
Loading