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 Bytes32x2Set #5442

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add Bytes32x2Set #5442

wants to merge 4 commits into from

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 20, 2025

  • Refactor EnumerableSet.opt.js to support arrays of basetype
    • doesn't affect currently generated code (backward compatible)
  • Use the refactor to implement Bytes32x2Set

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Jan 20, 2025

🦋 Changeset detected

Latest commit: eb050b8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Amxx Amxx requested review from arr00 and ernestognw January 22, 2025 16:15
bytes32[2][] _values;
// Position is the index of the value in the `values` array plus 1.
// Position 0 is used to mean a value is not in the self.
mapping(bytes32 valueHash => uint256) _positions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mapping(bytes32 valueHash => uint256) _positions;
mapping(bytes32 valueHash => uint256) _positionsPlusOne;

Thoughts on this for code clarity?

Copy link
Collaborator Author

@Amxx Amxx Jan 23, 2025

Choose a reason for hiding this comment

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

I looked into the history of this piece of code:

  1. EnumerableSet was introduced in 2.5. In that version, the code only supported addresses, and the structure was defined as:
    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;
    }
  1. In version 3.0, the inner/outer structure was used, with the default being a bytes32 set that is
    struct Set {
        // Storage of set values
        bytes32[] _values;

        // Position of the value in the `values` array, plus 1 because index 0
        // means a value is not in the set.
        mapping (bytes32 => uint256) _indexes;
    }
  1. In version 5.0, the naming was changed to was we have currently (this is the PR). Note that the PR was the result of an audit issue.

  2. This PR is just reusing the wording that we have in the other sets.


IMO:

  • No user should touch that storage (if we could make it private we would)
  • The description is really clear
  • Overall, I tend to favor short names to long ones. IMO _positionsPlusOne is difficult/long to read and offer little benefit.
  • Changing the name in the existing set would be breaking, and using a different name here would be inconsistent.

So I'd personally keep it that way. @ernestognw WDYT ?

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.

2 participants