From 49fbdc499501c302c987f7efb0f5d302cf174f79 Mon Sep 17 00:00:00 2001 From: Peter Emil Jensen Date: Tue, 5 Feb 2019 23:15:27 +0100 Subject: [PATCH] Added several ERC20 optimizations (#93) * Removed redundant requires as they are tested in SafeMath * Added increased and decreased balance for storage contract * Added first change suggested by @trulsa * Added the second change as suggested by @trulsa * Added extra use of storage decrease and increase in ERC20 --- contracts/token/ERC20/ERC20.sol | 50 ++-- contracts/token/ERC20/Storage.sol | 364 +++++++++++++++++------------- test/token/ERC20/Storage.test.js | 160 +++++++++++++ 3 files changed, 390 insertions(+), 184 deletions(-) diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index ca000cb..f1feaac 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -134,16 +134,10 @@ contract ERC20 { internal returns (bool) { - require(value <= externalStorage.getBalance(originSender)); require(to != address(0)); - externalStorage.setBalance( - originSender, - externalStorage.getBalance(originSender).sub(value)); - externalStorage.setBalance( - to, - externalStorage.getBalance(to).add(value) - ); + externalStorage.decreaseBalance(originSender, value); + externalStorage.increaseBalance(to, value); emit Transfer(originSender, to, value); @@ -191,13 +185,11 @@ contract ERC20 { internal returns (bool) { - require(value <= externalStorage.getAllowed(from, originSender)); - externalStorage.setAllowed( - from, originSender, - externalStorage.getAllowed(from, originSender).sub(value) - ); + externalStorage.decreaseAllowed(from, originSender, value); + _transfer(from, to, value); + emit Approval( from, originSender, @@ -227,10 +219,8 @@ contract ERC20 { { require(spender != address(0)); - externalStorage.setAllowed( - originSender, spender, - externalStorage.getAllowed(originSender, spender).add(addedValue) - ); + externalStorage.increaseAllowed(originSender, spender, addedValue); + emit Approval( originSender, spender, externalStorage.getAllowed(originSender, spender) @@ -259,10 +249,10 @@ contract ERC20 { { require(spender != address(0)); - externalStorage.setAllowed( - originSender, spender, - externalStorage.getAllowed(originSender, spender).sub(subtractedValue) - ); + externalStorage.decreaseAllowed(originSender, + spender, + subtractedValue); + emit Approval( originSender, spender, externalStorage.getAllowed(originSender, spender) @@ -281,10 +271,11 @@ contract ERC20 { function _mint(address account, uint256 value) internal returns (bool) { require(account != 0); + externalStorage.setTotalSupply( externalStorage.getTotalSupply().add(value)); - externalStorage.setBalance( - account, externalStorage.getBalance(account).add(value)); + externalStorage.increaseBalance(account, value); + emit Transfer(address(0), account, value); return true; @@ -299,14 +290,11 @@ contract ERC20 { function _burn(address originSender, uint256 value) internal returns (bool) { require(originSender != 0); - require(value <= externalStorage.getBalance(originSender)); externalStorage.setTotalSupply( externalStorage.getTotalSupply().sub(value)); - externalStorage.setBalance( - originSender, - externalStorage.getBalance(originSender).sub(value) - ); + externalStorage.decreaseBalance(originSender, value); + emit Transfer(originSender, address(0), value); return true; @@ -326,11 +314,9 @@ contract ERC20 { { require(value <= externalStorage.getAllowed(account, originSender)); - externalStorage.setAllowed( - account, originSender, - externalStorage.getAllowed(account, originSender).sub(value) - ); + externalStorage.decreaseAllowed(account, originSender, value); _burn(account, value); + emit Approval(account, originSender, externalStorage.getAllowed(account, originSender)); diff --git a/contracts/token/ERC20/Storage.sol b/contracts/token/ERC20/Storage.sol index f660201..81acb77 100644 --- a/contracts/token/ERC20/Storage.sol +++ b/contracts/token/ERC20/Storage.sol @@ -1,6 +1,7 @@ pragma solidity 0.4.24; import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; +import "openzeppelin-solidity/contracts/math/SafeMath.sol"; /** * @title External ERC20 Storage @@ -12,156 +13,215 @@ import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; * by the implementor. */ contract Storage is Ownable { - - mapping (address => uint256) private balances; - mapping (address => mapping (address => uint256)) private allowed; - uint256 private totalSupply; - - address private _implementor; - - event StorageImplementorTransferred(address indexed from, - address indexed to); - - /** - * @dev Contructor. - * @param owner The address of the owner of the contract. - * Must not be the zero address. - * @param implementor The address of the contract that is - * allowed to change state. Must not be the zero address. - */ - constructor(address owner, address implementor) public { - - require( - owner != address(0), - "Owner should not be the zero address" - ); - - require( - implementor != address(0), - "Implementor should not be the zero address" - ); - - transferOwnership(owner); - _implementor = implementor; - } - - /** - * @dev Return whether the sender is an implementor. - */ - function isImplementor() public view returns(bool) { - return msg.sender == _implementor; - } - - /** - * @dev Sets new balance. - * Can only be done by owner or implementor contract. - */ - function setBalance(address owner, - uint256 value) - public - onlyImplementor - { - balances[owner] = value; - } - - /** - * @dev Can only be done by owner or implementor contract. - * @return The current balance of owner - */ - function getBalance(address owner) - public - view - returns (uint256) - { - return balances[owner]; - } - - /** - * @dev Sets new allowance. - * Can only be called by implementor contract. - */ - function setAllowed(address owner, - address spender, - uint256 value) - public - onlyImplementor - { - allowed[owner][spender] = value; - } - - /** - * @dev Can only be called by implementor contract. - * @return The current allowance for spender from owner - */ - function getAllowed(address owner, - address spender) - public - view - returns (uint256) - { - return allowed[owner][spender]; - } - - /** - * @dev Change totalSupply. - * Can only be called by implementor contract. - */ - function setTotalSupply(uint256 value) - public - onlyImplementor - { - totalSupply = value; - } - - /** - * @dev Can only be called by implementor contract. - * @return Current supply - */ - function getTotalSupply() - public - view - returns (uint256) - { - return totalSupply; - } - - /** - * @dev Transfer implementor to new contract - * Can only be called by owner or implementor contract. - */ - function transferImplementor(address newImplementor) - public - requireNonZero(newImplementor) - onlyImplementorOrOwner - { - require(newImplementor != _implementor, - "Cannot transfer to same implementor as existing"); - address curImplementor = _implementor; - _implementor = newImplementor; - emit StorageImplementorTransferred(curImplementor, newImplementor); - } - - /** - * @dev Asserts that sender is either owner or implementor. - */ - modifier onlyImplementorOrOwner() { - require(isImplementor() || isOwner(), "Is not implementor or owner"); - _; - } - - /** - * @dev Asserts that sender is the implementor. - */ - modifier onlyImplementor() { - require(isImplementor(), "Is not implementor"); - _; - } - - /** - * @dev Asserts that the given address is not the null-address - */ - modifier requireNonZero(address addr) { - require(addr != address(0), "Expected a non-zero address"); - _; - } + using SafeMath for uint256; + + mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; + uint256 private totalSupply; + + address private _implementor; + + event StorageImplementorTransferred(address indexed from, + address indexed to); + + /** + * @dev Contructor. + * @param owner The address of the owner of the contract. + * Must not be the zero address. + * @param implementor The address of the contract that is + * allowed to change state. Must not be the zero address. + */ + constructor(address owner, address implementor) public { + + require( + owner != address(0), + "Owner should not be the zero address" + ); + + require( + implementor != address(0), + "Implementor should not be the zero address" + ); + + transferOwnership(owner); + _implementor = implementor; + } + + /** + * @dev Return whether the sender is an implementor. + */ + function isImplementor() public view returns(bool) { + return msg.sender == _implementor; + } + + /** + * @dev Sets new balance. + * Can only be done by owner or implementor contract. + */ + function setBalance(address owner, + uint256 value) + public + onlyImplementor + { + balances[owner] = value; + } + + /** + * @dev Increases the balances relatively + * @param owner the address for which to increase balance + * @param addedValue the value to increase with + */ + function increaseBalance(address owner, uint256 addedValue) + public + onlyImplementor + { + balances[owner] = balances[owner].add(addedValue); + } + + /** + * @dev Decreases the balances relatively + * @param owner the address for which to decrease balance + * @param subtractedValue the value to decrease with + */ + function decreaseBalance(address owner, uint256 subtractedValue) + public + onlyImplementor + { + balances[owner] = balances[owner].sub(subtractedValue); + } + + /** + * @dev Can only be done by owner or implementor contract. + * @return The current balance of owner + */ + function getBalance(address owner) + public + view + returns (uint256) + { + return balances[owner]; + } + + /** + * @dev Sets new allowance. + * Can only be called by implementor contract. + */ + function setAllowed(address owner, + address spender, + uint256 value) + public + onlyImplementor + { + allowed[owner][spender] = value; + } + + /** + * @dev Increases the allowance relatively + * @param owner the address for which to allow from + * @param spender the addres for which the allowance increase is granted + * @param addedValue the value to increase with + */ + function increaseAllowed( + address owner, + address spender, + uint256 addedValue + ) + public + onlyImplementor + { + allowed[owner][spender] = allowed[owner][spender].add(addedValue); + } + + /** + * @dev Decreases the allowance relatively + * @param owner the address for which to allow from + * @param spender the addres for which the allowance decrease is granted + * @param subtractedValue the value to decrease with + */ + function decreaseAllowed( + address owner, + address spender, + uint256 subtractedValue + ) + public + onlyImplementor + { + allowed[owner][spender] = allowed[owner][spender].sub(subtractedValue); + } + + /** + * @dev Can only be called by implementor contract. + * @return The current allowance for spender from owner + */ + function getAllowed(address owner, + address spender) + public + view + returns (uint256) + { + return allowed[owner][spender]; + } + + /** + * @dev Change totalSupply. + * Can only be called by implementor contract. + */ + function setTotalSupply(uint256 value) + public + onlyImplementor + { + totalSupply = value; + } + + /** + * @dev Can only be called by implementor contract. + * @return Current supply + */ + function getTotalSupply() + public + view + returns (uint256) + { + return totalSupply; + } + + /** + * @dev Transfer implementor to new contract + * Can only be called by owner or implementor contract. + */ + function transferImplementor(address newImplementor) + public + requireNonZero(newImplementor) + onlyImplementorOrOwner + { + require(newImplementor != _implementor, + "Cannot transfer to same implementor as existing"); + address curImplementor = _implementor; + _implementor = newImplementor; + emit StorageImplementorTransferred(curImplementor, newImplementor); + } + + /** + * @dev Asserts that sender is either owner or implementor. + */ + modifier onlyImplementorOrOwner() { + require(isImplementor() || isOwner(), "Is not implementor or owner"); + _; + } + + /** + * @dev Asserts that sender is the implementor. + */ + modifier onlyImplementor() { + require(isImplementor(), "Is not implementor"); + _; + } + + /** + * @dev Asserts that the given address is not the null-address + */ + modifier requireNonZero(address addr) { + require(addr != address(0), "Expected a non-zero address"); + _; + } } diff --git a/test/token/ERC20/Storage.test.js b/test/token/ERC20/Storage.test.js index 0ded21a..18051d1 100644 --- a/test/token/ERC20/Storage.test.js +++ b/test/token/ERC20/Storage.test.js @@ -138,6 +138,78 @@ contract('Storage', function ([_, owner, implementor, anotherAccount, thirdAccou }); }); + describe('increaseBalance', function () { + const initialBalance = 100; + const increasedValue = 200; + + const totalValue = initialBalance + increasedValue; + + describe('when is owner', function () { + it('should not allow changing balance', async function () { + await utils.assertRevertsReason( + this.token.increaseBalance(anotherAccount, increasedValue, { from: owner }), + 'Is not implementor'); + }); + }); + + describe('when is implementor', function () { + it('should allow increasing balance', async function () { + await this.token.setBalance(anotherAccount, initialBalance, { from: implementor }); + (await this.token.getBalance(anotherAccount)).should.be.bignumber.equal(initialBalance); + await this.token.increaseBalance(anotherAccount, increasedValue, { from: implementor }); + (await this.token.getBalance(anotherAccount)).should.be.bignumber.equal(totalValue); + }); + }); + + describe('when is not owner or implementor', function () { + it('should not allow increasing balance', async function () { + const oldBalance = await this.token.getBalance(anotherAccount); + + oldBalance.should.not.be.bignumber.equal(totalValue); + await utils.assertRevertsReason( + this.token.increaseBalance(anotherAccount, increasedValue, { from: thirdAccount }), + 'Is not implementor'); + (await this.token.getBalance(anotherAccount)).should.be.bignumber.equal(oldBalance); + }); + }); + }); + + describe('decreaseBalance', function () { + const initialBalance = 100; + const decreasedValue = 50; + + const totalValue = initialBalance - decreasedValue; + + describe('when is owner', function () { + it('should not allow changing balance', async function () { + await utils.assertRevertsReason( + this.token.decreaseBalance(anotherAccount, decreasedValue, { from: owner }), + 'Is not implementor'); + }); + }); + + describe('when is implementor', function () { + it('should allow decreasing balance', async function () { + await this.token.setBalance(anotherAccount, initialBalance, { from: implementor }); + (await this.token.getBalance(anotherAccount)).should.be.bignumber.equal(initialBalance); + await this.token.decreaseBalance(anotherAccount, decreasedValue, { from: implementor }); + (await this.token.getBalance(anotherAccount)).should.be.bignumber.equal(totalValue); + }); + }); + + describe('when is not owner or implementor', function () { + it('should not allow decreasing balance', async function () { + const oldBalance = await this.token.getBalance(anotherAccount); + + oldBalance.should.not.be.bignumber.equal(totalValue); + await utils.assertRevertsReason( + this.token.decreaseBalance(anotherAccount, decreasedValue, { from: thirdAccount }), + 'Is not implementor'); + (await this.token.getBalance(anotherAccount)).should.be.bignumber.equal(oldBalance); + }); + }); + }); + describe('setAllowed', function () { const newAllowance = 200; @@ -170,6 +242,94 @@ contract('Storage', function ([_, owner, implementor, anotherAccount, thirdAccou }); }); + describe('increaseAllowed', function () { + const initialBalance = 100; + const increasedValue = 200; + + const totalValue = initialBalance + increasedValue; + + describe('when is owner', function () { + it('should not allow changing allowance', async function () { + await utils.assertRevertsReason( + this.token.increaseAllowed(anotherAccount, thirdAccount, + increasedValue, { from: owner }), + 'Is not implementor'); + }); + }); + + describe('when is implementor', function () { + it('should allow increasing allowance', async function () { + await this.token.setAllowed(anotherAccount, thirdAccount, + initialBalance, { from: implementor }); + (await this.token.getAllowed(anotherAccount, thirdAccount)) + .should.be.bignumber.equal(initialBalance); + + await this.token.increaseAllowed(anotherAccount, thirdAccount, + increasedValue, { from: implementor }); + (await this.token.getAllowed(anotherAccount, thirdAccount)) + .should.be.bignumber.equal(totalValue); + }); + }); + + describe('when is not owner or implementor', function () { + it('should not allow increasing allowance', async function () { + const oldBalance = await this.token.getAllowed(anotherAccount, thirdAccount); + + oldBalance.should.not.be.bignumber.equal(totalValue); + await utils.assertRevertsReason( + this.token.increaseAllowed(anotherAccount, thirdAccount, + increasedValue, { from: thirdAccount }), + 'Is not implementor'); + (await this.token.getAllowed(anotherAccount, thirdAccount)) + .should.be.bignumber.equal(oldBalance); + }); + }); + }); + + describe('decreaseAllowed', function () { + const initialBalance = 100; + const decreasedValue = 50; + + const totalValue = initialBalance - decreasedValue; + + describe('when is owner', function () { + it('should not allow changing allowance', async function () { + await utils.assertRevertsReason( + this.token.decreaseAllowed(anotherAccount, thirdAccount, + decreasedValue, { from: owner }), + 'Is not implementor'); + }); + }); + + describe('when is implementor', function () { + it('should allow decreasing allowance', async function () { + await this.token.setAllowed(anotherAccount, thirdAccount, + initialBalance, { from: implementor }); + (await this.token.getAllowed(anotherAccount, thirdAccount)) + .should.be.bignumber.equal(initialBalance); + + await this.token.decreaseAllowed(anotherAccount, thirdAccount, + decreasedValue, { from: implementor }); + (await this.token.getAllowed(anotherAccount, thirdAccount)) + .should.be.bignumber.equal(totalValue); + }); + }); + + describe('when is not owner or implementor', function () { + it('should not allow decreasing allowance', async function () { + const oldBalance = await this.token.getAllowed(anotherAccount, thirdAccount); + + oldBalance.should.not.be.bignumber.equal(totalValue); + await utils.assertRevertsReason( + this.token.decreaseAllowed(anotherAccount, thirdAccount, + decreasedValue, { from: thirdAccount }), + 'Is not implementor'); + (await this.token.getAllowed(anotherAccount, thirdAccount)) + .should.be.bignumber.equal(oldBalance); + }); + }); + }); + describe('setTotalSupply', function () { const newTotalSupply = 200;