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

Improve Hash256 implementation and add unit tests #572

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

tcoratger
Copy link
Collaborator

Description

This pull request aims to enhance the implementation of the Hash256 type and introduce comprehensive unit tests to ensure its correctness and robustness.

Changes Made:

  • Improved Implementation: Refactored and optimized the existing implementation of Hash256.
  • Added Unit Tests: Introduced new unit tests to cover various scenarios and edge cases, ensuring the correctness and reliability of the Hash256 type.

@xJonathanLEI
Copy link
Owner

Some issues with this PR:

  • I'm not convinced that the from_str is more optimized than before. The reason it was written like that (slightly repetitive) was to avoid the extra allocation. Now you're using value.to_owned() on the first branch only to grab its reference. This is a wasted allocation.
  • Removing a method from public API is a breaking changes. While it's perfectly fine to implement From for the byte array, I see no good reasons to remove the from_bytes method.
  • Same issue as Add unit tests for eth address #571. Specific and tailored test cases are preferred over random data, especially considering we're literally just doing a hex::decode here. This is unnecessary complexity IMO.

@tcoratger
Copy link
Collaborator Author

Some issues with this PR:

  • I'm not convinced that the from_str is more optimized than before. The reason it was written like that (slightly repetitive) was to avoid the extra allocation. Now you're using value.to_owned() on the first branch only to grab its reference. This is a wasted allocation.
  • Removing a method from public API is a breaking changes. While it's perfectly fine to implement From for the byte array, I see no good reasons to remove the from_bytes method.
  • Same issue as Add unit tests for eth address #571. Specific and tailored test cases are preferred over random data, especially considering we're literally just doing a hex::decode here. This is unnecessary complexity IMO.

@xJonathanLEI

  • Modifications on from_str were just to simplify a bit the code and readability especially in the first two if/else if where the actions are almost the same and can be unified using the format macro.
  • Just pushed back the from_bytes method. I would say that we should put a deprecation warning on the top of it because it's always more rust idiomatic to use the dedicated From implementation for that.
  • Concerning the tests, my answer would be the same as Add unit tests for eth address #571 (comment), here it especially takes into account various scenarios for the hashes:
    • With prefix
    • Without prefix
    • 0xcase
    • It also tests various error configurations that were not tested previously (unexpected length, invalid character)
    • It also adds the from Felt unit test

@xJonathanLEI
Copy link
Owner

xJonathanLEI commented Mar 27, 2024

the actions are almost the same and can be unified using the format macro

Unified at the cost of an unnecessary allocation? I don't think so.

because it's always more rust idiomatic to use the dedicated From implementation for that

It's more idiomatic in most cases, but not always, so we definitely should not put deprecation on from_bytes. One counter example is const context. (from_bytes is not marked as const now but could be)

@xJonathanLEI
Copy link
Owner

Rebased on master for CI.

Regarding the JSON-based tests, I still believe that it's too much for testing a simple decode function. Different from #571, I like how this PR comes with test cases where the intent is explained inline. This helps code readers understand what's being tested without all the JSON complexity, with each single piece of test data tailored to a specific scenario. So I'm keeping that. Dropping the JSON part tho.

@xJonathanLEI xJonathanLEI merged commit 927f84d into xJonathanLEI:master Mar 27, 2024
26 checks passed
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