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(blockifier): unify get entry point for all contract classes #900

Conversation

meship-starkware
Copy link
Contributor

@meship-starkware meship-starkware commented Sep 19, 2024

This change is Reviewable

# Conflicts:
#	crates/blockifier/src/execution/syscalls/syscalls_test.rs
…cution-engine

# Conflicts:
#	Cargo.lock
#	crates/blockifier/src/execution/contract_class.rs
@meship-starkware meship-starkware changed the title Meship/blockifier/unify get entry point v0 and v1 chore(blokifier): unify get entry point for all contract classes Sep 19, 2024
@meship-starkware meship-starkware changed the title chore(blokifier): unify get entry point for all contract classes chore(blockifier): unify get entry point for all contract classes Sep 19, 2024
@meship-starkware meship-starkware force-pushed the meship/blockifier/unify_get_entry_point_v0_and_v1 branch from 7a4d4ad to e35dc60 Compare September 19, 2024 15:46
@meship-starkware meship-starkware changed the base branch from native/add-entry-point-execution to native/add-native-execution-engine September 19, 2024 15:48
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 27.48092% with 95 lines in your changes missing coverage. Please review.

Please upload report for BASE (native/add-native-execution-engine@0a88987). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/blockifier/src/execution/contract_class.rs 16.12% 78 Missing ⚠️
.../blockifier/src/execution/entry_point_execution.rs 75.00% 4 Missing and 1 partial ⚠️
crates/blockifier/src/execution/native/utils.rs 0.00% 5 Missing ⚠️
crates/blockifier/src/execution/execution_utils.rs 0.00% 2 Missing ⚠️
crates/papyrus_protobuf/src/converters/class.rs 0.00% 2 Missing ⚠️
crates/blockifier/src/execution/entry_point.rs 83.33% 1 Missing ⚠️
crates/blockifier/src/test_utils/contracts.rs 0.00% 1 Missing ⚠️
...ates/starknet_api/src/deprecated_contract_class.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                          Coverage Diff                          @@
##             native/add-native-execution-engine     #900   +/-   ##
=====================================================================
  Coverage                                      ?   74.38%           
=====================================================================
  Files                                         ?      367           
  Lines                                         ?    38657           
  Branches                                      ?    38657           
=====================================================================
  Hits                                          ?    28754           
  Misses                                        ?     7576           
  Partials                                      ?     2327           
Flag Coverage Δ
74.38% <27.48%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@meship-starkware meship-starkware force-pushed the meship/blockifier/unify_get_entry_point_v0_and_v1 branch 4 times, most recently from 86f9e9e to 236afd6 Compare September 22, 2024 10:47
Copy link
Contributor Author

@meship-starkware meship-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: 0 of 14 files reviewed, 1 unresolved discussion


crates/blockifier/src/execution/contract_class.rs line 65 at r2 (raw file):

}

#[derive(Clone)]

I am not sure an enum is the right solution here.

@meship-starkware meship-starkware force-pushed the meship/blockifier/unify_get_entry_point_v0_and_v1 branch from 236afd6 to f2ff7f2 Compare September 25, 2024 11:46
Copy link
Contributor Author

@meship-starkware meship-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: 0 of 14 files reviewed, 2 unresolved discussions


crates/blockifier/src/execution/contract_class.rs line 118 at r3 (raw file):

    }

    pub fn entry_points_of_same_type(&self, entry_point_type: EntryPointType) -> Vec<EntryPoint> {

This function is a bit expensive. Also, the name is a bit confusing because ContractClass::V0() also has an entry_points_of_same_type file, and calling this function with ContractClass::V0() will result in panic.

@rodrigo-pino rodrigo-pino force-pushed the native/add-native-execution-engine branch 5 times, most recently from ceeeaa4 to 9a9e4bb Compare October 7, 2024 15:39
@noaov1 noaov1 deleted the branch native/add-native-execution-engine October 7, 2024 19:56
@noaov1 noaov1 closed this Oct 7, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 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.

3 participants