From 90caacef3d96ada309ae640002743c97dbb993af Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jul 2021 14:46:38 +0200 Subject: [PATCH 1/7] add values() functions to enumerable sets --- contracts/mocks/EnumerableSetMock.sol | 12 +++++++ contracts/utils/structs/EnumerableSet.sol | 35 ++++++++++++++++++++ test/utils/structs/EnumerableSet.behavior.js | 19 ++++++----- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/contracts/mocks/EnumerableSetMock.sol b/contracts/mocks/EnumerableSetMock.sol index 26f6f60c21f..922ce46d250 100644 --- a/contracts/mocks/EnumerableSetMock.sol +++ b/contracts/mocks/EnumerableSetMock.sol @@ -33,6 +33,10 @@ contract EnumerableBytes32SetMock { function at(uint256 index) public view returns (bytes32) { return _set.at(index); } + + function values() public view returns (bytes32[] memory) { + return _set.values(); + } } // AddressSet @@ -64,6 +68,10 @@ contract EnumerableAddressSetMock { function at(uint256 index) public view returns (address) { return _set.at(index); } + + function values() public view returns (address[] memory) { + return _set.values(); + } } // UintSet @@ -95,4 +103,8 @@ contract EnumerableUintSetMock { function at(uint256 index) public view returns (uint256) { return _set.at(index); } + + function values() public view returns (uint256[] memory) { + return _set.values(); + } } diff --git a/contracts/utils/structs/EnumerableSet.sol b/contracts/utils/structs/EnumerableSet.sol index e90d55dedaf..2f537d208ef 100644 --- a/contracts/utils/structs/EnumerableSet.sol +++ b/contracts/utils/structs/EnumerableSet.sol @@ -184,6 +184,13 @@ library EnumerableSet { return _at(set._inner, index); } + /** + * @dev Return the entier set in an array + */ + function values(Bytes32Set storage set) internal view returns (bytes32[] memory) { + return set._inner._values; + } + // AddressSet struct AddressSet { @@ -238,6 +245,20 @@ library EnumerableSet { return address(uint160(uint256(_at(set._inner, index)))); } + /** + * @dev Return the entier set in an array + */ + function values(AddressSet storage set) internal view returns (address[] memory) { + bytes32[] storage store = set._inner._values; + address[] storage result; + + assembly { + result.slot := store.slot + } + + return result; + } + // UintSet struct UintSet { @@ -291,4 +312,18 @@ library EnumerableSet { function at(UintSet storage set, uint256 index) internal view returns (uint256) { return uint256(_at(set._inner, index)); } + + /** + * @dev Return the entier set in an array + */ + function values(UintSet storage set) internal view returns (uint256[] memory) { + bytes32[] storage store = set._inner._values; + uint256[] storage result; + + assembly { + result.slot := store.slot + } + + return result; + } } diff --git a/test/utils/structs/EnumerableSet.behavior.js b/test/utils/structs/EnumerableSet.behavior.js index be3f52a5e5a..19221a72dc8 100644 --- a/test/utils/structs/EnumerableSet.behavior.js +++ b/test/utils/structs/EnumerableSet.behavior.js @@ -3,18 +3,21 @@ const { expect } = require('chai'); function shouldBehaveLikeSet (valueA, valueB, valueC) { async function expectMembersMatch (set, values) { - await Promise.all(values.map(async value => - expect(await set.contains(value)).to.equal(true), - )); + await Promise.all(values.map(value => set.contains(value))) + .then(contains => expect(contains.every(Boolean)).to.be.equal(true)); - expect(await set.length()).to.bignumber.equal(values.length.toString()); + await set.length() + .then(length => expect(length).to.bignumber.equal(values.length.toString())); // To compare values we convert to strings to workaround Chai // limitations when dealing with nested arrays (required for BNs) - expect(await Promise.all([...Array(values.length).keys()].map(async (index) => { - const entry = await set.at(index); - return entry.toString(); - }))).to.have.same.members(values.map(v => v.toString())); + await Promise.all(Array(values.length).fill().map((_, index) => set.at(index))) + .then(values => values.map(entry => entry.toString())) + .then(values => expect(values).to.have.same.members(values.map(v => v.toString()))); + + await set.values() + .then(values => values.map(entry => entry.toString())) + .then(values => expect(values).to.have.same.members(values.map(v => v.toString()))); } it('starts empty', async function () { From b7c51c0ab494dbc58453c6a637f60ed56b4b5ea7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jul 2021 16:23:15 +0200 Subject: [PATCH 2/7] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67ae86dcc3b..2104d110ffc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased * `ERC2771Context`: use private variable from storage to store the forwarder address. Fixes issues where `_msgSender()` was not callable from constructors. ([#2754](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2754)) + * `EnumerableSet`: add `values()` functions that returns an array containing all values in a single call. ([#2768](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2768)) ## 4.2.0 (2021-06-30) From aae00276d62071e0694b45461e4c16aaf29afd2f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jul 2021 17:24:31 +0200 Subject: [PATCH 3/7] fix naming conflict in tests --- test/utils/structs/EnumerableSet.behavior.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/utils/structs/EnumerableSet.behavior.js b/test/utils/structs/EnumerableSet.behavior.js index 19221a72dc8..9104a0de88c 100644 --- a/test/utils/structs/EnumerableSet.behavior.js +++ b/test/utils/structs/EnumerableSet.behavior.js @@ -12,12 +12,18 @@ function shouldBehaveLikeSet (valueA, valueB, valueC) { // To compare values we convert to strings to workaround Chai // limitations when dealing with nested arrays (required for BNs) await Promise.all(Array(values.length).fill().map((_, index) => set.at(index))) - .then(values => values.map(entry => entry.toString())) - .then(values => expect(values).to.have.same.members(values.map(v => v.toString()))); + .then(results => expect( + results.map(v => v.toString()), + ).to.have.same.members( + values.map(v => v.toString()), + )); await set.values() - .then(values => values.map(entry => entry.toString())) - .then(values => expect(values).to.have.same.members(values.map(v => v.toString()))); + .then(results => expect( + results.map(v => v.toString()), + ).to.have.same.members( + values.map(v => v.toString()), + )); } it('starts empty', async function () { From c2340bcec479857538c024e957b8751dd8ea9991 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jul 2021 19:18:31 +0200 Subject: [PATCH 4/7] refactor values() code to avoid storage references --- contracts/utils/structs/EnumerableSet.sol | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/contracts/utils/structs/EnumerableSet.sol b/contracts/utils/structs/EnumerableSet.sol index 2f537d208ef..ee4b2f2850a 100644 --- a/contracts/utils/structs/EnumerableSet.sol +++ b/contracts/utils/structs/EnumerableSet.sol @@ -130,6 +130,13 @@ library EnumerableSet { return set._values[index]; } + /** + * @dev Return the entier set in an array + */ + function _values(Set storage set) private view returns (bytes32[] memory) { + return set._values; + } + // Bytes32Set struct Bytes32Set { @@ -188,7 +195,7 @@ library EnumerableSet { * @dev Return the entier set in an array */ function values(Bytes32Set storage set) internal view returns (bytes32[] memory) { - return set._inner._values; + return _values(set._inner); } // AddressSet @@ -249,11 +256,11 @@ library EnumerableSet { * @dev Return the entier set in an array */ function values(AddressSet storage set) internal view returns (address[] memory) { - bytes32[] storage store = set._inner._values; - address[] storage result; + bytes32[] memory store = _values(set._inner); + address[] memory result; assembly { - result.slot := store.slot + result := store } return result; @@ -317,11 +324,11 @@ library EnumerableSet { * @dev Return the entier set in an array */ function values(UintSet storage set) internal view returns (uint256[] memory) { - bytes32[] storage store = set._inner._values; - uint256[] storage result; + bytes32[] memory store = _values(set._inner); + uint256[] memory result; assembly { - result.slot := store.slot + result := store } return result; From 0cdee8abf2911a349774e667059a419440e46083 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 14 Jul 2021 16:07:05 -0300 Subject: [PATCH 5/7] remove use of .then --- test/utils/structs/EnumerableSet.behavior.js | 36 +++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/test/utils/structs/EnumerableSet.behavior.js b/test/utils/structs/EnumerableSet.behavior.js index 9104a0de88c..7a021bd6b96 100644 --- a/test/utils/structs/EnumerableSet.behavior.js +++ b/test/utils/structs/EnumerableSet.behavior.js @@ -3,27 +3,29 @@ const { expect } = require('chai'); function shouldBehaveLikeSet (valueA, valueB, valueC) { async function expectMembersMatch (set, values) { - await Promise.all(values.map(value => set.contains(value))) - .then(contains => expect(contains.every(Boolean)).to.be.equal(true)); + const contains = await Promise.all(values.map(value => set.contains(value))); + expect(contains.every(Boolean)).to.be.equal(true); - await set.length() - .then(length => expect(length).to.bignumber.equal(values.length.toString())); + const length = await set.length(); + expect(length).to.bignumber.equal(values.length.toString()); // To compare values we convert to strings to workaround Chai // limitations when dealing with nested arrays (required for BNs) - await Promise.all(Array(values.length).fill().map((_, index) => set.at(index))) - .then(results => expect( - results.map(v => v.toString()), - ).to.have.same.members( - values.map(v => v.toString()), - )); - - await set.values() - .then(results => expect( - results.map(v => v.toString()), - ).to.have.same.members( - values.map(v => v.toString()), - )); + const indexedValues = await Promise.all(Array(values.length).fill().map( + (_, index) => set.at(index) + )); + expect( + indexedValues.map(v => v.toString()), + ).to.have.same.members( + values.map(v => v.toString()), + ); + + const returnedValues = await set.values(); + expect( + returnedValues.map(v => v.toString()), + ).to.have.same.members( + values.map(v => v.toString()), + ); } it('starts empty', async function () { From ccb9305b2e79f4663969063e1d5832573c1eed2d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jul 2021 21:43:27 +0200 Subject: [PATCH 6/7] fix lint --- test/utils/structs/EnumerableSet.behavior.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/utils/structs/EnumerableSet.behavior.js b/test/utils/structs/EnumerableSet.behavior.js index 7a021bd6b96..17e08667129 100644 --- a/test/utils/structs/EnumerableSet.behavior.js +++ b/test/utils/structs/EnumerableSet.behavior.js @@ -11,9 +11,7 @@ function shouldBehaveLikeSet (valueA, valueB, valueC) { // To compare values we convert to strings to workaround Chai // limitations when dealing with nested arrays (required for BNs) - const indexedValues = await Promise.all(Array(values.length).fill().map( - (_, index) => set.at(index) - )); + const indexedValues = await Promise.all(Array(values.length).fill().map((_, index) => set.at(index))); expect( indexedValues.map(v => v.toString()), ).to.have.same.members( From 90e06619270174b3462f75136c527ca61eae5167 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 15 Jul 2021 15:52:42 +0200 Subject: [PATCH 7/7] improve values() documentation --- contracts/utils/structs/EnumerableSet.sol | 28 +++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/contracts/utils/structs/EnumerableSet.sol b/contracts/utils/structs/EnumerableSet.sol index ee4b2f2850a..e3cf13f12e3 100644 --- a/contracts/utils/structs/EnumerableSet.sol +++ b/contracts/utils/structs/EnumerableSet.sol @@ -131,7 +131,12 @@ library EnumerableSet { } /** - * @dev Return the entier set in an array + * @dev Return the entire set in an array + * + * WARNING: This operation will copy the entire storage to memory, which can be quite expensive. This is designed + * to mostly be used by view accessors that are queried without any gas fees. Developers should keep in mind that + * this function has an unbounded cost, and using it as part of a state-changing function may render the function + * uncallable if the set grows to a point where copying to memory consumes too much gas to fit in a block. */ function _values(Set storage set) private view returns (bytes32[] memory) { return set._values; @@ -192,7 +197,12 @@ library EnumerableSet { } /** - * @dev Return the entier set in an array + * @dev Return the entire set in an array + * + * WARNING: This operation will copy the entire storage to memory, which can be quite expensive. This is designed + * to mostly be used by view accessors that are queried without any gas fees. Developers should keep in mind that + * this function has an unbounded cost, and using it as part of a state-changing function may render the function + * uncallable if the set grows to a point where copying to memory consumes too much gas to fit in a block. */ function values(Bytes32Set storage set) internal view returns (bytes32[] memory) { return _values(set._inner); @@ -253,7 +263,12 @@ library EnumerableSet { } /** - * @dev Return the entier set in an array + * @dev Return the entire set in an array + * + * WARNING: This operation will copy the entire storage to memory, which can be quite expensive. This is designed + * to mostly be used by view accessors that are queried without any gas fees. Developers should keep in mind that + * this function has an unbounded cost, and using it as part of a state-changing function may render the function + * uncallable if the set grows to a point where copying to memory consumes too much gas to fit in a block. */ function values(AddressSet storage set) internal view returns (address[] memory) { bytes32[] memory store = _values(set._inner); @@ -321,7 +336,12 @@ library EnumerableSet { } /** - * @dev Return the entier set in an array + * @dev Return the entire set in an array + * + * WARNING: This operation will copy the entire storage to memory, which can be quite expensive. This is designed + * to mostly be used by view accessors that are queried without any gas fees. Developers should keep in mind that + * this function has an unbounded cost, and using it as part of a state-changing function may render the function + * uncallable if the set grows to a point where copying to memory consumes too much gas to fit in a block. */ function values(UintSet storage set) internal view returns (uint256[] memory) { bytes32[] memory store = _values(set._inner);