Skip to content

feat(starknet_batcher): use transaction converter to infer executable from mempool internal tx#3509

Merged
noamsp-starkware merged 4 commits intonoam.s/feat_starknet_mempool_add_TransactionReference_new_from_InteralRpcTransactionfrom
noam.s/feat_starknet_batcher_use_transaction_converter_to_infer_executable_from_mempool_internal_tx
Jan 22, 2025
Merged

feat(starknet_batcher): use transaction converter to infer executable from mempool internal tx#3509
noamsp-starkware merged 4 commits intonoam.s/feat_starknet_mempool_add_TransactionReference_new_from_InteralRpcTransactionfrom
noam.s/feat_starknet_batcher_use_transaction_converter_to_infer_executable_from_mempool_internal_tx

Conversation

@noamsp-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@noamsp-starkware noamsp-starkware marked this pull request as ready for review January 20, 2025 12:26
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_mempool_add_TransactionReference_new_from_InteralRpcTransaction branch from 58993e1 to e9a2da5 Compare January 20, 2025 14:19
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_batcher_use_transaction_converter_to_infer_executable_from_mempool_internal_tx branch from 59b9bb4 to b187730 Compare January 20, 2025 14:19
@github-actions
Copy link

github-actions bot commented Jan 20, 2025

@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_mempool_add_TransactionReference_new_from_InteralRpcTransaction branch from e9a2da5 to 835245d Compare January 21, 2025 05:35
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_batcher_use_transaction_converter_to_infer_executable_from_mempool_internal_tx branch from b187730 to 0c801e7 Compare January 21, 2025 05:35
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_mempool_add_TransactionReference_new_from_InteralRpcTransaction branch from 835245d to f4bc18e Compare January 21, 2025 12:52
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_batcher_use_transaction_converter_to_infer_executable_from_mempool_internal_tx branch from 0c801e7 to 0811a41 Compare January 21, 2025 12:52
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.

:lgtm:

Reviewed 6 of 8 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @noamsp-starkware)


crates/starknet_batcher/src/batcher.rs line 640 at r2 (raw file):

    let transaction_converter = TransactionConverter::new(
        class_manager_client,
        config.block_builder_config.chain_info.chain_id.clone(),

@ArniStarkware @ayeletstarkware is it more correct to get the chain id from here or from storage config?


crates/starknet_batcher/src/transaction_provider_test.rs line 93 at r2 (raw file):

        tx_sender,
        tx_receiver,
        // TODO(noamsp): use MockTransactionConverter

Same


crates/starknet_batcher/src/batcher_test.rs line 103 at r2 (raw file):

            mempool_client: MockMempoolClient::new(),
            block_builder_factory: MockBlockBuilderFactoryTrait::new(),
            // TODO(noamsp): use MockTransactionConverter

In this case I think it's more correct using MockClassManagerClient. Especially because you're going to use it in the state reader as well


crates/starknet_api/src/test_utils/invoke.rs line 159 at r2 (raw file):

}

pub fn internal_invoke_tx(invoke_args: InvokeTxArgs) -> InternalRpcTransaction {

I'd like the person who wrote the rest of these to review this as well
From a quick git blame it looks like it's either @ArniStarkware or @ayeletstarkware

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.

Reviewed 1 of 8 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware and @noamsp-starkware)


crates/starknet_api/src/test_utils/invoke.rs line 159 at r2 (raw file):

Previously, ShahakShama wrote…

I'd like the person who wrote the rest of these to review this as well
From a quick git blame it looks like it's either @ArniStarkware or @ayeletstarkware

:lgtm:

___ *[`crates/starknet_batcher/src/batcher.rs` line 640 at r2](https://reviewable.io/reviews/starkware-libs/sequencer/3509#-OH8AUrw62tRxPkYCukA:-OH8EB2s4JiTuCKwR7ai:bpkaehd) ([raw file](https://github.com/starkware-libs/sequencer/blob/0811a41b34eb77af741ec2e47a1c8037f149f3de/crates/starknet_batcher/src/batcher.rs#L640)):*
Previously, ShahakShama wrote…

@ArniStarkware @ayeletstarkware is it more correct to get the chain id from here or from storage config?

Theoretically, Both are fine. Both should be the same value (if not, we have a problem).

I see that in storage config the extraction of the value is more direct.

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)


crates/starknet_batcher/src/batcher.rs line 640 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Theoretically, Both are fine. Both should be the same value (if not, we have a problem).

I see that in storage config the extraction of the value is more direct.

@ArniStarkware Changed as you suggested

@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_mempool_add_TransactionReference_new_from_InteralRpcTransaction branch from f4bc18e to cb32339 Compare January 21, 2025 15:33
@noamsp-starkware noamsp-starkware force-pushed the noam.s/feat_starknet_batcher_use_transaction_converter_to_infer_executable_from_mempool_internal_tx branch from 0811a41 to 066ebde Compare January 21, 2025 15: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.

:lgtm:

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

@noamsp-starkware noamsp-starkware merged commit 823725b into noam.s/feat_starknet_mempool_add_TransactionReference_new_from_InteralRpcTransaction Jan 22, 2025
14 of 18 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 2025
@noamsp-starkware noamsp-starkware deleted the noam.s/feat_starknet_batcher_use_transaction_converter_to_infer_executable_from_mempool_internal_tx branch January 27, 2025 11:16
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