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 Strings.equal #3774

Merged
merged 10 commits into from
Dec 28, 2022
Merged

Add Strings.equal #3774

merged 10 commits into from
Dec 28, 2022

Conversation

0xCaso
Copy link
Contributor

@0xCaso 0xCaso commented Oct 21, 2022

Hi! I'd like to propose the addition of the new method equal for Strings, which would make things more elegant and make the code less repetitive. Also, it should be really useful for testing purposes with Foundry.

I'm referring to this (old) issue: #1795 (comment)

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

contracts/utils/Strings.sol Outdated Show resolved Hide resolved
@0xCaso 0xCaso requested a review from Amxx October 25, 2022 08:50
@Amxx
Copy link
Collaborator

Amxx commented Oct 26, 2022

Note: I'd love #3666 to be merge first, so that the tests can use the new syntax (instead of having to update #3666 yet again)

@Amxx Amxx requested a review from frangio October 26, 2022 15:04
@0xCaso
Copy link
Contributor Author

0xCaso commented Dec 18, 2022

What's the state of #3666? @Amxx

@frangio frangio removed the request for review from Amxx December 24, 2022 01:30
@0xCaso 0xCaso changed the title Add Strings.compare Add Strings.equal Dec 24, 2022
@0xCaso 0xCaso requested a review from frangio December 24, 2022 11:31
frangio
frangio previously approved these changes Dec 28, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM

@frangio frangio requested a review from ernestognw December 28, 2022 20:39
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

LGTM.

I'd like to explorer further if there's a cheaper way of doing this since keccak is an expensive operation.

I don't think it's worth it at the moment but mentioning for the record.

@frangio frangio merged commit 7a6a9d1 into OpenZeppelin:master Dec 28, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Dec 28, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 OpenZeppelin Contracts Contributor:

GitPOAP: 2022 OpenZeppelin Contracts Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@ernestognw ernestognw mentioned this pull request Dec 29, 2022
3 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.

4 participants