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

StandardToken encapsulation #1197

Merged
merged 13 commits into from Aug 16, 2018
4 changes: 1 addition & 3 deletions contracts/examples/SimpleToken.sol
Expand Up @@ -22,9 +22,7 @@ contract SimpleToken is StandardToken {
* @dev Constructor that gives msg.sender all of existing tokens.
*/
constructor() public {
totalSupply_ = INITIAL_SUPPLY;
balances[msg.sender] = INITIAL_SUPPLY;
emit Transfer(address(0), msg.sender, INITIAL_SUPPLY);
_mint(msg.sender, INITIAL_SUPPLY);
}

}
3 changes: 1 addition & 2 deletions contracts/mocks/BurnableTokenMock.sol
Expand Up @@ -6,8 +6,7 @@ import "../token/ERC20/BurnableToken.sol";
contract BurnableTokenMock is BurnableToken {

constructor(address _initialAccount, uint256 _initialBalance) public {
balances[_initialAccount] = _initialBalance;
totalSupply_ = _initialBalance;
_mint(_initialAccount, _initialBalance);
}

}
3 changes: 1 addition & 2 deletions contracts/mocks/ERC223TokenMock.sol
Expand Up @@ -11,8 +11,7 @@ contract ERC223ContractInterface {
contract ERC223TokenMock is StandardToken {

constructor(address _initialAccount, uint256 _initialBalance) public {
balances[_initialAccount] = _initialBalance;
totalSupply_ = _initialBalance;
_mint(_initialAccount, _initialBalance);
}

// ERC223 compatible transfer function (except the name)
Expand Down
2 changes: 1 addition & 1 deletion contracts/mocks/PausableTokenMock.sol
Expand Up @@ -7,7 +7,7 @@ import "../token/ERC20/PausableToken.sol";
contract PausableTokenMock is PausableToken {

constructor(address _initialAccount, uint _initialBalance) public {
balances[_initialAccount] = _initialBalance;
_mint(_initialAccount, _initialBalance);
}

}
15 changes: 13 additions & 2 deletions contracts/mocks/StandardTokenMock.sol
Expand Up @@ -7,8 +7,19 @@ import "../token/ERC20/StandardToken.sol";
contract StandardTokenMock is StandardToken {

constructor(address _initialAccount, uint256 _initialBalance) public {
balances[_initialAccount] = _initialBalance;
totalSupply_ = _initialBalance;
_mint(_initialAccount, _initialBalance);
}

function mint(address _account, uint256 _amount) public {
_mint(_account, _amount);
}

function burn(address _account, uint256 _amount) public {
_burn(_account, _amount);
}

function burnFrom(address _account, uint256 _amount) public {
_burnFrom(_account, _amount);
}

}
20 changes: 7 additions & 13 deletions contracts/token/ERC20/BurnableToken.sol
Expand Up @@ -25,21 +25,15 @@ contract BurnableToken is StandardToken {
* @param _value uint256 The amount of token to be burned
*/
function burnFrom(address _from, uint256 _value) public {
require(_value <= allowed[_from][msg.sender]);
// Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted,
// this function needs to emit an event with the updated approval.
allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value);
_burn(_from, _value);
_burnFrom(_from, _value);
}

/**
* @dev Overrides StandardToken._burn in order for burn and burnFrom to emit
* an additional Burn event.
*/
function _burn(address _who, uint256 _value) internal {
require(_value <= balances[_who]);
// no need to require value <= totalSupply, since that would imply the
// sender's balance is greater than the totalSupply, which *should* be an assertion failure

balances[_who] = balances[_who].sub(_value);
totalSupply_ = totalSupply_.sub(_value);
super._burn(_who, _value);
Copy link
Contributor

@nventuro nventuro Aug 13, 2018

Choose a reason for hiding this comment

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

This is confusing: the public burn function calls the internal _burn function, which then calls super._burn and emits an event. Why not remove the internal _burn and just call base function from the public one?

edit: you did it right in the mint function, so I'm assuming this wasn't intentional :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because both burn and burnFrom need to add the Burn event. I felt it was better than adding the event in both. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's even worse now that I realize it lol. burnFrom calls _burnFrom, which calls the overriden _burn, which calls the original burn: unless you know the details of StandardToken, you can't tell that burnFrom will emit a Burn event. The reason why you did it makes sense though.

I think the problem is the missing comment: if the overridden function said something like 'adds the Burn event when burning', it may read more clearly.

emit Burn(_who, _value);
emit Transfer(_who, address(0), _value);
}
}
}
2 changes: 1 addition & 1 deletion contracts/token/ERC20/CappedToken.sol
Expand Up @@ -29,7 +29,7 @@ contract CappedToken is MintableToken {
public
returns (bool)
{
require(totalSupply_.add(_amount) <= cap);
require(totalSupply().add(_amount) <= cap);

return super.mint(_to, _amount);
}
Expand Down
4 changes: 1 addition & 3 deletions contracts/token/ERC20/MintableToken.sol
Expand Up @@ -41,10 +41,8 @@ contract MintableToken is StandardToken, Ownable {
canMint
returns (bool)
{
totalSupply_ = totalSupply_.add(_amount);
balances[_to] = balances[_to].add(_amount);
_mint(_to, _amount);
emit Mint(_to, _amount);
emit Transfer(address(0), _to, _amount);
return true;
}

Expand Down
52 changes: 48 additions & 4 deletions contracts/token/ERC20/StandardToken.sol
Expand Up @@ -8,17 +8,17 @@ import "../../math/SafeMath.sol";
* @title Standard ERC20 token
*
* @dev Implementation of the basic standard token.
* https://github.com/ethereum/EIPs/issues/20
* https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20.md
* Based on code by FirstBlood: https://github.com/Firstbloodio/token/blob/master/smart_contract/FirstBloodToken.sol
*/
contract StandardToken is ERC20 {
using SafeMath for uint256;

mapping(address => uint256) balances;
mapping (address => uint256) private balances;

mapping (address => mapping (address => uint256)) internal allowed;
mapping (address => mapping (address => uint256)) private allowed;

uint256 totalSupply_;
uint256 private totalSupply_;

/**
* @dev Total number of tokens in existence
Expand Down Expand Up @@ -156,4 +156,48 @@ contract StandardToken is ERC20 {
return true;
}

/**
* @dev Internal function that mints an amount of the token and assigns it to
* an account. This encapsulates the modification of balances such that the
* proper events are emitted.
* @param _account The account that will receive the created tokens.
* @param _amount The amount that will be created.
*/
function _mint(address _account, uint256 _amount) internal {
require(_account != 0);
totalSupply_ = totalSupply_.add(_amount);
balances[_account] = balances[_account].add(_amount);
emit Transfer(address(0), _account, _amount);
}

/**
* @dev Internal function that burns an amount of the token of a given
* account.
* @param _account The account whose tokens will be burnt.
* @param _amount The amount that will be burnt.
*/
function _burn(address _account, uint256 _amount) internal {
require(_account != 0);
require(balances[_account] > _amount);

totalSupply_ = totalSupply_.sub(_amount);
balances[_account] = balances[_account].sub(_amount);
emit Transfer(_account, address(0), _amount);
}

/**
* @dev Internal function that burns an amount of the token of a given
* account, deducting from the sender's allowance for said account. Uses the
* internal _burn function.
* @param _account The account whose tokens will be burnt.
* @param _amount The amount that will be burnt.
*/
function _burnFrom(address _account, uint256 _amount) internal {
require(allowed[_account][msg.sender] > _amount);

// Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted,
// this function needs to emit an event with the updated approval.
allowed[_account][msg.sender] = allowed[_account][msg.sender].sub(_amount);
_burn(_account, _amount);
}
}
5 changes: 3 additions & 2 deletions test/token/ERC20/CappedToken.behavior.js
@@ -1,4 +1,5 @@
const { expectThrow } = require('../../helpers/expectThrow');
const expectEvent = require('../../helpers/expectEvent');

const BigNumber = web3.BigNumber;

Expand All @@ -15,8 +16,8 @@ function shouldBehaveLikeCappedToken (minter, [anyone], cap) {
});

it('should mint when amount is less than cap', async function () {
const result = await this.token.mint(anyone, cap.sub(1), { from });
result.logs[0].event.should.equal('Mint');
const { logs } = await this.token.mint(anyone, cap.sub(1), { from });
expectEvent.inLogs(logs, 'Mint');
});

it('should fail to mint if the ammount exceeds the cap', async function () {
Expand Down
18 changes: 13 additions & 5 deletions test/token/ERC20/MintableToken.behavior.js
@@ -1,4 +1,5 @@
const { assertRevert } = require('../../helpers/assertRevert');
const expectEvent = require('../../helpers/expectEvent');

const BigNumber = web3.BigNumber;

Expand All @@ -7,6 +8,8 @@ require('chai')
.should();

function shouldBehaveLikeMintableToken (owner, minter, [anyone]) {
const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';

describe('as a basic mintable token', function () {
describe('after token creation', function () {
it('sender should be token owner', async function () {
Expand Down Expand Up @@ -104,11 +107,16 @@ function shouldBehaveLikeMintableToken (owner, minter, [anyone]) {
it('emits a mint and a transfer event', async function () {
const { logs } = await this.token.mint(owner, amount, { from });

logs.length.should.eq(2);
logs[0].event.should.eq('Mint');
logs[0].args.to.should.eq(owner);
logs[0].args.amount.should.be.bignumber.equal(amount);
logs[1].event.should.eq('Transfer');
const mintEvent = expectEvent.inLogs(logs, 'Mint', {
to: owner,
});
mintEvent.args.amount.should.be.bignumber.equal(amount);
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 actually put that in the args object as well:

const mintEvent = expectEvent.inLogs(logs, 'Mint', {
  to: owner,
  amount,
});

and it will do the bignumber equals check. likewise below

Copy link
Contributor Author

@frangio frangio Aug 15, 2018

Choose a reason for hiding this comment

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

Are you sure about this? The code for the helper looks like it uses JavaScript's equality operator.

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/8d11dcc0e5fb0fd50e018d7786750fa45e54d4c5/test/helpers/expectEvent.js#L6-L9

This was actually one of the items @nventuro mentioned in #1026.


const transferEvent = expectEvent.inLogs(logs, 'Transfer', {
from: ZERO_ADDRESS,
to: owner,
});
transferEvent.args.value.should.be.bignumber.equal(amount);
});
});

Expand Down
145 changes: 140 additions & 5 deletions test/token/ERC20/StandardToken.test.js
@@ -1,4 +1,6 @@
const { assertRevert } = require('../../helpers/assertRevert');
const expectEvent = require('../../helpers/expectEvent');

const StandardToken = artifacts.require('StandardTokenMock');

const BigNumber = web3.BigNumber;
Expand Down Expand Up @@ -68,11 +70,12 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) {
it('emits a transfer event', async function () {
const { logs } = await this.token.transfer(to, amount, { from: owner });

logs.length.should.eq(1);
logs[0].event.should.eq('Transfer');
logs[0].args.from.should.eq(owner);
logs[0].args.to.should.eq(to);
logs[0].args.value.should.be.bignumber.equal(amount);
const event = expectEvent.inLogs(logs, 'Transfer', {
from: owner,
to: to,
});

event.args.value.should.be.bignumber.equal(amount);
});
});
});
Expand Down Expand Up @@ -486,4 +489,136 @@ contract('StandardToken', function ([_, owner, recipient, anotherAccount]) {
});
});
});

describe('_mint', function () {
const initialSupply = new BigNumber(100);
const amount = new BigNumber(50);

it('rejects a null account', async function () {
await assertRevert(this.token.mint(ZERO_ADDRESS, amount));
});

describe('for a non null account', function () {
beforeEach('minting', async function () {
const { logs } = await this.token.mint(recipient, amount);
this.logs = logs;
});

it('increments totalSupply', async function () {
const expectedSupply = initialSupply.plus(amount);
(await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply);
});

it('increments recipient balance', async function () {
(await this.token.balanceOf(recipient)).should.be.bignumber.equal(amount);
});

it('emits Transfer event', async function () {
const event = expectEvent.inLogs(this.logs, 'Transfer', {
from: ZERO_ADDRESS,
to: recipient,
});

event.args.value.should.be.bignumber.equal(amount);
});
});
});

describe('_burn', function () {
const initialSupply = new BigNumber(100);
const amount = new BigNumber(50);

it('rejects a null account', async function () {
await assertRevert(this.token.burn(ZERO_ADDRESS, amount));
});

describe('for a non null account', function () {
it('rejects burning more than balance', async function () {
await assertRevert(this.token.burn(owner, initialSupply.plus(1)));
});

describe('for less amount than balance', function () {
beforeEach('burning', async function () {
const { logs } = await this.token.burn(owner, amount);
this.logs = logs;
});

it('decrements totalSupply', async function () {
const expectedSupply = initialSupply.minus(amount);
(await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply);
});

it('decrements owner balance', async function () {
const expectedBalance = initialSupply.minus(amount);
(await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance);
});

it('emits Transfer event', async function () {
const event = expectEvent.inLogs(this.logs, 'Transfer', {
from: owner,
to: ZERO_ADDRESS,
});

event.args.value.should.be.bignumber.equal(amount);
});
});
});
});

describe('_burnFrom', function () {
const initialSupply = new BigNumber(100);
const allowance = new BigNumber(70);
const amount = new BigNumber(50);

const spender = anotherAccount;

beforeEach('approving', async function () {
await this.token.approve(spender, allowance, { from: owner });
});

it('rejects a null account', async function () {
await assertRevert(this.token.burnFrom(ZERO_ADDRESS, amount));
});

describe('for a non null account', function () {
it('rejects burning more than allowance', async function () {
await assertRevert(this.token.burnFrom(owner, allowance.plus(1)));
});

it('rejects burning more than balance', async function () {
await assertRevert(this.token.burnFrom(owner, initialSupply.plus(1)));
});

describe('for less amount than allowance', function () {
beforeEach('burning', async function () {
const { logs } = await this.token.burnFrom(owner, amount, { from: spender });
this.logs = logs;
});

it('decrements totalSupply', async function () {
const expectedSupply = initialSupply.minus(amount);
(await this.token.totalSupply()).should.be.bignumber.equal(expectedSupply);
});

it('decrements owner balance', async function () {
const expectedBalance = initialSupply.minus(amount);
(await this.token.balanceOf(owner)).should.be.bignumber.equal(expectedBalance);
});

it('decrements spender allowance', async function () {
const expectedAllowance = allowance.minus(amount);
(await this.token.allowance(owner, spender)).should.be.bignumber.equal(expectedAllowance);
});

it('emits Transfer event', async function () {
const event = expectEvent.inLogs(this.logs, 'Transfer', {
from: owner,
to: ZERO_ADDRESS,
});

event.args.value.should.be.bignumber.equal(amount);
});
});
});
});
});