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

refactor(starknet_api): remove duplicate definition of contract class #2298

Merged

Conversation

AvivYossef-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

github-actions bot commented Nov 27, 2024

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.82%. Comparing base (e3165c4) to head (6383cd0).
Report is 612 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2298       +/-   ##
===========================================
- Coverage   40.10%   27.82%   -12.29%     
===========================================
  Files          26      199      +173     
  Lines        1895    25435    +23540     
  Branches     1895    25435    +23540     
===========================================
+ Hits          760     7077     +6317     
- Misses       1100    17776    +16676     
- Partials       35      582      +547     

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

@AvivYossef-starkware AvivYossef-starkware force-pushed the 11-26-refactor_starknet_api_entry_points_by_type_field_of_sierra_contact branch 2 times, most recently from 607c640 to 378185a Compare November 27, 2024 09:43
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/remove_duplicate_deffinition_of_contract_class branch from f4b3c28 to 2a84a7a Compare November 27, 2024 09:43
@AvivYossef-starkware AvivYossef-starkware changed the base branch from 11-26-refactor_starknet_api_entry_points_by_type_field_of_sierra_contact to graphite-base/2298 November 27, 2024 13:08
@AvivYossef-starkware AvivYossef-starkware changed the base branch from graphite-base/2298 to main November 27, 2024 13:41
@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/remove_duplicate_deffinition_of_contract_class branch from 2a84a7a to 1da9eae Compare November 27, 2024 13:48
Copy link
Collaborator

@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.

Reviewed 8 of 12 files at r1, all commit messages.
Reviewable status: 8 of 12 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware)


crates/starknet_sierra_compile/src/utils.rs line 12 at r2 (raw file):

use starknet_api::state::{
    EntryPoint as StarknetApiEntryPoint,
    SierraContractClass as RpcContractClass,

Suggestion:

SierraContractClass as StarknetApiContractClass,

crates/papyrus_protobuf/src/converters/rpc_transaction.rs line 334 at r2 (raw file):

}

impl TryFrom<protobuf::Cairo1Class> for ContractClass {

@DvirYo-starkware are we okay with removing this?

Code quote:

impl TryFrom<protobuf::Cairo1Class> for ContractClass {

crates/starknet_gateway/src/stateless_transaction_validator_test.rs line 359 at r2 (raw file):

    #[case] expected_error: StatelessTransactionValidatorError,
) {
    use starknet_api::state::SierraContractClass;

Is this necessary?

Code quote:

use starknet_api::state::SierraContractClass;

@AvivYossef-starkware AvivYossef-starkware force-pushed the aviv/remove_duplicate_deffinition_of_contract_class branch from 1da9eae to 6383cd0 Compare November 27, 2024 15:42
Copy link
Contributor Author

@AvivYossef-starkware AvivYossef-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: 8 of 12 files reviewed, 3 unresolved discussions (waiting on @DvirYo-starkware and @Yoni-Starkware)


crates/papyrus_protobuf/src/converters/rpc_transaction.rs line 334 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

@DvirYo-starkware are we okay with removing this?

Dvir approves the PR as long as the tests pass.


crates/starknet_gateway/src/stateless_transaction_validator_test.rs line 359 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Is this necessary?

No


crates/starknet_sierra_compile/src/utils.rs line 12 at r2 (raw file):

use starknet_api::state::{
    EntryPoint as StarknetApiEntryPoint,
    SierraContractClass as RpcContractClass,

Done.

Copy link
Collaborator

@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.

:lgtm:

Reviewed 1 of 12 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: 11 of 12 files reviewed, all discussions resolved

Copy link
Collaborator

@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.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

@AvivYossef-starkware AvivYossef-starkware merged commit 5047301 into main Nov 28, 2024
20 checks passed
Copy link
Contributor Author

Merge activity

  • Nov 28, 2:53 AM EST: A user merged this pull request with Graphite.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 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