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

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jul 14, 2021

Address request from #2390

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

Comment on lines 190 to 192
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.

contracts/utils/structs/EnumerableSet.sol Outdated Show resolved Hide resolved
@frangio frangio merged commit f88e555 into OpenZeppelin:master Jul 16, 2021
@Amxx Amxx deleted the feature/enumerableset/values branch July 16, 2021 15:52
@Amxx Amxx mentioned this pull request Aug 5, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants