Skip to content

Commit

Permalink
Merge branch 'master' into erc20snapshot-fix#1209
Browse files Browse the repository at this point in the history
  • Loading branch information
nventuro authored Jan 22, 2019
2 parents a00f801 + f80c65f commit da36463
Show file tree
Hide file tree
Showing 10 changed files with 261 additions and 181 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@

### New features:
* `ERC20Snapshot`: create snapshots on demand of the token balances and total supply, to later retrieve and e.g. calculate voting power at a past time.
* `ERC20`: added internal `_approve(address owner, address spender, uint256 value)`, allowing derived contracts to set the allowance of arbitrary accounts.

### Improvements:
* Upgraded the minimum compiler version to v0.5.2: this removes many Solidity warnings that were false positives.
* `Counter`'s API has been improved, and is now used by `ERC721` (though it is still in `drafts`).
* `ERC721`'s transfers are now more gas efficient due to removal of unnecessary `SafeMath` calls.
* Fixed variable shadowing issues.

### Bugfixes:
Expand Down
35 changes: 24 additions & 11 deletions contracts/drafts/Counters.sol
Original file line number Diff line number Diff line change
@@ -1,24 +1,37 @@
pragma solidity ^0.5.2;

import "../math/SafeMath.sol";

/**
* @title Counters
* @author Matt Condon (@shrugs)
* @dev Provides an incrementing uint256 id acquired by the `Counter#next` getter.
* Use this for issuing ERC721 ids or keeping track of request ids, anything you want, really.
* @dev Provides counters that can only be incremented or decremented by one. This can be used e.g. to track the number
* of elements in a mapping, issuing ERC721 ids, or counting request ids
*
* Include with `using Counters` for Counters.Counter;`
* @notice Does not allow an Id of 0, which is popularly used to signify a null state in solidity.
* Does not protect from overflows, but if you have 2^256 ids, you have other problems.
* (But actually, it's generally impossible to increment a counter this many times, energy wise
* so it's not something you have to worry about.)
* Include with `using Counter for Counter.Counter;`
* Since it is not possible to overflow a 256 bit integer with increments of one, `increment` can skip the SafeMath
* overflow check, thereby saving gas. This does assume however correct usage, in that the underlying `_value` is never
* directly accessed.
*/
library Counters {
using SafeMath for uint256;

struct Counter {
uint256 current; // default: 0
// This variable should never be directly accessed by users of the library: interactions must be restricted to
// the library's function. As of Solidity v0.5.2, this cannot be enforced, though there is a proposal to add
// this feature: see https://github.com/ethereum/solidity/issues/4637
uint256 _value; // default: 0
}

function current(Counter storage counter) internal view returns (uint256) {
return counter._value;
}

function increment(Counter storage counter) internal {
counter._value += 1;
}

function next(Counter storage index) internal returns (uint256) {
index.current += 1;
return index.current;
function decrement(Counter storage counter) internal {
counter._value = counter._value.sub(1);
}
}
16 changes: 10 additions & 6 deletions contracts/mocks/CountersImpl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@ import "../drafts/Counters.sol";
contract CountersImpl {
using Counters for Counters.Counter;

uint256 public theId;
Counters.Counter private _counter;

// use whatever key you want to track your counters
mapping(string => Counters.Counter) private _counters;
function current() public view returns (uint256) {
return _counter.current();
}

function increment() public {
_counter.increment();
}

function doThing(string memory key) public returns (uint256) {
theId = _counters[key].next();
return theId;
function decrement() public {
_counter.decrement();
}
}
4 changes: 4 additions & 0 deletions contracts/mocks/ERC20Mock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,8 @@ contract ERC20Mock is ERC20 {
function burnFrom(address account, uint256 amount) public {
_burnFrom(account, amount);
}

function approveInternal(address owner, address spender, uint256 value) public {
_approve(owner, spender, value);
}
}
35 changes: 19 additions & 16 deletions contracts/token/ERC20/ERC20.sol
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ contract ERC20 is IERC20 {
* @param value The amount of tokens to be spent.
*/
function approve(address spender, uint256 value) public returns (bool) {
require(spender != address(0));

_allowed[msg.sender][spender] = value;
emit Approval(msg.sender, spender, value);
_approve(msg.sender, spender, value);
return true;
}

Expand All @@ -86,9 +83,8 @@ contract ERC20 is IERC20 {
* @param value uint256 the amount of tokens to be transferred
*/
function transferFrom(address from, address to, uint256 value) public returns (bool) {
_allowed[from][msg.sender] = _allowed[from][msg.sender].sub(value);
_transfer(from, to, value);
emit Approval(from, msg.sender, _allowed[from][msg.sender]);
_approve(from, msg.sender, _allowed[from][msg.sender].sub(value));
return true;
}

Expand All @@ -103,10 +99,7 @@ contract ERC20 is IERC20 {
* @param addedValue The amount of tokens to increase the allowance by.
*/
function increaseAllowance(address spender, uint256 addedValue) public returns (bool) {
require(spender != address(0));

_allowed[msg.sender][spender] = _allowed[msg.sender][spender].add(addedValue);
emit Approval(msg.sender, spender, _allowed[msg.sender][spender]);
_approve(msg.sender, spender, _allowed[msg.sender][spender].add(addedValue));
return true;
}

Expand All @@ -121,10 +114,7 @@ contract ERC20 is IERC20 {
* @param subtractedValue The amount of tokens to decrease the allowance by.
*/
function decreaseAllowance(address spender, uint256 subtractedValue) public returns (bool) {
require(spender != address(0));

_allowed[msg.sender][spender] = _allowed[msg.sender][spender].sub(subtractedValue);
emit Approval(msg.sender, spender, _allowed[msg.sender][spender]);
_approve(msg.sender, spender, _allowed[msg.sender][spender].sub(subtractedValue));
return true;
}

Expand Down Expand Up @@ -171,6 +161,20 @@ contract ERC20 is IERC20 {
emit Transfer(account, address(0), value);
}

/**
* @dev Approve an address to spend another addresses' tokens.
* @param owner The address that owns the tokens.
* @param spender The address that will spend the tokens.
* @param value The number of tokens that can be spent.
*/
function _approve(address owner, address spender, uint256 value) internal {
require(spender != address(0));
require(owner != address(0));

_allowed[owner][spender] = value;
emit Approval(owner, spender, value);
}

/**
* @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
Expand All @@ -180,8 +184,7 @@ contract ERC20 is IERC20 {
* @param value The amount that will be burnt.
*/
function _burnFrom(address account, uint256 value) internal {
_allowed[account][msg.sender] = _allowed[account][msg.sender].sub(value);
_burn(account, value);
emit Approval(account, msg.sender, _allowed[account][msg.sender]);
_approve(account, msg.sender, _allowed[account][msg.sender].sub(value));
}
}
14 changes: 8 additions & 6 deletions contracts/token/ERC721/ERC721.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import "./IERC721.sol";
import "./IERC721Receiver.sol";
import "../../math/SafeMath.sol";
import "../../utils/Address.sol";
import "../../drafts/Counters.sol";
import "../../introspection/ERC165.sol";

/**
Expand All @@ -13,6 +14,7 @@ import "../../introspection/ERC165.sol";
contract ERC721 is ERC165, IERC721 {
using SafeMath for uint256;
using Address for address;
using Counters for Counters.Counter;

// Equals to `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`
// which can be also obtained as `IERC721Receiver(0).onERC721Received.selector`
Expand All @@ -25,7 +27,7 @@ contract ERC721 is ERC165, IERC721 {
mapping (uint256 => address) private _tokenApprovals;

// Mapping from owner to number of owned token
mapping (address => uint256) private _ownedTokensCount;
mapping (address => Counters.Counter) private _ownedTokensCount;

// Mapping from owner to operator approvals
mapping (address => mapping (address => bool)) private _operatorApprovals;
Expand Down Expand Up @@ -56,7 +58,7 @@ contract ERC721 is ERC165, IERC721 {
*/
function balanceOf(address owner) public view returns (uint256) {
require(owner != address(0));
return _ownedTokensCount[owner];
return _ownedTokensCount[owner].current();
}

/**
Expand Down Expand Up @@ -200,7 +202,7 @@ contract ERC721 is ERC165, IERC721 {
require(!_exists(tokenId));

_tokenOwner[tokenId] = to;
_ownedTokensCount[to] = _ownedTokensCount[to].add(1);
_ownedTokensCount[to].increment();

emit Transfer(address(0), to, tokenId);
}
Expand All @@ -217,7 +219,7 @@ contract ERC721 is ERC165, IERC721 {

_clearApproval(tokenId);

_ownedTokensCount[owner] = _ownedTokensCount[owner].sub(1);
_ownedTokensCount[owner].decrement();
_tokenOwner[tokenId] = address(0);

emit Transfer(owner, address(0), tokenId);
Expand Down Expand Up @@ -245,8 +247,8 @@ contract ERC721 is ERC165, IERC721 {

_clearApproval(tokenId);

_ownedTokensCount[from] = _ownedTokensCount[from].sub(1);
_ownedTokensCount[to] = _ownedTokensCount[to].add(1);
_ownedTokensCount[from].decrement();
_ownedTokensCount[to].increment();

_tokenOwner[tokenId] = to;

Expand Down
71 changes: 46 additions & 25 deletions test/drafts/Counters.test.js
Original file line number Diff line number Diff line change
@@ -1,37 +1,58 @@
const { BN } = require('openzeppelin-test-helpers');
const { shouldFail } = require('openzeppelin-test-helpers');

const CountersImpl = artifacts.require('CountersImpl');

const EXPECTED = [new BN(1), new BN(2), new BN(3), new BN(4)];
const KEY1 = web3.utils.sha3('key1');
const KEY2 = web3.utils.sha3('key2');

contract('Counters', function ([_, owner]) {
contract('Counters', function () {
beforeEach(async function () {
this.mock = await CountersImpl.new({ from: owner });
this.counter = await CountersImpl.new();
});

it('starts at zero', async function () {
(await this.counter.current()).should.be.bignumber.equal('0');
});

context('custom key', async function () {
it('should return expected values', async function () {
for (const expectedId of EXPECTED) {
await this.mock.doThing(KEY1, { from: owner });
const actualId = await this.mock.theId();
actualId.should.be.bignumber.equal(expectedId);
}
describe('increment', function () {
it('increments the current value by one', async function () {
await this.counter.increment();
(await this.counter.current()).should.be.bignumber.equal('1');
});

it('can be called multiple times', async function () {
await this.counter.increment();
await this.counter.increment();
await this.counter.increment();

(await this.counter.current()).should.be.bignumber.equal('3');
});
});

context('parallel keys', async function () {
it('should return expected values for each counter', async function () {
for (const expectedId of EXPECTED) {
await this.mock.doThing(KEY1, { from: owner });
let actualId = await this.mock.theId();
actualId.should.be.bignumber.equal(expectedId);

await this.mock.doThing(KEY2, { from: owner });
actualId = await this.mock.theId();
actualId.should.be.bignumber.equal(expectedId);
}
describe('decrement', function () {
beforeEach(async function () {
await this.counter.increment();
(await this.counter.current()).should.be.bignumber.equal('1');
});

it('decrements the current value by one', async function () {
await this.counter.decrement();
(await this.counter.current()).should.be.bignumber.equal('0');
});

it('reverts if the current value is 0', async function () {
await this.counter.decrement();
await shouldFail.reverting(this.counter.decrement());
});

it('can be called multiple times', async function () {
await this.counter.increment();
await this.counter.increment();

(await this.counter.current()).should.be.bignumber.equal('3');

await this.counter.decrement();
await this.counter.decrement();
await this.counter.decrement();

(await this.counter.current()).should.be.bignumber.equal('0');
});
});
});
Loading

0 comments on commit da36463

Please sign in to comment.