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

signed u256 - i257 #236

Merged
merged 10 commits into from
Jan 12, 2024
Merged

signed u256 - i257 #236

merged 10 commits into from
Jan 12, 2024

Conversation

shramee
Copy link
Contributor

@shramee shramee commented Jan 4, 2024

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

We don't have signed u256.

Issue Number: N/A

What is the new behavior?

We have signed u256.

Does this introduce a breaking change?

  • Yes
  • No

Other information

The code and tests for this is largely taken from old i129 int. I've tried to make commits with old pasted code and tweaks made on that commit. Hopefully that will make reviewing easier.

Source: https://github.com/shramee/alexandria/blob/7e0f050b97c42b2a5916d552494d3724ae2c0197/alexandria/math/src/signed_integers.cairo#L1618

Tests come from: https://github.com/shramee/alexandria/blob/7e0f050b97c42b2a5916d552494d3724ae2c0197/alexandria/math/src/tests/signed_integers_test.cairo#L1279

@shramee shramee requested a review from 0xLucqs as a code owner January 4, 2024 11:23
@shramee shramee changed the title Signed u256 aka i257 signed u256 aka i257 Jan 4, 2024
@shramee shramee changed the title signed u256 aka i257 signed u256 - i257 Jan 4, 2024
@shramee
Copy link
Contributor Author

shramee commented Jan 4, 2024

I'm using i257 for Euclidean algorithm implementation of alexandria_math::mod_arithmetics.
image

https://x.com/shrameetweets/status/1743034431521124704?s=20

src/math/src/signed_u256.cairo Show resolved Hide resolved
src/math/src/signed_u256.cairo Outdated Show resolved Hide resolved
src/math/src/signed_u256.cairo Outdated Show resolved Hide resolved
Copy link
Collaborator

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

revert the DivRem plz otherwise lgtm

@shramee
Copy link
Contributor Author

shramee commented Jan 10, 2024

@LucasLvy Done

@0xLucqs 0xLucqs merged commit 1781538 into keep-starknet-strange:main Jan 12, 2024
3 checks passed
@shramee shramee deleted the i257 branch January 12, 2024 17:57
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants