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 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
Contributor

@jacqueswww 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
Copy link
Contributor Author

@jakerockland jakerockland commented Jul 2, 2019

@jacqueswww updated 👍😄

Copy link
Contributor

@jacqueswww jacqueswww left a comment

Excellent! Thanks for this @jakerockland

@jacqueswww jacqueswww merged commit f890abe into vyperlang:master Jul 2, 2019
3 checks passed
typ=BaseType('bytes32'),
pos=getpos(expr),
)
elif sub.location == "storage":
Copy link
Contributor

@jacqueswww jacqueswww Jul 2, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jakerockland jakerockland Jul 2, 2019

Choose a reason for hiding this comment

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

@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? 😅

Copy link
Contributor

@jacqueswww jacqueswww Jul 2, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jakerockland jakerockland Jul 2, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jakerockland jakerockland Jul 2, 2019

Choose a reason for hiding this comment

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

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 😅

Copy link
Contributor

@jacqueswww jacqueswww Jul 2, 2019

Choose a reason for hiding this comment

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants