-
Notifications
You must be signed in to change notification settings - Fork 210
fix: save x1 variable to execution scope in fast_ec_add_assign_new_x #2266
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: save x1 variable to execution scope in fast_ec_add_assign_new_x #2266
Conversation
|
Hi, @Bilogweb3!
|
|
@gabrielbosio HI! The Python hint is actually right in our test file here: https://github.com/lambdaclass/cairo-vm/blob/main/cairo_programs/fast_ec_add_v2.cairo#L41-L51 You can see it creates x1 in the scope along with x0 and y0. About a failing test - honestly, I don't have one. The thing is, x1 isn't actually read from the scope afterwards, so programs run fine without this fix. What bothered me was that we're computing x1 and using it in the calculation, but then not saving it to scope like the hint says we should. Every other similar function (like ec_double_assign_new_x) saves all the variables it computes, so this looked like someone just forgot to add that line when copying code. I can write a test that verifies x1 exists in the scope after execution if you think that would be useful? It wouldn't catch a crash, but it would verify we're actually doing what the hint specification says. Let me know what you think. |
I think it wasn't added to the execution scope because there are no other hints that use that variable so it wasn't necessary.
It might be useful since a crate that uses the |
|
Added a test for this. It checks that x0, x1, and y0 are in the scope after execution - same pattern as the other tests in the file. |
gabrielbosio
left a comment
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.
Nice job!
JulianGCalderon
left a comment
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!
The function computes x1 from point1.x and uses it in calculations, but never saves it to execution scope. This doesn't match the Python hint spec which explicitly defines x1 in scope, and breaks the pattern used in similar functions like ec_double_assign_new_x where all computed variables are saved.
Added exec_scopes.insert_value("x1", x1) to fix this oversight.