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: use snapi class info in process declare tx #516

Merged
merged 1 commit into from
Aug 25, 2024

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Aug 19, 2024

During the function process_tx in the gateway, the compilation (sierra to casm) occurs.
In this PR, we change the compilation's return type from blockifier::execution::contract_class::ClassInfo to starknet_api::contract_class::ClassInfo.

In the following PRs, MempoolInput will (indirectly) use starknet_api::..::ClassInfo.

This change is Reviewable

Copy link
Contributor Author

ArniStarkware commented Aug 19, 2024

@ArniStarkware ArniStarkware marked this pull request as ready for review August 19, 2024 13:25
@ArniStarkware ArniStarkware force-pushed the arni/executable_tx_in_gateway/update_process_tx branch from 9a5d8bd to a5af9bc Compare August 19, 2024 14:42
@ArniStarkware ArniStarkware changed the base branch from main to arni/snapi/contract_class/streamline August 19, 2024 14:42
@ArniStarkware ArniStarkware force-pushed the arni/snapi/contract_class/streamline branch from b38f6d8 to a5fa529 Compare August 20, 2024 11:43
@ArniStarkware ArniStarkware force-pushed the arni/executable_tx_in_gateway/update_process_tx branch from a5af9bc to 65e6550 Compare August 20, 2024 11:43
@ArniStarkware ArniStarkware force-pushed the arni/snapi/contract_class/streamline branch from a5fa529 to 7a53f19 Compare August 20, 2024 14:04
@ArniStarkware ArniStarkware force-pushed the arni/executable_tx_in_gateway/update_process_tx branch from 65e6550 to 70056e5 Compare August 20, 2024 14:04
@ArniStarkware ArniStarkware changed the base branch from arni/snapi/contract_class/streamline to graphite-base/516 August 21, 2024 06:57
@ArniStarkware ArniStarkware force-pushed the arni/executable_tx_in_gateway/update_process_tx branch from 70056e5 to b0a4971 Compare August 21, 2024 06:58
@ArniStarkware ArniStarkware changed the base branch from graphite-base/516 to main August 21, 2024 06:58
@ArniStarkware ArniStarkware force-pushed the arni/executable_tx_in_gateway/update_process_tx branch from b0a4971 to 10fc2e9 Compare August 21, 2024 06:58
Copy link
Collaborator

@dafnamatsry dafnamatsry 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 4 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @ayeletstarkware, @MohammadNassar1, and @yair-starkware)


crates/gateway/src/compilation_test.rs line 105 at r3 (raw file):

    let class_info = gateway_compiler.process_declare_tx(&declare_tx).unwrap();
    assert_eq!(class_info.sierra_program_length, sierra_program_length);

Please add also an assertion for the casm_contract_class.

@ArniStarkware ArniStarkware force-pushed the arni/executable_tx_in_gateway/update_process_tx branch from 10fc2e9 to 6bc63b8 Compare August 21, 2024 09:16
Copy link
Contributor Author

@ArniStarkware ArniStarkware 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: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @dafnamatsry, @MohammadNassar1, and @yair-starkware)


crates/gateway/src/compilation_test.rs line 105 at r3 (raw file):

Previously, dafnamatsry wrote…

Please add also an assertion for the casm_contract_class.

Done.
This is enough - it hard to compare the entire contract class is as expected. It is enough to see the result casm contract class has the expected hash.

@ArniStarkware ArniStarkware force-pushed the arni/executable_tx_in_gateway/update_process_tx branch from 6bc63b8 to 5aa7df9 Compare August 22, 2024 13:56
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 76.60%. Comparing base (d056964) to head (5aa7df9).
Report is 12 commits behind head on main.

Files Patch % Lines
crates/gateway/src/gateway.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #516   +/-   ##
=======================================
  Coverage   76.60%   76.60%           
=======================================
  Files         348      348           
  Lines       36771    36768    -3     
  Branches    36771    36768    -3     
=======================================
- Hits        28167    28165    -2     
- Misses       6281     6283    +2     
+ Partials     2323     2320    -3     

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

Copy link
Collaborator

@dafnamatsry dafnamatsry 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 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware, @MohammadNassar1, and @yair-starkware)

@ArniStarkware ArniStarkware merged commit ed6ab6f into main Aug 25, 2024
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 27, 2024
@ArniStarkware ArniStarkware deleted the arni/executable_tx_in_gateway/update_process_tx branch September 22, 2024 11:36
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