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

chore: use BuiltinName enum in EntryPointV1 instead of string #314

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

Yoni-Starkware
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware commented Aug 5, 2024

This change is Reviewable

@Yoni-Starkware Yoni-Starkware force-pushed the yoni/entry-point/use-builtin-enum branch from f0a1157 to b537551 Compare August 5, 2024 08:47
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)

a discussion (no related file):
python side PR? (builtin names are sensitive)



crates/blockifier/src/execution/contract_class.rs line 464 at r1 (raw file):

                .builtins
                .into_iter()
                .map(|builtin| BuiltinName::from_str(&builtin).expect("Unrecognized builtin."))

you are introducing a panic where there was once an error; is this safe? non-blocking

Code quote:

BuiltinName::from_str(&builtin).expect("Unrecognized builtin."))

crates/blockifier/src/execution/entry_point_execution.rs line 166 at r1 (raw file):

    )?;

    runner.initialize_function_runner_cairo_1(&entry_point.builtins)?;

what is this change? why no longer taking all builtins?

Code quote:

&entry_point.builtins

Copy link
Collaborator Author

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @dorimedini-starkware, and @noaov1)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

python side PR? (builtin names are sensitive)

Right: https://github.com/starkware-industries/starkware/pull/35685



crates/blockifier/src/execution/contract_class.rs line 464 at r1 (raw file):

Previously, dorimedini-starkware wrote…

you are introducing a panic where there was once an error; is this safe? non-blocking

I like panic :(
I'll think about it


crates/blockifier/src/execution/entry_point_execution.rs line 166 at r1 (raw file):

Previously, dorimedini-starkware wrote…

what is this change? why no longer taking all builtins?

The entrypoint builtins are all we need. Initializing all of them might hide bugs

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware and @noaov1)

@Yoni-Starkware Yoni-Starkware merged commit e9a9f67 into main-v0.13.2 Aug 5, 2024
14 checks passed
@Yoni-Starkware Yoni-Starkware deleted the yoni/entry-point/use-builtin-enum branch August 5, 2024 14:52
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 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