-
Notifications
You must be signed in to change notification settings - Fork 81
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: addmod #88
fix: addmod #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
smol conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: parenthesis for 2 comments
src/tests/test_instructions/test_stop_and_arithmetic_operations.cairo
Outdated
Show resolved
Hide resolved
src/tests/test_instructions/test_stop_and_arithmetic_operations.cairo
Outdated
Show resolved
Hide resolved
…s.cairo Co-authored-by: greged93 <82421016+greged93@users.noreply.github.com>
…s.cairo Co-authored-by: greged93 <82421016+greged93@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: Closes #83
What is the new behavior?
Does this introduce a breaking change?
Added changes to
CHANGELOG.md
Other information
ADDMOD
I tried two different implementations for this ADDMOD opcode, leveraging the fact that
(x + y) mod N <=> (x mod N) + (y mod N) mod N
The first try was using u256_wide_add, a function that I created that adds two u256s and returns a u512 to avoid overflowing on the a+b addition, and then dividing it by N and returning the remainder.
Here is the test output
The other implementation is
Here is the test output
This second version is ~3% more costly. Therefore, we will keep the first one with u256_wide_add.