From c6c8b1b8ca20fda75bf596e6564497b14b009e80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Thu, 26 Mar 2020 19:03:33 -0300 Subject: [PATCH 1/6] Add revert reason to EnumerableSet.get. --- contracts/utils/EnumerableSet.sol | 4 +++- test/utils/EnumerableSet.test.js | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/contracts/utils/EnumerableSet.sol b/contracts/utils/EnumerableSet.sol index 08cc2f1ea77..bfe51de9f9e 100644 --- a/contracts/utils/EnumerableSet.sol +++ b/contracts/utils/EnumerableSet.sol @@ -121,7 +121,8 @@ library EnumerableSet { return set.values.length; } - /** @dev Returns the element stored at position `index` in the set. O(1). + /** + * @dev Returns the element stored at position `index` in the set. O(1). * Note that there are no guarantees on the ordering of values inside the * array, and it may change when more values are added or removed. * @@ -134,6 +135,7 @@ library EnumerableSet { view returns (address) { + require(set.values.length > index, "EnumerableSet: index out of bounds"); return set.values[index]; } } diff --git a/test/utils/EnumerableSet.test.js b/test/utils/EnumerableSet.test.js index 137c8051820..6abfbec477f 100644 --- a/test/utils/EnumerableSet.test.js +++ b/test/utils/EnumerableSet.test.js @@ -1,5 +1,5 @@ const { accounts, contract } = require('@openzeppelin/test-environment'); -const { expectEvent } = require('@openzeppelin/test-helpers'); +const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const EnumerableSetMock = contract.fromArtifact('EnumerableSetMock'); @@ -55,6 +55,10 @@ describe('EnumerableSet', function () { await expectMembersMatch(this.set, [accountA]); }); + it('reverts when retrieving non-existent elements', async function () { + await expectRevert(this.set.get(0), 'EnumerableSet: index out of bounds'); + }); + it('removes added values', async function () { await this.set.add(accountA); From d27f777f6d1d7db28bb3c15c5bf2db108255e3ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 27 Mar 2020 11:59:13 -0300 Subject: [PATCH 2/6] Rename EnumerableSet values to keys --- contracts/utils/EnumerableSet.sol | 92 ++++++++++++++++--------------- 1 file changed, 48 insertions(+), 44 deletions(-) diff --git a/contracts/utils/EnumerableSet.sol b/contracts/utils/EnumerableSet.sol index bfe51de9f9e..12c151bed69 100644 --- a/contracts/utils/EnumerableSet.sol +++ b/contracts/utils/EnumerableSet.sol @@ -20,25 +20,26 @@ pragma solidity ^0.6.0; library EnumerableSet { struct AddressSet { - // Position of the value in the `values` array, plus 1 because index 0 - // means a value is not in the set. - mapping (address => uint256) index; - address[] values; + address[] keys; + // Position of the key in the `keys` array, plus 1 because index 0 + // means a key is not in the set. + mapping (address => uint256) indexes; } /** - * @dev Add a value to a set. O(1). - * Returns false if the value was already in the set. + * @dev Add a key to a set. O(1). + * + * Returns false if the key was already in the set. */ - function add(AddressSet storage set, address value) + function add(AddressSet storage set, address key) internal returns (bool) { - if (!contains(set, value)) { - set.values.push(value); - // The element is stored at length-1, but we add 1 to all indexes + if (!contains(set, key)) { + set.keys.push(key); + // The key is stored at length-1, but we add 1 to all indexes // and use 0 as a sentinel value - set.index[value] = set.values.length; + set.indexes[key] = set.keys.length; return true; } else { return false; @@ -46,32 +47,33 @@ library EnumerableSet { } /** - * @dev Removes a value from a set. O(1). - * Returns false if the value was not present in the set. + * @dev Removes a key from a set. O(1). + * + * Returns false if the key was not present in the set. */ - function remove(AddressSet storage set, address value) + function remove(AddressSet storage set, address key) internal returns (bool) { - if (contains(set, value)){ - uint256 toDeleteIndex = set.index[value] - 1; - uint256 lastIndex = set.values.length - 1; + if (contains(set, key)){ + uint256 toDeleteIndex = set.indexes[key] - 1; + uint256 lastIndex = set.keys.length - 1; - // If the element we're deleting is the last one, we can just remove it without doing a swap + // If the key we're deleting is the last one, we can just remove it without doing a swap if (lastIndex != toDeleteIndex) { - address lastValue = set.values[lastIndex]; + address lastKey = set.keys[lastIndex]; - // Move the last value to the index where the deleted value is - set.values[toDeleteIndex] = lastValue; - // Update the index for the moved value - set.index[lastValue] = toDeleteIndex + 1; // All indexes are 1-based + // Move the last key to the index where the deleted key is + set.keys[toDeleteIndex] = lastKey; + // Update the index for the moved key + set.indexes[lastKey] = toDeleteIndex + 1; // All indexes are 1-based } - // Delete the index entry for the deleted value - delete set.index[value]; + // Delete the slot where the moved key was stored + set.keys.pop(); - // Delete the old entry for the moved value - set.values.pop(); + // Delete the index for the deleted slot + delete set.indexes[key]; return true; } else { @@ -80,20 +82,21 @@ library EnumerableSet { } /** - * @dev Returns true if the value is in the set. O(1). + * @dev Returns true if the key is in the set. O(1). */ - function contains(AddressSet storage set, address value) + function contains(AddressSet storage set, address key) internal view returns (bool) { - return set.index[value] != 0; + return set.indexes[key] != 0; } /** - * @dev Returns an array with all values in the set. O(N). - * Note that there are no guarantees on the ordering of values inside the - * array, and it may change when more values are added or removed. + * @dev Returns an array with all keys in the set. O(N). + * + * Note that there are no guarantees on the ordering of keys inside the + * array, and it may change when more keys are added or removed. * WARNING: This function may run out of gas on large sets: use {length} and * {get} instead in these cases. @@ -103,28 +106,29 @@ library EnumerableSet { view returns (address[] memory) { - address[] memory output = new address[](set.values.length); - for (uint256 i; i < set.values.length; i++){ - output[i] = set.values[i]; + address[] memory output = new address[](set.keys.length); + for (uint256 i; i < set.keys.length; i++){ + output[i] = set.keys[i]; } return output; } /** - * @dev Returns the number of elements on the set. O(1). + * @dev Returns the number of keys on the set. O(1). */ function length(AddressSet storage set) internal view returns (uint256) { - return set.values.length; + return set.keys.length; } - /** - * @dev Returns the element stored at position `index` in the set. O(1). - * Note that there are no guarantees on the ordering of values inside the - * array, and it may change when more values are added or removed. + /** + * @dev Returns the key stored at position `index` in the set. O(1). + * + * Note that there are no guarantees on the ordering of keys inside the + * array, and it may change when more keys are added or removed. * * Requirements: * @@ -135,7 +139,7 @@ library EnumerableSet { view returns (address) { - require(set.values.length > index, "EnumerableSet: index out of bounds"); - return set.values[index]; + require(set.keys.length > index, "EnumerableSet: index out of bounds"); + return set.keys[index]; } } From aacb082c0f61cbee3c429de1be6755960687c92e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 27 Mar 2020 12:55:01 -0300 Subject: [PATCH 3/6] Rename get to at --- contracts/access/AccessControl.sol | 2 +- contracts/mocks/EnumerableSetMock.sol | 4 ++-- contracts/utils/EnumerableSet.sol | 4 ++-- test/utils/EnumerableSet.test.js | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 03095d45105..8f82c1bddbc 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -96,7 +96,7 @@ abstract contract AccessControl is Context { * for more information. */ function getRoleMember(bytes32 role, uint256 index) public view returns (address) { - return _roles[role].members.get(index); + return _roles[role].members.at(index); } /** diff --git a/contracts/mocks/EnumerableSetMock.sol b/contracts/mocks/EnumerableSetMock.sol index 7eeb49513f8..2868b67fcf2 100644 --- a/contracts/mocks/EnumerableSetMock.sol +++ b/contracts/mocks/EnumerableSetMock.sol @@ -31,7 +31,7 @@ contract EnumerableSetMock{ return set.length(); } - function get(uint256 index) public view returns (address) { - return set.get(index); + function at(uint256 index) public view returns (address) { + return set.at(index); } } diff --git a/contracts/utils/EnumerableSet.sol b/contracts/utils/EnumerableSet.sol index 12c151bed69..31971d00c08 100644 --- a/contracts/utils/EnumerableSet.sol +++ b/contracts/utils/EnumerableSet.sol @@ -99,7 +99,7 @@ library EnumerableSet { * array, and it may change when more keys are added or removed. * WARNING: This function may run out of gas on large sets: use {length} and - * {get} instead in these cases. + * {at} instead in these cases. */ function enumerate(AddressSet storage set) internal @@ -134,7 +134,7 @@ library EnumerableSet { * * - `index` must be strictly less than {length}. */ - function get(AddressSet storage set, uint256 index) + function at(AddressSet storage set, uint256 index) internal view returns (address) diff --git a/test/utils/EnumerableSet.test.js b/test/utils/EnumerableSet.test.js index 6abfbec477f..9d309c661c6 100644 --- a/test/utils/EnumerableSet.test.js +++ b/test/utils/EnumerableSet.test.js @@ -21,7 +21,7 @@ describe('EnumerableSet', function () { expect(await set.length()).to.bignumber.equal(members.length.toString()); expect(await Promise.all([...Array(members.length).keys()].map(index => - set.get(index) + set.at(index) ))).to.have.same.members(members); } @@ -56,7 +56,7 @@ describe('EnumerableSet', function () { }); it('reverts when retrieving non-existent elements', async function () { - await expectRevert(this.set.get(0), 'EnumerableSet: index out of bounds'); + await expectRevert(this.set.at(0), 'EnumerableSet: index out of bounds'); }); it('removes added values', async function () { From 182e9d316d486ecc162bf2eefe724ae7c64ebb67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 27 Mar 2020 12:55:25 -0300 Subject: [PATCH 4/6] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d94c23c552..6326411485f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ * `Address`: removed `toPayable`, use `payable(address)` instead. ([#2133](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2133)) * `ERC777`: `_send`, `_mint` and `_burn` now use the caller as the operator. ([#2134](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2134)) * `ERC777`: removed `_callsTokensToSend` and `_callTokensReceived`. ([#2134](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2134)) + * `EnumerableSet`: renamed `get` to `at`. ([#2151](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2151)) ## 2.5.0 (2020-02-04) From 1ef8ba700cf50a38197dd62e9b898b872ac14dca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 27 Mar 2020 17:40:52 -0300 Subject: [PATCH 5/6] Rename keys to values --- contracts/mocks/EnumerableSetMock.sol | 2 +- contracts/utils/EnumerableSet.sol | 82 +++++++++++++-------------- 2 files changed, 42 insertions(+), 42 deletions(-) diff --git a/contracts/mocks/EnumerableSetMock.sol b/contracts/mocks/EnumerableSetMock.sol index 2868b67fcf2..39ca7b7a3c4 100644 --- a/contracts/mocks/EnumerableSetMock.sol +++ b/contracts/mocks/EnumerableSetMock.sol @@ -2,7 +2,7 @@ pragma solidity ^0.6.0; import "../utils/EnumerableSet.sol"; -contract EnumerableSetMock{ +contract EnumerableSetMock { using EnumerableSet for EnumerableSet.AddressSet; event TransactionResult(bool result); diff --git a/contracts/utils/EnumerableSet.sol b/contracts/utils/EnumerableSet.sol index 31971d00c08..a152114751c 100644 --- a/contracts/utils/EnumerableSet.sol +++ b/contracts/utils/EnumerableSet.sol @@ -20,26 +20,26 @@ pragma solidity ^0.6.0; library EnumerableSet { struct AddressSet { - address[] keys; - // Position of the key in the `keys` array, plus 1 because index 0 - // means a key is not in the set. + address[] values; + // Position of the value in the `values` array, plus 1 because index 0 + // means a value is not in the set. mapping (address => uint256) indexes; } /** - * @dev Add a key to a set. O(1). + * @dev Add a value to a set. O(1). * - * Returns false if the key was already in the set. + * Returns false if the value was already in the set. */ - function add(AddressSet storage set, address key) + function add(AddressSet storage set, address value) internal returns (bool) { - if (!contains(set, key)) { - set.keys.push(key); - // The key is stored at length-1, but we add 1 to all indexes + if (!contains(set, value)) { + set.values.push(value); + // The value is stored at length-1, but we add 1 to all indexes // and use 0 as a sentinel value - set.indexes[key] = set.keys.length; + set.indexes[value] = set.values.length; return true; } else { return false; @@ -47,33 +47,33 @@ library EnumerableSet { } /** - * @dev Removes a key from a set. O(1). + * @dev Removes a value from a set. O(1). * - * Returns false if the key was not present in the set. + * Returns false if the value was not present in the set. */ - function remove(AddressSet storage set, address key) + function remove(AddressSet storage set, address value) internal returns (bool) { - if (contains(set, key)){ - uint256 toDeleteIndex = set.indexes[key] - 1; - uint256 lastIndex = set.keys.length - 1; + if (contains(set, value)){ + uint256 toDeleteIndex = set.indexes[value] - 1; + uint256 lastIndex = set.values.length - 1; - // If the key we're deleting is the last one, we can just remove it without doing a swap + // If the value we're deleting is the last one, we can just remove it without doing a swap if (lastIndex != toDeleteIndex) { - address lastKey = set.keys[lastIndex]; + address lastvalue = set.values[lastIndex]; - // Move the last key to the index where the deleted key is - set.keys[toDeleteIndex] = lastKey; - // Update the index for the moved key - set.indexes[lastKey] = toDeleteIndex + 1; // All indexes are 1-based + // Move the last value to the index where the deleted value is + set.values[toDeleteIndex] = lastvalue; + // Update the index for the moved value + set.indexes[lastvalue] = toDeleteIndex + 1; // All indexes are 1-based } - // Delete the slot where the moved key was stored - set.keys.pop(); + // Delete the slot where the moved value was stored + set.values.pop(); // Delete the index for the deleted slot - delete set.indexes[key]; + delete set.indexes[value]; return true; } else { @@ -82,21 +82,21 @@ library EnumerableSet { } /** - * @dev Returns true if the key is in the set. O(1). + * @dev Returns true if the value is in the set. O(1). */ - function contains(AddressSet storage set, address key) + function contains(AddressSet storage set, address value) internal view returns (bool) { - return set.indexes[key] != 0; + return set.indexes[value] != 0; } /** - * @dev Returns an array with all keys in the set. O(N). + * @dev Returns an array with all values in the set. O(N). * - * Note that there are no guarantees on the ordering of keys inside the - * array, and it may change when more keys are added or removed. + * Note that there are no guarantees on the ordering of values inside the + * array, and it may change when more values are added or removed. * WARNING: This function may run out of gas on large sets: use {length} and * {at} instead in these cases. @@ -106,29 +106,29 @@ library EnumerableSet { view returns (address[] memory) { - address[] memory output = new address[](set.keys.length); - for (uint256 i; i < set.keys.length; i++){ - output[i] = set.keys[i]; + address[] memory output = new address[](set.values.length); + for (uint256 i; i < set.values.length; i++){ + output[i] = set.values[i]; } return output; } /** - * @dev Returns the number of keys on the set. O(1). + * @dev Returns the number of values on the set. O(1). */ function length(AddressSet storage set) internal view returns (uint256) { - return set.keys.length; + return set.values.length; } /** - * @dev Returns the key stored at position `index` in the set. O(1). + * @dev Returns the value stored at position `index` in the set. O(1). * - * Note that there are no guarantees on the ordering of keys inside the - * array, and it may change when more keys are added or removed. + * Note that there are no guarantees on the ordering of values inside the + * array, and it may change when more values are added or removed. * * Requirements: * @@ -139,7 +139,7 @@ library EnumerableSet { view returns (address) { - require(set.keys.length > index, "EnumerableSet: index out of bounds"); - return set.keys[index]; + require(set.values.length > index, "EnumerableSet: index out of bounds"); + return set.values[index]; } } From c7c5e3b1e57b03a5fe769b5da06d364229c30d4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Fri, 27 Mar 2020 18:06:49 -0300 Subject: [PATCH 6/6] Add leading underscore to struct members --- contracts/utils/EnumerableSet.sol | 36 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/contracts/utils/EnumerableSet.sol b/contracts/utils/EnumerableSet.sol index a152114751c..dbe54458be1 100644 --- a/contracts/utils/EnumerableSet.sol +++ b/contracts/utils/EnumerableSet.sol @@ -20,10 +20,10 @@ pragma solidity ^0.6.0; library EnumerableSet { struct AddressSet { - address[] values; + address[] _values; // Position of the value in the `values` array, plus 1 because index 0 // means a value is not in the set. - mapping (address => uint256) indexes; + mapping (address => uint256) _indexes; } /** @@ -36,10 +36,10 @@ library EnumerableSet { returns (bool) { if (!contains(set, value)) { - set.values.push(value); + set._values.push(value); // The value is stored at length-1, but we add 1 to all indexes // and use 0 as a sentinel value - set.indexes[value] = set.values.length; + set._indexes[value] = set._values.length; return true; } else { return false; @@ -56,24 +56,24 @@ library EnumerableSet { returns (bool) { if (contains(set, value)){ - uint256 toDeleteIndex = set.indexes[value] - 1; - uint256 lastIndex = set.values.length - 1; + uint256 toDeleteIndex = set._indexes[value] - 1; + uint256 lastIndex = set._values.length - 1; // If the value we're deleting is the last one, we can just remove it without doing a swap if (lastIndex != toDeleteIndex) { - address lastvalue = set.values[lastIndex]; + address lastvalue = set._values[lastIndex]; // Move the last value to the index where the deleted value is - set.values[toDeleteIndex] = lastvalue; + set._values[toDeleteIndex] = lastvalue; // Update the index for the moved value - set.indexes[lastvalue] = toDeleteIndex + 1; // All indexes are 1-based + set._indexes[lastvalue] = toDeleteIndex + 1; // All indexes are 1-based } // Delete the slot where the moved value was stored - set.values.pop(); + set._values.pop(); // Delete the index for the deleted slot - delete set.indexes[value]; + delete set._indexes[value]; return true; } else { @@ -89,7 +89,7 @@ library EnumerableSet { view returns (bool) { - return set.indexes[value] != 0; + return set._indexes[value] != 0; } /** @@ -106,9 +106,9 @@ library EnumerableSet { view returns (address[] memory) { - address[] memory output = new address[](set.values.length); - for (uint256 i; i < set.values.length; i++){ - output[i] = set.values[i]; + address[] memory output = new address[](set._values.length); + for (uint256 i; i < set._values.length; i++){ + output[i] = set._values[i]; } return output; } @@ -121,7 +121,7 @@ library EnumerableSet { view returns (uint256) { - return set.values.length; + return set._values.length; } /** @@ -139,7 +139,7 @@ library EnumerableSet { view returns (address) { - require(set.values.length > index, "EnumerableSet: index out of bounds"); - return set.values[index]; + require(set._values.length > index, "EnumerableSet: index out of bounds"); + return set._values[index]; } }