Skip to content

Add non-value types in EnumerableSet and EnumerableMap #5658

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

Merged

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented May 2, 2025

Check #5663 before

PR Checklist

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

Copy link

changeset-bot bot commented May 2, 2025

🦋 Changeset detected

Latest commit: 326c466

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

@ernestognw ernestognw force-pushed the feature/enumerable-extended branch from f1ae7d6 to 80edba8 Compare May 3, 2025 07:21
@ernestognw ernestognw marked this pull request as ready for review May 3, 2025 07:21
@ernestognw ernestognw force-pushed the feature/enumerable-extended branch from 29c48d9 to 6508e9b Compare May 3, 2025 16:27
@ernestognw ernestognw force-pushed the feature/enumerable-extended branch from ba06447 to e412bd9 Compare May 3, 2025 16:31
@ernestognw ernestognw mentioned this pull request May 3, 2025
3 tasks
@Amxx
Copy link
Collaborator

Amxx commented Jun 2, 2025

The reason for the name ...Extended was to avoid conflicts if importing botht he vanila version, and the community extension. We would not want both library to have the same name.

But since we are moving the code from community to here, we should put all that in the existing library and not create new ones.

@Amxx
Copy link
Collaborator

Amxx commented Jun 2, 2025

Note: this is actually very close to #5558 that was closed when we decided to put these structs in the community repo.

@Amxx Amxx changed the title Add EnumerableSetExtended and EnumerableMapExtended Add non-value types in EnumerableSet and EnumerableMap (migration from community repo) Jun 2, 2025
.flatMap((keyType, _, array) => array.map(valueType => ({ key: { type: keyType }, value: { type: valueType } })))
.slice(0, -1), // remove bytes32 → bytes32 (last one) that is already defined
// non-value type maps
{ key: { type: 'bytes', memory: true }, value: { type: 'uint256' } },
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to keep this one ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, let's remove it and just keep bytes to bytes for now given is the most generic version.

We could theoretically do all variants for ['uint256', 'address', 'bytes32', 'bytes', 'string'], but I think it needs more thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, as you mentioned in #5558, Bytes -> Uint would be useful for weights:

BytesToUintMap (usefull for something like an ERC-7093 contract for guardian->weight)

However, weights are just tracked with a mapping in the current SignerMultiERC7913Weighted and equivalent ERC7579MultisigWeighted module.

// Mapping from signer to weight
mapping(bytes signer => uint256) private _weights;

Amxx
Amxx previously approved these changes Jun 2, 2025
@ernestognw ernestognw changed the title Add non-value types in EnumerableSet and EnumerableMap (migration from community repo) Add non-value types in EnumerableSet and EnumerableMap Jun 2, 2025
@ernestognw ernestognw enabled auto-merge (squash) June 2, 2025 20:42
@ernestognw ernestognw requested a review from Amxx June 2, 2025 20:42
@ernestognw ernestognw merged commit 784d4f7 into OpenZeppelin:master Jun 3, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants