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

review and fix string vs. bytes handling in cast #669

Open
2 tasks done
mds1 opened this issue Feb 4, 2022 · 5 comments · Fixed by #671
Open
2 tasks done

review and fix string vs. bytes handling in cast #669

mds1 opened this issue Feb 4, 2022 · 5 comments · Fixed by #671
Labels
C-cast Command: cast D-easy Difficulty: easy first issue A good way to start contributing P-normal Priority: normal T-debt Type: code debt T-to-reproduce Type: requires reproduction
Milestone

Comments

@mds1
Copy link
Collaborator

mds1 commented Feb 4, 2022

Component

Cast

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

cast 0.1.0 (fe2dbfe 2022-02-04T00:24:58.188894+00:00)

What command(s) is the bug in?

cast keccak and possibly others

Operating System

macOS (amd)

Describe the bug

Right now, cast keccak does not handle strings vs. bytes correctly, which @SusheendharVijay first noticed when implementing cast index as part of #29.

The correct way to handle input to cast keccak <data> is:

  • If <data> has a 0x prefix, read it as hexdata. Multiple hexstrings can be concatenated with :
  • If <data> has no 0x prefix, read it as text

Instead, we currently treat all input as text and convert it to bytes, which is incorrect. seth and ethers.js use the behavior above

This makes me concerned that there may be other commands where we don't distinguish between text vs. bytes inputs correctly, so the scope of this issue is:

  1. Review how that's handled for the relevant cast commands, using seth as the source of truth
  2. For ones that are incorrect, fix the input handing
  3. Whether correct or incorrect, ensure we have doctests for both text (no 0x prefix) and bytes (0x prefix) on all relevant cast commands

For example, if we had a keccak doctest of assert_eq!(Cast::keccak("0x1234")?, "0x56570de287d73cd1cb6092bb8fdee6173974955fdef345ae579ee9f475ea7432");, we would have noticed this error sooner

@mds1 mds1 added the T-bug Type: bug label Feb 4, 2022
@mds1
Copy link
Collaborator Author

mds1 commented Feb 4, 2022

@gakonst mind re-opening this one? we fixed cast keccak but IMO should also review other methods that should accept both text or bytes

@gakonst gakonst reopened this Feb 5, 2022
@onbjerg onbjerg added C-cast Command: cast D-easy Difficulty: easy first issue A good way to start contributing P-normal Priority: normal labels Feb 5, 2022
@onbjerg onbjerg added T-debt Type: code debt and removed T-bug Type: bug labels Mar 28, 2022
@onbjerg onbjerg added this to Foundry Apr 17, 2022
@onbjerg onbjerg moved this to Todo in Foundry Apr 17, 2022
@onbjerg onbjerg added this to the v1.0.0 milestone Jul 1, 2022
@onbjerg onbjerg removed this from the v1.0.0 milestone Aug 23, 2022
@lucas-manuel
Copy link

Close-able? @mds1

@mds1
Copy link
Collaborator Author

mds1 commented May 18, 2023

@tynes do you know offhand if this can be closed? I haven't been using cast much recently for strings/bytes so am not certain, but feel like you may be using it a lot 😅

The issue is kinda vague in that it says to review all relevant commands since some might be incorrect, so hard to know if this should be closed unless someone checks each

@0xSyntellect
Copy link

@mds1 bumping this.

Also had issues with generating init code hash via chisel and cast from uniswap pair contract creation code. It always returned an incorrect value against https://emn178.github.io/online-tools/keccak_256.html.

What I was trying in chisel: bytes32 initCode = keccak256(".....") or cast keccak ....
but it reads bytecode as string and returns incorrect hash.

I think we need to be able to specify input format somehow

@mds1
Copy link
Collaborator Author

mds1 commented May 2, 2024

@0xSyntellect Can you provide concrete steps to reproduce?

@zerosnacks zerosnacks added the T-to-reproduce Type: requires reproduction label Jun 26, 2024
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cast Command: cast D-easy Difficulty: easy first issue A good way to start contributing P-normal Priority: normal T-debt Type: code debt T-to-reproduce Type: requires reproduction
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants