Skip to content

apollo_gateway: dependency inject transaction converter #9476

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 5, 2025
Merged

apollo_gateway: dependency inject transaction converter #9476

merged 1 commit into from
Oct 5, 2025

Conversation

alonh5
Copy link
Collaborator

@alonh5 alonh5 commented Sep 30, 2025

No description provided.

@alonh5 alonh5 mentioned this pull request Sep 30, 2025
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 30, 2025 •
edited
Loading

Artifacts upload workflows:

@alonh5 Graphite App
Copy link
Collaborator Author

alonh5 commented Sep 30, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@reviewable-StarkWare
Copy link

This change is Reviewable

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 30, 2025 •
edited
Loading

Benchmark movements: full_committer_flow performance improved 😺 full_committer_flow time: [22.271 ms 22.577 ms 22.904 ms] change: [-10.197% -8.4500% -6.6726%] (p = 0.00 < 0.05) Performance has improved. Found 3 outliers among 100 measurements (3.00%) 3 (3.00%) high mild

@alonh5 alonh5 force-pushed the 09-30-dependency_inject_transaction_converter branch from 77814b1 to 71ac816 Compare September 30, 2025 11:28
@alonh5 alonh5 changed the title dependency inject transaction converter apollo_gateway: dependency inject transaction converter Sep 30, 2025
@alonh5 alonh5 force-pushed the 09-30-dependency_inject_transaction_converter branch from 71ac816 to ebb0f7a Compare September 30, 2025 12:17
ArniStarkware
ArniStarkware reviewed Oct 5, 2025
View reviewed changes
Copy link
Contributor

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

@ArniStarkware reviewed 9 of 12 files at r1, all commit messages.
Reviewable status: 9 of 12 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

crates/starknet_api/src/test_utils/declare.rs line 227 at r1 (raw file):

            self.args.clone(),
            ClassInfo::new(&default_compiled_contract_class(), 100, 100, SierraVersion::LATEST)
                .unwrap(),

You can't use self.contract_class, because it is of the wrong type. (self.contract_class: SierraContractClass vs default_compiled_contract_class(): ContractClass).

Code quote:

            ClassInfo::new(&default_compiled_contract_class(), 100, 100, SierraVersion::LATEST)
                .unwrap(),

ArniStarkware
ArniStarkware reviewed Oct 5, 2025
View reviewed changes
Copy link
Contributor

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

@ArniStarkware reviewed 1 of 12 files at r1.
Reviewable status: 10 of 12 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

ArniStarkware
ArniStarkware reviewed Oct 5, 2025
View reviewed changes
Copy link
Contributor

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

@ArniStarkware reviewed 1 of 12 files at r1.
Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

ArniStarkware
ArniStarkware requested changes Oct 5, 2025
View reviewed changes
Copy link
Contributor

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

@ArniStarkware reviewed 1 of 12 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

crates/apollo_gateway/src/gateway_test.rs line 214 at r1 (raw file):

    tx_args: &impl TestingTxArgs,
) {
    let internal_tx = tx_args.get_internal_tx();

No need to pass the rpc_tx separately.

Suggestion:

fn setup_transaction_converter_mock(
    mock_transaction_converter: &mut MockTransactionConverterTrait,
    tx_args: &impl TestingTxArgs,
) {
    let rpc_tx = tx_args.get_rpc_tx();
    let internal_tx = tx_args.get_internal_tx();

@alonh5 alonh5 force-pushed the 09-25-apollo_gateway_extract_async_code_from_blocking_task branch from d28bb76 to 4f9614e Compare October 5, 2025 09:11
@alonh5 alonh5 force-pushed the 09-30-dependency_inject_transaction_converter branch from ebb0f7a to 150836a Compare October 5, 2025 09:11
This was referenced Oct 5, 2025
Open
Open
alonh5
alonh5 commented Oct 5, 2025
View reviewed changes
Copy link
Collaborator Author

@alonh5 alonh5 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: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

crates/apollo_gateway/src/gateway_test.rs line 214 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

No need to pass the rpc_tx separately.

Done.

crates/starknet_api/src/test_utils/declare.rs line 227 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

You can't use self.contract_class, because it is of the wrong type. (self.contract_class: SierraContractClass vs default_compiled_contract_class(): ContractClass).

Right, that's why I'm not using it.

@alonh5 alonh5 force-pushed the 09-30-dependency_inject_transaction_converter branch from 150836a to 63c1a13 Compare October 5, 2025 09:16
ArniStarkware
ArniStarkware approved these changes Oct 5, 2025
View reviewed changes
Copy link
Contributor

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

@ArniStarkware reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

ArniStarkware
ArniStarkware approved these changes Oct 5, 2025
View reviewed changes
Copy link
Contributor

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

:lgtm:

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

@alonh5 alonh5 changed the base branch from 09-25-apollo_gateway_extract_async_code_from_blocking_task to main-v0.14.1 October 5, 2025 12:35
@alonh5 alonh5 force-pushed the 09-30-dependency_inject_transaction_converter branch from 63c1a13 to 38cfe26 Compare October 5, 2025 12:35
@graphite-app Graphite App
Copy link

graphite-app bot commented Oct 5, 2025

Merge activity

  • Oct 5, 12:36 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

ArniStarkware
ArniStarkware approved these changes Oct 5, 2025
View reviewed changes
Copy link
Contributor

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

@ArniStarkware reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware, @noamsp-starkware, and @ShahakShama)

@alonh5 alonh5 added this pull request to the merge queue Oct 5, 2025
Merged via the queue into main-v0.14.1 with commit 9ff5109 Oct 5, 2025
34 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArniStarkware ArniStarkware ArniStarkware approved these changes

@ayeletstarkware ayeletstarkware Awaiting requested review from ayeletstarkware

@noamsp-starkware noamsp-starkware Awaiting requested review from noamsp-starkware

@ShahakShama ShahakShama Awaiting requested review from ShahakShama

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alonh5 @reviewable-StarkWare @ArniStarkware