Skip to content

Create detailed ERC20 interface#477

Merged
frangio merged 1 commit intoOpenZeppelin:masterfrom
facuspagnuolo:feaure/create_detailed_erc20_interface
Nov 13, 2017
Merged

Create detailed ERC20 interface#477
frangio merged 1 commit intoOpenZeppelin:masterfrom
facuspagnuolo:feaure/create_detailed_erc20_interface

Conversation

@facuspagnuolo
Copy link
Copy Markdown
Contributor

No description provided.

@maraoz maraoz requested review from frangio and maraoz September 29, 2017 19:29
Copy link
Copy Markdown
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks a lot @facuspagnuolo! Good idea. 😄

Suggested a few changes before merging

Comment thread contracts/token/DetailedERC20.sol Outdated
contract DetailedERC20 is ERC20 {
string public name;
string public symbol;
uint256 public decimals;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be uint8. (See ERC20.)

Comment thread test/helpers/DetailedERC20Mock.sol Outdated

contract DetailedERC20Mock is StandardToken, DetailedERC20 {
function DetailedERC20Mock(uint256 _decimals, string _symbol, string _name) {
require(_decimals > 0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, I think decimals could be 0, in the case of indivisible tokens.

Comment thread test/helpers/DetailedERC20Mock.sol Outdated
import '../../contracts/token/DetailedERC20.sol';

contract DetailedERC20Mock is StandardToken, DetailedERC20 {
function DetailedERC20Mock(uint256 _decimals, string _symbol, string _name) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I'd actually make this the constructor of DetailedERC20 itself. In that way, contracts can inherit from it and call the constructor without having to modify the state variables. It will look cleaner IMO. (And the mock wouldn't be necessary for testing.)

Also, the order of arguments (name, symbol, decimals) makes a bit more sense to me.

Comment thread test/DetailedERC20.js Outdated

it('has a name', async function () {
const name = await detailedERC20.name();
name.should.be.equal('My Detailed ERC20');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can use the constants defined above in these tests! _name, etc.

Comment thread contracts/token/DetailedERC20.sol Outdated
uint256 public decimals;
uint8 public decimals;

function DetailedERC20(string name, string _symbol, uint8 _decimals) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Travis is failing because name should be _name.

@maraoz
Copy link
Copy Markdown
Contributor

maraoz commented Nov 12, 2017

@facuspagnuolo 1 test still failing, please check

@facuspagnuolo facuspagnuolo force-pushed the feaure/create_detailed_erc20_interface branch from 2a289e4 to cde7f44 Compare November 13, 2017 18:34
@facuspagnuolo facuspagnuolo force-pushed the feaure/create_detailed_erc20_interface branch from cde7f44 to 365c875 Compare November 13, 2017 18:36
@frangio
Copy link
Copy Markdown
Contributor

frangio commented Nov 13, 2017

Thanks @facuspagnuolo!

@frangio frangio merged commit 84bffb8 into OpenZeppelin:master Nov 13, 2017
@facuspagnuolo facuspagnuolo deleted the feaure/create_detailed_erc20_interface branch November 13, 2017 18:45
ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018
…detailed_erc20_interface

Create detailed ERC20 interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants