Skip to content

Conversation

dougiebuckets
Copy link
Contributor

@dougiebuckets dougiebuckets commented Jul 18, 2018

  • Added an Architecture section that speaks to the different types of contracts
  • Added a Tests section that provides high-level visibility into what is used for unit testing
  • Added a How To Use and Modify OpenZeppelin Contracts section
  • Added development principles to the existing Security section

Fixes #1081

🚀 Description

  • 📘 I've reviewed the OpenZeppelin Contributor Guidelines
  • 📖 I've made sure that my contracts are well-documented.
  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

## Architecture
The following provides visibility into how OpenZeppelin's contracts are organized:

- **access** - Smart contracts that enable functionality that can be used for selective restrictions and basic authorization control functions. Includes address whitelisting and signature-based permissions management.
Copy link
Contributor

Choose a reason for hiding this comment

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

From this description, it's clear to me that this is where RBAC belongs, @shrugs WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Even the 'a' stands for 'access'.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah agreed. I think Ownable should go in here as well, but I doubt we want to move that :D

added an issue: #1093

README.md Outdated
- *Implementations* - Includes ERC-721 token implementations that include all required and some optional ERC-721 functionality.

## Tests
Unit test are critical to the OpenZeppelin framework. They help ensure code quality and mitigate against security vulnerabilities. The directory structure within the `/tests` directory corresponds to the `/contracts` directory. OpenZeppelin uses Mocha’s JavaScript testing framework and Chai’s assertion library. To learn more about how to tests are structured, please reference OpenZeppelin’s Testing Guide.
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 already have a Testing Guide that I'm unaware of, or is this referencing #1077?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nventuro Good question. Yup, this is referencing #1077.

## Security
OpenZeppelin is meant to provide secure, tested and community-audited code, but please use common sense when doing anything that deals with real money! We take no responsibility for your implementation decisions and any security problem you might experience.

The core development principles and strategies that OpenZeppelin is based on include: security in depth, simple and modular code, clarity-driven naming conventions, comprehensive unit testing, pre-and-post-condition sanity checks, code consistency, and regular audits.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a link to the wiki? https://github.com/OpenZeppelin/openzeppelin-solidity/wiki/Contribution-guidelines

The wiki will be deprecated eventually though, so I'm not sure how to proceed here.

@nventuro
Copy link
Contributor

This looks great, thanks! 🎉

I think some of the directory descriptions may be a bit too specific, detailing what's currently there, instead of the spirit of that location. E.g.: **math** - Libraries with safety checks used to prevent against overflows and underflows., or introspection directly referencing ERC-165. @frangio thoughts?

README.md Outdated
- **introspection** - An interface that can be used to make a contract comply with the ERC-165 standard as well as a contract that implements ERC-165 using a lookup table.
- **lifecycle** - A collection of base contracts used to manage the existence and behavior of your contracts and their funds.
- **math** - Libraries with safety checks used to prevent against overflows and underflows.
- **mocks** - A collection of abstract contracts that can be used as the foundation for your own custom implementations. The mock contracts demonstrate how OpenZeppelin’s secure base contracts can be used with multiple inheritance.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's mention that the mocks are used primarily for tests, but also serve as good examples for usage and combinations

README.md Outdated
- **math** - Libraries with safety checks used to prevent against overflows and underflows.
- **mocks** - A collection of abstract contracts that can be used as the foundation for your own custom implementations. The mock contracts demonstrate how OpenZeppelin’s secure base contracts can be used with multiple inheritance.
- **ownership** - A collection of smart contracts that can be used to manage contract and token ownership
- **rbac** - A library used to manage addresses assigned to different user roles and an example RBAC interface that demonstrates how to handle setters and getters for roles and addresses.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add the full "Role Based Access Control" text instead of RBAC interface so people know what it stands for right away.

README.md Outdated
- **rbac** - A library used to manage addresses assigned to different user roles and an example RBAC interface that demonstrates how to handle setters and getters for roles and addresses.

- **payment** - A collection of smart contracts that can be used to manage payments through escrow arrangements, withdrawals, and claims. Includes support for both single payees and multiple payees.
- **proposals** - A collection of smart contracts that reflect community Ethereum Improvement Proposals (EIPs). The smart contracts here are under consideration for inclusion in OpenZeppelin. They are not recommended for use until the community has thoroughly evaluated and approved them.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a link to https://github.com/OpenZeppelin/openzeppelin-solidity/wiki/ERC-Process so people can get the full details.

Maybe instead of "not recommended for use..." we can say something like "these contracts are under development and standardization, so they're not recommended for production contracts, but are useful for experimentation with pending EIP standards."

