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

Execution resource ops add subtract #774

Merged

Conversation

0xNonCents
Copy link
Contributor

@0xNonCents 0xNonCents commented Jan 25, 2023

The ExecutionResource struct ought to have same functionalities it has in the python vm. They are used in StarkNet
https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/lang/vm/cairo_pie.py#L123

#767
Added two unit tests for each added op.

I do not believe documentation needs to be updated

@Oppen
Copy link
Member

Oppen commented Jan 25, 2023

Hello @0xNonCents, congrats on your first contribution! 🎉
This looks good. Is there any way to add a Cairo program that triggers this code as integration test?

Re: documentation: I'm on the fence about whether these APIs need it. Particularly, about whether or not I find the behavior obvious enough.

Copy link
Contributor

@Jrigada Jrigada left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for your contribution, there are some small clippy changes required!

Copy link
Contributor

@mmsc2 mmsc2 left a comment

Choose a reason for hiding this comment

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

Hello @0xNonCents nice one!. The only thing i would mention is that i added filter_unused_builtins function to have it all in the same PR, besides that it looks good.

@fmoletta fmoletta requested a review from Jrigada January 27, 2023 19:25
@Jrigada Jrigada merged commit 8dba86d into lambdaclass:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants