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

Fix bytesToNumber() #42

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

assafmo
Copy link
Contributor

@assafmo assafmo commented Nov 8, 2023

This fixes bytesToNumber() to always return the result as an unsigned integer.

Example: byteArray holds the value 2^32 - 3, which is 11111111111111111111111111111101 in binary. The old implementation uses the number type, which is a signed integer, to calculate intermediate values. And because the most significant bit (MSB) is turned on, it interprets this as a negative number and returns -3.

This fixes `bytesToNumber()` to always return the result as an unsigned integer.

Example: `byteArray` holds the value `2^32 - 3`, which is `11111111111111111111111111111101` in binary. The old implementation uses the `number` type, which is a signed integer, to calculate intermediate values. And because the most significant bit (MSB) is turned on, it interprets this as a negative number and returns `-3`.
Copy link

cla-bot bot commented Nov 8, 2023

Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @assafmo on file. In order for us to review and merge your code, please sign:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

@immortal-tofu
Copy link
Collaborator

immortal-tofu commented Nov 8, 2023

Thank you!

Buffer is not available in browser but I think we use polyfill. Did you test it in browser? I'll checkout the branch and test.

@immortal-tofu immortal-tofu self-requested a review November 8, 2023 13:19
@assafmo
Copy link
Contributor Author

assafmo commented Nov 8, 2023

No, I've only tested it on nodejs.

Copy link

cla-bot bot commented Nov 9, 2023

Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @assafmo on file. In order for us to review and merge your code, please sign:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

@aquint-zama
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Nov 10, 2023
Copy link

cla-bot bot commented Nov 10, 2023

The cla-bot has been summoned, and re-checked this pull request!

@immortal-tofu immortal-tofu merged commit 7b1f4b1 into zama-ai:main Nov 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants