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

Use bytes.concat in MessageHashUtils #4504

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Aug 2, 2023

We can use bytes.concat instead of abi.encodePacked here which is clearer.

Updating an existing changeset to mention this PR too.

In toDataWithIntendedValidatorHash we keep abi.encodePacked because one of the arguments is an address.

@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2023

⚠️ No Changeset found

Latest commit: f4e85f3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

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

Click here to learn what changesets are, and how to add one.

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

@frangio frangio requested review from ernestognw and Amxx and removed request for ernestognw August 2, 2023 16:25
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.

FYI, I considered that change in #4296, but was not sure because of the casting.

both

keccak256(bytes.concat("\x19Ethereum Signed Message:\n", bytes(Strings.toString(message.length)), message));

and

keccak256(string.concat("\x19Ethereum Signed Message:\n", Strings.toString(message.length), string(message)));

should work. Any reason to favor the first one ?

@frangio
Copy link
Contributor Author

frangio commented Aug 2, 2023

keccak256 requires a bytes argument.

@frangio frangio enabled auto-merge (squash) August 2, 2023 18:32
@frangio frangio merged commit d39df78 into OpenZeppelin:master Aug 2, 2023
@frangio frangio deleted the bytes-concat branch August 2, 2023 18:45
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