Skip to content

feat(starknet_gateway): add class manager client#3497

Closed
noamsp-starkware wants to merge 10 commits intomainfrom
noam.s/feat_starknet_gateway_add_class_manager_client
Closed

feat(starknet_gateway): add class manager client#3497
noamsp-starkware wants to merge 10 commits intomainfrom
noam.s/feat_starknet_gateway_add_class_manager_client

Conversation

@noamsp-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

noamsp-starkware commented Jan 20, 2025

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

Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware)


crates/starknet_gateway/bench/utils.rs line 90 at r1 (raw file):

            GatewayCompiler::new_command_line_compiler(config.compiler_config.clone());
        let mut mempool_client = MockMempoolClient::new();
        let class_manager_client = Arc::new(MemoryClassManagerClient::new());

I think you should use EmptyClassManagerClient here. @alonh5 WDYT?

Copy link
Contributor Author

@noamsp-starkware noamsp-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, 1 unresolved discussion (waiting on @alonh5)


crates/starknet_gateway/bench/utils.rs line 90 at r1 (raw file):

Previously, ShahakShama wrote…

I think you should use EmptyClassManagerClient here. @alonh5 WDYT?

I didn't use the Empty variant because this is a test

@noamsp-starkware noamsp-starkware force-pushed the noam.s/fix_starknet_class_manager_types_add_transaction_converter_constructor branch from a0b4c79 to a85c4ea Compare January 20, 2025 14:18
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from faed35e to 73c53e9 Compare January 20, 2025 14:18
@noamsp-starkware noamsp-starkware force-pushed the noam.s/fix_starknet_class_manager_types_add_transaction_converter_constructor branch from a85c4ea to 8a1f433 Compare January 20, 2025 15:01
Copy link
Collaborator

@ShahakShama ShahakShama 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, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5)


crates/starknet_gateway/bench/utils.rs line 90 at r1 (raw file):

Previously, noamsp-starkware wrote…

I didn't use the Empty variant because this is a test

After the discussion with Elin you should change to Empty

@noamsp-starkware noamsp-starkware force-pushed the noam.s/fix_starknet_class_manager_types_add_transaction_converter_constructor branch from 8a1f433 to 36492a6 Compare January 20, 2025 15:34
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from 73c53e9 to 6eb4a91 Compare January 20, 2025 15:34
Copy link
Contributor Author

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


crates/starknet_gateway/bench/utils.rs line 90 at r1 (raw file):

Previously, ShahakShama wrote…

After the discussion with Elin you should change to Empty

Done.

@noamsp-starkware noamsp-starkware force-pushed the noam.s/fix_starknet_class_manager_types_add_transaction_converter_constructor branch from 36492a6 to 00d7e07 Compare January 21, 2025 05:34
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from 6eb4a91 to eba52cb Compare January 21, 2025 05:34
Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware)


crates/starknet_gateway/src/gateway_test.rs line 63 at r4 (raw file):

) -> MockDependencies {
    let mock_mempool_client = MockMempoolClient::new();
    // TODO(noamsp): use MockClassManagerClient

Same here


crates/starknet_gateway/bench/utils.rs line 90 at r4 (raw file):

            GatewayCompiler::new_command_line_compiler(config.compiler_config.clone());
        let mut mempool_client = MockMempoolClient::new();
        // TODO(noamsp): use MockClassManagerClient

Change the TODO to "use MockTransactionConverter"

@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from eba52cb to 6ba0987 Compare January 21, 2025 12:52
@ArniStarkware
Copy link
Contributor

crates/starknet_sequencer_node/src/components.rs line 87 at r17 (raw file):

                .get_state_sync_shared_client()
                .expect("State Sync Client should be available");
            // TODO: Remove this and use the real client instead once implemented.

Please put it on your name, and ask for help later.
This code is intended for production (vs test).

Suggestion:

// TODO(NoamS):

@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from 6590c16 to 69749db Compare January 23, 2025 09:46
@noamsp-starkware noamsp-starkware changed the base branch from main to noam.s/remove_compiled_class_hash_mismatch January 23, 2025 09:46
@noamsp-starkware noamsp-starkware force-pushed the noam.s/remove_compiled_class_hash_mismatch branch from 534dbda to bf1d245 Compare January 23, 2025 09:49
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from 69749db to f3f6070 Compare January 23, 2025 09:49
@ShahakShama ShahakShama force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from f3f6070 to 8f3af62 Compare January 23, 2025 11:04
@github-actions
Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.262 ms 35.746 ms 36.307 ms]
change: [+2.5342% +3.9862% +5.7497%] (p = 0.00 < 0.05)
Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) high mild
7 (7.00%) high severe

@ShahakShama ShahakShama force-pushed the noam.s/feat_starknet_gateway_add_class_manager_client branch from 8f3af62 to 8c34d6b Compare January 26, 2025 08:22
@noamsp-starkware noamsp-starkware changed the base branch from noam.s/remove_compiled_class_hash_mismatch to main January 26, 2025 08:23
Copy link
Collaborator

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 249 of 249 files at r27, 3 of 61 files at r28, 1 of 246 files at r29, 5 of 62 files at r30, 49 of 246 files at r31, 62 of 62 files at r32, 1 of 245 files at r33, 1 of 57 files at r34, 13 of 266 files at r35, 13 of 257 files at r37, 12 of 72 files at r38, 4 of 62 files at r40, 10 of 239 files at r41, 9 of 66 files at r42, 2 of 225 files at r43, 225 of 225 files at r45.
Reviewable status: 75 of 358 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @noamsp-starkware)

Copy link
Collaborator

@ShahakShama ShahakShama 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 62 files at r46, 5 of 65 files at r56, 2 of 216 files at r57, 1 of 58 files at r58, 8 of 65 files at r60, 7 of 206 files at r61, 60 of 61 files at r62, 199 of 199 files at r63, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @noamsp-starkware)

@ShahakShama ShahakShama enabled auto-merge January 26, 2025 08:30
Copy link
Collaborator

@ShahakShama ShahakShama 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, 3 unresolved discussions (waiting on @noamsp-starkware)

@ArniStarkware
Copy link
Contributor

crates/starknet_api/src/rpc_transaction.rs line 56 at r26 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

What do you think about this?
I like it better because RpcDeployAccountTransaction is a wrapper, not unique for version 3. If and when version 4 txs are added (or older version txs are supported - not probable, but still), the wrapper is still relevant.

Also - it better implies the fact that there is an unwrapping to be made to reach the V3 tx.

Non -blocking.

@github-actions
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.966 ms 35.009 ms 35.056 ms]
change: [-4.0471% -2.5487% -1.2503%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe

Copy link
Collaborator

@ShahakShama ShahakShama 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 @noamsp-starkware)

auto-merge was automatically disabled January 26, 2025 09:08

Pull request was closed

@noamsp-starkware
Copy link
Contributor Author

merged @ #3656

@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2025
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.

4 participants