Skip to content

Conversation

@facuspagnuolo
Copy link
Contributor

No description provided.

@maraoz maraoz requested review from frangio and maraoz September 29, 2017 19:29
Copy link
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

contract DetailedERC20 is ERC20 {
string public name;
string public symbol;
uint256 public decimals;
Copy link
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.)


contract DetailedERC20Mock is StandardToken, DetailedERC20 {
function DetailedERC20Mock(uint256 _decimals, string _symbol, string _name) {
require(_decimals > 0);
Copy link
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.

import '../../contracts/token/DetailedERC20.sol';

contract DetailedERC20Mock is StandardToken, DetailedERC20 {
function DetailedERC20Mock(uint256 _decimals, string _symbol, string _name) {
Copy link
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.


it('has a name', async function () {
const name = await detailedERC20.name();
name.should.be.equal('My Detailed ERC20');
Copy link
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.

uint256 public decimals;
uint8 public decimals;

function DetailedERC20(string name, string _symbol, uint8 _decimals) {
Copy link
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
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
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