-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat: usort
hints integration tests
#459
feat: usort
hints integration tests
#459
Conversation
@cicr99 @har777 @MaksymMalicki Usort integration test is finished. I modified a few things in usort hints:
Let's wait Finalize Segment to be merged to make sure the memory traces match, but i tested outputs and everything against other VMs and its ok |
@har777 @cicr99 @MaksymMalicki ready for approval/merging I added a new test for it to make sure multiplicities works well (lambdaclass didnt cover this) |
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.
Well the integration test looks good. Other than that I believe its mostly relocation with a change in one place where you have converted felt to uint64. I think @cicr99 should have a take on that. I just left one comment on the change in implementation of a hint.
@Sh0g0-1758 we already discussed with Carmen the change to uint64 here. If you check in detail, you'll see i changed it always for variable that represents either a length of array or a index in an array. Its ok then |
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
@cicr99 i checked hints separately locally and it seems fine, except
usortBodyCode
which is not detected by our VM, just likesignedDivRem
We might need to do like lambda go and declare each hint as a const