diff --git a/CHANGELOG.md b/CHANGELOG.md index e520d84fa4f..5c8b4b90384 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,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)) * `ERC165Checker`: functions no longer have a leading underscore. ([#2150](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2150)) ## 2.5.0 (2020-02-04) 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 f9be2df63cd..f33f2bb098c 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); @@ -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 08cc2f1ea77..dbe54458be1 100644 --- a/contracts/utils/EnumerableSet.sol +++ b/contracts/utils/EnumerableSet.sol @@ -20,14 +20,15 @@ pragma solidity ^0.6.0; library EnumerableSet { struct AddressSet { + 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) index; - address[] values; + mapping (address => uint256) _indexes; } /** * @dev Add a value to a set. O(1). + * * Returns false if the value was already in the set. */ function add(AddressSet storage set, address value) @@ -35,10 +36,10 @@ library EnumerableSet { returns (bool) { if (!contains(set, value)) { - set.values.push(value); - // The element is stored at length-1, but we add 1 to all indexes + 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.index[value] = set.values.length; + set._indexes[value] = set._values.length; return true; } else { return false; @@ -47,6 +48,7 @@ library EnumerableSet { /** * @dev Removes a value from a set. O(1). + * * Returns false if the value was not present in the set. */ function remove(AddressSet storage set, address value) @@ -54,24 +56,24 @@ library EnumerableSet { returns (bool) { if (contains(set, value)){ - uint256 toDeleteIndex = set.index[value] - 1; - uint256 lastIndex = set.values.length - 1; + uint256 toDeleteIndex = set._indexes[value] - 1; + uint256 lastIndex = set._values.length - 1; - // If the element 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 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.index[lastValue] = toDeleteIndex + 1; // All indexes are 1-based + set._indexes[lastvalue] = 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 value was stored + set._values.pop(); - // Delete the old entry for the moved value - set.values.pop(); + // Delete the index for the deleted slot + delete set._indexes[value]; return true; } else { @@ -87,41 +89,44 @@ library EnumerableSet { view returns (bool) { - return set.index[value] != 0; + return set._indexes[value] != 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. * 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 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; } /** - * @dev Returns the number of elements 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.values.length; + return set._values.length; } - /** @dev Returns the element 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 values inside the * array, and it may change when more values are added or removed. * @@ -129,11 +134,12 @@ 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) { - return set.values[index]; + 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..9d309c661c6 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'); @@ -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); } @@ -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.at(0), 'EnumerableSet: index out of bounds'); + }); + it('removes added values', async function () { await this.set.add(accountA);