Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add values() functions to EnumerableSets #2768

Merged
merged 7 commits into from
Jul 16, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
12 changes: 12 additions & 0 deletions contracts/mocks/EnumerableSetMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
}
35 changes: 35 additions & 0 deletions contracts/utils/structs/EnumerableSet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that returning a memory array implies reading the entire thing from storage and copying it - not sure if that was the original intent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't this be implemented for Set as Set._values(), and call that here?

Copy link
Collaborator Author

@Amxx Amxx Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a storage pointer will potentially encourage the users to write/modify it, which would break the structure. We really want to avoid this.


// AddressSet

struct AddressSet {
Expand Down Expand Up @@ -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;
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}

// UintSet

struct UintSet {
Expand Down Expand Up @@ -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;
}
}
25 changes: 17 additions & 8 deletions test/utils/structs/EnumerableSet.behavior.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,27 @@ 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(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()),
));
}

it('starts empty', async function () {
Expand Down