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

eip1898: add unit tests to secure all conversions and impl #1544

Merged
merged 10 commits into from
Oct 24, 2024

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Oct 21, 2024

  • Full path under serde is used instead of import under cfg flag.
  • Some functions are simplified.
  • Unit tests are added to protect all implementations and conversions that can be tricky to mix.

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Proposed changes are sensible and does not appear to contain breaking changes. Tests look good, ran tests against main and verified there were no regressions.

@tcoratger
Copy link
Contributor Author

Proposed changes are sensible and does not appear to contain breaking changes. Tests look good, ran tests against main and verified there were no regressions.

@zerosnacks Yes exactly, that's exactly why I was doing this PR. My constant was that there were a lot of conversions in this file and that simplifications could be made in order to end up with exactly the same code while having a less wordy and error-prone implementation.

  • So I simplified the places that could be simplified by using all the available conversions as much as possible.
  • I took care to normally add all the unit tests covering all situations because these unit tests are very quick to run and they allow two things:
    • Check that this PR doesn't break anything and that everything is correct (conversions, string manipulations...)
    • Be sure that we don't break anything in the future as soon as we touch this EIP.

@zerosnacks
Copy link
Member

cc @yash-atreya pending final review, LGTM

@yash-atreya yash-atreya merged commit 7cd817a into alloy-rs:main Oct 24, 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.

3 participants