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

Adding SafeCast variants for signed integers #2243

Merged

Conversation

julianmrodri
Copy link
Contributor

#2240

Fixes #2240

Description: * Added SafeCast variants for signed Integers. This includes the following functions:

  • toInt8(int256 x): Downcasts to int8 from int256
  • toInt16(int256 x): Downcasts to int16 from int256
  • toInt32(int256 x): Downcasts to int32 from int256
  • toInt64(int256 x): Downcasts to int64 from int256
  • toInt128(int256 x): Downcasts to int128 from int256

Also added the corresponding Tests and adjustments to the Mockup contract. All tests works correctly and linter is also validated.

Modified Files:

  1. contracts/utils/SafeCast.sol
  2. contracks/mocks/SafeCastMock.sol
  3. test/utils/SafeCast.test.js

@julianmrodri
Copy link
Contributor Author

julianmrodri commented May 23, 2020

@nventuro @frangio Ready to be reviewed. Let me know in case of any changes or modifications that need to be done. Thanks!

@julianmrodri julianmrodri force-pushed the feat/safecast-signedints-#2240 branch from 895ab5b to 9c07599 Compare May 23, 2020 21:06
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks a lot @julianmrodri! Code style, tests, documentation, it's all perfect. I really don't have any comments other than an extremely minor suggestions. Do you have any questions or concerns about these new functions?

contracts/utils/SafeCast.sol Outdated Show resolved Hide resolved
Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
@julianmrodri
Copy link
Contributor Author

Thanks @nventuro! I was sure I had to update the CHANGELOG as well but it looked quite sensitive as I wasn't sure I should include it under the next release, so I left it you to you guys.

I don't have any concerns about these functions I did quite a bunch of manual testing as well interacting with the Contract and seems to work as expected. The formula extrapolated nicely to all the different types.

Let me know if there's anything else to do or include and I can take care. Thanks again!

@nventuro
Copy link
Contributor

Awesome, thanks! We don't ask contributors to edit the changelog to reduce the burden on them, specially since it's very simple for us to do from the web UI.

Thank you very much once more!

@nventuro nventuro merged commit 5513dfd into OpenZeppelin:master May 26, 2020
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.

Add SafeCast variants for signed integers
2 participants