Skip to content

Add bytes32 to bytes32 enumerable map#3192

Merged
Amxx merged 3 commits into
OpenZeppelin:masterfrom
wiasliaw:wiasliaw/bytes32tobytes32Map
Mar 22, 2022
Merged

Add bytes32 to bytes32 enumerable map#3192
Amxx merged 3 commits into
OpenZeppelin:masterfrom
wiasliaw:wiasliaw/bytes32tobytes32Map

Conversation

@wiasliaw

@wiasliaw wiasliaw commented Feb 15, 2022

Copy link
Copy Markdown
Contributor

Add Bytes32ToBytes32Map in EnumerableMap.sol.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx

Amxx commented Feb 16, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

frangio commented Feb 17, 2022

Copy link
Copy Markdown
Contributor

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

wiasliaw commented Feb 18, 2022

Copy link
Copy Markdown
Contributor Author

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

@frangio

frangio commented Feb 18, 2022

Copy link
Copy Markdown
Contributor

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

Amxx commented Feb 18, 2022

Copy link
Copy Markdown
Collaborator

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

@wiasliaw

Copy link
Copy Markdown
Contributor Author

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

@wiasliaw

wiasliaw commented Feb 21, 2022

Copy link
Copy Markdown
Contributor Author

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

@Amxx

Amxx commented Feb 21, 2022

Copy link
Copy Markdown
Collaborator

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

@wiasliaw

Copy link
Copy Markdown
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

@Amxx Amxx left a comment

Copy link
Copy Markdown
Collaborator

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