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

Allow equality checks between string types #1507

Merged
merged 2 commits into from Jul 2, 2019

Conversation

@jakerockland
Copy link
Contributor

jakerockland commented Jun 28, 2019

What I did

Implemented VIP #1355, allowing equality checks between string types.

How I did it

Check strings with length > 32 or unequal lengths by comparing keccak256 hash of each.

How to verify it

Run the tests: make test. More specifically: pytest -k test_string and pytest -k test_bytes

Description for the changelog

Allow equality checks between string types

Cute Animal Picture

image

Copy link
Collaborator

jacqueswww left a comment

Good stuff! I would just like 2 more simple test cases to be added:

  • Variable from storage equality check
  • Variable from parameter equality check
@jakerockland

This comment has been minimized.

Copy link
Contributor Author

jakerockland commented Jul 2, 2019

@jacqueswww updated 👍😄

Copy link
Collaborator

jacqueswww left a comment

Excellent! Thanks for this @jakerockland

@jacqueswww jacqueswww merged commit f890abe into ethereum:master Jul 2, 2019
3 checks passed
3 checks passed
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
typ=BaseType('bytes32'),
pos=getpos(expr),
)
elif sub.location == "storage":

This comment has been minimized.

Copy link
@jacqueswww

jacqueswww Jul 2, 2019

Collaborator

@jakerockland I just read this now, after having merged it. Will this not only hash the first 32bytes?

This comment has been minimized.

Copy link
@jakerockland

jakerockland Jul 2, 2019

Author Contributor

@jacqueswww hrm yeah this is an oversight on my part too 😓

Given that this is the same code generation that is (and was) being used for the built-in keccak256 function as well, I think the issue here is a bit broader in scope than the string comparison stuff, in that for any hashes of any string literal/bytes array longer than 32 bytes, we are only hashing the first 32 bytes.

Given our definition of the built-in supporting string literals + byte arrays:

def sha3(a) -> b:
  """
  :param a: value to hash
  :type a: either str_literal, bytes, bytes32

  :output b: bytes32
  """

I think we may want to open another ticket specifying that we need to fix the keccak256 behavior all-around to hash all bytes + need to add tests around this both in the context of string comparison but also just using the built-in.

I will have some time this evening that I can get started with writing tests around this to verify the issue + can open up a ticket from there.

Thoughts? 😅

This comment has been minimized.

Copy link
@jacqueswww

jacqueswww Jul 2, 2019

Collaborator

Hmm just check again, pretty sure keccak256/sha3 has tests for longer than 32 byte strings. Maybe only storage values ?

This comment has been minimized.

Copy link
@jakerockland

jakerockland Jul 2, 2019

Author Contributor

Hrm, yeah you're right we do have a couple of tests for that in tests/parser/functions/test_keccak256.py.

I need to do a bit more reading on the LLL here, but it does seem to me that if things currently work properly for hashing byte arrays longer than 32, that string comparison will work as well, as they use the same helper method?

Likewise, if there was an issue with the string comparison for strings/byte arrays longer than 32, it would make me think we need to add some more tests with the keccak256 built-in.

Does that make sense? @jacqueswww

This comment has been minimized.

Copy link
@jakerockland

jakerockland Jul 2, 2019

Author Contributor

Either way, it probably would be good to add more nuanced tests to both functions/test_keccak256.py and types/test_string.py, which I'm happy to do this evening @jacqueswww — from there it will be a lot more clear what exactly is wrong 😅

This comment has been minimized.

Copy link
@jacqueswww

jacqueswww Jul 2, 2019

Collaborator

Yeah, clearly more tricky than one would initially think! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.