README.md Outdated
- **payment** - A collection of smart contracts that can be used to manage payments through escrow arrangements, withdrawals, and claims. Includes support for both single payees and multiple payees.
- **proposals** - A collection of smart contracts that reflect community Ethereum Improvement Proposals (EIPs). The smart contracts here are under consideration for inclusion in OpenZeppelin. They are not recommended for use until the community has thoroughly evaluated and approved them.
- **token** - A collection of approved ERC standard tokens -- their interfaces and implementations.
- **ERC20** - A standard interface for fungible tokens:
Copy link
Contributor

Choose a reason for hiding this comment

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

indenting on ERC20 and ERC721 needs to be under token I believe

README.md Outdated
Unit test are critical to the OpenZeppelin framework. They help ensure code quality and mitigate against security vulnerabilities. The directory structure within the `/tests` directory corresponds to the `/contracts` directory. OpenZeppelin uses Mocha’s JavaScript testing framework and Chai’s assertion library. To learn more about how to tests are structured, please reference OpenZeppelin’s Testing Guide.

## How To Use And Modify OpenZeppelin Contracts
When using OpenZeppelin to build your own distributed applications, for security reasons we encourage you NOT to modify the framework’s base contracts, libraries, and interfaces. In order to leverage and extend their functionality, we encourage you to inherit from them.
Copy link
Contributor

Choose a reason for hiding this comment

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

"inherit from them or compose them together with your own contracts."

README.md Outdated
## How To Use And Modify OpenZeppelin Contracts
When using OpenZeppelin to build your own distributed applications, for security reasons we encourage you NOT to modify the framework’s base contracts, libraries, and interfaces. In order to leverage and extend their functionality, we encourage you to inherit from them.

The Solidity programming language supports multiple inheritance. This is very powerful yet it can also be confusing. The more complexity you introduce to your distributed applications through multiple inheritance, the greater your application’s attack surface is.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we add a : in between these two sentences, like: This is very powerful, but can also be confusing: the more complexity you introduce to your distributed applications...

README.md Outdated

The Solidity programming language supports multiple inheritance. This is very powerful yet it can also be confusing. The more complexity you introduce to your distributed applications through multiple inheritance, the greater your application’s attack surface is.

You’ll notice in the `/mocks` directory there are a collection of abstract contracts that can be used as the foundation for your own custom implementations. These mock contracts demonstrate how OpenZeppelin’s secure base contracts can be used with multiple inheritance.
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise for testing purposes here

@dougiebuckets dougiebuckets force-pushed the more-detail-in-getting-started-guide-#1081 branch from 0005687 to fcb663f Compare July 19, 2018 11:11
@dougiebuckets
Copy link
Contributor Author

@shrugs @nventuro - Thanks for the feedback! Updates made :)

@nventuro nventuro added kind:improvement documentation Inline comments, guides, and examples. labels Jul 21, 2018
README.md Outdated
- **math** - Libraries with safety checks on operations that throw on errors.
- **mocks** - A collection of abstract contracts that are primarily used for unit testing. They also serve as good usage examples and demonstrate how to combine contracts with inheritence when developing your own custom applciations.
- **ownership** - A collection of smart contracts that can be used to manage contract and token ownership
- **rbac** - A library used to manage addresses assigned to different user roles and an example Role-Based Access Control (RBAC) interface that demonstrates how to handle setters and getters for roles and addresses.
Copy link
Contributor

Choose a reason for hiding this comment

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

As of #1114 (thanks to you!), RBAC is now under access :)

Copy link
Contributor Author

@dougiebuckets dougiebuckets Jul 27, 2018

Choose a reason for hiding this comment

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

Awesome! Update made to docs. Thanks for your help.

README.md Outdated

The Solidity programming language supports multiple inheritance. This is very powerful yet it can also be confusing: the more complexity you introduce to your distributed applications through multiple inheritance, the greater your application’s attack surface is.

You’ll notice in the `/mocks` directory there are a collection of abstract contracts used for testing that can also be used as the foundation for your own custom implementations. These mock contracts demonstrate how OpenZeppelin’s secure base contracts can be used with multiple inheritance.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd mention once more that mocks are intended primarily for testing (though it's true that they may illustrate usage).

* Added an Architecture section that speaks to the different types of contracts
* Added a Tests section that provides high-level visibility into what is used for unit testing
* Added a How To Use and Modify OpenZeppelin Contracts section
* Added development principles to the existing Security section
@dougiebuckets dougiebuckets force-pushed the more-detail-in-getting-started-guide-#1081 branch from fcb663f to 4c4fd7f Compare July 27, 2018 12:11
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Great, thanks @dougiebuckets! We might revamp the directories a bit before the 2.0 release, this is a great starting point for the upcoming readme :)

@nventuro nventuro merged commit f5b0bb3 into OpenZeppelin:master Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Inline comments, guides, and examples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write a more comprehensive "Getting started" guide
3 participants