From c9547d4808d1f09dd57c57f59f01517c33bb6fa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 16 Jan 2019 16:15:27 -0300 Subject: [PATCH 1/8] Refactor Counter to support increment and decrement. --- contracts/drafts/Counter.sol | 21 +++++++-- contracts/mocks/CounterImpl.sol | 17 ++++--- test/drafts/Counter.test.js | 81 +++++++++++++++++++++++---------- 3 files changed, 84 insertions(+), 35 deletions(-) diff --git a/contracts/drafts/Counter.sol b/contracts/drafts/Counter.sol index af8f1e45d52..ff6f1e51e1f 100644 --- a/contracts/drafts/Counter.sol +++ b/contracts/drafts/Counter.sol @@ -1,5 +1,7 @@ pragma solidity ^0.5.0; +import "../math/SafeMath.sol"; + /** * @title Counter * @author Matt Condon (@shrugs) @@ -13,12 +15,23 @@ pragma solidity ^0.5.0; * so it's not something you have to worry about.) */ library Counter { + using SafeMath for uint256; + struct Counter { - uint256 current; // default: 0 + uint256 _value; // default: 0 + } + + function current(Counter storage counter) internal view returns (uint256) { + return counter._value; + } + + function increment(Counter storage counter) internal returns (uint256) { + counter._value += 1; + return counter._value; } - function next(Counter storage index) internal returns (uint256) { - index.current += 1; - return index.current; + function decrement(Counter storage counter) internal returns (uint256) { + counter._value = counter._value.sub(1); + return counter._value; } } diff --git a/contracts/mocks/CounterImpl.sol b/contracts/mocks/CounterImpl.sol index 4d976f55d0a..1d52351a405 100644 --- a/contracts/mocks/CounterImpl.sol +++ b/contracts/mocks/CounterImpl.sol @@ -5,13 +5,18 @@ import "../drafts/Counter.sol"; contract CounterImpl { using Counter for Counter.Counter; - uint256 public theId; + Counter.Counter private _counter; + event ValueChange(uint256 current); - // use whatever key you want to track your counters - mapping(string => Counter.Counter) private _counters; + function current() public view returns (uint256) { + return _counter.current(); + } + + function increment() public { + emit ValueChange(_counter.increment()); + } - function doThing(string memory key) public returns (uint256) { - theId = _counters[key].next(); - return theId; + function decrement() public { + emit ValueChange(_counter.decrement()); } } diff --git a/test/drafts/Counter.test.js b/test/drafts/Counter.test.js index e486ed1e896..079d8a5d999 100644 --- a/test/drafts/Counter.test.js +++ b/test/drafts/Counter.test.js @@ -1,37 +1,68 @@ -const { BN } = require('openzeppelin-test-helpers'); +const { expectEvent, shouldFail } = require('openzeppelin-test-helpers'); const CounterImpl = artifacts.require('CounterImpl'); -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('Counter', function ([_, owner]) { +contract('Counter', function () { beforeEach(async function () { - this.mock = await CounterImpl.new({ from: owner }); + this.counter = await CounterImpl.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('returns the new value', async function () { + const { logs } = await this.counter.increment(); + expectEvent.inLogs(logs, 'ValueChange', { current: '1' }); + }); + + it('can be called multipl etimes', 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('returns the new value', async function () { + const { logs } = await this.counter.decrement(); + expectEvent.inLogs(logs, 'ValueChange', { current: '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'); }); }); }); From 23023754bcc1e617834f017d033ad90fec11f480 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 16 Jan 2019 16:17:08 -0300 Subject: [PATCH 2/8] Move Counter out of drafts. --- CHANGELOG.md | 3 +++ contracts/mocks/CounterImpl.sol | 2 +- contracts/{drafts => utils}/Counter.sol | 0 3 files changed, 4 insertions(+), 1 deletion(-) rename contracts/{drafts => utils}/Counter.sol (100%) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5dfeee4825..2fc4fb39e9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## 2.2.0 (unreleased) +### New features: + * `Counter`'s API has been stabilized, and therefore moved out of the `drafts` directory into `utils`. + ## 2.1.1 (2019-04-01) * Version bump to avoid conflict in the npm registry. diff --git a/contracts/mocks/CounterImpl.sol b/contracts/mocks/CounterImpl.sol index 1d52351a405..26dc5ee116f 100644 --- a/contracts/mocks/CounterImpl.sol +++ b/contracts/mocks/CounterImpl.sol @@ -1,6 +1,6 @@ pragma solidity ^0.5.0; -import "../drafts/Counter.sol"; +import "../utils/Counter.sol"; contract CounterImpl { using Counter for Counter.Counter; diff --git a/contracts/drafts/Counter.sol b/contracts/utils/Counter.sol similarity index 100% rename from contracts/drafts/Counter.sol rename to contracts/utils/Counter.sol From d3cc67260466fcc451541e10f7af383dcd2345d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 16 Jan 2019 16:28:25 -0300 Subject: [PATCH 3/8] Refactor ERC721 to use Counter. --- CHANGELOG.md | 3 +++ contracts/token/ERC721/ERC721.sol | 14 ++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2fc4fb39e9a..4b89fb45469 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ ### New features: * `Counter`'s API has been stabilized, and therefore moved out of the `drafts` directory into `utils`. +### Improvements:: + * `ERC721`'s transfers are now more gas efficient due to removal of unnecessary `SafeMath` calls. + ## 2.1.1 (2019-04-01) * Version bump to avoid conflict in the npm registry. diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 7c51ce60aa3..d0917c3d7a3 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -4,6 +4,7 @@ import "./IERC721.sol"; import "./IERC721Receiver.sol"; import "../../math/SafeMath.sol"; import "../../utils/Address.sol"; +import "../../utils/Counter.sol"; import "../../introspection/ERC165.sol"; /** @@ -13,6 +14,7 @@ import "../../introspection/ERC165.sol"; contract ERC721 is ERC165, IERC721 { using SafeMath for uint256; using Address for address; + using Counter for Counter.Counter; // Equals to `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))` // which can be also obtained as `IERC721Receiver(0).onERC721Received.selector` @@ -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 => Counter.Counter) private _ownedTokensCount; // Mapping from owner to operator approvals mapping (address => mapping (address => bool)) private _operatorApprovals; @@ -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(); } /** @@ -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); } @@ -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); @@ -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; From 25fd4b3c7fe05d85cc306935b5a0d201dd6db67c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 16 Jan 2019 16:49:15 -0300 Subject: [PATCH 4/8] Rollback Counter returning the current value in increment and decrement. --- contracts/mocks/CounterImpl.sol | 5 ++--- contracts/utils/Counter.sol | 6 ++---- test/drafts/Counter.test.js | 12 +----------- 3 files changed, 5 insertions(+), 18 deletions(-) diff --git a/contracts/mocks/CounterImpl.sol b/contracts/mocks/CounterImpl.sol index 26dc5ee116f..37aeb5c474e 100644 --- a/contracts/mocks/CounterImpl.sol +++ b/contracts/mocks/CounterImpl.sol @@ -6,17 +6,16 @@ contract CounterImpl { using Counter for Counter.Counter; Counter.Counter private _counter; - event ValueChange(uint256 current); function current() public view returns (uint256) { return _counter.current(); } function increment() public { - emit ValueChange(_counter.increment()); + _counter.increment(); } function decrement() public { - emit ValueChange(_counter.decrement()); + _counter.decrement(); } } diff --git a/contracts/utils/Counter.sol b/contracts/utils/Counter.sol index ff6f1e51e1f..dce607d1ea3 100644 --- a/contracts/utils/Counter.sol +++ b/contracts/utils/Counter.sol @@ -25,13 +25,11 @@ library Counter { return counter._value; } - function increment(Counter storage counter) internal returns (uint256) { + function increment(Counter storage counter) internal { counter._value += 1; - return counter._value; } - function decrement(Counter storage counter) internal returns (uint256) { + function decrement(Counter storage counter) internal { counter._value = counter._value.sub(1); - return counter._value; } } diff --git a/test/drafts/Counter.test.js b/test/drafts/Counter.test.js index 079d8a5d999..29bc5aebac4 100644 --- a/test/drafts/Counter.test.js +++ b/test/drafts/Counter.test.js @@ -1,4 +1,4 @@ -const { expectEvent, shouldFail } = require('openzeppelin-test-helpers'); +const { shouldFail } = require('openzeppelin-test-helpers'); const CounterImpl = artifacts.require('CounterImpl'); @@ -17,11 +17,6 @@ contract('Counter', function () { (await this.counter.current()).should.be.bignumber.equal('1'); }); - it('returns the new value', async function () { - const { logs } = await this.counter.increment(); - expectEvent.inLogs(logs, 'ValueChange', { current: '1' }); - }); - it('can be called multipl etimes', async function () { await this.counter.increment(); await this.counter.increment(); @@ -42,11 +37,6 @@ contract('Counter', function () { (await this.counter.current()).should.be.bignumber.equal('0'); }); - it('returns the new value', async function () { - const { logs } = await this.counter.decrement(); - expectEvent.inLogs(logs, 'ValueChange', { current: '0' }); - }); - it('reverts if the current value is 0', async function () { await this.counter.decrement(); await shouldFail.reverting(this.counter.decrement()); From 20de3e06530a8071d26d99bb993a41c7e671ce58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn?= Date: Wed, 16 Jan 2019 17:14:54 -0300 Subject: [PATCH 5/8] Update test/drafts/Counter.test.js Co-Authored-By: nventuro --- test/drafts/Counter.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/drafts/Counter.test.js b/test/drafts/Counter.test.js index 29bc5aebac4..48c8a7a2eae 100644 --- a/test/drafts/Counter.test.js +++ b/test/drafts/Counter.test.js @@ -17,7 +17,7 @@ contract('Counter', function () { (await this.counter.current()).should.be.bignumber.equal('1'); }); - it('can be called multipl etimes', async function () { + it('can be called multiple times', async function () { await this.counter.increment(); await this.counter.increment(); await this.counter.increment(); From 1bcd07c24074705608de9fa89ffdf5e29e05ad7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 16 Jan 2019 18:45:55 -0300 Subject: [PATCH 6/8] Improve Counter documentation. --- contracts/utils/Counter.sol | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/contracts/utils/Counter.sol b/contracts/utils/Counter.sol index dce607d1ea3..9e82147815b 100644 --- a/contracts/utils/Counter.sol +++ b/contracts/utils/Counter.sol @@ -5,19 +5,21 @@ import "../math/SafeMath.sol"; /** * @title Counter * @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 Counter for Counter.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.) + * 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 Counter { using SafeMath for uint256; struct Counter { + // 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 } From 20d73b65ab9ed6e6ccce96a4e005e57f530dc2fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 16 Jan 2019 18:46:13 -0300 Subject: [PATCH 7/8] Move Counter.test to utils. --- test/{drafts => utils}/Counter.test.js | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{drafts => utils}/Counter.test.js (100%) diff --git a/test/drafts/Counter.test.js b/test/utils/Counter.test.js similarity index 100% rename from test/drafts/Counter.test.js rename to test/utils/Counter.test.js From 992cbb4309f0592d8c2973f599338f676c7d447d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 17 Jan 2019 15:57:00 -0300 Subject: [PATCH 8/8] Move back Counter to drafts. --- CHANGELOG.md | 2 +- contracts/{utils => drafts}/Counter.sol | 0 contracts/mocks/CounterImpl.sol | 2 +- contracts/token/ERC721/ERC721.sol | 2 +- test/{utils => drafts}/Counter.test.js | 0 5 files changed, 3 insertions(+), 3 deletions(-) rename contracts/{utils => drafts}/Counter.sol (100%) rename test/{utils => drafts}/Counter.test.js (100%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b89fb45469..8515ed31564 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,9 +3,9 @@ ## 2.2.0 (unreleased) ### New features: - * `Counter`'s API has been stabilized, and therefore moved out of the `drafts` directory into `utils`. ### Improvements:: + * `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. ## 2.1.1 (2019-04-01) diff --git a/contracts/utils/Counter.sol b/contracts/drafts/Counter.sol similarity index 100% rename from contracts/utils/Counter.sol rename to contracts/drafts/Counter.sol diff --git a/contracts/mocks/CounterImpl.sol b/contracts/mocks/CounterImpl.sol index 37aeb5c474e..b1b327dff57 100644 --- a/contracts/mocks/CounterImpl.sol +++ b/contracts/mocks/CounterImpl.sol @@ -1,6 +1,6 @@ pragma solidity ^0.5.0; -import "../utils/Counter.sol"; +import "../drafts/Counter.sol"; contract CounterImpl { using Counter for Counter.Counter; diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index d0917c3d7a3..8ae3c1d7798 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -4,7 +4,7 @@ import "./IERC721.sol"; import "./IERC721Receiver.sol"; import "../../math/SafeMath.sol"; import "../../utils/Address.sol"; -import "../../utils/Counter.sol"; +import "../../drafts/Counter.sol"; import "../../introspection/ERC165.sol"; /** diff --git a/test/utils/Counter.test.js b/test/drafts/Counter.test.js similarity index 100% rename from test/utils/Counter.test.js rename to test/drafts/Counter.test.js