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 bytes32 to bytes32 enumerable map #3192

Merged
merged 3 commits into from
Mar 22, 2022

Conversation

wiasliaw
Copy link
Contributor

@wiasliaw wiasliaw commented Feb 15, 2022

Add Bytes32ToBytes32Map in EnumerableMap.sol.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Feb 16, 2022

Hello @wiasliaw, and thank you for submitting this PR.

We currently have a request for wider type support in the SafeCast library. Exhaustive type support there calls for procedural generation. I'm wondering if that is something we would like to do here as well. @frangio ?

@wiasliaw
Copy link
Contributor Author

Hi @Amxx. Currently, map supports for uint256=>address and address=>uint256. It doesn't support bytes32=>bytes32 to let user customize. I think that SafeCast should work with casting uint256 or bytes32 to uintX or bytesX.

@frangio
Copy link
Contributor

frangio commented Feb 17, 2022

Thank you @wiasliaw. This type is kind of already present as the Map type though, right? I think we can just make the functions for that type internal instead of private, instead of adding an unnecessary indirection level.


@Amxx As for support for other types, I'm not really sure to be honest. I don't really like that we have all of these variations for different types. I don't trust the compiler to efficiently remove the casts and inline functions. I was in fact considering that we can only have a single type using bytes32 and let users cast to whatever type they need. This is what we did with the Deque type essentially.

@wiasliaw
Copy link
Contributor Author

wiasliaw commented Feb 18, 2022

@frangio I think that expose through a new struct will have more readability.
Comparing between Bytes32ToBytes32Map and Map.

@frangio
Copy link
Contributor

frangio commented Feb 18, 2022

Yeah possibly. In that case I'd rather we renamed the current Map to Bytes32ToBytes32Map, instead of adding an extra indirection layer that will add overhead. @Amxx What do you think?

@Amxx
Copy link
Collaborator

Amxx commented Feb 18, 2022

I'd be ok to do that, but we'll also need to change _set(...) private to set(...) internal

@wiasliaw
Copy link
Contributor Author

I think we got the result of implementation. I will work on this PR.

@wiasliaw
Copy link
Contributor Author

wiasliaw commented Feb 21, 2022

Hi @Amxx, @frangio, renaming into _set() internal will violate the role private-vars-leading-underscore.

@Amxx
Copy link
Collaborator

Amxx commented Feb 21, 2022

I meant set(...) internal, sorry for the typo.

@wiasliaw
Copy link
Contributor Author

I had done with the PR. Plz review. TKS

@frangio frangio requested a review from Amxx February 25, 2022 15:09
@frangio frangio mentioned this pull request Feb 25, 2022
1 task
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you !

@frangio frangio changed the title feat(enumerablemap): add bytes32 to bytes32 map Add bytes32 to bytes32 enumerable map Mar 2, 2022
@Amxx Amxx merged commit b13bdb0 into OpenZeppelin:master Mar 22, 2022
@wiasliaw wiasliaw deleted the wiasliaw/bytes32tobytes32Map branch April 6, 2023 05:55
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